From: Phillip Wood Date: Tue, 22 Nov 2022 14:20:08 GMT Subject: Re: [PATCH v2] range-diff: support reading mbox files Message-ID: In-Reply-To: Hi Dscho On 22/11/2022 07:40, Johannes Schindelin wrote: > [...] >> 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 don't think it matters much but can't you match an empty line with /^$/ ? Then you can loop on non-empty lines with /^$/!b1 > 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: Thanks for taking the trouble to show the mbox files, I didn't have time to run the tests my self yesterday. The processed mbox file looks good. > 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, Thanks for your kind words, I glad you found my comments helpful Best Wishes Phillip