[BUG] Spurious leading '/' in filename in "git diff --raw --relative=<subdirectory>"

[BUG] Spurious leading '/' in filename in "git diff --raw --relative=<subdirectory>"

[Jakub Narebski]

I wanted to get non-recursive raw diff (difftree), but for a given
subdirectory and not starting from root.  

I have found '--relative[=<path>]', introduced in 

  c0cb4a0 (diff --relative: help working in a bare repository, 2008-02-13)
  cd676a5 (diff --relative: output paths as relative to the current subdirectory, 2008-02-12)

But when examining it's output in 'raw' mode, I have notice spurious
leading '/' in filename:

  $ git diff-tree --abbrev -r --raw HEAD --relative=sub
  a3a8425fe5496c61921010cb1e7b455a1f52bb86
  :100644 100644 d90bda0... cefcae0... M	/quux

The output without '-r' (recurse into sub-trees) is even more strange

  $ git diff-tree --abbrev --raw HEAD --relative=sub
  a3a8425fe5496c61921010cb1e7b455a1f52bb86
  :040000 040000 e62aa6e... b5d4a43... M	

(that's a trailing TAB, and no filename).

What I expected was the following output:

  $ git diff-tree --abbrev --raw HEAD --relative=sub
  a3a8425fe5496c61921010cb1e7b455a1f52bb86
  :040000 040000 e62aa6e... b5d4a43... M	quux


I see that the '--relative' and '--relative=<path>' options were introduced
for patch ('-p') format, and not for difftree / raw format, but I think
they should work for it, too.


P.S. I have noticed this bug when working on proof-of-concept tree-blame
(i.e. when given file was modified) in Perl.
-- 
Jakub Narebski
Poland

Re: [BUG] Spurious leading '/' in filename in "git diff --raw --relative=<subdirectory>"

[Jakub Narebski]

Jakub Narebski wrote:

> The output without '-r' (recurse into sub-trees) is even more strange
> 
>   $ git diff-tree --abbrev --raw HEAD --relative=sub
>   a3a8425fe5496c61921010cb1e7b455a1f52bb86
>   :040000 040000 e62aa6e... b5d4a43... M	
> 
> (that's a trailing TAB, and no filename).
> 
> What I expected was the following output:
> 
>   $ git diff-tree --abbrev --raw HEAD --relative=sub
>   a3a8425fe5496c61921010cb1e7b455a1f52bb86
>   :040000 040000 e62aa6e... b5d4a43... M	quux

If I change directory to 'sub', and use '--relative', it works
correctly with '-r' (and doesn't show any output without '-r'):

    $ ( cd sub &&  git diff-tree --abbrev -r --raw HEAD --relative )
    a3a8425fe5496c61921010cb1e7b455a1f52bb86
    :100644 100644 d90bda0... cefcae0... M	quux

-- 
Jakub Narebski
Poland

[BUG/RFC] Raw diff output format (git-diff-tree) and --relative[=<path>] option

[Jakub Narebski]

I wanted to get non-recursive raw diff (difftree), but for a given
subdirectory and not starting from root.

In February 2008 Junio C Hamano added support for --relative and 
--relative=<path> options to git-diff:
* cd676a5 (diff --relative: output paths as relative
           to the current subdirectory, 2008-02-12)
* c0cb4a0 (diff --relative: help working in a bare repository,
           2008-02-13)

>From the commit message for cd676a5 (the c0cb4a0 just allows the 
--relative option to say which subdirectory to pretend to be in,
i.e. adds the --relative=<path> version) it looks like this option
was intended for patch (-p) output format.

There was added support also for raw output format, so both 
'git diff-tree' and 'git diff --raw' works with --relative option,
but support for this is buggy, and in my opinion wrong way around.


The '--relative[=<path>]' option works currently like this.  First,
if git command is invoked in subdirectory the diffopts structure
gets set prefix and prefix_length (in init_revisions).  If 
--relative[=<path>] option is passed, git sets RELATIVE_NAME flag,
and if there is argument, prefix is set to it.  Later git removes
prefix (sets it to NULL) if RELATIVE_NAME option is not set.

What's important in this step is that prefix is set without any 
normalization from --relative=<path> argument, while (from what
I understand) if it is set from current directory i.e. with --relative 
option without argument, it is set with trailing slash.  So using
--relative=sub instead of --relative=sub/ might be thought as user
error... but I think here it is lack of robustness in the API.


Then comes the filtering part.  In functions such as diff_change
or stuff_change, if path(s) does not begin with prefix, they are
simply skipped.  This solution limits --relative[=<path>] to work
only with *recursive* (full tree) output, such as patch output format,
or "git diff", or "git diff-tree -r" (and "git diff-tree -t", which
implies "-r").


Last there is filename munging, done using strip_prefix function.
This is done using prefix_length only, and that is the cause of
the bug:
  $ git diff-tree --abbrev -r --raw HEAD --relative=sub
  a3a8425fe5496c61921010cb1e7b455a1f52bb86
  :100644 100644 d90bda0... cefcae0... M	/quux

if one uses '--relative=sub' instead of '--relative=sub/'.


What I'd like to see for the raw output format is to work with 
--relative[=<path>] to work as if <path> was top directory of 
repository.  For example for diff between two trees

  $ git diff-tree A B --relative=sub/

would be equivalent to running

  $ git diff-tree A:sub/ B:sub/

*This* could be done, I think, by modifying diff_tree_sha1 to do a diff 
betweem A:sub/ and B:sub/ (taking 'sub/' from prefix) and unsetting 
prefix (setting prefix to NULL and prefix_length to 0).  But that would 
work only in the case that can be reducted to diff between two tree 
objects.  This wouldn't work for diff in raw output format between tree 
and working area, tree and index, or index and working area.

Is the idea of automagically translating <sha1> into <sha1>:<prefix>
to support --relative / relative=<prefix> well in raw diff output format
a good idea?  Or should I search for another solution.

I also do not know code enough (and it is not simple) to guess how
one would go with the same result for diff between trees, index, and
working area files.

BTW. the approach proposed here has the advantage that for B:<sub>,
if <sub> does not exist in B, we can try to do what 'subtree' merge 
strategy does (and what wholesame directory rename detection did),
namely try to find given directory under different path (like for 
example subtree-merged git-gui and gitk).

-- 
Jakub Narebski
Poland

Re: [BUG/RFC] Raw diff output format (git-diff-tree) and --relative[=<path>] option

[Jeff King]

On Thu, Jul 08, 2010 at 01:00:17PM +0200, Jakub Narebski wrote:

> Last there is filename munging, done using strip_prefix function.
> This is done using prefix_length only, and that is the cause of
> the bug:
>   $ git diff-tree --abbrev -r --raw HEAD --relative=sub
>   a3a8425fe5496c61921010cb1e7b455a1f52bb86
>   :100644 100644 d90bda0... cefcae0... M	/quux
> 
> if one uses '--relative=sub' instead of '--relative=sub/'.

Is that a bug or a feature? You need to say "sub/" to get what you want,
which is annoying. But it means you can also you "--relative=su" to get
"b/quux". In that example, it's probably useless, but consider a set of
filenames "foo-1" through "foo-5". You don't always want to break on a
directory boundary.

I believe "git-archive --prefix" has the same behavior for the same
reason.

-Peff

Re: [BUG/RFC] Raw diff output format (git-diff-tree) and --relative[=<path>] option

[Jakub Narebski]

On Thu, 8 Jul 2010, Jeff King wrote:
> On Thu, Jul 08, 2010 at 01:00:17PM +0200, Jakub Narebski wrote:
> 
> > Last there is filename munging, done using strip_prefix function.
> > This is done using prefix_length only, and that is the cause of
> > the bug:
> >   $ git diff-tree --abbrev -r --raw HEAD --relative=sub
> >   a3a8425fe5496c61921010cb1e7b455a1f52bb86
> >   :100644 100644 d90bda0... cefcae0... M	/quux
> > 
> > if one uses '--relative=sub' instead of '--relative=sub/'.
> 
> Is that a bug or a feature? You need to say "sub/" to get what you want,
> which is annoying. But it means you can also you "--relative=su" to get
> "b/quux". In that example, it's probably useless, but consider a set of
> filenames "foo-1" through "foo-5". You don't always want to break on a
> directory boundary.
> 
> I believe "git-archive --prefix" has the same behavior for the same
> reason.

It is called --relative=<path>, *not* --relative=<prefix>.  I also
think that it was meant to be the opposite of --directory=<root> of
git-am and git-apply.

"git archive --prefix" is IMVHO a bit of horse of different color.

Nevertheless in current situation it would be good to have the
following applied.
-- >8 --
diff --git i/Documentation/diff-options.txt w/Documentation/diff-options.txt
index 2371262..bc837cd 100644
--- i/Documentation/diff-options.txt
+++ w/Documentation/diff-options.txt
@@ -278,7 +278,7 @@ ifndef::git-format-patch[]
 	Swap two inputs; that is, show differences from index or
 	on-disk file to tree contents.
 
---relative[=<path>]::
+--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
-- 8< --

-- 
Jakub Narebski
Poland

Re: [BUG/RFC] Raw diff output format (git-diff-tree) and --relative[=<path>] option

[Jakub Narebski]

On Thu, 8 Jul 2010, Jeff King wrote:
> On Thu, Jul 08, 2010 at 01:00:17PM +0200, Jakub Narebski wrote:
> 
> > Last there is filename munging, done using strip_prefix function.
> > This is done using prefix_length only, and that is the cause of
> > the bug:
> >   $ git diff-tree --abbrev -r --raw HEAD --relative=sub
> >   a3a8425fe5496c61921010cb1e7b455a1f52bb86
> >   :100644 100644 d90bda0... cefcae0... M	/quux
> > 
> > if one uses '--relative=sub' instead of '--relative=sub/'.
> 
> Is that a bug or a feature? You need to say "sub/" to get what you want,
> which is annoying. But it means you can also you "--relative=su" to get
> "b/quux". In that example, it's probably useless, but consider a set of
> filenames "foo-1" through "foo-5". You don't always want to break on a
> directory boundary.
> 
> I believe "git-archive --prefix" has the same behavior for the same
> reason.

Nevertheless for the patch output format both "git diff --relative=sub"
and "git diff --relative=sub/" give the same output, without 'b//quux'.
The same IMHO should be done for raw output format, so we don't have
'/quux' but 'quux'.

-- 
Jakub Narebski
Poland

Re: [BUG/RFC] Raw diff output format (git-diff-tree) and --relative[=<path>] option

[Jeff King]

On Thu, Jul 08, 2010 at 02:19:42PM +0200, Jakub Narebski wrote:

> Nevertheless for the patch output format both "git diff --relative=sub"
> and "git diff --relative=sub/" give the same output, without 'b//quux'.
> The same IMHO should be done for raw output format, so we don't have
> '/quux' but 'quux'.

Hmm. That is because the diff output properly eliminates the double "/".
But AFAICT, all of the following do what I would expect:

  git diff --relative=sub
  git diff --relative=sub/ ;# same as above
  git diff --relative=foo- ;# yields "a/10" for file "foo-10"

Doing

  git diff --relative=sub --stat

shows the same issue as your --raw version, as does --name-only. I think
the right solution is to clean up a leading "/" for those cases. That
leaves the possibility for non-directory prefixes, but should do what
the user wants in the directory case (since a leading "/" is
nonsensical).

Or was that what you had in mind the whole time? My impression was that
you wanted --relative=foo to always be equivalent to --relative=foo/.
The subtle difference is that I want the "/" removed only if it is the
next character (or another way of thinking about it is to append "/" to
the prefix only if it is an actual directory).

-Peff

Re: [BUG/RFC] Raw diff output format (git-diff-tree) and --relative[=<path>] option

[Jakub Narebski]

On Thu, 8 Jul 2010, Jeff King wrote:
> On Thu, Jul 08, 2010 at 02:19:42PM +0200, Jakub Narebski wrote:
> 
> > Nevertheless for the patch output format both "git diff --relative=sub"
> > and "git diff --relative=sub/" give the same output, without 'b//quux'.
> > The same IMHO should be done for raw output format, so we don't have
> > '/quux' but 'quux'.
> 
> Hmm. That is because the diff output properly eliminates the double "/".
> But AFAICT, all of the following do what I would expect:
> 
>   git diff --relative=sub
>   git diff --relative=sub/ ;# same as above
>   git diff --relative=foo- ;# yields "a/10" for file "foo-10"
> 
> Doing
> 
>   git diff --relative=sub --stat
> 
> shows the same issue as your --raw version, as does --name-only. I think
> the right solution is to clean up a leading "/" for those cases. That
> leaves the possibility for non-directory prefixes, but should do what
> the user wants in the directory case (since a leading "/" is
> nonsensical).

Perhaps this would be enough:

-- >8 --
diff --git i/diff.c w/diff.c
index 3aa695d..3a4696e 100644
--- i/diff.c
+++ w/diff.c
@@ -2705,10 +2705,16 @@ static void diff_fill_sha1_info(struct diff_filespec *one)
 static void strip_prefix(int prefix_length, const char **namep, const char **otherp)
 {
 	/* Strip the prefix but do not molest /dev/null and absolute paths */
-	if (*namep && **namep != '/')
+	if (*namep && **namep != '/') {
 		*namep += prefix_length;
-	if (*otherp && **otherp != '/')
+		if (**namep == '/')
+			++*namep;
+	}
+	if (*otherp && **otherp != '/') {
 		*otherp += prefix_length;
+		if (**otherp == '/')
+			++*otherp;
+	}
 }
 
 static void run_diff(struct diff_filepair *p, struct diff_options *o)
-- 8< --

> 
> Or was that what you had in mind the whole time? My impression was that
> you wanted --relative=foo to always be equivalent to --relative=foo/.
> The subtle difference is that I want the "/" removed only if it is the
> next character (or another way of thinking about it is to append "/" to
> the prefix only if it is an actual directory).

What I wanted is for "git diff-tree A B --relative=sub" to behave as
"git diff-tree A:sub B:sub".  Currently without -r / -t (without turning
on recursive mode) it produces no output; well at least no output if
'sub' is really subdirectory.

What's more I wanted for "git diff --raw" in any combination to behave
the same... although I guess here point is moot, as "git diff" is
automatically recursive regardless of output format, and you can't turn
it off.

Stil I'd like for "git diff-tree <commit>" to behave appropriately
with --relative or --relative=<path>.

-- 
Jakub Narebski
Poland

Re: [BUG/RFC] Raw diff output format (git-diff-tree) and --relative[=<path>] option

[Jeff King]

[Sorry for the long delay in responding. I was moving and am just now
 clearing my backlog of mail.]

On Thu, Jul 08, 2010 at 04:56:20PM +0200, Jakub Narebski wrote:

> > Hmm. That is because the diff output properly eliminates the double "/".
> > But AFAICT, all of the following do what I would expect:
> > 
> >   git diff --relative=sub
> >   git diff --relative=sub/ ;# same as above
> >   git diff --relative=foo- ;# yields "a/10" for file "foo-10"
> > 
> > Doing
> > 
> >   git diff --relative=sub --stat
> > 
> > shows the same issue as your --raw version, as does --name-only. I think
> > the right solution is to clean up a leading "/" for those cases. That
> > leaves the possibility for non-directory prefixes, but should do what
> > the user wants in the directory case (since a leading "/" is
> > nonsensical).
> 
> Perhaps this would be enough:

I think your patch is the right solution. Here it is with a commit
message and some tests.

-- >8 --
From: Jakub Narebski <jnareb@gmail.com>
Subject: [PATCH] diff: strip extra "/" when stripping prefix

There are two ways a user might want to use "diff
--relative":

  1. For a file in a directory, like "subdir/file", the user
     can use "--relative=subdir/" to strip the directory.

  2. To strip part of a filename, like "foo-10", they can
     use "--relative=foo-".

We currently handle both of those situations. However, if
the user passes "--relative=subdir" (without the trailing
slash), we produce inconsistent results. For the unified
diff format, we collapse the double-slash of "a//file"
correctly into "a/file". But for other formats (raw, stat,
name-status), we end up with "/file".

We can do what the user means here and strip the extra "/"
(and only a slash).  We are not hurting any existing users
of (2) above with this behavior change because the existing
output for this case was nonsensical.

Patch by Jakub, tests and commit message by Jeff King.

Signed-off-by: Jeff King <peff@peff.net>
---
 diff.c                   |   10 ++++++-
 t/t4045-diff-relative.sh |   61 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 69 insertions(+), 2 deletions(-)
 create mode 100755 t/t4045-diff-relative.sh

diff --git a/diff.c b/diff.c
index 782896d..bf65892 100644
--- a/diff.c
+++ b/diff.c
@@ -2704,10 +2704,16 @@ static void diff_fill_sha1_info(struct diff_filespec *one)
 static void strip_prefix(int prefix_length, const char **namep, const char **otherp)
 {
 	/* Strip the prefix but do not molest /dev/null and absolute paths */
-	if (*namep && **namep != '/')
+	if (*namep && **namep != '/') {
 		*namep += prefix_length;
-	if (*otherp && **otherp != '/')
+		if (**namep == '/')
+			++*namep;
+	}
+	if (*otherp && **otherp != '/') {
 		*otherp += prefix_length;
+		if (**otherp == '/')
+			++*otherp;
+	}
 }
 
 static void run_diff(struct diff_filepair *p, struct diff_options *o)
diff --git a/t/t4045-diff-relative.sh b/t/t4045-diff-relative.sh
new file mode 100755
index 0000000..8a3c63b
--- /dev/null
+++ b/t/t4045-diff-relative.sh
@@ -0,0 +1,61 @@
+#!/bin/sh
+
+test_description='diff --relative tests'
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	git commit --allow-empty -m empty &&
+	echo content >file1 &&
+	mkdir subdir &&
+	echo other content >subdir/file2 &&
+	git add . &&
+	git commit -m one
+'
+
+check_diff() {
+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 "-p $*" "
+	git diff -p $* HEAD^ >actual &&
+	test_cmp expected actual
+"
+}
+
+check_stat() {
+expect=$1; shift
+cat >expected <<EOF
+ $expect |    1 +
+ 1 files changed, 1 insertions(+), 0 deletions(-)
+EOF
+test_expect_success "--stat $*" "
+	git diff --stat $* HEAD^ >actual &&
+	test_cmp expected actual
+"
+}
+
+check_raw() {
+expect=$1; shift
+cat >expected <<EOF
+:000000 100644 0000000000000000000000000000000000000000 25c05ef3639d2d270e7fe765a67668f098092bc5 A	$expect
+EOF
+test_expect_success "--raw $*" "
+	git diff --no-abbrev --raw $* HEAD^ >actual &&
+	test_cmp expected actual
+"
+}
+
+for type in diff stat raw; do
+	check_$type file2 --relative=subdir/
+	check_$type file2 --relative=subdir
+	check_$type dir/file2 --relative=sub
+done
+
+test_done
-- 
1.7.2.1.63.g6ffaf

Re: [BUG/RFC] Raw diff output format (git-diff-tree) and --relative[=<path>] option

[Jeff King]

On Thu, Jul 08, 2010 at 04:56:20PM +0200, Jakub Narebski wrote:

> > Or was that what you had in mind the whole time? My impression was that
> > you wanted --relative=foo to always be equivalent to --relative=foo/.
> > The subtle difference is that I want the "/" removed only if it is the
> > next character (or another way of thinking about it is to append "/" to
> > the prefix only if it is an actual directory).
> 
> What I wanted is for "git diff-tree A B --relative=sub" to behave as
> "git diff-tree A:sub B:sub".  Currently without -r / -t (without turning
> on recursive mode) it produces no output; well at least no output if
> 'sub' is really subdirectory.

I think this is a separate issue from the extra slash problem. I don't
think it would hurt anything to enable "-r" automatically in diff-tree,
but _only_ if we know that the prefix we stripped ended in "/". Since
that is just changing a case that is already nonsensical, whereas
turning on recursion for "git diff-tree --relative=foo- A B" would
actually be a change in behavior.

It should be possible, but it is a bit more surgery, since the prefix
stripping code is pretty dumb currently.

> What's more I wanted for "git diff --raw" in any combination to behave
> the same... although I guess here point is moot, as "git diff" is
> automatically recursive regardless of output format, and you can't turn
> it off.

Yeah, I think "git diff" already does what you want, since 82cb8af
(git-diff: turn on recursion by default, 2007-07-29).

-Peff