Jay Soffian <jaysoffian@xxxxxxxxx> writes:
> Implemented internally instead of as "git merge --no-commit && git commit"
> so that "merge --edit" is otherwise consistent (hooks, etc) with "merge".
>
> Note: the edit message does not include the status information that one
> gets with "commit --status" and it is cleaned up after editing like one
> gets with "commit --cleanup=default". A later patch could add the status
> information if desired.
>
> Note: previously we were not calling stripspace() after running the
> prepare-commit-msg hook. Now we are, stripping comments and
> leading/trailing whitespace lines if --edit is given, otherwise only
> stripping leading/trailing whitespace lines if not given --edit.
>
> Signed-off-by: Jay Soffian <jaysoffian@xxxxxxxxx>
> ---
Thanks.
> +static void prepare_to_commit(void)
> +{
> + struct strbuf msg = STRBUF_INIT;
> + strbuf_addbuf(&msg, &merge_msg);
> + strbuf_addch(&msg, '\n');
> + write_merge_msg(&msg);
> run_hook(get_index_file(), "prepare-commit-msg",
> git_path("MERGE_MSG"), "merge", NULL, NULL);
> - read_merge_msg();
> + if (option_edit) {
> + if (launch_editor(git_path("MERGE_MSG"), NULL, NULL))
> + abort_commit(NULL);
> + }
> + read_merge_msg(&msg);
> + stripspace(&msg, option_edit);
> + if (!msg.len)
> + abort_commit(_("Empty commit message."));
> + strbuf_release(&merge_msg);
> + strbuf_addbuf(&merge_msg, &msg);
> + strbuf_release(&msg);
> }
An abstraction very nicely done.
I am not sure about the '\n' you unconditionally added at the end of the
existing message.
I think running stripspace(&msg, option_edit) is a good change, even
though some people might feel it is a regression. "git commit" also cleans
up the whitespace cruft left by prepare-commit-message hook when the
editor is not in use, and this change makes it consistent.
> @@ -1015,6 +1041,36 @@ static int setup_with_upstream(const char ***argv)
> ...
> +static void write_merge_state(void)
> +{
> + int fd;
> + struct commit_list *j;
> + struct strbuf buf = STRBUF_INIT;
> +
> + for (j = remoteheads; j; j = j->next)
> + strbuf_addf(&buf, "%s\n",
> + sha1_to_hex(j->item->object.sha1));
> + fd = open(git_path("MERGE_HEAD"), O_WRONLY | O_CREAT, 0666);
> + if (fd < 0)
> + die_errno(_("Could not open '%s' for writing"),
> + git_path("MERGE_HEAD"));
> + if (write_in_full(fd, buf.buf, buf.len) != buf.len)
> + die_errno(_("Could not write to '%s'"), git_path("MERGE_HEAD"));
> + close(fd);
> + strbuf_addch(&merge_msg, '\n');
> + write_merge_msg(&merge_msg);
> + fd = open(git_path("MERGE_MODE"), O_WRONLY | O_CREAT | O_TRUNC, 0666);
> + if (fd < 0)
> + die_errno(_("Could not open '%s' for writing"),
> + git_path("MERGE_MODE"));
> + strbuf_reset(&buf);
> + if (!allow_fast_forward)
> + strbuf_addf(&buf, "no-ff");
> + if (write_in_full(fd, buf.buf, buf.len) != buf.len)
> + die_errno(_("Could not write to '%s'"), git_path("MERGE_MODE"));
> + close(fd);
> +}
Again very nicely done.
> diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
> index 87aac835a1..8c6b811718 100755
> --- a/t/t7600-merge.sh
> +++ b/t/t7600-merge.sh
> @@ -643,4 +643,19 @@ test_expect_success 'amending no-ff merge commit' '
>
> test_debug 'git log --graph --decorate --oneline --all'
>
> +cat >editor <<\EOF
> +#!/bin/sh
> +# strip comments and blank lines from end of message
> +sed -e '/^#/d' < "$1" | sed -e :a -e '/^\n*$/{$d;N;ba' -e '}' > expected
> +EOF
> +chmod 755 editor
I am not sure about this one. Wouldn't this want to be editing the given
file to make sure that the edited content appear in the result, not just
testing the additional stripspace() call you added in the codepath?
> +test_expect_success 'merge --no-ff --edit' '
> + git reset --hard c0 &&
> + EDITOR=./editor git merge --no-ff --edit c1 &&
> + verify_parents $c0 $c1 &&
> + git cat-file commit HEAD | sed "1,/^$/d" > actual &&
> + test_cmp actual expected
> +'
> +
> test_done
So perhaps this one on top? I am just suspecting that your additional '\n'
is to make sure we do not write out a file with an incomplete line with
this patch, but that change is not explained in your commit log message,
so I am not sure.
builtin/merge.c | 3 ++-
t/t7600-merge.sh | 10 +++++++++-
2 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/builtin/merge.c b/builtin/merge.c
index a8dbf4a..09ffc07 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -867,7 +867,8 @@ static void prepare_to_commit(void)
{
struct strbuf msg = STRBUF_INIT;
strbuf_addbuf(&msg, &merge_msg);
- strbuf_addch(&msg, '\n');
+ if (msg.len && msg.buf[msg.len-1] != '\n')
+ strbuf_addch(&msg, '\n');
write_merge_msg(&msg);
run_hook(get_index_file(), "prepare-commit-msg",
git_path("MERGE_MSG"), "merge", NULL, NULL);
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index 8c6b811..3008e4e 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -645,6 +645,12 @@ test_debug 'git log --graph --decorate --oneline --all'
cat >editor <<\EOF
#!/bin/sh
+# Add a new message string that was not in the template
+(
+ echo "Merge work done on the side branch c1"
+ echo
+ cat <"$1"
+) >"$1.tmp" && mv "$1.tmp" "$1"
# strip comments and blank lines from end of message
sed -e '/^#/d' < "$1" | sed -e :a -e '/^\n*$/{$d;N;ba' -e '}' > expected
EOF
@@ -654,7 +660,9 @@ test_expect_success 'merge --no-ff --edit' '
git reset --hard c0 &&
EDITOR=./editor git merge --no-ff --edit c1 &&
verify_parents $c0 $c1 &&
- git cat-file commit HEAD | sed "1,/^$/d" > actual &&
+ git cat-file commit HEAD >raw &&
+ grep "work done on the side branch" raw &&
+ sed "1,/^$/d" >actual raw &&
test_cmp actual expected
'
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html