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

[Eric Sunshine <sunshine@sunshineco.com>]

On Tue, Jul 3, 2018 at 7:15 PM Joshua Nelson <jyn514@gmail.com> wrote:
> On 07/03/2018 03:15 AM, Eric Sunshine wrote:
> >> +               /* taken from checkout.c;
> >> +                * we have a simpler case because we never create a branch */
> >
> > 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 :)

An even more common workflow is to use git-format-patch to generate
the patches locally, then edit the patches to add commentary below the
"---" line if needed, proof-read everything, and finally use
git-send-email to send out the already-generated patches.

> >> +                       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.

Oh, the intention of 'initialized' was quite clear, but it seems
unnecessary, which is why I was suggesting an alternative. Unless I'm
misunderstanding the code (certainly a possibility), I think
'initialized' is redundant, thus you can get by without it
Specifically, the only time you set 'initialized' is when you don't
set 'object', which means that you can infer whether 'oid' was
initialized based upon whether 'object' was set. So, my suggestion
was:

    const char *object = NULL;
    ...
    ... conditionals possibly assigning to 'object' ...
    ...
    if (object && get_oid(object, &oid))
        die(_("not a valid object name: %s", object);

If you arrived at that final 'if' statement and object is still NULL,
then you know that 'oid' is already initialized; if 'object' is not
NULL, then you use it to initialized 'oid'.