[PATCH 0/5] Assorted fixes revolving around rebase and merges

[PATCH 0/5] Assorted fixes revolving around rebase and merges

[Johannes Schindelin via GitGitGadget]

I noticed a couple of weeks ago that I had bogus merge commits after my
rebases, where the original commits had been regular commits.

This set me out on the adventure that is reflected in this patch series.

Of course, the thing I wanted to fix is demonstrated by 1/5 and fixed in
2/5. But while at it, I ran into other issues and fixed them since I was at
it anyway.

Johannes Schindelin (5):
  rebase -r: demonstrate bug with conflicting merges
  rebase -r: do not write MERGE_HEAD unless needed
  rebase -i: include MERGE_HEAD into files to clean up
  built-in rebase --skip/--abort: clean up stale .git/<name> files
  status: rebase and merge can be in progress at the same time

 builtin/rebase.c         |  3 +++
 sequencer.c              | 10 ++++++----
 t/t3430-rebase-merges.sh | 16 ++++++++++++++++
 wt-status.c              |  9 +++++++--
 4 files changed, 32 insertions(+), 6 deletions(-)


base-commit: 8858448bb49332d353febc078ce4a3abcc962efe
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-75%2Fdscho%2Frebase-r-and-merge-head-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-75/dscho/rebase-r-and-merge-head-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/75
-- 
gitgitgadget

[PATCH 1/5] rebase -r: demonstrate bug with conflicting merges

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

When calling `merge` on a branch that has already been merged, that
`merge` is skipped quietly, but currently a MERGE_HEAD file is being
left behind and will then be grabbed by the next `pick` (that did
not want to create a *merge* commit).

Demonstrate this.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t3430-rebase-merges.sh | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index aa7bfc88ec..1f08a33687 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -396,4 +396,20 @@ test_expect_success 'with --autosquash and --exec' '
 	grep "G: +G" actual
 '
 
+test_expect_failure '--continue after resolving conflicts after a merge' '
+	git checkout -b already-has-g E &&
+	git cherry-pick E..G &&
+	test_commit H2 &&
+
+	git checkout -b conflicts-in-merge H &&
+	test_commit H2 H2.t conflicts H2-conflict &&
+	test_must_fail git rebase -r already-has-g &&
+	grep conflicts H2.t &&
+	echo resolved >H2.t &&
+	git add -u &&
+	git rebase --continue &&
+	test_must_fail git rev-parse --verify HEAD^2 &&
+	test_path_is_missing .git/MERGE_HEAD
+'
+
 test_done
-- 
gitgitgadget

[PATCH 2/5] rebase -r: do not write MERGE_HEAD unless needed

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

When we detect that a `merge` can be skipped because the merged commit
is already an ancestor of HEAD, we do not need to commit, therefore
writing the MERGE_HEAD file is useless.

It is actually worse than useless: a subsequent `git commit` will pick
it up and think that we want to merge that commit, still.

To avoid that, move the code that writes the MERGE_HEAD file to a
location where we already know that the `merge` cannot be skipped.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c              | 8 ++++----
 t/t3430-rebase-merges.sh | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 9e1ab3a2a7..7a9cd81afb 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3191,10 +3191,6 @@ static int do_merge(struct commit *commit, const char *arg, int arg_len,
 	}
 
 	merge_commit = to_merge->item;
-	write_message(oid_to_hex(&merge_commit->object.oid), GIT_SHA1_HEXSZ,
-		      git_path_merge_head(the_repository), 0);
-	write_message("no-ff", 5, git_path_merge_mode(the_repository), 0);
-
 	bases = get_merge_bases(head_commit, merge_commit);
 	if (bases && oideq(&merge_commit->object.oid,
 			   &bases->item->object.oid)) {
@@ -3203,6 +3199,10 @@ static int do_merge(struct commit *commit, const char *arg, int arg_len,
 		goto leave_merge;
 	}
 
+	write_message(oid_to_hex(&merge_commit->object.oid), GIT_SHA1_HEXSZ,
+		      git_path_merge_head(the_repository), 0);
+	write_message("no-ff", 5, git_path_merge_mode(the_repository), 0);
+
 	for (j = bases; j; j = j->next)
 		commit_list_insert(j->item, &reversed);
 	free_commit_list(bases);
diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index 1f08a33687..cc5646836f 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -396,7 +396,7 @@ test_expect_success 'with --autosquash and --exec' '
 	grep "G: +G" actual
 '
 
-test_expect_failure '--continue after resolving conflicts after a merge' '
+test_expect_success '--continue after resolving conflicts after a merge' '
 	git checkout -b already-has-g E &&
 	git cherry-pick E..G &&
 	test_commit H2 &&
-- 
gitgitgadget

[PATCH 3/5] rebase -i: include MERGE_HEAD into files to clean up

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

Every once in a while, the interactive rebase makes sure that no stale
files are lying around. These days, we need to include MERGE_HEAD into
that set of files, as the `merge` command will generate them.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/sequencer.c b/sequencer.c
index 7a9cd81afb..2f526390ac 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3459,6 +3459,7 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)
 			unlink(rebase_path_author_script());
 			unlink(rebase_path_stopped_sha());
 			unlink(rebase_path_amend());
+			unlink(git_path_merge_head(the_repository));
 			delete_ref(NULL, "REBASE_HEAD", NULL, REF_NO_DEREF);
 
 			if (item->command == TODO_BREAK)
@@ -3829,6 +3830,7 @@ static int commit_staged_changes(struct replay_opts *opts,
 			   opts, flags))
 		return error(_("could not commit staged changes."));
 	unlink(rebase_path_amend());
+	unlink(git_path_merge_head(the_repository));
 	if (final_fixup) {
 		unlink(rebase_path_fixup_msg());
 		unlink(rebase_path_squash_msg());
-- 
gitgitgadget

[PATCH 4/5] built-in rebase --skip/--abort: clean up stale .git/<name> files

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

The scripted version of the rebase used to execute `git reset --hard`
when skipping or aborting. When we ported this to C, we did update the
worktree and some reflogs, but we failed to imitate `git reset --hard`'s
behavior regarding files in .git/ such as MERGE_HEAD.

Let's address this oversight.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/rebase.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 0ee06aa363..017dce1b50 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -23,6 +23,7 @@
 #include "revision.h"
 #include "commit-reach.h"
 #include "rerere.h"
+#include "branch.h"
 
 static char const * const builtin_rebase_usage[] = {
 	N_("git rebase [-i] [options] [--exec <cmd>] [--onto <newbase>] "
@@ -1002,6 +1003,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 
 		if (reset_head(NULL, "reset", NULL, 0, NULL, NULL) < 0)
 			die(_("could not discard worktree changes"));
+		remove_branch_state();
 		if (read_basic_state(&options))
 			exit(1);
 		goto run_rebase;
@@ -1019,6 +1021,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			       options.head_name, 0, NULL, NULL) < 0)
 			die(_("could not move back to %s"),
 			    oid_to_hex(&options.orig_head));
+		remove_branch_state();
 		ret = finish_rebase(&options);
 		goto cleanup;
 	}
-- 
gitgitgadget

[PATCH 5/5] status: rebase and merge can be in progress at the same time

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

Since `git rebase -r` was introduced, that is possible. But our
machinery did not think that possible, and failed to say anything about
the rebase in progress when in the middle of a merge.

Let's work around that in the minimal fashion.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 wt-status.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/wt-status.c b/wt-status.c
index 187568a112..a24711374c 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1559,6 +1559,7 @@ void wt_status_get_state(struct wt_status_state *state,
 	struct object_id oid;
 
 	if (!stat(git_path_merge_head(the_repository), &st)) {
+		wt_status_check_rebase(NULL, state);
 		state->merge_in_progress = 1;
 	} else if (wt_status_check_rebase(NULL, state)) {
 		;		/* all set */
@@ -1583,9 +1584,13 @@ static void wt_longstatus_print_state(struct wt_status *s)
 	const char *state_color = color(WT_STATUS_HEADER, s);
 	struct wt_status_state *state = &s->state;
 
-	if (state->merge_in_progress)
+	if (state->merge_in_progress) {
+		if (state->rebase_interactive_in_progress) {
+			show_rebase_information(s, state_color);
+			fputs("\n", s->fp);
+		}
 		show_merge_in_progress(s, state_color);
-	else if (state->am_in_progress)
+	} else if (state->am_in_progress)
 		show_am_in_progress(s, state_color);
 	else if (state->rebase_in_progress || state->rebase_interactive_in_progress)
 		show_rebase_in_progress(s, state_color);
-- 
gitgitgadget

Re: [PATCH 3/5] rebase -i: include MERGE_HEAD into files to clean up

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> Every once in a while, the interactive rebase makes sure that no stale
> files are lying around. These days, we need to include MERGE_HEAD into
> that set of files, as the `merge` command will generate them.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  sequencer.c | 2 ++
>  1 file changed, 2 insertions(+)

Makes sense.

>
> diff --git a/sequencer.c b/sequencer.c
> index 7a9cd81afb..2f526390ac 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -3459,6 +3459,7 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)
>  			unlink(rebase_path_author_script());
>  			unlink(rebase_path_stopped_sha());
>  			unlink(rebase_path_amend());
> +			unlink(git_path_merge_head(the_repository));
>  			delete_ref(NULL, "REBASE_HEAD", NULL, REF_NO_DEREF);
>  
>  			if (item->command == TODO_BREAK)
> @@ -3829,6 +3830,7 @@ static int commit_staged_changes(struct replay_opts *opts,
>  			   opts, flags))
>  		return error(_("could not commit staged changes."));
>  	unlink(rebase_path_amend());
> +	unlink(git_path_merge_head(the_repository));
>  	if (final_fixup) {
>  		unlink(rebase_path_fixup_msg());
>  		unlink(rebase_path_squash_msg());

Re: [PATCH 4/5] built-in rebase --skip/--abort: clean up stale .git/<name> files

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> The scripted version of the rebase used to execute `git reset --hard`
> when skipping or aborting. When we ported this to C, we did update the
> worktree and some reflogs, but we failed to imitate `git reset --hard`'s
> behavior regarding files in .git/ such as MERGE_HEAD.
>
> Let's address this oversight.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  builtin/rebase.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 0ee06aa363..017dce1b50 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -23,6 +23,7 @@
>  #include "revision.h"
>  #include "commit-reach.h"
>  #include "rerere.h"
> +#include "branch.h"
>  
>  static char const * const builtin_rebase_usage[] = {
>  	N_("git rebase [-i] [options] [--exec <cmd>] [--onto <newbase>] "
> @@ -1002,6 +1003,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  
>  		if (reset_head(NULL, "reset", NULL, 0, NULL, NULL) < 0)
>  			die(_("could not discard worktree changes"));
> +		remove_branch_state();
>  		if (read_basic_state(&options))
>  			exit(1);
>  		goto run_rebase;
> @@ -1019,6 +1021,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  			       options.head_name, 0, NULL, NULL) < 0)
>  			die(_("could not move back to %s"),
>  			    oid_to_hex(&options.orig_head));
> +		remove_branch_state();

Hmph.  Among 5 or so callsites of reset_head(), only these two
places need it, so reset_head() is clearly not a substitute for
"reset --hard HEAD", and it sometimes used to switch branches or
something?  Perhaps we may need to rename it to avoid confusion but
it can wait until we actually decide to make it externally
available.  Until then, it's OK as-is, I would think.

>  		ret = finish_rebase(&options);
>  		goto cleanup;
>  	}

Re: [PATCH 1/5] rebase -r: demonstrate bug with conflicting merges

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> When calling `merge` on a branch that has already been merged, that
> `merge` is skipped quietly, but currently a MERGE_HEAD file is being
> left behind and will then be grabbed by the next `pick` (that did
> not want to create a *merge* commit).
>
> Demonstrate this.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  t/t3430-rebase-merges.sh | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)

For a trivially small change/fix like this, it is OK and even
preferrable to make 1+2 a single step, as applying t/ part only to
try to see the breakage (or "am"ing everything and then "diff |
apply -R" the part outside t/ for the same purpose) is easy enough.

Because the patch 2 with your method ends up showing only the test
set-up part in the context by changing _failure to _success, without
showing what end-user visible breakage the step fixed, which usually
comes near the end of the added test piece.  A single patch that
gives tests that ought to succeed would not force the readers to
switch between patches 1 and 2 while reading the fix.

Of course, the above would not apply for a more involved case where
the actual fix to the code needs to span multiple patches.

Thanks.

> diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
> index aa7bfc88ec..1f08a33687 100755
> --- a/t/t3430-rebase-merges.sh
> +++ b/t/t3430-rebase-merges.sh
> @@ -396,4 +396,20 @@ test_expect_success 'with --autosquash and --exec' '
>  	grep "G: +G" actual
>  '
>  
> +test_expect_failure '--continue after resolving conflicts after a merge' '
> +	git checkout -b already-has-g E &&
> +	git cherry-pick E..G &&
> +	test_commit H2 &&
> +
> +	git checkout -b conflicts-in-merge H &&
> +	test_commit H2 H2.t conflicts H2-conflict &&
> +	test_must_fail git rebase -r already-has-g &&
> +	grep conflicts H2.t &&

Is this making sure that the above test_must_fail succeeded because
of a conflict and not due to any other failure?  I would have used
"ls-files -u H2.t" to see if the index is unmerged, which probably
is a more direct way to test what this is trying to test, but if we
are in the conflicted state, the one side of << == >> has this
string (the other has "H2" in it, presumably?), so in practice this
should be good enough.

> +	echo resolved >H2.t &&
> +	git add -u &&

and we resolve to continue.

> +	git rebase --continue &&
> +	test_must_fail git rev-parse --verify HEAD^2 &&

Even if we made an octopus by mistake, the above will catch it,
which is good.

> +	test_path_is_missing .git/MERGE_HEAD
> +'
> +
>  test_done

And from the proposed log message, I am reading that the last two
things (i.e. resulting tip is a child with a single parent and there
is no leftover MERGE_HEAD file) fail without the fix.  

This is enough material to convince me or anybody that the bug is
worth fixing.  Thanks for being careful noticing a glitch during
your real (and otherwise unrelated to the bug) work and following
through.

Re: [PATCH 1/5] rebase -r: demonstrate bug with conflicting merges

[Johannes Schindelin]

Hi Junio,

On Tue, 13 Nov 2018, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
> 
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > When calling `merge` on a branch that has already been merged, that
> > `merge` is skipped quietly, but currently a MERGE_HEAD file is being
> > left behind and will then be grabbed by the next `pick` (that did
> > not want to create a *merge* commit).
> >
> > Demonstrate this.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  t/t3430-rebase-merges.sh | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> 
> For a trivially small change/fix like this, it is OK and even
> preferrable to make 1+2 a single step, as applying t/ part only to
> try to see the breakage (or "am"ing everything and then "diff |
> apply -R" the part outside t/ for the same purpose) is easy enough.

I disagree. It helps both development and porting to different branches to
be able to cherry-pick the regression test individually. Please do not ask
me to violate this hard-learned principle.

> Because the patch 2 with your method ends up showing only the test
> set-up part in the context by changing _failure to _success, without
> showing what end-user visible breakage the step fixed, which usually
> comes near the end of the added test piece.  A single patch that
> gives tests that ought to succeed would not force the readers to
> switch between patches 1 and 2 while reading the fix.

That is why I put in a verbose commit message, so that you do not have to
guess. And even the test title talks about this.

Seriously, I am very much opposed to changing the patches in the direction
you suggested. In my mind, they would make the story substantially worse.

Thank you for your review,
Dscho

> 
> Of course, the above would not apply for a more involved case where
> the actual fix to the code needs to span multiple patches.
> 
> Thanks.
> 
> > diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
> > index aa7bfc88ec..1f08a33687 100755
> > --- a/t/t3430-rebase-merges.sh
> > +++ b/t/t3430-rebase-merges.sh
> > @@ -396,4 +396,20 @@ test_expect_success 'with --autosquash and --exec' '
> >  	grep "G: +G" actual
> >  '
> >  
> > +test_expect_failure '--continue after resolving conflicts after a merge' '
> > +	git checkout -b already-has-g E &&
> > +	git cherry-pick E..G &&
> > +	test_commit H2 &&
> > +
> > +	git checkout -b conflicts-in-merge H &&
> > +	test_commit H2 H2.t conflicts H2-conflict &&
> > +	test_must_fail git rebase -r already-has-g &&
> > +	grep conflicts H2.t &&
> 
> Is this making sure that the above test_must_fail succeeded because
> of a conflict and not due to any other failure?  I would have used
> "ls-files -u H2.t" to see if the index is unmerged, which probably
> is a more direct way to test what this is trying to test, but if we
> are in the conflicted state, the one side of << == >> has this
> string (the other has "H2" in it, presumably?), so in practice this
> should be good enough.
> 
> > +	echo resolved >H2.t &&
> > +	git add -u &&
> 
> and we resolve to continue.
> 
> > +	git rebase --continue &&
> > +	test_must_fail git rev-parse --verify HEAD^2 &&
> 
> Even if we made an octopus by mistake, the above will catch it,
> which is good.
> 
> > +	test_path_is_missing .git/MERGE_HEAD
> > +'
> > +
> >  test_done
> 
> And from the proposed log message, I am reading that the last two
> things (i.e. resulting tip is a child with a single parent and there
> is no leftover MERGE_HEAD file) fail without the fix.  
> 
> This is enough material to convince me or anybody that the bug is
> worth fixing.  Thanks for being careful noticing a glitch during
> your real (and otherwise unrelated to the bug) work and following
> through.
> 

Re: [PATCH 4/5] built-in rebase --skip/--abort: clean up stale .git/<name> files

[Johannes Schindelin]

Hi Junio,

On Tue, 13 Nov 2018, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
> 
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > The scripted version of the rebase used to execute `git reset --hard`
> > when skipping or aborting. When we ported this to C, we did update the
> > worktree and some reflogs, but we failed to imitate `git reset --hard`'s
> > behavior regarding files in .git/ such as MERGE_HEAD.
> >
> > Let's address this oversight.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  builtin/rebase.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/builtin/rebase.c b/builtin/rebase.c
> > index 0ee06aa363..017dce1b50 100644
> > --- a/builtin/rebase.c
> > +++ b/builtin/rebase.c
> > @@ -23,6 +23,7 @@
> >  #include "revision.h"
> >  #include "commit-reach.h"
> >  #include "rerere.h"
> > +#include "branch.h"
> >  
> >  static char const * const builtin_rebase_usage[] = {
> >  	N_("git rebase [-i] [options] [--exec <cmd>] [--onto <newbase>] "
> > @@ -1002,6 +1003,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> >  
> >  		if (reset_head(NULL, "reset", NULL, 0, NULL, NULL) < 0)
> >  			die(_("could not discard worktree changes"));
> > +		remove_branch_state();
> >  		if (read_basic_state(&options))
> >  			exit(1);
> >  		goto run_rebase;
> > @@ -1019,6 +1021,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> >  			       options.head_name, 0, NULL, NULL) < 0)
> >  			die(_("could not move back to %s"),
> >  			    oid_to_hex(&options.orig_head));
> > +		remove_branch_state();
> 
> Hmph.  Among 5 or so callsites of reset_head(), only these two
> places need it, so reset_head() is clearly not a substitute for
> "reset --hard HEAD", and it sometimes used to switch branches or
> something?

Indeed. There is also the `git reset --hard` call in the scripted
version's autostash code path. But that definitely does not need to remove
the branch state, as it is not recovering from a merge or cherry-pick or
revert.

> Perhaps we may need to rename it to avoid confusion but it can wait
> until we actually decide to make it externally available.  Until then,
> it's OK as-is, I would think.

Okay.

Ciao,
Dscho

> 
> >  		ret = finish_rebase(&options);
> >  		goto cleanup;
> >  	}
> 

Re: [PATCH 1/5] rebase -r: demonstrate bug with conflicting merges

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> For a trivially small change/fix like this, it is OK and even
>> preferrable to make 1+2 a single step, as applying t/ part only to
>> try to see the breakage (or "am"ing everything and then "diff |
>> apply -R" the part outside t/ for the same purpose) is easy enough.
>
> I disagree. It helps both development and porting to different branches to
> be able to cherry-pick the regression test individually. Please do not ask
> me to violate this hard-learned principle.

A trivially small change/fix like this, by definition (of "trivial"
and "small" ness), it is trivial to develop and port to different
branches a single patch, and test with just one half (either the
test part or the code-change part) of the change reversed, to ensure
that the codebase is broken without the code-change and to
demonstrate that the code-change does fix the problem revealed by
the test change.  And "porting" by cherry-picking a single patch is
always easier than two patch series.

So you may disagree all you want in your project, but do not make
reviewer's lives unnecessarily harder in this project.

Thanks.

Re: [PATCH 1/5] rebase -r: demonstrate bug with conflicting merges

[Johannes Schindelin]

Hi Junio,

On Tue, 13 Nov 2018, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> For a trivially small change/fix like this, it is OK and even
> >> preferrable to make 1+2 a single step, as applying t/ part only to
> >> try to see the breakage (or "am"ing everything and then "diff |
> >> apply -R" the part outside t/ for the same purpose) is easy enough.
> >
> > I disagree. It helps both development and porting to different branches to
> > be able to cherry-pick the regression test individually. Please do not ask
> > me to violate this hard-learned principle.
> 
> A trivially small change/fix like this, by definition (of "trivial"
> and "small" ness), it is trivial to develop and port to different
> branches a single patch, and test with just one half (either the
> test part or the code-change part) of the change reversed, to ensure
> that the codebase is broken without the code-change and to
> demonstrate that the code-change does fix the problem revealed by
> the test change.  And "porting" by cherry-picking a single patch is
> always easier than two patch series.
> 
> So you may disagree all you want in your project, but do not make
> reviewer's lives unnecessarily harder in this project.

You misunderstand. In this case it is crucial to read the regression test
first. The fix does not make much sense before one understands the
condition under which the order of the code statements matters.

By trying to force me to smoosh them together, you are trying to force me
to combine them in one patch where you would read the (now seemingly
non-sensical) fix first, and only then the test.

That's just really unhelpful. If I were a reviewer, I would want it
presented in the way it *was* presented. I firmly believe most reviewers
would.

Ciao,
Dscho

Re: [PATCH 1/5] rebase -r: demonstrate bug with conflicting merges

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> You misunderstand. In this case it is crucial to read the regression test
> first. The fix does not make much sense before one understands the
> condition under which the order of the code statements matters.

I am not sure what you mean.  It sounds as if you want to use
diff-orderfile to present change for t/ before changes to other
files are presented in format-patch output to help readers, and I
think that may make sense for certain cases.  It may even include
this case.

But that is not incompatible with "avoid showing the patch that
updates the code to fix breakages separately, ending up with showing
the changes to t/ that are mostly about s/_failure/_success/ and
readers are forced to go back to the previous patch to remind
themselves what the broken scenario was about; by keeping it in a
single patch, the readers can get the tests in one piece".