[PATCH v1 0/8] format-patch: introduce --confirm-overwrite

Hi all,

This patch series aims to prevent git-format-patch from overwriting cover
letter and patches silently. It also tries to fix git_prompt but there
are still some works to do (see NEED HELPS).

BACKGROUND
==========

Currently, git-format-patch, along with the option --cover-letter,
unconditionally overwrites a cover letter with the same name (if
present). Although this is a desired behaviour for patches which are
auto-generated from Git commits log, it might not be the case for a
cover letter whose the content is meticulously written manually.

Particulary, this behaviour could be awkward in the following
hypothetical situations:

* The user can easily erase a cover letter coming from prior versions or
another patch series by reusing an old command line (e.g. autocompleted
from the shell history).

* Assuming that the user is writing a cover letter and realizes that
small changes should be made. They make the change, amend and
format-patch again to regenerate patches. If it happens that they use
the same command again (e.g. with --cover-letter), the cover letter
being written is gone.

This patch series addresses this kind of issue by asking confirmation
from the user whenever a cover letter or a patch is subject to be
overwritten.

CHANGES
=======

Substantial changes include:

* New options format.confirmOverwrite and --confirm-overwrite=<when>
  which decide when to prompt the user for confirmation to overwrite
  patches or cover letter. There are three possible values "always",
  "never", and "cover". The default value is "cover", which means "only
  prompt when a cover letter already exists".  This is a breaking change:
  prior this patch series, the behaviour of Git corresponds to
  format.confirmOverwrite = never.
* Some tests (t4014) who overwrites cover letter in silence are fixed to
  address this breaking change.
* compat/terminal.c::git_terminal_prompt is modified to accept input
  from pipe.  This makes Git subcommands using prompt.c::git_prompt
  testable. As far as I know, the two occurrences of git_prompt are from
  credentials.c and builtin/git-bisect--helper.c, and they are not
  tested so far (see BUG below).

NEED HELPS
==========

I would appreciate some guidance on the following points.

git_prompt
~~~~~~~~~~
There are currently three issues related to changes made into
git_terminal_prompt (see patch #1).

1. All tests have passed in my machine (Ubuntu 20.04), but CI reported tests
   in t4014 that all failed at the same point:

    fopen("/dev/tty", "w") in Linux and OSX, and fopen("CONOUT$", "wt") in Windows.
    
    - Linux error: No such device or address
    - Windows error: Invalid argument
    - OSX error: Device not configured

   I also observed that one cannot write into /dev/stderr. Is this a CI
   specific issue ? (see patch #5 for the new tests).

2. (BUG) While all tests passed locally, I realized that "git push" (to
   Github) can't read my credentials. That's because, for some obscure
   reason, isatty(0) returns 0 when "git push" is asking for credentials.
   Consequently, the new code will read input from stdin, which is wrong.

   What would be the reason that causes isatty(0) returns 0 when
   git-push calls credential.c::credential_getpass ?

3. Finally, while testing git_prompt's caller works, it uglifies the
   output of "make prove" (as git_prompt writes into /dev/tty). I tried to
   "redirect" the output of /dev/tty to /dev/null to silent it using the
   "script" command (along the line of "script -e -c 'echo Y | git ...'
   /dev/stderr 2>/dev/null"). Unfortunately, it is not portable
   (specifically, OSX doesn't have the option -e, and "script" is not
   available under Windows). Are there other ways to hide the output of
   the prompts ?

I will squash patches #2-#8 for the last version.

Thanks,

Firmin


Firmin Martin (8):
  compat/terminal: let prompt accept input from pipe
  format-patch: confirmation whenever patches exist
  format-patch: add config option confirmOverwrite
  format-patch: add the option --confirm-overwrite
  t4014: test patches overwrite confirmation
  t4014: fix tests overwriting cover letter in silent
  doc/git-format-patch: describe --confirm-overwrite
  config/format: describe format.confirmOverwrite

 Documentation/config/format.txt    |   5 +++
 Documentation/git-format-patch.txt |  20 +++++
 builtin/log.c                      |  65 ++++++++++++--
 compat/terminal.c                  |  47 ++++++----
 t/t4014-format-patch.sh            | 140 +++++++++++++++++++++++++++--
 5 files changed, 247 insertions(+), 30 deletions(-)

-- 
2.31.1.449.gf23dcf53bc

Currently, git_prompt ignores input coming from anywhere other than
terminal (pipe, redirection etc.) meaning that standard prompt
auto-answering methods would have no effect:

        echo 'Y' | git ...
        yes 'Y' | git ...
        git ... <input.txt

It also prevents git subcommands using git_prompt to be tested using
such methods.

This patch fixes this issue by considering standard input when !isatty(0).
It also rearranges the control flow to close input and output file handlers.

Signed-off-by: Firmin Martin <redacted>
---
 compat/terminal.c | 47 ++++++++++++++++++++++++++++-------------------
 1 file changed, 28 insertions(+), 19 deletions(-)

diff --git a/compat/terminal.c b/compat/terminal.c
index 43b73ddc75..c12e0b9ab9 100644
--- a/compat/terminal.c
+++ b/compat/terminal.c

@@ -202,41 +202,50 @@ static int mingw_getchar(void)
 char *git_terminal_prompt(const char *prompt, int echo)
 {
 	static struct strbuf buf = STRBUF_INIT;
-	int r;
-	FILE *input_fh, *output_fh;
+	int r, input_not_from_tty = !isatty(STDIN_FILENO);
+	FILE *input_fh = NULL, *output_fh = NULL;
+	char* ret = NULL;
+
+	if (input_not_from_tty) 
+		input_fh = stdin;
+	else
+		input_fh = fopen(INPUT_PATH, "r" FORCE_TEXT);
 
-	input_fh = fopen(INPUT_PATH, "r" FORCE_TEXT);
 	if (!input_fh)
-		return NULL;
+		goto done;
 
 	output_fh = fopen(OUTPUT_PATH, "w" FORCE_TEXT);
-	if (!output_fh) {
-		fclose(input_fh);
-		return NULL;
-	}
 
-	if (!echo && disable_echo()) {
-		fclose(input_fh);
-		fclose(output_fh);
-		return NULL;
-	}
+	if (!output_fh) 
+		goto done;
+
+	if (!echo && disable_echo()) 
+		goto done;
 
 	fputs(prompt, output_fh);
 	fflush(output_fh);
 
 	r = strbuf_getline_lf(&buf, input_fh);
-	if (!echo) {
+
+	if (input_not_from_tty) 
+		fputs(buf.buf, output_fh);
+
+	if (!echo || input_not_from_tty) {
 		putc('\n', output_fh);
 		fflush(output_fh);
 	}
 
 	restore_term();
-	fclose(input_fh);
-	fclose(output_fh);
 
-	if (r == EOF)
-		return NULL;
-	return buf.buf;
+	if (r != EOF)
+		ret = buf.buf;
+done:
+	if (input_fh && input_fh != stdin) 
+		fclose(input_fh);
+	if (output_fh)
+		fclose(output_fh);
+
+	return ret;
 }
 
 /*

-- 
2.31.1.449.g4a44fa8106

Currently, git-format-patch, along with the option --cover-letter,
unconditionaly overwrites a cover letter with the same name (if
present). Although this is a desired behaviour for patches which are
auto-generated from Git commits log, it might not be the case for a
cover letter whose the content is meticulously written manually.

Particulary, this behaviour could be awkward in the following
hypothetical situations:

* The user can easily erase a cover letter coming from prior versions or
  another patch series by reusing an old command line (e.g. autocompleted
  from the shell history).

* Assuming that the user is writing a cover letter and realizes that
  small changes should be made. They make the change, amend and
  format-patch again to regenerate patches. If it happens that they use
  the same command again (e.g. with --cover-letter), the cover letter
  being written is gone.

This patch addresses this issue by asking confirmation from the user
whenever a cover letter or a patch with the same name already exists.

Signed-off-by: Firmin Martin <redacted>
---
 builtin/log.c | 33 ++++++++++++++++++++++++++++-----
 1 file changed, 28 insertions(+), 5 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 6102893fcc..bada3db9eb 100644
--- a/builtin/log.c
+++ b/builtin/log.c

@@ -35,6 +35,7 @@
 #include "repository.h"
 #include "commit-reach.h"
 #include "range-diff.h"
+#include "prompt.h"
 
 #define MAIL_DEFAULT_WRAP 72
 #define COVER_FROM_AUTO_MAX_SUBJECT_LEN 100

@@ -959,6 +960,10 @@ static int open_next_file(struct commit *commit, const char *subject,
 			 struct rev_info *rev, int quiet)
 {
 	struct strbuf filename = STRBUF_INIT;
+	struct strbuf file_exists_prompt = STRBUF_INIT;
+	const char *yesno;
+	static int not_prompted = 1;
+	int res = 0;
 
 	if (output_directory) {
 		strbuf_addstr(&filename, output_directory);

@@ -972,17 +977,35 @@ static int open_next_file(struct commit *commit, const char *subject,
 	else
 		fmt_output_subject(&filename, subject, rev);
 
-	if (!quiet)
-		printf("%s\n", filename.buf + outdir_offset);
+	if (not_prompted && !access(filename.buf, F_OK)) {
+
+		/*
+		 * TRANSLATORS: Make sure to include [Y] and [n] in your
+		 * translation. The program will only accept English input
+		 * at this point.
+		 */
+		strbuf_addf(&file_exists_prompt, _("The file '%s' already exists.\n"
+			"Would you overwrite this file and subsequent ones [Y/n]? "), filename.buf);
+		yesno = git_prompt(file_exists_prompt.buf, PROMPT_ECHO);
+		not_prompted = 0;
+		if (tolower(*yesno) == 'n') {
+			res = -1;
+			goto done;
+		}
+	}
 
 	if ((rev->diffopt.file = fopen(filename.buf, "w")) == NULL) {
 		error_errno(_("cannot open patch file %s"), filename.buf);
-		strbuf_release(&filename);
-		return -1;
+		res = -1;
+		goto done;
 	}
 
+	if (!quiet)
+		printf("%s\n", filename.buf + outdir_offset);
+done:
 	strbuf_release(&filename);
-	return 0;
+	strbuf_release(&file_exists_prompt);
+	return res;
 }
 
 static void get_patch_ids(struct rev_info *rev, struct patch_ids *ids)

-- 
2.31.1.449.g4a44fa8106

Provide the configuration option format.confirmOverwrite. This option
will decide whether a confirmation is required to overwrite cover letter
or patches issued by git-format-patch.

It accepts three values.
* "never"/"always": never/always ask confirmation whenever cover letter
or patches are subject to be overwritten.
* "cover": ask confirmation only if a cover letter is subject to be
overwritten.

format.confirmOverwrite defaults to "cover" to avoid cover letter
being written be overwritten mistakenly.

Signed-off-by: Firmin Martin <redacted>
---
 builtin/log.c | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/builtin/log.c b/builtin/log.c
index bada3db9eb..ec9848da70 100644
--- a/builtin/log.c
+++ b/builtin/log.c

@@ -816,6 +816,12 @@ enum auto_base_setting {
 	AUTO_BASE_WHEN_ABLE
 };
 
+enum confirm_overwrite_setting {
+	CONFIRM_OVERWRITE_NEVER,
+	CONFIRM_OVERWRITE_ALWAYS,
+	CONFIRM_OVERWRITE_COVER
+};
+
 static enum thread_level thread;
 static int do_signoff;
 static enum auto_base_setting auto_base;

@@ -827,6 +833,7 @@ static const char *config_output_directory;
 static enum cover_from_description cover_from_description_mode = COVER_FROM_MESSAGE;
 static int show_notes;
 static struct display_notes_opt notes_opt;
+static enum confirm_overwrite_setting confirm_overwrite = CONFIRM_OVERWRITE_COVER;
 
 static enum cover_from_description parse_cover_from_description(const char *arg)
 {

@@ -844,6 +851,18 @@ static enum cover_from_description parse_cover_from_description(const char *arg)
 		die(_("%s: invalid cover from description mode"), arg);
 }
 
+static enum confirm_overwrite_setting parse_confirm_overwrite(const char *arg)
+{
+	if (!arg || !strcasecmp(arg, "cover"))
+		return CONFIRM_OVERWRITE_COVER;
+	else if (!strcasecmp(arg, "always"))
+		return CONFIRM_OVERWRITE_ALWAYS;
+	else if (!strcasecmp(arg, "never"))
+		return CONFIRM_OVERWRITE_NEVER;
+	else
+		die(_("%s: invalid file overwrite setting"), arg);
+}
+
 static int git_format_config(const char *var, const char *value, void *cb)
 {
 	if (!strcmp(var, "format.headers")) {

@@ -949,6 +968,10 @@ static int git_format_config(const char *var, const char *value, void *cb)
 		cover_from_description_mode = parse_cover_from_description(value);
 		return 0;
 	}
+	if (!strcmp(var, "format.confirmoverwrite")) {
+		confirm_overwrite = parse_confirm_overwrite(value);
+		return 0;
+	}
 
 	return git_log_config(var, value, cb);
 }

@@ -977,7 +1000,10 @@ static int open_next_file(struct commit *commit, const char *subject,
 	else
 		fmt_output_subject(&filename, subject, rev);
 
-	if (not_prompted && !access(filename.buf, F_OK)) {
+	if (not_prompted &&
+        ((rev->nr == 0 && confirm_overwrite == CONFIRM_OVERWRITE_COVER) || 
+          confirm_overwrite == CONFIRM_OVERWRITE_ALWAYS) &&
+	  !access(filename.buf, F_OK)) {
 
 		/*
 		 * TRANSLATORS: Make sure to include [Y] and [n] in your

-- 
2.31.1.449.g4a44fa8106

This command line option acts in the same way as the configuration option
format.confirmOverwrite.

Signed-off-by: Firmin Martin <redacted>
---
 builtin/log.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/builtin/log.c b/builtin/log.c
index ec9848da70..0ce8778338 100644
--- a/builtin/log.c
+++ b/builtin/log.c

@@ -1776,6 +1776,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	int quiet = 0;
 	const char *reroll_count = NULL;
 	char *cover_from_description_arg = NULL;
+	char *confirm_overwrite_arg = NULL;
 	char *branch_name = NULL;
 	char *base_commit = NULL;
 	struct base_tree_info bases;

@@ -1818,6 +1819,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		OPT_STRING(0, "cover-from-description", &cover_from_description_arg,
 			    N_("cover-from-description-mode"),
 			    N_("generate parts of a cover letter based on a branch's description")),
+		OPT_STRING(0, "confirm-overwrite", &confirm_overwrite_arg, N_("when"),
+			    N_("overwrite cover letter/patches with or without confirmation")),
 		OPT_CALLBACK_F(0, "subject-prefix", &rev, N_("prefix"),
 			    N_("use [<prefix>] instead of [PATCH]"),
 			    PARSE_OPT_NONEG, subject_prefix_callback),

@@ -1919,6 +1922,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	if (cover_from_description_arg)
 		cover_from_description_mode = parse_cover_from_description(cover_from_description_arg);
 
+	if (confirm_overwrite_arg)
+		confirm_overwrite = parse_confirm_overwrite(confirm_overwrite_arg);
+
 	if (reroll_count) {
 		struct strbuf sprefix = STRBUF_INIT;
 

-- 
2.31.1.449.g4a44fa8106

Signed-off-by: Firmin Martin <redacted>
---
 t/t4014-format-patch.sh | 123 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 123 insertions(+)

diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 712d4b5ddf..cf7e48f4de 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh

@@ -2256,6 +2256,129 @@ test_expect_success 'format-patch --pretty=mboxrd' '
 	test_cmp expect actual
 '
 
+# $1 = git format-patch extra arguments
+confirm_overwrite_setup () {
+	test_when_finished "rm -rf confirm-overwrite" &&
+	git format-patch $1 -o confirm-overwrite main..side &&
+	for patch in confirm-overwrite/*; do echo 'APPENDUM' >>$patch; done
+}
+
+# $1 = format.confirmOverwrite value
+# $2 = git format-patch extra arguments
+# $3 = git format-patch with prompt (Y/N) or without it
+confirm_overwrite_test_body () {
+	if test ! -z $1; 
+	then
+		test_config format.confirmOverwrite $1
+	fi &&
+	case "$3" in
+	Y)
+		echo Y | git format-patch $2 -o confirm-overwrite main..side 
+		;;
+	N)
+		echo N | test_must_fail git format-patch $2 -o confirm-overwrite main..side
+		;;
+	*)
+		git format-patch $2 -o confirm-overwrite main..side 
+		;;
+	esac
+}
+
+# true if all patches are overwritten, false otherwise
+confirm_overwrite_all_overwritten () {
+	for patch in confirm-overwrite/*; do test_i18ngrep ! "^APPENDUM$" $patch; done 
+}
+
+test_expect_success 'format-patch overwrite unconditionally patch series without cover letter' '
+	confirm_overwrite_setup &&
+	confirm_overwrite_test_body &&
+	confirm_overwrite_all_overwritten
+'
+
+test_expect_success 'format-patch overwrites present cover letter (prompt/Y)' '
+	confirm_overwrite_setup "--cover-letter" &&
+	confirm_overwrite_test_body "" "--cover-letter" "Y" &&
+	confirm_overwrite_all_overwritten
+'
+
+test_expect_success 'format-patch does not overwrite present cover letter (prompt/N)' '
+	confirm_overwrite_setup "--cover-letter" &&
+	confirm_overwrite_test_body "" "--cover-letter" "N" &&
+	! confirm_overwrite_all_overwritten
+'
+
+test_expect_success 'format-patch --numbered-files overwrites existing cover letter (prompt/Y)' '
+	confirm_overwrite_setup "--cover-letter --numbered-files" &&
+	confirm_overwrite_test_body "" "--cover-letter --numbered-files" "Y" &&
+	confirm_overwrite_all_overwritten
+'
+
+test_expect_success 'format-patch --numbered-files does not overwrite existing cover letter (prompt/N)' '
+	confirm_overwrite_setup "--cover-letter --numbered-files" &&
+	confirm_overwrite_test_body "" "--cover-letter --numbered-files" "N" &&
+	! confirm_overwrite_all_overwritten
+'
+
+test_expect_success 'format-patch overwrites existing cover letter (format.confirmOverwrite = never)' '
+	confirm_overwrite_setup "--cover-letter" &&
+	confirm_overwrite_test_body "never" "--cover-letter" &&
+	confirm_overwrite_all_overwritten
+'
+
+test_expect_success 'format-patch: the user disagrees to overwrite existing cover letter (format.confirmOverwrite = always)' '
+	confirm_overwrite_setup "--cover-letter" &&
+	confirm_overwrite_test_body "always" "--cover-letter" "N" &&
+	! confirm_overwrite_all_overwritten
+'
+
+test_expect_success 'format-patch: the user agrees to overwrite existing cover letter (format.confirmOverwrite = always)' '
+	confirm_overwrite_setup "--cover-letter" &&
+	confirm_overwrite_test_body "always" "--cover-letter" "Y" &&
+	confirm_overwrite_all_overwritten
+'
+
+test_expect_success 'format-patch --confirm-overwrite has higher priority than format.confirmOverwrite' '
+	confirm_overwrite_setup &&
+	confirm_overwrite_test_body "always" "--confirm-overwrite never" &&
+	confirm_overwrite_all_overwritten
+'
+
+test_expect_success 'format-patch --confirm-overwrite cover: the user agrees to overwrite existing cover letter' '
+	confirm_overwrite_setup "--cover-letter" &&
+	confirm_overwrite_test_body "never" "--cover-letter --confirm-overwrite cover" "Y" &&
+	confirm_overwrite_all_overwritten
+'
+
+test_expect_success 'format-patch --confirm-overwrite cover: the user disagrees to overwrite existing cover letter' '
+	confirm_overwrite_setup "--cover-letter" &&
+	confirm_overwrite_test_body "never" "--cover-letter --confirm-overwrite cover" "N" &&
+	! confirm_overwrite_all_overwritten
+'
+
+test_expect_success 'format-patch --confirm-overwrite always: the user agrees to overwrite existing patches' '
+	confirm_overwrite_setup &&
+	confirm_overwrite_test_body "never" "--confirm-overwrite always" "Y" &&
+	confirm_overwrite_all_overwritten
+'
+
+test_expect_success 'format-patch --confirm-overwrite always: the user disagrees to overwrite existing patches' '
+	confirm_overwrite_setup &&
+	confirm_overwrite_test_body "never" "--confirm-overwrite always" "N" &&
+	! confirm_overwrite_all_overwritten
+'
+
+test_expect_success 'format-patch --confirm-overwrite never: overwrite cover letter unconditionally' '
+	confirm_overwrite_setup "--cover-letter" &&
+	confirm_overwrite_test_body "always" "--cover-letter --confirm-overwrite never" &&
+	confirm_overwrite_all_overwritten
+'
+
+test_expect_success 'format-patch --confirm-overwrite never: overwrite patches unconditionally' '
+	confirm_overwrite_setup &&
+	confirm_overwrite_test_body "always" "--confirm-overwrite never" &&
+	confirm_overwrite_all_overwritten
+'
+
 test_expect_success 'interdiff: setup' '
 	git checkout -b boop main &&
 	test_commit fnorp blorp &&

-- 
2.31.1.449.g4a44fa8106

These tests are broken due to the new configuration option
format.confirmOvewrite whose the default value requires confirmation
from user whenever an existing cover letter is subject to be
overwritten.

Also change some pairs of git config and git config --unset to a single
test_config.

Signed-off-by: Firmin Martin <redacted>
---
 t/t4014-format-patch.sh | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index cf7e48f4de..a34598beca 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh

@@ -669,11 +669,13 @@ test_expect_success 'failure to write cover-letter aborts gracefully' '
 '
 
 test_expect_success 'cover-letter inherits diff options' '
+	test_when_finished "rm 0000-cover-letter.patch" &&
 	git mv file foo &&
 	git commit -m foo &&
 	git format-patch --no-renames --cover-letter -1 &&
 	check_patch 0000-cover-letter.patch &&
 	! grep "file => foo .* 0 *\$" 0000-cover-letter.patch &&
+	rm 0000-cover-letter.patch &&
 	git format-patch --cover-letter -1 -M &&
 	grep "file => foo .* 0 *\$" 0000-cover-letter.patch
 '

@@ -1917,10 +1919,9 @@ test_expect_success 'cover letter with nothing' '
 
 test_expect_success 'cover letter auto' '
 	mkdir -p tmp &&
-	test_when_finished "rm -rf tmp;
-		git config --unset format.coverletter" &&
+	test_when_finished "rm -rf tmp" &&
 
-	git config format.coverletter auto &&
+	test_config format.coverletter auto &&
 	git format-patch -o tmp -1 >list &&
 	test_line_count = 1 list &&
 	git format-patch -o tmp -2 >list &&

@@ -1929,10 +1930,10 @@ test_expect_success 'cover letter auto' '
 
 test_expect_success 'cover letter auto user override' '
 	mkdir -p tmp &&
-	test_when_finished "rm -rf tmp;
-		git config --unset format.coverletter" &&
+	test_when_finished "rm -rf tmp" &&
 
-	git config format.coverletter auto &&
+	test_config format.confirmOverwrite never &&
+	test_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 &&

@@ -2386,6 +2387,7 @@ test_expect_success 'interdiff: setup' '
 '
 
 test_expect_success 'interdiff: cover-letter' '
+	test_when_finished "rm 0000-cover-letter.patch" &&
 	sed "y/q/ /" >expect <<-\EOF &&
 	+fleep
 	--q

@@ -2398,16 +2400,19 @@ test_expect_success 'interdiff: cover-letter' '
 '
 
 test_expect_success 'interdiff: reroll-count' '
+	test_when_finished "rm v2-0000-cover-letter.patch" &&
 	git format-patch --cover-letter --interdiff=boop~2 -v2 -1 boop &&
 	test_i18ngrep "^Interdiff ..* v1:$" v2-0000-cover-letter.patch
 '
 
 test_expect_success 'interdiff: reroll-count with a non-integer' '
+	test_when_finished "rm v2.2-0000-cover-letter.patch" &&
 	git format-patch --cover-letter --interdiff=boop~2 -v2.2 -1 boop &&
 	test_i18ngrep "^Interdiff:$" v2.2-0000-cover-letter.patch
 '
 
 test_expect_success 'interdiff: reroll-count with a integer' '
+	test_when_finished "rm v2-0000-cover-letter.patch" &&
 	git format-patch --cover-letter --interdiff=boop~2 -v2 -1 boop &&
 	test_i18ngrep "^Interdiff ..* v1:$" v2-0000-cover-letter.patch
 '

-- 
2.31.1.449.g4a44fa8106

Signed-off-by: Firmin Martin <redacted>
---
 Documentation/git-format-patch.txt | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index 911da181a1..49f08b5e51 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt

@@ -20,6 +20,7 @@ SYNOPSIS
 		   [--in-reply-to=<message id>] [--suffix=.<sfx>]
 		   [--ignore-if-in-upstream]
 		   [--cover-from-description=<mode>]
+		   [--confirm-overwrite=<when>]
 		   [--rfc] [--subject-prefix=<subject prefix>]
 		   [(--reroll-count|-v) <n>]
 		   [--to=<email>] [--cc=<email>]

@@ -195,6 +196,7 @@ will want to ensure that threading is disabled for `git send-email`.
 --cover-from-description=<mode>::
 	Controls which parts of the cover letter will be automatically
 	populated using the branch's description.
+
 +
 If `<mode>` is `message` or `default`, the cover letter subject will be
 populated with placeholder text. The body of the cover letter will be

@@ -212,6 +214,23 @@ is greater than 100 bytes, then the mode will be `message`, otherwise
 If `<mode>` is `none`, both the cover letter subject and body will be
 populated with placeholder text.
 
+`--confirm-overwrite`=<when>::
+	Specifies when Git must ask the user to confirm whether existing
+	patches or cover letter of the same name should be overwritten.
+	<when> possible values are:
++
+--
+always;;
+never;;
+	Always/never prompt for confirmation whenever patches or a cover letter
+	are subject to be overwritten.
+cover;;
+	Ask confirmation whenever a cover letter is subject to be overwritten.
+--
++
+Defaults to the value of the `format.confirmOverwrite` variable, or
+"cover" if unconfigured.
+
 --subject-prefix=<subject prefix>::
 	Instead of the standard '[PATCH]' prefix in the subject
 	line, instead use '[<subject prefix>]'. This

@@ -409,6 +428,7 @@ with configuration variables.
 	outputDirectory = <directory>
 	coverLetter = auto
 	coverFromDescription = auto
+	confirmOverwrite = onlyCover
 ------------
 
 

-- 
2.31.1.449.g4a44fa8106

Signed-off-by: Firmin Martin <redacted>
---
 Documentation/config/format.txt | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/config/format.txt b/Documentation/config/format.txt
index fdbc06a4d2..bc189e3ec8 100644
--- a/Documentation/config/format.txt
+++ b/Documentation/config/format.txt

@@ -42,6 +42,11 @@ format.coverFromDescription::
 	description. See the `--cover-from-description` option in
 	linkgit:git-format-patch[1].
 
+format.confirmOverwrite::
+	Determines when the user must be prompted for confirmation whenever a
+	cover letter or a patch is subject to be overwritten. See the
+	`--confirm-overwrite` option in linkgit:git-format-patch[1].
+
 format.signature::
 	The default for format-patch is to output a signature containing
 	the Git version number. Use this variable to change that default.

-- 
2.31.1.449.g4a44fa8106

Firmin Martin wrote:

Currently, git-format-patch, along with the option --cover-letter,
unconditionally overwrites a cover letter with the same name (if
present). Although this is a desired behaviour for patches which are
auto-generated from Git commits log, it might not be the case for a
cover letter whose the content is meticulously written manually.

This is one of the reasons I never use git format-patch directly, but I
use a tool on top: git send-series[1].

It would be nice if git format-patch grabbed the text of the body from
somewhere, and even better if git branch learned --edit-cover-letter.

None of this invalidates the usefulness of your patches, of course.

Cheers.

[1] https://github.com/felipec/git-send-series

-- 
Felipe Contreras

On 06/05/21 23.51, Firmin Martin wrote:

+always;;
+never;;
+	Always/never prompt for confirmation whenever patches or a cover letter
+	are subject to be overwritten.
+cover;;
+	Ask confirmation whenever a cover letter is subject to be overwritten.
+--

For `always` and `never`, I think s/patches or/patches and\/or
For `cover`, I think s/whenever/whenever only (add `only` after `whenever`

-- 
An old man doll... just what I always wanted! - Clara

Hi Felipe,

On Thu, May 06, 2021 at 08:46:16PM -0500, Felipe Contreras wrote:

Firmin Martin wrote:

quoted

Currently, git-format-patch, along with the option --cover-letter,
unconditionally overwrites a cover letter with the same name (if
present). Although this is a desired behaviour for patches which are
auto-generated from Git commits log, it might not be the case for a
cover letter whose the content is meticulously written manually.

This is one of the reasons I never use git format-patch directly, but I
use a tool on top: git send-series[1].

It seems like everyone has written some sort of tooling on top of
format-patch at this point. Taking a cursory look at your tool, perhaps
a feature like `--previous-cover-letter <file>` might provide most of
the functionality that most tooling that I've seen gives.

Perhaps this option could parse a cover letter from a previous version
of a patch and use it to populate the next version number, In-Reply-To,
cover letter subject/body, To/Cc lists and maybe more. I think that
extracting the information would be pretty easy but designing the UI it
in a non-obtuse way would be pretty challenging.

It would be nice if git format-patch grabbed the text of the body from
somewhere, and even better if git branch learned --edit-cover-letter.

Well, you're in luck! I wanted the same thing a couple of years back so
I implemented the --cover-from-description option[0]. It allows the cover
letter to be populated by the text given in
`git branch --edit-description`.

-Denton

[0]: https://git-scm.com/docs/git-format-patch#Documentation/git-format-patch.txt---cover-from-descriptionltmodegt

Hi Bagas,

Bagas Sanjaya [off-list ref] writes:

On 06/05/21 23.51, Firmin Martin wrote:

quoted

+always;;
+never;;
+	Always/never prompt for confirmation whenever patches or a cover letter
+	are subject to be overwritten.
+cover;;
+	Ask confirmation whenever a cover letter is subject to be overwritten.
+--

For `always` and `never`, I think s/patches or/patches and\/or
For `cover`, I think s/whenever/whenever only (add `only` after `whenever`

Makes sense. I will change it if this text remains in v2.

-- 
An old man doll... just what I always wanted! - Clara

Thanks,

Firmin

On Thu, May 06 2021, Firmin Martin wrote:

BACKGROUND
==========

Currently, git-format-patch, along with the option --cover-letter,
unconditionally overwrites a cover letter with the same name (if
present). Although this is a desired behaviour for patches which are
auto-generated from Git commits log, it might not be the case for a
cover letter whose the content is meticulously written manually.

Particulary, this behaviour could be awkward in the following
hypothetical situations:

* The user can easily erase a cover letter coming from prior versions or
another patch series by reusing an old command line (e.g. autocompleted
from the shell history).

* Assuming that the user is writing a cover letter and realizes that
small changes should be made. They make the change, amend and
format-patch again to regenerate patches. If it happens that they use
the same command again (e.g. with --cover-letter), the cover letter
being written is gone.

This patch series addresses this kind of issue by asking confirmation
from the user whenever a cover letter or a patch is subject to be
overwritten.

I like the goal here, I'm another person with ad-hoc tooling around
format-patch to manage / avoid this particular scenario and related
issues (e.g. I have a wrapper to rm -rf the output directory & re-crete
it, in case I want to rebase but use the same -vN version).

I wonder if you've considered some ways to automatically and more gently
detect these cases, such as:

 1. When we generate the patch, set the mtime manually to the time in
    the commit object. When clobbering a file see if they correspond. If
    mtime != expected => *boom* and ask for confirmation.

 2. We already include a blurb like "2.31.1.838.g7ac6e98bb53" (the git
    version) if you have format.signature set. How about making that
    include a short hash of the preceding lines. If it doesn't match we
    can ask, but if it does we clobber.

    This has the added benefit that other could script their MUAs to
    highlight manually edited patches.

 3. Ditto #2 but generate the new file in a tempfile, diff them, if
    they're different complain. This also opens the door to make this
    neatly integrate with git-diff's -I option, so you could specify
    regexes to ignore. By default we could ignore lines that change
    known headers we expect to change.

 4. The format we output would need parsing, but it's not that
    hard. I.e. we expect something like:

        [...]        
        Subject: [PATCH 0/1] *** SUBJECT HERE ***
        MIME-Version: 1.0
        Content-Type: text/plain; charset=UTF-8
        Content-Transfer-Encoding: 8bit
        
        *** BLURB HERE ***
        
        Ævar Arnfjörð Bjarmason (1):
        [...]


    And similarly for patches we could narrow things to look between the
    "---" and an expected diffstat. So we could complain just about
    changes there.

    But maybe that's a fool's errand, e.g. it would be hard to catch
    manually commented-on range-diffs without implementing a full parser
    for that format...

None of these suggestions should be read as making perfect the enemy of
the good. I *do* rely on the behavior of the setting you're introducing
and "breaking", but I think the user-base of advanced format-patch users
is small enough that we could just configure things to "never" and move
on, but accidentally losing data (as happens now) is a worse default...

Hi Felipe,

Felipe Contreras [off-list ref] writes:

Firmin Martin wrote:

quoted

Currently, git-format-patch, along with the option --cover-letter,
unconditionally overwrites a cover letter with the same name (if
present). Although this is a desired behaviour for patches which are
auto-generated from Git commits log, it might not be the case for a
cover letter whose the content is meticulously written manually.

This is one of the reasons I never use git format-patch directly, but I
use a tool on top: git send-series[1].

This is good to know. As a newcomer to email-based workflow, I ignored
how people use git format-patch/send-mail efficiently.

It would be nice if git format-patch grabbed the text of the body from
somewhere,

In v2, I planned to grab the letter body from the cover letter subject
to being overwritten. Maybe if such a letter doesn't exist, we can instead
inherit the content of the cover letter from prior series.

and even better if git branch learned --edit-cover-letter.
None of this invalidates the usefulness of your patches, of course.

Cheers.

[1] https://github.com/felipec/git-send-series

-- 
Felipe Contreras

Thanks for your comment,

Firmin

Hi Denton,

Denton Liu [off-list ref] writes:

Hi Felipe,

On Thu, May 06, 2021 at 08:46:16PM -0500, Felipe Contreras wrote:

quoted

Firmin Martin wrote:

quoted

Currently, git-format-patch, along with the option --cover-letter,
unconditionally overwrites a cover letter with the same name (if
present). Although this is a desired behaviour for patches which are
auto-generated from Git commits log, it might not be the case for a
cover letter whose the content is meticulously written manually.

This is one of the reasons I never use git format-patch directly, but I
use a tool on top: git send-series[1].

It seems like everyone has written some sort of tooling on top of
format-patch at this point. Taking a cursory look at your tool, perhaps
a feature like `--previous-cover-letter <file>` might provide most of
the functionality that most tooling that I've seen gives.

This is a good idea. We can default <file> to the target cover letter
(e.g., if -v2 is passed, v2-0000-cover-letter.patch or if
--numbered-files is passed, 0) if present, or the previous series' cover
letter.

Perhaps this option could parse a cover letter from a previous version
of a patch and use it to populate the next version number, In-Reply-To,
cover letter subject/body, To/Cc lists and maybe more.

Absolutely.

I think that extracting the information would be pretty easy but
designing the UI it in a non-obtuse way would be pretty challenging.

quoted

It would be nice if git format-patch grabbed the text of the body from
somewhere, and even better if git branch learned --edit-cover-letter.

Well, you're in luck! I wanted the same thing a couple of years back so
I implemented the --cover-from-description option[0]. It allows the cover
letter to be populated by the text given in
`git branch --edit-description`.

This is the reason I CCed you!

Thanks for your comment,

Firmin

-Denton

[0]: https://git-scm.com/docs/git-format-patch#Documentation/git-format-patch.txt---cover-from-descriptionltmodegt

Denton Liu wrote:

Hi Felipe,

On Thu, May 06, 2021 at 08:46:16PM -0500, Felipe Contreras wrote:

quoted

Firmin Martin wrote:

quoted

Currently, git-format-patch, along with the option --cover-letter,
unconditionally overwrites a cover letter with the same name (if
present). Although this is a desired behaviour for patches which are
auto-generated from Git commits log, it might not be the case for a
cover letter whose the content is meticulously written manually.

This is one of the reasons I never use git format-patch directly, but I
use a tool on top: git send-series[1].

It seems like everyone has written some sort of tooling on top of
format-patch at this point. Taking a cursory look at your tool, perhaps
a feature like `--previous-cover-letter <file>` might provide most of
the functionality that most tooling that I've seen gives.

If that worked correctly, maybe, but not for my tool.

Some of the features still missing:

 * List of people to cc
 * refs of where the branch was in vX
 * Automatic rangediff
 * Storing other metadata like last Message-Id

Perhaps this option could parse a cover letter from a previous version
of a patch and use it to populate the next version number, In-Reply-To,
cover letter subject/body, To/Cc lists and maybe more. I think that
extracting the information would be pretty easy but designing the UI it
in a non-obtuse way would be pretty challenging.

Where would you put the Cc list for example?

quoted

It would be nice if git format-patch grabbed the text of the body from
somewhere, and even better if git branch learned --edit-cover-letter.

Well, you're in luck! I wanted the same thing a couple of years back so
I implemented the --cover-from-description option[0]. It allows the cover
letter to be populated by the text given in
`git branch --edit-description`.

I did see --cover-from-description and thought of making use of it on my
tool, but I thought it only updated the subject of the cover-letter. Now
I see --cover-from-description=subject does exactly what I would want.
Nice! Although I've no idea why that option is called "subject".

My only comment is: why doesn't --cover-from-description do something
useful?

Cheers.

-- 
Felipe Contreras

Firmin Martin wrote:

Denton Liu [off-list ref] writes:

quoted

It seems like everyone has written some sort of tooling on top of
format-patch at this point. Taking a cursory look at your tool, perhaps
a feature like `--previous-cover-letter <file>` might provide most of
the functionality that most tooling that I've seen gives.

This is a good idea. We can default <file> to the target cover letter
(e.g., if -v2 is passed, v2-0000-cover-letter.patch or if
--numbered-files is passed, 0) if present, or the previous series' cover
letter.

That's a good idea.

Actually I don't see any reason why by default it isn't
0000-v2-cover-letter.patch.

If it was it would diminish the need to warn people before overwriting
0000-cover-letter.patch.

-- 
Felipe Contreras