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