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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jul 4, 2018, 09:29 UTC
Message-ID: <CAPig+cQu-e63NUUXAB_QA+M-rgPfqBLOOm5fdjsSVuWHJt7TJA@mail.gmail.com>
In-Reply-To: <26b538bd-df59-d9a6-460d-0b1042b35250@gmail.com>

On Tue, Jul 3, 2018 at 7:15 PM Joshua Nelson <jyn514@gmail.com> wrote:

Show 12 quoted lines

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

Show 17 quoted lines

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