problematic git log submodule-dir/

problematic git log submodule-dir/

[Paweł Sikora]

Hi all.

i've noticed that 'git log submodule-dir' and 'git log submodule-dir/'
return different results (module's head changes vs. nothing). is it a bug?
looks like a trailing slash is a problem for git-log.

tested on git-1.8.5.3-2.fc20.x86_64.

BR,
Paweł.

-- 
gpg key fingerprint = 60B4 9886 AD53 EB3E 88BB 1EB5 C52E D01B 683B 9411

Re: problematic git log submodule-dir/

[Jens Lehmann]

Am 20.01.2014 19:25, schrieb Paweł Sikora:
> i've noticed that 'git log submodule-dir' and 'git log submodule-dir/'
> return different results (module's head changes vs. nothing). is it a bug?
> looks like a trailing slash is a problem for git-log.

I think this is a bug, and for example "git diff" has similar problems.
This is especially nasty as shell auto-completion adds the '/' at the
end.

Duy, without having looked into the code myself yet: is this something
that might be easily fixed by using PATHSPEC_STRIP_SUBMODULE_SLASH*?

Re: problematic git log submodule-dir/

[Duy Nguyen]

On Thu, Jan 23, 2014 at 3:35 AM, Jens Lehmann <Jens.Lehmann@web.de> wrote:
> Am 20.01.2014 19:25, schrieb Paweł Sikora:
>> i've noticed that 'git log submodule-dir' and 'git log submodule-dir/'
>> return different results (module's head changes vs. nothing). is it a bug?
>> looks like a trailing slash is a problem for git-log.
>
> I think this is a bug, and for example "git diff" has similar problems.
> This is especially nasty as shell auto-completion adds the '/' at the
> end.
>
> Duy, without having looked into the code myself yet: is this something
> that might be easily fixed by using PATHSPEC_STRIP_SUBMODULE_SLASH*?

I think so. But I dislike those preprocessing because it looks
inefficient and the same change may be needed for other diff commands
as well. Maybe we can handle this at diff or log-tree.c level. Will
look further into it tonight.
-- 
Duy

[PATCH 1/2] tree-walk.c: ignore trailing slash on submodule in tree_entry_interesting()

[Nguyễn Thái Ngọc Duy]

We do ignore trailing slash on a directory, so pathspec "abc/" matches
directory "abc". A submodule is also a directory. Apply the same logic
to it. This makes "git log submodule-path" and "git log submodule-path/"
produce the same output.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 t/t4010-diff-pathspec.sh | 17 +++++++++++++++++
 tree-walk.c              |  2 +-
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/t/t4010-diff-pathspec.sh b/t/t4010-diff-pathspec.sh
index af5134b..15a4912 100755
--- a/t/t4010-diff-pathspec.sh
+++ b/t/t4010-diff-pathspec.sh
@@ -110,4 +110,21 @@ test_expect_success 'diff-tree -r with wildcard' '
 	test_cmp expected result
 '
 
+test_expect_success 'setup submodules' '
+	test_tick &&
+	git init submod &&
+	( cd submod && test_commit first; ) &&
+	git add submod &&
+	git commit -m first &&
+	( cd submod && test_commit second; ) &&
+	git add submod &&
+	git commit -m second
+'
+
+test_expect_success 'diff-tree ignores trailing slash on submodule path' '
+	git diff --name-only HEAD^ HEAD submod >expect &&
+	git diff --name-only HEAD^ HEAD submod/ >actual &&
+	test_cmp expect actual
+'
+
 test_done
diff --git a/tree-walk.c b/tree-walk.c
index 680afda..c29b6a3 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -543,7 +543,7 @@ static int match_entry(const struct pathspec_item *item,
 	if (matchlen > pathlen) {
 		if (match[pathlen] != '/')
 			return 0;
-		if (!S_ISDIR(entry->mode))
+		if (!S_ISDIR(entry->mode) && !S_ISGITLINK(entry->mode))
 			return 0;
 	}
 
-- 
1.8.5.2.240.g8478abd

[PATCH 2/2] t4010: match_pathspec_depth() and trailing slash after submodule

[Nguyễn Thái Ngọc Duy]

When you do "git diff HEAD submodule/", "submodule" from the index is
picked out and match_pathspec_depth() in charge of matching it with
the pathspec "submodule/".

Unlike tree_entry_interesting(), match_pathspec_depth() has no
knowledge about entry mode to realize "submodule" is a directory and
treat the trailing slash specially. And it does not have too, mostly,
because the index only contains files, not directories (not until
submodules come)

I have no solutions for it (no, stripping '/' at pathspec
preprocessing phase seems like a workaround than a solution). So let's
mark it. Maybe I or somebody else could revisit it later.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 t/t4010-diff-pathspec.sh | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/t/t4010-diff-pathspec.sh b/t/t4010-diff-pathspec.sh
index 15a4912..b54251a 100755
--- a/t/t4010-diff-pathspec.sh
+++ b/t/t4010-diff-pathspec.sh
@@ -127,4 +127,10 @@ test_expect_success 'diff-tree ignores trailing slash on submodule path' '
 	test_cmp expect actual
 '
 
+test_expect_failure 'diff-cache ignores trailing slash on submodule path' '
+	git diff --name-only HEAD^ submod >expect &&
+	git diff --name-only HEAD^ submod/ >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
1.8.5.2.240.g8478abd

Re: [PATCH 2/2] t4010: match_pathspec_depth() and trailing slash after submodule

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> When you do "git diff HEAD submodule/", "submodule" from the index is
> picked out and match_pathspec_depth() in charge of matching it with
> the pathspec "submodule/".

Is "... is called" or something missing at the end of this sentence?

> Unlike tree_entry_interesting(), match_pathspec_depth() has no
> knowledge about entry mode to realize "submodule" is a directory and
> treat the trailing slash specially. And it does not have too, mostly,

s/too/to/, I think.

> because the index only contains files, not directories (not until
> submodules come)
>
> I have no solutions for it (no, stripping '/' at pathspec
> preprocessing phase seems like a workaround than a solution). So let's
> mark it. Maybe I or somebody else could revisit it later.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  t/t4010-diff-pathspec.sh | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/t/t4010-diff-pathspec.sh b/t/t4010-diff-pathspec.sh
> index 15a4912..b54251a 100755
> --- a/t/t4010-diff-pathspec.sh
> +++ b/t/t4010-diff-pathspec.sh
> @@ -127,4 +127,10 @@ test_expect_success 'diff-tree ignores trailing slash on submodule path' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_failure 'diff-cache ignores trailing slash on submodule path' '
> +	git diff --name-only HEAD^ submod >expect &&
> +	git diff --name-only HEAD^ submod/ >actual &&

I actually doubt that the second line is expecting the right
behaviour in the first place.  As far as the top-level project is
concerned, "submod" is the name it wants, as there is nothing
underneath it.  Even if asked to recurse infinite levels, the caller
shouldn't be feeding paths like "submod/a/b/c" to
match_pathspec_depth() in the first place, no?



> +	test_cmp expect actual
> +'
> +
>  test_done

Re: [PATCH 2/2] t4010: match_pathspec_depth() and trailing slash after submodule

[Jens Lehmann]

Am 23.01.2014 22:09, schrieb Junio C Hamano:
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>> diff --git a/t/t4010-diff-pathspec.sh b/t/t4010-diff-pathspec.sh
>> index 15a4912..b54251a 100755
>> --- a/t/t4010-diff-pathspec.sh
>> +++ b/t/t4010-diff-pathspec.sh
>> @@ -127,4 +127,10 @@ test_expect_success 'diff-tree ignores trailing slash on submodule path' '
>>  	test_cmp expect actual
>>  '
>>  
>> +test_expect_failure 'diff-cache ignores trailing slash on submodule path' '
>> +	git diff --name-only HEAD^ submod >expect &&
>> +	git diff --name-only HEAD^ submod/ >actual &&
> 
> I actually doubt that the second line is expecting the right
> behaviour in the first place.  As far as the top-level project is
> concerned, "submod" is the name it wants, as there is nothing
> underneath it.  Even if asked to recurse infinite levels, the caller
> shouldn't be feeding paths like "submod/a/b/c" to
> match_pathspec_depth() in the first place, no?

Agreed, "submod/a/b/c" would not make sense here. But a single
trailing '/' does mark "submod" as a directory, which I think is
ok for a submodule. And it makes life easier for the user if we
accept that, as shell completion will add it there automatically.

Re: [PATCH 2/2] t4010: match_pathspec_depth() and trailing slash after submodule

[Junio C Hamano]

Jens Lehmann <Jens.Lehmann@web.de> writes:

> ... But a single
> trailing '/' does mark "submod" as a directory, which I think is
> ok for a submodule. And it makes life easier for the user if we
> accept that, as shell completion will add it there automatically.

OK, that would be annoying.

Perhaps the completion is what is broken here, then?  I dunno, and
haven't thought things through.

Re: [PATCH 2/2] t4010: match_pathspec_depth() and trailing slash after submodule

[Duy Nguyen]

On Fri, Jan 24, 2014 at 4:38 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jens Lehmann <Jens.Lehmann@web.de> writes:
>
>> ... But a single
>> trailing '/' does mark "submod" as a directory, which I think is
>> ok for a submodule. And it makes life easier for the user if we
>> accept that, as shell completion will add it there automatically.
>
> OK, that would be annoying.
>
> Perhaps the completion is what is broken here, then?  I dunno, and
> haven't thought things through.

My reasoning is more simple minded: if "git diff HEAD HEAD^ submod/"
works, the user would expect "git diff HEAD submod/" to work too. I
think I've got something working with a bit refactoring. Will send out
soon.
-- 
Duy

[PATCH v2 0/9] About the trailing slashes

[Nguyễn Thái Ngọc Duy]

So this is the reroll that makes

  git diff HEAD submodule
  git diff HEAD submodule/

and

  git diff HEAD HEAD^ submodule
  git diff HEAD HEAD^ submodule/

behave the same way. The main patches are 04/09 and 09/09. The rest is
just refactoring and cleaning up.

While looking at this, I found a funny behavior of fill_directory.

  $ git init
  $ mkdir b
  $ >b/c
  $ >b/d
  $ git status b
  Untracked files:
          b/
  $ git status b/
  Untracked files:
          b/c b/d

Notice how the trailing slash produces different untracked listing.
This is because of common_prefix_len(). In the "b" case,
common_prefix_len() returns empty prefix, so read_directory reads top
directory, traverses through, reaches "b" and eventually calls
treat_directory() on it, which results in "b/" in the output.

In the "b/" case, common_prefix_len() found the prefix "b", so
read_directory() starts at "b" instead of b's parent.
treat_directory() is never called on "b" itself, which results in
"b/c" and "b/d".

I'm tempted to make it consistent, favoring "b/" output. But that
involves extra lstat, or reordering the complex read_directory() code.
This is probably very unusual case to pay attention to anyway.

Nguyễn Thái Ngọc Duy (9):
  Convert some match_pathspec_depth() to ce_path_match()
  Convert some match_pathspec_depth() to dir_path_match()
  Rename match_pathspec_depth() to match_pathspec()
  dir.c: prepare match_pathspec_item for taking more flags
  match_pathspec: match pathspec "foo/" against directory "foo"
  Pass directory indicator to match_pathspec_item()
  clean: replace match_pathspec() with dir_path_match()
  clean: use cache_name_is_other()
  tree-walk.c: ignore trailing slash on submodule in
    tree_entry_interesting()

 builtin/add.c             |  3 +--
 builtin/checkout.c        |  3 +--
 builtin/clean.c           | 24 +++---------------------
 builtin/commit.c          |  2 +-
 builtin/grep.c            |  6 ++----
 builtin/ls-files.c        |  9 ++++++---
 builtin/ls-tree.c         |  2 +-
 builtin/rm.c              |  2 +-
 builtin/update-index.c    |  3 ++-
 cache.h                   |  2 --
 diff-lib.c                |  5 +++--
 dir.c                     | 40 ++++++++++++++++++++++++++--------------
 dir.h                     | 24 +++++++++++++++++++++---
 pathspec.c                |  2 +-
 preload-index.c           |  3 ++-
 read-cache.c              |  8 +-------
 rerere.c                  |  4 ++--
 resolve-undo.c            |  2 +-
 revision.c                |  3 ++-
 t/t4010-diff-pathspec.sh  | 23 +++++++++++++++++++++++
 t/t6131-pathspec-icase.sh |  6 +++---
 tree-walk.c               |  2 +-
 wt-status.c               |  6 +++---
 23 files changed, 107 insertions(+), 77 deletions(-)

-- 
1.8.5.2.240.g8478abd

[PATCH v2 1/9] Convert some match_pathspec_depth() to ce_path_match()

[Nguyễn Thái Ngọc Duy]

This helps reduce the number of match_pathspec_depth() call sites and
show how match_pathspec_depth() is used.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/checkout.c     | 3 +--
 builtin/commit.c       | 2 +-
 builtin/grep.c         | 2 +-
 builtin/rm.c           | 2 +-
 builtin/update-index.c | 3 ++-
 cache.h                | 2 --
 diff-lib.c             | 5 +++--
 dir.h                  | 7 +++++++
 pathspec.c             | 2 +-
 preload-index.c        | 3 ++-
 read-cache.c           | 8 +-------
 resolve-undo.c         | 2 +-
 revision.c             | 3 ++-
 wt-status.c            | 2 +-
 14 files changed, 24 insertions(+), 22 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 5df3837..ada51fa 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -297,8 +297,7 @@ static int checkout_paths(const struct checkout_opts *opts,
 		 * match_pathspec() for _all_ entries when
 		 * opts->source_tree != NULL.
 		 */
-		if (match_pathspec_depth(&opts->pathspec, ce->name, ce_namelen(ce),
-				   0, ps_matched))
+		if (ce_path_match(ce, &opts->pathspec, ps_matched))
 			ce->ce_flags |= CE_MATCHED;
 	}
 
diff --git a/builtin/commit.c b/builtin/commit.c
index 3767478..26b2986 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -234,7 +234,7 @@ static int list_paths(struct string_list *list, const char *with_tree,
 
 		if (ce->ce_flags & CE_UPDATE)
 			continue;
-		if (!match_pathspec_depth(pattern, ce->name, ce_namelen(ce), 0, m))
+		if (!ce_path_match(ce, pattern, m))
 			continue;
 		item = string_list_insert(list, ce->name);
 		if (ce_skip_worktree(ce))
diff --git a/builtin/grep.c b/builtin/grep.c
index 63f8603..3d924c2 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -379,7 +379,7 @@ static int grep_cache(struct grep_opt *opt, const struct pathspec *pathspec, int
 		const struct cache_entry *ce = active_cache[nr];
 		if (!S_ISREG(ce->ce_mode))
 			continue;
-		if (!match_pathspec_depth(pathspec, ce->name, ce_namelen(ce), 0, NULL))
+		if (!ce_path_match(ce, pathspec, NULL))
 			continue;
 		/*
 		 * If CE_VALID is on, we assume worktree file and its cache entry
diff --git a/builtin/rm.c b/builtin/rm.c
index 3a0e0ea..0564218 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -308,7 +308,7 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 
 	for (i = 0; i < active_nr; i++) {
 		const struct cache_entry *ce = active_cache[i];
-		if (!match_pathspec_depth(&pathspec, ce->name, ce_namelen(ce), 0, seen))
+		if (!ce_path_match(ce, &pathspec, seen))
 			continue;
 		ALLOC_GROW(list.entry, list.nr + 1, list.alloc);
 		list.entry[list.nr].name = ce->name;
diff --git a/builtin/update-index.c b/builtin/update-index.c
index e3a10d7..aaa6f78 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -12,6 +12,7 @@
 #include "resolve-undo.h"
 #include "parse-options.h"
 #include "pathspec.h"
+#include "dir.h"
 
 /*
  * Default to not allowing changes to the list of files. The
@@ -564,7 +565,7 @@ static int do_reupdate(int ac, const char **av,
 		struct cache_entry *old = NULL;
 		int save_nr;
 
-		if (ce_stage(ce) || !ce_path_match(ce, &pathspec))
+		if (ce_stage(ce) || !ce_path_match(ce, &pathspec, NULL))
 			continue;
 		if (has_head)
 			old = read_one_ent(NULL, head_sha1,
diff --git a/cache.h b/cache.h
index 83a2726..3c558f8 100644
--- a/cache.h
+++ b/cache.h
@@ -500,8 +500,6 @@ extern void *read_blob_data_from_index(struct index_state *, const char *, unsig
 extern int ie_match_stat(const struct index_state *, const struct cache_entry *, struct stat *, unsigned int);
 extern int ie_modified(const struct index_state *, const struct cache_entry *, struct stat *, unsigned int);
 
-extern int ce_path_match(const struct cache_entry *ce, const struct pathspec *pathspec);
-
 #define HASH_WRITE_OBJECT 1
 #define HASH_FORMAT_CHECK 2
 extern int index_fd(unsigned char *sha1, int fd, struct stat *st, enum object_type type, const char *path, unsigned flags);
diff --git a/diff-lib.c b/diff-lib.c
index 346cac6..2eddc66 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -11,6 +11,7 @@
 #include "unpack-trees.h"
 #include "refs.h"
 #include "submodule.h"
+#include "dir.h"
 
 /*
  * diff-files
@@ -108,7 +109,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 		if (diff_can_quit_early(&revs->diffopt))
 			break;
 
-		if (!ce_path_match(ce, &revs->prune_data))
+		if (!ce_path_match(ce, &revs->prune_data, NULL))
 			continue;
 
 		if (ce_stage(ce)) {
@@ -438,7 +439,7 @@ static int oneway_diff(const struct cache_entry * const *src,
 	if (tree == o->df_conflict_entry)
 		tree = NULL;
 
-	if (ce_path_match(idx ? idx : tree, &revs->prune_data)) {
+	if (ce_path_match(idx ? idx : tree, &revs->prune_data, NULL)) {
 		do_oneway_diff(o, idx, tree);
 		if (diff_can_quit_early(&revs->diffopt)) {
 			o->exiting_early = 1;
diff --git a/dir.h b/dir.h
index 9b7e4e7..42793e5 100644
--- a/dir.h
+++ b/dir.h
@@ -205,4 +205,11 @@ extern int git_fnmatch(const struct pathspec_item *item,
 		       const char *pattern, const char *string,
 		       int prefix);
 
+static inline int ce_path_match(const struct cache_entry *ce,
+				const struct pathspec *pathspec,
+				char *seen)
+{
+	return match_pathspec_depth(pathspec, ce->name, ce_namelen(ce), 0, seen);
+}
+
 #endif
diff --git a/pathspec.c b/pathspec.c
index 6cb9fd3..8043099 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -33,7 +33,7 @@ void add_pathspec_matches_against_index(const struct pathspec *pathspec,
 		return;
 	for (i = 0; i < active_nr; i++) {
 		const struct cache_entry *ce = active_cache[i];
-		match_pathspec_depth(pathspec, ce->name, ce_namelen(ce), 0, seen);
+		ce_path_match(ce, pathspec, seen);
 	}
 }
 
diff --git a/preload-index.c b/preload-index.c
index 8c44ceb..968ee25 100644
--- a/preload-index.c
+++ b/preload-index.c
@@ -3,6 +3,7 @@
  */
 #include "cache.h"
 #include "pathspec.h"
+#include "dir.h"
 
 #ifdef NO_PTHREADS
 static void preload_index(struct index_state *index,
@@ -53,7 +54,7 @@ static void *preload_thread(void *_data)
 			continue;
 		if (ce_uptodate(ce))
 			continue;
-		if (!ce_path_match(ce, &p->pathspec))
+		if (!ce_path_match(ce, &p->pathspec, NULL))
 			continue;
 		if (threaded_has_symlink_leading_path(&cache, ce->name, ce_namelen(ce)))
 			continue;
diff --git a/read-cache.c b/read-cache.c
index 33dd676..23eb251 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -728,11 +728,6 @@ int ce_same_name(const struct cache_entry *a, const struct cache_entry *b)
 	return ce_namelen(b) == len && !memcmp(a->name, b->name, len);
 }
 
-int ce_path_match(const struct cache_entry *ce, const struct pathspec *pathspec)
-{
-	return match_pathspec_depth(pathspec, ce->name, ce_namelen(ce), 0, NULL);
-}
-
 /*
  * We fundamentally don't like some paths: we don't want
  * dot or dot-dot anywhere, and for obvious reasons don't
@@ -1149,8 +1144,7 @@ int refresh_index(struct index_state *istate, unsigned int flags,
 		if (ignore_submodules && S_ISGITLINK(ce->ce_mode))
 			continue;
 
-		if (pathspec &&
-		    !match_pathspec_depth(pathspec, ce->name, ce_namelen(ce), 0, seen))
+		if (pathspec && !ce_path_match(ce, pathspec, seen))
 			filtered = 1;
 
 		if (ce_stage(ce)) {
diff --git a/resolve-undo.c b/resolve-undo.c
index c09b006..67d1543 100644
--- a/resolve-undo.c
+++ b/resolve-undo.c
@@ -182,7 +182,7 @@ void unmerge_index(struct index_state *istate, const struct pathspec *pathspec)
 
 	for (i = 0; i < istate->cache_nr; i++) {
 		const struct cache_entry *ce = istate->cache[i];
-		if (!match_pathspec_depth(pathspec, ce->name, ce_namelen(ce), 0, NULL))
+		if (!ce_path_match(ce, pathspec, NULL))
 			continue;
 		i = unmerge_index_entry_at(istate, i);
 	}
diff --git a/revision.c b/revision.c
index a68fde6..e3dff61 100644
--- a/revision.c
+++ b/revision.c
@@ -16,6 +16,7 @@
 #include "line-log.h"
 #include "mailmap.h"
 #include "commit-slab.h"
+#include "dir.h"
 
 volatile show_early_output_fn_t show_early_output;
 
@@ -1396,7 +1397,7 @@ static void prepare_show_merge(struct rev_info *revs)
 		const struct cache_entry *ce = active_cache[i];
 		if (!ce_stage(ce))
 			continue;
-		if (ce_path_match(ce, &revs->prune_data)) {
+		if (ce_path_match(ce, &revs->prune_data, NULL)) {
 			prune_num++;
 			prune = xrealloc(prune, sizeof(*prune) * prune_num);
 			prune[prune_num-2] = ce->name;
diff --git a/wt-status.c b/wt-status.c
index 4e55810..295c09e 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -510,7 +510,7 @@ static void wt_status_collect_changes_initial(struct wt_status *s)
 		struct wt_status_change_data *d;
 		const struct cache_entry *ce = active_cache[i];
 
-		if (!ce_path_match(ce, &s->pathspec))
+		if (!ce_path_match(ce, &s->pathspec, NULL))
 			continue;
 		it = string_list_insert(&s->change, ce->name);
 		d = it->util;
-- 
1.8.5.2.240.g8478abd

[PATCH v2 2/9] Convert some match_pathspec_depth() to dir_path_match()

[Nguyễn Thái Ngọc Duy]

This helps reduce the number of match_pathspec_depth() call sites and
show how m_p_d() is used. And it usage is:

 - match against an index entry (ce_path_match or match_pathspec_depth
   in ls-files)

 - match against a dir_entry from read_directory (dir_path_match and
   match_pathspec_depth in clean.c, which will be converted later)

 - resolve-undo (rerere.c and ls-files.c)

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/add.c      | 3 +--
 builtin/grep.c     | 4 +---
 builtin/ls-files.c | 2 +-
 dir.h              | 7 +++++++
 wt-status.c        | 4 ++--
 5 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index 2a2722f..672adc0 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -208,8 +208,7 @@ static char *prune_directory(struct dir_struct *dir, struct pathspec *pathspec,
 	i = dir->nr;
 	while (--i >= 0) {
 		struct dir_entry *entry = *src++;
-		if (match_pathspec_depth(pathspec, entry->name, entry->len,
-					 prefix, seen))
+		if (dir_path_match(entry, pathspec, prefix, seen))
 			*dst++ = entry;
 		else if (flag & WARN_IMPLICIT_DOT)
 			/*
diff --git a/builtin/grep.c b/builtin/grep.c
index 3d924c2..69ac2d8 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -524,9 +524,7 @@ static int grep_directory(struct grep_opt *opt, const struct pathspec *pathspec,
 
 	fill_directory(&dir, pathspec);
 	for (i = 0; i < dir.nr; i++) {
-		const char *name = dir.entries[i]->name;
-		int namelen = strlen(name);
-		if (!match_pathspec_depth(pathspec, name, namelen, 0, NULL))
+		if (!dir_path_match(dir.entries[i], pathspec, 0, NULL))
 			continue;
 		hit |= grep_file(opt, dir.entries[i]->name);
 		if (hit && opt->status_only)
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index e1cf6d8..e238608 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -64,7 +64,7 @@ static void show_dir_entry(const char *tag, struct dir_entry *ent)
 	if (len >= ent->len)
 		die("git ls-files: internal error - directory entry not superset of prefix");
 
-	if (!match_pathspec_depth(&pathspec, ent->name, ent->len, len, ps_matched))
+	if (!dir_path_match(ent, &pathspec, len, ps_matched))
 		return;
 
 	fputs(tag, stdout);
diff --git a/dir.h b/dir.h
index 42793e5..65f54b6 100644
--- a/dir.h
+++ b/dir.h
@@ -212,4 +212,11 @@ static inline int ce_path_match(const struct cache_entry *ce,
 	return match_pathspec_depth(pathspec, ce->name, ce_namelen(ce), 0, seen);
 }
 
+static inline int dir_path_match(const struct dir_entry *ent,
+				 const struct pathspec *pathspec,
+				 int prefix, char *seen)
+{
+	return match_pathspec_depth(pathspec, ent->name, ent->len, prefix, seen);
+}
+
 #endif
diff --git a/wt-status.c b/wt-status.c
index 295c09e..a452407 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -552,7 +552,7 @@ static void wt_status_collect_untracked(struct wt_status *s)
 	for (i = 0; i < dir.nr; i++) {
 		struct dir_entry *ent = dir.entries[i];
 		if (cache_name_is_other(ent->name, ent->len) &&
-		    match_pathspec_depth(&s->pathspec, ent->name, ent->len, 0, NULL))
+		    dir_path_match(ent, &s->pathspec, 0, NULL))
 			string_list_insert(&s->untracked, ent->name);
 		free(ent);
 	}
@@ -560,7 +560,7 @@ static void wt_status_collect_untracked(struct wt_status *s)
 	for (i = 0; i < dir.ignored_nr; i++) {
 		struct dir_entry *ent = dir.ignored[i];
 		if (cache_name_is_other(ent->name, ent->len) &&
-		    match_pathspec_depth(&s->pathspec, ent->name, ent->len, 0, NULL))
+		    dir_path_match(ent, &s->pathspec, 0, NULL))
 			string_list_insert(&s->ignored, ent->name);
 		free(ent);
 	}
-- 
1.8.5.2.240.g8478abd

[PATCH v2 3/9] Rename match_pathspec_depth() to match_pathspec()

[Nguyễn Thái Ngọc Duy]

A long time ago, for some reason I was not happy with
match_pathspec(). I created a better version, match_pathspec_depth()
that was suppose to replace match_pathspec()
eventually. match_pathspec() has finally been gone since 6 months
ago. Use the shorter name for match_pathspec_depth().

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/clean.c           |  4 ++--
 builtin/ls-files.c        |  6 ++++--
 builtin/ls-tree.c         |  2 +-
 dir.c                     | 20 ++++++++++----------
 dir.h                     | 10 +++++-----
 rerere.c                  |  4 ++--
 t/t6131-pathspec-icase.sh |  6 +++---
 7 files changed, 27 insertions(+), 25 deletions(-)

diff --git a/builtin/clean.c b/builtin/clean.c
index 2f26297..f59c753 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -961,8 +961,8 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 			die_errno("Cannot lstat '%s'", ent->name);
 
 		if (pathspec.nr)
-			matches = match_pathspec_depth(&pathspec, ent->name,
-						       len, 0, NULL);
+			matches = match_pathspec(&pathspec, ent->name,
+						 len, 0, NULL);
 
 		if (S_ISDIR(st.st_mode)) {
 			if (remove_directories || (matches == MATCHED_EXACTLY)) {
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index e238608..02db0e1 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -139,7 +139,8 @@ static void show_ce_entry(const char *tag, const struct cache_entry *ce)
 	if (len >= ce_namelen(ce))
 		die("git ls-files: internal error - cache entry not superset of prefix");
 
-	if (!match_pathspec_depth(&pathspec, ce->name, ce_namelen(ce), len, ps_matched))
+	if (!match_pathspec(&pathspec, ce->name, ce_namelen(ce),
+			    len, ps_matched))
 		return;
 
 	if (tag && *tag && show_valid_bit &&
@@ -195,7 +196,8 @@ static void show_ru_info(void)
 		len = strlen(path);
 		if (len < max_prefix_len)
 			continue; /* outside of the prefix */
-		if (!match_pathspec_depth(&pathspec, path, len, max_prefix_len, ps_matched))
+		if (!match_pathspec(&pathspec, path, len,
+				    max_prefix_len, ps_matched))
 			continue; /* uninterested */
 		for (i = 0; i < 3; i++) {
 			if (!ui->mode[i])
diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
index 65ec931..51184df 100644
--- a/builtin/ls-tree.c
+++ b/builtin/ls-tree.c
@@ -171,7 +171,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
 	 * show_recursive() rolls its own matching code and is
 	 * generally ignorant of 'struct pathspec'. The magic mask
 	 * cannot be lifted until it is converted to use
-	 * match_pathspec_depth() or tree_entry_interesting()
+	 * match_pathspec() or tree_entry_interesting()
 	 */
 	parse_pathspec(&pathspec, PATHSPEC_GLOB | PATHSPEC_ICASE,
 		       PATHSPEC_PREFER_CWD,
diff --git a/dir.c b/dir.c
index d10a63f..8960ea1 100644
--- a/dir.c
+++ b/dir.c
@@ -218,7 +218,7 @@ static int match_pathspec_item(const struct pathspec_item *item, int prefix,
 	 * The normal call pattern is:
 	 * 1. prefix = common_prefix_len(ps);
 	 * 2. prune something, or fill_directory
-	 * 3. match_pathspec_depth()
+	 * 3. match_pathspec()
 	 *
 	 * 'prefix' at #1 may be shorter than the command's prefix and
 	 * it's ok for #2 to match extra files. Those extras will be
@@ -282,10 +282,10 @@ static int match_pathspec_item(const struct pathspec_item *item, int prefix,
  * pathspec did not match any names, which could indicate that the
  * user mistyped the nth pathspec.
  */
-static int match_pathspec_depth_1(const struct pathspec *ps,
-				  const char *name, int namelen,
-				  int prefix, char *seen,
-				  int exclude)
+static int do_match_pathspec(const struct pathspec *ps,
+			     const char *name, int namelen,
+			     int prefix, char *seen,
+			     int exclude)
 {
 	int i, retval = 0;
 
@@ -350,15 +350,15 @@ static int match_pathspec_depth_1(const struct pathspec *ps,
 	return retval;
 }
 
-int match_pathspec_depth(const struct pathspec *ps,
-			 const char *name, int namelen,
-			 int prefix, char *seen)
+int match_pathspec(const struct pathspec *ps,
+		   const char *name, int namelen,
+		   int prefix, char *seen)
 {
 	int positive, negative;
-	positive = match_pathspec_depth_1(ps, name, namelen, prefix, seen, 0);
+	positive = do_match_pathspec(ps, name, namelen, prefix, seen, 0);
 	if (!(ps->magic & PATHSPEC_EXCLUDE) || !positive)
 		return positive;
-	negative = match_pathspec_depth_1(ps, name, namelen, prefix, seen, 1);
+	negative = do_match_pathspec(ps, name, namelen, prefix, seen, 1);
 	return negative ? 0 : positive;
 }
 
diff --git a/dir.h b/dir.h
index 65f54b6..c31ed9a 100644
--- a/dir.h
+++ b/dir.h
@@ -132,9 +132,9 @@ struct dir_struct {
 extern int simple_length(const char *match);
 extern int no_wildcard(const char *string);
 extern char *common_prefix(const struct pathspec *pathspec);
-extern int match_pathspec_depth(const struct pathspec *pathspec,
-				const char *name, int namelen,
-				int prefix, char *seen);
+extern int match_pathspec(const struct pathspec *pathspec,
+			  const char *name, int namelen,
+			  int prefix, char *seen);
 extern int within_depth(const char *name, int namelen, int depth, int max_depth);
 
 extern int fill_directory(struct dir_struct *dir, const struct pathspec *pathspec);
@@ -209,14 +209,14 @@ static inline int ce_path_match(const struct cache_entry *ce,
 				const struct pathspec *pathspec,
 				char *seen)
 {
-	return match_pathspec_depth(pathspec, ce->name, ce_namelen(ce), 0, seen);
+	return match_pathspec(pathspec, ce->name, ce_namelen(ce), 0, seen);
 }
 
 static inline int dir_path_match(const struct dir_entry *ent,
 				 const struct pathspec *pathspec,
 				 int prefix, char *seen)
 {
-	return match_pathspec_depth(pathspec, ent->name, ent->len, prefix, seen);
+	return match_pathspec(pathspec, ent->name, ent->len, prefix, seen);
 }
 
 #endif
diff --git a/rerere.c b/rerere.c
index 1f2d21a..34a21c4 100644
--- a/rerere.c
+++ b/rerere.c
@@ -672,8 +672,8 @@ int rerere_forget(struct pathspec *pathspec)
 	find_conflict(&conflict);
 	for (i = 0; i < conflict.nr; i++) {
 		struct string_list_item *it = &conflict.items[i];
-		if (!match_pathspec_depth(pathspec, it->string, strlen(it->string),
-					  0, NULL))
+		if (!match_pathspec(pathspec, it->string,
+				    strlen(it->string), 0, NULL))
 			continue;
 		rerere_forget_one_path(it->string, &merge_rr);
 	}
diff --git a/t/t6131-pathspec-icase.sh b/t/t6131-pathspec-icase.sh
index a7c7ff5..39fc3f6 100755
--- a/t/t6131-pathspec-icase.sh
+++ b/t/t6131-pathspec-icase.sh
@@ -69,7 +69,7 @@ test_expect_success 'tree_entry_interesting matches :(icase)bar with empty prefi
 	test_cmp expect actual
 '
 
-test_expect_success 'match_pathspec_depth matches :(icase)bar' '
+test_expect_success 'match_pathspec matches :(icase)bar' '
 	cat <<-EOF >expect &&
 	BAR
 	bAr
@@ -79,7 +79,7 @@ test_expect_success 'match_pathspec_depth matches :(icase)bar' '
 	test_cmp expect actual
 '
 
-test_expect_success 'match_pathspec_depth matches :(icase)bar with prefix' '
+test_expect_success 'match_pathspec matches :(icase)bar with prefix' '
 	cat <<-EOF >expect &&
 	fOo/BAR
 	fOo/bAr
@@ -89,7 +89,7 @@ test_expect_success 'match_pathspec_depth matches :(icase)bar with prefix' '
 	test_cmp expect actual
 '
 
-test_expect_success 'match_pathspec_depth matches :(icase)bar with empty prefix' '
+test_expect_success 'match_pathspec matches :(icase)bar with empty prefix' '
 	cat <<-EOF >expect &&
 	bar
 	fOo/BAR
-- 
1.8.5.2.240.g8478abd

[PATCH v2 4/9] dir.c: prepare match_pathspec_item for taking more flags

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 dir.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/dir.c b/dir.c
index 8960ea1..35e09bd 100644
--- a/dir.c
+++ b/dir.c
@@ -195,6 +195,8 @@ int within_depth(const char *name, int namelen,
 	return 1;
 }
 
+#define DO_MATCH_EXCLUDE   1
+
 /*
  * Does 'match' match the given name?
  * A match is found if
@@ -208,7 +210,7 @@ int within_depth(const char *name, int namelen,
  * It returns 0 when there is no match.
  */
 static int match_pathspec_item(const struct pathspec_item *item, int prefix,
-			       const char *name, int namelen)
+			       const char *name, int namelen, unsigned flags)
 {
 	/* name/namelen has prefix cut off by caller */
 	const char *match = item->match + prefix;
@@ -285,9 +287,9 @@ static int match_pathspec_item(const struct pathspec_item *item, int prefix,
 static int do_match_pathspec(const struct pathspec *ps,
 			     const char *name, int namelen,
 			     int prefix, char *seen,
-			     int exclude)
+			     unsigned flags)
 {
-	int i, retval = 0;
+	int i, retval = 0, exclude = flags & DO_MATCH_EXCLUDE;
 
 	GUARD_PATHSPEC(ps,
 		       PATHSPEC_FROMTOP |
@@ -327,7 +329,8 @@ static int do_match_pathspec(const struct pathspec *ps,
 		 */
 		if (seen && ps->items[i].magic & PATHSPEC_EXCLUDE)
 			seen[i] = MATCHED_FNMATCH;
-		how = match_pathspec_item(ps->items+i, prefix, name, namelen);
+		how = match_pathspec_item(ps->items+i, prefix, name,
+					  namelen, flags);
 		if (ps->recursive &&
 		    (ps->magic & PATHSPEC_MAXDEPTH) &&
 		    ps->max_depth != -1 &&
@@ -355,10 +358,14 @@ int match_pathspec(const struct pathspec *ps,
 		   int prefix, char *seen)
 {
 	int positive, negative;
-	positive = do_match_pathspec(ps, name, namelen, prefix, seen, 0);
+	unsigned flags = 0;
+	positive = do_match_pathspec(ps, name, namelen,
+				     prefix, seen, flags);
 	if (!(ps->magic & PATHSPEC_EXCLUDE) || !positive)
 		return positive;
-	negative = do_match_pathspec(ps, name, namelen, prefix, seen, 1);
+	negative = do_match_pathspec(ps, name, namelen,
+				     prefix, seen,
+				     flags | DO_MATCH_EXCLUDE);
 	return negative ? 0 : positive;
 }
 
-- 
1.8.5.2.240.g8478abd

[PATCH v2 5/9] match_pathspec: match pathspec "foo/" against directory "foo"

[Nguyễn Thái Ngọc Duy]

Currently we do support matching pathspec "foo/" against directory
"foo". That is because match_pathspec() has no way to tell "foo" is a
directory and matching "foo/" against _file_ "foo" is wrong.

The callers can now tell match_pathspec if "foo" is a directory, we
could make an exception for this case. Code is not executed though
because no callers pass the flag yet.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 dir.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/dir.c b/dir.c
index 35e09bd..7b50072 100644
--- a/dir.c
+++ b/dir.c
@@ -196,6 +196,7 @@ int within_depth(const char *name, int namelen,
 }
 
 #define DO_MATCH_EXCLUDE   1
+#define DO_MATCH_DIRECTORY 2
 
 /*
  * Does 'match' match the given name?
@@ -259,7 +260,11 @@ static int match_pathspec_item(const struct pathspec_item *item, int prefix,
 
 		if (match[matchlen-1] == '/' || name[matchlen] == '/')
 			return MATCHED_RECURSIVELY;
-	}
+	} else if ((flags & DO_MATCH_DIRECTORY) &&
+		   match[matchlen - 1] == '/' &&
+		   namelen == matchlen - 1 &&
+		   !ps_strncmp(item, match, name, namelen))
+		return MATCHED_EXACTLY;
 
 	if (item->nowildcard_len < item->len &&
 	    !git_fnmatch(item, match, name,
-- 
1.8.5.2.240.g8478abd

[PATCH v2 6/9] Pass directory indicator to match_pathspec_item()

[Nguyễn Thái Ngọc Duy]

This patch activates the DO_MATCH_DIRECTORY code in m_p_i(), which
makes "git diff HEAD submodule/" and "git diff HEAD submodule" produce
the same output. Previously only the version without trailing slash
returns the difference (if any).

That's the effect of new ce_path_match(). dir_path_match() is not
executed by the new tests. And it should not introduce regressions.

Previously if path "dir/" is passed in with pathspec "dir/", they
obviously match. With new dir_path_match(), the path becomes
_directory_ "dir" vs pathspec "dir/", which is not executed by the old
code path in m_p_i(). The new code path is executed and produces the
same result.

The other case is pathspec "dir" and path "dir/" is now turned to
"dir" (with DO_MATCH_DIRECTORY). Still the same result before or after
the patch.

So why change? Because of the next patch about clean.c.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/clean.c          |  2 +-
 builtin/ls-files.c       |  5 +++--
 dir.c                    |  4 ++--
 dir.h                    | 10 +++++++---
 rerere.c                 |  2 +-
 t/t4010-diff-pathspec.sh | 17 +++++++++++++++++
 6 files changed, 31 insertions(+), 9 deletions(-)

diff --git a/builtin/clean.c b/builtin/clean.c
index f59c753..4c9680a 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -962,7 +962,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 
 		if (pathspec.nr)
 			matches = match_pathspec(&pathspec, ent->name,
-						 len, 0, NULL);
+						 len, 0, NULL, 0);
 
 		if (S_ISDIR(st.st_mode)) {
 			if (remove_directories || (matches == MATCHED_EXACTLY)) {
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 02db0e1..47c3880 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -140,7 +140,8 @@ static void show_ce_entry(const char *tag, const struct cache_entry *ce)
 		die("git ls-files: internal error - cache entry not superset of prefix");
 
 	if (!match_pathspec(&pathspec, ce->name, ce_namelen(ce),
-			    len, ps_matched))
+			    len, ps_matched,
+			    S_ISDIR(ce->ce_mode) || S_ISGITLINK(ce->ce_mode)))
 		return;
 
 	if (tag && *tag && show_valid_bit &&
@@ -197,7 +198,7 @@ static void show_ru_info(void)
 		if (len < max_prefix_len)
 			continue; /* outside of the prefix */
 		if (!match_pathspec(&pathspec, path, len,
-				    max_prefix_len, ps_matched))
+				    max_prefix_len, ps_matched, 0))
 			continue; /* uninterested */
 		for (i = 0; i < 3; i++) {
 			if (!ui->mode[i])
diff --git a/dir.c b/dir.c
index 7b50072..2fd0ebd 100644
--- a/dir.c
+++ b/dir.c
@@ -360,10 +360,10 @@ static int do_match_pathspec(const struct pathspec *ps,
 
 int match_pathspec(const struct pathspec *ps,
 		   const char *name, int namelen,
-		   int prefix, char *seen)
+		   int prefix, char *seen, int is_dir)
 {
 	int positive, negative;
-	unsigned flags = 0;
+	unsigned flags = is_dir ? DO_MATCH_DIRECTORY : 0;
 	positive = do_match_pathspec(ps, name, namelen,
 				     prefix, seen, flags);
 	if (!(ps->magic & PATHSPEC_EXCLUDE) || !positive)
diff --git a/dir.h b/dir.h
index c31ed9a..55e5345 100644
--- a/dir.h
+++ b/dir.h
@@ -134,7 +134,7 @@ extern int no_wildcard(const char *string);
 extern char *common_prefix(const struct pathspec *pathspec);
 extern int match_pathspec(const struct pathspec *pathspec,
 			  const char *name, int namelen,
-			  int prefix, char *seen);
+			  int prefix, char *seen, int is_dir);
 extern int within_depth(const char *name, int namelen, int depth, int max_depth);
 
 extern int fill_directory(struct dir_struct *dir, const struct pathspec *pathspec);
@@ -209,14 +209,18 @@ static inline int ce_path_match(const struct cache_entry *ce,
 				const struct pathspec *pathspec,
 				char *seen)
 {
-	return match_pathspec(pathspec, ce->name, ce_namelen(ce), 0, seen);
+	return match_pathspec(pathspec, ce->name, ce_namelen(ce), 0, seen,
+			      S_ISDIR(ce->ce_mode) || S_ISGITLINK(ce->ce_mode));
 }
 
 static inline int dir_path_match(const struct dir_entry *ent,
 				 const struct pathspec *pathspec,
 				 int prefix, char *seen)
 {
-	return match_pathspec(pathspec, ent->name, ent->len, prefix, seen);
+	int has_trailing_dir = ent->len && ent->name[ent->len - 1] == '/';
+	int len = has_trailing_dir ? ent->len - 1 : ent->len;
+	return match_pathspec(pathspec, ent->name, len, prefix, seen,
+			      has_trailing_dir);
 }
 
 #endif
diff --git a/rerere.c b/rerere.c
index 34a21c4..d55aa8a 100644
--- a/rerere.c
+++ b/rerere.c
@@ -673,7 +673,7 @@ int rerere_forget(struct pathspec *pathspec)
 	for (i = 0; i < conflict.nr; i++) {
 		struct string_list_item *it = &conflict.items[i];
 		if (!match_pathspec(pathspec, it->string,
-				    strlen(it->string), 0, NULL))
+				    strlen(it->string), 0, NULL, 0))
 			continue;
 		rerere_forget_one_path(it->string, &merge_rr);
 	}
diff --git a/t/t4010-diff-pathspec.sh b/t/t4010-diff-pathspec.sh
index af5134b..167af53 100755
--- a/t/t4010-diff-pathspec.sh
+++ b/t/t4010-diff-pathspec.sh
@@ -110,4 +110,21 @@ test_expect_success 'diff-tree -r with wildcard' '
 	test_cmp expected result
 '
 
+test_expect_success 'setup submodules' '
+	test_tick &&
+	git init submod &&
+	( cd submod && test_commit first; ) &&
+	git add submod &&
+	git commit -m first &&
+	( cd submod && test_commit second; ) &&
+	git add submod &&
+	git commit -m second
+'
+
+test_expect_success 'diff-cache ignores trailing slash on submodule path' '
+	git diff --name-only HEAD^ submod >expect &&
+	git diff --name-only HEAD^ submod/ >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
1.8.5.2.240.g8478abd

Re: [PATCH v2 6/9] Pass directory indicator to match_pathspec_item()

[Eric Sunshine]

On Fri, Jan 24, 2014 at 8:40 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> diff --git a/t/t4010-diff-pathspec.sh b/t/t4010-diff-pathspec.sh
> index af5134b..167af53 100755
> --- a/t/t4010-diff-pathspec.sh
> +++ b/t/t4010-diff-pathspec.sh
> @@ -110,4 +110,21 @@ test_expect_success 'diff-tree -r with wildcard' '
>         test_cmp expected result
>  '
>
> +test_expect_success 'setup submodules' '
> +       test_tick &&
> +       git init submod &&
> +       ( cd submod && test_commit first; ) &&

Unnecessary semicolon might confuse the reader into thinking something
unusual is going on here.

> +       git add submod &&
> +       git commit -m first &&
> +       ( cd submod && test_commit second; ) &&

Ditto.

> +       git add submod &&
> +       git commit -m second
> +'
> +
> +test_expect_success 'diff-cache ignores trailing slash on submodule path' '
> +       git diff --name-only HEAD^ submod >expect &&
> +       git diff --name-only HEAD^ submod/ >actual &&
> +       test_cmp expect actual
> +'
> +
>  test_done

Re: [PATCH v2 6/9] Pass directory indicator to match_pathspec_item()

[Duy Nguyen]

On Sat, Jan 25, 2014 at 4:22 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Fri, Jan 24, 2014 at 8:40 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
>> diff --git a/t/t4010-diff-pathspec.sh b/t/t4010-diff-pathspec.sh
>> index af5134b..167af53 100755
>> --- a/t/t4010-diff-pathspec.sh
>> +++ b/t/t4010-diff-pathspec.sh
>> @@ -110,4 +110,21 @@ test_expect_success 'diff-tree -r with wildcard' '
>>         test_cmp expected result
>>  '
>>
>> +test_expect_success 'setup submodules' '
>> +       test_tick &&

Also the test_tick here. I thought I would need to match SHA-1 but I
did not and still forgot to take this line out.

>> +       git init submod &&
>> +       ( cd submod && test_commit first; ) &&
>
> Unnecessary semicolon might confuse the reader into thinking something
> unusual is going on here.
>
>> +       git add submod &&
>> +       git commit -m first &&
>> +       ( cd submod && test_commit second; ) &&
>
> Ditto.
>
>> +       git add submod &&
>> +       git commit -m second
>> +'
>> +
>> +test_expect_success 'diff-cache ignores trailing slash on submodule path' '
>> +       git diff --name-only HEAD^ submod >expect &&
>> +       git diff --name-only HEAD^ submod/ >actual &&
>> +       test_cmp expect actual
>> +'
>> +
>>  test_done



-- 
Duy

[PATCH v2 7/9] clean: replace match_pathspec() with dir_path_match()

[Nguyễn Thái Ngọc Duy]

This instance was left out when many match_pathspec() call sites that
take input from dir_entry were converted to dir_path_match() because
it passed a path with the trailing slash stripped out to match_pathspec()
while the others did not. Stripping for all call sites back then would
be a regression because match_pathspec() did not know how to match
pathspec foo/ against _directory_ foo (the stripped version of path
"foo/").

match_pathspec() knows how to do it now. And dir_path_match() strips
the trailing slash also. Use the new function, because the stripping
code is removed in the next patch.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/clean.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/builtin/clean.c b/builtin/clean.c
index 4c9680a..5adb52d 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -961,8 +961,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 			die_errno("Cannot lstat '%s'", ent->name);
 
 		if (pathspec.nr)
-			matches = match_pathspec(&pathspec, ent->name,
-						 len, 0, NULL, 0);
+			matches = dir_path_match(ent, &pathspec, 0, NULL);
 
 		if (S_ISDIR(st.st_mode)) {
 			if (remove_directories || (matches == MATCHED_EXACTLY)) {
-- 
1.8.5.2.240.g8478abd

[PATCH v2 8/9] clean: use cache_name_is_other()

[Nguyễn Thái Ngọc Duy]

cmd_clean() has the exact same code of index_name_is_other(). Reduce
code duplication.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/clean.c | 21 ++-------------------
 1 file changed, 2 insertions(+), 19 deletions(-)

diff --git a/builtin/clean.c b/builtin/clean.c
index 5adb52d..cb02a53 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -933,29 +933,12 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 
 	for (i = 0; i < dir.nr; i++) {
 		struct dir_entry *ent = dir.entries[i];
-		int len, pos;
 		int matches = 0;
-		const struct cache_entry *ce;
 		struct stat st;
 		const char *rel;
 
-		/*
-		 * Remove the '/' at the end that directory
-		 * walking adds for directory entries.
-		 */
-		len = ent->len;
-		if (len && ent->name[len-1] == '/')
-			len--;
-		pos = cache_name_pos(ent->name, len);
-		if (0 <= pos)
-			continue;	/* exact match */
-		pos = -pos - 1;
-		if (pos < active_nr) {
-			ce = active_cache[pos];
-			if (ce_namelen(ce) == len &&
-			    !memcmp(ce->name, ent->name, len))
-				continue; /* Yup, this one exists unmerged */
-		}
+		if (!cache_name_is_other(ent->name, ent->len))
+			continue;
 
 		if (lstat(ent->name, &st))
 			die_errno("Cannot lstat '%s'", ent->name);
-- 
1.8.5.2.240.g8478abd

[PATCH v2 9/9] tree-walk.c: ignore trailing slash on submodule in tree_entry_interesting()

[Nguyễn Thái Ngọc Duy]

We do ignore trailing slash on a directory, so pathspec "abc/" matches
directory "abc". A submodule is also a directory. Apply the same logic
to it. This makes "git log submodule-path" and "git log submodule-path/"
produce the same output.

Reported-by: Paweł Sikora <pawel.sikora@agmk.net>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 t/t4010-diff-pathspec.sh | 6 ++++++
 tree-walk.c              | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/t/t4010-diff-pathspec.sh b/t/t4010-diff-pathspec.sh
index 167af53..2424af0 100755
--- a/t/t4010-diff-pathspec.sh
+++ b/t/t4010-diff-pathspec.sh
@@ -127,4 +127,10 @@ test_expect_success 'diff-cache ignores trailing slash on submodule path' '
 	test_cmp expect actual
 '
 
+test_expect_success 'diff-tree ignores trailing slash on submodule path' '
+	git diff --name-only HEAD^ HEAD submod >expect &&
+	git diff --name-only HEAD^ HEAD submod/ >actual &&
+	test_cmp expect actual
+'
+
 test_done
diff --git a/tree-walk.c b/tree-walk.c
index 680afda..c29b6a3 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -543,7 +543,7 @@ static int match_entry(const struct pathspec_item *item,
 	if (matchlen > pathlen) {
 		if (match[pathlen] != '/')
 			return 0;
-		if (!S_ISDIR(entry->mode))
+		if (!S_ISDIR(entry->mode) && !S_ISGITLINK(entry->mode))
 			return 0;
 	}
 
-- 
1.8.5.2.240.g8478abd

Re: [PATCH v2 0/9] About the trailing slashes

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> While looking at this, I found a funny behavior of fill_directory.
>
>   $ git init
>   $ mkdir b
>   $ >b/c
>   $ >b/d
>   $ git status b
>   Untracked files:
>           b/
>   $ git status b/
>   Untracked files:
>           b/c b/d
>
> Notice how the trailing slash produces different untracked listing.
> This is because of common_prefix_len(). In the "b" case,
> common_prefix_len() returns empty prefix, so read_directory reads top
> directory, traverses through, reaches "b" and eventually calls
> treat_directory() on it, which results in "b/" in the output.
>
> In the "b/" case, common_prefix_len() found the prefix "b", so
> read_directory() starts at "b" instead of b's parent.
> treat_directory() is never called on "b" itself, which results in
> "b/c" and "b/d".

Hmm, this feels like a borderline case.

If the user asked "git status ?", or even "git status '?'", it seems
to me that the user wanted to get "git status" output but with a
scope limited to top-level entries with a single letter.  And the
unlimited output asks to consolidate output for directories that
have nothing interesting.  "git status b" and "git status '[b]'" are
asking similar thing, but not just limiting the scope to any one
letter but to 'b'.  So the output you showed above looks correct to
me.  On the other hand, the other request "git status b/" seems to
me that the user is very aware that 'b' is a directory and wants to
look inside, so again the output you showed looks correct to me.

How does "git status '[b]/'" behave?

Re: [PATCH v2 0/9] About the trailing slashes

[Duy Nguyen]

On Sat, Jan 25, 2014 at 2:22 AM, Junio C Hamano <gitster@pobox.com> wrote:
> How does "git status '[b]/'" behave?

It outputs "b/" (like "git status b"). I think that's because
common_prefix_len() stops looking at the first wildcard char, '['. So
common prefix is empty, just like "b".
-- 
Duy

Re: [PATCH v2 0/9] About the trailing slashes

[Junio C Hamano]

Thanks; will try to rebase on top of more recent codebase and then
review.