Re: [PATCH v3 1/1] merge: add scissors line on merge conflict

2018-11-18 Thread SZEDER Gábor
On Sat, Nov 17, 2018 at 06:32:33PM -0500, Denton Liu wrote:
> diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
> index 106148254d..0d3db34f08 100755
> --- a/t/t7600-merge.sh
> +++ b/t/t7600-merge.sh
> @@ -247,6 +247,54 @@ test_expect_success 'merge --squash c3 with c7' '
>   test_cmp expect actual
>  '
>  
> +test_expect_success 'merge c3 with c7 with commit.cleanup = scissors' '
> + git config commit.cleanup scissors &&
> + git reset --hard c3 &&
> + test_must_fail git merge c7 &&
> + cat result.9z >file &&
> + git commit --no-edit -a &&
> +
> + {
> + cat <<-EOF
> + Merge tag '"'"'c7'"'"'
> +
> + #  >8 
> + # Do not modify or remove the line above.
> + # Everything below it will be ignored.

Note that these two lines of advice text are translated; see the
consequences below.

> + #
> + # Conflicts:
> + #   file
> + EOF
> + } >expect &&

The {...} block is unnecessary, because there is only a single command
in there.

> + git cat-file commit HEAD | sed -e '1,/^$/d' >actual &&

Please don't run git commands upstream of a pipe, because the pipe
hides their exit code.  Furthermore, put the sed script inside double
quotes, because the whole test is already in a single-quoted block.

I presume you wrote the test this way because you simply followed suit
of the previous test 'merge --squash c3 with c7', which did all the
same.  Bonus points for a preparatory patch that cleans up the
previous test ;)

> + test_cmp expect actual

But most importantly, here 'test_cmp' compares translated advice text
as well, which fails in the GETTEXT_POISON build.  Use 'test_i18ncmp'
instead.

> +'
> +
> +test_expect_success 'merge c3 with c7 with --squash commit.cleanup = 
> scissors' '
> + git config commit.cleanup scissors &&
> + 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)
> +
> + #  >8 
> + # Do not modify or remove the line above.
> + # Everything below it will be ignored.
> + #
> + # Conflicts:
> + #   file
> + EOF
> + } >expect &&
> + git cat-file commit HEAD | sed -e '1,/^$/d' >actual &&
> + test_cmp expect actual

Likewise.

> +'
> +
>  test_debug 'git log --graph --decorate --oneline --all'
>  
>  test_expect_success 'merge c1 with c2 and c3' '
> -- 
> 2.19.1
> 


[PATCH v3 1/1] merge: add scissors line on merge conflict

2018-11-17 Thread Denton Liu
This fixes a bug where the scissors line is placed after the Conflicts:
section, in the case where a merge conflict occurs and
merge.cleanup = scissors.

Next, if commit.cleanup = scissors is specified, don't produce a
scissors line in commit if one already exists in the MERGE_MSG file.

Finally, we give pull the passthrough option of --cleanup so that it
can also take advantage of this change.

Signed-off-by: Denton Liu 
---
 Documentation/merge-options.txt |  6 +
 builtin/commit.c| 20 ++
 builtin/merge.c | 16 +++
 builtin/pull.c  |  6 +
 t/t7600-merge.sh| 48 +
 5 files changed, 91 insertions(+), 5 deletions(-)

diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index 63a3fc0954..115e0ca6f0 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -27,6 +27,12 @@ they run `git merge`. To make it easier to adjust such 
scripts to the
 updated behaviour, the environment variable `GIT_MERGE_AUTOEDIT` can be
 set to `no` at the beginning of them.
 
+--cleanup=::
+   This option determines how the merge message will be cleaned up
+   before being passed on. Specifically, if the '' is given a
+   value of `scissors`, scissors will be prepended to the message in
+   the case of a merge conflict. See also linkgit:git-commit[1].
+
 --ff::
When the merge resolves as a fast-forward, only update the branch
pointer, without creating a merge commit.  This is the default
diff --git a/builtin/commit.c b/builtin/commit.c
index 0d9828e29e..7902645bc9 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -659,6 +659,7 @@ static int prepare_to_commit(const char *index_file, const 
char *prefix,
const char *hook_arg2 = NULL;
int clean_message_contents = (cleanup_mode != COMMIT_MSG_CLEANUP_NONE);
int old_display_comment_prefix;
+   int merge_contains_scissors = 0;
 
/* This checks and barfs if author is badly specified */
determine_author_info(author_ident);
@@ -719,6 +720,8 @@ static int prepare_to_commit(const char *index_file, const 
char *prefix,
strbuf_addbuf(&sb, &message);
hook_arg1 = "message";
} else if (!stat(git_path_merge_msg(the_repository), &statbuf)) {
+   size_t merge_msg_start;
+
/*
 * prepend SQUASH_MSG here if it exists and a
 * "merge --squash" was originally performed
@@ -729,8 +732,14 @@ static int prepare_to_commit(const char *index_file, const 
char *prefix,
hook_arg1 = "squash";
} else
hook_arg1 = "merge";
+
+   merge_msg_start = sb.len;
if (strbuf_read_file(&sb, git_path_merge_msg(the_repository), 
0) < 0)
die_errno(_("could not read MERGE_MSG"));
+
+   if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS &&
+   wt_status_locate_end(sb.buf + merge_msg_start, sb.len - 
merge_msg_start) < sb.len - merge_msg_start)
+   merge_contains_scissors = 1;
} else if (!stat(git_path_squash_msg(the_repository), &statbuf)) {
if (strbuf_read_file(&sb, git_path_squash_msg(the_repository), 
0) < 0)
die_errno(_("could not read SQUASH_MSG"));
@@ -798,7 +807,8 @@ static int prepare_to_commit(const char *index_file, const 
char *prefix,
struct ident_split ci, ai;
 
if (whence != FROM_COMMIT) {
-   if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS)
+   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
@@ -823,10 +833,10 @@ static int prepare_to_commit(const char *index_file, 
const char *prefix,
_("Please enter the commit message for your 
changes."
  " Lines starting\nwith '%c' will be ignored, 
and an empty"
  " message aborts the commit.\n"), 
comment_line_char);
-   else if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS &&
-whence == FROM_COMMIT)
-   wt_status_add_cut_line(s->fp);
-   else /* COMMIT_MSG_CLEANUP_SPACE, that is. */
+   else if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS) {
+   if (whence == FROM_COMMIT && !merge_contains_scissors)
+   wt_status_add_cut_line(s->fp);
+   } else /* COMMIT_MSG_CLEANUP_SPACE, that is. */
status_printf(s, GIT_COLOR_NORMAL,
_("Please