[PATCH v4 0/6] send-email: configuration improvements

[PATCH v4 0/6] send-email: configuration improvements

[Felipe Contreras]

Hi,

Some comments have been addressed, tests have been added, and a bug fixed.
Also, after the reshuffling of code, cleanup possibilities are realized.

Felipe Contreras (6):
  send-email: make annotate configurable
  format-patch: improve head calculation for cover-letter
  format-patch: refactor branch name calculation
  log: update to OPT_BOOL
  format-patch: add format.cover-letter configuration
  format-patch: trivial cleanups

 Documentation/config.txt           |   6 ++
 Documentation/git-format-patch.txt |   5 +-
 Documentation/git-send-email.txt   |   5 +-
 builtin/log.c                      | 166 +++++++++++++++++++------------------
 git-send-email.perl                |   7 +-
 t/t4014-format-patch.sh            |  33 ++++++++
 6 files changed, 134 insertions(+), 88 deletions(-)

-- 
1.8.2

[PATCH v4 1/6] send-email: make annotate configurable

[Felipe Contreras]

Some people always do --annotate, lets not force them to always type
that.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Documentation/config.txt         | 1 +
 Documentation/git-send-email.txt | 5 +++--
 git-send-email.perl              | 7 ++++---
 3 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index bbba728..c8e2178 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1994,6 +1994,7 @@ sendemail.<identity>.*::
 
 sendemail.aliasesfile::
 sendemail.aliasfiletype::
+sendemail.annotate::
 sendemail.bcc::
 sendemail.cc::
 sendemail.cccmd::
diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index 44a1f7c..2facc18 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -45,8 +45,9 @@ Composing
 ~~~~~~~~~
 
 --annotate::
-	Review and edit each patch you're about to send. See the
-	CONFIGURATION section for 'sendemail.multiedit'.
+	Review and edit each patch you're about to send. Default is the value
+	of 'sendemail.annotate'. See the CONFIGURATION section for
+	'sendemail.multiedit'.
 
 --bcc=<address>::
 	Specify a "Bcc:" value for each email. Default is the value of
diff --git a/git-send-email.perl b/git-send-email.perl
index be809e5..e187b12 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -54,7 +54,7 @@ git send-email [options] <file | directory | rev-list options >
     --[no-]bcc              <str>  * Email Bcc:
     --subject               <str>  * Email "Subject:"
     --in-reply-to           <str>  * Email "In-Reply-To:"
-    --annotate                     * Review each patch that will be sent in an editor.
+    --[no-]annotate                * Review each patch that will be sent in an editor.
     --compose                      * Open an editor for introduction.
     --compose-encoding      <str>  * Encoding to assume for introduction.
     --8bit-encoding         <str>  * Encoding to assume 8bit mails if undeclared
@@ -212,7 +212,8 @@ my %config_bool_settings = (
     "signedoffbycc" => [\$signed_off_by_cc, undef],
     "signedoffcc" => [\$signed_off_by_cc, undef],      # Deprecated
     "validate" => [\$validate, 1],
-    "multiedit" => [\$multiedit, undef]
+    "multiedit" => [\$multiedit, undef],
+    "annotate" => [\$annotate, undef]
 );
 
 my %config_settings = (
@@ -304,7 +305,7 @@ my $rc = GetOptions("h" => \$help,
 		    "smtp-debug:i" => \$debug_net_smtp,
 		    "smtp-domain:s" => \$smtp_domain,
 		    "identity=s" => \$identity,
-		    "annotate" => \$annotate,
+		    "annotate!" => \$annotate,
 		    "compose" => \$compose,
 		    "quiet" => \$quiet,
 		    "cc-cmd=s" => \$cc_cmd,
-- 
1.8.2

[PATCH v4 2/6] format-patch: improve head calculation for cover-letter

[Felipe Contreras]

If we do it after the revision traversal we can be sure that this is
indeed a commit that will be processed (i.e. not a merge) and it's the
top most one (thus removing the NEEDSWORK comment, at least we show the
same as 'git diff --stat').

While we are at it, since we know there's nothing to generate, exit
sooner in all cases, like --cover-letter currently does.

Also, if there's nothing to generate and cover-letter is specified, a
different code-path might be triggered that is not currently covered in
the test-case, so add a test for it.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/log.c           | 22 ++++------------------
 t/t4014-format-patch.sh |  5 +++++
 2 files changed, 9 insertions(+), 18 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 8f0b2e8..e0c8b6f 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1319,24 +1319,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	rev.show_root_diff = 1;
 
 	if (cover_letter) {
-		/*
-		 * NEEDSWORK:randomly pick one positive commit to show
-		 * diffstat; this is often the tip and the command
-		 * happens to do the right thing in most cases, but a
-		 * complex command like "--cover-letter a b c ^bottom"
-		 * picks "c" and shows diffstat between bottom..c
-		 * which may not match what the series represents at
-		 * all and totally broken.
-		 */
-		int i;
-		for (i = 0; i < rev.pending.nr; i++) {
-			struct object *o = rev.pending.objects[i].item;
-			if (!(o->flags & UNINTERESTING))
-				head = (struct commit *)o;
-		}
-		/* There is nothing to show; it is not an error, though. */
-		if (!head)
-			return 0;
 		if (!branch_name)
 			branch_name = find_branch_name(&rev);
 	}
@@ -1372,6 +1354,10 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		list = xrealloc(list, nr * sizeof(list[0]));
 		list[nr - 1] = commit;
 	}
+	if (nr == 0)
+		/* nothing to do */
+		return 0;
+	head = list[0];
 	total = nr;
 	if (!keep_subject && auto_number && total > 1)
 		numbered = 1;
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 7fa3647..71b35e7 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1021,4 +1021,9 @@ test_expect_success 'cover letter using branch description (6)' '
 	grep hello actual >/dev/null
 '
 
+test_expect_success 'cover letter with nothing' '
+	git format-patch --stdout --cover-letter >actual &&
+	test_line_count = 0 actual
+'
+
 test_done
-- 
1.8.2

[PATCH v4 3/6] format-patch: refactor branch name calculation

[Felipe Contreras]

By moving the part that relies on rev->pending earlier, where we are
already checking the special case where there's only one ref.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/log.c | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index e0c8b6f..cd942ee 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1049,15 +1049,6 @@ static char *find_branch_name(struct rev_info *rev)
 	if (0 <= positive) {
 		ref = rev->cmdline.rev[positive].name;
 		tip_sha1 = rev->cmdline.rev[positive].item->sha1;
-	} else if (!rev->cmdline.nr && rev->pending.nr == 1 &&
-		   !strcmp(rev->pending.objects[0].name, "HEAD")) {
-		/*
-		 * No actual ref from command line, but "HEAD" from
-		 * rev->def was added in setup_revisions()
-		 * e.g. format-patch --cover-letter -12
-		 */
-		ref = "HEAD";
-		tip_sha1 = rev->pending.objects[0].item->sha1;
 	} else {
 		return NULL;
 	}
@@ -1288,28 +1279,36 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	}
 
 	if (rev.pending.nr == 1) {
+		int check_head = 0;
+
 		if (rev.max_count < 0 && !rev.show_root_diff) {
 			/*
 			 * This is traditional behaviour of "git format-patch
 			 * origin" that prepares what the origin side still
 			 * does not have.
 			 */
-			unsigned char sha1[20];
-			const char *ref;
-
 			rev.pending.objects[0].item->flags |= UNINTERESTING;
 			add_head_to_pending(&rev);
-			ref = resolve_ref_unsafe("HEAD", sha1, 1, NULL);
-			if (ref && !prefixcmp(ref, "refs/heads/"))
-				branch_name = xstrdup(ref + strlen("refs/heads/"));
-			else
-				branch_name = xstrdup(""); /* no branch */
+			check_head = 1;
 		}
 		/*
 		 * Otherwise, it is "format-patch -22 HEAD", and/or
 		 * "format-patch --root HEAD".  The user wants
 		 * get_revision() to do the usual traversal.
 		 */
+
+		if (!strcmp(rev.pending.objects[0].name, "HEAD"))
+			check_head = 1;
+
+		if (check_head) {
+			unsigned char sha1[20];
+			const char *ref;
+			ref = resolve_ref_unsafe("HEAD", sha1, 1, NULL);
+			if (ref && !prefixcmp(ref, "refs/heads/"))
+				branch_name = xstrdup(ref + strlen("refs/heads/"));
+			else
+				branch_name = xstrdup(""); /* no branch */
+		}
 	}
 
 	/*
-- 
1.8.2

[PATCH v4 4/6] log: update to OPT_BOOL

[Felipe Contreras]

OPT_BOOLEAN is deprecated, and this is what we want.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/log.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index cd942ee..488a254 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -99,9 +99,9 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
 	int quiet = 0, source = 0, mailmap = 0;
 
 	const struct option builtin_log_options[] = {
-		OPT_BOOLEAN(0, "quiet", &quiet, N_("suppress diff output")),
-		OPT_BOOLEAN(0, "source", &source, N_("show source")),
-		OPT_BOOLEAN(0, "use-mailmap", &mailmap, N_("Use mail map file")),
+		OPT_BOOL(0, "quiet", &quiet, N_("suppress diff output")),
+		OPT_BOOL(0, "source", &source, N_("show source")),
+		OPT_BOOL(0, "use-mailmap", &mailmap, N_("Use mail map file")),
 		{ OPTION_CALLBACK, 0, "decorate", NULL, NULL, N_("decorate options"),
 		  PARSE_OPT_OPTARG, decorate_callback},
 		OPT_END()
@@ -1090,12 +1090,12 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		{ OPTION_CALLBACK, 'N', "no-numbered", &numbered, NULL,
 			    N_("use [PATCH] even with multiple patches"),
 			    PARSE_OPT_NOARG, no_numbered_callback },
-		OPT_BOOLEAN('s', "signoff", &do_signoff, N_("add Signed-off-by:")),
-		OPT_BOOLEAN(0, "stdout", &use_stdout,
+		OPT_BOOL('s', "signoff", &do_signoff, N_("add Signed-off-by:")),
+		OPT_BOOL(0, "stdout", &use_stdout,
 			    N_("print patches to standard out")),
-		OPT_BOOLEAN(0, "cover-letter", &cover_letter,
+		OPT_BOOL(0, "cover-letter", &cover_letter,
 			    N_("generate a cover letter")),
-		OPT_BOOLEAN(0, "numbered-files", &just_numbers,
+		OPT_BOOL(0, "numbered-files", &just_numbers,
 			    N_("use simple number sequence for output file names")),
 		OPT_STRING(0, "suffix", &fmt_patch_suffix, N_("sfx"),
 			    N_("use <sfx> instead of '.patch'")),
-- 
1.8.2

[PATCH v4 5/6] format-patch: add format.cover-letter configuration

[Felipe Contreras]

Also, add a new option: 'auto', so if there's more than one patch, the
cover letter is generated, otherwise it's not.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Documentation/config.txt           |  5 +++++
 Documentation/git-format-patch.txt |  5 +++--
 builtin/log.c                      | 32 ++++++++++++++++++++++++++------
 t/t4014-format-patch.sh            | 28 ++++++++++++++++++++++++++++
 4 files changed, 62 insertions(+), 8 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index c8e2178..670094f 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1092,6 +1092,11 @@ format.signoff::
     the rights to submit this work under the same open source license.
     Please see the 'SubmittingPatches' document for further discussion.
 
+format.coverLetter::
+	A boolean that controls whether to generate a cover-letter when
+	format-patch is invoked, but in addition can be set to "auto", to
+	generate a cover-letter only when there's more than one patch.
+
 filter.<driver>.clean::
 	The command which is used to convert the content of a worktree
 	file to a blob upon checkin.  See linkgit:gitattributes[5] for
diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index 3a62f50..3911877 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -20,7 +20,7 @@ SYNOPSIS
 		   [--ignore-if-in-upstream]
 		   [--subject-prefix=Subject-Prefix] [(--reroll-count|-v) <n>]
 		   [--to=<email>] [--cc=<email>]
-		   [--cover-letter] [--quiet] [--notes[=<ref>]]
+		   [--[no-]cover-letter] [--quiet] [--notes[=<ref>]]
 		   [<common diff options>]
 		   [ <since> | <revision range> ]
 
@@ -195,7 +195,7 @@ will want to ensure that threading is disabled for `git send-email`.
 	`Cc:`, and custom) headers added so far from config or command
 	line.
 
---cover-letter::
+--[no-]cover-letter::
 	In addition to the patches, generate a cover letter file
 	containing the shortlog and the overall diffstat.  You can
 	fill in a description in the file before sending it out.
@@ -260,6 +260,7 @@ attachments, and sign off patches with configuration variables.
 	cc = <email>
 	attach [ = mime-boundary-string ]
 	signoff = true
+	coverletter = auto
 ------------
 
 
diff --git a/builtin/log.c b/builtin/log.c
index 488a254..cf09a81 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -619,6 +619,14 @@ static void add_header(const char *value)
 static int thread;
 static int do_signoff;
 static const char *signature = git_version_string;
+static int config_cover_letter;
+
+enum {
+	COVER_UNSET,
+	COVER_OFF,
+	COVER_ON,
+	COVER_AUTO
+};
 
 static int git_format_config(const char *var, const char *value, void *cb)
 {
@@ -680,6 +688,14 @@ static int git_format_config(const char *var, const char *value, void *cb)
 	}
 	if (!strcmp(var, "format.signature"))
 		return git_config_string(&signature, var, value);
+	if (!strcmp(var, "format.coverletter")) {
+		if (value && !strcasecmp(value, "auto")) {
+			config_cover_letter = COVER_AUTO;
+			return 0;
+		}
+		config_cover_letter = git_config_bool(var, value) ? COVER_ON : COVER_OFF;
+		return 0;
+	}
 
 	return git_log_config(var, value, cb);
 }
@@ -1071,7 +1087,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	int start_number = -1;
 	int just_numbers = 0;
 	int ignore_if_in_upstream = 0;
-	int cover_letter = 0;
+	int cover_letter = -1;
 	int boundary_count = 0;
 	int no_binary_diff = 0;
 	struct commit *origin = NULL, *head = NULL;
@@ -1317,11 +1333,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	 */
 	rev.show_root_diff = 1;
 
-	if (cover_letter) {
-		if (!branch_name)
-			branch_name = find_branch_name(&rev);
-	}
-
 	if (ignore_if_in_upstream) {
 		/* Don't say anything if head and upstream are the same. */
 		if (rev.pending.nr == 2) {
@@ -1362,6 +1373,13 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		numbered = 1;
 	if (numbered)
 		rev.total = total + start_number - 1;
+	if (cover_letter == -1) {
+		if (config_cover_letter == COVER_AUTO)
+			cover_letter = (total > 1);
+		else
+			cover_letter = (config_cover_letter == COVER_ON);
+	}
+
 	if (in_reply_to || thread || cover_letter)
 		rev.ref_message_ids = xcalloc(1, sizeof(struct string_list));
 	if (in_reply_to) {
@@ -1373,6 +1391,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	if (cover_letter) {
 		if (thread)
 			gen_message_id(&rev, "cover");
+		if (!branch_name)
+			branch_name = find_branch_name(&rev);
 		make_cover_letter(&rev, use_stdout,
 				  origin, nr, list, head, branch_name, quiet);
 		total++;
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 71b35e7..01c2a47 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1026,4 +1026,32 @@ test_expect_success 'cover letter with nothing' '
 	test_line_count = 0 actual
 '
 
+test_expect_success 'cover letter auto' '
+	mkdir -p tmp &&
+	test_when_finished "rm -rf tmp;
+		git config --unset format.coverletter" &&
+
+	git config format.coverletter auto &&
+	git format-patch -o tmp -1 >list &&
+	test_line_count = 1 list &&
+	git format-patch -o tmp -2 >list &&
+	test_line_count = 3 list
+'
+
+test_expect_success 'cover letter auto user override' '
+	mkdir -p tmp &&
+	test_when_finished "rm -rf tmp;
+		git config --unset format.coverletter" &&
+
+	git config format.coverletter auto &&
+	git format-patch -o tmp --cover-letter -1 >list &&
+	test_line_count = 2 list &&
+	git format-patch -o tmp --cover-letter -2 >list &&
+	test_line_count = 3 list &&
+	git format-patch -o tmp --no-cover-letter -1 >list &&
+	test_line_count = 1 list &&
+	git format-patch -o tmp --no-cover-letter -2 >list &&
+	test_line_count = 2 list
+'
+
 test_done
-- 
1.8.2

[PATCH v4 6/6] format-patch: trivial cleanups

[Felipe Contreras]

Now that the cover-letter code has been shuffled, we can do some
cleanups.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/log.c | 71 +++++++++++++++++++++++++++++------------------------------
 1 file changed, 35 insertions(+), 36 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index cf09a81..bbfe6bc 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -807,9 +807,37 @@ static void add_branch_description(struct strbuf *buf, const char *branch_name)
 	}
 }
 
+static char *find_branch_name(struct rev_info *rev)
+{
+	int i, positive = -1;
+	unsigned char branch_sha1[20];
+	const unsigned char *tip_sha1;
+	const char *ref;
+	char *full_ref, *branch = NULL;
+
+	for (i = 0; i < rev->cmdline.nr; i++) {
+		if (rev->cmdline.rev[i].flags & UNINTERESTING)
+			continue;
+		if (positive < 0)
+			positive = i;
+		else
+			return NULL;
+	}
+	if (positive < 0)
+		return NULL;
+	ref = rev->cmdline.rev[positive].name;
+	tip_sha1 = rev->cmdline.rev[positive].item->sha1;
+	if (dwim_ref(ref, strlen(ref), branch_sha1, &full_ref) &&
+	    !prefixcmp(full_ref, "refs/heads/") &&
+	    !hashcmp(tip_sha1, branch_sha1))
+		branch = xstrdup(full_ref + strlen("refs/heads/"));
+	free(full_ref);
+	return branch;
+}
+
 static void make_cover_letter(struct rev_info *rev, int use_stdout,
 			      struct commit *origin,
-			      int nr, struct commit **list, struct commit *head,
+			      int nr, struct commit **list,
 			      const char *branch_name,
 			      int quiet)
 {
@@ -823,6 +851,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 	struct diff_options opts;
 	int need_8bit_cte = 0;
 	struct pretty_print_context pp = {0};
+	struct commit *head = list[0];
 
 	if (rev->commit_format != CMIT_FMT_EMAIL)
 		die(_("Cover letter needs email format"));
@@ -840,6 +869,9 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 		if (has_non_ascii(list[i]->buffer))
 			need_8bit_cte = 1;
 
+	if (!branch_name)
+		branch_name = find_branch_name(rev);
+
 	msg = body;
 	pp.fmt = CMIT_FMT_EMAIL;
 	pp.date_mode = DATE_RFC2822;
@@ -1046,36 +1078,6 @@ static int cc_callback(const struct option *opt, const char *arg, int unset)
 	return 0;
 }
 
-static char *find_branch_name(struct rev_info *rev)
-{
-	int i, positive = -1;
-	unsigned char branch_sha1[20];
-	const unsigned char *tip_sha1;
-	const char *ref;
-	char *full_ref, *branch = NULL;
-
-	for (i = 0; i < rev->cmdline.nr; i++) {
-		if (rev->cmdline.rev[i].flags & UNINTERESTING)
-			continue;
-		if (positive < 0)
-			positive = i;
-		else
-			return NULL;
-	}
-	if (0 <= positive) {
-		ref = rev->cmdline.rev[positive].name;
-		tip_sha1 = rev->cmdline.rev[positive].item->sha1;
-	} else {
-		return NULL;
-	}
-	if (dwim_ref(ref, strlen(ref), branch_sha1, &full_ref) &&
-	    !prefixcmp(full_ref, "refs/heads/") &&
-	    !hashcmp(tip_sha1, branch_sha1))
-		branch = xstrdup(full_ref + strlen("refs/heads/"));
-	free(full_ref);
-	return branch;
-}
-
 int cmd_format_patch(int argc, const char **argv, const char *prefix)
 {
 	struct commit *commit;
@@ -1090,7 +1092,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	int cover_letter = -1;
 	int boundary_count = 0;
 	int no_binary_diff = 0;
-	struct commit *origin = NULL, *head = NULL;
+	struct commit *origin = NULL;
 	const char *in_reply_to = NULL;
 	struct patch_ids ids;
 	char *add_signoff = NULL;
@@ -1367,7 +1369,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	if (nr == 0)
 		/* nothing to do */
 		return 0;
-	head = list[0];
 	total = nr;
 	if (!keep_subject && auto_number && total > 1)
 		numbered = 1;
@@ -1391,10 +1392,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	if (cover_letter) {
 		if (thread)
 			gen_message_id(&rev, "cover");
-		if (!branch_name)
-			branch_name = find_branch_name(&rev);
 		make_cover_letter(&rev, use_stdout,
-				  origin, nr, list, head, branch_name, quiet);
+				  origin, nr, list, branch_name, quiet);
 		total++;
 		start_number--;
 	}
-- 
1.8.2

Re: [PATCH v4 5/6] format-patch: add format.cover-letter configuration

[Simon Ruderich]

On Sun, Apr 07, 2013 at 12:46:23PM -0500, Felipe Contreras wrote:
> [snip]
>
> +test_expect_success 'cover letter auto' '
> +	mkdir -p tmp &&
> +	test_when_finished "rm -rf tmp;
> +		git config --unset format.coverletter" &&
> +
> +	git config format.coverletter auto &&

    test_config format.coverletter auto &&

takes automatically care of the git config --unset cleanup.

I'm not sure if it's better to use test_when_finished with rm or
just && rm -rf tmp at the end of the test in case someone wants
to look at the output.

Regards
Simon
-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9

Re: [PATCH v4 5/6] format-patch: add format.cover-letter configuration

[Jonathan Nieder]

Hi,

Simon Ruderich wrote:
> On Sun, Apr 07, 2013 at 12:46:23PM -0500, Felipe Contreras wrote:

>> +test_expect_success 'cover letter auto' '
>> +	mkdir -p tmp &&
>> +	test_when_finished "rm -rf tmp;
[...]
> I'm not sure if it's better to use test_when_finished with rm or
> just && rm -rf tmp at the end of the test in case someone wants
> to look at the output.

test_when_finished is better here, since it means later tests can
run and provide useful information about how bad a regression is.
Cleanup commands requested using test_when_finished are not run when a
test being run with --immediate fails, so you can still inspect output
after a failed test.

Thanks and hope that helps,
Jonathan

[PATCH] t/README: --immediate skips cleanup commands for failed tests

[Simon Ruderich]

On Sun, Apr 07, 2013 at 03:32:00PM -0700, Jonathan Nieder wrote:
>> I'm not sure if it's better to use test_when_finished with rm or
>> just && rm -rf tmp at the end of the test in case someone wants
>> to look at the output.
>
> test_when_finished is better here, since it means later tests can
> run and provide useful information about how bad a regression is.
> Cleanup commands requested using test_when_finished are not run when a
> test being run with --immediate fails, so you can still inspect output
> after a failed test.

Hello Jonathan,

Thanks for the explanation.

I couldn't find this documented in t/README, the following patch
adds it.

-- 8< --
Subject: [PATCH] t/README: --immediate skips cleanup commands for failed tests

---
 t/README | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/README b/t/README
index 9b41fe7..e5e7d37 100644
--- a/t/README
+++ b/t/README
@@ -86,7 +86,8 @@ appropriately before running "make".
 
 --immediate::
 	This causes the test to immediately exit upon the first
-	failed test.
+	failed test. Cleanup commands requested with
+	test_when_finished are not executed if the test failed.
 
 --long-tests::
 	This causes additional long-running tests to be run (where
-- 
1.8.2.481.g0d034d4

-- 8< --

Regards
Simon
-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9

Re: [PATCH v4 1/6] send-email: make annotate configurable

[Jakub Narębski]

W dniu 07.04.2013 19:46, Felipe Contreras pisze:
> @@ -212,7 +212,8 @@ my %config_bool_settings = (
>      "signedoffbycc" => [\$signed_off_by_cc, undef],
>      "signedoffcc" => [\$signed_off_by_cc, undef],      # Deprecated
>      "validate" => [\$validate, 1],
> -    "multiedit" => [\$multiedit, undef]
> +    "multiedit" => [\$multiedit, undef],
> +    "annotate" => [\$annotate, undef]
>  );

Why not leave hanging "," to make it easier on future changes,
i.e.:

  -    "multiedit" => [\$multiedit, undef]
  +    "multiedit" => [\$multiedit, undef],
  +    "annotate" => [\$annotate, undef],

-- 
Jakub Narębski

Re: [PATCH] t/README: --immediate skips cleanup commands for failed tests

[Junio C Hamano]

Simon Ruderich <simon@ruderich.org> writes:

> On Sun, Apr 07, 2013 at 03:32:00PM -0700, Jonathan Nieder wrote:
>>> I'm not sure if it's better to use test_when_finished with rm or
>>> just && rm -rf tmp at the end of the test in case someone wants
>>> to look at the output.
>>
>> test_when_finished is better here, since it means later tests can
>> run and provide useful information about how bad a regression is.
>> Cleanup commands requested using test_when_finished are not run when a
>> test being run with --immediate fails, so you can still inspect output
>> after a failed test.
>
> Hello Jonathan,
>
> Thanks for the explanation.
>
> I couldn't find this documented in t/README, the following patch
> adds it.
>
> -- 8< --
> Subject: [PATCH] t/README: --immediate skips cleanup commands for failed tests
>
> ---

Sign-off?

>  t/README | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/t/README b/t/README
> index 9b41fe7..e5e7d37 100644
> --- a/t/README
> +++ b/t/README
> @@ -86,7 +86,8 @@ appropriately before running "make".
>  
>  --immediate::
>  	This causes the test to immediately exit upon the first
> -	failed test.
> +	failed test. Cleanup commands requested with
> +	test_when_finished are not executed if the test failed.

Perhaps adding "... to keep the state for inspection by the tester
to diagnose the bug" or something is in order?

>  
>  --long-tests::
>  	This causes additional long-running tests to be run (where
> -- 
> 1.8.2.481.g0d034d4
>
> -- 8< --
>
> Regards
> Simon

[PATCH v2] t/README: --immediate skips cleanup commands for failed tests

[Simon Ruderich]

Signed-off-by: Simon Ruderich <simon@ruderich.org>
---
On Tue, Apr 09, 2013 at 12:16:56PM -0700, Junio C Hamano wrote:
> Sign-off?

Sorry, forgot it.

> Perhaps adding "... to keep the state for inspection by the tester
> to diagnose the bug" or something is in order?

Good idea.

Revised patch is attached.

Regards
Simon

 t/README | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/t/README b/t/README
index 9b41fe7..e6c060e 100644
--- a/t/README
+++ b/t/README
@@ -86,7 +86,10 @@ appropriately before running "make".
 
 --immediate::
 	This causes the test to immediately exit upon the first
-	failed test.
+	failed test. Cleanup commands requested with
+	test_when_finished are not executed if the test failed to
+	keep the state for inspection by the tester to diagnose
+	the bug.
 
 --long-tests::
 	This causes additional long-running tests to be run (where
-- 
1.8.2.481.g0d034d4

-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9