Re: [PATCH] format-patch: do not let its diff-options affect --range-diff (was Re: [PATCH 2/2] format-patch: allow for independent diff & range-diff options)
On Fri, 30 Nov 2018 at 10:32, Eric Sunshine wrote: > > On Thu, Nov 29, 2018 at 11:27 PM Junio C Hamano wrote: > > Junio C Hamano writes: > > So how about doing this on top of 'master' instead? As this leaks > > *no* information wrt how range-diff machinery should behave from the > > format-patch side by not passing any diffopt, as long as the new > > code I added to show_range_diff() comes up with a reasonable default > > diffopts (for which I really would appreciate extra sets of eyes to > > make sure), this change by definition cannot be wrong (famous last > > words). > Another benefit of calling show_range_diff() directly is that when > "git format-patch --stdout --range-diff=..." is sent to the terminal, > the range-diff gets colored output for free. I'm pleased to see that > that still works after this change. (If the patch below makes any sense to you and you know more about this diff/color thing, the fourth bullet in the log message below might interest you.) > > diff --git a/range-diff.h b/range-diff.h > > @@ -5,6 +5,11 @@ > > +/* > > + * Compare series of commmits in RANGE1 and RANGE2, and emit to the > > + * standard output. NULL can be passed to DIFFOPT to use the built-in > > + * default. > > + */ > > It is more correct to say that the range-diff is emitted to > diffopt->file (which may be stdout). This seems to be an important remark. There's a pretty bad regression here since when `diffopt` is NULL, we've lost our original, intended `diffopt->file` and the range-diff ends up on stdout. Here's my whitespace-damaged WIP. I would be able to pick this up again in about 6h, but anyone is more than welcome to pick this up and run with it in the meantime. This is not a corner of the code that I'm particularly familiar with... -->8-- Subject: [PATCH] range-diff: always pass at least minimal diff options MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Commit d8981c3f88 ("format-patch: do not let its diff-options affect --range-diff", 2018-11-30) taught `show_range_diff()` to accept a NULL-pointer as an indication that it should use its own "reasonable default". That fixed a regression from a5170794 ("Merge branch 'ab/range-diff-no-patch'", 2018-11-18), but unfortunately it introduced a regression of its own. In particular, it means we forget the `file` member of the diff options, so rather than placing a range-diff in the cover-letter, we write it to stdout. In order to fix this, rewrite the two callers adjusted by d8981c3f88 to create a "dummy" set of diff options where they only fill in which file to use. A couple of remarks about this commit: * No tests. The change in builtin/log.c has been tested manually, the one in log-tree.c not at all, other than by running existing tests. * I have not convinced myself 100% that there aren't other things that are just as important as `file` to pass down. * `show_range_diff()` can still take NULL, although that is now dead code. If something like this here commit is deemed the proper fix for this, that code path could also go, either as part of this commit, or separately, once we've cut 2.20. * The range-diff is written colored regardless of destination, i.e., when written to a file, it contains garbage. Signed-off-by: Martin Ågren --- builtin/log.c | 12 +++- log-tree.c| 12 +++- 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/builtin/log.c b/builtin/log.c index 5ac18e2848..0609e41ae5 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -1094,9 +1094,19 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout, } if (rev->rdiff1) { +/** + * (At least for now) we only want to pass down + * the file handle where we want the range-diff + * to appear. Avoid any other diff options until + * we know how we want to handle them. + */ +struct diff_options opts; +diff_setup(); +opts.file = rev->diffopt.file; +diff_setup_done(); fprintf_ln(rev->diffopt.file, "%s", rev->rdiff_title); show_range_diff(rev->rdiff1, rev->rdiff2, -rev->creation_factor, 1, NULL); +rev->creation_factor, 1, ); } } diff --git a/log-tree.c b/log-tree.c index b243779a0b..bc355a4e91 100644 --- a/log-tree.c +++ b/log-tree.c @@ -755,14 +755,24 @@ void show_log(struct rev_info *opt) if (cmit_fmt_is_mail(ctx.fmt) && opt->rdiff1) { struct diff_queue_struct dq; +struct diff_options opts; memcpy(, _queued_diff, sizeof(diff_queued_diff)); DIFF_QUEUE_CLEAR(_queued_diff); next_commentary_block(opt, NULL); fprintf_ln(opt->diffopt.file, "%s", opt->rdiff_title); +/** + * (At least for now) we only want to pass down + * the file handle where we want the range-diff + * to appear. Avoid any other diff options until + * we know how
Re: [PATCH] format-patch: do not let its diff-options affect --range-diff (was Re: [PATCH 2/2] format-patch: allow for independent diff & range-diff options)
Hi Junio, On Fri, 30 Nov 2018, Junio C Hamano wrote: > Junio C Hamano writes: > > >> I had to delay -rc2 to see these last minute tweaks come to some > >> reasonable place to stop at, and I do not think we want to delay the > >> final any longer or destablizing it further by piling last minute > >> undercooked changes on top. > > > > So how about doing this on top of 'master' instead? As this leaks > > *no* information wrt how range-diff machinery should behave from the > > format-patch side by not passing any diffopt, as long as the new > > code I added to show_range_diff() comes up with a reasonable default > > diffopts (for which I really would appreciate extra sets of eyes to > > make sure), this change by definition cannot be wrong (famous last > > words). > > As listed in today's "What's cooking" report, I've merged this to > 'next' in today's pushout and planning to have it in the -rc2. I am > not married to this exact implementation, and I'd welcome to have an > even simpler and less disruptive solution if exists, but I am hoping > that this is a good-enough interim measure for the upcoming release, > until we decide what to do with the customizability of range-diff > driven by format-patch. > > In addition to this, I am planning the "rebase --stat" and "reflog > that does not say 'rebase -i' but 'rebase'" fixes merged to 'master' > before cutting -rc2. Thank you for integrating them. That way, we have an -rc2 with no issues in the built-in rebase/rebase -i that we know of. Ciao, Dscho
Re: [PATCH 2/2] format-patch: allow for independent diff & range-diff options
On Thu, Nov 29, 2018 at 11:03 AM Ævar Arnfjörð Bjarmason wrote: > I mean not just nasty in terms of implementation, yeah we could do it, > but also a nasty UX for things like --word-diff-regex. I.e. instead of: > > --range-diff-word-diff-regex='[0-9"]' > > You need: > > --range-diff-opts="--word-diff-regex='[0-9\"]'" > > Now admittedly that in itself isn't very painful *in this case*, but in > terms of precedent I really dislike that option, i.e. git having some > mode where I need to work to escape input to pass to another command. > > Not saying that this --range-diff-* thing is what we should go for, but > surely we can find some way to do deal with this that doesn't involve > the user needing to escape stuff like this. I should mention that it was never the intention that git-format-patch's --range-diff option would allow passing _all_ possible options to git-range-diff, but only those most likely to be tweaked by the user (such as --creation-factor). It was understood from the start (and stated by me either in the cover letter to that series or in discussion) that the user always has the escape hatch of running git-range-diff manually and copy/pasting its output into the cover letter. So, I'm not convinced that we need this sort of flexibility in git-format-patch's --range-diff option, but we certainly can add convenience options (such as I did with --creation-factor) as people complain that their favorite option is missing. For a UI, I think we already have sufficient precedent for "--range-diff-opts=nopatch,creation-factor:60" as a possibility.
Re: [PATCH] format-patch: do not let its diff-options affect --range-diff (was Re: [PATCH 2/2] format-patch: allow for independent diff & range-diff options)
On Thu, Nov 29, 2018 at 11:27 PM Junio C Hamano wrote: > Junio C Hamano writes: > > In any case, I tend to agree with the conclusion in the downthread > > by Dscho that we should just clearly mark that invocations of the > > "format-patch --range-diff" command with additional diff options is > > an experimental feature that may not do anything sensible in the > > upcoming release, and declare that the UI to pass diff options to > > affect only the range-diff part may later be invented. IOW, I am > > coming a bit stronger than Dscho's suggestion in that we should not > > even pretend that we aimed to make the options used for range-diff > > customizable when driven from format-patch in the upcoming release, > > or aimed to make --range-diff option compatible with other diff > > options given to the format-patch command. I agree that this way forward makes sense. It's clear that I overlooked how there could be unexpected interactions from passing git-format-patch's own diff_options to show_range_diff(), so taking time to think it through without the pressure of a looming release is preferable to rushing out some "fixes". > So how about doing this on top of 'master' instead? As this leaks > *no* information wrt how range-diff machinery should behave from the > format-patch side by not passing any diffopt, as long as the new > code I added to show_range_diff() comes up with a reasonable default > diffopts (for which I really would appreciate extra sets of eyes to > make sure), this change by definition cannot be wrong (famous last > words). I, myself, was going to suggest this approach of leaking none of the git-format-patch's options into range_diff(), so I think it is a good one. Later, we can selectively pass certain _sensible_ options into show_range_diff() once we figure out the correct UI (for instance, --range-diff-opts=nopatch,creation-factor:60). A couple comments on the patch itself... > diff --git a/range-diff.c b/range-diff.c > @@ -460,7 +460,11 @@ int show_range_diff(const char *range1, const char > *range2, > - memcpy(, diffopt, sizeof(opts)); > + if (diffopt) > + memcpy(, diffopt, sizeof(opts)); > + else > + repo_diff_setup(the_repository, ); The first attempt at adding --range-diff to git-format-patch invoked the git-range-diff command, so no diff_options were passed at all. After Dscho libified the range-diff machinery in one of his major re-rolls, I took advantage of that to avoid the subprocess invocation. Another benefit of calling show_range_diff() directly is that when "git format-patch --stdout --range-diff=..." is sent to the terminal, the range-diff gets colored output for free. I'm pleased to see that that still works after this change. > diff --git a/range-diff.h b/range-diff.h > @@ -5,6 +5,11 @@ > +/* > + * Compare series of commmits in RANGE1 and RANGE2, and emit to the > + * standard output. NULL can be passed to DIFFOPT to use the built-in > + * default. > + */ It is more correct to say that the range-diff is emitted to diffopt->file (which may be stdout). Thanks for working on this.
Re: [PATCH] format-patch: do not let its diff-options affect --range-diff (was Re: [PATCH 2/2] format-patch: allow for independent diff & range-diff options)
On Fri, Nov 30 2018, Junio C Hamano wrote: > Junio C Hamano writes: > >>> I had to delay -rc2 to see these last minute tweaks come to some >>> reasonable place to stop at, and I do not think we want to delay the >>> final any longer or destablizing it further by piling last minute >>> undercooked changes on top. >> >> So how about doing this on top of 'master' instead? As this leaks >> *no* information wrt how range-diff machinery should behave from the >> format-patch side by not passing any diffopt, as long as the new >> code I added to show_range_diff() comes up with a reasonable default >> diffopts (for which I really would appreciate extra sets of eyes to >> make sure), this change by definition cannot be wrong (famous last >> words). > > As listed in today's "What's cooking" report, I've merged this to > 'next' in today's pushout and planning to have it in the -rc2. I am > not married to this exact implementation, and I'd welcome to have an > even simpler and less disruptive solution if exists, but I am hoping > that this is a good-enough interim measure for the upcoming release, > until we decide what to do with the customizability of range-diff > driven by format-patch. > > In addition to this, I am planning the "rebase --stat" and "reflog > that does not say 'rebase -i' but 'rebase'" fixes merged to 'master' > before cutting -rc2. Thanks a lot, yeah having this wait looks good to me.
Re: [PATCH] format-patch: do not let its diff-options affect --range-diff (was Re: [PATCH 2/2] format-patch: allow for independent diff & range-diff options)
Junio C Hamano writes: >> I had to delay -rc2 to see these last minute tweaks come to some >> reasonable place to stop at, and I do not think we want to delay the >> final any longer or destablizing it further by piling last minute >> undercooked changes on top. > > So how about doing this on top of 'master' instead? As this leaks > *no* information wrt how range-diff machinery should behave from the > format-patch side by not passing any diffopt, as long as the new > code I added to show_range_diff() comes up with a reasonable default > diffopts (for which I really would appreciate extra sets of eyes to > make sure), this change by definition cannot be wrong (famous last > words). As listed in today's "What's cooking" report, I've merged this to 'next' in today's pushout and planning to have it in the -rc2. I am not married to this exact implementation, and I'd welcome to have an even simpler and less disruptive solution if exists, but I am hoping that this is a good-enough interim measure for the upcoming release, until we decide what to do with the customizability of range-diff driven by format-patch. In addition to this, I am planning the "rebase --stat" and "reflog that does not say 'rebase -i' but 'rebase'" fixes merged to 'master' before cutting -rc2.
[PATCH] format-patch: do not let its diff-options affect --range-diff (was Re: [PATCH 2/2] format-patch: allow for independent diff & range-diff options)
Junio C Hamano writes: > In any case, I tend to agree with the conclusion in the downthread > by Dscho that we should just clearly mark that invocations of the > "format-patch --range-diff" command with additional diff options is > an experimental feature that may not do anything sensible in the > upcoming release, and declare that the UI to pass diff options to > affect only the range-diff part may later be invented. IOW, I am > coming a bit stronger than Dscho's suggestion in that we should not > even pretend that we aimed to make the options used for range-diff > customizable when driven from format-patch in the upcoming release, > or aimed to make --range-diff option compatible with other diff > options given to the format-patch command. > > I had to delay -rc2 to see these last minute tweaks come to some > reasonable place to stop at, and I do not think we want to delay the > final any longer or destablizing it further by piling last minute > undercooked changes on top. So how about doing this on top of 'master' instead? As this leaks *no* information wrt how range-diff machinery should behave from the format-patch side by not passing any diffopt, as long as the new code I added to show_range_diff() comes up with a reasonable default diffopts (for which I really would appreciate extra sets of eyes to make sure), this change by definition cannot be wrong (famous last words). -- >8 -- Subject: format-patch: do not let its diff-options affect --range-diff Stop leaking how the primary output of format-patch is customized to the range-diff machinery and instead let the latter use its own "reasonable default", in order to correct the breakage introduced by a5170794 ("Merge branch 'ab/range-diff-no-patch'", 2018-11-18) on the 'master' front. "git format-patch --range-diff..." without any weird diff option started to include the "range-diff --stat" output, which is rather useless right now, that made the whole thing unusable and this is probably the least disruptive way to whip the codebase into a shippable shape. We may want to later make the range-diff driven by format-patch more configurable, but that would have to wait until we have a good design. Signed-off-by: Junio C Hamano --- Documentation/git-format-patch.txt | 5 + builtin/log.c | 2 +- log-tree.c | 2 +- range-diff.c | 6 +- range-diff.h | 5 + 5 files changed, 17 insertions(+), 3 deletions(-) diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt index aba4c5febe..27304428a1 100644 --- a/Documentation/git-format-patch.txt +++ b/Documentation/git-format-patch.txt @@ -250,6 +250,11 @@ feeding the result to `git send-email`. feature/v2`), or a revision range if the two versions of the series are disjoint (for example `git format-patch --cover-letter --range-diff=feature/v1~3..feature/v1 -3 feature/v2`). ++ +Note that diff options passed to the command affect how the primary +product of `format-patch` is generated, and they are not passed to +the underlying `range-diff` machinery used to generate the cover-letter +material (this may change in the future). --creation-factor=:: Used with `--range-diff`, tweak the heuristic which matches up commits diff --git a/builtin/log.c b/builtin/log.c index 0fe6f9ba1e..5ac18e2848 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -1096,7 +1096,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout, if (rev->rdiff1) { fprintf_ln(rev->diffopt.file, "%s", rev->rdiff_title); show_range_diff(rev->rdiff1, rev->rdiff2, - rev->creation_factor, 1, >diffopt); + rev->creation_factor, 1, NULL); } } diff --git a/log-tree.c b/log-tree.c index 7a83e99250..b243779a0b 100644 --- a/log-tree.c +++ b/log-tree.c @@ -762,7 +762,7 @@ void show_log(struct rev_info *opt) next_commentary_block(opt, NULL); fprintf_ln(opt->diffopt.file, "%s", opt->rdiff_title); show_range_diff(opt->rdiff1, opt->rdiff2, - opt->creation_factor, 1, >diffopt); + opt->creation_factor, 1, NULL); memcpy(_queued_diff, , sizeof(diff_queued_diff)); } diff --git a/range-diff.c b/range-diff.c index 767af8c5bb..8e52a85c19 100644 --- a/range-diff.c +++ b/range-diff.c @@ -460,7 +460,11 @@ int show_range_diff(const char *range1, const char *range2, struct diff_options opts; struct strbuf indent = STRBUF_INIT; - memcpy(, diffopt, sizeof(opts)); + if (diffopt) + memcpy(, diffopt, sizeof(opts)); + else + repo_diff_setup(the_repository, ); + if (!opts.output_format) opts.output_format =
Re: [PATCH 2/2] format-patch: allow for independent diff & range-diff options
Ævar Arnfjörð Bjarmason writes: >> What prevents you from using `sq_dequote_to_argv()`? > > I mean not just nasty in terms of implementation, yeah we could do it, > but also a nasty UX for things like --word-diff-regex. I.e. instead of: > > --range-diff-word-diff-regex='[0-9"]' > > You need: > > --range-diff-opts="--word-diff-regex='[0-9\"]'" > > Now admittedly that in itself isn't very painful *in this case*, but in > terms of precedent I really dislike that option, i.e. git having some > mode where I need to work to escape input to pass to another command. In addition, sq_dequote are meant to be used on quoted string we internally produce; I do not think we want to promise that it is safe to use on a random string that comes from end users. In any case, I tend to agree with the conclusion in the downthread by Dscho that we should just clearly mark that invocations of the "format-patch --range-diff" command with additional diff options is an experimental feature that may not do anything sensible in the upcoming release, and declare that the UI to pass diff options to affect only the range-diff part may later be invented. IOW, I am coming a bit stronger than Dscho's suggestion in that we should not even pretend that we aimed to make the options used for range-diff customizable when driven from format-patch in the upcoming release, or aimed to make --range-diff option compatible with other diff options given to the format-patch command. I had to delay -rc2 to see these last minute tweaks come to some reasonable place to stop at, and I do not think we want to delay the final any longer or destablizing it further by piling last minute undercooked changes on top.
Re: [PATCH 2/2] format-patch: allow for independent diff & range-diff options
Hi Ævar, On Thu, 29 Nov 2018, Ævar Arnfjörð Bjarmason wrote: > On Thu, Nov 29 2018, Johannes Schindelin wrote: > > > On Thu, 29 Nov 2018, Ævar Arnfjörð Bjarmason wrote: > > > >> On Thu, Nov 29 2018, Johannes Schindelin wrote: > >> > >> > On Thu, 29 Nov 2018, Ævar Arnfjörð Bjarmason wrote: > >> > > >> >> On Thu, Nov 29 2018, Johannes Schindelin wrote: > >> >> > >> >> > On Wed, 28 Nov 2018, Ævar Arnfjörð Bjarmason wrote: > >> >> > > >> >> >> Change the semantics of the "--range-diff" option so that the regular > >> >> >> diff options can be provided separately for the range-diff and the > >> >> >> patch. This allows for supplying e.g. --range-diff-U0 and -U1 to > >> >> >> "format-patch" to provide different context for the range-diff and > >> >> >> the > >> >> >> patch. This wasn't possible before. > >> >> > > >> >> > I really, really dislike the `--range-diff-`. We have > >> >> > precedent for passing optional arguments that are passed to some other > >> >> > command, so a much more logical and consistent convention would be to > >> >> > use > >> >> > `--range-diff[=..]`, allowing all of the diff options > >> >> > that > >> >> > you might want to pass to the outer diff in one go rather than having > >> >> > a > >> >> > lengthy string of `--range-diff-this` and `--range-diff-that` options. > >> >> > >> >> Where do we pass those sorts of arguments? > >> >> > >> >> Reasons I did it this way: > >> >> > >> >> a) Passing it as one option will require the user to double-quote those > >> >> options that take quoted arguments (e.g. --word-diff-regex), which I > >> >> thought sucked more than the prefix. On the implementation side we > >> >> couldn't leave the parsing of the command-line to the shell anymore. > >> >> > >> >> b) I think people will want to tweak this very rarely, much more rarely > >> >> than e.g. -U10 in format-patch itself, so having something long-ish > >> >> doesn't sound bad. > >> > > >> > Hmm. I still don't like it. It sets a precedent, and we simply do not do > >> > it that way in other circumstances (most obvious would be the -X merge > >> > options). The more divergent user interfaces for the same sort of thing > >> > are, the more brain cycles you force users to spend on navigating said > >> > interfaces. > >> > >> Yeah it sucks, I just think it sucks less than the alternative :) > >> I.e. I'm not picky about --range-diff-* prefix the name, but I think > >> doing our own shell parsing would be nasty. > > > > What prevents you from using `sq_dequote_to_argv()`? > > I mean not just nasty in terms of implementation, yeah we could do it, > but also a nasty UX for things like --word-diff-regex. I.e. instead of: > > --range-diff-word-diff-regex='[0-9"]' > > You need: > > --range-diff-opts="--word-diff-regex='[0-9\"]'" Really? I think that would not work. It would pass the single quotes as part of the regex to the diff machinery. Or maybe not. But the extra quotes do not strike me as necessary, as there is no shell script involved (thank deity!) after `git range-diff` parsed the options. > Now admittedly that in itself isn't very painful *in this case*, but in > terms of precedent I really dislike that option, i.e. git having some > mode where I need to work to escape input to pass to another command. > > Not saying that this --range-diff-* thing is what we should go for, but > surely we can find some way to do deal with this that doesn't involve > the user needing to escape stuff like this. > > It also has other downstream effects in the UI, e.g. it's presumably > easy to teach the bash completion that a --foo=XYZ option is also called > --some-prefix--foo=XYZ and to enable completion for that, less so for > making it smart enough to complete "--some-prefix-opts="--foo=". These are all good points, and need proper discussion. Sadly, all that time needed for a proper discussion is not left before v2.20.0 is supposed to come out. Quite honestly, I think what we will have to do is to describe in the documentation of `format-patch`'s `--range-diff` option that the exact user interface how to pass diff options down to `range-diff` is in flux and not final. That way, we can give your design the proper treatment, and work together on making a user interface we all can be happy with. Ciao, Dscho
Re: [PATCH 2/2] format-patch: allow for independent diff & range-diff options
On Thu, Nov 29 2018, Johannes Schindelin wrote: > Hi Ævar, > > On Thu, 29 Nov 2018, Ævar Arnfjörð Bjarmason wrote: > >> On Thu, Nov 29 2018, Johannes Schindelin wrote: >> >> > On Thu, 29 Nov 2018, Ævar Arnfjörð Bjarmason wrote: >> > >> >> On Thu, Nov 29 2018, Johannes Schindelin wrote: >> >> >> >> > On Wed, 28 Nov 2018, Ævar Arnfjörð Bjarmason wrote: >> >> > >> >> >> Change the semantics of the "--range-diff" option so that the regular >> >> >> diff options can be provided separately for the range-diff and the >> >> >> patch. This allows for supplying e.g. --range-diff-U0 and -U1 to >> >> >> "format-patch" to provide different context for the range-diff and the >> >> >> patch. This wasn't possible before. >> >> > >> >> > I really, really dislike the `--range-diff-`. We have >> >> > precedent for passing optional arguments that are passed to some other >> >> > command, so a much more logical and consistent convention would be to >> >> > use >> >> > `--range-diff[=..]`, allowing all of the diff options that >> >> > you might want to pass to the outer diff in one go rather than having a >> >> > lengthy string of `--range-diff-this` and `--range-diff-that` options. >> >> >> >> Where do we pass those sorts of arguments? >> >> >> >> Reasons I did it this way: >> >> >> >> a) Passing it as one option will require the user to double-quote those >> >> options that take quoted arguments (e.g. --word-diff-regex), which I >> >> thought sucked more than the prefix. On the implementation side we >> >> couldn't leave the parsing of the command-line to the shell anymore. >> >> >> >> b) I think people will want to tweak this very rarely, much more rarely >> >> than e.g. -U10 in format-patch itself, so having something long-ish >> >> doesn't sound bad. >> > >> > Hmm. I still don't like it. It sets a precedent, and we simply do not do >> > it that way in other circumstances (most obvious would be the -X merge >> > options). The more divergent user interfaces for the same sort of thing >> > are, the more brain cycles you force users to spend on navigating said >> > interfaces. >> >> Yeah it sucks, I just think it sucks less than the alternative :) >> I.e. I'm not picky about --range-diff-* prefix the name, but I think >> doing our own shell parsing would be nasty. > > What prevents you from using `sq_dequote_to_argv()`? I mean not just nasty in terms of implementation, yeah we could do it, but also a nasty UX for things like --word-diff-regex. I.e. instead of: --range-diff-word-diff-regex='[0-9"]' You need: --range-diff-opts="--word-diff-regex='[0-9\"]'" Now admittedly that in itself isn't very painful *in this case*, but in terms of precedent I really dislike that option, i.e. git having some mode where I need to work to escape input to pass to another command. Not saying that this --range-diff-* thing is what we should go for, but surely we can find some way to do deal with this that doesn't involve the user needing to escape stuff like this. It also has other downstream effects in the UI, e.g. it's presumably easy to teach the bash completion that a --foo=XYZ option is also called --some-prefix--foo=XYZ and to enable completion for that, less so for making it smart enough to complete "--some-prefix-opts="--foo=". >> >> > I only had time to skim the patch, and I have to wonder why you pass >> >> > around full-blown `rev_info` structs for range diff (and with that >> >> > really >> >> > awful name `rd_rev`) rather than just the `diff_options` that you >> >> > *actually* care about? >> >> >> >> Because setup_revisions() which does all the command-line parsing needs >> >> a rev_info, so even if we only need the diffopt in the end we need to >> >> initiate the whole thing. >> >> >> >> Suggestions for a better varibale name most welcome. >> > >> > `range_diff_revs` >> > >> > And you do not need to pass around the whole thing. You can easily pass >> > `_diff_revs.diffopt`. >> > >> > Don't pass around what you do not need to pass around. >> >> Ah, you mean internally in log.c, yes that makes sense. I thought you >> meant just pass diffopt to setup_revisions() (which needs the containing >> struct). Willdo. > > Thanks, > Dscho > >> >> > Ciao, >> > Dscho >> > >> >> >> >> > Ciao, >> >> > Dscho >> >> > >> >> >> >> >> >> Ever since the "--range-diff" option was added in >> >> >> 31e2617a5f ("format-patch: add --range-diff option to embed diff in >> >> >> cover letter", 2018-07-22) the "rev->diffopt" we pass down to the diff >> >> >> machinery has been the one we get from "format-patch"'s own >> >> >> setup_revisions(). >> >> >> >> >> >> This sort of thing is unique among the log-like commands in >> >> >> builtin/log.c, no command than format-patch will embed the output of >> >> >> another log-like command. Since the "rev->diffopt" is reused we need >> >> >> to munge it before we pass it to show_range_diff(). See >> >> >> 43dafc4172 ("format-patch: don't include --stat with --range-diff >> >>
Re: [PATCH 2/2] format-patch: allow for independent diff & range-diff options
Hi Ævar, On Thu, 29 Nov 2018, Ævar Arnfjörð Bjarmason wrote: > On Thu, Nov 29 2018, Johannes Schindelin wrote: > > > On Thu, 29 Nov 2018, Ævar Arnfjörð Bjarmason wrote: > > > >> On Thu, Nov 29 2018, Johannes Schindelin wrote: > >> > >> > On Wed, 28 Nov 2018, Ævar Arnfjörð Bjarmason wrote: > >> > > >> >> Change the semantics of the "--range-diff" option so that the regular > >> >> diff options can be provided separately for the range-diff and the > >> >> patch. This allows for supplying e.g. --range-diff-U0 and -U1 to > >> >> "format-patch" to provide different context for the range-diff and the > >> >> patch. This wasn't possible before. > >> > > >> > I really, really dislike the `--range-diff-`. We have > >> > precedent for passing optional arguments that are passed to some other > >> > command, so a much more logical and consistent convention would be to use > >> > `--range-diff[=..]`, allowing all of the diff options that > >> > you might want to pass to the outer diff in one go rather than having a > >> > lengthy string of `--range-diff-this` and `--range-diff-that` options. > >> > >> Where do we pass those sorts of arguments? > >> > >> Reasons I did it this way: > >> > >> a) Passing it as one option will require the user to double-quote those > >> options that take quoted arguments (e.g. --word-diff-regex), which I > >> thought sucked more than the prefix. On the implementation side we > >> couldn't leave the parsing of the command-line to the shell anymore. > >> > >> b) I think people will want to tweak this very rarely, much more rarely > >> than e.g. -U10 in format-patch itself, so having something long-ish > >> doesn't sound bad. > > > > Hmm. I still don't like it. It sets a precedent, and we simply do not do > > it that way in other circumstances (most obvious would be the -X merge > > options). The more divergent user interfaces for the same sort of thing > > are, the more brain cycles you force users to spend on navigating said > > interfaces. > > Yeah it sucks, I just think it sucks less than the alternative :) > I.e. I'm not picky about --range-diff-* prefix the name, but I think > doing our own shell parsing would be nasty. What prevents you from using `sq_dequote_to_argv()`? > >> > I only had time to skim the patch, and I have to wonder why you pass > >> > around full-blown `rev_info` structs for range diff (and with that really > >> > awful name `rd_rev`) rather than just the `diff_options` that you > >> > *actually* care about? > >> > >> Because setup_revisions() which does all the command-line parsing needs > >> a rev_info, so even if we only need the diffopt in the end we need to > >> initiate the whole thing. > >> > >> Suggestions for a better varibale name most welcome. > > > > `range_diff_revs` > > > > And you do not need to pass around the whole thing. You can easily pass > > `_diff_revs.diffopt`. > > > > Don't pass around what you do not need to pass around. > > Ah, you mean internally in log.c, yes that makes sense. I thought you > meant just pass diffopt to setup_revisions() (which needs the containing > struct). Willdo. Thanks, Dscho > > > Ciao, > > Dscho > > > >> > >> > Ciao, > >> > Dscho > >> > > >> >> > >> >> Ever since the "--range-diff" option was added in > >> >> 31e2617a5f ("format-patch: add --range-diff option to embed diff in > >> >> cover letter", 2018-07-22) the "rev->diffopt" we pass down to the diff > >> >> machinery has been the one we get from "format-patch"'s own > >> >> setup_revisions(). > >> >> > >> >> This sort of thing is unique among the log-like commands in > >> >> builtin/log.c, no command than format-patch will embed the output of > >> >> another log-like command. Since the "rev->diffopt" is reused we need > >> >> to munge it before we pass it to show_range_diff(). See > >> >> 43dafc4172 ("format-patch: don't include --stat with --range-diff > >> >> output", 2018-11-22) for a related regression fix which is being > >> >> mostly reverted here. > >> >> > >> >> Implementation notes: 1) We're not bothering with the full teardown > >> >> around die() and will leak memory, but it's too much boilerplate to do > >> >> all the frees with/without the die() and not worth it. 2) We call > >> >> repo_init_revisions() for "rd_rev" even though we could get away with > >> >> a shallow copy like the code we're replacing (and which > >> >> show_range_diff() itself does). This is to make this code more easily > >> >> understood. > >> >> > >> >> Signed-off-by: Ævar Arnfjörð Bjarmason > >> >> --- > >> >> Documentation/git-format-patch.txt | 10 ++- > >> >> builtin/log.c | 42 +++--- > >> >> t/t3206-range-diff.sh | 41 + > >> >> 3 files changed, 82 insertions(+), 11 deletions(-) > >> >> > >> >> diff --git a/Documentation/git-format-patch.txt > >> >> b/Documentation/git-format-patch.txt > >> >> index aba4c5febe..6c048f415f 100644 > >> >> ---
Re: [PATCH 2/2] format-patch: allow for independent diff & range-diff options
On Thu, Nov 29 2018, Johannes Schindelin wrote: > Hi Ævar, > > On Thu, 29 Nov 2018, Ævar Arnfjörð Bjarmason wrote: > >> On Thu, Nov 29 2018, Johannes Schindelin wrote: >> >> > On Wed, 28 Nov 2018, Ævar Arnfjörð Bjarmason wrote: >> > >> >> Change the semantics of the "--range-diff" option so that the regular >> >> diff options can be provided separately for the range-diff and the >> >> patch. This allows for supplying e.g. --range-diff-U0 and -U1 to >> >> "format-patch" to provide different context for the range-diff and the >> >> patch. This wasn't possible before. >> > >> > I really, really dislike the `--range-diff-`. We have >> > precedent for passing optional arguments that are passed to some other >> > command, so a much more logical and consistent convention would be to use >> > `--range-diff[=..]`, allowing all of the diff options that >> > you might want to pass to the outer diff in one go rather than having a >> > lengthy string of `--range-diff-this` and `--range-diff-that` options. >> >> Where do we pass those sorts of arguments? >> >> Reasons I did it this way: >> >> a) Passing it as one option will require the user to double-quote those >> options that take quoted arguments (e.g. --word-diff-regex), which I >> thought sucked more than the prefix. On the implementation side we >> couldn't leave the parsing of the command-line to the shell anymore. >> >> b) I think people will want to tweak this very rarely, much more rarely >> than e.g. -U10 in format-patch itself, so having something long-ish >> doesn't sound bad. > > Hmm. I still don't like it. It sets a precedent, and we simply do not do > it that way in other circumstances (most obvious would be the -X merge > options). The more divergent user interfaces for the same sort of thing > are, the more brain cycles you force users to spend on navigating said > interfaces. Yeah it sucks, I just think it sucks less than the alternative :) I.e. I'm not picky about --range-diff-* prefix the name, but I think doing our own shell parsing would be nasty. >> > I only had time to skim the patch, and I have to wonder why you pass >> > around full-blown `rev_info` structs for range diff (and with that really >> > awful name `rd_rev`) rather than just the `diff_options` that you >> > *actually* care about? >> >> Because setup_revisions() which does all the command-line parsing needs >> a rev_info, so even if we only need the diffopt in the end we need to >> initiate the whole thing. >> >> Suggestions for a better varibale name most welcome. > > `range_diff_revs` > > And you do not need to pass around the whole thing. You can easily pass > `_diff_revs.diffopt`. > > Don't pass around what you do not need to pass around. Ah, you mean internally in log.c, yes that makes sense. I thought you meant just pass diffopt to setup_revisions() (which needs the containing struct). Willdo. > Ciao, > Dscho > >> >> > Ciao, >> > Dscho >> > >> >> >> >> Ever since the "--range-diff" option was added in >> >> 31e2617a5f ("format-patch: add --range-diff option to embed diff in >> >> cover letter", 2018-07-22) the "rev->diffopt" we pass down to the diff >> >> machinery has been the one we get from "format-patch"'s own >> >> setup_revisions(). >> >> >> >> This sort of thing is unique among the log-like commands in >> >> builtin/log.c, no command than format-patch will embed the output of >> >> another log-like command. Since the "rev->diffopt" is reused we need >> >> to munge it before we pass it to show_range_diff(). See >> >> 43dafc4172 ("format-patch: don't include --stat with --range-diff >> >> output", 2018-11-22) for a related regression fix which is being >> >> mostly reverted here. >> >> >> >> Implementation notes: 1) We're not bothering with the full teardown >> >> around die() and will leak memory, but it's too much boilerplate to do >> >> all the frees with/without the die() and not worth it. 2) We call >> >> repo_init_revisions() for "rd_rev" even though we could get away with >> >> a shallow copy like the code we're replacing (and which >> >> show_range_diff() itself does). This is to make this code more easily >> >> understood. >> >> >> >> Signed-off-by: Ævar Arnfjörð Bjarmason >> >> --- >> >> Documentation/git-format-patch.txt | 10 ++- >> >> builtin/log.c | 42 +++--- >> >> t/t3206-range-diff.sh | 41 + >> >> 3 files changed, 82 insertions(+), 11 deletions(-) >> >> >> >> diff --git a/Documentation/git-format-patch.txt >> >> b/Documentation/git-format-patch.txt >> >> index aba4c5febe..6c048f415f 100644 >> >> --- a/Documentation/git-format-patch.txt >> >> +++ b/Documentation/git-format-patch.txt >> >> @@ -24,7 +24,8 @@ SYNOPSIS >> >> [--to=] [--cc=] >> >> [--[no-]cover-letter] [--quiet] [--notes[=]] >> >> [--interdiff=] >> >> -[--range-diff= [--creation-factor=]] >> >> +[--range-diff=
Re: [PATCH 2/2] format-patch: allow for independent diff & range-diff options
Hi Ævar, On Thu, 29 Nov 2018, Ævar Arnfjörð Bjarmason wrote: > On Thu, Nov 29 2018, Johannes Schindelin wrote: > > > On Wed, 28 Nov 2018, Ævar Arnfjörð Bjarmason wrote: > > > >> Change the semantics of the "--range-diff" option so that the regular > >> diff options can be provided separately for the range-diff and the > >> patch. This allows for supplying e.g. --range-diff-U0 and -U1 to > >> "format-patch" to provide different context for the range-diff and the > >> patch. This wasn't possible before. > > > > I really, really dislike the `--range-diff-`. We have > > precedent for passing optional arguments that are passed to some other > > command, so a much more logical and consistent convention would be to use > > `--range-diff[=..]`, allowing all of the diff options that > > you might want to pass to the outer diff in one go rather than having a > > lengthy string of `--range-diff-this` and `--range-diff-that` options. > > Where do we pass those sorts of arguments? > > Reasons I did it this way: > > a) Passing it as one option will require the user to double-quote those > options that take quoted arguments (e.g. --word-diff-regex), which I > thought sucked more than the prefix. On the implementation side we > couldn't leave the parsing of the command-line to the shell anymore. > > b) I think people will want to tweak this very rarely, much more rarely > than e.g. -U10 in format-patch itself, so having something long-ish > doesn't sound bad. Hmm. I still don't like it. It sets a precedent, and we simply do not do it that way in other circumstances (most obvious would be the -X merge options). The more divergent user interfaces for the same sort of thing are, the more brain cycles you force users to spend on navigating said interfaces. > > I only had time to skim the patch, and I have to wonder why you pass > > around full-blown `rev_info` structs for range diff (and with that really > > awful name `rd_rev`) rather than just the `diff_options` that you > > *actually* care about? > > Because setup_revisions() which does all the command-line parsing needs > a rev_info, so even if we only need the diffopt in the end we need to > initiate the whole thing. > > Suggestions for a better varibale name most welcome. `range_diff_revs` And you do not need to pass around the whole thing. You can easily pass `_diff_revs.diffopt`. Don't pass around what you do not need to pass around. Ciao, Dscho > > > Ciao, > > Dscho > > > >> > >> Ever since the "--range-diff" option was added in > >> 31e2617a5f ("format-patch: add --range-diff option to embed diff in > >> cover letter", 2018-07-22) the "rev->diffopt" we pass down to the diff > >> machinery has been the one we get from "format-patch"'s own > >> setup_revisions(). > >> > >> This sort of thing is unique among the log-like commands in > >> builtin/log.c, no command than format-patch will embed the output of > >> another log-like command. Since the "rev->diffopt" is reused we need > >> to munge it before we pass it to show_range_diff(). See > >> 43dafc4172 ("format-patch: don't include --stat with --range-diff > >> output", 2018-11-22) for a related regression fix which is being > >> mostly reverted here. > >> > >> Implementation notes: 1) We're not bothering with the full teardown > >> around die() and will leak memory, but it's too much boilerplate to do > >> all the frees with/without the die() and not worth it. 2) We call > >> repo_init_revisions() for "rd_rev" even though we could get away with > >> a shallow copy like the code we're replacing (and which > >> show_range_diff() itself does). This is to make this code more easily > >> understood. > >> > >> Signed-off-by: Ævar Arnfjörð Bjarmason > >> --- > >> Documentation/git-format-patch.txt | 10 ++- > >> builtin/log.c | 42 +++--- > >> t/t3206-range-diff.sh | 41 + > >> 3 files changed, 82 insertions(+), 11 deletions(-) > >> > >> diff --git a/Documentation/git-format-patch.txt > >> b/Documentation/git-format-patch.txt > >> index aba4c5febe..6c048f415f 100644 > >> --- a/Documentation/git-format-patch.txt > >> +++ b/Documentation/git-format-patch.txt > >> @@ -24,7 +24,8 @@ SYNOPSIS > >> [--to=] [--cc=] > >> [--[no-]cover-letter] [--quiet] [--notes[=]] > >> [--interdiff=] > >> - [--range-diff= [--creation-factor=]] > >> + [--range-diff= [--creation-factor=] > >> +[--range-diff]] > >> [--progress] > >> [] > >> [ | ] > >> @@ -257,6 +258,13 @@ feeding the result to `git send-email`. > >>creation/deletion cost fudge factor. See linkgit:git-range-diff[1]) > >>for details. > >> > >> +--range-diff:: > >> + Other options prefixed with `--range-diff` are stripped of > >> + that prefix and passed as-is to the diff machinery used to > >> + generate the range-diff,
Re: [PATCH 2/2] format-patch: allow for independent diff & range-diff options
On Thu, Nov 29 2018, Johannes Schindelin wrote: > Hi Ævar, > > On Wed, 28 Nov 2018, Ævar Arnfjörð Bjarmason wrote: > >> Change the semantics of the "--range-diff" option so that the regular >> diff options can be provided separately for the range-diff and the >> patch. This allows for supplying e.g. --range-diff-U0 and -U1 to >> "format-patch" to provide different context for the range-diff and the >> patch. This wasn't possible before. > > I really, really dislike the `--range-diff-`. We have > precedent for passing optional arguments that are passed to some other > command, so a much more logical and consistent convention would be to use > `--range-diff[=..]`, allowing all of the diff options that > you might want to pass to the outer diff in one go rather than having a > lengthy string of `--range-diff-this` and `--range-diff-that` options. Where do we pass those sorts of arguments? Reasons I did it this way: a) Passing it as one option will require the user to double-quote those options that take quoted arguments (e.g. --word-diff-regex), which I thought sucked more than the prefix. On the implementation side we couldn't leave the parsing of the command-line to the shell anymore. b) I think people will want to tweak this very rarely, much more rarely than e.g. -U10 in format-patch itself, so having something long-ish doesn't sound bad. > I only had time to skim the patch, and I have to wonder why you pass > around full-blown `rev_info` structs for range diff (and with that really > awful name `rd_rev`) rather than just the `diff_options` that you > *actually* care about? Because setup_revisions() which does all the command-line parsing needs a rev_info, so even if we only need the diffopt in the end we need to initiate the whole thing. Suggestions for a better varibale name most welcome. > Ciao, > Dscho > >> >> Ever since the "--range-diff" option was added in >> 31e2617a5f ("format-patch: add --range-diff option to embed diff in >> cover letter", 2018-07-22) the "rev->diffopt" we pass down to the diff >> machinery has been the one we get from "format-patch"'s own >> setup_revisions(). >> >> This sort of thing is unique among the log-like commands in >> builtin/log.c, no command than format-patch will embed the output of >> another log-like command. Since the "rev->diffopt" is reused we need >> to munge it before we pass it to show_range_diff(). See >> 43dafc4172 ("format-patch: don't include --stat with --range-diff >> output", 2018-11-22) for a related regression fix which is being >> mostly reverted here. >> >> Implementation notes: 1) We're not bothering with the full teardown >> around die() and will leak memory, but it's too much boilerplate to do >> all the frees with/without the die() and not worth it. 2) We call >> repo_init_revisions() for "rd_rev" even though we could get away with >> a shallow copy like the code we're replacing (and which >> show_range_diff() itself does). This is to make this code more easily >> understood. >> >> Signed-off-by: Ævar Arnfjörð Bjarmason >> --- >> Documentation/git-format-patch.txt | 10 ++- >> builtin/log.c | 42 +++--- >> t/t3206-range-diff.sh | 41 + >> 3 files changed, 82 insertions(+), 11 deletions(-) >> >> diff --git a/Documentation/git-format-patch.txt >> b/Documentation/git-format-patch.txt >> index aba4c5febe..6c048f415f 100644 >> --- a/Documentation/git-format-patch.txt >> +++ b/Documentation/git-format-patch.txt >> @@ -24,7 +24,8 @@ SYNOPSIS >> [--to=] [--cc=] >> [--[no-]cover-letter] [--quiet] [--notes[=]] >> [--interdiff=] >> - [--range-diff= [--creation-factor=]] >> + [--range-diff= [--creation-factor=] >> + [--range-diff]] >> [--progress] >> [] >> [ | ] >> @@ -257,6 +258,13 @@ feeding the result to `git send-email`. >> creation/deletion cost fudge factor. See linkgit:git-range-diff[1]) >> for details. >> >> +--range-diff:: >> +Other options prefixed with `--range-diff` are stripped of >> +that prefix and passed as-is to the diff machinery used to >> +generate the range-diff, e.g. `--range-diff-U0` and >> +`--range-diff--no-color`. This allows for adjusting the format >> +of the range-diff independently from the patch itself. >> + >> --notes[=]:: >> Append the notes (see linkgit:git-notes[1]) for the commit >> after the three-dash line. >> diff --git a/builtin/log.c b/builtin/log.c >> index 02d88fa233..7658e56ecc 100644 >> --- a/builtin/log.c >> +++ b/builtin/log.c >> @@ -1023,7 +1023,8 @@ static void show_diffstat(struct rev_info *rev, >> fprintf(rev->diffopt.file, "\n"); >> } >> >> -static void make_cover_letter(struct rev_info *rev, int use_stdout, >> +static void make_cover_letter(struct rev_info *rev, struct rev_info *rd_rev, >>
Re: [PATCH 2/2] format-patch: allow for independent diff & range-diff options
Hi Ævar, On Wed, 28 Nov 2018, Ævar Arnfjörð Bjarmason wrote: > Change the semantics of the "--range-diff" option so that the regular > diff options can be provided separately for the range-diff and the > patch. This allows for supplying e.g. --range-diff-U0 and -U1 to > "format-patch" to provide different context for the range-diff and the > patch. This wasn't possible before. I really, really dislike the `--range-diff-`. We have precedent for passing optional arguments that are passed to some other command, so a much more logical and consistent convention would be to use `--range-diff[=..]`, allowing all of the diff options that you might want to pass to the outer diff in one go rather than having a lengthy string of `--range-diff-this` and `--range-diff-that` options. I only had time to skim the patch, and I have to wonder why you pass around full-blown `rev_info` structs for range diff (and with that really awful name `rd_rev`) rather than just the `diff_options` that you *actually* care about? Ciao, Dscho > > Ever since the "--range-diff" option was added in > 31e2617a5f ("format-patch: add --range-diff option to embed diff in > cover letter", 2018-07-22) the "rev->diffopt" we pass down to the diff > machinery has been the one we get from "format-patch"'s own > setup_revisions(). > > This sort of thing is unique among the log-like commands in > builtin/log.c, no command than format-patch will embed the output of > another log-like command. Since the "rev->diffopt" is reused we need > to munge it before we pass it to show_range_diff(). See > 43dafc4172 ("format-patch: don't include --stat with --range-diff > output", 2018-11-22) for a related regression fix which is being > mostly reverted here. > > Implementation notes: 1) We're not bothering with the full teardown > around die() and will leak memory, but it's too much boilerplate to do > all the frees with/without the die() and not worth it. 2) We call > repo_init_revisions() for "rd_rev" even though we could get away with > a shallow copy like the code we're replacing (and which > show_range_diff() itself does). This is to make this code more easily > understood. > > Signed-off-by: Ævar Arnfjörð Bjarmason > --- > Documentation/git-format-patch.txt | 10 ++- > builtin/log.c | 42 +++--- > t/t3206-range-diff.sh | 41 + > 3 files changed, 82 insertions(+), 11 deletions(-) > > diff --git a/Documentation/git-format-patch.txt > b/Documentation/git-format-patch.txt > index aba4c5febe..6c048f415f 100644 > --- a/Documentation/git-format-patch.txt > +++ b/Documentation/git-format-patch.txt > @@ -24,7 +24,8 @@ SYNOPSIS > [--to=] [--cc=] > [--[no-]cover-letter] [--quiet] [--notes[=]] > [--interdiff=] > -[--range-diff= [--creation-factor=]] > +[--range-diff= [--creation-factor=] > + [--range-diff]] > [--progress] > [] > [ | ] > @@ -257,6 +258,13 @@ feeding the result to `git send-email`. > creation/deletion cost fudge factor. See linkgit:git-range-diff[1]) > for details. > > +--range-diff:: > + Other options prefixed with `--range-diff` are stripped of > + that prefix and passed as-is to the diff machinery used to > + generate the range-diff, e.g. `--range-diff-U0` and > + `--range-diff--no-color`. This allows for adjusting the format > + of the range-diff independently from the patch itself. > + > --notes[=]:: > Append the notes (see linkgit:git-notes[1]) for the commit > after the three-dash line. > diff --git a/builtin/log.c b/builtin/log.c > index 02d88fa233..7658e56ecc 100644 > --- a/builtin/log.c > +++ b/builtin/log.c > @@ -1023,7 +1023,8 @@ static void show_diffstat(struct rev_info *rev, > fprintf(rev->diffopt.file, "\n"); > } > > -static void make_cover_letter(struct rev_info *rev, int use_stdout, > +static void make_cover_letter(struct rev_info *rev, struct rev_info *rd_rev, > + int use_stdout, > struct commit *origin, > int nr, struct commit **list, > const char *branch_name, > @@ -1095,13 +1096,9 @@ static void make_cover_letter(struct rev_info *rev, > int use_stdout, > } > > if (rev->rdiff1) { > - struct diff_options opts; > - memcpy(, >diffopt, sizeof(opts)); > - opts.output_format &= ~(DIFF_FORMAT_DIFFSTAT | > DIFF_FORMAT_SUMMARY); > - > fprintf_ln(rev->diffopt.file, "%s", rev->rdiff_title); > show_range_diff(rev->rdiff1, rev->rdiff2, > - rev->creation_factor, 1, ); > + rev->creation_factor, 1, _rev->diffopt); > } > } > > @@ -1485,6 +1482,7 @@ int cmd_format_patch(int argc, const char
Re: [PATCH 2/2] format-patch: allow for independent diff & range-diff options
Ævar Arnfjörð Bjarmason writes: > + [--range-diff]] Let's make sure a random string thrown at this mechanism will properly get noticed and diagnosed. > @@ -257,6 +258,13 @@ feeding the result to `git send-email`. > creation/deletion cost fudge factor. See linkgit:git-range-diff[1]) > for details. > > +--range-diff:: > + Other options prefixed with `--range-diff` are stripped of > + that prefix and passed as-is to the diff machinery used to > + generate the range-diff, e.g. `--range-diff-U0` and > + `--range-diff--no-color`. This allows for adjusting the format > + of the range-diff independently from the patch itself. Taking anything is of course the most general, but I am afraid if this backfires if there are some options that do not make sense to be different between the invocations of range-diff and format-patch. > @@ -1689,8 +1688,32 @@ int cmd_format_patch(int argc, const char **argv, > const char *prefix) > rev.preserve_subject = keep_subject; > > argc = setup_revisions(argc, argv, , _r_opt); > - if (argc > 1) > - die(_("unrecognized argument: %s"), argv[1]); > + if (argc > 1) { > + struct argv_array args = ARGV_ARRAY_INIT; > + const char *prefix = "--range-diff"; Please call that anything but "prefix" that hides the parameter to the function. const char *range_diff_opt = "--range-diff"; might work OK, or it might not. Let's read on. > + int have_prefix = 0; > + > + for (i = 0; i < argc; i++) { > + struct strbuf sb = STRBUF_INIT; > + char *str; > + > + strbuf_addstr(, argv[i]); > + if (starts_with(argv[i], prefix)) { > + have_prefix = 1; > + strbuf_remove(, 0, strlen(prefix)); > + } > + str = strbuf_detach(, NULL); > + strbuf_release(); > + > + argv_array_push(, str); > + } > + Is the body of the loop essentially this? char *passopt = argv[i]; if (!skip_prefix(passopt, range_diff_opt, )) saw_range_diff_opt = 1; argv_array_push(, xstrdup(passopt)); We only use that "prefix" thing once, so we may not even need the variable. > + if (!have_prefix) > + die(_("unrecognized argument: %s"), argv[1]); So we take normal options and check the leftover args; if there is no --range-diff among the leftover bits, we pretend that we stumbled while reading the first such leftover arg. > + argc = setup_revisions(args.argc, args.argv, _rev, NULL); > + if (argc > 1) > + die(_("unrecognized argument: %s"), argv[1]); > + } Otherwise, we pass all the leftover bits, which is a random mixture but guaranteed to have at least one meant for range-diff, to another setup_revisions(). If it leaves a leftover arg, then that is diagnosed here, so we'd be OK (iow, this is not a new "attack vector" to inject random string to command line parser). One minor glitch I can see is "format-patch --range-diffSilly" would report "unrecognised arg: Silly". As we are pretending to be and reporting errors as format-patch, it would be better if we report that --range-diffSilly was what we did not understand. > Junio: I know it's late, but unless Eric has objections to this UI > change I'd really like to have this in 2.20 since this is a change to > a new command-line UI that's newly added in 2.20. Quite honestly, I'd rather document "driving range-diff from format-patch is experimental and does silly things when given non-standard options in this release" and not touch the code at this late stage in the game. Would it be less intrusive a change to *not* support the --range-diff option, still use rd_rev that is separate from the main rev, and use a reasonable hardcoded default settings when preparing rd_rev?
[PATCH 2/2] format-patch: allow for independent diff & range-diff options
Change the semantics of the "--range-diff" option so that the regular diff options can be provided separately for the range-diff and the patch. This allows for supplying e.g. --range-diff-U0 and -U1 to "format-patch" to provide different context for the range-diff and the patch. This wasn't possible before. Ever since the "--range-diff" option was added in 31e2617a5f ("format-patch: add --range-diff option to embed diff in cover letter", 2018-07-22) the "rev->diffopt" we pass down to the diff machinery has been the one we get from "format-patch"'s own setup_revisions(). This sort of thing is unique among the log-like commands in builtin/log.c, no command than format-patch will embed the output of another log-like command. Since the "rev->diffopt" is reused we need to munge it before we pass it to show_range_diff(). See 43dafc4172 ("format-patch: don't include --stat with --range-diff output", 2018-11-22) for a related regression fix which is being mostly reverted here. Implementation notes: 1) We're not bothering with the full teardown around die() and will leak memory, but it's too much boilerplate to do all the frees with/without the die() and not worth it. 2) We call repo_init_revisions() for "rd_rev" even though we could get away with a shallow copy like the code we're replacing (and which show_range_diff() itself does). This is to make this code more easily understood. Signed-off-by: Ævar Arnfjörð Bjarmason --- Documentation/git-format-patch.txt | 10 ++- builtin/log.c | 42 +++--- t/t3206-range-diff.sh | 41 + 3 files changed, 82 insertions(+), 11 deletions(-) diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt index aba4c5febe..6c048f415f 100644 --- a/Documentation/git-format-patch.txt +++ b/Documentation/git-format-patch.txt @@ -24,7 +24,8 @@ SYNOPSIS [--to=] [--cc=] [--[no-]cover-letter] [--quiet] [--notes[=]] [--interdiff=] - [--range-diff= [--creation-factor=]] + [--range-diff= [--creation-factor=] + [--range-diff]] [--progress] [] [ | ] @@ -257,6 +258,13 @@ feeding the result to `git send-email`. creation/deletion cost fudge factor. See linkgit:git-range-diff[1]) for details. +--range-diff:: + Other options prefixed with `--range-diff` are stripped of + that prefix and passed as-is to the diff machinery used to + generate the range-diff, e.g. `--range-diff-U0` and + `--range-diff--no-color`. This allows for adjusting the format + of the range-diff independently from the patch itself. + --notes[=]:: Append the notes (see linkgit:git-notes[1]) for the commit after the three-dash line. diff --git a/builtin/log.c b/builtin/log.c index 02d88fa233..7658e56ecc 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -1023,7 +1023,8 @@ static void show_diffstat(struct rev_info *rev, fprintf(rev->diffopt.file, "\n"); } -static void make_cover_letter(struct rev_info *rev, int use_stdout, +static void make_cover_letter(struct rev_info *rev, struct rev_info *rd_rev, + int use_stdout, struct commit *origin, int nr, struct commit **list, const char *branch_name, @@ -1095,13 +1096,9 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout, } if (rev->rdiff1) { - struct diff_options opts; - memcpy(, >diffopt, sizeof(opts)); - opts.output_format &= ~(DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY); - fprintf_ln(rev->diffopt.file, "%s", rev->rdiff_title); show_range_diff(rev->rdiff1, rev->rdiff2, - rev->creation_factor, 1, ); + rev->creation_factor, 1, _rev->diffopt); } } @@ -1485,6 +1482,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) struct commit *commit; struct commit **list = NULL; struct rev_info rev; + struct rev_info rd_rev; struct setup_revision_opt s_r_opt; int nr = 0, total, i; int use_stdout = 0; @@ -1603,6 +1601,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) init_log_defaults(); git_config(git_format_config, NULL); repo_init_revisions(the_repository, , prefix); + repo_init_revisions(the_repository, _rev, prefix); rev.commit_format = CMIT_FMT_EMAIL; rev.expand_tabs_in_log_default = 0; rev.verbose_header = 1; @@ -1689,8 +1688,32 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) rev.preserve_subject = keep_subject; argc = setup_revisions(argc, argv,