* [PATCH] added git-config support for diff.relative setting @ 2014-12-11 7:28 Kelson 2014-12-11 13:37 ` Duy Nguyen 2014-12-12 23:25 ` [PATCH v2] " Kelson 0 siblings, 2 replies; 13+ messages in thread From: Kelson @ 2014-12-11 7:28 UTC (permalink / raw) To: git By default, git-diff shows changes and pathnames relative to the repository root. Setting the diff.relative config option to "true" shows pathnames relative to the current directory and excludes changes outside this directory. --- Documentation/diff-config.txt | 6 ++++++ diff.c | 8 ++++++++ t/t4045-diff-relative.sh | 21 +++++++++++++++++++++ 3 files changed, 35 insertions(+) diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt index b001779..10f183f 100644 --- a/Documentation/diff-config.txt +++ b/Documentation/diff-config.txt @@ -182,3 +182,9 @@ diff.algorithm:: low-occurrence common elements". -- + + +diff.relative:: + By default, linkgit:git-diff[1] shows changes and pathnames + relative to the repository root. Setting this variable to + `true` shows pathnames relative to the current directory and + excludes changes outside this directory. diff --git a/diff.c b/diff.c index d1bd534..22daa2f 100644 --- a/diff.c +++ b/diff.c @@ -270,6 +270,14 @@ int git_diff_basic_config(const char *var, const char *value, void *cb) return 0; } + if (!strcmp(var, "diff.relative")) { + if (git_config_bool(var, value)) + DIFF_OPT_SET(&default_diff_options, RELATIVE_NAME); + else + DIFF_OPT_CLR(&default_diff_options, RELATIVE_NAME); + return 0; + } + if (starts_with(var, "submodule.")) return parse_submodule_config_option(var, value); diff --git a/t/t4045-diff-relative.sh b/t/t4045-diff-relative.sh index 3950f50..8c8fe0b 100755 --- a/t/t4045-diff-relative.sh +++ b/t/t4045-diff-relative.sh @@ -29,6 +29,23 @@ test_expect_success "-p $*" " " } +check_config() { +expect=$1; shift +cat >expected <<EOF +diff --git a/$expect b/$expect +new file mode 100644 +index 0000000..25c05ef +--- /dev/null ++++ b/$expect +@@ -0,0 +1 @@ ++other content +EOF +test_expect_success "git-config diff.relative=true in $1" " + (cd $1; git -c diff.relative=true diff -p HEAD^ >../actual) && + test_cmp expected actual +" +} + check_numstat() { expect=$1; shift cat >expected <<EOF @@ -69,5 +86,9 @@ for type in diff numstat stat raw; do check_$type file2 --relative=subdir check_$type dir/file2 --relative=sub done +for type in config; do + check_$type file2 subdir/ + check_$type file2 subdir +done test_done -- 1.9.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] added git-config support for diff.relative setting 2014-12-11 7:28 [PATCH] added git-config support for diff.relative setting Kelson @ 2014-12-11 13:37 ` Duy Nguyen 2014-12-11 21:41 ` Kelson 2014-12-12 23:25 ` [PATCH v2] " Kelson 1 sibling, 1 reply; 13+ messages in thread From: Duy Nguyen @ 2014-12-11 13:37 UTC (permalink / raw) To: Kelson; +Cc: Git Mailing List On Thu, Dec 11, 2014 at 2:28 PM, Kelson <kelson@shysecurity.com> wrote: > @@ -270,6 +270,14 @@ int git_diff_basic_config(const char *var, const char > *value, void *cb) > return 0; > } > > + if (!strcmp(var, "diff.relative")) { > + if (git_config_bool(var, value)) > + DIFF_OPT_SET(&default_diff_options, RELATIVE_NAME); > + else > + DIFF_OPT_CLR(&default_diff_options, RELATIVE_NAME); > + return 0; > + } > + > if (starts_with(var, "submodule.")) > return parse_submodule_config_option(var, value); > This affects more than just git-diff. git_diff_ui_config() may be a better place. -- Duy ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] added git-config support for diff.relative setting 2014-12-11 13:37 ` Duy Nguyen @ 2014-12-11 21:41 ` Kelson 0 siblings, 0 replies; 13+ messages in thread From: Kelson @ 2014-12-11 21:41 UTC (permalink / raw) To: Duy Nguyen; +Cc: Git Mailing List That is quite manageable. I was concerned that --relative changes the UI (relative paths) and behavior (excluding files outside the current directory), which might not be clear if placed in just the UI component. You make a great point that git_diff_basic_config drives other commands though, like git-bisect, which --relative would not effect. -----Original Message----- From: Duy Nguyen <pclouds@gmail.com> Sent: 12/11/2014 08:37 AM To: Kelson <kelson@shysecurity.com> CC: Git Mailing List <git@vger.kernel.org> Subject: Re: [PATCH] added git-config support for diff.relative setting On Thu, Dec 11, 2014 at 2:28 PM, Kelson <kelson@shysecurity.com> wrote: > @@ -270,6 +270,14 @@ int git_diff_basic_config(const char *var, const char > *value, void *cb) > return 0; > } > > + if (!strcmp(var, "diff.relative")) { > + if (git_config_bool(var, value)) > + DIFF_OPT_SET(&default_diff_options, RELATIVE_NAME); > + else > + DIFF_OPT_CLR(&default_diff_options, RELATIVE_NAME); > + return 0; > + } > + > if (starts_with(var, "submodule.")) > return parse_submodule_config_option(var, value); > This affects more than just git-diff. git_diff_ui_config() may be a better place. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2] added git-config support for diff.relative setting 2014-12-11 7:28 [PATCH] added git-config support for diff.relative setting Kelson 2014-12-11 13:37 ` Duy Nguyen @ 2014-12-12 23:25 ` Kelson 2014-12-21 20:23 ` [PATCH v3 1/2] " kelson 1 sibling, 1 reply; 13+ messages in thread From: Kelson @ 2014-12-12 23:25 UTC (permalink / raw) To: git By default, git-diff shows changes and pathnames relative to the repository root. Setting the diff.relative config option to "true" shows pathnames relative to the current directory and excludes changes outside this directory. Signed-off-by: Brandon Phillips <kelson@shysecurity.com> --- Documentation/diff-config.txt | 6 ++++++ diff.c | 8 ++++++++ t/t4045-diff-relative.sh | 21 +++++++++++++++++++++ 3 files changed, 35 insertions(+) diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt index b001779..10f183f 100644 --- a/Documentation/diff-config.txt +++ b/Documentation/diff-config.txt @@ -182,3 +182,9 @@ diff.algorithm:: low-occurrence common elements". -- + + +diff.relative:: + By default, linkgit:git-diff[1] shows changes and pathnames + relative to the repository root. Setting this variable to + `true` shows pathnames relative to the current directory and + excludes changes outside this directory. diff --git a/diff.c b/diff.c index d1bd534..03697a9 100644 --- a/diff.c +++ b/diff.c @@ -223,6 +223,14 @@ int git_diff_ui_config(const char *var, const char *value, void *cb) return 0; } + if (!strcmp(var, "diff.relative")) { + if (git_config_bool(var, value)) + DIFF_OPT_SET(&default_diff_options, RELATIVE_NAME); + else + DIFF_OPT_CLR(&default_diff_options, RELATIVE_NAME); + return 0; + } + if (git_color_config(var, value, cb) < 0) return -1; diff --git a/t/t4045-diff-relative.sh b/t/t4045-diff-relative.sh index 3950f50..8c8fe0b 100755 --- a/t/t4045-diff-relative.sh +++ b/t/t4045-diff-relative.sh @@ -29,6 +29,23 @@ test_expect_success "-p $*" " " } +check_config() { +expect=$1; shift +cat >expected <<EOF +diff --git a/$expect b/$expect +new file mode 100644 +index 0000000..25c05ef +--- /dev/null ++++ b/$expect +@@ -0,0 +1 @@ ++other content +EOF +test_expect_success "git-config diff.relative=true in $1" " + (cd $1; git -c diff.relative=true diff -p HEAD^ >../actual) && + test_cmp expected actual +" +} + check_numstat() { expect=$1; shift cat >expected <<EOF @@ -69,5 +86,9 @@ for type in diff numstat stat raw; do check_$type file2 --relative=subdir check_$type dir/file2 --relative=sub done +for type in config; do + check_$type file2 subdir/ + check_$type file2 subdir +done test_done -- 1.9.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v3 1/2] git-config support for diff.relative setting 2014-12-12 23:25 ` [PATCH v2] " Kelson @ 2014-12-21 20:23 ` kelson 2014-12-30 17:56 ` kelson 0 siblings, 1 reply; 13+ messages in thread From: kelson @ 2014-12-21 20:23 UTC (permalink / raw) To: Git Mailing List Cc: Junio C Hamano, Philip Oakley, Duy Nguyen, Jonathan Nieder By default, git-diff shows changes and pathnames relative to the repository root. Setting diff.relative to "true" shows pathnames relative to the current directory and excludes changes outside this directory. --- Documentation/diff-config.txt | 5 +++++ diff.c | 8 ++++++++ t/t4045-diff-relative.sh | 21 +++++++++++++++++++++ 3 files changed, 34 insertions(+) diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt index b001779..496e9b0 100644 --- a/Documentation/diff-config.txt +++ b/Documentation/diff-config.txt @@ -103,6 +103,11 @@ diff.orderfile:: one shell glob pattern per line. Can be overridden by the '-O' option to linkgit:git-diff[1]. +diff.relative:: + Show pathnames relative to the current directory and exclude + changes outside this directory; equivalent to the 'git diff' + option '--relative'. + diff.renameLimit:: The number of files to consider when performing the copy/rename detection; equivalent to the 'git diff' option '-l'. diff --git a/diff.c b/diff.c index d1bd534..03697a9 100644 --- a/diff.c +++ b/diff.c @@ -223,6 +223,14 @@ int git_diff_ui_config(const char *var, const char *value, void *cb) return 0; } + if (!strcmp(var, "diff.relative")) { + if (git_config_bool(var, value)) + DIFF_OPT_SET(&default_diff_options, RELATIVE_NAME); + else + DIFF_OPT_CLR(&default_diff_options, RELATIVE_NAME); + return 0; + } + if (git_color_config(var, value, cb) < 0) return -1; diff --git a/t/t4045-diff-relative.sh b/t/t4045-diff-relative.sh index 3950f50..8c8fe0b 100755 --- a/t/t4045-diff-relative.sh +++ b/t/t4045-diff-relative.sh @@ -29,6 +29,23 @@ test_expect_success "-p $*" " " } +check_config() { +expect=$1; shift +cat >expected <<EOF +diff --git a/$expect b/$expect +new file mode 100644 +index 0000000..25c05ef +--- /dev/null ++++ b/$expect +@@ -0,0 +1 @@ ++other content +EOF +test_expect_success "git-config diff.relative=true in $1" " + (cd $1; git -c diff.relative=true diff -p HEAD^ >../actual) && + test_cmp expected actual +" +} + check_numstat() { expect=$1; shift cat >expected <<EOF @@ -69,5 +86,9 @@ for type in diff numstat stat raw; do check_$type file2 --relative=subdir check_$type dir/file2 --relative=sub done +for type in config; do + check_$type file2 subdir/ + check_$type file2 subdir +done test_done -- 1.9.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v3 1/2] git-config support for diff.relative setting 2014-12-21 20:23 ` [PATCH v3 1/2] " kelson @ 2014-12-30 17:56 ` kelson 2014-12-30 18:16 ` Junio C Hamano 2014-12-30 19:32 ` [PATCH 1/2] support for --no-relative and diff.relative kelson 0 siblings, 2 replies; 13+ messages in thread From: kelson @ 2014-12-30 17:56 UTC (permalink / raw) To: Git Mailing List Cc: Junio C Hamano, Philip Oakley, Duy Nguyen, Jonathan Nieder By default, git-diff shows changes and pathnames relative to the repository root. Setting diff.relative to "true" shows pathnames relative to the current directory and excludes changes outside this directory. --- Documentation/diff-config.txt | 5 +++++ diff.c | 8 ++++++++ t/t4045-diff-relative.sh | 21 +++++++++++++++++++++ 3 files changed, 34 insertions(+) diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt index b001779..496e9b0 100644 --- a/Documentation/diff-config.txt +++ b/Documentation/diff-config.txt @@ -103,6 +103,11 @@ diff.orderfile:: one shell glob pattern per line. Can be overridden by the '-O' option to linkgit:git-diff[1]. +diff.relative:: + Show pathnames relative to the current directory and exclude + changes outside this directory; equivalent to the 'git diff' + option '--relative'. + diff.renameLimit:: The number of files to consider when performing the copy/rename detection; equivalent to the 'git diff' option '-l'. diff --git a/diff.c b/diff.c index d1bd534..03697a9 100644 --- a/diff.c +++ b/diff.c @@ -223,6 +223,14 @@ int git_diff_ui_config(const char *var, const char *value, void *cb) return 0; } + if (!strcmp(var, "diff.relative")) { + if (git_config_bool(var, value)) + DIFF_OPT_SET(&default_diff_options, RELATIVE_NAME); + else + DIFF_OPT_CLR(&default_diff_options, RELATIVE_NAME); + return 0; + } + if (git_color_config(var, value, cb) < 0) return -1; diff --git a/t/t4045-diff-relative.sh b/t/t4045-diff-relative.sh index 3950f50..8c8fe0b 100755 --- a/t/t4045-diff-relative.sh +++ b/t/t4045-diff-relative.sh @@ -29,6 +29,23 @@ test_expect_success "-p $*" " " } +check_config() { +expect=$1; shift +cat >expected <<EOF +diff --git a/$expect b/$expect +new file mode 100644 +index 0000000..25c05ef +--- /dev/null ++++ b/$expect +@@ -0,0 +1 @@ ++other content +EOF +test_expect_success "git-config diff.relative=true in $1" " + (cd $1; git -c diff.relative=true diff -p HEAD^ >../actual) && + test_cmp expected actual +" +} + check_numstat() { expect=$1; shift cat >expected <<EOF @@ -69,5 +86,9 @@ for type in diff numstat stat raw; do check_$type file2 --relative=subdir check_$type dir/file2 --relative=sub done +for type in config; do + check_$type file2 subdir/ + check_$type file2 subdir +done test_done -- 1.9.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v3 1/2] git-config support for diff.relative setting 2014-12-30 17:56 ` kelson @ 2014-12-30 18:16 ` Junio C Hamano 2014-12-30 19:32 ` [PATCH 1/2] support for --no-relative and diff.relative kelson 1 sibling, 0 replies; 13+ messages in thread From: Junio C Hamano @ 2014-12-30 18:16 UTC (permalink / raw) To: kelson; +Cc: Git Mailing List, Philip Oakley, Duy Nguyen, Jonathan Nieder kelson@shysecurity.com writes: > By default, git-diff shows changes and pathnames relative to the > repository root. > Setting diff.relative to "true" shows pathnames relative to the > current directory > and excludes changes outside this directory. The above does not tell any lie, but it is mostly a description of what "--relative" does. I think the main point of this change is that by configuring a variable "diff.relative" (and forget about it) you do not have to keep saying "--relative" from the command line, so that is how you should "sell" this change to others, I think. > --- No signoff? > Documentation/diff-config.txt | 5 +++++ > diff.c | 8 ++++++++ > t/t4045-diff-relative.sh | 21 +++++++++++++++++++++ > 3 files changed, 34 insertions(+) > > diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt > index b001779..496e9b0 100644 > --- a/Documentation/diff-config.txt > +++ b/Documentation/diff-config.txt > @@ -103,6 +103,11 @@ diff.orderfile:: > one shell glob pattern per line. > Can be overridden by the '-O' option to linkgit:git-diff[1]. Whitespace damaged patch? HTs are all gone. Please practice first by sending your patches to yourself, and then try to "git am" them by saving the e-mail messages you received from yourself. "git grep -i thunder Documentation" seem to tell me that there are hints on using Thunderbird in SubmittingPatches and format-patch documentation. I might have said this already, but I think this change, without support for "--no-relative", would be an incomplete one that can break the end-user experience, and it would be better to swap the order of the patches. Thanks. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/2] support for --no-relative and diff.relative 2014-12-30 17:56 ` kelson 2014-12-30 18:16 ` Junio C Hamano @ 2014-12-30 19:32 ` kelson 2015-01-06 16:19 ` kelson 1 sibling, 1 reply; 13+ messages in thread From: kelson @ 2014-12-30 19:32 UTC (permalink / raw) To: Git Mailing List Cc: Junio C Hamano, Philip Oakley, Duy Nguyen, Jonathan Nieder added --no-relative option for git-diff (code, documentation, and tests) --no-relative overrides --relative causing a return to standard behavior Signed-off-by: Brandon Phillips <kelson@shysecurity.com> --- Documentation/diff-options.txt | 4 ++++ diff.c | 2 ++ t/t4045-diff-relative.sh | 46 +++++++++++++++++++++++++++++++++++++++--- 3 files changed, 49 insertions(+), 3 deletions(-) diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt index 6cb083a..2b15050 100644 --- a/Documentation/diff-options.txt +++ b/Documentation/diff-options.txt @@ -448,6 +448,10 @@ ifndef::git-format-patch[] not in a subdirectory (e.g. in a bare repository), you can name which subdirectory to make the output relative to by giving a <path> as an argument. + +--no-relative:: + Turn off relative pathnames and include all changes in the + repository. endif::git-format-patch[] -a:: diff --git a/diff.c b/diff.c index d1bd534..7bceba8 100644 --- a/diff.c +++ b/diff.c @@ -3695,6 +3695,8 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac) DIFF_OPT_SET(options, RELATIVE_NAME); options->prefix = arg; } + else if (!strcmp(arg, "--no-relative")) + DIFF_OPT_CLR(options, RELATIVE_NAME); /* xdiff options */ else if (!strcmp(arg, "--minimal")) diff --git a/t/t4045-diff-relative.sh b/t/t4045-diff-relative.sh index 3950f50..ccd67c7 100755 --- a/t/t4045-diff-relative.sh +++ b/t/t4045-diff-relative.sh @@ -12,8 +12,8 @@ test_expect_success 'setup' ' git commit -m one ' -check_diff() { -expect=$1; shift +store_diff_relative() { +expect=$1; cat >expected <<EOF diff --git a/$expect b/$expect new file mode 100644 @@ -23,12 +23,52 @@ index 0000000..25c05ef @@ -0,0 +1 @@ +other content EOF +} + +store_diff_absolute() { +expect=$1; +cat >expected <<EOF +diff --git a/file1 b/file1 +new file mode 100644 +index 0000000..d95f3ad +--- /dev/null ++++ b/file1 +@@ -0,0 +1 @@ ++content +diff --git a/subdir/file2 b/subdir/file2 +new file mode 100644 +index 0000000..25c05ef +--- /dev/null ++++ b/subdir/file2 +@@ -0,0 +1 @@ ++other content +EOF +} + +check_diff() { +store_diff_relative $1; shift test_expect_success "-p $*" " git diff -p $* HEAD^ >actual && test_cmp expected actual " } +check_norel_pre() { +store_diff_relative $1; shift +test_expect_success "-p --no-relative $*" " + git diff -p --no-relative $* HEAD^ >actual && + test_cmp expected actual +" +} + +check_norel_post() { +store_diff_absolute $1; shift +test_expect_success "-p $* --no-relative" " + git diff -p $* --no-relative HEAD^ >actual && + test_cmp expected actual +" +} + check_numstat() { expect=$1; shift cat >expected <<EOF @@ -64,7 +104,7 @@ test_expect_success "--raw $*" " " } -for type in diff numstat stat raw; do +for type in diff numstat stat raw norel_pre norel_post; do check_$type file2 --relative=subdir/ check_$type file2 --relative=subdir check_$type dir/file2 --relative=sub -- 1.9.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 1/2] support for --no-relative and diff.relative 2014-12-30 19:32 ` [PATCH 1/2] support for --no-relative and diff.relative kelson @ 2015-01-06 16:19 ` kelson 2015-01-07 18:09 ` Junio C Hamano 0 siblings, 1 reply; 13+ messages in thread From: kelson @ 2015-01-06 16:19 UTC (permalink / raw) To: Git Mailing List Cc: Junio C Hamano, Philip Oakley, Duy Nguyen, Jonathan Nieder added --no-relative option for git-diff (code, documentation, and tests) --no-relative overrides --relative causing a return to standard behavior Signed-off-by: Brandon Phillips <kelson@shysecurity.com> --- Documentation/diff-options.txt | 4 ++++ diff.c | 2 ++ t/t4045-diff-relative.sh | 46 +++++++++++++++++++++++++++++++++++++++--- 3 files changed, 49 insertions(+), 3 deletions(-) diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt index 6cb083a..2b15050 100644 --- a/Documentation/diff-options.txt +++ b/Documentation/diff-options.txt @@ -448,6 +448,10 @@ ifndef::git-format-patch[] not in a subdirectory (e.g. in a bare repository), you can name which subdirectory to make the output relative to by giving a <path> as an argument. + +--no-relative:: + Turn off relative pathnames and include all changes in the + repository. endif::git-format-patch[] -a:: diff --git a/diff.c b/diff.c index d1bd534..7bceba8 100644 --- a/diff.c +++ b/diff.c @@ -3695,6 +3695,8 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac) DIFF_OPT_SET(options, RELATIVE_NAME); options->prefix = arg; } + else if (!strcmp(arg, "--no-relative")) + DIFF_OPT_CLR(options, RELATIVE_NAME); /* xdiff options */ else if (!strcmp(arg, "--minimal")) diff --git a/t/t4045-diff-relative.sh b/t/t4045-diff-relative.sh index 3950f50..ccd67c7 100755 --- a/t/t4045-diff-relative.sh +++ b/t/t4045-diff-relative.sh @@ -12,8 +12,8 @@ test_expect_success 'setup' ' git commit -m one ' -check_diff() { -expect=$1; shift +store_diff_relative() { +expect=$1; cat >expected <<EOF diff --git a/$expect b/$expect new file mode 100644 @@ -23,12 +23,52 @@ index 0000000..25c05ef @@ -0,0 +1 @@ +other content EOF +} + +store_diff_absolute() { +expect=$1; +cat >expected <<EOF +diff --git a/file1 b/file1 +new file mode 100644 +index 0000000..d95f3ad +--- /dev/null ++++ b/file1 +@@ -0,0 +1 @@ ++content +diff --git a/subdir/file2 b/subdir/file2 +new file mode 100644 +index 0000000..25c05ef +--- /dev/null ++++ b/subdir/file2 +@@ -0,0 +1 @@ ++other content +EOF +} + +check_diff() { +store_diff_relative $1; shift test_expect_success "-p $*" " git diff -p $* HEAD^ >actual && test_cmp expected actual " } +check_norel_pre() { +store_diff_relative $1; shift +test_expect_success "-p --no-relative $*" " + git diff -p --no-relative $* HEAD^ >actual && + test_cmp expected actual +" +} + +check_norel_post() { +store_diff_absolute $1; shift +test_expect_success "-p $* --no-relative" " + git diff -p $* --no-relative HEAD^ >actual && + test_cmp expected actual +" +} + check_numstat() { expect=$1; shift cat >expected <<EOF @@ -64,7 +104,7 @@ test_expect_success "--raw $*" " " } -for type in diff numstat stat raw; do +for type in diff numstat stat raw norel_pre norel_post; do check_$type file2 --relative=subdir/ check_$type file2 --relative=subdir check_$type dir/file2 --relative=sub -- 1.9.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] support for --no-relative and diff.relative 2015-01-06 16:19 ` kelson @ 2015-01-07 18:09 ` Junio C Hamano 2015-01-07 18:46 ` kelson 2015-01-07 19:02 ` Junio C Hamano 0 siblings, 2 replies; 13+ messages in thread From: Junio C Hamano @ 2015-01-07 18:09 UTC (permalink / raw) To: kelson; +Cc: Git Mailing List, Philip Oakley, Duy Nguyen, Jonathan Nieder kelson@shysecurity.com writes: > Content-Type: text/plain; charset=utf-8; format=flowed Please. No format=flawed. Really. > Subject: Re: [PATCH 1/2] support for --no-relative and diff.relative "diff: teach --no-relative to override earlier --relative" or something. Saying the affeced area upfront, terminated with a colon ':', will make it easier to spot in "git shortlog" output later. Also this step is not about --no-relative and diff.relative but is only about --no-relative option. > added --no-relative option for git-diff (code, documentation, and tests) "Add --no-relative option..."; we write in imperative, as if we are giving an order to the project secretary to "make the code do/be so". > --no-relative overrides --relative causing a return to standard behavior OK (modulo missing full-stop). > > Signed-off-by: Brandon Phillips <kelson@shysecurity.com> Please also have From: Brandon Phillips <kelson@shysecurity.com> as the first line of the body of your e-mail message, if you are letting your MUA only give your e-mail address without name. Alternatively, please ask/configure your MUA to put your name as well as your address on From: header of the e-mail (which is preferrable). > diff --git a/diff.c b/diff.c > index d1bd534..7bceba8 100644 > --- a/diff.c > +++ b/diff.c > @@ -3695,6 +3695,8 @@ int diff_opt_parse(struct diff_options *options, > const char **av, int ac) Line-wrapped. > DIFF_OPT_SET(options, RELATIVE_NAME); > options->prefix = arg; > } > + else if (!strcmp(arg, "--no-relative")) > + DIFF_OPT_CLR(options, RELATIVE_NAME); > When "--relative" is given, options->prefix is set to something as we can see above. The --relative option is described as optionally taking <path> in the doc: --relative[=<path>]:: When run from a subdirectory of the project, it can be told to exclude changes outside the directory and show pathnames relative to it with this option. When you are not in a subdirectory (e.g. in a bare repository), you can name which subdirectory to make the output relative to by giving a <path> as an argument. Doesn't "--no-relative" codepath have to undo the effect of that assignment to options->prefix? For example, after applying this patch, shouldn't $ cd t $ git show --relative=Documentation --no-relative --relative work the same way as $ cd t $ git show --relative i.e. limiting its output to the changes in the 't/' directory and not to the changes in the 'Documentation/' directory? Patch 2/2 also seems to share similar line-wrapping breakages that make it unappliable, but more importantly, the configuration that is supposed to correspond to --relative option only parses a boolean. Is that the right design, or should it also be able to substitute a command line `--relative=<path>` with an argument? The last was a half-way rhetorical question and my answer is that boolean-only is the best you could do, because we cannot do the usual "bool or string" trick when "string" can be arbitrary. In other words, "diff.relative=true" could mean "limit to the current subdirectory" aka "--relative" or it could mean "limit to true/ subdirectory" aka "--relative=true", and there is no good way to disambiguate between the two [*1*]. So I can agree with the design decision but only after spending 6 lines to think about it. For the end-users, this design decision needs to be explained and spelled out in the documentation. Saying "equivalent to `--relative`" is not sufficient, because the way `--relative` option itself is described elsewhere. The option appears as `--relative[=<path>]` (see above), so some people _will_ read "equivalent to `--relative`" to mean "Setting diff.relative=t should be equivalent to --relative=t", which is not what actually happens. [Footnote] *1* Actually, you could declare that "diff.relative=true/" means the 'true/' directory while "diff.relative=true" means the boolean 'true' aka 'diff --relative', but I think it is too confusing. Let's not make it worse by going that route. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] support for --no-relative and diff.relative 2015-01-07 18:09 ` Junio C Hamano @ 2015-01-07 18:46 ` kelson 2015-01-07 20:26 ` Junio C Hamano 2015-01-07 19:02 ` Junio C Hamano 1 sibling, 1 reply; 13+ messages in thread From: kelson @ 2015-01-07 18:46 UTC (permalink / raw) To: Junio C Hamano Cc: Git Mailing List, Philip Oakley, Duy Nguyen, Jonathan Nieder >> Content-Type: text/plain; charset=utf-8; format=flowed >Please. No format=flawed. Really. I'll figure out the line-wrapping. > Also this step is not about --no-relative and diff.relative but is only about --no-relative option. Should I submit as two independent patches then? I took the approach of splitting them out into 1/2 vs 2/2 to distinguish, but it sounds like that isn't optimal. > When "--relative" is given, options->prefix is set to something as we can see above. > > The --relative option is described as optionally taking <path> in the doc: ... > Doesn't "--no-relative" codepath have to undo the effect of that assignment to options->prefix? diff_setup_done NULLs options->prefix when DIFF_OPT_TST(options, RELATIVE_NAME) is cleared (--no-relative clears this option). On review, this may be a bad approach though. Non-locality makes it harder to follow/understand and introduces a subtle bug. current: "git-diff --relative=path --no-relative --relative" == "git-diff --relative=path" expected: "git-diff --relative=path --no-relative --relative" == "git-diff --relative" > So I can agree with the design decision but only after spending 6 > lines to think about it. For the end-users, this design decision > needs to be explained and spelled out in the documentation. Agreed; update to come. -----Original Message----- From: Junio C Hamano <gitster@pobox.com> Sent: 01/07/2015 01:09 PM To: kelson@shysecurity.com CC: Git Mailing List <git@vger.kernel.org>, Philip Oakley <philipoakley@iee.org>, Duy Nguyen <pclouds@gmail.com>, Jonathan Nieder <jrnieder@gmail.com> Subject: Re: [PATCH 1/2] support for --no-relative and diff.relative > kelson@shysecurity.com writes: > Content-Type: text/plain; charset=utf-8; format=flowed Please. No format=flawed. Really. > Subject: Re: [PATCH 1/2] support for --no-relative and diff.relative "diff: teach --no-relative to override earlier --relative" or something. Saying the affeced area upfront, terminated with a colon ':', will make it easier to spot in "git shortlog" output later. Also this step is not about --no-relative and diff.relative but is only about --no-relative option. > added --no-relative option for git-diff (code, documentation, and tests) "Add --no-relative option..."; we write in imperative, as if we are giving an order to the project secretary to "make the code do/be so". > --no-relative overrides --relative causing a return to standard behavior OK (modulo missing full-stop). > > Signed-off-by: Brandon Phillips <kelson@shysecurity.com> Please also have From: Brandon Phillips <kelson@shysecurity.com> as the first line of the body of your e-mail message, if you are letting your MUA only give your e-mail address without name. Alternatively, please ask/configure your MUA to put your name as well as your address on From: header of the e-mail (which is preferrable). > diff --git a/diff.c b/diff.c > index d1bd534..7bceba8 100644 > --- a/diff.c > +++ b/diff.c > @@ -3695,6 +3695,8 @@ int diff_opt_parse(struct diff_options *options, > const char **av, int ac) Line-wrapped. > DIFF_OPT_SET(options, RELATIVE_NAME); > options->prefix = arg; > } > + else if (!strcmp(arg, "--no-relative")) > + DIFF_OPT_CLR(options, RELATIVE_NAME); > When "--relative" is given, options->prefix is set to something as we can see above. The --relative option is described as optionally taking <path> in the doc: --relative[=<path>]:: When run from a subdirectory of the project, it can be told to exclude changes outside the directory and show pathnames relative to it with this option. When you are not in a subdirectory (e.g. in a bare repository), you can name which subdirectory to make the output relative to by giving a <path> as an argument. Doesn't "--no-relative" codepath have to undo the effect of that assignment to options->prefix? For example, after applying this patch, shouldn't $ cd t $ git show --relative=Documentation --no-relative --relative work the same way as $ cd t $ git show --relative i.e. limiting its output to the changes in the 't/' directory and not to the changes in the 'Documentation/' directory? Patch 2/2 also seems to share similar line-wrapping breakages that make it unappliable, but more importantly, the configuration that is supposed to correspond to --relative option only parses a boolean. Is that the right design, or should it also be able to substitute a command line `--relative=<path>` with an argument? The last was a half-way rhetorical question and my answer is that boolean-only is the best you could do, because we cannot do the usual "bool or string" trick when "string" can be arbitrary. In other words, "diff.relative=true" could mean "limit to the current subdirectory" aka "--relative" or it could mean "limit to true/ subdirectory" aka "--relative=true", and there is no good way to disambiguate between the two [*1*]. So I can agree with the design decision but only after spending 6 lines to think about it. For the end-users, this design decision needs to be explained and spelled out in the documentation. Saying "equivalent to `--relative`" is not sufficient, because the way `--relative` option itself is described elsewhere. The option appears as `--relative[=<path>]` (see above), so some people _will_ read "equivalent to `--relative`" to mean "Setting diff.relative=t should be equivalent to --relative=t", which is not what actually happens. [Footnote] *1* Actually, you could declare that "diff.relative=true/" means the 'true/' directory while "diff.relative=true" means the boolean 'true' aka 'diff --relative', but I think it is too confusing. Let's not make it worse by going that route. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] support for --no-relative and diff.relative 2015-01-07 18:46 ` kelson @ 2015-01-07 20:26 ` Junio C Hamano 0 siblings, 0 replies; 13+ messages in thread From: Junio C Hamano @ 2015-01-07 20:26 UTC (permalink / raw) To: kelson; +Cc: Git Mailing List, Philip Oakley, Duy Nguyen, Jonathan Nieder kelson@shysecurity.com writes: >>> Content-Type: text/plain; charset=utf-8; format=flowed >>Please. No format=flawed. Really. > I'll figure out the line-wrapping. > >> Also this step is not about --no-relative and diff.relative but is >> only about --no-relative option. > Should I submit as two independent patches then? I took the approach > of splitting them out into 1/2 vs 2/2 to distinguish, but it sounds > like that isn't optimal. They are indeed better to be 1/2 and 2/2; they do not have to share the same subject, though. 1/2 now adds only --no-relative and makes sure an earlier --relative is cancelled without even knowing that diff.relative might appear in the future (well, you may know that, but the system with only 1/2 applied without 2/2 would work perfectly fine). 2/2 adds diff.relative and makes sure --no-relative cancels its effect as well. > On review, this may be a bad approach though. Non-locality makes it > harder to follow/understand and introduces a subtle bug. > current: "git-diff --relative=path --no-relative --relative" == > "git-diff --relative=path" > expected: "git-diff --relative=path --no-relative --relative" == > "git-diff --relative" Exactly. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] support for --no-relative and diff.relative 2015-01-07 18:09 ` Junio C Hamano 2015-01-07 18:46 ` kelson @ 2015-01-07 19:02 ` Junio C Hamano 1 sibling, 0 replies; 13+ messages in thread From: Junio C Hamano @ 2015-01-07 19:02 UTC (permalink / raw) To: kelson; +Cc: Git Mailing List, Philip Oakley, Duy Nguyen, Jonathan Nieder Junio C Hamano <gitster@pobox.com> writes: > Patch 2/2 also seems to share similar line-wrapping breakages that > make it unappliable, but more importantly, the configuration that is > supposed to correspond to --relative option only parses a boolean. > Is that the right design, or should it also be able to substitute a > command line `--relative=<path>` with an argument? > > The last was a half-way rhetorical question and my answer is that > boolean-only is the best you could do... > ... > [Footnote] > > *1* Actually, you could declare that "diff.relative=true/" means the > 'true/' directory while "diff.relative=true" means the boolean > 'true' aka 'diff --relative', but I think it is too confusing. > Let's not make it worse by going that route. Addendum. It was only a "half-way rhetorical question", because I am willing to be persuaded that diff.relative=true/ vs diff.relative=true is *not* too subtle/confusing to be a good idea, if enough people whose judgement I trust agrees. ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2015-01-07 20:26 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-12-11 7:28 [PATCH] added git-config support for diff.relative setting Kelson 2014-12-11 13:37 ` Duy Nguyen 2014-12-11 21:41 ` Kelson 2014-12-12 23:25 ` [PATCH v2] " Kelson 2014-12-21 20:23 ` [PATCH v3 1/2] " kelson 2014-12-30 17:56 ` kelson 2014-12-30 18:16 ` Junio C Hamano 2014-12-30 19:32 ` [PATCH 1/2] support for --no-relative and diff.relative kelson 2015-01-06 16:19 ` kelson 2015-01-07 18:09 ` Junio C Hamano 2015-01-07 18:46 ` kelson 2015-01-07 20:26 ` Junio C Hamano 2015-01-07 19:02 ` Junio C Hamano
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).