Merge branch 'jc/builtin-am-signoff-regression-fix'
authorJunio C Hamano <gitster@pobox.com>
Tue, 8 Sep 2015 22:34:57 +0000 (15:34 -0700)
committerJunio C Hamano <gitster@pobox.com>
Tue, 8 Sep 2015 22:35:05 +0000 (15:35 -0700)
Recent "git am" had regression when adding a Signed-off-by line
with its "-s" option by an unintended tightening of how an existing
trailer block is detected.

* jc/builtin-am-signoff-regression-fix:
am: match --signoff to the original scripted version

1  2 
builtin/am.c
t/t4150-am.sh
diff --combined builtin/am.c
index 83b3d86e67f1ef335d0d3011ef0205909b315692,e7828e5d9beb225ece1feeaefd271baec2dda840..4f77e07b9549f8ff144d770fb0b66e9124ae4dc6
@@@ -10,7 -10,6 +10,7 @@@
  #include "dir.h"
  #include "run-command.h"
  #include "quote.h"
 +#include "tempfile.h"
  #include "lockfile.h"
  #include "cache-tree.h"
  #include "refs.h"
@@@ -194,27 -193,6 +194,27 @@@ static inline const char *am_path(cons
        return mkpath("%s/%s", state->dir, path);
  }
  
 +/**
 + * For convenience to call write_file()
 + */
 +static int write_state_text(const struct am_state *state,
 +                          const char *name, const char *string)
 +{
 +      return write_file(am_path(state, name), "%s", string);
 +}
 +
 +static int write_state_count(const struct am_state *state,
 +                           const char *name, int value)
 +{
 +      return write_file(am_path(state, name), "%d", value);
 +}
 +
 +static int write_state_bool(const struct am_state *state,
 +                          const char *name, int value)
 +{
 +      return write_state_text(state, name, value ? "t" : "f");
 +}
 +
  /**
   * If state->quiet is false, calls fprintf(fp, fmt, ...), and appends a newline
   * at the end.
@@@ -384,7 -362,7 +384,7 @@@ static void write_author_script(const s
        sq_quote_buf(&sb, state->author_date);
        strbuf_addch(&sb, '\n');
  
 -      write_file(am_path(state, "author-script"), 1, "%s", sb.buf);
 +      write_state_text(state, "author-script", sb.buf);
  
        strbuf_release(&sb);
  }
@@@ -1022,10 -1000,13 +1022,10 @@@ static void am_setup(struct am_state *s
        if (state->rebasing)
                state->threeway = 1;
  
 -      write_file(am_path(state, "threeway"), 1, state->threeway ? "t" : "f");
 -
 -      write_file(am_path(state, "quiet"), 1, state->quiet ? "t" : "f");
 -
 -      write_file(am_path(state, "sign"), 1, state->signoff ? "t" : "f");
 -
 -      write_file(am_path(state, "utf8"), 1, state->utf8 ? "t" : "f");
 +      write_state_bool(state, "threeway", state->threeway);
 +      write_state_bool(state, "quiet", state->quiet);
 +      write_state_bool(state, "sign", state->signoff);
 +      write_state_bool(state, "utf8", state->utf8);
  
        switch (state->keep) {
        case KEEP_FALSE:
                die("BUG: invalid value for state->keep");
        }
  
 -      write_file(am_path(state, "keep"), 1, "%s", str);
 -
 -      write_file(am_path(state, "messageid"), 1, state->message_id ? "t" : "f");
 +      write_state_text(state, "keep", str);
 +      write_state_bool(state, "messageid", state->message_id);
  
        switch (state->scissors) {
        case SCISSORS_UNSET:
        default:
                die("BUG: invalid value for state->scissors");
        }
 -
 -      write_file(am_path(state, "scissors"), 1, "%s", str);
 +      write_state_text(state, "scissors", str);
  
        sq_quote_argv(&sb, state->git_apply_opts.argv, 0);
 -      write_file(am_path(state, "apply-opt"), 1, "%s", sb.buf);
 +      write_state_text(state, "apply-opt", sb.buf);
  
        if (state->rebasing)
 -              write_file(am_path(state, "rebasing"), 1, "%s", "");
 +              write_state_text(state, "rebasing", "");
        else
 -              write_file(am_path(state, "applying"), 1, "%s", "");
 +              write_state_text(state, "applying", "");
  
        if (!get_sha1("HEAD", curr_head)) {
 -              write_file(am_path(state, "abort-safety"), 1, "%s", sha1_to_hex(curr_head));
 +              write_state_text(state, "abort-safety", sha1_to_hex(curr_head));
                if (!state->rebasing)
                        update_ref("am", "ORIG_HEAD", curr_head, NULL, 0,
                                        UPDATE_REFS_DIE_ON_ERR);
        } else {
 -              write_file(am_path(state, "abort-safety"), 1, "%s", "");
 +              write_state_text(state, "abort-safety", "");
                if (!state->rebasing)
                        delete_ref("ORIG_HEAD", NULL, 0);
        }
         * session is in progress, they should be written last.
         */
  
 -      write_file(am_path(state, "next"), 1, "%d", state->cur);
 -
 -      write_file(am_path(state, "last"), 1, "%d", state->last);
 +      write_state_count(state, "next", state->cur);
 +      write_state_count(state, "last", state->last);
  
        strbuf_release(&sb);
  }
@@@ -1117,12 -1101,12 +1117,12 @@@ static void am_next(struct am_state *st
        unlink(am_path(state, "original-commit"));
  
        if (!get_sha1("HEAD", head))
 -              write_file(am_path(state, "abort-safety"), 1, "%s", sha1_to_hex(head));
 +              write_state_text(state, "abort-safety", sha1_to_hex(head));
        else
 -              write_file(am_path(state, "abort-safety"), 1, "%s", "");
 +              write_state_text(state, "abort-safety", "");
  
        state->cur++;
 -      write_file(am_path(state, "next"), 1, "%d", state->cur);
 +      write_state_count(state, "next", state->cur);
  }
  
  /**
@@@ -1207,6 -1191,33 +1207,33 @@@ static void NORETURN die_user_resolve(c
        exit(128);
  }
  
+ static void am_signoff(struct strbuf *sb)
+ {
+       char *cp;
+       struct strbuf mine = STRBUF_INIT;
+       /* Does it end with our own sign-off? */
+       strbuf_addf(&mine, "\n%s%s\n",
+                   sign_off_header,
+                   fmt_name(getenv("GIT_COMMITTER_NAME"),
+                            getenv("GIT_COMMITTER_EMAIL")));
+       if (mine.len < sb->len &&
+           !strcmp(mine.buf, sb->buf + sb->len - mine.len))
+               goto exit; /* no need to duplicate */
+       /* Does it have any Signed-off-by: in the text */
+       for (cp = sb->buf;
+            cp && *cp && (cp = strstr(cp, sign_off_header)) != NULL;
+            cp = strchr(cp, '\n')) {
+               if (sb->buf == cp || cp[-1] == '\n')
+                       break;
+       }
+       strbuf_addstr(sb, mine.buf + !!cp);
+ exit:
+       strbuf_release(&mine);
+ }
  /**
   * Appends signoff to the "msg" field of the am_state.
   */
@@@ -1215,7 -1226,7 +1242,7 @@@ static void am_append_signoff(struct am
        struct strbuf sb = STRBUF_INIT;
  
        strbuf_attach(&sb, state->msg, state->msg_len, state->msg_len);
-       append_signoff(&sb, 0, 0);
+       am_signoff(&sb);
        state->msg = strbuf_detach(&sb, &state->msg_len);
  }
  
@@@ -1319,7 -1330,7 +1346,7 @@@ static int parse_mail(struct am_state *
        stripspace(&msg, 0);
  
        if (state->signoff)
-               append_signoff(&msg, 0, 0);
+               am_signoff(&msg);
  
        assert(!state->author_name);
        state->author_name = strbuf_detach(&author_name, NULL);
@@@ -1495,7 -1506,8 +1522,7 @@@ static int parse_mail_rebase(struct am_
        write_commit_patch(state, commit);
  
        hashcpy(state->orig_commit, commit_sha1);
 -      write_file(am_path(state, "original-commit"), 1, "%s",
 -                      sha1_to_hex(commit_sha1));
 +      write_state_text(state, "original-commit", sha1_to_hex(commit_sha1));
  
        return 0;
  }
@@@ -1797,7 -1809,7 +1824,7 @@@ static void am_run(struct am_state *sta
        refresh_and_write_cache();
  
        if (index_has_changes(&sb)) {
 -              write_file(am_path(state, "dirtyindex"), 1, "t");
 +              write_state_bool(state, "dirtyindex", 1);
                die(_("Dirty index: cannot apply patches (dirty: %s)"), sb.buf);
        }
  
@@@ -1976,49 -1988,16 +2003,49 @@@ static int fast_forward_to(struct tree 
        return 0;
  }
  
 +/**
 + * Merges a tree into the index. The index's stat info will take precedence
 + * over the merged tree's. Returns 0 on success, -1 on failure.
 + */
 +static int merge_tree(struct tree *tree)
 +{
 +      struct lock_file *lock_file;
 +      struct unpack_trees_options opts;
 +      struct tree_desc t[1];
 +
 +      if (parse_tree(tree))
 +              return -1;
 +
 +      lock_file = xcalloc(1, sizeof(struct lock_file));
 +      hold_locked_index(lock_file, 1);
 +
 +      memset(&opts, 0, sizeof(opts));
 +      opts.head_idx = 1;
 +      opts.src_index = &the_index;
 +      opts.dst_index = &the_index;
 +      opts.merge = 1;
 +      opts.fn = oneway_merge;
 +      init_tree_desc(&t[0], tree->buffer, tree->size);
 +
 +      if (unpack_trees(1, t, &opts)) {
 +              rollback_lock_file(lock_file);
 +              return -1;
 +      }
 +
 +      if (write_locked_index(&the_index, lock_file, COMMIT_LOCK))
 +              die(_("unable to write new index file"));
 +
 +      return 0;
 +}
 +
  /**
   * Clean the index without touching entries that are not modified between
   * `head` and `remote`.
   */
  static int clean_index(const unsigned char *head, const unsigned char *remote)
  {
 -      struct lock_file *lock_file;
        struct tree *head_tree, *remote_tree, *index_tree;
        unsigned char index[GIT_SHA1_RAWSZ];
 -      struct pathspec pathspec;
  
        head_tree = parse_tree_indirect(head);
        if (!head_tree)
        if (fast_forward_to(index_tree, remote_tree, 0))
                return -1;
  
 -      memset(&pathspec, 0, sizeof(pathspec));
 -
 -      lock_file = xcalloc(1, sizeof(struct lock_file));
 -      hold_locked_index(lock_file, 1);
 -
 -      if (read_tree(remote_tree, 0, &pathspec)) {
 -              rollback_lock_file(lock_file);
 +      if (merge_tree(remote_tree))
                return -1;
 -      }
 -
 -      if (write_locked_index(&the_index, lock_file, COMMIT_LOCK))
 -              die(_("unable to write new index file"));
  
        remove_branch_state();
  
  static void am_rerere_clear(void)
  {
        struct string_list merge_rr = STRING_LIST_INIT_DUP;
 -      int fd = setup_rerere(&merge_rr, 0);
 -
 -      if (fd < 0)
 -              return;
 -
        rerere_clear(&merge_rr);
        string_list_clear(&merge_rr, 1);
  }
@@@ -2200,7 -2194,7 +2227,7 @@@ int cmd_am(int argc, const char **argv
                OPT_BOOL('i', "interactive", &state.interactive,
                        N_("run interactively")),
                OPT_HIDDEN_BOOL('b', "binary", &binary,
 -                      N_("(historical option -- no-op")),
 +                      N_("historical option -- no-op")),
                OPT_BOOL('3', "3way", &state.threeway,
                        N_("allow fall back on 3way merging if needed")),
                OPT__QUIET(&state.quiet, N_("be quiet")),
diff --combined t/t4150-am.sh
index af6053a2429785c3d9981194177f05aa4beb57d2,5e4855595bf31bc1de1355b856d531669e7f7ae1..b41bd17264f93d54d5aa280fbc305cebb18950a0
@@@ -873,40 -873,52 +873,88 @@@ test_expect_success 'am --message-id -
        test_cmp expected actual
  '
  
 +test_expect_success 'am -3 works with rerere' '
 +      rm -fr .git/rebase-apply &&
 +      git reset --hard &&
 +
 +      # make patches one->two and two->three...
 +      test_commit one file &&
 +      test_commit two file &&
 +      test_commit three file &&
 +      git format-patch -2 --stdout >seq.patch &&
 +
 +      # and create a situation that conflicts...
 +      git reset --hard one &&
 +      test_commit other file &&
 +
 +      # enable rerere...
 +      test_config rerere.enabled true &&
 +      test_when_finished "rm -rf .git/rr-cache" &&
 +
 +      # ...and apply. Our resolution is to skip the first
 +      # patch, and the rerere the second one.
 +      test_must_fail git am -3 seq.patch &&
 +      test_must_fail git am --skip &&
 +      echo resolved >file &&
 +      git add file &&
 +      git am --resolved &&
 +
 +      # now apply again, and confirm that rerere engaged (we still
 +      # expect failure from am because rerere does not auto-commit
 +      # for us).
 +      git reset --hard other &&
 +      test_must_fail git am -3 seq.patch &&
 +      test_must_fail git am --skip &&
 +      echo resolved >expect &&
 +      test_cmp expect file
 +'
 +
+ test_expect_success 'am -s unexpected trailer block' '
+       rm -fr .git/rebase-apply &&
+       git reset --hard &&
+       echo signed >file &&
+       git add file &&
+       cat >msg <<-EOF &&
+       subject here
+       Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
+       [jc: tweaked log message]
+       Signed-off-by: J C H <j@c.h>
+       EOF
+       git commit -F msg &&
+       git cat-file commit HEAD | sed -e '1,/^$/d' >original &&
+       git format-patch --stdout -1 >patch &&
+       git reset --hard HEAD^ &&
+       git am -s patch &&
+       (
+               cat original &&
+               echo "Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>"
+       ) >expect &&
+       git cat-file commit HEAD | sed -e '1,/^$/d' >actual &&
+       test_cmp expect actual &&
+       cat >msg <<-\EOF &&
+       subject here
+       We make sure that there is a blank line between the log
+       message proper and Signed-off-by: line added.
+       EOF
+       git reset HEAD^ &&
+       git commit -F msg file &&
+       git cat-file commit HEAD | sed -e '1,/^$/d' >original &&
+       git format-patch --stdout -1 >patch &&
+       git reset --hard HEAD^ &&
+       git am -s patch &&
+       (
+               cat original &&
+               echo &&
+               echo "Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>"
+       ) >expect &&
+       git cat-file commit HEAD | sed -e '1,/^$/d' >actual &&
+       test_cmp expect actual
+ '
  test_done