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

Eric Sunshine [off-list ref] writes:

On Sun, Apr 17, 2016 at 6:26 PM,  [off-list ref] wrote:

quoted

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

quoted

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

quoted

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.

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

quoted

Signed-off-by: Santiago Torres <redacted>
---

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.

quoted

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

quoted

+               }
+               else if (verify_tag(sha1, name, flags))
+                       had_error = 1;

Style: cuddle '}' and else:

    } else

quoted

+       }
        return had_error;
 }