diff --no-index: support symlinks and pipes
v2 cover diff --no-index: support symlinks and pipes
git diff <(command1) <(command2) is less useful than it could be, all it outputs is:
dev/fd/62 renamed from dev/fd/63 +1 -1
@@ -1 +1 @@ -pipe:[464811685] +pipe:[464811687]
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/[email protected]/
Changes since the RFC/v1 patch:
- Following symlinks is now the default. I think an accurate summary of the discussion on v1 is that this behaviour is useful enough to be the default, but to add an escape hatch. That escape hatch is named --no-dereference, name stolen from gnu diff.
- Added tests and documentation
Specifically not changed:
These changes affect only diff --no-index. Using --no-dereference is an error without --no-index.
Dennis Kaarsemaker (2):
diff --no-index: follow symlinks
diff --no-index: support reading from pipes
Documentation/diff-options.txt | 7 +++++++ diff-no-index.c | 15 ++++++++++++--- diff.c | 23 +++++++++++++++++++---- diff.h | 2 +- t/t4053-diff-no-index.sh | 40 ++++++++++++++++++++++++++++++++++++++++ t/test-lib.sh | 4 ++++ 6 files changed, 83 insertions(+), 8 deletions(-)
Signature · 1 line
2.11.0-234-gaf85957
PATCH v2 1/2 diff --no-index: 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 to follow symlinks, matching the behaviour of ordinary diff. A new --no-dereference (name copied from diff) option has been added to disable this behaviour.
--- Documentation/diff-options.txt | 7 +++++++ diff-no-index.c | 7 ++++--- diff.c | 10 ++++++++-- diff.h | 2 +- t/t4053-diff-no-index.sh | 30 ++++++++++++++++++++++++++++++ 5 files changed, 50 insertions(+), 6 deletions(-)
Documentation/diff-options.txt +7 -0
@@ -216,6 +216,13 @@ any of those replacements occurred. commit range. Defaults to `diff.submodule` or the 'short' format if the config option is unset. +ifdef::git-diff[] +--no-dereference:: + Normally, "git diff --no-index" will dereference symlinks and compare + the contents of the linked files, mimicking ordinary diff. This + option disables that behaviour. +endif::git-diff[] + --color[=<when>]:: Show colored diff. `--color` (i.e. without '=<when>') is the same as `--color=always`.
diff-no-index.c +4 -3
@@ -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 no_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 (no_dereference ? lstat(path, &st) : stat(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,NO_DEREFERENCE )) || +get_mode (name2, &mode2,DIFF_OPT_TST (o,NO_DEREFERENCE )))return -1 ;if (mode1 && mode2 &&S_ISDIR (mode1) !=S_ISDIR (mode2)) {
diff.c +8 -2
@@ -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,"--no-dereference" )) +DIFF_OPT_SET (options,NO_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.h +1 -1
@@ -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_NO_DEREFERENCE (1 << 9)#define DIFF_OPT_HAS_CHANGES (1 << 10)#define DIFF_OPT_QUICK (1 << 11)#define DIFF_OPT_NO_INDEX (1 << 12)
t/t4053-diff-no-index.sh +30 -0
@@ -127,4 +127,34 @@ test_expect_success 'diff --no-index from repo subdir respects config (implicit) test_cmp expect actual.head ' +test_expect_success SYMLINKS 'diff --no-index 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 @@ + -a + +b + 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 --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
Signature · 1 line
2.11.0-234-gaf85957
Re: [PATCH v2 1/2] diff --no-index: follow symlinks
Quoted 6 lines · 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 to follow symlinks, matching the behaviour of ordinary diff. A new --no-dereference (name copied from diff) option has been added to disable this behaviour.
If you add a --no-dereference option, --dereference option should also be there, so that "--no-dereference" earlier on the command line (perhaps coming from a configured alias) can be countermanded.
While I am not opposed to giving an optional feature to treat a symlink as if it is a regular file with the contents of its link target, I am not enthused that this patch tries to make that the default behaviour. We are not matching the behaviour of ordinary diff anyway [*1*].
It probably makes more sense for our first step to introduce this feature that is only enabled when "--dereference" option is given. Making it the default for "--no-index" case should be discussed as a separate step.
[Footnote]
*1* E.g. "git diff --no-index dirA/ dirB/" does not say "Only in
dirA: file". It also recurses into subdirectories of dirA/ and dirB/ without the --recursive option.
PATCH v2 2/2 diff --no-index: support reading from pipes
In reply to Dennis Kaarsemakerdiff <(command1) <(command2) provides useful output, let's make it possible for git to do the same.
--- diff-no-index.c | 8 ++++++++ diff.c | 13 +++++++++++-- t/t4053-diff-no-index.sh | 10 ++++++++++ t/test-lib.sh | 4 ++++ 4 files changed, 33 insertions(+), 2 deletions(-)
diff-no-index.c +8 -0
@@ -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.c +11 -2
@@ -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
t/t4053-diff-no-index.sh +10 -0
@@ -157,4 +157,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 <(echo 1) <(echo 2) | tail -n +5 > actual && + test_cmp expect actual +' + test_done
t/test-lib.sh +4 -0
@@ -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 +'
Signature · 1 line
2.11.0-234-gaf85957
Re: [PATCH v2 2/2] diff --no-index: support reading from pipes
diff-no-index.c static struct diff_filespec *noindex_filespec(const char *name, int mode) · line 93
Quoted 8 lines · Dennis Kaarsemaker
diff-no-index.c +8
-0+/* +* 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);
Hmph. Pipes on your system may satisfy S_ISGITLINK() and confuse later code, and this hack may work it around. But a proper gitlink that was thrown at this codepath (probably by mistake) will also be caught and pretend as if it were a regular file. Do we know for certain that pipes everywhere will be munged to appear as S_ISGITLINK()? Is it possible to do the "are we looking at an end of a pipe?" check _before_ canon_mode() munges and stores the result in s->mode in diff-no-index.c somewhere, perhaps inside get_mode()?
Quoted 12 lines
diff.c +3
-1@@ -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 );
diff.c int diff_populate_filespec(struct diff_filespec *s, unsigned int flags) · line 2844
Quoted 10 lines
diff.c +8
-1+ 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 ;
I like the fact that, by extending the !S_ISREG() check this patch introduces, we can later use the new "do not mmap but allocate to read" codepath for small files, which may be more efficient. We may want to have a small helper
static int should_mmap_file_contents(struct stat *st)
{
return S_ISREG(st->st_mode);
}so that we can do such an enhancement later more easily.
So, I am skeptical with the "do we have pipe" check in the earlier hunk, but otherwise I think what this patch wanted to solve is a reasonable problem to tackle.
Thanks.