From: Junio C Hamano <hidden> Date: 2021-09-10 18:33:55
Eric Sunshine [off-list ref] 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.
From: Eric Sunshine <hidden> Date: 2021-09-10 20:31:36
On Fri, Sep 10, 2021 at 2:33 PM Junio C Hamano [off-list ref] wrote:
Eric Sunshine [off-list ref] writes:
quoted
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 at 2:33 PM Junio C Hamano [off-list ref] wrote:
quoted
Eric Sunshine [off-list ref] writes:
quoted
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 [off-list ref]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.
From: Eric Sunshine <hidden> Date: 2021-09-11 09:15:57
On Fri, Sep 10, 2021 at 9:59 PM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:
quoted
quoted
Eric Sunshine [off-list ref] writes:
quoted
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 Fri, Sep 10, 2021 at 9:59 PM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:
quoted
quoted
quoted
Eric Sunshine [off-list ref] writes:
quoted
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.
quoted
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...
quoted
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.
quoted
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.
From: Eric Sunshine <hidden> Date: 2021-09-12 05:53:37
On Sat, Sep 11, 2021 at 6:43 AM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:
On Sat, Sep 11 2021, Eric Sunshine wrote:
quoted
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.)
quoted
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).
From: Junio C Hamano <hidden> Date: 2021-09-12 08:22:54
Eric Sunshine [off-list ref] 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.