diff mbox series

[2/3] Treat CHERRY_PICK_HEAD as a pseudo ref

Message ID 06a8e8cbd1370ba3d4ea8a0a9f1e69d27b1d62c4.1597753075.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series Use refs API for handling sundry pseudorefs | expand

Commit Message

Philippe Blain via GitGitGadget Aug. 18, 2020, 12:17 p.m. UTC 
From: Han-Wen Nienhuys <hanwen@google.com>

Check for existence and delete CHERRY_PICK_HEAD through ref functions.
This will help cherry-pick work with alternate ref storage backends.

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
 builtin/commit.c | 34 +++++++++++++++++++---------------
 builtin/merge.c  |  2 +-
 path.c           |  1 -
 path.h           |  7 ++++---
 sequencer.c      | 42 ++++++++++++++++++++++++++----------------
 wt-status.c      |  4 ++--
 6 files changed, 52 insertions(+), 38 deletions(-)

Comments

Junio C Hamano Aug. 18, 2020, 9:05 p.m. UTC | #1 
"Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Han-Wen Nienhuys <hanwen@google.com>
>
> Check for existence and delete CHERRY_PICK_HEAD through ref functions.
> This will help cherry-pick work with alternate ref storage backends.

These two sentences are true, but this patch does another thing that
is not advertised here.  It stops recommending removal of
CHERRY_PICK_HEAD from the filesystem with "rm" (or using "git
update-ref -d" for that matter) as a way to avoid recording the
current commit as a cherry-pick.

The intent of the warning message touched by this patch, I think, is
that there is CHERRY_PICK_HEAD, but that might be a leftover one
that does not have to do anything to do with the commit the user is
making, and continuing blindly may record the commit incorrectly
(perhaps 'cherry-picked-from' trailer is added incorrectly?
existing notes data would be copied to the newly created commit?).

To recover from the situation, the user would want to abort the
commit while keeping the changes made to the working tree and to the
index to avoid the result recorded as a cherry-pick of an irrelevant
commit, get rid of CHERRY_PICK_HEAD and then attempt the "git
commit" again.

Does "git cherry-pick --abort" only remove CHERRY_PICK_HEAD without
doing any other damage to the working tree files and to the index?

> Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
> ---
>  builtin/commit.c | 34 +++++++++++++++++++---------------
>  builtin/merge.c  |  2 +-
>  path.c           |  1 -
>  path.h           |  7 ++++---
>  sequencer.c      | 42 ++++++++++++++++++++++++++----------------
>  wt-status.c      |  4 ++--
>  6 files changed, 52 insertions(+), 38 deletions(-)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 69ac78d5e5..619b71bcb4 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -847,21 +847,25 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>  			if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS &&
>  				!merge_contains_scissors)
>  				wt_status_add_cut_line(s->fp);
> -			status_printf_ln(s, GIT_COLOR_NORMAL,
> -			    whence == FROM_MERGE
> -				? _("\n"
> -					"It looks like you may be committing a merge.\n"
> -					"If this is not correct, please remove the file\n"
> -					"	%s\n"
> -					"and try again.\n")
> -				: _("\n"
> -					"It looks like you may be committing a cherry-pick.\n"
> -					"If this is not correct, please remove the file\n"
> -					"	%s\n"
> -					"and try again.\n"),
> -				whence == FROM_MERGE ?
> -					git_path_merge_head(the_repository) :
> -					git_path_cherry_pick_head(the_repository));
> +			if (whence == FROM_MERGE)
> +				status_printf_ln(
> +					s, GIT_COLOR_NORMAL,
> +
> +					_("\n"
> +					  "It looks like you may be committing a merge.\n"
> +					  "If this is not correct, please remove the file\n"
> +					  "	%s\n"
> +					  "and try again.\n"),
> +					git_path_merge_head(the_repository));
> +			else
> +				status_printf_ln(
> +					s, GIT_COLOR_NORMAL,
> +
> +					_("\n"
> +					  "It looks like you may be committing a cherry-pick.\n"
> +					  "If this is not correct, please run\n"
> +					  "	git cherry-pick --abort\n"
> +					  "and try again.\n"));
>  		}

I do not know about this change (see above).

> diff --git a/builtin/merge.c b/builtin/merge.c
> index 74829a838e..b9a89ba858 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -1348,7 +1348,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>  		else
>  			die(_("You have not concluded your merge (MERGE_HEAD exists)."));
>  	}
> -	if (file_exists(git_path_cherry_pick_head(the_repository))) {
> +	if (ref_exists("CHERRY_PICK_HEAD")) {

This, and all the rest, look quite sensible.

> diff --git a/wt-status.c b/wt-status.c
> index d75399085d..c6abf2f3ca 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -1672,8 +1672,8 @@ void wt_status_get_state(struct repository *r,
>  		state->merge_in_progress = 1;
>  	} else if (wt_status_check_rebase(NULL, state)) {
>  		;		/* all set */
> -	} else if (!stat(git_path_cherry_pick_head(r), &st) &&
> -			!get_oid("CHERRY_PICK_HEAD", &oid)) {
> +	} else if (refs_ref_exists(get_main_ref_store(r), "CHERRY_PICK_HEAD") &&
> +		   !get_oid("CHERRY_PICK_HEAD", &oid)) {
>  		state->cherry_pick_in_progress = 1;
>  		oidcpy(&state->cherry_pick_head_oid, &oid);
>  	}
Han-Wen Nienhuys Aug. 19, 2020, 3:04 p.m. UTC | #2 
On Tue, Aug 18, 2020 at 11:05 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Han-Wen Nienhuys <hanwen@google.com>
> >
> > Check for existence and delete CHERRY_PICK_HEAD through ref functions.
> > This will help cherry-pick work with alternate ref storage backends.
>
> These two sentences are true, but this patch does another thing that
> is not advertised here.  It stops recommending removal of
> CHERRY_PICK_HEAD from the filesystem with "rm" (or using "git
> update-ref -d" for that matter) as a way to avoid recording the
> current commit as a cherry-pick.
..
> Does "git cherry-pick --abort" only remove CHERRY_PICK_HEAD without
> doing any other damage to the working tree files and to the index?

Good catch. I just added cherry-pick --abort without much thinking. I
reverted to update-ref -d which should be equivalent to what is
currently recommended.
Junio C Hamano Aug. 19, 2020, 4:14 p.m. UTC | #3 
Han-Wen Nienhuys <hanwen@google.com> writes:

>> Does "git cherry-pick --abort" only remove CHERRY_PICK_HEAD without
>> doing any other damage to the working tree files and to the index?
>
> Good catch. I just added cherry-pick --abort without much thinking. I
> reverted to update-ref -d which should be equivalent to what is
> currently recommended.

I think that is a good change, although it is unfortunate that the
command line completion considers "git update-ref -d CHERRY_P<TAB>"
something it should help the user with.

I tried "cherry-pick --abort" to see what happens after

    $ edit A B
    $ git add A
    $ git rev-parse HEAD~200 >.git/CHERRY_PICK_HEAD
    $ git commit

and aborting the last "git commit" step.  As "cherry-pick --abort"
involves running "git reset --merge" internally, of course some
states the user wanted to commit were wiped out and lost.

Thanks.
diff mbox series

Patch

diff --git a/builtin/commit.c b/builtin/commit.c
index 69ac78d5e5..619b71bcb4 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -847,21 +847,25 @@  static int prepare_to_commit(const char *index_file, const char *prefix,
 			if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS &&
 				!merge_contains_scissors)
 				wt_status_add_cut_line(s->fp);
-			status_printf_ln(s, GIT_COLOR_NORMAL,
-			    whence == FROM_MERGE
-				? _("\n"
-					"It looks like you may be committing a merge.\n"
-					"If this is not correct, please remove the file\n"
-					"	%s\n"
-					"and try again.\n")
-				: _("\n"
-					"It looks like you may be committing a cherry-pick.\n"
-					"If this is not correct, please remove the file\n"
-					"	%s\n"
-					"and try again.\n"),
-				whence == FROM_MERGE ?
-					git_path_merge_head(the_repository) :
-					git_path_cherry_pick_head(the_repository));
+			if (whence == FROM_MERGE)
+				status_printf_ln(
+					s, GIT_COLOR_NORMAL,
+
+					_("\n"
+					  "It looks like you may be committing a merge.\n"
+					  "If this is not correct, please remove the file\n"
+					  "	%s\n"
+					  "and try again.\n"),
+					git_path_merge_head(the_repository));
+			else
+				status_printf_ln(
+					s, GIT_COLOR_NORMAL,
+
+					_("\n"
+					  "It looks like you may be committing a cherry-pick.\n"
+					  "If this is not correct, please run\n"
+					  "	git cherry-pick --abort\n"
+					  "and try again.\n"));
 		}
 
 		fprintf(s->fp, "\n");
diff --git a/builtin/merge.c b/builtin/merge.c
index 74829a838e..b9a89ba858 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1348,7 +1348,7 @@  int cmd_merge(int argc, const char **argv, const char *prefix)
 		else
 			die(_("You have not concluded your merge (MERGE_HEAD exists)."));
 	}
-	if (file_exists(git_path_cherry_pick_head(the_repository))) {
+	if (ref_exists("CHERRY_PICK_HEAD")) {
 		if (advice_resolve_conflict)
 			die(_("You have not concluded your cherry-pick (CHERRY_PICK_HEAD exists).\n"
 			    "Please, commit your changes before you merge."));
diff --git a/path.c b/path.c
index 8b2c753191..783cc2ae81 100644
--- a/path.c
+++ b/path.c
@@ -1528,7 +1528,6 @@  char *xdg_cache_home(const char *filename)
 	return NULL;
 }
 
-REPO_GIT_PATH_FUNC(cherry_pick_head, "CHERRY_PICK_HEAD")
 REPO_GIT_PATH_FUNC(revert_head, "REVERT_HEAD")
 REPO_GIT_PATH_FUNC(squash_msg, "SQUASH_MSG")
 REPO_GIT_PATH_FUNC(merge_msg, "MERGE_MSG")
diff --git a/path.h b/path.h
index 1f1bf8f87a..8941c018a9 100644
--- a/path.h
+++ b/path.h
@@ -170,7 +170,6 @@  void report_linked_checkout_garbage(void);
 	}
 
 struct path_cache {
-	const char *cherry_pick_head;
 	const char *revert_head;
 	const char *squash_msg;
 	const char *merge_msg;
@@ -182,9 +181,11 @@  struct path_cache {
 	const char *shallow;
 };
 
-#define PATH_CACHE_INIT { NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL }
+#define PATH_CACHE_INIT                                              \
+	{                                                            \
+		NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL \
+	}
 
-const char *git_path_cherry_pick_head(struct repository *r);
 const char *git_path_revert_head(struct repository *r);
 const char *git_path_squash_msg(struct repository *r);
 const char *git_path_merge_msg(struct repository *r);
diff --git a/sequencer.c b/sequencer.c
index cc3f8fa88e..09e2ff659e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -381,7 +381,8 @@  static void print_advice(struct repository *r, int show_hint,
 		 * (typically rebase --interactive) wants to take care
 		 * of the commit itself so remove CHERRY_PICK_HEAD
 		 */
-		unlink(git_path_cherry_pick_head(r));
+		refs_delete_ref(get_main_ref_store(r), "", "CHERRY_PICK_HEAD",
+				NULL, 0);
 		return;
 	}
 
@@ -1455,7 +1456,8 @@  static int do_commit(struct repository *r,
 				    author, opts, flags, &oid);
 		strbuf_release(&sb);
 		if (!res) {
-			unlink(git_path_cherry_pick_head(r));
+			refs_delete_ref(get_main_ref_store(r), "",
+					"CHERRY_PICK_HEAD", NULL, 0);
 			unlink(git_path_merge_msg(r));
 			if (!is_rebase_i(opts))
 				print_commit_summary(r, NULL, &oid,
@@ -1966,7 +1968,8 @@  static int do_pick_commit(struct repository *r,
 		flags |= ALLOW_EMPTY;
 	} else if (allow == 2) {
 		drop_commit = 1;
-		unlink(git_path_cherry_pick_head(r));
+		refs_delete_ref(get_main_ref_store(r), "", "CHERRY_PICK_HEAD",
+				NULL, 0);
 		unlink(git_path_merge_msg(r));
 		fprintf(stderr,
 			_("dropping %s %s -- patch contents already upstream\n"),
@@ -2305,8 +2308,10 @@  void sequencer_post_commit_cleanup(struct repository *r, int verbose)
 	struct replay_opts opts = REPLAY_OPTS_INIT;
 	int need_cleanup = 0;
 
-	if (file_exists(git_path_cherry_pick_head(r))) {
-		if (!unlink(git_path_cherry_pick_head(r)) && verbose)
+	if (refs_ref_exists(get_main_ref_store(r), "CHERRY_PICK_HEAD")) {
+		if (!refs_delete_ref(get_main_ref_store(r), "",
+				     "CHERRY_PICK_HEAD", NULL, 0) &&
+		    verbose)
 			warning(_("cancelling a cherry picking in progress"));
 		opts.action = REPLAY_PICK;
 		need_cleanup = 1;
@@ -2671,8 +2676,9 @@  static int create_seq_dir(struct repository *r)
 	enum replay_action action;
 	const char *in_progress_error = NULL;
 	const char *in_progress_advice = NULL;
-	unsigned int advise_skip = file_exists(git_path_revert_head(r)) ||
-				file_exists(git_path_cherry_pick_head(r));
+	unsigned int advise_skip =
+		file_exists(git_path_revert_head(r)) ||
+		refs_ref_exists(get_main_ref_store(r), "CHERRY_PICK_HEAD");
 
 	if (!sequencer_get_last_command(r, &action)) {
 		switch (action) {
@@ -2771,7 +2777,7 @@  static int rollback_single_pick(struct repository *r)
 {
 	struct object_id head_oid;
 
-	if (!file_exists(git_path_cherry_pick_head(r)) &&
+	if (!refs_ref_exists(get_main_ref_store(r), "CHERRY_PICK_HEAD") &&
 	    !file_exists(git_path_revert_head(r)))
 		return error(_("no cherry-pick or revert in progress"));
 	if (read_ref_full("HEAD", 0, &head_oid, NULL))
@@ -2874,7 +2880,8 @@  int sequencer_skip(struct repository *r, struct replay_opts *opts)
 		}
 		break;
 	case REPLAY_PICK:
-		if (!file_exists(git_path_cherry_pick_head(r))) {
+		if (!refs_ref_exists(get_main_ref_store(r),
+				     "CHERRY_PICK_HEAD")) {
 			if (action != REPLAY_PICK)
 				return error(_("no cherry-pick in progress"));
 			if (!rollback_is_safe())
@@ -3569,7 +3576,8 @@  static int do_merge(struct repository *r,
 				    oid_to_hex(&j->item->object.oid));
 
 		strbuf_release(&ref_name);
-		unlink(git_path_cherry_pick_head(r));
+		refs_delete_ref(get_main_ref_store(r), "", "CHERRY_PICK_HEAD",
+				NULL, 0);
 		rollback_lock_file(&lock);
 
 		rollback_lock_file(&lock);
@@ -4201,7 +4209,7 @@  static int continue_single_pick(struct repository *r)
 {
 	const char *argv[] = { "commit", NULL };
 
-	if (!file_exists(git_path_cherry_pick_head(r)) &&
+	if (!refs_ref_exists(get_main_ref_store(r), "CHERRY_PICK_HEAD") &&
 	    !file_exists(git_path_revert_head(r)))
 		return error(_("no cherry-pick or revert in progress"));
 	return run_command_v_opt(argv, RUN_GIT_CMD);
@@ -4318,9 +4326,10 @@  static int commit_staged_changes(struct repository *r,
 	}
 
 	if (is_clean) {
-		const char *cherry_pick_head = git_path_cherry_pick_head(r);
-
-		if (file_exists(cherry_pick_head) && unlink(cherry_pick_head))
+		if (refs_ref_exists(get_main_ref_store(r),
+				    "CHERRY_PICK_HEAD") &&
+		    refs_delete_ref(get_main_ref_store(r), "",
+				    "CHERRY_PICK_HEAD", NULL, 0))
 			return error(_("could not remove CHERRY_PICK_HEAD"));
 		if (!final_fixup)
 			return 0;
@@ -4379,7 +4388,8 @@  int sequencer_continue(struct repository *r, struct replay_opts *opts)
 
 	if (!is_rebase_i(opts)) {
 		/* Verify that the conflict has been resolved */
-		if (file_exists(git_path_cherry_pick_head(r)) ||
+		if (refs_ref_exists(get_main_ref_store(r),
+				    "CHERRY_PICK_HEAD") ||
 		    file_exists(git_path_revert_head(r))) {
 			res = continue_single_pick(r);
 			if (res)
@@ -5442,7 +5452,7 @@  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))) {
+	if (refs_ref_exists(get_main_ref_store(r), "CHERRY_PICK_HEAD")) {
 		struct object_id cherry_pick_head, rebase_head;
 
 		if (file_exists(git_path_seq_dir()))
diff --git a/wt-status.c b/wt-status.c
index d75399085d..c6abf2f3ca 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1672,8 +1672,8 @@  void wt_status_get_state(struct repository *r,
 		state->merge_in_progress = 1;
 	} else if (wt_status_check_rebase(NULL, state)) {
 		;		/* all set */
-	} else if (!stat(git_path_cherry_pick_head(r), &st) &&
-			!get_oid("CHERRY_PICK_HEAD", &oid)) {
+	} else if (refs_ref_exists(get_main_ref_store(r), "CHERRY_PICK_HEAD") &&
+		   !get_oid("CHERRY_PICK_HEAD", &oid)) {
 		state->cherry_pick_in_progress = 1;
 		oidcpy(&state->cherry_pick_head_oid, &oid);
 	}