Re: [PATCH 1/3] ls-tree: make <tree-ish> optional

[Joshua Nelson <jyn514@gmail.com>]

On 07/03/2018 03:15 AM, Eric Sunshine wrote:
> Thanks for contributing to Git. As this seems to be your first
> submission to the project, don't be alarmed by the extent and nature
> of the review comments. They are intended to help you polish the
> submission, and are not meant with ill-intent.
> 
> On Mon, Jul 2, 2018 at 11:58 PM Joshua Nelson <jyn514@gmail.com> wrote:
>> use syntax similar to `git-checkout` to make <tree-ish> optional for
>> `ls-tree`. if <tree-ish> is omitted, default to HEAD. infer arguments as
>> follows:
> 
> Nit: Capitalize first word of each sentence.
> 
> This commit message explains what the patch changes (which is a good
> thing to do), but it's missing the really important explanation of
> _why_ this change is desirable. Without such justification, it's
> difficult to judge if such a change is worthwhile. As this is a
> plumbing command, people may need more convincing (especially if other
> plumbing commands don't provide convenient defaults like this).
> 
>> 1. if args start with --
>>         assume <tree-ish> to be HEAD
>> 2. if exactly one arg precedes --, treat the argument as <tree-ish>
>> 3. if more than one arg precedes --, exit with an error
>> 4. if -- is not in args
>>         a) if args[0] is a valid <tree-ish> object, treat is as such
>>         b) else, assume <tree-ish> to be HEAD
>>
>> in all cases, every argument besides <tree-ish> is treated as a <path>
> 
> This and the other patches are missing your Signed-off-by: (which
> should be placed right here).
> 
> The three patches of this series are all directly related to this one
> change. As such, they should be combined into a single patch. (At the
> very least, 1/3 and 2/3 should be combined; one could argue that 3/3
> is lengthy enough to make it separate, but that's a judgment call.)
> 
> Now, on to the actual code...
> 
>> diff --git builtin/ls-tree.c builtin/ls-tree.c
>> @@ -163,10 +163,39 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
>>             ((LS_TREE_ONLY|LS_RECURSIVE) & ls_options))
>>                 ls_options |= LS_SHOW_TREES;
>>
>> +       const char *object;
>> +       short initialized = 0;
> 
> This project frowns on declaration-after-statement. Move these to the
> top of the {...} block where other variables are declared.
> 
> Why use 'short'? Unless there's a very good reason that this needs to
> be a particular size, you shouldn't deviate from project norm, which
> is to use the natural word size 'int'.
> 
> 'initialized' is too generic, thus isn't a great name.
> 
>>         if (argc < 1)
>> -               usage_with_options(ls_tree_usage, ls_tree_options);
>> -       if (get_oid(argv[0], &oid))
>> -               die("Not a valid object name %s", argv[0]);
>> +               object = "HEAD";
>> +       else {
>> +               /* taken from checkout.c;
>> +                * we have a simpler case because we never create a branch */
> 
> Style: Multi-line comments are formatted like this:
> 
>     /*
>      * Gobble
>      * wobble.
>      */
> 
> However, this comment doesn't belong in the code, as it won't be
> particularly helpful to anyone reading the code in the future. This
> sort of note would be more appropriate in the commit message or, even
> better, in the commentary section just below the "---" lines following
> your Signed-off-by:.

I wasn't aware I could put comments in email generated by
git-send-email, thanks for the tip :)

> 
>> +               short dash_dash_pos = -1, i = 0;
> 
> Same question about why you used 'short' rather than 'int' (especially
> as 'dash_dash_pos' is declared 'int' in checkout.c).
> 
> Is there a good reason why you initialize 'i' in the declaration
> rather than in the for-loop? A _good_ reason to do so in the for-loop
> is that doing so makes the for-loop more idiomatic, reduces cognitive
> load on readers (since they don't have to chase down the
> initialization), and safeguards against someone adding new code
> between the declaration and the for-loop which might inadvertently
> alter the initial value.

No good reason, it happened to end up that way after I finished
debugging. I've changed it to a more conventional declaration.

> 
>> +               for (; i < argc; i++) {
>> +                       if (!strcmp(argv[i], "--")) {
>> +                               dash_dash_pos = i;
>> +                               break;
>> +                       }
>> +               }
>> +               if (dash_dash_pos == 0) {
>> +                       object = "HEAD";
>> +                       argv++, argc++;
> 
> 'argc' is never accessed beyond this point, so changing its value is
> pointless. Same observation for the 'else' arms. (And, even if there
> was a valid reason to modify 'argc', I think you'd want to be
> decrementing it, not incrementing it, to show that you've consumed an
> argument.)
> 
>> +               } else if (dash_dash_pos == 1) {
>> +                       object = argv[0];
>> +                       argv += 2, argc += 2;
>> +               } else if (dash_dash_pos >= 2)
>> +                       die(_("only one reference expected, %d given."), dash_dash_pos);
>> +               else if (get_oid(argv[0], &oid)) // not a valid object
> 
> Style: Use /* comments */ in this codebase, not // comments.
> 
>> +                       object = "HEAD";
>> +               else {
>> +                       argv++, argc++;
>> +                       initialized = 1;
>> +               }
>> +       }
>> +
>> +       if (!initialized) // if we've already run get_oid, don't run it again
>> +               if (get_oid(object, &oid))
>> +                       die("Not a valid object name %s", object);
> 
> Can't you accomplish the same without the 'initialized' variable by
> instead initializing 'object' to NULL and then checking it here?

I think my code wasn't very clear here - 'initialized' checks if 'oid'
has been initialized, not 'object'. AFAIK, structs can't be initialized
to NULL, but my C is not very good so I'd be interested to learn otherwise.

I'm writing a new patch with variable names that are more descriptive.

> 
>     if (object && get_oid(object, &oid))
>         die(_("not a valid object name: %s", object);
>