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

[Junio C Hamano]

Dennis Kaarsemaker  writes:

> 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 1/2] diff --no-index: 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 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.

Signed-off-by: Dennis Kaarsemaker 
---
 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(-)

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 2d77a19626..48bcf3cc5e 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -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[=]::
Show colored diff.
`--color` (i.e. without '=') is the same as `--color=always`.
diff --git a/diff-no-index.c b/diff-no-index.c
index f420786039..826fe97ffc 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 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, ))
+   else if (no_dereference ? lstat(path, ) : stat(path, ))
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, ) || get_mode(name2, ))
+   if (get_mode(name1, , DIFF_OPT_TST(o, NO_DEREFERENCE)) ||
+   get_mode(name2, , DIFF_OPT_TST(o, NO_DEREFERENCE)))
return -1;
 
if (mode1 && mode2 && S_ISDIR(mode1) != S_ISDIR(mode2)) {
diff --git a/diff.c b/diff.c
index be11e4ef2b..2fc0226338 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(, 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, );
+   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=", )) {
int value = git_config_colorbool(NULL, arg);
diff --git a/diff.h b/diff.h
index 25ae60d5ff..74883db1eb 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_NO_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..c6046fef19 100755
--- a/t/t4053-diff-no-index.sh
+++ b/t/t4053-diff-no-index.sh
@@ -127,4 +127,34 @@ test_expect_success 'diff --no-index from repo subdir 
respects config (implicit)