cherry-pick: don't forget -s on failure

In case 'git cherry-pick -s <commit>' failed, the user had to use 'git
commit -s' (i.e. state the -s option again), which is easy to forget
about.  Instead, write the signed-off-by line early, so plain 'git
commit' will have the same result.

Also update 'git commit -s', so that in case there is already a relevant
Signed-off-by line before the Conflicts: line, it won't add one more at
the end of the message. If there is no such line, then add it before the
the Conflicts: line.

Signed-off-by: Miklos Vajna <vmiklos@suse.cz>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/commit.c b/builtin/commit.c
index 778cf16fde64ab81bbe75ec0949a77e6fd9ee702..4d50484837ece50276ca7a09239815dcca5b5f7d 100644 (file)
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -28,6 +28,7 @@
 #include "submodule.h"
 #include "gpg-interface.h"
 #include "column.h"
+#include "sequencer.h"
 
 static const char * const builtin_commit_usage[] = {
        N_("git commit [options] [--] <filepattern>..."),
@@ -466,8 +467,6 @@ static int is_a_merge(const struct commit *current_head)
        return !!(current_head->parents && current_head->parents->next);
 }
 
-static const char sign_off_header[] = "Signed-off-by: ";
-
 static void export_one(const char *var, const char *s, const char *e, int hack)
 {
        struct strbuf buf = STRBUF_INIT;
@@ -552,47 +551,6 @@ static void determine_author_info(struct strbuf *author_ident)
        }
 }
 
-static int ends_rfc2822_footer(struct strbuf *sb)
-{
-       int ch;
-       int hit = 0;
-       int i, j, k;
-       int len = sb->len;
-       int first = 1;
-       const char *buf = sb->buf;
-
-       for (i = len - 1; i > 0; i--) {
-               if (hit && buf[i] == '\n')
-                       break;
-               hit = (buf[i] == '\n');
-       }
-
-       while (i < len - 1 && buf[i] == '\n')
-               i++;
-
-       for (; i < len; i = k) {
-               for (k = i; k < len && buf[k] != '\n'; k++)
-                       ; /* do nothing */
-               k++;
-
-               if ((buf[k] == ' ' || buf[k] == '\t') && !first)
-                       continue;
-
-               first = 0;
-
-               for (j = 0; i + j < len; j++) {
-                       ch = buf[i + j];
-                       if (ch == ':')
-                               break;
-                       if (isalnum(ch) ||
-                           (ch == '-'))
-                               continue;
-                       return 0;
-               }
-       }
-       return 1;
-}
-
 static char *cut_ident_timestamp_part(char *string)
 {
        char *ket = strrchr(string, '>');
@@ -717,21 +675,30 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
                stripspace(&sb, 0);
 
        if (signoff) {
-               struct strbuf sob = STRBUF_INIT;
-               int i;
-
-               strbuf_addstr(&sob, sign_off_header);
-               strbuf_addstr(&sob, fmt_name(getenv("GIT_COMMITTER_NAME"),
-                                            getenv("GIT_COMMITTER_EMAIL")));
-               strbuf_addch(&sob, '\n');
-               for (i = sb.len - 1; i > 0 && sb.buf[i - 1] != '\n'; i--)
-                       ; /* do nothing */
-               if (prefixcmp(sb.buf + i, sob.buf)) {
-                       if (!i || !ends_rfc2822_footer(&sb))
-                               strbuf_addch(&sb, '\n');
-                       strbuf_addbuf(&sb, &sob);
+               /*
+                * See if we have a Conflicts: block at the end. If yes, count
+                * its size, so we can ignore it.
+                */
+               int ignore_footer = 0;
+               int i, eol, previous = 0;
+               const char *nl;
+
+               for (i = 0; i < sb.len; i++) {
+                       nl = memchr(sb.buf + i, '\n', sb.len - i);
+                       if (nl)
+                               eol = nl - sb.buf;
+                       else
+                               eol = sb.len;
+                       if (!prefixcmp(sb.buf + previous, "\nConflicts:\n")) {
+                               ignore_footer = sb.len - previous;
+                               break;
+                       }
+                       while (i < eol)
+                               i++;
+                       previous = eol;
                }
-               strbuf_release(&sob);
+
+               append_signoff(&sb, ignore_footer);
        }
 
        if (fwrite(sb.buf, 1, sb.len, s->fp) < sb.len)

diff --git a/sequencer.c b/sequencer.c
index f86f116a15326f514fbe0405a9301f4ee2416344..dbef5cea027f61a8172c4889005c090f1d0df048 100644 (file)
--- a/sequencer.c
+++ b/sequencer.c
@@ -17,6 +17,8 @@
 
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 
+const char sign_off_header[] = "Signed-off-by: ";
+
 void remove_sequencer_state(void)
 {
        struct strbuf seq_dir = STRBUF_INIT;
@@ -233,6 +235,9 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
                die(_("%s: Unable to write new index file"), action_name(opts));
        rollback_lock_file(&index_lock);
 
+       if (opts->signoff)
+               append_signoff(msgbuf, 0);
+
        if (!clean) {
                int i;
                strbuf_addstr(msgbuf, "\nConflicts:\n");
@@ -1011,3 +1016,63 @@ int sequencer_pick_revisions(struct replay_opts *opts)
        save_opts(opts);
        return pick_commits(todo_list, opts);
 }
+
+static int ends_rfc2822_footer(struct strbuf *sb, int ignore_footer)
+{
+       int ch;
+       int hit = 0;
+       int i, j, k;
+       int len = sb->len - ignore_footer;
+       int first = 1;
+       const char *buf = sb->buf;
+
+       for (i = len - 1; i > 0; i--) {
+               if (hit && buf[i] == '\n')
+                       break;
+               hit = (buf[i] == '\n');
+       }
+
+       while (i < len - 1 && buf[i] == '\n')
+               i++;
+
+       for (; i < len; i = k) {
+               for (k = i; k < len && buf[k] != '\n'; k++)
+                       ; /* do nothing */
+               k++;
+
+               if ((buf[k] == ' ' || buf[k] == '\t') && !first)
+                       continue;
+
+               first = 0;
+
+               for (j = 0; i + j < len; j++) {
+                       ch = buf[i + j];
+                       if (ch == ':')
+                               break;
+                       if (isalnum(ch) ||
+                           (ch == '-'))
+                               continue;
+                       return 0;
+               }
+       }
+       return 1;
+}
+
+void append_signoff(struct strbuf *msgbuf, int ignore_footer)
+{
+       struct strbuf sob = STRBUF_INIT;
+       int i;
+
+       strbuf_addstr(&sob, sign_off_header);
+       strbuf_addstr(&sob, fmt_name(getenv("GIT_COMMITTER_NAME"),
+                               getenv("GIT_COMMITTER_EMAIL")));
+       strbuf_addch(&sob, '\n');
+       for (i = msgbuf->len - 1 - ignore_footer; i > 0 && msgbuf->buf[i - 1] != '\n'; i--)
+               ; /* do nothing */
+       if (prefixcmp(msgbuf->buf + i, sob.buf)) {
+               if (!i || !ends_rfc2822_footer(msgbuf, ignore_footer))
+                       strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, "\n", 1);
+               strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, sob.buf, sob.len);
+       }
+       strbuf_release(&sob);
+}

diff --git a/sequencer.h b/sequencer.h
index d8494201e0a0ba052356de1094b25bac5b9024c0..60287b8da2cddf635bc5ef13121e33e3cf25047c 100644 (file)
--- a/sequencer.h
+++ b/sequencer.h
@@ -49,4 +49,8 @@ extern void remove_sequencer_state(void);
 
 int sequencer_pick_revisions(struct replay_opts *opts);
 
+extern const char sign_off_header[];
+
+void append_signoff(struct strbuf *msgbuf, int ignore_footer);
+
 #endif

diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
index 0c81b3c427af47f0b0ef73170372ef9df70b4ade..c82f7210c4ca10e35d9d2000aa8da2d10194a6c6 100755 (executable)
--- a/t/t3507-cherry-pick-conflict.sh
+++ b/t/t3507-cherry-pick-conflict.sh
@@ -30,6 +30,7 @@ test_expect_success setup '
        test_commit initial foo a &&
        test_commit base foo b &&
        test_commit picked foo c &&
+       test_commit --signoff picked-signed foo d &&
        git config advice.detachedhead false
 
 '
@@ -340,4 +341,35 @@ test_expect_success 'revert conflict, diff3 -m style' '
        test_cmp expected actual
 '
 
+test_expect_success 'failed cherry-pick does not forget -s' '
+       pristine_detach initial &&
+       test_must_fail git cherry-pick -s picked &&
+       test_i18ngrep -e "Signed-off-by" .git/MERGE_MSG
+'
+
+test_expect_success 'commit after failed cherry-pick does not add duplicated -s' '
+       pristine_detach initial &&
+       test_must_fail git cherry-pick -s picked-signed &&
+       git commit -a -s &&
+       test $(git show -s |grep -c "Signed-off-by") = 1
+'
+
+test_expect_success 'commit after failed cherry-pick adds -s at the right place' '
+       pristine_detach initial &&
+       test_must_fail git cherry-pick picked &&
+       git commit -a -s &&
+       pwd &&
+       cat <<EOF > expected &&
+picked
+
+Signed-off-by: C O Mitter <committer@example.com>
+
+Conflicts:
+       foo
+EOF
+
+       git show -s --pretty=format:%B > actual &&
+       test_cmp expected actual
+'
+
 test_done

diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index f4e6450d6a88ae0abd158e52634c47a23872ad2c..b5fb527b2e686d158a6e7d53d6246b9e66f80a7b 100755 (executable)
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -410,7 +410,7 @@ test_expect_success '--continue respects -x in first commit in multi-pick' '
        grep "cherry picked from.*$picked" msg
 '
 
-test_expect_success '--signoff is not automatically propagated to resolved conflict' '
+test_expect_failure '--signoff is automatically propagated to resolved conflict' '
        pristine_detach initial &&
        test_expect_code 1 git cherry-pick --signoff base..anotherpick &&
        echo "c" >foo &&
@@ -428,7 +428,7 @@ test_expect_success '--signoff is not automatically propagated to resolved confl
        grep "Signed-off-by:" anotherpick_msg
 '
 
-test_expect_success '--signoff dropped for implicit commit of resolution, multi-pick case' '
+test_expect_failure '--signoff dropped for implicit commit of resolution, multi-pick case' '
        pristine_detach initial &&
        test_must_fail git cherry-pick -s picked anotherpick &&
        echo c >foo &&
@@ -441,7 +441,7 @@ test_expect_success '--signoff dropped for implicit commit of resolution, multi-
        ! grep Signed-off-by: msg
 '
 
-test_expect_success 'sign-off needs to be reaffirmed after conflict resolution, single-pick case' '
+test_expect_failure 'sign-off needs to be reaffirmed after conflict resolution, single-pick case' '
        pristine_detach initial &&
        test_must_fail git cherry-pick -s picked &&
        echo c >foo &&

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 9bc57d27e961720c98f75f95ab3186723e77f88b..8889ba5104cd3f7c783a179d45816d3a82685ef0 100644 (file)
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -144,11 +144,22 @@ test_pause () {
 
 test_commit () {
        notick= &&
-       if test "z$1" = "z--notick"
-       then
-               notick=yes
+       signoff= &&
+       while test $# != 0
+       do
+               case "$1" in
+               --notick)
+                       notick=yes
+                       ;;
+               --signoff)
+                       signoff="$1"
+                       ;;
+               *)
+                       break
+                       ;;
+               esac
                shift
-       fi &&
+       done &&
        file=${2:-"$1.t"} &&
        echo "${3-$1}" > "$file" &&
        git add "$file" &&
@@ -156,7 +167,7 @@ test_commit () {
        then
                test_tick
        fi &&
-       git commit -m "$1" &&
+       git commit $signoff -m "$1" &&
        git tag "$1"
 }