On Thu, Nov 29, 2018 at 11:27 PM Junio C Hamano <gits...@pobox.com> wrote: > Junio C Hamano <gits...@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.
