| Message ID | 20190222201111.98196-4-gitster@pobox.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | format-patch --no-clobber | expand |
On Fri, Feb 22, 2019 at 3:11 PM Junio C Hamano <gitster@pobox.com> wrote: > If you keep an output for an older iteration of the same topic in > the same directory around and use "git format-patch" to prepare a > newer iteration of the topic, those commits that happen to be at the > same position in the series that have not been retitled will get the > same filename---and the command opens them for writing without any > check. > > Existing "-o outdir" and "-v number" options are both good ways to > avoid such name collisions, and in general helps to give good ways > to compare the latest iteration with older iteration(s), but let's > see if "--no-clobber" option that forbids overwrting existing files > would also help people. s/overwrting/overwriting/ Meh. I haven't particularly been following the thread, but this commit message doesn't necessarily provide sufficient justification for further bloating git-format-patch's set of options, its documentation, and implementation, not to mention potential user-brain overload. With the possible exception of a 1-patch series, anyone who stores multiple versions of a patch series without using -o and/or -v is going to have a mess to deal with regardless of this new option. (Just trying to figure out which *.patch file belongs to which version of a patch series will be a nightmare without use of -o and/or -v.) > Signed-off-by: Junio C Hamano <gitster@pobox.com>
On Fri, Feb 22, 2019 at 12:11:11PM -0800, Junio C Hamano wrote: > If you keep an output for an older iteration of the same topic in > the same directory around and use "git format-patch" to prepare a > newer iteration of the topic, those commits that happen to be at the > same position in the series that have not been retitled will get the > same filename---and the command opens them for writing without any > check. > > Existing "-o outdir" and "-v number" options are both good ways to > avoid such name collisions, and in general helps to give good ways > to compare the latest iteration with older iteration(s), but let's > see if "--no-clobber" option that forbids overwrting existing files > would also help people. I suspect it won't help much, because remembering to use --no-clobber is just as hard as remembering to clean up the stale patches in the first place. If we were starting from scratch, I'd suggest that --no-clobber be the default[1]. But at this point I wonder if people would be annoyed (because the clobbering behavior is convenient and works _most_ of the time, as long as you don't add, remove, reorder, or retitle patches). I suppose that implies having a config option, so at least people who want it only have to remember once. > Documentation/git-format-patch.txt | 8 +++++++- > builtin/log.c | 32 ++++++++++++++++++++++++------ > t/t4014-format-patch.sh | 16 +++++++++++++++ > 3 files changed, 49 insertions(+), 7 deletions(-) The patch itself looks well done. -Peff [1] Actually, I'd suggest that --stdout be the default, which is what I always use. But then I typically feed the result into mutt anyway. Separate files is probably nicer if you're hand-editing.
Hello there, Apologies for "jumping in". I was mentioned in [PATCH 0/3] but then for a (good) reason or another, I wasn't CC-ed in the patches. I was the original "suggester" for this feature in the mailing list (https://public-inbox.org/git/CAHMHMxXxo4zXcriBJE2k3mWgwAj7KGA_AChuEmyciESGOC_7Bg@mail.gmail.com/) On 22/2/2019 10:38 μ.μ., Eric Sunshine wrote: > Meh. I haven't particularly been following the thread, but this commit > message doesn't necessarily provide sufficient justification for > further bloating git-format-patch's set of options, its documentation, > and implementation, not to mention potential user-brain overload. With > the possible exception of a 1-patch series, anyone who stores multiple > versions of a patch series without using -o and/or -v is going to have > a mess to deal with regardless of this new option. (Just trying to > figure out which *.patch file belongs to which version of a patch > series will be a nightmare without use of -o and/or -v.) On 23/2/2019 3:34 μ.μ., Jeff King wrote: > I suspect it won't help much, because remembering to use --no-clobber is > just as hard as remembering to clean up the stale patches in the first > place. > > If we were starting from scratch, I'd suggest that --no-clobber be the > default[1]. But at this point I wonder if people would be annoyed > (because the clobbering behavior is convenient and works _most_ of the > time, as long as you don't add, remove, reorder, or retitle patches). > > I suppose that implies having a config option, so at least people who > want it only have to remember once. > > The patch itself looks well done. > > -Peff On the last mail of the thread (the one on the link) I mentioned that a "set and forget" setting was my original idea / wish. "I have heard around" that [PATCH 3/3] was really meh from the reviewers' point of view, and felt that it might not cut it. I thought a benign nudge (and some disambiguation) could help tip the scales, since I am the one doing "not nice things" with git-format-patch :-)
diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt index 1af85d404f..540822b3b4 100644 --- a/Documentation/git-format-patch.txt +++ b/Documentation/git-format-patch.txt @@ -25,7 +25,7 @@ SYNOPSIS [--[no-]cover-letter] [--quiet] [--notes[=<ref>]] [--interdiff=<previous>] [--range-diff=<previous> [--creation-factor=<percent>]] - [--progress] + [--progress] [--[no-]clobber] [<common diff options>] [ <since> | <revision range> ] @@ -93,6 +93,12 @@ include::diff-options.txt[] Use <dir> to store the resulting files, instead of the current working directory. +--clobber:: +--no-clobber:: + (experimental) + Allow overwriting existing files, which is the default. To + make the command refrain from overwriting, use `--no-clobber`. + -n:: --numbered:: Name output in '[PATCH n/m]' format, even with a single patch. diff --git a/builtin/log.c b/builtin/log.c index ca86611efe..7421f1cc93 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -867,8 +867,16 @@ static int git_format_config(const char *var, const char *value, void *cb) static const char *output_directory = NULL; static int outdir_offset; +static FILE *fopen_excl(const char *filename) +{ + int fd = open(filename, O_CREAT | O_EXCL | O_WRONLY, 0666); + if (fd < 0) + return NULL; + return fdopen(fd, "w"); +} + static int open_next_file(struct commit *commit, const char *subject, - struct rev_info *rev, int quiet) + struct rev_info *rev, int quiet, int clobber) { struct strbuf filename = STRBUF_INIT; int suffix_len = strlen(rev->patch_suffix) + 1; @@ -893,7 +901,12 @@ static int open_next_file(struct commit *commit, const char *subject, if (!quiet) printf("%s\n", filename.buf + outdir_offset); - if ((rev->diffopt.file = fopen(filename.buf, "w")) == NULL) { + if (clobber) + rev->diffopt.file = fopen(filename.buf, "w"); + else + rev->diffopt.file = fopen_excl(filename.buf); + + if (!rev->diffopt.file) { error_errno(_("cannot open patch file %s"), filename.buf); strbuf_release(&filename); return -1; @@ -1030,7 +1043,8 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout, struct commit *origin, int nr, struct commit **list, const char *branch_name, - int quiet) + int quiet, + int clobber) { const char *committer; const char *body = "*** SUBJECT HERE ***\n\n*** BLURB HERE ***\n"; @@ -1049,7 +1063,8 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout, committer = git_committer_info(0); if (!use_stdout && - open_next_file(NULL, rev->numbered_files ? NULL : "cover-letter", rev, quiet)) + open_next_file(NULL, rev->numbered_files ? NULL : "cover-letter", + rev, quiet, clobber)) die(_("failed to create cover-letter file")); log_write_email_headers(rev, head, &pp.after_subject, &need_8bit_cte, 0); @@ -1509,6 +1524,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) struct strbuf buf = STRBUF_INIT; int use_patch_format = 0; int quiet = 0; + int clobber = 1; int reroll_count = -1; char *branch_name = NULL; char *base_commit = NULL; @@ -1595,6 +1611,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) OPT__QUIET(&quiet, N_("don't print the patch filenames")), OPT_BOOL(0, "progress", &show_progress, N_("show progress while generating patches")), + OPT_BOOL(0, "clobber", &clobber, + N_("allow overwriting output files")), OPT_CALLBACK(0, "interdiff", &idiff_prev, N_("rev"), N_("show changes against <rev> in cover letter or single patch"), parse_opt_object_name), @@ -1885,7 +1903,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) if (thread) gen_message_id(&rev, "cover"); make_cover_letter(&rev, use_stdout, - origin, nr, list, branch_name, quiet); + origin, nr, list, branch_name, + quiet, clobber); print_bases(&bases, rev.diffopt.file); print_signature(rev.diffopt.file); total++; @@ -1940,7 +1959,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) } if (!use_stdout && - open_next_file(rev.numbered_files ? NULL : commit, NULL, &rev, quiet)) + open_next_file(rev.numbered_files ? NULL : commit, NULL, + &rev, quiet, clobber)) die(_("failed to create output files")); shown = log_tree_commit(&rev, commit); free_commit_buffer(the_repository->parsed_objects, diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh index b6e2fdbc44..384a1fd9e7 100755 --- a/t/t4014-format-patch.sh +++ b/t/t4014-format-patch.sh @@ -595,6 +595,22 @@ test_expect_success 'failure to write cover-letter aborts gracefully' ' test_must_fail git format-patch --no-renames --cover-letter -1 ' +test_expect_success 'refrain from overwriting a patch with --no-clobber' ' + rm -f 000[01]-*.patch && + git format-patch --no-clobber --no-renames --cover-letter -1 >filelist && + # empty the files output by the command ... + for f in $(cat filelist) + do + : >"$f" || return 1 + done && + test_must_fail git format-patch --no-clobber --cover-letter --no-renames -1 && + # ... and make sure they stay empty + for f in $(cat filelist) + do + ! test -s "$f" || return 1 + done +' + test_expect_success 'cover-letter inherits diff options' ' git mv file foo && git commit -m foo &&
If you keep an output for an older iteration of the same topic in the same directory around and use "git format-patch" to prepare a newer iteration of the topic, those commits that happen to be at the same position in the series that have not been retitled will get the same filename---and the command opens them for writing without any check. Existing "-o outdir" and "-v number" options are both good ways to avoid such name collisions, and in general helps to give good ways to compare the latest iteration with older iteration(s), but let's see if "--no-clobber" option that forbids overwrting existing files would also help people. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- Documentation/git-format-patch.txt | 8 +++++++- builtin/log.c | 32 ++++++++++++++++++++++++------ t/t4014-format-patch.sh | 16 +++++++++++++++ 3 files changed, 49 insertions(+), 7 deletions(-)