commit: fix advice for empty commits during rebases #417
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In dcb500d (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, but that was never tested for. Here is a test that verifies that a message is given to the user that contains the correct invocation. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
The presence of this path will be used in the next commit to fix an incorrect piece of advice in `git commit` when in the middle of a rebase. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
In dcb500d (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 <johannes.schindelin@gmx.de>
|
/submit |
|
Submitted as pull.417.git.1571787022.gitgitgadget@gmail.com |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -59,6 +59,9 @@ N_("The previous cherry-pick is now empty, possibly due to conflict resolution.\ | |||
| " git commit --allow-empty\n" | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
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.
|
On the Git mailing list, Junio C Hamano wrote (reply to this): |
|
This branch is now known as |
|
This patch series was integrated into pu via git@bd00388. |
|
This patch series was integrated into pu via git@da7915e. |
|
This patch series was integrated into pu via git@3f395fa. |
|
This patch series was integrated into next via git@e2edd2d. |
|
This patch series was integrated into pu via git@1f6d263. |
| @@ -59,6 +59,9 @@ N_("The previous cherry-pick is now empty, possibly due to conflict resolution.\ | |||
| " git commit --allow-empty\n" | |||
There was a problem hiding this comment.
On the Git mailing list, Phillip Wood wrote (reply to this):
Hi Dscho
On 23/10/2019 00:30, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> 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 <johannes.schindelin@gmx.de>
> ---
> 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
>
There was a problem hiding this comment.
On the Git mailing list, Johannes Schindelin wrote (reply to this):
Hi Phillip,
On Thu, 24 Oct 2019, Phillip Wood wrote:
> On 23/10/2019 00:30, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > 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 rebas=
e.
> >
> > 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 th=
at
> > 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/` an=
d
> > `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
=2D-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 <johannes.schindelin@gmx.de>
> > ---
> > 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[] =3D
> > +N_("Otherwise, please use 'git rebase --skip'\n");
> > +
> > static const char empty_cherry_pick_advice_single[] =3D
> > 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 =3D 1, include_status =3D 1;
> > static int have_option_m;
> > static struct strbuf message =3D STRBUF_INIT;
> > @@ -183,6 +186,8 @@ static void determine_whence(struct wt_status *s)
> > whence =3D FROM_CHERRY_PICK;
> > if (file_exists(git_path_seq_dir()))
> > sequencer_in_use =3D 1;
> > + if (file_exists(git_path_rebase_merge_dir()))
> > + rebase_in_progress =3D 1;
> > }
> > else
> > whence =3D FROM_COMMIT;
> > @@ -453,8 +458,11 @@ static const char *prepare_index(int argc, const =
char **argv, const char *prefix
> > if (whence !=3D FROM_COMMIT) {
> > if (whence =3D=3D FROM_MERGE)
> > die(_("cannot do a partial commit during a merge."));
> > - else if (whence =3D=3D FROM_CHERRY_PICK)
> > + else if (whence =3D=3D 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_f=
ile, const char *prefix,
> > fputs(_(empty_amend_advice), stderr);
> > else if (whence =3D=3D 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 !=3D FROM_COMMIT) {
> > if (whence =3D=3D FROM_MERGE)
> > die(_("You are in the middle of a merge -- cannot amend."));
> > - else if (whence =3D=3D FROM_CHERRY_PICK)
> > + else if (whence =3D=3D 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, con=
st char *prefix)
> > reduce_heads_replace(&parents);
> > } else {
> > if (!reflog_msg)
> > - reflog_msg =3D (whence =3D=3D FROM_CHERRY_PICK)
> > - ? "commit (cherry-pick)"
> > - : "commit";
> > + reflog_msg =3D (whence !=3D 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
> >
>
>
There was a problem hiding this comment.
On the Git mailing list, Phillip Wood wrote (reply to this):
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 <johannes.schindelin@gmx.de>
>>>
>>> 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/1297876835-70613-1-git-send-email-jaysoffian@gmail.com/
[2]
https://public-inbox.org/git/CAMP44s1EAwHjQ7S2ArLvhNg5qkR05DRJ70tQmP8sXYdOP=i_zQ@mail.gmail.com/
>> 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 <johannes.schindelin@gmx.de>
>>> ---
>>> 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
>>>
>>
>>
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Phillip Wood <phillip.wood123@gmail.com> 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.
There was a problem hiding this comment.
On the Git mailing list, Johannes Schindelin wrote (reply to this):
Hi,
On Fri, 8 Nov 2019, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> 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 pic=
k
> >>> performed by rebase as both create CHERRY_PICK_HEAD while
> >>> .git/rebase-merge exists. It seems to assume that CHERRY_PICK_HEAD w=
as
> >>> created by rebase and prints advise based on that which may or may n=
ot
> >>> be the correct. I think we could distinguish the two by checking if
> >>> CHERRY_PICK_HEAD matches .git/rebase-merge/stopped-sha or REBASE_HEA=
D.
> >>
> >> I guess we could, but then, I would rather worry about that in the ne=
xt
> >> cycle. In this cycle, I would rather fix the common case, which is th=
at
> >> a `git rebase -i` fails and tells me to `git cherry-pick --skip` inst=
ead
> >> 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 rebas=
e
> >> --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
There was a problem hiding this comment.
On the Git mailing list, Phillip Wood wrote (reply to this):
Hi Dscho & Junio
On 08/11/2019 14:09, Johannes Schindelin wrote:
> Hi,
>
> On Fri, 8 Nov 2019, Junio C Hamano wrote:
>
>> Phillip Wood <phillip.wood123@gmail.com> 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
>
|
On the Git mailing list, Johannes Schindelin wrote (reply to this): |
|
This patch series was integrated into pu via git@a724def. |
This comment has been minimized.
This comment has been minimized.
|
This patch series was integrated into pu via git@b1198b3. |
This comment has been minimized.
This comment has been minimized.
|
This patch series was integrated into pu via git@d236b38. |
|
This patch series was integrated into pu via git@c326bac. |
|
This patch series was integrated into pu via git@ee6b49e. |
|
This patch series was integrated into pu via git@b179f9e. |
|
This patch series was integrated into pu via git@cb4a58d. |
|
This patch series was integrated into pu via git@d3f5dd2. |
|
This patch series was integrated into pu via git@7d8ad53. |
|
This patch series was integrated into pu via git@ac732c6. |
|
This patch series was integrated into pu via git@bac9040. |
|
This patch series was integrated into pu via git@7096984. |
|
This patch series was integrated into pu via git@7e9fc1c. |
|
This patch series was integrated into pu via git@6273b55. |
|
This patch series was integrated into pu via git@ecdcc38. |
|
This patch series was integrated into pu via git@30e2fa9. |
|
This patch series was integrated into pu via git@95ae1ac. |
|
This patch series was integrated into pu via git@956740b. |
|
@dscho I've got some updated patches for this which are almost ready to post - what do you think is the best way of doing that? Is there some way I can add them here? If I open a new pull request the patch version number will be wrong. I can always use send-email to reply to your version if that's easier. |
|
How about just opening another GitGitGadget PR, mentioning that this supersedes this here PR? I can follow up and close this one. |
|
This patch series was integrated into pu via git@ef57dd8. |
|
On the Git mailing list, Phillip Wood wrote (reply to this): |
|
On the Git mailing list, Phillip Wood wrote (reply to this): |
|
On the Git mailing list, Phillip Wood wrote (reply to this): |
|
On the Git mailing list, Phillip Wood wrote (reply to this): |
|
On the Git mailing list, Phillip Wood wrote (reply to this): |
|
On the Git mailing list, Phillip Wood wrote (reply to this): |
|
On the Git mailing list, Phillip Wood wrote (reply to this): |
|
On the Git mailing list, Phillip Wood wrote (reply to this): |
|
On the Git mailing list, Phillip Wood wrote (reply to this): |
|
On the Git mailing list, Phillip Wood wrote (reply to this): |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): |
|
Closing in favor of |
|
On the Git mailing list, Phillip Wood wrote (reply to this): |
|
On the Git mailing list, Phillip Wood wrote (reply to this): |
|
On the Git mailing list, Elijah Newren wrote (reply to this): |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In dcb500d (cherry-pick/revert: advise using --skip, 2019-07-02), we introduced a helpful message that suggests to run
git cherry-pick --skip(instead of the previous message that talked aboutgit reset) when a cherry-pick failed due to an empty patch.However, the same message is displayed during a rebase, when the patch to-be-committed is empty. In this case,
git resetwould also have worked, butgit cherry-pick --skipdoes not work. This is a regression introduced in this cycle.Let's be more careful here.
Cc: Rohit Ashiwal rohit.ashiwal265@gmail.com