Re: [PATCH v5] fast-(import|export): improve on commit signature output format

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 9, 2025, 00:03 UTC
Message-ID: <xmqqqzyqqlh3.fsf@gitster.g>
In-Reply-To: <CABPp-BF6OvH8oh=jG_8fWoC5gW+9E+wx=uDEk1uerJTOva5isg@mail.gmail.com>

Elijah Newren <newren@gmail.com> writes:

Show 10 quoted lines

>> +       if (!space)
>> +               die("Expected gpgsig format: 'gpgsig <hash-algo> <signature-format>', "
>> +                   "got 'gpgsig %s'", args);
>> +       *space++ = '\0';
>> +
>> +       sig->hash_algo = args;
>> +       sig->sig_format = space;
>
> Really minor nitpick, but it might be clearer to pre-increment space
> here than to increment it above.

FWIW, I find what Christian wrote easier to follow. We find a " " in the buffer, point it with a pointer and NUL-terminate the substring. We know we want to further process bytes that follow, so the pointer is post-incremented after the NUL-termination. The next user of that pointer relies on the fact that the previous user concluded its use with that post-increment.

If 'space' variable were named more genericly, like '*cp', it would have been perfect. Perhaps only the first half of the code was written first, and it looked for a space, so the name was chosen, but then later ...

>> +
>> +       /* Remove any trailing newline from format */
>> +       space = strchr(sig->sig_format, '\n');

... it is used to point at a LF X-<, at which time the author could have renamed it to keep readers' sanity ;-)

>> +       if (space)
>> +               *space = '\0';

I also wonder what should (not "does", as I can see that the code does not do anything) happen if we do not find the LF we were looking for. Is the caller of this function so loosely written that it may or may not guarantee that the data it calls this function with is properly terminated?

Show 26 quoted lines

>> +test_expect_success GPG 'export and import of doubly signed commit' '
>> +       git -C explicit-sha256 fast-export --signed-commits=verbatim dual-signed >output &&
>> +       test_grep -E "^gpgsig sha1 openpgp" output &&
>> +       test_grep -E "^gpgsig sha256 openpgp" output &&
>> +
>> +       (
>> +               cd new &&
>> +               git fast-import &&
>> +               git cat-file commit refs/heads/dual-signed >actual &&
>> +               test_grep -E "^gpgsig " actual &&
>> +               test_grep -E "^gpgsig-sha256 " actual &&
>> +               IMPORTED=$(git rev-parse refs/heads/dual-signed) &&
>> +               if test "$GIT_DEFAULT_HASH" = "sha1"
>> +               then
>> +                       test $SHA1_B = $IMPORTED
>> +               else
>> +                       test $SHA256_B = $IMPORTED
>> +               fi
>> +       ) <output
>
> This last bit seems a bit fragile; could the redirection of output to
> the stdin of `git fast-import` be made specific to that one line
> instead of to the whole range of commands?
>
> Otherwise, I very much appreciate the work to create a testcase with
> both signature types on a single commit.

Yup, thanks, both of you. It seems that we are getting closer to the finish line?

Thanks.