threads / rfc / 38880

RFC patchalign D/F handling of "diff --no-index" with that of normal Git

Subject: [RFC/PATCH] align D/F handling of "diff --no-index" with that of normal Git

## tl;dr

7 messages between Mar 22, 2015 and Mar 26, 2015. Diffs are folded; open one to read it.

replies: 6people: 3as markdown or json

Junio C Hamano· Mar 22, 2015, 05:11 UTC · lore

When a commit changes a path P that used to be a file to a directory and create a new path P/X in it, "git show" would say that file P was removed and file P/X was created for such a commit.

However, if we compare two directories, D1 and D2, where D1 has a file D1/P in it and D2 has a directory D2/P under which there is a file D2/P/X, and ask "git diff --no-index D1 D2" to show their differences, we simply get a refusal "file/directory conflict".

The "diff --no-index" implementation has an underlying machinery that can make it more in line with the normal Git if it wanted to, but somehow it is not being exercised. The only thing we need to do, when we see a file P and a directory P/ (or the other way around) is to show the removal of a file P and then pretend as if we are comparing nothing with a whole directory P/, as the code is fully prepared to express a creation of everything in a directory (and if the comparison is between a directory P/ and a file P, then show the creation of the file and then let the existing code remove everything in P/).

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 diff-no-index.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)
Show changes to diff-no-index.c +21 −2
diff --git a/diff-no-index.c b/diff-no-index.c
index 265709b..52e9546 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -97,8 +97,27 @@ static int queue_diff(struct diff_options *o,
 	if (get_mode(name1, &mode1) || get_mode(name2, &mode2))
 		return -1;
 
-	if (mode1 && mode2 && S_ISDIR(mode1) != S_ISDIR(mode2))
-		return error("file/directory conflict: %s, %s", name1, name2);
+	if (mode1 && mode2 && S_ISDIR(mode1) != S_ISDIR(mode2)) {
+		struct diff_filespec *d1, *d2;
+
+		if (S_ISDIR(mode1)) {
+			/* 2 is file that is created */
+			d1 = noindex_filespec(NULL, 0);
+			d2 = noindex_filespec(name2, mode2);
+			name2 = NULL;
+			mode2 = 0;
+		} else {
+			/* 1 is file that is deleted */
+			d1 = noindex_filespec(name1, mode2);
+			d2 = noindex_filespec(NULL, 0);
+			name1 = NULL;
+			mode1 = 0;
+		}
+		/* emit that file */
+		diff_queue(&diff_queued_diff, d1, d2);
+
+		/* and then let the entire directory created or deleted */
+	}
 
 	if (S_ISDIR(mode1) || S_ISDIR(mode2)) {
 		struct strbuf buffer1 = STRBUF_INIT;
Eric Sunshine· Mar 22, 2015, 07:01 UTC · re: Junio C Hamano · lore

Re: [RFC/PATCH] align D/F handling of "diff --no-index" with that of normal Git

On Sun, Mar 22, 2015 at 1:11 AM, Junio C Hamano <gitster@pobox.com> wrote:
> When a commit changes a path P that used to be a file to a directory
> and create a new path P/X in it, "git show" would say that file P
s/create/creates/
More below...
Show 53 quoted lines
> was removed and file P/X was created for such a commit.
>
> However, if we compare two directories, D1 and D2, where D1 has a
> file D1/P in it and D2 has a directory D2/P under which there is a
> file D2/P/X, and ask "git diff --no-index D1 D2" to show their
> differences, we simply get a refusal "file/directory conflict".
>
> The "diff --no-index" implementation has an underlying machinery
> that can make it more in line with the normal Git if it wanted to,
> but somehow it is not being exercised.  The only thing we need to
> do, when we see a file P and a directory P/ (or the other way
> around) is to show the removal of a file P and then pretend as if we
> are comparing nothing with a whole directory P/, as the code is
> fully prepared to express a creation of everything in a directory
> (and if the comparison is between a directory P/ and a file P, then
> show the creation of the file and then let the existing code remove
> everything in P/).
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  diff-no-index.c | 23 +++++++++++++++++++++--
>  1 file changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/diff-no-index.c b/diff-no-index.c
> index 265709b..52e9546 100644
> --- a/diff-no-index.c
> +++ b/diff-no-index.c
> @@ -97,8 +97,27 @@ static int queue_diff(struct diff_options *o,
>         if (get_mode(name1, &mode1) || get_mode(name2, &mode2))
>                 return -1;
>
> -       if (mode1 && mode2 && S_ISDIR(mode1) != S_ISDIR(mode2))
> -               return error("file/directory conflict: %s, %s", name1, name2);
> +       if (mode1 && mode2 && S_ISDIR(mode1) != S_ISDIR(mode2)) {
> +               struct diff_filespec *d1, *d2;
> +
> +               if (S_ISDIR(mode1)) {
> +                       /* 2 is file that is created */
> +                       d1 = noindex_filespec(NULL, 0);
> +                       d2 = noindex_filespec(name2, mode2);
> +                       name2 = NULL;
> +                       mode2 = 0;
> +               } else {
> +                       /* 1 is file that is deleted */
> +                       d1 = noindex_filespec(name1, mode2);
> +                       d2 = noindex_filespec(NULL, 0);
> +                       name1 = NULL;
> +                       mode1 = 0;
> +               }
> +               /* emit that file */
> +               diff_queue(&diff_queued_diff, d1, d2);
> +
> +               /* and then let the entire directory created or deleted */
s/created/be created/
Show 5 quoted lines
> +       }
>
>         if (S_ISDIR(mode1) || S_ISDIR(mode2)) {
>                 struct strbuf buffer1 = STRBUF_INIT;
> --
Ramsay Jones· Mar 22, 2015, 12:37 UTC · re: Junio C Hamano · lore

Re: [RFC/PATCH] align D/F handling of "diff --no-index" with that of normal Git

On 22/03/15 05:11, Junio C Hamano wrote:
Show 47 quoted lines
> When a commit changes a path P that used to be a file to a directory
> and create a new path P/X in it, "git show" would say that file P
> was removed and file P/X was created for such a commit.
> 
> However, if we compare two directories, D1 and D2, where D1 has a
> file D1/P in it and D2 has a directory D2/P under which there is a
> file D2/P/X, and ask "git diff --no-index D1 D2" to show their
> differences, we simply get a refusal "file/directory conflict".
> 
> The "diff --no-index" implementation has an underlying machinery
> that can make it more in line with the normal Git if it wanted to,
> but somehow it is not being exercised.  The only thing we need to
> do, when we see a file P and a directory P/ (or the other way
> around) is to show the removal of a file P and then pretend as if we
> are comparing nothing with a whole directory P/, as the code is
> fully prepared to express a creation of everything in a directory
> (and if the comparison is between a directory P/ and a file P, then
> show the creation of the file and then let the existing code remove
> everything in P/).
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  diff-no-index.c | 23 +++++++++++++++++++++--
>  1 file changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/diff-no-index.c b/diff-no-index.c
> index 265709b..52e9546 100644
> --- a/diff-no-index.c
> +++ b/diff-no-index.c
> @@ -97,8 +97,27 @@ static int queue_diff(struct diff_options *o,
>  	if (get_mode(name1, &mode1) || get_mode(name2, &mode2))
>  		return -1;
>  
> -	if (mode1 && mode2 && S_ISDIR(mode1) != S_ISDIR(mode2))
> -		return error("file/directory conflict: %s, %s", name1, name2);
> +	if (mode1 && mode2 && S_ISDIR(mode1) != S_ISDIR(mode2)) {
> +		struct diff_filespec *d1, *d2;
> +
> +		if (S_ISDIR(mode1)) {
> +			/* 2 is file that is created */
> +			d1 = noindex_filespec(NULL, 0);
> +			d2 = noindex_filespec(name2, mode2);
> +			name2 = NULL;
> +			mode2 = 0;
> +		} else {
> +			/* 1 is file that is deleted */
> +			d1 = noindex_filespec(name1, mode2);
-----------------------------------------------------^^^^^

I have not been following the discussion (or even really studied this patch), but the asymmetry here caught my eye as I was skimming the email.

[So, if this is actually correct, sorry for the noise!]

ATB, Ramsay Jones

Show 12 quoted lines
> +			d2 = noindex_filespec(NULL, 0);
> +			name1 = NULL;
> +			mode1 = 0;
> +		}
> +		/* emit that file */
> +		diff_queue(&diff_queued_diff, d1, d2);
> +
> +		/* and then let the entire directory created or deleted */
> +	}
>  
>  	if (S_ISDIR(mode1) || S_ISDIR(mode2)) {
>  		struct strbuf buffer1 = STRBUF_INIT;
Junio C Hamano· Mar 22, 2015, 17:56 UTC · re: Ramsay Jones · lore

Re: [RFC/PATCH] align D/F handling of "diff --no-index" with that of normal Git

On Sun, Mar 22, 2015 at 5:37 AM, Ramsay Jones <ramsay@ramsay1.demon.co.uk> wrote:

Show 14 quoted lines
> On 22/03/15 05:11, Junio C Hamano wrote:
>> +             if (S_ISDIR(mode1)) {
>> +                     /* 2 is file that is created */
>> +                     d1 = noindex_filespec(NULL, 0);
>> +                     d2 = noindex_filespec(name2, mode2);
>> +                     name2 = NULL;
>> +                     mode2 = 0;
>> +             } else {
>> +                     /* 1 is file that is deleted */
>> +                     d1 = noindex_filespec(name1, mode2);
>
> I have not been following the discussion (or even really
> studied this patch), but the asymmetry here caught my eye
> as I was skimming the email.

Yes, the assymetry is a stupid cut-and-paste error. Thanks for catching.

There was actually no discussion on this point other than a tangential mention of the problem to be solved, so you are up to date as long as you read the proposed log message in the message you are responding to ;-)

Another thing I noticed while I was playing with this is that when comparing D1 and D2, a path that was modified is shown as a patch between a/D1/path and b/D2/path, but a path that was created or removed shows D1 or D2 on both sides of the comparison, which we may also want to fix. This problem appears in the current codebase without the patch under discussion.

Junio C Hamano· Mar 26, 2015, 06:20 UTC · re: Junio C Hamano · lore

[PATCH v2 0/2] "diff --no-index" updates

Here are a few patches to scratch my itches in "diff --no-index" I have had for quite some time, but didn't feel strong enough to fix them myself so far.

The first one is to make "diff File Directory" (and "diff Directory File") more useful by aligning its behaviour with more mainstream "diff" implementations. We used to complain that one is a file and the other is a directory and died without doing anything useful. Instead, we pretend as if the user asked to compare "diff File Directory/File" (or vice versa), which is what GNU diff does.

The second one on the other hand is to make it behave more like the normal "git diff" when comparing two directories, when one directory has a path as a file while the other directory has the same path as a directory. GNU diff punts and says "File D1/path is a regular file while file D2/path is a directory" in such a case, and that is what "diff --no-index" does, too, but when the normal "git diff" compares two such trees, we show a removal of the file and creations of all the files in the directory (or vice versa). With this patch, we teach "diff --no-index" codepath to do the same, which would be more in line with the spirit of "diff --no-index", which is to make "git diff" goodies available to people who compare paths outside control of any git repository.

Junio C Hamano (2):
  diff-no-index: DWIM "diff D F" into "diff D/F F"
  diff: align D/F handling of "diff --no-index" with that of normal Git
 diff-no-index.c          | 66 ++++++++++++++++++++++++++++++++++++++++++++++--
 t/t4053-diff-no-index.sh | 34 +++++++++++++++++++++++++
 2 files changed, 98 insertions(+), 2 deletions(-)
-- 
2.3.4-475-g3180e2e
Junio C Hamano· Mar 26, 2015, 06:20 UTC · re: Junio C Hamano · lore

[PATCH v2 1/2] diff-no-index: DWIM "diff D F" into "diff D/F F"

"git diff --no-index" was supposed to be a poor-man's approach to allow using Git diff goodies outside of a Git repository, without having to patch mainstream diff implementations.

Unlike a POSIX diff that treats "diff D F" (or "diff F D") as a request to compare D/F and F (or F and D/F) when D is a directory and F is a file, however, we did not accept such a command line and instead barfed with "file/directory conflict".

Imitate what POSIX diff does and append the basename of the file after the name of the directory before comparing.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 diff-no-index.c          | 43 +++++++++++++++++++++++++++++++++++++++++++
 t/t4053-diff-no-index.sh | 22 ++++++++++++++++++++++
 2 files changed, 65 insertions(+)
Show changes to 2 files +65 −0

diff-no-index.c, t/t4053-diff-no-index.sh

diff --git a/diff-no-index.c b/diff-no-index.c
index 265709b..49c4536 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -182,12 +182,50 @@ static int queue_diff(struct diff_options *o,
 	}
 }
 
+/* append basename of F to D */
+static void append_basename(struct strbuf *path, const char *dir, const char *file)
+{
+	const char *tail = strrchr(file, '/');
+
+	strbuf_addstr(path, dir);
+	while (path->len && path->buf[path->len - 1] == '/')
+		path->len--;
+	strbuf_addch(path, '/');
+	strbuf_addstr(path, tail ? tail + 1 : file);
+}
+
+/*
+ * DWIM "diff D F" into "diff D/F F" and "diff F D" into "diff F D/F"
+ * Note that we append the basename of F to D/, so "diff a/b/file D"
+ * becomes "diff a/b/file D/file", not "diff a/b/file D/a/b/file".
+ */
+static void fixup_paths(const char **path, struct strbuf *replacement)
+{
+	unsigned int isdir0, isdir1;
+
+	if (path[0] == file_from_standard_input ||
+	    path[1] == file_from_standard_input)
+		return;
+	isdir0 = is_directory(path[0]);
+	isdir1 = is_directory(path[1]);
+	if (isdir0 == isdir1)
+		return;
+	if (isdir0) {
+		append_basename(replacement, path[0], path[1]);
+		path[0] = replacement->buf;
+	} else {
+		append_basename(replacement, path[1], path[0]);
+		path[1] = replacement->buf;
+	}
+}
+
 void diff_no_index(struct rev_info *revs,
 		   int argc, const char **argv,
 		   const char *prefix)
 {
 	int i, prefixlen;
 	const char *paths[2];
+	struct strbuf replacement = STRBUF_INIT;
 
 	diff_setup(&revs->diffopt);
 	for (i = 1; i < argc - 2; ) {
@@ -217,6 +255,9 @@ void diff_no_index(struct rev_info *revs,
 			p = xstrdup(prefix_filename(prefix, prefixlen, p));
 		paths[i] = p;
 	}
+
+	fixup_paths(paths, &replacement);
+
 	revs->diffopt.skip_stat_unmatch = 1;
 	if (!revs->diffopt.output_format)
 		revs->diffopt.output_format = DIFF_FORMAT_PATCH;
@@ -235,6 +276,8 @@ void diff_no_index(struct rev_info *revs,
 	diffcore_std(&revs->diffopt);
 	diff_flush(&revs->diffopt);
 
+	strbuf_release(&replacement);
+
 	/*
 	 * The return code for --no-index imitates diff(1):
 	 * 0 = no changes, 1 = changes, else error
diff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh
index 2ab3c48..01eca4c 100755
--- a/t/t4053-diff-no-index.sh
+++ b/t/t4053-diff-no-index.sh
@@ -55,4 +55,26 @@ test_expect_success 'git diff --no-index executed outside repo gives correct err
 	)
 '
 
+test_expect_success 'diff D F and diff F D' '
+	(
+		cd repo &&
+		echo in-repo >a &&
+		echo non-repo >../non/git/a &&
+		mkdir sub &&
+		echo sub-repo >sub/a &&
+
+		test_must_fail git diff --no-index sub/a ../non/git/a >expect &&
+		test_must_fail git diff --no-index sub/a ../non/git/ >actual &&
+		test_cmp expect actual &&
+
+		test_must_fail git diff --no-index a ../non/git/a >expect &&
+		test_must_fail git diff --no-index a ../non/git/ >actual &&
+		test_cmp expect actual &&
+
+		test_must_fail git diff --no-index ../non/git/a a >expect &&
+		test_must_fail git diff --no-index ../non/git a >actual &&
+		test_cmp expect actual
+	)
+'
+
 test_done

-- 
2.3.4-475-g3180e2e
Junio C Hamano· Mar 26, 2015, 06:20 UTC · re: Junio C Hamano · lore

[PATCH v2 2/2] diff: align D/F handling of "diff --no-index" with that of normal Git

When a commit changes a path P that used to be a file to a directory and creates a new path P/X in it, "git show" would say that file P was removed and file P/X was created for such a commit.

However, if we compare two directories, D1 and D2, where D1 has a file D1/P in it and D2 has a directory D2/P under which there is a file D2/P/X, and ask "git diff --no-index D1 D2" to show their differences, we simply get a refusal "file/directory conflict".

Surely, that may be what GNU diff does, but we can do better and it is easy to do so.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 diff-no-index.c          | 23 +++++++++++++++++++++--
 t/t4053-diff-no-index.sh | 12 ++++++++++++
 2 files changed, 33 insertions(+), 2 deletions(-)
Show changes to 2 files +33 −2

diff-no-index.c, t/t4053-diff-no-index.sh

diff --git a/diff-no-index.c b/diff-no-index.c
index 49c4536..0320605 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -97,8 +97,27 @@ static int queue_diff(struct diff_options *o,
 	if (get_mode(name1, &mode1) || get_mode(name2, &mode2))
 		return -1;
 
-	if (mode1 && mode2 && S_ISDIR(mode1) != S_ISDIR(mode2))
-		return error("file/directory conflict: %s, %s", name1, name2);
+	if (mode1 && mode2 && S_ISDIR(mode1) != S_ISDIR(mode2)) {
+		struct diff_filespec *d1, *d2;
+
+		if (S_ISDIR(mode1)) {
+			/* 2 is file that is created */
+			d1 = noindex_filespec(NULL, 0);
+			d2 = noindex_filespec(name2, mode2);
+			name2 = NULL;
+			mode2 = 0;
+		} else {
+			/* 1 is file that is deleted */
+			d1 = noindex_filespec(name1, mode1);
+			d2 = noindex_filespec(NULL, 0);
+			name1 = NULL;
+			mode1 = 0;
+		}
+		/* emit that file */
+		diff_queue(&diff_queued_diff, d1, d2);
+
+		/* and then let the entire directory be created or deleted */
+	}
 
 	if (S_ISDIR(mode1) || S_ISDIR(mode2)) {
 		struct strbuf buffer1 = STRBUF_INIT;
diff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh
index 01eca4c..596dfe7 100755
--- a/t/t4053-diff-no-index.sh
+++ b/t/t4053-diff-no-index.sh
@@ -77,4 +77,16 @@ test_expect_success 'diff D F and diff F D' '
 	)
 '
 
+test_expect_success 'turning a file into a directory' '
+	(
+		cd non/git &&
+		mkdir d e e/sub &&
+		echo 1 >d/sub &&
+		echo 2 >e/sub/file &&
+		printf "D\td/sub\nA\te/sub/file\n" >expect &&
+		test_must_fail git diff --no-index --name-status d e >actual &&
+		test_cmp expect actual
+	)
+'
+
 test_done

-- 
2.3.4-475-g3180e2e

← back to recent threads