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)

From: Eric Sunshine <sunshine@sunshineco.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: "Johannes Schindelin" <Johannes.Schindelin@gmx.de>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Git List" <git@vger.kernel.org>
Subject: 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)
Date: Fri, 30 Nov 2018 04:31:46 -0500	[thread overview]
Message-ID: <CAPig+cS4peHDOz0cK7RJEHb6ch0+czvau_aGTD1j6xM23G0-pQ@mail.gmail.com> (raw)
In-Reply-To: <xmqqwoovkx5s.fsf_-_@gitster-ct.c.googlers.com>

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.