[PATCH] added git-config support for diff.relative setting

[PATCH] added git-config support for diff.relative setting

[Kelson]

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

Re: [PATCH] added git-config support for diff.relative setting

[Duy Nguyen]

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

Re: [PATCH] added git-config support for diff.relative setting

[Kelson]

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.

[PATCH v2] added git-config support for diff.relative setting

[Kelson]

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

[PATCH v3 1/2] git-config support for diff.relative setting

[kelson]

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

[PATCH v3 1/2] git-config support for diff.relative setting

[kelson]

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

Re: [PATCH v3 1/2] git-config support for diff.relative setting

[Junio C Hamano]

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.

[PATCH 1/2] support for --no-relative and diff.relative

[kelson]

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

[PATCH 1/2] support for --no-relative and diff.relative

[kelson]

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

Re: [PATCH 1/2] support for --no-relative and diff.relative

[Junio C Hamano]

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.

Re: [PATCH 1/2] support for --no-relative and diff.relative

[kelson]

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

Re: [PATCH 1/2] support for --no-relative and diff.relative

[Junio C Hamano]

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.

Re: [PATCH 1/2] support for --no-relative and diff.relative

[Junio C Hamano]

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.