| Message ID | 20211115115153.48307-2-dyroneteng@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | support `--oid-only` in `ls-tree` | expand |
lorn Mon, Nov 15 2021, Teng Long wrote: > This commit supply an option names `--oid-only` to let `git ls-tree` > only print out the OID of the object. `--oid-only` and `--name-only` > are mutually exclusive in use. > > Signed-off-by: Teng Long <dyroneteng@gmail.com> > --- > builtin/ls-tree.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > 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")); If you make these an OPT_CMDMODE you get this behavior for free. See e.g. my https://lore.kernel.org/git/patch-v2-06.10-d945fc94774-20211112T221506Z-avarab@gmail.com/
On Mon, Nov 15, 2021 at 07:51:51PM +0800, Teng Long wrote: > 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
On Mon, Nov 15, 2021 at 02:16:27PM -0500, Jeff King wrote: > On Mon, Nov 15, 2021 at 07:51:51PM +0800, Teng Long wrote: > > > 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. Oh, and whichever direction we go, it would probably make sense for --long to be handled in the same way. I.e.: git ls-tree --long --oid-only does not really make sense. Though we currently just ignore --long for: git ls-tree --long --name-only which is arguably a bug. -Peff
> If you make these an OPT_CMDMODE you get this behavior for free. See > e.g. my > https://lore.kernel.org/git/patch-v2-06.10-d945fc94774-20211112T221506Z-avarab@gmail.com/ Thank you very much for providing this input. So I try to read this patch your mentioned and try to repeat the idea in my understanding. First, OPT_CMDMODE() can be used for: 1. Easy for checking the combined command options, such as "mutually exclusive" conditions. 2. Die and output the error message consistently when the incompatible options are found. 3. Brings better extensibilites, no need to change a lot of if/elses. Then, you suggest to consider about to use OPT_CMDMODE instead of the current implementations. Did I understand your suggestion right and comprehensive?
On Thu, Nov 18 2021, Teng Long wrote: >> If you make these an OPT_CMDMODE you get this behavior for free. See >> e.g. my >> https://lore.kernel.org/git/patch-v2-06.10-d945fc94774-20211112T221506Z-avarab@gmail.com/ > > Thank you very much for providing this input. > > So I try to read this patch your mentioned and try to repeat the idea in my understanding. > > First, OPT_CMDMODE() can be used for: > > 1. Easy for checking the combined command options, such as "mutually exclusive" conditions. > > 2. Die and output the error message consistently when the incompatible options are found. > > 3. Brings better extensibilites, no need to change a lot of if/elses. > > Then, you suggest to consider about to use OPT_CMDMODE instead of the current implementations. > > Did I understand your suggestion right and comprehensive? Yes, all of that is correct. It's a way of defining N options, --foo, --bar, --baz, where combining any of them is an error. We usually use it for a "command mode" (hence the name), but it can be used when the command has flags that are mutually exclusive. I think (but am not sure, and didn't check) that you can even use it for --foo AND --bar that are exclusive, and --other --flags that are also mutually exclusive (but could be combined with one of --foo or --bar), you just need to provide another variable for it to set. But I haven't tested that or used it like that, maybe it doesn't work for some reason I'm forgetting...
On Mon, 15 Nov 2021 14:16:27 -0500, Jeff King wrote: > 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: Yes, it's better here as a fail-fast case. According to the suggestion of the new location I think why not put the logic further head, after the parse_options() return, like: diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c index 1f82229649..003a9ade54 100644 --- a/builtin/ls-tree.c +++ b/builtin/ls-tree.c @@ -166,6 +166,10 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix) argc = parse_options(argc, argv, prefix, ls_tree_options, ls_tree_usage, 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 (full_tree) { ls_tree_prefix = prefix = NULL; chomp_prefix = 0; Will it bring other new problems? Thank you.
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")); + + if (ls_options & LS_OID_ONLY) { + printf("%s\n", find_unique_abbrev(oid, abbrev)); + return 0; + } + if (!(ls_options & LS_NAME_ONLY)) { if (ls_options & LS_SHOW_SIZE) { char size_text[24]; @@ -139,6 +148,8 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix) LS_NAME_ONLY), OPT_BIT(0, "name-status", &ls_options, N_("list only filenames"), LS_NAME_ONLY), + OPT_BIT(0, "oid-only", &ls_options, N_("list only oids"), + LS_OID_ONLY), OPT_SET_INT(0, "full-name", &chomp_prefix, N_("use full path names"), 0), OPT_BOOL(0, "full-tree", &full_tree,
This commit supply an option names `--oid-only` to let `git ls-tree` only print out the OID of the object. `--oid-only` and `--name-only` are mutually exclusive in use. Signed-off-by: Teng Long <dyroneteng@gmail.com> --- builtin/ls-tree.c | 11 +++++++++++ 1 file changed, 11 insertions(+)