Re: [PATCH v2] range-diff: support reading mbox files
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Nov 22, 2022, 07:40 UTC
- Message-ID
- <rr7sp534-43o9-7n1o-5700-369n5rprq75p@tzk.qr>
- In-Reply-To
- <8d40f170-650f-800a-02d7-d279186d5883@dunelm.org.uk>
Hi Phillip,
On Mon, 21 Nov 2022, Phillip Wood wrote:
Show 21 quoted lines
> [...]
> > @@ 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.
Show 42 quoted lines
>
> > + 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.
Show 19 quoted lines
> > @@ 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:- <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 <bugs@bun.ny>/ > > This changes the From: header in the pattern space
👍
Show 6 quoted lines
> > ++ :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 <trast@inf.ethz.ch> 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 <bugs@bun.ny> Date: Mon, 22 Jul 2013 11:23:44 +0200 Subject: [PATCH 1/3] s/4/A/ + add other-file
From: Thomas Rast <trast@inf.ethz.ch> --- file | 2 +- other-file | 0 [...]
Show 12 quoted lines
> > > ++ }" <mbox >mbox.from && > > ++ git range-diff mode-only-change..topic mbox:./mbox.from > > >actual.from && > > ++ test_cmp expect actual.from && > > ++ > > ++ append_cr <mbox >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