threads / patch / 21940

patch, 3 partscommit/status: check $GIT_DIR/MERGE_HEAD only once

Subject: [PATCH 1/3] commit/status: check $GIT_DIR/MERGE_HEAD only once

## tl;dr

10 messages between Dec 12, 2009 and Dec 12, 2009. Diffs are folded; open one to read it.

replies: 9people: 5as markdown or json

Junio C Hamano· Dec 12, 2009, 09:02 UTC · lore

[PATCH 0/3] Update advice in commit/status output

Jay Soffian noticed that we give "git reset HEAD <path>" as an instruction to get rid of the local change that has already been added to the index even when <path> is unmerged, or it is merged and we are about to commit a merge.

In neither case, "git reset HEAD <path>" is absolutely a wrong thing to do while merging.

This miniseries updates the advices given in status/commit. It applies on top of the jk/1.7.0-status topic, and has trivial conflicts in wt-status.c with the jk/unwanted-advices topic that has already graduated to 'maint'.

Junio C Hamano (3):
  commit/status: check $GIT_DIR/MERGE_HEAD only once
  commit/status: "git add <path>" is not necessarily how to resolve
  status/commit: do not suggest "reset HEAD <path>" while merging
 builtin-commit.c    |   12 ++++++------
 t/t7060-wtstatus.sh |    3 +--
 wt-status.c         |   16 +++++++++++-----
 wt-status.h         |    1 +
 4 files changed, 19 insertions(+), 13 deletions(-)
Junio C Hamano· Dec 12, 2009, 09:02 UTC · re: Junio C Hamano · lore

The code checked for the MERGE_HEAD file to see if we were about to commit a merge twice in the codepath; also one of them used a variable merge_head_sha1[] which was set but was never used.

Just check it once, but do so also in "git status", too, as we will be using this for status generation in the next patch.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-commit.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)
Show changes to builtin-commit.c +4 −6
diff --git a/builtin-commit.c b/builtin-commit.c
index b39295f..17dd462 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -36,7 +36,7 @@ static const char * const builtin_status_usage[] = {
 	NULL
 };
 
-static unsigned char head_sha1[20], merge_head_sha1[20];
+static unsigned char head_sha1[20];
 static char *use_message_buffer;
 static const char commit_editmsg[] = "COMMIT_EDITMSG";
 static struct lock_file index_lock; /* real index */
@@ -319,7 +319,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix, int
 	 */
 	commit_style = COMMIT_PARTIAL;
 
-	if (file_exists(git_path("MERGE_HEAD")))
+	if (in_merge)
 		die("cannot do a partial commit during a merge.");
 
 	memset(&partial, 0, sizeof(partial));
@@ -758,9 +758,6 @@ static int parse_and_validate_options(int argc, const char *argv[],
 	if (get_sha1("HEAD", head_sha1))
 		initial_commit = 1;
 
-	if (!get_sha1("MERGE_HEAD", merge_head_sha1))
-		in_merge = 1;
-
 	/* Sanity check options */
 	if (amend && initial_commit)
 		die("You have nothing to amend.");
@@ -951,6 +948,7 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 
 	wt_status_prepare(&s);
 	git_config(git_status_config, &s);
+	in_merge = file_exists(git_path("MERGE_HEAD"));
 	argc = parse_options(argc, argv, prefix,
 			     builtin_status_options,
 			     builtin_status_usage, 0);
@@ -1057,10 +1055,10 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 
 	wt_status_prepare(&s);
 	git_config(git_commit_config, &s);
+	in_merge = file_exists(git_path("MERGE_HEAD"));
 
 	if (s.use_color == -1)
 		s.use_color = git_use_color_default;
-
 	argc = parse_and_validate_options(argc, argv, builtin_commit_usage,
 					  prefix, &s);
 	if (dry_run) {
-- 
1.6.6.rc2.5.g49666
Junio C Hamano· Dec 12, 2009, 09:02 UTC · re: Junio C Hamano · lore

[PATCH 2/3] commit/status: "git add <path>" is not necessarily how to resolve

When the desired resolution is to remove the path, "git rm <path>" is the command the user needs to use. Just like in "Changed but not updated" section, suggest to use "git add/rm" as appropriate.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t7060-wtstatus.sh |    2 +-
 wt-status.c         |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
Show changes to 2 files +2 −2

t/t7060-wtstatus.sh, wt-status.c

diff --git a/t/t7060-wtstatus.sh b/t/t7060-wtstatus.sh
index 7b5db80..6c1af26 100755
--- a/t/t7060-wtstatus.sh
+++ b/t/t7060-wtstatus.sh
@@ -32,7 +32,7 @@ cat >expect <<EOF
 # On branch side
 # Unmerged paths:
 #   (use "git reset HEAD <file>..." to unstage)
-#   (use "git add <file>..." to mark resolution)
+#   (use "git add/rm <file>..." as appropriately to mark resolution)
 #
 #	deleted by us:      foo
 #
diff --git a/wt-status.c b/wt-status.c
index 3fdcf97..5271b6a 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -52,7 +52,7 @@ static void wt_status_print_unmerged_header(struct wt_status *s)
 		color_fprintf_ln(s->fp, c, "#   (use \"git reset %s <file>...\" to unstage)", s->reference);
 	else
 		color_fprintf_ln(s->fp, c, "#   (use \"git rm --cached <file>...\" to unstage)");
-	color_fprintf_ln(s->fp, c, "#   (use \"git add <file>...\" to mark resolution)");
+	color_fprintf_ln(s->fp, c, "#   (use \"git add/rm <file>...\" as appropriately to mark resolution)");
 	color_fprintf_ln(s->fp, c, "#");
 }
 
-- 
1.6.6.rc2.5.g49666
Junio C Hamano· Dec 12, 2009, 09:02 UTC · re: Junio C Hamano · lore

[PATCH 3/3] status/commit: do not suggest "reset HEAD <path>" while merging

Suggesting "'reset HEAD <path>' to unstage" is dead wrong if we are about to record a merge commit. For either an unmerged path (i.e. with unresolved conflicts), or an updated path, it would result in discarding what the other branch did.

Note that we do not do anything special in a case where we are amending a merge. The user is making an evil merge starting from an already committed merge, and running "reset HEAD <path>" is the right way to get rid of the local edit that has been added to the index.

Once "reset --unresolve <path>" becomes available, we might want to suggest it for a merged path that has unresolve information, but until then, just remove the incorrect advice.

We might also want to suggest "checkout --conflict <path>" to revert the file in the work tree to the state of failed automerge for an unmerged path, but we never did that, and this commit does not change that.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-commit.c    |    2 ++
 t/t7060-wtstatus.sh |    1 -
 wt-status.c         |   14 ++++++++++----
 wt-status.h         |    1 +
 4 files changed, 13 insertions(+), 5 deletions(-)
Show changes to 4 files +13 −5

builtin-commit.c, t/t7060-wtstatus.sh, wt-status.c, wt-status.h

diff --git a/builtin-commit.c b/builtin-commit.c
index 17dd462..7218454 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -960,6 +960,7 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 	read_cache();
 	refresh_cache(REFRESH_QUIET|REFRESH_UNMERGED);
 	s.is_initial = get_sha1(s.reference, sha1) ? 1 : 0;
+	s.in_merge = in_merge;
 	wt_status_collect(&s);
 
 	if (s.relative_paths)
@@ -1056,6 +1057,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	wt_status_prepare(&s);
 	git_config(git_commit_config, &s);
 	in_merge = file_exists(git_path("MERGE_HEAD"));
+	s.in_merge = in_merge;
 
 	if (s.use_color == -1)
 		s.use_color = git_use_color_default;
diff --git a/t/t7060-wtstatus.sh b/t/t7060-wtstatus.sh
index 6c1af26..6dd7077 100755
--- a/t/t7060-wtstatus.sh
+++ b/t/t7060-wtstatus.sh
@@ -31,7 +31,6 @@ test_expect_success 'Report new path with conflict' '
 cat >expect <<EOF
 # On branch side
 # Unmerged paths:
-#   (use "git reset HEAD <file>..." to unstage)
 #   (use "git add/rm <file>..." as appropriately to mark resolution)
 #
 #	deleted by us:      foo
diff --git a/wt-status.c b/wt-status.c
index 5271b6a..3f62c44 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -47,8 +47,11 @@ void wt_status_prepare(struct wt_status *s)
 static void wt_status_print_unmerged_header(struct wt_status *s)
 {
 	const char *c = color(WT_STATUS_HEADER, s);
+
 	color_fprintf_ln(s->fp, c, "# Unmerged paths:");
-	if (!s->is_initial)
+	if (s->in_merge)
+		;
+	else if (!s->is_initial)
 		color_fprintf_ln(s->fp, c, "#   (use \"git reset %s <file>...\" to unstage)", s->reference);
 	else
 		color_fprintf_ln(s->fp, c, "#   (use \"git rm --cached <file>...\" to unstage)");
@@ -59,12 +62,14 @@ static void wt_status_print_unmerged_header(struct wt_status *s)
 static void wt_status_print_cached_header(struct wt_status *s)
 {
 	const char *c = color(WT_STATUS_HEADER, s);
+
 	color_fprintf_ln(s->fp, c, "# Changes to be committed:");
-	if (!s->is_initial) {
+	if (s->in_merge)
+		; /* NEEDSWORK: use "git reset --unresolve"??? */
+	else if (!s->is_initial)
 		color_fprintf_ln(s->fp, c, "#   (use \"git reset %s <file>...\" to unstage)", s->reference);
-	} else {
+	else
 		color_fprintf_ln(s->fp, c, "#   (use \"git rm --cached <file>...\" to unstage)");
-	}
 	color_fprintf_ln(s->fp, c, "#");
 }
 
@@ -72,6 +77,7 @@ static void wt_status_print_dirty_header(struct wt_status *s,
 					 int has_deleted)
 {
 	const char *c = color(WT_STATUS_HEADER, s);
+
 	color_fprintf_ln(s->fp, c, "# Changed but not updated:");
 	if (!has_deleted)
 		color_fprintf_ln(s->fp, c, "#   (use \"git add <file>...\" to update what will be committed)");
diff --git a/wt-status.h b/wt-status.h
index a4bddcf..c60f40a 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -34,6 +34,7 @@ struct wt_status {
 	const char **pathspec;
 	int verbose;
 	int amend;
+	int in_merge;
 	int nowarn;
 	int use_color;
 	int relative_paths;
-- 
1.6.6.rc2.5.g49666
Jeff King· Dec 12, 2009, 09:15 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/3] commit/status: "git add <path>" is not necessarily how to resolve

On Sat, Dec 12, 2009 at 01:02:02AM -0800, Junio C Hamano wrote:
> When the desired resolution is to remove the path, "git rm <path>" is the
> command the user needs to use.  Just like in "Changed but not updated"
> section, suggest to use "git add/rm" as appropriate.

I no longer even see these messages due to advice.statushints, but the overall direction of the series looks sane to me.

However:
> -	color_fprintf_ln(s->fp, c, "#   (use \"git add <file>...\" to mark resolution)");
> +	color_fprintf_ln(s->fp, c, "#   (use \"git add/rm <file>...\" as appropriately to mark resolution)");
This should be "as appropriate".
-Peff
Junio C Hamano· Dec 12, 2009, 09:24 UTC · re: Jeff King · lore

Re: [PATCH 2/3] commit/status: "git add <path>" is not necessarily how to resolve

Jeff King <peff@peff.net> writes:
Show 15 quoted lines
> On Sat, Dec 12, 2009 at 01:02:02AM -0800, Junio C Hamano wrote:
>
>> When the desired resolution is to remove the path, "git rm <path>" is the
>> command the user needs to use.  Just like in "Changed but not updated"
>> section, suggest to use "git add/rm" as appropriate.
>
> I no longer even see these messages due to advice.statushints, but the
> overall direction of the series looks sane to me.
>
> However:
>
>> -	color_fprintf_ln(s->fp, c, "#   (use \"git add <file>...\" to mark resolution)");
>> +	color_fprintf_ln(s->fp, c, "#   (use \"git add/rm <file>...\" as appropriately to mark resolution)");
>
> This should be "as appropriate".
Heh, I kant speel.  Fixed and re-pushed out on 'pu'.
Thanks.
Nanako Shiraishi· Dec 12, 2009, 09:19 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/3] commit/status: "git add <path>" is not necessarily how to resolve

Quoting Junio C Hamano <gitster@pobox.com>
Show 5 quoted lines
>  # On branch side
>  # Unmerged paths:
>  #   (use "git reset HEAD <file>..." to unstage)
> -#   (use "git add <file>..." to mark resolution)
> +#   (use "git add/rm <file>..." as appropriately to mark resolution)
Shouldn't this either be "as appropriate" or just "appropriately"?
-- 
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/
Sverre Rabbelier· Dec 12, 2009, 16:13 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/3] Update advice in commit/status output

Heya,
On Sat, Dec 12, 2009 at 10:02, Junio C Hamano <gitster@pobox.com> wrote:
> In neither case, "git reset HEAD <path>" is absolutely a wrong thing to do
> while merging.
>From the patches I'm guessing you mean "In either case" instead?
-- 
Cheers,

Sverre Rabbelier
Junio C Hamano· Dec 12, 2009, 18:27 UTC · re: Sverre Rabbelier · lore

Re: [PATCH 0/3] Update advice in commit/status output

Sverre Rabbelier <srabbelier@gmail.com> writes:
Show 7 quoted lines
> Heya,
>
> On Sat, Dec 12, 2009 at 10:02, Junio C Hamano <gitster@pobox.com> wrote:
>> In neither case, "git reset HEAD <path>" is absolutely a wrong thing to do
>> while merging.
>
>>From the patches I'm guessing you mean "In either case" instead?
Correct.
Jay Soffian· Dec 12, 2009, 22:06 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/3] Update advice in commit/status output

On Sat, Dec 12, 2009 at 4:02 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 11 quoted lines
> Jay Soffian noticed that we give "git reset HEAD <path>" as an instruction
> to get rid of the local change that has already been added to the index
> even when <path> is unmerged, or it is merged and we are about to commit a
> merge.
>
> In neither case, "git reset HEAD <path>" is absolutely a wrong thing to do
> while merging.
>
> This miniseries updates the advices given in status/commit.  It applies on
> top of the jk/1.7.0-status topic, and has trivial conflicts in wt-status.c
> with the jk/unwanted-advices topic that has already graduated to 'maint'.
Series looks good to me after the spelling correction. Thank you.
j.

← back to recent threads