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

[Eric Sunshine <sunshine@sunshineco.com>]

On Sun, Apr 3, 2016 at 12:45 AM, Jeff King <peff@peff.net> wrote:
> On Sat, Apr 02, 2016 at 07:16:14PM -0400, santiago@nyu.edu wrote:
>> -     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