Re: [PATCH 3/3] fast-export, fast-import: implement signed-commits

From: Luke Shumaker <lukeshu@lukeshu.com>
Date: Apr 20, 2021, 16:23 UTC
Message-ID: <87v98gq60k.wl-lukeshu@lukeshu.com>
In-Reply-To: <YH4yN2OAghWB9/97@nand.local>

On Mon, 19 Apr 2021 19:45:46 -0600, Taylor Blau wrote:

Show 23 quoted lines

> > diff --git a/Documentation/git-fast-export.txt b/Documentation/git-fast-export.txt
> > index d4a2bfe037..6fdb678b54 100644
> > --- a/Documentation/git-fast-export.txt
> > +++ b/Documentation/git-fast-export.txt
> > @@ -39,6 +39,18 @@ warning will be displayed, with 'verbatim', they will be silently
> >  exported and with 'warn-verbatim', they will be exported, but you will
> >  see a warning.
> >
> > +--signed-commits=(verbatim|warn-verbatim|warn-strip|strip|abort)::
> > +	Specify how to handle signed commits.  Since any transformation
> > +	after the export can change the commit (which can also happen
> > +	when excluding revisions) the signatures will not match.
> > ++
> > +When asking to 'abort', this program will die when encountering a
> > +signed commit.  With 'strip' (which is the default), the commits will
> > +silently be made unsigned, with 'warn-strip' they will be made
> > +unsigned but a warning will be displayed, with 'verbatim', they will
> > +be silently exported and with 'warn-verbatim', they will be exported,
> > +but you will see a warning.
> > +
> 
> OK, this all seems normal to me. But it may be worth shortening it to
> say "behaves exactly as --signed-tags, but for commits", or something.

Good suggestion, it would also make it more obvious that the default is different, since I'd have to call it out explictly.

Show 14 quoted lines

> >  --tag-of-filtered-object=(abort|drop|rewrite)::
> >  	Specify how to handle tags whose tagged object is filtered out.
> >  	Since revisions and files to export can be limited by path,
> > diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
> > index 458af0a2d6..3d0c5dbf7d 100644
> > --- a/Documentation/git-fast-import.txt
> > +++ b/Documentation/git-fast-import.txt
> > @@ -437,6 +437,7 @@ change to the project.
> >  	original-oid?
> >  	('author' (SP <name>)? SP LT <email> GT SP <when> LF)?
> >  	'committer' (SP <name>)? SP LT <email> GT SP <when> LF
> > +	('gpgsig' LF data)?
> 
> Is this missing a LF after data?

No, the definition of `data` has a byte-count prefix, so it doesn't need an `LF` to act as a terminator (and it also already includes an optional trailing `LF?` just in case you want to include one).

In fact, my implementation in fast-export does not include the LF, which is why the test greps for "encoding ISO-8859-1" instead of "^encoding ISO-8859-1".

I'll add a comment saying that it's intentional.

Show 21 quoted lines

> > +static const char *find_signature(const char *begin, const char *end)
> > +{
> > +	const char *needle = "\ngpgsig ";
> > +	char *bod, *eod, *eol;
> > +
> > +	bod = memmem(begin, end ? end - begin : strlen(begin),
> > +		     needle, strlen(needle));
> > +	if (!bod)
> > +		return NULL;
> > +	bod += strlen(needle);
> > +	eod = strchrnul(bod, '\n');
> > +	while (eod[0] == '\n' && eod[1] == ' ') {
> > +		eod = strchrnul(eod+1, '\n');
> > +	}
> > +	*eod = '\0';
> > +
> > +	while ((eol = strstr(bod, "\n ")))
> > +		memmove(eol+1, eol+2, strlen(eol+1));
> 
> Hmm. I'm not quite sure I follow these last two lines. Perhaps a comment
> would help? The rest of this patch looks reasonable to me.

In the commit object, multi-line header values are stored by prefixing continuation lines begin with a space. So within the commit object, it looks like

    "gpgsig -----BEGIN PGP SIGNATURE-----\n"
    " Version: GnuPG v1.4.5 (GNU/Linux)\n"
    " \n"
    " base64_pem_here\n"
    " -----END PGP SIGNATURE-----\n"

However, we want the raw value; we want to return

    "-----BEGIN PGP SIGNATURE-----\n"
    "Version: GnuPG v1.4.5 (GNU/Linux)\n"
    "\n"
    "base64_pem_here\n"
    "-----END PGP SIGNATURE-----\n"

without all the extra spaces. That's what those two lines are doing, stripping out the extra spaces.

I'll add some comments.

-- 
Happy hacking,
~ Luke Shumaker