Re: [PATCH 2/2] tag: convert gpg_verify_tag to use struct object_id

[Stefan Beller <sbeller@google.com>]

On Thu, Jul 13, 2017 at 2:23 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> On Thu, Jul 13, 2017 at 1:52 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Stefan Beller <sbeller@google.com> writes:
>>>
>>>> diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
>>>> index f9a5f7535a..ed8329340f 100644
>>>> --- a/builtin/verify-tag.c
>>>> +++ b/builtin/verify-tag.c
>>>> @@ -56,20 +56,21 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
>>>>       }
>>>>
>>>>       while (i < argc) {
>>>> -             unsigned char sha1[20];
>>>> +             struct object_id oid;
>>>>               const char *name = argv[i++];
>>>> -             if (get_sha1(name, sha1)) {
>>>> +
>>>> +             if (get_oid(name, &oid)) {
>>>>                       had_error = !!error("tag '%s' not found.", name);
>>>>                       continue;
>>>>               }
>>>
>>> This part is already done, it seems, in bc/object-id topic, even
>>> though other parts are not yet done?
>>
>> Oops. I assumed the latest bc/object-id would have been in master
>> already, but after checking it is not. 967635dc3c2
>> (builtin/verify-tag: convert to struct object_id)
>> converts this part, although there are 2 differences:
>> * I added a stray newline before get_oid
>> * The argument to gpg_verify_tag is a sha1 or oid
>>
>> So yes, this produces a merge conflict. :/
>
> That is OK.  This actually shouldn't create any meaningful conflict.
> Both try to do the same code, with only a blank-line difference.
>
> As Brian said bc/object-id would be rerolled, I was wondering if I
> should queue these two patches (even though I already queued them)
> myself, or it would be better for you to send them to Brian to make
> it part of his series.

+cc Brian

Snarkily I wanted to link to an essay "large patch series
considered harmful"[1], but could not find any. So a couple
of bullet points:
(a) humans dislike larger series, hence fewer reviewers
(b) the likelihood of a mistake in new code is proportional to its size
  We can use the number of patches as a proxy for size
(c) If a mistake is found, the whole series needs rerolling.
  The effort of rerolling a series can be approximated with
  its size as well.

From combining (b) and (c), we conclude that the effort to
land a patch series is O(n^2) with n as the number of patches.
Also from (a) we conclude that two smaller series containing
the same output as one large series, has better code quality.
So with that, we conclude that all series shall be as small
as possible.

So I'd ask to queue these 2 separately, asking Brian to drop
"builtin/verify-tag: convert to struct object_id"

[1] https://www.cs.princeton.edu/~dpw/papers/hotos.pdf, 2005
    seems interesting to me in hindsight.

I can also send my patches to Brian, as you (both) like.

Thanks,
Stefan