On Mon, Jul 29, 2024 at 01:12:42PM -0700, Junio C Hamano wrote:

Patrick Steinhardt [off-list ref] writes:

quoted

On Fri, Jun 21, 2024 at 04:18:26PM -0700, Junio C Hamano wrote:

quoted

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

quoted

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