Re: [PATCH 2/2] format-patch: allow for independent diff & range-diff options

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

[-- Attachment #1: Type: text/plain, Size: 4326 bytes --]

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-<random-thing>`. 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[=<diff-option>..]`, 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=<TAB>".

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