On Fri, Mar 25, 2016 at 10:45 AM, Santiago Torres <santiago@xxxxxxx> wrote:
>> > while (i < argc)
>> > - if (verify_tag(argv[i++], flags))
>> > + name = argv[i++];
>> > + if (get_sha1(name, sha1))
>> > + return error("tag '%s' not found.", name);
>> > +
>> > + if (pgp_verify_tag(NULL, NULL, sha1, flags))
>> > had_error = 1;
>>
>> Meh, this isn't Python. Due to the missing braces, the only thing
>> inside the while() loop is the assignment to 'name'; all the other
>> indented code is outside the while().
>>
>> Did you run the test suite following this change? Did it all pass? If
>> so, perhaps an additional test or two to catch this sort of error
>> would be warranted.
>
> Wow, you're right! I just re-ran the tests again to make sure I didn't
> miss anything. All the tests pass for me, so I'll write an extra case to
> avoid this. Just to be sure, I should include it in t7030-verify-tag.sh
> right?
Generally speaking, it is desirable to have test coverage for all
functionality you're refactoring to ensure that the refactoring
doesn't break that functionality.
t7030-verify-tag.sh indeed seems like a good place to add a new test
for catching this sort of regression.
t7004-tag.sh is also of interest since, in an earlier version of this
patch, if I recall correctly, Peff caught a regression where "git tag
-v" failed to pass the GPG_VERIFY_VERBOSE flag, and I don't think
t7004-tag.sh would have caught that problem either, so a new test for
that would be good, as well.
Speaking of GPG_VERIFY_VERBOSE, now that I'm examining the changes
more closely, it appears that this version of the patch no longer
respects GPG_VERIFY_VERBOSE at all but is instead hard-coded to be
verbose. Is that desirable or intentional?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html