git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Firmin Martin <firminmartin24@gmail.com>
To: Firmin Martin <firminmartin24@gmail.com>, git@vger.kernel.org
Cc: 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: [PATCH v1 0/8] format-patch: introduce --confirm-overwrite
Date: Thu,  6 May 2021 18:50:54 +0200	[thread overview]
Message-ID: <20210506165102.123739-1-firminmartin24@gmail.com> (raw)

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


             reply	other threads:[~2021-05-06 16:52 UTC|newest]

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

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=20210506165102.123739-1-firminmartin24@gmail.com \
    --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 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).