* [PATCH v2] sequencer: beautify subject of reverts of reverts
@ 2023-04-28 8:35 Oswald Buddenhagen
2023-04-28 18:35 ` Junio C Hamano
` (2 more replies)
0 siblings, 3 replies; 53+ messages in thread
From: Oswald Buddenhagen @ 2023-04-28 8:35 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Kristoffer Haugsbakk
Instead of generating a silly-looking `Revert "Revert "foo""`, make it
a more humane `Reapply "foo"`.
The alternative `Revert^2 "foo"`, etc. was considered, but it was deemed
over-engineered and "too nerdy". Instead, people should get creative
with the subjects when they recurse reverts that deeply. The proposed
change encourages that by example and explicit recommendation.
Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
Cc: Junio C Hamano <gitster@pobox.com>
Cc: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
v2:
- add discussion to commit message
- add paragraph to docu
- add test
- use skip_prefix() instead of starts_with()
- catch pre-existing double reverts
---
Documentation/git-revert.txt | 6 ++++++
sequencer.c | 14 ++++++++++++++
t/t3515-revert-subjects.sh | 32 ++++++++++++++++++++++++++++++++
3 files changed, 52 insertions(+)
create mode 100755 t/t3515-revert-subjects.sh
diff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt
index d2e10d3dce..e8fa513607 100644
--- a/Documentation/git-revert.txt
+++ b/Documentation/git-revert.txt
@@ -31,6 +31,12 @@ both will discard uncommitted changes in your working directory.
See "Reset, restore and revert" in linkgit:git[1] for the differences
between the three commands.
+The command generates the subject 'Revert "<title>"' for the resulting
+commit, assuming the original commit's subject is '<title>'. Reverting
+such a reversion commit in turn yields the subject 'Reapply "<title>"'.
+These can of course be modified in the editor when the reason for
+reverting is described.
+
OPTIONS
-------
<commit>...::
diff --git a/sequencer.c b/sequencer.c
index 3be23d7ca2..61e466470e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2227,13 +2227,27 @@ static int do_pick_commit(struct repository *r,
*/
if (command == TODO_REVERT) {
+ const char *orig_subject;
+
base = commit;
base_label = msg.label;
next = parent;
next_label = msg.parent_label;
if (opts->commit_use_reference) {
strbuf_addstr(&msgbuf,
"# *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***");
+ } else if (skip_prefix(msg.subject, "Revert \"", &orig_subject)) {
+ if (skip_prefix(orig_subject, "Revert \"", &orig_subject)) {
+ /*
+ * This prevents the generation of somewhat unintuitive (even if
+ * not incorrect) 'Reapply "Revert "' titles from legacy double
+ * reverts. Fixing up deeper recursions is left to the user.
+ */
+ strbuf_addstr(&msgbuf, "Revert \"Reapply \"");
+ } else {
+ strbuf_addstr(&msgbuf, "Reapply \"");
+ }
+ strbuf_addstr(&msgbuf, orig_subject);
} else {
strbuf_addstr(&msgbuf, "Revert \"");
strbuf_addstr(&msgbuf, msg.subject);
diff --git a/t/t3515-revert-subjects.sh b/t/t3515-revert-subjects.sh
new file mode 100755
index 0000000000..ea4319fd15
--- /dev/null
+++ b/t/t3515-revert-subjects.sh
@@ -0,0 +1,32 @@
+#!/bin/sh
+
+test_description='git revert produces the expected subject'
+
+. ./test-lib.sh
+
+test_expect_success 'fresh reverts' '
+ test_commit --no-tag A file1 &&
+ test_commit --no-tag B file1 &&
+ git revert --no-edit HEAD &&
+ echo "Revert \"B\"" > expect &&
+ git log -1 --pretty=%s > actual &&
+ test_cmp expect actual &&
+ git revert --no-edit HEAD &&
+ echo "Reapply \"B\"" > expect &&
+ git log -1 --pretty=%s > actual &&
+ test_cmp expect actual &&
+ git revert --no-edit HEAD &&
+ echo "Revert \"Reapply \"B\"\"" > expect &&
+ git log -1 --pretty=%s > actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'legacy double revert' '
+ test_commit --no-tag "Revert \"Revert \"B\"\"" file1 &&
+ git revert --no-edit HEAD &&
+ echo "Revert \"Reapply \"B\"\"" > expect &&
+ git log -1 --pretty=%s > actual &&
+ test_cmp expect actual
+'
+
+test_done
--
2.40.0.152.g15d061e6df
^ permalink raw reply related [flat|nested] 53+ messages in thread
* Re: [PATCH v2] sequencer: beautify subject of reverts of reverts
2023-04-28 8:35 [PATCH v2] sequencer: beautify subject of reverts of reverts Oswald Buddenhagen
@ 2023-04-28 18:35 ` Junio C Hamano
2023-04-28 19:11 ` Oswald Buddenhagen
2023-05-17 9:05 ` Phillip Wood
2023-08-09 17:15 ` [PATCH v3 1/2] " Oswald Buddenhagen
2 siblings, 1 reply; 53+ messages in thread
From: Junio C Hamano @ 2023-04-28 18:35 UTC (permalink / raw)
To: Oswald Buddenhagen; +Cc: git, Kristoffer Haugsbakk
Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
> Instead of generating a silly-looking `Revert "Revert "foo""`, make it
> a more humane `Reapply "foo"`.
>
> The alternative `Revert^2 "foo"`, etc. was considered, but it was deemed
> over-engineered and "too nerdy". Instead, people should get creative
> with the subjects when they recurse reverts that deeply. The proposed
> change encourages that by example and explicit recommendation.
>
> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
> ---
> diff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt
> index d2e10d3dce..e8fa513607 100644
> --- a/Documentation/git-revert.txt
> +++ b/Documentation/git-revert.txt
> @@ -31,6 +31,12 @@ both will discard uncommitted changes in your working directory.
> See "Reset, restore and revert" in linkgit:git[1] for the differences
> between the three commands.
>
> +The command generates the subject 'Revert "<title>"' for the resulting
> +commit, assuming the original commit's subject is '<title>'. Reverting
> +such a reversion commit in turn yields the subject 'Reapply "<title>"'.
Clearly written.
> +These can of course be modified in the editor when the reason for
> +reverting is described.
Not just the title but the entire message can be edited and that is
by design. Having to modify what this new mechanism does when
existing users do not like the new behaviour will annoy them, and
this sentence will not be a good enough excuse to ask them
forgiveness for breaking their established practice, either.
So, I am not sure if there is a point to have this sentence here.
> diff --git a/sequencer.c b/sequencer.c
> index 3be23d7ca2..61e466470e 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2227,13 +2227,27 @@ static int do_pick_commit(struct repository *r,
> */
>
> if (command == TODO_REVERT) {
> + const char *orig_subject;
> +
> base = commit;
> base_label = msg.label;
> next = parent;
> next_label = msg.parent_label;
> if (opts->commit_use_reference) {
> strbuf_addstr(&msgbuf,
> "# *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***");
> + } else if (skip_prefix(msg.subject, "Revert \"", &orig_subject)) {
> + if (skip_prefix(orig_subject, "Revert \"", &orig_subject)) {
> + /*
> + * This prevents the generation of somewhat unintuitive (even if
> + * not incorrect) 'Reapply "Revert "' titles from legacy double
> + * reverts. Fixing up deeper recursions is left to the user.
> + */
Good comment but in an overwide paragraph.
> + strbuf_addstr(&msgbuf, "Revert \"Reapply \"");
> + } else {
> + strbuf_addstr(&msgbuf, "Reapply \"");
> + }
> + strbuf_addstr(&msgbuf, orig_subject);
> } else {
> strbuf_addstr(&msgbuf, "Revert \"");
> strbuf_addstr(&msgbuf, msg.subject);
> diff --git a/t/t3515-revert-subjects.sh b/t/t3515-revert-subjects.sh
> new file mode 100755
> index 0000000000..ea4319fd15
> --- /dev/null
> +++ b/t/t3515-revert-subjects.sh
It is a bit unexpectd that we need an entire new file to test this.
It is doubly bad that the title of the file is only about the
subject of revert commits and does not allow other things to be
added later. Are we planning to have a lot more creativity in how
automatically generated subject of revert commits would read?
If there isn't a good enough test coverage for the "git revert"
command already, then having a new file to test "git revert" would
be an excellent idea, adding one here is a very welcome addition,
and it is perfectly fine to start such a new test with only these
new tests that protects the new "Revert Revert to Reapply" feature.
But if there is a test file already for "git revert" that covers
other behaviour of the command, "create two new commits, i.e. revert
and revert of revert, and then try reverting them and see what their
subject says" ought to be a simple addition or two to such an
existing test file. Doesn't t3501 seem a better home for them? The
last handful of tests there are about how the auto-generated log is
phrased, and would form a good group with this new feature, wouldn't
it?
> @@ -0,0 +1,32 @@
> +#!/bin/sh
> +
> +test_description='git revert produces the expected subject'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'fresh reverts' '
> + test_commit --no-tag A file1 &&
> + test_commit --no-tag B file1 &&
> + git revert --no-edit HEAD &&
> + echo "Revert \"B\"" > expect &&
Style. See Documentation/CodingGuidelines and look for "For shell
scripts specifically".
> + git log -1 --pretty=%s > actual &&
> + test_cmp expect actual &&
> + git revert --no-edit HEAD &&
> + echo "Reapply \"B\"" > expect &&
> + git log -1 --pretty=%s > actual &&
> + test_cmp expect actual &&
> + git revert --no-edit HEAD &&
> + echo "Revert \"Reapply \"B\"\"" > expect &&
> + git log -1 --pretty=%s > actual &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'legacy double revert' '
> + test_commit --no-tag "Revert \"Revert \"B\"\"" file1 &&
> + git revert --no-edit HEAD &&
> + echo "Revert \"Reapply \"B\"\"" > expect &&
> + git log -1 --pretty=%s > actual &&
> + test_cmp expect actual
> +'
> +
> +test_done
Thanks.
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v2] sequencer: beautify subject of reverts of reverts
2023-04-28 18:35 ` Junio C Hamano
@ 2023-04-28 19:11 ` Oswald Buddenhagen
2023-05-01 16:44 ` Junio C Hamano
2023-05-05 17:25 ` Junio C Hamano
0 siblings, 2 replies; 53+ messages in thread
From: Oswald Buddenhagen @ 2023-04-28 19:11 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Kristoffer Haugsbakk
On Fri, Apr 28, 2023 at 11:35:28AM -0700, Junio C Hamano wrote:
>Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
>> +The command generates the subject 'Revert "<title>"' for the resulting
>> +commit, assuming the original commit's subject is '<title>'. Reverting
>> +such a reversion commit in turn yields the subject 'Reapply "<title>"'.
>
>Clearly written.
>
>> +These can of course be modified in the editor when the reason for
>> +reverting is described.
>
>Not just the title but the entire message can be edited and that is
>by design. Having to modify what this new mechanism does when
>existing users do not like the new behaviour will annoy them, and
>this sentence will not be a good enough excuse to ask them
>forgiveness for breaking their established practice, either.
>
>So, I am not sure if there is a point to have this sentence here.
>
well, it's the one sentence i copied verbatim from your proposal. :-D
but i don't get the argument anyway. i think the docu is pretty
pointless except to emphasize that the generated subject is a default
that should be edited when circumstances recommend it. in fact, i
wouldn't mind writing just that, with a notice that the default attempts
to be somewhat natural for repeated reverts.
>> strbuf_addstr(&msgbuf,
>> "# *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***");
>> + } else if (skip_prefix(msg.subject, "Revert \"", &orig_subject)) {
>> + if (skip_prefix(orig_subject, "Revert \"", &orig_subject)) {
>> + /*
>> + * This prevents the generation of somewhat unintuitive (even if
>> + * not incorrect) 'Reapply "Revert "' titles from legacy double
>> + * reverts. Fixing up deeper recursions is left to the user.
>> + */
>
>Good comment but in an overwide paragraph.
>
there are several lines in the lower 90-ies in that file, one of them
seen in the patch context. would 88 be fine?
(too narrow flowed text looks silly, imo.)
>Doesn't t3501 seem a better home for them?
>
looking closer at it, i guess it kind of does. the file's contents have
clearly grown to fulfill the filename's broad promise, but nobody
bothered to adjust the test description and make the setup title more
specific. any takers?
-- ossi
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v2] sequencer: beautify subject of reverts of reverts
2023-04-28 19:11 ` Oswald Buddenhagen
@ 2023-05-01 16:44 ` Junio C Hamano
2023-05-01 19:10 ` Oswald Buddenhagen
2023-05-05 17:25 ` Junio C Hamano
1 sibling, 1 reply; 53+ messages in thread
From: Junio C Hamano @ 2023-05-01 16:44 UTC (permalink / raw)
To: Oswald Buddenhagen; +Cc: git, Kristoffer Haugsbakk
Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
> but i don't get the argument anyway. i think the docu is pretty
> pointless except to emphasize that the generated subject is a default
> that should be edited when circumstances recommend it. in fact, i
> wouldn't mind writing just that, with a notice that the default
> attempts to be somewhat natural for repeated reverts.
I think it is very well known that the user gets the default message
to be edited in the editor. I would understand if the instruction
is "do not edit this line, because ...", but otherwise it is pretty
up to the user to do whatever they like to the log message, no?
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v2] sequencer: beautify subject of reverts of reverts
2023-05-01 16:44 ` Junio C Hamano
@ 2023-05-01 19:10 ` Oswald Buddenhagen
2023-05-01 19:12 ` Junio C Hamano
0 siblings, 1 reply; 53+ messages in thread
From: Oswald Buddenhagen @ 2023-05-01 19:10 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Kristoffer Haugsbakk
On Mon, May 01, 2023 at 09:44:06AM -0700, Junio C Hamano wrote:
>Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
>
>> but i don't get the argument anyway. i think the docu is pretty
>> pointless except to emphasize that the generated subject is a default
>> that should be edited when circumstances recommend it. in fact, i
>> wouldn't mind writing just that, with a notice that the default
>> attempts to be somewhat natural for repeated reverts.
>
>I think it is very well known that the user gets the default message
>to be edited in the editor. I would understand if the instruction
>is "do not edit this line, because ...", but otherwise it is pretty
>up to the user to do whatever they like to the log message, no?
>
well, yeah. but not everybody seems to get that editing the title
specifically is actually an acceptable thing to do - see my earlier
messages in the other sub-thread. so i'd go with something like:
Git attempts to make the subject of reverts of reverts somewhat
natural, but will inevitably fail at greater depths. Editing these
subjects is recommended.
also, this should be probably a note near the bottom, rather than being
so close to the top. in fact, it's probably a good idea to add a
DISCUSSION section with some basic guidelines, like git-commit has.
-- ossi
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v2] sequencer: beautify subject of reverts of reverts
2023-05-01 19:10 ` Oswald Buddenhagen
@ 2023-05-01 19:12 ` Junio C Hamano
0 siblings, 0 replies; 53+ messages in thread
From: Junio C Hamano @ 2023-05-01 19:12 UTC (permalink / raw)
To: Oswald Buddenhagen; +Cc: git, Kristoffer Haugsbakk
Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
> being so close to the top. in fact, it's probably a good idea to add a
> DISCUSSION section with some basic guidelines, like git-commit has.
;-) Sounds quite sensible.
Thanks.
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v2] sequencer: beautify subject of reverts of reverts
2023-04-28 19:11 ` Oswald Buddenhagen
2023-05-01 16:44 ` Junio C Hamano
@ 2023-05-05 17:25 ` Junio C Hamano
1 sibling, 0 replies; 53+ messages in thread
From: Junio C Hamano @ 2023-05-05 17:25 UTC (permalink / raw)
To: Oswald Buddenhagen; +Cc: git, Kristoffer Haugsbakk
Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
>>Doesn't t3501 seem a better home for them?
>>
> looking closer at it, i guess it kind of does. the file's contents
> have clearly grown to fulfill the filename's broad promise, but nobody
> bothered to adjust the test description and make the setup title more
> specific. any takers?
Just dropping "with renames" from the test description would be
fine, no? Existing tests in the early part of the script cover
not just renames but unknown command line option, operating on a
dirty working tree, etc. that are not specific to any renames.
One more thing I forgot was that your test scripts were indented by
4 spaces; please use tabs for indent to match existing ones when you
add tests to an existing script.
Thanks.
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v2] sequencer: beautify subject of reverts of reverts
2023-04-28 8:35 [PATCH v2] sequencer: beautify subject of reverts of reverts Oswald Buddenhagen
2023-04-28 18:35 ` Junio C Hamano
@ 2023-05-17 9:05 ` Phillip Wood
2023-05-17 10:00 ` Oswald Buddenhagen
2023-08-09 17:15 ` [PATCH v3 1/2] " Oswald Buddenhagen
2 siblings, 1 reply; 53+ messages in thread
From: Phillip Wood @ 2023-05-17 9:05 UTC (permalink / raw)
To: Oswald Buddenhagen, git; +Cc: Junio C Hamano, Kristoffer Haugsbakk
Hi Oswald
On 28/04/2023 09:35, Oswald Buddenhagen wrote:
> Instead of generating a silly-looking `Revert "Revert "foo""`, make it
> a more humane `Reapply "foo"`.
>
> The alternative `Revert^2 "foo"`, etc. was considered, but it was deemed
> over-engineered and "too nerdy". Instead, people should get creative
> with the subjects when they recurse reverts that deeply. The proposed
> change encourages that by example and explicit recommendation.
>
> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
> Cc: Junio C Hamano <gitster@pobox.com>
> Cc: Kristoffer Haugsbakk <code@khaugsbakk.name>
> ---
> v2:
> - add discussion to commit message
> - add paragraph to docu
> - add test
> - use skip_prefix() instead of starts_with()
> - catch pre-existing double reverts
> ---
> Documentation/git-revert.txt | 6 ++++++
> sequencer.c | 14 ++++++++++++++
> t/t3515-revert-subjects.sh | 32 ++++++++++++++++++++++++++++++++
> 3 files changed, 52 insertions(+)
> create mode 100755 t/t3515-revert-subjects.sh
>
> diff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt
> index d2e10d3dce..e8fa513607 100644
> --- a/Documentation/git-revert.txt
> +++ b/Documentation/git-revert.txt
> @@ -31,6 +31,12 @@ both will discard uncommitted changes in your working directory.
> See "Reset, restore and revert" in linkgit:git[1] for the differences
> between the three commands.
>
> +The command generates the subject 'Revert "<title>"' for the resulting
> +commit, assuming the original commit's subject is '<title>'. Reverting
> +such a reversion commit in turn yields the subject 'Reapply "<title>"'.
> +These can of course be modified in the editor when the reason for
> +reverting is described.
> +
> OPTIONS
> -------
> <commit>...::
> diff --git a/sequencer.c b/sequencer.c
> index 3be23d7ca2..61e466470e 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2227,13 +2227,27 @@ static int do_pick_commit(struct repository *r,
> */
>
> if (command == TODO_REVERT) {
> + const char *orig_subject;
> +
> base = commit;
> base_label = msg.label;
> next = parent;
> next_label = msg.parent_label;
> if (opts->commit_use_reference) {
> strbuf_addstr(&msgbuf,
> "# *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***");
> + } else if (skip_prefix(msg.subject, "Revert \"", &orig_subject)) {
> + if (skip_prefix(orig_subject, "Revert \"", &orig_subject)) {
I think it is probably worth adding
if (starts_with(orig_subject, "Revert \""))
strbuf_addstr(&msgbuf, "Revert \"");
else
here to make sure that we don't end up with a subject starting "Revert
\"Reapply \"Revert ...".
Best Wishes
Phillip
> + /*
> + * This prevents the generation of somewhat unintuitive (even if
> + * not incorrect) 'Reapply "Revert "' titles from legacy double
> + * reverts. Fixing up deeper recursions is left to the user.
> + */
> + strbuf_addstr(&msgbuf, "Revert \"Reapply \"");
> + } else {
> + strbuf_addstr(&msgbuf, "Reapply \"");
> + }
> + strbuf_addstr(&msgbuf, orig_subject);
> } else {
> strbuf_addstr(&msgbuf, "Revert \"");
> strbuf_addstr(&msgbuf, msg.subject);
> diff --git a/t/t3515-revert-subjects.sh b/t/t3515-revert-subjects.sh
> new file mode 100755
> index 0000000000..ea4319fd15
> --- /dev/null
> +++ b/t/t3515-revert-subjects.sh
> @@ -0,0 +1,32 @@
> +#!/bin/sh
> +
> +test_description='git revert produces the expected subject'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'fresh reverts' '
> + test_commit --no-tag A file1 &&
> + test_commit --no-tag B file1 &&
> + git revert --no-edit HEAD &&
> + echo "Revert \"B\"" > expect &&
> + git log -1 --pretty=%s > actual &&
> + test_cmp expect actual &&
> + git revert --no-edit HEAD &&
> + echo "Reapply \"B\"" > expect &&
> + git log -1 --pretty=%s > actual &&
> + test_cmp expect actual &&
> + git revert --no-edit HEAD &&
> + echo "Revert \"Reapply \"B\"\"" > expect &&
> + git log -1 --pretty=%s > actual &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'legacy double revert' '
> + test_commit --no-tag "Revert \"Revert \"B\"\"" file1 &&
> + git revert --no-edit HEAD &&
> + echo "Revert \"Reapply \"B\"\"" > expect &&
> + git log -1 --pretty=%s > actual &&
> + test_cmp expect actual
> +'
> +
> +test_done
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v2] sequencer: beautify subject of reverts of reverts
2023-05-17 9:05 ` Phillip Wood
@ 2023-05-17 10:00 ` Oswald Buddenhagen
2023-05-17 11:20 ` Phillip Wood
0 siblings, 1 reply; 53+ messages in thread
From: Oswald Buddenhagen @ 2023-05-17 10:00 UTC (permalink / raw)
To: phillip.wood; +Cc: git, Junio C Hamano, Kristoffer Haugsbakk
On Wed, May 17, 2023 at 10:05:51AM +0100, Phillip Wood wrote:
>On 28/04/2023 09:35, Oswald Buddenhagen wrote:
>> + } else if (skip_prefix(msg.subject, "Revert \"", &orig_subject)) {
>> + if (skip_prefix(orig_subject, "Revert \"", &orig_subject)) {
>
>I think it is probably worth adding
>
> if (starts_with(orig_subject, "Revert \""))
> strbuf_addstr(&msgbuf, "Revert \"");
> else
>
>here to make sure that we don't end up with a subject starting "Revert
>\"Reapply \"Revert ...".
>
i can't follow you.
how is the concern not covered by the subsequent comment?
>> + /*
>> + * This prevents the generation of somewhat unintuitive (even if
>> + * not incorrect) 'Reapply "Revert "' titles from legacy double
>> + * reverts. Fixing up deeper recursions is left to the user.
>> + */
regards,
ossi
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v2] sequencer: beautify subject of reverts of reverts
2023-05-17 10:00 ` Oswald Buddenhagen
@ 2023-05-17 11:20 ` Phillip Wood
2023-05-17 17:02 ` Oswald Buddenhagen
0 siblings, 1 reply; 53+ messages in thread
From: Phillip Wood @ 2023-05-17 11:20 UTC (permalink / raw)
To: phillip.wood, git, Junio C Hamano, Kristoffer Haugsbakk
On 17/05/2023 11:00, Oswald Buddenhagen wrote:
> On Wed, May 17, 2023 at 10:05:51AM +0100, Phillip Wood wrote:
>> On 28/04/2023 09:35, Oswald Buddenhagen wrote:
>>> + } else if (skip_prefix(msg.subject, "Revert \"",
>>> &orig_subject)) {
>>> + if (skip_prefix(orig_subject, "Revert \"",
>>> &orig_subject)) {
>>
>> I think it is probably worth adding
>>
>> if (starts_with(orig_subject, "Revert \""))
>> strbuf_addstr(&msgbuf, "Revert \"");
>> else
>>
>> here to make sure that we don't end up with a subject starting "Revert
>> \"Reapply \"Revert ...".
>>
> i can't follow you.
>
> how is the concern not covered by the subsequent comment?
That comment says that reverting a commit with a subject line
Revert "Revert some subject"
will result in the new commit having a subject
Revert "Reapply some subject"
I'm saying that reverting a commit with a subject line
Revert "Revert "Revert some subject""
should result in the new commit having the subject
Revert "Revert "Revert "Revert some subject"""
(i.e. at that point we stop trying to be clever) rather than
Revert "Reapply "Revert some subject""
which I think is what this patch produces.
Best Wishes
Phillip
>>> + /*
>>> + * This prevents the generation of somewhat
>>> unintuitive (even if
>>> + * not incorrect) 'Reapply "Revert "' titles from
>>> legacy double
>>> + * reverts. Fixing up deeper recursions is left to
>>> the user.
>>> + */
>
> regards,
> ossi
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v2] sequencer: beautify subject of reverts of reverts
2023-05-17 11:20 ` Phillip Wood
@ 2023-05-17 17:02 ` Oswald Buddenhagen
2023-05-18 9:58 ` Phillip Wood
0 siblings, 1 reply; 53+ messages in thread
From: Oswald Buddenhagen @ 2023-05-17 17:02 UTC (permalink / raw)
To: phillip.wood; +Cc: git, Junio C Hamano, Kristoffer Haugsbakk
On Wed, May 17, 2023 at 12:20:03PM +0100, Phillip Wood wrote:
>I'm saying that reverting a commit with a subject line
>
> Revert "Revert "Revert some subject""
>
>should result in the new commit having the subject
>
> Revert "Revert "Revert "Revert some subject"""
>
>(i.e. at that point we stop trying to be clever) rather than
>
> Revert "Reapply "Revert some subject""
>
right.
how about filing that under GIGO and extending the comment?
i mean, when you actually run into this situation, you should be
re-thinking your life choices rather than stressing about git producing
a somewhat suboptimal commit message template ...
regards,
ossi
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v2] sequencer: beautify subject of reverts of reverts
2023-05-17 17:02 ` Oswald Buddenhagen
@ 2023-05-18 9:58 ` Phillip Wood
2023-05-18 16:28 ` Junio C Hamano
0 siblings, 1 reply; 53+ messages in thread
From: Phillip Wood @ 2023-05-18 9:58 UTC (permalink / raw)
To: phillip.wood, git, Junio C Hamano, Kristoffer Haugsbakk
On 17/05/2023 18:02, Oswald Buddenhagen wrote:
> On Wed, May 17, 2023 at 12:20:03PM +0100, Phillip Wood wrote:
>> I'm saying that reverting a commit with a subject line
>>
>> Revert "Revert "Revert some subject""
>>
>> should result in the new commit having the subject
>>
>> Revert "Revert "Revert "Revert some subject"""
>>
>> (i.e. at that point we stop trying to be clever) rather than
>>
>> Revert "Reapply "Revert some subject""
>>
> right.
> how about filing that under GIGO and extending the comment?
> i mean, when you actually run into this situation, you should be
> re-thinking your life choices rather than stressing about git producing
> a somewhat suboptimal commit message template ...
Given that it is simple to handle this case and Junio is expecting a
re-roll of this topic[1] then I think it would be worth just adding
those three lines.
Best Wishes
Phillip
[1] https://lore.kernel.org/git/xmqqa5y3ssss.fsf@gitster.g/
> regards,
> ossi
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v2] sequencer: beautify subject of reverts of reverts
2023-05-18 9:58 ` Phillip Wood
@ 2023-05-18 16:28 ` Junio C Hamano
2023-07-28 5:26 ` Linus Arver
0 siblings, 1 reply; 53+ messages in thread
From: Junio C Hamano @ 2023-05-18 16:28 UTC (permalink / raw)
To: Phillip Wood, Oswald Buddenhagen; +Cc: phillip.wood, git, Kristoffer Haugsbakk
Phillip Wood <phillip.wood123@gmail.com> writes:
>>> (i.e. at that point we stop trying to be clever) rather than
>>>
>>> Revert "Reapply "Revert some subject""
>>>
>> right.
>>
>> how about filing that under GIGO and extending the comment?
>> i mean, when you actually run into this situation, you should be
>> re-thinking your life choices rather than stressing about git
>> producing a somewhat suboptimal commit message template ...
>
> Given that it is simple to handle this case and Junio is expecting a
> re-roll of this topic[1] then I think it would be worth just adding
> those three lines.
Phillip, your first message with these three lines were a bit dense
to understand (in other words, to me, it did not immediately "click"
how these lines contribute to avoiding the revert-reapply-revert
sequence that is awkward).
But after reading the exchange bewteen you two [*], your suggestion
makes quite a lot of sense to me. It is better for a code to behave
in a dumb but explainable way, than to attempting and failing to act
too clever for its own worth.
Oswald, I do not think GIGO is really an excuse in this case, when
the only value of the topic is to make the behaviour less awkward by
creating something better than a repeated revert-revert sequence,
revert-reapply-revert is worse, as it is markedly harder to guess
what it really means for a reversion of revert-revert-revert than
"revert" repeated four times. If anything, it is the cleverness of
"lets call revert of revert a reapply" code without Phillip's
suggestion that creates a garbage output, no?
Thanks.
[Footnote]
* Oswald, please refrain from using Mail-Followup-To; I wanted to
deliver the contents of this message specifically to you and
Phillip, but Phillip's MUA followed your Mail-Followup-To and
lost your address, which made me look it up and add it myself.
Please refer to these first before saying "I use it because..."
if you are going to respond:
https://lore.kernel.org/git/7v4pndfjym.fsf@assigned-by-dhcp.cox.net/
https://lore.kernel.org/git/7vei7zjr3y.fsf@alter.siamese.dyndns.org/
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v2] sequencer: beautify subject of reverts of reverts
2023-05-18 16:28 ` Junio C Hamano
@ 2023-07-28 5:26 ` Linus Arver
2023-07-28 9:45 ` Oswald Buddenhagen
0 siblings, 1 reply; 53+ messages in thread
From: Linus Arver @ 2023-07-28 5:26 UTC (permalink / raw)
To: Junio C Hamano, Phillip Wood, Oswald Buddenhagen
Cc: phillip.wood, git, Kristoffer Haugsbakk
Junio C Hamano <gitster@pobox.com> writes:
> It is better for a code to behave
> in a dumb but explainable way, than to attempting and failing to act
> too clever for its own worth.
I completely agree.
> Oswald, I do not think GIGO is really an excuse in this case, when
> the only value of the topic is to make the behaviour less awkward by
> creating something better than a repeated revert-revert sequence,
> revert-reapply-revert is worse, as it is markedly harder to guess
> what it really means for a reversion of revert-revert-revert than
> "revert" repeated four times.
How about introducing a suffix (+ or -) after the word "Revert" to
indicate the application/inclusion (+) or removal (-) of a commit? Example:
- "foo: bar baz quux"
- Revert "foo: bar baz quux"
- Revert(+) Revert(-) "foo: bar baz quux"
- Revert(-) Revert(+) Revert(-) "foo: bar baz quux"
- Revert(+) Revert(-) Revert(+) Revert(-) "foo: bar baz quux"
I think the above increases readability. I chose to keep the same style
as the status quo for the first revert, because the "(-)" suffix alone
without a neighboring "(+)", as in
Revert(-) "foo: bar baz quux"
might confuse users. This style would also do away with the multiple
quoting levels that make the current multi-revert subject lines look
messy at the end. Example:
Revert "Revert "Revert "Revert some subject"""
^
This part is starting to
become noisy.
(Sorry for jumping into this thread so late, but the mention of this
topic on the recent "What's cooking" message [1] (that this topic would
be discarded) got me interested.)
[1] https://lore.kernel.org/git/xmqqpm4d9g54.fsf@gitster.g/T/#mf4edccc7bbc6365a03eaf106121694a27559d275
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v2] sequencer: beautify subject of reverts of reverts
2023-07-28 5:26 ` Linus Arver
@ 2023-07-28 9:45 ` Oswald Buddenhagen
2023-07-28 15:10 ` Junio C Hamano
0 siblings, 1 reply; 53+ messages in thread
From: Oswald Buddenhagen @ 2023-07-28 9:45 UTC (permalink / raw)
To: Linus Arver; +Cc: Junio C Hamano, Phillip Wood, git, Kristoffer Haugsbakk
On Thu, Jul 27, 2023 at 10:26:01PM -0700, Linus Arver wrote:
>How about introducing a suffix (+ or -) after the word "Revert" to
>indicate the application/inclusion (+) or removal (-) of a commit?
>
i think that falls squarely into the "too nerdy" category, like the
Revert^n proposal does.
>(Sorry for jumping into this thread so late, but the mention of this
>topic on the recent "What's cooking" message [1] (that this topic would
>be discarded) got me interested.)
>
i actually have finally updated my patches, but then found a problem in
my script i'm using for submitting them, and want to fix that first for
dogfooding purposes. shouldn't be long.
regards
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v2] sequencer: beautify subject of reverts of reverts
2023-07-28 9:45 ` Oswald Buddenhagen
@ 2023-07-28 15:10 ` Junio C Hamano
2023-07-28 15:37 ` Oswald Buddenhagen
2023-07-28 17:36 ` Linus Arver
0 siblings, 2 replies; 53+ messages in thread
From: Junio C Hamano @ 2023-07-28 15:10 UTC (permalink / raw)
To: Oswald Buddenhagen; +Cc: Linus Arver, Phillip Wood, git, Kristoffer Haugsbakk
Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
> On Thu, Jul 27, 2023 at 10:26:01PM -0700, Linus Arver wrote:
>>How about introducing a suffix (+ or -) after the word "Revert" to
>>indicate the application/inclusion (+) or removal (-) of a commit?
>>
> i think that falls squarely into the "too nerdy" category, like the
> Revert^n proposal does.
True, but instead of dismissing it (or ^n) as "too nerdy", let's
compare it with what we are trying to achieve and see why we feel it
is not desirable. I think we are trying to find a good balance
between aesthetics and usefulness. The former should take lower
precedence, as it would be more subjective between the two.
The usefulness of the message comes from its information content.
What do we want to read out of these messages? I think we want
a title that immediately lets us know three things:
(1) What the original patch was about.
(2) What the final state is.
(3) How involved was the road to get to the final state has been.
As to (1), we are not proposing to lose what comes "Revert", so this
information is not lost under any proposal we have seen so far in
the discussion.
As to (2), with the current "Revert" -> "Revert Revert" -> "Revert
Revert Revert" -> ..., you have to count, which is cumbersome and
does not give you an immediate access to that information. With
"Revert^n", you'd see if n is even or odd to determine, which is
much better than the status quo, but it takes practice to interpret.
With "Revert" -> "Reapply" -> "Revert Reapply" -> "Reapply Reapply"
-> ..., the first word would give you the final state immediately.
We want to know (3), because between a change whose revert was
reverted and a change that hasn't been involved in any revert, there
may be no difference in the end result, the former is likely to be
trickier and merits more careful inspection than the latter. With
"Revert^n", we read how large the number n is to find the
information. With the current "the Revert repeated number of times"
or your "a pair of frontmost Reverts become one Reapply", the length
of the Revert/Reapply prefix conveys this information, but this is
associated with the cost of pushing the original title further to
the right and hard to read/find. Note that, while the number of
times revert-reapply sequence took place is a useful piece of
information, the exact number may not be all that important.
And from the above discussion, I wonder if the following would be a
good place to stop:
- The first revert is as before: Revert "original title"
- A revert of a revert becomes: Reapply "original title"
- A revert of a reapply becomes: Revert Reapply "original title"
- A revert of "Revert Reapply" becomes: Reapply Reapply "original title"
- A revert of "Reapply Reapply" becomes: Revert Reapply "original title"
In other words, we accept the fact that wedo not need exact number
of times reversions were done, and use that to simplify the output
to make sure we will not spend more than two words in the front of
the title. That would help to keep the original title visible,
while still allowing us to distinguish the ones that was reverted up
to four times (and "Revert Reapply" and "Reapply Reapply" only tell
us "final state is to (discard|accept) the original but it took us
_many_ times", without saying exactly how many).
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v2] sequencer: beautify subject of reverts of reverts
2023-07-28 15:10 ` Junio C Hamano
@ 2023-07-28 15:37 ` Oswald Buddenhagen
2023-07-28 16:31 ` Junio C Hamano
2023-07-28 17:36 ` Linus Arver
1 sibling, 1 reply; 53+ messages in thread
From: Oswald Buddenhagen @ 2023-07-28 15:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Linus Arver, Phillip Wood, git, Kristoffer Haugsbakk
On Fri, Jul 28, 2023 at 08:10:42AM -0700, Junio C Hamano wrote:
>And from the above discussion, I wonder if the following would be a
>good place to stop:
>
> - The first revert is as before: Revert "original title"
> - A revert of a revert becomes: Reapply "original title"
> - A revert of a reapply becomes: Revert Reapply "original title"
> - A revert of "Revert Reapply" becomes: Reapply Reapply "original title"
> - A revert of "Reapply Reapply" becomes: Revert Reapply "original title"
>
>In other words, we accept the fact that we do not need exact number
>of times reversions were done, and use that to simplify the output
>to make sure we will not spend more than two words in the front of
>the title. That would help to keep the original title visible,
>while still allowing us to distinguish the ones that was reverted up
>to four times (and "Revert Reapply" and "Reapply Reapply" only tell
>us "final state is to (discard|accept) the original but it took us
>_many_ times", without saying exactly how many).
>
i would not bother automating it, because it falls into the "you should
get creative when that happens" category (which is codified in the
manual by my reworked patches).
also, the "no more than two words" is sort of arbitrary - one can make a
pretty convincing argument for just one word as well.
finally, just dropping that info would typically result in multiple
(non-trivial) commits with the same summary, which i don't really like.
leaving the uglier long variant (and the user hopefully amending it)
avoids it.
i think i'll steal some of the text i didn't quote for the commit
message, though. ^^
regards
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v2] sequencer: beautify subject of reverts of reverts
2023-07-28 15:37 ` Oswald Buddenhagen
@ 2023-07-28 16:31 ` Junio C Hamano
2023-07-28 16:47 ` Oswald Buddenhagen
0 siblings, 1 reply; 53+ messages in thread
From: Junio C Hamano @ 2023-07-28 16:31 UTC (permalink / raw)
To: Oswald Buddenhagen; +Cc: Linus Arver, Phillip Wood, git, Kristoffer Haugsbakk
Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
> also, the "no more than two words" is sort of arbitrary - one can make
> a pretty convincing argument for just one word as well.
I doubt it. If you squash "revert revert revert" into "revert", it
means "revert" no longer means "singly reverted", so you destroy the
goal (3) completely. Using two at least lets you differentiate
"ended up rejecting after reverted multiple times" and "reverted
just once".
> finally, just dropping that info would typically result in multiple
> (non-trivial) commits with the same summary, which i don't really
> like. leaving the uglier long variant (and the user hopefully
> amending it) avoids it.
Actually, I am fine with your
> ... it falls into the "you
> should get creative when that happens" category (which is codified in
> the manual by my reworked patches).
and leave this whole discussion behind it.
If we were doing something, we should make sure what we are doing is
reasonable, and moving away from evaluation criteria like "beautify"
and "too nerdy" and steping back to see what we are trying to
achieve was an attempt to refocus the discussion. From that point
of view, allowing arbitrary number of "Reapply" repeated, optionally
prefixed by a single "Revert", does not sound like it is much better
compared to the current one---is it worth this much time to discuss,
only to halve the length of long runs of "Revert"?
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v2] sequencer: beautify subject of reverts of reverts
2023-07-28 16:31 ` Junio C Hamano
@ 2023-07-28 16:47 ` Oswald Buddenhagen
0 siblings, 0 replies; 53+ messages in thread
From: Oswald Buddenhagen @ 2023-07-28 16:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Linus Arver, Phillip Wood, git, Kristoffer Haugsbakk
On Fri, Jul 28, 2023 at 09:31:49AM -0700, Junio C Hamano wrote:
>From that point
>of view, allowing arbitrary number of "Reapply" repeated, optionally
>prefixed by a single "Revert", does not sound like it is much better
>compared to the current one---is it worth this much time to discuss,
>only to halve the length of long runs of "Revert"?
>
yes, for two reasons:
- the single "reapply" case is actually common; it's usually done after
a previously missed pre-requisite was applied.
- the fact that it's "beautified" _at all_ sends a signal (see previous
mails). it doesn't have to be particularly sophisticated for that.
regards
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v2] sequencer: beautify subject of reverts of reverts
2023-07-28 15:10 ` Junio C Hamano
2023-07-28 15:37 ` Oswald Buddenhagen
@ 2023-07-28 17:36 ` Linus Arver
1 sibling, 0 replies; 53+ messages in thread
From: Linus Arver @ 2023-07-28 17:36 UTC (permalink / raw)
To: Junio C Hamano, Oswald Buddenhagen
Cc: Phillip Wood, git, Kristoffer Haugsbakk
Junio C Hamano <gitster@pobox.com> writes:
> The usefulness of the message comes from its information content.
> What do we want to read out of these messages? I think we want
> a title that immediately lets us know three things:
>
> (1) What the original patch was about.
> (2) What the final state is.
> (3) How involved was the road to get to the final state has been.
>
> As to (1), we are not proposing to lose what comes "Revert", so this
> information is not lost under any proposal we have seen so far in
> the discussion.
Agreed.
> As to (2), with the current "Revert" -> "Revert Revert" -> "Revert
> Revert Revert" -> ..., you have to count, which is cumbersome and
> does not give you an immediate access to that information. With
> "Revert^n", you'd see if n is even or odd to determine, which is
> much better than the status quo
I actually think "Revert^n" is much worse than what we have, mainly
because the "^n" syntax is already used in other subcommands (e.g.,
rebase). It may well be that we already have overloaded syntax
elsewhere, but we should avoid overloading where possible.
> , but it takes practice to interpret.
> With "Revert" -> "Reapply" -> "Revert Reapply" -> "Reapply Reapply"
> -> ..., the first word would give you the final state immediately.
I agree. But to take the idea further, maybe "Reapply Reapply" should be
shortened to just
Reapply* "original title"
and likewise any ultimate _removal_ of the commit (no matter the exact
pairwise count of the word "Revert") should be shortened to
Revert* "original title"
? The trailing asterisk in each case would indicate that this
reapplication was not the first reapplication. We could then put
in the commit message body text something like
--8<---------------cut here--------------------------------->8---
*NOTE: This is not the first time we've reverted XXX.
--8<---------------cut here----------------^---------^------>8---
| |
| Commit SHA.
|
Could be "reapplied" if
"Reapply*" is the title
prefix.
to help stress point (3) that you described in your list above.
> We want to know (3), because between a change whose revert was
> reverted and a change that hasn't been involved in any revert, there
> may be no difference in the end result, the former is likely to be
> trickier and merits more careful inspection than the latter.
Agreed.
> With
> "Revert^n", we read how large the number n is to find the
> information. With the current "the Revert repeated number of times"
> or your "a pair of frontmost Reverts become one Reapply", the length
> of the Revert/Reapply prefix conveys this information, but this is
> associated with the cost of pushing the original title further to
> the right and hard to read/find.
I agree that trying to keep the title short is very important. I didn't
think too deeply about this point previously, but I think it is equally
important as the other points you've enumerated above.
> Note that, while the number of
> times revert-reapply sequence took place is a useful piece of
> information, the exact number may not be all that important.
Agreed. For this reason I would prefer just using a single asterisk (as
in my example) after the first occurrence of the revert/reapplication.
> And from the above discussion, I wonder if the following would be a
> good place to stop:
>
> - The first revert is as before: Revert "original title"
> - A revert of a revert becomes: Reapply "original title"
> - A revert of a reapply becomes: Revert Reapply "original title"
> - A revert of "Revert Reapply" becomes: Reapply Reapply "original title"
> - A revert of "Reapply Reapply" becomes: Revert Reapply "original title"
>
> In other words, we accept the fact that wedo not need exact number
> of times reversions were done, and use that to simplify the output
> to make sure we will not spend more than two words in the front of
> the title. That would help to keep the original title visible,
> while still allowing us to distinguish the ones that was reverted up
> to four times (and "Revert Reapply" and "Reapply Reapply" only tell
> us "final state is to (discard|accept) the original but it took us
> _many_ times", without saying exactly how many).
I very much like your idea of just "stopping the recursion" early on.
The only difference is that I would prefer to drop the second "Reapply"
word in your examples and replace them with an asterisk (this makes them
the same as in my example).
I think having this second word "Reapply" would make the title
potentially hurt readability, especially because the first word is
really the only one users should be looking at anyway to figure out the
final state. Another reason for dropping it is because a well-written
commit message title should have a single verb near the beginning, but
your style would break this convention.
If you don't like the "asterisk" idea, another form might be
- A revert of a reapply becomes: Revert reapplication of "original title"
- A revert of the above becomes: Reapply revert of "original title"
- A revert of the above becomes: Revert reapplication of "original title"
- A revert of the above becomes: Reapply revert of "original title"
and so forth.
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH v3 1/2] sequencer: beautify subject of reverts of reverts
2023-04-28 8:35 [PATCH v2] sequencer: beautify subject of reverts of reverts Oswald Buddenhagen
2023-04-28 18:35 ` Junio C Hamano
2023-05-17 9:05 ` Phillip Wood
@ 2023-08-09 17:15 ` Oswald Buddenhagen
2023-08-09 17:15 ` [PATCH v3 2/2] doc: revert: add discussion Oswald Buddenhagen
` (2 more replies)
2 siblings, 3 replies; 53+ messages in thread
From: Oswald Buddenhagen @ 2023-08-09 17:15 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Kristoffer Haugsbakk, Phillip Wood
Instead of generating a silly-looking `Revert "Revert "foo""`, make it
a more humane `Reapply "foo"`.
This is done for two reasons:
- To cover the actually common case of just a double revert.
- To encourage people to rewrite summaries of recursive reverts by
setting an example (a subsequent commit will also do this explicitly
in the documentation).
To achieve these goals, the mechanism does not need to be particularly
sophisticated. Therefore, more complicated alternatives which would
"compress more efficiently" have not been implemented.
Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
v3:
- capitulate at first sight of a pre-existing recursive reversion, as
handling the edge cases is a bottomless pit
- reworked commit message again
- moved test into existing file
- generalized docu change and factored it out
v2:
- add discussion to commit message
- add paragraph to docu
- add test
- use skip_prefix() instead of starts_with()
- catch pre-existing double reverts
Cc: Junio C Hamano <gitster@pobox.com>
Cc: Kristoffer Haugsbakk <code@khaugsbakk.name>
Cc: Phillip Wood <phillip.wood123@gmail.com>
---
sequencer.c | 11 +++++++++++
t/t3501-revert-cherry-pick.sh | 25 +++++++++++++++++++++++++
2 files changed, 36 insertions(+)
diff --git a/sequencer.c b/sequencer.c
index cc9821ece2..12ec158922 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2249,13 +2249,24 @@ static int do_pick_commit(struct repository *r,
*/
if (command == TODO_REVERT) {
+ const char *orig_subject;
+
base = commit;
base_label = msg.label;
next = parent;
next_label = msg.parent_label;
if (opts->commit_use_reference) {
strbuf_addstr(&msgbuf,
"# *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***");
+ } else if (skip_prefix(msg.subject, "Revert \"", &orig_subject) &&
+ /*
+ * We don't touch pre-existing repeated reverts, because
+ * theoretically these can be nested arbitrarily deeply,
+ * thus requiring excessive complexity to deal with.
+ */
+ !starts_with(orig_subject, "Revert \"")) {
+ strbuf_addstr(&msgbuf, "Reapply \"");
+ strbuf_addstr(&msgbuf, orig_subject);
} else {
strbuf_addstr(&msgbuf, "Revert \"");
strbuf_addstr(&msgbuf, msg.subject);
diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
index e2ef619323..7011e3a421 100755
--- a/t/t3501-revert-cherry-pick.sh
+++ b/t/t3501-revert-cherry-pick.sh
@@ -176,6 +176,31 @@ test_expect_success 'advice from failed revert' '
test_cmp expected actual
'
+test_expect_success 'title of fresh reverts' '
+ test_commit --no-tag A file1 &&
+ test_commit --no-tag B file1 &&
+ git revert --no-edit HEAD &&
+ echo "Revert \"B\"" >expect &&
+ git log -1 --pretty=%s >actual &&
+ test_cmp expect actual &&
+ git revert --no-edit HEAD &&
+ echo "Reapply \"B\"" >expect &&
+ git log -1 --pretty=%s >actual &&
+ test_cmp expect actual &&
+ git revert --no-edit HEAD &&
+ echo "Revert \"Reapply \"B\"\"" >expect &&
+ git log -1 --pretty=%s >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'title of legacy double revert' '
+ test_commit --no-tag "Revert \"Revert \"B\"\"" file1 &&
+ git revert --no-edit HEAD &&
+ echo "Revert \"Revert \"Revert \"B\"\"\"" >expect &&
+ git log -1 --pretty=%s >actual &&
+ test_cmp expect actual
+'
+
test_expect_success 'identification of reverted commit (default)' '
test_commit to-ident &&
test_when_finished "git reset --hard to-ident" &&
--
2.40.0.152.g15d061e6df
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [PATCH v3 2/2] doc: revert: add discussion
2023-08-09 17:15 ` [PATCH v3 1/2] " Oswald Buddenhagen
@ 2023-08-09 17:15 ` Oswald Buddenhagen
2023-08-10 21:50 ` Linus Arver
2023-08-11 15:05 ` [PATCH v3 1/2] sequencer: beautify subject of reverts of reverts Phillip Wood
2023-08-21 17:07 ` [PATCH v4 " Oswald Buddenhagen
2 siblings, 1 reply; 53+ messages in thread
From: Oswald Buddenhagen @ 2023-08-09 17:15 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
The section is inspired by git-commit.txt.
Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
Cc: Junio C Hamano <gitster@pobox.com>
---
while thinking about what to write, i came up with an idea for another
improvement: with (implicit) --edit, the template message would end up
being:
This reverts commit <sha1>,
because <PUT REASON HERE>.
---
Documentation/git-revert.txt | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt
index d2e10d3dce..2b52dc89a8 100644
--- a/Documentation/git-revert.txt
+++ b/Documentation/git-revert.txt
@@ -142,6 +142,16 @@ EXAMPLES
changes. The revert only modifies the working tree and the
index.
+DISCUSSION
+----------
+
+While git creates a basic commit message automatically, you really
+should not leave it at that. In particular, it is _strongly_
+recommended to explain why the original commit is being reverted.
+Repeatedly reverting reversions yields increasingly unwieldy
+commit subjects; latest when you arrive at 'Reapply "Reapply
+"<original subject>""' you should get creative.
+
CONFIGURATION
-------------
--
2.40.0.152.g15d061e6df
^ permalink raw reply related [flat|nested] 53+ messages in thread
* Re: [PATCH v3 2/2] doc: revert: add discussion
2023-08-09 17:15 ` [PATCH v3 2/2] doc: revert: add discussion Oswald Buddenhagen
@ 2023-08-10 21:50 ` Linus Arver
2023-08-10 22:00 ` Linus Arver
` (3 more replies)
0 siblings, 4 replies; 53+ messages in thread
From: Linus Arver @ 2023-08-10 21:50 UTC (permalink / raw)
To: Oswald Buddenhagen, git; +Cc: Junio C Hamano
Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
> while thinking about what to write, i came up with an idea for another
> improvement: with (implicit) --edit, the template message would end up
> being:
>
> This reverts commit <sha1>,
> because <PUT REASON HERE>.
This sounds great to me.
Nit: the "doc: revert: add discussion" subject line should probably be more
like "revert doc: suggest adding the 'why' behind reverts".
> ---
> Documentation/git-revert.txt | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt
> index d2e10d3dce..2b52dc89a8 100644
> --- a/Documentation/git-revert.txt
> +++ b/Documentation/git-revert.txt
> @@ -142,6 +142,16 @@ EXAMPLES
> changes. The revert only modifies the working tree and the
> index.
>
> +DISCUSSION
> +----------
> +
> +While git creates a basic commit message automatically, you really
> +should not leave it at that. In particular, it is _strongly_
> +recommended to explain why the original commit is being reverted.
> +Repeatedly reverting reversions yields increasingly unwieldy
> +commit subjects; latest when you arrive at 'Reapply "Reapply
> +"<original subject>""' you should get creative.
The word "latest" here sounds odd. Ditto for "get creative". How about
the following rewording?
While git creates a basic commit message automatically, it is
_strongly_ recommended to explain why the original commit is being
reverted. In addition, repeatedly reverting the same commit will
result in increasingly unwieldy subject lines, for example 'Reapply
"Reapply "<original subject>""'. Please consider rewording such
subject lines to reflect the reason why the original commit is being
reapplied again.
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v3 2/2] doc: revert: add discussion
2023-08-10 21:50 ` Linus Arver
@ 2023-08-10 22:00 ` Linus Arver
2023-08-11 15:10 ` Phillip Wood
2023-08-11 12:49 ` Oswald Buddenhagen
` (2 subsequent siblings)
3 siblings, 1 reply; 53+ messages in thread
From: Linus Arver @ 2023-08-10 22:00 UTC (permalink / raw)
To: Oswald Buddenhagen, git; +Cc: Junio C Hamano
Linus Arver <linusa@google.com> writes:
> How about
> the following rewording?
>
> While git creates a basic commit message automatically, it is
> _strongly_ recommended to explain why the original commit is being
> reverted. In addition, repeatedly reverting the same commit will
Hmph, "repeatedly reverting the same commit" sounds wrong because
strictly speaking there is only 1 "same commit" (the original commit).
Perhaps
In addition, repeatedly reverting the same progression of reverts will
or even
In addition, repeatedly reverting the same revert chain will
is better here?
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v3 2/2] doc: revert: add discussion
2023-08-10 21:50 ` Linus Arver
2023-08-10 22:00 ` Linus Arver
@ 2023-08-11 12:49 ` Oswald Buddenhagen
2023-08-11 23:00 ` Linus Arver
2023-08-11 15:08 ` Phillip Wood
2023-08-11 17:05 ` Junio C Hamano
3 siblings, 1 reply; 53+ messages in thread
From: Oswald Buddenhagen @ 2023-08-11 12:49 UTC (permalink / raw)
To: Linus Arver; +Cc: git, Junio C Hamano
On Thu, Aug 10, 2023 at 02:50:59PM -0700, Linus Arver wrote:
>Nit: the "doc: revert: add discussion" subject line should probably be more
>like "revert doc: suggest adding the 'why' behind reverts".
>
this is counter to the prevalent "big endian" prefix style, and is in
this case really easy to misread.
i also intentionally kept the subject generic, because the content
covers two matters (the reasoning and the subjects, which is also the
reason why this is a separate patch to start with).
>Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
>> +DISCUSSION
>> +----------
>> +
>> +While git creates a basic commit message automatically, you really
>> +should not leave it at that. In particular, it is _strongly_
>> +recommended to explain why the original commit is being reverted.
>> +Repeatedly reverting reversions yields increasingly unwieldy
>> +commit subjects; latest when you arrive at 'Reapply "Reapply
>> +"<original subject>""' you should get creative.
>
>The word "latest" here sounds odd. Ditto for "get creative".
>
yeah, i suppose. i wasn't sure how formal i should make it - things
aren't consistent to start with.
> How about the following rewording?
>
> While git creates a basic commit message automatically, it is
> _strongly_ recommended to explain why the original commit is being
> reverted. In addition, repeatedly reverting the same commit will
> result in increasingly unwieldy subject lines,
>for example 'Reapply "Reapply "<original subject>""'.
>
you turned it from a suggested threshold into an example. at this point
it appears superfluous to me.
>Please consider rewording such
> subject lines to reflect the reason why the original commit is being
> reapplied again.
>
the reasoning most likely wouldn't fit into the subject.
also, the original request to explain the reasoning applies
transitively, so i don't think it's really necessary to point it out
explicitly.
On Thu, Aug 10, 2023 at 03:00:41PM -0700, Linus Arver wrote:
>Hmph, "repeatedly reverting the same commit" sounds wrong because
>strictly speaking there is only 1 "same commit" (the original commit).
>Perhaps
>
> In addition, repeatedly reverting the same progression of reverts will
>
>or even
>
> In addition, repeatedly reverting the same revert chain will
>
>is better here?
>
we used "recursive reverts" elsewhere. but i'm not sure whether that's
sufficiently intuitive and formally correct.
anyway, what's wrong with my original proposal?
so in summary, how about:
While git creates a basic commit message automatically, it is
_strongly_ recommended to explain why the original commit is being
reverted. In addition, repeatedly reverting reversions will
result in increasingly unwieldy subject lines. Please consider
rewording these into something shorter and more unique.
regards
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v3 1/2] sequencer: beautify subject of reverts of reverts
2023-08-09 17:15 ` [PATCH v3 1/2] " Oswald Buddenhagen
2023-08-09 17:15 ` [PATCH v3 2/2] doc: revert: add discussion Oswald Buddenhagen
@ 2023-08-11 15:05 ` Phillip Wood
2023-08-11 16:59 ` Junio C Hamano
2023-08-21 17:07 ` [PATCH v4 " Oswald Buddenhagen
2 siblings, 1 reply; 53+ messages in thread
From: Phillip Wood @ 2023-08-11 15:05 UTC (permalink / raw)
To: Oswald Buddenhagen, git; +Cc: Junio C Hamano, Kristoffer Haugsbakk
Hi Oswald
On 09/08/2023 18:15, Oswald Buddenhagen wrote:
> Instead of generating a silly-looking `Revert "Revert "foo""`, make it
> a more humane `Reapply "foo"`.
>
> This is done for two reasons:
> - To cover the actually common case of just a double revert.
> - To encourage people to rewrite summaries of recursive reverts by
> setting an example (a subsequent commit will also do this explicitly
> in the documentation).
>
> To achieve these goals, the mechanism does not need to be particularly
> sophisticated. Therefore, more complicated alternatives which would
> "compress more efficiently" have not been implemented.
This all looks good to me, it seems quite sensible just to bail out if
we see an existing recursive reversion. I'm not suggesting you change
these tests but for future reference we now have a test_commit_message()
function which was merged a few days ago to simplify tests like these.
Thanks for working on it
Phillip
> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
>
> ---
> v3:
> - capitulate at first sight of a pre-existing recursive reversion, as
> handling the edge cases is a bottomless pit
> - reworked commit message again
> - moved test into existing file
> - generalized docu change and factored it out
>
> v2:
> - add discussion to commit message
> - add paragraph to docu
> - add test
> - use skip_prefix() instead of starts_with()
> - catch pre-existing double reverts
>
> Cc: Junio C Hamano <gitster@pobox.com>
> Cc: Kristoffer Haugsbakk <code@khaugsbakk.name>
> Cc: Phillip Wood <phillip.wood123@gmail.com>
> ---
> sequencer.c | 11 +++++++++++
> t/t3501-revert-cherry-pick.sh | 25 +++++++++++++++++++++++++
> 2 files changed, 36 insertions(+)
>
> diff --git a/sequencer.c b/sequencer.c
> index cc9821ece2..12ec158922 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2249,13 +2249,24 @@ static int do_pick_commit(struct repository *r,
> */
>
> if (command == TODO_REVERT) {
> + const char *orig_subject;
> +
> base = commit;
> base_label = msg.label;
> next = parent;
> next_label = msg.parent_label;
> if (opts->commit_use_reference) {
> strbuf_addstr(&msgbuf,
> "# *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***");
> + } else if (skip_prefix(msg.subject, "Revert \"", &orig_subject) &&
> + /*
> + * We don't touch pre-existing repeated reverts, because
> + * theoretically these can be nested arbitrarily deeply,
> + * thus requiring excessive complexity to deal with.
> + */
> + !starts_with(orig_subject, "Revert \"")) {
> + strbuf_addstr(&msgbuf, "Reapply \"");
> + strbuf_addstr(&msgbuf, orig_subject);
> } else {
> strbuf_addstr(&msgbuf, "Revert \"");
> strbuf_addstr(&msgbuf, msg.subject);
> diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
> index e2ef619323..7011e3a421 100755
> --- a/t/t3501-revert-cherry-pick.sh
> +++ b/t/t3501-revert-cherry-pick.sh
> @@ -176,6 +176,31 @@ test_expect_success 'advice from failed revert' '
> test_cmp expected actual
> '
>
> +test_expect_success 'title of fresh reverts' '
> + test_commit --no-tag A file1 &&
> + test_commit --no-tag B file1 &&
> + git revert --no-edit HEAD &&
> + echo "Revert \"B\"" >expect &&
> + git log -1 --pretty=%s >actual &&
> + test_cmp expect actual &&
> + git revert --no-edit HEAD &&
> + echo "Reapply \"B\"" >expect &&
> + git log -1 --pretty=%s >actual &&
> + test_cmp expect actual &&
> + git revert --no-edit HEAD &&
> + echo "Revert \"Reapply \"B\"\"" >expect &&
> + git log -1 --pretty=%s >actual &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'title of legacy double revert' '
> + test_commit --no-tag "Revert \"Revert \"B\"\"" file1 &&
> + git revert --no-edit HEAD &&
> + echo "Revert \"Revert \"Revert \"B\"\"\"" >expect &&
> + git log -1 --pretty=%s >actual &&
> + test_cmp expect actual
> +'
> +
> test_expect_success 'identification of reverted commit (default)' '
> test_commit to-ident &&
> test_when_finished "git reset --hard to-ident" &&
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v3 2/2] doc: revert: add discussion
2023-08-10 21:50 ` Linus Arver
2023-08-10 22:00 ` Linus Arver
2023-08-11 12:49 ` Oswald Buddenhagen
@ 2023-08-11 15:08 ` Phillip Wood
2023-08-11 17:10 ` Junio C Hamano
2023-08-11 17:05 ` Junio C Hamano
3 siblings, 1 reply; 53+ messages in thread
From: Phillip Wood @ 2023-08-11 15:08 UTC (permalink / raw)
To: Linus Arver, Oswald Buddenhagen, git; +Cc: Junio C Hamano
On 10/08/2023 22:50, Linus Arver wrote:
> Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
>> +DISCUSSION
>> +----------
>> +
>> +While git creates a basic commit message automatically, you really
>> +should not leave it at that. In particular, it is _strongly_
>> +recommended to explain why the original commit is being reverted.
>> +Repeatedly reverting reversions yields increasingly unwieldy
>> +commit subjects; latest when you arrive at 'Reapply "Reapply
>> +"<original subject>""' you should get creative.
>
> The word "latest" here sounds odd. Ditto for "get creative". How about
> the following rewording?
>
> While git creates a basic commit message automatically, it is
> _strongly_ recommended to explain why the original commit is being
> reverted. In addition, repeatedly reverting the same commit will
> result in increasingly unwieldy subject lines, for example 'Reapply
> "Reapply "<original subject>""'. Please consider rewording such
> subject lines to reflect the reason why the original commit is being
> reapplied again.
That's a good suggestion, I think having the example will help readers
understand the issue being described.
Best Wishes
Phillip
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v3 2/2] doc: revert: add discussion
2023-08-10 22:00 ` Linus Arver
@ 2023-08-11 15:10 ` Phillip Wood
2023-08-12 6:25 ` Oswald Buddenhagen
0 siblings, 1 reply; 53+ messages in thread
From: Phillip Wood @ 2023-08-11 15:10 UTC (permalink / raw)
To: Linus Arver, Oswald Buddenhagen, git; +Cc: Junio C Hamano
On 10/08/2023 23:00, Linus Arver wrote:
> Linus Arver <linusa@google.com> writes:
>
>> How about
>> the following rewording?
>>
>> While git creates a basic commit message automatically, it is
>> _strongly_ recommended to explain why the original commit is being
>> reverted. In addition, repeatedly reverting the same commit will
>
> Hmph, "repeatedly reverting the same commit" sounds wrong because
> strictly speaking there is only 1 "same commit" (the original commit).
While it isn't strictly accurate I think that wording is easy enough to
understand. I think it is hard to find a more accurate wording that
isn't too verbose or cumbersome.
Best Wishes
Phillip
> Perhaps
>
> In addition, repeatedly reverting the same progression of reverts will
>
> or even
>
> In addition, repeatedly reverting the same revert chain will
>
> is better here?
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v3 1/2] sequencer: beautify subject of reverts of reverts
2023-08-11 15:05 ` [PATCH v3 1/2] sequencer: beautify subject of reverts of reverts Phillip Wood
@ 2023-08-11 16:59 ` Junio C Hamano
2023-08-11 17:13 ` Oswald Buddenhagen
0 siblings, 1 reply; 53+ messages in thread
From: Junio C Hamano @ 2023-08-11 16:59 UTC (permalink / raw)
To: Phillip Wood; +Cc: Oswald Buddenhagen, git, Kristoffer Haugsbakk
Phillip Wood <phillip.wood123@gmail.com> writes:
> Hi Oswald
>
> On 09/08/2023 18:15, Oswald Buddenhagen wrote:
>> Instead of generating a silly-looking `Revert "Revert "foo""`, make it
>> a more humane `Reapply "foo"`.
>> This is done for two reasons:
>> - To cover the actually common case of just a double revert.
>> - To encourage people to rewrite summaries of recursive reverts by
>> setting an example (a subsequent commit will also do this explicitly
>> in the documentation).
>> To achieve these goals, the mechanism does not need to be
>> particularly
>> sophisticated. Therefore, more complicated alternatives which would
>> "compress more efficiently" have not been implemented.
>
> This all looks good to me, it seems quite sensible just to bail out if
> we see an existing recursive reversion.
Yes, explicitly refraining from becoming overly cute is a good
design decision.
>> diff --git a/sequencer.c b/sequencer.c
>> index cc9821ece2..12ec158922 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -2249,13 +2249,24 @@ static int do_pick_commit(struct repository *r,
>> */
>> if (command == TODO_REVERT) {
>> + const char *orig_subject;
>> +
>> base = commit;
>> base_label = msg.label;
>> next = parent;
>> next_label = msg.parent_label;
>> if (opts->commit_use_reference) {
>> strbuf_addstr(&msgbuf,
>> "# *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***");
>> + } else if (skip_prefix(msg.subject, "Revert \"", &orig_subject) &&
>> + /*
>> + * We don't touch pre-existing repeated reverts, because
>> + * theoretically these can be nested arbitrarily deeply,
>> + * thus requiring excessive complexity to deal with.
>> + */
>> + !starts_with(orig_subject, "Revert \"")) {
>> + strbuf_addstr(&msgbuf, "Reapply \"");
>> + strbuf_addstr(&msgbuf, orig_subject);
Being simple-and-stupid to deal only with the most common case, and
documenting that it is deliberate that we do not deal with more
complex cases in the in-code comment and in the log message, are
very good in this case.
>> diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
>> index e2ef619323..7011e3a421 100755
>> --- a/t/t3501-revert-cherry-pick.sh
>> +++ b/t/t3501-revert-cherry-pick.sh
>> @@ -176,6 +176,31 @@ test_expect_success 'advice from failed revert' '
>> test_cmp expected actual
>> '
>> +test_expect_success 'title of fresh reverts' '
>> + test_commit --no-tag A file1 &&
>> + test_commit --no-tag B file1 &&
>> + git revert --no-edit HEAD &&
>> + echo "Revert \"B\"" >expect &&
>> + git log -1 --pretty=%s >actual &&
>> + test_cmp expect actual &&
>> + git revert --no-edit HEAD &&
>> + echo "Reapply \"B\"" >expect &&
>> + git log -1 --pretty=%s >actual &&
>> + test_cmp expect actual &&
>> + git revert --no-edit HEAD &&
>> + echo "Revert \"Reapply \"B\"\"" >expect &&
>> + git log -1 --pretty=%s >actual &&
>> + test_cmp expect actual
>> +'
Presumably the next time this gets reverted we will see a doubled
reapply? Isn't that something we care about documenting as a part
of this test? i.e. another four-line block after the above?
git revert --no-edit HEAD &&
echo "Reapply \"Reapply \"B\"\"" >expect &&
git log -1 --pretty=%s >actual &&
test_cmp expect actual
>> +test_expect_success 'title of legacy double revert' '
>> + test_commit --no-tag "Revert \"Revert \"B\"\"" file1 &&
>> + git revert --no-edit HEAD &&
>> + echo "Revert \"Revert \"Revert \"B\"\"\"" >expect &&
>> + git log -1 --pretty=%s >actual &&
>> + test_cmp expect actual
>> +'
Good.
>> test_expect_success 'identification of reverted commit (default)' '
>> test_commit to-ident &&
>> test_when_finished "git reset --hard to-ident" &&
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v3 2/2] doc: revert: add discussion
2023-08-10 21:50 ` Linus Arver
` (2 preceding siblings ...)
2023-08-11 15:08 ` Phillip Wood
@ 2023-08-11 17:05 ` Junio C Hamano
2023-08-11 17:44 ` Re* " Junio C Hamano
3 siblings, 1 reply; 53+ messages in thread
From: Junio C Hamano @ 2023-08-11 17:05 UTC (permalink / raw)
To: Linus Arver; +Cc: Oswald Buddenhagen, git
Linus Arver <linusa@google.com> writes:
> Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
>
>> while thinking about what to write, i came up with an idea for another
>> improvement: with (implicit) --edit, the template message would end up
>> being:
>>
>> This reverts commit <sha1>,
>> because <PUT REASON HERE>.
>
> This sounds great to me.
Oh, absolutely. I rarely do a revert myself (other than reverting a
premature merge out of 'next'), but giving a better instruction in
the commit log editor buffer as template is a very good idea.
> Nit: the "doc: revert: add discussion" subject line should probably be more
> like "revert doc: suggest adding the 'why' behind reverts".
Good suggestion.
> The word "latest" here sounds odd. Ditto for "get creative". How about
> the following rewording?
>
> While git creates a basic commit message automatically, it is
> _strongly_ recommended to explain why the original commit is being
> reverted. In addition, repeatedly reverting the same commit will
> result in increasingly unwieldy subject lines, for example 'Reapply
> "Reapply "<original subject>""'. Please consider rewording such
> subject lines to reflect the reason why the original commit is being
> reapplied again.
Sounds better, but let me read the remaining discussion first ;-)
Thanks.
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v3 2/2] doc: revert: add discussion
2023-08-11 15:08 ` Phillip Wood
@ 2023-08-11 17:10 ` Junio C Hamano
0 siblings, 0 replies; 53+ messages in thread
From: Junio C Hamano @ 2023-08-11 17:10 UTC (permalink / raw)
To: Phillip Wood; +Cc: Linus Arver, Oswald Buddenhagen, git
Phillip Wood <phillip.wood123@gmail.com> writes:
> On 10/08/2023 22:50, Linus Arver wrote:
>> Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
>>> +DISCUSSION
>>> +----------
>>> +
>>> +While git creates a basic commit message automatically, you really
>>> +should not leave it at that. In particular, it is _strongly_
>>> +recommended to explain why the original commit is being reverted.
>>> +Repeatedly reverting reversions yields increasingly unwieldy
>>> +commit subjects; latest when you arrive at 'Reapply "Reapply
>>> +"<original subject>""' you should get creative.
>> The word "latest" here sounds odd. Ditto for "get creative". How
>> about
>> the following rewording?
>> While git creates a basic commit message automatically, it is
>> _strongly_ recommended to explain why the original commit is being
>> reverted. In addition, repeatedly reverting the same commit will
>> result in increasingly unwieldy subject lines, for example 'Reapply
>> "Reapply "<original subject>""'. Please consider rewording such
>> subject lines to reflect the reason why the original commit is being
>> reapplied again.
>
> That's a good suggestion, I think having the example will help readers
> understand the issue being described.
Sounds very good.
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v3 1/2] sequencer: beautify subject of reverts of reverts
2023-08-11 16:59 ` Junio C Hamano
@ 2023-08-11 17:13 ` Oswald Buddenhagen
0 siblings, 0 replies; 53+ messages in thread
From: Oswald Buddenhagen @ 2023-08-11 17:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Phillip Wood, git, Kristoffer Haugsbakk
On Fri, Aug 11, 2023 at 09:59:34AM -0700, Junio C Hamano wrote:
>Presumably the next time this gets reverted we will see a doubled
>reapply?
>
yes
>Isn't that something we care about documenting as a part
>of this test? i.e. another four-line block after the above?
>
the third case documents that it's the same as the first case, i.e.,
"nothing special". so at this point we have full coverage in all
regards. going beyond that would be redundant, and we'd again get into
the "uh, where do we stop?" situation.
regards
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re* [PATCH v3 2/2] doc: revert: add discussion
2023-08-11 17:05 ` Junio C Hamano
@ 2023-08-11 17:44 ` Junio C Hamano
2023-08-11 17:53 ` Junio C Hamano
` (2 more replies)
0 siblings, 3 replies; 53+ messages in thread
From: Junio C Hamano @ 2023-08-11 17:44 UTC (permalink / raw)
To: Linus Arver; +Cc: Oswald Buddenhagen, git
Junio C Hamano <gitster@pobox.com> writes:
> Linus Arver <linusa@google.com> writes:
>
>> Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
>>
>>> while thinking about what to write, i came up with an idea for another
>>> improvement: with (implicit) --edit, the template message would end up
>>> being:
>>>
>>> This reverts commit <sha1>,
>>> because <PUT REASON HERE>.
>>
>> This sounds great to me.
>
> Oh, absolutely. I rarely do a revert myself (other than reverting a
> premature merge out of 'next'), but giving a better instruction in
> the commit log editor buffer as template is a very good idea.
It might be just the matter of doing something like the attached
patch on top of Oswald's, reusing the existing code to instruct the
user to describe the reversion.
------- >8 ------------- >8 ------------- >8 ------------- >8 -------
Subject: [PATCH 3/2] revert: force explaining overly complex revert chain
Once we revert reverts of revert and reach "Reapply "Reapply "..."",
it becomes too unweirdly to read a reversion of such a comit.
We instruct the user to explain why the reversion is done in their
own words when using the revert.reference mode, and the instruction
applies equally for such an overly complex revert chain. The
rationale for such a sequence of events should be recorded to help
future developers.
Building on top of the recent Oswald's work to turn "revert revert"
into "reapply", let's turn the reference mode automatically on in
such a case.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* I left the reference to the second parent to honor the command
line option and configuration variable even when this new
mechanism kicks in and this is very much deliberate. As a commit
that is a revert (or reapply) should be single parent (because a
revert of a merge is a single parent commit), the choice does not
make any difference in practice.
sequencer.c | 16 +++++++++++-----
t/t3501-revert-cherry-pick.sh | 11 ++++++++++-
2 files changed, 21 insertions(+), 6 deletions(-)
diff --git c/sequencer.c w/sequencer.c
index 7dc13fdcca..43bb558518 100644
--- c/sequencer.c
+++ w/sequencer.c
@@ -2130,10 +2130,10 @@ static int should_edit(struct replay_opts *opts) {
return opts->edit;
}
-static void refer_to_commit(struct replay_opts *opts,
+static void refer_to_commit(int use_reference,
struct strbuf *msgbuf, struct commit *commit)
{
- if (opts->commit_use_reference) {
+ if (use_reference) {
struct pretty_print_context ctx = {
.abbrev = DEFAULT_ABBREV,
.date_mode.type = DATE_SHORT,
@@ -2250,12 +2250,18 @@ static int do_pick_commit(struct repository *r,
if (command == TODO_REVERT) {
const char *orig_subject;
+ int use_reference = opts->commit_use_reference;
base = commit;
base_label = msg.label;
next = parent;
next_label = msg.parent_label;
- if (opts->commit_use_reference) {
+
+ if (starts_with(msg.subject, "Reapply \"Reapply \""))
+ /* fifth time is too many - force reference format*/
+ use_reference = 1;
+
+ if (use_reference) {
strbuf_addstr(&msgbuf,
"# *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***");
} else if (skip_prefix(msg.subject, "Revert \"", &orig_subject) &&
@@ -2273,11 +2279,11 @@ static int do_pick_commit(struct repository *r,
strbuf_addstr(&msgbuf, "\"");
}
strbuf_addstr(&msgbuf, "\n\nThis reverts commit ");
- refer_to_commit(opts, &msgbuf, commit);
+ refer_to_commit(use_reference, &msgbuf, commit);
if (commit->parents && commit->parents->next) {
strbuf_addstr(&msgbuf, ", reversing\nchanges made to ");
- refer_to_commit(opts, &msgbuf, parent);
+ refer_to_commit(opts->commit_use_reference, &msgbuf, parent);
}
strbuf_addstr(&msgbuf, ".\n");
} else {
diff --git c/t/t3501-revert-cherry-pick.sh w/t/t3501-revert-cherry-pick.sh
index 7011e3a421..7a8715d3f4 100755
--- c/t/t3501-revert-cherry-pick.sh
+++ w/t/t3501-revert-cherry-pick.sh
@@ -190,7 +190,16 @@ test_expect_success 'title of fresh reverts' '
git revert --no-edit HEAD &&
echo "Revert \"Reapply \"B\"\"" >expect &&
git log -1 --pretty=%s >actual &&
- test_cmp expect actual
+ test_cmp expect actual &&
+ git revert --no-edit HEAD &&
+ echo "Reapply \"Reapply \"B\"\"" >expect &&
+ git log -1 --pretty=%s >actual &&
+ test_cmp expect actual &&
+
+ # Give the stronger instruction for unusually complex case
+ git revert --no-edit HEAD &&
+ git log -1 --pretty=%s >actual &&
+ grep -F -e "# *** SAY WHY WE ARE REVERTING" actual
'
test_expect_success 'title of legacy double revert' '
^ permalink raw reply related [flat|nested] 53+ messages in thread
* Re: Re* [PATCH v3 2/2] doc: revert: add discussion
2023-08-11 17:44 ` Re* " Junio C Hamano
@ 2023-08-11 17:53 ` Junio C Hamano
2023-08-11 17:56 ` rsbecker
2023-08-11 18:16 ` Eric Sunshine
2023-08-11 18:16 ` Oswald Buddenhagen
2 siblings, 1 reply; 53+ messages in thread
From: Junio C Hamano @ 2023-08-11 17:53 UTC (permalink / raw)
To: Linus Arver; +Cc: Oswald Buddenhagen, git
Junio C Hamano <gitster@pobox.com> writes:
> +
> + if (starts_with(msg.subject, "Reapply \"Reapply \""))
> + /* fifth time is too many - force reference format*/
> + use_reference = 1;
Come to think of it, as the documentation patch in the series cited
double reapply as too unwieldy, we probably should stop before
producing such commit. We can update "Reapply \"Reapply" above to
"Revert \"Reapply" and then "fifth" -> "fourth". The test update
below must also be adjusted, if we want to take that route.
^ permalink raw reply [flat|nested] 53+ messages in thread
* RE: Re* [PATCH v3 2/2] doc: revert: add discussion
2023-08-11 17:53 ` Junio C Hamano
@ 2023-08-11 17:56 ` rsbecker
0 siblings, 0 replies; 53+ messages in thread
From: rsbecker @ 2023-08-11 17:56 UTC (permalink / raw)
To: 'Junio C Hamano', 'Linus Arver'
Cc: 'Oswald Buddenhagen', git
On Friday, August 11, 2023 1:54 PM, Junio C Hamano wrote:
>Junio C Hamano <gitster@pobox.com> writes:
>
>> +
>> + if (starts_with(msg.subject, "Reapply \"Reapply \""))
>> + /* fifth time is too many - force reference format*/
>> + use_reference = 1;
>
>Come to think of it, as the documentation patch in the series cited double
reapply as
>too unwieldy, we probably should stop before producing such commit. We can
>update "Reapply \"Reapply" above to "Revert \"Reapply" and then "fifth" ->
"fourth".
>The test update below must also be adjusted, if we want to take that route.
May I suggest a potential quick solution. Perhaps we could leave this up to
users by putting in an --amend or --reword option to cause a prompt for a
comment for the reverted commit instead of trying to come up with a
one-size-fits-all solution.
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: Re* [PATCH v3 2/2] doc: revert: add discussion
2023-08-11 17:44 ` Re* " Junio C Hamano
2023-08-11 17:53 ` Junio C Hamano
@ 2023-08-11 18:16 ` Eric Sunshine
2023-08-11 18:16 ` Oswald Buddenhagen
2 siblings, 0 replies; 53+ messages in thread
From: Eric Sunshine @ 2023-08-11 18:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Linus Arver, Oswald Buddenhagen, git
On Fri, Aug 11, 2023 at 2:12 PM Junio C Hamano <gitster@pobox.com> wrote:
> Subject: [PATCH 3/2] revert: force explaining overly complex revert chain
>
> Once we revert reverts of revert and reach "Reapply "Reapply "..."",
> it becomes too unweirdly to read a reversion of such a comit.
s/unweirdly/unwieldy/
s/comit/commit/
> We instruct the user to explain why the reversion is done in their
> own words when using the revert.reference mode, and the instruction
> applies equally for such an overly complex revert chain. The
> rationale for such a sequence of events should be recorded to help
> future developers.
>
> Building on top of the recent Oswald's work to turn "revert revert"
> into "reapply", let's turn the reference mode automatically on in
> such a case.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: Re* [PATCH v3 2/2] doc: revert: add discussion
2023-08-11 17:44 ` Re* " Junio C Hamano
2023-08-11 17:53 ` Junio C Hamano
2023-08-11 18:16 ` Eric Sunshine
@ 2023-08-11 18:16 ` Oswald Buddenhagen
2023-08-11 19:43 ` Junio C Hamano
2 siblings, 1 reply; 53+ messages in thread
From: Oswald Buddenhagen @ 2023-08-11 18:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Linus Arver, git
On Fri, Aug 11, 2023 at 10:44:50AM -0700, Junio C Hamano wrote:
>Junio C Hamano <gitster@pobox.com> writes:
>
>> Linus Arver <linusa@google.com> writes:
>>
>>> Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
>>>
>>>> while thinking about what to write, i came up with an idea for another
>>>> improvement: with (implicit) --edit, the template message would end up
>>>> being:
>>>>
>>>> This reverts commit <sha1>,
>>>> because <PUT REASON HERE>.
>>>
>>> This sounds great to me.
>>
>> Oh, absolutely. I rarely do a revert myself (other than reverting a
>> premature merge out of 'next'), but giving a better instruction in
>> the commit log editor buffer as template is a very good idea.
>
>It might be just the matter of doing something like the attached
>patch on top of Oswald's, reusing the existing code to instruct the
>user to describe the reversion.
>
hmm, this seems to be going down a too narrow road - my idea was to make
this fully orthogonal to reverting reverts in particular (note that i
attached it to the generic "discussion" patch rather than the "reverts
of reverts" one).
i didn't think about the integration with existing options yet.
regards
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: Re* [PATCH v3 2/2] doc: revert: add discussion
2023-08-11 18:16 ` Oswald Buddenhagen
@ 2023-08-11 19:43 ` Junio C Hamano
0 siblings, 0 replies; 53+ messages in thread
From: Junio C Hamano @ 2023-08-11 19:43 UTC (permalink / raw)
To: Oswald Buddenhagen; +Cc: Linus Arver, git
Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
> make this fully orthogonal to reverting reverts in particular (note
> that i attached it to the generic "discussion" patch rather than the
> "reverts of reverts" one).
I viewed that as a patch to add "discussion on how to explain
revert" (not necessarily "revert of revert", but how "revert" in
general should be explained).
> i didn't think about the integration with existing options yet.
Now I did ;-).
The discussion of what the right way to present and justify a revert
was done before, and I think revert.reference and the "--reference"
option came out of it. It aims the same "do not just describe the
fact what was reverted, but you should explain why" spirit. While
what I showed in my illustration patch was not meant as "integration
with existing options", it is inevitable that reuse of existing code
that was written earlier for improving the workflow in the same
spirit may get involved.
Perhaps we should also tweak the commit log template to give a
gentle knudge to the user to use revert.reference and then enhance
the help text we give.
Instead of
Revert "doc: revert: add discussion"
This reverts commit 7139d1298993b0148ad429cd7cb4824223b7f420.
you may see something along the lines of ...
# Consider retitling to reflect WHY you are reverting.
# You may also want to set revert.reference configuration to
# help encuraging a better title for revert commits.
Revert "doc: revert: add discussion"
This reverts commit 7139d1298993b0148ad429cd7cb4824223b7f420
because <REASON OF REVERSION HERE>
I ran out of time budget for today to think about a topic that is
not releant to the current release, so I'd stop here.
THanks.
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v3 2/2] doc: revert: add discussion
2023-08-11 12:49 ` Oswald Buddenhagen
@ 2023-08-11 23:00 ` Linus Arver
2023-08-12 7:19 ` Oswald Buddenhagen
0 siblings, 1 reply; 53+ messages in thread
From: Linus Arver @ 2023-08-11 23:00 UTC (permalink / raw)
To: Oswald Buddenhagen; +Cc: git, Junio C Hamano
Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
> On Thu, Aug 10, 2023 at 02:50:59PM -0700, Linus Arver wrote:
>>Nit: the "doc: revert: add discussion" subject line should probably be more
>>like "revert doc: suggest adding the 'why' behind reverts".
>>
> this is counter to the prevalent "big endian" prefix style, and is in
> this case really easy to misread.
> i also intentionally kept the subject generic, because the content
> covers two matters (the reasoning and the subjects, which is also the
> reason why this is a separate patch to start with).
I think the phrase "add discussion" in "doc: revert: add discussion"
doesn't add much value, because your patch's diff is very easy to read
(in that it adds a new DISCUSSION section). I just wanted to replace it
with something more useful that gives more information than just repeat
(somewhat redundantly) what is obvious by looking at the patch.
I also learned recently that there should just be one colon ":" in the
subject, which is why I suggested "revert doc" as the prefix instead of
"doc: revert: ...".
>>Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
>>> +DISCUSSION
>>> +----------
>>> +
>>> +While git creates a basic commit message automatically, you really
>>> +should not leave it at that. In particular, it is _strongly_
>>> +recommended to explain why the original commit is being reverted.
>>> +Repeatedly reverting reversions yields increasingly unwieldy
>>> +commit subjects; latest when you arrive at 'Reapply "Reapply
>>> +"<original subject>""' you should get creative.
>>
>>The word "latest" here sounds odd. Ditto for "get creative".
>>
> yeah, i suppose. i wasn't sure how formal i should make it - things
> aren't consistent to start with.
For our discussion I will define "formal" style as the writing style
used in traditional reference texts, for example dictionaries and
encyclopedias.
For manpages, I think we should stick to formal style as much as
possible. The main concern I have is for readers of our manpages where
English may not be their first language. Although I understood what you
meant by the phrase "get creative", others may not understand so
readily. If there are places in our manpages where we do not use formal
style, I think we should fix them.
For other types of documentation like tutorials, I think the style
doesn't have to be as formal, (in fact it should be as informal as
possible) because a tutorial should be enjoyable to read from beginning
to end. This is unlike a manpage where most of the time the user reads
specific (sub)sections to get the exact, precise information they need
(just like looking up a word in a dictionary).
>> How about the following rewording?
>>
>> While git creates a basic commit message automatically, it is
>> _strongly_ recommended to explain why the original commit is being
>> reverted. In addition, repeatedly reverting the same commit will
>> result in increasingly unwieldy subject lines,
>
>>for example 'Reapply "Reapply "<original subject>""'.
>>
> you turned it from a suggested threshold into an example. at this point
> it appears superfluous to me.
>
>>Please consider rewording such
>> subject lines to reflect the reason why the original commit is being
>> reapplied again.
>>
> the reasoning most likely wouldn't fit into the subject.
Hence the language "to _reflect_ the reason", because the "reason"
should belong in the commit message body text.
> also, the original request to explain the reasoning applies
> transitively, so i don't think it's really necessary to point it out
> explicitly.
It may be that a user will think only giving the revert reason in the
body text is enough, while leaving the subject line as is. I wanted to
break this line of thinking by providing additional instructions.
> On Thu, Aug 10, 2023 at 03:00:41PM -0700, Linus Arver wrote:
>>Hmph, "repeatedly reverting the same commit" sounds wrong because
>>strictly speaking there is only 1 "same commit" (the original commit).
>>Perhaps
>>
>> In addition, repeatedly reverting the same progression of reverts will
>>
>>or even
>>
>> In addition, repeatedly reverting the same revert chain will
>>
>>is better here?
>>
> we used "recursive reverts" elsewhere. but i'm not sure whether that's
> sufficiently intuitive and formally correct.
Agreed. We might have to just define the correct phrasing ourselves in
"man gitglossary". Surprisingly I see that the term "revert" (which can
be both a verb _and_ a noun) is not defined there.
> [...]
>
> so in summary, how about:
>
> While git creates a basic commit message automatically, it is
> _strongly_ recommended to explain why the original commit is being
> reverted. In addition, repeatedly reverting reversions will
> result in increasingly unwieldy subject lines. Please consider
> rewording these into something shorter and more unique.
This is definitely better. But others in this thread have already
commented that my version looks good (after seeing your version also,
presumably).
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v3 2/2] doc: revert: add discussion
2023-08-11 15:10 ` Phillip Wood
@ 2023-08-12 6:25 ` Oswald Buddenhagen
2023-08-13 22:09 ` Junio C Hamano
0 siblings, 1 reply; 53+ messages in thread
From: Oswald Buddenhagen @ 2023-08-12 6:25 UTC (permalink / raw)
To: phillip.wood; +Cc: Linus Arver, git, Junio C Hamano
On Fri, Aug 11, 2023 at 04:10:55PM +0100, Phillip Wood wrote:
>On 10/08/2023 23:00, Linus Arver wrote:
>> Linus Arver <linusa@google.com> writes:
>>
>>> How about
>>> the following rewording?
>>>
>>> While git creates a basic commit message automatically, it is
>>> _strongly_ recommended to explain why the original commit is being
>>> reverted. In addition, repeatedly reverting the same commit will
>>
>> Hmph, "repeatedly reverting the same commit" sounds wrong because
>> strictly speaking there is only 1 "same commit" (the original commit).
>
>While it isn't strictly accurate I think that wording is easy enough to
>understand.
>
yes, but why would that be _better_ than saying "repeatedly reverting
reversions" like i did?
regards
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v3 2/2] doc: revert: add discussion
2023-08-11 23:00 ` Linus Arver
@ 2023-08-12 7:19 ` Oswald Buddenhagen
2023-09-07 21:29 ` Linus Arver
0 siblings, 1 reply; 53+ messages in thread
From: Oswald Buddenhagen @ 2023-08-12 7:19 UTC (permalink / raw)
To: Linus Arver; +Cc: git, Junio C Hamano
On Fri, Aug 11, 2023 at 04:00:53PM -0700, Linus Arver wrote:
>Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
>
>> On Thu, Aug 10, 2023 at 02:50:59PM -0700, Linus Arver wrote:
>>>Nit: the "doc: revert: add discussion" subject line should probably be more
>>>like "revert doc: suggest adding the 'why' behind reverts".
>>>
>> this is counter to the prevalent "big endian" prefix style, and is in
>> this case really easy to misread.
>
>I also learned recently that there should just be one colon ":" in the
>subject, which is why I suggested "revert doc" as the prefix instead of
>"doc: revert: ...".
>
in what context was this preference expressed?
because here, it's rather counter-productive: most commands are verbs
for obvious reasons, so using that style sets the reader up for
misparsing the subject on first try. this could be avoided by quoting
the command, but that looks noisy in the subject.
so rather, i'd follow another precedent, 'git-revert.txt: ', which is
unambiguous.
>> i also intentionally kept the subject generic, because the content
>> covers two matters (the reasoning and the subjects, which is also the
>> reason why this is a separate patch to start with).
>
>I think the phrase "add discussion" in "doc: revert: add discussion"
>doesn't add much value, because your patch's diff is very easy to read
>(in that it adds a new DISCUSSION section). I just wanted to replace it
>with something more useful that gives more information than
>just repeat
>(somewhat redundantly) what is obvious by looking at the patch.
>
but ... that's exactly what a subject is supposed to do!
>>>Please consider rewording such
>>> subject lines to reflect the reason why the original commit is being
>>> reapplied again.
>>>
>> the reasoning most likely wouldn't fit into the subject.
>
>Hence the language "to _reflect_ the reason", because the "reason"
>should belong in the commit message body text.
>
i don't think that's how most people would actually read this.
and i still don't see how that instruction could be meaningfully
followed.
>> also, the original request to explain the reasoning applies
>> transitively, so i don't think it's really necessary to point it out
>> explicitly.
>
>It may be that a user will think only giving the revert reason in the
>body text is enough, while leaving the subject line as is. I wanted to
>break this line of thinking by providing additional instructions.
>
yes, that's the whole intention of this patch. but i don't see how
making it more convoluted than my proposal helps in any way.
>This is definitely better. But others in this thread have already
>commented that my version looks good (after seeing your version also,
>presumably).
>
well, i'm also an "others" when it comes to your proposal, and i find it
confusing.
regards
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v3 2/2] doc: revert: add discussion
2023-08-12 6:25 ` Oswald Buddenhagen
@ 2023-08-13 22:09 ` Junio C Hamano
2023-08-14 14:13 ` Oswald Buddenhagen
0 siblings, 1 reply; 53+ messages in thread
From: Junio C Hamano @ 2023-08-13 22:09 UTC (permalink / raw)
To: Oswald Buddenhagen; +Cc: phillip.wood, Linus Arver, git
Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
> On Fri, Aug 11, 2023 at 04:10:55PM +0100, Phillip Wood wrote:
>>On 10/08/2023 23:00, Linus Arver wrote:
>>> Hmph, "repeatedly reverting the same commit" sounds wrong because
>>> strictly speaking there is only 1 "same commit" (the original commit).
>>
>> While it isn't strictly accurate I think that wording is easy enough
>> to understand.
>>
> yes, but why would that be _better_ than saying "repeatedly reverting
> reversions" like i did?
To me at least, "repeatedly reverting reversions" sounds more like a
riddle, compared to "repeatedly reverting the same commit", whose
intent sounds fairly obvious. An explicit mention of "commit", which
is a more familiar noun to folks than "reversion", does contribute to
it, I suspect.
That would be how I explain why one is _better_ over the other, but
of course these things are subjective, so I'd rather see us not
asking such questions too often: which is more familiar, "commit" vs
"reversion", especially to new folks who are starting to use "git"
and reading the manual page for "git revert"?
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v3 2/2] doc: revert: add discussion
2023-08-13 22:09 ` Junio C Hamano
@ 2023-08-14 14:13 ` Oswald Buddenhagen
0 siblings, 0 replies; 53+ messages in thread
From: Oswald Buddenhagen @ 2023-08-14 14:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: phillip.wood, Linus Arver, git
On Sun, Aug 13, 2023 at 03:09:02PM -0700, Junio C Hamano wrote:
>Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
>
>> On Fri, Aug 11, 2023 at 04:10:55PM +0100, Phillip Wood wrote:
>>>On 10/08/2023 23:00, Linus Arver wrote:
>>>> Hmph, "repeatedly reverting the same commit" sounds wrong because
>>>> strictly speaking there is only 1 "same commit" (the original commit).
>>>
>>> While it isn't strictly accurate I think that wording is easy enough
>>> to understand.
>>>
>> yes, but why would that be _better_ than saying "repeatedly reverting
>> reversions" like i did?
>
>To me at least, "repeatedly reverting reversions" sounds more like a
>riddle, compared to "repeatedly reverting the same commit", whose
>intent sounds fairly obvious.
>
a more natural way for git users to say it would be "reverting reverts",
which i think everyone in the target audience would understand, but it
seems linguistically questionable to me. native speakers may want to
opine ...
>An explicit mention of "commit", which
>is a more familiar noun to folks than "reversion", does contribute to
>it, I suspect.
>
yes, but "commit" may be misunderstood, as linus pointed out in his
reply to himself. phillip dismissed the concern, but i don't think
ambiguity is a good idea in the authoritative documentation.
unfortunately, linus' proposed alternatives seem even more like
"riddles" to me than what i am proposing.
regards
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH v4 1/2] sequencer: beautify subject of reverts of reverts
2023-08-09 17:15 ` [PATCH v3 1/2] " Oswald Buddenhagen
2023-08-09 17:15 ` [PATCH v3 2/2] doc: revert: add discussion Oswald Buddenhagen
2023-08-11 15:05 ` [PATCH v3 1/2] sequencer: beautify subject of reverts of reverts Phillip Wood
@ 2023-08-21 17:07 ` Oswald Buddenhagen
2023-08-21 17:07 ` [PATCH v4 2/2] git-revert.txt: add discussion Oswald Buddenhagen
` (2 more replies)
2 siblings, 3 replies; 53+ messages in thread
From: Oswald Buddenhagen @ 2023-08-21 17:07 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Kristoffer Haugsbakk, Phillip Wood
Instead of generating a silly-looking `Revert "Revert "foo""`, make it
a more humane `Reapply "foo"`.
This is done for two reasons:
- To cover the actually common case of just a double revert.
- To encourage people to rewrite summaries of recursive reverts by
setting an example (a subsequent commit will also do this explicitly
in the documentation).
To achieve these goals, the mechanism does not need to be particularly
sophisticated. Therefore, more complicated alternatives which would
"compress more efficiently" have not been implemented.
Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
v3:
- capitulate at first sight of a pre-existing recursive reversion, as
handling the edge cases is a bottomless pit
- reworked commit message again
- moved test into existing file
- generalized docu change and factored it out
v2:
- add discussion to commit message
- add paragraph to docu
- add test
- use skip_prefix() instead of starts_with()
- catch pre-existing double reverts
Cc: Junio C Hamano <gitster@pobox.com>
Cc: Kristoffer Haugsbakk <code@khaugsbakk.name>
Cc: Phillip Wood <phillip.wood123@gmail.com>
---
sequencer.c | 11 +++++++++++
t/t3501-revert-cherry-pick.sh | 25 +++++++++++++++++++++++++
2 files changed, 36 insertions(+)
diff --git a/sequencer.c b/sequencer.c
index cc9821ece2..12ec158922 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2249,13 +2249,24 @@ static int do_pick_commit(struct repository *r,
*/
if (command == TODO_REVERT) {
+ const char *orig_subject;
+
base = commit;
base_label = msg.label;
next = parent;
next_label = msg.parent_label;
if (opts->commit_use_reference) {
strbuf_addstr(&msgbuf,
"# *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***");
+ } else if (skip_prefix(msg.subject, "Revert \"", &orig_subject) &&
+ /*
+ * We don't touch pre-existing repeated reverts, because
+ * theoretically these can be nested arbitrarily deeply,
+ * thus requiring excessive complexity to deal with.
+ */
+ !starts_with(orig_subject, "Revert \"")) {
+ strbuf_addstr(&msgbuf, "Reapply \"");
+ strbuf_addstr(&msgbuf, orig_subject);
} else {
strbuf_addstr(&msgbuf, "Revert \"");
strbuf_addstr(&msgbuf, msg.subject);
diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
index e2ef619323..7011e3a421 100755
--- a/t/t3501-revert-cherry-pick.sh
+++ b/t/t3501-revert-cherry-pick.sh
@@ -176,6 +176,31 @@ test_expect_success 'advice from failed revert' '
test_cmp expected actual
'
+test_expect_success 'title of fresh reverts' '
+ test_commit --no-tag A file1 &&
+ test_commit --no-tag B file1 &&
+ git revert --no-edit HEAD &&
+ echo "Revert \"B\"" >expect &&
+ git log -1 --pretty=%s >actual &&
+ test_cmp expect actual &&
+ git revert --no-edit HEAD &&
+ echo "Reapply \"B\"" >expect &&
+ git log -1 --pretty=%s >actual &&
+ test_cmp expect actual &&
+ git revert --no-edit HEAD &&
+ echo "Revert \"Reapply \"B\"\"" >expect &&
+ git log -1 --pretty=%s >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'title of legacy double revert' '
+ test_commit --no-tag "Revert \"Revert \"B\"\"" file1 &&
+ git revert --no-edit HEAD &&
+ echo "Revert \"Revert \"Revert \"B\"\"\"" >expect &&
+ git log -1 --pretty=%s >actual &&
+ test_cmp expect actual
+'
+
test_expect_success 'identification of reverted commit (default)' '
test_commit to-ident &&
test_when_finished "git reset --hard to-ident" &&
--
2.40.0.152.g15d061e6df
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [PATCH v4 2/2] git-revert.txt: add discussion
2023-08-21 17:07 ` [PATCH v4 " Oswald Buddenhagen
@ 2023-08-21 17:07 ` Oswald Buddenhagen
2023-08-21 18:32 ` [PATCH v4 1/2] sequencer: beautify subject of reverts of reverts Junio C Hamano
2023-08-23 20:08 ` Taylor Blau
2 siblings, 0 replies; 53+ messages in thread
From: Oswald Buddenhagen @ 2023-08-21 17:07 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Linus Arver, Phillip Wood, Kristoffer Haugsbakk
The section is inspired by git-commit.txt.
Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
v4:
- adjusted summary prefix & payload wording
Cc: Junio C Hamano <gitster@pobox.com>
Cc: Linus Arver <linusa@google.com>
Cc: Phillip Wood <phillip.wood123@gmail.com>
Cc: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
Documentation/git-revert.txt | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt
index d2e10d3dce..cbe0208834 100644
--- a/Documentation/git-revert.txt
+++ b/Documentation/git-revert.txt
@@ -142,6 +142,16 @@ EXAMPLES
changes. The revert only modifies the working tree and the
index.
+DISCUSSION
+----------
+
+While git creates a basic commit message automatically, it is
+_strongly_ recommended to explain why the original commit is being
+reverted.
+In addition, repeatedly reverting reverts will result in increasingly
+unwieldy subject lines, for example 'Reapply "Reapply "<original subject>""'.
+Please consider rewording these to be shorter and more unique.
+
CONFIGURATION
-------------
--
2.40.0.152.g15d061e6df
^ permalink raw reply related [flat|nested] 53+ messages in thread
* Re: [PATCH v4 1/2] sequencer: beautify subject of reverts of reverts
2023-08-21 17:07 ` [PATCH v4 " Oswald Buddenhagen
2023-08-21 17:07 ` [PATCH v4 2/2] git-revert.txt: add discussion Oswald Buddenhagen
@ 2023-08-21 18:32 ` Junio C Hamano
2023-08-23 20:08 ` Taylor Blau
2 siblings, 0 replies; 53+ messages in thread
From: Junio C Hamano @ 2023-08-21 18:32 UTC (permalink / raw)
To: Oswald Buddenhagen; +Cc: git, Kristoffer Haugsbakk, Phillip Wood
Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
> Instead of generating a silly-looking `Revert "Revert "foo""`, make it
> a more humane `Reapply "foo"`.
Looking good. Will requeue. Thanks.
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v4 1/2] sequencer: beautify subject of reverts of reverts
2023-08-21 17:07 ` [PATCH v4 " Oswald Buddenhagen
2023-08-21 17:07 ` [PATCH v4 2/2] git-revert.txt: add discussion Oswald Buddenhagen
2023-08-21 18:32 ` [PATCH v4 1/2] sequencer: beautify subject of reverts of reverts Junio C Hamano
@ 2023-08-23 20:08 ` Taylor Blau
2023-08-23 21:38 ` Junio C Hamano
2 siblings, 1 reply; 53+ messages in thread
From: Taylor Blau @ 2023-08-23 20:08 UTC (permalink / raw)
To: Oswald Buddenhagen
Cc: git, Junio C Hamano, Kristoffer Haugsbakk, Phillip Wood
On Mon, Aug 21, 2023 at 07:07:19PM +0200, Oswald Buddenhagen wrote:
> To achieve these goals, the mechanism does not need to be particularly
> sophisticated. Therefore, more complicated alternatives which would
> "compress more efficiently" have not been implemented.
This version is looking good. The main functionality is well-reasoned
and straightforwardly implemented. One minor suggestion that you could
consider squashing in is some test clean-up like so:
--- 8< ---
diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
index 7011e3a421..4dee71d6d5 100755
--- a/t/t3501-revert-cherry-pick.sh
+++ b/t/t3501-revert-cherry-pick.sh
@@ -176,29 +176,27 @@ test_expect_success 'advice from failed revert' '
test_cmp expected actual
'
+test_expect_commit_msg () {
+ echo "$@" >expect &&
+ git log -1 --pretty=%s >actual &&
+ test_cmp expect actual
+}
+
test_expect_success 'title of fresh reverts' '
test_commit --no-tag A file1 &&
test_commit --no-tag B file1 &&
git revert --no-edit HEAD &&
- echo "Revert \"B\"" >expect &&
- git log -1 --pretty=%s >actual &&
- test_cmp expect actual &&
+ test_expect_commit_msg "Revert \"B\"" &&
git revert --no-edit HEAD &&
- echo "Reapply \"B\"" >expect &&
- git log -1 --pretty=%s >actual &&
- test_cmp expect actual &&
+ test_expect_commit_msg "Reapply \"B\"" &&
git revert --no-edit HEAD &&
- echo "Revert \"Reapply \"B\"\"" >expect &&
- git log -1 --pretty=%s >actual &&
- test_cmp expect actual
+ test_expect_commit_msg "Revert \"Reapply \"B\"\""
'
test_expect_success 'title of legacy double revert' '
test_commit --no-tag "Revert \"Revert \"B\"\"" file1 &&
git revert --no-edit HEAD &&
- echo "Revert \"Revert \"Revert \"B\"\"\"" >expect &&
- git log -1 --pretty=%s >actual &&
- test_cmp expect actual
+ test_expect_commit_msg "Revert \"Revert \"Revert \"B\"\"\""
'
test_expect_success 'identification of reverted commit (default)' '
--- >8 ---
To my eyes, it makes checking the subject of our revert commit against
an expected value more readable by factoring out the echo, git log,
test_cmp pattern.
Thanks,
Taylor
^ permalink raw reply related [flat|nested] 53+ messages in thread
* Re: [PATCH v4 1/2] sequencer: beautify subject of reverts of reverts
2023-08-23 20:08 ` Taylor Blau
@ 2023-08-23 21:38 ` Junio C Hamano
2023-08-24 6:14 ` Oswald Buddenhagen
2023-09-02 7:20 ` [PATCH v5] " Oswald Buddenhagen
0 siblings, 2 replies; 53+ messages in thread
From: Junio C Hamano @ 2023-08-23 21:38 UTC (permalink / raw)
To: Taylor Blau; +Cc: Oswald Buddenhagen, git, Kristoffer Haugsbakk, Phillip Wood
Taylor Blau <me@ttaylorr.com> writes:
> This version is looking good. The main functionality is well-reasoned
> and straightforwardly implemented. One minor suggestion that you could
> consider squashing in is some test clean-up like so:
>
> --- 8< ---
> diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
> index 7011e3a421..4dee71d6d5 100755
> --- a/t/t3501-revert-cherry-pick.sh
> +++ b/t/t3501-revert-cherry-pick.sh
> @@ -176,29 +176,27 @@ test_expect_success 'advice from failed revert' '
> test_cmp expected actual
> '
>
> +test_expect_commit_msg () {
> + echo "$@" >expect &&
> + git log -1 --pretty=%s >actual &&
> + test_cmp expect actual
> +}
> +
> test_expect_success 'title of fresh reverts' '
> test_commit --no-tag A file1 &&
> test_commit --no-tag B file1 &&
> git revert --no-edit HEAD &&
> - echo "Revert \"B\"" >expect &&
> - git log -1 --pretty=%s >actual &&
> - test_cmp expect actual &&
> + test_expect_commit_msg "Revert \"B\"" &&
> git revert --no-edit HEAD &&
> - echo "Reapply \"B\"" >expect &&
> - git log -1 --pretty=%s >actual &&
> - test_cmp expect actual &&
> + test_expect_commit_msg "Reapply \"B\"" &&
> git revert --no-edit HEAD &&
> - echo "Revert \"Reapply \"B\"\"" >expect &&
> - git log -1 --pretty=%s >actual &&
> - test_cmp expect actual
> + test_expect_commit_msg "Revert \"Reapply \"B\"\""
> '
>
> test_expect_success 'title of legacy double revert' '
> test_commit --no-tag "Revert \"Revert \"B\"\"" file1 &&
> git revert --no-edit HEAD &&
> - echo "Revert \"Revert \"Revert \"B\"\"\"" >expect &&
> - git log -1 --pretty=%s >actual &&
> - test_cmp expect actual
> + test_expect_commit_msg "Revert \"Revert \"Revert \"B\"\"\""
> '
>
> test_expect_success 'identification of reverted commit (default)' '
> --- >8 ---
>
> To my eyes, it makes checking the subject of our revert commit against
> an expected value more readable by factoring out the echo, git log,
> test_cmp pattern.
Yeah it does make the test more concise and what is expected stand
out more clearly. Good suggestion.
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v4 1/2] sequencer: beautify subject of reverts of reverts
2023-08-23 21:38 ` Junio C Hamano
@ 2023-08-24 6:14 ` Oswald Buddenhagen
2023-09-02 7:20 ` [PATCH v5] " Oswald Buddenhagen
1 sibling, 0 replies; 53+ messages in thread
From: Oswald Buddenhagen @ 2023-08-24 6:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Taylor Blau, git, Kristoffer Haugsbakk, Phillip Wood
On Wed, Aug 23, 2023 at 02:38:36PM -0700, Junio C Hamano wrote:
>Taylor Blau <me@ttaylorr.com> writes:
>
>> This version is looking good. The main functionality is well-reasoned
>> and straightforwardly implemented. One minor suggestion that you could
>> consider squashing in is some test clean-up like so:
>>
>
>Yeah it does make the test more concise and what is expected stand
>out more clearly. Good suggestion.
>
agreed. do you want to squash it on your end, or should i reroll?
regards
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH v5] sequencer: beautify subject of reverts of reverts
2023-08-23 21:38 ` Junio C Hamano
2023-08-24 6:14 ` Oswald Buddenhagen
@ 2023-09-02 7:20 ` Oswald Buddenhagen
2023-09-02 22:24 ` Junio C Hamano
2023-09-11 20:12 ` Kristoffer Haugsbakk
1 sibling, 2 replies; 53+ messages in thread
From: Oswald Buddenhagen @ 2023-09-02 7:20 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Kristoffer Haugsbakk, Phillip Wood
Instead of generating a silly-looking `Revert "Revert "foo""`, make it
a more humane `Reapply "foo"`.
This is done for two reasons:
- To cover the actually common case of just a double revert.
- To encourage people to rewrite summaries of recursive reverts by
setting an example (a subsequent commit will also do this explicitly
in the documentation).
To achieve these goals, the mechanism does not need to be particularly
sophisticated. Therefore, more complicated alternatives which would
"compress more efficiently" have not been implemented.
Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
v4:
- factor out verification of subject as per taylor's patch, with minor
modifications.
fwiw, it might make sense to put this into test-lib-functions.sh right
after test_commit_message(), then named test_commit_subject(). not
sure it would be worth it, given an equally generic implementation
would be kinda over-engineered, and the discoverability is kinda poor.
v3:
- capitulate at first sight of a pre-existing recursive reversion, as
handling the edge cases is a bottomless pit
- reworked commit message again
- moved test into existing file
- generalized docu change and factored it out
v2:
- add discussion to commit message
- add paragraph to docu
- add test
- use skip_prefix() instead of starts_with()
- catch pre-existing double reverts
Cc: Junio C Hamano <gitster@pobox.com>
Cc: Kristoffer Haugsbakk <code@khaugsbakk.name>
Cc: Phillip Wood <phillip.wood123@gmail.com>
---
sequencer.c | 11 +++++++++++
t/t3501-revert-cherry-pick.sh | 23 +++++++++++++++++++++++
2 files changed, 34 insertions(+)
diff --git a/sequencer.c b/sequencer.c
index cc9821ece2..12ec158922 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2249,13 +2249,24 @@ static int do_pick_commit(struct repository *r,
*/
if (command == TODO_REVERT) {
+ const char *orig_subject;
+
base = commit;
base_label = msg.label;
next = parent;
next_label = msg.parent_label;
if (opts->commit_use_reference) {
strbuf_addstr(&msgbuf,
"# *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***");
+ } else if (skip_prefix(msg.subject, "Revert \"", &orig_subject) &&
+ /*
+ * We don't touch pre-existing repeated reverts, because
+ * theoretically these can be nested arbitrarily deeply,
+ * thus requiring excessive complexity to deal with.
+ */
+ !starts_with(orig_subject, "Revert \"")) {
+ strbuf_addstr(&msgbuf, "Reapply \"");
+ strbuf_addstr(&msgbuf, orig_subject);
} else {
strbuf_addstr(&msgbuf, "Revert \"");
strbuf_addstr(&msgbuf, msg.subject);
diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
index e2ef619323..4158590322 100755
--- a/t/t3501-revert-cherry-pick.sh
+++ b/t/t3501-revert-cherry-pick.sh
@@ -176,6 +176,29 @@ test_expect_success 'advice from failed revert' '
test_cmp expected actual
'
+test_expect_subject () {
+ echo "$1" >expect &&
+ git log -1 --pretty=%s >actual &&
+ test_cmp expect actual
+}
+
+test_expect_success 'titles of fresh reverts' '
+ test_commit --no-tag A file1 &&
+ test_commit --no-tag B file1 &&
+ git revert --no-edit HEAD &&
+ test_expect_subject "Revert \"B\"" &&
+ git revert --no-edit HEAD &&
+ test_expect_subject "Reapply \"B\"" &&
+ git revert --no-edit HEAD &&
+ test_expect_subject "Revert \"Reapply \"B\"\""
+'
+
+test_expect_success 'title of legacy double revert' '
+ test_commit --no-tag "Revert \"Revert \"B\"\"" file1 &&
+ git revert --no-edit HEAD &&
+ test_expect_subject "Revert \"Revert \"Revert \"B\"\"\""
+'
+
test_expect_success 'identification of reverted commit (default)' '
test_commit to-ident &&
test_when_finished "git reset --hard to-ident" &&
--
2.40.0.152.g15d061e6df
^ permalink raw reply related [flat|nested] 53+ messages in thread
* Re: [PATCH v5] sequencer: beautify subject of reverts of reverts
2023-09-02 7:20 ` [PATCH v5] " Oswald Buddenhagen
@ 2023-09-02 22:24 ` Junio C Hamano
2023-09-11 20:12 ` Kristoffer Haugsbakk
1 sibling, 0 replies; 53+ messages in thread
From: Junio C Hamano @ 2023-09-02 22:24 UTC (permalink / raw)
To: Oswald Buddenhagen; +Cc: git, Kristoffer Haugsbakk, Phillip Wood
Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
> ---
> v4:
> - factor out verification of subject as per taylor's patch, with minor
> modifications.
The change seems to make the test quite straight-forward to read.
Let's mark the topic for 'next'.
Thanks.
> diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
> index e2ef619323..4158590322 100755
> --- a/t/t3501-revert-cherry-pick.sh
> +++ b/t/t3501-revert-cherry-pick.sh
> @@ -176,6 +176,29 @@ test_expect_success 'advice from failed revert' '
> test_cmp expected actual
> '
>
> +test_expect_subject () {
> + echo "$1" >expect &&
> + git log -1 --pretty=%s >actual &&
> + test_cmp expect actual
> +}
> +
> +test_expect_success 'titles of fresh reverts' '
> + test_commit --no-tag A file1 &&
> + test_commit --no-tag B file1 &&
> + git revert --no-edit HEAD &&
> + test_expect_subject "Revert \"B\"" &&
> + git revert --no-edit HEAD &&
> + test_expect_subject "Reapply \"B\"" &&
> + git revert --no-edit HEAD &&
> + test_expect_subject "Revert \"Reapply \"B\"\""
> +'
> +
> +test_expect_success 'title of legacy double revert' '
> + test_commit --no-tag "Revert \"Revert \"B\"\"" file1 &&
> + git revert --no-edit HEAD &&
> + test_expect_subject "Revert \"Revert \"Revert \"B\"\"\""
> +'
> +
> test_expect_success 'identification of reverted commit (default)' '
> test_commit to-ident &&
> test_when_finished "git reset --hard to-ident" &&
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v3 2/2] doc: revert: add discussion
2023-08-12 7:19 ` Oswald Buddenhagen
@ 2023-09-07 21:29 ` Linus Arver
0 siblings, 0 replies; 53+ messages in thread
From: Linus Arver @ 2023-09-07 21:29 UTC (permalink / raw)
To: Oswald Buddenhagen; +Cc: git, Junio C Hamano
First, I apologize for the long delay in my response. I only work on Git
20% of the time, and that 20% can become 0% due to factors outside my
control.
Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
> On Fri, Aug 11, 2023 at 04:00:53PM -0700, Linus Arver wrote:
>>Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
>>
>>> On Thu, Aug 10, 2023 at 02:50:59PM -0700, Linus Arver wrote:
>>>>Nit: the "doc: revert: add discussion" subject line should probably be more
>>>>like "revert doc: suggest adding the 'why' behind reverts".
>>>>
>>> this is counter to the prevalent "big endian" prefix style, and is in
>>> this case really easy to misread.
>>
>>I also learned recently that there should just be one colon ":" in the
>>subject, which is why I suggested "revert doc" as the prefix instead of
>>"doc: revert: ...".
>>
> in what context was this preference expressed?
IIRC, it was from a conversation off-list with the folks at Google's
Git-core team.
> because here, it's rather counter-productive: most commands are verbs
> for obvious reasons, so using that style sets the reader up for
> misparsing the subject on first try.
I think the convention for commit titles is
<prefix>: <action>
so the phrase "revert doc: add discussion", where the <prefix> is
"revert doc" does not parse any worse than "doc: revert: add
discussion". That is, the <prefix> is never confused with the <action>
(they are separated by the colon).
> this could be avoided by quoting
> the command, but that looks noisy in the subject.
> so rather, i'd follow another precedent, 'git-revert.txt: ', which is
> unambiguous.
SGTM.
>>> i also intentionally kept the subject generic, because the content
>>> covers two matters (the reasoning and the subjects, which is also the
>>> reason why this is a separate patch to start with).
>>
>>I think the phrase "add discussion" in "doc: revert: add discussion"
>>doesn't add much value, because your patch's diff is very easy to read
>>(in that it adds a new DISCUSSION section). I just wanted to replace it
>>with something more useful that gives more information than
>
>>just repeat
>>(somewhat redundantly) what is obvious by looking at the patch.
>>
> but ... that's exactly what a subject is supposed to do!
I think the rule of thumb is to explain the goodness of what a commit
brings, rather than focus on what is literally happening. This is
because the former is more valuable. So instead of
"git-revert.txt: add discussion"
you could say
"git-revert.txt: advise against default commit message"
and now you don't have to look at the patch to see (roughly) what kind
of discussion was added.
>>>>Please consider rewording such
>>>> subject lines to reflect the reason why the original commit is being
>>>> reapplied again.
>>>>
>>> the reasoning most likely wouldn't fit into the subject.
>>
>>Hence the language "to _reflect_ the reason", because the "reason"
>>should belong in the commit message body text.
>>
> i don't think that's how most people would actually read this.
> and i still don't see how that instruction could be meaningfully
> followed.
OK, you may be right.
>>> also, the original request to explain the reasoning applies
>>> transitively, so i don't think it's really necessary to point it out
>>> explicitly.
>>
>>It may be that a user will think only giving the revert reason in the
>>body text is enough, while leaving the subject line as is. I wanted to
>>break this line of thinking by providing additional instructions.
>>
> yes, that's the whole intention of this patch. but i don't see how
> making it more convoluted than my proposal helps in any way.
Well, even if a review makes something more convoluted, it may generate
discussion and drive consensus on the better way(s) of doing something.
I see value in that course of events.
Of course you are free to reject review comments that you truly believe
are inferior to the approach you've already taken.
But overall, when I see a reviewer's comment on this mailing list, I
assume they are trying to make my patch better. Similarly when I
reviewed your patch my intent was to provide actionable feedback to try
to make it better. I'm sorry if I did not come across that way.
>>This is definitely better. But others in this thread have already
>>commented that my version looks good (after seeing your version also,
>>presumably).
>>
> well, i'm also an "others" when it comes to your proposal, and i find it
> confusing.
I think you did the right thing by responding to my comments, and
pointing to things you found confusing.
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v5] sequencer: beautify subject of reverts of reverts
2023-09-02 7:20 ` [PATCH v5] " Oswald Buddenhagen
2023-09-02 22:24 ` Junio C Hamano
@ 2023-09-11 20:12 ` Kristoffer Haugsbakk
1 sibling, 0 replies; 53+ messages in thread
From: Kristoffer Haugsbakk @ 2023-09-11 20:12 UTC (permalink / raw)
To: Oswald Buddenhagen; +Cc: Junio C Hamano, Phillip Wood, git
On Sat, Sep 2, 2023, at 09:20, Oswald Buddenhagen wrote:
> Instead of generating a silly-looking `Revert "Revert "foo""`, make it
> a more humane `Reapply "foo"`.
Congrats on a nice series. It's very “lean and mean”—focused, not
excessive.
And I think I will remember the phrase “too nerdy” for a while. ;)
Maybe we will get this message template the next time we revert a
merge.[1]
> If you merge the updated side branch (with D at its tip), none of the
> changes made in A or B will be in the result, because they were reverted
> by W. That is what Alan saw.
>
> [...]
>
> In such a situation, you would want to first revert the previous revert,
> which would make the history look like this: ...
🔗 1: https://github.com/git/git/blob/master/Documentation/howto/revert-a-faulty-merge.txt
Cheers
--
Kristoffer
^ permalink raw reply [flat|nested] 53+ messages in thread
end of thread, other threads:[~2023-09-11 21:38 UTC | newest]
Thread overview: 53+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-04-28 8:35 [PATCH v2] sequencer: beautify subject of reverts of reverts Oswald Buddenhagen
2023-04-28 18:35 ` Junio C Hamano
2023-04-28 19:11 ` Oswald Buddenhagen
2023-05-01 16:44 ` Junio C Hamano
2023-05-01 19:10 ` Oswald Buddenhagen
2023-05-01 19:12 ` Junio C Hamano
2023-05-05 17:25 ` Junio C Hamano
2023-05-17 9:05 ` Phillip Wood
2023-05-17 10:00 ` Oswald Buddenhagen
2023-05-17 11:20 ` Phillip Wood
2023-05-17 17:02 ` Oswald Buddenhagen
2023-05-18 9:58 ` Phillip Wood
2023-05-18 16:28 ` Junio C Hamano
2023-07-28 5:26 ` Linus Arver
2023-07-28 9:45 ` Oswald Buddenhagen
2023-07-28 15:10 ` Junio C Hamano
2023-07-28 15:37 ` Oswald Buddenhagen
2023-07-28 16:31 ` Junio C Hamano
2023-07-28 16:47 ` Oswald Buddenhagen
2023-07-28 17:36 ` Linus Arver
2023-08-09 17:15 ` [PATCH v3 1/2] " Oswald Buddenhagen
2023-08-09 17:15 ` [PATCH v3 2/2] doc: revert: add discussion Oswald Buddenhagen
2023-08-10 21:50 ` Linus Arver
2023-08-10 22:00 ` Linus Arver
2023-08-11 15:10 ` Phillip Wood
2023-08-12 6:25 ` Oswald Buddenhagen
2023-08-13 22:09 ` Junio C Hamano
2023-08-14 14:13 ` Oswald Buddenhagen
2023-08-11 12:49 ` Oswald Buddenhagen
2023-08-11 23:00 ` Linus Arver
2023-08-12 7:19 ` Oswald Buddenhagen
2023-09-07 21:29 ` Linus Arver
2023-08-11 15:08 ` Phillip Wood
2023-08-11 17:10 ` Junio C Hamano
2023-08-11 17:05 ` Junio C Hamano
2023-08-11 17:44 ` Re* " Junio C Hamano
2023-08-11 17:53 ` Junio C Hamano
2023-08-11 17:56 ` rsbecker
2023-08-11 18:16 ` Eric Sunshine
2023-08-11 18:16 ` Oswald Buddenhagen
2023-08-11 19:43 ` Junio C Hamano
2023-08-11 15:05 ` [PATCH v3 1/2] sequencer: beautify subject of reverts of reverts Phillip Wood
2023-08-11 16:59 ` Junio C Hamano
2023-08-11 17:13 ` Oswald Buddenhagen
2023-08-21 17:07 ` [PATCH v4 " Oswald Buddenhagen
2023-08-21 17:07 ` [PATCH v4 2/2] git-revert.txt: add discussion Oswald Buddenhagen
2023-08-21 18:32 ` [PATCH v4 1/2] sequencer: beautify subject of reverts of reverts Junio C Hamano
2023-08-23 20:08 ` Taylor Blau
2023-08-23 21:38 ` Junio C Hamano
2023-08-24 6:14 ` Oswald Buddenhagen
2023-09-02 7:20 ` [PATCH v5] " Oswald Buddenhagen
2023-09-02 22:24 ` Junio C Hamano
2023-09-11 20:12 ` Kristoffer Haugsbakk
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).