On Thu, Apr 24, 2025 at 2:19 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
[...]
> >
> > This is a follow up from cc/signed-fast-export-import that was merged
> > by 01d17c0530 (Merge branch 'cc/signed-fast-export-import', 2025-03-29)
> > and introduced the support for signed commits.
> >
> > The format that this series implemented was lacking a bit, so the goal
> > with this patch is to improve it and handle signed commits a bit more
> > consistently in the code base. It also shows in the tests and in our
> > documentation that SSH and X.509 signatures are supported.
>
> Thanks.
>
> It is a bit surprising and slightly sad that nobody bothered to
> report/complain about the brokenness until the original author
> follows up one month later X-<. Nobody but the original author is
> using this feature? I would have expected that use of signed
> commits were of high demand and many more people were actively
> interested in the topic.
Signed commits may be high demand, but we do have the intersection of
signed commits, usage of fast-export/fast-import, and using a
development version of Git. I suspect that intersection is somewhat
small; my guess is that signed commits tends to be associated with
large enterprisy things, development versions of git tend to be the
opposite, and fast-export/fast-import are by design infrequently used
tools for any given repo.
(Also, not sure if this helps, but the original author was Luke, not
Christian; Christian was the one who came by three years later to jump
in and polish up the patches.)
[...]
> > @@ -2830,12 +2831,15 @@ static void parse_new_commit(const char *arg)
> > "encoding %s\n",
> > encoding);
> > if (sig_alg) {
> > - if (!strcmp(sig_alg, "sha1"))
> > - strbuf_addstr(&new_data, "gpgsig ");
> > - else if (!strcmp(sig_alg, "sha256"))
> > + if (!strcmp(sig_alg, "sha256"))
> > strbuf_addstr(&new_data, "gpgsig-sha256 ");
> > - else
> > - die("Expected gpgsig algorithm sha1 or sha256, got %s", sig_alg);
> > + else if (valid_signature_name(sig_alg))
> > + strbuf_addstr(&new_data, "gpgsig ");
> > + else if (!strcmp(sig_alg, "unknown")) {
> > + warning("Unknown gpgsig algorithm name!");
> > + strbuf_addstr(&new_data, "gpgsig ");
> > + } else
> > + die("Invalid gpgsig algorithm name, got '%s'", sig_alg);
>
> Hmph, we used to have special cases for sha1 and sha256 but now we
> can handle sha1 with a more generic "valid_signature_name()" logic?
> And yet we need to still special case sha256? Not that I trust the
> old code all that much and take deviations from the patterns in the
> old code as a sign of something not right...
>
> The fast-export stream produced by the code with d9cb0e6f
> (fast-export, fast-import: add support for signed-commits,
> 2025-03-10) used to identify a signature algorithm "sha1", but this
> new version of fast-import lost the support for it, and will barf
> when seeing such an existing fast-export stream? I am not sure what
> is going on around this code.
>
> I am not so worried about the other case, where the stream produced
> by fast-export contained in this version may or may not be readable
> by an older version of fast-import.
I certainly can't answer anything here as I know little about
signatures, but your comment brought up a different question for me:
Given that d9cb0e6ff8b3 (fast-export, fast-import: add support for
signed-commits, 2025-03-10) isn't part of any release (not even a
release candidate), do we need to have backward compatibility with
that version?
