| Message ID | xmqqwoovkx5s.fsf_-_@gitster-ct.c.googlers.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | 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) | expand |
Junio C Hamano <gitster@pobox.com> 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.
On Fri, Nov 30 2018, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> 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.
On Thu, Nov 29, 2018 at 11:27 PM Junio C Hamano <gitster@pobox.com> wrote: > Junio C Hamano <gitster@pobox.com> 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(&opts, diffopt, sizeof(opts)); > + if (diffopt) > + memcpy(&opts, diffopt, sizeof(opts)); > + else > + repo_diff_setup(the_repository, &opts); 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.
Hi Junio, On Fri, 30 Nov 2018, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> 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
On Fri, 30 Nov 2018 at 10:32, Eric Sunshine <sunshine@sunshineco.com> wrote: > > On Thu, Nov 29, 2018 at 11:27 PM Junio C Hamano <gitster@pobox.com> wrote: > > Junio C Hamano <gitster@pobox.com> 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 <martin.agren@gmail.com> --- 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); + opts.file = rev->diffopt.file; + diff_setup_done(&opts); 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, &opts); } } 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(&dq, &diff_queued_diff, sizeof(diff_queued_diff)); DIFF_QUEUE_CLEAR(&diff_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 we want to handle them. + */ + diff_setup(&opts); + opts.file = opt->diffopt.file; + diff_setup_done(&opts); show_range_diff(opt->rdiff1, opt->rdiff2, - opt->creation_factor, 1, NULL); + opt->creation_factor, 1, &opts); memcpy(&diff_queued_diff, &dq, sizeof(diff_queued_diff)); }
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=<percent>:: 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, &rev->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, &opt->diffopt); + opt->creation_factor, 1, NULL); memcpy(&diff_queued_diff, &dq, 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(&opts, diffopt, sizeof(opts)); + if (diffopt) + memcpy(&opts, diffopt, sizeof(opts)); + else + repo_diff_setup(the_repository, &opts); + if (!opts.output_format) opts.output_format = DIFF_FORMAT_PATCH; opts.flags.suppress_diff_headers = 1; diff --git a/range-diff.h b/range-diff.h index 190593f0c7..08a50b6e98 100644 --- a/range-diff.h +++ b/range-diff.h @@ -5,6 +5,11 @@ #define RANGE_DIFF_CREATION_FACTOR_DEFAULT 60 +/* + * 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. + */ int show_range_diff(const char *range1, const char *range2, int creation_factor, int dual_color, struct diff_options *diffopt);