login
Search
Introduce config variable "diff.primer".
Improve porcelain diff's accommodation of user preference by allowing
some settings to (a) persist over all invocations and (b) stay consistent
over multiple tools (e.g. command-line and gui). The approach taken here
is good because it delivers the consistency a user expects without breaking
any plumbing. It works by allowing the user, via git-config, to specify
arbitrary options to pass to porcelain diff on every invocation, including
internal invocations from other programs, e.g. git-gui. Introduce diff
command-line options --primer and --no-primer. Affect only porcelain diff:
we suppress primer options for plumbing diff-{files,index,tree},
format-patch, and all other commands unless explicitly requested using
--primer (opt-in). Teach gitk to use --primer, but protect it from
inapplicable options like --color.
Signed-off-by: Keith Cascio <keith@cs.ucla.edu>
---
Documentation/config.txt | 14 +++++++
Documentation/diff-options.txt | 10 +++++
builtin-diff.c | 2 +
diff.c | 77 +++++++++++++++++++++++++++++++++++-----
diff.h | 14 ++++++--
gitk-git/gitk | 16 ++++----
6 files changed, 113 insertions(+), 20 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index e2b8775..bd85c4a 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -601,6 +601,20 @@ diff.autorefreshindex::
affects only 'git-diff' Porcelain, and not lower level
'diff' commands, such as 'git-diff-files'.
+diff.primer::
+ Whitespace-separated list of options to pass to 'git-diff'
+ on every invocation, including internal invocations from
+ linkgit:git-gui[1] and linkgit:gitk[1],
+ e.g. `"--patience --color --ignore-space-at-eol --exit-code"`.
+ See linkgit:git-diff[1]. You can suppress these at run time with
+ option `--no-primer`. Supports a subset of
+ 'git-diff'\'s many options, at least:
+ `-b --binary ...Test functionality of new config variable "diff.primer" Signed-off-by: Keith Cascio <keith@cs.ucla.edu> --- t/t4035-diff-primer.sh | 129 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 129 insertions(+), 0 deletions(-) create mode 100755 t/t4035-diff-primer.sh diff --git a/t/t4035-diff-primer.sh b/t/t4035-diff-primer.sh new file mode 100755 index 0000000..c33911c --- /dev/null +++ b/t/t4035-diff-primer.sh @@ -0,0 +1,129 @@ +#!/bin/sh +# +# Copyright (c) 2009 Keith G. Cascio +# +# based on t4015-diff-whitespace.sh by Johannes E. Schindelin +# + +test_description='Ensure diff engine honors config variable "diff.primer". + +' +. ./test-lib.sh +. "$TEST_DIRECTORY"/diff-lib.sh + +tr 'Q' '\015' << EOF > x +whitespace at beginning +whitespace change +whitespace in the middle +whitespace at end +unchanged line +CR at endQ +EOF + +git add x +git commit -m '1.0' >/dev/null 2>&1 + +tr '_' ' ' << EOF > x + whitespace at beginning +whitespace change +white space in the middle +whitespace at end__ +unchanged line +CR at end +EOF + +test_expect_success 'ensure diff.primer born empty' ' +[ -z $(git config --get diff.primer) ] +' + +tr 'Q_' '\015 ' << EOF > expect_noprimer +diff --git a/x b/x +index d99af23..8b32fb5 100644 +--- a/x ++++ b/x +@@ -1,6 +1,6 @@ +-whitespace at beginning +-whitespace change +-whitespace in the middle +-whitespace at end ++ whitespace at beginning ++whitespace change ++white space in the middle ++whitespace at end__ + unchanged line +-CR at endQ ++CR at end +EOF +git diff > out +test_expect_success 'test git-diff with empty value of diff.primer' 'test_cmp expect_noprimer out' + +git config diff.primer '-w' + +test_expect_success 'ensure diff.primer value set' ' +[ $(git config --get diff.primer) = "-w" ] +' + +git diff --no-primer > out +test_expect_success 'test git-diff --no-primer' 'test_cmp expect_noprimer out' +git diff-files -p > out +test_expect_success 'ensure diff-files unaffected ...
Seems like the summary email for this patch refuses to deliver through the list.
I can send it to anyone individually if you are interested.
-- Keith
--
The next two patches introduce a means by which to specify non-default options porcelain diff automatically obeys. This version v2 fixes the problem with v1 (violation of plumbing guarantee) by switching to opt_in rather than opt_out semantics. v2 also corrects code style to mimic established convention. At least one list poster expressed interest in implementing complimentary functionality, i.e. "primer.*": http://article.gmane.org/gmane.comp.version-control.git/107158 "primer.diff" compliments "diff.primer", the two styles in no way exclude each other, and therefore "primer.*" is a good opportunity for future work. Keith Cascio (2): Introduce config variable "diff.primer" Test functionality of new config variable "diff.primer" Documentation/config.txt | 14 ++++ Documentation/diff-options.txt | 10 +++ builtin-diff.c | 2 + diff.c | 77 +++++++++++++++++++++--- diff.h | 14 ++++- gitk-git/gitk | 16 +++--- t/t4035-diff-primer.sh | 129 ++++++++++++++++++++++++++++++++++++++++ 7 files changed, 242 insertions(+), 20 deletions(-) --
Funny indentation?
This seems really clunky to list all of the options here. I thought the
point was to respect _all_ of them, but do it from porcelain so that it
is up to the user what they want to put in.
Some of the manpages use a more terse form for negatable options, like:
--[no-]primer::
which often helps focus the text a bit. Something like:
--[no-]primer::
Respect (or ignore) options specifed in the diff.primer
configuration variable. By default, porcelain commands (such as `git
diff` and `git log`) respect this variable, but plumbing commands
(such as `git diff-{files,index,tree}`) do not.
Also, don't mention ".git/config" by name: configuration can come from
Probably ALLOW_PRIMER is a more sensible name, to match ALLOW_EXTERNAL
This doesn't appear to have any quoting mechanism. Is it impossible to
have an option with spaces (e.g., --relative='foo bar')? I guess that is
probably uncommon, but I would expect normal shell quoting rules to
Style: whitespace between operands and operators.
I have to admit that this particular line is pretty dense to read. You
have eliminated any meaning from the variable names (like the fact that
you have a master/slave pair of flag/mask pairs). Yes, you point to the
Quine-McCluskey algorithm in the comment above, but I think something
like this would be easier to see what is going on:
/*
* Our desired flags are:
*
* 1. Anything the master hasn't explicitly set, we can take from
* the slave.
* 2. Anything the slave didn't explicitly, we can take whether or
* not the master set it explicitly.
* 3. Anything the master explicitly set, we take.
*/
master->flags =
/* (1) */ (~master->flags & slave->flags & slave->mask) |
/* (2) */ (master->flags & ~slave->mask) |
Hmm. I haven't gotten to any changes to DIFF_OPT_{SET,CLR} yet. But it
is a little worrisome that this patch is so invasive as to require a
change like this. I wouldn't be ...Peff,
First of all, thanks for the tips!
How about:
Improve porcelain diff's accommodation of user preference by allowing
some settings to (a) persist over all invocations and (b) stay consistent
over multiple tools (e.g. command-line and gui). The approach taken here
is good because it delivers the consistency a user expects without breaking
any plumbing. It works by allowing the user, via git-config, to specify
arbitrary options to pass to porcelain diff on every invocation, including
internal invocations from other programs, e.g. git-gui.
Introduce diff command-line options --primer and --no-primer.
Affect only porcelain diff: we suppress primer options for plumbing
diff-{files,index,tree}, format-patch, and all other commands unless explicitly
requested using --primer (opt-in).
Teach gitk to use --primer, but protect it from inapplicable options like
The last part is a backtick-quoted list, I think it either must occur on one
The current version does not try to support all diff options. It only supports
those that are recorded in struct diff_options.flags and .xdl_opts - that is the
The current version only supports the flags listed above in
My proposal is to make DIFF_OPT_{SET,CLR} more meaningful. Instead of just
flipping a bit, they mean "{un}set this bit and honor my intention in the
Yes I believe I did. The only danger zones are spots that use struct
diff_options member "flags" or "xdl_opts" along with assignment and bitwise
operators. The following recursive grep call (output included) satisfies me
that no such spots exist after my patch.
grep -EHnr '--include=*.[ch]' ...I am not so sure about this claim. I kind of like idea of "we know the desirable value of this bit, do not touch it any further" mask, but I think it is independent of flattening. In fact, I do not think flattening is a good thing. Any codepath could call DIFF_OPT_SET()/CLR(), whether it is in response to end user's input from the command line (e.g. "the user said --foo, so I am flipping the foo bit on/off), or to enforce restriction to achieve sane semantics (e.g. "it does not make any sense to run this internal diff without --binary set, so I am using OPT_SET()"). Is it still true, or do some bit flipping now need to be protected by "is it locked" check and some others don't? The latter one (i.e. in the above example, "this internal diff must run with --binary") we may want to use "opts->mask" to lock down (I wouldn't call it "dirty", it is more like "locked") the "binary" bit, and we may even want to issue a warning or an error when the end user attempts to countermand with --no-binary. Similarly, I think you would want to lock down what you got from the true command line so that you can leave them untouched when you process the value you read from diff.primer. Doesn't it suggest that you may want two layers of masks, not a flat one, if you really want the mechanism to scale? In any case, I think the mechanism based on the lock-down mask is worth considering when we enhance the option parsing mechanism for the diff and log family, and if it is done right, I think the code to parse revision options would benefit quite a bit. There are codepaths that initialize the bits to the command's liking (e.g. show wants to always have diff), let setup_revisions() to process command line flags, and then further tweak the remaining bits (e.g. whatchanged wants to default to raw), all interacting in quite subtle ways. But that should probably be for later cycle, post 1.6.2. --
Yes but the trick is the flips and maskings happen on different structs. We
accumulate the shell command line flags/masks in a separate struct from the
There are indeed two layers of masks (and there can be as many as needed). In
my current patch, the shell command line becomes "master" and primer becomes
"slave". Both layers exist independently of each other, in two separate
diff_option structs, until just before "go time", when I flatten them (but that
does not destroy the slave, it is reused). As Peff put it: "a master/slave pair
of flag/mask pairs". I specifically designed the code to make it easy to create
an arbitrary number of layers, then flatten them all together just before it's
time to do something. The code only needs to keep track of the order of
precedence, i.e. always pass the higher precedence struct to
flatten_diff_options() as master and the lower precedence struct as slave, and
the bit logic in that function does the rest. I was specifically thinking of
GIMP or Photoshop when I wrote this patch. The concept is the same. Those
programs support an arbitrary number of layers, and when they produce the final
image, they call it "flatten layers".
If you and Peff like this design then I could clean up everything based on all
of Peff's suggestions (i.e. xmalloc instead of malloc, etc) and hopefully move
on to the stage of building consensus for the actual name. No rush of course.
Just give me the word. Peff?
-- Keith
--
We have had trouble in the past figuring out exactly when "go time" is (not specifically for diff options, but for things like color config). That is, there is code which wants to munge the options based on some other input much later than the setting of most options, and so any finalizing work you do ends up happening too early. And maybe you can argue that such code is wrong or bad, but it does get written and it does cause problems. So as I mentioned in another mail I just sent, I think you are best to stop thinking of it as a general flattening, and think of it more as a set of accessors that do the flattening Just In Time. -Peff --
Ah, yes, I see. I wonder if that is a sign that we can use some kind of
list construct instead. I also wonder if we need the exact list. It
seems like a poor user interface to have to enumerate each option. As a
user, should I be saying "I would like to put --foobar into my primer
variable. Let me check whether it is supported"? Or should there be some
sane and succint rule by which to say "I know whether --foobar is
supported or not, because it falls into categroy X"?
I think the latter is much nicer to users. And then the help text
describes that rule (and I think the rule should be "everything that
tweaks what diffs _look_ like is included" or something like that).
That is a very unsatisfying list, as it bears no relation to what users
actually want to put in diff.primer, or what even makes sense there. For
example:
- "-u" is not supported, but that is something I would expect people
to want to use (in fact, it is the _only_ thing supported by
GIT_DIFF_OPTS)
- "--follow" is supported, but it makes no sense to say "my diffs
should always use --follow" (and yes, this is a result of it
confusingly being part of diff opts, when it is really about
revision processing).
- "--exit-code" and "--quiet" are supported, but why would you want to
have them on all the time as defaults?
- "--relative" is supported, but "--relative=" is not.
So I think the behavior is quite confusing for potential users. I don't
mind as much that some options don't make sense (like --exit-code and
--follow), because people who put them in diff.primer get what they
deserve. But not supporting some options that people do want to use is
Right, but you can see that I think that should change. :) I think there
are some quote-parsing routines that we use for config and for aliases
Right, I am calling into question whether we want "--primer" at all.
That is, if you think of it as just "prepend these command line options"
we can get the same thing with ...Sir I appreciate the intention, as I interpret it, that it's always better to accomplish something without adding new vocabulary. I'd much rather avoid adding new vocab if possible. If I'm missing something, I apologize ahead of time, but let me describe the problem I see. Let's take the context size setting as an example, i.e. -U<n> or --unified=<n>. Default is 3. Let's say someone defines diff.primer = -U6. Now, without --no-primer, how does a program say "use the default value for context." Aren't there options for which no inverse counterpart exists? Is there command-line syntax to disable all whitespace ignore options, e.g. to disable -b? If not then we need --no-primer. --
Good point. I think there are actually two separate problems you
describe:
1. some default behaviors which can be changed via an option have no
option that represents the default. This is your "-b" example (and
there are others, like "-a", "--full-index", etc).
Generally I think we try to allow boolean options to be specified
in either positive or negative ways. Our parse-options library even
automatically handles --no-$foo by default for any boolean option
(as long as it has a corresponding long option).
So I consider the lack of --no-ignore-space-change to be a failing
of git, but one that we can correct. Either manually or by moving
the diff code to parse-options.
2. For options which take a value, there is no way to say "pretend I
didn't specify a value at all".
Actually, that is not entirely true. parse-options handles
"--foo=bar --no-foo" as if "--foo" was never specified at all.
But there are still two failure cases:
- as above, the diff options are not handled by parse-options
- not all value options are quite as straightforward. Some
options run callbacks that do things which take specialized
code to undo.
Both are fixable. But I have to wonder if it is really all that
useful to say "use the default for this option". Either you don't
care what the value is, in which case you can take the default
given by your primer value. Or you do care, in which case wouldn't
you want to be setting the value yourself?
So I think doing it right is a bit more work in the long run, but the
extra work is generally improving git.
All that being said, though, I still think we can do the equivalent of
--no-primer. The trick to avoiding multiple passes is for the option to
exist outside of the set of primer'd options. I can think of two places:
1. an environment variable, GIT_PRIMER. E.g., "GIT_PRIMER=0 git diff".
This strikes me as ...Hi, FWIW I cannot stop to hate the term "primer" for this thing. A "primer" has been explained to me as being a short leaflet that introduces you to the basic principles of something. Or as a sequence of RNA (or in rare cases, not RNA but something similar) that starts replication of DNA. IOW a primer is something that _has_ to come before something else. Without which the latter thing does not work. Not a set of defaults, which this here patch is all about. Ciao, Dscho --
Yes, I also hate the name. I have been using it in the discussion for lack of a better term, but I really dislike it, as well. I think "diff.defaults" makes sense, but maybe something that suggests it is about command line options would be better. -Peff --
Hi, Why not spell it out? diff.defaultOptions Ciao, Dscho --
Yes, that is much better than my suggestion. -Peff --
OK in v3 I'll use "defaultOptions". Now what do we call the switches? --
Peff,
I like the idea of using parse-options to handle diff options and I too would
like all switches negatable. I will come back to the other ideas you mention if
necessary. You laid it all out nicely.
Assuming we can do away with the switches --[no-]default-options, thereby
hopefully eliminating the need to accumulate options in any kind of fancy way,
certainly the right place to "walk" is in diff_setup(). But diff_setup() must
still ascertain at least one runtime fact: whether or not we are running one of
the commands that respects default options {diff, log, show}. Is there an
elegant way to ascertain that fact from inside diff_setup()? How do you
recommend? (BTW I believe my design achieves this elegantly).
-- Keith
--
If you are interested in parse-optification of diff options, search the archive for messages from Pierre Habouzit on the topic in the last 6 months or so. It was discussed at the GitTogether, and he had some You can impact the argument parsing by touching the diffopt struct before doing the parsing. I.e., something like: /* we generally get diff options from a rev_info structure */ struct rev_info rev; /* initialize the structures */ init_revisions(&rev, prefix); /* now set any preferences specific to this command */ DIFF_OPT_SET(&rev.diffopt, ALLOW_DEFAULT_CONFIG); /* and then actually parse */ setup_revisions(argc, argv, rev, "HEAD"); See for example how cmd_whatchanged does it in builtin-log.c. Any porcelains which wanted this feature would opt in to it. -Peff --
Peff, I'm replying to http://permalink.gmane.org/gmane.comp.version-control.git/108158 In order to answer your questions as convincingly as possible, I wrote up a one-page PDF document, downloadable here: http://preview.tinyurl.com/c769dd You will see I clarified my arguments, and I found very compelling reasons for my design. Also, BTW, v3 supports all diff options under the sun, instead of a limited subset. That addresses your primary complaint WRT functionality. Please take a look at the PDF and I hope you agree. -- Keith --
Thanks for following up on this. And I appreciate that it is probably
nicer to compose in whatever you used to make this PDF due to enhanced
typography and linked footnotes, but posting a link to a PDF has a few
downsides:
- the text in the PDF does not become part of the list archive; if
your tinyurl or your hosted file ever goes away, the content of
your message is permanently lost
- it makes it very difficult for readers to reply inline to your
comments
So please just send text emails in the future.
This time, however, I converted your PDF to text so I could reply.
I think there is a step 1.5, where callers can record their own
"default" intentions -- things that this particular caller will default
to, but which users can override via command-line options. E.g., "git
log" tweaks several options in cmd_log_init, such as turning on
More or less. I think in our past discussion, it came about that there
are some things the user cannot say, like undoing certain options. This
could be a problem if a caller defaults options to something un-doable.
For example, I don't think there is a way to turn off OPT_RECURSIVE in
I don't agree that it is hopelessly flawed. It requires a new call at
each callsite to say "I have set up my defaults, now take the user
defaults from the config, before I proceed to step 2 and parse the
user intentions". Which sounds awful to add a new call at each site,
but I am not sure that is not necessary anyway.
I don't know that all callsites will _want_ to respect such a config
option, especially not plumbing. So any callsite is going to have to opt
I think I suggested last time that the idea of whether or not to perform
the pre-load doesn't _have_ to come from the same set of user intention.
That is, in the call
git [git-options] diff [diff-options]
we actually parse [git-options] and [diff-options] at different times.
Pre-load intention can go into [git-options], which avoids this
Because I outlined it ...Peff, Thank you for this extremely thoughtful reply. First, I want to ease concern over the point about "intent-to-change". BTW, everything I describe here is already implemented in v3. Only the bit flag fields need special infrastructure! IOW, the macros are only necessary for the bit flags. For numeric data or pointer data, there's no need to keep extra state, and there's no need for callsites to change from direct assignment. Only for bit flags, we need an extra bit to remember whether the value is pristine or not. For all other data: (a) numeric data (integers, chars, and floats): define magic value(s) that represent pristineness. Initialize all fields to PRISTINE. Later, if a field has any value other than PRISTINE, we know there was intent-to-change. (b) pointer data: NULL is the pristine value. Any value other than NULL means As of 628d5c2, all callsites that set bit flags are already updated to use the macros. As mentioned above, all other locations can keep on keepin' on using Yes, in the future, someone could write naughty code that sets a bit flag directly rather than using one of the macros. In C, we probably can't make that impossible. But generally speaking we can't protect against all forms of gross negligence. In order to commit his crime, this hypothetical programmer must ignore the fact that these bits are never set directly, anywhere in the code, period. A competent programmer would, after deciding that he needs to set a bit, look at other spots where such bits are set, and mimic. I think the normal patch auditing process this community follows would raise alarms on patches coming from negligent programmers (there are always tell-tale signs). And, in the event that, nevertheless, Junio applies a bit-flag-direct-assignment patch, it will result in a bug of precisely the following form: an explicitly-given command-line option to a porcelain command fails to override a default option. It will be noticed and fixed. ...
Good point. Though we will need to make sure that existing code is never
looking at PRISTINE values, which aren't likely to make much sense (I
suspect most will be INT_MAX or -1, as 0 is a reasonable value for many
of the options). This should be easy for most code, since the flattening
will get rid of PRISTINE. But remember that there are pieces of code
that do something like:
if (some_diff_option_is_set)
set_some_other_related_diff_option;
I think you can safely ignore this complaint. I was worried we would
need something like:
DIFF_SET(&opt, stat_name_width, 10);
It is much easier to mistakenly write this as
opt.state_name_width = 10;
than it is to accidentally do a bit-set when there is a DIFF_OPT_SET
macro. That is, I think most people _want_ to use DIFF_OPT_SET because
it is easier to read and less typing.
So I saw this as a problem more for non-bit options, but you have
Yes, please. It is much better to be talking about actual code than
hypotheticals.
-Peff
--
Improve porcelain diff's accommodation of user preference by allowing any
command-line option to (a) persist over all invocations and (b) stay consistent
over multiple tools (e.g. command-line and gui). The approach taken here is
good because it delivers the consistency a user expects without breaking any
plumbing. It works by allowing the user, via git-config, to specify arbitrary
options to pass to porcelain diff on every invocation, including internal
invocations from other programs, e.g. git-gui.
Introduce diff command-line options --default-options and --no-default-options.
Affect only porcelain diff: we suppress default options for plumbing
diff-{files,index,tree}, format-patch, and all other commands unless explicitly
requested using --default-options (opt-in).
Signed-off-by: Keith Cascio <keith@cs.ucla.edu>
---
Please notice v3 supports all diff options (improvement over v2).
This is a RFC. I omitted the documentation and test portions for now.
-- Keith
diff.h | 17 ++++++--
diff.c | 112 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
builtin-diff.c | 1 +
builtin-log.c | 1 +
4 files changed, 125 insertions(+), 6 deletions(-)
diff --git a/diff.h b/diff.h
index 6616877..66f1518 100644
--- a/diff.h
+++ b/diff.h
@@ -66,12 +66,17 @@ typedef void (*diff_format_fn_t)(struct diff_queue_struct *q,
#define DIFF_OPT_DIRSTAT_CUMULATIVE (1 << 19)
#define DIFF_OPT_DIRSTAT_BY_FILE (1 << 20)
#define DIFF_OPT_ALLOW_TEXTCONV (1 << 21)
+#define DIFF_OPT_ALLOW_DEFAULT_OPTIONS (1 << 22)
#define DIFF_OPT_TST(opts, flag) ((opts)->flags & DIFF_OPT_##flag)
-#define DIFF_OPT_SET(opts, flag) ((opts)->flags |= DIFF_OPT_##flag)
+#define DIFF_OPT_SET(opts, flag) ((opts)->flags |= DIFF_OPT_##flag),\
+ ((opts)->mask |= DIFF_OPT_##flag)
-#define DIFF_OPT_CLR(opts, flag) ((opts)->flags &= ~DIFF_OPT_##flag)
+#define DIFF_OPT_CLR(opts, flag) ((opts)->flags &= ...The idea is from Keith Cascio. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> --- I do not particularly like what this patch does, but I like the non-intrusiveness and conciseness of it. Documentation/config.txt | 4 ++++ Documentation/git-diff.txt | 3 +++ builtin-diff.c | 1 + builtin-log.c | 4 ++++ diff.c | 25 +++++++++++++++++++++++++ diff.h | 2 ++ t/t4037-diff-default-options.sh | 19 +++++++++++++++++++ 7 files changed, 58 insertions(+), 0 deletions(-) create mode 100755 t/t4037-diff-default-options.sh diff --git a/Documentation/config.txt b/Documentation/config.txt index 7506755..4913bd6 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -625,6 +625,10 @@ diff.autorefreshindex:: affects only 'git-diff' Porcelain, and not lower level 'diff' commands, such as 'git-diff-files'. +diff.defaultoptions: + The value of this option will be prepended to the command line + options of the porcelains showing diffs. + diff.external:: If this config variable is set, diff generation is not performed using the internal diff machinery, but using the diff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt index a2f192f..7025717 100644 --- a/Documentation/git-diff.txt +++ b/Documentation/git-diff.txt @@ -74,6 +74,9 @@ and the range notations ("<commit>..<commit>" and "<commit>\...<commit>") do not mean a range as defined in the "SPECIFYING RANGES" section in linkgit:git-rev-parse[1]. +Default options can be set via the config variable +`diff.defaultOptions`. + OPTIONS ------- :git-diff: 1 diff --git a/builtin-diff.c b/builtin-diff.c index d75d69b..d9a6e7d 100644 --- a/builtin-diff.c +++ b/builtin-diff.c @@ -296,6 +296,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix) if (nongit) die("Not a git ...
Johannes,
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
--
Hi, 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. Of course, to keep things simple, this has to be a separate patch. Ciao, Dscho --
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 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 --
Hi, So what? We parse the config a gazillion times, and there we have to access the _disk_. Ciao, Dscho --
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:
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.
2. You could loosen (2) above by assuming that all options are
idempotent. I don't know whether this is the case (I think it
isn't for all git options, but a cursory look shows that it may be
for diff options).
-Peff
--
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@gmx.de>
---
I go with 1)
builtin-diff.c | 2 +-
builtin-log.c | 8 ++++----
diff.c | 29 ++++++++++++++++++++++-------
diff.h | 2 +-
t/t4037-diff-default-options.sh | 5 +++++
5 files changed, 33 insertions(+), 13 deletions(-)
diff --git a/builtin-diff.c b/builtin-diff.c
index d9a6e7d..8da0052 100644
--- a/builtin-diff.c
+++ b/builtin-diff.c
@@ -296,7 +296,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
if (nongit)
die("Not a git repository");
- parse_default_diff_options(&rev.diffopt);
+ argc = parse_default_diff_options(argc, argv, &rev.diffopt);
argc = setup_revisions(argc, argv, &rev, NULL);
if (!rev.diffopt.output_format) {
rev.diffopt.output_format = DIFF_FORMAT_PATCH;
diff --git a/builtin-log.c b/builtin-log.c
index e926774..2f36537 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -243,7 +243,7 @@ int cmd_whatchanged(int argc, const char **argv, const char *prefix)
init_revisions(&rev, prefix);
rev.diff = 1;
rev.simplify_history = 0;
- parse_default_diff_options(&rev.diffopt);
+ argc = parse_default_diff_options(argc, argv, &rev.diffopt);
cmd_log_init(argc, argv, prefix, &rev);
if (!rev.diffopt.output_format)
rev.diffopt.output_format = DIFF_FORMAT_RAW;
@@ -315,7 +315,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)
rev.always_show_header = 1;
rev.ignore_merges = 0;
rev.no_walk = 1;
- parse_default_diff_options(&rev.diffopt);
+ argc = parse_default_diff_options(argc, argv, &rev.diffopt);
cmd_log_init(argc, argv, prefix, &rev);
count = rev.pending.nr;
@@ ...This feels very hack-ish to me, but perhaps this is a case of "perfect is the enemy of the good". -Peff --
Hi, 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 --
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.
2. If it does happen, it is likely to be in a transition to parseopt,
which would fall under the case mentioned above.
I think the biggest danger is that it is a potential bomb for somebody
to add a new revision option which takes an arbitrary string. They
would probably need to keep it as "--option=<arg>" only.
-Peff
--
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? --
Hi, That would not work if you use an alias: $ git config alias.grmpfl log --stat $ git grmpfl --no-defaults Ciao, Dscho --
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 --
Hi, 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 --
Dscho,
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.
-- Keith
--
Hi Keith, 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 --
Dscho,
--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.
A reasonable worry! But blind paranoia is paralyzing. Peff expressed some
specific concerns which he and I addressed: (1) whether I'd investigated all
callsites for possible problems (yes I did), (2) whether we'd have to switch
every callsite to a macro, rather than direct assignment (no we don't).
Outside of diff.h/diff.c, my v3 deletes no lines and adds only two. That
doesn't really seem "intrusive" to me. By comparison, your patch adds at least
ten lines outside of diff.h/diff.c. I'd rather call my patch "innovative".
Possible?
-- Keith
--
Hi. 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 That is _such_ a red herring. Ciao, Dscho --
Johannes, 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 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 --
Johannes, Hi. Was my last message understandable or should I add more explanation? http://comments.gmane.org/gmane.comp.version-control.git/115506 -- Keith --
Hi, Sorry, I am just short on time for that hobby called Git. Ciao, Dscho --
Your point made sense to me, and is the expected difference between the two approaches (as we discussed before). I'm sorry I haven't had a chance to review your patch in detail. It is pleasantly much shorter than previous iterations, but I wanted to very carefully check a few of the diff callsites to make sure they work properly (e.g., some of the cases I laid out earlier in the thread). I have a lot of regular life responsibilities right now, but I'll try take a close look this weekend. -Peff --
I still think that naming this configuration variable (or
configuration section in the reverse primer.diff) 'primer' is not a
good idea, because it is quite obscure and not well known word. In
computer related context I have seen it only when talking about
introductory / novice / basic level documentation ('primer (textbook)'
meaning). Git user, who might be not a native English speaker,
shouldn't have to look up in dictionary what it is about...
I think that 'defaults' or 'options' would be a much better name.
--
Jakub Narebski
Poland
ShadeHawk on #git
--
Point well taken. Let's consider "primer" the working name. The final name
will be the last finishing touch once (and if) we achieve consensus on the
functionality. Ultimately the name is best chosen after the functionality is
certain. IOW, open to discussion.
-- Keith
--
