Hi Phillip, On Mon, 21 Nov 2022, Phillip Wood wrote: > [...] > > @@ range-diff.c: struct patch_util { > > + > > + line = contents.buf; > > + size = contents.len; > > -+ for (; size > 0; size -= len, line += len) { > > ++ for (; size; size -= len, line += len) { > > + const char *p; > > + > > -+ len = find_eol(line, size); > > ++ len = find_next_line(line, size); > > + > > -+ if (state == MBOX_BEFORE_HEADER) { > > ++ if (state == MBOX_BEFORE_HEADER || > > ++ (state == MBOX_IN_DIFF && line[0] == 'F')) { > > If we see an 'F' when parsing a diff then it is probably a From: line > indicating that we've reached the start of a new patch. I think we need to add > the diff we've just parsed to the list of commits before continuing and change > state to MBOX_BEFORE_HEADER otherwise we'll hit the continue just below this > comment with state unchanged. As always, thank you *so* much for modeling how to perform productive, actionable and *thorough* code reviews that focus on the relevant. You spotted something important. It is a bug, as you suspected, and there is even more: `util` leaks because it is allocated both after the patch was parsed and when we see a diff header, without releasing the first (but granted, this leak has been there before). You have no idea how much I appreciate your diligence; I understand how much of an effort it takes to study patches in depth until you understand them enough to spot complex issues like this one. (In Parkinson's parlance: I am grateful that you focus on the design of the nuclear power plant, see https://en.wikipedia.org/w/index.php?title=Atwood%27s_duck) Back to our regularly scheduled programming (pun intended): I indeed made a mistake here and fixed it, by removing that 'F' check again, and instead adding a `goto` label and jumping to that upon encountering the end of the diff. I also de-duplicated the `util = xcalloc(1, sizeof(*util))` calls so that we do that _only_ in the `MBOX_BEFORE_HEADER` arm, and no longer at the end of the `MBOX_IN_DIFF` arm. Since I already looked at a leak, I looked further and saw that also `copy` is leaked in `show_range_diff()` in case of an mbox. I've addressed that, too. > > > + if (!skip_prefix(line, "From ", &p)) > > + continue; > > + > > @@ range-diff.c: struct patch_util { > > + author = subject = NULL; > > + > > + state = MBOX_IN_HEADER; > > ++ continue; > > + } > > + > > + if (starts_with(line, "diff --git ")) { > > @@ range-diff.c: struct patch_util { > > + strbuf_addch(&buf, '\n'); > > + if (!util->diff_offset) > > + util->diff_offset = buf.len; > > -+ line[len - 1] = '\n'; > > ++ > > + orig_len = len; > > -+ len = parse_git_diff_header(&root, &linenr, 1, > > line, > > -+ len, size, > > &patch); > > ++ /* `find_next_line()`'s replaced the LF with a > > NUL */ > > ++ line[len - 1] = '\n'; > > ++ len = len > 1 && line[len - 2] == '\r' ? > > ++ error(_("cannot handle diff headers > > with " > > ++ "CR/LF line endings")) : > > ++ parse_git_diff_header(&root, &linenr, > > 1, line, > > ++ len, size, > > &patch); > > + if (len < 0) { > > + error(_("could not parse git header > > '%.*s'"), > > + orig_len, line); > > ++ release_patch(&patch); > > Thank you for using release_patch() rather than the "free random struct > members" approach that was suggested. Indeed, it sounded like a specific suggestion to release a specific `struct` member, but that was of course not the end of the story, and I completed the analysis myself that I had wished the reviewer would have completed _before_ sending off a review. > > @@ t/t3206-range-diff.sh: test_expect_success 'ranges with pathspecs' > > ' > > +test_expect_success 'compare range vs mbox' ' > > + git format-patch --stdout topic..mode-only-change >mbox && > > + git range-diff topic...mode-only-change >expect && > > ++ > > + git range-diff mode-only-change..topic mbox:./mbox >actual && > > + test_cmp expect actual && > > -+ git range-diff mode-only-change..topic mbox:- actual && > > -+ test_cmp expect actual > > ++ > > I'm a bit confused by this sed command, I've annotated it with my probably > flawed understanding. > > > ++ sed -e "/^From: .*/{ > > ++ h > > This stores the From: header in the hold space 👍 > > ++ s/.*/From: Bugs Bunny / > > This changes the From: header in the pattern space 👍 > > ++ :1 > > ++ N > > ++ /[ -z]$/b1 > > We loop until we find a line that does not end with a space, letter or number > adding the lines to the hold space I would have _loved_ to match on an empty line, i.e. `/[^\n]$/b1`. But that construct is not understood by the `sed` on macOS. I even went so far as to search for the source code of a BSD `sed` (and I found it, and modified the code so that it builds on Linux, see https://github.com/dscho/sed-bsd) to try a couple of things, but could not make it work with any variation of `\n`. Therefore, I settled on expecting all the lines in the commit header to end in printable ASCII characters between the space and the `z`. > > ++ G > > This appends the hold space to the pattern space, then the pattern space is > printed. 👍 > Doesn't this mean we end up with two From: headers? Is the in-body From: > line already present? There is no in-body `From:` header because the patch author matches the `GIT_AUTHOR_IDENT` that is in effect while running the `format-patch` command. Let me show you what this `sed` call deals with. In the local test run, it modified an `mbox` starting with this: From 4d39cb329d3ef4c8e69b43859c2e11adb83f8613 Mon Sep 17 00:00:00 2001 From: Thomas Rast Date: Mon, 22 Jul 2013 11:23:44 +0200 Subject: [PATCH 1/3] s/4/A/ + add other-file --- file | 2 +- other-file | 0 [...] to a modified `mbox` that starts with this: From 4d39cb329d3ef4c8e69b43859c2e11adb83f8613 Mon Sep 17 00:00:00 2001 From: Bugs Bunny Date: Mon, 22 Jul 2013 11:23:44 +0200 Subject: [PATCH 1/3] s/4/A/ + add other-file From: Thomas Rast --- file | 2 +- other-file | 0 [...] > > > ++ }" mbox.from && > > ++ git range-diff mode-only-change..topic mbox:./mbox.from > > >actual.from && > > ++ test_cmp expect actual.from && > > ++ > > ++ append_cr mbox.cr && > > ++ test_must_fail git range-diff \ > > ++ mode-only-change..topic mbox:./mbox.cr 2>err && > > ++ grep CR/LF err && > > Thanks for adding that Thank you for the suggestion to add it! And thank you again for modeling how to perform actionable, helpful and productive code reviews, Dscho