[PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified

[PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified

[Sergey Organov]

When cherry-picking multiple commits, it's impossible to have both
merge- and non-merge commits on the same command-line. Not specifying
'-m 1' results in cherry-pick refusing to handle merge commits, while
specifying '-m 1' fails on non-merge commits.

This patch allows '-m 1' for non-merge commits. Besides, as mainline is
always the only parent for a non-merge commit, it made little sense to
disable it in the first place.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 sequencer.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 1ce6326..2393bdf 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1543,9 +1543,11 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
 			return error(_("commit %s does not have parent %d"),
 				oid_to_hex(&commit->object.oid), opts->mainline);
 		parent = p->item;
-	} else if (0 < opts->mainline)
-		return error(_("mainline was specified but commit %s is not a merge."),
-			oid_to_hex(&commit->object.oid));
+	} else if (1 < opts->mainline)
+		/* Non-first parent explicitly specified as mainline for
+		 * non-merge commit */
+		return error(_("commit %s does not have parent %d"),
+			     oid_to_hex(&commit->object.oid), opts->mainline);
 	else
 		parent = commit->parents->item;
 
-- 
2.10.0.1.g57b01a3

Re: [PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com> writes:

> When cherry-picking multiple commits, it's impossible to have both
> merge- and non-merge commits on the same command-line. Not specifying
> '-m 1' results in cherry-pick refusing to handle merge commits, while
> specifying '-m 1' fails on non-merge commits.

Allowing "-m1" even when picking a single parent commit, because
the 1-st parent is defined for such a commit, makes sense, espeially
when running a cherry-pick on a range, exactly for the above reason.
It is slightly less so when cherry-picking a single commit, but not
by a large margin.

I think the original reasoning for requiring "-m $n" not present,
especially because cherry-pick was originally for replaying only a
single commit, was because it would lead somebody to propose that
the command should behave as if -m1 is always given (and when trying
to cherry-pick a merge relative to its second parent, give -m2 to
override it), which in turn encourage the 'first-parent is special'
world-view from the tool-side.  IOW, "The worldview to treat the
first-parent chain specially is correct, because Git has many
features to work with that worldview conveniently" was something we
wanted to avoid; rather "Such and such workflows benefit from
treating the first-parent chain specially, so let's add features to
do so" was we wanted to do, and of course, back then cherry-pick
that allows mixture of merges and single-parent commits to be
picked, which would have made the need to do something like this
patch does felt greater, did not exist.

Now, it appears, at least to me, that the world pretty much accepted
that the first-parent worldview is often very convenient and worth
supporting by the tool, so the next logical step might be to set
opts->mainline to 1 by default (and allow an explicit "-m $n" from
the command line to override it).  But that should happen after this
patch lands---it is logically a separate step, I would think.

> This patch allows '-m 1' for non-merge commits. Besides, as mainline is
> always the only parent for a non-merge commit, it made little sense to
> disable it in the first place.
>
> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> ---
>  sequencer.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)

>
> diff --git a/sequencer.c b/sequencer.c
> index 1ce6326..2393bdf 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1543,9 +1543,11 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
>  			return error(_("commit %s does not have parent %d"),
>  				oid_to_hex(&commit->object.oid), opts->mainline);
>  		parent = p->item;
> -	} else if (0 < opts->mainline)
> -		return error(_("mainline was specified but commit %s is not a merge."),
> -			oid_to_hex(&commit->object.oid));
> +	} else if (1 < opts->mainline)
> +		/* Non-first parent explicitly specified as mainline for
> +		 * non-merge commit */

Style.

	/*
	 * Our multi-line comments are to be
	 * formatted like this.
	 */

> +		return error(_("commit %s does not have parent %d"),
> +			     oid_to_hex(&commit->object.oid), opts->mainline);
>  	else
>  		parent = commit->parents->item;

Re: [PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified

[Sergey Organov]

Junio C Hamano <gitster@pobox.com> writes:

> Sergey Organov <sorganov@gmail.com> writes:
>
>> When cherry-picking multiple commits, it's impossible to have both
>> merge- and non-merge commits on the same command-line. Not specifying
>> '-m 1' results in cherry-pick refusing to handle merge commits, while
>> specifying '-m 1' fails on non-merge commits.
>
> Allowing "-m1" even when picking a single parent commit, because
> the 1-st parent is defined for such a commit, makes sense, espeially
> when running a cherry-pick on a range, exactly for the above reason.
> It is slightly less so when cherry-picking a single commit, but not
> by a large margin.

[...]

>> +	} else if (1 < opts->mainline)
>> +		/* Non-first parent explicitly specified as mainline for
>> +		 * non-merge commit */
>
> Style.
>
> 	/*
> 	 * Our multi-line comments are to be
> 	 * formatted like this.
> 	 */

Ah, sorry for that. Will you fix it for me?

Thanks!

-- Sergey

Re: [PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified

[Sergey Organov]

Junio C Hamano <gitster@pobox.com> writes:
> Sergey Organov <sorganov@gmail.com> writes:
>
>> When cherry-picking multiple commits, it's impossible to have both
>> merge- and non-merge commits on the same command-line. Not specifying
>> '-m 1' results in cherry-pick refusing to handle merge commits, while
>> specifying '-m 1' fails on non-merge commits.
>

[...]

> Now, it appears, at least to me, that the world pretty much accepted
> that the first-parent worldview is often very convenient and worth
> supporting by the tool, so the next logical step might be to set
> opts->mainline to 1 by default (and allow an explicit "-m $n" from
> the command line to override it).  But that should happen after this
> patch lands---it is logically a separate step, I would think.
>

[...]

>> +	} else if (1 < opts->mainline)
>> +		/* Non-first parent explicitly specified as mainline for
>> +		 * non-merge commit */
>
> Style.
>
> 	/*
> 	 * Our multi-line comments are to be
> 	 * formatted like this.
> 	 */

Comment style fixed:

-- 8< --

When cherry-picking multiple commits, it's impossible to have both
merge- and non-merge commits on the same command-line. Not specifying
'-m 1' results in cherry-pick refusing to handle merge commits, while
specifying '-m 1' fails on non-merge commits.

This patch allows '-m 1' for non-merge commits. Besides, as mainline is
always the only parent for a non-merge commit, it made little sense to
disable it in the first place.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 sequencer.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index e1a4dd1..d0fd61b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1766,9 +1766,13 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
 			return error(_("commit %s does not have parent %d"),
 				oid_to_hex(&commit->object.oid), opts->mainline);
 		parent = p->item;
-	} else if (0 < opts->mainline)
-		return error(_("mainline was specified but commit %s is not a merge."),
-			oid_to_hex(&commit->object.oid));
+	} else if (1 < opts->mainline)
+		/*
+		 *  Non-first parent explicitly specified as mainline for
+		 *  non-merge commit
+		 */
+		return error(_("commit %s does not have parent %d"),
+			     oid_to_hex(&commit->object.oid), opts->mainline);
 	else
 		parent = commit->parents->item;
 
-- 
2.10.0.1.g57b01a3

Re: [PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com> writes:

> When cherry-picking multiple commits, it's impossible to have both
> merge- and non-merge commits on the same command-line. Not specifying
> '-m 1' results in cherry-pick refusing to handle merge commits, while
> specifying '-m 1' fails on non-merge commits.
>
> This patch allows '-m 1' for non-merge commits. Besides, as mainline is
> always the only parent for a non-merge commit, it made little sense to
> disable it in the first place.


The feature to give a range to cherry-pick came much much later in
7e2bfd3f ("revert: allow cherry-picking more than one commit",
2010-06-02) that first appeared in v1.7.2.  The feature to allow
picking a merge commit came in 7791ecbc ("revert/cherry-pick: work
on merge commits as well", 2007-10-23), first appeared in v1.5.4.

In the original context to pick a single commit, it made perfect
sense to avoid mistakes by blindly passing '-m 1' to non-merge
commit.  It may be fair to say that we failed to reconsider what to
do with '-m 1' when we did 7e2bfd3f, but it is utterly an unfair 
history revisionism to say that it made little sense to disable it
in the first place.

The change to the code itself looks sane, but applying this patch
alone will break existing tests whose expectations must be updated,
and this new behaviour must be protected by a new test (or two) so
that we won't accidentally stop accepting "-m 1" for a single-parent
commit.

Thanks.

> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> ---
>  sequencer.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index e1a4dd1..d0fd61b 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1766,9 +1766,13 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
>  			return error(_("commit %s does not have parent %d"),
>  				oid_to_hex(&commit->object.oid), opts->mainline);
>  		parent = p->item;
> -	} else if (0 < opts->mainline)
> -		return error(_("mainline was specified but commit %s is not a merge."),
> -			oid_to_hex(&commit->object.oid));
> +	} else if (1 < opts->mainline)
> +		/*
> +		 *  Non-first parent explicitly specified as mainline for
> +		 *  non-merge commit
> +		 */
> +		return error(_("commit %s does not have parent %d"),
> +			     oid_to_hex(&commit->object.oid), opts->mainline);
>  	else
>  		parent = commit->parents->item;

Re: [PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified

[Sergey Organov]

Junio C Hamano <gitster@pobox.com> writes:

> Sergey Organov <sorganov@gmail.com> writes:
>
>> When cherry-picking multiple commits, it's impossible to have both
>> merge- and non-merge commits on the same command-line. Not specifying
>> '-m 1' results in cherry-pick refusing to handle merge commits, while
>> specifying '-m 1' fails on non-merge commits.
>>
>> This patch allows '-m 1' for non-merge commits. Besides, as mainline is
>> always the only parent for a non-merge commit, it made little sense to
>> disable it in the first place.
>
> In the original context to pick a single commit, it made perfect
> sense to avoid mistakes by blindly passing '-m 1' to non-merge
> commit.
>
> It may be fair to say that we failed to reconsider what to
> do with '-m 1' when we did 7e2bfd3f, but it is utterly an unfair 
> history revisionism to say that it made little sense to disable it
> in the first place.

In fact I had zero intention on any revisionism. Now I see I should have
said "makes little sense" in present tense to avoid touching deep
historical issues, sorry!

Something like:

"This patch allows '-m 1' for non-merge commits. As mainline is always
the only parent for a non-merge commit, it makes little sense to disable
it."

sounds OK?

>
> The change to the code itself looks sane, but applying this patch
> alone will break existing tests whose expectations must be updated,
> and this new behaviour must be protected by a new test (or two) so
> that we won't accidentally stop accepting "-m 1" for a single-parent
> commit.

I fixed most of the tests, but

"t3510/4: cherry-pick persists opts correctly"

is an offender for me. It looks like it [ab]uses current "-m 1" behavior
just to stop in the middle of the sequence, and I'm not sure how to fix
it most suitably.

Thanks!

-- 
Sergey

Re: [PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified

[Sergey Organov]

Sergey Organov <sorganov@gmail.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Sergey Organov <sorganov@gmail.com> writes:
>>

[...]

>>
>> The change to the code itself looks sane, but applying this patch
>> alone will break existing tests whose expectations must be updated,
>> and this new behaviour must be protected by a new test (or two) so
>> that we won't accidentally stop accepting "-m 1" for a single-parent
>> commit.
>
> I fixed most of the tests, but
>
> "t3510/4: cherry-pick persists opts correctly"
>
> is an offender for me. It looks like it [ab]uses current "-m 1" behavior
> just to stop in the middle of the sequence, and I'm not sure how to fix
> it most suitably.

I came up with the following as a preparatory change. Looks acceptable?

-- 8< --

    t3510: stop using '-m 1' to force failure mid-sequence of cherry-picks
    
    We are going to allow 'git cherry-pick -m 1' for non-merge commits, so
    this method to force failure will stop to work.
    
    Use '-m 4' instead as it's very unlikely we will ever have such an
    octopus in this test setup.

	Modified   t/t3510-cherry-pick-sequence.sh
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index c84eeef..a873cf4 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -61,7 +61,8 @@ test_expect_success 'cherry-pick mid-cherry-pick-sequence' '
 
 test_expect_success 'cherry-pick persists opts correctly' '
 	pristine_detach initial &&
-	test_expect_code 128 git cherry-pick -s -m 1 --strategy=recursive -X patience -X ours initial..anotherpick &&
+	m=4 &&
+	test_expect_code 128 git cherry-pick -s -m $m --strategy=recursive -X patience -X ours initial..anotherpick &&
 	test_path_is_dir .git/sequencer &&
 	test_path_is_file .git/sequencer/head &&
 	test_path_is_file .git/sequencer/todo &&
@@ -69,7 +70,7 @@ test_expect_success 'cherry-pick persists opts correctly' '
 	echo "true" >expect &&
 	git config --file=.git/sequencer/opts --get-all options.signoff >actual &&
 	test_cmp expect actual &&
-	echo "1" >expect &&
+	echo "$m" >expect &&
 	git config --file=.git/sequencer/opts --get-all options.mainline >actual &&
 	test_cmp expect actual &&
 	echo "recursive" >expect &&

-- 8< --

-- 
Sergey

Re: [PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com> writes:

> I came up with the following as a preparatory change. Looks acceptable?
>
> -- 8< --
>
>     t3510: stop using '-m 1' to force failure mid-sequence of cherry-picks
>     
>     We are going to allow 'git cherry-pick -m 1' for non-merge commits, so
>     this method to force failure will stop to work.
>     
>     Use '-m 4' instead as it's very unlikely we will ever have such an
>     octopus in this test setup.

Yeah, that is a good approach.  Thanks for coming up with it.

I agree that it also is a good idea to use a variable to avoid
repeating "4" (and risking the two uses of the constant drifting
apart), but I find a single letter variable 'm' a bit too bland and
not descriptive enough.  Perhaps spell it out as mainline=4,
possibly with a comment why that is not a more-commonly-seen number
like "1"?

	# to make sure that the session to cherry-pick a sequence
	# gets interrupted, use a high-enough number that is larger
	# than the number of parents of any commit
	mainline=4 &&

or something.

> 	Modified   t/t3510-cherry-pick-sequence.sh
> diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
> index c84eeef..a873cf4 100755
> --- a/t/t3510-cherry-pick-sequence.sh
> +++ b/t/t3510-cherry-pick-sequence.sh
> @@ -61,7 +61,8 @@ test_expect_success 'cherry-pick mid-cherry-pick-sequence' '
>  
>  test_expect_success 'cherry-pick persists opts correctly' '
>  	pristine_detach initial &&
> -	test_expect_code 128 git cherry-pick -s -m 1 --strategy=recursive -X patience -X ours initial..anotherpick &&
> +	m=4 &&
> +	test_expect_code 128 git cherry-pick -s -m $m --strategy=recursive -X patience -X ours initial..anotherpick &&
>  	test_path_is_dir .git/sequencer &&
>  	test_path_is_file .git/sequencer/head &&
>  	test_path_is_file .git/sequencer/todo &&
> @@ -69,7 +70,7 @@ test_expect_success 'cherry-pick persists opts correctly' '
>  	echo "true" >expect &&
>  	git config --file=.git/sequencer/opts --get-all options.signoff >actual &&
>  	test_cmp expect actual &&
> -	echo "1" >expect &&
> +	echo "$m" >expect &&
>  	git config --file=.git/sequencer/opts --get-all options.mainline >actual &&
>  	test_cmp expect actual &&
>  	echo "recursive" >expect &&
>
> -- 8< --

[PATCH v2 0/4] Allow 'cherry-pick -m 1' for non-merge commits

[Sergey Organov]

When cherry-picking multiple commits, it's impossible to have both
merge- and non-merge commits on the same command-line. Not specifying
'-m 1' results in cherry-pick refusing to handle merge commits, while
specifying '-m 1' fails on non-merge commits.

This patch series allow '-m 1' for non-merge commits as well as fixes
relevant tests in accordance.

It also opens the way to making '-m 1' the default, that would be
inline with the trends to assume first parent to be the mainline in
most workflows.

Sergey Organov (4):
  t3510: stop using '-m 1' to force failure mid-sequence of cherry-picks
  cherry-pick: do not error on non-merge commits when '-m 1' is
    specified
  t3502: validate '-m 1' argument is now accepted for non-merge commits
  t3506: validate '-m 1 -ff' is now accepted for non-merge commits

 sequencer.c                     | 10 +++++++---
 t/t3502-cherry-pick-merge.sh    | 12 ++++++------
 t/t3506-cherry-pick-ff.sh       |  6 +++---
 t/t3510-cherry-pick-sequence.sh |  8 ++++++--
 4 files changed, 22 insertions(+), 14 deletions(-)

-- 
2.10.0.1.g57b01a3

[PATCH v2 2/4] cherry-pick: do not error on non-merge commits when '-m 1' is specified

[Sergey Organov]

When cherry-picking multiple commits, it's impossible to have both
merge- and non-merge commits on the same command-line. Not specifying
'-m 1' results in cherry-pick refusing to handle merge commits, while
specifying '-m 1' fails on non-merge commits.

This patch allows '-m 1' for non-merge commits. As mainline is always
the only parent for a non-merge commit, it makes little sense to
disable it.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 sequencer.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index e1a4dd1..d0fd61b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1766,9 +1766,13 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
 			return error(_("commit %s does not have parent %d"),
 				oid_to_hex(&commit->object.oid), opts->mainline);
 		parent = p->item;
-	} else if (0 < opts->mainline)
-		return error(_("mainline was specified but commit %s is not a merge."),
-			oid_to_hex(&commit->object.oid));
+	} else if (1 < opts->mainline)
+		/*
+		 *  Non-first parent explicitly specified as mainline for
+		 *  non-merge commit
+		 */
+		return error(_("commit %s does not have parent %d"),
+			     oid_to_hex(&commit->object.oid), opts->mainline);
 	else
 		parent = commit->parents->item;
 
-- 
2.10.0.1.g57b01a3

[PATCH v2 1/4] t3510: stop using '-m 1' to force failure mid-sequence of cherry-picks

[Sergey Organov]

We are going to allow 'git cherry-pick -m 1' for non-merge commits, so
this method to force failure will stop to work.

Use '-m 4' instead as it's very unlikely we will ever have such an
octopus in this test setup.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 t/t3510-cherry-pick-sequence.sh | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index c84eeef..941d502 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -61,7 +61,11 @@ test_expect_success 'cherry-pick mid-cherry-pick-sequence' '
 
 test_expect_success 'cherry-pick persists opts correctly' '
 	pristine_detach initial &&
-	test_expect_code 128 git cherry-pick -s -m 1 --strategy=recursive -X patience -X ours initial..anotherpick &&
+	# to make sure that the session to cherry-pick a sequence
+	# gets interrupted, use a high-enough number that is larger
+	# than the number of parents of any commit we have created
+	mainline=4 &&
+	test_expect_code 128 git cherry-pick -s -m $mainline --strategy=recursive -X patience -X ours initial..anotherpick &&
 	test_path_is_dir .git/sequencer &&
 	test_path_is_file .git/sequencer/head &&
 	test_path_is_file .git/sequencer/todo &&
@@ -69,7 +73,7 @@ test_expect_success 'cherry-pick persists opts correctly' '
 	echo "true" >expect &&
 	git config --file=.git/sequencer/opts --get-all options.signoff >actual &&
 	test_cmp expect actual &&
-	echo "1" >expect &&
+	echo "$mainline" >expect &&
 	git config --file=.git/sequencer/opts --get-all options.mainline >actual &&
 	test_cmp expect actual &&
 	echo "recursive" >expect &&
-- 
2.10.0.1.g57b01a3

[PATCH v2 4/4] t3506: validate '-m 1 -ff' is now accepted for non-merge commits

[Sergey Organov]

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 t/t3506-cherry-pick-ff.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t3506-cherry-pick-ff.sh b/t/t3506-cherry-pick-ff.sh
index fb889ac..127dd00 100755
--- a/t/t3506-cherry-pick-ff.sh
+++ b/t/t3506-cherry-pick-ff.sh
@@ -64,10 +64,10 @@ test_expect_success 'merge setup' '
 	git checkout -b new A
 '
 
-test_expect_success 'cherry-pick a non-merge with --ff and -m should fail' '
+test_expect_success 'cherry-pick explicit first parent of a non-merge with --ff' '
 	git reset --hard A -- &&
-	test_must_fail git cherry-pick --ff -m 1 B &&
-	git diff --exit-code A --
+	git cherry-pick --ff -m 1 B &&
+	git diff --exit-code C --
 '
 
 test_expect_success 'cherry pick a merge with --ff but without -m should fail' '
-- 
2.10.0.1.g57b01a3

[PATCH v2 3/4] t3502: validate '-m 1' argument is now accepted for non-merge commits

[Sergey Organov]

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 t/t3502-cherry-pick-merge.sh | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/t/t3502-cherry-pick-merge.sh b/t/t3502-cherry-pick-merge.sh
index b160271..3259bd5 100755
--- a/t/t3502-cherry-pick-merge.sh
+++ b/t/t3502-cherry-pick-merge.sh
@@ -40,12 +40,12 @@ test_expect_success 'cherry-pick -m complains of bogus numbers' '
 	test_expect_code 129 git cherry-pick -m 0 b
 '
 
-test_expect_success 'cherry-pick a non-merge with -m should fail' '
+test_expect_success 'cherry-pick explicit first parent of a non-merge' '
 
 	git reset --hard &&
 	git checkout a^0 &&
-	test_expect_code 128 git cherry-pick -m 1 b &&
-	git diff --exit-code a --
+	git cherry-pick -m 1 b &&
+	git diff --exit-code c --
 
 '
 
@@ -84,12 +84,12 @@ test_expect_success 'cherry pick a merge relative to nonexistent parent should f
 
 '
 
-test_expect_success 'revert a non-merge with -m should fail' '
+test_expect_success 'revert explicit first parent of a non-merge' '
 
 	git reset --hard &&
 	git checkout c^0 &&
-	test_must_fail git revert -m 1 b &&
-	git diff --exit-code c
+	git revert -m 1 b &&
+	git diff --exit-code a
 
 '
 
-- 
2.10.0.1.g57b01a3

Re: [PATCH v2 3/4] t3502: validate '-m 1' argument is now accepted for non-merge commits

[SZEDER Gábor]

On Fri, Dec 14, 2018 at 07:53:51AM +0300, Sergey Organov wrote:
> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> ---
>  t/t3502-cherry-pick-merge.sh | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/t/t3502-cherry-pick-merge.sh b/t/t3502-cherry-pick-merge.sh
> index b160271..3259bd5 100755
> --- a/t/t3502-cherry-pick-merge.sh
> +++ b/t/t3502-cherry-pick-merge.sh
> @@ -40,12 +40,12 @@ test_expect_success 'cherry-pick -m complains of bogus numbers' '
>  	test_expect_code 129 git cherry-pick -m 0 b
>  '
>  
> -test_expect_success 'cherry-pick a non-merge with -m should fail' '
> +test_expect_success 'cherry-pick explicit first parent of a non-merge' '
>  
>  	git reset --hard &&
>  	git checkout a^0 &&
> -	test_expect_code 128 git cherry-pick -m 1 b &&
> -	git diff --exit-code a --
> +	git cherry-pick -m 1 b &&
> +	git diff --exit-code c --
>  
>  '
>  
> @@ -84,12 +84,12 @@ test_expect_success 'cherry pick a merge relative to nonexistent parent should f
>  
>  '
>  
> -test_expect_success 'revert a non-merge with -m should fail' '
> +test_expect_success 'revert explicit first parent of a non-merge' '
>  
>  	git reset --hard &&
>  	git checkout c^0 &&
> -	test_must_fail git revert -m 1 b &&
> -	git diff --exit-code c
> +	git revert -m 1 b &&
> +	git diff --exit-code a

You need disambiguaion here, otherwise this test fails on
case-insensitive file systems:

  ++git diff --exit-code a
  fatal: ambiguous argument 'a': both revision and filename
  Use '--' to separate paths from revisions, like this:
  'git <command> [<revision>...] -- [<file>...]'
  error: last command exited with $?=128
  not ok 8 - revert explicit first parent of a non-merge

>  
>  '
>  
> -- 
> 2.10.0.1.g57b01a3
> 

Re: [PATCH v2 3/4] t3502: validate '-m 1' argument is now accepted for non-merge commits

[Sergey Organov]

SZEDER Gábor <szeder.dev@gmail.com> writes:

> On Fri, Dec 14, 2018 at 07:53:51AM +0300, Sergey Organov wrote:
>> Signed-off-by: Sergey Organov <sorganov@gmail.com>

[...]

>>  
>> @@ -84,12 +84,12 @@ test_expect_success 'cherry pick a merge relative to nonexistent parent should f
>>  
>>  '
>>  
>> -test_expect_success 'revert a non-merge with -m should fail' '
>> +test_expect_success 'revert explicit first parent of a non-merge' '
>>  
>>  	git reset --hard &&
>>  	git checkout c^0 &&
>> -	test_must_fail git revert -m 1 b &&
>> -	git diff --exit-code c
>> +	git revert -m 1 b &&
>> +	git diff --exit-code a
>
> You need disambiguaion here, otherwise this test fails on
> case-insensitive file systems:
>
>   ++git diff --exit-code a
>   fatal: ambiguous argument 'a': both revision and filename
>   Use '--' to separate paths from revisions, like this:
>   'git <command> [<revision>...] -- [<file>...]'
>   error: last command exited with $?=128
>   not ok 8 - revert explicit first parent of a non-merge

Good catch, -- thanks a lot!

-- Sergey

Re: [PATCH v2 0/4] Allow 'cherry-pick -m 1' for non-merge commits

[Sergey Organov, Sergey Organov]

Hi Junio,

What's the status of these patches?

Thanks,

-- Sergey

Sergey Organov <sorganov@gmail.com> writes:
> When cherry-picking multiple commits, it's impossible to have both
> merge- and non-merge commits on the same command-line. Not specifying
> '-m 1' results in cherry-pick refusing to handle merge commits, while
> specifying '-m 1' fails on non-merge commits.
>
> This patch series allow '-m 1' for non-merge commits as well as fixes
> relevant tests in accordance.
>
> It also opens the way to making '-m 1' the default, that would be
> inline with the trends to assume first parent to be the mainline in
> most workflows.
>
> Sergey Organov (4):
>   t3510: stop using '-m 1' to force failure mid-sequence of cherry-picks
>   cherry-pick: do not error on non-merge commits when '-m 1' is
>     specified
>   t3502: validate '-m 1' argument is now accepted for non-merge commits
>   t3506: validate '-m 1 -ff' is now accepted for non-merge commits
>
>  sequencer.c                     | 10 +++++++---
>  t/t3502-cherry-pick-merge.sh    | 12 ++++++------
>  t/t3506-cherry-pick-ff.sh       |  6 +++---
>  t/t3510-cherry-pick-sequence.sh |  8 ++++++--
>  4 files changed, 22 insertions(+), 14 deletions(-)

Re: [PATCH v2 0/4] Allow 'cherry-pick -m 1' for non-merge commits

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com>, Sergey Organov
<sorganov@gmail.com> writes:

> Hi Junio,
>
> What's the status of these patches?

The status of these patches is "Just updated on the list", as far as
I am concerned, and its cover letter would have described what's
improved relative to the previous round better than whatever answer
I could give here ;-)

I checked the overall diff between the previous round and the result
of applying all four patches to find out that the updates are only
to the tests.  The changes in the code (still) looked correct.




diff --git a/t/t3502-cherry-pick-merge.sh b/t/t3502-cherry-pick-merge.sh
index b1602718f8..3259bd59eb 100755
--- a/t/t3502-cherry-pick-merge.sh
+++ b/t/t3502-cherry-pick-merge.sh
@@ -40,12 +40,12 @@ test_expect_success 'cherry-pick -m complains of bogus numbers' '
 	test_expect_code 129 git cherry-pick -m 0 b
 '
 
-test_expect_success 'cherry-pick a non-merge with -m should fail' '
+test_expect_success 'cherry-pick explicit first parent of a non-merge' '
 
 	git reset --hard &&
 	git checkout a^0 &&
-	test_expect_code 128 git cherry-pick -m 1 b &&
-	git diff --exit-code a --
+	git cherry-pick -m 1 b &&
+	git diff --exit-code c --
 
 '
 
@@ -84,12 +84,12 @@ test_expect_success 'cherry pick a merge relative to nonexistent parent should f
 
 '
 
-test_expect_success 'revert a non-merge with -m should fail' '
+test_expect_success 'revert explicit first parent of a non-merge' '
 
 	git reset --hard &&
 	git checkout c^0 &&
-	test_must_fail git revert -m 1 b &&
-	git diff --exit-code c
+	git revert -m 1 b &&
+	git diff --exit-code a
 
 '
 
diff --git a/t/t3506-cherry-pick-ff.sh b/t/t3506-cherry-pick-ff.sh
index fb889ac6f0..127dd0082f 100755
--- a/t/t3506-cherry-pick-ff.sh
+++ b/t/t3506-cherry-pick-ff.sh
@@ -64,10 +64,10 @@ test_expect_success 'merge setup' '
 	git checkout -b new A
 '
 
-test_expect_success 'cherry-pick a non-merge with --ff and -m should fail' '
+test_expect_success 'cherry-pick explicit first parent of a non-merge with --ff' '
 	git reset --hard A -- &&
-	test_must_fail git cherry-pick --ff -m 1 B &&
-	git diff --exit-code A --
+	git cherry-pick --ff -m 1 B &&
+	git diff --exit-code C --
 '
 
 test_expect_success 'cherry pick a merge with --ff but without -m should fail' '
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index c84eeefdc9..941d5026da 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -61,7 +61,11 @@ test_expect_success 'cherry-pick mid-cherry-pick-sequence' '
 
 test_expect_success 'cherry-pick persists opts correctly' '
 	pristine_detach initial &&
-	test_expect_code 128 git cherry-pick -s -m 1 --strategy=recursive -X patience -X ours initial..anotherpick &&
+	# to make sure that the session to cherry-pick a sequence
+	# gets interrupted, use a high-enough number that is larger
+	# than the number of parents of any commit we have created
+	mainline=4 &&
+	test_expect_code 128 git cherry-pick -s -m $mainline --strategy=recursive -X patience -X ours initial..anotherpick &&
 	test_path_is_dir .git/sequencer &&
 	test_path_is_file .git/sequencer/head &&
 	test_path_is_file .git/sequencer/todo &&
@@ -69,7 +73,7 @@ test_expect_success 'cherry-pick persists opts correctly' '
 	echo "true" >expect &&
 	git config --file=.git/sequencer/opts --get-all options.signoff >actual &&
 	test_cmp expect actual &&
-	echo "1" >expect &&
+	echo "$mainline" >expect &&
 	git config --file=.git/sequencer/opts --get-all options.mainline >actual &&
 	test_cmp expect actual &&
 	echo "recursive" >expect &&

Re: [PATCH v2 0/4] Allow 'cherry-pick -m 1' for non-merge commits

[Sergey Organov]

Junio C Hamano <gitster@pobox.com> writes:

> Sergey Organov <sorganov@gmail.com>, Sergey Organov
> <sorganov@gmail.com> writes:
>
>> Hi Junio,
>>
>> What's the status of these patches?
>
> The status of these patches is "Just updated on the list", as far as
> I am concerned, and its cover letter would have described what's
> improved relative to the previous round better than whatever answer
> I could give here ;-)

Yeah, I need to be more thorough, thanks!

-- Sergey

[PATCH v3 0/4] Allow 'cherry-pick -m 1' for non-merge commits

[Sergey Organov]

Change from v2: t/t3502: disambiguation added to prevent failing on
case-insensitive file systems.

When cherry-picking multiple commits, it's impossible to have both
merge- and non-merge commits on the same command-line. Not specifying
'-m 1' results in cherry-pick refusing to handle merge commits, while
specifying '-m 1' fails on non-merge commits.

This patch series allow '-m 1' for non-merge commits as well as fixes
relevant tests in accordance.

It also opens the way to making '-m 1' the default, that would be
inline with the trends to assume first parent to be the mainline in
most workflows.

Sergey Organov (4):
  t3510: stop using '-m 1' to force failure mid-sequence of cherry-picks
  cherry-pick: do not error on non-merge commits when '-m 1' is
    specified
  t3502: validate '-m 1' argument is now accepted for non-merge commits
  t3506: validate '-m 1 -ff' is now accepted for non-merge commits

 sequencer.c                     | 10 +++++++---
 t/t3502-cherry-pick-merge.sh    | 12 ++++++------
 t/t3506-cherry-pick-ff.sh       |  6 +++---
 t/t3510-cherry-pick-sequence.sh |  8 ++++++--
 4 files changed, 22 insertions(+), 14 deletions(-)

-- 
2.10.0.1.g57b01a3

[PATCH v3 2/4] cherry-pick: do not error on non-merge commits when '-m 1' is specified

[Sergey Organov]

When cherry-picking multiple commits, it's impossible to have both
merge- and non-merge commits on the same command-line. Not specifying
'-m 1' results in cherry-pick refusing to handle merge commits, while
specifying '-m 1' fails on non-merge commits.

This patch allows '-m 1' for non-merge commits. As mainline is always
the only parent for a non-merge commit, it makes little sense to
disable it.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 sequencer.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index e1a4dd1..d0fd61b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1766,9 +1766,13 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
 			return error(_("commit %s does not have parent %d"),
 				oid_to_hex(&commit->object.oid), opts->mainline);
 		parent = p->item;
-	} else if (0 < opts->mainline)
-		return error(_("mainline was specified but commit %s is not a merge."),
-			oid_to_hex(&commit->object.oid));
+	} else if (1 < opts->mainline)
+		/*
+		 *  Non-first parent explicitly specified as mainline for
+		 *  non-merge commit
+		 */
+		return error(_("commit %s does not have parent %d"),
+			     oid_to_hex(&commit->object.oid), opts->mainline);
 	else
 		parent = commit->parents->item;
 
-- 
2.10.0.1.g57b01a3

[PATCH v3 3/4] t3502: validate '-m 1' argument is now accepted for non-merge commits

[Sergey Organov]

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 t/t3502-cherry-pick-merge.sh | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/t/t3502-cherry-pick-merge.sh b/t/t3502-cherry-pick-merge.sh
index b160271..8b635a1 100755
--- a/t/t3502-cherry-pick-merge.sh
+++ b/t/t3502-cherry-pick-merge.sh
@@ -40,12 +40,12 @@ test_expect_success 'cherry-pick -m complains of bogus numbers' '
 	test_expect_code 129 git cherry-pick -m 0 b
 '
 
-test_expect_success 'cherry-pick a non-merge with -m should fail' '
+test_expect_success 'cherry-pick explicit first parent of a non-merge' '
 
 	git reset --hard &&
 	git checkout a^0 &&
-	test_expect_code 128 git cherry-pick -m 1 b &&
-	git diff --exit-code a --
+	git cherry-pick -m 1 b &&
+	git diff --exit-code c --
 
 '
 
@@ -84,12 +84,12 @@ test_expect_success 'cherry pick a merge relative to nonexistent parent should f
 
 '
 
-test_expect_success 'revert a non-merge with -m should fail' '
+test_expect_success 'revert explicit first parent of a non-merge' '
 
 	git reset --hard &&
 	git checkout c^0 &&
-	test_must_fail git revert -m 1 b &&
-	git diff --exit-code c
+	git revert -m 1 b &&
+	git diff --exit-code a --
 
 '
 
-- 
2.10.0.1.g57b01a3

[PATCH v3 1/4] t3510: stop using '-m 1' to force failure mid-sequence of cherry-picks

[Sergey Organov]

We are going to allow 'git cherry-pick -m 1' for non-merge commits, so
this method to force failure will stop to work.

Use '-m 4' instead as it's very unlikely we will ever have such an
octopus in this test setup.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 t/t3510-cherry-pick-sequence.sh | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index c84eeef..941d502 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -61,7 +61,11 @@ test_expect_success 'cherry-pick mid-cherry-pick-sequence' '
 
 test_expect_success 'cherry-pick persists opts correctly' '
 	pristine_detach initial &&
-	test_expect_code 128 git cherry-pick -s -m 1 --strategy=recursive -X patience -X ours initial..anotherpick &&
+	# to make sure that the session to cherry-pick a sequence
+	# gets interrupted, use a high-enough number that is larger
+	# than the number of parents of any commit we have created
+	mainline=4 &&
+	test_expect_code 128 git cherry-pick -s -m $mainline --strategy=recursive -X patience -X ours initial..anotherpick &&
 	test_path_is_dir .git/sequencer &&
 	test_path_is_file .git/sequencer/head &&
 	test_path_is_file .git/sequencer/todo &&
@@ -69,7 +73,7 @@ test_expect_success 'cherry-pick persists opts correctly' '
 	echo "true" >expect &&
 	git config --file=.git/sequencer/opts --get-all options.signoff >actual &&
 	test_cmp expect actual &&
-	echo "1" >expect &&
+	echo "$mainline" >expect &&
 	git config --file=.git/sequencer/opts --get-all options.mainline >actual &&
 	test_cmp expect actual &&
 	echo "recursive" >expect &&
-- 
2.10.0.1.g57b01a3

[PATCH v3 4/4] t3506: validate '-m 1 -ff' is now accepted for non-merge commits

[Sergey Organov]

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 t/t3506-cherry-pick-ff.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t3506-cherry-pick-ff.sh b/t/t3506-cherry-pick-ff.sh
index fb889ac..127dd00 100755
--- a/t/t3506-cherry-pick-ff.sh
+++ b/t/t3506-cherry-pick-ff.sh
@@ -64,10 +64,10 @@ test_expect_success 'merge setup' '
 	git checkout -b new A
 '
 
-test_expect_success 'cherry-pick a non-merge with --ff and -m should fail' '
+test_expect_success 'cherry-pick explicit first parent of a non-merge with --ff' '
 	git reset --hard A -- &&
-	test_must_fail git cherry-pick --ff -m 1 B &&
-	git diff --exit-code A --
+	git cherry-pick --ff -m 1 B &&
+	git diff --exit-code C --
 '
 
 test_expect_success 'cherry pick a merge with --ff but without -m should fail' '
-- 
2.10.0.1.g57b01a3

Re: [PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified

[Sergey Organov]

Hi Junio,

[It's been a while since this discussion, and recently I've got some
thoughts and questions about "first-parent" issues in general, below.]

Junio C Hamano <gitster@pobox.com> writes:

> Sergey Organov <sorganov@gmail.com> writes:
>
>> When cherry-picking multiple commits, it's impossible to have both
>> merge- and non-merge commits on the same command-line. Not specifying
>> '-m 1' results in cherry-pick refusing to handle merge commits, while
>> specifying '-m 1' fails on non-merge commits.
>
> Allowing "-m1" even when picking a single parent commit, because
> the 1-st parent is defined for such a commit, makes sense, espeially
> when running a cherry-pick on a range, exactly for the above reason.
> It is slightly less so when cherry-picking a single commit, but not
> by a large margin.
>
> I think the original reasoning for requiring "-m $n" not present,
> especially because cherry-pick was originally for replaying only a
> single commit, was because it would lead somebody to propose that
> the command should behave as if -m1 is always given (and when trying
> to cherry-pick a merge relative to its second parent, give -m2 to
> override it), which in turn encourage the 'first-parent is special'
> world-view from the tool-side.  IOW, "The worldview to treat the
> first-parent chain specially is correct, because Git has many
> features to work with that worldview conveniently" was something we
> wanted to avoid; rather "Such and such workflows benefit from
> treating the first-parent chain specially, so let's add features to
> do so" was we wanted to do, and of course, back then cherry-pick
> that allows mixture of merges and single-parent commits to be
> picked, which would have made the need to do something like this
> patch does felt greater, did not exist.
>
> Now, it appears, at least to me, that the world pretty much accepted
> that the first-parent worldview is often very convenient and worth
> supporting by the tool, so the next logical step might be to set
> opts->mainline to 1 by default (and allow an explicit "-m $n" from
> the command line to override it).  But that should happen after this
> patch lands---it is logically a separate step, I would think.

I think that "first-parent is special" is the way to go indeed for
porcelain, as it does make many thing easier and more convenient[*].

Is there a road-map already outlined in that direction, I wonder?

OTOH, for plumbing, it's rather keeping the original pure-DAG
"symmetrical merges" approach that seems to be the right thing to do.

[*] One example that immediately comes to mind is "git log -p" for a
merge commit. I doesn't currently (as of v2.10) show the first-parent
diff, for whatever reason. "git log -p -m --first-parent" is needed to
get the answer to most "obvious" question: what (merge) commit did to my
mainline? "git show" has its own issues.

-- Sergey

Re: [PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com> writes:

> I think that "first-parent is special" is the way to go indeed for
> porcelain, as it does make many thing easier and more convenient[*].

Perhaps.  However ...

> [*] One example that immediately comes to mind is "git log -p" for a
> merge commit. I doesn't currently (as of v2.10) show the first-parent
> diff, for whatever reason. "git log -p -m --first-parent" is needed to
> get the answer to most "obvious" question: what (merge) commit did to my
> mainline? "git show" has its own issues.

... this is very much deliberate and will remain so.

A single ball of wax "diff M^ M" for a merge commit is not always
what you would want, especially while viewing "git log -p" (without
"--first-parent").  The reason why the user does not explicitly say
"--first-parent" is because the user wants to even see the details
of individual steps of what happened on side branches.

Incidentally, in such an "I am interested in what happened in each
individual step" mode, the primary change that a merge by itself
does is better shown with "diff --cc M^ M", not "diff -p M^ M".
That is why "show" defaults to "--cc".

"git log -p --first-parent" that requires "-m" to show the single
ball of wax diff for a merge is a separate matter.  When the user
explicitly says "log --first-parent", it is a clear indication that
the user does not want to see individual steps of how side branches
built what each merge brings into the mainline.  From that point of
view, ever sice I introduced "--first-parent" traversal, I've been
wondering what the true downside would be if we turned "-m" on
automatically when these two options are used without "-m".

But it has not disturbed me deeply enough to bother looking into it
further.

Re: [PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified

[Jeff King]

On Wed, Mar 20, 2019 at 09:38:57AM +0900, Junio C Hamano wrote:

> "git log -p --first-parent" that requires "-m" to show the single
> ball of wax diff for a merge is a separate matter.  When the user
> explicitly says "log --first-parent", it is a clear indication that
> the user does not want to see individual steps of how side branches
> built what each merge brings into the mainline.  From that point of
> view, ever sice I introduced "--first-parent" traversal, I've been
> wondering what the true downside would be if we turned "-m" on
> automatically when these two options are used without "-m".

Sort of a drive-by two cents, but I have often wondered the same thing.
I cannot think of a time when I wanted "--first-parent" without "-m"
(unless I was not viewing diffs at all).

(And I agree with everything else you said :) ).

-Peff

Re: [PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified

[Sergey Organov]

Junio C Hamano <gitster@pobox.com> writes:

> Sergey Organov <sorganov@gmail.com> writes:
>
>> I think that "first-parent is special" is the way to go indeed for
>> porcelain, as it does make many thing easier and more convenient[*].
>
> Perhaps.  However ...
>
>> [*] One example that immediately comes to mind is "git log -p" for a
>> merge commit. I doesn't currently (as of v2.10) show the first-parent
>> diff, for whatever reason. "git log -p -m --first-parent" is needed to
>> get the answer to most "obvious" question: what (merge) commit did to my
>> mainline? "git show" has its own issues.
>
> ... this is very much deliberate and will remain so.

> A single ball of wax "diff M^ M" for a merge commit is not always
> what you would want, especially while viewing "git log -p" (without
> "--first-parent").

OK, point taken. Then it's an issue of suppressing (presumably huge)
parts of output for merge commits by default, and is only vaguely
relevant to the "first parent is special" trend that I intended to
discuss. So, let's leave in peace the "git log -p" for now, and let me
try it from different angle.

How about changing "git show -p M" to output "diff -p M^ M" rather than
"diff-tree --cc M" for merge commits? It's really surprising specifying
-p has no visible effect.

Also, is current output of "git log -m", being extremely confusing,
suitable for anything? Maybe consider to change it to output diff with
respect to the first parent only? Though it's then a pity "-m" lacks
argument here, similar to what it has in cherry-pick.

-- Sergey

Re: [PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified

[Jeff King]

On Mon, Mar 25, 2019 at 09:43:09AM +0300, Sergey Organov wrote:

> How about changing "git show -p M" to output "diff -p M^ M" rather than
> "diff-tree --cc M" for merge commits? It's really surprising specifying
> -p has no visible effect.

That's because "-p" is already the default, and the format selection is
orthogonal to the handling of merge commits. Providing "-m" would
actually override the "--cc" default (though "--first-parent -m" is
likely to be less noisy, per this discussion).

As far as defaults go, I dunno. The idea is that "--cc" would give you a
nice summary of what the merge _itself_ had to touch. I think that's
valuable, too. If we were starting from scratch, I think there could be
a discussion about whether one default is better than the other. But at
this point I have a hard time finding one so much obviously better than
the other to merit changing the behavior.

> Also, is current output of "git log -m", being extremely confusing,
> suitable for anything? Maybe consider to change it to output diff with
> respect to the first parent only? Though it's then a pity "-m" lacks
> argument here, similar to what it has in cherry-pick.

I've used "-m" without "--first-parent" sometimes in order to track down
mis-merges manually. It's not usually a big deal to say "--first-parent"
if that's what you want. But one thing I don't think is currently
possible is to ask only for the first-parent diff, but _not_ restrict
the actual traversal.

If that's what you mean by giving an argument to "-m", then yeah, I
think that would be a useful addition.

-Peff

Re: [PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified

[Elijah Newren]

On Tue, Mar 26, 2019 at 9:35 AM Jeff King <peff@peff.net> wrote:
>
> On Mon, Mar 25, 2019 at 09:43:09AM +0300, Sergey Organov wrote:
>
> > How about changing "git show -p M" to output "diff -p M^ M" rather than
> > "diff-tree --cc M" for merge commits? It's really surprising specifying
> > -p has no visible effect.
>
> That's because "-p" is already the default, and the format selection is
> orthogonal to the handling of merge commits. Providing "-m" would
> actually override the "--cc" default (though "--first-parent -m" is
> likely to be less noisy, per this discussion).
>
> As far as defaults go, I dunno. The idea is that "--cc" would give you a
> nice summary of what the merge _itself_ had to touch. I think that's
> valuable, too. If we were starting from scratch, I think there could be
> a discussion about whether one default is better than the other. But at
> this point I have a hard time finding one so much obviously better than
> the other to merit changing the behavior.

Indeed, some of us would view a first parent diff default for merges
as problematic.  However, I'd like to point out (or remind) that these
two options aren't the only ways you could view a merge.  Thomas
Rast's --remerge-diff[1] is another (even if not yet part of git.git).
Gerrit uses something similar-ish for its default way of showing a
merge.

[1] See e.g. https://bugs.chromium.org/p/git/issues/detail?id=12

Re: [PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified

[Jeff King]

On Tue, Mar 26, 2019 at 03:07:42PM -0700, Elijah Newren wrote:

> On Tue, Mar 26, 2019 at 9:35 AM Jeff King <peff@peff.net> wrote:
> >
> > On Mon, Mar 25, 2019 at 09:43:09AM +0300, Sergey Organov wrote:
> >
> > > How about changing "git show -p M" to output "diff -p M^ M" rather than
> > > "diff-tree --cc M" for merge commits? It's really surprising specifying
> > > -p has no visible effect.
> >
> > That's because "-p" is already the default, and the format selection is
> > orthogonal to the handling of merge commits. Providing "-m" would
> > actually override the "--cc" default (though "--first-parent -m" is
> > likely to be less noisy, per this discussion).
> >
> > As far as defaults go, I dunno. The idea is that "--cc" would give you a
> > nice summary of what the merge _itself_ had to touch. I think that's
> > valuable, too. If we were starting from scratch, I think there could be
> > a discussion about whether one default is better than the other. But at
> > this point I have a hard time finding one so much obviously better than
> > the other to merit changing the behavior.
> 
> Indeed, some of us would view a first parent diff default for merges
> as problematic.  However, I'd like to point out (or remind) that these
> two options aren't the only ways you could view a merge.  Thomas
> Rast's --remerge-diff[1] is another (even if not yet part of git.git).
> Gerrit uses something similar-ish for its default way of showing a
> merge.

Heh, I almost mentioned remerge-diff, but since it's not actually part
of Git, I didn't want to get into a tangent. But since you mention it,
yes, I actually find it quite a useful way of looking at the diff,
especially when I want to see what the person resolving the conflicts
actually _did_. The --cc combined diff is too eager to throw away hunks
that resolved purely to one side (which _most_ of the time is what you
want, but when you're hunting a possible error in the merge, it's quite
confusing).

How close is merge-recursive.c to actually doing a pure in-memory merge?

I seem to recall that was a (the?) sticking point for the original
remerge-diff.

-Peff

Re: [PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified

[Elijah Newren]

On Tue, Mar 26, 2019 at 3:20 PM Jeff King <peff@peff.net> wrote:
>
> On Tue, Mar 26, 2019 at 03:07:42PM -0700, Elijah Newren wrote:
>
> > On Tue, Mar 26, 2019 at 9:35 AM Jeff King <peff@peff.net> wrote:
> > >
> > > On Mon, Mar 25, 2019 at 09:43:09AM +0300, Sergey Organov wrote:
> > >
> > > > How about changing "git show -p M" to output "diff -p M^ M" rather than
> > > > "diff-tree --cc M" for merge commits? It's really surprising specifying
> > > > -p has no visible effect.
> > >
> > > That's because "-p" is already the default, and the format selection is
> > > orthogonal to the handling of merge commits. Providing "-m" would
> > > actually override the "--cc" default (though "--first-parent -m" is
> > > likely to be less noisy, per this discussion).
> > >
> > > As far as defaults go, I dunno. The idea is that "--cc" would give you a
> > > nice summary of what the merge _itself_ had to touch. I think that's
> > > valuable, too. If we were starting from scratch, I think there could be
> > > a discussion about whether one default is better than the other. But at
> > > this point I have a hard time finding one so much obviously better than
> > > the other to merit changing the behavior.
> >
> > Indeed, some of us would view a first parent diff default for merges
> > as problematic.  However, I'd like to point out (or remind) that these
> > two options aren't the only ways you could view a merge.  Thomas
> > Rast's --remerge-diff[1] is another (even if not yet part of git.git).
> > Gerrit uses something similar-ish for its default way of showing a
> > merge.
>
> Heh, I almost mentioned remerge-diff, but since it's not actually part
> of Git, I didn't want to get into a tangent. But since you mention it,
> yes, I actually find it quite a useful way of looking at the diff,
> especially when I want to see what the person resolving the conflicts
> actually _did_. The --cc combined diff is too eager to throw away hunks
> that resolved purely to one side (which _most_ of the time is what you
> want, but when you're hunting a possible error in the merge, it's quite
> confusing).
>
> How close is merge-recursive.c to actually doing a pure in-memory merge?
>
> I seem to recall that was a (the?) sticking point for the original
> remerge-diff.

Doing a pure in-memory merge is tied up with my overall
merge-recursive rewrite, but I haven't touched merge stuff in quite a
while now other than the recent
make-directory-rename-detection-be-a-conflict-by-default stuff.  I'm
hoping I can finish that soon (though I've struggled a bit to find the
time to do so), and finish out the filter-repo stuff, then I plan to
get back to merge stuff again.  A pure in-memory merge is near the top
of my priorities for the rewrite, so we'll get it...eventually.
(Maybe a Christmas present?)

I do think there's more than just that sticking point for the
remerge-diff, but the other things are on my radar too (a few slots
later on my todo list).

Re: [PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified

[Sergey Organov]

Jeff King <peff@peff.net> writes:

> On Mon, Mar 25, 2019 at 09:43:09AM +0300, Sergey Organov wrote:
>
>> How about changing "git show -p M" to output "diff -p M^ M" rather than
>> "diff-tree --cc M" for merge commits? It's really surprising specifying
>> -p has no visible effect.
>
> That's because "-p" is already the default, and the format selection is
> orthogonal to the handling of merge commits.

Seems to be more convoluted than that. If "-p" were simply default, then
"git show --raw" and "git show -p --raw" would output the same thing.
They don't.

That said, does "-p" select a format, or not? Should it? For me,
"--patch" sounds specific enough to expect it to select the format that
is most close to what "patch" utility is able to apply, that would still
be "diff -p M^ M" format, universally, be it merge commit or not.

> Providing "-m" would actually override the "--cc" default (though
> "--first-parent -m" is likely to be less noisy, per this discussion).

Right, but "--first-parent" (while accepted) even is not documented for
"git show", and it could be argued if history traversal option has any
sense for a command that shows separate commits.

Further, even in "git log", having the "--first-parent" change the way
diffs are output is, strictly speaking, violation of orthogonality (that
nobody seems to care about).

> As far as defaults go, I dunno.

I'm not after changing the default behavior. I'm rather after making the
whole system of options somewhat more logical, self consistent, and thus
less confusing.

> The idea is that "--cc" would give you a nice summary of what the
> merge _itself_ had to touch. I think that's valuable, too.

I'm not against "--cc" either, be it a default or not, even though
personally for me it's not very useful, as it shows how merge (the
operation) supposedly went, when I'm usually interested in how merge
(the resulting commit) affects the mainline, no matter how this result
has been achieved.

Another thought about "--cc" is that it's in fact a case of "merges are
symmetric" approach to the UI that is supposedly shifting to "mainline
is special" one.

> If we were starting from scratch, I think there could be
> a discussion about whether one default is better than the other. But at
> this point I have a hard time finding one so much obviously better than
> the other to merit changing the behavior.

I'm yet to figure what exactly the best set of options would be, even
personally for me, even in the "start from scratch" case, so, for now,
I'm basically just gathering relevant information and opinions.

>> Also, is current output of "git log -m", being extremely confusing,
>> suitable for anything? Maybe consider to change it to output diff with
>> respect to the first parent only? Though it's then a pity "-m" lacks
>> argument here, similar to what it has in cherry-pick.
>
> I've used "-m" without "--first-parent" sometimes in order to track down
> mis-merges manually.

Have you been interested specifically in diffs with respect to side
branches in these cases, I wonder, or did you actually look only at "-m
1" part of the whole "-m" output?

When I tried "-m", I found it rather difficult to even visually find the
margin between diffs to different parents, that confused me and forced
to resort to other methods. Besides, it didn't appear to me at that time
that there is "--first-parent" that might "help", so as I recall I ended
up using "diff -p M~1 M" for the merge in question.

> It's not usually a big deal to say "--first-parent" if that's what you
> want. But one thing I don't think is currently possible is to ask only
> for the first-parent diff, but _not_ restrict the actual traversal.

That's due to "--first-parent" breaking orthogonality. It should rather
only affect graph traversal, I'd expect.

Admittedly, it may imply some other option(s) for convenience (say, such
option could have been "-m 1", if it existed), but then there /must/
exist the option(s) it implies in the first place. Currently,
"--first-parent" behaves as if it implies some nonexistent option, so
the user has no way indeed to provide the latter without the former.

> If that's what you mean by giving an argument to "-m", then yeah, I
> think that would be a useful addition.

I thought that maybe the part of "-m" that outputs diffs to side
branch(es) is in fact useless feature when result is to be directly
consumed by human beings. Then, if we decide to change it to output diff
to single parent for porcelain command(s), it may be useful to be able
to explicitly ask for other parents than the default, the first one.

It also strikes me as inconsistent that "-m" in log/show on one hand,
and "-m" in cherry-pick on the other, being essentially the same thing,
are so different in appearance and description.

Unfortunately, adding an argument to "-m" bumps either into generic
evilness of optional arguments for options, or into backward
incompatibility (if the argument to "-m" becomes mandatory), so it
doesn't seem to be such a good thing to do after all.

-- Sergey

[PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified

[Sergey Organov]

When cherry-picking multiple commits, it's impossible to have both
merge- and non-merge commits on the same command-line. Not specifying
'-m 1' results in cherry-pick refusing to handle merge commits, while
specifying '-m 1' fails on non-merge commits.

This patch allows '-m 1' for non-merge commits. Besides, as mainline is
always the only parent for a non-merge commit, it made little sense to
disable it in the first place.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 sequencer.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 1ce6326..2393bdf 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1543,9 +1543,11 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
 			return error(_("commit %s does not have parent %d"),
 				oid_to_hex(&commit->object.oid), opts->mainline);
 		parent = p->item;
-	} else if (0 < opts->mainline)
-		return error(_("mainline was specified but commit %s is not a merge."),
-			oid_to_hex(&commit->object.oid));
+	} else if (1 < opts->mainline)
+		/* Non-first parent explicitly specified as mainline for
+		 * non-merge commit */
+		return error(_("commit %s does not have parent %d"),
+			     oid_to_hex(&commit->object.oid), opts->mainline);
 	else
 		parent = commit->parents->item;
 
-- 
2.10.0.1.g57b01a3