threads / rfc / 44467

RFC patch, 2 partsgit diff <(command1) <(command2)

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

## tl;dr

18 messages between Nov 11, 2016 and Nov 16, 2016. Diffs are folded; open one to read it.

replies: 17people: 6as markdown or json

Dennis Kaarsemaker· Nov 11, 2016, 20:19 UTC · lore

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
Dennis Kaarsemaker· Nov 11, 2016, 20:19 UTC · re: Dennis Kaarsemaker · lore

[PATCH 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 |  8 ++++++++
 diff.c          | 13 +++++++++++--
 2 files changed, 19 insertions(+), 2 deletions(-)
Show changes to 2 files +19 −2

diff-no-index.c, diff.c

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
Dennis Kaarsemaker· Nov 11, 2016, 20:19 UTC · re: Dennis Kaarsemaker · lore

[PATCH 1/2] diff --no-index: add option to 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.

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(-)
Show changes to 3 files +13 −6

diff-no-index.c, diff.c, diff.h

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
Junio C Hamano· Nov 11, 2016, 21:27 UTC · re: Dennis Kaarsemaker · lore

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

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.
Jacob Keller· Nov 11, 2016, 23:14 UTC · re: Junio C Hamano · lore

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

On Fri, Nov 11, 2016 at 1:27 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 11 quoted lines
> 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

Johannes Schindelin· Nov 12, 2016, 10:08 UTC · re: Jacob Keller · lore

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

Hi,
On Fri, 11 Nov 2016, Jacob Keller wrote:
Show 14 quoted lines
> 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

Junio C Hamano· Nov 14, 2016, 03:14 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 20 quoted lines
> 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).

Michael J Gruber· Nov 14, 2016, 15:31 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin venit, vidit, dixit 12.11.2016 11:08:
Show 22 quoted lines
> 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
Johannes Schindelin· Nov 14, 2016, 16:40 UTC · re: Michael J Gruber · lore

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

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

Junio C Hamano· Nov 14, 2016, 18:01 UTC · re: Michael J Gruber · lore

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

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?

Michael J Gruber· Nov 14, 2016, 20:23 UTC · re: Junio C Hamano · lore

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

Junio C Hamano venit, vidit, dixit 14.11.2016 19:01:
Show 7 quoted lines
> 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.

Show 9 quoted lines
>> 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
Junio C Hamano· Nov 14, 2016, 21:10 UTC · re: Michael J Gruber · lore

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

Michael J Gruber <git@drmicha.warpmail.net> writes:
Show 9 quoted lines
> 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
Johannes Schindelin· Nov 16, 2016, 09:50 UTC · re: Junio C Hamano · lore

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

Hi Junio,
On Mon, 14 Nov 2016, Junio C Hamano wrote:
Show 5 quoted lines
> 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

Junio C Hamano· Nov 16, 2016, 18:29 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 9 quoted lines
> 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 ...
Show 7 quoted lines
>
> 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.

Junio C Hamano· Nov 16, 2016, 18:37 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 42 quoted lines
> 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".

Show 26 quoted lines
> 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."

Dennis Kaarsemaker· Nov 12, 2016, 06:11 UTC · re: Junio C Hamano · lore

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

On Fri, 2016-11-11 at 13:27 -0800, Junio C Hamano wrote:
Show 11 quoted lines
> 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.
Show 6 quoted lines
> 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.
Jeff King· Nov 12, 2016, 07:06 UTC · re: Dennis Kaarsemaker · lore

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

On Sat, Nov 12, 2016 at 07:11:34AM +0100, Dennis Kaarsemaker wrote:
Show 6 quoted lines
> >  * 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
Jacob Keller· Nov 11, 2016, 23:15 UTC · re: Dennis Kaarsemaker · lore

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

On Fri, Nov 11, 2016 at 12:19 PM, Dennis Kaarsemaker <dennis@kaarsemaker.net> wrote:

Show 12 quoted lines
> 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

Show 16 quoted lines
> 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
>

← back to recent threads