diff mbox series

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

Message ID 20210506165102.123739-3-firminmartin24@gmail.com (mailing list archive)
State New
Headers show
Series format-patch: introduce --confirm-overwrite | expand

Commit Message

Firmin Martin May 6, 2021, 4:50 p.m. UTC
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(-)

Comments

Junio C Hamano May 6, 2021, 11:48 p.m. UTC | #1
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.
Firmin Martin May 10, 2021, 3:30 a.m. UTC | #2
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
Junio C Hamano May 10, 2021, 7:32 a.m. UTC | #3
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".
Firmin Martin May 11, 2021, 3:17 a.m. UTC | #4
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.
diff mbox series

Patch

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)