git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Commit message not helpful after merge squash with conflicts
@ 2016-03-05 10:38 Sven Strickroth
  2016-03-08  4:17 ` [PATCH] Also read SQUASH_MSG if a conflict on a merge squash occurred Sven Strickroth
  0 siblings, 1 reply; 13+ messages in thread
From: Sven Strickroth @ 2016-03-05 10:38 UTC (permalink / raw)
  To: git; +Cc: gitster

Hi,

after a "git merge --squash" with a conflict the commit message is not
helpful as it only includes the conflicted files information, however, I
expect to see the content of SQUASH_MSG which contains the summary of
the merged commits. SQUASH_MSG seems to be just ignored.

I think git should either read SQUASH_MSG and append COMMIT_MSG or add
the conflict information into SQUASH_MSG and create no COMMIT_MSG.

References:
 *
https://stackoverflow.com/questions/3605385/merge-conflicts-ruin-my-commit-message-while-squashing-commits
 * https://gitlab.com/tortoisegit/tortoisegit/issues/1902

-- 
Best regards,
 Sven Strickroth
 PGP key id F5A9D4C4 @ any key-server

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH] Also read SQUASH_MSG if a conflict on a merge squash occurred
  2016-03-05 10:38 Commit message not helpful after merge squash with conflicts Sven Strickroth
@ 2016-03-08  4:17 ` Sven Strickroth
  2016-03-08 18:32   ` Junio C Hamano
  0 siblings, 1 reply; 13+ messages in thread
From: Sven Strickroth @ 2016-03-08  4:17 UTC (permalink / raw)
  To: Git List; +Cc: Junio C Hamano

After a merge --squash with a conflict the commit message did
not contain the information about the squashed commits, but
only the "# Conflicts:" information.

Signed-off-by: Sven Strickroth <sven@cs-ware.de>
---
 builtin/commit.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/builtin/commit.c b/builtin/commit.c
index d054f84..0405d68 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -729,6 +729,12 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		if (strbuf_read_file(&sb, git_path_merge_msg(), 0) < 0)
 			die_errno(_("could not read MERGE_MSG"));
 		hook_arg1 = "merge";
+		/* append SQUASH_MSG here if it exists and a merge --squash was originally performed */
+		if (!stat(git_path_squash_msg(), &statbuf)) {
+			if (strbuf_read_file(&sb, git_path_squash_msg(), 0) < 0)
+				die_errno(_("could not read SQUASH_MSG"));
+			hook_arg1 = "squash";
+		}
 	} else if (!stat(git_path_squash_msg(), &statbuf)) {
 		if (strbuf_read_file(&sb, git_path_squash_msg(), 0) < 0)
 			die_errno(_("could not read SQUASH_MSG"));
-- 
Best regards,
 Sven Strickroth
 PGP key id F5A9D4C4 @ any key-server

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH] Also read SQUASH_MSG if a conflict on a merge squash occurred
  2016-03-08  4:17 ` [PATCH] Also read SQUASH_MSG if a conflict on a merge squash occurred Sven Strickroth
@ 2016-03-08 18:32   ` Junio C Hamano
  2016-03-08 18:49     ` Sven Strickroth
                       ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Junio C Hamano @ 2016-03-08 18:32 UTC (permalink / raw)
  To: Sven Strickroth; +Cc: Git List

Sven Strickroth <sven@cs-ware.de> writes:

> Subject: Re: [PATCH] Also read SQUASH_MSG if a conflict on a merge squash occurred

A reader sees this line in the output of "git shortlog --no-merges";
does it sufficiently tell her which Git subcommand is affected by
this change, if this is a bugfix or a new feature, i.e. enough for
her to decide how important the change is?

We often prefix our log message with the name of the area followed
by a colon and describe the purpose of the change, not the means how
the objective is achieved, e.g.

    Subject: [PATCH] commit: do not lose SQUASH_MSG contents

    When concluding a conflicted "git merge --squash", the command
    failed to read SQUASH_MSG that was prepared by "git merge", and
    showed only the "# Conflicts:" list of conflicted paths.

> diff --git a/builtin/commit.c b/builtin/commit.c
> index d054f84..0405d68 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -729,6 +729,12 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>  		if (strbuf_read_file(&sb, git_path_merge_msg(), 0) < 0)
>  			die_errno(_("could not read MERGE_MSG"));
>  		hook_arg1 = "merge";
> +		/* append SQUASH_MSG here if it exists and a merge --squash was originally performed */

	/*
         * Our multi-line comment reads more like
         * this.  That is, the first slash-asterisk is on its
         * own line, so is the last asterisk-slash.
         */

> +		if (!stat(git_path_squash_msg(), &statbuf)) {
> +			if (strbuf_read_file(&sb, git_path_squash_msg(), 0) < 0)
> +				die_errno(_("could not read SQUASH_MSG"));
> +			hook_arg1 = "squash";
> +		}
>  	} else if (!stat(git_path_squash_msg(), &statbuf)) {
>  		if (strbuf_read_file(&sb, git_path_squash_msg(), 0) < 0)
>  			die_errno(_("could not read SQUASH_MSG"));

This reads MERGE_MSG first and then SQUASH_MSG; is that what we
really want?  When you are resolving a conflicted rebase, you would
see the original log message and then conflicts section.  What is in
the SQUASH_MSG is the moral equivalent of the "original log message"
but in a less summarized form, so I suspect that the list of conflicts
should come to end.

The duplicated code to read the same file bothers me somewhat.

I wondered if it makes the result easier to follow (and easier to
update) if this part of the code is restructured like this:

	if (file_exists(git_path_merge_msg()) ||
            file_exists(git_path_squash_msg())) {
	    if (file_exists(git_path_squash_msg())) {
		read SQUASH_MSG;
	    }
            if (file_exists(git_path_merge_msg()))
            	read MERGE_MSG;
	    }
            hook_arg1 = "merge";
	}

but I am not sure if that structure is better.

Thanks.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] Also read SQUASH_MSG if a conflict on a merge squash occurred
  2016-03-08 18:32   ` Junio C Hamano
@ 2016-03-08 18:49     ` Sven Strickroth
  2016-03-08 18:51       ` Junio C Hamano
  2016-03-08 19:03     ` [PATCH] commit: do not lose SQUASH_MSG contents Sven Strickroth
  2016-03-09 18:04     ` [PATCH] Also read SQUASH_MSG if a conflict on a merge squash occurred Junio C Hamano
  2 siblings, 1 reply; 13+ messages in thread
From: Sven Strickroth @ 2016-03-08 18:49 UTC (permalink / raw)
  To: Junio C Hamano, Git List

Am 08.03.2016 um 19:32 schrieb Junio C Hamano:
>> +		if (!stat(git_path_squash_msg(), &statbuf)) {
>> +			if (strbuf_read_file(&sb, git_path_squash_msg(), 0) < 0)
>> +				die_errno(_("could not read SQUASH_MSG"));
>> +			hook_arg1 = "squash";
>> +		}
>>  	} else if (!stat(git_path_squash_msg(), &statbuf)) {
>>  		if (strbuf_read_file(&sb, git_path_squash_msg(), 0) < 0)
>>  			die_errno(_("could not read SQUASH_MSG"));
> 
> This reads MERGE_MSG first and then SQUASH_MSG; is that what we
> really want?  When you are resolving a conflicted rebase, you would
> see the original log message and then conflicts section.  What is in
> the SQUASH_MSG is the moral equivalent of the "original log message"
> but in a less summarized form, so I suspect that the list of conflicts
> should come to end.

I put them first because the squash commit list could be really long.
I'll put MERGE_MSG at the end...

> The duplicated code to read the same file bothers me somewhat.
> 
> I wondered if it makes the result easier to follow (and easier to
> update) if this part of the code is restructured like this:
> 
> 	if (file_exists(git_path_merge_msg()) ||
>             file_exists(git_path_squash_msg())) {
> 	    if (file_exists(git_path_squash_msg())) {
> 		read SQUASH_MSG;
> 	    }
>             if (file_exists(git_path_merge_msg()))
>             	read MERGE_MSG;
> 	    }
>             hook_arg1 = "merge";
> 	}

Here hook_arg1 would be always "merge" and never "squash"... Before my
change it was only "squash" if no conflict occurred.

-- 
Best regards,
 Sven Strickroth
 PGP key id F5A9D4C4 @ any key-server

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] Also read SQUASH_MSG if a conflict on a merge squash occurred
  2016-03-08 18:49     ` Sven Strickroth
@ 2016-03-08 18:51       ` Junio C Hamano
  0 siblings, 0 replies; 13+ messages in thread
From: Junio C Hamano @ 2016-03-08 18:51 UTC (permalink / raw)
  To: Sven Strickroth; +Cc: Git List

Sven Strickroth <sven@cs-ware.de> writes:

> Here hook_arg1 would be always "merge" and never "squash"... Before my
> change it was only "squash" if no conflict occurred.

Oh, that wasn't an intended change.  It was merely an illustration
of a possible restructuring of the flow to avoid having to have
identical read functions in two separate places.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH] commit: do not lose SQUASH_MSG contents
  2016-03-08 18:32   ` Junio C Hamano
  2016-03-08 18:49     ` Sven Strickroth
@ 2016-03-08 19:03     ` Sven Strickroth
  2016-03-09 18:04     ` [PATCH] Also read SQUASH_MSG if a conflict on a merge squash occurred Junio C Hamano
  2 siblings, 0 replies; 13+ messages in thread
From: Sven Strickroth @ 2016-03-08 19:03 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git List

When concluding a conflicted "git merge --squash", the command
failed to read SQUASH_MSG that was prepared by "git merge", and
showed only the "# Conflicts:" list of conflicted paths.

Signed-off-by: Sven Strickroth <email@cs-ware.de>
---
 builtin/commit.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index d054f84..0e48e1d 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -725,14 +725,18 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		format_commit_message(commit, "fixup! %s\n\n",
 				      &sb, &ctx);
 		hook_arg1 = "message";
-	} else if (!stat(git_path_merge_msg(), &statbuf)) {
-		if (strbuf_read_file(&sb, git_path_merge_msg(), 0) < 0)
-			die_errno(_("could not read MERGE_MSG"));
-		hook_arg1 = "merge";
-	} else if (!stat(git_path_squash_msg(), &statbuf)) {
-		if (strbuf_read_file(&sb, git_path_squash_msg(), 0) < 0)
-			die_errno(_("could not read SQUASH_MSG"));
-		hook_arg1 = "squash";
+	} else if (!stat(git_path_squash_msg(), &statbuf) ||
+			   !stat(git_path_merge_msg(), &statbuf)) {
+		if (!stat(git_path_squash_msg(), &statbuf)) {
+			if (strbuf_read_file(&sb, git_path_squash_msg(), 0) < 0)
+				die_errno(_("could not read SQUASH_MSG"));
+			hook_arg1 = "squash";
+		} else
+			hook_arg1 = "merge";
+		if (!stat(git_path_merge_msg(), &statbuf)) {
+			if (strbuf_read_file(&sb, git_path_merge_msg(), 0) < 0)
+				die_errno(_("could not read MERGE_MSG"));
+		}
 	} else if (template_file) {
 		if (strbuf_read_file(&sb, template_file, 0) < 0)
 			die_errno(_("could not read '%s'"), template_file);
-- 
2.7.0.windows.1

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH] Also read SQUASH_MSG if a conflict on a merge squash occurred
  2016-03-08 18:32   ` Junio C Hamano
  2016-03-08 18:49     ` Sven Strickroth
  2016-03-08 19:03     ` [PATCH] commit: do not lose SQUASH_MSG contents Sven Strickroth
@ 2016-03-09 18:04     ` Junio C Hamano
  2016-03-09 20:24       ` Junio C Hamano
  2 siblings, 1 reply; 13+ messages in thread
From: Junio C Hamano @ 2016-03-09 18:04 UTC (permalink / raw)
  To: Sven Strickroth; +Cc: Git List

Junio C Hamano <gitster@pobox.com> writes:

> The duplicated code to read the same file bothers me somewhat.
>
> I wondered if it makes the result easier to follow (and easier to
> update) if this part of the code is restructured like this:
>
> 	if (file_exists(git_path_merge_msg()) ||
>             file_exists(git_path_squash_msg())) {
> 	    if (file_exists(git_path_squash_msg())) {
> 		read SQUASH_MSG;
> 	    }
>             if (file_exists(git_path_merge_msg()))
>             	read MERGE_MSG;
> 	    }
>             hook_arg1 = "merge";
> 	}
>
> but I am not sure if that structure is better.

... as this duplicates file_exists() call to the same thing, which
is no better than duplicated calls to read *_MSG files.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] Also read SQUASH_MSG if a conflict on a merge squash occurred
  2016-03-09 18:04     ` [PATCH] Also read SQUASH_MSG if a conflict on a merge squash occurred Junio C Hamano
@ 2016-03-09 20:24       ` Junio C Hamano
  2016-03-13 18:39         ` [PATCH] commit: do not lose SQUASH_MSG contents Sven Strickroth
  0 siblings, 1 reply; 13+ messages in thread
From: Junio C Hamano @ 2016-03-09 20:24 UTC (permalink / raw)
  To: Sven Strickroth; +Cc: Git List

Junio C Hamano <gitster@pobox.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> The duplicated code to read the same file bothers me somewhat.
>>
>> I wondered if it makes the result easier to follow (and easier to
>> update) if this part of the code is restructured like this:
>>
>> 	if (file_exists(git_path_merge_msg()) ||
>>             file_exists(git_path_squash_msg())) {
>> 	    if (file_exists(git_path_squash_msg())) {
>> 		read SQUASH_MSG;
>> 	    }
>>             if (file_exists(git_path_merge_msg()))
>>             	read MERGE_MSG;
>> 	    }
>>             hook_arg1 = "merge";
>> 	}
>>
>> but I am not sure if that structure is better.
>
> ... as this duplicates file_exists() call to the same thing, which
> is no better than duplicated calls to read *_MSG files.

So, let's take the program structure from your original, but fix the
order of the inclusion (and the log message), perhaps like the
attached patch.

Don't we also want to have a new test so that this "contents from
both files are included in the result in the expected order" feature
will not get broken in the future?

-- >8 --
Subject: [PATCH] commit: do not lose SQUASH_MSG contents

When concluding a conflicted "git merge --squash", the command
failed to read SQUASH_MSG that was prepared by "git merge", and
showed only the "# Conflicts:" list of conflicted paths.

Place the contents from SQUASH_MSG at the beginning, just like we
show the commit log skeleton first when concluding a normal merge,
and then show the "# Conflicts:" list, to help the user write the
log message for the resulting commit.

Signed-off-by: Sven Strickroth <sven@cs-ware.de>
---

 builtin/commit.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index b3bd2d4..4ad3931 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -726,9 +726,19 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 				      &sb, &ctx);
 		hook_arg1 = "message";
 	} else if (!stat(git_path_merge_msg(), &statbuf)) {
+		hook_arg1 = "merge";
+
+		/*
+		 * In a conflicted 'merge squash', the material to help
+		 * writing the log message is found in SQUASH_MSG.
+		 */
+		if (!stat(git_path_squash_msg(), &statbuf)) {
+			if (strbuf_read_file(&sb, git_path_squash_msg(), 0) < 0)
+				die_errno(_("could not read SQUASH_MSG"));
+			hook_arg1 = "squash";
+		}
 		if (strbuf_read_file(&sb, git_path_merge_msg(), 0) < 0)
 			die_errno(_("could not read MERGE_MSG"));
-		hook_arg1 = "merge";
 	} else if (!stat(git_path_squash_msg(), &statbuf)) {
 		if (strbuf_read_file(&sb, git_path_squash_msg(), 0) < 0)
 			die_errno(_("could not read SQUASH_MSG"));
-- 
2.8.0-rc1-141-gbaa22e3

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH] commit: do not lose SQUASH_MSG contents
  2016-03-09 20:24       ` Junio C Hamano
@ 2016-03-13 18:39         ` Sven Strickroth
  2016-03-14 18:19           ` Junio C Hamano
  0 siblings, 1 reply; 13+ messages in thread
From: Sven Strickroth @ 2016-03-13 18:39 UTC (permalink / raw)
  To: Junio C Hamano, Git List

When concluding a conflicted "git merge --squash", the command
failed to read SQUASH_MSG that was prepared by "git merge", and
showed only the "# Conflicts:" list of conflicted paths.

Place the contents from SQUASH_MSG at the beginning, just like we
show the commit log skeleton first when concluding a normal merge,
and then show the "# Conflicts:" list, to help the user write the
log message for the resulting commit.

Signed-off-by: Sven Strickroth <sven@cs-ware.de>
---
 builtin/commit.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index d054f84..d40b788 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -726,9 +726,18 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 				      &sb, &ctx);
 		hook_arg1 = "message";
 	} else if (!stat(git_path_merge_msg(), &statbuf)) {
+		/*
+		 * prepend SQUASH_MSG here if it exists and a
+		 * "merge --squash" was originally performed
+		*/
+		if (!stat(git_path_squash_msg(), &statbuf)) {
+			if (strbuf_read_file(&sb, git_path_squash_msg(), 0) < 0)
+				die_errno(_("could not read SQUASH_MSG"));
+			hook_arg1 = "squash";
+		} else
+			hook_arg1 = "merge";
 		if (strbuf_read_file(&sb, git_path_merge_msg(), 0) < 0)
 			die_errno(_("could not read MERGE_MSG"));
-		hook_arg1 = "merge";
 	} else if (!stat(git_path_squash_msg(), &statbuf)) {
 		if (strbuf_read_file(&sb, git_path_squash_msg(), 0) < 0)
 			die_errno(_("could not read SQUASH_MSG"));
-- 
Best regards,
 Sven Strickroth
 PGP key id F5A9D4C4 @ any key-server

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH] commit: do not lose SQUASH_MSG contents
  2016-03-13 18:39         ` [PATCH] commit: do not lose SQUASH_MSG contents Sven Strickroth
@ 2016-03-14 18:19           ` Junio C Hamano
  2016-03-14 20:19             ` Junio C Hamano
  0 siblings, 1 reply; 13+ messages in thread
From: Junio C Hamano @ 2016-03-14 18:19 UTC (permalink / raw)
  To: Sven Strickroth; +Cc: Git List

Sven Strickroth <sven@cs-ware.de> writes:

> When concluding a conflicted "git merge --squash", the command
> failed to read SQUASH_MSG that was prepared by "git merge", and
> showed only the "# Conflicts:" list of conflicted paths.
>
> Place the contents from SQUASH_MSG at the beginning, just like we
> show the commit log skeleton first when concluding a normal merge,
> and then show the "# Conflicts:" list, to help the user write the
> log message for the resulting commit.
>
> Signed-off-by: Sven Strickroth <sven@cs-ware.de>
> ---
>  builtin/commit.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)

The updated code looks good to me; sorry for misleading you with
fuzzy comments earlier.

We may want to have a test to prevent this from getting broken in
the future updates.

> diff --git a/builtin/commit.c b/builtin/commit.c
> index d054f84..d40b788 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -726,9 +726,18 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>  				      &sb, &ctx);
>  		hook_arg1 = "message";
>  	} else if (!stat(git_path_merge_msg(), &statbuf)) {
> +		/*
> +		 * prepend SQUASH_MSG here if it exists and a
> +		 * "merge --squash" was originally performed
> +		*/
> +		if (!stat(git_path_squash_msg(), &statbuf)) {
> +			if (strbuf_read_file(&sb, git_path_squash_msg(), 0) < 0)
> +				die_errno(_("could not read SQUASH_MSG"));
> +			hook_arg1 = "squash";
> +		} else
> +			hook_arg1 = "merge";
>  		if (strbuf_read_file(&sb, git_path_merge_msg(), 0) < 0)
>  			die_errno(_("could not read MERGE_MSG"));
> -		hook_arg1 = "merge";
>  	} else if (!stat(git_path_squash_msg(), &statbuf)) {
>  		if (strbuf_read_file(&sb, git_path_squash_msg(), 0) < 0)
>  			die_errno(_("could not read SQUASH_MSG"));

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] commit: do not lose SQUASH_MSG contents
  2016-03-14 18:19           ` Junio C Hamano
@ 2016-03-14 20:19             ` Junio C Hamano
  2016-03-21 22:29               ` Sven Strickroth
  0 siblings, 1 reply; 13+ messages in thread
From: Junio C Hamano @ 2016-03-14 20:19 UTC (permalink / raw)
  To: Sven Strickroth; +Cc: Git List

Junio C Hamano <gitster@pobox.com> writes:

>> Place the contents from SQUASH_MSG at the beginning, just like we
>> show the commit log skeleton first when concluding a normal merge,
>> and then show the "# Conflicts:" list, to help the user write the
>> log message for the resulting commit.
>>
>> Signed-off-by: Sven Strickroth <sven@cs-ware.de>
>> ---
>>  builtin/commit.c | 11 ++++++++++-
>>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> The updated code looks good to me; sorry for misleading you with
> fuzzy comments earlier.
>
> We may want to have a test to prevent this from getting broken in
> the future updates.

Perhaps like so:

 t/t7600-merge.sh | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index 75c50ee..55b9da4 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -33,9 +33,11 @@ printf '%s\n' 1 2 3 4 5 6 7 8 9 >file
 printf '%s\n' '1 X' 2 3 4 5 6 7 8 9 >file.1
 printf '%s\n' 1 2 3 4 '5 X' 6 7 8 9 >file.5
 printf '%s\n' 1 2 3 4 5 6 7 8 '9 X' >file.9
+printf '%s\n' 1 2 3 4 5 6 7 8 '9 Y' >file.9y
 printf '%s\n' '1 X' 2 3 4 5 6 7 8 9 >result.1
 printf '%s\n' '1 X' 2 3 4 '5 X' 6 7 8 9 >result.1-5
 printf '%s\n' '1 X' 2 3 4 '5 X' 6 7 8 '9 X' >result.1-5-9
+printf '%s\n' 1 2 3 4 5 6 7 8 '9 Z' >result.9z
 >empty
 
 create_merge_msgs () {
@@ -128,6 +130,12 @@ test_expect_success 'setup' '
 	git tag c2 &&
 	c2=$(git rev-parse HEAD) &&
 	git reset --hard "$c0" &&
+	cp file.9y file &&
+	git add file &&
+	test_tick &&
+	git commit -m "commit 7" &&
+	git tag c7 &&
+	git reset --hard "$c0" &&
 	cp file.9 file &&
 	git add file &&
 	test_tick &&
@@ -218,6 +226,26 @@ test_expect_success 'merge c1 with c2' '
 	verify_parents $c1 $c2
 '
 
+test_expect_success 'merge --squash c3 with c7' '
+	git reset --hard c3 &&
+	test_must_fail git merge --squash c7 &&
+	cat result.9z >file &&
+	git commit --no-edit -a &&
+
+	{
+		cat <<-EOF
+		Squashed commit of the following:
+
+		$(git show -s c7)
+
+		# Conflicts:
+		#	file
+		EOF
+	} >expect &&
+	git cat-file commit HEAD | sed -e '1,/^$/d' >actual &&
+	test_cmp expect actual
+'
+
 test_debug 'git log --graph --decorate --oneline --all'
 
 test_expect_success 'merge c1 with c2 and c3' '
-- 
2.8.0-rc2-154-gd146b22

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH] commit: do not lose SQUASH_MSG contents
  2016-03-14 20:19             ` Junio C Hamano
@ 2016-03-21 22:29               ` Sven Strickroth
  2016-03-21 22:34                 ` Junio C Hamano
  0 siblings, 1 reply; 13+ messages in thread
From: Sven Strickroth @ 2016-03-21 22:29 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git List

When concluding a conflicted "git merge --squash", the command
failed to read SQUASH_MSG that was prepared by "git merge", and
showed only the "# Conflicts:" list of conflicted paths.

Place the contents from SQUASH_MSG at the beginning, just like we
show the commit log skeleton first when concluding a normal merge,
and then show the "# Conflicts:" list, to help the user write the
log message for the resulting commit.

Test by Junio C Hamano <gitster@pobox.com>.

Signed-off-by: Sven Strickroth <sven@cs-ware.de>
---
 builtin/commit.c | 11 ++++++++++-
 t/t7600-merge.sh | 28 ++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index d054f84..d40b788 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -726,9 +726,18 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 				      &sb, &ctx);
 		hook_arg1 = "message";
 	} else if (!stat(git_path_merge_msg(), &statbuf)) {
+		/*
+		 * prepend SQUASH_MSG here if it exists and a
+		 * "merge --squash" was originally performed
+		*/
+		if (!stat(git_path_squash_msg(), &statbuf)) {
+			if (strbuf_read_file(&sb, git_path_squash_msg(), 0) < 0)
+				die_errno(_("could not read SQUASH_MSG"));
+			hook_arg1 = "squash";
+		} else
+			hook_arg1 = "merge";
 		if (strbuf_read_file(&sb, git_path_merge_msg(), 0) < 0)
 			die_errno(_("could not read MERGE_MSG"));
-		hook_arg1 = "merge";
 	} else if (!stat(git_path_squash_msg(), &statbuf)) {
 		if (strbuf_read_file(&sb, git_path_squash_msg(), 0) < 0)
 			die_errno(_("could not read SQUASH_MSG"));
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index 302e238..ba35e00 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -33,9 +33,11 @@ printf '%s\n' 1 2 3 4 5 6 7 8 9 >file
 printf '%s\n' '1 X' 2 3 4 5 6 7 8 9 >file.1
 printf '%s\n' 1 2 3 4 '5 X' 6 7 8 9 >file.5
 printf '%s\n' 1 2 3 4 5 6 7 8 '9 X' >file.9
+printf '%s\n' 1 2 3 4 5 6 7 8 '9 Y' >file.9y
 printf '%s\n' '1 X' 2 3 4 5 6 7 8 9 >result.1
 printf '%s\n' '1 X' 2 3 4 '5 X' 6 7 8 9 >result.1-5
 printf '%s\n' '1 X' 2 3 4 '5 X' 6 7 8 '9 X' >result.1-5-9
+printf '%s\n' 1 2 3 4 5 6 7 8 '9 Z' >result.9z
 >empty
 
 create_merge_msgs () {
@@ -128,6 +130,12 @@ test_expect_success 'setup' '
 	git tag c2 &&
 	c2=$(git rev-parse HEAD) &&
 	git reset --hard "$c0" &&
+	cp file.9y file &&
+	git add file &&
+	test_tick &&
+	git commit -m "commit 7" &&
+	git tag c7 &&
+	git reset --hard "$c0" &&
 	cp file.9 file &&
 	git add file &&
 	test_tick &&
@@ -218,6 +226,26 @@ test_expect_success 'merge c1 with c2' '
 	verify_parents $c1 $c2
 '
 
+test_expect_success 'merge --squash c3 with c7' '
+	git reset --hard c3 &&
+	test_must_fail git merge --squash c7 &&
+	cat result.9z >file &&
+	git commit --no-edit -a &&
+
+	{
+		cat <<-EOF
+		Squashed commit of the following:
+
+		$(git show -s c7)
+
+		# Conflicts:
+		#	file
+		EOF
+	} >expect &&
+	git cat-file commit HEAD | sed -e '1,/^$/d' >actual &&
+	test_cmp expect actual
+'
+
 test_debug 'git log --graph --decorate --oneline --all'
 
 test_expect_success 'merge c1 with c2 and c3' '
-- 
2.7.4.windows.1

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH] commit: do not lose SQUASH_MSG contents
  2016-03-21 22:29               ` Sven Strickroth
@ 2016-03-21 22:34                 ` Junio C Hamano
  0 siblings, 0 replies; 13+ messages in thread
From: Junio C Hamano @ 2016-03-21 22:34 UTC (permalink / raw)
  To: Sven Strickroth; +Cc: Git List

Sven Strickroth <sven@cs-ware.de> writes:

> When concluding a conflicted "git merge --squash", the command
> failed to read SQUASH_MSG that was prepared by "git merge", and
> showed only the "# Conflicts:" list of conflicted paths.
>
> Place the contents from SQUASH_MSG at the beginning, just like we
> show the commit log skeleton first when concluding a normal merge,
> and then show the "# Conflicts:" list, to help the user write the
> log message for the resulting commit.
>
> Test by Junio C Hamano <gitster@pobox.com>.
>
> Signed-off-by: Sven Strickroth <sven@cs-ware.de>
> ---

You must somehow read my mind, as I was about to send a friendly
ping to you saying "unless you have a reroll, I'll squash the test
in" ;-)

Will replace those two commits with this one (after fixing one nit).

Thanks.

>  builtin/commit.c | 11 ++++++++++-
>  t/t7600-merge.sh | 28 ++++++++++++++++++++++++++++
>  2 files changed, 38 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index d054f84..d40b788 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -726,9 +726,18 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>  				      &sb, &ctx);
>  		hook_arg1 = "message";
>  	} else if (!stat(git_path_merge_msg(), &statbuf)) {
> +		/*
> +		 * prepend SQUASH_MSG here if it exists and a
> +		 * "merge --squash" was originally performed
> +		*/

Here is a nit ("*/" needs one more space indent to align).

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2016-03-21 22:34 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-05 10:38 Commit message not helpful after merge squash with conflicts Sven Strickroth
2016-03-08  4:17 ` [PATCH] Also read SQUASH_MSG if a conflict on a merge squash occurred Sven Strickroth
2016-03-08 18:32   ` Junio C Hamano
2016-03-08 18:49     ` Sven Strickroth
2016-03-08 18:51       ` Junio C Hamano
2016-03-08 19:03     ` [PATCH] commit: do not lose SQUASH_MSG contents Sven Strickroth
2016-03-09 18:04     ` [PATCH] Also read SQUASH_MSG if a conflict on a merge squash occurred Junio C Hamano
2016-03-09 20:24       ` Junio C Hamano
2016-03-13 18:39         ` [PATCH] commit: do not lose SQUASH_MSG contents Sven Strickroth
2016-03-14 18:19           ` Junio C Hamano
2016-03-14 20:19             ` Junio C Hamano
2016-03-21 22:29               ` Sven Strickroth
2016-03-21 22:34                 ` Junio C Hamano

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