git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Firmin Martin <firminmartin24@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, Jeff King <peff@peff.net>,
	Johannes Schindelin <johannes.schindelin@gmail.com>,
	Erik Faye-Lund <kusmabite@gmail.com>,
	Denton Liu <liu.denton@gmail.com>
Subject: Re: [PATCH v1 2/8] format-patch: confirmation whenever patches exist
Date: Mon, 10 May 2021 05:30:13 +0200	[thread overview]
Message-ID: <878s4ngta2.fsf@Inspiron.i-did-not-set--mail-host-address--so-tickle-me> (raw)
In-Reply-To: <xmqqk0obwhi2.fsf@gitster.g>


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

  reply	other threads:[~2021-05-10  3:30 UTC|newest]

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-06 16:50 [PATCH v1 0/8] format-patch: introduce --confirm-overwrite Firmin Martin
2021-05-06 16:50 ` [PATCH v1 1/8] compat/terminal: let prompt accept input from pipe Firmin Martin
2021-05-06 23:37   ` Junio C Hamano
2021-05-07  4:54     ` Jeff King
2021-05-07  5:25       ` Junio C Hamano
2021-05-10  4:18       ` Firmin Martin
2021-05-10 21:32         ` Jeff King
2021-05-11  3:38           ` Junio C Hamano
2021-05-11  6:10             ` Jeff King
2021-05-11  6:17               ` Junio C Hamano
2021-05-11  6:37                 ` Jeff King
2021-05-06 16:50 ` [PATCH v1 2/8] format-patch: confirmation whenever patches exist Firmin Martin
2021-05-06 23:48   ` Junio C Hamano
2021-05-10  3:30     ` Firmin Martin [this message]
2021-05-10  7:32       ` Junio C Hamano
2021-05-11  3:17         ` Firmin Martin
2021-05-06 16:50 ` [PATCH v1 3/8] format-patch: add config option confirmOverwrite Firmin Martin
2021-05-06 16:50 ` [PATCH v1 4/8] format-patch: add the option --confirm-overwrite Firmin Martin
2021-05-06 16:50 ` [PATCH v1 5/8] t4014: test patches overwrite confirmation Firmin Martin
2021-05-06 16:51 ` [PATCH v1 6/8] t4014: fix tests overwriting cover letter in silent Firmin Martin
2021-05-06 16:51 ` [PATCH v1 7/8] doc/format-patch: describe --confirm-overwrite Firmin Martin
2021-05-07  3:32   ` Bagas Sanjaya
2021-05-10  4:22     ` Firmin Martin
2021-05-06 16:51 ` [PATCH v1 8/8] config/format: describe format.confirmOverwrite Firmin Martin
2021-05-06 22:33 ` [PATCH v1 0/8] format-patch: introduce --confirm-overwrite Junio C Hamano
2021-05-11  0:18   ` Firmin Martin
2021-05-07  1:46 ` Felipe Contreras
2021-05-07  8:55   ` Denton Liu
2021-05-11  1:09     ` Firmin Martin
2021-05-11  5:12       ` Felipe Contreras
2021-05-11  5:03     ` Felipe Contreras
2021-05-07 14:02   ` Sergey Organov
2021-05-11  0:46   ` Firmin Martin
2021-05-10 12:02 ` Ævar Arnfjörð Bjarmason

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=878s4ngta2.fsf@Inspiron.i-did-not-set--mail-host-address--so-tickle-me \
    --to=firminmartin24@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=johannes.schindelin@gmail.com \
    --cc=kusmabite@gmail.com \
    --cc=liu.denton@gmail.com \
    --cc=peff@peff.net \
    --subject='Re: [PATCH v1 2/8] format-patch: confirmation whenever patches exist' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).