threads / patch / 45426

v3, 2 partsdiff --no-index: support symlinks and pipes

Subject: [PATCH v3 0/2] diff --no-index: support symlinks and pipes

## tl;dr

8 messages between Mar 18, 2017 and Mar 20, 2017. Diffs are folded; open one to read it.

replies: 7people: 2as markdown or json

Dennis Kaarsemaker· Mar 18, 2017, 21:00 UTC · lore
git diff <(command1) <(command2) is less useful than it could be, all it outputs is:
Show changes to 2 files +1 −4

dev/fd/63, dev/fd/62

diff --git a/dev/fd/63 b/dev/fd/62
index 9e6542b297..9f7b2c291b 120000
--- a/dev/fd/63
+++ b/dev/fd/62
@@ -1 +1 @@
-pipe:[464811685]
\ No newline at end of file
+pipe:[464811687]
\ No newline at end of file

Normal diff provides arguably better output: the diff of the output of the
commands. This series makes it possible for git diff --no-index to follow
symlinks and read from pipes, mimicking the behaviour of normal diff.

v1: http://public-inbox.org/git/20161111201958.2175-1-dennis@kaarsemaker.net/
v2: http://public-inbox.org/git/20170113102021.6054-1-dennis@kaarsemaker.net/

Changes since v2, prompted by feedback from Junio:

- A --derefence option was added and the default is no longer to dereference
  symlinks.
- Instead of looking at what canon_mode returns, use the original mode of 
  files to override behaviour for pipes.
- Turn the !S_ISREG(...) check into a should_mmap_file_contents helper.

Dennis Kaarsemaker (2):
  diff --no-index: optionally follow symlinks
  diff --no-index: support reading from pipes

 Documentation/diff-options.txt |  9 +++++++
 diff-no-index.c                | 16 ++++++++++---
 diff.c                         | 30 +++++++++++++++++++----
 diff.h                         |  2 +-
 t/t4053-diff-no-index.sh       | 54 ++++++++++++++++++++++++++++++++++++++++++
 t/test-lib.sh                  |  4 ++++
 6 files changed, 107 insertions(+), 8 deletions(-)
-- 
2.12.0-437-g0cc2799
Dennis Kaarsemaker· Mar 18, 2017, 21:00 UTC · re: Dennis Kaarsemaker · lore

[PATCH v3 2/2] diff --no-index: support reading from pipes

diff <(command1) <(command2) provides useful output, let's make it possible for git to do the same.

Signed-off-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>
---
 diff-no-index.c          |  9 +++++++++
 diff.c                   | 18 ++++++++++++++++--
 t/t4053-diff-no-index.sh | 10 ++++++++++
 t/test-lib.sh            |  4 ++++
 4 files changed, 39 insertions(+), 2 deletions(-)
Show changes to 4 files +39 −2

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

diff --git a/diff-no-index.c b/diff-no-index.c
index fe48f32ddd..1262a587e5 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -83,6 +83,15 @@ static struct diff_filespec *noindex_filespec(const char *name, int mode)
 		name = "/dev/null";
 	s = alloc_filespec(name);
 	fill_filespec(s, null_sha1, 0, mode);
+	/*
+	 * In --no-index mode, we support reading from pipes. canon_mode, called by
+	 * fill_filespec, gets confused by this and thinks we now have subprojects.
+	 * To help the rest of the diff machinery along, we now override what
+	 * canon_mode says. This is done here instead of in canon_mode, because the
+	 * rest of git does not (and should not) support pipes.
+	 */
+	if (S_ISFIFO(mode))
+		s->mode = S_IFREG | ce_permissions(mode);
 	if (name == file_from_standard_input)
 		populate_from_stdin(s);
 	return s;
diff --git a/diff.c b/diff.c
index 2afecfb939..4f74a54d74 100644
--- a/diff.c
+++ b/diff.c
@@ -2765,6 +2765,11 @@ static int diff_populate_gitlink(struct diff_filespec *s, int size_only)
 	return 0;
 }
 
+static int should_mmap_file_contents(struct stat *st)
+{
+	return S_ISREG(st->st_mode);
+}
+
 /*
  * While doing rename detection and pickaxe operation, we may need to
  * grab the data for the blob (or file) for our own in-core comparison.
@@ -2839,9 +2844,18 @@ int diff_populate_filespec(struct diff_filespec *s, unsigned int flags)
 		fd = open(s->path, O_RDONLY);
 		if (fd < 0)
 			goto err_empty;
-		s->data = xmmap(NULL, s->size, PROT_READ, MAP_PRIVATE, fd, 0);
+		if (!should_mmap_file_contents(&st)) {
+			struct strbuf sb = STRBUF_INIT;
+			strbuf_read(&sb, fd, 0);
+			s->size = sb.len;
+			s->data = strbuf_detach(&sb, NULL);
+			s->should_free = 1;
+		}
+		else {
+			s->data = xmmap(NULL, s->size, PROT_READ, MAP_PRIVATE, fd, 0);
+			s->should_munmap = 1;
+		}
 		close(fd);
-		s->should_munmap = 1;
 
 		/*
 		 * Convert from working tree format to canonical git format
diff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh
index 8c87bffb34..2d9b322315 100755
--- a/t/t4053-diff-no-index.sh
+++ b/t/t4053-diff-no-index.sh
@@ -171,4 +171,14 @@ test_expect_success SYMLINKS 'diff --no-index --no-dereference does not follow s
 	test_cmp expect actual
 '
 
+test_expect_success PROCESS_SUBSTITUTION 'diff --no-index works on fifos' '
+	cat >expect <<-EOF &&
+		@@ -1 +1 @@
+		-1
+		+2
+	EOF
+	test_expect_code 1 git diff --no-index --dereference <(echo 1) <(echo 2) | tail -n +5 > actual &&
+	test_cmp expect actual
+'
+
 test_done
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 11562bde10..78f3d24651 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1128,3 +1128,7 @@ build_option () {
 test_lazy_prereq LONG_IS_64BIT '
 	test 8 -le "$(build_option sizeof-long)"
 '
+
+test_lazy_prereq PROCESS_SUBSTITUTION '
+	eval "foo=<(echo test)" 2>/dev/null
+'
-- 
2.12.0-437-g0cc2799
Dennis Kaarsemaker· Mar 18, 2017, 21:00 UTC · re: Dennis Kaarsemaker · lore

[PATCH v3 1/2] diff --no-index: optionally follow symlinks

Git's diff machinery does not follow symlinks, which makes sense as git itself also does not, but stores the symlink destination.

In --no-index mode however, it is useful for diff to be able to follow symlinks, matching the behaviour of ordinary diff. A new --dereference (name copied from diff) option has been added to enable this behaviour. --no-dereference can be used to disable it again.

Signed-off-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>
---
 Documentation/diff-options.txt |  9 +++++++++
 diff-no-index.c                |  7 ++++---
 diff.c                         | 12 ++++++++++--
 diff.h                         |  2 +-
 t/t4053-diff-no-index.sh       | 44 ++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 68 insertions(+), 6 deletions(-)
Show changes to 5 files +68 −6

Documentation/diff-options.txt, diff-no-index.c, diff.c, diff.h, t/t4053-diff-no-index.sh

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 2d77a19626..5a9d58b701 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -216,6 +216,15 @@ any of those replacements occurred.
 	commit range.  Defaults to `diff.submodule` or the 'short' format
 	if the config option is unset.
 
+ifdef::git-diff[]
+--dereference::
+--no-dereference::
+	Normally, "git diff --no-index" will compare symlinks by comparing what
+	they point to. The `--dereference` option will make it compare the content
+	of the linked files. The `--no-dereference` option disables an earlier
+	`--dereference`.
+endif::git-diff[]
+
 --color[=<when>]::
 	Show colored diff.
 	`--color` (i.e. without '=<when>') is the same as `--color=always`.
diff --git a/diff-no-index.c b/diff-no-index.c
index f420786039..fe48f32ddd 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -40,7 +40,7 @@ static int read_directory_contents(const char *path, struct string_list *list)
  */
 static const char file_from_standard_input[] = "-";
 
-static int get_mode(const char *path, int *mode)
+static int get_mode(const char *path, int *mode, int dereference)
 {
 	struct stat st;
 
@@ -52,7 +52,7 @@ static int get_mode(const char *path, int *mode)
 #endif
 	else if (path == file_from_standard_input)
 		*mode = create_ce_mode(0666);
-	else if (lstat(path, &st))
+	else if (dereference ? stat(path, &st) : lstat(path, &st))
 		return error("Could not access '%s'", path);
 	else
 		*mode = st.st_mode;
@@ -93,7 +93,8 @@ static int queue_diff(struct diff_options *o,
 {
 	int mode1 = 0, mode2 = 0;
 
-	if (get_mode(name1, &mode1) || get_mode(name2, &mode2))
+	if (get_mode(name1, &mode1, DIFF_OPT_TST(o, DEREFERENCE)) ||
+		get_mode(name2, &mode2, DIFF_OPT_TST(o, DEREFERENCE)))
 		return -1;
 
 	if (mode1 && mode2 && S_ISDIR(mode1) != S_ISDIR(mode2)) {
diff --git a/diff.c b/diff.c
index be11e4ef2b..2afecfb939 100644
--- a/diff.c
+++ b/diff.c
@@ -2815,7 +2815,7 @@ int diff_populate_filespec(struct diff_filespec *s, unsigned int flags)
 		s->size = xsize_t(st.st_size);
 		if (!s->size)
 			goto empty;
-		if (S_ISLNK(st.st_mode)) {
+		if (S_ISLNK(s->mode)) {
 			struct strbuf sb = STRBUF_INIT;
 
 			if (strbuf_readlink(&sb, s->path, s->size))
@@ -2825,6 +2825,10 @@ int diff_populate_filespec(struct diff_filespec *s, unsigned int flags)
 			s->should_free = 1;
 			return 0;
 		}
+		if (S_ISLNK(st.st_mode)) {
+			stat(s->path, &st);
+			s->size = xsize_t(st.st_size);
+		}
 		if (size_only)
 			return 0;
 		if ((flags & CHECK_BINARY) &&
@@ -3884,7 +3888,11 @@ int diff_opt_parse(struct diff_options *options,
 	else if (!strcmp(arg, "--no-follow")) {
 		DIFF_OPT_CLR(options, FOLLOW_RENAMES);
 		DIFF_OPT_CLR(options, DEFAULT_FOLLOW_RENAMES);
-	} else if (!strcmp(arg, "--color"))
+	} else if (!strcmp(arg, "--dereference"))
+		DIFF_OPT_SET(options, DEREFERENCE);
+	else if (!strcmp(arg, "--no-dereference"))
+		DIFF_OPT_CLR(options, DEREFERENCE);
+	else if (!strcmp(arg, "--color"))
 		options->use_color = 1;
 	else if (skip_prefix(arg, "--color=", &arg)) {
 		int value = git_config_colorbool(NULL, arg);
diff --git a/diff.h b/diff.h
index 25ae60d5ff..db33dc67f6 100644
--- a/diff.h
+++ b/diff.h
@@ -69,7 +69,7 @@ typedef struct strbuf *(*diff_prefix_fn_t)(struct diff_options *opt, void *data)
 #define DIFF_OPT_FIND_COPIES_HARDER  (1 <<  6)
 #define DIFF_OPT_FOLLOW_RENAMES      (1 <<  7)
 #define DIFF_OPT_RENAME_EMPTY        (1 <<  8)
-/* (1 <<  9) unused */
+#define DIFF_OPT_DEREFERENCE         (1 <<  9)
 #define DIFF_OPT_HAS_CHANGES         (1 << 10)
 #define DIFF_OPT_QUICK               (1 << 11)
 #define DIFF_OPT_NO_INDEX            (1 << 12)
diff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh
index 453e6c35eb..8c87bffb34 100755
--- a/t/t4053-diff-no-index.sh
+++ b/t/t4053-diff-no-index.sh
@@ -127,4 +127,48 @@ test_expect_success 'diff --no-index from repo subdir respects config (implicit)
 	test_cmp expect actual.head
 '
 
+test_expect_success SYMLINKS 'diff --no-index does not follows symlinks' '
+	echo a >1 &&
+	echo b >2 &&
+	ln -s 1 3 &&
+	ln -s 2 4 &&
+	cat >expect <<-EOF &&
+		--- a/3
+		+++ b/4
+		@@ -1 +1 @@
+		-1
+		\ No newline at end of file
+		+2
+		\ No newline at end of file
+	EOF
+	test_expect_code 1 git diff --no-index 3 4 | tail -n +3 >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success SYMLINKS 'diff --no-index --dereference does follows symlinks' '
+	cat >expect <<-EOF &&
+		--- a/3
+		+++ b/4
+		@@ -1 +1 @@
+		-a
+		+b
+	EOF
+	test_expect_code 1 git diff --no-index --dereference 3 4 | tail -n +3 >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success SYMLINKS 'diff --no-index --no-dereference does not follow symlinks' '
+	cat >expect <<-EOF &&
+		--- a/3
+		+++ b/4
+		@@ -1 +1 @@
+		-1
+		\ No newline at end of file
+		+2
+		\ No newline at end of file
+	EOF
+	test_expect_code 1 git diff --no-index --no-dereference 3 4 | tail -n +3 > actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.12.0-437-g0cc2799
Junio C Hamano· Mar 19, 2017, 22:14 UTC · re: Dennis Kaarsemaker · lore

Re: [PATCH v3 1/2] diff --no-index: optionally follow symlinks

Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:
Show 20 quoted lines
> diff --git a/diff.c b/diff.c
> index be11e4ef2b..2afecfb939 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -2815,7 +2815,7 @@ int diff_populate_filespec(struct diff_filespec *s, unsigned int flags)
>  		s->size = xsize_t(st.st_size);
>  		if (!s->size)
>  			goto empty;
> -		if (S_ISLNK(st.st_mode)) {
> +		if (S_ISLNK(s->mode)) {
>  			struct strbuf sb = STRBUF_INIT;
>  
>  			if (strbuf_readlink(&sb, s->path, s->size))
> @@ -2825,6 +2825,10 @@ int diff_populate_filespec(struct diff_filespec *s, unsigned int flags)
>  			s->should_free = 1;
>  			return 0;
>  		}
> +		if (S_ISLNK(st.st_mode)) {
> +			stat(s->path, &st);
> +			s->size = xsize_t(st.st_size);

Doesn't this affect --no-index mode? We never need to do a wasteful stat() after lstat() and we are penalizing the normal codepath with this change, no?

Show 11 quoted lines
> @@ -3884,7 +3888,11 @@ int diff_opt_parse(struct diff_options *options,
>  	else if (!strcmp(arg, "--no-follow")) {
>  		DIFF_OPT_CLR(options, FOLLOW_RENAMES);
>  		DIFF_OPT_CLR(options, DEFAULT_FOLLOW_RENAMES);
> -	} else if (!strcmp(arg, "--color"))
> +	} else if (!strcmp(arg, "--dereference"))
> +		DIFF_OPT_SET(options, DEREFERENCE);
> +	else if (!strcmp(arg, "--no-dereference"))
> +		DIFF_OPT_CLR(options, DEREFERENCE);
> +	else if (!strcmp(arg, "--color"))
>  		options->use_color = 1;

Also shouldn't be some code to detect --[no-]dereference options given when --no-index is not in effect and error out? As the patch title says, this change should be a no-op for normal codepath and only affect the no-index hack.

Dennis Kaarsemaker· Mar 20, 2017, 10:50 UTC · re: Junio C Hamano · lore

Re: [PATCH v3 1/2] diff --no-index: optionally follow symlinks

On Sun, 2017-03-19 at 15:14 -0700, Junio C Hamano wrote:
Show 26 quoted lines
> Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:
> 
> > diff --git a/diff.c b/diff.c
> > index be11e4ef2b..2afecfb939 100644
> > --- a/diff.c
> > +++ b/diff.c
> > @@ -2815,7 +2815,7 @@ int diff_populate_filespec(struct diff_filespec *s, unsigned int flags)
> >  		s->size = xsize_t(st.st_size);
> >  		if (!s->size)
> >  			goto empty;
> > -		if (S_ISLNK(st.st_mode)) {
> > +		if (S_ISLNK(s->mode)) {
> >  			struct strbuf sb = STRBUF_INIT;
> >  
> >  			if (strbuf_readlink(&sb, s->path, s->size))
> > @@ -2825,6 +2825,10 @@ int diff_populate_filespec(struct diff_filespec *s, unsigned int flags)
> >  			s->should_free = 1;
> >  			return 0;
> >  		}
> > +		if (S_ISLNK(st.st_mode)) {
> > +			stat(s->path, &st);
> > +			s->size = xsize_t(st.st_size);
> 
> Doesn't this affect --no-index mode?  We never need to do a wasteful
> stat() after lstat() and we are penalizing the normal codepath with
> this change, no?

the S_ISLNK(s->mode) conditional above is for the normal codepath, which returns early. So the stat I added is only done for symlinks in no_index mode.

Show 16 quoted lines
> > @@ -3884,7 +3888,11 @@ int diff_opt_parse(struct diff_options *options,
> >  	else if (!strcmp(arg, "--no-follow")) {
> >  		DIFF_OPT_CLR(options, FOLLOW_RENAMES);
> >  		DIFF_OPT_CLR(options, DEFAULT_FOLLOW_RENAMES);
> > -	} else if (!strcmp(arg, "--color"))
> > +	} else if (!strcmp(arg, "--dereference"))
> > +		DIFF_OPT_SET(options, DEREFERENCE);
> > +	else if (!strcmp(arg, "--no-dereference"))
> > +		DIFF_OPT_CLR(options, DEREFERENCE);
> > +	else if (!strcmp(arg, "--color"))
> >  		options->use_color = 1;
> 
> Also shouldn't be some code to detect --[no-]dereference options
> given when --no-index is not in effect and error out?  As the patch
> title says, this change should be a no-op for normal codepath and
> only affect the no-index hack.

But erroring out isn't a no-op. With the current patch you can do  --dereference without --no-index and it simply wouldn't affect anything.

I don't mind either way, so I'll make it error out.
-- 
Dennis Kaarsemaker
http://www.kaarsemaker.net
Junio C Hamano· Mar 19, 2017, 22:08 UTC · re: Dennis Kaarsemaker · lore

Re: [PATCH v3 0/2] diff --no-index: support symlinks and pipes

Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:
Show 11 quoted lines
> Normal diff provides arguably better output: the diff of the output of the
> commands. This series makes it possible for git diff --no-index to follow
> symlinks and read from pipes, mimicking the behaviour of normal diff.
>
> v1: http://public-inbox.org/git/20161111201958.2175-1-dennis@kaarsemaker.net/
> v2: http://public-inbox.org/git/20170113102021.6054-1-dennis@kaarsemaker.net/
>
> Changes since v2, prompted by feedback from Junio:
>
> - A --derefence option was added and the default is no longer to dereference
>   symlinks.

I do agree that it makes sense to have --[no-]dereference options, but I do not think it was my feedback and suggestion to make it optional (not default) to dereference, so please do not blame me for that choice.

Dennis Kaarsemaker· Mar 20, 2017, 10:40 UTC · re: Junio C Hamano · lore

Re: [PATCH v3 0/2] diff --no-index: support symlinks and pipes

On Sun, 2017-03-19 at 15:08 -0700, Junio C Hamano wrote:
Show 18 quoted lines
> Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:
> 
> > Normal diff provides arguably better output: the diff of the output of the
> > commands. This series makes it possible for git diff --no-index to follow
> > symlinks and read from pipes, mimicking the behaviour of normal diff.
> > 
> > v1: http://public-inbox.org/git/20161111201958.2175-1-dennis@kaarsemaker.net/
> > v2: http://public-inbox.org/git/20170113102021.6054-1-dennis@kaarsemaker.net/
> > 
> > Changes since v2, prompted by feedback from Junio:
> > 
> > - A --derefence option was added and the default is no longer to dereference
> >   symlinks.
> 
> I do agree that it makes sense to have --[no-]dereference options,
> but I do not think it was my feedback and suggestion to make it
> optional (not default) to dereference, so please do not blame me for
> that choice.

Then I misinterpreted your message at http://public-inbox.org/git/xmqqk29yedkv.fsf@gitster.mtv.corp.google.com/ No blame inteded, my apologies for coming across as blaming.

-- 
Dennis Kaarsemaker
http://www.kaarsemaker.net
Junio C Hamano· Mar 20, 2017, 16:02 UTC · re: Dennis Kaarsemaker · lore

Re: [PATCH v3 0/2] diff --no-index: support symlinks and pipes

Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:
Show 13 quoted lines
> On Sun, 2017-03-19 at 15:08 -0700, Junio C Hamano wrote:
> ...
>> > - A --derefence option was added and the default is no longer to dereference
>> >   symlinks.
>> 
>> I do agree that it makes sense to have --[no-]dereference options,
>> but I do not think it was my feedback and suggestion to make it
>> optional (not default) to dereference, so please do not blame me for
>> that choice.
>
> Then I misinterpreted your message at 
> http://public-inbox.org/git/xmqqk29yedkv.fsf@gitster.mtv.corp.google.com/
> No blame inteded, my apologies for coming across as blaming.

s/blame/credit/ then. I do not too deeply care which one is the default, and if we were adding --no-index without any existing users today, I probably would suggest making it deref by default (i.e. to make "diff --no-index" match better what other peoples' diffs do), but that would be a behaviour change to existing users if done today, so I think what you did probably is a good thing.

Thanks.

← back to recent threads