From: Phillip Wood <phillip.wood123@gmail.com>
To: Git Mailing List <git@vger.kernel.org>
Cc: Johannes Schindelin <Johannes.Schindelin@gmx.de>,
Junio C Hamano <gitster@pobox.com>,
Phillip Wood <phillip.wood@dunelm.org.uk>
Subject: [PATCH v2 0/9] commit: fix advice for empty commits during rebases
Date: Fri, 6 Dec 2019 16:06:05 +0000 [thread overview]
Message-ID: <20191206160614.631724-1-phillip.wood123@gmail.com> (raw)
In-Reply-To: <pull.417.git.1571787022.gitgitgadget@gmail.com>
From: Phillip Wood <phillip.wood@dunelm.org.uk>
I've added some more commits to improve the test coverage and handle
edit & reword as well as pick commands. I've also changed the advice
printed by an empty cherry-pick performed during a rebase to be the
same whether it is part of a sequence of cherry-picks or just a single
pick.
There are a couple of RFC patches at the end that change the advice
for fixups that empty HEAD and change the handling of CHERRY_PICK_HEAD
so if a user commits a conflict resolution the authorship is
preserved. They can be split into a separate series if necessary to
avoid holding up the earlier patches.
The first patch is a general cleanup, not really related to the rest
of the series
These patches are based on a merge of ra/cherry-pick-revert-skip and
pw/sequencer-compare-with-right-parent-to-check-empty-commits
Johannes Schindelin (1):
cherry-pick: add test for `--skip` advice in `git commit`
Phillip Wood (8):
t3404: use test_cmp_rev
cherry-pick: check commit error messages
sequencer: write CHERRY_PICK_HEAD for reword and edit
commit: use enum value for multiple cherry-picks
commit: encapsulate determine_whence() for sequencer
commit: give correct advice for empty commit during a rebase
[RFC] rebase: fix advice when a fixup creates an empty commit
[RFC] rebase -i: leave CHERRY_PICK_HEAD when there are conflicts
builtin/commit.c | 68 +++++++++----
sequencer.c | 93 +++++++++++++++---
sequencer.h | 4 +-
t/t3403-rebase-skip.sh | 97 ++++++++++++++++--
t/t3404-rebase-interactive.sh | 168 +++++++++++++++++++++++---------
t/t3507-cherry-pick-conflict.sh | 23 +++++
t/t3510-cherry-pick-sequence.sh | 3 +-
t/t7512-status-help.sh | 2 -
wt-status.h | 16 ++-
9 files changed, 387 insertions(+), 87 deletions(-)
Range-diff to what is in pu at the moment
-: ---------- > 1: a4ad3e798c t3404: use test_cmp_rev
1: b9f97083f1 = 2: 1e9ea48348 cherry-pick: add test for `--skip` advice in `git commit`
2: 8eff6be234 < -: ---------- sequencer: export the function to get the path of `.git/rebase-merge/`
-: ---------- > 3: f9be4dc3ae cherry-pick: check commit error messages
-: ---------- > 4: fe15c61f1e sequencer: write CHERRY_PICK_HEAD for reword and edit
-: ---------- > 5: d2cc4a59f1 commit: use enum value for multiple cherry-picks
-: ---------- > 6: 06ab99b367 commit: encapsulate determine_whence() for sequencer
3: 116a408b6f ! 7: 637f17212b commit: give correct advice for empty commit during a rebase
@@
## Metadata ##
-Author: Johannes Schindelin <Johannes.Schindelin@gmx.de>
+Author: Phillip Wood <phillip.wood@dunelm.org.uk>
## Commit message ##
commit: give correct advice for empty commit during a rebase
@@ Commit message
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`.
+ `sequencer/` exist or CHERRY_PICK_HEAD and REBASE_HEAD point to the same
+ commit , we still want to advise to use `git cherry-pick --skip`.
- Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
+ Original-patch-by: Johannes Schindelin <johannes.schindelin@gmx.de>
+ Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
## builtin/commit.c ##
@@ builtin/commit.c: 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[] =
++static const char empty_rebase_pick_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");
-@@ builtin/commit.c: 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;
-@@ builtin/commit.c: 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;
@@ builtin/commit.c: 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."));
+ else if (is_from_cherry_pick(whence))
die(_("cannot do a partial commit during a cherry-pick."));
-+ }
++ else if (is_from_rebase(whence))
++ die(_("cannot do a partial commit during a rebase."));
}
if (list_paths(&partial, !current_head ? NULL : "HEAD", &pathspec))
@@ builtin/commit.c: static int prepare_to_commit(const char *index_file, const char *prefix,
+ */
+ else if (whence == FROM_MERGE)
+ hook_arg1 = "merge";
+- else if (is_from_cherry_pick(whence)) {
++ else if (is_from_cherry_pick(whence) || whence == FROM_REBASE_PICK) {
+ hook_arg1 = "commit";
+ hook_arg2 = "CHERRY_PICK_HEAD";
+ }
+@@ builtin/commit.c: static int prepare_to_commit(const char *index_file, const char *prefix,
+ run_status(stdout, index_file, prefix, 0, s);
+ if (amend)
fputs(_(empty_amend_advice), stderr);
- else if (whence == FROM_CHERRY_PICK) {
+- else if (is_from_cherry_pick(whence)) {
++ else if (is_from_cherry_pick(whence) ||
++ whence == FROM_REBASE_PICK) {
fputs(_(empty_cherry_pick_advice), stderr);
-- if (!sequencer_in_use)
-- fputs(_(empty_cherry_pick_advice_single), stderr);
+ if (whence == FROM_CHERRY_PICK_SINGLE)
+ fputs(_(empty_cherry_pick_advice_single), stderr);
- else
-+ if (sequencer_in_use)
++ else if (whence == FROM_CHERRY_PICK_MULTI)
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);
++ fputs(_(empty_rebase_pick_advice), stderr);
}
return 0;
}
@@ builtin/commit.c: 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."));
+ else if (is_from_cherry_pick(whence))
die(_("You are in the middle of a cherry-pick -- cannot amend."));
-+ }
++ else if (whence == FROM_REBASE_PICK)
++ die(_("You are in the middle of a rebase -- cannot amend."));
}
if (fixup_message && squash_message)
die(_("Options --squash and --fixup cannot be used together"));
+@@ builtin/commit.c: static int parse_and_validate_options(int argc, const char *argv[],
+ use_message = edit_message;
+ if (amend && !use_message && !fixup_message)
+ use_message = "HEAD";
+- if (!use_message && !is_from_cherry_pick(whence) && renew_authorship)
++ if (!use_message && !is_from_cherry_pick(whence) &&
++ !is_from_rebase(whence) && renew_authorship)
+ die(_("--reset-author can be used only with -C, -c or --amend."));
+ if (use_message) {
+ use_message_buffer = read_commit_message(use_message);
+@@ builtin/commit.c: static int parse_and_validate_options(int argc, const char *argv[],
+ author_message_buffer = use_message_buffer;
+ }
+ }
+- if (is_from_cherry_pick(whence) && !renew_authorship) {
++ if ((is_from_cherry_pick(whence) || whence == FROM_REBASE_PICK) &&
++ !renew_authorship) {
+ author_message = "CHERRY_PICK_HEAD";
+ author_message_buffer = read_commit_message(author_message);
+ }
@@ builtin/commit.c: 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
+ reflog_msg = is_from_cherry_pick(whence)
+ ? "commit (cherry-pick)"
++ : is_from_rebase(whence)
+ ? "commit (rebase)"
-+ : "commit (cherry-pick)";
+ : "commit";
commit_list_insert(current_head, &parents);
}
+
+ ## sequencer.c ##
+@@ sequencer.c: static int try_to_commit(struct repository *r,
+ return res;
+ }
+
++static int write_rebase_head(struct object_id *oid)
++{
++ if (update_ref("rebase", "REBASE_HEAD", oid,
++ NULL, REF_NO_DEREF, UPDATE_REFS_MSG_ON_ERR))
++ return error(_("could not update %s"), "REBASE_HEAD");
++
++ return 0;
++}
++
+ static int do_commit(struct repository *r,
+ const char *msg_file, const char *author,
+- struct replay_opts *opts, unsigned int flags)
++ struct replay_opts *opts, unsigned int flags,
++ struct object_id *oid)
+ {
+ int res = 1;
+
+@@ sequencer.c: static int do_commit(struct repository *r,
+ return res;
+ }
+ }
+- if (res == 1)
++ if (res == 1) {
++ if (is_rebase_i(opts) && oid)
++ if (write_rebase_head(oid))
++ return -1;
+ return run_git_commit(r, msg_file, opts, flags);
++ }
+
+ return res;
+ }
+@@ sequencer.c: static int do_pick_commit(struct repository *r,
+ flags |= ALLOW_EMPTY;
+ if (!opts->no_commit) {
+ if (author || command == TODO_REVERT || (flags & AMEND_MSG))
+- res = do_commit(r, msg_file, author, opts, flags);
++ res = do_commit(r, msg_file, author, opts, flags,
++ commit? &commit->object.oid : NULL);
+ else
+ res = error(_("unable to parse commit author"));
+ *check_todo = !!(flags & EDIT_MSG);
+@@ sequencer.c: static int make_patch(struct repository *r,
+ p = short_commit_name(commit);
+ if (write_message(p, strlen(p), rebase_path_stopped_sha(), 1) < 0)
+ return -1;
+- if (update_ref("rebase", "REBASE_HEAD", &commit->object.oid,
+- NULL, REF_NO_DEREF, UPDATE_REFS_MSG_ON_ERR))
+- res |= error(_("could not update %s"), "REBASE_HEAD");
++ res |= write_rebase_head(&commit->object.oid);
+
+ strbuf_addf(&buf, "%s/patch", get_dir(opts));
+ memset(&log_tree_opt, 0, sizeof(log_tree_opt));
+@@ sequencer.c: int todo_list_rearrange_squash(struct todo_list *todo_list)
+ int sequencer_determine_whence(struct repository *r, enum commit_whence *whence)
+ {
+ if (file_exists(git_path_cherry_pick_head(r))) {
+- *whence = file_exists(git_path_seq_dir()) ?
+- FROM_CHERRY_PICK_MULTI : FROM_CHERRY_PICK_SINGLE;
++ struct object_id cherry_pick_head, rebase_head;
++
++ if (file_exists(git_path_seq_dir()))
++ *whence = FROM_CHERRY_PICK_MULTI;
++ if (file_exists(rebase_path()) &&
++ !get_oid("REBASE_HEAD", &rebase_head) &&
++ !get_oid("CHERRY_PICK_HEAD", &cherry_pick_head) &&
++ oideq(&rebase_head, &cherry_pick_head))
++ *whence = FROM_REBASE_PICK;
++ else
++ *whence = FROM_CHERRY_PICK_SINGLE;
++
+ return 1;
+ }
## t/t3403-rebase-skip.sh ##
-@@ t/t3403-rebase-skip.sh: test_expect_success 'moved back to branch correctly' '
+@@ t/t3403-rebase-skip.sh: test_expect_success 'correct advice upon picking empty commit' '
+ test_must_fail git rebase -i --onto goodbye \
+ amended-goodbye^ amended-goodbye 2>err &&
+ test_i18ngrep "previous cherry-pick is now empty" err &&
+- test_i18ngrep "git cherry-pick --skip" err &&
++ test_i18ngrep "git rebase --skip" err &&
+ test_must_fail git commit &&
+- test_i18ngrep "git cherry-pick --skip" err
++ test_i18ngrep "git rebase --skip" err
+ '
- test_debug 'gitk --all & sleep 1'
+ test_expect_success 'correct authorship when committing empty pick' '
+@@ t/t3403-rebase-skip.sh: test_expect_success 'correct advice upon rewording empty commit' '
+ --onto goodbye amended-goodbye^ amended-goodbye 2>err
+ ) &&
+ test_i18ngrep "previous cherry-pick is now empty" err &&
+- test_i18ngrep "git cherry-pick --skip" err &&
++ test_i18ngrep "git rebase --skip" err &&
+ test_must_fail git commit &&
+- test_i18ngrep "git cherry-pick --skip" err
++ test_i18ngrep "git rebase --skip" err
+ '
-+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_expect_success 'correct advice upon editing empty commit' '
+@@ t/t3403-rebase-skip.sh: test_expect_success 'correct advice upon editing empty commit' '
+ --onto goodbye amended-goodbye^ amended-goodbye 2>err
+ ) &&
+ test_i18ngrep "previous cherry-pick is now empty" err &&
+- test_i18ngrep "git cherry-pick --skip" err &&
++ test_i18ngrep "git rebase --skip" err &&
+ test_must_fail git commit &&
+ test_i18ngrep "git rebase --skip" err
+'
+
- test_done
++test_expect_success 'correct advice upon cherry-picking an empty commit during a rebase' '
++ test_when_finished "git rebase --abort" &&
++ (
++ set_fake_editor &&
++ test_must_fail env FAKE_LINES="1 exec_git_cherry-pick_amended-goodbye" \
++ git rebase -i goodbye^ goodbye 2>err
++ ) &&
++ test_i18ngrep "previous cherry-pick is now empty" err &&
++ test_i18ngrep "git cherry-pick --skip" err &&
++ test_must_fail git commit 2>err &&
++ test_i18ngrep "git cherry-pick --skip" err
++'
++
++test_expect_success 'correct advice upon multi cherry-pick picking an empty commit during a rebase' '
++ test_when_finished "git rebase --abort" &&
++ (
++ set_fake_editor &&
++ test_must_fail env FAKE_LINES="1 exec_git_cherry-pick_goodbye_amended-goodbye" \
++ git rebase -i goodbye^^ goodbye 2>err
++ ) &&
++ test_i18ngrep "previous cherry-pick is now empty" err &&
++ test_i18ngrep "git cherry-pick --skip" err &&
++ test_must_fail git commit 2>err &&
+ test_i18ngrep "git cherry-pick --skip" err
+ '
+
+
+ ## t/t3404-rebase-interactive.sh ##
+@@ t/t3404-rebase-interactive.sh: test_expect_success 'post-commit hook is called' '
+ test_cmp expect actual
+ '
+
++test_expect_success 'correct error message for partial commit after empty pick' '
++ test_when_finished "git rebase --abort" &&
++ (
++ set_fake_editor &&
++ FAKE_LINES="2 1 1" &&
++ export FAKE_LINES &&
++ test_must_fail git rebase -i A D
++ ) &&
++ echo x >file1 &&
++ test_must_fail git commit file1 2>err &&
++ test_i18ngrep "cannot do a partial commit during a rebase." err
++'
++
++test_expect_success 'correct error message for commit --amend after empty pick' '
++ test_when_finished "git rebase --abort" &&
++ (
++ set_fake_editor &&
++ FAKE_LINES="1 1" &&
++ export FAKE_LINES &&
++ test_must_fail git rebase -i A D
++ ) &&
++ echo x>file1 &&
++ test_must_fail git commit -a --amend 2>err &&
++ test_i18ngrep "middle of a rebase -- cannot amend." err
++'
++
+ # This must be the last test in this file
+ test_expect_success '$EDITOR and friends are unchanged' '
+ test_editor_unchanged
+
+ ## wt-status.h ##
+@@ wt-status.h: enum commit_whence {
+ FROM_COMMIT, /* normal */
+ FROM_MERGE, /* commit came from merge */
+ FROM_CHERRY_PICK_SINGLE, /* commit came from cherry-pick */
+- FROM_CHERRY_PICK_MULTI /* commit came from a sequence of cherry-picks */
++ FROM_CHERRY_PICK_MULTI, /* commit came from a sequence of cherry-picks */
++ FROM_REBASE_PICK /* commit came from a pick/reword/edit */
+ };
+
+ static inline int is_from_cherry_pick(enum commit_whence whence)
+@@ wt-status.h: static inline int is_from_cherry_pick(enum commit_whence whence)
+ whence == FROM_CHERRY_PICK_MULTI;
+ }
+
++static inline int is_from_rebase(enum commit_whence whence)
++{
++ return whence == FROM_REBASE_PICK;
++}
++
+ struct wt_status_change_data {
+ int worktree_status;
+ int index_status;
-: ---------- > 8: 6a00263595 [RFC] rebase: fix advice when a fixup creates an empty commit
-: ---------- > 9: 8b5ca6b60b [RFC] rebase -i: leave CHERRY_PICK_HEAD when there are conflicts
--
2.24.0
next prev parent reply other threads:[~2019-12-06 16:08 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-22 23:30 [PATCH 0/3] commit: fix advice for empty commits during rebases Johannes Schindelin via GitGitGadget
2019-10-22 23:30 ` [PATCH 1/3] cherry-pick: add test for `--skip` advice in `git commit` Johannes Schindelin via GitGitGadget
2019-10-22 23:30 ` [PATCH 2/3] sequencer: export the function to get the path of `.git/rebase-merge/` Johannes Schindelin via GitGitGadget
2019-10-22 23:30 ` [PATCH 3/3] commit: give correct advice for empty commit during a rebase Johannes Schindelin via GitGitGadget
2019-10-23 2:45 ` Junio C Hamano
2019-10-24 10:15 ` Phillip Wood
2019-10-25 11:48 ` Johannes Schindelin
2019-10-25 14:01 ` Phillip Wood
2019-11-08 5:28 ` Junio C Hamano
2019-11-08 14:09 ` Johannes Schindelin
2019-11-11 16:13 ` Phillip Wood
2019-10-23 3:02 ` [PATCH 0/3] commit: fix advice for empty commits during rebases Junio C Hamano
2019-10-25 12:11 ` Johannes Schindelin
2019-10-29 2:05 ` Junio C Hamano
2019-10-29 13:00 ` Johannes Schindelin
2019-12-06 16:06 ` Phillip Wood [this message]
2019-12-06 16:06 ` [PATCH v2 1/9] t3404: use test_cmp_rev Phillip Wood
2019-12-06 17:39 ` Junio C Hamano
2019-12-06 16:06 ` [PATCH v2 2/9] cherry-pick: add test for `--skip` advice in `git commit` Phillip Wood
2019-12-06 16:06 ` [PATCH v2 3/9] cherry-pick: check commit error messages Phillip Wood
2019-12-06 16:06 ` [PATCH v2 4/9] sequencer: write CHERRY_PICK_HEAD for reword and edit Phillip Wood
2019-12-06 16:06 ` [PATCH v2 5/9] commit: use enum value for multiple cherry-picks Phillip Wood
2019-12-06 18:13 ` Junio C Hamano
2019-12-06 16:06 ` [PATCH v2 6/9] commit: encapsulate determine_whence() for sequencer Phillip Wood
2019-12-06 18:24 ` Junio C Hamano
2019-12-18 14:26 ` Phillip Wood
2019-12-06 16:06 ` [PATCH v2 7/9] commit: give correct advice for empty commit during a rebase Phillip Wood
2019-12-06 16:06 ` [PATCH v2 8/9] [RFC] rebase: fix advice when a fixup creates an empty commit Phillip Wood
2020-02-26 19:45 ` Elijah Newren
2019-12-06 16:06 ` [PATCH v2 9/9] [RFC] rebase -i: leave CHERRY_PICK_HEAD when there are conflicts Phillip Wood
2019-12-18 14:35 ` Phillip Wood
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20191206160614.631724-1-phillip.wood123@gmail.com \
--to=phillip.wood123@gmail.com \
--cc=Johannes.Schindelin@gmx.de \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=phillip.wood@dunelm.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).