git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Firmin Martin <firminmartin24@gmail.com>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>,
	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 0/8] format-patch: introduce --confirm-overwrite
Date: Mon, 10 May 2021 14:02:19 +0200	[thread overview]
Message-ID: <87r1ieu6q4.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <20210506165102.123739-1-firminmartin24@gmail.com>


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

      parent reply	other threads:[~2021-05-10 13:40 UTC|newest]

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-06 16:50 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
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 [this message]

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=87r1ieu6q4.fsf@evledraar.gmail.com \
    --to=avarab@gmail.com \
    --cc=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 0/8] format-patch: introduce --confirm-overwrite' \
    /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).