| Message ID | 20210830072118.91921-4-sunshine@sunshineco.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | suppress trailing whitespace on empty "notes" lines | expand |
Eric Sunshine <sunshine@sunshineco.com> writes: > Like other Git commands, `git notes` takes care to call `stripspace` on > the user-supplied note content, thereby ensuring that it has no trailing > whitespace, among other cleanups. However, when notes are inserted into > a patch via `git format-patch --notes`, all lines of the note are > indented unconditionally, including empty lines, which leaves trailing > whitespace on lines which previously were empty, thus negating the > normalization done earlier. Fix this shortcoming. Playing the devil's advocate, it can be argued that using the same leading whitespace on a paragraph break line is actually a good thing. Leaving them in would give the consumer an easy way to see which part was inserted from a note.
On Mon, Aug 30, 2021 at 1:10 PM Junio C Hamano <gitster@pobox.com> wrote: > Eric Sunshine <sunshine@sunshineco.com> writes: > > Like other Git commands, `git notes` takes care to call `stripspace` on > > the user-supplied note content, thereby ensuring that it has no trailing > > whitespace, among other cleanups. However, when notes are inserted into > > a patch via `git format-patch --notes`, all lines of the note are > > indented unconditionally, including empty lines, which leaves trailing > > whitespace on lines which previously were empty, thus negating the > > normalization done earlier. Fix this shortcoming. > > Playing the devil's advocate, it can be argued that using the same > leading whitespace on a paragraph break line is actually a good > thing. Leaving them in would give the consumer an easy way to see > which part was inserted from a note. The, um, angel's response: `git format-patch --notes` is a convenience for the _submitter_ of a series. It is difficult to imagine a scenario[1] in which the _consumer_ of a series would care or need to know whether patch commentary was written by hand, inserted mechanically (by `--notes`), or inserted mechanically and then hand-edited. The trailing whitespace is unusual within the Git sphere, as well as unsightly if you happen to have your editor configured to highlight trailing whitespace, and just "feels" sloppy. [1]: I suppose mechanical extraction of notes may be one such scenario, allowing for simple-minded (not necessarily robust) extraction mechanics; i.e. start extracting after the /^Notes:$/ line and stop at the first line not indented with four blanks.
Eric Sunshine <sunshine@sunshineco.com> writes: > The trailing whitespace is unusual within the Git sphere, as well as > unsightly if you happen to have your editor configured to highlight > trailing whitespace, and just "feels" sloppy. But we are discussing this in the context of format-patch output, where patches that change lines near a blank line will have a line with a single SP on it in common context ;-) I do not feel very strongly either way, though. > [1]: I suppose mechanical extraction of notes may be one such > scenario, allowing for simple-minded (not necessarily robust) > extraction mechanics; i.e. start extracting after the /^Notes:$/ line > and stop at the first line not indented with four blanks. Yup, that was what I had in mind.
On Mon, Aug 30, 2021 at 1:56 PM Junio C Hamano <gitster@pobox.com> wrote: > Eric Sunshine <sunshine@sunshineco.com> writes: > > [1]: I suppose mechanical extraction of notes may be one such > > scenario, allowing for simple-minded (not necessarily robust) > > extraction mechanics; i.e. start extracting after the /^Notes:$/ line > > and stop at the first line not indented with four blanks. > > Yup, that was what I had in mind. In the general case, such an extraction mechanism would be far too fragile since there are no guarantees that the commentary in the "Notes:" section hasn't been hand-edited after patch-generation. However, it's certainly possible that such a simple-minded extraction technique might be applicable in some well-controlled development pipeline somewhere. If we are worried about that, then we can drop this patch series.
On Mon, Aug 30, 2021 at 2:04 PM Eric Sunshine <sunshine@sunshineco.com> wrote: > In the general case, such an extraction mechanism would be far too > fragile since there are no guarantees that the commentary in the > "Notes:" section hasn't been hand-edited after patch-generation. > However, it's certainly possible that such a simple-minded extraction > technique might be applicable in some well-controlled development > pipeline somewhere. > > If we are worried about that, then we can drop this patch series. Have we made a decision about whether this patch series -- which avoids indenting blank notes lines -- is desirable? Or are we worried about backward-compatibility? If we think there is value in this series, then I can re-roll with Ævar suggestions. If not, perhaps I can re-submit just patch [1/3] which makes a few tests less brittle. Or, since those brittle tests aren't necessarily hurting anything, we can just let this series die. Thoughts?
On Fri, Sep 10, 2021 at 1:18 AM Eric Sunshine <sunshine@sunshineco.com> wrote: > Have we made a decision about whether this patch series -- which > avoids indenting blank notes lines -- is desirable? Or are we worried > about backward-compatibility? If we think there is value in this > series, then I can re-roll with Ævar suggestions. If not, perhaps I > can re-submit just patch [1/3] which makes a few tests less brittle. > Or, since those brittle tests aren't necessarily hurting anything, we > can just let this series die. I meant [2/3], not [1/3], as a possibility for a standalone re-submission. That's the patch in which a few tests in t3303 and t9301 which care only whether notes are present (or not) are made less brittle by removing dependence upon the default output format of git-log.
Eric Sunshine <sunshine@sunshineco.com> writes: > Have we made a decision about whether this patch series -- which > avoids indenting blank notes lines -- is desirable? Or are we worried > about backward-compatibility? I do not know about "have we made" part of the question, but an input from me to come to an answer to the question is that, while I can see why it may be desirable in some cases, I do not view it as compelling enough to risk any unforeseen breakage to other peoples' workflow. My opinion is based on an assumption that it is desirable because it would squelch "here is a trailing whitespace" noise in an editor and/or a pager that is appropriately configured and allow the user to spot whitespace breakages in the payload more easily and for no other reason. If there are other reasons that make this change desirable, they might influence my opinion.
On Fri, Sep 10, 2021 at 2:33 PM Junio C Hamano <gitster@pobox.com> wrote: > Eric Sunshine <sunshine@sunshineco.com> writes: > > Have we made a decision about whether this patch series -- which > > avoids indenting blank notes lines -- is desirable? Or are we worried > > about backward-compatibility? > > I do not know about "have we made" part of the question, but an > input from me to come to an answer to the question is that, while I > can see why it may be desirable in some cases, I do not view it as > compelling enough to risk any unforeseen breakage to other peoples' > workflow. My opinion is based on an assumption that it is desirable > because it would squelch "here is a trailing whitespace" noise in an > editor and/or a pager that is appropriately configured and allow the > user to spot whitespace breakages in the payload more easily and for > no other reason. If there are other reasons that make this change > desirable, they might influence my opinion. Thank you for the response. I didn't have any other reason beyond squelching "here is trailing whitespace" noise when submitting the series. Thus, I can't provide any other reasons to promote the change as desirable.
On Fri, Sep 10 2021, Eric Sunshine wrote: > On Fri, Sep 10, 2021 at 2:33 PM Junio C Hamano <gitster@pobox.com> wrote: >> Eric Sunshine <sunshine@sunshineco.com> writes: >> > Have we made a decision about whether this patch series -- which >> > avoids indenting blank notes lines -- is desirable? Or are we worried >> > about backward-compatibility? >> >> I do not know about "have we made" part of the question, but an >> input from me to come to an answer to the question is that, while I >> can see why it may be desirable in some cases, I do not view it as >> compelling enough to risk any unforeseen breakage to other peoples' >> workflow. My opinion is based on an assumption that it is desirable >> because it would squelch "here is a trailing whitespace" noise in an >> editor and/or a pager that is appropriately configured and allow the >> user to spot whitespace breakages in the payload more easily and for >> no other reason. If there are other reasons that make this change >> desirable, they might influence my opinion. > > Thank you for the response. I didn't have any other reason beyond > squelching "here is trailing whitespace" noise when submitting the > series. Thus, I can't provide any other reasons to promote the change > as desirable. This change per-se seems nice, but even having reviewed it to the point of rewriting parts of it, I didn't really look into what the whole workflow you were trying to address is. So e.g. just to pick a random commit of your for show: $ git show c990a4c11dd | sed 's/$/Z/' commit c990a4c11ddZ Author: Eric Sunshine <sunshine@sunshineco.com>Z Date: Mon Jul 6 13:30:45 2015 -0400Z Z checkout: fix bug with --to and relative HEADZ Z Given "git checkout --to <path> HEAD~1", the new worktree's HEAD shouldZ begin life at the current branch's HEAD~1, however, it actually ends upZ at HEAD~2. This happens because:Z Z 1. git-checkout resolves HEAD~1Z Z [...] Here we end up also adding the whitespace indenting to the empty lines, whereas if we were trying to feed this to an editor we'd place those later Z's at the start of our line. Are notes different? Or are they just similarly indented? For commits we don't insert that leading whitespace in the commit object, do notes get that part wrong too? It might be showing, but I've only used notes a few times, my main use of them is Junio's amlog. So even for someone experienced in git, I think some show & tell of step-by-step showing in the commit message how we end up with X before, and have Y with this change would help a lot.
On Fri, Sep 10, 2021 at 9:59 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > >> Eric Sunshine <sunshine@sunshineco.com> writes: > >> > Have we made a decision about whether this patch series -- which > >> > avoids indenting blank notes lines -- is desirable? Or are we worried > >> > about backward-compatibility? > > This change per-se seems nice, but even having reviewed it to the point > of rewriting parts of it, I didn't really look into what the whole > workflow you were trying to address is. > > So e.g. just to pick a random commit of your for show: > $ git show c990a4c11dd | sed 's/$/Z/' > Here we end up also adding the whitespace indenting to the empty lines, > whereas if we were trying to feed this to an editor we'd place those > later Z's at the start of our line. I'm not sure what you mean by "feed this to an editor". Do you mean sending the output of `git show` to an editor? I'm guessing that's not what you mean, and that you instead are talking about editing the commit message in an editor (say, via the "reword" option of `git rebase --interactive`). > Are notes different? Or are they just similarly indented? For commits we > don't insert that leading whitespace in the commit object, do notes get > that part wrong too? Notes don't store the indented blank lines; it's only at output time, such as with `git format-patch --notes` that the blank lines get indented along with the rest of the note text (just as is happening in your `git show` example in which the entire commit message is being indented, including the blank lines). > It might be showing, but I've only used notes a few times, my main use > of them is Junio's amlog. I also have only used notes a few times. > So even for someone experienced in git, I think some show & tell of > step-by-step showing in the commit message how we end up with X before, > and have Y with this change would help a lot. This all came about due to two unrelated circumstances: (1) a few months ago, I configured Emacs to highlight trailing whitespace, and (2) I decided to use `notes` to add commentary to a commit since, although I normally just write the commentary directly in the patch itself after running `git format-patch`, in this case, it likely will be weeks or months before I finish the series, and was worried that I'd forget the intended commentary by that time, thus recorded it as a note. Since I've almost never used notes, I ran `git format-patch --notes` as a test and was surprised to see the trailing whitespace on the "blank" lines when viewing the patch in the editor. This submission started as a single patch which just "fixed" the bug and added a test. Only after that was complete (but before I submitted the patch), did I discover that other tests in the suite were failing since the "fix" also changed git-log's default output format which includes notes (indented). Since I so rarely use notes, I had either forgotten that git-log showed notes or didn't know in the first place. The submission grew to multiple patches due to fixing those newly-failing tests. Anyhow, since then, I've discovered that `git format-patch --range-diff` also indents blank lines. And you've now shown that `git show` does, as well, so the behavior which triggered this "fix" turns out to be somewhat normal in this project, rather than a one-off "bug" in need of a fix.
On Sat, Sep 11 2021, Eric Sunshine wrote: > On Fri, Sep 10, 2021 at 9:59 PM Ævar Arnfjörð Bjarmason > <avarab@gmail.com> wrote: >> >> Eric Sunshine <sunshine@sunshineco.com> writes: >> >> > Have we made a decision about whether this patch series -- which >> >> > avoids indenting blank notes lines -- is desirable? Or are we worried >> >> > about backward-compatibility? >> >> This change per-se seems nice, but even having reviewed it to the point >> of rewriting parts of it, I didn't really look into what the whole >> workflow you were trying to address is. >> >> So e.g. just to pick a random commit of your for show: >> $ git show c990a4c11dd | sed 's/$/Z/' >> Here we end up also adding the whitespace indenting to the empty lines, >> whereas if we were trying to feed this to an editor we'd place those >> later Z's at the start of our line. > > I'm not sure what you mean by "feed this to an editor". Do you mean > sending the output of `git show` to an editor? I'm guessing that's not > what you mean, and that you instead are talking about editing the > commit message in an editor (say, via the "reword" option of `git > rebase --interactive`). Feed it to whatever, maybe I have a commit message in my terminal I highlight and copy/paste, my shell/terminal is highlighting line-endings etc. I've got a default bias towards trimming this whitespace, I'm just wondering why notes are a special-case as opposed to our more general log/notes etc. output. >> Are notes different? Or are they just similarly indented? For commits we >> don't insert that leading whitespace in the commit object, do notes get >> that part wrong too? > > Notes don't store the indented blank lines; it's only at output time, > such as with `git format-patch --notes` that the blank lines get > indented along with the rest of the note text (just as is happening in > your `git show` example in which the entire commit message is being > indented, including the blank lines). Ah, so with your change we'd end up with trimmed notes, but not the trimmed main body of the commit message? We don't have to fix everything at once, just establishing context, maybe it's useful for format-patch etc. in isolation... >> It might be showing, but I've only used notes a few times, my main use >> of them is Junio's amlog. > > I also have only used notes a few times. > >> So even for someone experienced in git, I think some show & tell of >> step-by-step showing in the commit message how we end up with X before, >> and have Y with this change would help a lot. > > This all came about due to two unrelated circumstances: (1) a few > months ago, I configured Emacs to highlight trailing whitespace, and > (2) I decided to use `notes` to add commentary to a commit since, > although I normally just write the commentary directly in the patch > itself after running `git format-patch`, in this case, it likely will > be weeks or months before I finish the series, and was worried that > I'd forget the intended commentary by that time, thus recorded it as a > note. Since I've almost never used notes, I ran `git format-patch > --notes` as a test and was surprised to see the trailing whitespace on > the "blank" lines when viewing the patch in the editor. > > This submission started as a single patch which just "fixed" the bug > and added a test. Only after that was complete (but before I submitted > the patch), did I discover that other tests in the suite were failing > since the "fix" also changed git-log's default output format which > includes notes (indented). Since I so rarely use notes, I had either > forgotten that git-log showed notes or didn't know in the first place. > The submission grew to multiple patches due to fixing those > newly-failing tests. > > Anyhow, since then, I've discovered that `git format-patch > --range-diff` also indents blank lines. And you've now shown that `git > show` does, as well, so the behavior which triggered this "fix" turns > out to be somewhat normal in this project, rather than a one-off "bug" > in need of a fix. Per the above I wouldn't mind this just being changed for all of them, even one at a time.
On Sat, Sep 11, 2021 at 6:43 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > On Sat, Sep 11 2021, Eric Sunshine wrote: > > Notes don't store the indented blank lines; it's only at output time, > > such as with `git format-patch --notes` that the blank lines get > > indented along with the rest of the note text (just as is happening in > > your `git show` example in which the entire commit message is being > > indented, including the blank lines). > > Ah, so with your change we'd end up with trimmed notes, but not the > trimmed main body of the commit message? That's correct. This "fix" is specific to the note-printing machinery which is invoked by (at least) git-format-patch and git-log. (Until your demonstration of git-show indentation, I wasn't even aware that blank lines in commit messages were getting indented there, as well.) > > Anyhow, since then, I've discovered that `git format-patch > > --range-diff` also indents blank lines. And you've now shown that `git > > show` does, as well, so the behavior which triggered this "fix" turns > > out to be somewhat normal in this project, rather than a one-off "bug" > > in need of a fix. > > Per the above I wouldn't mind this just being changed for all of them, > even one at a time. I'm not a fan of the trailing whitespace either, however, Junio does have the concern that there may be some tooling somewhere which relies upon the "indented blank lines" (even if such tooling would not be robust).
Eric Sunshine <sunshine@sunshineco.com> writes: > I'm not a fan of the trailing whitespace either, however, Junio does > have the concern that there may be some tooling somewhere which relies > upon the "indented blank lines" (even if such tooling would not be > robust). Note that such a "concern" is always relative. If the upside were so great, perhaps risking possible breakage may be warranted. FWIW, I am not a big fan of trailing whitespaces that human writers leave in what they write, either. Because the patch text will be full of lines with trailing whitespaces anyway due to a blank lines in the patch context, however, it does not sound so great an upside to tweak how the paragraphs taken from the notes are inserted. Thanks.
diff --git a/notes.c b/notes.c index f87dac4068..25e0a59899 100644 --- a/notes.c +++ b/notes.c @@ -1295,7 +1295,7 @@ static void format_note(struct notes_tree *t, const struct object_id *object_oid for (msg_p = msg; msg_p < msg + msglen; msg_p += linelen + 1) { linelen = strchrnul(msg_p, '\n') - msg_p; - if (!raw) + if (!raw && linelen) strbuf_addstr(sb, " "); strbuf_add(sb, msg_p, linelen); strbuf_addch(sb, '\n'); diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh index 712d4b5ddf..7406e5fe99 100755 --- a/t/t4014-format-patch.sh +++ b/t/t4014-format-patch.sh @@ -906,6 +906,23 @@ test_expect_success 'format-patch with multiple notes refs in config' ' grep "this is note 2" out ' +test_expect_success 'format-patch --notes does not indent empty lines' ' + git notes add --file=- HEAD <<-\EOF && + paragraph 1 + + paragraph 2 + EOF + test_when_finished "git notes remove HEAD" && + git format-patch -1 --stdout --notes >out && + sed -n "/^[ ]*paragraph 1$/,/^[ ]*paragraph 2$/{s/^[ ][ ]*/[indent]/;p;}" out >actual && + cat >expect <<-\EOF && + [indent]paragraph 1 + + [indent]paragraph 2 + EOF + test_cmp expect actual +' + echo "fatal: --name-only does not make sense" >expect.name-only echo "fatal: --name-status does not make sense" >expect.name-status echo "fatal: --check does not make sense" >expect.check
Like other Git commands, `git notes` takes care to call `stripspace` on the user-supplied note content, thereby ensuring that it has no trailing whitespace, among other cleanups. However, when notes are inserted into a patch via `git format-patch --notes`, all lines of the note are indented unconditionally, including empty lines, which leaves trailing whitespace on lines which previously were empty, thus negating the normalization done earlier. Fix this shortcoming. Signed-off-by: Eric Sunshine <sunshine@sunshineco.com> --- notes.c | 2 +- t/t4014-format-patch.sh | 17 +++++++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-)