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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Apr 3, 2016, 08:11 UTC
Message-ID: <CAPig+cQzeRk66eptatSy8=1xbnBNrO5ATrfqBX8730t3gOmNQw@mail.gmail.com>
In-Reply-To: <20160403044502.GC1519@sigill.intra.peff.net>

On Sun, Apr 3, 2016 at 12:45 AM, Jeff King <peff@peff.net> wrote:

Show 21 quoted lines

> 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