{"thread":{"id":"17509","subject":"[PATCH v2 1/2] Introduce config variable \"diff.primer\"","startedAt":"2009-02-02T18:20:54Z","lastAt":"2009-04-18T21:15:05Z","messageCount":50,"participants":["Keith Cascio","Jeff King","Jakub Narebski","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"102877","messageId":"1233598855-1088-2-git-send-email-keith@cs.ucla.edu","threadId":"17509","inReplyTo":"1233598855-1088-1-git-send-email-keith@cs.ucla.edu","subject":"[PATCH v2 1/2] Introduce config variable \"diff.primer\"","fromName":"Keith Cascio","fromEmail":"keith@cs.ucla.edu","sentAt":"2009-02-02T18:20:54Z","receivedAt":"2009-02-02T18:20:54Z","isPatch":true,"sender":{"key":"keith@cs.ucla.edu","avatar":"https://gravatar.com/avatar/c5ec3a8f1cd1f449fdf8bdb7125fdbfd10b729507f32cbf0aa4ad07b4f7127ae?d=mp&s=160"},"body":"Introduce config variable \"diff.primer\".\nImprove porcelain diff's accommodation of user preference by allowing\nsome settings to (a) persist over all invocations and (b) stay consistent\nover multiple tools (e.g. command-line and gui). The approach taken here\nis good because it delivers the consistency a user expects without breaking\nany plumbing. It works by allowing the user, via git-config, to specify\narbitrary options to pass to porcelain diff on every invocation, including\ninternal invocations from other programs, e.g. git-gui. Introduce diff\ncommand-line options --primer and --no-primer. Affect only porcelain diff:\nwe suppress primer options for plumbing diff-{files,index,tree},\nformat-patch, and all other commands unless explicitly requested using\n--primer (opt-in). Teach gitk to use --primer, but protect it from\ninapplicable options like --color.\n\nSigned-off-by: Keith Cascio \n---\n Documentation/config.txt | 14 +++++++\n Documentation/diff-options.txt | 10 +++++\n builtin-diff.c | 2 +\n diff.c | 77 +++++++++++++++++++++++++++++++++++-----\n diff.h | 14 ++++++--\n gitk-git/gitk | 16 ++++----\n 6 files changed, 113 insertions(+), 20 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex e2b8775..bd85c4a 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. `\"--patience --color --ignore-space-at-eol --exit-code\"`.\n+\tSee linkgit:git-diff[1]. You can suppress these at run time with\n+\toption `--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 --patience -q\n+--quiet -R -r --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 813a7b1..f422055 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -254,5 +254,15 @@ override configuration settings.\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 \"--patience --color --ignore-space-at-eol --exit-code\"`\n+\n+--primer::\n+\tOpt-in for default options specified in '.git/config'. This option is\n+\tmost often used with the three plumbing commands diff-{files,index,tree}.\n+\tThese commands normally suppress default options.\n+\n For more detailed explanation on these common options, see also\n linkgit:gitdiffcore[7].\ndiff --git a/builtin-diff.c b/builtin-diff.c\nindex d75d69b..b3c3e87 100644\n--- a/builtin-diff.c\n+++ b/builtin-diff.c\n@@ -284,6 +284,8 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \n \tinit_revisions(&rev, prefix);\n \n+\tDIFF_OPT_SET(&rev.diffopt, PRIMER);\n+\n \t/* If this is a no-index diff, just run it and exit there. */\n \tdiff_no_index(&rev, argc, argv, nongit, prefix);\n \ndiff --git a/diff.c b/diff.c\nindex a5a540f..32455c3 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -26,6 +26,8 @@ static int diff_suppress_blank_empty;\n int diff_use_color_default = -1;\n static const char *diff_word_regex_cfg;\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@@ -106,6 +108,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@@ -2316,6 +2320,45 @@ 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 parse_diff_primer(struct diff_options *options)\n+{\n+\tchar *str1, *token, *saveptr;\n+\tint len;\n+\n+\tif ((! diff_primer) || ((len = (strlen(diff_primer)+1)) < 3))\n+\t\treturn;\n+\n+\ttoken = str1 = strncpy((char*) malloc(len), diff_primer, len);\n+\tif ((saveptr = strpbrk(token += strspn(token, blank), blank)))\n+\t\t*(saveptr++) = '\\0';\n+\twhile (token) {\n+\t\tif (*token == '-')\n+\t\t\tdiff_opt_parse(options, (const char **) &token, -1);\n+\t\tif ((token = saveptr))\n+\t\t\tif ((saveptr = strpbrk(token += strspn(token, blank), blank)))\n+\t\t\t\t*(saveptr++) = '\\0';\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-McCluskey\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@@ -2326,14 +2369,15 @@ 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\tDIFF_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+\telse if (DIFF_OPT_TST(options, COLOR_DIFF))\n \t\tDIFF_OPT_CLR(options, COLOR_DIFF);\n \toptions->detect_rename = diff_detect_rename_default;\n \n@@ -2423,6 +2467,14 @@ 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, PRIMER)) {\n+\t\tif (! primer) {\n+\t\t\tdiff_setup(primer = (struct diff_options *) malloc(sizeof(struct diff_options)));\n+\t\t\tparse_diff_primer(primer);\n+\t\t}\n+\t\tflatten_diff_options(options, primer);\n+\t}\n+\n \treturn 0;\n }\n \n@@ -2570,13 +2622,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@@ -2597,10 +2649,13 @@ 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 (!prefixcmp(arg, \"--color-words=\")) {\n-\t\toptions->flags |= DIFF_OPT_COLOR_DIFF | DIFF_OPT_COLOR_DIFF_WORDS;\n+\t\tDIFF_OPT_SET(options, COLOR_DIFF);\n+\t\tDIFF_OPT_SET(options, COLOR_DIFF_WORDS);\n \t\toptions->word_regex = arg + 14;\n \t}\n \telse if (!strcmp(arg, \"--exit-code\"))\n@@ -2617,6 +2672,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, \"--primer\"))\n+\t\tDIFF_OPT_SET(options, PRIMER);\n+\telse if (!strcmp(arg, \"--no-primer\"))\n+\t\tDIFF_OPT_CLR(options, PRIMER);\n \n \t/* misc options */\n \telse if (!strcmp(arg, \"-z\"))\ndiff --git a/diff.h b/diff.h\nindex 23cd90c..7f11b12 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -66,9 +66,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_PRIMER (1 << 22)\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_OPT_DRT(opts, 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_XDL_DRT(opts, flag) ((opts)->xdl_mask & XDF_##flag)\n \n struct diff_options {\n \tconst char *filter;\n@@ -77,6 +83,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 +102,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/gitk-git/gitk b/gitk-git/gitk\nindex dc2a439..b67bbaa 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 --primer --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 --primer --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 --primer --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 --primer --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 --primer --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 --primer --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 --primer --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 --primer --no-color --stdin -p --pretty\"\n \n set gitencoding {}\n catch {\n-- \n1.6.1\n"},{"id":"102880","messageId":"1233598855-1088-3-git-send-email-keith@cs.ucla.edu","threadId":"17509","inReplyTo":"1233598855-1088-2-git-send-email-keith@cs.ucla.edu","subject":"[PATCH v2 2/2] Test functionality of new config variable \"diff.primer\"","fromName":"Keith Cascio","fromEmail":"keith@cs.ucla.edu","sentAt":"2009-02-02T18:20:55Z","receivedAt":"2009-02-02T18:20:55Z","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 \n---\n t/t4035-diff-primer.sh | 129 ++++++++++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 129 insertions(+), 0 deletions(-)\n create mode 100755 t/t4035-diff-primer.sh\n\ndiff --git a/t/t4035-diff-primer.sh b/t/t4035-diff-primer.sh\nnew file mode 100755\nindex 0000000..c33911c\n--- /dev/null\n+++ b/t/t4035-diff-primer.sh\n@@ -0,0 +1,129 @@\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 born empty' '\n+[ -z $(git config --get diff.primer) ]\n+'\n+\n+tr 'Q_' '\\015 ' << EOF > expect_noprimer\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 git-diff with empty value of diff.primer' 'test_cmp expect_noprimer 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_noprimer out'\n+git diff-files -p > out\n+test_expect_success 'ensure diff-files unaffected by diff.primer' 'test_cmp expect_noprimer out'\n+git diff-index -p HEAD > out\n+test_expect_success 'ensure diff-index unaffected by diff.primer' 'test_cmp expect_noprimer out'\n+\n+cat << EOF > expect_primer\n+diff --git a/x b/x\n+index d99af23..8b32fb5 100644\n+EOF\n+git diff > out\n+test_expect_success 'test git-diff with diff.primer = -w' 'test_cmp expect_primer out'\n+git diff-files -p --primer > out\n+test_expect_success 'ensure diff-files honors --primer' 'test_cmp expect_primer out'\n+git diff-index -p --primer HEAD > out\n+test_expect_success 'ensure diff-index honors --primer' 'test_cmp expect_primer 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 format-patch unaffected by diff.primer' 'test_cmp expect out'\n+\n+git add x\n+git commit -m '2.0' >/dev/null 2>&1\n+\n+git config diff.primer '-w'\n+\n+git diff-tree -p -r HEAD^ HEAD > out\n+test_expect_success 'ensure diff-tree unaffected by diff.primer' 'test_cmp expect_noprimer out'\n+git diff-tree -p -r --primer HEAD^ HEAD > out\n+test_expect_success 'ensure diff-tree honors --primer' 'test_cmp expect_primer out'\n+\n+test_done\n+\n-- \n1.6.1\n"},{"id":"102893","messageId":"alpine.GSO.2.00.0902021242060.7881@kiwi.cs.ucla.edu","threadId":"17509","inReplyTo":"1233598855-1088-3-git-send-email-keith@cs.ucla.edu","subject":"[PATCH v2 0/2] Introduce config variable \"diff.primer\"","fromName":"Keith Cascio","fromEmail":"keith@cs.ucla.edu","sentAt":"2009-02-02T20:45:04Z","receivedAt":"2009-02-02T20:45:04Z","isPatch":true,"sender":{"key":"keith@cs.ucla.edu","avatar":"https://gravatar.com/avatar/c5ec3a8f1cd1f449fdf8bdb7125fdbfd10b729507f32cbf0aa4ad07b4f7127ae?d=mp&s=160"},"body":"Seems like the summary email for this patch refuses to deliver through the list. \nI can send it to anyone individually if you are interested.\n\n -- Keith\n"},{"id":"102895","messageId":"alpine.GSO.2.00.0902021302000.7881@kiwi.cs.ucla.edu","threadId":"17509","inReplyTo":"1233598855-1088-2-git-send-email-keith@cs.ucla.edu","subject":"[PATCH v2 0/2] Introduce config variable \"diff.primer\"","fromName":"Keith Cascio","fromEmail":"keith@cs.ucla.edu","sentAt":"2009-02-02T21:03:59Z","receivedAt":"2009-02-02T21:03:59Z","isPatch":true,"sender":{"key":"keith@cs.ucla.edu","avatar":"https://gravatar.com/avatar/c5ec3a8f1cd1f449fdf8bdb7125fdbfd10b729507f32cbf0aa4ad07b4f7127ae?d=mp&s=160"},"body":"The next two patches introduce a means by which to specify non-default options \nporcelain diff automatically obeys. This version v2 fixes the problem with v1 \n(violation of plumbing guarantee) by switching to opt_in rather than opt_out \nsemantics. v2 also corrects code style to mimic established convention.\n\nAt least one list poster expressed interest in implementing complimentary\nfunctionality, i.e. \"primer.*\":\nhttp://article.gmane.org/gmane.comp.version-control.git/107158\n\"primer.diff\" compliments \"diff.primer\", the two styles in no way exclude each\nother, and therefore \"primer.*\" is a good opportunity for future work.\n\nKeith Cascio (2):\n Introduce config variable \"diff.primer\"\n Test functionality of new config variable \"diff.primer\"\n\n Documentation/config.txt | 14 ++++\n Documentation/diff-options.txt | 10 +++\n builtin-diff.c | 2 +\n diff.c | 77 +++++++++++++++++++++---\n diff.h | 14 ++++-\n gitk-git/gitk | 16 +++---\n t/t4035-diff-primer.sh | 129 ++++++++++++++++++++++++++++++++++++++++\n 7 files changed, 242 insertions(+), 20 deletions(-)\n"},{"id":"102931","messageId":"20090203071516.GC21367@sigill.intra.peff.net","threadId":"17509","inReplyTo":"1233598855-1088-2-git-send-email-keith@cs.ucla.edu","subject":"Re: [PATCH v2 1/2] Introduce config variable \"diff.primer\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-02-03T07:15:16Z","receivedAt":"2009-02-03T07:15:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 02, 2009 at 10:20:54AM -0800, Keith Cascio wrote:\n\n> Introduce config variable \"diff.primer\".\n\nYou don't need to repeat the subject in the body.\n\n> Improve porcelain diff's accommodation of user preference by allowing\n> some settings to (a) persist over all invocations and (b) stay consistent\n> over multiple tools (e.g. command-line and gui). The approach taken here\n> is good because it delivers the consistency a user expects without breaking\n> any plumbing. It works by allowing the user, via git-config, to specify\n> arbitrary options to pass to porcelain diff on every invocation, including\n> internal invocations from other programs, e.g. git-gui. Introduce diff\n> command-line options --primer and --no-primer. Affect only porcelain diff:\n> we suppress primer options for plumbing diff-{files,index,tree},\n> format-patch, and all other commands unless explicitly requested using\n> --primer (opt-in). Teach gitk to use --primer, but protect it from\n> inapplicable options like --color.\n\nParagraph breaks might have made this a bit easier to read.\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. `\"--patience --color --ignore-space-at-eol --exit-code\"`.\n> +\tSee linkgit:git-diff[1]. You can suppress these at run time with\n> +\toption `--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 --patience -q\n> +--quiet -R -r --relative -t --text --textconv -w`\n\nFunny indentation?\n\nThis seems really clunky to list all of the options here. I thought the\npoint was to respect _all_ of them, but do it from porcelain so that it\nis up to the user what they want to put in.\n\nHow was this list chosen?\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 \"--patience --color --ignore-space-at-eol --exit-code\"`\n> +\n> +--primer::\n> +\tOpt-in for default options specified in '.git/config'. This option is\n> +\tmost often used with the three plumbing commands diff-{files,index,tree}.\n> +\tThese commands normally suppress default options.\n> +\n\nSome of the manpages use a more terse form for negatable options, like:\n\n --[no-]primer::\n\nwhich often helps focus the text a bit. Something like:\n\n --[no-]primer::\n Respect (or ignore) options specifed in the diff.primer\n configuration variable. By default, porcelain commands (such as `git\n diff` and `git log`) respect this variable, but plumbing commands\n (such as `git diff-{files,index,tree}`) do not.\n\nAlso, don't mention \".git/config\" by name: configuration can come from\n~/.gitconfig, a system-wide gitconfig, or .git/config.\n\n> @@ -284,6 +284,8 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n> [...]\n> +\tDIFF_OPT_SET(&rev.diffopt, PRIMER);\n\nProbably ALLOW_PRIMER is a more sensible name, to match ALLOW_EXTERNAL\nand ALLOW_TEXTCONV.\n\n> +static const char blank[] = \" \\t\\r\\n\";\n> +\n> +void parse_diff_primer(struct diff_options *options)\n> +{\n> +\tchar *str1, *token, *saveptr;\n> +\tint len;\n> +\n> +\tif ((! diff_primer) || ((len = (strlen(diff_primer)+1)) < 3))\n> +\t\treturn;\n> +\n> +\ttoken = str1 = strncpy((char*) malloc(len), diff_primer, len);\n> +\tif ((saveptr = strpbrk(token += strspn(token, blank), blank)))\n> +\t\t*(saveptr++) = '\\0';\n> +\twhile (token) {\n> +\t\tif (*token == '-')\n> +\t\t\tdiff_opt_parse(options, (const char **) &token, -1);\n> +\t\tif ((token = saveptr))\n> +\t\t\tif ((saveptr = strpbrk(token += strspn(token, blank), blank)))\n> +\t\t\t\t*(saveptr++) = '\\0';\n> +\t}\n> +\n> +\tfree( str1 );\n> +}\n\nThis doesn't appear to have any quoting mechanism. Is it impossible to\nhave an option with spaces (e.g., --relative='foo bar')? I guess that is\nprobably uncommon, but I would expect normal shell quoting rules to\napply.\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\nStyle: long lines.\n\n> +\t//minimized by Quine-McCluskey\n\nStyle: no C99/C++ comments.\n\n> +\tmaster->flags = (~x1&x2&x3)|(x0&~x3)|(x0&x1);\n\nStyle: whitespace between operands and operators.\n\nI have to admit that this particular line is pretty dense to read. You\nhave eliminated any meaning from the variable names (like the fact that\nyou have a master/slave pair of flag/mask pairs). Yes, you point to the\nQuine-McCluskey algorithm in the comment above, but I think something\nlike this would be easier to see what is going on:\n\n /*\n * Our desired flags are:\n *\n * 1. Anything the master hasn't explicitly set, we can take from\n * the slave.\n * 2. Anything the slave didn't explicitly, we can take whether or\n * not the master set it explicitly.\n * 3. Anything the master explicitly set, we take.\n */\n master->flags =\n /* (1) */ (~master->flags & slave->flags & slave->mask) |\n /* (2) */ (master->flags & ~slave->mask) |\n /* (3) */ (master->flags & master->mask);\n\n> @@ -2326,14 +2369,15 @@ 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\tDIFF_OPT_CLR(options, DIRSTAT_CUMULATIVE);\n\nHmm. I haven't gotten to any changes to DIFF_OPT_{SET,CLR} yet. But it\nis a little worrisome that this patch is so invasive as to require a\nchange like this. I wouldn't be surprised to find other spots outside of\ndiff.c where the options are munged by various programs. Did you audit\nfor all such spots?\n\n> +\tif (DIFF_OPT_TST(options, PRIMER)) {\n> +\t\tif (! primer) {\n> +\t\t\tdiff_setup(primer = (struct diff_options *) malloc(sizeof(struct diff_options)));\n\nFirst, don't use malloc. Use the xmalloc wrapper that will try to free\npack memory and/or die if it fails.\n\nSecondly, don't cast the result of malloc. At best it is pointless and\nverbose, and at worst it can hide errors caused by a missing function\ndeclaration.\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\nIt often makes the patch easier to review if you split changes like this\nout into a separate patch. Then your series is\n\n 1/2: use DIFF_XDL_SET instead of raw bit-masking\n\n This is a cleanup in preparation for option-setting doing\n something more complex than just setting a bit-mask. The code\n should behave exactly the same.\n\n 2/2: primer patch\n\n ... DIFF_XDL_SET tracks not only the set options, but which ones\n were set explicitly via a mask ...\n\nThen we can all see pretty easily that patch 1/2 doesn't change the\nbehavior, and each patch is a much smaller, succint chunk to review.\n\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\nDitto here with DIFF_OPT_SET.\n\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_OPT_DRT(opts, flag) ((opts)->mask & DIFF_OPT_##flag)\n\nOK, I see what it is supposed to do, but what does DRT stand for? Also,\nwhat practical use does it have? I don't see anybody _calling_ it.\n\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 --primer --no-color -r -s --stdin $gdtargs]\n\nDoes gitk really want to respect --primer? Might it not make more sense\nfor it, as a porcelain, to respect the diff.primer variable itself, and\nprepend it to the list of diff args? Then it has the power to veto any\noptions which it doesn't handle.\n\nAlso, any gitk changes should almost certainly be split into a different\npatch.\n\n\n\nAll in all, this was a lot more complicated than I was expecting. Why\nisn't the behavior of \"diff.primer\" simply \"pretend as if the options in\ndiff.primer were prepended to the command line\"? That is easy to\nexplain, and easy to implement (the only trick is that you have to do an\nextra pass to find --[no-]primer). Is there some drawback to such a\nsimple scheme that I am missing?\n\n-Peff\n"},{"id":"103003","messageId":"alpine.GSO.2.00.0902030833250.5994@kiwi.cs.ucla.edu","threadId":"17509","inReplyTo":"20090203071516.GC21367@sigill.intra.peff.net","subject":"Re: [PATCH v2 1/2] Introduce config variable \"diff.primer\"","fromName":"Keith Cascio","fromEmail":"keith@cs.ucla.edu","sentAt":"2009-02-03T17:55:08Z","receivedAt":"2009-02-03T17:55:08Z","isPatch":true,"sender":{"key":"keith@cs.ucla.edu","avatar":"https://gravatar.com/avatar/c5ec3a8f1cd1f449fdf8bdb7125fdbfd10b729507f32cbf0aa4ad07b4f7127ae?d=mp&s=160"},"body":"Peff,\nFirst of all, thanks for the tips!\n\nOn Tue, 3 Feb 2009, Jeff King wrote:\n\n> You don't need to repeat the subject in the body.\n\nOK.\n\n> Paragraph breaks might have made this a bit easier to read.\n\nHow about:\n\nImprove porcelain diff's accommodation of user preference by allowing\nsome settings to (a) persist over all invocations and (b) stay consistent\nover multiple tools (e.g. command-line and gui). The approach taken here\nis good because it delivers the consistency a user expects without breaking\nany plumbing. It works by allowing the user, via git-config, to specify\narbitrary options to pass to porcelain diff on every invocation, including\ninternal invocations from other programs, e.g. git-gui.\n\nIntroduce diff command-line options --primer and --no-primer.\n\nAffect only porcelain diff: we suppress primer options for plumbing \ndiff-{files,index,tree}, format-patch, and all other commands unless explicitly \nrequested using --primer (opt-in).\n\nTeach gitk to use --primer, but protect it from inapplicable options like \n--color.\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. `\"--patience --color --ignore-space-at-eol --exit-code\"`.\n> > +\tSee linkgit:git-diff[1]. You can suppress these at run time with\n> > +\toption `--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 --patience -q\n> > +--quiet -R -r --relative -t --text --textconv -w`\n> \n> Funny indentation?\n\nThe last part is a backtick-quoted list, I think it either must occur on one \nline or as is.\n\n> This seems really clunky to list all of the options here. I thought the\n> point was to respect _all_ of them, but do it from porcelain so that it\n> is up to the user what they want to put in.\n> \n> How was this list chosen?\n\nThe current version does not try to support all diff options. It only supports \nthose that are recorded in struct diff_options.flags and .xdl_opts - that is the \npresent list.\n\n> Some of the manpages use a more terse form for negatable options, like:\n> \n> --[no-]primer::\n> \n> which often helps focus the text a bit. Something like:\n> \n> --[no-]primer::\n> Respect (or ignore) options specifed in the diff.primer\n> configuration variable. By default, porcelain commands (such as `git\n> diff` and `git log`) respect this variable, but plumbing commands\n> (such as `git diff-{files,index,tree}`) do not.\n\nAgree.\n\n> Also, don't mention \".git/config\" by name: configuration can come from\n> ~/.gitconfig, a system-wide gitconfig, or .git/config.\n\nOK.\n\n> > @@ -284,6 +284,8 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n> > [...]\n> > +\tDIFF_OPT_SET(&rev.diffopt, PRIMER);\n> \n> Probably ALLOW_PRIMER is a more sensible name, to match ALLOW_EXTERNAL\n> and ALLOW_TEXTCONV.\n\nAgree.\n\n> > +static const char blank[] = \" \\t\\r\\n\";\n> > +\n> > +void parse_diff_primer(struct diff_options *options)\n> > +{\n> > +\tchar *str1, *token, *saveptr;\n> > +\tint len;\n> > +\n> > +\tif ((! diff_primer) || ((len = (strlen(diff_primer)+1)) < 3))\n> > +\t\treturn;\n> > +\n> > +\ttoken = str1 = strncpy((char*) malloc(len), diff_primer, len);\n> > +\tif ((saveptr = strpbrk(token += strspn(token, blank), blank)))\n> > +\t\t*(saveptr++) = '\\0';\n> > +\twhile (token) {\n> > +\t\tif (*token == '-')\n> > +\t\t\tdiff_opt_parse(options, (const char **) &token, -1);\n> > +\t\tif ((token = saveptr))\n> > +\t\t\tif ((saveptr = strpbrk(token += strspn(token, blank), blank)))\n> > +\t\t\t\t*(saveptr++) = '\\0';\n> > +\t}\n> > +\n> > +\tfree( str1 );\n> > +}\n> \n> This doesn't appear to have any quoting mechanism. Is it impossible to have an \n> option with spaces (e.g., --relative='foo bar')? I guess that is probably \n> uncommon, but I would expect normal shell quoting rules to apply.\n\nThe current version only supports the flags listed above in \nDocumentation/config.txt.\n\n> Style: long lines.\n\nOK.\n\n> Style: no C99/C++ comments.\n\nOK.\n\n> > +\tmaster->flags = (~x1&x2&x3)|(x0&~x3)|(x0&x1);\n> \n> Style: whitespace between operands and operators.\n> \n> I have to admit that this particular line is pretty dense to read. You have \n> eliminated any meaning from the variable names (like the fact that you have a \n> master/slave pair of flag/mask pairs). Yes, you point to the Quine-McCluskey \n> algorithm in the comment above, but I think something like this would be \n> easier to see what is going on:\n> \n> /*\n> * Our desired flags are:\n> *\n> * 1. Anything the master hasn't explicitly set, we can take from\n> * the slave.\n> * 2. Anything the slave didn't explicitly, we can take whether or\n> * not the master set it explicitly.\n> * 3. Anything the master explicitly set, we take.\n> */\n> master->flags =\n> /* (1) */ (~master->flags & slave->flags & slave->mask) |\n> /* (2) */ (master->flags & ~slave->mask) |\n> /* (3) */ (master->flags & master->mask);\n\nYour version is much better.\n\n> > @@ -2326,14 +2369,15 @@ 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\tDIFF_OPT_CLR(options, DIRSTAT_CUMULATIVE);\n> \n> Hmm. I haven't gotten to any changes to DIFF_OPT_{SET,CLR} yet. But it is a \n> little worrisome that this patch is so invasive as to require a change like \n> this.\n\nMy proposal is to make DIFF_OPT_{SET,CLR} more meaningful. Instead of just \nflipping a bit, they mean \"{un}set this bit and honor my intention in the \nfuture\".\n\n> I wouldn't be surprised to find other spots outside of diff.c where the \n> options are munged by various programs. Did you audit for all such spots?\n\nYes I believe I did. The only danger zones are spots that use struct \ndiff_options member \"flags\" or \"xdl_opts\" along with assignment and bitwise \noperators. The following recursive grep call (output included) satisfies me \nthat no such spots exist after my patch.\n\ngrep -EHnr '--include=*.[ch]' '(flags|xdl_opts).*=.*(DIFF_OPT_|\\b)(RECURSIVE|TREE_IN_RECURSIVE|BINARY|TEXT|FULL_INDEX|SILENT_ON_REMOVE|FIND_COPIES_HARDER|FOLLOW_RENAMES|COLOR_DIFF|COLOR_DIFF_WORDS|HAS_CHANGES|QUIET|NO_INDEX|ALLOW_EXTERNAL|EXIT_WITH_STATUS|REVERSE_DIFF|CHECK_FAILED|RELATIVE_NAME|IGNORE_SUBMODULES|DIRSTAT_CUMULATIVE|DIRSTAT_BY_FILE|ALLOW_TEXTCONV|PRIMER|XDF_NEED_MINIMAL|XDF_IGNORE_WHITESPACE|XDF_IGNORE_WHITESPACE_CHANGE|XDF_IGNORE_WHITESPACE_AT_EOL|XDF_PATIENCE_DIFF|XDF_WHITESPACE_FLAGS)\\b' . | grep -Ev TST | less -S\n\nmerge-tree.c:109: xpp.flags = XDF_NEED_MINIMAL;\nmerge-file.c:65: xpp.flags = XDF_NEED_MINIMAL;\nxdiff/xpatience.c:308: xpp.flags = map->xpp->flags & ~XDF_PATIENCE_DIFF;\nbuiltin-blame.c:41:static int xdl_opts = XDF_NEED_MINIMAL;\ncombine-diff.c:217: xpp.flags = XDF_NEED_MINIMAL;\nbuiltin-rerere.c:101: xpp.flags = XDF_NEED_MINIMAL;\ndiff.c:517: xpp.flags = XDF_NEED_MINIMAL;\ndiff.c:1570: xpp.flags = XDF_NEED_MINIMAL | o->xdl_opts;\ndiff.c:1658: xpp.flags = XDF_NEED_MINIMAL | o->xdl_opts;\ndiff.c:1706: xpp.flags = XDF_NEED_MINIMAL;\ndiff.c:3238: xpp.flags = XDF_NEED_MINIMAL;\n\n> > +\tif (DIFF_OPT_TST(options, PRIMER)) {\n> > +\t\tif (! primer) {\n> > +\t\t\tdiff_setup(primer = (struct diff_options *) malloc(sizeof(struct diff_options)));\n> \n> First, don't use malloc. Use the xmalloc wrapper that will try to free pack \n> memory and/or die if it fails.\n\nOK.\n\n> Secondly, don't cast the result of malloc. At best it is pointless and \n> verbose, and at worst it can hide errors caused by a missing function \n> declaration.\n\nOK.\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> \n> It often makes the patch easier to review if you split changes like this\n> out into a separate patch. Then your series is\n> \n> 1/2: use DIFF_XDL_SET instead of raw bit-masking\n> \n> This is a cleanup in preparation for option-setting doing\n> something more complex than just setting a bit-mask. The code\n> should behave exactly the same.\n> \n> 2/2: primer patch\n> \n> ... DIFF_XDL_SET tracks not only the set options, but which ones\n> were set explicitly via a mask ...\n> \n> Then we can all see pretty easily that patch 1/2 doesn't change the behavior, \n> and each patch is a much smaller, succint chunk to review.\n\nAgree.\n\n> Ditto here with DIFF_OPT_SET.\n\nOK.\n\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_OPT_DRT(opts, flag) ((opts)->mask & DIFF_OPT_##flag)\n> \n> OK, I see what it is supposed to do, but what does DRT stand for? Also,\n> what practical use does it have? I don't see anybody _calling_ it.\n\nDRT means \"dirty\". The masks here are dirty bits. You're right, I never called \nit. But it seemed so meaningful to me, I felt it was right to include it while \nintroducing the concept of dirty bits.\n\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 --primer --no-color -r -s --stdin $gdtargs]\n> \n> Does gitk really want to respect --primer? Might it not make more sense for \n> it, as a porcelain, to respect the diff.primer variable itself, and prepend it \n> to the list of diff args? Then it has the power to veto any options which it \n> doesn't handle.\n\nThe point of --primer was for scripts like gitk and git-gui. I wouldn't say \n\"respect --primer\" so much as \"pass --primer\" (which causes diff-tree to respect \ndiff.primer, which it normally wouldn't). Reviewing the options that \ndiff.primer currently supports, except for --color which I specifically guarded \nagainst, I don't see a scenario that would break gitk as is.\n\n> Also, any gitk changes should almost certainly be split into a different\n> patch.\n\nOK.\n\n> All in all, this was a lot more complicated than I was expecting. Why isn't \n> the behavior of \"diff.primer\" simply \"pretend as if the options in diff.primer \n> were prepended to the command line\"? That is easy to explain, and easy to \n> implement (the only trick is that you have to do an extra pass to find \n> --[no-]primer). Is there some drawback to such a simple scheme that I am \n> missing?\n\nI think we could call that simple scheme the linear walk, and it was my first \nimpulse as well. We walk through all the places where diff options live, in \norder from lowest precedence to highest precedence, accumulating/overriding \noptions as we go along. When we're done, we know the user's intention.\n\nBut as you say, we need that extra pass. Once I noticed we need that extra \npass, the software engineer in me saw the concept of linear walk as too weak for \nthis feature. These options occur in precedence layers and we need to combine \nthose layers the right way. The right way means: honor the intuitive semantics \nthe user expects. Hence the function name flatten_diff_options().\n\nA multi-pass walk is one possible implementation of such semantics, but in my \nopinion not the best. My implementation is much more explicit, meaningful, and \ndeclarative. It's not at all hard to understand looking at the code. In fact, \nit positively declares itself to you and annouces exactly what it is doing. It \n\"underlines the intent\" and also \"eases further extensions\" in Samuel Tardieu's \nwords:\nhttp://article.gmane.org/gmane.comp.version-control.git/105654\n\nThat's a desirability I believe you agree with :)\nhttp://article.gmane.org/gmane.comp.version-control.git/105657\n\nI think introducing explicit dirty masks and explicit layer flattening is the \nright way to go forward, but I agree it would be better to split this up into \nsmaller patches. My design would require a lot more lines of code if we wanted \nto support more diff options than are represented by struct diff_options.flags \nand .xdl_opts. That would mean introducing more masks and more CPP macros. \nBut I have faith in declarative programming. 100 lines of clear-as-day \nmeaningful code don't scare me nearly as much as 10 lines of secret obfuscation. \nAlso, I'm not convinced it is necessary for diff.primer to support all diff \noptions under the sun. Thoughts?\n\n -- Keith\n"},{"id":"103012","messageId":"m3d4dzwdr3.fsf@localhost.localdomain","threadId":"17509","inReplyTo":"1233598855-1088-2-git-send-email-keith@cs.ucla.edu","subject":"Re: [PATCH v2 1/2] Introduce config variable \"diff.primer\"","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-02-03T18:56:47Z","receivedAt":"2009-02-03T18:56:47Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Keith Cascio writes:\n\n> Introduce config variable \"diff.primer\".\n\nI still think that naming this configuration variable (or\nconfiguration section in the reverse primer.diff) 'primer' is not a\ngood idea, because it is quite obscure and not well known word. In\ncomputer related context I have seen it only when talking about\nintroductory / novice / basic level documentation ('primer (textbook)'\nmeaning). Git user, who might be not a native English speaker,\nshouldn't have to look up in dictionary what it is about...\n\nI think that 'defaults' or 'options' would be a much better name.\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"},{"id":"103016","messageId":"alpine.GSO.2.00.0902031108531.5994@kiwi.cs.ucla.edu","threadId":"17509","inReplyTo":"m3d4dzwdr3.fsf@localhost.localdomain","subject":"Re: [PATCH v2 1/2] Introduce config variable \"diff.primer\"","fromName":"Keith Cascio","fromEmail":"keith@cs.ucla.edu","sentAt":"2009-02-03T19:13:12Z","receivedAt":"2009-02-03T19:13:12Z","isPatch":true,"sender":{"key":"keith@cs.ucla.edu","avatar":"https://gravatar.com/avatar/c5ec3a8f1cd1f449fdf8bdb7125fdbfd10b729507f32cbf0aa4ad07b4f7127ae?d=mp&s=160"},"body":"On Tue, 3 Feb 2009, Jakub Narebski wrote:\n\n> I still think that naming this configuration variable (or configuration \n> section in the reverse primer.diff) 'primer' is not a good idea, because it is \n> quite obscure and not well known word. In computer related context I have \n> seen it only when talking about introductory / novice / basic level \n> documentation ('primer (textbook)' meaning). Git user, who might be not a \n> native English speaker, shouldn't have to look up in dictionary what it is \n> about...\n> \n> I think that 'defaults' or 'options' would be a much better name.\n\nPoint well taken. Let's consider \"primer\" the working name. The final name \nwill be the last finishing touch once (and if) we achieve consensus on the \nfunctionality. Ultimately the name is best chosen after the functionality is \ncertain. IOW, open to discussion.\n\n -- Keith\n"},{"id":"103069","messageId":"7v7i4692p4.fsf@gitster.siamese.dyndns.org","threadId":"17509","inReplyTo":"alpine.GSO.2.00.0902030833250.5994@kiwi.cs.ucla.edu","subject":"Re: [PATCH v2 1/2] Introduce config variable \"diff.primer\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-02-04T05:43:51Z","receivedAt":"2009-02-04T05:43:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Keith Cascio writes:\n\n> I think introducing explicit dirty masks and explicit layer flattening is the \n> right way to go forward,...\n\nI am not so sure about this claim. I kind of like idea of \"we know the\ndesirable value of this bit, do not touch it any further\" mask, but I\nthink it is independent of flattening. In fact, I do not think flattening\nis a good thing.\n\nAny codepath could call DIFF_OPT_SET()/CLR(), whether it is in response to\nend user's input from the command line (e.g. \"the user said --foo, so I am\nflipping the foo bit on/off), or to enforce restriction to achieve sane\nsemantics (e.g. \"it does not make any sense to run this internal diff\nwithout --binary set, so I am using OPT_SET()\"). Is it still true, or do\nsome bit flipping now need to be protected by \"is it locked\" check and\nsome others don't?\n\nThe latter one (i.e. in the above example, \"this internal diff must run\nwith --binary\") we may want to use \"opts->mask\" to lock down (I wouldn't\ncall it \"dirty\", it is more like \"locked\") the \"binary\" bit, and we may\neven want to issue a warning or an error when the end user attempts to\ncountermand with --no-binary. Similarly, I think you would want to lock\ndown what you got from the true command line so that you can leave them\nuntouched when you process the value you read from diff.primer.\n\nDoesn't it suggest that you may want two layers of masks, not a flat one,\nif you really want the mechanism to scale?\n\nIn any case, I think the mechanism based on the lock-down mask is worth\nconsidering when we enhance the option parsing mechanism for the diff and\nlog family, and if it is done right, I think the code to parse revision\noptions would benefit quite a bit. There are codepaths that initialize\nthe bits to the command's liking (e.g. show wants to always have diff),\nlet setup_revisions() to process command line flags, and then further\ntweak the remaining bits (e.g. whatchanged wants to default to raw), all\ninteracting in quite subtle ways.\n\nBut that should probably be for later cycle, post 1.6.2.\n"},{"id":"103071","messageId":"alpine.GSO.2.00.0902032217380.25760@kiwi.cs.ucla.edu","threadId":"17509","inReplyTo":"7v7i4692p4.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH v2 1/2] Introduce config variable \"diff.primer\"","fromName":"Keith Cascio","fromEmail":"keith@cs.ucla.edu","sentAt":"2009-02-04T06:36:48Z","receivedAt":"2009-02-04T06:36:48Z","isPatch":true,"sender":{"key":"keith@cs.ucla.edu","avatar":"https://gravatar.com/avatar/c5ec3a8f1cd1f449fdf8bdb7125fdbfd10b729507f32cbf0aa4ad07b4f7127ae?d=mp&s=160"},"body":"On Tue, 3 Feb 2009, Junio C Hamano wrote:\n\n> Any codepath could call DIFF_OPT_SET()/CLR(), whether it is in response to end \n> user's input from the command line (e.g. \"the user said --foo, so I am \n> flipping the foo bit on/off), or to enforce restriction to achieve sane \n> semantics (e.g. \"it does not make any sense to run this internal diff without \n> --binary set, so I am using OPT_SET()\").\n\nYes but the trick is the flips and maskings happen on different structs. We \naccumulate the shell command line flags/masks in a separate struct from the \nprimer flags/masks. IOW, there's a whole lotta flags and masks!!\n\n> Doesn't it suggest that you may want two layers of masks, not a flat one, if \n> you really want the mechanism to scale?\n\nThere are indeed two layers of masks (and there can be as many as needed). In \nmy current patch, the shell command line becomes \"master\" and primer becomes \n\"slave\". Both layers exist independently of each other, in two separate \ndiff_option structs, until just before \"go time\", when I flatten them (but that \ndoes not destroy the slave, it is reused). As Peff put it: \"a master/slave pair \nof flag/mask pairs\". I specifically designed the code to make it easy to create \nan arbitrary number of layers, then flatten them all together just before it's \ntime to do something. The code only needs to keep track of the order of \nprecedence, i.e. always pass the higher precedence struct to \nflatten_diff_options() as master and the lower precedence struct as slave, and \nthe bit logic in that function does the rest. I was specifically thinking of \nGIMP or Photoshop when I wrote this patch. The concept is the same. Those \nprograms support an arbitrary number of layers, and when they produce the final \nimage, they call it \"flatten layers\".\n\nIf you and Peff like this design then I could clean up everything based on all \nof Peff's suggestions (i.e. xmalloc instead of malloc, etc) and hopefully move \non to the stage of building consensus for the actual name. No rush of course. \nJust give me the word. Peff?\n\n -- Keith\n"},{"id":"103528","messageId":"20090206161954.GA18956@coredump.intra.peff.net","threadId":"17509","inReplyTo":"alpine.GSO.2.00.0902030833250.5994@kiwi.cs.ucla.edu","subject":"Re: [PATCH v2 1/2] Introduce config variable \"diff.primer\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-02-06T16:19:54Z","receivedAt":"2009-02-06T16:19:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 03, 2009 at 09:55:08AM -0800, Keith Cascio wrote:\n\n> First of all, thanks for the tips!\n\nOf course. Thank you for spending time to refine your patch.\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. `\"--patience --color --ignore-space-at-eol --exit-code\"`.\n> > > +\tSee linkgit:git-diff[1]. You can suppress these at run time with\n> > > +\toption `--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 --patience -q\n> > > +--quiet -R -r --relative -t --text --textconv -w`\n> > \n> > Funny indentation?\n> \n> The last part is a backtick-quoted list, I think it either must occur on one \n> line or as is.\n\nAh, yes, I see. I wonder if that is a sign that we can use some kind of\nlist construct instead. I also wonder if we need the exact list. It\nseems like a poor user interface to have to enumerate each option. As a\nuser, should I be saying \"I would like to put --foobar into my primer\nvariable. Let me check whether it is supported\"? Or should there be some\nsane and succint rule by which to say \"I know whether --foobar is\nsupported or not, because it falls into categroy X\"?\n\nI think the latter is much nicer to users. And then the help text\ndescribes that rule (and I think the rule should be \"everything that\ntweaks what diffs _look_ like is included\" or something like that).\n\nBut more on that:\n\n> > This seems really clunky to list all of the options here. I thought\n> > the point was to respect _all_ of them, but do it from porcelain so\n> > that it is up to the user what they want to put in.\n> > \n> > How was this list chosen?\n> \n> The current version does not try to support all diff options. It only\n> supports those that are recorded in struct diff_options.flags and\n> .xdl_opts - that is the present list.\n\nThat is a very unsatisfying list, as it bears no relation to what users\nactually want to put in diff.primer, or what even makes sense there. For\nexample:\n\n - \"-u\" is not supported, but that is something I would expect people\n to want to use (in fact, it is the _only_ thing supported by\n GIT_DIFF_OPTS)\n\n - \"--follow\" is supported, but it makes no sense to say \"my diffs\n should always use --follow\" (and yes, this is a result of it\n confusingly being part of diff opts, when it is really about\n revision processing).\n\n - \"--exit-code\" and \"--quiet\" are supported, but why would you want to\n have them on all the time as defaults?\n\n - \"--relative\" is supported, but \"--relative=\" is not.\n\nSo I think the behavior is quite confusing for potential users. I don't\nmind as much that some options don't make sense (like --exit-code and\n--follow), because people who put them in diff.primer get what they\ndeserve. But not supporting some options that people do want to use is\ngoing to look like a bug.\n\n> > This doesn't appear to have any quoting mechanism. Is it impossible\n> > to have an option with spaces (e.g., --relative='foo bar')? I guess\n> > that is probably uncommon, but I would expect normal shell quoting\n> > rules to apply.\n> \n> The current version only supports the flags listed above in\n> Documentation/config.txt.\n\nRight, but you can see that I think that should change. :) I think there\nare some quote-parsing routines that we use for config and for aliases\nthat may work for re-use, but I haven't checked.\n\n> > /*\n> > * Our desired flags are:\n> > *\n> > * 1. Anything the master hasn't explicitly set, we can take from\n> > * the slave.\n> > * 2. Anything the slave didn't explicitly, we can take whether or\n> > * not the master set it explicitly.\n> > * 3. Anything the master explicitly set, we take.\n> > */\n> > master->flags =\n> > /* (1) */ (~master->flags & slave->flags & slave->mask) |\n> > /* (2) */ (master->flags & ~slave->mask) |\n> > /* (3) */ (master->flags & master->mask);\n> \n> Your version is much better.\n\nExcept I think in (1) it should be \"~master->mask\". Oops. :)\n\n> The point of --primer was for scripts like gitk and git-gui. I\n\nRight, I am calling into question whether we want \"--primer\" at all.\nThat is, if you think of it as just \"prepend these command line options\"\nwe can get the same thing with something like:\n\n git diff-tree `git config diff.primer` $other_options\n\nif the caller wants to be totally promiscuous, and\n\n git diff-tree `git config diff.primer | filter_options` $other_options\n\nif it wants to be paranoid (and obviously in tcl the code would be\ndifferent, but I think you can see the point).\n\n> wouldn't say \"respect --primer\" so much as \"pass --primer\" (which\n> causes diff-tree to respect diff.primer, which it normally wouldn't).\n> Reviewing the options that diff.primer currently supports, except for\n> --color which I specifically guarded against, I don't see a scenario\n> that would break gitk as is.\n\nWell, \"--quiet\" and \"--exit-code\" certainly would produce confusing\nresults. Though I can't imagine anyone putting those into their\ndiff.primer variable.\n\n> I think we could call that simple scheme the linear walk, and it was\n> my first impulse as well. We walk through all the places where diff\n> options live, in order from lowest precedence to highest precedence,\n> accumulating/overriding options as we go along. When we're done, we\n> know the user's intention.\n> \n> But as you say, we need that extra pass. Once I noticed we need that\n> extra pass, the software engineer in me saw the concept of linear walk\n> as too weak for this feature. These options occur in precedence\n> layers and we need to combine those layers the right way. The right\n> way means: honor the intuitive semantics the user expects. Hence the\n> function name flatten_diff_options().\n\nHrm. Do we really need the multiple passes, though? Let's assume for a\nminute that we don't want \"--primer\". The rule is: \"if you are a\nporcelain, you respect diff.primer; if you're not, you don't\". And\nbecause diff.primer is, by definition, a set of command line options, it\nis trivial for a caller who wants to selectively apply them to a\nplumbing to do so. Similarly, if somebody really wants to call a\nporcelain and _disable_ options, I don't think \"--no-primer\" is\nnecessarily the right interface. Instead, the actual command line\noptions given override what's in diff.primer, so you can selectively\ndisable whatever you like.\n\nAnd of course all of this extends naturally to a \"primer.*\" set of\nconfig, rather than just \"diff.primer\".\n\n> A multi-pass walk is one possible implementation of such semantics,\n> but in my opinion not the best. My implementation is much more\n> explicit, meaningful, and declarative. It's not at all hard to\n> understand looking at the code. In fact, it positively declares\n\nI do think declarative interfaces can be nice, but I really have two\ncomplaints with your approach:\n\n 1. It is somewhat fragile, because it is easy to bypass the\n declarative nature in C. That is, there is an ordering constraint\n with the flattening that we may not be fulfilling everywhere in\n git. For example, let's say some code checks option A and then does\n something with option B as a result. If this code runs before the\n flattening, then it may fail to correctly pick up option A, since\n it is in the slave spot. But if it runs after, the flattening may\n logic may miss the option setting.\n\n I think the \"after\" case above is not a problem in practice,\n because we are only flattening two cases, and post-flattening we\n always just operate directly on the master case. So I think we\n really want to flatten at the last minute. So rather than\n flattening everything and storing it, I think a nicer approach is\n to put it in the accessor:\n\n /* If master set it, take that. If not, and slave set it,\n take that. If nobody set it, take the master as default. */\n #define DIFF_OPT_TST(opts, flag) \\\n ((opts)->master_mask & DIFF_OPT_##flag ? \\\n (opts)->master_flag & DIFF_OPT_##flag : \\\n (opts)->slave_mask & DIFF_OPT_##flag ? \\\n (opts)->slave_flag & DIFF_OPT_##flag : \\\n (opts)->master_flag & DIFF_OPT_##flag;\n\n 2. The current flattening code deals only with the flags field, which\n is fairly easy to handle using a mask and some macros. But what\n about fields that have a more complex implementation? I think the\n amount of work to do flattening is going to be linear with the\n number of options. Which means that now there is new work to be\n done every time a new non-flag option is added, and a chance for\n the developer to forget to do that work. Which hurts\n maintainability in the long run.\n\nSo yes, I think a declarative solution can be nice, but there is really\nvery little language support in C for doing it in a safe way. I think\nyou can get by on the flags with some macros, but I don't think there is\nreally a nice general solution for all of the options that won't make\nper-option work. If you really think it is possible, I invite you to try\nto make a nice set of macros that cover all cases.\n\n-Peff\n\nPS I am behind on git mail and catching up slowly; I see there are some\nother messages in this thread, but I don't have time to read them right\nat this second. So don't think I am ignoring issues raised there -- I\njust haven't gotten to them yet.\n"},{"id":"103530","messageId":"20090206165453.GA19026@coredump.intra.peff.net","threadId":"17509","inReplyTo":"alpine.GSO.2.00.0902032217380.25760@kiwi.cs.ucla.edu","subject":"Re: [PATCH v2 1/2] Introduce config variable \"diff.primer\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-02-06T16:54:53Z","receivedAt":"2009-02-06T16:54:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 03, 2009 at 10:36:48PM -0800, Keith Cascio wrote:\n\n> There are indeed two layers of masks (and there can be as many as\n> needed). In my current patch, the shell command line becomes \"master\"\n> and primer becomes \"slave\". Both layers exist independently of each\n> other, in two separate diff_option structs, until just before \"go\n> time\", when I flatten them (but that does not destroy the slave, it is\n> reused).\n\nWe have had trouble in the past figuring out exactly when \"go time\" is\n(not specifically for diff options, but for things like color config).\nThat is, there is code which wants to munge the options based on some\nother input much later than the setting of most options, and so any\nfinalizing work you do ends up happening too early. And maybe you can\nargue that such code is wrong or bad, but it does get written and it\ndoes cause problems.\n\nSo as I mentioned in another mail I just sent, I think you are best to\nstop thinking of it as a general flattening, and think of it more as a\nset of accessors that do the flattening Just In Time.\n\n-Peff\n"},{"id":"103654","messageId":"7vocxdudj0.fsf@gitster.siamese.dyndns.org","threadId":"17509","inReplyTo":"20090206161954.GA18956@coredump.intra.peff.net","subject":"Re: [PATCH v2 1/2] Introduce config variable \"diff.primer\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-02-07T21:45:39Z","receivedAt":"2009-02-07T21:45:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Right, I am calling into question whether we want \"--primer\" at all.\n> That is, if you think of it as just \"prepend these command line options\"\n> we can get the same thing with something like:\n>\n> git diff-tree `git config diff.primer` $other_options\n>\n> if the caller wants to be totally promiscuous, and\n>\n> git diff-tree `git config diff.primer | filter_options` $other_options\n>\n> if it wants to be paranoid (and obviously in tcl the code would be\n> different, but I think you can see the point).\n\nI agree with this 100%.\n\nAlso, I think we should explain the semantics of diff.primer to new people\nlike that. \"The diff Porcelain command acts as if whatever you have in\ndiff.primer are prepended on your command line\".\n\nWhich means that we do not have to touch the plumbing at all. That allows\nme sleep much better at night.\n"},{"id":"103863","messageId":"alpine.GSO.2.00.0902090921270.719@kiwi.cs.ucla.edu","threadId":"17509","inReplyTo":"20090206161954.GA18956@coredump.intra.peff.net","subject":"Re: [PATCH v2 1/2] Introduce config variable \"diff.primer\"","fromName":"Keith Cascio","fromEmail":"keith@cs.ucla.edu","sentAt":"2009-02-09T17:24:37Z","receivedAt":"2009-02-09T17:24:37Z","isPatch":true,"sender":{"key":"keith@cs.ucla.edu","avatar":"https://gravatar.com/avatar/c5ec3a8f1cd1f449fdf8bdb7125fdbfd10b729507f32cbf0aa4ad07b4f7127ae?d=mp&s=160"},"body":"On Fri, 6 Feb 2009, Jeff King wrote:\n\n> if somebody really wants to call a porcelain and _disable_ options, I don't \n> think \"--no-primer\" is necessarily the right interface. Instead, the actual \n> command line options given override what's in diff.primer, so you can \n> selectively disable whatever you like.\n\nSir I appreciate the intention, as I interpret it, that it's always better to \naccomplish something without adding new vocabulary. I'd much rather avoid \nadding new vocab if possible. If I'm missing something, I apologize ahead of \ntime, but let me describe the problem I see. Let's take the context size \nsetting as an example, i.e. -U or --unified=. Default is 3. Let's say \nsomeone defines diff.primer = -U6. Now, without --no-primer, how does a program \nsay \"use the default value for context.\" Aren't there options for which no \ninverse counterpart exists? Is there command-line syntax to disable all \nwhitespace ignore options, e.g. to disable -b? If not then we need --no-primer.\n"},{"id":"104547","messageId":"20090213222233.GA7424@coredump.intra.peff.net","threadId":"17509","inReplyTo":"alpine.GSO.2.00.0902090921270.719@kiwi.cs.ucla.edu","subject":"Re: [PATCH v2 1/2] Introduce config variable \"diff.primer\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-02-13T22:22:33Z","receivedAt":"2009-02-13T22:22:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 09, 2009 at 09:24:37AM -0800, Keith Cascio wrote:\n\n> adding new vocab if possible. If I'm missing something, I apologize\n> ahead of time, but let me describe the problem I see. Let's take the\n> context size setting as an example, i.e. -U or --unified=.\n> Default is 3. Let's say someone defines diff.primer = -U6. Now,\n> without --no-primer, how does a program say \"use the default value for\n> context.\" Aren't there options for which no inverse counterpart\n> exists? Is there command-line syntax to disable all whitespace ignore\n> options, e.g. to disable -b? If not then we need --no-primer.\n\nGood point. I think there are actually two separate problems you\ndescribe:\n\n 1. some default behaviors which can be changed via an option have no\n option that represents the default. This is your \"-b\" example (and\n there are others, like \"-a\", \"--full-index\", etc).\n\n Generally I think we try to allow boolean options to be specified\n in either positive or negative ways. Our parse-options library even\n automatically handles --no-$foo by default for any boolean option\n (as long as it has a corresponding long option).\n\n So I consider the lack of --no-ignore-space-change to be a failing\n of git, but one that we can correct. Either manually or by moving\n the diff code to parse-options.\n\n 2. For options which take a value, there is no way to say \"pretend I\n didn't specify a value at all\".\n\n Actually, that is not entirely true. parse-options handles\n \"--foo=bar --no-foo\" as if \"--foo\" was never specified at all.\n\n But there are still two failure cases:\n\n - as above, the diff options are not handled by parse-options\n\n - not all value options are quite as straightforward. Some\n options run callbacks that do things which take specialized\n code to undo.\n\n Both are fixable. But I have to wonder if it is really all that\n useful to say \"use the default for this option\". Either you don't\n care what the value is, in which case you can take the default\n given by your primer value. Or you do care, in which case wouldn't\n you want to be setting the value yourself?\n\nSo I think doing it right is a bit more work in the long run, but the\nextra work is generally improving git.\n\nAll that being said, though, I still think we can do the equivalent of\n--no-primer. The trick to avoiding multiple passes is for the option to\nexist outside of the set of primer'd options. I can think of two places:\n\n 1. an environment variable, GIT_PRIMER. E.g., \"GIT_PRIMER=0 git diff\".\n This strikes me as hack-ish and unfriendly, but would work.\n\n 2. in the git option list, but not the git command option list. IOW, we\n actually have two sets of options in any command:\n\n git [options] diff [options]\n\n So you could suppress primers (all of them, including diff.primer\n or primer.*) via:\n\n git --no-primer diff\n\nMake sense?\n\n-Peff\n"},{"id":"104594","messageId":"alpine.DEB.1.00.0902140658050.10279@pacific.mpi-cbg.de","threadId":"17509","inReplyTo":"20090213222233.GA7424@coredump.intra.peff.net","subject":"Re: [PATCH v2 1/2] Introduce config variable \"diff.primer\"","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-02-14T06:03:26Z","receivedAt":"2009-02-14T06:03:26Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 13 Feb 2009, Jeff King wrote:\n\n> 1. an environment variable, GIT_PRIMER. E.g., \"GIT_PRIMER=0 git diff\".\n> This strikes me as hack-ish and unfriendly, but would work.\n\nFWIW I cannot stop to hate the term \"primer\" for this thing.\n\nA \"primer\" has been explained to me as being a short leaflet that \nintroduces you to the basic principles of something. Or as a sequence of \nRNA (or in rare cases, not RNA but something similar) that starts \nreplication of DNA.\n\nIOW a primer is something that _has_ to come before something else. \nWithout which the latter thing does not work.\n\nNot a set of defaults, which this here patch is all about.\n\nCiao,\nDscho\n"},{"id":"104595","messageId":"20090214061538.GB3223@sigill.intra.peff.net","threadId":"17509","inReplyTo":"alpine.DEB.1.00.0902140658050.10279@pacific.mpi-cbg.de","subject":"Re: [PATCH v2 1/2] Introduce config variable \"diff.primer\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-02-14T06:15:38Z","receivedAt":"2009-02-14T06:15:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Feb 14, 2009 at 07:03:26AM +0100, Johannes Schindelin wrote:\n\n> On Fri, 13 Feb 2009, Jeff King wrote:\n> \n> > 1. an environment variable, GIT_PRIMER. E.g., \"GIT_PRIMER=0 git diff\".\n> > This strikes me as hack-ish and unfriendly, but would work.\n> \n> FWIW I cannot stop to hate the term \"primer\" for this thing.\n\nYes, I also hate the name. I have been using it in the discussion for\nlack of a better term, but I really dislike it, as well. I think\n\"diff.defaults\" makes sense, but maybe something that suggests it is\nabout command line options would be better.\n\n-Peff\n"},{"id":"104597","messageId":"alpine.DEB.1.00.0902140724360.10279@pacific.mpi-cbg.de","threadId":"17509","inReplyTo":"20090214061538.GB3223@sigill.intra.peff.net","subject":"Re: [PATCH v2 1/2] Introduce config variable \"diff.primer\"","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-02-14T06:24:54Z","receivedAt":"2009-02-14T06:24:54Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 14 Feb 2009, Jeff King wrote:\n\n> On Sat, Feb 14, 2009 at 07:03:26AM +0100, Johannes Schindelin wrote:\n> \n> > On Fri, 13 Feb 2009, Jeff King wrote:\n> > \n> > > 1. an environment variable, GIT_PRIMER. E.g., \"GIT_PRIMER=0 git diff\".\n> > > This strikes me as hack-ish and unfriendly, but would work.\n> > \n> > FWIW I cannot stop to hate the term \"primer\" for this thing.\n> \n> Yes, I also hate the name. I have been using it in the discussion for\n> lack of a better term, but I really dislike it, as well. I think\n> \"diff.defaults\" makes sense, but maybe something that suggests it is\n> about command line options would be better.\n\nWhy not spell it out? diff.defaultOptions\n\nCiao,\nDscho\n"},{"id":"104640","messageId":"20090214151759.GC3887@sigill.intra.peff.net","threadId":"17509","inReplyTo":"alpine.DEB.1.00.0902140724360.10279@pacific.mpi-cbg.de","subject":"Re: [PATCH v2 1/2] Introduce config variable \"diff.primer\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-02-14T15:17:59Z","receivedAt":"2009-02-14T15:17:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Feb 14, 2009 at 07:24:54AM +0100, Johannes Schindelin wrote:\n\n> > Yes, I also hate the name. I have been using it in the discussion for\n> > lack of a better term, but I really dislike it, as well. I think\n> > \"diff.defaults\" makes sense, but maybe something that suggests it is\n> > about command line options would be better.\n> \n> Why not spell it out? diff.defaultOptions\n\nYes, that is much better than my suggestion.\n\n-Peff\n"},{"id":"104852","messageId":"alpine.GSO.2.00.0902151521550.24570@kiwi.cs.ucla.edu","threadId":"17509","inReplyTo":"alpine.DEB.1.00.0902140724360.10279@pacific.mpi-cbg.de","subject":"Re: [PATCH v2 1/2] Introduce config variable \"diff.primer\"","fromName":"Keith Cascio","fromEmail":"keith@cs.ucla.edu","sentAt":"2009-02-15T23:26:54Z","receivedAt":"2009-02-15T23:26:54Z","isPatch":true,"sender":{"key":"keith@cs.ucla.edu","avatar":"https://gravatar.com/avatar/c5ec3a8f1cd1f449fdf8bdb7125fdbfd10b729507f32cbf0aa4ad07b4f7127ae?d=mp&s=160"},"body":"On Sat, 14 Feb 2009, Johannes Schindelin wrote:\n\n> Why not spell it out? diff.defaultOptions\n\nOK in v3 I'll use \"defaultOptions\". Now what do we call the switches?\n"},{"id":"104858","messageId":"7vtz6vgthi.fsf@gitster.siamese.dyndns.org","threadId":"17509","inReplyTo":"alpine.GSO.2.00.0902151521550.24570@kiwi.cs.ucla.edu","subject":"Re: [PATCH v2 1/2] Introduce config variable \"diff.primer\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-02-15T23:39:37Z","receivedAt":"2009-02-15T23:39:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Keith Cascio writes:\n\n> On Sat, 14 Feb 2009, Johannes Schindelin wrote:\n>\n>> Why not spell it out? diff.defaultOptions\n>\n> OK in v3 I'll use \"defaultOptions\". Now what do we call the switches?\n\nignore-default?\n"},{"id":"105085","messageId":"alpine.GSO.2.00.0902162312030.17111@kiwi.cs.ucla.edu","threadId":"17509","inReplyTo":"20090213222233.GA7424@coredump.intra.peff.net","subject":"Re: diff.defaultOptions implementation design [was diff.primer]","fromName":"Keith Cascio","fromEmail":"keith@cs.ucla.edu","sentAt":"2009-02-17T07:24:33Z","receivedAt":"2009-02-17T07:24:33Z","isPatch":false,"sender":{"key":"keith@cs.ucla.edu","avatar":"https://gravatar.com/avatar/c5ec3a8f1cd1f449fdf8bdb7125fdbfd10b729507f32cbf0aa4ad07b4f7127ae?d=mp&s=160"},"body":"Peff,\n\nOn Fri, 13 Feb 2009, Jeff King wrote:\n\n> So I think doing it right is a bit more work in the long run, but the extra \n> work is generally improving git.\n> \n> All that being said, though, I still think we can do the equivalent of \n> --no-primer. The trick to avoiding multiple passes is for the option to exist \n> outside of the set of primer'd options.\n\nI like the idea of using parse-options to handle diff options and I too would \nlike all switches negatable. I will come back to the other ideas you mention if \nnecessary. You laid it all out nicely.\n\nAssuming we can do away with the switches --[no-]default-options, thereby \nhopefully eliminating the need to accumulate options in any kind of fancy way, \ncertainly the right place to \"walk\" is in diff_setup(). But diff_setup() must \nstill ascertain at least one runtime fact: whether or not we are running one of \nthe commands that respects default options {diff, log, show}. Is there an \nelegant way to ascertain that fact from inside diff_setup()? How do you \nrecommend? (BTW I believe my design achieves this elegantly).\n\n -- Keith\n"},{"id":"105212","messageId":"20090217195658.GC16067@coredump.intra.peff.net","threadId":"17509","inReplyTo":"alpine.GSO.2.00.0902162312030.17111@kiwi.cs.ucla.edu","subject":"Re: diff.defaultOptions implementation design [was diff.primer]","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-02-17T19:56:58Z","receivedAt":"2009-02-17T19:56:58Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 16, 2009 at 11:24:33PM -0800, Keith Cascio wrote:\n\n> I like the idea of using parse-options to handle diff options and I\n> too would like all switches negatable. I will come back to the other\n> ideas you mention if necessary. You laid it all out nicely.\n\nIf you are interested in parse-optification of diff options, search the\narchive for messages from Pierre Habouzit on the topic in the last 6\nmonths or so. It was discussed at the GitTogether, and he had some\npreliminary patches.\n\n> diff_setup(). But diff_setup() must still ascertain at least one\n> runtime fact: whether or not we are running one of the commands that\n> respects default options {diff, log, show}. Is there an elegant way\n> to ascertain that fact from inside diff_setup()? How do you\n> recommend? (BTW I believe my design achieves this elegantly).\n\nYou can impact the argument parsing by touching the diffopt struct\nbefore doing the parsing. I.e., something like:\n\n /* we generally get diff options from a rev_info structure */\n struct rev_info rev;\n /* initialize the structures */\n init_revisions(&rev, prefix);\n /* now set any preferences specific to this command */\n DIFF_OPT_SET(&rev.diffopt, ALLOW_DEFAULT_CONFIG);\n /* and then actually parse */\n setup_revisions(argc, argv, rev, \"HEAD\");\n\nSee for example how cmd_whatchanged does it in builtin-log.c. Any\nporcelains which wanted this feature would opt in to it.\n\n-Peff\n"},{"id":"108243","messageId":"alpine.GSO.2.00.0903170825340.16242@kiwi.cs.ucla.edu","threadId":"17509","inReplyTo":"20090203071516.GC21367@sigill.intra.peff.net","subject":"Re: [PATCH v2 1/2] Introduce config variable \"diff.defaultOptions\"","fromName":"Keith Cascio","fromEmail":"keith@cs.ucla.edu","sentAt":"2009-03-17T16:05:18Z","receivedAt":"2009-03-17T16:05:18Z","isPatch":true,"sender":{"key":"keith@cs.ucla.edu","avatar":"https://gravatar.com/avatar/c5ec3a8f1cd1f449fdf8bdb7125fdbfd10b729507f32cbf0aa4ad07b4f7127ae?d=mp&s=160"},"body":"Peff,\n\nI'm replying to http://permalink.gmane.org/gmane.comp.version-control.git/108158\n\nOn Tue, 3 Feb 2009, Jeff King wrote:\n\n> All in all, this was a lot more complicated than I was expecting. Why isn't \n> the behavior of \"diff.primer\" simply \"pretend as if the options in diff.primer \n> were prepended to the command line\"? That is easy to explain, and easy to \n> implement (the only trick is that you have to do an extra pass to find \n> --[no-]primer). Is there some drawback to such a simple scheme that I am \n> missing?\n\nIn order to answer your questions as convincingly as possible, I wrote up a \none-page PDF document, downloadable here:\n http://preview.tinyurl.com/c769dd\n\nYou will see I clarified my arguments, and I found very compelling reasons for \nmy design. Also, BTW, v3 supports all diff options under the sun, instead of a \nlimited subset. That addresses your primary complaint WRT functionality. \nPlease take a look at the PDF and I hope you agree.\n\n -- Keith\n"},{"id":"108660","messageId":"20090320070148.GD27008@coredump.intra.peff.net","threadId":"17509","inReplyTo":"alpine.GSO.2.00.0903170825340.16242@kiwi.cs.ucla.edu","subject":"Re: [PATCH v2 1/2] Introduce config variable \"diff.defaultOptions\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-03-20T07:01:48Z","receivedAt":"2009-03-20T07:01:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 17, 2009 at 09:05:18AM -0700, Keith Cascio wrote:\n\n> In order to answer your questions as convincingly as possible, I wrote up a \n> one-page PDF document, downloadable here:\n> http://preview.tinyurl.com/c769dd\n\nThanks for following up on this. And I appreciate that it is probably\nnicer to compose in whatever you used to make this PDF due to enhanced\ntypography and linked footnotes, but posting a link to a PDF has a few\ndownsides:\n\n - the text in the PDF does not become part of the list archive; if\n your tinyurl or your hosted file ever goes away, the content of\n your message is permanently lost\n\n - it makes it very difficult for readers to reply inline to your\n comments\n\nSo please just send text emails in the future.\n\nThis time, however, I converted your PDF to text so I could reply.\n\nYour PDF said:\n\n> No matter which implementation, we all agree the semantics are \"pretend\n> as if the options in [diff.defaultOptions] were prepended to the command\n> line\" [1]. That requirement is subject neither to confusion nor doubt.\n> An implementation is acceptable only if it delivers exactly that\n> behavior to the user.\n\nOK, good, I think we are in agreement there.\n\n> Git's diff options processing scheme is a mature[2], versatile\n> instrument relied upon by at least 47 client codepaths. The API it\n> provides for recording user intent to an instance of struct diff_options\n> entails the following steps[3]:\n> \n> 1) to prepare the structure for recording, call diff_setup() once\n> \n> 2) to record user intention, write to the structure[4], calling\n> diff_opt_parse() as needed\n\nI think there is a step 1.5, where callers can record their own\n\"default\" intentions -- things that this particular caller will default\nto, but which users can override via command-line options. E.g., \"git\nlog\" tweaks several options in cmd_log_init, such as turning on\nALLOW_TEXTCONV and RECURSIVE.\n\n> 3) to prepare the structure for \"playback\", call diff_setup_done() once\n>\n> 4) to test user intention, read from the structure\n\nOK, makes sense.\n\n> Git's code is already equipped to react to every kind of user intention\n> during step 2.\n\nMore or less. I think in our past discussion, it came about that there\nare some things the user cannot say, like undoing certain options. This\ncould be a problem if a caller defaults options to something un-doable.\nFor example, I don't think there is a way to turn off OPT_RECURSIVE in\ngit-log. In practice, this hasn't been a problem.\n\n> An attractive (but hopelessly flawed) strategy is to pre-load the\n> structure with defaultOptions before step 2. Step 1, diff_setup(), is\n> the only place to do that, since the client starts overwriting as soon\n> as diff_setup() returns.\n\nI don't agree that it is hopelessly flawed. It requires a new call at\neach callsite to say \"I have set up my defaults, now take the user\ndefaults from the config, before I proceed to step 2 and parse the\nuser intentions\". Which sounds awful to add a new call at each site,\nbut I am not sure that is not necessary anyway.\n\nI don't know that all callsites will _want_ to respect such a config\noption, especially not plumbing. So any callsite is going to have to opt\ninto this functionality anyway.\n\n> Some aspects of user intention dictate whether or not to perform the\n> pre-load at all, most notably: which Git command the user invoked (also\n> switches). But at step 1, inside diff_setup(), we don't know the user's\n> full intention yet, so we can't decide whether or not to perform the\n> pre-load.\n\nI think I suggested last time that the idea of whether or not to perform\nthe pre-load doesn't _have_ to come from the same set of user intention.\nThat is, in the call\n\n git [git-options] diff [diff-options]\n\nwe actually parse [git-options] and [diff-options] at different times.\nPre-load intention can go into [git-options], which avoids this\ndependency cycle.\n\n> This could tempt an undisciplined programmer to try to peek ahead at\n> part of the user's intention before the code sees it in its normal\n> course. That would be a disaster because it would undermine the\n> integrity, not to mention beauty, of Git's entire initialization scheme.\n> It would be a cheap hack; an inelegant, fragile, ugly, and utterly\n> half-baked attempt to bypass and circumvent the existing architecture.\n\nYour poetry aside, I agree that way madness lies.\n\n> My proposal:\n> \n> a) patiently accumulates user intention via Git's well-established\n> initialization scheme, never needing to peek ahead or misbehave in any\n> way, thus attaining harmony.\n> \n> b) postpones the decision whether or not to load defaultOptions until\n> step 3, diff_setup_done(), after we've had every opportunity to examine\n> user intention, but loads them effectively underneath any explicit\n> command line options, thereby fulfilling the agreed upon semantic\n> obligation.\n> \n> c) is inspired by a simple, powerful, easy-to-understand, and popular\n> metaphor: layer flattening.\n> \n> Why, oh why do some people think there's an \"easier\" way[1]?!\n\nBecause I outlined it above?\n\nLook, I am not opposed to layer flattening if that's what is required to\nget it right. But consider the downside of layer flattening: we must\nalways record intent-to-change when making a change to the struct (i.e.,\nthe \"mask\" variable in your original patches). This is fine for members\nhidden behind macros, but there are a lot of members that are assigned\nto directly. We would need to:\n\n 1. Introduce new infrastructure for assigning to these members.\n\n 2. Fix existing locations by converting them to this infrastructure.\n\n 3. Introduce some mechanism to help future callers get it right (since\n otherwise assigning directly is a subtle bug).\n\nThis is elementary encapsulation; in a language with better OO support,\nyou would hide all of your struct members behind accessors. But this is\nC, and a dialect of C where that doesn't usually happen. So I think it\nis going to introduce a lot of code changes, and the resulting code will\nnot look as much like the rest of git as it once did.\n\nSo what I am suggesting is that _if_ there is an easier way to do it,\nthen it is worth exploring.\n\n-Peff\n"},{"id":"108737","messageId":"alpine.GSO.2.00.0903200911530.16242@kiwi.cs.ucla.edu","threadId":"17509","inReplyTo":"20090320070148.GD27008@coredump.intra.peff.net","subject":"Re: [PATCH v2 1/2] Introduce config variable \"diff.defaultOptions\"","fromName":"Keith Cascio","fromEmail":"keith@cs.ucla.edu","sentAt":"2009-03-20T17:11:27Z","receivedAt":"2009-03-20T17:11:27Z","isPatch":true,"sender":{"key":"keith@cs.ucla.edu","avatar":"https://gravatar.com/avatar/c5ec3a8f1cd1f449fdf8bdb7125fdbfd10b729507f32cbf0aa4ad07b4f7127ae?d=mp&s=160"},"body":"Peff,\n\nThank you for this extremely thoughtful reply. First, I want to ease concern \nover the point about \"intent-to-change\". BTW, everything I describe here is \nalready implemented in v3.\n\nOn Fri, 20 Mar 2009, Jeff King wrote:\n\n> Look, I am not opposed to layer flattening if that's what is required to get \n> it right. But consider the downside of layer flattening: we must always record \n> intent-to-change when making a change to the struct (i.e., the \"mask\" variable \n> in your original patches). This is fine for members hidden behind macros, but \n> there are a lot of members that are assigned to directly. We would need to:\n> \n> 1. Introduce new infrastructure for assigning to these members.\n\nOnly the bit flag fields need special infrastructure! IOW, the macros are only \nnecessary for the bit flags. For numeric data or pointer data, there's no need \nto keep extra state, and there's no need for callsites to change from direct \nassignment. Only for bit flags, we need an extra bit to remember whether the \nvalue is pristine or not. For all other data:\n\n(a) numeric data (integers, chars, and floats): define magic value(s) that \nrepresent pristineness. Initialize all fields to PRISTINE. Later, if a field \nhas any value other than PRISTINE, we know there was intent-to-change.\n\n(b) pointer data: NULL is the pristine value. Any value other than NULL means \nintent-to-change.\n\n> 2. Fix existing locations by converting them to this infrastructure.\n\nAs of 628d5c2, all callsites that set bit flags are already updated to use the \nmacros. As mentioned above, all other locations can keep on keepin' on using \ndirect assignment. No change here.\n\n> 3. Introduce some mechanism to help future callers get it right (since\n> otherwise assigning directly is a subtle bug).\n\nYes, in the future, someone could write naughty code that sets a bit flag \ndirectly rather than using one of the macros. In C, we probably can't make that \nimpossible. But generally speaking we can't protect against all forms of gross \nnegligence. In order to commit his crime, this hypothetical programmer must \nignore the fact that these bits are never set directly, anywhere in the code, \nperiod. A competent programmer would, after deciding that he needs to set a \nbit, look at other spots where such bits are set, and mimic. I think the normal \npatch auditing process this community follows would raise alarms on patches \ncoming from negligent programmers (there are always tell-tale signs). And, in \nthe event that, nevertheless, Junio applies a bit-flag-direct-assignment patch, \nit will result in a bug of precisely the following form: an explicitly-given \ncommand-line option to a porcelain command fails to override a default option. \nIt will be noticed and fixed. It's not fatal, it doesn't corrupt data, it \naffects only porcelain and it's not hidden. Of all the insect kingdom (grand \nscheme of hypothetical bugs), this one isn't worth abandoning a good design \nover.\n\nAll in all, turns out v3 requires surprisingly little modification of existing \ncode outside of diff.h/diff.c. Actually, it only adds 3 lines, that's it!!\n\n builtin-diff.c | 2 +\n builtin-log.c | 1 +\n diff.c | 112 ++++++++++++++++++++++++-\n diff.h | 17 +++-\n\nShall I post v3?\n -- Keith\n"},{"id":"108752","messageId":"20090320194930.GB26934@coredump.intra.peff.net","threadId":"17509","inReplyTo":"alpine.GSO.2.00.0903200911530.16242@kiwi.cs.ucla.edu","subject":"Re: [PATCH v2 1/2] Introduce config variable \"diff.defaultOptions\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-03-20T19:49:30Z","receivedAt":"2009-03-20T19:49:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 20, 2009 at 10:11:27AM -0700, Keith Cascio wrote:\n\n> (a) numeric data (integers, chars, and floats): define magic value(s)\n> that represent pristineness. Initialize all fields to PRISTINE.\n> Later, if a field has any value other than PRISTINE, we know there was\n> intent-to-change.\n\nGood point. Though we will need to make sure that existing code is never\nlooking at PRISTINE values, which aren't likely to make much sense (I\nsuspect most will be INT_MAX or -1, as 0 is a reasonable value for many\nof the options). This should be easy for most code, since the flattening\nwill get rid of PRISTINE. But remember that there are pieces of code\nthat do something like:\n\n if (some_diff_option_is_set)\n set_some_other_related_diff_option;\n\nwhich will need to be PRISTINE-aware.\n\n> > 3. Introduce some mechanism to help future callers get it right\n> > (since otherwise assigning directly is a subtle bug).\n> \n> Yes, in the future, someone could write naughty code that sets a bit\n> flag directly rather than using one of the macros. In C, we probably\n> can't make that impossible. But generally speaking we can't protect\n> against all forms of gross negligence.\n\nI think you can safely ignore this complaint. I was worried we would\nneed something like:\n\n DIFF_SET(&opt, stat_name_width, 10);\n\nIt is much easier to mistakenly write this as\n\n opt.state_name_width = 10;\n\nthan it is to accidentally do a bit-set when there is a DIFF_OPT_SET\nmacro. That is, I think most people _want_ to use DIFF_OPT_SET because\nit is easier to read and less typing.\n\nSo I saw this as a problem more for non-bit options, but you have\nalready addressed that above.\n\n> All in all, turns out v3 requires surprisingly little modification of\n> existing code outside of diff.h/diff.c. Actually, it only adds 3\n> lines, that's it!!\n>\n> builtin-diff.c | 2 +\n> builtin-log.c | 1 +\n> diff.c | 112 ++++++++++++++++++++++++-\n> diff.h | 17 +++-\n>\n> Shall I post v3?\n\nYes, please. It is much better to be talking about actual code than\nhypotheticals.\n\n-Peff\n"},{"id":"108777","messageId":"1237600853-22815-1-git-send-email-keith@cs.ucla.edu","threadId":"17509","inReplyTo":"20090320194930.GB26934@coredump.intra.peff.net","subject":"[PATCH/RFC v3] Introduce config variable \"diff.defaultoptions\"","fromName":"Keith Cascio","fromEmail":"keith@cs.ucla.edu","sentAt":"2009-03-21T02:00:53Z","receivedAt":"2009-03-21T02:00:53Z","isPatch":true,"sender":{"key":"keith@cs.ucla.edu","avatar":"https://gravatar.com/avatar/c5ec3a8f1cd1f449fdf8bdb7125fdbfd10b729507f32cbf0aa4ad07b4f7127ae?d=mp&s=160"},"body":"Improve porcelain diff's accommodation of user preference by allowing any\ncommand-line option to (a) persist over all invocations and (b) stay consistent\nover multiple tools (e.g. command-line and gui). The approach taken here is\ngood because it delivers the consistency a user expects without breaking any\nplumbing. It works by allowing the user, via git-config, to specify arbitrary\noptions to pass to porcelain diff on every invocation, including internal\ninvocations from other programs, e.g. git-gui.\n\nIntroduce diff command-line options --default-options and --no-default-options.\n\nAffect only porcelain diff: we suppress default options for plumbing\ndiff-{files,index,tree}, format-patch, and all other commands unless explicitly\nrequested using --default-options (opt-in).\n\nSigned-off-by: Keith Cascio \n---\n\nPlease notice v3 supports all diff options (improvement over v2).\n\nThis is a RFC. I omitted the documentation and test portions for now.\n\n -- Keith\n\n diff.h | 17 ++++++--\n diff.c | 112 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n builtin-diff.c | 1 +\n builtin-log.c | 1 +\n 4 files changed, 125 insertions(+), 6 deletions(-)\n\ndiff --git a/diff.h b/diff.h\nindex 6616877..66f1518 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -66,12 +66,17 @@ 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_ALLOW_DEFAULT_OPTIONS (1 << 22)\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_SET(opts, flag) ((opts)->flags |= DIFF_OPT_##flag),\\\n+\t\t\t\t ((opts)->mask |= DIFF_OPT_##flag)\n-#define DIFF_OPT_CLR(opts, flag) ((opts)->flags &= ~DIFF_OPT_##flag)\n+#define DIFF_OPT_CLR(opts, flag) ((opts)->flags &= ~DIFF_OPT_##flag),\\\n+\t\t\t\t ((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)\n+#define DIFF_XDL_SET(opts, flag) ((opts)->xdl_opts |= XDF_##flag),\\\n+\t\t\t\t ((opts)->xdl_mask |= XDF_##flag)\n-#define DIFF_XDL_CLR(opts, flag) ((opts)->xdl_opts &= ~XDF_##flag)\n+#define DIFF_XDL_CLR(opts, flag) ((opts)->xdl_opts &= ~XDF_##flag),\\\n+\t\t\t\t ((opts)->xdl_mask |= XDF_##flag)\n \n struct diff_options {\n \tconst char *filter;\n@@ -80,6 +85,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@@ -98,6 +104,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;\n@@ -193,7 +200,7 @@ extern void diff_unmerge(struct diff_options *,\n extern int git_diff_basic_config(const char *var, const char *value, void *cb);\n extern int git_diff_ui_config(const char *var, const char *value, void *cb);\n extern int diff_use_color_default;\n-extern void diff_setup(struct diff_options *);\n+extern struct diff_options* diff_setup(struct diff_options *);\n extern int diff_opt_parse(struct diff_options *, const char **, int);\n extern int diff_setup_done(struct diff_options *);\n \ndiff --git a/diff.c b/diff.c\nindex 75d9fab..1c4fec4 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -26,6 +26,8 @@ static int diff_suppress_blank_empty;\n int diff_use_color_default = -1;\n static const char *diff_word_regex_cfg;\n static const char *external_diff_cmd_cfg;\n+static const char *diff_defaults;\n+static struct diff_options *defaults;\n int diff_auto_refresh_index = 1;\n static int diff_mnemonic_prefix;\n \n@@ -106,6 +108,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.defaultoptions\"))\n+\t\treturn git_config_string(&diff_defaults, var, value);\n \n \tswitch (userdiff_config(var, value)) {\n \t\tcase 0: break;\n@@ -2314,7 +2318,101 @@ 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+struct diff_options* parse_diff_defaults(struct diff_options *options)\n+{\n+\tint count, len, i;\n+\tconst char **new_argv;\n+\n+\tif ((! diff_defaults) || ((len = (strlen(diff_defaults)+1)) < 3))\n+\t\treturn options;\n+\tcount = split_cmdline(strncpy(xmalloc(len), diff_defaults, len),\n+\t\t\t&new_argv);\n+\tfor (i=0; ip = master->p ? master->p : slave->p\n+#define COALESCE_INT(i) master->i = master->i != PRISTINE ? master->i : slave->i\n+\n+struct diff_options* flatten_diff_options(struct diff_options *master,\n+\t\t\t\t\t struct diff_options *slave)\n+{\n+\t/*\n+\t * Our desired flags are:\n+\t *\n+\t * 1. Anything the master hasn't explicitly set, we can take from\n+\t * the slave.\n+\t * 2. Anything the slave didn't explicitly set, we can take whether\n+\t * or not the master set it explicitly.\n+\t * 3. Anything the master explicitly set, we take.\n+\t */\n+\tmaster->flags =\n+\t /* (1) */ (~master->mask & slave->flags & slave->mask) |\n+\t /* (2) */ (master->flags & ~slave->mask) |\n+\t /* (3) */ (master->flags & master->mask);\n+\tmaster->mask |= slave->mask;\n+\tmaster->xdl_opts =\n+\t /* (1) */ (~master->xdl_mask & slave->xdl_opts & slave->xdl_mask) |\n+\t /* (2) */ (master->xdl_opts & ~slave->xdl_mask) |\n+\t /* (3) */ (master->xdl_opts & master->xdl_mask);\n+\tmaster->xdl_mask |= slave->xdl_mask;\n+\tmaster->output_format |= slave->output_format;\n+\tmaster->setup |= slave->setup;\n+\tCOALESCE_PTR(a_prefix);\n+\tCOALESCE_PTR(b_prefix);\n+\tCOALESCE_PTR(filter);\n+\tCOALESCE_PTR(orderfile);\n+\tCOALESCE_PTR(pickaxe);\n+\tCOALESCE_PTR(prefix);\n+\tCOALESCE_PTR(single_follow);\n+\tCOALESCE_PTR(stat_sep);\n+\tCOALESCE_PTR(word_regex);\n+\tCOALESCE_INT(abbrev);\n+\tCOALESCE_INT(break_opt);\n+\tCOALESCE_INT(close_file);\n+\tCOALESCE_INT(context);\n+\tCOALESCE_INT(detect_rename);\n+\tCOALESCE_INT(dirstat_percent);\n+\tCOALESCE_INT(interhunkcontext);\n+\tCOALESCE_INT(line_termination);\n+\tCOALESCE_INT(pickaxe_opts);\n+\tCOALESCE_INT(prefix_length);\n+\tCOALESCE_INT(rename_limit);\n+\tCOALESCE_INT(rename_score);\n+\tCOALESCE_INT(skip_stat_unmatch);\n+\tCOALESCE_INT(stat_name_width);\n+\tCOALESCE_INT(stat_width);\n+\tCOALESCE_INT(warn_on_too_large_rename);\n+\tCOALESCE_PTR(file);\n+\tCOALESCE_PTR(change);\n+\tCOALESCE_PTR(add_remove);\n+\tCOALESCE_PTR(format_callback);\n+\tCOALESCE_PTR(format_callback_data);\n+\tif((! master->paths) && slave->paths){\n+\t\tmaster->nr_paths = slave->nr_paths;\n+\t\tmaster->paths = slave->paths;\n+\t\tmaster->pathlens = slave->pathlens;\n+\t}\n+\treturn master;\n+}\n+\n-void diff_setup(struct diff_options *options)\n+struct diff_options* diff_setup(struct diff_options *options)\n+{\n+\tmemset(options, 0, sizeof(*options));\n+\toptions->abbrev = options->break_opt = options->close_file =\n+\toptions->context = options->detect_rename =\n+\toptions->dirstat_percent = options->interhunkcontext =\n+\toptions->line_termination = options->pickaxe_opts =\n+\toptions->prefix_length = options->rename_limit =\n+\toptions->rename_score = options->skip_stat_unmatch =\n+\toptions->stat_name_width = options->stat_width =\n+\toptions->warn_on_too_large_rename = PRISTINE;\n+\treturn options;\n+}\n+\n+struct diff_options* diff_fallback_values(struct diff_options *options)\n {\n \tmemset(options, 0, sizeof(*options));\n \n@@ -2336,11 +2434,19 @@ void diff_setup(struct diff_options *options)\n \t\toptions->a_prefix = \"a/\";\n \t\toptions->b_prefix = \"b/\";\n \t}\n+\treturn options;\n }\n \n int diff_setup_done(struct diff_options *options)\n {\n \tint count = 0;\n+\tstruct diff_options fallback;\n+\n+\tif (DIFF_OPT_TST(options, ALLOW_DEFAULT_OPTIONS))\n+\t\tflatten_diff_options(options, defaults ? defaults :\n+\t\t\tparse_diff_defaults(diff_setup(defaults =\n+\t\t\t\txmalloc(sizeof(struct diff_options)))));\n+\tflatten_diff_options(options, diff_fallback_values(&fallback));\n \n \tif (options->output_format & DIFF_FORMAT_NAME)\n \t\tcount++;\n@@ -2615,6 +2721,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, \"--default-options\"))\n+\t\tDIFF_OPT_SET(options, ALLOW_DEFAULT_OPTIONS);\n+\telse if (!strcmp(arg, \"--no-default-options\"))\n+\t\tDIFF_OPT_CLR(options, ALLOW_DEFAULT_OPTIONS);\n \n \t/* misc options */\n \telse if (!strcmp(arg, \"-z\"))\ndiff --git a/builtin-diff.c b/builtin-diff.c\nindex d75d69b..34c6ce2 100644\n--- a/builtin-diff.c\n+++ b/builtin-diff.c\n@@ -283,6 +283,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\tdiff_use_color_default = git_use_color_default;\n \n \tinit_revisions(&rev, prefix);\n+\tDIFF_OPT_SET(&rev.diffopt, ALLOW_DEFAULT_OPTIONS);\n \n \t/* If this is a no-index diff, just run it and exit there. */\n \tdiff_no_index(&rev, argc, argv, nongit, prefix);\ndiff --git a/builtin-log.c b/builtin-log.c\nindex 8af55d2..1fa583f 100644\n--- a/builtin-log.c\n+++ b/builtin-log.c\n@@ -37,6 +37,7 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,\n \t\tget_commit_format(fmt_pretty, rev);\n \trev->verbose_header = 1;\n \tDIFF_OPT_SET(&rev->diffopt, RECURSIVE);\n+\tDIFF_OPT_SET(&rev->diffopt, ALLOW_DEFAULT_OPTIONS);\n \trev->show_root_diff = default_show_root;\n \trev->subject_prefix = fmt_patch_subject_prefix;\n \tDIFF_OPT_SET(&rev->diffopt, ALLOW_TEXTCONV);\n-- \n1.6.2.1.337.g3b73.dirty\n"},{"id":"108780","messageId":"alpine.DEB.1.00.0903210415110.10279@pacific.mpi-cbg.de","threadId":"17509","inReplyTo":"1237600853-22815-1-git-send-email-keith@cs.ucla.edu","subject":"[PATCH] Allow setting default diff options via diff.defaultOptions","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-03-21T03:15:39Z","receivedAt":"2009-03-21T03:15:39Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"\nThe idea is from Keith Cascio.\n\nSigned-off-by: Johannes Schindelin \n---\n\n\tI do not particularly like what this patch does, but I like\n\tthe non-intrusiveness and conciseness of it.\n\n Documentation/config.txt | 4 ++++\n Documentation/git-diff.txt | 3 +++\n builtin-diff.c | 1 +\n builtin-log.c | 4 ++++\n diff.c | 25 +++++++++++++++++++++++++\n diff.h | 2 ++\n t/t4037-diff-default-options.sh | 19 +++++++++++++++++++\n 7 files changed, 58 insertions(+), 0 deletions(-)\n create mode 100755 t/t4037-diff-default-options.sh\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 7506755..4913bd6 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -625,6 +625,10 @@ diff.autorefreshindex::\n \taffects only 'git-diff' Porcelain, and not lower level\n \t'diff' commands, such as 'git-diff-files'.\n \n+diff.defaultoptions:\n+\tThe value of this option will be prepended to the command line\n+\toptions of the porcelains showing diffs.\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/git-diff.txt b/Documentation/git-diff.txt\nindex a2f192f..7025717 100644\n--- a/Documentation/git-diff.txt\n+++ b/Documentation/git-diff.txt\n@@ -74,6 +74,9 @@ and the range notations (\"..\" and\n \"\\...\") do not mean a range as defined in the\n \"SPECIFYING RANGES\" section in linkgit:git-rev-parse[1].\n \n+Default options can be set via the config variable\n+`diff.defaultOptions`.\n+\n OPTIONS\n -------\n :git-diff: 1\ndiff --git a/builtin-diff.c b/builtin-diff.c\nindex d75d69b..d9a6e7d 100644\n--- a/builtin-diff.c\n+++ b/builtin-diff.c\n@@ -296,6 +296,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \n \tif (nongit)\n \t\tdie(\"Not a git repository\");\n+\tparse_default_diff_options(&rev.diffopt);\n \targc = setup_revisions(argc, argv, &rev, NULL);\n \tif (!rev.diffopt.output_format) {\n \t\trev.diffopt.output_format = DIFF_FORMAT_PATCH;\ndiff --git a/builtin-log.c b/builtin-log.c\nindex 8af55d2..2a63652 100644\n--- a/builtin-log.c\n+++ b/builtin-log.c\n@@ -243,6 +243,7 @@ int cmd_whatchanged(int argc, const char **argv, const char *prefix)\n \tinit_revisions(&rev, prefix);\n \trev.diff = 1;\n \trev.simplify_history = 0;\n+\tparse_default_diff_options(&rev.diffopt);\n \tcmd_log_init(argc, argv, prefix, &rev);\n \tif (!rev.diffopt.output_format)\n \t\trev.diffopt.output_format = DIFF_FORMAT_RAW;\n@@ -314,6 +315,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)\n \trev.always_show_header = 1;\n \trev.ignore_merges = 0;\n \trev.no_walk = 1;\n+\tparse_default_diff_options(&rev.diffopt);\n \tcmd_log_init(argc, argv, prefix, &rev);\n \n \tcount = rev.pending.nr;\n@@ -381,6 +383,7 @@ int cmd_log_reflog(int argc, const char **argv, const char *prefix)\n \tinit_reflog_walk(&rev.reflog_info);\n \trev.abbrev_commit = 1;\n \trev.verbose_header = 1;\n+\tparse_default_diff_options(&rev.diffopt);\n \tcmd_log_init(argc, argv, prefix, &rev);\n \n \t/*\n@@ -412,6 +415,7 @@ int cmd_log(int argc, const char **argv, const char *prefix)\n \n \tinit_revisions(&rev, prefix);\n \trev.always_show_header = 1;\n+\tparse_default_diff_options(&rev.diffopt);\n \tcmd_log_init(argc, argv, prefix, &rev);\n \treturn cmd_log_walk(&rev);\n }\ndiff --git a/diff.c b/diff.c\nindex 75d9fab..0e1b321 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2657,6 +2657,31 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)\n \treturn 1;\n }\n \n+static int default_diff_options(const char *key, const char *value, void *cb)\n+{\n+\tif (!strcmp(key, \"diff.defaultoptions\")) {\n+\t\tchar **options = cb;\n+\t\tfree(*options);\n+\t\t*options = xstrdup(value);\n+\t}\n+\treturn 0;\n+}\n+\n+void parse_default_diff_options(struct diff_options *options)\n+{\n+\tchar *default_options = NULL;\n+\tconst char **argv;\n+\tint argc;\n+\n+\tgit_config(default_diff_options, &default_options);\n+\tif (!default_options)\n+\t\treturn;\n+\n+\targc = split_cmdline(default_options, &argv);\n+\tdiff_opt_parse(options, argv, argc);\n+\tfree(argv);\n+}\n+\n static int parse_num(const char **cp_p)\n {\n \tunsigned long num, scale;\ndiff --git a/diff.h b/diff.h\nindex 6616877..e05e796 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -270,4 +270,6 @@ extern void diff_no_index(struct rev_info *, int, const char **, int, const char\n \n extern int index_differs_from(const char *def, int diff_flags);\n \n+extern void parse_default_diff_options(struct diff_options *options);\n+\n #endif /* DIFF_H */\ndiff --git a/t/t4037-diff-default-options.sh b/t/t4037-diff-default-options.sh\nnew file mode 100755\nindex 0000000..0284f7b\n--- /dev/null\n+++ b/t/t4037-diff-default-options.sh\n@@ -0,0 +1,19 @@\n+#!/bin/sh\n+\n+test_description='default options for diff'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\ttest_commit a &&\n+\ttest_commit b\n+'\n+\n+test_expect_success 'diff.defaultOptions' '\n+\tgit config diff.defaultOptions --raw &&\n+\tgit diff a > output &&\n+\tgrep ^: output &&\n+\ttest 1 = $(wc -l < output)\n+'\n+\n+test_done\n-- \n1.6.2.1.493.g67cf3\n"},{"id":"110256","messageId":"alpine.GSO.2.00.0904021647120.16242@kiwi.cs.ucla.edu","threadId":"17509","inReplyTo":"alpine.DEB.1.00.0903210415110.10279@pacific.mpi-cbg.de","subject":"Re: [PATCH] Allow setting default diff options via diff.defaultOptions","fromName":"Keith Cascio","fromEmail":"keith@cs.ucla.edu","sentAt":"2009-04-03T00:04:18Z","receivedAt":"2009-04-03T00:04:18Z","isPatch":true,"sender":{"key":"keith@cs.ucla.edu","avatar":"https://gravatar.com/avatar/c5ec3a8f1cd1f449fdf8bdb7125fdbfd10b729507f32cbf0aa4ad07b4f7127ae?d=mp&s=160"},"body":"Johannes,\n\nOn Sat, 21 Mar 2009, Johannes Schindelin wrote:\n\n> The idea is from Keith Cascio.\n> \n> Signed-off-by: Johannes Schindelin \n> ---\n> \tI do not particularly like what this patch does, but I like\n> \tthe non-intrusiveness and conciseness of it.\n\nYour patch does not provide a command line opt_out flag. Let me describe a \nworkflow situation and ask you how to handle it if the user were running your \npatch. Let diff.defaultOptions = \"-b\". The user is getting closer to \nsubmitting his patch and he wants to see patch output identical to what `git format-patch`\nwill produce. What command should he use?\n\n `git format-patch --stdout master` ?\n\n -- Keith\n"},{"id":"110884","messageId":"alpine.GSO.2.00.0904081741410.15657@kiwi.cs.ucla.edu","threadId":"17509","inReplyTo":"alpine.DEB.1.00.0903210415110.10279@pacific.mpi-cbg.de","subject":"Re: [PATCH] Allow setting default diff options via diff.defaultOptions","fromName":"Keith Cascio","fromEmail":"keith@cs.ucla.edu","sentAt":"2009-04-09T00:44:29Z","receivedAt":"2009-04-09T00:44:29Z","isPatch":true,"sender":{"key":"keith@cs.ucla.edu","avatar":"https://gravatar.com/avatar/c5ec3a8f1cd1f449fdf8bdb7125fdbfd10b729507f32cbf0aa4ad07b4f7127ae?d=mp&s=160"},"body":"Johannes,\n\nHi. Was my last message understandable or should I add more explanation?\n http://comments.gmane.org/gmane.comp.version-control.git/115506\n\n -- Keith\n"},{"id":"110899","messageId":"alpine.DEB.1.00.0904091029360.10279@pacific.mpi-cbg.de","threadId":"17509","inReplyTo":"alpine.GSO.2.00.0904081741410.15657@kiwi.cs.ucla.edu","subject":"Re: [PATCH] Allow setting default diff options via diff.defaultOptions","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-04-09T08:29:56Z","receivedAt":"2009-04-09T08:29:56Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 8 Apr 2009, Keith Cascio wrote:\n\n> Hi. Was my last message understandable or should I add more \n> explanation?\n> http://comments.gmane.org/gmane.comp.version-control.git/115506\n\nSorry, I am just short on time for that hobby called Git.\n\nCiao,\nDscho\n"},{"id":"110900","messageId":"20090409083115.GA17622@coredump.intra.peff.net","threadId":"17509","inReplyTo":"alpine.GSO.2.00.0904081741410.15657@kiwi.cs.ucla.edu","subject":"Re: [PATCH] Allow setting default diff options via diff.defaultOptions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-04-09T08:31:15Z","receivedAt":"2009-04-09T08:31:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 08, 2009 at 05:44:29PM -0700, Keith Cascio wrote:\n\n> Hi. Was my last message understandable or should I add more explanation?\n> http://comments.gmane.org/gmane.comp.version-control.git/115506\n\nYour point made sense to me, and is the expected difference between the\ntwo approaches (as we discussed before).\n\nI'm sorry I haven't had a chance to review your patch in detail. It is\npleasantly much shorter than previous iterations, but I wanted to very\ncarefully check a few of the diff callsites to make sure they work\nproperly (e.g., some of the cases I laid out earlier in the thread).\n\nI have a lot of regular life responsibilities right now, but I'll try\ntake a close look this weekend.\n\n-Peff\n"},{"id":"110901","messageId":"alpine.DEB.1.00.0904091030030.10279@pacific.mpi-cbg.de","threadId":"17509","inReplyTo":"alpine.GSO.2.00.0904021647120.16242@kiwi.cs.ucla.edu","subject":"Re: [PATCH] Allow setting default diff options via diff.defaultOptions","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-04-09T08:45:40Z","receivedAt":"2009-04-09T08:45:40Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 2 Apr 2009, Keith Cascio wrote:\n\n> On Sat, 21 Mar 2009, Johannes Schindelin wrote:\n> \n> > The idea is from Keith Cascio.\n> > \n> > Signed-off-by: Johannes Schindelin \n> > ---\n> > \tI do not particularly like what this patch does, but I like\n> > \tthe non-intrusiveness and conciseness of it.\n> \n> Your patch does not provide a command line opt_out flag. Let me describe a \n> workflow situation and ask you how to handle it if the user were running your \n> patch. Let diff.defaultOptions = \"-b\". The user is getting closer to \n> submitting his patch and he wants to see patch output identical to what `git format-patch`\n> will produce. What command should he use?\n> \n> `git format-patch --stdout master` ?\n\nThe proper way would be to have options to _undo_ every diff option, I \nguess, as this would also help aliases in addition to defaultOptions.\n\nIn the case of format-patch, though, I am pretty certain that I do not \nwant any diff.defaultOptions: the output is almost always intended for \nmachine consumption, so it is a different kind of cattle.\n\nNow, it is easy to put a patch on top of my patch to support something \nlike --no-defaults.\n\nOf course, to keep things simple, this has to be a separate patch.\n\nCiao,\nDscho\n"},{"id":"110903","messageId":"20090409084903.GA18947@coredump.intra.peff.net","threadId":"17509","inReplyTo":"alpine.DEB.1.00.0904091030030.10279@pacific.mpi-cbg.de","subject":"Re: [PATCH] Allow setting default diff options via diff.defaultOptions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-04-09T08:49:03Z","receivedAt":"2009-04-09T08:49:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 09, 2009 at 10:45:40AM +0200, Johannes Schindelin wrote:\n\n> The proper way would be to have options to _undo_ every diff option, I \n> guess, as this would also help aliases in addition to defaultOptions.\n\nI agree with this sentiment, no matter which approach is taken. I am\nmore like to say \"take my usual defaults, but tweak this one thing\" than\nto say \"turn off all of my defaults\".\n\n> Now, it is easy to put a patch on top of my patch to support something \n> like --no-defaults.\n\nNo, it's not. We went over this in great detail earlier in the thread.\nIf you want:\n\n git diff --no-defaults\n\nthen you basically have to parse twice to avoid the chicken-and-egg\nproblem. Which is why I suggested:\n\n git --no-defaults diff\n\nwhich does work. Keith's solution does allow \"git diff --no-defaults\".\n\n-Peff\n"},{"id":"110910","messageId":"alpine.DEB.1.00.0904091242430.10279@pacific.mpi-cbg.de","threadId":"17509","inReplyTo":"20090409084903.GA18947@coredump.intra.peff.net","subject":"Re: [PATCH] Allow setting default diff options via diff.defaultOptions","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-04-09T10:43:28Z","receivedAt":"2009-04-09T10:43:28Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 9 Apr 2009, Jeff King wrote:\n\n> On Thu, Apr 09, 2009 at 10:45:40AM +0200, Johannes Schindelin wrote:\n> \n> > Now, it is easy to put a patch on top of my patch to support something \n> > like --no-defaults.\n> \n> No, it's not. We went over this in great detail earlier in the thread. \n> If you want:\n> \n> git diff --no-defaults\n> \n> then you basically have to parse twice to avoid the chicken-and-egg\n> problem.\n\nSo what? We parse the config a gazillion times, and there we have to \naccess the _disk_.\n\nCiao,\nDscho\n"},{"id":"110928","messageId":"alpine.GSO.2.00.0904090922160.15657@kiwi.cs.ucla.edu","threadId":"17509","inReplyTo":"alpine.DEB.1.00.0904091030030.10279@pacific.mpi-cbg.de","subject":"Re: [PATCH] Allow setting default diff options via diff.defaultOptions","fromName":"Keith Cascio","fromEmail":"keith@cs.ucla.edu","sentAt":"2009-04-09T16:29:38Z","receivedAt":"2009-04-09T16:29:38Z","isPatch":true,"sender":{"key":"keith@cs.ucla.edu","avatar":"https://gravatar.com/avatar/c5ec3a8f1cd1f449fdf8bdb7125fdbfd10b729507f32cbf0aa4ad07b4f7127ae?d=mp&s=160"},"body":"Johannes,\n\nOn Thu, 9 Apr 2009, Johannes Schindelin wrote:\n\n> In the case of format-patch, though, I am pretty certain that I do not want \n> any diff.defaultOptions: the output is almost always intended for machine \n> consumption, so it is a different kind of cattle.\n\nJust to clarify: I agree. I certainly would never want diff.defaultOptions to \naffect format-patch, and none of my patches did so. The reason I brought up \nformat-patch is because, without an opt_out mechanism, it's harder for the user \nto use `git diff` to produce patch output identical to format-patch.\n\n> Now, it is easy to put a patch on top of my patch to support something like \n> --no-defaults.\n\nWith all due respect sir, I think you would find that if you sit down and \nattempt to add such functionality on top of your version, it would be an \nunpleasant experience. I predict the code would quickly turn inelegant and \nfragile. I believe it would prompt you to consider a redesign (assuming you \nwork and think quickly, after about 15 minutes), and the obvious solution would \nclosely resemble my v3:\n http://comments.gmane.org/gmane.comp.version-control.git/114021\n\n -- Keith\n"},{"id":"110994","messageId":"20090410080155.GB32195@coredump.intra.peff.net","threadId":"17509","inReplyTo":"alpine.DEB.1.00.0904091242430.10279@pacific.mpi-cbg.de","subject":"Re: [PATCH] Allow setting default diff options via diff.defaultOptions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-04-10T08:01:55Z","receivedAt":"2009-04-10T08:01:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 09, 2009 at 12:43:28PM +0200, Johannes Schindelin wrote:\n\n> > > Now, it is easy to put a patch on top of my patch to support something \n> > > like --no-defaults.\n> > \n> > No, it's not. We went over this in great detail earlier in the thread. \n> > If you want:\n> > \n> > git diff --no-defaults\n> > \n> > then you basically have to parse twice to avoid the chicken-and-egg\n> > problem.\n> \n> So what? We parse the config a gazillion times, and there we have to \n> access the _disk_.\n\nBut the first parse is only looking for \"--no-defaults\". So you need to:\n\n 1. Understand the semantics of the other options to correctly parse\n around them (i.e., knowing which ones take arguments).\n\n 2. Parse the arguments without actually respecting most of them, since\n they will be parsed again later.\n\nThis would actually be pretty easy if we had a declarative structure\ndescribing each option (like parseopt-ified options do). But the diff\noptions are parsed by a big conditional statement.\n\nTwo ways to make it easier would be:\n\n 1. You could loosen (1) above by assuming that --no-defaults will\n never appears as the argument to an option, and therefore any time\n you find it, it should be respected. Thus your first parse is just\n a simple loop looking for the option.\n\n 2. You could loosen (2) above by assuming that all options are\n idempotent. I don't know whether this is the case (I think it\n isn't for all git options, but a cursory look shows that it may be\n for diff options).\n\n-Peff\n"},{"id":"111244","messageId":"alpine.DEB.1.00.0904140036341.10279@pacific.mpi-cbg.de","threadId":"17509","inReplyTo":"20090410080155.GB32195@coredump.intra.peff.net","subject":"[PATCH] Add the diff option --no-defaults","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-04-13T22:37:42Z","receivedAt":"2009-04-13T22:37:42Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"\nIt would be desirable to undo every setting in diff.defaultOptions\nindividually, but until there are options to reset every command\nline option, there is the \"--no-defaults\" option (which can be\noverridden by the \"--defaults\" option) to ignore the config setting.\n\nSigned-off-by: Johannes Schindelin \n---\n\nOn Fri, 10 Apr 2009, Jeff King wrote:\n\n> On Thu, Apr 09, 2009 at 12:43:28PM +0200, Johannes Schindelin wrote:\n> \n> > > > Now, it is easy to put a patch on top of my patch to support something \n> > > > like --no-defaults.\n> > > \n> > > No, it's not. We went over this in great detail earlier in the thread. \n> > > If you want:\n> > > \n> > > git diff --no-defaults\n> > > \n> > > then you basically have to parse twice to avoid the chicken-and-egg\n> > > problem.\n> > \n> > So what? We parse the config a gazillion times, and there we have to \n> > access the _disk_.\n> \n> But the first parse is only looking for \"--no-defaults\". So you need to:\n> \n> 1. Understand the semantics of the other options to correctly parse\n> around them (i.e., knowing which ones take arguments).\n> \n> 2. Parse the arguments without actually respecting most of them, since\n> they will be parsed again later.\n> \n> This would actually be pretty easy if we had a declarative structure\n> describing each option (like parseopt-ified options do). But the diff\n> options are parsed by a big conditional statement.\n> \n> Two ways to make it easier would be:\n> \n> 1. You could loosen (1) above by assuming that --no-defaults will\n> never appears as the argument to an option, and therefore any time\n> you find it, it should be respected. Thus your first parse is just\n> a simple loop looking for the option.\n> \n> 2. You could loosen (2) above by assuming that all options are\n> idempotent. I don't know whether this is the case (I think it\n> isn't for all git options, but a cursory look shows that it may be\n> for diff options).\n\nI go with 1)\n\n builtin-diff.c | 2 +-\n builtin-log.c | 8 ++++----\n diff.c | 29 ++++++++++++++++++++++-------\n diff.h | 2 +-\n t/t4037-diff-default-options.sh | 5 +++++\n 5 files changed, 33 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin-diff.c b/builtin-diff.c\nindex d9a6e7d..8da0052 100644\n--- a/builtin-diff.c\n+++ b/builtin-diff.c\n@@ -296,7 +296,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \n \tif (nongit)\n \t\tdie(\"Not a git repository\");\n-\tparse_default_diff_options(&rev.diffopt);\n+\targc = parse_default_diff_options(argc, argv, &rev.diffopt);\n \targc = setup_revisions(argc, argv, &rev, NULL);\n \tif (!rev.diffopt.output_format) {\n \t\trev.diffopt.output_format = DIFF_FORMAT_PATCH;\ndiff --git a/builtin-log.c b/builtin-log.c\nindex e926774..2f36537 100644\n--- a/builtin-log.c\n+++ b/builtin-log.c\n@@ -243,7 +243,7 @@ int cmd_whatchanged(int argc, const char **argv, const char *prefix)\n \tinit_revisions(&rev, prefix);\n \trev.diff = 1;\n \trev.simplify_history = 0;\n-\tparse_default_diff_options(&rev.diffopt);\n+\targc = parse_default_diff_options(argc, argv, &rev.diffopt);\n \tcmd_log_init(argc, argv, prefix, &rev);\n \tif (!rev.diffopt.output_format)\n \t\trev.diffopt.output_format = DIFF_FORMAT_RAW;\n@@ -315,7 +315,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)\n \trev.always_show_header = 1;\n \trev.ignore_merges = 0;\n \trev.no_walk = 1;\n-\tparse_default_diff_options(&rev.diffopt);\n+\targc = parse_default_diff_options(argc, argv, &rev.diffopt);\n \tcmd_log_init(argc, argv, prefix, &rev);\n \n \tcount = rev.pending.nr;\n@@ -383,7 +383,7 @@ int cmd_log_reflog(int argc, const char **argv, const char *prefix)\n \tinit_reflog_walk(&rev.reflog_info);\n \trev.abbrev_commit = 1;\n \trev.verbose_header = 1;\n-\tparse_default_diff_options(&rev.diffopt);\n+\targc = parse_default_diff_options(argc, argv, &rev.diffopt);\n \tcmd_log_init(argc, argv, prefix, &rev);\n \n \t/*\n@@ -415,7 +415,7 @@ int cmd_log(int argc, const char **argv, const char *prefix)\n \n \tinit_revisions(&rev, prefix);\n \trev.always_show_header = 1;\n-\tparse_default_diff_options(&rev.diffopt);\n+\targc = parse_default_diff_options(argc, argv, &rev.diffopt);\n \tcmd_log_init(argc, argv, prefix, &rev);\n \treturn cmd_log_walk(&rev);\n }\ndiff --git a/diff.c b/diff.c\nindex 6e76377..903dbb4 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -13,6 +13,7 @@\n #include \"utf8.h\"\n #include \"userdiff.h\"\n #include \"sigchain.h\"\n+#include \"parse-options.h\"\n \n #ifdef NO_FAST_WORKING_DIRECTORY\n #define FAST_WORKING_DIRECTORY 0\n@@ -2682,19 +2683,33 @@ static int default_diff_options(const char *key, const char *value, void *cb)\n \treturn 0;\n }\n \n-void parse_default_diff_options(struct diff_options *options)\n+int parse_default_diff_options(int real_argc, const char **real_argv,\n+\tstruct diff_options *options)\n {\n+\tint use_defaults = 1;\n+\tstruct option option[] = {\n+\t\tOPT_BOOLEAN(0, \"defaults\", &use_defaults, \"use diff defaults\"),\n+\t\tOPT_END()\n+\t};\n \tchar *default_options = NULL;\n \tconst char **argv;\n \tint argc;\n \n-\tgit_config(default_diff_options, &default_options);\n-\tif (!default_options)\n-\t\treturn;\n+\treal_argc = parse_options(real_argc, real_argv, option, NULL,\n+\t\tPARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN |\n+\t\tPARSE_OPT_NO_INTERNAL_HELP);\n+\n+\tif (use_defaults) {\n+\t\tgit_config(default_diff_options, &default_options);\n+\t\tif (!default_options)\n+\t\t\treturn real_argc;\n+\n+\t\targc = split_cmdline(default_options, &argv);\n+\t\tdiff_opt_parse(options, argv, argc);\n+\t\tfree(argv);\n+\t}\n \n-\targc = split_cmdline(default_options, &argv);\n-\tdiff_opt_parse(options, argv, argc);\n-\tfree(argv);\n+\treturn real_argc;\n }\n \n static int parse_num(const char **cp_p)\ndiff --git a/diff.h b/diff.h\nindex e05e796..764e2f6 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -270,6 +270,6 @@ extern void diff_no_index(struct rev_info *, int, const char **, int, const char\n \n extern int index_differs_from(const char *def, int diff_flags);\n \n-extern void parse_default_diff_options(struct diff_options *options);\n+extern int parse_default_diff_options(int argc, const char **argv, struct diff_options *options);\n \n #endif /* DIFF_H */\ndiff --git a/t/t4037-diff-default-options.sh b/t/t4037-diff-default-options.sh\nindex 0284f7b..f57d65f 100755\n--- a/t/t4037-diff-default-options.sh\n+++ b/t/t4037-diff-default-options.sh\n@@ -16,4 +16,9 @@ test_expect_success 'diff.defaultOptions' '\n \ttest 1 = $(wc -l < output)\n '\n \n+test_expect_success '--no-defaults' '\n+\tgit diff --no-defaults > output &&\n+\t! grep ^: output\n+'\n+\n test_done\n-- \n1.6.2.1.613.g25746\n"},{"id":"111435","messageId":"20090416083443.GA27399@coredump.intra.peff.net","threadId":"17509","inReplyTo":"alpine.DEB.1.00.0904140036341.10279@pacific.mpi-cbg.de","subject":"Re: [PATCH] Add the diff option --no-defaults","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-04-16T08:34:43Z","receivedAt":"2009-04-16T08:34:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 14, 2009 at 12:37:42AM +0200, Johannes Schindelin wrote:\n\n> > 1. You could loosen (1) above by assuming that --no-defaults will\n> > never appears as the argument to an option, and therefore any time\n> > you find it, it should be respected. Thus your first parse is just\n> > a simple loop looking for the option.\n> \n> I go with 1)\n\nThis feels very hack-ish to me, but perhaps this is a case of \"perfect\nis the enemy of the good\".\n\n-Peff\n"},{"id":"111436","messageId":"alpine.DEB.1.00.0904161124000.10279@pacific.mpi-cbg.de","threadId":"17509","inReplyTo":"20090416083443.GA27399@coredump.intra.peff.net","subject":"Re: [PATCH] Add the diff option --no-defaults","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-04-16T09:25:08Z","receivedAt":"2009-04-16T09:25:08Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 16 Apr 2009, Jeff King wrote:\n\n> On Tue, Apr 14, 2009 at 12:37:42AM +0200, Johannes Schindelin wrote:\n> \n> > > 1. You could loosen (1) above by assuming that --no-defaults will\n> > > never appears as the argument to an option, and therefore any time\n> > > you find it, it should be respected. Thus your first parse is just\n> > > a simple loop looking for the option.\n> > \n> > I go with 1)\n> \n> This feels very hack-ish to me, but perhaps this is a case of \"perfect\n> is the enemy of the good\".\n\nI have a strong feeling that none of our diff/rev options can sanely take \na parameter looking like \"--defaults\" or \"--no-defaults\".\n\nBut I do not have the time to audit the options. Maybe you have?\n\nCiao,\nDscho\n"},{"id":"111437","messageId":"20090416094154.GA30479@coredump.intra.peff.net","threadId":"17509","inReplyTo":"alpine.DEB.1.00.0904161124000.10279@pacific.mpi-cbg.de","subject":"Re: [PATCH] Add the diff option --no-defaults","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-04-16T09:41:55Z","receivedAt":"2009-04-16T09:41:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 16, 2009 at 11:25:08AM +0200, Johannes Schindelin wrote:\n\n> > This feels very hack-ish to me, but perhaps this is a case of \"perfect\n> > is the enemy of the good\".\n> \n> I have a strong feeling that none of our diff/rev options can sanely take \n> a parameter looking like \"--defaults\" or \"--no-defaults\".\n> \n> But I do not have the time to audit the options. Maybe you have?\n\nRight now, I think we are safe. A few options like \"--default\" do take a\nseparated string argument, but saying \"--default --no-defaults\" seems a\nlittle crazy to me (besides being confusing because they are talking\nabout two totally unrelated defaults).\n\nMost of the string-taking options require --option= and don't\nsupport the separated version. If the code were ever parseopt-ified,\nthey would start to support \"--option \"; however, at that time we\nshould be able to write an \"I know about these parseopt options, but\nplease ignore them according to what we know about them taking an\nargument\" function.\n\nThe one I would worry most about is \"-S\" since \"-S--no-defaults\" is a\nvery reasonable thing to ask for. Right now its argument _must_ be\nconnected. To be consistent with other git options, \"-S --no-defaults\"\n_should_ be the same thing. But we can perhaps ignore that because:\n\n 1. That might never happen, because it breaks historical usage. IOW,\n it changes the meaning of \"git log -S HEAD\" to something else.\n\n 2. If it does happen, it is likely to be in a transition to parseopt,\n which would fall under the case mentioned above.\n\nI think the biggest danger is that it is a potential bomb for somebody\nto add a new revision option which takes an arbitrary string. They\nwould probably need to keep it as \"--option=\" only.\n\n-Peff\n"},{"id":"111460","messageId":"7v4owok0bh.fsf@gitster.siamese.dyndns.org","threadId":"17509","inReplyTo":"20090416094154.GA30479@coredump.intra.peff.net","subject":"Re: [PATCH] Add the diff option --no-defaults","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-04-16T16:52:50Z","receivedAt":"2009-04-16T16:52:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Apr 16, 2009 at 11:25:08AM +0200, Johannes Schindelin wrote:\n>\n>> > This feels very hack-ish to me, but perhaps this is a case of \"perfect\n>> > is the enemy of the good\".\n>> \n>> I have a strong feeling that none of our diff/rev options can sanely take \n>> a parameter looking like \"--defaults\" or \"--no-defaults\".\n>> \n>> But I do not have the time to audit the options. Maybe you have?\n>\n> Right now, I think we are safe. A few options like \"--default\" do take a\n> separated string argument, but saying \"--default --no-defaults\" seems a\n> little crazy to me (besides being confusing because they are talking\n> about two totally unrelated defaults).\n\nMaybe you guys have already considered and discarded this as too hacky,\nbut isn't it the easiest to explain and code to declare --no-defaults is\nacceptable only at the beginning?\n"},{"id":"111463","messageId":"alpine.DEB.1.00.0904161935570.6798@intel-tinevez-2-302","threadId":"17509","inReplyTo":"7v4owok0bh.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Add the diff option --no-defaults","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-04-16T17:36:52Z","receivedAt":"2009-04-16T17:36:52Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 16 Apr 2009, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > On Thu, Apr 16, 2009 at 11:25:08AM +0200, Johannes Schindelin wrote:\n> >\n> >> > This feels very hack-ish to me, but perhaps this is a case of \"perfect\n> >> > is the enemy of the good\".\n> >> \n> >> I have a strong feeling that none of our diff/rev options can sanely take \n> >> a parameter looking like \"--defaults\" or \"--no-defaults\".\n> >> \n> >> But I do not have the time to audit the options. Maybe you have?\n> >\n> > Right now, I think we are safe. A few options like \"--default\" do take a\n> > separated string argument, but saying \"--default --no-defaults\" seems a\n> > little crazy to me (besides being confusing because they are talking\n> > about two totally unrelated defaults).\n> \n> Maybe you guys have already considered and discarded this as too hacky,\n> but isn't it the easiest to explain and code to declare --no-defaults is\n> acceptable only at the beginning?\n\nThat would not work if you use an alias:\n\n\t$ git config alias.grmpfl log --stat\n\t$ git grmpfl --no-defaults\n\nCiao,\nDscho\n"},{"id":"111480","messageId":"20090417115414.GA29121@coredump.intra.peff.net","threadId":"17509","inReplyTo":"7v4owok0bh.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Add the diff option --no-defaults","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-04-17T11:54:14Z","receivedAt":"2009-04-17T11:54:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 16, 2009 at 09:52:50AM -0700, Junio C Hamano wrote:\n\n> > Right now, I think we are safe. A few options like \"--default\" do take a\n> > separated string argument, but saying \"--default --no-defaults\" seems a\n> > little crazy to me (besides being confusing because they are talking\n> > about two totally unrelated defaults).\n> \n> Maybe you guys have already considered and discarded this as too hacky,\n> but isn't it the easiest to explain and code to declare --no-defaults is\n> acceptable only at the beginning?\n\nI discarded that as \"too hacky\". If I had to choose my poison between\n\"insane string options don't work\" and \"option must inexplicably be at\nthe front\", I think I take the former. It is perhaps a more difficult\nrule to realize you are triggering, but it is much less likely to come\nup in practice.\n\nBut I think all of this is just ending up in the same place that Keith\nand I arrived at much earlier in the thread: you _are_ choosing a\npoison, and his patch was meant to avoid that. The question is whether\nthe added code complexity is worth it.\n\n-Peff\n"},{"id":"111486","messageId":"alpine.DEB.1.00.0904171514440.6675@intel-tinevez-2-302","threadId":"17509","inReplyTo":"20090417115414.GA29121@coredump.intra.peff.net","subject":"Re: [PATCH] Add the diff option --no-defaults","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-04-17T13:15:44Z","receivedAt":"2009-04-17T13:15:44Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 17 Apr 2009, Jeff King wrote:\n\n> But I think all of this is just ending up in the same place that Keith \n> and I arrived at much earlier in the thread: you _are_ choosing a \n> poison, and his patch was meant to avoid that. The question is whether \n> the added code complexity is worth it.\n\nWell, I think I gave my answer in the form of two patches.\n\nBesides, you still will have a poison:\n\n\tgit config diff.defaultOptions --no-defaults\n\nwhich is Russel's paradoxon right there.\n\nCiao,\nDscho\n"},{"id":"111590","messageId":"alpine.GSO.2.00.0904180930390.16775@kiwi.cs.ucla.edu","threadId":"17509","inReplyTo":"alpine.DEB.1.00.0904171514440.6675@intel-tinevez-2-302","subject":"Re: [PATCH] Add the diff option --no-defaults","fromName":"Keith Cascio","fromEmail":"keith@cs.ucla.edu","sentAt":"2009-04-18T16:41:01Z","receivedAt":"2009-04-18T16:41:01Z","isPatch":true,"sender":{"key":"keith@cs.ucla.edu","avatar":"https://gravatar.com/avatar/c5ec3a8f1cd1f449fdf8bdb7125fdbfd10b729507f32cbf0aa4ad07b4f7127ae?d=mp&s=160"},"body":"Dscho,\n\nOn Fri, 17 Apr 2009, Johannes Schindelin wrote:\n\n> Besides, you still will have a poison:\n> \n> \tgit config diff.defaultOptions --no-defaults\n> \n> which is Russel's paradoxon right there.\n\nI can cleanly modify my v3 to handle this case. In diff_setup_done(), change \nthis:\n\n+\tif (DIFF_OPT_TST(options, ALLOW_DEFAULT_OPTIONS))\n+\t\tflatten_diff_options(options, defaults ? defaults :\n+\t\t\tparse_diff_defaults(diff_setup(defaults =\n+\t\t\t\txmalloc(sizeof(struct diff_options)))));\n\nto this:\n\n+\tif (DIFF_OPT_TST(options, ALLOW_DEFAULT_OPTIONS) && (defaults ||\n+\t\tparse_diff_defaults(diff_setup(defaults = xmalloc(\n+\t\t\tsizeof(struct diff_options))))) && DIFF_OPT_TST(\n+\t\t\t\tdefaults, ALLOW_DEFAULT_OPTIONS))\n+\t\t\t\t\tflatten_diff_options(options,\n+\t\t\t\t\t\tdefaults);\n\nAll I did there was add the test DIFF_OPT_TST(defaults, ALLOW_DEFAULT_OPTIONS) \nto the condition that controls whether to perform the flattening. Clean and \nclear.\n -- Keith\n"},{"id":"111605","messageId":"alpine.DEB.1.00.0904181937520.10279@pacific.mpi-cbg.de","threadId":"17509","inReplyTo":"alpine.GSO.2.00.0904180930390.16775@kiwi.cs.ucla.edu","subject":"Re: [PATCH] Add the diff option --no-defaults","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-04-18T17:40:15Z","receivedAt":"2009-04-18T17:40:15Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Keith,\n\nOn Sat, 18 Apr 2009, Keith Cascio wrote:\n\n> On Fri, 17 Apr 2009, Johannes Schindelin wrote:\n> \n> > Besides, you still will have a poison:\n> > \n> > \tgit config diff.defaultOptions --no-defaults\n> > \n> > which is Russel's paradoxon right there.\n> \n> I can cleanly modify my v3 to handle this case.\n\nYou cannot. --no-defaults means that diff.defaultOptions should be \ndisregarded. If the diff.defaultOptions say that they should be \ndisregarded themselves, then --no-defaults should be disregarded.\n\nAnd I still do not like the intrusiveness of your patch. The last time we \ndid something like that with options (some parseoptifications), we had a \nlot of fallout as a consequence.\n\nCiao,\nDscho\n"},{"id":"111613","messageId":"alpine.GSO.2.00.0904181308440.16775@kiwi.cs.ucla.edu","threadId":"17509","inReplyTo":"alpine.DEB.1.00.0904181937520.10279@pacific.mpi-cbg.de","subject":"Re: [PATCH] Add the diff option --no-defaults","fromName":"Keith Cascio","fromEmail":"keith@cs.ucla.edu","sentAt":"2009-04-18T20:32:41Z","receivedAt":"2009-04-18T20:32:41Z","isPatch":true,"sender":{"key":"keith@cs.ucla.edu","avatar":"https://gravatar.com/avatar/c5ec3a8f1cd1f449fdf8bdb7125fdbfd10b729507f32cbf0aa4ad07b4f7127ae?d=mp&s=160"},"body":"Dscho,\n\nOn Sat, 18 Apr 2009, Johannes Schindelin wrote:\n\n> On Sat, 18 Apr 2009, Keith Cascio wrote:\n> \n> > On Fri, 17 Apr 2009, Johannes Schindelin wrote:\n> > \n> > > Besides, you still will have a poison:\n> > > \tgit config diff.defaultOptions --no-defaults\n> > > which is Russel's paradoxon right there.\n> > \n> > I can cleanly modify my v3 to handle this case.\n> \n> You cannot. --no-defaults means that diff.defaultOptions should be \n> disregarded. If the diff.defaultOptions say that they should be disregarded \n> themselves, then --no-defaults should be disregarded.\n\n--no-defaults *could* mean as you say there. But a much better meaning for \n--no-defaults is: suppress the values in diff.defaultOptions after options \nprocessing. We don't have to disregard them, just suppress them after options \nprocessing. In that sense, --no-defaults is a meta-option. It is an option \nabout options. Even users unfamiliar with set theory would assume the \nsuppression semantics.\n\nNevertheless I applaud the Russell reference. Very intriguing.\n\n> And I still do not like the intrusiveness of your patch. The last time we did \n> something like that with options (some parseoptifications), we had a lot of \n> fallout as a consequence.\n\nA reasonable worry! But blind paranoia is paralyzing. Peff expressed some \nspecific concerns which he and I addressed: (1) whether I'd investigated all \ncallsites for possible problems (yes I did), (2) whether we'd have to switch \nevery callsite to a macro, rather than direct assignment (no we don't). \nOutside of diff.h/diff.c, my v3 deletes no lines and adds only two. That \ndoesn't really seem \"intrusive\" to me. By comparison, your patch adds at least \nten lines outside of diff.h/diff.c. I'd rather call my patch \"innovative\". \nPossible?\n -- Keith\n"},{"id":"111617","messageId":"alpine.DEB.1.00.0904182311330.10279@pacific.mpi-cbg.de","threadId":"17509","inReplyTo":"alpine.GSO.2.00.0904181308440.16775@kiwi.cs.ucla.edu","subject":"Re: [PATCH] Add the diff option --no-defaults","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-04-18T21:15:05Z","receivedAt":"2009-04-18T21:15:05Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi.\n\nOn Sat, 18 Apr 2009, Keith Cascio wrote:\n\n> On Sat, 18 Apr 2009, Johannes Schindelin wrote:\n> \n> > And I still do not like the intrusiveness of your patch. The last \n> > time we did something like that with options (some \n> > parseoptifications), we had a lot of fallout as a consequence.\n> \n> [...]\n>\n> Peff expressed some specific concerns [...]\n\nMy concerns were also very specific: your patches are way too large. \nThere is a rule of thumb that the likelihood of a bug is the square of the \nnumber of changed lines, I am sure you heard that before.\n\n> [...] your patch adds at least ten lines outside of diff.h/diff.c.\n\nThat is _such_ a red herring.\n\nCiao,\nDscho\n"}]}