[RFC/PATCH 0/2] git diff <(command1) <(command2)

[RFC/PATCH 0/2] git diff <(command1) <(command2)

[Dennis Kaarsemaker]

Today on #git, a user asked why git diff <(command1) <(command2) gave only some
gibberish about pipes as output. The answer is fairly simple: git diff gets as
arguments /dev/fd/62 and /dev/fd/63, which are symlinks. So git simply
readlink()s them and gets pipe:[123456] as destination of that link which it
then outputs.

Given that 'normal' diff provides arguably better output in this case (a diff
of the output of the two commands), I wanted to look at what it would take for
git to handle this. Surprisingly: not much. 1/2 adds support for
--follow-symlinks to git diff --no-index (and only the --no-index variant) and
2/2 adds support for reading from pipes.

No tests or documentation updates yet, and I'm not sure whether
--follow-symlinks in other modes than --no-index should be supported, ignored
(as it is now) or cause an error, but I'm leaning towards the third option.

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

 diff-no-index.c | 15 ++++++++++++---
 diff.c          | 23 +++++++++++++++++++----
 diff.h          |  2 +-
 3 files changed, 32 insertions(+), 8 deletions(-)

-- 
2.10.1-449-gab0f84c

[PATCH 1/2] diff --no-index: add option to follow symlinks

[Dennis Kaarsemaker]

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.

Signed-off-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>
---
 diff-no-index.c |  7 ++++---
 diff.c          | 10 ++++++++--
 diff.h          |  2 +-
 3 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/diff-no-index.c b/diff-no-index.c
index f420786..15811c2 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 follow_symlinks)
 {
 	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 (follow_symlinks ? 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, FOLLOW_SYMLINKS)) ||
+		get_mode(name2, &mode2, DIFF_OPT_TST(o, FOLLOW_SYMLINKS)))
 		return -1;
 
 	if (mode1 && mode2 && S_ISDIR(mode1) != S_ISDIR(mode2)) {
diff --git a/diff.c b/diff.c
index be11e4e..1eaf604 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,9 @@ 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, "--follow-symlinks"))
+		DIFF_OPT_SET(options, FOLLOW_SYMLINKS);
+	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 25ae60d..22b0c5a 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_FOLLOW_SYMLINKS     (1 <<  9)
 #define DIFF_OPT_HAS_CHANGES         (1 << 10)
 #define DIFF_OPT_QUICK               (1 << 11)
 #define DIFF_OPT_NO_INDEX            (1 << 12)
-- 
2.10.1-449-gab0f84c

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

[Dennis Kaarsemaker]

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 |  8 ++++++++
 diff.c          | 13 +++++++++++--
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/diff-no-index.c b/diff-no-index.c
index 15811c2..487dbf5 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -83,6 +83,14 @@ 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.
+	 * Detect this and tell the rest of the diff machinery to treat pipes as
+	 * normal files.
+	 */
+	if (S_ISGITLINK(s->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 1eaf604..c599efb 100644
--- a/diff.c
+++ b/diff.c
@@ -2839,9 +2839,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 (!S_ISREG(st.st_mode)) {
+			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
-- 
2.10.1-449-gab0f84c

Re: [RFC/PATCH 0/2] git diff <(command1) <(command2)

[Junio C Hamano]

Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:

> No tests or documentation updates yet, and I'm not sure whether
> --follow-symlinks in other modes than --no-index should be supported, ignored
> (as it is now) or cause an error, but I'm leaning towards the third option.

My knee-jerk reaction is:

 * The --no-index mode should default to your --follow-symlinks
   behaviour, without any option to turn it on or off.

 * If normal "diff" that follows symlinks by default has an option
   to disable it, then it is OK to also add --no-follow-symlinks
   that is only valid in the --no-index mode, so that we can mimick
   it better (I do not think this is the case, though).

 * Other modes should not follow symbolic links ever, no need for a
   new option.

In any case, I'd advise you not to reroll this too quickly and
frequently until the end of this cycle.  During a feature freeze, I
won't take new topics in 'pu' as that would add more things I need
to worry about, and if you reroll in too quick succession, it will
become harder to identify the latest set and queue after the
release.

Thanks.

Re: [RFC/PATCH 0/2] git diff <(command1) <(command2)

[Jacob Keller]

On Fri, Nov 11, 2016 at 1:27 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:
>
>> No tests or documentation updates yet, and I'm not sure whether
>> --follow-symlinks in other modes than --no-index should be supported, ignored
>> (as it is now) or cause an error, but I'm leaning towards the third option.
>
> My knee-jerk reaction is:
>
>  * The --no-index mode should default to your --follow-symlinks
>    behaviour, without any option to turn it on or off.
>

I agree. We shouldn't have to specify this for no-index.

Thanks,
Jake

Re: [RFC/PATCH 0/2] git diff <(command1) <(command2)

[Johannes Schindelin]

Hi,

On Fri, 11 Nov 2016, Jacob Keller wrote:

> On Fri, Nov 11, 2016 at 1:27 PM, Junio C Hamano <gitster@pobox.com> wrote:
> > Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:
> >
> >> No tests or documentation updates yet, and I'm not sure whether
> >> --follow-symlinks in other modes than --no-index should be supported, ignored
> >> (as it is now) or cause an error, but I'm leaning towards the third option.
> >
> > My knee-jerk reaction is:
> >
> >  * The --no-index mode should default to your --follow-symlinks
> >    behaviour, without any option to turn it on or off.
> >
> 
> I agree. We shouldn't have to specify this for no-index.

Ummm. *My* idea of --no-index was for it to behave as similar to the
--index version as possible. For example when comparing directories
containing symlinks. You seem intent on breaking this scenario.

Ciao,
Johannes

Re: [RFC/PATCH 0/2] git diff <(command1) <(command2)

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Fri, 11 Nov 2016, Jacob Keller wrote:
>
>> On Fri, Nov 11, 2016 at 1:27 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> > Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:
>> >
>> >> No tests or documentation updates yet, and I'm not sure whether
>> >> --follow-symlinks in other modes than --no-index should be supported, ignored
>> >> (as it is now) or cause an error, but I'm leaning towards the third option.
>> >
>> > My knee-jerk reaction is:
>> >
>> >  * The --no-index mode should default to your --follow-symlinks
>> >    behaviour, without any option to turn it on or off.
>> >
>> 
>> I agree. We shouldn't have to specify this for no-index.
>
> Ummm. *My* idea of --no-index was for it to behave as similar to the
> --index version as possible. For example when comparing directories
> containing symlinks. You seem intent on breaking this scenario.

Perhaps a viable compromise between the two is to only always
dereference at the top-level (i.e. the trees to be compared) under
"--no-index" mode and not changing anything else?

The original use case by Dennis is not even about doing a recursive
two-directories-in-a-filesystem comparison and encountering a
symbolic link (it was to compare two BLOBs, which happen to be
output from two commands).

Re: [RFC/PATCH 0/2] git diff <(command1) <(command2)

[Michael J Gruber]

Johannes Schindelin venit, vidit, dixit 12.11.2016 11:08:
> Hi,
> 
> On Fri, 11 Nov 2016, Jacob Keller wrote:
> 
>> On Fri, Nov 11, 2016 at 1:27 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:
>>>
>>>> No tests or documentation updates yet, and I'm not sure whether
>>>> --follow-symlinks in other modes than --no-index should be supported, ignored
>>>> (as it is now) or cause an error, but I'm leaning towards the third option.
>>>
>>> My knee-jerk reaction is:
>>>
>>>  * The --no-index mode should default to your --follow-symlinks
>>>    behaviour, without any option to turn it on or off.
>>>
>>
>> I agree. We shouldn't have to specify this for no-index.
> 
> Ummm. *My* idea of --no-index was for it to behave as similar to the
> --index version as possible. For example when comparing directories
> containing symlinks. You seem intent on breaking this scenario.

*My* idea of --no-index was for it to behave as similar to the
--index-version as possible, regarding formatting etc., and to be a good
substitute for ordinary diff. The proposed patch achieves exactly that -
why should a *file* argument (which is not a pathspec in --no-index
mode) not be treated in the same way in which every other command treats
a file argument? The patch un-breaks the most natural expectation.

Michael

Re: [RFC/PATCH 0/2] git diff <(command1) <(command2)

[Johannes Schindelin]

Hi Michael,

On Mon, 14 Nov 2016, Michael J Gruber wrote:

> why should a *file* argument (which is not a pathspec in --no-index
> mode) not be treated in the same way in which every other command treats
> a file argument?

We are talking about `<(command)` here, which is most distinctly not a
file argument at all.

Ciao,
Johannes

Re: [RFC/PATCH 0/2] git diff <(command1) <(command2)

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

> *My* idea of --no-index was for it to behave as similar to the
> --index-version as possible, regarding formatting etc., and to be a good
> substitute for ordinary diff. The proposed patch achieves exactly that -

Does it?  It looks to me that it does a lot more.

> why should a *file* argument (which is not a pathspec in --no-index
> mode) not be treated in the same way in which every other command treats
> a file argument? The patch un-breaks the most natural expectation.

I think a filename given as a command line argument, e.g. <(cmd), is
now treated more sensibly with [2/2].  Something that is not a
directory to be descended into and is not a regular file needs to be
made into a form that we can use as a blob, and reading it into an
in-core buffer is a workable way to do so.  

However, when taken together with [1/2], doesn't the proposed patch
"achieves" a lot more than "exactly that", namely, by not treating
symbolic links discovered during traversals of directories given
from the command line as such and dereferencing?

Re: [RFC/PATCH 0/2] git diff <(command1) <(command2)

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 14.11.2016 19:01:
> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
>> *My* idea of --no-index was for it to behave as similar to the
>> --index-version as possible, regarding formatting etc., and to be a good
>> substitute for ordinary diff. The proposed patch achieves exactly that -
> 
> Does it?  It looks to me that it does a lot more.

Yes, I didn't mean to say that it achieves only that - it achieves that
one goal exactly, and more.

>> why should a *file* argument (which is not a pathspec in --no-index
>> mode) not be treated in the same way in which every other command treats
>> a file argument? The patch un-breaks the most natural expectation.
> 
> I think a filename given as a command line argument, e.g. <(cmd), is
> now treated more sensibly with [2/2].  Something that is not a
> directory to be descended into and is not a regular file needs to be
> made into a form that we can use as a blob, and reading it into an
> in-core buffer is a workable way to do so.  

Yes.

> However, when taken together with [1/2], doesn't the proposed patch
> "achieves" a lot more than "exactly that", namely, by not treating
> symbolic links discovered during traversals of directories given
> from the command line as such and dereferencing?

It's not clear to me what you are saying here - 1/2 makes git diff
follow symbolic links, yes, just like ordinary diff. If I 'diff' two
dirs that contain symbolic links with the same name pointing to
different files I get a diff between the contents, not between the
filenames.

I like the proposed change a lot, maybe that didn't come across clearly.
I think it makes things more "predictable" in the sense that it meets
typical expectations.

Michael

Re: [RFC/PATCH 0/2] git diff <(command1) <(command2)

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

> Junio C Hamano venit, vidit, dixit 14.11.2016 19:01:
>> Michael J Gruber <git@drmicha.warpmail.net> writes:
>> 
>>> *My* idea of --no-index was for it to behave as similar to the
>>> --index-version as possible, regarding formatting etc., and to be a good
>>> substitute for ordinary diff. The proposed patch achieves exactly that -
>>  ...
> It's not clear to me what you are saying here - 1/2 makes git diff
> follow symbolic links, yes, just like ordinary diff.

Yes, which can be seen as deviating from your earlier "as similar to
the --index version as possible" goal, which I think was where Dscho's
complaint comes from.

I _think_ the no-index mode was primarily for those who want to use
our diff as a replacement for GNU and other diffs, and from that
point of view, I'd favour not doing the "comparing symbolic link?
We'll show the difference between the link contents, not target"
under no-index mode myself.  That is a lot closer to the diff other
people implemented, not ours.  Hence the knee-jerk reaction I gave
in

http://public-inbox.org/git/xmqqinrt1zcx.fsf@gitster.mtv.corp.google.com

Re: [RFC/PATCH 0/2] git diff <(command1) <(command2)

[Johannes Schindelin]

Hi Junio,

On Mon, 14 Nov 2016, Junio C Hamano wrote:

> I _think_ the no-index mode was primarily for those who want to use
> our diff as a replacement for GNU and other diffs, and from that
> point of view, I'd favour not doing the "comparing symbolic link?
> We'll show the difference between the link contents, not target"
> under no-index mode myself.

If I read this correctly, then we are in agreement that the default for
--no-index should be as it is right now, i.e. comparing symlink targets as
opposed to --follow-links.

> That is a lot closer to the diff other people implemented, not ours.
> Hence the knee-jerk reaction I gave in
> 
> http://public-inbox.org/git/xmqqinrt1zcx.fsf@gitster.mtv.corp.google.com

Let me quote the knee-jerk reaction:

> My knee-jerk reaction is:
>
>  * The --no-index mode should default to your --follow-symlinks
>    behaviour, without any option to turn it on or off.

But this is the exact opposite of what I find reasonable.

Ciao,
Johannes

Re: [RFC/PATCH 0/2] git diff <(command1) <(command2)

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Mon, 14 Nov 2016, Junio C Hamano wrote:
>
>> I _think_ the no-index mode was primarily for those who want to use
>> our diff as a replacement for GNU and other diffs, and from that
>> point of view, I'd favour not doing the "comparing symbolic link?
>> We'll show the difference between the link contents, not target"
>> under no-index mode myself.
>
> If I read this correctly,...

Now I re-read it and I can see it can be read either way.

By "link contents" in "comparing symbolic link? We'll show the
difference between the link contents, not target", I meant the
result you get from readlink(2), which will result in

    diff --git a/RelNotes b/RelNotes
    index c02235fe8c..b54330f7cd 120000
    --- a/RelNotes
    +++ b/RelNotes
    @@ -1 +1 @@
    -Documentation/RelNotes/2.10.2.txt
    \ No newline at end of file
    +Documentation/RelNotes/2.11.0.txt
    \ No newline at end of file

not the comparison between the files that are link targets,
i.e. hypothetical

    diff --git a/RelNotes b/RelNotes
    index c4d4397023..7a1fce7720 100644
    --- a/Documentation/RelNotes/2.10.2.txt
    +++ b/Documentation/RelNotes/2.11.0.txt
    @@ -1,41 +1,402 @@
    -Git v2.10.2 Release Notes
    -=========================
    +Git 2.11 Release Notes
    ...

And I'd favour *NOT* doing that if we are using our diff as a
replacement for GNU and other diffs in "no-index" mode.  Which leads
to ...

>> That is a lot closer to the diff other people implemented, not ours.
>> Hence the knee-jerk reaction I gave in
>> 
>> http://public-inbox.org/git/xmqqinrt1zcx.fsf@gitster.mtv.corp.google.com

... this conclusion, which is consistent with ...

>
> Let me quote the knee-jerk reaction:
>
>> My knee-jerk reaction is:
>>
>>  * The --no-index mode should default to your --follow-symlinks
>>    behaviour, without any option to turn it on or off.

... this one.

But notice "I _think_" in the first sentence you quoted.  That is a
basic assumption that leads to the conclusion, and that assumption
is not a fact.  Maybe users do *not* want the "no-index" mode as a
replacement for GNU and other diffs, in which case comparing the
result of readlink(2) even in no-index mode might have merit.  I
just didn't think it was the case.

Re: [RFC/PATCH 0/2] git diff <(command1) <(command2)

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>> On Mon, 14 Nov 2016, Junio C Hamano wrote:
>>
>>> I _think_ the no-index mode was primarily for those who want to use
>>> our diff as a replacement for GNU and other diffs, and from that
>>> point of view, I'd favour not doing the "comparing symbolic link?
>>> We'll show the difference between the link contents, not target"
>>> under no-index mode myself.
>>
>> If I read this correctly,...
>
> Now I re-read it and I can see it can be read either way.
>
> By "link contents" in "comparing symbolic link? We'll show the
> difference between the link contents, not target", I meant the
> result you get from readlink(2), which will result in
>
>     diff --git a/RelNotes b/RelNotes
>     index c02235fe8c..b54330f7cd 120000
>     --- a/RelNotes
>     +++ b/RelNotes
>     @@ -1 +1 @@
>     -Documentation/RelNotes/2.10.2.txt
>     \ No newline at end of file
>     +Documentation/RelNotes/2.11.0.txt
>     \ No newline at end of file
>
> not the comparison between the files that are link targets,
> i.e. hypothetical
>
>     diff --git a/RelNotes b/RelNotes
>     index c4d4397023..7a1fce7720 100644
>     --- a/Documentation/RelNotes/2.10.2.txt
>     +++ b/Documentation/RelNotes/2.11.0.txt
>     @@ -1,41 +1,402 @@
>     -Git v2.10.2 Release Notes
>     -=========================
>     +Git 2.11 Release Notes
>     ...
>
> And I'd favour *NOT* doing that if we are using our diff as a

Again, this can be read both ways.  By "that" on the above line I
meant "the former".

> replacement for GNU and other diffs in "no-index" mode.  Which leads
> to ...
>
>>> That is a lot closer to the diff other people implemented, not ours.
>>> Hence the knee-jerk reaction I gave in
>>> 
>>> http://public-inbox.org/git/xmqqinrt1zcx.fsf@gitster.mtv.corp.google.com
>
> ... this conclusion, which is consistent with ...
>
>>
>> Let me quote the knee-jerk reaction:
>>
>>> My knee-jerk reaction is:
>>>
>>>  * The --no-index mode should default to your --follow-symlinks
>>>    behaviour, without any option to turn it on or off.
>
> ... this one.
>
> But notice "I _think_" in the first sentence you quoted.  That is a
> basic assumption that leads to the conclusion, and that assumption
> is not a fact.  Maybe users do *not* want the "no-index" mode as a
> replacement for GNU and other diffs, in which case comparing the
> result of readlink(2) even in no-index mode might have merit.  I
> just didn't think it was the case.

And "I just didn't think it was the case", when fully spelt out, is
"I just didn't think that the assumption was incorrect."

Re: [RFC/PATCH 0/2] git diff <(command1) <(command2)

[Dennis Kaarsemaker]

On Fri, 2016-11-11 at 13:27 -0800, Junio C Hamano wrote:
> Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:
> 
> > No tests or documentation updates yet, and I'm not sure whether
> > --follow-symlinks in other modes than --no-index should be supported, ignored
> > (as it is now) or cause an error, but I'm leaning towards the third option.
> 
> 
> My knee-jerk reaction is:
> 
>  * The --no-index mode should default to your --follow-symlinks
>    behaviour, without any option to turn it on or off.

ok.

>  * If normal "diff" that follows symlinks by default has an option
>    to disable it, then it is OK to also add --no-follow-symlinks
>    that is only valid in the --no-index mode, so that we can mimick
>    it better (I do not think this is the case, though).

It does not, so no new option.

>  * Other modes should not follow symbolic links ever, no need for a
>    new option.

Makes sense.

> In any case, I'd advise you not to reroll this too quickly and
> frequently until the end of this cycle.  During a feature freeze, I
> won't take new topics in 'pu' as that would add more things I need
> to worry about, and if you reroll in too quick succession, it will
> become harder to identify the latest set and queue after the
> release.

I'm in no hurry, so I'll sit on this until v2.11 is done.

D.

Re: [RFC/PATCH 0/2] git diff <(command1) <(command2)

[Jeff King]

On Sat, Nov 12, 2016 at 07:11:34AM +0100, Dennis Kaarsemaker wrote:

> >  * If normal "diff" that follows symlinks by default has an option
> >    to disable it, then it is OK to also add --no-follow-symlinks
> >    that is only valid in the --no-index mode, so that we can mimick
> >    it better (I do not think this is the case, though).
> 
> It does not, so no new option.

There is "--no-dereference", but it is not that helpful:

  $ echo one >1
  $ echo two >2
  $ ln -s 1 sl1
  $ ln -s 2 sl2
  $ diff --no-dereference sl1 sl2
  Symbolic links sl1 and sl2 differ
  $ diff --no-dereference sl1 2
  File sl1 is a symbolic link while file 2 is a regular file

So it does do the thing we are talking about here, but diff's handling
of non-file content is not nearly as useful as git's.

That said, I have no problem with proceeding without it and waiting
until somebody actually shows up who really _wants_ --no-dereference. I
won't be surprised if that never happens.

-Peff

Re: [RFC/PATCH 0/2] git diff <(command1) <(command2)

[Jacob Keller]

On Fri, Nov 11, 2016 at 12:19 PM, Dennis Kaarsemaker
<dennis@kaarsemaker.net> wrote:
> Today on #git, a user asked why git diff <(command1) <(command2) gave only some
> gibberish about pipes as output. The answer is fairly simple: git diff gets as
> arguments /dev/fd/62 and /dev/fd/63, which are symlinks. So git simply
> readlink()s them and gets pipe:[123456] as destination of that link which it
> then outputs.
>
> Given that 'normal' diff provides arguably better output in this case (a diff
> of the output of the two commands), I wanted to look at what it would take for
> git to handle this. Surprisingly: not much. 1/2 adds support for
> --follow-symlinks to git diff --no-index (and only the --no-index variant) and
> 2/2 adds support for reading from pipes.
>

I think this is really useful. I have an alias so that "diff" is just
git diff --no-index. It's really useful, because I find the output of
git-diff to be much better. Not being able to diff from pipes or
symlinks is something I've run into before and it's annoying. So I
could really use this.

Thanks,
Jake

> No tests or documentation updates yet, and I'm not sure whether
> --follow-symlinks in other modes than --no-index should be supported, ignored
> (as it is now) or cause an error, but I'm leaning towards the third option.
>
> Dennis Kaarsemaker (2):
>   diff --no-index: add option to follow symlinks
>   diff --no-index: support reading from pipes
>
>  diff-no-index.c | 15 ++++++++++++---
>  diff.c          | 23 +++++++++++++++++++----
>  diff.h          |  2 +-
>  3 files changed, 32 insertions(+), 8 deletions(-)
>
> --
> 2.10.1-449-gab0f84c
>