Re: [PATCH 1/3] ls-tree.c: support `--oid-only` option for "git-ls-tree"
From: Jeff King <hidden>
Date: 2021-11-15 20:31:54
Subsystem:
the rest · Maintainer:
Linus Torvalds
On Mon, Nov 15, 2021 at 07:51:51PM +0800, Teng Long wrote:
quoted hunk ↗ jump to hunk
diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c index 3a442631c7..1f82229649 100644 --- a/builtin/ls-tree.c +++ b/builtin/ls-tree.c@@ -20,6 +20,7 @@ static int line_termination = '\n'; #define LS_SHOW_TREES 4 #define LS_NAME_ONLY 8 #define LS_SHOW_SIZE 16 +#define LS_OID_ONLY 32 static int abbrev; static int ls_options; static struct pathspec pathspec;@@ -90,6 +91,14 @@ static int show_tree(const struct object_id *oid, struct strbuf *base, else if (ls_options & LS_TREE_ONLY) return 0; + if ((ls_options & LS_NAME_ONLY) && (ls_options & LS_OID_ONLY)) + die(_("cannot specify --oid-only and --name-only at the same time"));
This seems reasonable to me. Letting them overwrite each other (i.e., "last one wins") would also be fine, but we can always loosen to that behavior later if we choose. This is a somewhat funny place to put the check, though. It will be run for every entry in the tree (so is a tiny bit less efficient, but also would not trigger for an empty tree). It probably should go in cmd_ls_tree(), perhaps here:
diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
index 1f82229649..3c9ea00489 100644
--- a/builtin/ls-tree.c
+++ b/builtin/ls-tree.c@@ -91,9 +91,6 @@ static int show_tree(const struct object_id *oid, struct strbuf *base, else if (ls_options & LS_TREE_ONLY) return 0; - if ((ls_options & LS_NAME_ONLY) && (ls_options & LS_OID_ONLY)) - die(_("cannot specify --oid-only and --name-only at the same time")); - if (ls_options & LS_OID_ONLY) { printf("%s\n", find_unique_abbrev(oid, abbrev)); return 0;
@@ -175,6 +172,9 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix) ((LS_TREE_ONLY|LS_RECURSIVE) & ls_options)) ls_options |= LS_SHOW_TREES; + if ((ls_options & LS_NAME_ONLY) && (ls_options & LS_OID_ONLY)) + die(_("cannot specify --oid-only and --name-only at the same time")); + if (argc < 1) usage_with_options(ls_tree_usage, ls_tree_options); if (get_oid(argv[0], &oid))
Ævar also mentioned using OPT_CMDMODE(), which I think would naturally move the logic in a similar way. -Peff