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

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

[Firmin Martin]

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

[PATCH v1 1/8] compat/terminal: let prompt accept input from pipe

[Firmin Martin]

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 <firminmartin24@gmail.com>
---
 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

Re: [PATCH v1 1/8] compat/terminal: let prompt accept input from pipe

[Junio C Hamano]

Firmin Martin <firminmartin24@gmail.com> writes:

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

For testing, wouldn't lib-terminal.sh be usable for your purpose?
If not, what is the reason why it is insufficient?  Can we fix that
instead?

Allowing prompter to read from pipe has a big downside in the
production code: you cannot pipe data into our command, and let it
ask interactive questions from the end user by opening /dev/tty.


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

So this "fix" is probably very unwelcome, especially if done
unconditionally.

Thanks.

Re: [PATCH v1 1/8] compat/terminal: let prompt accept input from pipe

[Jeff King]

On Fri, May 07, 2021 at 08:37:49AM +0900, Junio C Hamano wrote:

> Firmin Martin <firminmartin24@gmail.com> writes:
> 
> > 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.
> 
> For testing, wouldn't lib-terminal.sh be usable for your purpose?
> If not, what is the reason why it is insufficient?  Can we fix that
> instead?

That doesn't work, because it insists on reading from /dev/tty and not
the pty that lib-terminal will set up as stdin. But...

> Allowing prompter to read from pipe has a big downside in the
> production code: you cannot pipe data into our command, and let it
> ask interactive questions from the end user by opening /dev/tty.

Right. The main purpose of the function was to let git-remote-https,
whose stdin is connected to git-fetch, get a password from the user.
Reading from stdin would break things badly there[1].

Looking at the second patch, the motivation here seems to be to use
git_prompt() for another run-of-the-mill prompt. But the right answer
is: don't do that. In fact, we recently-ish removed a similar case in
97387c8bdd (am: read interactive input from stdin, 2019-05-20) that was
likewise causing problems with the test suite.

I think we might consider renaming git_prompt(), or adding an
explanatory comment above it.

-Peff

[1] Sadly I don't think our test suite could notice the breakage
    introduced by this function. It uses the askpass feature to avoid
    triggering this code at all, because of course we can not reliably
    read from /dev/tty in the script. But with just this patch applied,
    and no credential helpers defined, trying "git ls-remote
    https://github.com/you/some-private-repo" shows the problem: you get
    prompted, but it never reads your input.

Re: [PATCH v1 1/8] compat/terminal: let prompt accept input from pipe

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> For testing, wouldn't lib-terminal.sh be usable for your purpose?
>> If not, what is the reason why it is insufficient?  Can we fix that
>> instead?
>
> That doesn't work, because it insists on reading from /dev/tty and not
> the pty that lib-terminal will set up as stdin. But...

I somehow thought that lib-terminal was a bit more complete in that
it would make the pty we allocate to the controlling terminal for
the process that runs the test program.  Sigh.

Re: [PATCH v1 1/8] compat/terminal: let prompt accept input from pipe

[Firmin Martin]

Jeff King <peff@peff.net> writes:

Hi Peff,

> On Fri, May 07, 2021 at 08:37:49AM +0900, Junio C Hamano wrote:
>
>> Firmin Martin <firminmartin24@gmail.com> writes:
>> 
>> > 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.
>> 
>> For testing, wouldn't lib-terminal.sh be usable for your purpose?
>> If not, what is the reason why it is insufficient?  Can we fix that
>> instead?
>
> That doesn't work, because it insists on reading from /dev/tty and not
> the pty that lib-terminal will set up as stdin. But...
>
>> Allowing prompter to read from pipe has a big downside in the
>> production code: you cannot pipe data into our command, and let it
>> ask interactive questions from the end user by opening /dev/tty.
>
> Right. The main purpose of the function was to let git-remote-https,
> whose stdin is connected to git-fetch, get a password from the user.
> Reading from stdin would break things badly there[1].
>
> Looking at the second patch, the motivation here seems to be to use
> git_prompt() for another run-of-the-mill prompt. But the right answer
> is: don't do that. In fact, we recently-ish removed a similar case in
> 97387c8bdd (am: read interactive input from stdin, 2019-05-20) that was
> likewise causing problems with the test suite.

I actually inspired myself from the two occurrences of git_prompt in
builtin/bisect--helper.c introduced in 09535f056b (bisect--helper:
reimplement `bisect_autostart` shell function in C, 2020-09-24).
Not sure if they should also be converted to a simple fgets.
I will do that in the v2 of this series if the prompt is still proven useful.

>
> I think we might consider renaming git_prompt(), or adding an
> explanatory comment above it.

I would be happy to do that. A comment along the line of 97387c8bdd (am:
read interactive input from stdin, 2019-05-20) and a "CAUTION: don't use
it for regular prompt" would suffice ?

>
> -Peff
>
> [1] Sadly I don't think our test suite could notice the breakage
>     introduced by this function. It uses the askpass feature to avoid
>     triggering this code at all, because of course we can not reliably
>     read from /dev/tty in the script. But with just this patch applied,
>     and no credential helpers defined, trying "git ls-remote
>     https://github.com/you/some-private-repo" shows the problem: you get
>     prompted, but it never reads your input.

Many thanks for your comments,

Firmin

Re: [PATCH v1 1/8] compat/terminal: let prompt accept input from pipe

[Jeff King]

On Mon, May 10, 2021 at 06:18:36AM +0200, Firmin Martin wrote:

> > Looking at the second patch, the motivation here seems to be to use
> > git_prompt() for another run-of-the-mill prompt. But the right answer
> > is: don't do that. In fact, we recently-ish removed a similar case in
> > 97387c8bdd (am: read interactive input from stdin, 2019-05-20) that was
> > likewise causing problems with the test suite.
> 
> I actually inspired myself from the two occurrences of git_prompt in
> builtin/bisect--helper.c introduced in 09535f056b (bisect--helper:
> reimplement `bisect_autostart` shell function in C, 2020-09-24).
> Not sure if they should also be converted to a simple fgets.

Yes, I think they should be switched.

It looks like since my earlier patches to "am" we have grown a
git_read_line_interactively() helper. See:

  https://lore.kernel.org/git/pull.755.git.git.1586374380709.gitgitgadget@gmail.com/

They should probably use that (and likewise, it would make sense for
git-am to switch to it).

> > I think we might consider renaming git_prompt(), or adding an
> > explanatory comment above it.
> 
> I would be happy to do that. A comment along the line of 97387c8bdd (am:
> read interactive input from stdin, 2019-05-20) and a "CAUTION: don't use
> it for regular prompt" would suffice ?

Yeah. You might want to point people at git_read_line_interactively() in
the same header file, too.

-Peff

Re: [PATCH v1 1/8] compat/terminal: let prompt accept input from pipe

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, May 10, 2021 at 06:18:36AM +0200, Firmin Martin wrote:
>
>> > Looking at the second patch, the motivation here seems to be to use
>> > git_prompt() for another run-of-the-mill prompt. But the right answer
>> > is: don't do that. In fact, we recently-ish removed a similar case in
>> > 97387c8bdd (am: read interactive input from stdin, 2019-05-20) that was
>> > likewise causing problems with the test suite.
>> 
>> I actually inspired myself from the two occurrences of git_prompt in
>> builtin/bisect--helper.c introduced in 09535f056b (bisect--helper:
>> reimplement `bisect_autostart` shell function in C, 2020-09-24).
>> Not sure if they should also be converted to a simple fgets.
>
> Yes, I think they should be switched.

OK, that is because in the context of a "bisect" session, we won't
be feeding any real data from its standard input, unlike "git am"
that may well be eating a patch stream from its standard input
stream.  If so, makes sense.

Re: [PATCH v1 1/8] compat/terminal: let prompt accept input from pipe

[Jeff King]

On Tue, May 11, 2021 at 12:38:10PM +0900, Junio C Hamano wrote:

> >> I actually inspired myself from the two occurrences of git_prompt in
> >> builtin/bisect--helper.c introduced in 09535f056b (bisect--helper:
> >> reimplement `bisect_autostart` shell function in C, 2020-09-24).
> >> Not sure if they should also be converted to a simple fgets.
> >
> > Yes, I think they should be switched.
> 
> OK, that is because in the context of a "bisect" session, we won't
> be feeding any real data from its standard input, unlike "git am"
> that may well be eating a patch stream from its standard input
> stream.  If so, makes sense.

Yes, though even in "git am", we forbid using interactive mode with
patches on stdin (and did so even when we were reading from the tty;
presumably the rule dates back to when it was a shell script and was
using stdin).

-Peff

Re: [PATCH v1 1/8] compat/terminal: let prompt accept input from pipe

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, May 11, 2021 at 12:38:10PM +0900, Junio C Hamano wrote:
>
>> >> I actually inspired myself from the two occurrences of git_prompt in
>> >> builtin/bisect--helper.c introduced in 09535f056b (bisect--helper:
>> >> reimplement `bisect_autostart` shell function in C, 2020-09-24).
>> >> Not sure if they should also be converted to a simple fgets.
>> >
>> > Yes, I think they should be switched.
>> 
>> OK, that is because in the context of a "bisect" session, we won't
>> be feeding any real data from its standard input, unlike "git am"
>> that may well be eating a patch stream from its standard input
>> stream.  If so, makes sense.
>
> Yes, though even in "git am", we forbid using interactive mode with
> patches on stdin (and did so even when we were reading from the tty;
> presumably the rule dates back to when it was a shell script and was
> using stdin).

As long as the "prompt and accept an single-line answer from the end
user" is restricted to "git am -i", I'll be perfectly OK with that.
I just do not want my regular "type '|' in my MUA to pipe the
current article to a command, and give 'git am -s' as the command"
workflow to get broken in the future when somebody blindly follows a
carelessly written direction to use a helper that reads from the
standard input for confirmation.  The condition under which use of
that helper is appropriate needs to be clearly spelled out.

Thanks.

Re: [PATCH v1 1/8] compat/terminal: let prompt accept input from pipe

[Jeff King]

On Tue, May 11, 2021 at 03:17:13PM +0900, Junio C Hamano wrote:

> >> OK, that is because in the context of a "bisect" session, we won't
> >> be feeding any real data from its standard input, unlike "git am"
> >> that may well be eating a patch stream from its standard input
> >> stream.  If so, makes sense.
> >
> > Yes, though even in "git am", we forbid using interactive mode with
> > patches on stdin (and did so even when we were reading from the tty;
> > presumably the rule dates back to when it was a shell script and was
> > using stdin).
> 
> As long as the "prompt and accept an single-line answer from the end
> user" is restricted to "git am -i", I'll be perfectly OK with that.
> I just do not want my regular "type '|' in my MUA to pipe the
> current article to a command, and give 'git am -s' as the command"
> workflow to get broken in the future when somebody blindly follows a
> carelessly written direction to use a helper that reads from the
> standard input for confirmation.  The condition under which use of
> that helper is appropriate needs to be clearly spelled out.

Yeah, I don't think anybody is proposing to change the behavior of "git
am" here (we might swap out the current fgets(stdin) for a helper which
does the equivalent). But I agree that any comment recommending one
versus the other should probably remind people to think about how stdin
is otherwise used in the program, and whether that will cause any
conflicts.

-Peff

[PATCH v1 2/8] format-patch: confirmation whenever patches exist

[Firmin Martin]

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 <firminmartin24@gmail.com>
---
 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

Re: [PATCH v1 2/8] format-patch: confirmation whenever patches exist

[Junio C Hamano]

Firmin Martin <firminmartin24@gmail.com> writes:

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

True.  But if we require confirmation before overwriting patches,
that would be overall worsening the end-user experience, I am
afraid.  In a 5-patch series with a cover-letter that was formatted,
proofread, corrected with "rebase -i" and then re-formatted, unless
you rephrased the titles of the patches, you'd get prompted once for
the cover letter (which *IS* valuable) and five-times for patches
(which is annoying).

It also is unfortunate that this change does not address another
annoyance after retitling a patch---the stale output from the
previous run is not overwritten with the updated one but is left in
the same directory as the output files from the latest run.

So, while I very much do welcome the motivation, I am not onboard
with this particular design.

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

When the API passes a structure that keeps track of state (like
"struct rev_info *rev", used to hold rev->diffopt.file which is
clearly a state), add a member to it, instead of inventing a
function local static that will hurt reuse of the API you are
touching.  This static variable will make it impossible from a
single process to format two patch series, but if it is made a part
of rev_info, you do not have to introduce such a limitation.

Re: [PATCH v1 2/8] format-patch: confirmation whenever patches exist

[Firmin Martin]

Hi Junio,

Junio C Hamano <gitster@pobox.com> writes:

> Firmin Martin <firminmartin24@gmail.com> writes:
>
>> 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.
>
> True.  But if we require confirmation before overwriting patches,
> that would be overall worsening the end-user experience, I am
> afraid.  In a 5-patch series with a cover-letter that was formatted,
> proofread, corrected with "rebase -i" and then re-formatted, unless
> you rephrased the titles of the patches, you'd get prompted once for
> the cover letter (which *IS* valuable) and five-times for patches
> (which is annoying).
This is true for this patch, but the semantics changed after the patch
#3. I really should have squashed them together to not create
confusion. Sorry about that.

After patch #3:

- The prompt happens only one time so that the user could decide whether they
want overwrite the existing file and subsequent ones. In your example,
the session would go like this:

    git format-patch --cover-letter -o patch upstream/master

    The file 'patch/0000-cover-letter.patch' already exists.
    Would you overwrite this file and subsequent ones [Y/n]? n
    fatal: failed to create cover-letter file

(replying Y would overwrite the cover letter and the patches as usual)
 
- The default behaviour affects only cover letters, meaning that, in
  your example, if the user employs instead 

    git format-patch -o patch upstream/master

(without --cover-letter) the second time, the patches are overwritten
without any disturbance.

After all, the point of this patch series is to prevent overwriting an
existing cover letter by using a cover letter template
(--cover-letter). 

>
> It also is unfortunate that this change does not address another
> annoyance after retitling a patch---the stale output from the
> previous run is not overwritten with the updated one but is left in
> the same directory as the output files from the latest run.


> So, while I very much do welcome the motivation, I am not onboard
> with this particular design.
>
>> 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;
>
> When the API passes a structure that keeps track of state (like
> "struct rev_info *rev", used to hold rev->diffopt.file which is
> clearly a state), add a member to it, instead of inventing a
> function local static that will hurt reuse of the API you are
> touching.  This static variable will make it impossible from a
> single process to format two patch series, but if it is made a part
> of rev_info, you do not have to introduce such a limitation.

OK, I will keep in mind of that. But after the discussion on git_prompt,
I think this variable will be dropped.

Many thanks for your comments,

Firmin

Re: [PATCH v1 2/8] format-patch: confirmation whenever patches exist

[Junio C Hamano]

Firmin Martin <firminmartin24@gmail.com> writes:

>> True.  But if we require confirmation before overwriting patches,
>> that would be overall worsening the end-user experience, I am
>> afraid.  In a 5-patch series with a cover-letter that was formatted,
>> proofread, corrected with "rebase -i" and then re-formatted, unless
>> you rephrased the titles of the patches, you'd get prompted once for
>> the cover letter (which *IS* valuable) and five-times for patches
>> (which is annoying).
> This is true for this patch, but the semantics changed after the patch
> #3. I really should have squashed them together to not create
> confusion. Sorry about that.

No, please keep them separate.  What we can do to avoid confusion
like I showed is to make a note on the earlier one, saying "with
this the user experience looks like this, which may be suboptimal
for such and such reasons, but in a later step it will be improved
in this and that way".

Re: [PATCH v1 2/8] format-patch: confirmation whenever patches exist

[Firmin Martin]

Junio C Hamano <gitster@pobox.com> writes:

> Firmin Martin <firminmartin24@gmail.com> writes:
>
>>> True.  But if we require confirmation before overwriting patches,
>>> that would be overall worsening the end-user experience, I am
>>> afraid.  In a 5-patch series with a cover-letter that was formatted,
>>> proofread, corrected with "rebase -i" and then re-formatted, unless
>>> you rephrased the titles of the patches, you'd get prompted once for
>>> the cover letter (which *IS* valuable) and five-times for patches
>>> (which is annoying).
>> This is true for this patch, but the semantics changed after the patch
>> #3. I really should have squashed them together to not create
>> confusion. Sorry about that.
>
> No, please keep them separate.  What we can do to avoid confusion
> like I showed is to make a note on the earlier one, saying "with
> this the user experience looks like this, which may be suboptimal
> for such and such reasons, but in a later step it will be improved
> in this and that way".

Ok, it's noted.

[PATCH v1 3/8] format-patch: add config option confirmOverwrite

[Firmin Martin]

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 <firminmartin24@gmail.com>
---
 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

[PATCH v1 4/8] format-patch: add the option --confirm-overwrite

[Firmin Martin]

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

Signed-off-by: Firmin Martin <firminmartin24@gmail.com>
---
 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

[PATCH v1 5/8] t4014: test patches overwrite confirmation

[Firmin Martin]

Signed-off-by: Firmin Martin <firminmartin24@gmail.com>
---
 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

[PATCH v1 6/8] t4014: fix tests overwriting cover letter in silent

[Firmin Martin]

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 <firminmartin24@gmail.com>
---
 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

[PATCH v1 7/8] doc/format-patch: describe --confirm-overwrite

[Firmin Martin]

Signed-off-by: Firmin Martin <firminmartin24@gmail.com>
---
 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

Re: [PATCH v1 7/8] doc/format-patch: describe --confirm-overwrite

[Bagas Sanjaya]

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

Re: [PATCH v1 7/8] doc/format-patch: describe --confirm-overwrite

[Firmin Martin]

Hi Bagas,

Bagas Sanjaya <bagasdotme@gmail.com> writes:

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

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

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

Thanks,

Firmin

[PATCH v1 8/8] config/format: describe format.confirmOverwrite

[Firmin Martin]

Signed-off-by: Firmin Martin <firminmartin24@gmail.com>
---
 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

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

[Junio C Hamano]

Firmin Martin <firminmartin24@gmail.com> writes:

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

"prior versions" implies that the user is better off using -v$n
where $n is the number greater than the one used for the prior
iteration by one, and there won't be any overwriting, so this is not
a very compelling use case.

But the next one is real.

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

Yes, after preparing, say, -v2, but before sending them out, it is
very plausible that proofreading of your own patches may make you
realize more issues in the series, which may make you go back to your
commits, "rebase -i" to improve them and re-run "format-patch -v2".

We do want to encourage such careful preparation of your patch
series before sending it out, and we want to support it well with
our tools.  Preventing overwriting of the cover (which will have the
same filename, with the same v2- prefix) is very valuable here.

There is another thing that I suspect people may find irritating in
the same workflow.  If you fix the commit title while "rebase -i" to
polish your v2 patch, it would result in a different filename from
the original v2, so you'd end up with something like

    v2-0000-cover-letter.patch
    v2-0001-thes-forny-change.patch
    v2-0001-this-phoney-change.patch
    v2-0002-another-sample-change.patch

while redoing a two-patch series.  The "thes-forny" thing is a
leftover from the first "format-patch -v2" run, you fixed typoes
with "rebase -i" after a self-review and other three files have the
result of the second "format-patch -v2" run.  You need a way to
somehow exclude that stale file when driving send-email; in other
words, before running

    git send-email v2-*.patch

you would want to move away v2-0001-thes-forny-change.patch that no
longer is part of the series.  I wonder if format-patch can help by
looking at the output directory before writing its output and move
the old files away, say, to "old-v2-*.patch" or something?  That
incidentally would solve your "files getting overwritten is
irritating" issue at the same time.

Coming back to the topic of cover letter, even when there is no risk
of ovetwriting, there is another thing we may want to improve to
help our users.  Suppose you are preparing your v2 patch after
sending out the v1.  The cover letter we generate for v2 will have
the same **BOILERPLATE** placeholders that need to be filled from
scratch.  As many things you wrote for the cover letter in the
previous round should be reusable, even though the list of titles of
the patch should be generated afresh, it would be nice if
format-patch can carry forward what you wrote in the cover letter
for the v1 iteration to the cover letter for this v2 iteration.

And when we have such a "reuse description in the existing cover
letter" support, the value of "don't overwrite" knob will mostly go
away.  Instead of failing the command by refusing to overwrite, you
can read what is in the existing cover-letter that is about to be
overwritten, combine the hand-written description with the material
automatically generated, and ovewrite the existing file, and as long
as you do a good job of preserving, nobody would complain that you
overwrote the file.

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

[Firmin Martin]

Junio C Hamano <gitster@pobox.com> writes:

> Firmin Martin <firminmartin24@gmail.com> writes:
>
>> 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).
>
> "prior versions" implies that the user is better off using -v$n
> where $n is the number greater than the one used for the prior
> iteration by one, and there won't be any overwriting, so this is not
> a very compelling use case.

True, silly of me.

>
> But the next one is real.
>
>> * 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.
>
> Yes, after preparing, say, -v2, but before sending them out, it is
> very plausible that proofreading of your own patches may make you
> realize more issues in the series, which may make you go back to your
> commits, "rebase -i" to improve them and re-run "format-patch -v2".
>
> We do want to encourage such careful preparation of your patch
> series before sending it out, and we want to support it well with
> our tools.  Preventing overwriting of the cover (which will have the
> same filename, with the same v2- prefix) is very valuable here.
>
> There is another thing that I suspect people may find irritating in
> the same workflow.  If you fix the commit title while "rebase -i" to
> polish your v2 patch, it would result in a different filename from
> the original v2, so you'd end up with something like
>
>     v2-0000-cover-letter.patch
>     v2-0001-thes-forny-change.patch
>     v2-0001-this-phoney-change.patch
>     v2-0002-another-sample-change.patch
>
> while redoing a two-patch series.  The "thes-forny" thing is a
> leftover from the first "format-patch -v2" run, you fixed typoes
> with "rebase -i" after a self-review and other three files have the
> result of the second "format-patch -v2" run.  You need a way to
> somehow exclude that stale file when driving send-email; in other
> words, before running
>
>     git send-email v2-*.patch
>
> you would want to move away v2-0001-thes-forny-change.patch that no
> longer is part of the series.  I wonder if format-patch can help by
> looking at the output directory before writing its output and move
> the old files away, say, to "old-v2-*.patch" or something?  That

I would go with v2-*.patch.old as it won't be matched by *.patch
(something tempting to do when only one version is present). Or, we can
even keep their filenames unchanged and move them into a subdirectory
old_<timestamp>/.

The case of --numbered-files should also be taking into account. In the
same fashion, the files that will be renamed/moved would be those which
are constituted with numbers.

> incidentally would solve your "files getting overwritten is
> irritating" issue at the same time.

I will work towards this.

>
> Coming back to the topic of cover letter, even when there is no risk
> of ovetwriting, there is another thing we may want to improve to
> help our users.  Suppose you are preparing your v2 patch after
> sending out the v1.  The cover letter we generate for v2 will have
> the same **BOILERPLATE** placeholders that need to be filled from
> scratch.  As many things you wrote for the cover letter in the
> previous round should be reusable, even though the list of titles of
> the patch should be generated afresh, it would be nice if
> format-patch can carry forward what you wrote in the cover letter
> for the v1 iteration to the cover letter for this v2 iteration.
>
> And when we have such a "reuse description in the existing cover
> letter" support, the value of "don't overwrite" knob will mostly go
> away.  Instead of failing the command by refusing to overwrite, you
> can read what is in the existing cover-letter that is about to be
> overwritten, combine the hand-written description with the material
> automatically generated, and ovewrite the existing file, and as long
> as you do a good job of preserving, nobody would complain that you
> overwrote the file.

I have thought about that before this series and agree that this is the
way to go, but I found it a bit challenging: how should I distinguish
the cover letter body and the shortlog (i.e. any content we don't want
to keep to the next cover letter), given that the user could do anything
they want (e.g. remove the shortlog, write after the shortlog, etc.) ?

To easier this task, should we introduce a delimiter between the letter
body and the shortlog as we already do with patches right after the
commit message and before the diffstat (e.g. "---") ? I ignore whether
there are functions we can directly reuse in trailer.c for this purpose.

Thanks,

Firmin

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

[Felipe Contreras]

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

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

[Denton Liu]

Hi Felipe,

On Thu, May 06, 2021 at 08:46:16PM -0500, Felipe Contreras wrote:
> 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 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

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

[Firmin Martin]

Hi Denton,

Denton Liu <liu.denton@gmail.com> writes:

> Hi Felipe,
>
> On Thu, May 06, 2021 at 08:46:16PM -0500, Felipe Contreras wrote:
>> 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 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.
>
>> 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

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

[Felipe Contreras]

Firmin Martin wrote:
> Denton Liu <liu.denton@gmail.com> writes:
> > 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

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

[Felipe Contreras]

Denton Liu wrote:
> Hi Felipe,
> 
> On Thu, May 06, 2021 at 08:46:16PM -0500, Felipe Contreras wrote:
> > 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 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?

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

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

[Sergey Organov]

Felipe Contreras <felipe.contreras@gmail.com> writes:

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

It does already. I use:

  git format-patch --cover-letter --cover-from-description=auto

that takes both subject and text for cover letter from branch
description.

> and even better if git branch learned --edit-cover-letter.

It reads:

  git branch --edit-description

Works for me.

-- Sergey Organov

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

[Firmin Martin]

Hi Felipe,

Felipe Contreras <felipe.contreras@gmail.com> writes:

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

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

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

[Ævar Arnfjörð Bjarmason]

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