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

> 
> Luke Shumaker <lukeshu@lukeshu.com> writes:
> 
> > fast-export has an existing --signed-tags= flag that controls how to
> 
> Don't call a command line option "a flag", especially when it is not
> a boolean.

> > So, implement signed-commits.
> 
> That's misleading.  You are not inventing "git commit --signed"
> here.
> 
>     So implement `--signed-commits=<disposition>` that mirrors the
>     `--signed-tags=<disposition>` option.

> > +--signed-commits=(verbatim|warn-verbatim|warn-strip|strip|abort)::
> > +	Specify how to handle signed commits.  Behaves exactly as
> > +	--signed-tags (but for commits), except that the default is
> > +	'warn-strip' rather than 'abort'.
> 
> Why deliberate inconsistency?  I am not sure "historically we did a
> wrong thing" is a good reason (if we view that silently stripping
> was a disservice to the users, aborting would be a bugfix).

> >  
> > +enum sign_mode { SIGN_ABORT, SIGN_VERBATIM, SIGN_STRIP, SIGN_VERBATIM_WARN, SIGN_STRIP_WARN };
> > +
> >  static int progress;
> > -static enum { SIGNED_TAG_ABORT, VERBATIM, WARN, WARN_STRIP, STRIP } signed_tag_mode = SIGNED_TAG_ABORT;
> 
> Giving the enum values consistent prefix "SIGN_" is a great
> improvement.  On the other hand, swapping the word order,
> e.g. WARN_STRIP to SIGN_STRIP_WARN, is unwarranted.

> > +static enum sign_mode signed_tag_mode = SIGN_ABORT;
> > +static enum sign_mode signed_commit_mode = SIGN_STRIP_WARN;
> 
> I think it is safer to abort for both and sell it as a bugfix
> ("silently stripping commit signatures was wrong. we should abort
> the same way by default when encountering a signed tag").
> 
> > -static int parse_opt_signed_tag_mode(const struct option *opt,
> > +static int parse_opt_sign_mode(const struct option *opt,
> >  				     const char *arg, int unset)
> >  {
> > -	if (unset || !strcmp(arg, "abort"))
> > -		signed_tag_mode = SIGNED_TAG_ABORT;
> > +	enum sign_mode *valptr = opt->value;
> > +	if (unset)
> > +		return 0;
> > +	else if (!strcmp(arg, "abort"))
> > +		*valptr = SIGN_ABORT;
> >  	else if (!strcmp(arg, "verbatim") || !strcmp(arg, "ignore"))
> > -		signed_tag_mode = VERBATIM;
> > +		*valptr = SIGN_VERBATIM;
> 
> Interesting and not a new issue at all, but "ignore" is a confusing
> symonym to "verbatim"---I would have expected "ignore", if accepted
> as a choice, would strip the signature.  Not documenting it is
> probably good, but perhaps we would eventually remove it?

> > @@ -499,6 +505,60 @@ static void show_filemodify(struct diff_queue_struct *q,
> >  	}
> >  }
> >  
> > +static const char *find_signature(const char *begin, const char *end, const char *key)
> 
> This is only for in-header signature used in commit objects, and not
> for the traditional "attached to the end" signature used in tag
> objects, right?
> 
> The name of this function should be designed to answer the above
> question, but find_signature() that does not say either commit or
> tag implies it can accept both (which would be a horrible interface,
> though).  If this is only for in-header signature, rename it to make
> sure that the fact is readable out of its name?
> 
> > +{
> > +	static struct strbuf needle = STRBUF_INIT;
> > +	char *bod, *eod, *eol;
> > +
> > +	strbuf_reset(&needle);
> > +	strbuf_addch(&needle, '\n');
> > +	strbuf_addstr(&needle, key);
> > +	strbuf_addch(&needle, ' ');
> 
> strbuf_addf(), perhaps?

> > +	bod = memmem(begin, end ? end - begin : strlen(begin),
> > +		     needle.buf, needle.len);
> > +	if (!bod)
> > +		return NULL;
> > +	bod += needle.len;
> > +
> > +	/*
> > +	 * In the commit object, multi-line header values are stored
> > +	 * by prefixing continuation lines begin with a space.  So
> 
> "by prefixig continuation lines with a space"

> > +	 * 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"
> > +	 *
> > +	 * So we need to look for the first '\n' that *isn't* followed
> > +	 * by a ' ' (or the first '\0', if no such '\n' exists).
> > +	 */
> 
> > +	eod = strchrnul(bod, '\n');
> > +	while (eod[0] == '\n' && eod[1] == ' ') {
> > +		eod = strchrnul(eod+1, '\n');
> > +	}
> 
> SP on both sides of '+'; no {} around a block that consists of a
> single statement.

> > +	*eod = '\0';
> 
> The begin and end pointers pointed to a piece of memory that is
> supposed to be read-only, but this pointer points into that region
> of memory and then updates a byte?  The function signature is
> misleading---if you intend to muck with the string, accept them as
> mutable pointers.
> 
> Better yet, don't butcher the region of memory pointed by the
> "message" variable the caller uses to keep reading from the
> remainder of the commit object buffer with this and memmove()
> below.  Perhaps have the caller pass a strbuf to fill in the
> signature found by this helper as another parameter, and then return
> a bool "Yes, I found a sig" as its return value?

> > +
> > +	/*
> > +	 * We now have the value as it's stored in the commit object.
> > +	 * 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"
> > +	 *
> > +	 * So now we need to strip out all of those extra spaces.
> > +	 */
> > +	while ((eol = strstr(bod, "\n ")))
> > +		memmove(eol+1, eol+2, strlen(eol+1));
> 
> Besides, this is O(n^2), isn't it, as it always starts scanning at
> bod while there are lines in the signature block to be processed, it
> needs to skip over the lines that the loop already has processed.