Oswald Buddenhagen <oswald.buddenhagen@xxxxxx> 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@xxxxxx>
> ---
> 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.
