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

[Sergey Organov]

Junio C Hamano  writes:

> Sergey Organov  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

[Junio C Hamano]

Sergey Organov  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 
> ---
>  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(>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(>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(>object.oid), opts->mainline);
>   else
>   parent = commit->parents->item;

[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 
---
 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(>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(>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(>object.oid), opts->mainline);
else
parent = commit->parents->item;
 
-- 
2.10.0.1.g57b01a3

[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 
---
 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(>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(>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(>object.oid), opts->mainline);
else
parent = commit->parents->item;
 
-- 
2.10.0.1.g57b01a3