Re: [PATCH 1/3] ls-tree.c: support `--oid-only` option for "git-ls-tree"

From: Teng Long <dyroneteng@gmail.com>
Date: Nov 18, 2021, 11:23 UTC
Message-ID: <20211118112348.85948-1-dyroneteng@gmail.com>
In-Reply-To: <YZKyC7c8b9PnyXG8@coredump.intra.peff.net>

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.