Re: [PATCH v6 4/6] verify-tag: add sha1 argument to verify_tag()

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 18, 2016, 20:28 UTC
Message-ID: <xmqqoa964q6h.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CAPig+cSGR-AZPzPRenm8RkOa7XpUOqMAgUw_8p-OUL=qP0Hy6w@mail.gmail.com>

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 24 quoted lines

> On Sun, Apr 17, 2016 at 6:26 PM,  <santiago@nyu.edu> wrote:
>> verify-tag: add sha1 argument to verify_tag()
>
> Mentioned previously[1]: This subject is talking about low level
> details of the change rather than giving a high-level overview. A
> suggested replacement[1] would be:
>
>     verify-tag: prepare verify_tag() for libification
>
>> The current interface of verify_tag() resolves reference names to SHA1,
>> which might be redundant as future callers may resolve the refname to
>> SHA1 beforehand.
>
> There is no mention here that the plan is to libify verify_tag() and
> "might be redundant" is a somewhat weak way to argue in favor of this
> change. The commit messages proposed in the previous review[1] was
> more explicit:
>
>     verify_tag() accepts a tag name which it resolves to a SHA1
>     before verification, however, the plan is to make this
>     functionality public and it is possible that future callers will
>     already have a SHA1 in hand. Since it would be wasteful for them
>     to supply a tag name only to have it resolved again, change
>     verify_tag() to accept a tag SHA1 rather than a name.

Phrased that way, it is not "might be redundant" that this change is fixing, and "It is possible ... will already have" is still weak. I think the real reason for this change is that some future callers may only have object name without the end-user supplied textual input, and the current interface that takes strings is cumbersome for them to use--they have to use sha1_to_hex() only so that the callee can do get_sha1() on it.

I guess with 's/already/only/', your version is good.

By the way, it can also be read in a negative way: "Some current callers may let this function resolve tagname, but they no longer can rely on it, as we force them to resolve before we allow them to call this function."

>> Add a SHA1 parameter to use instead of the name parameter. We also
>> replace the name argument to report_name and use it for error reporting
>> only.

I know I am to blame, but perhaps "reported_name" or "name_to_report"? "report_name" sounds as if it is a boolean that tells the function to report the name of the tag (as opposed to stay silent).

I agree all the clean-up points in the code part of your review.

Thanks.

Show 44 quoted lines

> The patch itself looks okay, though see a few nits below (which may
> not be worth a re-roll).
>
> [1]: http://article.gmane.org/gmane.comp.version-control.git/290829
>
>> Signed-off-by: Santiago Torres <santiago@nyu.edu>
>> ---
>> diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
>> @@ -80,6 +79,8 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
>>  {
>>         int i = 1, verbose = 0, had_error = 0;
>>         unsigned flags = 0;
>> +       unsigned char sha1[20];
>> +       const char *name;
>
> Nit: These could have been declared in the scope of the while-loop
> (below) since you've added braces to it.
>
>> @@ -96,8 +97,14 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
>> -       while (i < argc)
>> -               if (verify_tag(argv[i++], flags))
>> +       while (i < argc) {
>> +               name = argv[i++];
>> +               if (get_sha1(name, sha1)) {
>> +                       error("tag '%s' not found.", name);
>>                         had_error = 1;
>
> These lines could be combined:
>
>     had_error = !!error("tag '%s' not found.", name);
>
> which would allow you to drop the braces.
>
>> +               }
>> +               else if (verify_tag(sha1, name, flags))
>> +                       had_error = 1;
>
> Style: cuddle '}' and else:
>
>     } else
>
>> +       }
>>         return had_error;
>>  }