Re: [PATCH 5/5] patch-id: tighten code to detect the patch header

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 2273 bytes --]

On Mon, Jul 29, 2024 at 01:12:42PM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > On Fri, Jun 21, 2024 at 04:18:26PM -0700, Junio C Hamano wrote:
> >> @@ -196,11 +211,13 @@ static void generate_id_list(unsigned flags)
> >>  	struct strbuf line_buf = STRBUF_INIT;
> >>  
> >>  	oidclr(&oid);
> >> +	flags |= GOPID_FIND_HEADER;
> >>  	while (!feof(stdin)) {
> >>  		patchlen = get_one_patchid(&n, &result, &line_buf, flags);
> >>  		if (patchlen)
> >>  			flush_current_id(&oid, &result);
> >>  		oidcpy(&oid, &n);
> >> +		flags &= ~GOPID_FIND_HEADER;
> >>  	}
> >
> > I think I'm missing the obvious. But why don't we have to set
> > `GOPID_FIND_HEADER` when we have flushed the current patch ID? Is this
> > because we know that `get_one_patchid()` stops once it finds the next
> > line starting with a commit?
> 
> Yup the original control flow is rather convoluted.  The first call
> stops when it finds the header that begins the log message part and
> returns, but the subsequent calls are to (1) skip the log message
> and then (2) parse and hash the diff part, until it finds another
> header that begins the log message part of the _next_ patch and
> return.  GOPID_FIND_HEADER bit is used to tell the callee when we
> haven't found the header (hence we can stop at a line whose
> beginning looks like a hash) or the previous round already found the
> header and we positively know we are now in the "skip the log
> message" phase (hence a line whose beginning looks like a hash is
> not a new header).

Okay.

> > Makes me wonder what happens when there is
> > non-diff garbage between patches for which we are about to generate
> > patch IDs.
> 
> "Skip non-diff garbage until we see a patch" is the mechanism used
> to skip the log message, so it would be a reasonable thing to skip
> such no-diff garbage between patches, no?

Oh, yes, it is reasonable. I just didn't quite figure out the flow of
the above loop when reading through the code. As you say, it is somewhat
convoluted and not all that straight forward.

In any case though, your changes improve readability, so the fact that
things are not quite straight forward is not the fault of this patch
series here.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]