[3/3] commit: give correct advice for empty commit during a rebase

Commit Message

Derrick Stolee via GitGitGadget Oct. 22, 2019, 11:30 p.m. UTC

From: Johannes Schindelin <[e-mail]>

In dcb500dc16c (cherry-pick/revert: advise using --skip, 2019-07-02),
`git commit` learned to suggest to run `git cherry-pick --skip` when
trying to cherry-pick an empty patch.

However, it was overlooked that there are more conditions than just a
`git cherry-pick` when this advice is printed (which originally
suggested the neutral `git reset`): the same can happen during a rebase.

Let's suggest the correct command, even during a rebase.

While at it, we adjust more places in `builtin/commit.c` that
incorrectly assumed that the presence of a `CHERRY_PICK_HEAD` meant that
surely this must be a `cherry-pick` in progress.

Note: we take pains to handle the situation when a user runs a `git
cherry-pick` _during_ a rebase. This is quite valid (e.g. in an `exec`
line in an interactive rebase). On the other hand, it is not possible to
run a rebase during a cherry-pick, meaning: if both `rebase-merge/` and
`sequencer/` exist, we still want to advise to use `git cherry-pick
--skip`.

Signed-off-by: Johannes Schindelin <[e-mail]>
---
 builtin/commit.c       | 33 ++++++++++++++++++++++++---------
 t/t3403-rebase-skip.sh |  9 +++++++++
 2 files changed, 33 insertions(+), 9 deletions(-)

Comments

Junio C Hamano Oct. 23, 2019, 2:45 a.m. UTC | #1

"Johannes Schindelin via GitGitGadget" <[e-mail]>
writes:

> Note: we take pains to handle the situation when a user runs a `git
> cherry-pick` _during_ a rebase. This is quite valid (e.g. in an `exec`
> line in an interactive rebase). On the other hand, it is not possible to
> run a rebase during a cherry-pick, meaning: if both `rebase-merge/` and
> `sequencer/` exist, we still want to advise to use `git cherry-pick
> --skip`.

Good description of why the code does what it does, that future
readers will surely appreciate.  Nice.

Phillip Wood Oct. 24, 2019, 10:15 a.m. UTC | #2

Hi Dscho

On 23/10/2019 00:30, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <[e-mail]>
> 
> In dcb500dc16c (cherry-pick/revert: advise using --skip, 2019-07-02),
> `git commit` learned to suggest to run `git cherry-pick --skip` when
> trying to cherry-pick an empty patch.
> 
> However, it was overlooked that there are more conditions than just a
> `git cherry-pick` when this advice is printed (which originally
> suggested the neutral `git reset`): the same can happen during a rebase.
> 
> Let's suggest the correct command, even during a rebase.
> 
> While at it, we adjust more places in `builtin/commit.c` that
> incorrectly assumed that the presence of a `CHERRY_PICK_HEAD` meant that
> surely this must be a `cherry-pick` in progress.
> 
> Note: we take pains to handle the situation when a user runs a `git
> cherry-pick` _during_ a rebase. This is quite valid (e.g. in an `exec`
> line in an interactive rebase). On the other hand, it is not possible to
> run a rebase during a cherry-pick, meaning: if both `rebase-merge/` and
> `sequencer/` exist, we still want to advise to use `git cherry-pick
> --skip`.

Thanks for working on this. It's unfortunate that rebase does not remove
CHERRY_PICK_HEAD for empty commits as it does if the commit is not
empty. I think this is because 'rebase --continue' will skip an empty
commit so the user _has_ to run 'git commit' manually to keep it. If it
had been designed so that 'rebase --continue' kept the empty commit and
'rebase --skip' skipped it then we would not have this problem but it's
a bit late to worry about that now.

I don't this patch can distinguish between an empty cherry-pick
performed by the user while a rebase is in progress and an empty pick
performed by rebase as both create CHERRY_PICK_HEAD while
.git/rebase-merge exists. It seems to assume that CHERRY_PICK_HEAD was
created by rebase and prints advise based on that which may or may not
be the correct. I think we could distinguish the two by checking if
CHERRY_PICK_HEAD matches .git/rebase-merge/stopped-sha or REBASE_HEAD.

Best Wishes


Phillip

> 
> Signed-off-by: Johannes Schindelin <[e-mail]>
> ---
>  builtin/commit.c       | 33 ++++++++++++++++++++++++---------
>  t/t3403-rebase-skip.sh |  9 +++++++++
>  2 files changed, 33 insertions(+), 9 deletions(-)
> 
> diff --git a/builtin/commit.c b/builtin/commit.c
> index e588bc6ad3..2beae13620 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -59,6 +59,9 @@ N_("The previous cherry-pick is now empty, possibly due to conflict resolution.\
>  "    git commit --allow-empty\n"
>  "\n");
>  
> +static const char empty_rebase_advice[] =
> +N_("Otherwise, please use 'git rebase --skip'\n");
> +
>  static const char empty_cherry_pick_advice_single[] =
>  N_("Otherwise, please use 'git cherry-pick --skip'\n");
>  
> @@ -122,7 +125,7 @@ static enum commit_msg_cleanup_mode cleanup_mode;
>  static const char *cleanup_arg;
>  
>  static enum commit_whence whence;
> -static int sequencer_in_use;
> +static int sequencer_in_use, rebase_in_progress;
>  static int use_editor = 1, include_status = 1;
>  static int have_option_m;
>  static struct strbuf message = STRBUF_INIT;
> @@ -183,6 +186,8 @@ static void determine_whence(struct wt_status *s)
>  		whence = FROM_CHERRY_PICK;
>  		if (file_exists(git_path_seq_dir()))
>  			sequencer_in_use = 1;
> +		if (file_exists(git_path_rebase_merge_dir()))
> +			rebase_in_progress = 1;
>  	}
>  	else
>  		whence = FROM_COMMIT;
> @@ -453,8 +458,11 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
>  	if (whence != FROM_COMMIT) {
>  		if (whence == FROM_MERGE)
>  			die(_("cannot do a partial commit during a merge."));
> -		else if (whence == FROM_CHERRY_PICK)
> +		else if (whence == FROM_CHERRY_PICK) {
> +			if (rebase_in_progress && !sequencer_in_use)
> +				die(_("cannot do a partial commit during a rebase."));
>  			die(_("cannot do a partial commit during a cherry-pick."));
> +		}
>  	}
>  
>  	if (list_paths(&partial, !current_head ? NULL : "HEAD", &pathspec))
> @@ -950,10 +958,12 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>  			fputs(_(empty_amend_advice), stderr);
>  		else if (whence == FROM_CHERRY_PICK) {
>  			fputs(_(empty_cherry_pick_advice), stderr);
> -			if (!sequencer_in_use)
> -				fputs(_(empty_cherry_pick_advice_single), stderr);
> -			else
> +			if (sequencer_in_use)
>  				fputs(_(empty_cherry_pick_advice_multi), stderr);
> +			else if (rebase_in_progress)
> +				fputs(_(empty_rebase_advice), stderr);
> +			else
> +				fputs(_(empty_cherry_pick_advice_single), stderr);
>  		}
>  		return 0;
>  	}
> @@ -1156,8 +1166,11 @@ static int parse_and_validate_options(int argc, const char *argv[],
>  	if (amend && whence != FROM_COMMIT) {
>  		if (whence == FROM_MERGE)
>  			die(_("You are in the middle of a merge -- cannot amend."));
> -		else if (whence == FROM_CHERRY_PICK)
> +		else if (whence == FROM_CHERRY_PICK) {
> +			if (rebase_in_progress && !sequencer_in_use)
> +				die(_("You are in the middle of a rebase -- cannot amend."));
>  			die(_("You are in the middle of a cherry-pick -- cannot amend."));
> +		}
>  	}
>  	if (fixup_message && squash_message)
>  		die(_("Options --squash and --fixup cannot be used together"));
> @@ -1628,9 +1641,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>  			reduce_heads_replace(&parents);
>  	} else {
>  		if (!reflog_msg)
> -			reflog_msg = (whence == FROM_CHERRY_PICK)
> -					? "commit (cherry-pick)"
> -					: "commit";
> +			reflog_msg = (whence != FROM_CHERRY_PICK)
> +					? "commit"
> +					: rebase_in_progress && !sequencer_in_use
> +					? "commit (rebase)"
> +					: "commit (cherry-pick)";
>  		commit_list_insert(current_head, &parents);
>  	}
>  
> diff --git a/t/t3403-rebase-skip.sh b/t/t3403-rebase-skip.sh
> index 1f5122b632..77b03ac49f 100755
> --- a/t/t3403-rebase-skip.sh
> +++ b/t/t3403-rebase-skip.sh
> @@ -76,4 +76,13 @@ test_expect_success 'moved back to branch correctly' '
>  
>  test_debug 'gitk --all & sleep 1'
>  
> +test_expect_success 'correct advice upon empty commit' '
> +	git checkout -b rebase-skip &&
> +	test_commit a1 &&
> +	test_tick &&
> +	git commit --amend -m amended --no-edit &&
> +	test_must_fail git rebase -m --onto a1 HEAD^ 2>err &&
> +	test_i18ngrep "git rebase --skip" err
> +'
> +
>  test_done
>

Johannes Schindelin Oct. 25, 2019, 11:48 a.m. UTC | #3

Hi Phillip,

On Thu, 24 Oct 2019, Phillip Wood wrote:

> On 23/10/2019 00:30, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <[e-mail]>
> >
> > In dcb500dc16c (cherry-pick/revert: advise using --skip, 2019-07-02),
> > `git commit` learned to suggest to run `git cherry-pick --skip` when
> > trying to cherry-pick an empty patch.
> >
> > However, it was overlooked that there are more conditions than just a
> > `git cherry-pick` when this advice is printed (which originally
> > suggested the neutral `git reset`): the same can happen during a rebase.
> >
> > Let's suggest the correct command, even during a rebase.
> >
> > While at it, we adjust more places in `builtin/commit.c` that
> > incorrectly assumed that the presence of a `CHERRY_PICK_HEAD` meant that
> > surely this must be a `cherry-pick` in progress.
> >
> > Note: we take pains to handle the situation when a user runs a `git
> > cherry-pick` _during_ a rebase. This is quite valid (e.g. in an `exec`
> > line in an interactive rebase). On the other hand, it is not possible to
> > run a rebase during a cherry-pick, meaning: if both `rebase-merge/` and
> > `sequencer/` exist, we still want to advise to use `git cherry-pick
> > --skip`.
>
> Thanks for working on this. It's unfortunate that rebase does not remove
> CHERRY_PICK_HEAD for empty commits as it does if the commit is not
> empty.

Oh, so that's what it is that bites me all the time? I frequently run
interactive rebases and sometimes mess up authorship (taking authorship
of patches that I did not, in fact, author). I guess what happens is
that I `git commit` in the middle of a rebase that was interrupted by
merge conflicts.

So I would actually like to see the _exact opposite_ of what you want to
see: I want to keep `CHERRY_PICK_HEAD` even in the non-empty case.

> I think this is because 'rebase --continue' will skip an empty commit
> so the user _has_ to run 'git commit' manually to keep it. If it had
> been designed so that 'rebase --continue' kept the empty commit and
> 'rebase --skip' skipped it then we would not have this problem but
> it's a bit late to worry about that now.

True.

> I don't this patch can distinguish between an empty cherry-pick
> performed by the user while a rebase is in progress and an empty pick
> performed by rebase as both create CHERRY_PICK_HEAD while
> .git/rebase-merge exists. It seems to assume that CHERRY_PICK_HEAD was
> created by rebase and prints advise based on that which may or may not
> be the correct. I think we could distinguish the two by checking if
> CHERRY_PICK_HEAD matches .git/rebase-merge/stopped-sha or REBASE_HEAD.

I guess we could, but then, I would rather worry about that in the next
cycle. In this cycle, I would rather fix the common case, which is that
a `git rebase -i` fails and tells me to `git cherry-pick --skip` instead
of `git rebase --skip`.

And even if I performed a `git cherry-pick` during a `git rebase` and
the result would be an empty commit, I'd rather be told to `git rebase
--skip` to continue...

But if you feel strongly that this should be fixed differently, I'll
gladly leave it to you ;-)

Ciao,
Dscho

>
> Best Wishes
>
>
> Phillip
>
> >
> > Signed-off-by: Johannes Schindelin <[e-mail]>
> > ---
> >  builtin/commit.c       | 33 ++++++++++++++++++++++++---------
> >  t/t3403-rebase-skip.sh |  9 +++++++++
> >  2 files changed, 33 insertions(+), 9 deletions(-)
> >
> > diff --git a/builtin/commit.c b/builtin/commit.c
> > index e588bc6ad3..2beae13620 100644
> > --- a/builtin/commit.c
> > +++ b/builtin/commit.c
> > @@ -59,6 +59,9 @@ N_("The previous cherry-pick is now empty, possibly due to conflict resolution.\
> >  "    git commit --allow-empty\n"
> >  "\n");
> >
> > +static const char empty_rebase_advice[] =
> > +N_("Otherwise, please use 'git rebase --skip'\n");
> > +
> >  static const char empty_cherry_pick_advice_single[] =
> >  N_("Otherwise, please use 'git cherry-pick --skip'\n");
> >
> > @@ -122,7 +125,7 @@ static enum commit_msg_cleanup_mode cleanup_mode;
> >  static const char *cleanup_arg;
> >
> >  static enum commit_whence whence;
> > -static int sequencer_in_use;
> > +static int sequencer_in_use, rebase_in_progress;
> >  static int use_editor = 1, include_status = 1;
> >  static int have_option_m;
> >  static struct strbuf message = STRBUF_INIT;
> > @@ -183,6 +186,8 @@ static void determine_whence(struct wt_status *s)
> >  		whence = FROM_CHERRY_PICK;
> >  		if (file_exists(git_path_seq_dir()))
> >  			sequencer_in_use = 1;
> > +		if (file_exists(git_path_rebase_merge_dir()))
> > +			rebase_in_progress = 1;
> >  	}
> >  	else
> >  		whence = FROM_COMMIT;
> > @@ -453,8 +458,11 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
> >  	if (whence != FROM_COMMIT) {
> >  		if (whence == FROM_MERGE)
> >  			die(_("cannot do a partial commit during a merge."));
> > -		else if (whence == FROM_CHERRY_PICK)
> > +		else if (whence == FROM_CHERRY_PICK) {
> > +			if (rebase_in_progress && !sequencer_in_use)
> > +				die(_("cannot do a partial commit during a rebase."));
> >  			die(_("cannot do a partial commit during a cherry-pick."));
> > +		}
> >  	}
> >
> >  	if (list_paths(&partial, !current_head ? NULL : "HEAD", &pathspec))
> > @@ -950,10 +958,12 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
> >  			fputs(_(empty_amend_advice), stderr);
> >  		else if (whence == FROM_CHERRY_PICK) {
> >  			fputs(_(empty_cherry_pick_advice), stderr);
> > -			if (!sequencer_in_use)
> > -				fputs(_(empty_cherry_pick_advice_single), stderr);
> > -			else
> > +			if (sequencer_in_use)
> >  				fputs(_(empty_cherry_pick_advice_multi), stderr);
> > +			else if (rebase_in_progress)
> > +				fputs(_(empty_rebase_advice), stderr);
> > +			else
> > +				fputs(_(empty_cherry_pick_advice_single), stderr);
> >  		}
> >  		return 0;
> >  	}
> > @@ -1156,8 +1166,11 @@ static int parse_and_validate_options(int argc, const char *argv[],
> >  	if (amend && whence != FROM_COMMIT) {
> >  		if (whence == FROM_MERGE)
> >  			die(_("You are in the middle of a merge -- cannot amend."));
> > -		else if (whence == FROM_CHERRY_PICK)
> > +		else if (whence == FROM_CHERRY_PICK) {
> > +			if (rebase_in_progress && !sequencer_in_use)
> > +				die(_("You are in the middle of a rebase -- cannot amend."));
> >  			die(_("You are in the middle of a cherry-pick -- cannot amend."));
> > +		}
> >  	}
> >  	if (fixup_message && squash_message)
> >  		die(_("Options --squash and --fixup cannot be used together"));
> > @@ -1628,9 +1641,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
> >  			reduce_heads_replace(&parents);
> >  	} else {
> >  		if (!reflog_msg)
> > -			reflog_msg = (whence == FROM_CHERRY_PICK)
> > -					? "commit (cherry-pick)"
> > -					: "commit";
> > +			reflog_msg = (whence != FROM_CHERRY_PICK)
> > +					? "commit"
> > +					: rebase_in_progress && !sequencer_in_use
> > +					? "commit (rebase)"
> > +					: "commit (cherry-pick)";
> >  		commit_list_insert(current_head, &parents);
> >  	}
> >
> > diff --git a/t/t3403-rebase-skip.sh b/t/t3403-rebase-skip.sh
> > index 1f5122b632..77b03ac49f 100755
> > --- a/t/t3403-rebase-skip.sh
> > +++ b/t/t3403-rebase-skip.sh
> > @@ -76,4 +76,13 @@ test_expect_success 'moved back to branch correctly' '
> >
> >  test_debug 'gitk --all & sleep 1'
> >
> > +test_expect_success 'correct advice upon empty commit' '
> > +	git checkout -b rebase-skip &&
> > +	test_commit a1 &&
> > +	test_tick &&
> > +	git commit --amend -m amended --no-edit &&
> > +	test_must_fail git rebase -m --onto a1 HEAD^ 2>err &&
> > +	test_i18ngrep "git rebase --skip" err
> > +'
> > +
> >  test_done
> >
>
>

Phillip Wood Oct. 25, 2019, 2:01 p.m. UTC | #4

Hi Dscho

On 25/10/2019 12:48, Johannes Schindelin wrote:
> Hi Phillip,
> 
> On Thu, 24 Oct 2019, Phillip Wood wrote:
> 
>> On 23/10/2019 00:30, Johannes Schindelin via GitGitGadget wrote:
>>> From: Johannes Schindelin <[e-mail]>
>>>
>>> In dcb500dc16c (cherry-pick/revert: advise using --skip, 2019-07-02),
>>> `git commit` learned to suggest to run `git cherry-pick --skip` when
>>> trying to cherry-pick an empty patch.
>>>
>>> However, it was overlooked that there are more conditions than just a
>>> `git cherry-pick` when this advice is printed (which originally
>>> suggested the neutral `git reset`): the same can happen during a rebase.
>>>
>>> Let's suggest the correct command, even during a rebase.
>>>
>>> While at it, we adjust more places in `builtin/commit.c` that
>>> incorrectly assumed that the presence of a `CHERRY_PICK_HEAD` meant that
>>> surely this must be a `cherry-pick` in progress.
>>>
>>> Note: we take pains to handle the situation when a user runs a `git
>>> cherry-pick` _during_ a rebase. This is quite valid (e.g. in an `exec`
>>> line in an interactive rebase). On the other hand, it is not possible to
>>> run a rebase during a cherry-pick, meaning: if both `rebase-merge/` and
>>> `sequencer/` exist, we still want to advise to use `git cherry-pick
>>> --skip`.
>>
>> Thanks for working on this. It's unfortunate that rebase does not remove
>> CHERRY_PICK_HEAD for empty commits as it does if the commit is not
>> empty.
> 
> Oh, so that's what it is that bites me all the time? I frequently run
> interactive rebases and sometimes mess up authorship (taking authorship
> of patches that I did not, in fact, author). I guess what happens is
> that I `git commit` in the middle of a rebase that was interrupted by
> merge conflicts.

I used to do that a lot, I eventually trained myself not to commit after 
conflicts during a rebase and leave it to `rebase --continue` but it is 
annoying that it does not work

> So I would actually like to see the _exact opposite_ of what you want to
> see: I want to keep `CHERRY_PICK_HEAD` even in the non-empty case.

I don't necessarily object - having it consistent one way or the other 
would be a distinct improvement, it has been removed when there are 
conflicts since it's inception [1] (That's v2 of the series which 
started removing CHERRY_PICK_HEAD in the case of rebase conflicts. I 
couldn't find any discussion of v1 that recommended that change though). 
There is also another thread [2].

[1] 
https://public-inbox.org/git/[e-mail]/
[2] 
https://public-inbox.org/git/CAMP44s1EAwHjQ7S2ArLvhNg5qkR05DRJ70tQmP8sXYdOP=[e-mail]/ 



>> I think this is because 'rebase --continue' will skip an empty commit
>> so the user _has_ to run 'git commit' manually to keep it. If it had
>> been designed so that 'rebase --continue' kept the empty commit and
>> 'rebase --skip' skipped it then we would not have this problem but
>> it's a bit late to worry about that now.
> 
> True.
> 
>> I don't this patch can distinguish between an empty cherry-pick
>> performed by the user while a rebase is in progress and an empty pick
>> performed by rebase as both create CHERRY_PICK_HEAD while
>> .git/rebase-merge exists. It seems to assume that CHERRY_PICK_HEAD was
>> created by rebase and prints advise based on that which may or may not
>> be the correct. I think we could distinguish the two by checking if
>> CHERRY_PICK_HEAD matches .git/rebase-merge/stopped-sha or REBASE_HEAD.
> 
> I guess we could, but then, I would rather worry about that in the next
> cycle. In this cycle, I would rather fix the common case, which is that
> a `git rebase -i` fails and tells me to `git cherry-pick --skip` instead
> of `git rebase --skip`.
> 
> And even if I performed a `git cherry-pick` during a `git rebase` and
> the result would be an empty commit, I'd rather be told to `git rebase
> --skip` to continue...
> 
> But if you feel strongly that this should be fixed differently, I'll
> gladly leave it to you ;-)

I'm happy to wait until the next cycle once we've decided what to do 
about CHERRY_PICK_HEAD during rebases.

Best Wishes

Phillip

> Ciao,
> Dscho
> 
>>
>> Best Wishes
>>
>>
>> Phillip
>>
>>>
>>> Signed-off-by: Johannes Schindelin <[e-mail]>
>>> ---
>>>   builtin/commit.c       | 33 ++++++++++++++++++++++++---------
>>>   t/t3403-rebase-skip.sh |  9 +++++++++
>>>   2 files changed, 33 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/builtin/commit.c b/builtin/commit.c
>>> index e588bc6ad3..2beae13620 100644
>>> --- a/builtin/commit.c
>>> +++ b/builtin/commit.c
>>> @@ -59,6 +59,9 @@ N_("The previous cherry-pick is now empty, possibly due to conflict resolution.\
>>>   "    git commit --allow-empty\n"
>>>   "\n");
>>>
>>> +static const char empty_rebase_advice[] =
>>> +N_("Otherwise, please use 'git rebase --skip'\n");
>>> +
>>>   static const char empty_cherry_pick_advice_single[] =
>>>   N_("Otherwise, please use 'git cherry-pick --skip'\n");
>>>
>>> @@ -122,7 +125,7 @@ static enum commit_msg_cleanup_mode cleanup_mode;
>>>   static const char *cleanup_arg;
>>>
>>>   static enum commit_whence whence;
>>> -static int sequencer_in_use;
>>> +static int sequencer_in_use, rebase_in_progress;
>>>   static int use_editor = 1, include_status = 1;
>>>   static int have_option_m;
>>>   static struct strbuf message = STRBUF_INIT;
>>> @@ -183,6 +186,8 @@ static void determine_whence(struct wt_status *s)
>>>   		whence = FROM_CHERRY_PICK;
>>>   		if (file_exists(git_path_seq_dir()))
>>>   			sequencer_in_use = 1;
>>> +		if (file_exists(git_path_rebase_merge_dir()))
>>> +			rebase_in_progress = 1;
>>>   	}
>>>   	else
>>>   		whence = FROM_COMMIT;
>>> @@ -453,8 +458,11 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
>>>   	if (whence != FROM_COMMIT) {
>>>   		if (whence == FROM_MERGE)
>>>   			die(_("cannot do a partial commit during a merge."));
>>> -		else if (whence == FROM_CHERRY_PICK)
>>> +		else if (whence == FROM_CHERRY_PICK) {
>>> +			if (rebase_in_progress && !sequencer_in_use)
>>> +				die(_("cannot do a partial commit during a rebase."));
>>>   			die(_("cannot do a partial commit during a cherry-pick."));
>>> +		}
>>>   	}
>>>
>>>   	if (list_paths(&partial, !current_head ? NULL : "HEAD", &pathspec))
>>> @@ -950,10 +958,12 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>>>   			fputs(_(empty_amend_advice), stderr);
>>>   		else if (whence == FROM_CHERRY_PICK) {
>>>   			fputs(_(empty_cherry_pick_advice), stderr);
>>> -			if (!sequencer_in_use)
>>> -				fputs(_(empty_cherry_pick_advice_single), stderr);
>>> -			else
>>> +			if (sequencer_in_use)
>>>   				fputs(_(empty_cherry_pick_advice_multi), stderr);
>>> +			else if (rebase_in_progress)
>>> +				fputs(_(empty_rebase_advice), stderr);
>>> +			else
>>> +				fputs(_(empty_cherry_pick_advice_single), stderr);
>>>   		}
>>>   		return 0;
>>>   	}
>>> @@ -1156,8 +1166,11 @@ static int parse_and_validate_options(int argc, const char *argv[],
>>>   	if (amend && whence != FROM_COMMIT) {
>>>   		if (whence == FROM_MERGE)
>>>   			die(_("You are in the middle of a merge -- cannot amend."));
>>> -		else if (whence == FROM_CHERRY_PICK)
>>> +		else if (whence == FROM_CHERRY_PICK) {
>>> +			if (rebase_in_progress && !sequencer_in_use)
>>> +				die(_("You are in the middle of a rebase -- cannot amend."));
>>>   			die(_("You are in the middle of a cherry-pick -- cannot amend."));
>>> +		}
>>>   	}
>>>   	if (fixup_message && squash_message)
>>>   		die(_("Options --squash and --fixup cannot be used together"));
>>> @@ -1628,9 +1641,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>>>   			reduce_heads_replace(&parents);
>>>   	} else {
>>>   		if (!reflog_msg)
>>> -			reflog_msg = (whence == FROM_CHERRY_PICK)
>>> -					? "commit (cherry-pick)"
>>> -					: "commit";
>>> +			reflog_msg = (whence != FROM_CHERRY_PICK)
>>> +					? "commit"
>>> +					: rebase_in_progress && !sequencer_in_use
>>> +					? "commit (rebase)"
>>> +					: "commit (cherry-pick)";
>>>   		commit_list_insert(current_head, &parents);
>>>   	}
>>>
>>> diff --git a/t/t3403-rebase-skip.sh b/t/t3403-rebase-skip.sh
>>> index 1f5122b632..77b03ac49f 100755
>>> --- a/t/t3403-rebase-skip.sh
>>> +++ b/t/t3403-rebase-skip.sh
>>> @@ -76,4 +76,13 @@ test_expect_success 'moved back to branch correctly' '
>>>
>>>   test_debug 'gitk --all & sleep 1'
>>>
>>> +test_expect_success 'correct advice upon empty commit' '
>>> +	git checkout -b rebase-skip &&
>>> +	test_commit a1 &&
>>> +	test_tick &&
>>> +	git commit --amend -m amended --no-edit &&
>>> +	test_must_fail git rebase -m --onto a1 HEAD^ 2>err &&
>>> +	test_i18ngrep "git rebase --skip" err
>>> +'
>>> +
>>>   test_done
>>>
>>
>>

Junio C Hamano Nov. 8, 2019, 5:28 a.m. UTC | #5

Phillip Wood <[e-mail]> writes:

>>> I don't this patch can distinguish between an empty cherry-pick
>>> performed by the user while a rebase is in progress and an empty pick
>>> performed by rebase as both create CHERRY_PICK_HEAD while
>>> .git/rebase-merge exists. It seems to assume that CHERRY_PICK_HEAD was
>>> created by rebase and prints advise based on that which may or may not
>>> be the correct. I think we could distinguish the two by checking if
>>> CHERRY_PICK_HEAD matches .git/rebase-merge/stopped-sha or REBASE_HEAD.
>>
>> I guess we could, but then, I would rather worry about that in the next
>> cycle. In this cycle, I would rather fix the common case, which is that
>> a `git rebase -i` fails and tells me to `git cherry-pick --skip` instead
>> of `git rebase --skip`.
>>
>> And even if I performed a `git cherry-pick` during a `git rebase` and
>> the result would be an empty commit, I'd rather be told to `git rebase
>> --skip` to continue...
>>
>> But if you feel strongly that this should be fixed differently, I'll
>> gladly leave it to you ;-)
>
> I'm happy to wait until the next cycle once we've decided what to do
> about CHERRY_PICK_HEAD during rebases.

So, is that agreed between the two?  

Should I eject js/advise-rebase-skip topic out of my tree and wait
for the decision wrt CHERRY_PICK_HEAD?

Thanks.

Johannes Schindelin Nov. 8, 2019, 2:09 p.m. UTC | #6

Hi,

On Fri, 8 Nov 2019, Junio C Hamano wrote:

> Phillip Wood <[e-mail]> writes:
>
> >>> I don't this patch can distinguish between an empty cherry-pick
> >>> performed by the user while a rebase is in progress and an empty pick
> >>> performed by rebase as both create CHERRY_PICK_HEAD while
> >>> .git/rebase-merge exists. It seems to assume that CHERRY_PICK_HEAD was
> >>> created by rebase and prints advise based on that which may or may not
> >>> be the correct. I think we could distinguish the two by checking if
> >>> CHERRY_PICK_HEAD matches .git/rebase-merge/stopped-sha or REBASE_HEAD.
> >>
> >> I guess we could, but then, I would rather worry about that in the next
> >> cycle. In this cycle, I would rather fix the common case, which is that
> >> a `git rebase -i` fails and tells me to `git cherry-pick --skip` instead
> >> of `git rebase --skip`.
> >>
> >> And even if I performed a `git cherry-pick` during a `git rebase` and
> >> the result would be an empty commit, I'd rather be told to `git rebase
> >> --skip` to continue...
> >>
> >> But if you feel strongly that this should be fixed differently, I'll
> >> gladly leave it to you ;-)
> >
> > I'm happy to wait until the next cycle once we've decided what to do
> > about CHERRY_PICK_HEAD during rebases.
>
> So, is that agreed between the two?
>
> Should I eject js/advise-rebase-skip topic out of my tree and wait
> for the decision wrt CHERRY_PICK_HEAD?

Phillip, if you have some time to spend on that, I'd be very grateful, I
am a bit under the weather and in dear need for an offline weekend.

Thanks,
Dscho

Phillip Wood Nov. 11, 2019, 4:13 p.m. UTC | #7

Hi Dscho & Junio

On 08/11/2019 14:09, Johannes Schindelin wrote:
> Hi,
> 
> On Fri, 8 Nov 2019, Junio C Hamano wrote:
> 
>> Phillip Wood <[e-mail]> writes:
>>
>>>>> I don't this patch can distinguish between an empty cherry-pick
>>>>> performed by the user while a rebase is in progress and an empty pick
>>>>> performed by rebase as both create CHERRY_PICK_HEAD while
>>>>> .git/rebase-merge exists. It seems to assume that CHERRY_PICK_HEAD was
>>>>> created by rebase and prints advise based on that which may or may not
>>>>> be the correct. I think we could distinguish the two by checking if
>>>>> CHERRY_PICK_HEAD matches .git/rebase-merge/stopped-sha or REBASE_HEAD.
>>>>
>>>> I guess we could, but then, I would rather worry about that in the next
>>>> cycle. In this cycle, I would rather fix the common case, which is that
>>>> a `git rebase -i` fails and tells me to `git cherry-pick --skip` instead
>>>> of `git rebase --skip`.
>>>>
>>>> And even if I performed a `git cherry-pick` during a `git rebase` and
>>>> the result would be an empty commit, I'd rather be told to `git rebase
>>>> --skip` to continue...

It depends if you want to continue immediately or do something else in 
which case running reset is probably better advice. I'm not sure that 
there's an easy solution for all possible scenarios though.

>>>>
>>>> But if you feel strongly that this should be fixed differently, I'll
>>>> gladly leave it to you ;-)
>>>
>>> I'm happy to wait until the next cycle once we've decided what to do
>>> about CHERRY_PICK_HEAD during rebases.
>>
>> So, is that agreed between the two?
>>
>> Should I eject js/advise-rebase-skip topic out of my tree and wait
>> for the decision wrt CHERRY_PICK_HEAD?
> 
> Phillip, if you have some time to spend on that, I'd be very grateful, I
> am a bit under the weather and in dear need for an offline weekend.

I'm happy to have a look at it but it probably wont be for a couple of 
weeks. I've been thinking about keeping CHERRY_PICK_HEAD and it's not 
totally straight forward. CHERRY_PICK_HEAD needs to be removed when 
committing fixups to keep the author data from the commit that's being 
amended (also I think `git commit --amend` errors out if 
CHERRY_PICK_HEAD is present). It's further complicated by --reset-date 
and --committer-date-is-author-date as we want commit to respect those 
when they've been passed to the rebase command.

Junio, if you drop this series for now I'll rework it with an enum as 
discussed elsewhere in this thread.

Best Wishes

Phillip

> Thanks,
> Dscho
>

Patch

diff --git a/builtin/commit.c b/builtin/commit.c
index e588bc6ad3..2beae13620 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -59,6 +59,9 @@  N_("The previous cherry-pick is now empty, possibly due to conflict resolution.\
 "    git commit --allow-empty\n"
 "\n");
 
+static const char empty_rebase_advice[] =
+N_("Otherwise, please use 'git rebase --skip'\n");
+
 static const char empty_cherry_pick_advice_single[] =
 N_("Otherwise, please use 'git cherry-pick --skip'\n");
 
@@ -122,7 +125,7 @@  static enum commit_msg_cleanup_mode cleanup_mode;
 static const char *cleanup_arg;
 
 static enum commit_whence whence;
-static int sequencer_in_use;
+static int sequencer_in_use, rebase_in_progress;
 static int use_editor = 1, include_status = 1;
 static int have_option_m;
 static struct strbuf message = STRBUF_INIT;
@@ -183,6 +186,8 @@  static void determine_whence(struct wt_status *s)
 		whence = FROM_CHERRY_PICK;
 		if (file_exists(git_path_seq_dir()))
 			sequencer_in_use = 1;
+		if (file_exists(git_path_rebase_merge_dir()))
+			rebase_in_progress = 1;
 	}
 	else
 		whence = FROM_COMMIT;
@@ -453,8 +458,11 @@  static const char *prepare_index(int argc, const char **argv, const char *prefix
 	if (whence != FROM_COMMIT) {
 		if (whence == FROM_MERGE)
 			die(_("cannot do a partial commit during a merge."));
-		else if (whence == FROM_CHERRY_PICK)
+		else if (whence == FROM_CHERRY_PICK) {
+			if (rebase_in_progress && !sequencer_in_use)
+				die(_("cannot do a partial commit during a rebase."));
 			die(_("cannot do a partial commit during a cherry-pick."));
+		}
 	}
 
 	if (list_paths(&partial, !current_head ? NULL : "HEAD", &pathspec))
@@ -950,10 +958,12 @@  static int prepare_to_commit(const char *index_file, const char *prefix,
 			fputs(_(empty_amend_advice), stderr);
 		else if (whence == FROM_CHERRY_PICK) {
 			fputs(_(empty_cherry_pick_advice), stderr);
-			if (!sequencer_in_use)
-				fputs(_(empty_cherry_pick_advice_single), stderr);
-			else
+			if (sequencer_in_use)
 				fputs(_(empty_cherry_pick_advice_multi), stderr);
+			else if (rebase_in_progress)
+				fputs(_(empty_rebase_advice), stderr);
+			else
+				fputs(_(empty_cherry_pick_advice_single), stderr);
 		}
 		return 0;
 	}
@@ -1156,8 +1166,11 @@  static int parse_and_validate_options(int argc, const char *argv[],
 	if (amend && whence != FROM_COMMIT) {
 		if (whence == FROM_MERGE)
 			die(_("You are in the middle of a merge -- cannot amend."));
-		else if (whence == FROM_CHERRY_PICK)
+		else if (whence == FROM_CHERRY_PICK) {
+			if (rebase_in_progress && !sequencer_in_use)
+				die(_("You are in the middle of a rebase -- cannot amend."));
 			die(_("You are in the middle of a cherry-pick -- cannot amend."));
+		}
 	}
 	if (fixup_message && squash_message)
 		die(_("Options --squash and --fixup cannot be used together"));
@@ -1628,9 +1641,11 @@  int cmd_commit(int argc, const char **argv, const char *prefix)
 			reduce_heads_replace(&parents);
 	} else {
 		if (!reflog_msg)
-			reflog_msg = (whence == FROM_CHERRY_PICK)
-					? "commit (cherry-pick)"
-					: "commit";
+			reflog_msg = (whence != FROM_CHERRY_PICK)
+					? "commit"
+					: rebase_in_progress && !sequencer_in_use
+					? "commit (rebase)"
+					: "commit (cherry-pick)";
 		commit_list_insert(current_head, &parents);
 	}
 
diff --git a/t/t3403-rebase-skip.sh b/t/t3403-rebase-skip.sh
index 1f5122b632..77b03ac49f 100755
--- a/t/t3403-rebase-skip.sh
+++ b/t/t3403-rebase-skip.sh
@@ -76,4 +76,13 @@  test_expect_success 'moved back to branch correctly' '
 
 test_debug 'gitk --all & sleep 1'
 
+test_expect_success 'correct advice upon empty commit' '
+	git checkout -b rebase-skip &&
+	test_commit a1 &&
+	test_tick &&
+	git commit --amend -m amended --no-edit &&
+	test_must_fail git rebase -m --onto a1 HEAD^ 2>err &&
+	test_i18ngrep "git rebase --skip" err
+'
+
 test_done