{"thread":{"id":"17354","subject":"[PATCH v1 0/3] Introduce config variable \"diff.primer\"","startedAt":"2009-01-25T17:30:54Z","lastAt":"2009-01-27T04:54:52Z","messageCount":41,"participants":["Keith Cascio","Johannes Schindelin","Junio C Hamano","Jeff King","Jay Soffian"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"101843","messageId":"1232904657-31831-1-git-send-email-keith@cs.ucla.edu","threadId":"17354","inReplyTo":null,"subject":"[PATCH v1 0/3] Introduce config variable \"diff.primer\"","fromName":"Keith Cascio","fromEmail":"keith@cs.ucla.edu","sentAt":"2009-01-25T17:30:54Z","receivedAt":"2009-01-25T17:30:54Z","isPatch":true,"sender":{"key":"keith@cs.ucla.edu","avatar":"https://gravatar.com/avatar/c5ec3a8f1cd1f449fdf8bdb7125fdbfd10b729507f32cbf0aa4ad07b4f7127ae?d=mp&s=160"},"body":"The next three patches introduce a way to specify diff options\ngit always obeys. Then use the new feature to\nenhance git-gui with white space ignore settings.  The fastest\nway to see this patch in action is: apply all three patches,\nfire up git-gui, modify a file, then right-click on the diff\npanel and look for the new \"White Space\" sub-menu.\n\nFuture work: Extend the gitattributes mechanism so it supports\nall [diff] config variables, including e.g. diff.mnemonicprefix\nand diff.primer.\n\nKeith Cascio (3):\n Introduce config variable \"diff.primer\"\n Test functionality of new config variable \"diff.primer\"\n git-gui hooks for new config variable \"diff.primer\"\n\n Documentation/config.txt       |   14 +++++\n Documentation/diff-options.txt |   13 +++++\n Makefile                       |    2 +\n builtin-log.c                  |    1 +\n diff.c                         |   83 ++++++++++++++++++++++++++----\n diff.h                         |   15 ++++-\n git-gui/git-gui.sh             |   51 ++++++++++++++++++\n git-gui/lib/diff.tcl           |    8 ++-\n git-gui/lib/option.tcl         |   57 +++++++++++++++++++--\n gitk-git/gitk                  |   16 +++---\n t/t4033-diff-primer.sh         |  111 ++++++++++++++++++++++++++++++++++++++++\n 11 files changed, 343 insertions(+), 28 deletions(-)\n create mode 100755 t/t4033-diff-primer.sh\n"},{"id":"101846","messageId":"1232904657-31831-2-git-send-email-keith@cs.ucla.edu","threadId":"17354","inReplyTo":"1232904657-31831-1-git-send-email-keith@cs.ucla.edu","subject":"[PATCH v1 1/3] Introduce config variable \"diff.primer\"","fromName":"Keith Cascio","fromEmail":"keith@cs.ucla.edu","sentAt":"2009-01-25T17:30:55Z","receivedAt":"2009-01-25T17:30:55Z","isPatch":true,"sender":{"key":"keith@cs.ucla.edu","avatar":"https://gravatar.com/avatar/c5ec3a8f1cd1f449fdf8bdb7125fdbfd10b729507f32cbf0aa4ad07b4f7127ae?d=mp&s=160"},"body":"Introduce config variable \"diff.primer\".\nAllows user to specify arbitrary options\nto pass to diff on every invocation,\nincluding internal invocations from other\nprograms, e.g. git-gui.\nIntroduce diff command-line options:\n--no-primer, --machine-friendly\nProtect git-format-patch, git-apply,\ngit-am, git-rebase, git-gui and gitk\nfrom inapplicable options.\n\nSigned-off-by: Keith Cascio <keith@cs.ucla.edu>\n---\n Documentation/config.txt       |   14 +++++++\n Documentation/diff-options.txt |   13 ++++++\n Makefile                       |    2 +\n builtin-log.c                  |    1 +\n diff.c                         |   83 +++++++++++++++++++++++++++++++++++-----\n diff.h                         |   15 ++++++-\n git-gui/lib/diff.tcl           |    8 +++-\n gitk-git/gitk                  |   16 ++++----\n 8 files changed, 129 insertions(+), 23 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 290cb48..dd00f98 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -601,6 +601,20 @@ diff.autorefreshindex::\n \taffects only 'git-diff' Porcelain, and not lower level\n \t'diff' commands, such as 'git-diff-files'.\n \n+diff.primer::\n+\tWhitespace-separated list of options to pass to 'git-diff'\n+\ton every invocation, including internal invocations from\n+\tlinkgit:git-gui[1] and linkgit:gitk[1],\n+\te.g. `\"--color --ignore-space-at-eol --exit-code\"`.\n+\tSee linkgit:git-diff[1]. You can override these at run time with the\n+\tdiff option --no-primer.  Supports a subset of\n+\t'git-diff'\\'s many options, at least:\n+\t`-b --binary --color --color-words --cumulative --dirstat-by-file\n+--exit-code --ext-diff --find-copies-harder --follow --full-index\n+--ignore-all-space --ignore-space-at-eol --ignore-space-change\n+--ignore-submodules --no-color --no-ext-diff --no-textconv -q --quiet -R -r\n+--relative -t --text --textconv -w`\n+\n diff.external::\n \tIf this config variable is set, diff generation is not\n \tperformed using the internal diff machinery, but using the\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex 1f8ce97..4d12359 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -240,5 +240,18 @@ endif::git-format-patch[]\n --no-prefix::\n \tDo not show any source or destination prefix.\n \n+--no-primer::\n+\tIgnore default options specified in '.git/config', i.e.\n+\tthose that were set using a command like\n+\t`git config diff.primer \"--color --ignore-space-at-eol --exit-code\"`\n+\n+--machine-friendly::\n+\tDeclaratively override and turn off all diff options that alter patch\n+\toutput in a way not suitable for input to a program that expects\n+\ta canonical patch.  For example, `--color`, and the whitespace ignore\n+\toptions `-w`, `-b` and `--ignore-space-at-eol`.  Important when\n+\t'git-format-patch' generates output for 'git-apply' or 'git-am', for\n+\texample in the context of 'git-rebase'.\n+\n For more detailed explanation on these common options, see also\n linkgit:gitdiffcore[7].\ndiff --git a/Makefile b/Makefile\nindex b4d9cb4..195f984 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1279,6 +1279,8 @@ git-http-push$X: revision.o http.o http-push.o $(GITLIBS)\n \t$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \\\n \t\t$(LIBS) $(CURL_LIBCURL) $(EXPAT_LIBEXPAT)\n \n+diff.h: xdiff/xdiff.h\n+\n $(LIB_OBJS) $(BUILTIN_OBJS): $(LIB_H)\n $(patsubst git-%$X,%.o,$(PROGRAMS)): $(LIB_H) $(wildcard */*.h)\n builtin-revert.o wt-status.o: wt-status.h\ndiff --git a/builtin-log.c b/builtin-log.c\nindex 2ae39af..b385e35 100644\n--- a/builtin-log.c\n+++ b/builtin-log.c\n@@ -784,6 +784,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \trev.combine_merges = 0;\n \trev.ignore_merges = 1;\n \tDIFF_OPT_SET(&rev.diffopt, RECURSIVE);\n+\tDIFF_OPT_SET(&rev.diffopt, MACHINE_FRIENDLY);\n \n \trev.subject_prefix = fmt_patch_subject_prefix;\n \ndiff --git a/diff.c b/diff.c\nindex 82cff97..a8c103f 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -24,6 +24,8 @@ static int diff_rename_limit_default = 200;\n static int diff_suppress_blank_empty;\n int diff_use_color_default = -1;\n static const char *external_diff_cmd_cfg;\n+static const char     *diff_primer;\n+static struct diff_options *primer;\n int diff_auto_refresh_index = 1;\n static int diff_mnemonic_prefix;\n \n@@ -102,6 +104,8 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)\n \t\tdiff_rename_limit_default = git_config_int(var, value);\n \t\treturn 0;\n \t}\n+\tif (!strcmp(var, \"diff.primer\"))\n+\t\treturn git_config_string(& diff_primer, var, value);\n \n \tswitch (userdiff_config(var, value)) {\n \t\tcase 0: break;\n@@ -2215,6 +2219,46 @@ static void run_checkdiff(struct diff_filepair *p, struct diff_options *o)\n \tbuiltin_checkdiff(name, other, attr_path, p->one, p->two, o);\n }\n \n+static const char blank[] = \" \\t\\r\\n\";\n+\n+void set_diff_primer(struct diff_options *options)\n+{\n+\tchar  *str1, *token, *saveptr;\n+\tint    len;\n+\n+\tif((DIFF_OPT_TST(options, SUPPRESS_PRIMER)) ||\n+\t   (!              diff_primer            ) ||\n+\t   ((len = (strlen(diff_primer)+1)) < 3   )){ return; }\n+\n+\ttoken = str1 = strncpy( (char*) malloc(len), diff_primer, len );\n+\tif( (           saveptr = strpbrk( token += strspn( token, blank ), blank )) ){ *(saveptr++) = '\\0'; }\n+\twhile( token ){\n+\t  if( *token == '-'      ){\n+\t    diff_opt_parse( options, (const char **) &token, -1 );\n+\t  }\n+\t  if( (token  = saveptr) ){\n+\t    if( (       saveptr = strpbrk( token += strspn( token, blank ), blank )) ){ *(saveptr++) = '\\0'; }\n+\t  }\n+\t}\n+\n+\tfree( str1 );\n+}\n+\n+struct diff_options* flatten_diff_options( struct diff_options *master, struct diff_options *slave )\n+{\n+\tunsigned   x0 = master->flags   , x1 = master->mask    , x2 = slave->flags   , x3 = slave->mask;\n+\tlong       w  = master->xdl_opts, x  = master->xdl_mask, y  = slave->xdl_opts, z  = slave->xdl_mask;\n+\n+\t//minimized by Quine-McClusk\n+\tmaster->flags    = (~x1&x2&x3)|(x0&~x3)|(x0&x1);\n+\tmaster->mask     = x1 | x3;\n+\n+\tmaster->xdl_opts = (~x &y &z )|(w &~z )|(w &x );\n+\tmaster->xdl_mask = x  | z ;\n+\n+\treturn master;\n+}\n+\n void diff_setup(struct diff_options *options)\n {\n \tmemset(options, 0, sizeof(*options));\n@@ -2225,15 +2269,16 @@ void diff_setup(struct diff_options *options)\n \toptions->break_opt = -1;\n \toptions->rename_limit = -1;\n \toptions->dirstat_percent = 3;\n-\tDIFF_OPT_CLR(options, DIRSTAT_CUMULATIVE);\n+\tif( DIFF_OPT_TST(options, DIRSTAT_CUMULATIVE))\n+\t    DIFF_OPT_CLR(options, DIRSTAT_CUMULATIVE);\n \toptions->context = 3;\n \n \toptions->change = diff_change;\n \toptions->add_remove = diff_addremove;\n \tif (diff_use_color_default > 0)\n-\t\tDIFF_OPT_SET(options, COLOR_DIFF);\n-\telse\n-\t\tDIFF_OPT_CLR(options, COLOR_DIFF);\n+\t\t DIFF_OPT_SET(options, COLOR_DIFF);\n+\telse if( DIFF_OPT_TST(options, COLOR_DIFF))\n+\t\t DIFF_OPT_CLR(options, COLOR_DIFF);\n \toptions->detect_rename = diff_detect_rename_default;\n \n \tif (!diff_mnemonic_prefix) {\n@@ -2322,6 +2367,18 @@ int diff_setup_done(struct diff_options *options)\n \t\tDIFF_OPT_SET(options, EXIT_WITH_STATUS);\n \t}\n \n+\tif( ! DIFF_OPT_TST(      options, SUPPRESS_PRIMER ) ){\n+\t  if( !                           primer ){\n+\t    diff_setup(                   primer = (struct diff_options *) malloc( sizeof(struct diff_options) ) );\n+\t    set_diff_primer(              primer );\n+\t  }\n+\t  flatten_diff_options(  options, primer );\n+\t}\n+\n+\tif(   DIFF_OPT_TST(      options, MACHINE_FRIENDLY ) ){\n+\t  DIFF_MACHINE_FRIENDLY( options );\n+\t}\n+\n \treturn 0;\n }\n \n@@ -2469,13 +2526,13 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)\n \n \t/* xdiff options */\n \telse if (!strcmp(arg, \"-w\") || !strcmp(arg, \"--ignore-all-space\"))\n-\t\toptions->xdl_opts |= XDF_IGNORE_WHITESPACE;\n+\t\tDIFF_XDL_SET(options, IGNORE_WHITESPACE);\n \telse if (!strcmp(arg, \"-b\") || !strcmp(arg, \"--ignore-space-change\"))\n-\t\toptions->xdl_opts |= XDF_IGNORE_WHITESPACE_CHANGE;\n+\t\tDIFF_XDL_SET(options, IGNORE_WHITESPACE_CHANGE);\n \telse if (!strcmp(arg, \"--ignore-space-at-eol\"))\n-\t\toptions->xdl_opts |= XDF_IGNORE_WHITESPACE_AT_EOL;\n+\t\tDIFF_XDL_SET(options, IGNORE_WHITESPACE_AT_EOL);\n \telse if (!strcmp(arg, \"--patience\"))\n-\t\toptions->xdl_opts |= XDF_PATIENCE_DIFF;\n+\t\tDIFF_XDL_SET(options, PATIENCE_DIFF);\n \n \t/* flags options */\n \telse if (!strcmp(arg, \"--binary\")) {\n@@ -2496,8 +2553,10 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)\n \t\tDIFF_OPT_SET(options, COLOR_DIFF);\n \telse if (!strcmp(arg, \"--no-color\"))\n \t\tDIFF_OPT_CLR(options, COLOR_DIFF);\n-\telse if (!strcmp(arg, \"--color-words\"))\n-\t\toptions->flags |= DIFF_OPT_COLOR_DIFF | DIFF_OPT_COLOR_DIFF_WORDS;\n+\telse if (!strcmp(arg, \"--color-words\")){\n+\t\tDIFF_OPT_SET(options, COLOR_DIFF);\n+\t\tDIFF_OPT_SET(options, COLOR_DIFF_WORDS);\n+\t}\n \telse if (!strcmp(arg, \"--exit-code\"))\n \t\tDIFF_OPT_SET(options, EXIT_WITH_STATUS);\n \telse if (!strcmp(arg, \"--quiet\"))\n@@ -2512,6 +2571,10 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)\n \t\tDIFF_OPT_CLR(options, ALLOW_TEXTCONV);\n \telse if (!strcmp(arg, \"--ignore-submodules\"))\n \t\tDIFF_OPT_SET(options, IGNORE_SUBMODULES);\n+\telse if (!strcmp(arg, \"--no-primer\"))\n+\t\tDIFF_OPT_SET(options, SUPPRESS_PRIMER);\n+\telse if (!strcmp(arg, \"--machine-friendly\"))\n+\t\tDIFF_OPT_SET(options, MACHINE_FRIENDLY);\n \n \t/* misc options */\n \telse if (!strcmp(arg, \"-z\"))\ndiff --git a/diff.h b/diff.h\nindex 4d5a327..e98c23a 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -5,6 +5,7 @@\n #define DIFF_H\n \n #include \"tree-walk.h\"\n+#include \"xdiff/xdiff.h\"\n \n struct rev_info;\n struct diff_options;\n@@ -66,9 +67,15 @@ typedef void (*diff_format_fn_t)(struct diff_queue_struct *q,\n #define DIFF_OPT_DIRSTAT_CUMULATIVE  (1 << 19)\n #define DIFF_OPT_DIRSTAT_BY_FILE     (1 << 20)\n #define DIFF_OPT_ALLOW_TEXTCONV      (1 << 21)\n-#define DIFF_OPT_TST(opts, flag)    ((opts)->flags & DIFF_OPT_##flag)\n-#define DIFF_OPT_SET(opts, flag)    ((opts)->flags |= DIFF_OPT_##flag)\n-#define DIFF_OPT_CLR(opts, flag)    ((opts)->flags &= ~DIFF_OPT_##flag)\n+#define DIFF_OPT_SUPPRESS_PRIMER     (1 << 22)\n+#define DIFF_OPT_MACHINE_FRIENDLY    (1 << 23)\n+#define DIFF_OPT_TST(opts, flag)    ((opts)->flags &   DIFF_OPT_##flag)\n+#define DIFF_OPT_SET(opts, flag)    ((opts)->flags |=  DIFF_OPT_##flag), ((opts)->mask |= DIFF_OPT_##flag)\n+#define DIFF_OPT_CLR(opts, flag)    ((opts)->flags &= ~DIFF_OPT_##flag), ((opts)->mask |= DIFF_OPT_##flag)\n+#define DIFF_XDL_TST(opts, flag)    ((opts)->xdl_opts &   XDF_##flag)\n+#define DIFF_XDL_SET(opts, flag)    ((opts)->xdl_opts |=  XDF_##flag), ((opts)->xdl_mask |= XDF_##flag)\n+#define DIFF_XDL_CLR(opts, flag)    ((opts)->xdl_opts &= ~XDF_##flag), ((opts)->xdl_mask |= XDF_##flag)\n+#define DIFF_MACHINE_FRIENDLY(opts) ((opts)->flags &= ~(DIFF_OPT_COLOR_DIFF)), ((opts)->xdl_opts &= ~(XDF_WHITESPACE_FLAGS))\n \n struct diff_options {\n \tconst char *filter;\n@@ -77,6 +84,7 @@ struct diff_options {\n \tconst char *single_follow;\n \tconst char *a_prefix, *b_prefix;\n \tunsigned flags;\n+\tunsigned  mask;\n \tint context;\n \tint interhunkcontext;\n \tint break_opt;\n@@ -95,6 +103,7 @@ struct diff_options {\n \tint prefix_length;\n \tconst char *stat_sep;\n \tlong xdl_opts;\n+\tlong xdl_mask;\n \n \tint stat_width;\n \tint stat_name_width;\ndiff --git a/git-gui/lib/diff.tcl b/git-gui/lib/diff.tcl\nindex bbbf15c..94faf95 100644\n--- a/git-gui/lib/diff.tcl\n+++ b/git-gui/lib/diff.tcl\n@@ -276,6 +276,7 @@ proc start_show_diff {cont_info {add_opts {}}} {\n \t}\n \n \tlappend cmd -p\n+\tlappend cmd --exit-code\n \tlappend cmd --no-color\n \tif {$repo_config(gui.diffcontext) >= 1} {\n \t\tlappend cmd \"-U$repo_config(gui.diffcontext)\"\n@@ -310,6 +311,7 @@ proc read_diff {fd cont_info} {\n \tglobal ui_diff diff_active\n \tglobal is_3way_diff is_conflict_diff current_diff_header\n \tglobal current_diff_queue\n+\tglobal errorCode\n \n \t$ui_diff conf -state normal\n \twhile {[gets $fd line] >= 0} {\n@@ -397,7 +399,9 @@ proc read_diff {fd cont_info} {\n \t$ui_diff conf -state disabled\n \n \tif {[eof $fd]} {\n-\t\tclose $fd\n+\t\tfconfigure $fd -blocking 1\n+\t\tcatch { close $fd } err\n+\t\tset diff_exit_status $errorCode\n \n \t\tif {$current_diff_queue ne {}} {\n \t\t\tadvance_diff_queue $cont_info\n@@ -413,7 +417,7 @@ proc read_diff {fd cont_info} {\n \t\t}\n \t\tui_ready\n \n-\t\tif {[$ui_diff index end] eq {2.0}} {\n+\t\tif {$diff_exit_status eq \"NONE\"} {\n \t\t\thandle_empty_diff\n \t\t}\n \t\tset callback [lindex $cont_info 1]\ndiff --git a/gitk-git/gitk b/gitk-git/gitk\nindex dc2a439..49e5cb7 100644\n--- a/gitk-git/gitk\n+++ b/gitk-git/gitk\n@@ -4259,7 +4259,7 @@ proc do_file_hl {serial} {\n \t# must be \"containing:\", i.e. we're searching commit info\n \treturn\n     }\n-    set cmd [concat | git diff-tree -r -s --stdin $gdtargs]\n+    set cmd [concat | git diff-tree --no-color -r -s --stdin $gdtargs]\n     set filehighlight [open $cmd r+]\n     fconfigure $filehighlight -blocking 0\n     filerun $filehighlight readfhighlight\n@@ -4753,7 +4753,7 @@ proc dodiffindex {} {\n \n     if {!$showlocalchanges || !$isworktree} return\n     incr lserial\n-    set cmd \"|git diff-index --cached HEAD\"\n+    set cmd \"|git diff-index --no-color --cached HEAD\"\n     if {$vfilelimit($curview) ne {}} {\n \tset cmd [concat $cmd -- $vfilelimit($curview)]\n     }\n@@ -4782,7 +4782,7 @@ proc readdiffindex {fd serial inst} {\n     }\n \n     # now see if there are any local changes not checked in to the index\n-    set cmd \"|git diff-files\"\n+    set cmd \"|git diff-files --no-color\"\n     if {$vfilelimit($curview) ne {}} {\n \tset cmd [concat $cmd -- $vfilelimit($curview)]\n     }\n@@ -7068,7 +7068,7 @@ proc diffcmd {ids flags} {\n     if {$i >= 0} {\n \tif {[llength $ids] > 1 && $j < 0} {\n \t    # comparing working directory with some specific revision\n-\t    set cmd [concat | git diff-index $flags]\n+\t    set cmd [concat | git diff-index --no-color $flags]\n \t    if {$i == 0} {\n \t\tlappend cmd -R [lindex $ids 1]\n \t    } else {\n@@ -7076,13 +7076,13 @@ proc diffcmd {ids flags} {\n \t    }\n \t} else {\n \t    # comparing working directory with index\n-\t    set cmd [concat | git diff-files $flags]\n+\t    set cmd [concat | git diff-files --no-color $flags]\n \t    if {$j == 1} {\n \t\tlappend cmd -R\n \t    }\n \t}\n     } elseif {$j >= 0} {\n-\tset cmd [concat | git diff-index --cached $flags]\n+\tset cmd [concat | git diff-index --no-color --cached $flags]\n \tif {[llength $ids] > 1} {\n \t    # comparing index with specific revision\n \t    if {$i == 0} {\n@@ -7095,7 +7095,7 @@ proc diffcmd {ids flags} {\n \t    lappend cmd HEAD\n \t}\n     } else {\n-\tset cmd [concat | git diff-tree -r $flags $ids]\n+\tset cmd [concat | git diff-tree --no-color -r $flags $ids]\n     }\n     return $cmd\n }\n@@ -10657,7 +10657,7 @@ if {[catch {package require Tk 8.4} err]} {\n }\n \n # defaults...\n-set wrcomcmd \"git diff-tree --stdin -p --pretty\"\n+set wrcomcmd \"git diff-tree --no-color --stdin -p --pretty\"\n \n set gitencoding {}\n catch {\n-- \n1.6.1\n"},{"id":"101844","messageId":"1232904657-31831-3-git-send-email-keith@cs.ucla.edu","threadId":"17354","inReplyTo":"1232904657-31831-2-git-send-email-keith@cs.ucla.edu","subject":"[PATCH v1 2/3] Test functionality of new config variable \"diff.primer\"","fromName":"Keith Cascio","fromEmail":"keith@cs.ucla.edu","sentAt":"2009-01-25T17:30:56Z","receivedAt":"2009-01-25T17:30:56Z","isPatch":true,"sender":{"key":"keith@cs.ucla.edu","avatar":"https://gravatar.com/avatar/c5ec3a8f1cd1f449fdf8bdb7125fdbfd10b729507f32cbf0aa4ad07b4f7127ae?d=mp&s=160"},"body":"Test functionality of new config variable \"diff.primer\"\n\nSigned-off-by: Keith Cascio <keith@cs.ucla.edu>\n---\n t/t4033-diff-primer.sh |  111 ++++++++++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 111 insertions(+), 0 deletions(-)\n create mode 100755 t/t4033-diff-primer.sh\n\ndiff --git a/t/t4033-diff-primer.sh b/t/t4033-diff-primer.sh\nnew file mode 100755\nindex 0000000..116d6ad\n--- /dev/null\n+++ b/t/t4033-diff-primer.sh\n@@ -0,0 +1,111 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2009 Keith G. Cascio\n+#\n+# based on t4015-diff-whitespace.sh by Johannes E. Schindelin\n+#\n+\n+test_description='Ensure diff engine honors config variable \"diff.primer\".\n+\n+'\n+. ./test-lib.sh\n+. \"$TEST_DIRECTORY\"/diff-lib.sh\n+\n+tr 'Q' '\\015' << EOF > x\n+whitespace at beginning\n+whitespace change\n+whitespace in the middle\n+whitespace at end\n+unchanged line\n+CR at endQ\n+EOF\n+\n+git add x\n+git commit -m '1.0' >/dev/null 2>&1\n+\n+tr '_' ' ' << EOF > x\n+\twhitespace at beginning\n+whitespace \t change\n+white space in the middle\n+whitespace at end__\n+unchanged line\n+CR at end\n+EOF\n+\n+test_expect_success 'ensure diff.primer begins with empty value' '\n+[ -z $(git config --get diff.primer) ]\n+'\n+\n+tr 'Q_' '\\015 ' << EOF > expect\n+diff --git a/x b/x\n+index d99af23..8b32fb5 100644\n+--- a/x\n++++ b/x\n+@@ -1,6 +1,6 @@\n+-whitespace at beginning\n+-whitespace change\n+-whitespace in the middle\n+-whitespace at end\n++\twhitespace at beginning\n++whitespace \t change\n++white space in the middle\n++whitespace at end__\n+ unchanged line\n+-CR at endQ\n++CR at end\n+EOF\n+git diff > out\n+test_expect_success 'test diff with empty value of diff.primer' 'test_cmp expect out'\n+\n+git config diff.primer '-w'\n+\n+test_expect_success 'ensure diff.primer value set' '\n+[ $(git config --get diff.primer) = \"-w\" ]\n+'\n+\n+git diff --no-primer > out\n+test_expect_success 'test git diff --no-primer' 'test_cmp expect out'\n+\n+cat << EOF > expect\n+diff --git a/x b/x\n+index d99af23..8b32fb5 100644\n+EOF\n+git diff > out\n+test_expect_success 'test with diff.primer = -w' 'test_cmp expect out'\n+\n+git add x\n+git commit -m 'whitespace changes' >/dev/null 2>&1\n+\n+git config diff.primer '-w --color'\n+\n+tr 'Q_' '\\015 ' << EOF > expect\n+Subject: [PATCH] whitespace changes\n+\n+---\n+ x |   10 +++++-----\n+ 1 files changed, 5 insertions(+), 5 deletions(-)\n+\n+diff --git a/x b/x\n+index d99af23..8b32fb5 100644\n+--- a/x\n++++ b/x\n+@@ -1,6 +1,6 @@\n+-whitespace at beginning\n+-whitespace change\n+-whitespace in the middle\n+-whitespace at end\n++\twhitespace at beginning\n++whitespace \t change\n++white space in the middle\n++whitespace at end__\n+ unchanged line\n+-CR at endQ\n++CR at end\n+--_\n+EOF\n+\n+git format-patch --stdout HEAD^..HEAD 2>&1 | sed -re '1,3d;$d' | sed -re '$d' > out\n+test_expect_success 'ensure git format-patch not affected by diff.primer' 'test_cmp expect out'\n+\n+test_done\n+\n-- \n1.6.1\n"},{"id":"101845","messageId":"1232904657-31831-4-git-send-email-keith@cs.ucla.edu","threadId":"17354","inReplyTo":"1232904657-31831-3-git-send-email-keith@cs.ucla.edu","subject":"[PATCH v1 3/3] git-gui hooks for new config variable \"diff.primer\"","fromName":"Keith Cascio","fromEmail":"keith@cs.ucla.edu","sentAt":"2009-01-25T17:30:57Z","receivedAt":"2009-01-25T17:30:57Z","isPatch":true,"sender":{"key":"keith@cs.ucla.edu","avatar":"https://gravatar.com/avatar/c5ec3a8f1cd1f449fdf8bdb7125fdbfd10b729507f32cbf0aa4ad07b4f7127ae?d=mp&s=160"},"body":"git-gui hooks for new config variable \"diff.primer\".\nAdd three checkboxes to both sides of\noptions panel (local/global).\nAdd a sub-menu named \"White Space\" to\ndiff-panel right-click context menu, with\nthree checkboxes.\n\nSigned-off-by: Keith Cascio <keith@cs.ucla.edu>\n---\n git-gui/git-gui.sh     |   51 ++++++++++++++++++++++++++++++++++++++++++\n git-gui/lib/option.tcl |   57 +++++++++++++++++++++++++++++++++++++++++++----\n 2 files changed, 103 insertions(+), 5 deletions(-)\n\ndiff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh\nindex e018e07..5d93351 100755\n--- a/git-gui/git-gui.sh\n+++ b/git-gui/git-gui.sh\n@@ -3075,10 +3075,43 @@ $ui_diff tag conf d>>>>>>> \\\n \n $ui_diff tag raise sel\n \n+proc mirror_diff_state {} {\n+\tglobal  diff__ignore_space_at_eol diff__ignore_space_change diff__ignore_all_space\n+\n+\tset key  \"diff.primer\"\n+\tset ddo [git config --get $key]\n+\tset diff__ignore_space_at_eol [expr {[string match \"*--ignore-space-at-eol*\" $ddo] ? \"true\" : \"false\"}]\n+\tset diff__ignore_space_change [expr {[string match \"*--ignore-space-change*\" $ddo] ? \"true\" : \"false\"}]\n+\tset diff__ignore_all_space    [expr {[string match \"*--ignore-all-space*\"    $ddo] ? \"true\" : \"false\"}]\n+}\n+\n+proc adjust_command_line { flag value str } {\n+\tif {$value eq \"true\"} {\n+\t  if { ! [string match \"*$flag*\" $str ] } {\n+\t    set              str [concat $str $flag] }\n+\t} else { regsub       -- $flag   $str \"\" str }\n+\treturn                           $str\n+}\n+\n+proc record_diff_state {} {\n+\tglobal  diff__ignore_space_at_eol diff__ignore_space_change diff__ignore_all_space\n+\n+\tset key  \"diff.primer\"\n+\tset ddo [git config --get $key]\n+\tset ddo [adjust_command_line --ignore-space-at-eol $diff__ignore_space_at_eol $ddo]\n+\tset ddo [adjust_command_line --ignore-space-change $diff__ignore_space_change $ddo]\n+\tset ddo [adjust_command_line --ignore-all-space    $diff__ignore_all_space    $ddo]\n+\n+\tgit config $key $ddo\n+\treshow_diff\n+}\n+\n # -- Diff Body Context Menu\n #\n \n proc create_common_diff_popup {ctxm} {\n+\tglobal  diff__ignore_space_at_eol diff__ignore_space_change diff__ignore_all_space\n+\n \t$ctxm add command \\\n \t\t-label [mc \"Show Less Context\"] \\\n \t\t-command show_less_context\n@@ -3087,6 +3120,24 @@ proc create_common_diff_popup {ctxm} {\n \t\t-label [mc \"Show More Context\"] \\\n \t\t-command show_more_context\n \tlappend diff_actions [list $ctxm entryconf [$ctxm index last] -state]\n+\tmirror_diff_state\n+\tset whitespacemenu $ctxm.ws\n+\tmenu $whitespacemenu -postcommand mirror_diff_state\n+\t$ctxm add cascade \\\n+\t\t-label [mc \"White Space\"] \\\n+\t\t-menu $whitespacemenu\n+\t$whitespacemenu add checkbutton \\\n+\t\t-label [mc \"--ignore-space-at-eol\"] \\\n+\t\t-variable diff__ignore_space_at_eol -onvalue \"true\" -offvalue \"false\" \\\n+\t\t-command record_diff_state\n+\t$whitespacemenu add checkbutton \\\n+\t\t-label [mc \"--ignore-space-change\"] \\\n+\t\t-variable diff__ignore_space_change -onvalue \"true\" -offvalue \"false\" \\\n+\t\t-command record_diff_state\n+\t$whitespacemenu add checkbutton \\\n+\t\t-label [mc \"--ignore-all-space\"   ] \\\n+\t\t-variable diff__ignore_all_space    -onvalue \"true\" -offvalue \"false\" \\\n+\t\t-command record_diff_state\n \t$ctxm add separator\n \t$ctxm add command \\\n \t\t-label [mc Refresh] \\\ndiff --git a/git-gui/lib/option.tcl b/git-gui/lib/option.tcl\nindex 1d55b49..fbdf4e8 100644\n--- a/git-gui/lib/option.tcl\n+++ b/git-gui/lib/option.tcl\n@@ -28,6 +28,7 @@ proc save_config {} {\n \tglobal repo_config global_config system_config\n \tglobal repo_config_new global_config_new\n \tglobal ui_comm_spell\n+\tglobal ddo diff_primer_global diff_primer_repo pseudovariables\n \n \tforeach option $font_descs {\n \t\tset name [lindex $option 0]\n@@ -46,17 +47,40 @@ proc save_config {} {\n \t\tunset global_config_new(gui.$font^^size)\n \t}\n \n+\tforeach name [get_diff_primer] {\n+\t\tset diff_option [string range $name 8 [string length $name]]\n+\t\tset ifound      [lsearch     $diff_primer_global $diff_option]\n+\t\tif {$global_config_new($name) eq \"true\"} {\n+\t\t  if {$ifound <  0} { lappend diff_primer_global $diff_option }\n+\t\t} else {\n+\t\t  if {$ifound >= 0} { set     diff_primer_global [lreplace $diff_primer_global $ifound $ifound]}\n+\t\t}\n+\t\tset ifound      [lsearch     $diff_primer_repo   $diff_option]\n+\t\tif {  $repo_config_new($name) eq \"true\"} {\n+\t\t  if {$ifound <  0} { lappend diff_primer_repo   $diff_option }\n+\t\t} else {\n+\t\t  if {$ifound >= 0} { set     diff_primer_repo   [lreplace $diff_primer_repo   $ifound $ifound]}\n+\t\t}\n+\t}\n+\tarray unset default_config gui.diff--ignore-*\n+\tset    default_config($ddo) \"\"\n+\tset global_config_new($ddo) [join    $diff_primer_global]\n+\tset   repo_config_new($ddo) [join    $diff_primer_repo  ]\n+\n \tforeach name [array names default_config] {\n \t\tset value $global_config_new($name)\n-\t\tif {$value ne $global_config($name)} {\n-\t\t\tif {$value eq $system_config($name)} {\n+\t\tset value_global [expr {[info exists global_config($name)] ? $global_config($name) : \"\"}]\n+\t\tset value_system [expr {[info exists system_config($name)] ? $system_config($name) : \"\"}]\n+\t\tset value_repo   [expr {[info exists   repo_config($name)] ?   $repo_config($name) : \"\"}]\n+\t\tif {$value ne $value_global} {\n+\t\t\tif {$value eq $value_system} {\n \t\t\t\tcatch {git config --global --unset $name}\n \t\t\t} else {\n \t\t\t\tregsub -all \"\\[{}\\]\" $value {\"} value\n \t\t\t\tgit config --global $name $value\n \t\t\t}\n \t\t\tset global_config($name) $value\n-\t\t\tif {$value eq $repo_config($name)} {\n+\t\t\tif {$value eq $value_repo} {\n \t\t\t\tcatch {git config --unset $name}\n \t\t\t\tset repo_config($name) $value\n \t\t\t}\n@@ -65,8 +89,10 @@ proc save_config {} {\n \n \tforeach name [array names default_config] {\n \t\tset value $repo_config_new($name)\n-\t\tif {$value ne $repo_config($name)} {\n-\t\t\tif {$value eq $global_config($name)} {\n+\t\tset value_global [expr {[info exists global_config($name)] ? $global_config($name) : \"\"}]\n+\t\tset value_repo   [expr {[info exists   repo_config($name)] ?   $repo_config($name) : \"\"}]\n+\t\tif {$value ne $value_repo} {\n+\t\t\tif {$value eq $value_global} {\n \t\t\t\tcatch {git config --unset $name}\n \t\t\t} else {\n \t\t\t\tregsub -all \"\\[{}\\]\" $value {\"} value\n@@ -88,10 +114,23 @@ proc save_config {} {\n \t}\n }\n \n+proc get_diff_primer {} {\n+\tglobal repo_config global_config\n+\tglobal ddo diff_primer_global diff_primer_repo pseudovariables\n+\n+\tset ddo \"diff.primer\"\n+\tset diff_primer_global [expr {[info exists global_config($ddo)] ? [split $global_config($ddo)] : [list]}]\n+\tset diff_primer_repo   [expr {[info exists   repo_config($ddo)] ? [split   $repo_config($ddo)] : [list]}]\n+\tset pseudovariables [list \"gui.diff--ignore-space-at-eol\" \"gui.diff--ignore-space-change\" \"gui.diff--ignore-all-space\"]\n+\n+\treturn $pseudovariables\n+}\n+\n proc do_options {} {\n \tglobal repo_config global_config font_descs\n \tglobal repo_config_new global_config_new\n \tglobal ui_comm_spell\n+\tglobal ddo diff_primer_global diff_primer_repo pseudovariables\n \n \tarray unset repo_config_new\n \tarray unset global_config_new\n@@ -108,6 +147,11 @@ proc do_options {} {\n \tforeach name [array names global_config] {\n \t\tset global_config_new($name) $global_config($name)\n \t}\n+\tforeach name [get_diff_primer] {\n+\t\tset diff_option [string range $name 8 [string length $name]]\n+\t\tset global_config_new($name) [expr {[lsearch $diff_primer_global $diff_option] < 0 ? \"false\" : \"true\"}]\n+\t\tset   repo_config_new($name) [expr {[lsearch $diff_primer_repo   $diff_option] < 0 ? \"false\" : \"true\"}]\n+\t}\n \n \tset w .options_editor\n \ttoplevel $w\n@@ -150,6 +194,9 @@ proc do_options {} {\n \t\t{i-20..200 gui.copyblamethreshold {mc \"Minimum Letters To Blame Copy On\"}}\n \t\t{i-0..300 gui.blamehistoryctx {mc \"Blame History Context Radius (days)\"}}\n \t\t{i-1..99 gui.diffcontext {mc \"Number of Diff Context Lines\"}}\n+\t\t{b gui.diff--ignore-space-at-eol {mc \"Diff Ignore Trailing White Space\"            }}\n+\t\t{b gui.diff--ignore-space-change {mc \"Diff Ignore Changes In Amount Of White Space\"}}\n+\t\t{b gui.diff--ignore-all-space    {mc \"Diff Ignore All White Space\"                 }}\n \t\t{i-0..99 gui.commitmsgwidth {mc \"Commit Message Text Width\"}}\n \t\t{t gui.newbranchtemplate {mc \"New Branch Name Template\"}}\n \t\t{c gui.encoding {mc \"Default File Contents Encoding\"}}\n-- \n1.6.1\n"},{"id":"101848","messageId":"alpine.DEB.1.00.0901251916010.14855@racer","threadId":"17354","inReplyTo":"1232904657-31831-2-git-send-email-keith@cs.ucla.edu","subject":"Re: [PATCH v1 1/3] Introduce config variable \"diff.primer\"","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-01-25T18:17:48Z","receivedAt":"2009-01-25T18:17:48Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 25 Jan 2009, Keith Cascio wrote:\n\n> Introduce config variable \"diff.primer\".\n> Allows user to specify arbitrary options\n> to pass to diff on every invocation,\n> including internal invocations from other\n> programs, e.g. git-gui.\n\nThat would break existing scripts using \"git diff\" rather badly.  We \nalready did not allow something like \"git config alias.diff ...\" from \nchanging the behavior of \"git diff\", so I cannot find a reason why we \nshould let diff.primer (a misnomer BTW) override the behavior.\n\nCiao,\nDscho\n"},{"id":"101849","messageId":"alpine.DEB.1.00.0901251918230.14855@racer","threadId":"17354","inReplyTo":"1232904657-31831-4-git-send-email-keith@cs.ucla.edu","subject":"Re: [PATCH v1 3/3] git-gui hooks for new config variable \"diff.primer\"","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-01-25T18:22:09Z","receivedAt":"2009-01-25T18:22:09Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 25 Jan 2009, Keith Cascio wrote:\n\n> git-gui hooks for new config variable \"diff.primer\".\n> Add three checkboxes to both sides of\n> options panel (local/global).\n> Add a sub-menu named \"White Space\" to\n> diff-panel right-click context menu, with\n> three checkboxes.\n\nRather than storing the information about how to call \"git diff\" as \ndiff.primer, why don't you store that information in a config variable \ngui.whiteSpaceMode and teach \"git gui\" to call \"git diff\" accordingly?\n\nThat would have the further advantage of not breaking other people's \nsetups...\n\n>  git-gui/git-gui.sh     |   51 ++++++++++++++++++++++++++++++++++++++++++\n>  git-gui/lib/option.tcl |   57 +++++++++++++++++++++++++++++++++++++++++++----\n\nPlease submit git-gui patches without the git-gui prefix, as it makes it \nharder on the maintainer of git-gui, Shawn (who you did not Cc: BTW).\n\nCiao,\nDscho\n"},{"id":"101851","messageId":"alpine.GSO.2.00.0901251033160.12651@kiwi.cs.ucla.edu","threadId":"17354","inReplyTo":"alpine.DEB.1.00.0901251916010.14855@racer","subject":"Re: [PATCH v1 1/3] Introduce config variable \"diff.primer\"","fromName":"Keith Cascio","fromEmail":"keith@cs.ucla.edu","sentAt":"2009-01-25T18:44:25Z","receivedAt":"2009-01-25T18:44:25Z","isPatch":true,"sender":{"key":"keith@cs.ucla.edu","avatar":"https://gravatar.com/avatar/c5ec3a8f1cd1f449fdf8bdb7125fdbfd10b729507f32cbf0aa4ad07b4f7127ae?d=mp&s=160"},"body":"On Sun, 25 Jan 2009, Johannes Schindelin wrote:\n\n> That would break existing scripts using \"git diff\" rather badly.  We already \n> did not allow something like \"git config alias.diff ...\" from changing the \n> behavior of \"git diff\", so I cannot find a reason why we should let \n> diff.primer (a misnomer BTW) override the behavior.\n\nI took special care to protect all core scripts from the effects.  Quote from \npatch 1/3:\n> Protect git-format-patch, git-apply,\n> git-am, git-rebase, git-gui and gitk\n> from inapplicable options.\n\nI fact, by introducing the cpp macro DIFF_MACHINE_FRIENDLY() and the \ncommand-line options \"--machine-friendly\" and \"--no-primer\", I made such \nprotection declarative.  Don't you find it preferable that existing programs and \nscripts would explicitly declare their desire for machine-friendly output?\n\nThe name \"primer\" is open to discussion, of course.  But I like it.\nFrom Merriam-Webster:\nprimer n 1: a device for priming 2: material used in priming a surface\nprime vb 1: fill, load 2: to prepare for firing 3: to apply the first color, coating or preparation to <~ a wall>\n\nThanks for your input.  More input welcome.\n                                    -- Keith"},{"id":"101859","messageId":"alpine.GSO.2.00.0901251045090.12651@kiwi.cs.ucla.edu","threadId":"17354","inReplyTo":"alpine.DEB.1.00.0901251918230.14855@racer","subject":"Re: [PATCH v1 3/3] git-gui hooks for new config variable \"diff.primer\"","fromName":"Keith Cascio","fromEmail":"keith@cs.ucla.edu","sentAt":"2009-01-25T18:58:51Z","receivedAt":"2009-01-25T18:58:51Z","isPatch":true,"sender":{"key":"keith@cs.ucla.edu","avatar":"https://gravatar.com/avatar/c5ec3a8f1cd1f449fdf8bdb7125fdbfd10b729507f32cbf0aa4ad07b4f7127ae?d=mp&s=160"},"body":"On Sun, 25 Jan 2009, Johannes Schindelin wrote:\n\n> Rather than storing the information about how to call \"git diff\" as \n> diff.primer, why don't you store that information in a config variable \n> gui.whiteSpaceMode and teach \"git gui\" to call \"git diff\" accordingly?\n> \n> That would have the further advantage of not breaking other people's \n> setups...\n\nThis wasn't just for the GUI.  Using Git for a project I imported from CVS, I \nwanted --ignore-space-at-eol to be in effect at all times on the command line.  \nOf course, the \"alias.dff\" approach suggested yesterday by Teemu would work for \nthat.  But I got the feeling this is a more general need.  I'll name my primary \ninspiration: ExamDiff (a Windows program).  ExamDiff lets you specify a wide \nrange of options that remain in effect over all invocations.  Seems like \nsomething a lot of users would find natural.  Please comment.\n\n> Please submit git-gui patches without the git-gui prefix, as it makes it \n> harder on the maintainer of git-gui, Shawn (who you did not Cc: BTW).\n\nSorry Shawn, I should have Cc'd you.  Please let me know if I can improve the \ngit-gui code in any way.\n\n                                        -- Keith\n"},{"id":"101861","messageId":"alpine.DEB.1.00.0901252016590.14855@racer","threadId":"17354","inReplyTo":"alpine.GSO.2.00.0901251033160.12651@kiwi.cs.ucla.edu","subject":"Re: [PATCH v1 1/3] Introduce config variable \"diff.primer\"","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-01-25T19:30:05Z","receivedAt":"2009-01-25T19:30:05Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 25 Jan 2009, Keith Cascio wrote:\n\n> On Sun, 25 Jan 2009, Johannes Schindelin wrote:\n> \n> > That would break existing scripts using \"git diff\" rather badly.  We \n> > already did not allow something like \"git config alias.diff ...\" from \n> > changing the behavior of \"git diff\", so I cannot find a reason why we \n> > should let diff.primer (a misnomer BTW) override the behavior.\n> \n> I took special care to protect all core scripts from the effects.\n\nWhat about my scripts I have here locally?  Do you want to change them, \ntoo?\n\n> I fact, by introducing the cpp macro DIFF_MACHINE_FRIENDLY() and the \n> command-line options \"--machine-friendly\" and \"--no-primer\", I made such \n> protection declarative.  Don't you find it preferable that existing \n> programs and scripts would explicitly declare their desire for \n> machine-friendly output?\n\nNo.  We made a promise long time ago that plumbing (and \"git diff\" is \npretty much plumbing, except for the configurable colorness) would not \nchange behind scripts' backs.\n\nAnd since Shawn uses plumbing for that very reason, your diff.primer patch \nwould not be allowed to make a difference.  Ever.\n\nNow, if you would have changed only the UI diff things (i.e. git diff, but \nnot git diff-files), I could have accepted the diff.primer patch for \ndifferent applications than \"git gui\", but from cursory reading of your \npatch it does not appear so.\n\nSpeaking of appearance (or for that matter, explaining why it was only a \ncursory reading): did it not occur to you that your coding style is \nutterly different from the surrounding code?\n\nJust to number a few things that would definitely prohibit this patch from \nbeing applied:\n\n- space instead of tabs,\n\n- horrible lengths of spaces within the line,\n\n- no space after if, but after the parenthesis.\n\nNow, this could be good explanation why you need the patch (to ignore \nwhite-space), but that is not a reason of letting us suffer, too.\n\nBesides, it seems you did a lot of \"fixes\" on the side that I do not like \nat all.  Simple example: if the original code cleared the \nDIRSTAT_CUMULATIVE flag, it is not acceptable for you to introduce an \nunnecessary if(), testing if the CUMULATIVE flag was set to begin with.\n\nCiao,\nDscho\n"},{"id":"101865","messageId":"alpine.GSO.2.00.0901251149030.12651@kiwi.cs.ucla.edu","threadId":"17354","inReplyTo":"alpine.DEB.1.00.0901252016590.14855@racer","subject":"Re: [PATCH v1 1/3] Introduce config variable \"diff.primer\"","fromName":"Keith Cascio","fromEmail":"keith@cs.ucla.edu","sentAt":"2009-01-25T20:14:35Z","receivedAt":"2009-01-25T20:14:35Z","isPatch":true,"sender":{"key":"keith@cs.ucla.edu","avatar":"https://gravatar.com/avatar/c5ec3a8f1cd1f449fdf8bdb7125fdbfd10b729507f32cbf0aa4ad07b4f7127ae?d=mp&s=160"},"body":"On Sun, 25 Jan 2009, Johannes Schindelin wrote:\n\n> What about my scripts I have here locally?  Do you want to change them, \n> too?\n\nSomeone who already wrote local scripts probably won't use the diff.primer \nfeature, or if he does, he'll be thoughtful about the consequences and modify \nhis scripts.  diff.primer has no effect unless you use it.  It is totally \npersonal.\n\n> > I fact, by introducing the cpp macro DIFF_MACHINE_FRIENDLY() and the \n> > command-line options \"--machine-friendly\" and \"--no-primer\", I made such \n> > protection declarative.  Don't you find it preferable that existing programs \n> > and scripts would explicitly declare their desire for machine-friendly \n> > output?\n\n> No.  We made a promise long time ago that plumbing (and \"git diff\" is \n> pretty much plumbing, except for the configurable colorness) would not \n> change behind scripts' backs.\n> \n> And since Shawn uses plumbing for that very reason, your diff.primer patch \n> would not be allowed to make a difference.  Ever.\n\nI fixed up every single place where gitk and git-gui internally call diff.  \nBefore I ever got there, Shawn already protected himself by passing --no-color \nto \"git diff\" (git-gui/lib/diff.tcl line 279).  My changes are in a good spirit \nbecause they enhance the declarativeness and explicitness of the code.  I added \nmeaningful information.\n\n> Now, if you would have changed only the UI diff things (i.e. git diff, but \n> not git diff-files), I could have accepted the diff.primer patch for \n> different applications than \"git gui\", but from cursory reading of your \n> patch it does not appear so.\n> \n> Speaking of appearance (or for that matter, explaining why it was only a \n> cursory reading): did it not occur to you that your coding style is \n> utterly different from the surrounding code?\n> \n> Just to number a few things that would definitely prohibit this patch from \n> being applied:\n> \n> - space instead of tabs,\n> - horrible lengths of spaces within the line,\n> - no space after if, but after the parenthesis.\n\nYou are right, I should mimic the existing conventions.  If we reach consensus \nabout the functionality of the patch, I would be happy to redo all the white \nspace to match what's already there.  In case you are curious, I put whitespace \nin lines in the name of readability, to make tokens line up with each other.  \nAgain, it is right to mimic local convention and I have no problem making that \nrevision.\n\n> Besides, it seems you did a lot of \"fixes\" on the side that I do not like at \n> all.  Simple example: if the original code cleared the DIRSTAT_CUMULATIVE \n> flag, it is not acceptable for you to introduce an unnecessary if(), testing \n> if the CUMULATIVE flag was set to begin with.\n\nI appreciate that you noticed this little detail.  You are extremely thorough \nand I take your criticism very seriously.  The reason for that little if() is \nthat, with the enhanced declarativeness of the code in diff.h, calling the cpp \nmacro DIFF_OPT_SET() is now more meaningful that just flipping a bit.  It is now \nthe equivalent of saying \"I intend to set this bit, and I want my intention \nrecorded and honored later.\"  The purpose of the piece of code where I \nintroduced the if() is only to setup the right defaults, not to declare user \nintention.  Therefore, IOW, if the default is already in place, do nothing.\n\nc is only one of many languages I write.  Designers of newer languages emphasize \nmeangingfulness, explicitness, declarativeness of code, e.g. the DRY principle, \netc.  c is very flexible, so it does not prevent us from following these \nbeneficial practices in c.  After all, the first c++ compilers simply emitted c \nand then called a c compiler, right?\n\nWith thanks,\nKeith\n"},{"id":"101866","messageId":"7v1vurf7lq.fsf@gitster.siamese.dyndns.org","threadId":"17354","inReplyTo":"1232904657-31831-2-git-send-email-keith@cs.ucla.edu","subject":"Re: [PATCH v1 1/3] Introduce config variable \"diff.primer\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-25T20:34:09Z","receivedAt":"2009-01-25T20:34:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Keith Cascio <keith@cs.ucla.edu> writes:\n\n> Introduce config variable \"diff.primer\".\n> Allows user to specify arbitrary options\n> to pass to diff on every invocation,\n> including internal invocations from other\n> programs, e.g. git-gui.\n> Introduce diff command-line options:\n> --no-primer, --machine-friendly\n> Protect git-format-patch, git-apply,\n> git-am, git-rebase, git-gui and gitk\n> from inapplicable options.\n>\n> Signed-off-by: Keith Cascio <keith@cs.ucla.edu>\n\nYour Subject is good; in a shortlog output that will be taken 3 months\ndown the road, it will still tell us what this patch was about, among 100\nother patches that are about different topics.\n\nThe proposed commit log message describes what the patch does, but it does\nnot explain what problem it solves, nor why the approach the patch takes\nto solve that problem is good.  Lines that are too short and too dense\nwithout paragraph breaks do not help readability either.\n\n> ---\n>  Documentation/config.txt       |   14 +++++++\n>  Documentation/diff-options.txt |   13 ++++++\n>  Makefile                       |    2 +\n>  builtin-log.c                  |    1 +\n>  diff.c                         |   83 +++++++++++++++++++++++++++++++++++-----\n>  diff.h                         |   15 ++++++-\n>  git-gui/lib/diff.tcl           |    8 +++-\n>  gitk-git/gitk                  |   16 ++++----\n>  8 files changed, 129 insertions(+), 23 deletions(-)\n\nYou can work around the backward incompatibility you are introducing for\nknown users that you broke with your patch, by including updates to them,\nand that is what your patches to git-gui and gitk are, but that is a sure\nsign that the approach is flawed.\n\nThe point of lowlevel plumbing (e.g. diff-{files,index,tree}) is to give\npeople's scripts an interface that they can rely on.  It is not about\ngiving a magic interface that all the users are somehow magically upgraded\nwithout change, when the underlying git is upgraded.\n\nIf a script X does not use \"ignore whitespace\" without an explicit request\nfrom the end user when it runs diff-tree internally, installing a new\nversion of diff-tree should *NOT* magically make script X to run it with\n\"ignore whitespace\", because you do not know what the script X uses\ndiff-tree output for and how, even when the end user sets diff.primer to\nget \"ignore whitespace\" applied to his command line invocation of \"git\ndiff\" Porcelain.  Imagine a case where the operation of that script X\nrelies on seeing at least two context lines around the hunk in order to\ncorrectly parse textual diff output from \"diff-index -p\", and the user\nsets \"-U1\" in diff.primer --- you would break the script and it is not\nfair to blame the script for not explicitly passing -U3 and relying on the\ndefault.\n\nScriptability by definition means you do not know how scripts written by\npeople around plumbing use the output; I do not think you can sensibly say\n\"this should not be turned on in a machine friendly output, but this is\nsafe to use\".\n\nI would not be opposed to an enhancement to the plumbing that the scripts\ncan use to say \"I am willing to take any option (or perhaps \"these\noptions\") given to me via diff.primer\".  Some scripts may want to be just\na pass-thru of whatever the underlying git-diff-* command outputs, and it\nmay be a handy way to magically upgrade them to allow their invocation of\nlowlevel plumbing to be affected by what the end-user configured.  But\nthat magic upgrade has to be an opt/in process.\n\nThere are funny indentation to align the same variable names on two\nadjacent lines and such; please don't.\n"},{"id":"101873","messageId":"7vr62rcee5.fsf@gitster.siamese.dyndns.org","threadId":"17354","inReplyTo":"1232904657-31831-1-git-send-email-keith@cs.ucla.edu","subject":"Re: [PATCH v1 0/3] Introduce config variable \"diff.primer\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-25T20:35:46Z","receivedAt":"2009-01-25T20:35:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Keith Cascio <keith@cs.ucla.edu> writes:\n\n> Future work: Extend the gitattributes mechanism so it supports\n> all [diff] config variables, including e.g. diff.mnemonicprefix\n> and diff.primer.\n\nI am puzzled.\n\nThe gitattributes mechanism is about per-path settings, but I do not think\na mnemonicprefix that is per-path makes much sense.\n"},{"id":"101875","messageId":"alpine.GSO.2.00.0901251239000.12651@kiwi.cs.ucla.edu","threadId":"17354","inReplyTo":"7vr62rcee5.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH v1 0/3] Introduce config variable \"diff.primer\"","fromName":"Keith Cascio","fromEmail":"keith@cs.ucla.edu","sentAt":"2009-01-25T20:41:21Z","receivedAt":"2009-01-25T20:41:21Z","isPatch":true,"sender":{"key":"keith@cs.ucla.edu","avatar":"https://gravatar.com/avatar/c5ec3a8f1cd1f449fdf8bdb7125fdbfd10b729507f32cbf0aa4ad07b4f7127ae?d=mp&s=160"},"body":"On Sun, 25 Jan 2009, Junio C Hamano wrote:\n\n> I am puzzled.\n> \n> The gitattributes mechanism is about per-path settings, but I do not think a \n> mnemonicprefix that is per-path makes much sense.\n\nThat was just an example (perhaps poorly chosen).  What I meant to suggest is \nmaking gitattributes consistent with gitconfig WRT at least the [diff] section.  \nBut maybe that's not appropriate.  Thanks for the insight.\n"},{"id":"101883","messageId":"20090125220756.GA18855@coredump.intra.peff.net","threadId":"17354","inReplyTo":"alpine.GSO.2.00.0901251239000.12651@kiwi.cs.ucla.edu","subject":"Re: [PATCH v1 0/3] Introduce config variable \"diff.primer\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-01-25T22:07:56Z","receivedAt":"2009-01-25T22:07:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jan 25, 2009 at 12:41:21PM -0800, Keith Cascio wrote:\n\n> > I am puzzled.\n> > \n> > The gitattributes mechanism is about per-path settings, but I do not\n> > think a mnemonicprefix that is per-path makes much sense.\n> \n> That was just an example (perhaps poorly chosen).  What I meant to\n> suggest is making gitattributes consistent with gitconfig WRT at least\n> the [diff] section.  But maybe that's not appropriate.  Thanks for the\n> insight.\n\nI don't think you want it entirely consistent. What would\ndiff.renamelimit mean in the context of a gitattribute? But I do think\nit makes sense for some (like specific diff options such as whitespace\nhandling).\n\nAlso, if you're going to have options that apply to gitattributes diff\ndrivers _and_ as a general fallback, I think we need to define when the\nfallback kicks in. That is, let's say I have a gitattributes file like\nthis:\n\n   *.c diff=c\n\nand my config says:\n\n  [diff]\n    opt1 = val1_default\n    opt2 = val2_default\n\n  [diff \"c\"]\n    opt1 = val1_c\n\nNow obviously if I want to use opt1 for my C files, it should be val1_c.\nBut if I want to use opt2, what should it use? There are two reasonable\nchoices, I think:\n\n  1. You use val2_default. The rationale is that the \"c\" diff driver did\n     not define an opt2, so you fall back to the global default.\n\n  2. It is unset. The rationale is that you are using the \"c\" diff\n     driver, and it has left the value unset. The default then means \"if\n     you have no diff driver setup\".\n\nI suspect \"1\" is what people would want most of the time, but \"2\" is\nactually more flexible (since there is otherwise no way to say \"I\nexplicitly left diff.c.opt2 unset\").\n\nIf (2) is desired, I think it makes more sense to put such \"default\"\noptions into their own diff driver section. Like:\n\n  [diff \"default\"]\n    opt2 = whatever\n\nAnd then it is more clear that once you have selected the \"c\" diff\ndriver, the values in the other \"default\" are not relevant.\n\nI don't think this is a huge issue overall, but it occurs to me that we\nhave just added diff.wordRegex and diff.*.wordRegex. So it makes sense\nto think for a minute which behavior we want before it ships and we are\nstuck with backwards compatibility forever.\n\n-Peff\n"},{"id":"101884","messageId":"20090125221141.GA17490@coredump.intra.peff.net","threadId":"17354","inReplyTo":"alpine.GSO.2.00.0901251033160.12651@kiwi.cs.ucla.edu","subject":"Re: [PATCH v1 1/3] Introduce config variable \"diff.primer\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-01-25T22:11:42Z","receivedAt":"2009-01-25T22:11:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jan 25, 2009 at 10:44:25AM -0800, Keith Cascio wrote:\n\n> The name \"primer\" is open to discussion, of course.  But I like it.\n> From Merriam-Webster:\n> primer n 1: a device for priming 2: material used in priming a surface\n> prime vb 1: fill, load 2: to prepare for firing 3: to apply the first color, coating or preparation to <~ a wall>\n\nFWIW, I found it very confusing. I would have expected \"diff.options\" or\n\"diff.defaults\". There is also some precedent in the form of\nGIT_DIFF_OPTS, but I believe it _only_ handles --unified and -u, so it\nis not necessarily a useful model.\n\n-Peff\n"},{"id":"101893","messageId":"alpine.GSO.2.00.0901251446260.12651@kiwi.cs.ucla.edu","threadId":"17354","inReplyTo":"20090125221141.GA17490@coredump.intra.peff.net","subject":"Re: [PATCH v1 1/3] Introduce config variable \"diff.primer\"","fromName":"Keith Cascio","fromEmail":"keith@cs.ucla.edu","sentAt":"2009-01-25T22:58:34Z","receivedAt":"2009-01-25T22:58:34Z","isPatch":true,"sender":{"key":"keith@cs.ucla.edu","avatar":"https://gravatar.com/avatar/c5ec3a8f1cd1f449fdf8bdb7125fdbfd10b729507f32cbf0aa4ad07b4f7127ae?d=mp&s=160"},"body":"On Sun, 25 Jan 2009, Jeff King wrote:\n\n> FWIW, I found it very confusing. I would have expected \"diff.options\" or \n> \"diff.defaults\". There is also some precedent in the form of GIT_DIFF_OPTS, \n> but I believe it _only_ handles --unified and -u, so it is not necessarily a \n> useful model.\n\nOK, point taken.  I wasn't trying to be idiosyncratic at all.  Just trying to be \nexplicit and avoid all confusion.  Since all diff options already have default \nvalues, primer looks to me like the layer one step above defaults, hence the \npainting analogy.  Mercurial calls it \"defaults\", but that doesn't mean we \nshould necessarily follow in their footsteps (see \nhttp://article.gmane.org/gmane.comp.version-control.git/107103).\n\nI think being as clear as possible about what primer is, that is it NOT \ndefaults, helps to feel more comfortable with its consequences, i.e. in my \nopinion, that it will not break things.\n\n                                   -- Keith\n"},{"id":"101901","messageId":"20090125232520.GB19099@coredump.intra.peff.net","threadId":"17354","inReplyTo":"alpine.GSO.2.00.0901251446260.12651@kiwi.cs.ucla.edu","subject":"Re: [PATCH v1 1/3] Introduce config variable \"diff.primer\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-01-25T23:25:20Z","receivedAt":"2009-01-25T23:25:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jan 25, 2009 at 02:58:34PM -0800, Keith Cascio wrote:\n\n> OK, point taken.  I wasn't trying to be idiosyncratic at all.  Just\n> trying to be explicit and avoid all confusion.  Since all diff options\n> already have default values, primer looks to me like the layer one\n> step above defaults, hence the painting analogy.  Mercurial calls it\n> \"defaults\", but that doesn't mean we should necessarily follow in\n> their footsteps (see\n> http://article.gmane.org/gmane.comp.version-control.git/107103).\n> \n> I think being as clear as possible about what primer is, that is it\n> NOT defaults, helps to feel more comfortable with its consequences,\n> i.e. in my opinion, that it will not break things.\n\nI'm not sure I agree that they are not new defaults, but any such\nargument is going to get into the exact definition of \"default\" which is\nnot really useful to the task at hand.\n\nI think \"options\" is a better word (as in, pretend like you already\nspecified these \"options\" on the command line), but I am not going to\ninsist on that. I mainly just wanted to point out that I found \"primer\"\nconfusing. Enough so that, even though I knew you were interested in\nthis topic from your previous mails, I saw the word \"primer\" and said to\nmyself: \"what in the world is this patch about?\"\n\n-Peff\n"},{"id":"101934","messageId":"7v1vuqdcjp.fsf@gitster.siamese.dyndns.org","threadId":"17354","inReplyTo":"7v1vurf7lq.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH v1 1/3] Introduce config variable \"diff.primer\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-26T02:30:18Z","receivedAt":"2009-01-26T02:30:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Scriptability by definition means you do not know how scripts written by\n> people around plumbing use the output; I do not think you can sensibly say\n> \"this should not be turned on in a machine friendly output, but this is\n> safe to use\".\n>\n> I would not be opposed to an enhancement to the plumbing that the scripts\n> can use to say \"I am willing to take any option (or perhaps \"these\n> options\") given to me via diff.primer\".  Some scripts may want to be just\n> a pass-thru of whatever the underlying git-diff-* command outputs, and it\n> may be a handy way to magically upgrade them to allow their invocation of\n> lowlevel plumbing to be affected by what the end-user configured.  But\n> that magic upgrade has to be an opt/in process.\n\nI suspect it is pretty much orthogonal to the \"use user's default without\nbeing told from the command line\", but it might be a worthy goal to\nintroduce a mechanism for the scripts to accept \"safe\" default options\nfrom the end user while rejecting undesirable ones that would interfere\nwith the way it uses plumbing.\n\nFor example, gitk drives \"git rev-list\" and many options you give from the\ncommand line (e.g. \"gitk --all --simplify-merges -- drivers/\") are passed\nto the underlying plumbing.\n\nThis is a double edged sword.  When we add new features to git-rev-list,\n(e.g. --simplify-merges or --simplify-by-decoration are fairly recent\ninventions that did not exist when gitk was written originally), some of\nthem can be safely passed and automagically translates to a new feature in\ngitk.  However, use of some options (e.g. --reverse) breaks the assumption\nthe tool makes on the output from the underlying plumbing and should not\nbe accepted from the end-user.\n\nIt would be a good addition to our toolset if scripts like gitk can\ndeclare which options and features are safe to accept from the end user to\npass down to the plumbing tools.  \"git rev-parse\", which lets the script\nsift between options that are meant to affect ancestry traversal and the\nones that are for other (primarily diff family) commands, does not do\nanything fancy like that, but it would be a logical place to do this sort\nof thing.\n\nAnd it is not limited to \"scripts\" use.  A recent topic on rejecting\ncolouring options from being given to format-patch would also be helped\nwith such a mechanism if it is available to builtins.\n\nJust an idle thought.\n"},{"id":"101936","messageId":"alpine.GSO.2.00.0901251836150.12651@kiwi.cs.ucla.edu","threadId":"17354","inReplyTo":"7v1vuqdcjp.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH v1 1/3] Introduce config variable \"diff.primer\"","fromName":"Keith Cascio","fromEmail":"keith@cs.ucla.edu","sentAt":"2009-01-26T02:37:26Z","receivedAt":"2009-01-26T02:37:26Z","isPatch":true,"sender":{"key":"keith@cs.ucla.edu","avatar":"https://gravatar.com/avatar/c5ec3a8f1cd1f449fdf8bdb7125fdbfd10b729507f32cbf0aa4ad07b4f7127ae?d=mp&s=160"},"body":"On Sun, 25 Jan 2009, Junio C Hamano wrote:\n\n> I suspect it is pretty much orthogonal to the \"use user's default without \n> being told from the command line\", but it might be a worthy goal to introduce \n> a mechanism for the scripts to accept \"safe\" default options from the end user \n> while rejecting undesirable ones that would interfere with the way it uses \n> plumbing.\n> \n> For example, gitk drives \"git rev-list\" and many options you give from the \n> command line (e.g. \"gitk --all --simplify-merges -- drivers/\") are passed to \n> the underlying plumbing.\n> \n> This is a double edged sword.  When we add new features to git-rev-list, (e.g. \n> --simplify-merges or --simplify-by-decoration are fairly recent inventions \n> that did not exist when gitk was written originally), some of them can be \n> safely passed and automagically translates to a new feature in gitk.  \n> However, use of some options (e.g. --reverse) breaks the assumption the tool \n> makes on the output from the underlying plumbing and should not be accepted \n> from the end-user.\n> \n> It would be a good addition to our toolset if scripts like gitk can declare \n> which options and features are safe to accept from the end user to pass down \n> to the plumbing tools.  \"git rev-parse\", which lets the script sift between \n> options that are meant to affect ancestry traversal and the ones that are for \n> other (primarily diff family) commands, does not do anything fancy like that, \n> but it would be a logical place to do this sort of thing.\n> \n> And it is not limited to \"scripts\" use.  A recent topic on rejecting colouring \n> options from being given to format-patch would also be helped with such a \n> mechanism if it is available to builtins.\n> \n> Just an idle thought.\n\n\nYes yes yes yes!!!!!  I've been working on a response to your previous message, \nin which I address exactly this possibility.  Coming soon.\n"},{"id":"101935","messageId":"alpine.GSO.2.00.0901251345240.12651@kiwi.cs.ucla.edu","threadId":"17354","inReplyTo":"7v1vurf7lq.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH v1 1/3] Introduce config variable \"diff.primer\"","fromName":"Keith Cascio","fromEmail":"keith@cs.ucla.edu","sentAt":"2009-01-26T02:40:02Z","receivedAt":"2009-01-26T02:40:02Z","isPatch":true,"sender":{"key":"keith@cs.ucla.edu","avatar":"https://gravatar.com/avatar/c5ec3a8f1cd1f449fdf8bdb7125fdbfd10b729507f32cbf0aa4ad07b4f7127ae?d=mp&s=160"},"body":"On Sun, 25 Jan 2009, Junio C Hamano wrote:\n\n> Your Subject is good; in a shortlog output that will be taken 3 months\n> down the road, it will still tell us what this patch was about, among 100\n> other patches that are about different topics.\n\nThanks, I value the encouragement.\n\n> The proposed commit log message describes what the patch does, but it does\n> not explain what problem it solves, nor why the approach the patch takes\n> to solve that problem is good.  Lines that are too short and too dense\n> without paragraph breaks do not help readability either.\n\nNoted, future versions will be better.\n\n> You can work around the backward incompatibility you are introducing for known \n> users that you broke with your patch, by including updates to them, and that \n> is what your patches to git-gui and gitk are, but that is a sure sign that the \n> approach is flawed.\n> \n> The point of lowlevel plumbing (e.g. diff-{files,index,tree}) is to give \n> people's scripts an interface that they can rely on.  It is not about giving a \n> magic interface that all the users are somehow magically upgraded without \n> change, when the underlying git is upgraded.\n\n\nI believe, as I know you do as well, concept is the right way to approach code.  \nI believe in the power of concept, and I agree that porcelain/plumbing is a good \nand powerful concept, and should not be violated.\n\n> If a script X does not use \"ignore whitespace\" without an explicit request\n> from the end user when it runs diff-tree internally, installing a new\n> version of diff-tree should *NOT* magically make script X to run it with\n> \"ignore whitespace\", because you do not know what the script X uses\n> diff-tree output for and how, even when the end user sets diff.primer to\n> get \"ignore whitespace\" applied to his command line invocation of \"git\n> diff\" Porcelain.  Imagine a case where the operation of that script X\n> relies on seeing at least two context lines around the hunk in order to\n> correctly parse textual diff output from \"diff-index -p\", and the user\n> sets \"-U1\" in diff.primer --- you would break the script and it is not\n> fair to blame the script for not explicitly passing -U3 and relying on the\n> default.\n>\n> Scriptability by definition means you do not know how scripts written by\n> people around plumbing use the output; I do not think you can sensibly say\n> \"this should not be turned on in a machine friendly output, but this is\n> safe to use\".\n> \n> I would not be opposed to an enhancement to the plumbing that the scripts\n> can use to say \"I am willing to take any option (or perhaps \"these\n> options\") given to me via diff.primer\".  Some scripts may want to be just\n> a pass-thru of whatever the underlying git-diff-* command outputs, and it\n> may be a handy way to magically upgrade them to allow their invocation of\n> lowlevel plumbing to be affected by what the end-user configured.  But\n> that magic upgrade has to be an opt/in process.\n\nI agree opt-in is always better with new grammar/semantics.  However, the \nconstraint I was trying to live inside is: if I call \"git diff\" on the command \nline with no options at all, then primer active.  Yet perhaps that's not \npossible, and the only way to do primer is to require opt-in spelled \"--primer\".  \nThen I can tell bash to alias 'gitdiff' as 'git diff --primer' and use that on \nthe command line.  And I could patch git-gui and gitk to call 'git diff --primer \n--no-color'.  Note that would still require the same magnitude of change to the \ngit-gui and gitk Tcl code as occurs with my v1 patch.\n\nIt would be really cool if there was a way\nto make this work without \"--primer\" !!!\n^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n\nWorth considering: I believe Git users who've written their own scripts are not \nthe type to use diff.primer blindly, if at all.  If they did use it, they'd be \nvery thoughtful about its consequences for their existing scripts.  Based on \nthat belief, and the fact that I already protected every script in Git core, and \nthat I could work with the authors of contrib to do the same there, I have a \nvery high degree of confidence that no existing script would be broken the day \nyou release diff.primer, if ever :(\n\nBecause an enhancement *could* break existing scripts is not sufficient grounds \nto declare it worthless.  My gut tells me there is a sane way to do this, but I \nneed more familiarity with the \"literature\" to discover it.\n\nI see evidence there is already anxiety surrounding the necessity of \n\"machine-friendliness\", Cf. \nhttp://article.gmane.org/gmane.comp.version-control.git/107006\n\nI think my DIFF_MACHINE_FRIENDLY() cpp macro with --machine-friendly command \nline option does a nice job of making that explicit, so we could all just relax!  \nWhen a monster is lurking under the bridge threatening the kingdom, put it to \ndeath once and for all!  Rather than relying on an elaborate system of \nconventions prescribing how to tip-toe acrosss the bridge just the right way \nwhile pretending the monster isn't there.\n\n> There are funny indentation to align the same variable names on two\n> adjacent lines and such; please don't.\n\nYou're right.  I'm sorry.  I'll fix all style to adhere to local convention.\n\n                                    -- Keith\n"},{"id":"101937","messageId":"20090126031206.GB14277@sigill.intra.peff.net","threadId":"17354","inReplyTo":"alpine.GSO.2.00.0901251345240.12651@kiwi.cs.ucla.edu","subject":"Re: [PATCH v1 1/3] Introduce config variable \"diff.primer\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-01-26T03:12:07Z","receivedAt":"2009-01-26T03:12:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jan 25, 2009 at 06:40:02PM -0800, Keith Cascio wrote:\n\n> I agree opt-in is always better with new grammar/semantics.  However,\n> the constraint I was trying to live inside is: if I call \"git diff\" on\n> the command line with no options at all, then primer active.  Yet\n> perhaps that's not possible, and the only way to do primer is to\n> require opt-in spelled \"--primer\".  Then I can tell bash to alias\n> 'gitdiff' as 'git diff --primer' and use that on the command line.\n\nWhat's the point of aliasing something that isn't \"git diff\" to \"git\ndiff --primer\"? At that point, couldn't you just do away with --primer\nentirely and alias \"gitdiff\" to \"git diff --whatever --your --primer\n--options --are\"?\n\nAnyway, I think that isn't necessary. We _do_ have a mechanism to handle\nthis already: some commands are plumbing, and must have stable\ninterfaces, and some commands are porcelain, and can do your magic\nautomatically. For example, gitk doesn't actually call \"git diff\"; it\ncalls \"git diff-tree\", \"git diff-index\", etc.\n\nSo if you just want this from the command line, then I think it is safe\nto have \"git diff\" always respect \"diff.primer\", and scripts shouldn't\nbe impacted.\n\nBut this can break down in two ways:\n\n  1. Sometimes we blur the line of plumbing and porcelain, where\n     functionality is available only through plumbing. For example,\n     gitweb until recently called \"git diff\" because there is no other\n     way to diff two arbitrary blobs. But the solution there is, I\n     think, to make that functionality available through plumbing. Not\n     to disallow enhancements to porcelain.\n\n  2. When you want a script to take advantage of porcelain-like options,\n     the situation is much more difficult (and this is what Junio was\n     talking about in his last mail).\n\n     What I think is sane is:\n\n       a. You grow new feature X.\n       b. Porcelain takes advantage of any config that asks us to use X.\n       c. Plumbing does _not_ respect such config, but will respect\n          command line options.\n       d. Scripts control which command line options they use; when the\n          script writer decides feature X will not interfere (either\n          because it is harmless to the script's use, or because the\n          script is enhanced to handle the new behavior), then it can\n          pass an \"--allow-X\" command line option.\n\n     And of course that has two disadvantages (and I'm running out of\n     numbering schemes):\n\n       I. You have to wait for the script to be updated before you can\n          start using X, even if _you_ know that it's harmless.\n\n      II. Point (d) is not always true. Junio mentioned the fact that\n          gitk passes command line parameters blindly to rev-list, which\n          is potentially unsafe. Up until now, our attitude has been \"if\n          it hurts, don't do it\". In other words, if you call \"gitk\n          --reverse\" and it looks ugly, then it is your fault. :)\n\n-Peff\n"},{"id":"101938","messageId":"20090126031820.GC14277@sigill.intra.peff.net","threadId":"17354","inReplyTo":"7v1vuqdcjp.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH v1 1/3] Introduce config variable \"diff.primer\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-01-26T03:18:20Z","receivedAt":"2009-01-26T03:18:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jan 25, 2009 at 06:30:18PM -0800, Junio C Hamano wrote:\n\n> It would be a good addition to our toolset if scripts like gitk can\n> declare which options and features are safe to accept from the end user to\n> pass down to the plumbing tools.  \"git rev-parse\", which lets the script\n> sift between options that are meant to affect ancestry traversal and the\n> ones that are for other (primarily diff family) commands, does not do\n> anything fancy like that, but it would be a logical place to do this sort\n> of thing.\n\nI'm not sure there is a good way of doing this at a less fine-grained\nlevel than \"each option\". That is, how can git-core, without knowing how\nthe script will use the output, classify options in groups according to\nhow the script will react to them?\n\nIt seems like \"--since\" is innocent enough for gitk. It just limits the\ncommits shown. So maybe it goes into the \"ancestry traversal\" list. But\nis that whole list safe? \"--reverse\" isn't, but I would have put it in\nthe same list.\n\nSo I think what you will end up with is a list in gitk of \"these\nparticular options are known good for passing through\". And that doesn't\nreally need tool support from git-core. It's up to each script how much\nit wants to protect the user.\n\nBut if you are proposing that some config options can be \"enabled\" by\nscripts selectively, then I think that does need tool support. Keith's\n\"primer\" example will be parsed by git, not by whatever script is\ncalling it. So we would need to feed it some list of \"these are the OK\noptions\".\n\n-Peff\n"},{"id":"101939","messageId":"7vd4eabuxf.fsf@gitster.siamese.dyndns.org","threadId":"17354","inReplyTo":"alpine.GSO.2.00.0901251345240.12651@kiwi.cs.ucla.edu","subject":"Re: [PATCH v1 1/3] Introduce config variable \"diff.primer\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-26T03:36:12Z","receivedAt":"2009-01-26T03:36:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Keith Cascio <keith@CS.UCLA.EDU> writes:\n\n> I agree opt-in is always better with new grammar/semantics.  However, the \n> constraint I was trying to live inside is: if I call \"git diff\" on the command \n> line with no options at all\n\n\"git diff\" is a Porcelain.\n\n> Worth considering: I believe Git users who've written their own scripts are not \n> the type to use diff.primer blindly, if at all.\n\nPerhaps this nonsense comes from your misunderstanding on the line between\nthe plumbing and the Porcelain.\n\nUsers of git, including me, would love to be able to use default options\nin our $HOME/.gitconfig file when using \"git diff\" interactively, but will\nrefuse to see our scripts that we wrote using \"git diff-files\" and \"git\ndiff-index\" broken, because the reason we explicitly used these plumbing\ncommands is to avoid getting broken with a change from the underlying\nversion of git.  That's the whole point of output stability for the\nplumbing.\n\nIf you want to be able to use -w or -b (or --color) in git-gui, you must\nfirst vet the script to see if it can sanely operate on the output from\ngit-diff-index with such options, and after it is determined that it is\nsafe, it should give its users a way to pass that to the underlying\nplumbing, or picked up such options from the configuration (perhaps using\nthe same diff.primer configuration).\n\nThis has to be a conscious opt-in process per script.\n"},{"id":"101940","messageId":"7v8woybut2.fsf@gitster.siamese.dyndns.org","threadId":"17354","inReplyTo":"20090126031820.GC14277@sigill.intra.peff.net","subject":"Re: [PATCH v1 1/3] Introduce config variable \"diff.primer\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-26T03:38:49Z","receivedAt":"2009-01-26T03:38:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> So I think what you will end up with is a list in gitk of \"these\n> particular options are known good for passing through\". And that doesn't\n> really need tool support from git-core. It's up to each script how much\n> it wants to protect the user.\n\nI tend to agree.  Also at the same time this does not have to contradict\nwith what Keith wants to do.  gitk just needs to learn to peek into\ndiff.primer, and use the safe ones while discarding others.  A tool\nsupport is already there in the form of git-config to do this, though.\n"},{"id":"101941","messageId":"7v4ozmbunl.fsf@gitster.siamese.dyndns.org","threadId":"17354","inReplyTo":"20090126031206.GB14277@sigill.intra.peff.net","subject":"Re: [PATCH v1 1/3] Introduce config variable \"diff.primer\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-26T03:42:06Z","receivedAt":"2009-01-26T03:42:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\nWe seem to think the same way these days, so I do not have very much to\nadd on top of what you already said.  I'll fix one typo, though.\n\n> But this can break down in two ways:\n>\n>   1. Sometimes we blur the line of plumbing and porcelain, where\n>      functionality is available only through plumbing. For example,\n\ns/through plumbing/through Porcelain/.\n"},{"id":"101942","messageId":"20090126034523.GA15407@sigill.intra.peff.net","threadId":"17354","inReplyTo":"7v4ozmbunl.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH v1 1/3] Introduce config variable \"diff.primer\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-01-26T03:45:23Z","receivedAt":"2009-01-26T03:45:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jan 25, 2009 at 07:42:06PM -0800, Junio C Hamano wrote:\n\n> We seem to think the same way these days, so I do not have very much to\n> add on top of what you already said.  I'll fix one typo, though.\n\nHeh. Am I corrupting you, or you me?\n\n> > But this can break down in two ways:\n> >\n> >   1. Sometimes we blur the line of plumbing and porcelain, where\n> >      functionality is available only through plumbing. For example,\n> \n> s/through plumbing/through Porcelain/.\n\nOops, yes, thank you. That was what I meant.\n\n-Peff\n"},{"id":"101959","messageId":"alpine.DEB.1.00.0901261152320.14855@racer","threadId":"17354","inReplyTo":"20090126031206.GB14277@sigill.intra.peff.net","subject":"Re: [PATCH v1 1/3] Introduce config variable \"diff.primer\"","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-01-26T10:54:26Z","receivedAt":"2009-01-26T10:54:26Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 25 Jan 2009, Jeff King wrote:\n\n> So if you just want this from the command line, then I think it is safe \n> to have \"git diff\" always respect \"diff.primer\", and scripts shouldn't \n> be impacted.\n\nBut as Keith made clear, he wanted to use it from _git-gui_.  Which \n-- naturally -- _has_ to use plumbing, to guarantee a stable interface.\n\nSo \"fixing\" this in \"git diff\" is the wrong place; anything else than \nteaching \"git gui\" to remember user-defined diff options and to use them \nwould be a complicator's glove.\n\nCiao,\nDscho\n"},{"id":"101960","messageId":"alpine.DEB.1.00.0901261154330.14855@racer","threadId":"17354","inReplyTo":"20090126031206.GB14277@sigill.intra.peff.net","subject":"backwards compatibility, was Re: [PATCH v1 1/3] Introduce config variable \"diff.primer\"","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-01-26T10:59:46Z","receivedAt":"2009-01-26T10:59:46Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 25 Jan 2009, Jeff King wrote:\n\n>   1. Sometimes we blur the line of plumbing and porcelain, where\n>      functionality is available only through plumbing. For example,\n>      gitweb until recently called \"git diff\" because there is no other\n>      way to diff two arbitrary blobs. But the solution there is, I\n>      think, to make that functionality available through plumbing. Not\n>      to disallow enhancements to porcelain.\n\nJust a reminder: we are very conservative when it comes to breaking \nbackwards compatibility.  For example, people running (but not upgrading) \ngitweb who want to upgrade Git may rightfully expect their setups not to \nbe broken for a long time, if ever.\n\nSo your mentioning gitweb using \"git diff\" precludes all kind of cute \ngames, methinks.\n\nAnd please no \"anybody who would do this and that would be nuts\" excuses: \nif you want to change something fundamental like this, _you_ have to \ndefend it.\n\nIt is not acceptable to just shout out what you want and expect those \naffected negatively to do the impact analysis for you.\n\nCiao,\nDscho\n"},{"id":"101962","messageId":"20090126110641.GA19993@coredump.intra.peff.net","threadId":"17354","inReplyTo":"alpine.DEB.1.00.0901261152320.14855@racer","subject":"Re: [PATCH v1 1/3] Introduce config variable \"diff.primer\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-01-26T11:06:41Z","receivedAt":"2009-01-26T11:06:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 26, 2009 at 11:54:26AM +0100, Johannes Schindelin wrote:\n\n> > So if you just want this from the command line, then I think it is safe \n> > to have \"git diff\" always respect \"diff.primer\", and scripts shouldn't \n> > be impacted.\n> \n> But as Keith made clear, he wanted to use it from _git-gui_.  Which \n> -- naturally -- _has_ to use plumbing, to guarantee a stable interface.\n>\n> So \"fixing\" this in \"git diff\" is the wrong place; anything else than \n> teaching \"git gui\" to remember user-defined diff options and to use them \n> would be a complicator's glove.\n\nI think what you are missing here is that he specifically mentioned the\ncommand line, and I was responding to those comments. There are two\nseparate problems: default options for command line usage and the\nmechanism by which one can set options for things like git-gui.\n\nIn this case he was asking specifically about \"git diff\" from the\ncommand line, so fixing it in there _is_ the place to fix it (the only\nother alternative being to make a wrapper or alias).\n\nFor the other problem, it _may_ benefit from tool support that would\nhelp porcelains respect a \"diff.primer\" variable like Keith proposed.\nBut that has already been discussed elsewhere in the thread, so I'm not\ngoing to repeat it here.\n\n-Peff\n"},{"id":"101963","messageId":"20090126111605.GB19993@coredump.intra.peff.net","threadId":"17354","inReplyTo":"alpine.DEB.1.00.0901261154330.14855@racer","subject":"Re: backwards compatibility, was Re: [PATCH v1 1/3] Introduce config variable \"diff.primer\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-01-26T11:16:05Z","receivedAt":"2009-01-26T11:16:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 26, 2009 at 11:59:46AM +0100, Johannes Schindelin wrote:\n\n> Just a reminder: we are very conservative when it comes to breaking \n> backwards compatibility.  For example, people running (but not upgrading) \n> gitweb who want to upgrade Git may rightfully expect their setups not to \n> be broken for a long time, if ever.\n> \n> So your mentioning gitweb using \"git diff\" precludes all kind of cute \n> games, methinks.\n\nAre you aware that gitweb no longer calls \"git diff\", exactly because\nof problems caused by calling a porcelain from a script?\n\nI don't want to break existing setups, either. But at some point you\nhave to say \"this is porcelain, so don't rely on there not being any\nuser-triggered effects in its behavior\". If porcelain is cast in stone,\nthen what is the point in differentiating plumbing from porcelain?\n\nAnd when the line is blurred (as I think it is in several places), then\nit has to be dealt with on a case-by-case basis. What is the benefit,\nand what is the likelihood and extent of harm?\n\n> And please no \"anybody who would do this and that would be nuts\" excuses: \n> if you want to change something fundamental like this, _you_ have to \n> defend it.\n> \n> It is not acceptable to just shout out what you want and expect those \n> affected negatively to do the impact analysis for you.\n\nThis message is addressed to me, but I don't know exactly what you think\nI'm proposing, failing to defend, or failing to do an impact analysis\nfor. Or are you speaking generally of the \"you\" who submit patches?\n\n-Peff\n"},{"id":"101966","messageId":"alpine.DEB.1.00.0901261220300.14855@racer","threadId":"17354","inReplyTo":"20090126111605.GB19993@coredump.intra.peff.net","subject":"Re: backwards compatibility, was Re: [PATCH v1 1/3] Introduce config variable \"diff.primer\"","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-01-26T11:28:55Z","receivedAt":"2009-01-26T11:28:55Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 26 Jan 2009, Jeff King wrote:\n\n> On Mon, Jan 26, 2009 at 11:59:46AM +0100, Johannes Schindelin wrote:\n> \n> > Just a reminder: we are very conservative when it comes to breaking \n> > backwards compatibility.  For example, people running (but not upgrading) \n> > gitweb who want to upgrade Git may rightfully expect their setups not to \n> > be broken for a long time, if ever.\n> \n> Are you aware that gitweb no longer calls \"git diff\", exactly because\n> of problems caused by calling a porcelain from a script?\n\nAs I said: do you really expect people not to forget to upgrade gitweb \nmanually when they do \"sudo make install\" with a new Git version?\n\n> I don't want to break existing setups, either. But at some point you \n> have to say \"this is porcelain, so don't rely on there not being any \n> user-triggered effects in its behavior\". If porcelain is cast in stone, \n> then what is the point in differentiating plumbing from porcelain?\n\nTwo points there:\n\n- with gitweb, we were the offenders ourselves.  So we should give the \n  users of gitweb at least _some_ slack.\n\n- Concretely for the \"porcelain\" git diff: This workflow\n\n\tgit diff > my-patch\n\t<attach and send to somebody>\n\n  is probably pretty wide spread.  And it is okay, a user is not a script, \n  they are very much allowed to use porcelain.  And we _would_ break \n  expectations there.\n\nNow, I have another two, fundamental problems with the diff options \ndefaults: you are restricting the thing to _one_ set of options, and when \nsomebody wants to run without those options, she has to actively _undo_ \nthem.\n\nRemember, sometimes you need another set of options. Like, when I send \nmail to a Git user, I want \"-M -C -C\", when I send mail to a non-Git user, \nI do not want any additional options (and try to undo \"-M -C -C\" on the \ncommand line, good luck), and sometimes it is much easier to see what \nhappened with a word diff.\n\nSo what I need are three different sets of diff options.\n\nGuess how well that works with aliases -- we are talking command line \nhere after all, right?\n\nCiao,\nDscho\n"},{"id":"101977","messageId":"20090126115957.GA20558@coredump.intra.peff.net","threadId":"17354","inReplyTo":"alpine.DEB.1.00.0901261220300.14855@racer","subject":"Re: backwards compatibility, was Re: [PATCH v1 1/3] Introduce config variable \"diff.primer\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-01-26T11:59:57Z","receivedAt":"2009-01-26T11:59:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 26, 2009 at 12:28:55PM +0100, Johannes Schindelin wrote:\n\n> > Are you aware that gitweb no longer calls \"git diff\", exactly because\n> > of problems caused by calling a porcelain from a script?\n> \n> As I said: do you really expect people not to forget to upgrade gitweb \n> manually when they do \"sudo make install\" with a new Git version?\n\nYes.\n\nBut my point is that gitweb was _already_ broken, because it was calling\na porcelain, and there were _already_ features that could cause serious\nbreakage.\n\nSo yes, adding a new feature that a user can trigger causes one more\nopportunity for breakage. But the solution isn't to never ever add more\nfeatures to \"git diff\". It's to close the avenue by which the new _and_\nold breakages are triggered.\n\n> > I don't want to break existing setups, either. But at some point you \n> > have to say \"this is porcelain, so don't rely on there not being any \n> > user-triggered effects in its behavior\". If porcelain is cast in stone, \n> > then what is the point in differentiating plumbing from porcelain?\n> \n> Two points there:\n> \n> - with gitweb, we were the offenders ourselves.  So we should give the \n>   users of gitweb at least _some_ slack.\n\nI'm not sure I agree. I always assumed that since gitweb, git-gui, and\ngitk are bundled with git during release that we have _more_ leeway in\nmaking matching changes between them.\n\nAre you sure that you can run random versions of gitweb with random\nversions of git in the first place?\n\n> - Concretely for the \"porcelain\" git diff: This workflow\n> \n> \tgit diff > my-patch\n> \t<attach and send to somebody>\n> \n>   is probably pretty wide spread.  And it is okay, a user is not a script, \n>   they are very much allowed to use porcelain.  And we _would_ break \n>   expectations there.\n\nSorry, but what in the world are we supposed to do? Never ever allow the\nuser to specify diff options to a porcelain because they might impact\nthe output? A user who sets a config option or a command line option to\nimpact the output of \"git diff\" is responsible for how they use \"git\ndiff\".\n\nThere are already options like this in \"git diff\". I don't see how one\nmore changes anything.\n\n> Now, I have another two, fundamental problems with the diff options \n> defaults: you are restricting the thing to _one_ set of options, and when \n> somebody wants to run without those options, she has to actively _undo_ \n> them.\n\nYep, that's what defaults are. And guess what: we _already_ have the\nsame thing. I have diff.renames set in my ~/.gitconfig. That does\n_exactly_ what\n\n  git config --global diff.primer -M\n\nwould do. It's just a syntax that saves us from having to introduce a\nboatload of new variables, one per command line option.\n\n> Remember, sometimes you need another set of options. Like, when I send \n> mail to a Git user, I want \"-M -C -C\", when I send mail to a non-Git user, \n> I do not want any additional options (and try to undo \"-M -C -C\" on the \n> command line, good luck), and sometimes it is much easier to see what \n> happened with a word diff.\n\nThis is a strawman. You have described a scenario where an alias or a\nwrapper script is a better fit. Great, then use that mechanism in this\nscenario. But that doesn't mean there aren't other scenarios where a\ndifferent setup makes more sense (I think Keith's original goal was to\nuse \"-w\").\n\n> So what I need are three different sets of diff options.\n> \n> Guess how well that works with aliases -- we are talking command line \n> here after all, right?\n\nPersonally, I have always found the suggestion that users simply put\ntheir preferences into an alias like \"mydiff\" to be a silly one: git has\nalready taken the obvious good names, so now I am stuck using \"git\nmydiff\" forever and forgetting that \"git diff\" even exists.\n\nBut then, I don't have your \"three sets of options\" scenario. I just\nwant one set of defaults. So I don't have a need to name each one, and\nhaving to choose a different name becomes a detriment rather than an\nadvantage.\n\nHowever, there are two other drawbacks of aliases that I can think of:\n\n  1. They are tied to a specific command, whereas diff options are tied\n     to the concept of diffing. So now I have to write an alias (with a\n     new name) for each command:\n\n       git config alias.mylog 'log -w'\n       git config alias.mydiff 'diff -w'\n       git config alias.myshow 'show -w'\n\n  2. They can't change defaults based on the file to be diffed. One of\n     the things Keith mentioned (and I don't remember if this was\n     implemented in his patch series) was supporting this for\n     gitattributes diff drivers. How do I do\n\n       git config diff.tex.primer -w\n\n     using aliases?\n\nBut now you have me defending Keith's proposal, which he should be doing\nhimself ;P I actually am not that excited about it, and will probably\nnot use it for anything myself. But I think:\n\n  - it lets the user accomplish useful things that would not\n    otherwise be possible\n\n  - supporting it in \"git diff\" does not create any danger that was not\n    already there\n\nwhich means that I have no objection to a clean version being applied.\n\n-Peff\n"},{"id":"101991","messageId":"76718490901260729m21ba140dke157d1d461aed2d5@mail.gmail.com","threadId":"17354","inReplyTo":"20090126111605.GB19993@coredump.intra.peff.net","subject":"Re: backwards compatibility, was Re: [PATCH v1 1/3] Introduce config variable \"diff.primer\"","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2009-01-26T15:29:17Z","receivedAt":"2009-01-26T15:29:17Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Mon, Jan 26, 2009 at 6:16 AM, Jeff King <peff@peff.net> wrote:\n> I don't want to break existing setups, either. But at some point you\n> have to say \"this is porcelain, so don't rely on there not being any\n> user-triggered effects in its behavior\". If porcelain is cast in stone,\n> then what is the point in differentiating plumbing from porcelain?\n>\n> And when the line is blurred (as I think it is in several places)\n\nAside, AIX has commands that are run both directly or via smit (a\ncurses-based interface). When smit calls the commands, it passes a\nswitch to let said commands know that they are being run from smit.\ne.g.:\n\n       -J\n            This flag is used when the installp command is executed from the\n            System Management Interface Tool (SMIT) menus.\n\nPerhaps adding such a concept to those git commands which can be used\nin both porcelain and plumbing contexts would be useful for git.\n\nj.\n"},{"id":"102018","messageId":"20090126184829.GA27543@coredump.intra.peff.net","threadId":"17354","inReplyTo":"76718490901260729m21ba140dke157d1d461aed2d5@mail.gmail.com","subject":"Re: backwards compatibility, was Re: [PATCH v1 1/3] Introduce config variable \"diff.primer\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-01-26T18:48:30Z","receivedAt":"2009-01-26T18:48:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 26, 2009 at 10:29:17AM -0500, Jay Soffian wrote:\n\n> Aside, AIX has commands that are run both directly or via smit (a\n> curses-based interface). When smit calls the commands, it passes a\n> switch to let said commands know that they are being run from smit.\n> e.g.:\n> \n>        -J\n>             This flag is used when the installp command is executed from the\n>             System Management Interface Tool (SMIT) menus.\n> \n> Perhaps adding such a concept to those git commands which can be used\n> in both porcelain and plumbing contexts would be useful for git.\n\nSure, I think that is one of many possible ways that we could\ndifferentiate between confusing plumbing and porcelain; another is\nsplitting functionality into two similar commands, one of which is\nplumbing and one of which is porcelain.\n\nThe real problem with plans like that, though, is that there are\n_already_ scripts in the wild that don't understand \"-J\" (or whatever).\nMy impression from your description above is that \"-J\" means \"don't use\nfancy features, because we're being called from the menus\". And you\nreally want the opposite, which is that scripts opt _in_ to fancy\nfeatures, not _out_.\n\nBut then you have that problem that the _user_ is stuck specifying \"OK,\nturn on fancy features.\" And I don't relish the thought of typing \"git\ndiff -J\" every time. :)\n\n-Peff\n"},{"id":"102031","messageId":"76718490901261149xfedc415j8f5dab677b90d693@mail.gmail.com","threadId":"17354","inReplyTo":"20090126184829.GA27543@coredump.intra.peff.net","subject":"Re: backwards compatibility, was Re: [PATCH v1 1/3] Introduce config variable \"diff.primer\"","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2009-01-26T19:49:15Z","receivedAt":"2009-01-26T19:49:15Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Mon, Jan 26, 2009 at 1:48 PM, Jeff King <peff@peff.net> wrote:\n> But then you have that problem that the _user_ is stuck specifying \"OK,\n> turn on fancy features.\" And I don't relish the thought of typing \"git\n> diff -J\" every time. :)\n\nWell, this issue seems to come up every so often, so the idea would be:\n\n- We're adding a mechanism for scripts to communicate that they need\nplumbing context\n- Start using it in your scripts when calling git if you rely on a\nstable interface\n- In the next major release, git may introduce changes to commands\nwhich are not clearly plumbing if you haven't adopted the mechanism\n\nWhere mechanism could be a switch, environment variable, etc.\nTypically in a network API, the client and server have a way to\nnegotiate the highest level each supports; that's missing from git,\nbut seems like it would be useful.\n\nj.\n\np.s. perhaps you'd prefer -P? :)\n"},{"id":"102038","messageId":"7vd4e96dh7.fsf@gitster.siamese.dyndns.org","threadId":"17354","inReplyTo":"76718490901261149xfedc415j8f5dab677b90d693@mail.gmail.com","subject":"Re: backwards compatibility, was Re: [PATCH v1 1/3] Introduce config variable \"diff.primer\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-26T20:04:20Z","receivedAt":"2009-01-26T20:04:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jay Soffian <jaysoffian@gmail.com> writes:\n\n> On Mon, Jan 26, 2009 at 1:48 PM, Jeff King <peff@peff.net> wrote:\n>> But then you have that problem that the _user_ is stuck specifying \"OK,\n>> turn on fancy features.\" And I don't relish the thought of typing \"git\n>> diff -J\" every time. :)\n>\n> Well, this issue seems to come up every so often, so the idea would be:\n>\n> - We're adding a mechanism for scripts to communicate that they need\n> plumbing context\n> - Start using it in your scripts when calling git if you rely on a\n> stable interface\n> - In the next major release, git may introduce changes to commands\n> which are not clearly plumbing if you haven't adopted the mechanism\n\nWhere do all of these nonsense come from?  We are not adding any mechanism\nfor scripts to say they need plumbing context.  By calling plumbing they\nare already asking for stable plumbing behaviour.\n\nThe scripts can, if they want to, use newer options updated versions of\nthe plumbing commands offer, by passing them when they want to.\n\nAnd the trigger to do so is up to the scripts.  They can get new options\nfrom the end user, or they can peek into user's configuration variables\nsimilar to the diff.primer mentioned earlier in the discussion.\n\nOne way could be a new option --screw-me-with=name that can be given to a\nplumbing command and tells it pretend as if the command line options\nspecified by the configuration variable of the given name were given\n(e.g. a script runs \"git diff-files --screw-me-with=diff.primer\").\n\nThe important point is that it has to be opt _IN_.\n"},{"id":"102041","messageId":"76718490901261232k526e80daha7d9cbaed0178922@mail.gmail.com","threadId":"17354","inReplyTo":"7vd4e96dh7.fsf@gitster.siamese.dyndns.org","subject":"Re: backwards compatibility, was Re: [PATCH v1 1/3] Introduce config variable \"diff.primer\"","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2009-01-26T20:32:07Z","receivedAt":"2009-01-26T20:32:07Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Mon, Jan 26, 2009 at 3:04 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Where do all of these nonsense come from?  We are not adding any mechanism\n> for scripts to say they need plumbing context.  By calling plumbing they\n> are already asking for stable plumbing behaviour.\n\nThe suggestion was wrt to commands which are not strictly plumbing.\n\nj.\n"},{"id":"102042","messageId":"20090126203508.GC27604@coredump.intra.peff.net","threadId":"17354","inReplyTo":"7vd4e96dh7.fsf@gitster.siamese.dyndns.org","subject":"Re: backwards compatibility, was Re: [PATCH v1 1/3] Introduce config variable \"diff.primer\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-01-26T20:35:08Z","receivedAt":"2009-01-26T20:35:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 26, 2009 at 12:04:20PM -0800, Junio C Hamano wrote:\n\n> > Well, this issue seems to come up every so often, so the idea would be:\n> >\n> > - We're adding a mechanism for scripts to communicate that they need\n> > plumbing context\n> > - Start using it in your scripts when calling git if you rely on a\n> > stable interface\n> > - In the next major release, git may introduce changes to commands\n> > which are not clearly plumbing if you haven't adopted the mechanism\n> \n> Where do all of these nonsense come from?  We are not adding any mechanism\n> for scripts to say they need plumbing context.  By calling plumbing they\n> are already asking for stable plumbing behaviour.\n\nI think this is my fault a little for mentioning \"blurred plumbing and\nporcelain\" and not explaining further. This really has nothing to do\nwith actual plumbing commands. Scripts should use diff-tree and not\ndiff, and that has always and probably will always be the case.\n\nBut what about something like \"git grep\"? Is it plumbing or porcelain?\nThe functionality isn't exposed in any other way, so I can imagine that\nsome scripts are using it. But it's sad to think that we could never\nhave config that might change its behavior, because it is used directly\nby users all the time.  I think the same applies for \"git archive\".\nThere may be others.\n\nSo something like Jay's proposal could future-proof those commands\nbetter by allowing scripts to say \"BTW, I am a script. Turn off your new\nfeatures.\" And there are two classes of alternatives:\n\n  - as you described, instead of making scripts turn _off_ features,\n    make them turn them _on_, effectively declaring these commands as\n    plumbing. This is obviously much nicer because it Just Works with\n    current scripts. But it means that these mixed porcelain/plumbing\n    commands suffer in their porcelain capacity; we can never add a\n    config option that might change the behavior without the user\n    specifying \"it's ok to use this feature\" at each invocation.\n\n  - we can provide support _now_ for splitting the functionality into\n    porcelain and plumbing, scripts can adapt over time to using the\n    plumbing version, and then eventually we can declare it safe to make\n    changes to the porcelain. And that is more or less what Jay's\n    proposal is doing.\n\n    However, I don't think a command-line option is the best way to say\n    \"I am a script\". It's too easy to type \"git grep\" in a script and\n    \"git grep -J\" (either because you are clueless about \"-J\", or\n    because you simply forget). Other signal methods include:\n\n      - just making two different commands to expose the same\n        functionality, one plumbing and one porcelain. This is what has\n        evolved in other areas, such as diff (though note that there\n        _isn't_ exactly a \"git diff\" plumbing command -- there is the\n        plumbing that \"git diff is based on). I'm not sure what the\n        plumbing name for \"git grep\" would be.\n\n      - set an environment variable like GIT_STRICT. It's easy to set\n        once at the top of your script, and it trickles down\n        automatically as we call other git commands and scripts.\n        We could even set it in git-sh-setup, though that of course\n        covers only shell scripts, and not other callers.\n\n> The scripts can, if they want to, use newer options updated versions of\n> the plumbing commands offer, by passing them when they want to.\n> \n> And the trigger to do so is up to the scripts.  They can get new options\n> from the end user, or they can peek into user's configuration variables\n> similar to the diff.primer mentioned earlier in the discussion.\n\nRight, I think that is absolutely the right thing for commands which are\nclearly plumbing.\n\n> One way could be a new option --screw-me-with=name that can be given to a\n> plumbing command and tells it pretend as if the command line options\n> specified by the configuration variable of the given name were given\n> (e.g. a script runs \"git diff-files --screw-me-with=diff.primer\").\n> \n> The important point is that it has to be opt _IN_.\n\nOur precedent so far has been to just add a new command line option that\nenables the feature (e.g., --ext-diff and --textconv). Functionally it\nis no different than --screw-me-with=diff.*.textconv. :)\n\n-Peff\n"},{"id":"102071","messageId":"alpine.GSO.2.00.0901261734360.16158@kiwi.cs.ucla.edu","threadId":"17354","inReplyTo":"20090125220756.GA18855@coredump.intra.peff.net","subject":"Re: [PATCH v1 0/3] Introduce config variable \"diff.primer\"","fromName":"Keith Cascio","fromEmail":"keith@cs.ucla.edu","sentAt":"2009-01-27T01:47:54Z","receivedAt":"2009-01-27T01:47:54Z","isPatch":true,"sender":{"key":"keith@cs.ucla.edu","avatar":"https://gravatar.com/avatar/c5ec3a8f1cd1f449fdf8bdb7125fdbfd10b729507f32cbf0aa4ad07b4f7127ae?d=mp&s=160"},"body":"On Sun, 25 Jan 2009, Jeff King wrote:\n\n> let's say I have a gitattributes file like this:\n>    *.c diff=c\n> and my config says:\n>   [diff]\n>     opt1 = val1_default\n>     opt2 = val2_default\n>   [diff \"c\"]\n>     opt1 = val1_c\n> \n> Now obviously if I want to use opt1 for my C files, it should be val1_c.\n> But if I want to use opt2, what should it use? There are two reasonable\n> choices, I think:\n>   1. You use val2_default. The rationale is that the \"c\" diff driver did\n>      not define an opt2, so you fall back to the global default.\n>   2. It is unset. The rationale is that you are using the \"c\" diff\n>      driver, and it has left the value unset. The default then means \"if\n>      you have no diff driver setup\".\n\nI'm in favor of option (2), because [diff] a.k.a [diff \"\"] serving as the \nfallback for [diff *] feels like a special case.  If a full system of precedence \nand fallbacks is desired for .git/config, we should adopt explicit grammar that \nlets me define an arbitrary precedence tree over all sections.  Once a powerful \nconcept is born, somewhere down the road, some user will desire its \nuniversalization.\n"},{"id":"102074","messageId":"alpine.GSO.2.00.0901261749230.16158@kiwi.cs.ucla.edu","threadId":"17354","inReplyTo":"20090126115957.GA20558@coredump.intra.peff.net","subject":"Re: backwards compatibility, was Re: [PATCH v1 1/3] Introduce config variable \"diff.primer\"","fromName":"Keith Cascio","fromEmail":"keith@cs.ucla.edu","sentAt":"2009-01-27T03:01:33Z","receivedAt":"2009-01-27T03:01:33Z","isPatch":true,"sender":{"key":"keith@cs.ucla.edu","avatar":"https://gravatar.com/avatar/c5ec3a8f1cd1f449fdf8bdb7125fdbfd10b729507f32cbf0aa4ad07b4f7127ae?d=mp&s=160"},"body":"On Mon, 26 Jan 2009, Jeff King wrote:\n\n> Yep, that's what defaults are. And guess what: we _already_ have the same \n> thing. I have diff.renames set in my ~/.gitconfig. That does _exactly_ what\n> \n>   git config --global diff.primer -M\n> \n> would do. It's just a syntax that saves us from having to introduce a boatload \n> of new variables, one per command line option.\n\n\nGreat point, IOW, primer magnifies already existing pitfalls.  I like that about \nprimer v1, however unsatisfying it is otherwise.  Rather than stick a piece of \nchewing gum in that chink in the dam, let's repair it and get the whole darn \nthing ready for the 21st century while we're at it.\n\n> However, there are two other drawbacks of aliases that I can think of:\n>   1. They are tied to a specific command, whereas diff options are tied\n>      to the concept of diffing. So now I have to write an alias (with a\n>      new name) for each command:\n>        git config alias.mylog 'log -w'\n>        git config alias.mydiff 'diff -w'\n>        git config alias.myshow 'show -w'\n>   2. They can't change defaults based on the file to be diffed. One of\n>      the things Keith mentioned (and I don't remember if this was\n>      implemented in his patch series) was supporting this for\n>      gitattributes diff drivers. How do I do\n>        git config diff.tex.primer -w\n>      using aliases?\n\nThis scenario was specifically part of my motivation for imagining primer v1 as \nI did.\n\n> But now you have me defending Keith's proposal\n\nAnd well.\n\n> which he should be doing himself ;P\n\nTrue.  I'm at the point here where I will demand of myself one of two outcomes.  \nEither I:\n\n(1) Satisfy myself on a deep, foundational level why the inescapable structure \nof not just this particularly well-constituted software project (Git!), but \nsoftware projects in general, since surely many a fearless development team has \nbraved this philosophical sticky place before us, prevents one from getting \neverything one wants, i.e. forces a compromise, a.k.a. the \"no silver bullet\" \ninterpretation.\n\n(2) Write primer patch v2 that somehow does THE RIGHT THING, also satisfying on \na deep level to at least myself, a.k.a. the silver bullet.\n\n                                            -- Keith\n"},{"id":"102082","messageId":"20090127045452.GB735@coredump.intra.peff.net","threadId":"17354","inReplyTo":"alpine.GSO.2.00.0901261734360.16158@kiwi.cs.ucla.edu","subject":"Re: [PATCH v1 0/3] Introduce config variable \"diff.primer\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-01-27T04:54:52Z","receivedAt":"2009-01-27T04:54:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 26, 2009 at 05:47:54PM -0800, Keith Cascio wrote:\n\n> >   2. It is unset. The rationale is that you are using the \"c\" diff\n> >      driver, and it has left the value unset. The default then means \"if\n> >      you have no diff driver setup\".\n> \n> I'm in favor of option (2), because [diff] a.k.a [diff \"\"] serving as\n\nNit: [diff] and [diff \"\"] are different. The \"dotted\" notation which we\nuse in the code and which git-config respects for a variable \"foo\" in\neach section would look like \"diff.foo\" and \"diff..foo\", respectively.\n\n> the fallback for [diff *] feels like a special case.  If a full system\n> of precedence \n\nYes, I think having a \"this is the default driver whose driver-specific\noptions are used if you don't have a different driver\" is semantically\nsimple and clear.\n\n> and fallbacks is desired for .git/config, we should adopt explicit\n> grammar that lets me define an arbitrary precedence tree over all\n> sections.  Once a powerful concept is born, somewhere down the road,\n> some user will desire its universalization.\n\nOK, now you're scaring me. :) I'm not sure I want to see the grammar you\nwould use to define an arbitrary precedence tree, or whether such\ncomplexity has any real-world use in git config. I think you would have\nto show a concrete example to prove the utility of something like that.\n\n-Peff\n"}]}