Re: [PATCH v3 3/4] builtin/verify-tag: move verification code to tag.c

On Sun, Apr 3, 2016 at 12:45 AM, Jeff King [off-list ref] wrote:

On Sat, Apr 02, 2016 at 07:16:14PM -0400, santiago@nyu.edu wrote:

quoted

-     len = parse_signature(buf, size);
-
-     if (size == len) {
-             if (flags & GPG_VERIFY_VERBOSE)
-                     write_in_full(1, buf, len);
-             return error("no signature found");
-     }
[...]
+     payload_size = parse_signature(buf, size);
+
+     if (size == payload_size) {
+             write_in_full(1, buf, payload_size);
+             return error("No PGP signature found in this tag!");
+     }

I'm happy to see the more readable variable name here. I wonder if we
should leave the error message as-is, though, as this is just supposed
to be about code movement (and if we are changing it, it should adhere
to our usual style of not starting with a capital letter, and not ending
in punctuation).

Agreed it would be nice for this patch to be just code movement since
it's difficult for a reviewer to spot actual changes. A pure code
movement patch was suggested by [1], but perhaps it should also have
explained the reason ("code changes are difficult to spot in
movement").

Such changes could be done as preparatory or follow-on patches.
Alternately, since these are such minor changes, it might also be okay
just to mention them in the commit message (as the function rename is
already mentioned).

[1]: http://article.gmane.org/gmane.comp.version-control.git/289847