From: Junio C Hamano <gitster@pobox.com>
Date: Tue, 8 Sep 2015 22:34:57 +0000 (-0700)
Subject: Merge branch 'jc/builtin-am-signoff-regression-fix'
X-Git-Tag: v2.6.0-rc1~1
X-Git-Url: https://git.lorimer.id.au/gitweb.git/diff_plain/d6a2b05cbb63b1a923e2bb614dedfca83014f870?hp=-c

Merge branch 'jc/builtin-am-signoff-regression-fix'

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

d6a2b05cbb63b1a923e2bb614dedfca83014f870
diff --combined builtin/am.c
index 83b3d86e67,e7828e5d9b..4f77e07b95
--- a/builtin/am.c
+++ b/builtin/am.c
@@@ -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:
@@@ -1041,8 -1022,9 +1041,8 @@@
  		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:
@@@ -1057,23 -1039,24 +1057,23 @@@
  	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);
  	}
@@@ -1083,8 -1066,9 +1083,8 @@@
  	 * 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)
@@@ -2043,8 -2022,18 +2070,8 @@@
  	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();
  
@@@ -2057,6 -2046,11 +2084,6 @@@
  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 af6053a242,5e4855595b..b41bd17264
--- a/t/t4150-am.sh
+++ b/t/t4150-am.sh
@@@ -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