| Message ID | 20201104132907.GC3030146@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/3] format-patch: refactor output selection | expand |
On Wed, Nov 4, 2020 at 8:29 AM Jeff King <peff@peff.net> wrote: > [...] > The simplest solution would be to just disallow --output with > format-patch, as nobody ever intended it to work. However, we have > accidentally documented it (because format-patch includes diff-options). > And it does work with "git log", which writes the whole output to the > specified file. It's easy enough to make that work for format-patch, > too: it's really the same as --stdout, but pointed at a specific file. > [...] > Signed-off-by: Jeff King <peff@peff.net> > --- > diff --git a/builtin/log.c b/builtin/log.c > @@ -1942,11 +1942,18 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) > + } else if (rev.diffopt.close_file) { > + /* > + * The diff code parsed --output; it has already opened the > + * file, but but we must instruct it not to close after each > + * diff. > + */ > + rev.diffopt.close_file = 0; > } else { The commit message's justification for supporting --output seems reasonable. However, my knee-jerk reaction to the implementation was that it feels overly magical and a bit too hacky. I can see the logic in it but it also leaves a bad taste when the implementation has to "undo" a side-effect of some other piece of code, which makes it feel unplanned and fragile. The question which popped into my mind immediately was "why not handle --output explicitly via builtin_format_patch_options[] along with other first-class options?". This review comment may or may not be actionable; it's just expressing surprise and a bit of nose-wrinkling I experienced. > diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh > @@ -1919,6 +1919,39 @@ test_expect_success 'format-patch -o overrides format.outputDirectory' ' > +test_expect_success 'format-patch forbids multiple outputs' ' > + rm -fr outfile outdir && > + test_must_fail \ > + git format-patch --stdout --output=outfile && > + test_must_fail \ > + git format-patch --stdout --output-directory=outdir && > + test_must_fail \ > + git format-patch --output=outfile --output-directory=outdir > +' I would have expected to see this test added in patch [1/3], and then extended by this patch with the addition of the --output case(s).
Jeff King <peff@peff.net> writes: > +test_expect_success 'format-patch --output' ' > + rm -fr outfile && > + git format-patch -3 --stdout HEAD >expect && > + git format-patch -3 --output=outfile HEAD && > + test_cmp expect outfile > +' > + > +test_expect_success 'format-patch --cover-letter --output' ' > + rm -fr outfile && > + git format-patch --cover-letter -3 --stdout HEAD >expect && > + git format-patch --cover-letter -3 --output=outfile HEAD && > + test_cmp expect outfile > +' It is pleasing to see an obvious and clear demonstration that "--output=X" is equivalent to "--stdout >X".
On Wed, Nov 04, 2020 at 12:27:30PM -0500, Eric Sunshine wrote: > > diff --git a/builtin/log.c b/builtin/log.c > > @@ -1942,11 +1942,18 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) > > + } else if (rev.diffopt.close_file) { > > + /* > > + * The diff code parsed --output; it has already opened the > > + * file, but but we must instruct it not to close after each > > + * diff. > > + */ > > + rev.diffopt.close_file = 0; > > } else { > > The commit message's justification for supporting --output seems > reasonable. However, my knee-jerk reaction to the implementation was > that it feels overly magical and a bit too hacky. I can see the logic > in it but it also leaves a bad taste when the implementation has to > "undo" a side-effect of some other piece of code, which makes it feel > unplanned and fragile. The question which popped into my mind > immediately was "why not handle --output explicitly via > builtin_format_patch_options[] along with other first-class options?". > This review comment may or may not be actionable; it's just expressing > surprise and a bit of nose-wrinkling I experienced. I agree it's a bit magical. I didn't really consider writing a new "--output" option for format-patch, because I came at it from the angle of "let's unbreak the existing diff --output option". Which isn't necessarily a defense, but just an explanation. FWIW, that unsetting of rev.diffopt.close_file is exactly how git-log does it. So while I agree it's a bit ugly, this is the intended way for --output to be used across multiple diffs, and with log_tree_commit(). I'd prefer to go this way for now, and if somebody wants to make it less ugly, they can clean up all of the callers in one go. Amusingly, 6ea57703f6 (log: prepare log/log-tree to reuse the diffopt.close_file attribute, 2016-06-22) which introduced this code foresaw the format-patch bug here. -Peff
On Wed, Nov 4, 2020 at 2:16 PM Jeff King <peff@peff.net> wrote: > On Wed, Nov 04, 2020 at 12:27:30PM -0500, Eric Sunshine wrote: > > The commit message's justification for supporting --output seems > > reasonable. However, my knee-jerk reaction to the implementation was > > that it feels overly magical and a bit too hacky. I can see the logic > > in it but it also leaves a bad taste when the implementation has to > > "undo" a side-effect of some other piece of code, which makes it feel > > unplanned and fragile. [...] > > FWIW, that unsetting of rev.diffopt.close_file is exactly how git-log > does it. So while I agree it's a bit ugly, this is the intended way for > --output to be used across multiple diffs, and with log_tree_commit(). > I'd prefer to go this way for now, and if somebody wants to make it less > ugly, they can clean up all of the callers in one go. If you intend to re-roll, it might make sense to include this explanation in the commit message since existing precedent helps sell the ugliness (bad taste), making for a less negative knee-jerk reaction.
diff --git a/builtin/log.c b/builtin/log.c index 27c6d612e4..ddb3ea7326 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -1942,11 +1942,18 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) if (rev.show_notes) load_display_notes(&rev.notes_opt); - if (use_stdout + !!output_directory > 1) + if (use_stdout + rev.diffopt.close_file + !!output_directory > 1) die(_("specify only one of --stdout, --output, and --output-directory")); if (use_stdout) { setup_pager(); + } else if (rev.diffopt.close_file) { + /* + * The diff code parsed --output; it has already opened the + * file, but but we must instruct it not to close after each + * diff. + */ + rev.diffopt.close_file = 0; } else { int saved; diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh index 294e76c860..64574c51c1 100755 --- a/t/t4014-format-patch.sh +++ b/t/t4014-format-patch.sh @@ -1919,6 +1919,39 @@ test_expect_success 'format-patch -o overrides format.outputDirectory' ' test_path_is_dir patchset ' +test_expect_success 'format-patch forbids multiple outputs' ' + rm -fr outfile outdir && + test_must_fail \ + git format-patch --stdout --output=outfile && + test_must_fail \ + git format-patch --stdout --output-directory=outdir && + test_must_fail \ + git format-patch --output=outfile --output-directory=outdir +' + +test_expect_success 'configured outdir does not conflict with output options' ' + rm -fr outfile outdir && + test_config format.outputDirectory outdir && + git format-patch --stdout && + test_path_is_missing outdir && + git format-patch --output=outfile && + test_path_is_missing outdir +' + +test_expect_success 'format-patch --output' ' + rm -fr outfile && + git format-patch -3 --stdout HEAD >expect && + git format-patch -3 --output=outfile HEAD && + test_cmp expect outfile +' + +test_expect_success 'format-patch --cover-letter --output' ' + rm -fr outfile && + git format-patch --cover-letter -3 --stdout HEAD >expect && + git format-patch --cover-letter -3 --output=outfile HEAD && + test_cmp expect outfile +' + test_expect_success 'format-patch --base' ' git checkout patchid &&
We've never intended to support diff's --output option in format-patch. And until baa4adc66a (parse-options: disable option abbreviation with PARSE_OPT_KEEP_UNKNOWN, 2019-01-27), it was impossible to trigger. We first parse the format-patch options before handing the remainder off to setup_revisions(). Before that commit, we'd accept "--output=foo" as an abbreviation for "--output-directory=foo". But afterwards, we don't check abbreviations, and --output gets passed to the diff code. This results in nonsense behavior and bugs. The diff code will have opened a filehandle at rev.diffopt.file, but we'll overwrite that with our own handles that we open for each individual patch file. So the --output file will always just be empty. But worse, the diff code also sets rev.diffopt.close_file, so log_tree_commit() will close the filehandle itself. And then the main loop in cmd_format_patch() will try to close it again, resulting in a double-free. The simplest solution would be to just disallow --output with format-patch, as nobody ever intended it to work. However, we have accidentally documented it (because format-patch includes diff-options). And it does work with "git log", which writes the whole output to the specified file. It's easy enough to make that work for format-patch, too: it's really the same as --stdout, but pointed at a specific file. We can detect the use of the --output option by the "close_file" flag (note that we can't use rev.diffopt.file, since the diff setup will otherwise set it to stdout). So we just need to unset that flag, but don't have to do anything else. Our situation is otherwise exactly like --stdout (note that we don't fclose() the file, but nor does the stdout case; exiting the program takes care of that for us). Reported-by: Johannes Postler <johannes.postler@txture.io> Signed-off-by: Jeff King <peff@peff.net> --- builtin/log.c | 9 ++++++++- t/t4014-format-patch.sh | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 1 deletion(-)