General and development mailing list for Git, a distributed version control system

Keith Cascio | 3 Apr 2009 02:04

Re: [PATCH] Allow setting default diff options via diff.defaultOptions

Johannes,

On Sat, 21 Mar 2009, Johannes Schindelin wrote:

> The idea is from Keith Cascio.
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin <at> gmx.de>
> ---
> 	I do not particularly like what this patch does, but I like
> 	the non-intrusiveness and conciseness of it.

Your patch does not provide a command line opt_out flag.  Let me describe a 
workflow situation and ask you how to handle it if the user were running your 
patch.  Let diff.defaultOptions = "-b".  The user is getting closer to 
submitting his patch and he wants to see patch output identical to what `git format-patch`
will produce.  What command should he use?

      `git format-patch --stdout master` ?

                                  -- Keith

Johannes Schindelin | 9 Apr 2009 10:45

Picon

Picon

Re: [PATCH] Allow setting default diff options via diff.defaultOptions

Hi,

On Thu, 2 Apr 2009, Keith Cascio wrote:

> On Sat, 21 Mar 2009, Johannes Schindelin wrote:
> 
> > The idea is from Keith Cascio.
> > 
> > Signed-off-by: Johannes Schindelin <johannes.schindelin <at> gmx.de>
> > ---
> > 	I do not particularly like what this patch does, but I like
> > 	the non-intrusiveness and conciseness of it.
> 
> Your patch does not provide a command line opt_out flag.  Let me describe a 
> workflow situation and ask you how to handle it if the user were running your 
> patch.  Let diff.defaultOptions = "-b".  The user is getting closer to 
> submitting his patch and he wants to see patch output identical to what `git format-patch`
> will produce.  What command should he use?
> 
>       `git format-patch --stdout master` ?

The proper way would be to have options to _undo_ every diff option, I 
guess, as this would also help aliases in addition to defaultOptions.

In the case of format-patch, though, I am pretty certain that I do not 
want any diff.defaultOptions: the output is almost always intended for 
machine consumption, so it is a different kind of cattle.

Now, it is easy to put a patch on top of my patch to support something 
like --no-defaults.

(Continue reading)

Keith Cascio | 9 Apr 2009 18:29

Re: [PATCH] Allow setting default diff options via diff.defaultOptions

Johannes,

On Thu, 9 Apr 2009, Johannes Schindelin wrote:

> In the case of format-patch, though, I am pretty certain that I do not want 
> any diff.defaultOptions: the output is almost always intended for machine 
> consumption, so it is a different kind of cattle.

Just to clarify:  I agree.  I certainly would never want diff.defaultOptions to 
affect format-patch, and none of my patches did so.  The reason I brought up 
format-patch is because, without an opt_out mechanism, it's harder for the user 
to use `git diff` to produce patch output identical to format-patch.

> Now, it is easy to put a patch on top of my patch to support something like 
> --no-defaults.

With all due respect sir, I think you would find that if you sit down and 
attempt to add such functionality on top of your version, it would be an 
unpleasant experience.  I predict the code would quickly turn inelegant and 
fragile.  I believe it would prompt you to consider a redesign (assuming you 
work and think quickly, after about 15 minutes), and the obvious solution would 
closely resemble my v3:
  http://comments.gmane.org/gmane.comp.version-control.git/114021

                             -- Keith

Jeff King | 9 Apr 2009 10:49

Re: [PATCH] Allow setting default diff options via diff.defaultOptions

On Thu, Apr 09, 2009 at 10:45:40AM +0200, Johannes Schindelin wrote:

> The proper way would be to have options to _undo_ every diff option, I 
> guess, as this would also help aliases in addition to defaultOptions.

I agree with this sentiment, no matter which approach is taken. I am
more like to say "take my usual defaults, but tweak this one thing" than
to say "turn off all of my defaults".

> Now, it is easy to put a patch on top of my patch to support something 
> like --no-defaults.

No, it's not. We went over this in great detail earlier in the thread.
If you want:

  git diff --no-defaults

then you basically have to parse twice to avoid the chicken-and-egg
problem. Which is why I suggested:

  git --no-defaults diff

which does work. Keith's solution does allow "git diff --no-defaults".

-Peff

Johannes Schindelin | 9 Apr 2009 12:43

Picon

Picon

Re: [PATCH] Allow setting default diff options via diff.defaultOptions

Hi,

On Thu, 9 Apr 2009, Jeff King wrote:

> On Thu, Apr 09, 2009 at 10:45:40AM +0200, Johannes Schindelin wrote:
> 
> > Now, it is easy to put a patch on top of my patch to support something 
> > like --no-defaults.
> 
> No, it's not. We went over this in great detail earlier in the thread. 
> If you want:
> 
>   git diff --no-defaults
> 
> then you basically have to parse twice to avoid the chicken-and-egg
> problem.

So what?  We parse the config a gazillion times, and there we have to 
access the _disk_.

Ciao,
Dscho

Jeff King | 10 Apr 2009 10:01

Re: [PATCH] Allow setting default diff options via diff.defaultOptions

On Thu, Apr 09, 2009 at 12:43:28PM +0200, Johannes Schindelin wrote:

> > > Now, it is easy to put a patch on top of my patch to support something 
> > > like --no-defaults.
> > 
> > No, it's not. We went over this in great detail earlier in the thread. 
> > If you want:
> > 
> >   git diff --no-defaults
> > 
> > then you basically have to parse twice to avoid the chicken-and-egg
> > problem.
> 
> So what?  We parse the config a gazillion times, and there we have to 
> access the _disk_.

But the first parse is only looking for "--no-defaults". So you need to:

  1. Understand the semantics of the other options to correctly parse
     around them (i.e., knowing which ones take arguments).

  2. Parse the arguments without actually respecting most of them, since
     they will be parsed again later.

This would actually be pretty easy if we had a declarative structure
describing each option (like parseopt-ified options do). But the diff
options are parsed by a big conditional statement.

Two ways to make it easier would be:

(Continue reading)

Johannes Schindelin | 14 Apr 2009 00:37

Picon

Picon

[PATCH] Add the diff option --no-defaults


It would be desirable to undo every setting in diff.defaultOptions
individually, but until there are options to reset every command
line option, there is the "--no-defaults" option (which can be
overridden by the "--defaults" option) to ignore the config setting.

Signed-off-by: Johannes Schindelin <johannes.schindelin <at> gmx.de>
---

On Fri, 10 Apr 2009, Jeff King wrote:

> On Thu, Apr 09, 2009 at 12:43:28PM +0200, Johannes Schindelin wrote:
> 
> > > > Now, it is easy to put a patch on top of my patch to support something 
> > > > like --no-defaults.
> > > 
> > > No, it's not. We went over this in great detail earlier in the thread. 
> > > If you want:
> > > 
> > >   git diff --no-defaults
> > > 
> > > then you basically have to parse twice to avoid the chicken-and-egg
> > > problem.
> > 
> > So what?  We parse the config a gazillion times, and there we have to 
> > access the _disk_.
> 
> But the first parse is only looking for "--no-defaults". So you need to:
> 
>   1. Understand the semantics of the other options to correctly parse

(Continue reading)

Jeff King | 16 Apr 2009 10:34

Re: [PATCH] Add the diff option --no-defaults

On Tue, Apr 14, 2009 at 12:37:42AM +0200, Johannes Schindelin wrote:

> >   1. You could loosen (1) above by assuming that --no-defaults will
> >      never appears as the argument to an option, and therefore any time
> >      you find it, it should be respected. Thus your first parse is just
> >      a simple loop looking for the option.
> 
> I go with 1)

This feels very hack-ish to me, but perhaps this is a case of "perfect
is the enemy of the good".

-Peff

Johannes Schindelin | 16 Apr 2009 11:25

Picon

Picon

Re: [PATCH] Add the diff option --no-defaults

Hi,

On Thu, 16 Apr 2009, Jeff King wrote:

> On Tue, Apr 14, 2009 at 12:37:42AM +0200, Johannes Schindelin wrote:
> 
> > >   1. You could loosen (1) above by assuming that --no-defaults will
> > >      never appears as the argument to an option, and therefore any time
> > >      you find it, it should be respected. Thus your first parse is just
> > >      a simple loop looking for the option.
> > 
> > I go with 1)
> 
> This feels very hack-ish to me, but perhaps this is a case of "perfect
> is the enemy of the good".

I have a strong feeling that none of our diff/rev options can sanely take 
a parameter looking like "--defaults" or "--no-defaults".

But I do not have the time to audit the options.  Maybe you have?

Ciao,
Dscho

Jeff King | 16 Apr 2009 11:41

Re: [PATCH] Add the diff option --no-defaults

On Thu, Apr 16, 2009 at 11:25:08AM +0200, Johannes Schindelin wrote:

> > This feels very hack-ish to me, but perhaps this is a case of "perfect
> > is the enemy of the good".
> 
> I have a strong feeling that none of our diff/rev options can sanely take 
> a parameter looking like "--defaults" or "--no-defaults".
> 
> But I do not have the time to audit the options.  Maybe you have?

Right now, I think we are safe. A few options like "--default" do take a
separated string argument, but saying "--default --no-defaults" seems a
little crazy to me (besides being confusing because they are talking
about two totally unrelated defaults).

Most of the string-taking options require --option=<arg> and don't
support the separated version. If the code were ever parseopt-ified,
they would start to support "--option <arg>"; however, at that time we
should be able to write an "I know about these parseopt options, but
please ignore them according to what we know about them taking an
argument" function.

The one I would worry most about is "-S" since "-S--no-defaults" is a
very reasonable thing to ask for. Right now its argument _must_ be
connected. To be consistent with other git options, "-S --no-defaults"
_should_ be the same thing. But we can perhaps ignore that because:

  1. That might never happen, because it breaks historical usage. IOW,
     it changes the meaning of "git log -S HEAD" to something else.

(Continue reading)

Junio C Hamano | 16 Apr 2009 18:52

Picon

Picon

Re: [PATCH] Add the diff option --no-defaults

Jeff King <peff <at> peff.net> writes:

> On Thu, Apr 16, 2009 at 11:25:08AM +0200, Johannes Schindelin wrote:
>
>> > This feels very hack-ish to me, but perhaps this is a case of "perfect
>> > is the enemy of the good".
>> 
>> I have a strong feeling that none of our diff/rev options can sanely take 
>> a parameter looking like "--defaults" or "--no-defaults".
>> 
>> But I do not have the time to audit the options.  Maybe you have?
>
> Right now, I think we are safe. A few options like "--default" do take a
> separated string argument, but saying "--default --no-defaults" seems a
> little crazy to me (besides being confusing because they are talking
> about two totally unrelated defaults).

Maybe you guys have already considered and discarded this as too hacky,
but isn't it the easiest to explain and code to declare --no-defaults is
acceptable only at the beginning?

Jeff King | 17 Apr 2009 13:54

Re: [PATCH] Add the diff option --no-defaults

On Thu, Apr 16, 2009 at 09:52:50AM -0700, Junio C Hamano wrote:

> > Right now, I think we are safe. A few options like "--default" do take a
> > separated string argument, but saying "--default --no-defaults" seems a
> > little crazy to me (besides being confusing because they are talking
> > about two totally unrelated defaults).
> 
> Maybe you guys have already considered and discarded this as too hacky,
> but isn't it the easiest to explain and code to declare --no-defaults is
> acceptable only at the beginning?

I discarded that as "too hacky". If I had to choose my poison between
"insane string options don't work" and "option must inexplicably be at
the front", I think I take the former. It is perhaps a more difficult
rule to realize you are triggering, but it is much less likely to come
up in practice.

But I think all of this is just ending up in the same place that Keith
and I arrived at much earlier in the thread: you _are_ choosing a
poison, and his patch was meant to avoid that. The question is whether
the added code complexity is worth it.

-Peff

Johannes Schindelin | 17 Apr 2009 15:15

Picon

Picon

Re: [PATCH] Add the diff option --no-defaults

Hi,

On Fri, 17 Apr 2009, Jeff King wrote:

> But I think all of this is just ending up in the same place that Keith 
> and I arrived at much earlier in the thread: you _are_ choosing a 
> poison, and his patch was meant to avoid that. The question is whether 
> the added code complexity is worth it.

Well, I think I gave my answer in the form of two patches.

Besides, you still will have a poison:

	git config diff.defaultOptions --no-defaults

which is Russel's paradoxon right there.

Ciao,
Dscho

Keith Cascio | 18 Apr 2009 18:41

Re: [PATCH] Add the diff option --no-defaults

Dscho,

On Fri, 17 Apr 2009, Johannes Schindelin wrote:

> Besides, you still will have a poison:
> 
> 	git config diff.defaultOptions --no-defaults
> 
> which is Russel's paradoxon right there.

I can cleanly modify my v3 to handle this case.  In diff_setup_done(), change 
this:

+	if (DIFF_OPT_TST(options, ALLOW_DEFAULT_OPTIONS))
+		flatten_diff_options(options, defaults ? defaults :
+			parse_diff_defaults(diff_setup(defaults =
+				xmalloc(sizeof(struct diff_options)))));

to this:

+	if (DIFF_OPT_TST(options, ALLOW_DEFAULT_OPTIONS) && (defaults ||
+		parse_diff_defaults(diff_setup(defaults = xmalloc(
+			sizeof(struct diff_options))))) && DIFF_OPT_TST(
+				defaults, ALLOW_DEFAULT_OPTIONS))
+					flatten_diff_options(options,
+						defaults);

All I did there was add the test DIFF_OPT_TST(defaults, ALLOW_DEFAULT_OPTIONS) 
to the condition that controls whether to perform the flattening.  Clean and 
clear.

(Continue reading)

Johannes Schindelin | 18 Apr 2009 19:40

Picon

Picon

Re: [PATCH] Add the diff option --no-defaults

Hi Keith,

On Sat, 18 Apr 2009, Keith Cascio wrote:

> On Fri, 17 Apr 2009, Johannes Schindelin wrote:
> 
> > Besides, you still will have a poison:
> > 
> > 	git config diff.defaultOptions --no-defaults
> > 
> > which is Russel's paradoxon right there.
> 
> I can cleanly modify my v3 to handle this case.

You cannot.  --no-defaults means that diff.defaultOptions should be 
disregarded.  If the diff.defaultOptions say that they should be 
disregarded themselves, then --no-defaults should be disregarded.

And I still do not like the intrusiveness of your patch.  The last time we 
did something like that with options (some parseoptifications), we had a 
lot of fallout as a consequence.

Ciao,
Dscho

Keith Cascio | 18 Apr 2009 22:32

Re: [PATCH] Add the diff option --no-defaults

Dscho,

On Sat, 18 Apr 2009, Johannes Schindelin wrote:

> On Sat, 18 Apr 2009, Keith Cascio wrote:
> 
> > On Fri, 17 Apr 2009, Johannes Schindelin wrote:
> > 
> > > Besides, you still will have a poison:
> > > 	git config diff.defaultOptions --no-defaults
> > > which is Russel's paradoxon right there.
> > 
> > I can cleanly modify my v3 to handle this case.
> 
> You cannot.  --no-defaults means that diff.defaultOptions should be 
> disregarded.  If the diff.defaultOptions say that they should be disregarded 
> themselves, then --no-defaults should be disregarded.

--no-defaults *could* mean as you say there.  But a much better meaning for 
--no-defaults is: suppress the values in diff.defaultOptions after options 
processing.  We don't have to disregard them, just suppress them after options 
processing.  In that sense, --no-defaults is a meta-option.  It is an option 
about options.  Even users unfamiliar with set theory would assume the 
suppression semantics.

Nevertheless I applaud the Russell reference.  Very intriguing.

> And I still do not like the intrusiveness of your patch.  The last time we did 
> something like that with options (some parseoptifications), we had a lot of 
> fallout as a consequence.

(Continue reading)

Johannes Schindelin | 18 Apr 2009 23:15

Picon

Picon

Re: [PATCH] Add the diff option --no-defaults

Hi.

On Sat, 18 Apr 2009, Keith Cascio wrote:

> On Sat, 18 Apr 2009, Johannes Schindelin wrote:
> 
> > And I still do not like the intrusiveness of your patch.  The last 
> > time we did something like that with options (some 
> > parseoptifications), we had a lot of fallout as a consequence.
> 
> [...]
>
> Peff expressed some specific concerns [...]

My concerns were also very specific: your patches are way too large.  
There is a rule of thumb that the likelihood of a bug is the square of the 
number of changed lines, I am sure you heard that before.

> [...] your patch adds at least ten lines outside of diff.h/diff.c.

That is _such_ a red herring.

Ciao,
Dscho

Johannes Schindelin | 16 Apr 2009 19:36

Picon

Picon

Re: [PATCH] Add the diff option --no-defaults

Hi,

On Thu, 16 Apr 2009, Junio C Hamano wrote:

> Jeff King <peff <at> peff.net> writes:
> 
> > On Thu, Apr 16, 2009 at 11:25:08AM +0200, Johannes Schindelin wrote:
> >
> >> > This feels very hack-ish to me, but perhaps this is a case of "perfect
> >> > is the enemy of the good".
> >> 
> >> I have a strong feeling that none of our diff/rev options can sanely take 
> >> a parameter looking like "--defaults" or "--no-defaults".
> >> 
> >> But I do not have the time to audit the options.  Maybe you have?
> >
> > Right now, I think we are safe. A few options like "--default" do take a
> > separated string argument, but saying "--default --no-defaults" seems a
> > little crazy to me (besides being confusing because they are talking
> > about two totally unrelated defaults).
> 
> Maybe you guys have already considered and discarded this as too hacky,
> but isn't it the easiest to explain and code to declare --no-defaults is
> acceptable only at the beginning?

That would not work if you use an alias:

	$ git config alias.grmpfl log --stat
	$ git grmpfl --no-defaults

(Continue reading)

Return

Return to gmane.comp.version-control.git.

Search Archive

Language

Change language

Options

Current view: Threads only / Showing only 20 lines / Not hiding cited text.
Change to All messages, whole messages, or hide cited text.

Post a message
NNTP Newsgroup
Classic Gmane web interface
XML

XML

RSS Feed
List Information

About Gmane