[PATCH v2 1/2] Introduce config variable "diff.primer"

[PATCH v2 1/2] Introduce config variable "diff.primer"

[Keith Cascio]

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 --color --color-words --cumulative --dirstat-by-file
+--exit-code --ext-diff --find-copies-harder --follow --full-index
+--ignore-all-space --ignore-space-at-eol --ignore-space-change
+--ignore-submodules --no-color --no-ext-diff --no-textconv --patience -q
+--quiet -R -r --relative -t --text --textconv -w`
+
 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/diff-options.txt b/Documentation/diff-options.txt
index 813a7b1..f422055 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -254,5 +254,15 @@ override configuration settings.
 --no-prefix::
 	Do not show any source or destination prefix.
 
+--no-primer::
+	Ignore default options specified in '.git/config', i.e.
+	those that were set using a command like
+	`git config diff.primer "--patience --color --ignore-space-at-eol --exit-code"`
+
+--primer::
+	Opt-in for default options specified in '.git/config'.  This option is
+	most often used with the three plumbing commands diff-{files,index,tree}.
+	These commands normally suppress default options.
+
 For more detailed explanation on these common options, see also
 linkgit:gitdiffcore[7].
diff --git a/builtin-diff.c b/builtin-diff.c
index d75d69b..b3c3e87 100644
--- a/builtin-diff.c
+++ b/builtin-diff.c
@@ -284,6 +284,8 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 
 	init_revisions(&rev, prefix);
 
+	DIFF_OPT_SET(&rev.diffopt, PRIMER);
+
 	/* If this is a no-index diff, just run it and exit there. */
 	diff_no_index(&rev, argc, argv, nongit, prefix);
 
diff --git a/diff.c b/diff.c
index a5a540f..32455c3 100644
--- a/diff.c
+++ b/diff.c
@@ -26,6 +26,8 @@ static int diff_suppress_blank_empty;
 int diff_use_color_default = -1;
 static const char *diff_word_regex_cfg;
 static const char *external_diff_cmd_cfg;
+static const char *diff_primer;
+static struct diff_options *primer;
 int diff_auto_refresh_index = 1;
 static int diff_mnemonic_prefix;
 
@@ -106,6 +108,8 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)
 		diff_rename_limit_default = git_config_int(var, value);
 		return 0;
 	}
+	if (!strcmp(var, "diff.primer"))
+		return git_config_string(&diff_primer, var, value);
 
 	switch (userdiff_config(var, value)) {
 		case 0: break;
@@ -2316,6 +2320,45 @@ static void run_checkdiff(struct diff_filepair *p, struct diff_options *o)
 	builtin_checkdiff(name, other, attr_path, p->one, p->two, o);
 }
 
+static const char blank[] = " \t\r\n";
+
+void parse_diff_primer(struct diff_options *options)
+{
+	char *str1, *token, *saveptr;
+	int len;
+
+	if ((! diff_primer) || ((len = (strlen(diff_primer)+1)) < 3))
+		return;
+
+	token = str1 = strncpy((char*) malloc(len), diff_primer, len);
+	if ((saveptr = strpbrk(token += strspn(token, blank), blank)))
+		*(saveptr++) = '\0';
+	while (token) {
+		if (*token == '-')
+			diff_opt_parse(options, (const char **) &token, -1);
+		if ((token = saveptr))
+			if ((saveptr = strpbrk(token += strspn(token, blank), blank)))
+				*(saveptr++) = '\0';
+	}
+
+	free( str1 );
+}
+
+struct diff_options* flatten_diff_options(struct diff_options *master, struct diff_options *slave)
+{
+	unsigned x0 = master->flags, x1 = master->mask, x2 = slave->flags, x3 = slave->mask;
+	long w = master->xdl_opts, x = master->xdl_mask, y = slave->xdl_opts, z = slave->xdl_mask;
+
+	//minimized by Quine-McCluskey
+	master->flags = (~x1&x2&x3)|(x0&~x3)|(x0&x1);
+	master->mask = x1|x3;
+
+	master->xdl_opts = (~x&y&z)|(w&~z)|(w&x);
+	master->xdl_mask = x|z;
+
+	return master;
+}
+
 void diff_setup(struct diff_options *options)
 {
 	memset(options, 0, sizeof(*options));
@@ -2326,14 +2369,15 @@ void diff_setup(struct diff_options *options)
 	options->break_opt = -1;
 	options->rename_limit = -1;
 	options->dirstat_percent = 3;
-	DIFF_OPT_CLR(options, DIRSTAT_CUMULATIVE);
+	if (DIFF_OPT_TST(options, DIRSTAT_CUMULATIVE))
+		DIFF_OPT_CLR(options, DIRSTAT_CUMULATIVE);
 	options->context = 3;
 
 	options->change = diff_change;
 	options->add_remove = diff_addremove;
 	if (diff_use_color_default > 0)
 		DIFF_OPT_SET(options, COLOR_DIFF);
-	else
+	else if (DIFF_OPT_TST(options, COLOR_DIFF))
 		DIFF_OPT_CLR(options, COLOR_DIFF);
 	options->detect_rename = diff_detect_rename_default;
 
@@ -2423,6 +2467,14 @@ int diff_setup_done(struct diff_options *options)
 		DIFF_OPT_SET(options, EXIT_WITH_STATUS);
 	}
 
+	if (DIFF_OPT_TST(options, PRIMER)) {
+		if (! primer) {
+			diff_setup(primer = (struct diff_options *) malloc(sizeof(struct diff_options)));
+			parse_diff_primer(primer);
+		}
+		flatten_diff_options(options, primer);
+	}
+
 	return 0;
 }
 
@@ -2570,13 +2622,13 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 
 	/* xdiff options */
 	else if (!strcmp(arg, "-w") || !strcmp(arg, "--ignore-all-space"))
-		options->xdl_opts |= XDF_IGNORE_WHITESPACE;
+		DIFF_XDL_SET(options, IGNORE_WHITESPACE);
 	else if (!strcmp(arg, "-b") || !strcmp(arg, "--ignore-space-change"))
-		options->xdl_opts |= XDF_IGNORE_WHITESPACE_CHANGE;
+		DIFF_XDL_SET(options, IGNORE_WHITESPACE_CHANGE);
 	else if (!strcmp(arg, "--ignore-space-at-eol"))
-		options->xdl_opts |= XDF_IGNORE_WHITESPACE_AT_EOL;
+		DIFF_XDL_SET(options, IGNORE_WHITESPACE_AT_EOL);
 	else if (!strcmp(arg, "--patience"))
-		options->xdl_opts |= XDF_PATIENCE_DIFF;
+		DIFF_XDL_SET(options, PATIENCE_DIFF);
 
 	/* flags options */
 	else if (!strcmp(arg, "--binary")) {
@@ -2597,10 +2649,13 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 		DIFF_OPT_SET(options, COLOR_DIFF);
 	else if (!strcmp(arg, "--no-color"))
 		DIFF_OPT_CLR(options, COLOR_DIFF);
-	else if (!strcmp(arg, "--color-words"))
-		options->flags |= DIFF_OPT_COLOR_DIFF | DIFF_OPT_COLOR_DIFF_WORDS;
+	else if (!strcmp(arg, "--color-words")) {
+		DIFF_OPT_SET(options, COLOR_DIFF);
+		DIFF_OPT_SET(options, COLOR_DIFF_WORDS);
+	}
 	else if (!prefixcmp(arg, "--color-words=")) {
-		options->flags |= DIFF_OPT_COLOR_DIFF | DIFF_OPT_COLOR_DIFF_WORDS;
+		DIFF_OPT_SET(options, COLOR_DIFF);
+		DIFF_OPT_SET(options, COLOR_DIFF_WORDS);
 		options->word_regex = arg + 14;
 	}
 	else if (!strcmp(arg, "--exit-code"))
@@ -2617,6 +2672,10 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 		DIFF_OPT_CLR(options, ALLOW_TEXTCONV);
 	else if (!strcmp(arg, "--ignore-submodules"))
 		DIFF_OPT_SET(options, IGNORE_SUBMODULES);
+	else if (!strcmp(arg, "--primer"))
+		DIFF_OPT_SET(options, PRIMER);
+	else if (!strcmp(arg, "--no-primer"))
+		DIFF_OPT_CLR(options, PRIMER);
 
 	/* misc options */
 	else if (!strcmp(arg, "-z"))
diff --git a/diff.h b/diff.h
index 23cd90c..7f11b12 100644
--- a/diff.h
+++ b/diff.h
@@ -66,9 +66,15 @@ 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_TST(opts, flag)    ((opts)->flags & DIFF_OPT_##flag)
-#define DIFF_OPT_SET(opts, flag)    ((opts)->flags |= DIFF_OPT_##flag)
-#define DIFF_OPT_CLR(opts, flag)    ((opts)->flags &= ~DIFF_OPT_##flag)
+#define DIFF_OPT_PRIMER              (1 << 22)
+#define DIFF_OPT_TST(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), ((opts)->mask |= DIFF_OPT_##flag)
+#define DIFF_OPT_DRT(opts, flag)    ((opts)->mask  &   DIFF_OPT_##flag)
+#define DIFF_XDL_TST(opts, flag)    ((opts)->xdl_opts &   XDF_##flag)
+#define DIFF_XDL_SET(opts, flag)    ((opts)->xdl_opts |=  XDF_##flag), ((opts)->xdl_mask |= XDF_##flag)
+#define DIFF_XDL_CLR(opts, flag)    ((opts)->xdl_opts &= ~XDF_##flag), ((opts)->xdl_mask |= XDF_##flag)
+#define DIFF_XDL_DRT(opts, flag)    ((opts)->xdl_mask &   XDF_##flag)
 
 struct diff_options {
 	const char *filter;
@@ -77,6 +83,7 @@ struct diff_options {
 	const char *single_follow;
 	const char *a_prefix, *b_prefix;
 	unsigned flags;
+	unsigned mask;
 	int context;
 	int interhunkcontext;
 	int break_opt;
@@ -95,6 +102,7 @@ struct diff_options {
 	int prefix_length;
 	const char *stat_sep;
 	long xdl_opts;
+	long xdl_mask;
 
 	int stat_width;
 	int stat_name_width;
diff --git a/gitk-git/gitk b/gitk-git/gitk
index dc2a439..b67bbaa 100644
--- a/gitk-git/gitk
+++ b/gitk-git/gitk
@@ -4259,7 +4259,7 @@ proc do_file_hl {serial} {
 	# must be "containing:", i.e. we're searching commit info
 	return
     }
-    set cmd [concat | git diff-tree -r -s --stdin $gdtargs]
+    set cmd [concat | git diff-tree --primer --no-color -r -s --stdin $gdtargs]
     set filehighlight [open $cmd r+]
     fconfigure $filehighlight -blocking 0
     filerun $filehighlight readfhighlight
@@ -4753,7 +4753,7 @@ proc dodiffindex {} {
 
     if {!$showlocalchanges || !$isworktree} return
     incr lserial
-    set cmd "|git diff-index --cached HEAD"
+    set cmd "|git diff-index --primer --no-color --cached HEAD"
     if {$vfilelimit($curview) ne {}} {
 	set cmd [concat $cmd -- $vfilelimit($curview)]
     }
@@ -4782,7 +4782,7 @@ proc readdiffindex {fd serial inst} {
     }
 
     # now see if there are any local changes not checked in to the index
-    set cmd "|git diff-files"
+    set cmd "|git diff-files --primer --no-color"
     if {$vfilelimit($curview) ne {}} {
 	set cmd [concat $cmd -- $vfilelimit($curview)]
     }
@@ -7068,7 +7068,7 @@ proc diffcmd {ids flags} {
     if {$i >= 0} {
 	if {[llength $ids] > 1 && $j < 0} {
 	    # comparing working directory with some specific revision
-	    set cmd [concat | git diff-index $flags]
+	    set cmd [concat | git diff-index --primer --no-color $flags]
 	    if {$i == 0} {
 		lappend cmd -R [lindex $ids 1]
 	    } else {
@@ -7076,13 +7076,13 @@ proc diffcmd {ids flags} {
 	    }
 	} else {
 	    # comparing working directory with index
-	    set cmd [concat | git diff-files $flags]
+	    set cmd [concat | git diff-files --primer --no-color $flags]
 	    if {$j == 1} {
 		lappend cmd -R
 	    }
 	}
     } elseif {$j >= 0} {
-	set cmd [concat | git diff-index --cached $flags]
+	set cmd [concat | git diff-index --primer --no-color --cached $flags]
 	if {[llength $ids] > 1} {
 	    # comparing index with specific revision
 	    if {$i == 0} {
@@ -7095,7 +7095,7 @@ proc diffcmd {ids flags} {
 	    lappend cmd HEAD
 	}
     } else {
-	set cmd [concat | git diff-tree -r $flags $ids]
+	set cmd [concat | git diff-tree --primer --no-color -r $flags $ids]
     }
     return $cmd
 }
@@ -10657,7 +10657,7 @@ if {[catch {package require Tk 8.4} err]} {
 }
 
 # defaults...
-set wrcomcmd "git diff-tree --stdin -p --pretty"
+set wrcomcmd "git diff-tree --primer --no-color --stdin -p --pretty"
 
 set gitencoding {}
 catch {
-- 
1.6.1

[PATCH v2 2/2] Test functionality of new config variable "diff.primer"

[Keith Cascio]

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 by diff.primer' 'test_cmp expect_noprimer out'
+git diff-index -p HEAD > out
+test_expect_success 'ensure diff-index unaffected by diff.primer' 'test_cmp expect_noprimer out'
+
+cat << EOF > expect_primer
+diff --git a/x b/x
+index d99af23..8b32fb5 100644
+EOF
+git diff > out
+test_expect_success 'test git-diff with diff.primer = -w' 'test_cmp expect_primer out'
+git diff-files -p --primer > out
+test_expect_success 'ensure diff-files honors --primer' 'test_cmp expect_primer out'
+git diff-index -p --primer HEAD > out
+test_expect_success 'ensure diff-index honors --primer' 'test_cmp expect_primer out'
+
+git add x
+git commit -m 'whitespace changes' >/dev/null 2>&1
+
+git config diff.primer '-w --color'
+
+tr 'Q_' '\015 ' << EOF > expect
+Subject: [PATCH] whitespace changes
+
+---
+ x |   10 +++++-----
+ 1 files changed, 5 insertions(+), 5 deletions(-)
+
+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 format-patch --stdout HEAD^..HEAD 2>&1 | sed -re '1,3d;$d' | sed -re '$d' > out
+test_expect_success 'ensure format-patch unaffected by diff.primer' 'test_cmp expect out'
+
+git add x
+git commit -m '2.0' >/dev/null 2>&1
+
+git config diff.primer '-w'
+
+git diff-tree -p -r          HEAD^ HEAD > out
+test_expect_success 'ensure diff-tree unaffected by diff.primer' 'test_cmp expect_noprimer out'
+git diff-tree -p -r --primer HEAD^ HEAD > out
+test_expect_success 'ensure diff-tree honors --primer' 'test_cmp expect_primer out'
+
+test_done
+
-- 
1.6.1

[PATCH v2 0/2] Introduce config variable "diff.primer"

[Keith Cascio]

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

[PATCH v2 0/2] Introduce config variable "diff.primer"

[Keith Cascio]

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(-)

Re: [PATCH v2 1/2] Introduce config variable "diff.primer"

[Jeff King]

On Mon, Feb 02, 2009 at 10:20:54AM -0800, Keith Cascio wrote:

> Introduce config variable "diff.primer".

You don't need to repeat the subject in the body.

> 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.

Paragraph breaks might have made this a bit easier to read.

> +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 --color --color-words --cumulative --dirstat-by-file
> +--exit-code --ext-diff --find-copies-harder --follow --full-index
> +--ignore-all-space --ignore-space-at-eol --ignore-space-change
> +--ignore-submodules --no-color --no-ext-diff --no-textconv --patience -q
> +--quiet -R -r --relative -t --text --textconv -w`

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.

How was this list chosen?

> +--no-primer::
> +	Ignore default options specified in '.git/config', i.e.
> +	those that were set using a command like
> +	`git config diff.primer "--patience --color --ignore-space-at-eol --exit-code"`
> +
> +--primer::
> +	Opt-in for default options specified in '.git/config'.  This option is
> +	most often used with the three plumbing commands diff-{files,index,tree}.
> +	These commands normally suppress default options.
> +

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
~/.gitconfig, a system-wide gitconfig, or .git/config.

> @@ -284,6 +284,8 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
> [...]
> +	DIFF_OPT_SET(&rev.diffopt, PRIMER);

Probably ALLOW_PRIMER is a more sensible name, to match ALLOW_EXTERNAL
and ALLOW_TEXTCONV.

> +static const char blank[] = " \t\r\n";
> +
> +void parse_diff_primer(struct diff_options *options)
> +{
> +	char *str1, *token, *saveptr;
> +	int len;
> +
> +	if ((! diff_primer) || ((len = (strlen(diff_primer)+1)) < 3))
> +		return;
> +
> +	token = str1 = strncpy((char*) malloc(len), diff_primer, len);
> +	if ((saveptr = strpbrk(token += strspn(token, blank), blank)))
> +		*(saveptr++) = '\0';
> +	while (token) {
> +		if (*token == '-')
> +			diff_opt_parse(options, (const char **) &token, -1);
> +		if ((token = saveptr))
> +			if ((saveptr = strpbrk(token += strspn(token, blank), blank)))
> +				*(saveptr++) = '\0';
> +	}
> +
> +	free( str1 );
> +}

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
apply.

> +struct diff_options* flatten_diff_options(struct diff_options *master, struct diff_options *slave)
> +{
> +	unsigned x0 = master->flags, x1 = master->mask, x2 = slave->flags, x3 = slave->mask;
> +	long w = master->xdl_opts, x = master->xdl_mask, y = slave->xdl_opts, z = slave->xdl_mask;
> +

Style: long lines.

> +	//minimized by Quine-McCluskey

Style: no C99/C++ comments.

> +	master->flags = (~x1&x2&x3)|(x0&~x3)|(x0&x1);

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) |
     /* (3) */ (master->flags & master->mask);

> @@ -2326,14 +2369,15 @@ void diff_setup(struct diff_options *options)
>  	options->break_opt = -1;
>  	options->rename_limit = -1;
>  	options->dirstat_percent = 3;
> -	DIFF_OPT_CLR(options, DIRSTAT_CUMULATIVE);
> +	if (DIFF_OPT_TST(options, DIRSTAT_CUMULATIVE))
> +		DIFF_OPT_CLR(options, DIRSTAT_CUMULATIVE);

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 surprised to find other spots outside of
diff.c where the options are munged by various programs. Did you audit
for all such spots?

> +	if (DIFF_OPT_TST(options, PRIMER)) {
> +		if (! primer) {
> +			diff_setup(primer = (struct diff_options *) malloc(sizeof(struct diff_options)));

First, don't use malloc. Use the xmalloc wrapper that will try to free
pack memory and/or die if it fails.

Secondly, don't cast the result of malloc. At best it is pointless and
verbose, and at worst it can hide errors caused by a missing function
declaration.

>  	/* xdiff options */
>  	else if (!strcmp(arg, "-w") || !strcmp(arg, "--ignore-all-space"))
> -		options->xdl_opts |= XDF_IGNORE_WHITESPACE;
> +		DIFF_XDL_SET(options, IGNORE_WHITESPACE);

It often makes the patch easier to review if you split changes like this
out into a separate patch. Then your series is

  1/2: use DIFF_XDL_SET instead of raw bit-masking

       This is a cleanup in preparation for option-setting doing
       something more complex than just setting a bit-mask. The code
       should behave exactly the same.

  2/2: primer patch

       ... DIFF_XDL_SET tracks not only the set options, but which ones
       were set explicitly via a mask ...

Then we can all see pretty easily that patch 1/2 doesn't change the
behavior, and each patch is a much smaller, succint chunk to review.

> -	else if (!strcmp(arg, "--color-words"))
> -		options->flags |= DIFF_OPT_COLOR_DIFF | DIFF_OPT_COLOR_DIFF_WORDS;
> +	else if (!strcmp(arg, "--color-words")) {
> +		DIFF_OPT_SET(options, COLOR_DIFF);
> +		DIFF_OPT_SET(options, COLOR_DIFF_WORDS);
> +	}

Ditto here with DIFF_OPT_SET.

> +#define DIFF_OPT_TST(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), ((opts)->mask |= DIFF_OPT_##flag)
> +#define DIFF_OPT_DRT(opts, flag)    ((opts)->mask  &   DIFF_OPT_##flag)

OK, I see what it is supposed to do, but what does DRT stand for? Also,
what practical use does it have? I don't see anybody _calling_ it.

> --- a/gitk-git/gitk
> +++ b/gitk-git/gitk
> @@ -4259,7 +4259,7 @@ proc do_file_hl {serial} {
>  	# must be "containing:", i.e. we're searching commit info
>  	return
>      }
> -    set cmd [concat | git diff-tree -r -s --stdin $gdtargs]
> +    set cmd [concat | git diff-tree --primer --no-color -r -s --stdin $gdtargs]

Does gitk really want to respect --primer? Might it not make more sense
for it, as a porcelain, to respect the diff.primer variable itself, and
prepend it to the list of diff args? Then it has the power to veto any
options which it doesn't handle.

Also, any gitk changes should almost certainly be split into a different
patch.



All in all, this was a lot more complicated than I was expecting. Why
isn't the behavior of "diff.primer" simply "pretend as if the options in
diff.primer were prepended to the command line"? That is easy to
explain, and easy to implement (the only trick is that you have to do an
extra pass to find --[no-]primer). Is there some drawback to such a
simple scheme that I am missing?

-Peff

Re: [PATCH v2 1/2] Introduce config variable "diff.primer"

[Keith Cascio]

Peff,
First of all, thanks for the tips!

On Tue, 3 Feb 2009, Jeff King wrote:

> You don't need to repeat the subject in the body.

OK.

> Paragraph breaks might have made this a bit easier to read.

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 
--color.

> > +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 --color --color-words --cumulative --dirstat-by-file
> > +--exit-code --ext-diff --find-copies-harder --follow --full-index
> > +--ignore-all-space --ignore-space-at-eol --ignore-space-change
> > +--ignore-submodules --no-color --no-ext-diff --no-textconv --patience -q
> > +--quiet -R -r --relative -t --text --textconv -w`
> 
> Funny indentation?

The last part is a backtick-quoted list, I think it either must occur on one 
line or as is.

> 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.
> 
> How was this list chosen?

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 
present list.

> 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.

Agree.

> Also, don't mention ".git/config" by name: configuration can come from
> ~/.gitconfig, a system-wide gitconfig, or .git/config.

OK.

> > @@ -284,6 +284,8 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
> > [...]
> > +	DIFF_OPT_SET(&rev.diffopt, PRIMER);
> 
> Probably ALLOW_PRIMER is a more sensible name, to match ALLOW_EXTERNAL
> and ALLOW_TEXTCONV.

Agree.

> > +static const char blank[] = " \t\r\n";
> > +
> > +void parse_diff_primer(struct diff_options *options)
> > +{
> > +	char *str1, *token, *saveptr;
> > +	int len;
> > +
> > +	if ((! diff_primer) || ((len = (strlen(diff_primer)+1)) < 3))
> > +		return;
> > +
> > +	token = str1 = strncpy((char*) malloc(len), diff_primer, len);
> > +	if ((saveptr = strpbrk(token += strspn(token, blank), blank)))
> > +		*(saveptr++) = '\0';
> > +	while (token) {
> > +		if (*token == '-')
> > +			diff_opt_parse(options, (const char **) &token, -1);
> > +		if ((token = saveptr))
> > +			if ((saveptr = strpbrk(token += strspn(token, blank), blank)))
> > +				*(saveptr++) = '\0';
> > +	}
> > +
> > +	free( str1 );
> > +}
> 
> 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 apply.

The current version only supports the flags listed above in 
Documentation/config.txt.

> Style: long lines.

OK.

> Style: no C99/C++ comments.

OK.

> > +	master->flags = (~x1&x2&x3)|(x0&~x3)|(x0&x1);
> 
> 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) |
>      /* (3) */ (master->flags & master->mask);

Your version is much better.

> > @@ -2326,14 +2369,15 @@ void diff_setup(struct diff_options *options)
> >  	options->break_opt = -1;
> >  	options->rename_limit = -1;
> >  	options->dirstat_percent = 3;
> > -	DIFF_OPT_CLR(options, DIRSTAT_CUMULATIVE);
> > +	if (DIFF_OPT_TST(options, DIRSTAT_CUMULATIVE))
> > +		DIFF_OPT_CLR(options, DIRSTAT_CUMULATIVE);
> 
> 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.

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 
future".

> I wouldn't be surprised to find other spots outside of diff.c where the 
> options are munged by various programs. Did you audit for all such spots?

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]' '(flags|xdl_opts).*=.*(DIFF_OPT_|\b)(RECURSIVE|TREE_IN_RECURSIVE|BINARY|TEXT|FULL_INDEX|SILENT_ON_REMOVE|FIND_COPIES_HARDER|FOLLOW_RENAMES|COLOR_DIFF|COLOR_DIFF_WORDS|HAS_CHANGES|QUIET|NO_INDEX|ALLOW_EXTERNAL|EXIT_WITH_STATUS|REVERSE_DIFF|CHECK_FAILED|RELATIVE_NAME|IGNORE_SUBMODULES|DIRSTAT_CUMULATIVE|DIRSTAT_BY_FILE|ALLOW_TEXTCONV|PRIMER|XDF_NEED_MINIMAL|XDF_IGNORE_WHITESPACE|XDF_IGNORE_WHITESPACE_CHANGE|XDF_IGNORE_WHITESPACE_AT_EOL|XDF_PATIENCE_DIFF|XDF_WHITESPACE_FLAGS)\b' . | grep -Ev TST | less -S

merge-tree.c:109:     xpp.flags = XDF_NEED_MINIMAL;
merge-file.c:65:      xpp.flags = XDF_NEED_MINIMAL;
xdiff/xpatience.c:308:        xpp.flags = map->xpp->flags & ~XDF_PATIENCE_DIFF;
builtin-blame.c:41:static int xdl_opts = XDF_NEED_MINIMAL;
combine-diff.c:217:   xpp.flags = XDF_NEED_MINIMAL;
builtin-rerere.c:101: xpp.flags = XDF_NEED_MINIMAL;
diff.c:517:   xpp.flags = XDF_NEED_MINIMAL;
diff.c:1570:          xpp.flags = XDF_NEED_MINIMAL | o->xdl_opts;
diff.c:1658:          xpp.flags = XDF_NEED_MINIMAL | o->xdl_opts;
diff.c:1706:          xpp.flags = XDF_NEED_MINIMAL;
diff.c:3238:          xpp.flags = XDF_NEED_MINIMAL;

> > +	if (DIFF_OPT_TST(options, PRIMER)) {
> > +		if (! primer) {
> > +			diff_setup(primer = (struct diff_options *) malloc(sizeof(struct diff_options)));
> 
> First, don't use malloc. Use the xmalloc wrapper that will try to free pack 
> memory and/or die if it fails.

OK.

> Secondly, don't cast the result of malloc. At best it is pointless and 
> verbose, and at worst it can hide errors caused by a missing function 
> declaration.

OK.

> >  	/* xdiff options */
> >  	else if (!strcmp(arg, "-w") || !strcmp(arg, "--ignore-all-space"))
> > -		options->xdl_opts |= XDF_IGNORE_WHITESPACE;
> > +		DIFF_XDL_SET(options, IGNORE_WHITESPACE);
> 
> It often makes the patch easier to review if you split changes like this
> out into a separate patch. Then your series is
> 
>   1/2: use DIFF_XDL_SET instead of raw bit-masking
> 
>        This is a cleanup in preparation for option-setting doing
>        something more complex than just setting a bit-mask. The code
>        should behave exactly the same.
> 
>   2/2: primer patch
> 
>        ... DIFF_XDL_SET tracks not only the set options, but which ones
>        were set explicitly via a mask ...
> 
> Then we can all see pretty easily that patch 1/2 doesn't change the behavior, 
> and each patch is a much smaller, succint chunk to review.

Agree.

> Ditto here with DIFF_OPT_SET.

OK.

> > +#define DIFF_OPT_TST(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), ((opts)->mask |= DIFF_OPT_##flag)
> > +#define DIFF_OPT_DRT(opts, flag)    ((opts)->mask  &   DIFF_OPT_##flag)
> 
> OK, I see what it is supposed to do, but what does DRT stand for? Also,
> what practical use does it have? I don't see anybody _calling_ it.

DRT means "dirty".  The masks here are dirty bits.  You're right, I never called 
it.  But it seemed so meaningful to me, I felt it was right to include it while 
introducing the concept of dirty bits.

> > --- a/gitk-git/gitk
> > +++ b/gitk-git/gitk
> > @@ -4259,7 +4259,7 @@ proc do_file_hl {serial} {
> >  	# must be "containing:", i.e. we're searching commit info
> >  	return
> >      }
> > -    set cmd [concat | git diff-tree -r -s --stdin $gdtargs]
> > +    set cmd [concat | git diff-tree --primer --no-color -r -s --stdin $gdtargs]
> 
> Does gitk really want to respect --primer? Might it not make more sense for 
> it, as a porcelain, to respect the diff.primer variable itself, and prepend it 
> to the list of diff args? Then it has the power to veto any options which it 
> doesn't handle.

The point of --primer was for scripts like gitk and git-gui.  I wouldn't say 
"respect --primer" so much as "pass --primer" (which causes diff-tree to respect 
diff.primer, which it normally wouldn't).  Reviewing the options that 
diff.primer currently supports, except for --color which I specifically guarded 
against, I don't see a scenario that would break gitk as is.

> Also, any gitk changes should almost certainly be split into a different
> patch.

OK.

> All in all, this was a lot more complicated than I was expecting. Why isn't 
> the behavior of "diff.primer" simply "pretend as if the options in diff.primer 
> were prepended to the command line"? That is easy to explain, and easy to 
> implement (the only trick is that you have to do an extra pass to find 
> --[no-]primer). Is there some drawback to such a simple scheme that I am 
> missing?

I think we could call that simple scheme the linear walk, and it was my first 
impulse as well.  We walk through all the places where diff options live, in 
order from lowest precedence to highest precedence, accumulating/overriding 
options as we go along.  When we're done, we know the user's intention.

But as you say, we need that extra pass.  Once I noticed we need that extra 
pass, the software engineer in me saw the concept of linear walk as too weak for 
this feature.  These options occur in precedence layers and we need to combine 
those layers the right way.  The right way means: honor the intuitive semantics 
the user expects.  Hence the function name flatten_diff_options().

A multi-pass walk is one possible implementation of such semantics, but in my 
opinion not the best.  My implementation is much more explicit, meaningful, and 
declarative.  It's not at all hard to understand looking at the code.  In fact, 
it positively declares itself to you and annouces exactly what it is doing.  It 
"underlines the intent" and also "eases further extensions" in Samuel Tardieu's 
words:
http://article.gmane.org/gmane.comp.version-control.git/105654

That's a desirability I believe you agree with :)
http://article.gmane.org/gmane.comp.version-control.git/105657

I think introducing explicit dirty masks and explicit layer flattening is the 
right way to go forward, but I agree it would be better to split this up into 
smaller patches.  My design would require a lot more lines of code if we wanted 
to support more diff options than are represented by struct diff_options.flags 
and .xdl_opts.  That would mean introducing more masks and more CPP macros.  
But I have faith in declarative programming.  100 lines of clear-as-day 
meaningful code don't scare me nearly as much as 10 lines of secret obfuscation.  
Also, I'm not convinced it is necessary for diff.primer to support all diff 
options under the sun.  Thoughts?

                                   -- Keith

Re: [PATCH v2 1/2] Introduce config variable "diff.primer"

[Junio C Hamano]

Keith Cascio <keith@CS.UCLA.EDU> writes:

> I think introducing explicit dirty masks and explicit layer flattening is the 
> right way to go forward,...

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.

Re: [PATCH v2 1/2] Introduce config variable "diff.primer"

[Keith Cascio]

On Tue, 3 Feb 2009, Junio C Hamano wrote:

> 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()").

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 
primer flags/masks.  IOW, there's a whole lotta flags and masks!!

> Doesn't it suggest that you may want two layers of masks, not a flat one, if 
> you really want the mechanism to scale?

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

Re: [PATCH v2 1/2] Introduce config variable "diff.primer"

[Jeff King]

On Tue, Feb 03, 2009 at 10:36:48PM -0800, Keith Cascio wrote:

> 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).

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

Re: [PATCH v2 1/2] Introduce config variable "diff.primer"

[Jeff King]

On Tue, Feb 03, 2009 at 09:55:08AM -0800, Keith Cascio wrote:

> First of all, thanks for the tips!

Of course. Thank you for spending time to refine your patch.

> > > +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 --color --color-words --cumulative --dirstat-by-file
> > > +--exit-code --ext-diff --find-copies-harder --follow --full-index
> > > +--ignore-all-space --ignore-space-at-eol --ignore-space-change
> > > +--ignore-submodules --no-color --no-ext-diff --no-textconv --patience -q
> > > +--quiet -R -r --relative -t --text --textconv -w`
> > 
> > Funny indentation?
> 
> The last part is a backtick-quoted list, I think it either must occur on one 
> line or as is.

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).

But more on that:

> > 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.
> > 
> > How was this list chosen?
> 
> 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 present list.

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
going to look like a bug.

> > 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 apply.
> 
> The current version only supports the flags listed above in
> Documentation/config.txt.

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
that may work for re-use, but I haven't checked.

> >   /*
> >    * 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) |
> >      /* (3) */ (master->flags & master->mask);
> 
> Your version is much better.

Except I think in (1) it should be "~master->mask". Oops. :)

> The point of --primer was for scripts like gitk and git-gui.  I

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 something like:

  git diff-tree `git config diff.primer` $other_options

if the caller wants to be totally promiscuous, and

  git diff-tree `git config diff.primer | filter_options` $other_options

if it wants to be paranoid (and obviously in tcl the code would be
different, but I think you can see the point).

> wouldn't say "respect --primer" so much as "pass --primer" (which
> causes diff-tree to respect diff.primer, which it normally wouldn't).
> Reviewing the options that diff.primer currently supports, except for
> --color which I specifically guarded against, I don't see a scenario
> that would break gitk as is.

Well, "--quiet" and "--exit-code" certainly would produce confusing
results. Though I can't imagine anyone putting those into their
diff.primer variable.

> I think we could call that simple scheme the linear walk, and it was
> my first impulse as well.  We walk through all the places where diff
> options live, in order from lowest precedence to highest precedence,
> accumulating/overriding options as we go along.  When we're done, we
> know the user's intention.
> 
> But as you say, we need that extra pass.  Once I noticed we need that
> extra pass, the software engineer in me saw the concept of linear walk
> as too weak for this feature.  These options occur in precedence
> layers and we need to combine those layers the right way.  The right
> way means: honor the intuitive semantics the user expects.  Hence the
> function name flatten_diff_options().

Hrm. Do we really need the multiple passes, though? Let's assume for a
minute that we don't want "--primer". The rule is: "if you are a
porcelain, you respect diff.primer; if you're not, you don't". And
because diff.primer is, by definition, a set of command line options, it
is trivial for a caller who wants to selectively apply them to a
plumbing to do so. Similarly, if somebody really wants to call a
porcelain and _disable_ options, I don't think "--no-primer" is
necessarily the right interface. Instead, the actual command line
options given override what's in diff.primer, so you can selectively
disable whatever you like.

And of course all of this extends naturally to a "primer.*" set of
config, rather than just "diff.primer".

> A multi-pass walk is one possible implementation of such semantics,
> but in my opinion not the best.  My implementation is much more
> explicit, meaningful, and declarative.  It's not at all hard to
> understand looking at the code.  In fact, it positively declares

I do think declarative interfaces can be nice, but I really have two
complaints with your approach:

  1. It is somewhat fragile, because it is easy to bypass the
     declarative nature in C. That is, there is an ordering constraint
     with the flattening that we may not be fulfilling everywhere in
     git. For example, let's say some code checks option A and then does
     something with option B as a result. If this code runs before the
     flattening, then it may fail to correctly pick up option A, since
     it is in the slave spot. But if it runs after, the flattening may
     logic may miss the option setting.

     I think the "after" case above is not a problem in practice,
     because we are only flattening two cases, and post-flattening we
     always just operate directly on the master case. So I think we
     really want to flatten at the last minute. So rather than
     flattening everything and storing it, I think a nicer approach is
     to put it in the accessor:

       /* If master set it, take that. If not, and slave set it,
          take that. If nobody set it, take the master as default. */
       #define DIFF_OPT_TST(opts, flag) \
         ((opts)->master_mask & DIFF_OPT_##flag ?  \
           (opts)->master_flag & DIFF_OPT_##flag : \
           (opts)->slave_mask & DIFF_OPT_##flag ? \
             (opts)->slave_flag & DIFF_OPT_##flag : \
             (opts)->master_flag & DIFF_OPT_##flag;

  2. The current flattening code deals only with the flags field, which
     is fairly easy to handle using a mask and some macros. But what
     about fields that have a more complex implementation? I think the
     amount of work to do flattening is going to be linear with the
     number of options. Which means that now there is new work to be
     done every time a new non-flag option is added, and a chance for
     the developer to forget to do that work. Which hurts
     maintainability in the long run.

So yes, I think a declarative solution can be nice, but there is really
very little language support in C for doing it in a safe way. I think
you can get by on the flags with some macros, but I don't think there is
really a nice general solution for all of the options that won't make
per-option work. If you really think it is possible, I invite you to try
to make a nice set of macros that cover all cases.

-Peff

PS I am behind on git mail and catching up slowly; I see there are some
other messages in this thread, but I don't have time to read them right
at this second. So don't think I am ignoring issues raised there -- I
just haven't gotten to them yet.

Re: [PATCH v2 1/2] Introduce config variable "diff.primer"

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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 something like:
>
>   git diff-tree `git config diff.primer` $other_options
>
> if the caller wants to be totally promiscuous, and
>
>   git diff-tree `git config diff.primer | filter_options` $other_options
>
> if it wants to be paranoid (and obviously in tcl the code would be
> different, but I think you can see the point).

I agree with this 100%.

Also, I think we should explain the semantics of diff.primer to new people
like that.  "The diff Porcelain command acts as if whatever you have in
diff.primer are prepended on your command line".

Which means that we do not have to touch the plumbing at all.  That allows
me sleep much better at night.

Re: [PATCH v2 1/2] Introduce config variable "diff.primer"

[Keith Cascio]

On Fri, 6 Feb 2009, Jeff King wrote:

> if somebody really wants to call a porcelain and _disable_ options, I don't 
> think "--no-primer" is necessarily the right interface. Instead, the actual 
> command line options given override what's in diff.primer, so you can 
> selectively disable whatever you like.

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.

Re: [PATCH v2 1/2] Introduce config variable "diff.primer"

[Jeff King]

On Mon, Feb 09, 2009 at 09:24:37AM -0800, Keith Cascio wrote:

> 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 hack-ish and unfriendly, but would work.

  2. in the git option list, but not the git command option list. IOW, we
     actually have two sets of options in any command:

        git [options] diff [options]

     So you could suppress primers (all of them, including diff.primer
     or primer.*) via:

       git --no-primer diff

Make sense?

-Peff

Re: [PATCH v2 1/2] Introduce config variable "diff.primer"

[Johannes Schindelin]

Hi,

On Fri, 13 Feb 2009, Jeff King wrote:

>   1. an environment variable, GIT_PRIMER. E.g., "GIT_PRIMER=0 git diff".
>      This strikes me as hack-ish and unfriendly, but would work.

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

Re: [PATCH v2 1/2] Introduce config variable "diff.primer"

[Jeff King]

On Sat, Feb 14, 2009 at 07:03:26AM +0100, Johannes Schindelin wrote:

> On Fri, 13 Feb 2009, Jeff King wrote:
> 
> >   1. an environment variable, GIT_PRIMER. E.g., "GIT_PRIMER=0 git diff".
> >      This strikes me as hack-ish and unfriendly, but would work.
> 
> FWIW I cannot stop to hate the term "primer" for this thing.

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

Re: [PATCH v2 1/2] Introduce config variable "diff.primer"

[Johannes Schindelin]

Hi,

On Sat, 14 Feb 2009, Jeff King wrote:

> On Sat, Feb 14, 2009 at 07:03:26AM +0100, Johannes Schindelin wrote:
> 
> > On Fri, 13 Feb 2009, Jeff King wrote:
> > 
> > >   1. an environment variable, GIT_PRIMER. E.g., "GIT_PRIMER=0 git diff".
> > >      This strikes me as hack-ish and unfriendly, but would work.
> > 
> > FWIW I cannot stop to hate the term "primer" for this thing.
> 
> 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.

Why not spell it out?  diff.defaultOptions

Ciao,
Dscho

Re: [PATCH v2 1/2] Introduce config variable "diff.primer"

[Jeff King]

On Sat, Feb 14, 2009 at 07:24:54AM +0100, Johannes Schindelin wrote:

> > 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.
> 
> Why not spell it out?  diff.defaultOptions

Yes, that is much better than my suggestion.

-Peff

Re: [PATCH v2 1/2] Introduce config variable "diff.primer"

[Keith Cascio]

On Sat, 14 Feb 2009, Johannes Schindelin wrote:

> Why not spell it out?  diff.defaultOptions

OK in v3 I'll use "defaultOptions".  Now what do we call the switches?

Re: [PATCH v2 1/2] Introduce config variable "diff.primer"

[Junio C Hamano]

Keith Cascio <keith@CS.UCLA.EDU> writes:

> On Sat, 14 Feb 2009, Johannes Schindelin wrote:
>
>> Why not spell it out?  diff.defaultOptions
>
> OK in v3 I'll use "defaultOptions".  Now what do we call the switches?

ignore-default?

Re: diff.defaultOptions implementation design [was diff.primer]

[Keith Cascio]

Peff,

On Fri, 13 Feb 2009, Jeff King wrote:

> 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 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

Re: diff.defaultOptions implementation design [was diff.primer]

[Jeff King]

On Mon, Feb 16, 2009 at 11:24:33PM -0800, Keith Cascio wrote:

> 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.

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
preliminary patches.

> 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).

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

Re: [PATCH v2 1/2] Introduce config variable "diff.defaultOptions"

[Keith Cascio]

Peff,

I'm replying to http://permalink.gmane.org/gmane.comp.version-control.git/108158

On Tue, 3 Feb 2009, Jeff King wrote:

> All in all, this was a lot more complicated than I was expecting. Why isn't 
> the behavior of "diff.primer" simply "pretend as if the options in diff.primer 
> were prepended to the command line"? That is easy to explain, and easy to 
> implement (the only trick is that you have to do an extra pass to find 
> --[no-]primer). Is there some drawback to such a simple scheme that I am 
> missing?

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

Re: [PATCH v2 1/2] Introduce config variable "diff.defaultOptions"

[Jeff King]

On Tue, Mar 17, 2009 at 09:05:18AM -0700, Keith Cascio wrote:

> 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

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.

Your PDF said:

> No matter which implementation, we all agree the semantics are "pretend
> as if the options in [diff.defaultOptions] were prepended to the command
> line" [1]. That requirement is subject neither to confusion nor doubt.
> An implementation is acceptable only if it delivers exactly that
> behavior to the user.

OK, good, I think we are in agreement there.

> Git's diff options processing scheme is a mature[2], versatile
> instrument relied upon by at least 47 client codepaths. The API it
> provides for recording user intent to an instance of struct diff_options
> entails the following steps[3]:
> 
> 1) to prepare the structure for recording, call diff_setup() once
> 
> 2) to record user intention, write to the structure[4], calling
> diff_opt_parse() as needed

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
ALLOW_TEXTCONV and RECURSIVE.

> 3) to prepare the structure for "playback", call diff_setup_done() once
>
> 4) to test user intention, read from the structure

OK, makes sense.

> Git's code is already equipped to react to every kind of user intention
> during step 2.

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
git-log.  In practice, this hasn't been a problem.

> An attractive (but hopelessly flawed) strategy is to pre-load the
> structure with defaultOptions before step 2. Step 1, diff_setup(), is
> the only place to do that, since the client starts overwriting as soon
> as diff_setup() returns.

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
into this functionality anyway.

> Some aspects of user intention dictate whether or not to perform the
> pre-load at all, most notably: which Git command the user invoked (also
> switches). But at step 1, inside diff_setup(), we don't know the user's
> full intention yet, so we can't decide whether or not to perform the
> pre-load.

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
dependency cycle.

> This could tempt an undisciplined programmer to try to peek ahead at
> part of the user's intention before the code sees it in its normal
> course. That would be a disaster because it would undermine the
> integrity, not to mention beauty, of Git's entire initialization scheme.
> It would be a cheap hack; an inelegant, fragile, ugly, and utterly
> half-baked attempt to bypass and circumvent the existing architecture.

Your poetry aside, I agree that way madness lies.

> My proposal:
> 
> a) patiently accumulates user intention via Git's well-established
> initialization scheme, never needing to peek ahead or misbehave in any
> way, thus attaining harmony.
> 
> b) postpones the decision whether or not to load defaultOptions until
> step 3, diff_setup_done(), after we've had every opportunity to examine
> user intention, but loads them effectively underneath any explicit
> command line options, thereby fulfilling the agreed upon semantic
> obligation.
> 
> c) is inspired by a simple, powerful, easy-to-understand, and popular
> metaphor: layer flattening.
> 
> Why, oh why do some people think there's an "easier" way[1]?!

Because I outlined it above?

Look, I am not opposed to layer flattening if that's what is required to
get it right. But consider the downside of layer flattening: we must
always record intent-to-change when making a change to the struct (i.e.,
the "mask" variable in your original patches). This is fine for members
hidden behind macros, but there are a lot of members that are assigned
to directly. We would need to:

  1. Introduce new infrastructure for assigning to these members.

  2. Fix existing locations by converting them to this infrastructure.

  3. Introduce some mechanism to help future callers get it right (since
     otherwise assigning directly is a subtle bug).

This is elementary encapsulation; in a language with better OO support,
you would hide all of your struct members behind accessors. But this is
C, and a dialect of C where that doesn't usually happen. So I think it
is going to introduce a lot of code changes, and the resulting code will
not look as much like the rest of git as it once did.

So what I am suggesting is that _if_ there is an easier way to do it,
then it is worth exploring.

-Peff

Re: [PATCH v2 1/2] Introduce config variable "diff.defaultOptions"

[Keith Cascio]

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.

On Fri, 20 Mar 2009, Jeff King wrote:

> Look, I am not opposed to layer flattening if that's what is required to get 
> it right. But consider the downside of layer flattening: we must always record 
> intent-to-change when making a change to the struct (i.e., the "mask" variable 
> in your original patches). This is fine for members hidden behind macros, but 
> there are a lot of members that are assigned to directly. We would need to:
> 
>   1. Introduce new infrastructure for assigning to these members.

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 
intent-to-change.

>   2. Fix existing locations by converting them to this infrastructure.

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 
direct assignment.  No change here.

>   3. Introduce some mechanism to help future callers get it right (since
>      otherwise assigning directly is a subtle bug).

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.  It's not fatal, it doesn't corrupt data, it 
affects only porcelain and it's not hidden.  Of all the insect kingdom (grand 
scheme of hypothetical bugs), this one isn't worth abandoning a good design 
over.

All in all, turns out v3 requires surprisingly little modification of existing 
code outside of diff.h/diff.c.  Actually, it only adds 3 lines, that's it!!

 builtin-diff.c                  |    2 +
 builtin-log.c                   |    1 +
 diff.c                          |  112 ++++++++++++++++++++++++-
 diff.h                          |   17 +++-

Shall I post v3?
                                 -- Keith

Re: [PATCH v2 1/2] Introduce config variable "diff.defaultOptions"

[Jeff King]

On Fri, Mar 20, 2009 at 10:11:27AM -0700, Keith Cascio wrote:

> (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.

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;

which will need to be PRISTINE-aware.

> >   3. Introduce some mechanism to help future callers get it right
> >   (since otherwise assigning directly is a subtle bug).
> 
> 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.

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
already addressed that above.

> All in all, turns out v3 requires surprisingly little modification of
> existing code outside of diff.h/diff.c.  Actually, it only adds 3
> lines, that's it!!
>
>  builtin-diff.c                  |    2 +
>  builtin-log.c                   |    1 +
>  diff.c                          |  112 ++++++++++++++++++++++++-
>  diff.h                          |   17 +++-
>
> Shall I post v3?

Yes, please. It is much better to be talking about actual code than
hypotheticals.

-Peff

[PATCH/RFC v3] Introduce config variable "diff.defaultoptions"

[Keith Cascio]

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 &= ~DIFF_OPT_##flag),\
+				    ((opts)->mask  |=  DIFF_OPT_##flag)
 #define DIFF_XDL_TST(opts, flag)    ((opts)->xdl_opts & XDF_##flag)
-#define DIFF_XDL_SET(opts, flag)    ((opts)->xdl_opts |= XDF_##flag)
+#define DIFF_XDL_SET(opts, flag)    ((opts)->xdl_opts |= XDF_##flag),\
+				    ((opts)->xdl_mask |= XDF_##flag)
-#define DIFF_XDL_CLR(opts, flag)    ((opts)->xdl_opts &= ~XDF_##flag)
+#define DIFF_XDL_CLR(opts, flag)    ((opts)->xdl_opts &= ~XDF_##flag),\
+				    ((opts)->xdl_mask |=  XDF_##flag)
 
 struct diff_options {
 	const char *filter;
@@ -80,6 +85,7 @@ struct diff_options {
 	const char *single_follow;
 	const char *a_prefix, *b_prefix;
 	unsigned flags;
+	unsigned mask;
 	int context;
 	int interhunkcontext;
 	int break_opt;
@@ -98,6 +104,7 @@ struct diff_options {
 	int prefix_length;
 	const char *stat_sep;
 	long xdl_opts;
+	long xdl_mask;
 
 	int stat_width;
 	int stat_name_width;
@@ -193,7 +200,7 @@ extern void diff_unmerge(struct diff_options *,
 extern int git_diff_basic_config(const char *var, const char *value, void *cb);
 extern int git_diff_ui_config(const char *var, const char *value, void *cb);
 extern int diff_use_color_default;
-extern void diff_setup(struct diff_options *);
+extern struct diff_options* diff_setup(struct diff_options *);
 extern int diff_opt_parse(struct diff_options *, const char **, int);
 extern int diff_setup_done(struct diff_options *);
 
diff --git a/diff.c b/diff.c
index 75d9fab..1c4fec4 100644
--- a/diff.c
+++ b/diff.c
@@ -26,6 +26,8 @@ static int diff_suppress_blank_empty;
 int diff_use_color_default = -1;
 static const char *diff_word_regex_cfg;
 static const char *external_diff_cmd_cfg;
+static const char *diff_defaults;
+static struct diff_options *defaults;
 int diff_auto_refresh_index = 1;
 static int diff_mnemonic_prefix;
 
@@ -106,6 +108,8 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)
 		diff_rename_limit_default = git_config_int(var, value);
 		return 0;
 	}
+	if (!strcmp(var, "diff.defaultoptions"))
+		return git_config_string(&diff_defaults, var, value);
 
 	switch (userdiff_config(var, value)) {
 		case 0: break;
@@ -2314,7 +2318,101 @@ static void run_checkdiff(struct diff_filepair *p, struct diff_options *o)
 	builtin_checkdiff(name, other, attr_path, p->one, p->two, o);
 }
 
+struct diff_options* parse_diff_defaults(struct diff_options *options)
+{
+	int count, len, i;
+	const char **new_argv;
+
+	if ((! diff_defaults) || ((len = (strlen(diff_defaults)+1)) < 3))
+		return options;
+	count = split_cmdline(strncpy(xmalloc(len), diff_defaults, len),
+			&new_argv);
+	for (i=0; i<count; i++)
+		diff_opt_parse(options, &new_argv[i], -1);
+	return options;
+}
+
+#define PRISTINE -0x40
+#define COALESCE_PTR(p) master->p = master->p ? master->p : slave->p
+#define COALESCE_INT(i) master->i = master->i != PRISTINE ? master->i : slave->i
+
+struct diff_options* flatten_diff_options(struct diff_options *master,
+					  struct diff_options *slave)
+{
+	/*
+	 * 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 set, we can take whether
+	 *      or not the master set it explicitly.
+	 *   3. Anything the master explicitly set, we take.
+	 */
+	master->flags =
+	 /* (1) */ (~master->mask & slave->flags & slave->mask) |
+	 /* (2) */ (master->flags & ~slave->mask) |
+	 /* (3) */ (master->flags & master->mask);
+	master->mask |= slave->mask;
+	master->xdl_opts =
+	 /* (1) */ (~master->xdl_mask & slave->xdl_opts & slave->xdl_mask) |
+	 /* (2) */ (master->xdl_opts & ~slave->xdl_mask) |
+	 /* (3) */ (master->xdl_opts & master->xdl_mask);
+	master->xdl_mask |= slave->xdl_mask;
+	master->output_format |= slave->output_format;
+	master->setup |= slave->setup;
+	COALESCE_PTR(a_prefix);
+	COALESCE_PTR(b_prefix);
+	COALESCE_PTR(filter);
+	COALESCE_PTR(orderfile);
+	COALESCE_PTR(pickaxe);
+	COALESCE_PTR(prefix);
+	COALESCE_PTR(single_follow);
+	COALESCE_PTR(stat_sep);
+	COALESCE_PTR(word_regex);
+	COALESCE_INT(abbrev);
+	COALESCE_INT(break_opt);
+	COALESCE_INT(close_file);
+	COALESCE_INT(context);
+	COALESCE_INT(detect_rename);
+	COALESCE_INT(dirstat_percent);
+	COALESCE_INT(interhunkcontext);
+	COALESCE_INT(line_termination);
+	COALESCE_INT(pickaxe_opts);
+	COALESCE_INT(prefix_length);
+	COALESCE_INT(rename_limit);
+	COALESCE_INT(rename_score);
+	COALESCE_INT(skip_stat_unmatch);
+	COALESCE_INT(stat_name_width);
+	COALESCE_INT(stat_width);
+	COALESCE_INT(warn_on_too_large_rename);
+	COALESCE_PTR(file);
+	COALESCE_PTR(change);
+	COALESCE_PTR(add_remove);
+	COALESCE_PTR(format_callback);
+	COALESCE_PTR(format_callback_data);
+	if((! master->paths) && slave->paths){
+		master->nr_paths = slave->nr_paths;
+		master->paths    = slave->paths;
+		master->pathlens = slave->pathlens;
+	}
+	return master;
+}
+
-void diff_setup(struct diff_options *options)
+struct diff_options* diff_setup(struct diff_options *options)
+{
+	memset(options, 0, sizeof(*options));
+	options->abbrev = options->break_opt = options->close_file =
+	options->context = options->detect_rename =
+	options->dirstat_percent = options->interhunkcontext =
+	options->line_termination = options->pickaxe_opts =
+	options->prefix_length = options->rename_limit =
+	options->rename_score = options->skip_stat_unmatch =
+	options->stat_name_width = options->stat_width =
+	options->warn_on_too_large_rename = PRISTINE;
+	return options;
+}
+
+struct diff_options* diff_fallback_values(struct diff_options *options)
 {
 	memset(options, 0, sizeof(*options));
 
@@ -2336,11 +2434,19 @@ void diff_setup(struct diff_options *options)
 		options->a_prefix = "a/";
 		options->b_prefix = "b/";
 	}
+	return options;
 }
 
 int diff_setup_done(struct diff_options *options)
 {
 	int count = 0;
+	struct diff_options fallback;
+
+	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)))));
+	flatten_diff_options(options, diff_fallback_values(&fallback));
 
 	if (options->output_format & DIFF_FORMAT_NAME)
 		count++;
@@ -2615,6 +2721,10 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 		DIFF_OPT_CLR(options, ALLOW_TEXTCONV);
 	else if (!strcmp(arg, "--ignore-submodules"))
 		DIFF_OPT_SET(options, IGNORE_SUBMODULES);
+	else if (!strcmp(arg, "--default-options"))
+		DIFF_OPT_SET(options, ALLOW_DEFAULT_OPTIONS);
+	else if (!strcmp(arg, "--no-default-options"))
+		DIFF_OPT_CLR(options, ALLOW_DEFAULT_OPTIONS);
 
 	/* misc options */
 	else if (!strcmp(arg, "-z"))
diff --git a/builtin-diff.c b/builtin-diff.c
index d75d69b..34c6ce2 100644
--- a/builtin-diff.c
+++ b/builtin-diff.c
@@ -283,6 +283,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 		diff_use_color_default = git_use_color_default;
 
 	init_revisions(&rev, prefix);
+	DIFF_OPT_SET(&rev.diffopt, ALLOW_DEFAULT_OPTIONS);
 
 	/* If this is a no-index diff, just run it and exit there. */
 	diff_no_index(&rev, argc, argv, nongit, prefix);
diff --git a/builtin-log.c b/builtin-log.c
index 8af55d2..1fa583f 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -37,6 +37,7 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,
 		get_commit_format(fmt_pretty, rev);
 	rev->verbose_header = 1;
 	DIFF_OPT_SET(&rev->diffopt, RECURSIVE);
+	DIFF_OPT_SET(&rev->diffopt, ALLOW_DEFAULT_OPTIONS);
 	rev->show_root_diff = default_show_root;
 	rev->subject_prefix = fmt_patch_subject_prefix;
 	DIFF_OPT_SET(&rev->diffopt, ALLOW_TEXTCONV);
-- 
1.6.2.1.337.g3b73.dirty

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

[Johannes Schindelin]

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 repository");
+	parse_default_diff_options(&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 8af55d2..2a63652 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -243,6 +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);
 	cmd_log_init(argc, argv, prefix, &rev);
 	if (!rev.diffopt.output_format)
 		rev.diffopt.output_format = DIFF_FORMAT_RAW;
@@ -314,6 +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);
 	cmd_log_init(argc, argv, prefix, &rev);
 
 	count = rev.pending.nr;
@@ -381,6 +383,7 @@ int cmd_log_reflog(int argc, const char **argv, const char *prefix)
 	init_reflog_walk(&rev.reflog_info);
 	rev.abbrev_commit = 1;
 	rev.verbose_header = 1;
+	parse_default_diff_options(&rev.diffopt);
 	cmd_log_init(argc, argv, prefix, &rev);
 
 	/*
@@ -412,6 +415,7 @@ int cmd_log(int argc, const char **argv, const char *prefix)
 
 	init_revisions(&rev, prefix);
 	rev.always_show_header = 1;
+	parse_default_diff_options(&rev.diffopt);
 	cmd_log_init(argc, argv, prefix, &rev);
 	return cmd_log_walk(&rev);
 }
diff --git a/diff.c b/diff.c
index 75d9fab..0e1b321 100644
--- a/diff.c
+++ b/diff.c
@@ -2657,6 +2657,31 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 	return 1;
 }
 
+static int default_diff_options(const char *key, const char *value, void *cb)
+{
+	if (!strcmp(key, "diff.defaultoptions")) {
+		char **options = cb;
+		free(*options);
+		*options = xstrdup(value);
+	}
+	return 0;
+}
+
+void parse_default_diff_options(struct diff_options *options)
+{
+	char *default_options = NULL;
+	const char **argv;
+	int argc;
+
+	git_config(default_diff_options, &default_options);
+	if (!default_options)
+		return;
+
+	argc = split_cmdline(default_options, &argv);
+	diff_opt_parse(options, argv, argc);
+	free(argv);
+}
+
 static int parse_num(const char **cp_p)
 {
 	unsigned long num, scale;
diff --git a/diff.h b/diff.h
index 6616877..e05e796 100644
--- a/diff.h
+++ b/diff.h
@@ -270,4 +270,6 @@ extern void diff_no_index(struct rev_info *, int, const char **, int, const char
 
 extern int index_differs_from(const char *def, int diff_flags);
 
+extern void parse_default_diff_options(struct diff_options *options);
+
 #endif /* DIFF_H */
diff --git a/t/t4037-diff-default-options.sh b/t/t4037-diff-default-options.sh
new file mode 100755
index 0000000..0284f7b
--- /dev/null
+++ b/t/t4037-diff-default-options.sh
@@ -0,0 +1,19 @@
+#!/bin/sh
+
+test_description='default options for diff'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	test_commit a &&
+	test_commit b
+'
+
+test_expect_success 'diff.defaultOptions' '
+	git config diff.defaultOptions --raw &&
+	git diff a > output &&
+	grep ^: output &&
+	test 1 = $(wc -l < output)
+'
+
+test_done
-- 
1.6.2.1.493.g67cf3

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

[Keith Cascio]

Johannes,

On Sat, 21 Mar 2009, Johannes Schindelin wrote:

> 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.

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

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

[Johannes Schindelin]

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@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.

Of course, to keep things simple, this has to be a separate patch.

Ciao,
Dscho

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

[Jeff King]

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

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

[Johannes Schindelin]

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

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

[Jeff King]

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:

  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

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

[Johannes Schindelin]

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>
---

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
>      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).

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;
@@ -383,7 +383,7 @@ int cmd_log_reflog(int argc, const char **argv, const char *prefix)
 	init_reflog_walk(&rev.reflog_info);
 	rev.abbrev_commit = 1;
 	rev.verbose_header = 1;
-	parse_default_diff_options(&rev.diffopt);
+	argc = parse_default_diff_options(argc, argv, &rev.diffopt);
 	cmd_log_init(argc, argv, prefix, &rev);
 
 	/*
@@ -415,7 +415,7 @@ int cmd_log(int argc, const char **argv, const char *prefix)
 
 	init_revisions(&rev, prefix);
 	rev.always_show_header = 1;
-	parse_default_diff_options(&rev.diffopt);
+	argc = parse_default_diff_options(argc, argv, &rev.diffopt);
 	cmd_log_init(argc, argv, prefix, &rev);
 	return cmd_log_walk(&rev);
 }
diff --git a/diff.c b/diff.c
index 6e76377..903dbb4 100644
--- a/diff.c
+++ b/diff.c
@@ -13,6 +13,7 @@
 #include "utf8.h"
 #include "userdiff.h"
 #include "sigchain.h"
+#include "parse-options.h"
 
 #ifdef NO_FAST_WORKING_DIRECTORY
 #define FAST_WORKING_DIRECTORY 0
@@ -2682,19 +2683,33 @@ static int default_diff_options(const char *key, const char *value, void *cb)
 	return 0;
 }
 
-void parse_default_diff_options(struct diff_options *options)
+int parse_default_diff_options(int real_argc, const char **real_argv,
+	struct diff_options *options)
 {
+	int use_defaults = 1;
+	struct option option[] = {
+		OPT_BOOLEAN(0, "defaults", &use_defaults, "use diff defaults"),
+		OPT_END()
+	};
 	char *default_options = NULL;
 	const char **argv;
 	int argc;
 
-	git_config(default_diff_options, &default_options);
-	if (!default_options)
-		return;
+	real_argc = parse_options(real_argc, real_argv, option, NULL,
+		PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN |
+		PARSE_OPT_NO_INTERNAL_HELP);
+
+	if (use_defaults) {
+		git_config(default_diff_options, &default_options);
+		if (!default_options)
+			return real_argc;
+
+		argc = split_cmdline(default_options, &argv);
+		diff_opt_parse(options, argv, argc);
+		free(argv);
+	}
 
-	argc = split_cmdline(default_options, &argv);
-	diff_opt_parse(options, argv, argc);
-	free(argv);
+	return real_argc;
 }
 
 static int parse_num(const char **cp_p)
diff --git a/diff.h b/diff.h
index e05e796..764e2f6 100644
--- a/diff.h
+++ b/diff.h
@@ -270,6 +270,6 @@ extern void diff_no_index(struct rev_info *, int, const char **, int, const char
 
 extern int index_differs_from(const char *def, int diff_flags);
 
-extern void parse_default_diff_options(struct diff_options *options);
+extern int parse_default_diff_options(int argc, const char **argv, struct diff_options *options);
 
 #endif /* DIFF_H */
diff --git a/t/t4037-diff-default-options.sh b/t/t4037-diff-default-options.sh
index 0284f7b..f57d65f 100755
--- a/t/t4037-diff-default-options.sh
+++ b/t/t4037-diff-default-options.sh
@@ -16,4 +16,9 @@ test_expect_success 'diff.defaultOptions' '
 	test 1 = $(wc -l < output)
 '
 
+test_expect_success '--no-defaults' '
+	git diff --no-defaults > output &&
+	! grep ^: output
+'
+
 test_done
-- 
1.6.2.1.613.g25746

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

[Jeff King]

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

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

[Johannes Schindelin]

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

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

[Jeff King]

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.

  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

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

[Junio C Hamano]

Jeff King <peff@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?

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

[Johannes Schindelin]

Hi,

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

> Jeff King <peff@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

Ciao,
Dscho

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

[Jeff King]

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

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

[Johannes Schindelin]

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

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

[Keith Cascio]

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.
                                  -- Keith

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

[Johannes Schindelin]

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

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

[Keith Cascio]

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.

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

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

[Johannes Schindelin]

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

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

[Keith Cascio]

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

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

[Keith Cascio]

Johannes,

Hi.  Was my last message understandable or should I add more explanation?
  http://comments.gmane.org/gmane.comp.version-control.git/115506

                       -- Keith

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

[Johannes Schindelin]

Hi,

On Wed, 8 Apr 2009, Keith Cascio wrote:

> Hi.  Was my last message understandable or should I add more 
> explanation?
>   http://comments.gmane.org/gmane.comp.version-control.git/115506

Sorry, I am just short on time for that hobby called Git.

Ciao,
Dscho

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

[Jeff King]

On Wed, Apr 08, 2009 at 05:44:29PM -0700, Keith Cascio wrote:

> Hi.  Was my last message understandable or should I add more explanation?
>   http://comments.gmane.org/gmane.comp.version-control.git/115506

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

Re: [PATCH v2 1/2] Introduce config variable "diff.primer"

[Jakub Narebski]

Keith Cascio <keith@cs.ucla.edu> writes:

> Introduce config variable "diff.primer".

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

Re: [PATCH v2 1/2] Introduce config variable "diff.primer"

[Keith Cascio]

On Tue, 3 Feb 2009, Jakub Narebski wrote:

> 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.

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