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

[Junio C Hamano <gitster@pobox.com>]

Elijah Newren <newren@gmail.com> writes:

>> +       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?

>> +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.