[PATCH v1 0/3] Introduce config variable "diff.primer"

[PATCH v1 0/3] Introduce config variable "diff.primer"

[Keith Cascio]

The next three patches introduce a way to specify diff options
git always obeys. Then use the new feature to
enhance git-gui with white space ignore settings.  The fastest
way to see this patch in action is: apply all three patches,
fire up git-gui, modify a file, then right-click on the diff
panel and look for the new "White Space" sub-menu.

Future work: Extend the gitattributes mechanism so it supports
all [diff] config variables, including e.g. diff.mnemonicprefix
and diff.primer.

Keith Cascio (3):
 Introduce config variable "diff.primer"
 Test functionality of new config variable "diff.primer"
 git-gui hooks for new config variable "diff.primer"

 Documentation/config.txt       |   14 +++++
 Documentation/diff-options.txt |   13 +++++
 Makefile                       |    2 +
 builtin-log.c                  |    1 +
 diff.c                         |   83 ++++++++++++++++++++++++++----
 diff.h                         |   15 ++++-
 git-gui/git-gui.sh             |   51 ++++++++++++++++++
 git-gui/lib/diff.tcl           |    8 ++-
 git-gui/lib/option.tcl         |   57 +++++++++++++++++++--
 gitk-git/gitk                  |   16 +++---
 t/t4033-diff-primer.sh         |  111 ++++++++++++++++++++++++++++++++++++++++
 11 files changed, 343 insertions(+), 28 deletions(-)
 create mode 100755 t/t4033-diff-primer.sh

[PATCH v1 1/3] Introduce config variable "diff.primer"

[Keith Cascio]

Introduce config variable "diff.primer".
Allows user to specify arbitrary options
to pass to diff on every invocation,
including internal invocations from other
programs, e.g. git-gui.
Introduce diff command-line options:
--no-primer, --machine-friendly
Protect git-format-patch, git-apply,
git-am, git-rebase, git-gui and gitk
from inapplicable options.

Signed-off-by: Keith Cascio <keith@cs.ucla.edu>
---
 Documentation/config.txt       |   14 +++++++
 Documentation/diff-options.txt |   13 ++++++
 Makefile                       |    2 +
 builtin-log.c                  |    1 +
 diff.c                         |   83 +++++++++++++++++++++++++++++++++++-----
 diff.h                         |   15 ++++++-
 git-gui/lib/diff.tcl           |    8 +++-
 gitk-git/gitk                  |   16 ++++----
 8 files changed, 129 insertions(+), 23 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 290cb48..dd00f98 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. `"--color --ignore-space-at-eol --exit-code"`.
+	See linkgit:git-diff[1]. You can override these at run time with the
+	diff 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 -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 1f8ce97..4d12359 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -240,5 +240,18 @@ endif::git-format-patch[]
 --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 "--color --ignore-space-at-eol --exit-code"`
+
+--machine-friendly::
+	Declaratively override and turn off all diff options that alter patch
+	output in a way not suitable for input to a program that expects
+	a canonical patch.  For example, `--color`, and the whitespace ignore
+	options `-w`, `-b` and `--ignore-space-at-eol`.  Important when
+	'git-format-patch' generates output for 'git-apply' or 'git-am', for
+	example in the context of 'git-rebase'.
+
 For more detailed explanation on these common options, see also
 linkgit:gitdiffcore[7].
diff --git a/Makefile b/Makefile
index b4d9cb4..195f984 100644
--- a/Makefile
+++ b/Makefile
@@ -1279,6 +1279,8 @@ git-http-push$X: revision.o http.o http-push.o $(GITLIBS)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
 		$(LIBS) $(CURL_LIBCURL) $(EXPAT_LIBEXPAT)
 
+diff.h: xdiff/xdiff.h
+
 $(LIB_OBJS) $(BUILTIN_OBJS): $(LIB_H)
 $(patsubst git-%$X,%.o,$(PROGRAMS)): $(LIB_H) $(wildcard */*.h)
 builtin-revert.o wt-status.o: wt-status.h
diff --git a/builtin-log.c b/builtin-log.c
index 2ae39af..b385e35 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -784,6 +784,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	rev.combine_merges = 0;
 	rev.ignore_merges = 1;
 	DIFF_OPT_SET(&rev.diffopt, RECURSIVE);
+	DIFF_OPT_SET(&rev.diffopt, MACHINE_FRIENDLY);
 
 	rev.subject_prefix = fmt_patch_subject_prefix;
 
diff --git a/diff.c b/diff.c
index 82cff97..a8c103f 100644
--- a/diff.c
+++ b/diff.c
@@ -24,6 +24,8 @@ static int diff_rename_limit_default = 200;
 static int diff_suppress_blank_empty;
 int diff_use_color_default = -1;
 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;
 
@@ -102,6 +104,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;
@@ -2215,6 +2219,46 @@ 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 set_diff_primer(struct diff_options *options)
+{
+	char  *str1, *token, *saveptr;
+	int    len;
+
+	if((DIFF_OPT_TST(options, SUPPRESS_PRIMER)) ||
+	   (!              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-McClusk
+	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));
@@ -2225,15 +2269,16 @@ 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
-		DIFF_OPT_CLR(options, COLOR_DIFF);
+		 DIFF_OPT_SET(options, COLOR_DIFF);
+	else if( DIFF_OPT_TST(options, COLOR_DIFF))
+		 DIFF_OPT_CLR(options, COLOR_DIFF);
 	options->detect_rename = diff_detect_rename_default;
 
 	if (!diff_mnemonic_prefix) {
@@ -2322,6 +2367,18 @@ int diff_setup_done(struct diff_options *options)
 		DIFF_OPT_SET(options, EXIT_WITH_STATUS);
 	}
 
+	if( ! DIFF_OPT_TST(      options, SUPPRESS_PRIMER ) ){
+	  if( !                           primer ){
+	    diff_setup(                   primer = (struct diff_options *) malloc( sizeof(struct diff_options) ) );
+	    set_diff_primer(              primer );
+	  }
+	  flatten_diff_options(  options, primer );
+	}
+
+	if(   DIFF_OPT_TST(      options, MACHINE_FRIENDLY ) ){
+	  DIFF_MACHINE_FRIENDLY( options );
+	}
+
 	return 0;
 }
 
@@ -2469,13 +2526,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")) {
@@ -2496,8 +2553,10 @@ 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 (!strcmp(arg, "--exit-code"))
 		DIFF_OPT_SET(options, EXIT_WITH_STATUS);
 	else if (!strcmp(arg, "--quiet"))
@@ -2512,6 +2571,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, "--no-primer"))
+		DIFF_OPT_SET(options, SUPPRESS_PRIMER);
+	else if (!strcmp(arg, "--machine-friendly"))
+		DIFF_OPT_SET(options, MACHINE_FRIENDLY);
 
 	/* misc options */
 	else if (!strcmp(arg, "-z"))
diff --git a/diff.h b/diff.h
index 4d5a327..e98c23a 100644
--- a/diff.h
+++ b/diff.h
@@ -5,6 +5,7 @@
 #define DIFF_H
 
 #include "tree-walk.h"
+#include "xdiff/xdiff.h"
 
 struct rev_info;
 struct diff_options;
@@ -66,9 +67,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_SUPPRESS_PRIMER     (1 << 22)
+#define DIFF_OPT_MACHINE_FRIENDLY    (1 << 23)
+#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_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_MACHINE_FRIENDLY(opts) ((opts)->flags &= ~(DIFF_OPT_COLOR_DIFF)), ((opts)->xdl_opts &= ~(XDF_WHITESPACE_FLAGS))
 
 struct diff_options {
 	const char *filter;
@@ -77,6 +84,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 +103,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/git-gui/lib/diff.tcl b/git-gui/lib/diff.tcl
index bbbf15c..94faf95 100644
--- a/git-gui/lib/diff.tcl
+++ b/git-gui/lib/diff.tcl
@@ -276,6 +276,7 @@ proc start_show_diff {cont_info {add_opts {}}} {
 	}
 
 	lappend cmd -p
+	lappend cmd --exit-code
 	lappend cmd --no-color
 	if {$repo_config(gui.diffcontext) >= 1} {
 		lappend cmd "-U$repo_config(gui.diffcontext)"
@@ -310,6 +311,7 @@ proc read_diff {fd cont_info} {
 	global ui_diff diff_active
 	global is_3way_diff is_conflict_diff current_diff_header
 	global current_diff_queue
+	global errorCode
 
 	$ui_diff conf -state normal
 	while {[gets $fd line] >= 0} {
@@ -397,7 +399,9 @@ proc read_diff {fd cont_info} {
 	$ui_diff conf -state disabled
 
 	if {[eof $fd]} {
-		close $fd
+		fconfigure $fd -blocking 1
+		catch { close $fd } err
+		set diff_exit_status $errorCode
 
 		if {$current_diff_queue ne {}} {
 			advance_diff_queue $cont_info
@@ -413,7 +417,7 @@ proc read_diff {fd cont_info} {
 		}
 		ui_ready
 
-		if {[$ui_diff index end] eq {2.0}} {
+		if {$diff_exit_status eq "NONE"} {
 			handle_empty_diff
 		}
 		set callback [lindex $cont_info 1]
diff --git a/gitk-git/gitk b/gitk-git/gitk
index dc2a439..49e5cb7 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 --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 --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 --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 --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 --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 --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 --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 --no-color --stdin -p --pretty"
 
 set gitencoding {}
 catch {
-- 
1.6.1

[PATCH v1 2/3] 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/t4033-diff-primer.sh |  111 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 111 insertions(+), 0 deletions(-)
 create mode 100755 t/t4033-diff-primer.sh

diff --git a/t/t4033-diff-primer.sh b/t/t4033-diff-primer.sh
new file mode 100755
index 0000000..116d6ad
--- /dev/null
+++ b/t/t4033-diff-primer.sh
@@ -0,0 +1,111 @@
+#!/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 begins with empty value' '
+[ -z $(git config --get diff.primer) ]
+'
+
+tr 'Q_' '\015 ' << EOF > expect
+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 diff with empty value of diff.primer' 'test_cmp expect 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 out'
+
+cat << EOF > expect
+diff --git a/x b/x
+index d99af23..8b32fb5 100644
+EOF
+git diff > out
+test_expect_success 'test with diff.primer = -w' 'test_cmp expect 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 git format-patch not affected by diff.primer' 'test_cmp expect out'
+
+test_done
+
-- 
1.6.1

[PATCH v1 3/3] git-gui hooks for new config variable "diff.primer"

[Keith Cascio]

git-gui hooks for new config variable "diff.primer".
Add three checkboxes to both sides of
options panel (local/global).
Add a sub-menu named "White Space" to
diff-panel right-click context menu, with
three checkboxes.

Signed-off-by: Keith Cascio <keith@cs.ucla.edu>
---
 git-gui/git-gui.sh     |   51 ++++++++++++++++++++++++++++++++++++++++++
 git-gui/lib/option.tcl |   57 +++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 103 insertions(+), 5 deletions(-)

diff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh
index e018e07..5d93351 100755
--- a/git-gui/git-gui.sh
+++ b/git-gui/git-gui.sh
@@ -3075,10 +3075,43 @@ $ui_diff tag conf d>>>>>>> \
 
 $ui_diff tag raise sel
 
+proc mirror_diff_state {} {
+	global  diff__ignore_space_at_eol diff__ignore_space_change diff__ignore_all_space
+
+	set key  "diff.primer"
+	set ddo [git config --get $key]
+	set diff__ignore_space_at_eol [expr {[string match "*--ignore-space-at-eol*" $ddo] ? "true" : "false"}]
+	set diff__ignore_space_change [expr {[string match "*--ignore-space-change*" $ddo] ? "true" : "false"}]
+	set diff__ignore_all_space    [expr {[string match "*--ignore-all-space*"    $ddo] ? "true" : "false"}]
+}
+
+proc adjust_command_line { flag value str } {
+	if {$value eq "true"} {
+	  if { ! [string match "*$flag*" $str ] } {
+	    set              str [concat $str $flag] }
+	} else { regsub       -- $flag   $str "" str }
+	return                           $str
+}
+
+proc record_diff_state {} {
+	global  diff__ignore_space_at_eol diff__ignore_space_change diff__ignore_all_space
+
+	set key  "diff.primer"
+	set ddo [git config --get $key]
+	set ddo [adjust_command_line --ignore-space-at-eol $diff__ignore_space_at_eol $ddo]
+	set ddo [adjust_command_line --ignore-space-change $diff__ignore_space_change $ddo]
+	set ddo [adjust_command_line --ignore-all-space    $diff__ignore_all_space    $ddo]
+
+	git config $key $ddo
+	reshow_diff
+}
+
 # -- Diff Body Context Menu
 #
 
 proc create_common_diff_popup {ctxm} {
+	global  diff__ignore_space_at_eol diff__ignore_space_change diff__ignore_all_space
+
 	$ctxm add command \
 		-label [mc "Show Less Context"] \
 		-command show_less_context
@@ -3087,6 +3120,24 @@ proc create_common_diff_popup {ctxm} {
 		-label [mc "Show More Context"] \
 		-command show_more_context
 	lappend diff_actions [list $ctxm entryconf [$ctxm index last] -state]
+	mirror_diff_state
+	set whitespacemenu $ctxm.ws
+	menu $whitespacemenu -postcommand mirror_diff_state
+	$ctxm add cascade \
+		-label [mc "White Space"] \
+		-menu $whitespacemenu
+	$whitespacemenu add checkbutton \
+		-label [mc "--ignore-space-at-eol"] \
+		-variable diff__ignore_space_at_eol -onvalue "true" -offvalue "false" \
+		-command record_diff_state
+	$whitespacemenu add checkbutton \
+		-label [mc "--ignore-space-change"] \
+		-variable diff__ignore_space_change -onvalue "true" -offvalue "false" \
+		-command record_diff_state
+	$whitespacemenu add checkbutton \
+		-label [mc "--ignore-all-space"   ] \
+		-variable diff__ignore_all_space    -onvalue "true" -offvalue "false" \
+		-command record_diff_state
 	$ctxm add separator
 	$ctxm add command \
 		-label [mc Refresh] \
diff --git a/git-gui/lib/option.tcl b/git-gui/lib/option.tcl
index 1d55b49..fbdf4e8 100644
--- a/git-gui/lib/option.tcl
+++ b/git-gui/lib/option.tcl
@@ -28,6 +28,7 @@ proc save_config {} {
 	global repo_config global_config system_config
 	global repo_config_new global_config_new
 	global ui_comm_spell
+	global ddo diff_primer_global diff_primer_repo pseudovariables
 
 	foreach option $font_descs {
 		set name [lindex $option 0]
@@ -46,17 +47,40 @@ proc save_config {} {
 		unset global_config_new(gui.$font^^size)
 	}
 
+	foreach name [get_diff_primer] {
+		set diff_option [string range $name 8 [string length $name]]
+		set ifound      [lsearch     $diff_primer_global $diff_option]
+		if {$global_config_new($name) eq "true"} {
+		  if {$ifound <  0} { lappend diff_primer_global $diff_option }
+		} else {
+		  if {$ifound >= 0} { set     diff_primer_global [lreplace $diff_primer_global $ifound $ifound]}
+		}
+		set ifound      [lsearch     $diff_primer_repo   $diff_option]
+		if {  $repo_config_new($name) eq "true"} {
+		  if {$ifound <  0} { lappend diff_primer_repo   $diff_option }
+		} else {
+		  if {$ifound >= 0} { set     diff_primer_repo   [lreplace $diff_primer_repo   $ifound $ifound]}
+		}
+	}
+	array unset default_config gui.diff--ignore-*
+	set    default_config($ddo) ""
+	set global_config_new($ddo) [join    $diff_primer_global]
+	set   repo_config_new($ddo) [join    $diff_primer_repo  ]
+
 	foreach name [array names default_config] {
 		set value $global_config_new($name)
-		if {$value ne $global_config($name)} {
-			if {$value eq $system_config($name)} {
+		set value_global [expr {[info exists global_config($name)] ? $global_config($name) : ""}]
+		set value_system [expr {[info exists system_config($name)] ? $system_config($name) : ""}]
+		set value_repo   [expr {[info exists   repo_config($name)] ?   $repo_config($name) : ""}]
+		if {$value ne $value_global} {
+			if {$value eq $value_system} {
 				catch {git config --global --unset $name}
 			} else {
 				regsub -all "\[{}\]" $value {"} value
 				git config --global $name $value
 			}
 			set global_config($name) $value
-			if {$value eq $repo_config($name)} {
+			if {$value eq $value_repo} {
 				catch {git config --unset $name}
 				set repo_config($name) $value
 			}
@@ -65,8 +89,10 @@ proc save_config {} {
 
 	foreach name [array names default_config] {
 		set value $repo_config_new($name)
-		if {$value ne $repo_config($name)} {
-			if {$value eq $global_config($name)} {
+		set value_global [expr {[info exists global_config($name)] ? $global_config($name) : ""}]
+		set value_repo   [expr {[info exists   repo_config($name)] ?   $repo_config($name) : ""}]
+		if {$value ne $value_repo} {
+			if {$value eq $value_global} {
 				catch {git config --unset $name}
 			} else {
 				regsub -all "\[{}\]" $value {"} value
@@ -88,10 +114,23 @@ proc save_config {} {
 	}
 }
 
+proc get_diff_primer {} {
+	global repo_config global_config
+	global ddo diff_primer_global diff_primer_repo pseudovariables
+
+	set ddo "diff.primer"
+	set diff_primer_global [expr {[info exists global_config($ddo)] ? [split $global_config($ddo)] : [list]}]
+	set diff_primer_repo   [expr {[info exists   repo_config($ddo)] ? [split   $repo_config($ddo)] : [list]}]
+	set pseudovariables [list "gui.diff--ignore-space-at-eol" "gui.diff--ignore-space-change" "gui.diff--ignore-all-space"]
+
+	return $pseudovariables
+}
+
 proc do_options {} {
 	global repo_config global_config font_descs
 	global repo_config_new global_config_new
 	global ui_comm_spell
+	global ddo diff_primer_global diff_primer_repo pseudovariables
 
 	array unset repo_config_new
 	array unset global_config_new
@@ -108,6 +147,11 @@ proc do_options {} {
 	foreach name [array names global_config] {
 		set global_config_new($name) $global_config($name)
 	}
+	foreach name [get_diff_primer] {
+		set diff_option [string range $name 8 [string length $name]]
+		set global_config_new($name) [expr {[lsearch $diff_primer_global $diff_option] < 0 ? "false" : "true"}]
+		set   repo_config_new($name) [expr {[lsearch $diff_primer_repo   $diff_option] < 0 ? "false" : "true"}]
+	}
 
 	set w .options_editor
 	toplevel $w
@@ -150,6 +194,9 @@ proc do_options {} {
 		{i-20..200 gui.copyblamethreshold {mc "Minimum Letters To Blame Copy On"}}
 		{i-0..300 gui.blamehistoryctx {mc "Blame History Context Radius (days)"}}
 		{i-1..99 gui.diffcontext {mc "Number of Diff Context Lines"}}
+		{b gui.diff--ignore-space-at-eol {mc "Diff Ignore Trailing White Space"            }}
+		{b gui.diff--ignore-space-change {mc "Diff Ignore Changes In Amount Of White Space"}}
+		{b gui.diff--ignore-all-space    {mc "Diff Ignore All White Space"                 }}
 		{i-0..99 gui.commitmsgwidth {mc "Commit Message Text Width"}}
 		{t gui.newbranchtemplate {mc "New Branch Name Template"}}
 		{c gui.encoding {mc "Default File Contents Encoding"}}
-- 
1.6.1

Re: [PATCH v1 1/3] Introduce config variable "diff.primer"

[Johannes Schindelin]

Hi,

On Sun, 25 Jan 2009, Keith Cascio wrote:

> Introduce config variable "diff.primer".
> Allows user to specify arbitrary options
> to pass to diff on every invocation,
> including internal invocations from other
> programs, e.g. git-gui.

That would break existing scripts using "git diff" rather badly.  We 
already did not allow something like "git config alias.diff ..." from 
changing the behavior of "git diff", so I cannot find a reason why we 
should let diff.primer (a misnomer BTW) override the behavior.

Ciao,
Dscho

Re: [PATCH v1 3/3] git-gui hooks for new config variable "diff.primer"

[Johannes Schindelin]

Hi,

On Sun, 25 Jan 2009, Keith Cascio wrote:

> git-gui hooks for new config variable "diff.primer".
> Add three checkboxes to both sides of
> options panel (local/global).
> Add a sub-menu named "White Space" to
> diff-panel right-click context menu, with
> three checkboxes.

Rather than storing the information about how to call "git diff" as 
diff.primer, why don't you store that information in a config variable 
gui.whiteSpaceMode and teach "git gui" to call "git diff" accordingly?

That would have the further advantage of not breaking other people's 
setups...

>  git-gui/git-gui.sh     |   51 ++++++++++++++++++++++++++++++++++++++++++
>  git-gui/lib/option.tcl |   57 +++++++++++++++++++++++++++++++++++++++++++----

Please submit git-gui patches without the git-gui prefix, as it makes it 
harder on the maintainer of git-gui, Shawn (who you did not Cc: BTW).

Ciao,
Dscho

Re: [PATCH v1 1/3] Introduce config variable "diff.primer"

[Keith Cascio]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1197 bytes --]

On Sun, 25 Jan 2009, Johannes Schindelin wrote:

> That would break existing scripts using "git diff" rather badly.  We already 
> did not allow something like "git config alias.diff ..." from changing the 
> behavior of "git diff", so I cannot find a reason why we should let 
> diff.primer (a misnomer BTW) override the behavior.

I took special care to protect all core scripts from the effects.  Quote from 
patch 1/3:
> Protect git-format-patch, git-apply,
> git-am, git-rebase, git-gui and gitk
> from inapplicable options.

I fact, by introducing the cpp macro DIFF_MACHINE_FRIENDLY() and the 
command-line options "--machine-friendly" and "--no-primer", I made such 
protection declarative.  Don't you find it preferable that existing programs and 
scripts would explicitly declare their desire for machine-friendly output?

The name "primer" is open to discussion, of course.  But I like it.
From Merriam-Webster:
primer n 1: a device for priming 2: material used in priming a surface
prime vb 1: fill, load 2: to prepare for firing 3: to apply the first color, coating or preparation to <~ a wall>

Thanks for your input.  More input welcome.
                                    -- Keith

Re: [PATCH v1 3/3] git-gui hooks for new config variable "diff.primer"

[Keith Cascio]

On Sun, 25 Jan 2009, Johannes Schindelin wrote:

> Rather than storing the information about how to call "git diff" as 
> diff.primer, why don't you store that information in a config variable 
> gui.whiteSpaceMode and teach "git gui" to call "git diff" accordingly?
> 
> That would have the further advantage of not breaking other people's 
> setups...

This wasn't just for the GUI.  Using Git for a project I imported from CVS, I 
wanted --ignore-space-at-eol to be in effect at all times on the command line.  
Of course, the "alias.dff" approach suggested yesterday by Teemu would work for 
that.  But I got the feeling this is a more general need.  I'll name my primary 
inspiration: ExamDiff (a Windows program).  ExamDiff lets you specify a wide 
range of options that remain in effect over all invocations.  Seems like 
something a lot of users would find natural.  Please comment.

> Please submit git-gui patches without the git-gui prefix, as it makes it 
> harder on the maintainer of git-gui, Shawn (who you did not Cc: BTW).

Sorry Shawn, I should have Cc'd you.  Please let me know if I can improve the 
git-gui code in any way.

                                        -- Keith

Re: [PATCH v1 1/3] Introduce config variable "diff.primer"

[Johannes Schindelin]

Hi,

On Sun, 25 Jan 2009, Keith Cascio wrote:

> On Sun, 25 Jan 2009, Johannes Schindelin wrote:
> 
> > That would break existing scripts using "git diff" rather badly.  We 
> > already did not allow something like "git config alias.diff ..." from 
> > changing the behavior of "git diff", so I cannot find a reason why we 
> > should let diff.primer (a misnomer BTW) override the behavior.
> 
> I took special care to protect all core scripts from the effects.

What about my scripts I have here locally?  Do you want to change them, 
too?

> I fact, by introducing the cpp macro DIFF_MACHINE_FRIENDLY() and the 
> command-line options "--machine-friendly" and "--no-primer", I made such 
> protection declarative.  Don't you find it preferable that existing 
> programs and scripts would explicitly declare their desire for 
> machine-friendly output?

No.  We made a promise long time ago that plumbing (and "git diff" is 
pretty much plumbing, except for the configurable colorness) would not 
change behind scripts' backs.

And since Shawn uses plumbing for that very reason, your diff.primer patch 
would not be allowed to make a difference.  Ever.

Now, if you would have changed only the UI diff things (i.e. git diff, but 
not git diff-files), I could have accepted the diff.primer patch for 
different applications than "git gui", but from cursory reading of your 
patch it does not appear so.

Speaking of appearance (or for that matter, explaining why it was only a 
cursory reading): did it not occur to you that your coding style is 
utterly different from the surrounding code?

Just to number a few things that would definitely prohibit this patch from 
being applied:

- space instead of tabs,

- horrible lengths of spaces within the line,

- no space after if, but after the parenthesis.

Now, this could be good explanation why you need the patch (to ignore 
white-space), but that is not a reason of letting us suffer, too.

Besides, it seems you did a lot of "fixes" on the side that I do not like 
at all.  Simple example: if the original code cleared the 
DIRSTAT_CUMULATIVE flag, it is not acceptable for you to introduce an 
unnecessary if(), testing if the CUMULATIVE flag was set to begin with.

Ciao,
Dscho

Re: [PATCH v1 1/3] Introduce config variable "diff.primer"

[Keith Cascio]

On Sun, 25 Jan 2009, Johannes Schindelin wrote:

> What about my scripts I have here locally?  Do you want to change them, 
> too?

Someone who already wrote local scripts probably won't use the diff.primer 
feature, or if he does, he'll be thoughtful about the consequences and modify 
his scripts.  diff.primer has no effect unless you use it.  It is totally 
personal.

> > I fact, by introducing the cpp macro DIFF_MACHINE_FRIENDLY() and the 
> > command-line options "--machine-friendly" and "--no-primer", I made such 
> > protection declarative.  Don't you find it preferable that existing programs 
> > and scripts would explicitly declare their desire for machine-friendly 
> > output?

> No.  We made a promise long time ago that plumbing (and "git diff" is 
> pretty much plumbing, except for the configurable colorness) would not 
> change behind scripts' backs.
> 
> And since Shawn uses plumbing for that very reason, your diff.primer patch 
> would not be allowed to make a difference.  Ever.

I fixed up every single place where gitk and git-gui internally call diff.  
Before I ever got there, Shawn already protected himself by passing --no-color 
to "git diff" (git-gui/lib/diff.tcl line 279).  My changes are in a good spirit 
because they enhance the declarativeness and explicitness of the code.  I added 
meaningful information.

> Now, if you would have changed only the UI diff things (i.e. git diff, but 
> not git diff-files), I could have accepted the diff.primer patch for 
> different applications than "git gui", but from cursory reading of your 
> patch it does not appear so.
> 
> Speaking of appearance (or for that matter, explaining why it was only a 
> cursory reading): did it not occur to you that your coding style is 
> utterly different from the surrounding code?
> 
> Just to number a few things that would definitely prohibit this patch from 
> being applied:
> 
> - space instead of tabs,
> - horrible lengths of spaces within the line,
> - no space after if, but after the parenthesis.

You are right, I should mimic the existing conventions.  If we reach consensus 
about the functionality of the patch, I would be happy to redo all the white 
space to match what's already there.  In case you are curious, I put whitespace 
in lines in the name of readability, to make tokens line up with each other.  
Again, it is right to mimic local convention and I have no problem making that 
revision.

> Besides, it seems you did a lot of "fixes" on the side that I do not like at 
> all.  Simple example: if the original code cleared the DIRSTAT_CUMULATIVE 
> flag, it is not acceptable for you to introduce an unnecessary if(), testing 
> if the CUMULATIVE flag was set to begin with.

I appreciate that you noticed this little detail.  You are extremely thorough 
and I take your criticism very seriously.  The reason for that little if() is 
that, with the enhanced declarativeness of the code in diff.h, calling the cpp 
macro DIFF_OPT_SET() is now more meaningful that just flipping a bit.  It is now 
the equivalent of saying "I intend to set this bit, and I want my intention 
recorded and honored later."  The purpose of the piece of code where I 
introduced the if() is only to setup the right defaults, not to declare user 
intention.  Therefore, IOW, if the default is already in place, do nothing.

c is only one of many languages I write.  Designers of newer languages emphasize 
meangingfulness, explicitness, declarativeness of code, e.g. the DRY principle, 
etc.  c is very flexible, so it does not prevent us from following these 
beneficial practices in c.  After all, the first c++ compilers simply emitted c 
and then called a c compiler, right?

With thanks,
Keith

Re: [PATCH v1 1/3] Introduce config variable "diff.primer"

[Junio C Hamano]

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

> Introduce config variable "diff.primer".
> Allows user to specify arbitrary options
> to pass to diff on every invocation,
> including internal invocations from other
> programs, e.g. git-gui.
> Introduce diff command-line options:
> --no-primer, --machine-friendly
> Protect git-format-patch, git-apply,
> git-am, git-rebase, git-gui and gitk
> from inapplicable options.
>
> Signed-off-by: Keith Cascio <keith@cs.ucla.edu>

Your Subject is good; in a shortlog output that will be taken 3 months
down the road, it will still tell us what this patch was about, among 100
other patches that are about different topics.

The proposed commit log message describes what the patch does, but it does
not explain what problem it solves, nor why the approach the patch takes
to solve that problem is good.  Lines that are too short and too dense
without paragraph breaks do not help readability either.

> ---
>  Documentation/config.txt       |   14 +++++++
>  Documentation/diff-options.txt |   13 ++++++
>  Makefile                       |    2 +
>  builtin-log.c                  |    1 +
>  diff.c                         |   83 +++++++++++++++++++++++++++++++++++-----
>  diff.h                         |   15 ++++++-
>  git-gui/lib/diff.tcl           |    8 +++-
>  gitk-git/gitk                  |   16 ++++----
>  8 files changed, 129 insertions(+), 23 deletions(-)

You can work around the backward incompatibility you are introducing for
known users that you broke with your patch, by including updates to them,
and that is what your patches to git-gui and gitk are, but that is a sure
sign that the approach is flawed.

The point of lowlevel plumbing (e.g. diff-{files,index,tree}) is to give
people's scripts an interface that they can rely on.  It is not about
giving a magic interface that all the users are somehow magically upgraded
without change, when the underlying git is upgraded.

If a script X does not use "ignore whitespace" without an explicit request
from the end user when it runs diff-tree internally, installing a new
version of diff-tree should *NOT* magically make script X to run it with
"ignore whitespace", because you do not know what the script X uses
diff-tree output for and how, even when the end user sets diff.primer to
get "ignore whitespace" applied to his command line invocation of "git
diff" Porcelain.  Imagine a case where the operation of that script X
relies on seeing at least two context lines around the hunk in order to
correctly parse textual diff output from "diff-index -p", and the user
sets "-U1" in diff.primer --- you would break the script and it is not
fair to blame the script for not explicitly passing -U3 and relying on the
default.

Scriptability by definition means you do not know how scripts written by
people around plumbing use the output; I do not think you can sensibly say
"this should not be turned on in a machine friendly output, but this is
safe to use".

I would not be opposed to an enhancement to the plumbing that the scripts
can use to say "I am willing to take any option (or perhaps "these
options") given to me via diff.primer".  Some scripts may want to be just
a pass-thru of whatever the underlying git-diff-* command outputs, and it
may be a handy way to magically upgrade them to allow their invocation of
lowlevel plumbing to be affected by what the end-user configured.  But
that magic upgrade has to be an opt/in process.

There are funny indentation to align the same variable names on two
adjacent lines and such; please don't.

Re: [PATCH v1 0/3] Introduce config variable "diff.primer"

[Junio C Hamano]

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

> Future work: Extend the gitattributes mechanism so it supports
> all [diff] config variables, including e.g. diff.mnemonicprefix
> and diff.primer.

I am puzzled.

The gitattributes mechanism is about per-path settings, but I do not think
a mnemonicprefix that is per-path makes much sense.

Re: [PATCH v1 0/3] Introduce config variable "diff.primer"

[Keith Cascio]

On Sun, 25 Jan 2009, Junio C Hamano wrote:

> I am puzzled.
> 
> The gitattributes mechanism is about per-path settings, but I do not think a 
> mnemonicprefix that is per-path makes much sense.

That was just an example (perhaps poorly chosen).  What I meant to suggest is 
making gitattributes consistent with gitconfig WRT at least the [diff] section.  
But maybe that's not appropriate.  Thanks for the insight.

Re: [PATCH v1 0/3] Introduce config variable "diff.primer"

[Jeff King]

On Sun, Jan 25, 2009 at 12:41:21PM -0800, Keith Cascio wrote:

> > I am puzzled.
> > 
> > The gitattributes mechanism is about per-path settings, but I do not
> > think a mnemonicprefix that is per-path makes much sense.
> 
> That was just an example (perhaps poorly chosen).  What I meant to
> suggest is making gitattributes consistent with gitconfig WRT at least
> the [diff] section.  But maybe that's not appropriate.  Thanks for the
> insight.

I don't think you want it entirely consistent. What would
diff.renamelimit mean in the context of a gitattribute? But I do think
it makes sense for some (like specific diff options such as whitespace
handling).

Also, if you're going to have options that apply to gitattributes diff
drivers _and_ as a general fallback, I think we need to define when the
fallback kicks in. That is, let's say I have a gitattributes file like
this:

   *.c diff=c

and my config says:

  [diff]
    opt1 = val1_default
    opt2 = val2_default

  [diff "c"]
    opt1 = val1_c

Now obviously if I want to use opt1 for my C files, it should be val1_c.
But if I want to use opt2, what should it use? There are two reasonable
choices, I think:

  1. You use val2_default. The rationale is that the "c" diff driver did
     not define an opt2, so you fall back to the global default.

  2. It is unset. The rationale is that you are using the "c" diff
     driver, and it has left the value unset. The default then means "if
     you have no diff driver setup".

I suspect "1" is what people would want most of the time, but "2" is
actually more flexible (since there is otherwise no way to say "I
explicitly left diff.c.opt2 unset").

If (2) is desired, I think it makes more sense to put such "default"
options into their own diff driver section. Like:

  [diff "default"]
    opt2 = whatever

And then it is more clear that once you have selected the "c" diff
driver, the values in the other "default" are not relevant.

I don't think this is a huge issue overall, but it occurs to me that we
have just added diff.wordRegex and diff.*.wordRegex. So it makes sense
to think for a minute which behavior we want before it ships and we are
stuck with backwards compatibility forever.

-Peff

Re: [PATCH v1 1/3] Introduce config variable "diff.primer"

[Jeff King]

On Sun, Jan 25, 2009 at 10:44:25AM -0800, Keith Cascio wrote:

> The name "primer" is open to discussion, of course.  But I like it.
> From Merriam-Webster:
> primer n 1: a device for priming 2: material used in priming a surface
> prime vb 1: fill, load 2: to prepare for firing 3: to apply the first color, coating or preparation to <~ a wall>

FWIW, I found it very confusing. I would have expected "diff.options" or
"diff.defaults". There is also some precedent in the form of
GIT_DIFF_OPTS, but I believe it _only_ handles --unified and -u, so it
is not necessarily a useful model.

-Peff

Re: [PATCH v1 1/3] Introduce config variable "diff.primer"

[Keith Cascio]

On Sun, 25 Jan 2009, Jeff King wrote:

> FWIW, I found it very confusing. I would have expected "diff.options" or 
> "diff.defaults". There is also some precedent in the form of GIT_DIFF_OPTS, 
> but I believe it _only_ handles --unified and -u, so it is not necessarily a 
> useful model.

OK, point taken.  I wasn't trying to be idiosyncratic at all.  Just trying to be 
explicit and avoid all confusion.  Since all diff options already have default 
values, primer looks to me like the layer one step above defaults, hence the 
painting analogy.  Mercurial calls it "defaults", but that doesn't mean we 
should necessarily follow in their footsteps (see 
http://article.gmane.org/gmane.comp.version-control.git/107103).

I think being as clear as possible about what primer is, that is it NOT 
defaults, helps to feel more comfortable with its consequences, i.e. in my 
opinion, that it will not break things.

                                   -- Keith

Re: [PATCH v1 1/3] Introduce config variable "diff.primer"

[Jeff King]

On Sun, Jan 25, 2009 at 02:58:34PM -0800, Keith Cascio wrote:

> OK, point taken.  I wasn't trying to be idiosyncratic at all.  Just
> trying to be explicit and avoid all confusion.  Since all diff options
> already have default values, primer looks to me like the layer one
> step above defaults, hence the painting analogy.  Mercurial calls it
> "defaults", but that doesn't mean we should necessarily follow in
> their footsteps (see
> http://article.gmane.org/gmane.comp.version-control.git/107103).
> 
> I think being as clear as possible about what primer is, that is it
> NOT defaults, helps to feel more comfortable with its consequences,
> i.e. in my opinion, that it will not break things.

I'm not sure I agree that they are not new defaults, but any such
argument is going to get into the exact definition of "default" which is
not really useful to the task at hand.

I think "options" is a better word (as in, pretend like you already
specified these "options" on the command line), but I am not going to
insist on that. I mainly just wanted to point out that I found "primer"
confusing. Enough so that, even though I knew you were interested in
this topic from your previous mails, I saw the word "primer" and said to
myself: "what in the world is this patch about?"

-Peff

Re: [PATCH v1 1/3] Introduce config variable "diff.primer"

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Scriptability by definition means you do not know how scripts written by
> people around plumbing use the output; I do not think you can sensibly say
> "this should not be turned on in a machine friendly output, but this is
> safe to use".
>
> I would not be opposed to an enhancement to the plumbing that the scripts
> can use to say "I am willing to take any option (or perhaps "these
> options") given to me via diff.primer".  Some scripts may want to be just
> a pass-thru of whatever the underlying git-diff-* command outputs, and it
> may be a handy way to magically upgrade them to allow their invocation of
> lowlevel plumbing to be affected by what the end-user configured.  But
> that magic upgrade has to be an opt/in process.

I suspect it is pretty much orthogonal to the "use user's default without
being told from the command line", but it might be a worthy goal to
introduce a mechanism for the scripts to accept "safe" default options
from the end user while rejecting undesirable ones that would interfere
with the way it uses plumbing.

For example, gitk drives "git rev-list" and many options you give from the
command line (e.g. "gitk --all --simplify-merges -- drivers/") are passed
to the underlying plumbing.

This is a double edged sword.  When we add new features to git-rev-list,
(e.g. --simplify-merges or --simplify-by-decoration are fairly recent
inventions that did not exist when gitk was written originally), some of
them can be safely passed and automagically translates to a new feature in
gitk.  However, use of some options (e.g. --reverse) breaks the assumption
the tool makes on the output from the underlying plumbing and should not
be accepted from the end-user.

It would be a good addition to our toolset if scripts like gitk can
declare which options and features are safe to accept from the end user to
pass down to the plumbing tools.  "git rev-parse", which lets the script
sift between options that are meant to affect ancestry traversal and the
ones that are for other (primarily diff family) commands, does not do
anything fancy like that, but it would be a logical place to do this sort
of thing.

And it is not limited to "scripts" use.  A recent topic on rejecting
colouring options from being given to format-patch would also be helped
with such a mechanism if it is available to builtins.

Just an idle thought.

Re: [PATCH v1 1/3] Introduce config variable "diff.primer"

[Keith Cascio]

On Sun, 25 Jan 2009, Junio C Hamano wrote:

> I suspect it is pretty much orthogonal to the "use user's default without 
> being told from the command line", but it might be a worthy goal to introduce 
> a mechanism for the scripts to accept "safe" default options from the end user 
> while rejecting undesirable ones that would interfere with the way it uses 
> plumbing.
> 
> For example, gitk drives "git rev-list" and many options you give from the 
> command line (e.g. "gitk --all --simplify-merges -- drivers/") are passed to 
> the underlying plumbing.
> 
> This is a double edged sword.  When we add new features to git-rev-list, (e.g. 
> --simplify-merges or --simplify-by-decoration are fairly recent inventions 
> that did not exist when gitk was written originally), some of them can be 
> safely passed and automagically translates to a new feature in gitk.  
> However, use of some options (e.g. --reverse) breaks the assumption the tool 
> makes on the output from the underlying plumbing and should not be accepted 
> from the end-user.
> 
> It would be a good addition to our toolset if scripts like gitk can declare 
> which options and features are safe to accept from the end user to pass down 
> to the plumbing tools.  "git rev-parse", which lets the script sift between 
> options that are meant to affect ancestry traversal and the ones that are for 
> other (primarily diff family) commands, does not do anything fancy like that, 
> but it would be a logical place to do this sort of thing.
> 
> And it is not limited to "scripts" use.  A recent topic on rejecting colouring 
> options from being given to format-patch would also be helped with such a 
> mechanism if it is available to builtins.
> 
> Just an idle thought.


Yes yes yes yes!!!!!  I've been working on a response to your previous message, 
in which I address exactly this possibility.  Coming soon.

Re: [PATCH v1 1/3] Introduce config variable "diff.primer"

[Keith Cascio]

On Sun, 25 Jan 2009, Junio C Hamano wrote:

> Your Subject is good; in a shortlog output that will be taken 3 months
> down the road, it will still tell us what this patch was about, among 100
> other patches that are about different topics.

Thanks, I value the encouragement.

> The proposed commit log message describes what the patch does, but it does
> not explain what problem it solves, nor why the approach the patch takes
> to solve that problem is good.  Lines that are too short and too dense
> without paragraph breaks do not help readability either.

Noted, future versions will be better.

> You can work around the backward incompatibility you are introducing for known 
> users that you broke with your patch, by including updates to them, and that 
> is what your patches to git-gui and gitk are, but that is a sure sign that the 
> approach is flawed.
> 
> The point of lowlevel plumbing (e.g. diff-{files,index,tree}) is to give 
> people's scripts an interface that they can rely on.  It is not about giving a 
> magic interface that all the users are somehow magically upgraded without 
> change, when the underlying git is upgraded.


I believe, as I know you do as well, concept is the right way to approach code.  
I believe in the power of concept, and I agree that porcelain/plumbing is a good 
and powerful concept, and should not be violated.

> If a script X does not use "ignore whitespace" without an explicit request
> from the end user when it runs diff-tree internally, installing a new
> version of diff-tree should *NOT* magically make script X to run it with
> "ignore whitespace", because you do not know what the script X uses
> diff-tree output for and how, even when the end user sets diff.primer to
> get "ignore whitespace" applied to his command line invocation of "git
> diff" Porcelain.  Imagine a case where the operation of that script X
> relies on seeing at least two context lines around the hunk in order to
> correctly parse textual diff output from "diff-index -p", and the user
> sets "-U1" in diff.primer --- you would break the script and it is not
> fair to blame the script for not explicitly passing -U3 and relying on the
> default.
>
> Scriptability by definition means you do not know how scripts written by
> people around plumbing use the output; I do not think you can sensibly say
> "this should not be turned on in a machine friendly output, but this is
> safe to use".
> 
> I would not be opposed to an enhancement to the plumbing that the scripts
> can use to say "I am willing to take any option (or perhaps "these
> options") given to me via diff.primer".  Some scripts may want to be just
> a pass-thru of whatever the underlying git-diff-* command outputs, and it
> may be a handy way to magically upgrade them to allow their invocation of
> lowlevel plumbing to be affected by what the end-user configured.  But
> that magic upgrade has to be an opt/in process.

I agree opt-in is always better with new grammar/semantics.  However, the 
constraint I was trying to live inside is: if I call "git diff" on the command 
line with no options at all, then primer active.  Yet perhaps that's not 
possible, and the only way to do primer is to require opt-in spelled "--primer".  
Then I can tell bash to alias 'gitdiff' as 'git diff --primer' and use that on 
the command line.  And I could patch git-gui and gitk to call 'git diff --primer 
--no-color'.  Note that would still require the same magnitude of change to the 
git-gui and gitk Tcl code as occurs with my v1 patch.

It would be really cool if there was a way
to make this work without "--primer" !!!
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Worth considering: I believe Git users who've written their own scripts are not 
the type to use diff.primer blindly, if at all.  If they did use it, they'd be 
very thoughtful about its consequences for their existing scripts.  Based on 
that belief, and the fact that I already protected every script in Git core, and 
that I could work with the authors of contrib to do the same there, I have a 
very high degree of confidence that no existing script would be broken the day 
you release diff.primer, if ever :(

Because an enhancement *could* break existing scripts is not sufficient grounds 
to declare it worthless.  My gut tells me there is a sane way to do this, but I 
need more familiarity with the "literature" to discover it.

I see evidence there is already anxiety surrounding the necessity of 
"machine-friendliness", Cf. 
http://article.gmane.org/gmane.comp.version-control.git/107006

I think my DIFF_MACHINE_FRIENDLY() cpp macro with --machine-friendly command 
line option does a nice job of making that explicit, so we could all just relax!  
When a monster is lurking under the bridge threatening the kingdom, put it to 
death once and for all!  Rather than relying on an elaborate system of 
conventions prescribing how to tip-toe acrosss the bridge just the right way 
while pretending the monster isn't there.

> There are funny indentation to align the same variable names on two
> adjacent lines and such; please don't.

You're right.  I'm sorry.  I'll fix all style to adhere to local convention.

                                    -- Keith

Re: [PATCH v1 1/3] Introduce config variable "diff.primer"

[Jeff King]

On Sun, Jan 25, 2009 at 06:40:02PM -0800, Keith Cascio wrote:

> I agree opt-in is always better with new grammar/semantics.  However,
> the constraint I was trying to live inside is: if I call "git diff" on
> the command line with no options at all, then primer active.  Yet
> perhaps that's not possible, and the only way to do primer is to
> require opt-in spelled "--primer".  Then I can tell bash to alias
> 'gitdiff' as 'git diff --primer' and use that on the command line.

What's the point of aliasing something that isn't "git diff" to "git
diff --primer"? At that point, couldn't you just do away with --primer
entirely and alias "gitdiff" to "git diff --whatever --your --primer
--options --are"?

Anyway, I think that isn't necessary. We _do_ have a mechanism to handle
this already: some commands are plumbing, and must have stable
interfaces, and some commands are porcelain, and can do your magic
automatically. For example, gitk doesn't actually call "git diff"; it
calls "git diff-tree", "git diff-index", etc.

So if you just want this from the command line, then I think it is safe
to have "git diff" always respect "diff.primer", and scripts shouldn't
be impacted.

But this can break down in two ways:

  1. Sometimes we blur the line of plumbing and porcelain, where
     functionality is available only through plumbing. For example,
     gitweb until recently called "git diff" because there is no other
     way to diff two arbitrary blobs. But the solution there is, I
     think, to make that functionality available through plumbing. Not
     to disallow enhancements to porcelain.

  2. When you want a script to take advantage of porcelain-like options,
     the situation is much more difficult (and this is what Junio was
     talking about in his last mail).

     What I think is sane is:

       a. You grow new feature X.
       b. Porcelain takes advantage of any config that asks us to use X.
       c. Plumbing does _not_ respect such config, but will respect
          command line options.
       d. Scripts control which command line options they use; when the
          script writer decides feature X will not interfere (either
          because it is harmless to the script's use, or because the
          script is enhanced to handle the new behavior), then it can
          pass an "--allow-X" command line option.

     And of course that has two disadvantages (and I'm running out of
     numbering schemes):

       I. You have to wait for the script to be updated before you can
          start using X, even if _you_ know that it's harmless.

      II. Point (d) is not always true. Junio mentioned the fact that
          gitk passes command line parameters blindly to rev-list, which
          is potentially unsafe. Up until now, our attitude has been "if
          it hurts, don't do it". In other words, if you call "gitk
          --reverse" and it looks ugly, then it is your fault. :)

-Peff

Re: [PATCH v1 1/3] Introduce config variable "diff.primer"

[Jeff King]

On Sun, Jan 25, 2009 at 06:30:18PM -0800, Junio C Hamano wrote:

> It would be a good addition to our toolset if scripts like gitk can
> declare which options and features are safe to accept from the end user to
> pass down to the plumbing tools.  "git rev-parse", which lets the script
> sift between options that are meant to affect ancestry traversal and the
> ones that are for other (primarily diff family) commands, does not do
> anything fancy like that, but it would be a logical place to do this sort
> of thing.

I'm not sure there is a good way of doing this at a less fine-grained
level than "each option". That is, how can git-core, without knowing how
the script will use the output, classify options in groups according to
how the script will react to them?

It seems like "--since" is innocent enough for gitk. It just limits the
commits shown. So maybe it goes into the "ancestry traversal" list. But
is that whole list safe? "--reverse" isn't, but I would have put it in
the same list.

So I think what you will end up with is a list in gitk of "these
particular options are known good for passing through". And that doesn't
really need tool support from git-core. It's up to each script how much
it wants to protect the user.

But if you are proposing that some config options can be "enabled" by
scripts selectively, then I think that does need tool support. Keith's
"primer" example will be parsed by git, not by whatever script is
calling it. So we would need to feed it some list of "these are the OK
options".

-Peff

Re: [PATCH v1 1/3] Introduce config variable "diff.primer"

[Junio C Hamano]

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

> I agree opt-in is always better with new grammar/semantics.  However, the 
> constraint I was trying to live inside is: if I call "git diff" on the command 
> line with no options at all

"git diff" is a Porcelain.

> Worth considering: I believe Git users who've written their own scripts are not 
> the type to use diff.primer blindly, if at all.

Perhaps this nonsense comes from your misunderstanding on the line between
the plumbing and the Porcelain.

Users of git, including me, would love to be able to use default options
in our $HOME/.gitconfig file when using "git diff" interactively, but will
refuse to see our scripts that we wrote using "git diff-files" and "git
diff-index" broken, because the reason we explicitly used these plumbing
commands is to avoid getting broken with a change from the underlying
version of git.  That's the whole point of output stability for the
plumbing.

If you want to be able to use -w or -b (or --color) in git-gui, you must
first vet the script to see if it can sanely operate on the output from
git-diff-index with such options, and after it is determined that it is
safe, it should give its users a way to pass that to the underlying
plumbing, or picked up such options from the configuration (perhaps using
the same diff.primer configuration).

This has to be a conscious opt-in process per script.

Re: [PATCH v1 1/3] Introduce config variable "diff.primer"

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> So I think what you will end up with is a list in gitk of "these
> particular options are known good for passing through". And that doesn't
> really need tool support from git-core. It's up to each script how much
> it wants to protect the user.

I tend to agree.  Also at the same time this does not have to contradict
with what Keith wants to do.  gitk just needs to learn to peek into
diff.primer, and use the safe ones while discarding others.  A tool
support is already there in the form of git-config to do this, though.

Re: [PATCH v1 1/3] Introduce config variable "diff.primer"

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

We seem to think the same way these days, so I do not have very much to
add on top of what you already said.  I'll fix one typo, though.

> But this can break down in two ways:
>
>   1. Sometimes we blur the line of plumbing and porcelain, where
>      functionality is available only through plumbing. For example,

s/through plumbing/through Porcelain/.

Re: [PATCH v1 1/3] Introduce config variable "diff.primer"

[Jeff King]

On Sun, Jan 25, 2009 at 07:42:06PM -0800, Junio C Hamano wrote:

> We seem to think the same way these days, so I do not have very much to
> add on top of what you already said.  I'll fix one typo, though.

Heh. Am I corrupting you, or you me?

> > But this can break down in two ways:
> >
> >   1. Sometimes we blur the line of plumbing and porcelain, where
> >      functionality is available only through plumbing. For example,
> 
> s/through plumbing/through Porcelain/.

Oops, yes, thank you. That was what I meant.

-Peff

Re: [PATCH v1 1/3] Introduce config variable "diff.primer"

[Johannes Schindelin]

Hi,

On Sun, 25 Jan 2009, Jeff King wrote:

> So if you just want this from the command line, then I think it is safe 
> to have "git diff" always respect "diff.primer", and scripts shouldn't 
> be impacted.

But as Keith made clear, he wanted to use it from _git-gui_.  Which 
-- naturally -- _has_ to use plumbing, to guarantee a stable interface.

So "fixing" this in "git diff" is the wrong place; anything else than 
teaching "git gui" to remember user-defined diff options and to use them 
would be a complicator's glove.

Ciao,
Dscho

backwards compatibility, was Re: [PATCH v1 1/3] Introduce config variable "diff.primer"

[Johannes Schindelin]

Hi,

On Sun, 25 Jan 2009, Jeff King wrote:

>   1. Sometimes we blur the line of plumbing and porcelain, where
>      functionality is available only through plumbing. For example,
>      gitweb until recently called "git diff" because there is no other
>      way to diff two arbitrary blobs. But the solution there is, I
>      think, to make that functionality available through plumbing. Not
>      to disallow enhancements to porcelain.

Just a reminder: we are very conservative when it comes to breaking 
backwards compatibility.  For example, people running (but not upgrading) 
gitweb who want to upgrade Git may rightfully expect their setups not to 
be broken for a long time, if ever.

So your mentioning gitweb using "git diff" precludes all kind of cute 
games, methinks.

And please no "anybody who would do this and that would be nuts" excuses: 
if you want to change something fundamental like this, _you_ have to 
defend it.

It is not acceptable to just shout out what you want and expect those 
affected negatively to do the impact analysis for you.

Ciao,
Dscho

Re: [PATCH v1 1/3] Introduce config variable "diff.primer"

[Jeff King]

On Mon, Jan 26, 2009 at 11:54:26AM +0100, Johannes Schindelin wrote:

> > So if you just want this from the command line, then I think it is safe 
> > to have "git diff" always respect "diff.primer", and scripts shouldn't 
> > be impacted.
> 
> But as Keith made clear, he wanted to use it from _git-gui_.  Which 
> -- naturally -- _has_ to use plumbing, to guarantee a stable interface.
>
> So "fixing" this in "git diff" is the wrong place; anything else than 
> teaching "git gui" to remember user-defined diff options and to use them 
> would be a complicator's glove.

I think what you are missing here is that he specifically mentioned the
command line, and I was responding to those comments. There are two
separate problems: default options for command line usage and the
mechanism by which one can set options for things like git-gui.

In this case he was asking specifically about "git diff" from the
command line, so fixing it in there _is_ the place to fix it (the only
other alternative being to make a wrapper or alias).

For the other problem, it _may_ benefit from tool support that would
help porcelains respect a "diff.primer" variable like Keith proposed.
But that has already been discussed elsewhere in the thread, so I'm not
going to repeat it here.

-Peff

Re: backwards compatibility, was Re: [PATCH v1 1/3] Introduce config variable "diff.primer"

[Jeff King]

On Mon, Jan 26, 2009 at 11:59:46AM +0100, Johannes Schindelin wrote:

> Just a reminder: we are very conservative when it comes to breaking 
> backwards compatibility.  For example, people running (but not upgrading) 
> gitweb who want to upgrade Git may rightfully expect their setups not to 
> be broken for a long time, if ever.
> 
> So your mentioning gitweb using "git diff" precludes all kind of cute 
> games, methinks.

Are you aware that gitweb no longer calls "git diff", exactly because
of problems caused by calling a porcelain from a script?

I don't want to break existing setups, either. But at some point you
have to say "this is porcelain, so don't rely on there not being any
user-triggered effects in its behavior". If porcelain is cast in stone,
then what is the point in differentiating plumbing from porcelain?

And when the line is blurred (as I think it is in several places), then
it has to be dealt with on a case-by-case basis. What is the benefit,
and what is the likelihood and extent of harm?

> And please no "anybody who would do this and that would be nuts" excuses: 
> if you want to change something fundamental like this, _you_ have to 
> defend it.
> 
> It is not acceptable to just shout out what you want and expect those 
> affected negatively to do the impact analysis for you.

This message is addressed to me, but I don't know exactly what you think
I'm proposing, failing to defend, or failing to do an impact analysis
for. Or are you speaking generally of the "you" who submit patches?

-Peff

Re: backwards compatibility, was Re: [PATCH v1 1/3] Introduce config variable "diff.primer"

[Johannes Schindelin]

Hi,

On Mon, 26 Jan 2009, Jeff King wrote:

> On Mon, Jan 26, 2009 at 11:59:46AM +0100, Johannes Schindelin wrote:
> 
> > Just a reminder: we are very conservative when it comes to breaking 
> > backwards compatibility.  For example, people running (but not upgrading) 
> > gitweb who want to upgrade Git may rightfully expect their setups not to 
> > be broken for a long time, if ever.
> 
> Are you aware that gitweb no longer calls "git diff", exactly because
> of problems caused by calling a porcelain from a script?

As I said: do you really expect people not to forget to upgrade gitweb 
manually when they do "sudo make install" with a new Git version?

> I don't want to break existing setups, either. But at some point you 
> have to say "this is porcelain, so don't rely on there not being any 
> user-triggered effects in its behavior". If porcelain is cast in stone, 
> then what is the point in differentiating plumbing from porcelain?

Two points there:

- with gitweb, we were the offenders ourselves.  So we should give the 
  users of gitweb at least _some_ slack.

- Concretely for the "porcelain" git diff: This workflow

	git diff > my-patch
	<attach and send to somebody>

  is probably pretty wide spread.  And it is okay, a user is not a script, 
  they are very much allowed to use porcelain.  And we _would_ break 
  expectations there.

Now, I have another two, fundamental problems with the diff options 
defaults: you are restricting the thing to _one_ set of options, and when 
somebody wants to run without those options, she has to actively _undo_ 
them.

Remember, sometimes you need another set of options. Like, when I send 
mail to a Git user, I want "-M -C -C", when I send mail to a non-Git user, 
I do not want any additional options (and try to undo "-M -C -C" on the 
command line, good luck), and sometimes it is much easier to see what 
happened with a word diff.

So what I need are three different sets of diff options.

Guess how well that works with aliases -- we are talking command line 
here after all, right?

Ciao,
Dscho

Re: backwards compatibility, was Re: [PATCH v1 1/3] Introduce config variable "diff.primer"

[Jeff King]

On Mon, Jan 26, 2009 at 12:28:55PM +0100, Johannes Schindelin wrote:

> > Are you aware that gitweb no longer calls "git diff", exactly because
> > of problems caused by calling a porcelain from a script?
> 
> As I said: do you really expect people not to forget to upgrade gitweb 
> manually when they do "sudo make install" with a new Git version?

Yes.

But my point is that gitweb was _already_ broken, because it was calling
a porcelain, and there were _already_ features that could cause serious
breakage.

So yes, adding a new feature that a user can trigger causes one more
opportunity for breakage. But the solution isn't to never ever add more
features to "git diff". It's to close the avenue by which the new _and_
old breakages are triggered.

> > I don't want to break existing setups, either. But at some point you 
> > have to say "this is porcelain, so don't rely on there not being any 
> > user-triggered effects in its behavior". If porcelain is cast in stone, 
> > then what is the point in differentiating plumbing from porcelain?
> 
> Two points there:
> 
> - with gitweb, we were the offenders ourselves.  So we should give the 
>   users of gitweb at least _some_ slack.

I'm not sure I agree. I always assumed that since gitweb, git-gui, and
gitk are bundled with git during release that we have _more_ leeway in
making matching changes between them.

Are you sure that you can run random versions of gitweb with random
versions of git in the first place?

> - Concretely for the "porcelain" git diff: This workflow
> 
> 	git diff > my-patch
> 	<attach and send to somebody>
> 
>   is probably pretty wide spread.  And it is okay, a user is not a script, 
>   they are very much allowed to use porcelain.  And we _would_ break 
>   expectations there.

Sorry, but what in the world are we supposed to do? Never ever allow the
user to specify diff options to a porcelain because they might impact
the output? A user who sets a config option or a command line option to
impact the output of "git diff" is responsible for how they use "git
diff".

There are already options like this in "git diff". I don't see how one
more changes anything.

> Now, I have another two, fundamental problems with the diff options 
> defaults: you are restricting the thing to _one_ set of options, and when 
> somebody wants to run without those options, she has to actively _undo_ 
> them.

Yep, that's what defaults are. And guess what: we _already_ have the
same thing. I have diff.renames set in my ~/.gitconfig. That does
_exactly_ what

  git config --global diff.primer -M

would do. It's just a syntax that saves us from having to introduce a
boatload of new variables, one per command line option.

> Remember, sometimes you need another set of options. Like, when I send 
> mail to a Git user, I want "-M -C -C", when I send mail to a non-Git user, 
> I do not want any additional options (and try to undo "-M -C -C" on the 
> command line, good luck), and sometimes it is much easier to see what 
> happened with a word diff.

This is a strawman. You have described a scenario where an alias or a
wrapper script is a better fit. Great, then use that mechanism in this
scenario. But that doesn't mean there aren't other scenarios where a
different setup makes more sense (I think Keith's original goal was to
use "-w").

> So what I need are three different sets of diff options.
> 
> Guess how well that works with aliases -- we are talking command line 
> here after all, right?

Personally, I have always found the suggestion that users simply put
their preferences into an alias like "mydiff" to be a silly one: git has
already taken the obvious good names, so now I am stuck using "git
mydiff" forever and forgetting that "git diff" even exists.

But then, I don't have your "three sets of options" scenario. I just
want one set of defaults. So I don't have a need to name each one, and
having to choose a different name becomes a detriment rather than an
advantage.

However, there are two other drawbacks of aliases that I can think of:

  1. They are tied to a specific command, whereas diff options are tied
     to the concept of diffing. So now I have to write an alias (with a
     new name) for each command:

       git config alias.mylog 'log -w'
       git config alias.mydiff 'diff -w'
       git config alias.myshow 'show -w'

  2. They can't change defaults based on the file to be diffed. One of
     the things Keith mentioned (and I don't remember if this was
     implemented in his patch series) was supporting this for
     gitattributes diff drivers. How do I do

       git config diff.tex.primer -w

     using aliases?

But now you have me defending Keith's proposal, which he should be doing
himself ;P I actually am not that excited about it, and will probably
not use it for anything myself. But I think:

  - it lets the user accomplish useful things that would not
    otherwise be possible

  - supporting it in "git diff" does not create any danger that was not
    already there

which means that I have no objection to a clean version being applied.

-Peff

Re: backwards compatibility, was Re: [PATCH v1 1/3] Introduce config variable "diff.primer"

[Jay Soffian]

On Mon, Jan 26, 2009 at 6:16 AM, Jeff King <peff@peff.net> wrote:
> I don't want to break existing setups, either. But at some point you
> have to say "this is porcelain, so don't rely on there not being any
> user-triggered effects in its behavior". If porcelain is cast in stone,
> then what is the point in differentiating plumbing from porcelain?
>
> And when the line is blurred (as I think it is in several places)

Aside, AIX has commands that are run both directly or via smit (a
curses-based interface). When smit calls the commands, it passes a
switch to let said commands know that they are being run from smit.
e.g.:

       -J
            This flag is used when the installp command is executed from the
            System Management Interface Tool (SMIT) menus.

Perhaps adding such a concept to those git commands which can be used
in both porcelain and plumbing contexts would be useful for git.

j.

Re: backwards compatibility, was Re: [PATCH v1 1/3] Introduce config variable "diff.primer"

[Jeff King]

On Mon, Jan 26, 2009 at 10:29:17AM -0500, Jay Soffian wrote:

> Aside, AIX has commands that are run both directly or via smit (a
> curses-based interface). When smit calls the commands, it passes a
> switch to let said commands know that they are being run from smit.
> e.g.:
> 
>        -J
>             This flag is used when the installp command is executed from the
>             System Management Interface Tool (SMIT) menus.
> 
> Perhaps adding such a concept to those git commands which can be used
> in both porcelain and plumbing contexts would be useful for git.

Sure, I think that is one of many possible ways that we could
differentiate between confusing plumbing and porcelain; another is
splitting functionality into two similar commands, one of which is
plumbing and one of which is porcelain.

The real problem with plans like that, though, is that there are
_already_ scripts in the wild that don't understand "-J" (or whatever).
My impression from your description above is that "-J" means "don't use
fancy features, because we're being called from the menus". And you
really want the opposite, which is that scripts opt _in_ to fancy
features, not _out_.

But then you have that problem that the _user_ is stuck specifying "OK,
turn on fancy features." And I don't relish the thought of typing "git
diff -J" every time. :)

-Peff

Re: backwards compatibility, was Re: [PATCH v1 1/3] Introduce config variable "diff.primer"

[Jay Soffian]

On Mon, Jan 26, 2009 at 1:48 PM, Jeff King <peff@peff.net> wrote:
> But then you have that problem that the _user_ is stuck specifying "OK,
> turn on fancy features." And I don't relish the thought of typing "git
> diff -J" every time. :)

Well, this issue seems to come up every so often, so the idea would be:

- We're adding a mechanism for scripts to communicate that they need
plumbing context
- Start using it in your scripts when calling git if you rely on a
stable interface
- In the next major release, git may introduce changes to commands
which are not clearly plumbing if you haven't adopted the mechanism

Where mechanism could be a switch, environment variable, etc.
Typically in a network API, the client and server have a way to
negotiate the highest level each supports; that's missing from git,
but seems like it would be useful.

j.

p.s. perhaps you'd prefer -P? :)

Re: backwards compatibility, was Re: [PATCH v1 1/3] Introduce config variable "diff.primer"

[Junio C Hamano]

Jay Soffian <jaysoffian@gmail.com> writes:

> On Mon, Jan 26, 2009 at 1:48 PM, Jeff King <peff@peff.net> wrote:
>> But then you have that problem that the _user_ is stuck specifying "OK,
>> turn on fancy features." And I don't relish the thought of typing "git
>> diff -J" every time. :)
>
> Well, this issue seems to come up every so often, so the idea would be:
>
> - We're adding a mechanism for scripts to communicate that they need
> plumbing context
> - Start using it in your scripts when calling git if you rely on a
> stable interface
> - In the next major release, git may introduce changes to commands
> which are not clearly plumbing if you haven't adopted the mechanism

Where do all of these nonsense come from?  We are not adding any mechanism
for scripts to say they need plumbing context.  By calling plumbing they
are already asking for stable plumbing behaviour.

The scripts can, if they want to, use newer options updated versions of
the plumbing commands offer, by passing them when they want to.

And the trigger to do so is up to the scripts.  They can get new options
from the end user, or they can peek into user's configuration variables
similar to the diff.primer mentioned earlier in the discussion.

One way could be a new option --screw-me-with=name that can be given to a
plumbing command and tells it pretend as if the command line options
specified by the configuration variable of the given name were given
(e.g. a script runs "git diff-files --screw-me-with=diff.primer").

The important point is that it has to be opt _IN_.

Re: backwards compatibility, was Re: [PATCH v1 1/3] Introduce config variable "diff.primer"

[Jay Soffian]

On Mon, Jan 26, 2009 at 3:04 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Where do all of these nonsense come from?  We are not adding any mechanism
> for scripts to say they need plumbing context.  By calling plumbing they
> are already asking for stable plumbing behaviour.

The suggestion was wrt to commands which are not strictly plumbing.

j.

Re: backwards compatibility, was Re: [PATCH v1 1/3] Introduce config variable "diff.primer"

[Jeff King]

On Mon, Jan 26, 2009 at 12:04:20PM -0800, Junio C Hamano wrote:

> > Well, this issue seems to come up every so often, so the idea would be:
> >
> > - We're adding a mechanism for scripts to communicate that they need
> > plumbing context
> > - Start using it in your scripts when calling git if you rely on a
> > stable interface
> > - In the next major release, git may introduce changes to commands
> > which are not clearly plumbing if you haven't adopted the mechanism
> 
> Where do all of these nonsense come from?  We are not adding any mechanism
> for scripts to say they need plumbing context.  By calling plumbing they
> are already asking for stable plumbing behaviour.

I think this is my fault a little for mentioning "blurred plumbing and
porcelain" and not explaining further. This really has nothing to do
with actual plumbing commands. Scripts should use diff-tree and not
diff, and that has always and probably will always be the case.

But what about something like "git grep"? Is it plumbing or porcelain?
The functionality isn't exposed in any other way, so I can imagine that
some scripts are using it. But it's sad to think that we could never
have config that might change its behavior, because it is used directly
by users all the time.  I think the same applies for "git archive".
There may be others.

So something like Jay's proposal could future-proof those commands
better by allowing scripts to say "BTW, I am a script. Turn off your new
features." And there are two classes of alternatives:

  - as you described, instead of making scripts turn _off_ features,
    make them turn them _on_, effectively declaring these commands as
    plumbing. This is obviously much nicer because it Just Works with
    current scripts. But it means that these mixed porcelain/plumbing
    commands suffer in their porcelain capacity; we can never add a
    config option that might change the behavior without the user
    specifying "it's ok to use this feature" at each invocation.

  - we can provide support _now_ for splitting the functionality into
    porcelain and plumbing, scripts can adapt over time to using the
    plumbing version, and then eventually we can declare it safe to make
    changes to the porcelain. And that is more or less what Jay's
    proposal is doing.

    However, I don't think a command-line option is the best way to say
    "I am a script". It's too easy to type "git grep" in a script and
    "git grep -J" (either because you are clueless about "-J", or
    because you simply forget). Other signal methods include:

      - just making two different commands to expose the same
        functionality, one plumbing and one porcelain. This is what has
        evolved in other areas, such as diff (though note that there
        _isn't_ exactly a "git diff" plumbing command -- there is the
        plumbing that "git diff is based on). I'm not sure what the
        plumbing name for "git grep" would be.

      - set an environment variable like GIT_STRICT. It's easy to set
        once at the top of your script, and it trickles down
        automatically as we call other git commands and scripts.
        We could even set it in git-sh-setup, though that of course
        covers only shell scripts, and not other callers.

> The scripts can, if they want to, use newer options updated versions of
> the plumbing commands offer, by passing them when they want to.
> 
> And the trigger to do so is up to the scripts.  They can get new options
> from the end user, or they can peek into user's configuration variables
> similar to the diff.primer mentioned earlier in the discussion.

Right, I think that is absolutely the right thing for commands which are
clearly plumbing.

> One way could be a new option --screw-me-with=name that can be given to a
> plumbing command and tells it pretend as if the command line options
> specified by the configuration variable of the given name were given
> (e.g. a script runs "git diff-files --screw-me-with=diff.primer").
> 
> The important point is that it has to be opt _IN_.

Our precedent so far has been to just add a new command line option that
enables the feature (e.g., --ext-diff and --textconv). Functionally it
is no different than --screw-me-with=diff.*.textconv. :)

-Peff

Re: [PATCH v1 0/3] Introduce config variable "diff.primer"

[Keith Cascio]

On Sun, 25 Jan 2009, Jeff King wrote:

> let's say I have a gitattributes file like this:
>    *.c diff=c
> and my config says:
>   [diff]
>     opt1 = val1_default
>     opt2 = val2_default
>   [diff "c"]
>     opt1 = val1_c
> 
> Now obviously if I want to use opt1 for my C files, it should be val1_c.
> But if I want to use opt2, what should it use? There are two reasonable
> choices, I think:
>   1. You use val2_default. The rationale is that the "c" diff driver did
>      not define an opt2, so you fall back to the global default.
>   2. It is unset. The rationale is that you are using the "c" diff
>      driver, and it has left the value unset. The default then means "if
>      you have no diff driver setup".

I'm in favor of option (2), because [diff] a.k.a [diff ""] serving as the 
fallback for [diff *] feels like a special case.  If a full system of precedence 
and fallbacks is desired for .git/config, we should adopt explicit grammar that 
lets me define an arbitrary precedence tree over all sections.  Once a powerful 
concept is born, somewhere down the road, some user will desire its 
universalization.

Re: backwards compatibility, was Re: [PATCH v1 1/3] Introduce config variable "diff.primer"

[Keith Cascio]

On Mon, 26 Jan 2009, Jeff King wrote:

> Yep, that's what defaults are. And guess what: we _already_ have the same 
> thing. I have diff.renames set in my ~/.gitconfig. That does _exactly_ what
> 
>   git config --global diff.primer -M
> 
> would do. It's just a syntax that saves us from having to introduce a boatload 
> of new variables, one per command line option.


Great point, IOW, primer magnifies already existing pitfalls.  I like that about 
primer v1, however unsatisfying it is otherwise.  Rather than stick a piece of 
chewing gum in that chink in the dam, let's repair it and get the whole darn 
thing ready for the 21st century while we're at it.

> However, there are two other drawbacks of aliases that I can think of:
>   1. They are tied to a specific command, whereas diff options are tied
>      to the concept of diffing. So now I have to write an alias (with a
>      new name) for each command:
>        git config alias.mylog 'log -w'
>        git config alias.mydiff 'diff -w'
>        git config alias.myshow 'show -w'
>   2. They can't change defaults based on the file to be diffed. One of
>      the things Keith mentioned (and I don't remember if this was
>      implemented in his patch series) was supporting this for
>      gitattributes diff drivers. How do I do
>        git config diff.tex.primer -w
>      using aliases?

This scenario was specifically part of my motivation for imagining primer v1 as 
I did.

> But now you have me defending Keith's proposal

And well.

> which he should be doing himself ;P

True.  I'm at the point here where I will demand of myself one of two outcomes.  
Either I:

(1) Satisfy myself on a deep, foundational level why the inescapable structure 
of not just this particularly well-constituted software project (Git!), but 
software projects in general, since surely many a fearless development team has 
braved this philosophical sticky place before us, prevents one from getting 
everything one wants, i.e. forces a compromise, a.k.a. the "no silver bullet" 
interpretation.

(2) Write primer patch v2 that somehow does THE RIGHT THING, also satisfying on 
a deep level to at least myself, a.k.a. the silver bullet.

                                            -- Keith

Re: [PATCH v1 0/3] Introduce config variable "diff.primer"

[Jeff King]

On Mon, Jan 26, 2009 at 05:47:54PM -0800, Keith Cascio wrote:

> >   2. It is unset. The rationale is that you are using the "c" diff
> >      driver, and it has left the value unset. The default then means "if
> >      you have no diff driver setup".
> 
> I'm in favor of option (2), because [diff] a.k.a [diff ""] serving as

Nit: [diff] and [diff ""] are different. The "dotted" notation which we
use in the code and which git-config respects for a variable "foo" in
each section would look like "diff.foo" and "diff..foo", respectively.

> the fallback for [diff *] feels like a special case.  If a full system
> of precedence 

Yes, I think having a "this is the default driver whose driver-specific
options are used if you don't have a different driver" is semantically
simple and clear.

> and fallbacks is desired for .git/config, we should adopt explicit
> grammar that lets me define an arbitrary precedence tree over all
> sections.  Once a powerful concept is born, somewhere down the road,
> some user will desire its universalization.

OK, now you're scaring me. :) I'm not sure I want to see the grammar you
would use to define an arbitrary precedence tree, or whether such
complexity has any real-world use in git config. I think you would have
to show a concrete example to prove the utility of something like that.

-Peff