Re: How to selectively recreate merge state?

Jay Soffian [off-list ref] writes:

Also, I think we could improve the output of "git status" during merge
resolution, both before and after conflicts have been resolved in a
file.

I think you are talking about something that is largely unrelated, even
though they would be a pair of good issues to discuss.  The solution to
them does not have much to do with what we have been discussing so far in
this thread, and actually should be much simpler, which is a good news
;-).

$ git status
foo: needs merge
# On branch master
# Changed but not updated:
#   (use "git add <file>..." to update what will be committed)
#   (use "git checkout -- <file>..." to discard changes in working directory)
#
#	unmerged:   foo
#
no changes added to commit (use "git add" and/or "git commit -a")

"unmerged" is good. But the instruction to use "git checkout --
<file>" to discard changes is wrong in this context:

You should be able to change this without any "unresolve" index extension
added to the index.  Just notice an unmerged entry in the index and reword
the message accordingly.

More importantly, note that "git status" lists "unmerged" entries in a
separate section in its output in 1.6.6 (and has been so on 'master' for
some time) and your problem report needs to be adjusted for a more recent
reality.  Here is what you would get:

        $ git status
        # On branch pu
        # Changes to be committed:
        #   (use "git reset HEAD <file>..." to unstage)
        #
        #       modified:   builtin-send-pack.c
        #       modified:   remote.c
        #       modified:   remote.h
        #       modified:   transport.c
        #
        # Unmerged paths:
        #   (use "git reset HEAD <file>..." to unstage)
        #   (use "git add <file>..." to mark resolution)
        #
        #       both modified:      transport-helper.c
        #

One problem we can see is that 'use "git reset HEAD <file>..." to unstage'
is an invalid advice if we are in the middle of a merge, but is perfectly
valid if this were during "rebase", "am -3", "cherry-pick" and "revert".

The solution to this issue is exactly the same as the next one.

$ git status
# On branch master
# Changes to be committed:
#   (use "git reset HEAD <file>..." to unstage)
#
#	modified:   foo
#

Well, yes, I can use git reset, but that just keeps my side of the merge.

If the conflict was coming from "rebase", "cherry-pick", etc., there is
nothing but one side, as there is no merge going on, and what "git reset"
does is exactly what the message tells you---to unstage.

I think "git status" should notice that the next commit you would make
from this state will be a merge commit, and remove these "reset HEAD"
lines.  Once you "git add" to resolve, it makes _no_ sense to reset to
HEAD, if you are concluding a merge.  Until "update-index --unresolve" is
revived as a modern version (and I suspect that a more logical Porcelain
interface would be a new option "reset --unmerge <paths>..."), we should
simply drop "reset HEAD" advice when we are in a merge.

Note that the "unresolve" index extension will not help you at all in
order for you to decide if you are going to make a merge commit.  You
should instead ask "does .git/MERGE_HEAD exist?", and it is something you
should be able to implement directly on top of upcoming 1.6.6 release.

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 <redacted>
---
 builtin-commit.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)

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

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 <redacted>
---
 builtin-commit.c    |    2 ++
 t/t7060-wtstatus.sh |    1 -
 wt-status.c         |   14 ++++++++++----
 wt-status.h         |    1 +
 4 files changed, 13 insertions(+), 5 deletions(-)

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

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(-)

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 <redacted>
---
 t/t7060-wtstatus.sh |    2 +-
 wt-status.c         |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

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

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

Quoting Junio C Hamano [off-list ref]

 # 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/

Heya,

On Sat, Dec 12, 2009 at 10:02, Junio C Hamano [off-list ref] 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

On Sat, Dec 12, 2009 at 4:02 AM, Junio C Hamano [off-list ref] wrote:

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.