[PATCH/WIP 00/11] read_directory() rewrite to support struct pathspec

[PATCH/WIP 00/11] read_directory() rewrite to support struct pathspec

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

This is the first time "make test" fully passes (*) for me, so it's
probably good enough for human eyes. Just heads up where this might
go.

A few points:

 - "git add --ignore-missing" is killed because I could not find an
   easy way to incorporate it to the new read_directory(). It looks
   like a hack to me, to expose .gitignore matching. Luckily no one
   except submodule seems to use it.

 - I chose to use tree_entry_interesting() instead of
   match_pathspec(). The former has more optimizations but requires a
   tree-based structure. So I have to read the whole directory in,
   re-construct a temporary tree object to make t_e_i() happy. I
   _think_ it does not impact performance with reasonable dir size.

 - there'll be more work to get rid of match_pathspec() calls after
   read_directory()/fill_directory(). I haven't got finished this part
   yet.

 - I really like to kill match_pathspec() so we only have one pathspec
   implementation instead of two now, but that may be real hard
   because of staged entries in index.

(*) t7012.7 fails but I think that's the test's fault.

Nguyễn Thái Ngọc Duy (11):
  Introduce "check-attr --excluded" as a replacement for "add --ignore-missing"
  notes-merge: use opendir/readdir instead of using read_directory()
  t5403: avoid doing "git add foo/bar" where foo/.git exists
  tree-walk.c: do not leak internal structure in tree_entry_len()
  symbolize return values of tree_entry_interesting()
  read_directory_recursive: reduce one indentation level
  tree_entry_interesting: make use of local pointer "item"
  tree-walk: mark useful pathspecs
  tree_entry_interesting: differentiate partial vs full match
  read-dir: stop using path_simplify code in favor of tree_entry_interesting()
  dir.c: remove dead code after read_directory() rewrite

 Documentation/git-check-attr.txt |    4 +
 builtin/add.c                    |   36 ++--
 builtin/check-attr.c             |   26 +++
 builtin/grep.c                   |   11 +-
 builtin/pack-objects.c           |    2 +-
 cache.h                          |    1 +
 dir.c                            |  428 +++++++++++++++++++-------------------
 dir.h                            |    8 +-
 git-submodule.sh                 |    2 +-
 list-objects.c                   |    9 +-
 notes-merge.c                    |   45 +++--
 t/t3700-add.sh                   |   19 --
 t/t5403-post-checkout-hook.sh    |   17 +-
 tree-diff.c                      |   19 +-
 tree-walk.c                      |   85 ++++----
 tree-walk.h                      |   19 ++-
 tree.c                           |   11 +-
 unpack-trees.c                   |    6 +-
 18 files changed, 394 insertions(+), 354 deletions(-)

-- 
1.7.3.1.256.g2539c.dirty

[PATCH/WIP 01/11] Introduce "check-attr --excluded" as a replacement for "add --ignore-missing"

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

--ignore-missing is used by submodule to check if a path may be
ignored by .gitignore files. It does not really fit in git-add (git
add takes pathspec, but --ignore-missing takes only paths)

Google reckons that --ignore-missing is not used anywhere but
git-submodule.sh. Remove --ignore-missing and introduce "check-attr
--excluded" as a replacement.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/git-check-attr.txt |    4 ++++
 builtin/add.c                    |   14 +++-----------
 builtin/check-attr.c             |   26 ++++++++++++++++++++++++++
 git-submodule.sh                 |    2 +-
 t/t3700-add.sh                   |   19 -------------------
 5 files changed, 34 insertions(+), 31 deletions(-)

diff --git a/Documentation/git-check-attr.txt b/Documentation/git-check-attr.txt
index 5abdbaa..94d2068 100644
--- a/Documentation/git-check-attr.txt
+++ b/Documentation/git-check-attr.txt
@@ -11,6 +11,7 @@ SYNOPSIS
 [verse]
 'git check-attr' [-a | --all | attr...] [--] pathname...
 'git check-attr' --stdin [-z] [-a | --all | attr...] < <list-of-paths>
+'git check-attr' --excluded pathname...
 
 DESCRIPTION
 -----------
@@ -34,6 +35,9 @@ OPTIONS
 	Only meaningful with `--stdin`; paths are separated with a
 	NUL character instead of a linefeed character.
 
+--excluded::
+	Check if given paths are excluded by standard .gitignore rules.
+
 \--::
 	Interpret all preceding arguments as attributes and all following
 	arguments as path names.
diff --git a/builtin/add.c b/builtin/add.c
index c59b0c9..23ad4b8 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -310,7 +310,7 @@ static const char ignore_error[] =
 N_("The following paths are ignored by one of your .gitignore files:\n");
 
 static int verbose = 0, show_only = 0, ignored_too = 0, refresh_only = 0;
-static int ignore_add_errors, addremove, intent_to_add, ignore_missing = 0;
+static int ignore_add_errors, addremove, intent_to_add;
 
 static struct option builtin_add_options[] = {
 	OPT__DRY_RUN(&show_only, "dry run"),
@@ -325,7 +325,6 @@ static struct option builtin_add_options[] = {
 	OPT_BOOLEAN('A', "all", &addremove, "add changes from all tracked and untracked files"),
 	OPT_BOOLEAN( 0 , "refresh", &refresh_only, "don't add, only refresh the index"),
 	OPT_BOOLEAN( 0 , "ignore-errors", &ignore_add_errors, "just skip files which cannot be added because of errors"),
-	OPT_BOOLEAN( 0 , "ignore-missing", &ignore_missing, "check if - even missing - files are ignored in dry run"),
 	OPT_END(),
 };
 
@@ -387,8 +386,6 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 
 	if (addremove && take_worktree_changes)
 		die(_("-A and -u are mutually incompatible"));
-	if (!show_only && ignore_missing)
-		die(_("Option --ignore-missing can only be used together with --dry-run"));
 	if ((addremove || take_worktree_changes) && !argc) {
 		static const char *here[2] = { ".", NULL };
 		argc = 1;
@@ -446,13 +443,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 		for (i = 0; pathspec[i]; i++) {
 			if (!seen[i] && pathspec[i][0]
 			    && !file_exists(pathspec[i])) {
-				if (ignore_missing) {
-					int dtype = DT_UNKNOWN;
-					if (excluded(&dir, pathspec[i], &dtype))
-						dir_add_ignored(&dir, pathspec[i], strlen(pathspec[i]));
-				} else
-					die(_("pathspec '%s' did not match any files"),
-					    pathspec[i]);
+				die(_("pathspec '%s' did not match any files"),
+				    pathspec[i]);
 			}
 		}
 		free(seen);
diff --git a/builtin/check-attr.c b/builtin/check-attr.c
index 44c421e..4c17ccc 100644
--- a/builtin/check-attr.c
+++ b/builtin/check-attr.c
@@ -2,11 +2,13 @@
 #include "cache.h"
 #include "attr.h"
 #include "quote.h"
+#include "dir.h"
 #include "parse-options.h"
 
 static int all_attrs;
 static int cached_attrs;
 static int stdin_paths;
+static int exclude;
 static const char * const check_attr_usage[] = {
 "git check-attr [-a | --all | attr...] [--] pathname...",
 "git check-attr --stdin [-a | --all | attr...] < <list-of-paths>",
@@ -21,6 +23,7 @@ static const struct option check_attr_options[] = {
 	OPT_BOOLEAN(0 , "stdin", &stdin_paths, "read file names from stdin"),
 	OPT_BOOLEAN('z', NULL, &null_term_line,
 		"input paths are terminated by a null character"),
+	OPT_BOOLEAN(0,  "excluded", &exclude, "check exclude patterns"),
 	OPT_END()
 };
 
@@ -43,6 +46,16 @@ static void output_attr(int cnt, struct git_attr_check *check,
 	}
 }
 
+static void check_exclude(struct dir_struct *dir, const char *prefix, const char *file)
+{
+	char *full_path =
+		prefix_path(prefix, prefix ? strlen(prefix) : 0, file);
+	int dtype = DT_UNKNOWN;
+	if (excluded(dir, full_path, &dtype))
+		die("%s is ignored by one of your .gitignore files", full_path);
+	free(full_path);
+}
+
 static void check_attr(const char *prefix, int cnt,
 	struct git_attr_check *check, const char *file)
 {
@@ -103,6 +116,19 @@ int cmd_check_attr(int argc, const char **argv, const char *prefix)
 		die("invalid cache");
 	}
 
+	if (exclude) {
+		struct dir_struct dir;
+
+		if (stdin_paths)
+			die("--excluded cannot be used with --stdin (yet)");
+
+		memset(&dir, 0, sizeof(dir));
+		setup_standard_excludes(&dir);
+		for (i = 0; i < argc; i++)
+			check_exclude(&dir, prefix, argv[i]);
+		return 0;
+	}
+
 	if (cached_attrs)
 		git_attr_set_direction(GIT_ATTR_INDEX, NULL);
 
diff --git a/git-submodule.sh b/git-submodule.sh
index 928a62f..0bc3762 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -262,7 +262,7 @@ cmd_add()
 	git ls-files --error-unmatch "$path" > /dev/null 2>&1 &&
 	die "$(eval_gettext "'\$path' already exists in the index")"
 
-	if test -z "$force" && ! git add --dry-run --ignore-missing "$path" > /dev/null 2>&1
+	if test -z "$force" && ! git check-attr --excluded "$path" > /dev/null 2>&1
 	then
 		eval_gettextln "The following path is ignored by one of your .gitignore files:
 \$path
diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index 575d950..23ff998 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -276,23 +276,4 @@ test_expect_success 'git add --dry-run of an existing file output' "
 	test_i18ncmp expect actual
 "
 
-cat >expect.err <<\EOF
-The following paths are ignored by one of your .gitignore files:
-ignored-file
-Use -f if you really want to add them.
-fatal: no files added
-EOF
-cat >expect.out <<\EOF
-add 'track-this'
-EOF
-
-test_expect_success 'git add --dry-run --ignore-missing of non-existing file' '
-	test_must_fail git add --dry-run --ignore-missing track-this ignored-file >actual.out 2>actual.err
-'
-
-test_expect_success 'git add --dry-run --ignore-missing of non-existing file output' '
-	test_i18ncmp expect.out actual.out &&
-	test_i18ncmp expect.err actual.err
-'
-
 test_done
-- 
1.7.3.1.256.g2539c.dirty

[PATCH/WIP 02/11] notes-merge: use opendir/readdir instead of using read_directory()

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

notes_merge_commit() only needs to list all entries (non-recursively)
under a directory, which can be easily accomplished with
opendir/readdir and would be more lightweight than read_directory().

read_directory() is designed to list paths inside a working
directory. Using it outside of its scope may lead to undesired effects.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 notes-merge.c |   45 +++++++++++++++++++++++++++------------------
 1 files changed, 27 insertions(+), 18 deletions(-)

diff --git a/notes-merge.c b/notes-merge.c
index e9e4199..80d64a2 100644
--- a/notes-merge.c
+++ b/notes-merge.c
@@ -680,48 +680,57 @@ int notes_merge_commit(struct notes_merge_options *o,
 	 * commit message and parents from 'partial_commit'.
 	 * Finally store the new commit object SHA1 into 'result_sha1'.
 	 */
-	struct dir_struct dir;
-	char *path = xstrdup(git_path(NOTES_MERGE_WORKTREE "/"));
-	int path_len = strlen(path), i;
+	DIR *dir;
+	struct dirent *e;
+	struct strbuf path = STRBUF_INIT;
 	const char *msg = strstr(partial_commit->buffer, "\n\n");
+	int baselen;
 
-	OUTPUT(o, 3, "Committing notes in notes merge worktree at %.*s",
-	       path_len - 1, path);
+	strbuf_addstr(&path, git_path(NOTES_MERGE_WORKTREE));
+	OUTPUT(o, 3, "Committing notes in notes merge worktree at %s", path.buf);
 
 	if (!msg || msg[2] == '\0')
 		die("partial notes commit has empty message");
 	msg += 2;
 
-	memset(&dir, 0, sizeof(dir));
-	read_directory(&dir, path, path_len, NULL);
-	for (i = 0; i < dir.nr; i++) {
-		struct dir_entry *ent = dir.entries[i];
+	dir = opendir(path.buf);
+	if (!dir)
+		die_errno("could not open %s", path.buf);
+
+	strbuf_addch(&path, '/');
+	baselen = path.len;
+	while ((e = readdir(dir)) != NULL) {
 		struct stat st;
-		const char *relpath = ent->name + path_len;
 		unsigned char obj_sha1[20], blob_sha1[20];
 
-		if (ent->len - path_len != 40 || get_sha1_hex(relpath, obj_sha1)) {
-			OUTPUT(o, 3, "Skipping non-SHA1 entry '%s'", ent->name);
+		if (is_dot_or_dotdot(e->d_name))
+			continue;
+
+		if (strlen(e->d_name) != 40 || get_sha1_hex(e->d_name, obj_sha1)) {
+			OUTPUT(o, 3, "Skipping non-SHA1 entry '%s%s'", path.buf, e->d_name);
 			continue;
 		}
 
+		strbuf_addstr(&path, e->d_name);
 		/* write file as blob, and add to partial_tree */
-		if (stat(ent->name, &st))
-			die_errno("Failed to stat '%s'", ent->name);
-		if (index_path(blob_sha1, ent->name, &st, HASH_WRITE_OBJECT))
-			die("Failed to write blob object from '%s'", ent->name);
+		if (stat(path.buf, &st))
+			die_errno("Failed to stat '%s'", path.buf);
+		if (index_path(blob_sha1, path.buf, &st, HASH_WRITE_OBJECT))
+			die("Failed to write blob object from '%s'", path.buf);
 		if (add_note(partial_tree, obj_sha1, blob_sha1, NULL))
 			die("Failed to add resolved note '%s' to notes tree",
-			    ent->name);
+			    path.buf);
 		OUTPUT(o, 4, "Added resolved note for object %s: %s",
 		       sha1_to_hex(obj_sha1), sha1_to_hex(blob_sha1));
+		strbuf_setlen(&path, baselen);
 	}
 
 	create_notes_commit(partial_tree, partial_commit->parents, msg,
 			    result_sha1);
 	OUTPUT(o, 4, "Finalized notes merge commit: %s",
 	       sha1_to_hex(result_sha1));
-	free(path);
+	strbuf_release(&path);
+	closedir(dir);
 	return 0;
 }
 
-- 
1.7.3.1.256.g2539c.dirty

[PATCH/WIP 03/11] t5403: avoid doing "git add foo/bar" where foo/.git exists

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

In this case, "foo" is considered a submodule and bar, if added,
belongs to foo/.git. "git add" should only allow "git add foo" in this
case, but it passes somehow.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 t/t5403-post-checkout-hook.sh |   17 ++++++++++-------
 1 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/t/t5403-post-checkout-hook.sh b/t/t5403-post-checkout-hook.sh
index 1753ef2..3b3e2c1 100755
--- a/t/t5403-post-checkout-hook.sh
+++ b/t/t5403-post-checkout-hook.sh
@@ -16,10 +16,13 @@ test_expect_success setup '
 	git update-ref refs/heads/master $commit0 &&
 	git clone ./. clone1 &&
 	git clone ./. clone2 &&
-	GIT_DIR=clone2/.git git branch new2 &&
-	echo Data for commit1. >clone2/b &&
-	GIT_DIR=clone2/.git git add clone2/b &&
-	GIT_DIR=clone2/.git git commit -m new2
+	(
+		cd clone2 &&
+		git branch new2 &&
+		echo Data for commit1. >b &&
+		git add b &&
+		git commit -m new2
+	)
 '
 
 for clone in 1 2; do
@@ -48,7 +51,7 @@ test_expect_success 'post-checkout runs as expected ' '
 '
 
 test_expect_success 'post-checkout args are correct with git checkout -b ' '
-	GIT_DIR=clone1/.git git checkout -b new1 &&
+	( cd clone1 && git checkout -b new1 ) &&
 	old=$(awk "{print \$1}" clone1/.git/post-checkout.args) &&
 	new=$(awk "{print \$2}" clone1/.git/post-checkout.args) &&
 	flag=$(awk "{print \$3}" clone1/.git/post-checkout.args) &&
@@ -56,7 +59,7 @@ test_expect_success 'post-checkout args are correct with git checkout -b ' '
 '
 
 test_expect_success 'post-checkout receives the right args with HEAD changed ' '
-	GIT_DIR=clone2/.git git checkout new2 &&
+	( cd clone2 && git checkout new2 ) &&
 	old=$(awk "{print \$1}" clone2/.git/post-checkout.args) &&
 	new=$(awk "{print \$2}" clone2/.git/post-checkout.args) &&
 	flag=$(awk "{print \$3}" clone2/.git/post-checkout.args) &&
@@ -64,7 +67,7 @@ test_expect_success 'post-checkout receives the right args with HEAD changed ' '
 '
 
 test_expect_success 'post-checkout receives the right args when not switching branches ' '
-	GIT_DIR=clone2/.git git checkout master b &&
+	( cd clone2 && git checkout master b ) &&
 	old=$(awk "{print \$1}" clone2/.git/post-checkout.args) &&
 	new=$(awk "{print \$2}" clone2/.git/post-checkout.args) &&
 	flag=$(awk "{print \$3}" clone2/.git/post-checkout.args) &&
-- 
1.7.3.1.256.g2539c.dirty

[PATCH/WIP 04/11] tree-walk.c: do not leak internal structure in tree_entry_len()

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

tree_entry_len() does not simply take two random arguments and return
a tree length. The two pointers must point to a tree item structure,
or struct name_entry. Passing random pointers will return incorrect
value.

Force callers to pass struct name_entry instead of two pointers (with
hope that they don't manually construct struct name_entry themselves)

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/grep.c         |    2 +-
 builtin/pack-objects.c |    2 +-
 tree-diff.c            |    6 +++---
 tree-walk.c            |   16 ++++++++--------
 tree-walk.h            |    6 +++---
 tree.c                 |    2 +-
 unpack-trees.c         |    6 +++---
 7 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 7d0779f..2cd0612 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -547,7 +547,7 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,
 	int old_baselen = base->len;
 
 	while (tree_entry(tree, &entry)) {
-		int te_len = tree_entry_len(entry.path, entry.sha1);
+		int te_len = tree_entry_len(&entry);
 
 		if (match != 2) {
 			match = tree_entry_interesting(&entry, base, tn_len, pathspec);
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 2b18de5..864154b 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -975,7 +975,7 @@ static void add_pbase_object(struct tree_desc *tree,
 	while (tree_entry(tree,&entry)) {
 		if (S_ISGITLINK(entry.mode))
 			continue;
-		cmp = tree_entry_len(entry.path, entry.sha1) != cmplen ? 1 :
+		cmp = tree_entry_len(&entry) != cmplen ? 1 :
 		      memcmp(name, entry.path, cmplen);
 		if (cmp > 0)
 			continue;
diff --git a/tree-diff.c b/tree-diff.c
index b3cc2e4..6782484 100644
--- a/tree-diff.c
+++ b/tree-diff.c
@@ -21,8 +21,8 @@ static int compare_tree_entry(struct tree_desc *t1, struct tree_desc *t2,
 	sha1 = tree_entry_extract(t1, &path1, &mode1);
 	sha2 = tree_entry_extract(t2, &path2, &mode2);
 
-	pathlen1 = tree_entry_len(path1, sha1);
-	pathlen2 = tree_entry_len(path2, sha2);
+	pathlen1 = tree_entry_len(&t1->entry);
+	pathlen2 = tree_entry_len(&t2->entry);
 	cmp = base_name_compare(path1, pathlen1, mode1, path2, pathlen2, mode2);
 	if (cmp < 0) {
 		show_entry(opt, "-", t1, base);
@@ -85,7 +85,7 @@ static void show_entry(struct diff_options *opt, const char *prefix,
 	unsigned mode;
 	const char *path;
 	const unsigned char *sha1 = tree_entry_extract(desc, &path, &mode);
-	int pathlen = tree_entry_len(path, sha1);
+	int pathlen = tree_entry_len(&desc->entry);
 	int old_baselen = base->len;
 
 	strbuf_add(base, path, pathlen);
diff --git a/tree-walk.c b/tree-walk.c
index 418107e..f5d19f9 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -116,7 +116,7 @@ void setup_traverse_info(struct traverse_info *info, const char *base)
 
 char *make_traverse_path(char *path, const struct traverse_info *info, const struct name_entry *n)
 {
-	int len = tree_entry_len(n->path, n->sha1);
+	int len = tree_entry_len(n);
 	int pathlen = info->pathlen;
 
 	path[pathlen + len] = 0;
@@ -126,7 +126,7 @@ char *make_traverse_path(char *path, const struct traverse_info *info, const str
 			break;
 		path[--pathlen] = '/';
 		n = &info->name;
-		len = tree_entry_len(n->path, n->sha1);
+		len = tree_entry_len(n);
 		info = info->prev;
 		pathlen -= len;
 	}
@@ -253,7 +253,7 @@ static void extended_entry_extract(struct tree_desc_x *t,
 	 * The caller wants "first" from this tree, or nothing.
 	 */
 	path = a->path;
-	len = tree_entry_len(a->path, a->sha1);
+	len = tree_entry_len(a);
 	switch (check_entry_match(first, first_len, path, len)) {
 	case -1:
 		entry_clear(a);
@@ -271,7 +271,7 @@ static void extended_entry_extract(struct tree_desc_x *t,
 	while (probe.size) {
 		entry_extract(&probe, a);
 		path = a->path;
-		len = tree_entry_len(a->path, a->sha1);
+		len = tree_entry_len(a);
 		switch (check_entry_match(first, first_len, path, len)) {
 		case -1:
 			entry_clear(a);
@@ -362,7 +362,7 @@ int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)
 			e = entry + i;
 			if (!e->path)
 				continue;
-			len = tree_entry_len(e->path, e->sha1);
+			len = tree_entry_len(e);
 			if (!first) {
 				first = e->path;
 				first_len = len;
@@ -381,7 +381,7 @@ int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)
 				/* Cull the ones that are not the earliest */
 				if (!e->path)
 					continue;
-				len = tree_entry_len(e->path, e->sha1);
+				len = tree_entry_len(e);
 				if (name_compare(e->path, len, first, first_len))
 					entry_clear(e);
 			}
@@ -434,8 +434,8 @@ static int find_tree_entry(struct tree_desc *t, const char *name, unsigned char
 		int entrylen, cmp;
 
 		sha1 = tree_entry_extract(t, &entry, mode);
+		entrylen = tree_entry_len(&t->entry);
 		update_tree_entry(t);
-		entrylen = tree_entry_len(entry, sha1);
 		if (entrylen > namelen)
 			continue;
 		cmp = memcmp(name, entry, entrylen);
@@ -596,7 +596,7 @@ int tree_entry_interesting(const struct name_entry *entry,
 				      ps->max_depth);
 	}
 
-	pathlen = tree_entry_len(entry->path, entry->sha1);
+	pathlen = tree_entry_len(entry);
 
 	for (i = ps->nr - 1; i >= 0; i--) {
 		const struct pathspec_item *item = ps->items+i;
diff --git a/tree-walk.h b/tree-walk.h
index 0089581..884d01a 100644
--- a/tree-walk.h
+++ b/tree-walk.h
@@ -20,9 +20,9 @@ static inline const unsigned char *tree_entry_extract(struct tree_desc *desc, co
 	return desc->entry.sha1;
 }
 
-static inline int tree_entry_len(const char *name, const unsigned char *sha1)
+static inline int tree_entry_len(const struct name_entry *ne)
 {
-	return (const char *)sha1 - name - 1;
+	return (const char *)ne->sha1 - ne->path - 1;
 }
 
 void update_tree_entry(struct tree_desc *);
@@ -58,7 +58,7 @@ extern void setup_traverse_info(struct traverse_info *info, const char *base);
 
 static inline int traverse_path_len(const struct traverse_info *info, const struct name_entry *n)
 {
-	return info->pathlen + tree_entry_len(n->path, n->sha1);
+	return info->pathlen + tree_entry_len(n);
 }
 
 extern int tree_entry_interesting(const struct name_entry *, struct strbuf *, int, const struct pathspec *ps);
diff --git a/tree.c b/tree.c
index 698ecf7..e622198 100644
--- a/tree.c
+++ b/tree.c
@@ -99,7 +99,7 @@ static int read_tree_1(struct tree *tree, struct strbuf *base,
 		else
 			continue;
 
-		len = tree_entry_len(entry.path, entry.sha1);
+		len = tree_entry_len(&entry);
 		strbuf_add(base, entry.path, len);
 		strbuf_addch(base, '/');
 		retval = read_tree_1(lookup_tree(sha1),
diff --git a/unpack-trees.c b/unpack-trees.c
index 8282f5e..7c9ecf6 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -446,7 +446,7 @@ static int traverse_trees_recursive(int n, unsigned long dirmask,
 	newinfo.prev = info;
 	newinfo.pathspec = info->pathspec;
 	newinfo.name = *p;
-	newinfo.pathlen += tree_entry_len(p->path, p->sha1) + 1;
+	newinfo.pathlen += tree_entry_len(p) + 1;
 	newinfo.conflicts |= df_conflicts;
 
 	for (i = 0; i < n; i++, dirmask >>= 1) {
@@ -495,7 +495,7 @@ static int do_compare_entry(const struct cache_entry *ce, const struct traverse_
 	ce_len -= pathlen;
 	ce_name = ce->name + pathlen;
 
-	len = tree_entry_len(n->path, n->sha1);
+	len = tree_entry_len(n);
 	return df_name_compare(ce_name, ce_len, S_IFREG, n->path, len, n->mode);
 }
 
@@ -626,7 +626,7 @@ static int find_cache_pos(struct traverse_info *info,
 	struct unpack_trees_options *o = info->data;
 	struct index_state *index = o->src_index;
 	int pfxlen = info->pathlen;
-	int p_len = tree_entry_len(p->path, p->sha1);
+	int p_len = tree_entry_len(p);
 
 	for (pos = o->cache_bottom; pos < index->cache_nr; pos++) {
 		struct cache_entry *ce = index->cache[pos];
-- 
1.7.3.1.256.g2539c.dirty

[PATCH/WIP 05/11] symbolize return values of tree_entry_interesting()

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

This helps extending the value later on for "interesting, but cannot
decide if the entry truely matches yet" (ie. prefix matches)

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/grep.c |    9 +++++----
 list-objects.c |    9 +++++----
 tree-diff.c    |   13 +++++++------
 tree-walk.c    |   45 +++++++++++++++++++++------------------------
 tree-walk.h    |   12 +++++++++++-
 tree.c         |    9 +++++----
 6 files changed, 54 insertions(+), 43 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 2cd0612..2fc51fa 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -542,18 +542,19 @@ static int grep_cache(struct grep_opt *opt, const struct pathspec *pathspec, int
 static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,
 		     struct tree_desc *tree, struct strbuf *base, int tn_len)
 {
-	int hit = 0, match = 0;
+	int hit = 0;
+	enum interesting match = entry_not_interesting;
 	struct name_entry entry;
 	int old_baselen = base->len;
 
 	while (tree_entry(tree, &entry)) {
 		int te_len = tree_entry_len(&entry);
 
-		if (match != 2) {
+		if (match != all_entries_interesting) {
 			match = tree_entry_interesting(&entry, base, tn_len, pathspec);
-			if (match < 0)
+			if (match == all_entries_not_interesting)
 				break;
-			if (match == 0)
+			if (match == entry_not_interesting)
 				continue;
 		}
 
diff --git a/list-objects.c b/list-objects.c
index 39d80c0..3dd4a96 100644
--- a/list-objects.c
+++ b/list-objects.c
@@ -71,7 +71,8 @@ static void process_tree(struct rev_info *revs,
 	struct tree_desc desc;
 	struct name_entry entry;
 	struct name_path me;
-	int match = revs->diffopt.pathspec.nr == 0 ? 2 : 0;
+	enum interesting match = revs->diffopt.pathspec.nr == 0 ?
+		all_entries_interesting: entry_not_interesting;
 	int baselen = base->len;
 
 	if (!revs->tree_objects)
@@ -97,12 +98,12 @@ static void process_tree(struct rev_info *revs,
 	init_tree_desc(&desc, tree->buffer, tree->size);
 
 	while (tree_entry(&desc, &entry)) {
-		if (match != 2) {
+		if (match != all_entries_interesting) {
 			match = tree_entry_interesting(&entry, base, 0,
 						       &revs->diffopt.pathspec);
-			if (match < 0)
+			if (match == all_entries_not_interesting)
 				break;
-			if (match == 0)
+			if (match == entry_not_interesting)
 				continue;
 		}
 
diff --git a/tree-diff.c b/tree-diff.c
index 6782484..25cc981 100644
--- a/tree-diff.c
+++ b/tree-diff.c
@@ -64,14 +64,14 @@ static int compare_tree_entry(struct tree_desc *t1, struct tree_desc *t2,
 static void show_tree(struct diff_options *opt, const char *prefix,
 		      struct tree_desc *desc, struct strbuf *base)
 {
-	int match = 0;
+	enum interesting match = entry_not_interesting;
 	for (; desc->size; update_tree_entry(desc)) {
-		if (match != 2) {
+		if (match != all_entries_interesting) {
 			match = tree_entry_interesting(&desc->entry, base, 0,
 						       &opt->pathspec);
-			if (match < 0)
+			if (match == all_entries_not_interesting)
 				break;
-			if (match == 0)
+			if (match == entry_not_interesting)
 				continue;
 		}
 		show_entry(opt, prefix, desc, base);
@@ -114,12 +114,13 @@ static void show_entry(struct diff_options *opt, const char *prefix,
 }
 
 static void skip_uninteresting(struct tree_desc *t, struct strbuf *base,
-			       struct diff_options *opt, int *match)
+			       struct diff_options *opt,
+			       enum interesting *match)
 {
 	while (t->size) {
 		*match = tree_entry_interesting(&t->entry, base, 0, &opt->pathspec);
 		if (*match) {
-			if (*match < 0)
+			if (*match == all_entries_not_interesting)
 				t->size = 0;
 			break;
 		}
diff --git a/tree-walk.c b/tree-walk.c
index f5d19f9..fc03262 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -573,27 +573,23 @@ static int match_dir_prefix(const char *base,
  *
  * Pre-condition: either baselen == base_offset (i.e. empty path)
  * or base[baselen-1] == '/' (i.e. with trailing slash).
- *
- * Return:
- *  - 2 for "yes, and all subsequent entries will be"
- *  - 1 for yes
- *  - zero for no
- *  - negative for "no, and no subsequent entries will be either"
  */
-int tree_entry_interesting(const struct name_entry *entry,
-			   struct strbuf *base, int base_offset,
-			   const struct pathspec *ps)
+enum interesting tree_entry_interesting(const struct name_entry *entry,
+					struct strbuf *base, int base_offset,
+					const struct pathspec *ps)
 {
 	int i;
 	int pathlen, baselen = base->len - base_offset;
-	int never_interesting = ps->has_wildcard ? 0 : -1;
+	int never_interesting = ps->has_wildcard ?
+		entry_not_interesting : all_entries_not_interesting;
 
 	if (!ps->nr) {
 		if (!ps->recursive || ps->max_depth == -1)
-			return 2;
-		return !!within_depth(base->buf + base_offset, baselen,
-				      !!S_ISDIR(entry->mode),
-				      ps->max_depth);
+			return all_entries_interesting;
+		return within_depth(base->buf + base_offset, baselen,
+				    !!S_ISDIR(entry->mode),
+				    ps->max_depth) ?
+			entry_interesting : entry_not_interesting;
 	}
 
 	pathlen = tree_entry_len(entry);
@@ -610,12 +606,13 @@ int tree_entry_interesting(const struct name_entry *entry,
 				goto match_wildcards;
 
 			if (!ps->recursive || ps->max_depth == -1)
-				return 2;
+				return all_entries_interesting;
 
-			return !!within_depth(base_str + matchlen + 1,
-					      baselen - matchlen - 1,
-					      !!S_ISDIR(entry->mode),
-					      ps->max_depth);
+			return within_depth(base_str + matchlen + 1,
+					    baselen - matchlen - 1,
+					    !!S_ISDIR(entry->mode),
+					    ps->max_depth) ?
+				entry_interesting : entry_not_interesting;
 		}
 
 		/* Either there must be no base, or the base must match. */
@@ -623,18 +620,18 @@ int tree_entry_interesting(const struct name_entry *entry,
 			if (match_entry(entry, pathlen,
 					match + baselen, matchlen - baselen,
 					&never_interesting))
-				return 1;
+				return entry_interesting;
 
 			if (ps->items[i].use_wildcard) {
 				if (!fnmatch(match + baselen, entry->path, 0))
-					return 1;
+					return entry_interesting;
 
 				/*
 				 * Match all directories. We'll try to
 				 * match files later on.
 				 */
 				if (ps->recursive && S_ISDIR(entry->mode))
-					return 1;
+					return entry_interesting;
 			}
 
 			continue;
@@ -653,7 +650,7 @@ match_wildcards:
 
 		if (!fnmatch(match, base->buf + base_offset, 0)) {
 			strbuf_setlen(base, base_offset + baselen);
-			return 1;
+			return entry_interesting;
 		}
 		strbuf_setlen(base, base_offset + baselen);
 
@@ -662,7 +659,7 @@ match_wildcards:
 		 * later on.
 		 */
 		if (ps->recursive && S_ISDIR(entry->mode))
-			return 1;
+			return entry_interesting;
 	}
 	return never_interesting; /* No matches */
 }
diff --git a/tree-walk.h b/tree-walk.h
index 884d01a..2bf0db9 100644
--- a/tree-walk.h
+++ b/tree-walk.h
@@ -61,6 +61,16 @@ static inline int traverse_path_len(const struct traverse_info *info, const stru
 	return info->pathlen + tree_entry_len(n);
 }
 
-extern int tree_entry_interesting(const struct name_entry *, struct strbuf *, int, const struct pathspec *ps);
+/* in general, positive means "kind of interesting" */
+enum interesting {
+	all_entries_not_interesting = -1, /* no, and no subsequent entries will be either */
+	entry_not_interesting = 0,
+	entry_interesting = 1,
+	all_entries_interesting = 2 /* yes, and all subsequent entries will be */
+};
+
+extern enum interesting tree_entry_interesting(const struct name_entry *,
+					       struct strbuf *, int,
+					       const struct pathspec *ps);
 
 #endif
diff --git a/tree.c b/tree.c
index e622198..676e9f7 100644
--- a/tree.c
+++ b/tree.c
@@ -52,7 +52,8 @@ static int read_tree_1(struct tree *tree, struct strbuf *base,
 	struct tree_desc desc;
 	struct name_entry entry;
 	unsigned char sha1[20];
-	int len, retval = 0, oldlen = base->len;
+	int len, oldlen = base->len;
+	enum interesting retval = entry_not_interesting;
 
 	if (parse_tree(tree))
 		return -1;
@@ -60,11 +61,11 @@ static int read_tree_1(struct tree *tree, struct strbuf *base,
 	init_tree_desc(&desc, tree->buffer, tree->size);
 
 	while (tree_entry(&desc, &entry)) {
-		if (retval != 2) {
+		if (retval != all_entries_interesting) {
 			retval = tree_entry_interesting(&entry, base, 0, pathspec);
-			if (retval < 0)
+			if (retval == all_entries_not_interesting)
 				break;
-			if (retval == 0)
+			if (retval == entry_not_interesting)
 				continue;
 		}
 
-- 
1.7.3.1.256.g2539c.dirty

[PATCH/WIP 06/11] read_directory_recursive: reduce one indentation level

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

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

diff --git a/dir.c b/dir.c
index 6c0d782..0a78d00 100644
--- a/dir.c
+++ b/dir.c
@@ -968,34 +968,34 @@ static int read_directory_recursive(struct dir_struct *dir,
 {
 	DIR *fdir = opendir(*base ? base : ".");
 	int contents = 0;
+	struct dirent *de;
+	char path[PATH_MAX + 1];
 
-	if (fdir) {
-		struct dirent *de;
-		char path[PATH_MAX + 1];
-		memcpy(path, base, baselen);
-
-		while ((de = readdir(fdir)) != NULL) {
-			int len;
-			switch (treat_path(dir, de, path, sizeof(path),
-					   baselen, simplify, &len)) {
-			case path_recurse:
-				contents += read_directory_recursive
-					(dir, path, len, 0, simplify);
-				continue;
-			case path_ignored:
-				continue;
-			case path_handled:
-				break;
-			}
-			contents++;
-			if (check_only)
-				goto exit_early;
-			else
-				dir_add_name(dir, path, len);
+	if (!fdir)
+		return 0;
+
+	memcpy(path, base, baselen);
+
+	while ((de = readdir(fdir)) != NULL) {
+		int len;
+		switch (treat_path(dir, de, path, sizeof(path),
+				   baselen, simplify, &len)) {
+		case path_recurse:
+			contents += read_directory_recursive(dir, path, len, 0, simplify);
+			continue;
+		case path_ignored:
+			continue;
+		case path_handled:
+			break;
 		}
-exit_early:
-		closedir(fdir);
+		contents++;
+		if (check_only)
+			goto exit_early;
+		else
+			dir_add_name(dir, path, len);
 	}
+exit_early:
+	closedir(fdir);
 
 	return contents;
 }
-- 
1.7.3.1.256.g2539c.dirty

[PATCH/WIP 07/11] tree_entry_interesting: make use of local pointer "item"

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

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 tree-walk.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tree-walk.c b/tree-walk.c
index fc03262..2d9d17a 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -622,7 +622,7 @@ enum interesting tree_entry_interesting(const struct name_entry *entry,
 					&never_interesting))
 				return entry_interesting;
 
-			if (ps->items[i].use_wildcard) {
+			if (item->use_wildcard) {
 				if (!fnmatch(match + baselen, entry->path, 0))
 					return entry_interesting;
 
@@ -638,7 +638,7 @@ enum interesting tree_entry_interesting(const struct name_entry *entry,
 		}
 
 match_wildcards:
-		if (!ps->items[i].use_wildcard)
+		if (!item->use_wildcard)
 			continue;
 
 		/*
-- 
1.7.3.1.256.g2539c.dirty

[PATCH/WIP 08/11] tree-walk: mark useful pathspecs

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

Useful pathspecs are those that help decide whether an item is in or
out, as opposed to useless ones whose existence does not change the
results.

Callers are responsible for cleaning before use, or doing anything
after.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 cache.h     |    1 +
 tree-walk.c |   13 ++++++++++---
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/cache.h b/cache.h
index be07ec7..946d910 100644
--- a/cache.h
+++ b/cache.h
@@ -532,6 +532,7 @@ struct pathspec {
 		const char *match;
 		int len;
 		unsigned int use_wildcard:1;
+		unsigned int useful:1;
 	} *items;
 };
 
diff --git a/tree-walk.c b/tree-walk.c
index 2d9d17a..5e9c522 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -595,11 +595,15 @@ enum interesting tree_entry_interesting(const struct name_entry *entry,
 	pathlen = tree_entry_len(entry);
 
 	for (i = ps->nr - 1; i >= 0; i--) {
-		const struct pathspec_item *item = ps->items+i;
+		struct pathspec_item *item = ps->items+i;
 		const char *match = item->match;
 		const char *base_str = base->buf + base_offset;
 		int matchlen = item->len;
 
+		/* assume it will be used (which usually means break
+		   the loop and return), reset it otherwise */
+		item->useful = 1;
+
 		if (baselen >= matchlen) {
 			/* If it doesn't match, move along... */
 			if (!match_dir_prefix(base_str, match, matchlen))
@@ -634,12 +638,12 @@ enum interesting tree_entry_interesting(const struct name_entry *entry,
 					return entry_interesting;
 			}
 
-			continue;
+			goto nouse;
 		}
 
 match_wildcards:
 		if (!item->use_wildcard)
-			continue;
+			goto nouse;
 
 		/*
 		 * Concatenate base and entry->path into one and do
@@ -660,6 +664,9 @@ match_wildcards:
 		 */
 		if (ps->recursive && S_ISDIR(entry->mode))
 			return entry_interesting;
+
+nouse:
+		item->useful = 0;
 	}
 	return never_interesting; /* No matches */
 }
-- 
1.7.3.1.256.g2539c.dirty

[PATCH/WIP 09/11] tree_entry_interesting: differentiate partial vs full match

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

Up until now, for a/b pathspec, both paths a and a/b would return
entry_interesting. Make it return entry_matched for the latter.

This way if the caller follows up to "a", but decide to stop for some
reason, then it knows that "a" has not really matched the given
pathspec yet.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 tree-walk.c |   13 ++++++++-----
 tree-walk.h |    5 +++--
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/tree-walk.c b/tree-walk.c
index 5e9c522..6e12f0f 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -616,19 +616,22 @@ enum interesting tree_entry_interesting(const struct name_entry *entry,
 					    baselen - matchlen - 1,
 					    !!S_ISDIR(entry->mode),
 					    ps->max_depth) ?
-				entry_interesting : entry_not_interesting;
+				entry_matched : entry_not_interesting;
 		}
 
 		/* Either there must be no base, or the base must match. */
 		if (baselen == 0 || !strncmp(base_str, match, baselen)) {
 			if (match_entry(entry, pathlen,
 					match + baselen, matchlen - baselen,
-					&never_interesting))
-				return entry_interesting;
+					&never_interesting)) {
+				if (match[baselen + pathlen] == '/')
+					return entry_interesting;
+				return entry_matched;
+			}
 
 			if (item->use_wildcard) {
 				if (!fnmatch(match + baselen, entry->path, 0))
-					return entry_interesting;
+					return entry_matched;
 
 				/*
 				 * Match all directories. We'll try to
@@ -654,7 +657,7 @@ match_wildcards:
 
 		if (!fnmatch(match, base->buf + base_offset, 0)) {
 			strbuf_setlen(base, base_offset + baselen);
-			return entry_interesting;
+			return entry_matched;
 		}
 		strbuf_setlen(base, base_offset + baselen);
 
diff --git a/tree-walk.h b/tree-walk.h
index 2bf0db9..a5f92fa 100644
--- a/tree-walk.h
+++ b/tree-walk.h
@@ -65,8 +65,9 @@ static inline int traverse_path_len(const struct traverse_info *info, const stru
 enum interesting {
 	all_entries_not_interesting = -1, /* no, and no subsequent entries will be either */
 	entry_not_interesting = 0,
-	entry_interesting = 1,
-	all_entries_interesting = 2 /* yes, and all subsequent entries will be */
+	entry_interesting = 1, /* a potential match, not not there yet  */
+	entry_matched = 2,
+	all_entries_interesting = 3 /* yes, and all subsequent entries will be */
 };
 
 extern enum interesting tree_entry_interesting(const struct name_entry *,
-- 
1.7.3.1.256.g2539c.dirty

[PATCH/WIP 10/11] read-dir: stop using path_simplify code in favor of tree_entry_interesting()

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

Current code tries to find a prefix set of given pathspecs and filter
on the set. Call sites are supposed to do exact pathspec matching
again to remove unmatched entries (but matches the prefix set).

This patch makes read_directory() use tree_entry_interesting()
directly, thus remove the need to filter again by call sites (although
call sites are untouched in this patch).

A less intrusive way would be to use match_pathspec_depth(), but I'd
rather reduce the use of that function and eventually remove it, so we
only have to maintain pathspec matching at one place:
tree_entry_interesting().

In order to make use of tree_entry_interesting(), directory content
from readdir() must be converted to tree object format, which means we
have to read all items of a directory at once and sort it. If the
directory is large, it may become expensive operation. But again,
current code does nothing to stop reading directory early, so nothing
is lost.

ignored_nr and ignored[] are not longer filled. read_directory() users
are supposed to use useful[] instead.

Many functions are left unused in this patch to avoid clutter up the
patch. They will be removed later.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/add.c |   22 +++--
 dir.c         |  317 ++++++++++++++++++++++++++++++++++++++-------------------
 dir.h         |    5 +
 tree-walk.c   |    2 +
 4 files changed, 236 insertions(+), 110 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index 23ad4b8..92ba3d4 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -307,7 +307,7 @@ static int edit_patch(int argc, const char **argv, const char *prefix)
 static struct lock_file lock_file;
 
 static const char ignore_error[] =
-N_("The following paths are ignored by one of your .gitignore files:\n");
+N_("The following pathspecs are ignored by one of your .gitignore files:\n");
 
 static int verbose = 0, show_only = 0, ignored_too = 0, refresh_only = 0;
 static int ignore_add_errors, addremove, intent_to_add;
@@ -342,12 +342,20 @@ static int add_files(struct dir_struct *dir, int flags)
 {
 	int i, exit_status = 0;
 
-	if (dir->ignored_nr) {
-		fprintf(stderr, _(ignore_error));
-		for (i = 0; i < dir->ignored_nr; i++)
-			fprintf(stderr, "%s\n", dir->ignored[i]->name);
-		fprintf(stderr, _("Use -f if you really want to add them.\n"));
-		die(_("no files added"));
+	if (dir->useful) {
+		int show_header = 0;
+		for (i = 0; i < dir->ps2.nr; i++)
+			if (!dir->useful[i]) {
+				if (!show_header) {
+					fprintf(stderr, _(ignore_error));
+					show_header = 1;
+				}
+				fprintf(stderr, "%s\n", dir->ps2.items[i].match);
+			}
+		if (show_header) {
+			fprintf(stderr, _("Use -f if you really want to add them.\n"));
+			die(_("no files added"));
+		}
 	}
 
 	for (i = 0; i < dir->nr; i++)
diff --git a/dir.c b/dir.c
index 0a78d00..2946b2d 100644
--- a/dir.c
+++ b/dir.c
@@ -8,14 +8,18 @@
 #include "cache.h"
 #include "dir.h"
 #include "refs.h"
+#include "tree-walk.h"
+#include "string-list.h"
 
 struct path_simplify {
 	int len;
 	const char *path;
 };
 
-static int read_directory_recursive(struct dir_struct *dir, const char *path, int len,
-	int check_only, const struct path_simplify *simplify);
+static int read_directory_recursive(struct dir_struct *dir,
+				    struct strbuf *base,
+				    int check_only,
+				    enum interesting match);
 static int get_dtype(struct dirent *de, const char *path, int len);
 
 /* helper string functions with support for the ignore_case flag */
@@ -609,6 +613,93 @@ struct dir_entry *dir_add_ignored(struct dir_struct *dir, const char *pathname,
 	return dir->ignored[dir->ignored_nr++] = dir_entry_new(pathname, len);
 }
 
+/* Read and convert directory to tree object (with invalid SHA-1) */
+static void* dir_to_tree(struct strbuf *path, unsigned long *size)
+{
+	int pathlen = path->len;
+	DIR *fdir = opendir(pathlen ? path->buf : ".");
+	struct string_list paths = STRING_LIST_INIT_DUP;
+	struct dirent *de;
+	char *tree, *p;
+	int i,dtype;
+
+	if (!fdir)
+		return NULL;
+
+	*size = 0;
+	while ((de = readdir(fdir)) != NULL) {
+		int namelen = strlen(de->d_name);
+		struct string_list_item *item;
+		const char *mode = NULL;
+
+		if (is_dot_or_dotdot(de->d_name) ||
+		    !strcmp(de->d_name, ".git") ||
+		    /* Ignore overly long pathnames! */
+		    namelen + pathlen + 8 > PATH_MAX)
+			continue;
+
+		strbuf_add(path, de->d_name, namelen);
+		dtype = get_dtype(de, path->buf, path->len);
+		strbuf_setlen(path, pathlen);
+
+		switch (dtype) {
+		case DT_DIR: mode = "040000 "; break;
+		case DT_REG: mode = "100644 "; break;
+		case DT_LNK: mode = "120000 "; break;
+		default: continue;
+		}
+		item = string_list_insert(&paths, de->d_name);
+		item->util = (void*)mode;
+		/* 100644 SPC path NUL SHA-1 */
+		*size += 6 + 1 + namelen + 1 + 20;
+	}
+	closedir(fdir);
+
+	tree = xmalloc(*size);
+	for (i = 0, p = tree;i < paths.nr; i++) {
+		int len = strlen(paths.items[i].string) + 1;
+		if (!paths.items[i].util ||
+		    strlen(paths.items[i].util) != 7)
+			die("BUG: util should contain a mode");
+		memcpy(p, paths.items[i].util, 7);
+		p += 7;
+		memcpy(p, paths.items[i].string, len);
+		p += len;
+		/* we don't need valid SHA-1 for tree_entry_interesting() */
+		memcpy(p, "\xbb\xaa\xdd\xbb\xaa\xdd\xbb\xaa\xdd", 9);
+		p += 20;
+	}
+	string_list_clear(&paths, 0);
+	return tree;
+}
+
+static enum interesting match_both_pathspecs(struct dir_struct *dir,
+					     struct strbuf *base,
+					     const struct name_entry *ne)
+{
+	int i;
+	enum interesting ret1, ret2;
+
+	/* ps1 contains the base path, no need to care about it */
+	for (i = 0; i < dir->ps2.nr; i++)
+		dir->ps2.items[i].useful = 0;
+
+	ret1 = tree_entry_interesting(ne, base, 0, &dir->ps1);
+	if (ret1 <= 0)
+		return ret1;
+	ret2 = tree_entry_interesting(ne, base, 0, &dir->ps2);
+	if (ret2 <= 0)
+		return ret2;
+
+	if (ret1 == all_entries_interesting && ret2 == all_entries_interesting)
+		return all_entries_interesting;
+	else if ((ret1 == entry_matched || ret1 == all_entries_interesting) &&
+		 (ret2 == entry_matched || ret2 == all_entries_interesting))
+		return entry_matched;
+	else
+		return entry_interesting;
+}
+
 enum exist_status {
 	index_nonexistent = 0,
 	index_directory,
@@ -722,11 +813,10 @@ enum directory_treatment {
 };
 
 static enum directory_treatment treat_directory(struct dir_struct *dir,
-	const char *dirname, int len,
-	const struct path_simplify *simplify)
+						struct strbuf *dirname)
 {
 	/* The "len-1" is to strip the final '/' */
-	switch (directory_exists_in_index(dirname, len-1)) {
+	switch (directory_exists_in_index(dirname->buf, dirname->len-1)) {
 	case index_directory:
 		return recurse_into_directory;
 
@@ -740,7 +830,7 @@ static enum directory_treatment treat_directory(struct dir_struct *dir,
 			break;
 		if (!(dir->flags & DIR_NO_GITLINKS)) {
 			unsigned char sha1[20];
-			if (resolve_gitlink_ref(dirname, "HEAD", sha1) == 0)
+			if (resolve_gitlink_ref(dirname->buf, "HEAD", sha1) == 0)
 				return show_directory;
 		}
 		return recurse_into_directory;
@@ -749,7 +839,7 @@ static enum directory_treatment treat_directory(struct dir_struct *dir,
 	/* This is the "show_other_directories" case */
 	if (!(dir->flags & DIR_HIDE_EMPTY_DIRECTORIES))
 		return show_directory;
-	if (!read_directory_recursive(dir, dirname, len, 1, simplify))
+	if (!read_directory_recursive(dir, dirname, 1, entry_not_interesting))
 		return ignore_directory;
 	return show_directory;
 }
@@ -780,31 +870,35 @@ static int simplify_away(const char *path, int pathlen, const struct path_simpli
 }
 
 /*
- * This function tells us whether an excluded path matches a
- * list of "interesting" pathspecs. That is, whether a path matched
- * by any of the pathspecs could possibly be ignored by excluding
- * the specified path. This can happen if:
+ * This function flags pathspecs that are completely excluded, which
+ * usually means an input mistake. In other words, if all matched
+ * _files_ of a pathspec are excluded, flag the pathspec.
  *
- *   1. the path is mentioned explicitly in the pathspec
+ * The negated version would be: if any of matched files (by pathspec
+ * X) are not excluded, pathspec X is clear, which is exactly what
+ * this function does.
  *
- *   2. the path is a directory prefix of some element in the
- *      pathspec
+ * This function ignores dir->ps1 because that contains exactly one
+ * pathspec item: the path base. No need to worry about that.
  */
-static int exclude_matches_pathspec(const char *path, int len,
-		const struct path_simplify *simplify)
+static void mark_useful(struct dir_struct *dir,
+			const char *path, int len,
+			int dtype,
+			struct pathspec *ps, int exclude,
+			enum interesting match)
 {
-	if (simplify) {
-		for (; simplify->path; simplify++) {
-			if (len == simplify->len
-			    && !memcmp(path, simplify->path, len))
-				return 1;
-			if (len < simplify->len
-			    && simplify->path[len] == '/'
-			    && !memcmp(path, simplify->path, len))
-				return 1;
-		}
-	}
-	return 0;
+	int i;
+	if (!(dir->flags & DIR_COLLECT_IGNORED))
+		return;
+	/* half-matches (eg. prefix matches) do not count as useful */
+	if (match != all_entries_interesting && match != entry_matched)
+		return;
+	if (exclude && cache_name_is_other(path, len))
+		return;
+
+	for (i = 0; i < ps->nr; i++)
+		if (ps->items[i].useful)
+			dir->useful[i] = 1;
 }
 
 static int get_index_dtype(const char *path, int len)
@@ -872,15 +966,24 @@ enum path_treatment {
 	path_recurse
 };
 
-static enum path_treatment treat_one_path(struct dir_struct *dir,
-					  char *path, int *len,
-					  const struct path_simplify *simplify,
-					  int dtype, struct dirent *de)
+/* base is modified to contain ne */
+static int treat_path(struct dir_struct *dir,
+		      struct strbuf *base, const struct name_entry *ne,
+		      enum interesting match)
 {
-	int exclude = excluded(dir, path, &dtype);
-	if (exclude && (dir->flags & DIR_COLLECT_IGNORED)
-	    && exclude_matches_pathspec(path, *len, simplify))
-		dir_add_ignored(dir, path, *len);
+	int exclude, dtype;
+
+	strbuf_add(base, ne->path, tree_entry_len(ne));
+
+	/* It does not matter DT_REG or something else, excluded()
+	 * only cares if it's DT_DIR or not */
+	dtype = S_ISDIR(ne->mode) ? DT_DIR : DT_REG;
+	exclude = excluded(dir, base->buf, &dtype);
+
+	/* intermediate directory match does not count */
+	if (dtype == DT_REG)
+		mark_useful(dir, base->buf, base->len, dtype,
+				       &dir->ps2, exclude, match);
 
 	/*
 	 * Excluded? If we don't explicitly want to show
@@ -889,9 +992,6 @@ static enum path_treatment treat_one_path(struct dir_struct *dir,
 	if (exclude && !(dir->flags & DIR_SHOW_IGNORED))
 		return path_ignored;
 
-	if (dtype == DT_UNKNOWN)
-		dtype = get_dtype(de, path, *len);
-
 	/*
 	 * Do we want to see just the ignored files?
 	 * We still need to recurse into directories,
@@ -899,17 +999,13 @@ static enum path_treatment treat_one_path(struct dir_struct *dir,
 	 * directory may contain files that we do..
 	 */
 	if (!exclude && (dir->flags & DIR_SHOW_IGNORED)) {
-		if (dtype != DT_DIR)
+		if (!S_ISDIR(ne->mode))
 			return path_ignored;
 	}
 
-	switch (dtype) {
-	default:
-		return path_ignored;
-	case DT_DIR:
-		memcpy(path + *len, "/", 2);
-		(*len)++;
-		switch (treat_directory(dir, path, *len, simplify)) {
+	if (S_ISDIR(ne->mode)) {
+		strbuf_addch(base, '/');
+		switch (treat_directory(dir, base)) {
 		case show_directory:
 			if (exclude != !!(dir->flags
 					  & DIR_SHOW_IGNORED))
@@ -920,38 +1016,14 @@ static enum path_treatment treat_one_path(struct dir_struct *dir,
 		case ignore_directory:
 			return path_ignored;
 		}
-		break;
-	case DT_REG:
-	case DT_LNK:
-		break;
+
+		/* path_handled for dirs, must be gitlinks */
+		mark_useful(dir, base->buf, base->len, dtype,
+				       &dir->ps2, exclude, match);
 	}
 	return path_handled;
 }
 
-static enum path_treatment treat_path(struct dir_struct *dir,
-				      struct dirent *de,
-				      char *path, int path_max,
-				      int baselen,
-				      const struct path_simplify *simplify,
-				      int *len)
-{
-	int dtype;
-
-	if (is_dot_or_dotdot(de->d_name) || !strcmp(de->d_name, ".git"))
-		return path_ignored;
-	*len = strlen(de->d_name);
-	/* Ignore overly long pathnames! */
-	if (*len + baselen + 8 > path_max)
-		return path_ignored;
-	memcpy(path + baselen, de->d_name, *len + 1);
-	*len += baselen;
-	if (simplify_away(path, *len, simplify))
-		return path_ignored;
-
-	dtype = DTYPE(de);
-	return treat_one_path(dir, path, len, simplify, dtype, de);
-}
-
 /*
  * Read a directory tree. We currently ignore anything but
  * directories, regular files and symlinks. That's because git
@@ -962,40 +1034,46 @@ static enum path_treatment treat_path(struct dir_struct *dir,
  * That likely will not change.
  */
 static int read_directory_recursive(struct dir_struct *dir,
-				    const char *base, int baselen,
+				    struct strbuf *base,
 				    int check_only,
-				    const struct path_simplify *simplify)
+				    enum interesting match)
 {
-	DIR *fdir = opendir(*base ? base : ".");
-	int contents = 0;
-	struct dirent *de;
-	char path[PATH_MAX + 1];
+	unsigned long size;
+	void *tree_buf = dir_to_tree(base, &size);
+	int contents = 0, baselen = base->len;
+	struct tree_desc desc;
+	struct name_entry ne;
 
-	if (!fdir)
+	if (!tree_buf)
 		return 0;
 
-	memcpy(path, base, baselen);
+	init_tree_desc(&desc, tree_buf, size);
 
-	while ((de = readdir(fdir)) != NULL) {
-		int len;
-		switch (treat_path(dir, de, path, sizeof(path),
-				   baselen, simplify, &len)) {
+	while (tree_entry(&desc, &ne)) {
+		strbuf_setlen(base, baselen);
+		if (match != all_entries_interesting) {
+			match = match_both_pathspecs(dir, base, &ne);
+			if (match == all_entries_not_interesting)
+				break;
+			if (match == entry_not_interesting)
+				continue;
+		}
+		switch (treat_path(dir, base, &ne, match)) {
 		case path_recurse:
-			contents += read_directory_recursive(dir, path, len, 0, simplify);
-			continue;
-		case path_ignored:
+			contents += read_directory_recursive(dir, base, 0, match);
 			continue;
 		case path_handled:
+			contents++;
+			if (check_only)
+				goto exit_early;
+
+			dir_add_name(dir, base->buf, base->len);
 			break;
 		}
-		contents++;
-		if (check_only)
-			goto exit_early;
-		else
-			dir_add_name(dir, path, len);
 	}
 exit_early:
-	closedir(fdir);
+	free(tree_buf);
+	strbuf_setlen(base, baselen);
 
 	return contents;
 }
@@ -1054,6 +1132,7 @@ static void free_simplify(struct path_simplify *simplify)
 	free(simplify);
 }
 
+#if 0
 static int treat_leading_path(struct dir_struct *dir,
 			      const char *path, int len,
 			      const struct path_simplify *simplify)
@@ -1088,20 +1167,52 @@ static int treat_leading_path(struct dir_struct *dir,
 			return 1; /* finished checking */
 	}
 }
+#endif
 
-int read_directory(struct dir_struct *dir, const char *path, int len, const char **pathspec)
+int read_directory(struct dir_struct *dir, const char *path, int len,
+		   const char **pathspec)
 {
-	struct path_simplify *simplify;
+	char *newpath = NULL;
+	struct strbuf base = STRBUF_INIT;
 
 	if (has_symlink_leading_path(path, len))
 		return dir->nr;
 
-	simplify = create_simplify(pathspec);
-	if (!len || treat_leading_path(dir, path, len, simplify))
-		read_directory_recursive(dir, path, len, 0, simplify);
-	free_simplify(simplify);
+	/*
+	 * tree_entry_interesting() does not implement AND operator on
+	 * pathspecs so we call tree_entry_interesting() twice and
+	 * join the results ourselves in match_both_pathspecs()
+	 */
+	if (path && *path) {
+		const char *pathspec1[2];
+		newpath = xmalloc(len + 1);
+		memcpy(newpath, path, len);
+		newpath[len] = 0;
+		pathspec1[0] = newpath;
+		pathspec1[1] = NULL;
+		init_pathspec(&dir->ps1, pathspec1);
+	}
+	else
+		init_pathspec(&dir->ps1, NULL);
+	init_pathspec(&dir->ps2, pathspec);
+
+	if (dir->flags & DIR_COLLECT_IGNORED) {
+		int size = sizeof(*dir->useful) * dir->ps2.nr;
+		dir->useful = xmalloc(size);
+		/* guilty until proven useful */
+		memset(dir->useful, 0, size);
+	}
+
+	read_directory_recursive(dir, &base, 0, entry_not_interesting);
+
+	strbuf_release(&base);
+	free_pathspec(&dir->ps1);
+	if (!(dir->flags & DIR_COLLECT_IGNORED))
+		free_pathspec(&dir->ps2);
+	free(newpath);
+
 	qsort(dir->entries, dir->nr, sizeof(struct dir_entry *), cmp_name);
-	qsort(dir->ignored, dir->ignored_nr, sizeof(struct dir_entry *), cmp_name);
+
 	return dir->nr;
 }
 
diff --git a/dir.h b/dir.h
index dd6947e..362d7b1 100644
--- a/dir.h
+++ b/dir.h
@@ -43,6 +43,11 @@ struct dir_struct {
 	} flags;
 	struct dir_entry **entries;
 	struct dir_entry **ignored;
+	int *useful;
+
+	/* Include info (a joint of ps1 and ps2) */
+	struct pathspec ps1;
+	struct pathspec ps2;
 
 	/* Exclude info */
 	const char *exclude_per_dir;
diff --git a/tree-walk.c b/tree-walk.c
index 6e12f0f..b56fec1 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -600,6 +600,8 @@ enum interesting tree_entry_interesting(const struct name_entry *entry,
 		const char *base_str = base->buf + base_offset;
 		int matchlen = item->len;
 
+		/* TODO: 07ccbff (runstatus: do not recurse into subdirectories if not needed - 2006-09-28) */
+
 		/* assume it will be used (which usually means break
 		   the loop and return), reset it otherwise */
 		item->useful = 1;
-- 
1.7.3.1.256.g2539c.dirty

[PATCH/WIP 11/11] dir.c: remove dead code after read_directory() rewrite

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

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 dir.c |  121 -----------------------------------------------------------------
 dir.h |    3 --
 2 files changed, 0 insertions(+), 124 deletions(-)

diff --git a/dir.c b/dir.c
index 2946b2d..4094962 100644
--- a/dir.c
+++ b/dir.c
@@ -11,11 +11,6 @@
 #include "tree-walk.h"
 #include "string-list.h"
 
-struct path_simplify {
-	int len;
-	const char *path;
-};
-
 static int read_directory_recursive(struct dir_struct *dir,
 				    struct strbuf *base,
 				    int check_only,
@@ -604,15 +599,6 @@ static struct dir_entry *dir_add_name(struct dir_struct *dir, const char *pathna
 	return dir->entries[dir->nr++] = dir_entry_new(pathname, len);
 }
 
-struct dir_entry *dir_add_ignored(struct dir_struct *dir, const char *pathname, int len)
-{
-	if (!cache_name_is_other(pathname, len))
-		return NULL;
-
-	ALLOC_GROW(dir->ignored, dir->ignored_nr+1, dir->ignored_alloc);
-	return dir->ignored[dir->ignored_nr++] = dir_entry_new(pathname, len);
-}
-
 /* Read and convert directory to tree object (with invalid SHA-1) */
 static void* dir_to_tree(struct strbuf *path, unsigned long *size)
 {
@@ -845,31 +831,6 @@ static enum directory_treatment treat_directory(struct dir_struct *dir,
 }
 
 /*
- * This is an inexact early pruning of any recursive directory
- * reading - if the path cannot possibly be in the pathspec,
- * return true, and we'll skip it early.
- */
-static int simplify_away(const char *path, int pathlen, const struct path_simplify *simplify)
-{
-	if (simplify) {
-		for (;;) {
-			const char *match = simplify->path;
-			int len = simplify->len;
-
-			if (!match)
-				break;
-			if (len > pathlen)
-				len = pathlen;
-			if (!memcmp(path, match, len))
-				return 0;
-			simplify++;
-		}
-		return 1;
-	}
-	return 0;
-}
-
-/*
  * This function flags pathspecs that are completely excluded, which
  * usually means an input mistake. In other words, if all matched
  * _files_ of a pathspec are excluded, flag the pathspec.
@@ -1087,88 +1048,6 @@ static int cmp_name(const void *p1, const void *p2)
 				  e2->name, e2->len);
 }
 
-/*
- * Return the length of the "simple" part of a path match limiter.
- */
-static int simple_length(const char *match)
-{
-	int len = -1;
-
-	for (;;) {
-		unsigned char c = *match++;
-		len++;
-		if (c == '\0' || is_glob_special(c))
-			return len;
-	}
-}
-
-static struct path_simplify *create_simplify(const char **pathspec)
-{
-	int nr, alloc = 0;
-	struct path_simplify *simplify = NULL;
-
-	if (!pathspec)
-		return NULL;
-
-	for (nr = 0 ; ; nr++) {
-		const char *match;
-		if (nr >= alloc) {
-			alloc = alloc_nr(alloc);
-			simplify = xrealloc(simplify, alloc * sizeof(*simplify));
-		}
-		match = *pathspec++;
-		if (!match)
-			break;
-		simplify[nr].path = match;
-		simplify[nr].len = simple_length(match);
-	}
-	simplify[nr].path = NULL;
-	simplify[nr].len = 0;
-	return simplify;
-}
-
-static void free_simplify(struct path_simplify *simplify)
-{
-	free(simplify);
-}
-
-#if 0
-static int treat_leading_path(struct dir_struct *dir,
-			      const char *path, int len,
-			      const struct path_simplify *simplify)
-{
-	char pathbuf[PATH_MAX];
-	int baselen, blen;
-	const char *cp;
-
-	while (len && path[len - 1] == '/')
-		len--;
-	if (!len)
-		return 1;
-	baselen = 0;
-	while (1) {
-		cp = path + baselen + !!baselen;
-		cp = memchr(cp, '/', path + len - cp);
-		if (!cp)
-			baselen = len;
-		else
-			baselen = cp - path;
-		memcpy(pathbuf, path, baselen);
-		pathbuf[baselen] = '\0';
-		if (!is_directory(pathbuf))
-			return 0;
-		if (simplify_away(pathbuf, baselen, simplify))
-			return 0;
-		blen = baselen;
-		if (treat_one_path(dir, pathbuf, &blen, simplify,
-				   DT_DIR, NULL) == path_ignored)
-			return 0; /* do not recurse into it */
-		if (len <= baselen)
-			return 1; /* finished checking */
-	}
-}
-#endif
-
 int read_directory(struct dir_struct *dir, const char *path, int len,
 		   const char **pathspec)
 {
diff --git a/dir.h b/dir.h
index 362d7b1..7a7d818 100644
--- a/dir.h
+++ b/dir.h
@@ -33,7 +33,6 @@ struct exclude_stack {
 
 struct dir_struct {
 	int nr, alloc;
-	int ignored_nr, ignored_alloc;
 	enum {
 		DIR_SHOW_IGNORED = 1<<0,
 		DIR_SHOW_OTHER_DIRECTORIES = 1<<1,
@@ -42,7 +41,6 @@ struct dir_struct {
 		DIR_COLLECT_IGNORED = 1<<4
 	} flags;
 	struct dir_entry **entries;
-	struct dir_entry **ignored;
 	int *useful;
 
 	/* Include info (a joint of ps1 and ps2) */
@@ -82,7 +80,6 @@ extern int read_directory(struct dir_struct *, const char *path, int len, const
 extern int excluded_from_list(const char *pathname, int pathlen, const char *basename,
 			      int *dtype, struct exclude_list *el);
 extern int excluded(struct dir_struct *, const char *, int *);
-struct dir_entry *dir_add_ignored(struct dir_struct *dir, const char *pathname, int len);
 extern int add_excludes_from_file_to_list(const char *fname, const char *base, int baselen,
 					  char **buf_p, struct exclude_list *which, int check_index);
 extern void add_excludes_from_file(struct dir_struct *, const char *fname);
-- 
1.7.3.1.256.g2539c.dirty

Re: [PATCH/WIP 00/11] read_directory() rewrite to support struct pathspec

[Junio C Hamano]

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

> This is the first time "make test" fully passes (*) for me, so it's
> probably good enough for human eyes.

Nice way to describe the done-ness of the series. Looking forward to read
it through ;-)

Thanks.

Re: [PATCH/WIP 03/11] t5403: avoid doing "git add foo/bar" where foo/.git exists

[Junio C Hamano]

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

> In this case, "foo" is considered a submodule and bar, if added,
> belongs to foo/.git. "git add" should only allow "git add foo" in this
> case, but it passes somehow.

I do not think the above description is correct.

The test:

 - populates the current directory;
 - makes a clone in ./clone2;
 - creates a file clone2/b;
 - runs "git add clone2/b" with GIT_DIR set to clone2/.git, without
   setting GIT_WORK_TREE nor having core.worktree in clone2/.git/config.

The last step should add a path "clone2/b" to $GIT_DIR/index (which is
clone2/.git/index in this case).  The clone2 is not a submodule to the top
level repository in this case, but even if it were, that would not change
the definition of what the command should do when GIT_DIR is set without
GIT_WORK_TREE nor core.worktree in $GIT_DIR/config.

Running (cd clone2 && git add b) is a _more natural_ thing to do, and it
will result in a path "b" added to the clone2 repository, so that the
result is more useful if you are going to chdir to the repository and keep
working on it.  But that does not mean the existing test is incorrect. It
does not just pass somehow but the test passes by design.

I did not check if later tests look at the contents of commit "new2" to
make sure it contains "clone2/b", but if they do this change should break
such tests.

So I am puzzled by this change; what is this trying to achieve?

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  t/t5403-post-checkout-hook.sh |   17 ++++++++++-------
>  1 files changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/t/t5403-post-checkout-hook.sh b/t/t5403-post-checkout-hook.sh
> index 1753ef2..3b3e2c1 100755
> --- a/t/t5403-post-checkout-hook.sh
> +++ b/t/t5403-post-checkout-hook.sh
> @@ -16,10 +16,13 @@ test_expect_success setup '
>  	git update-ref refs/heads/master $commit0 &&
>  	git clone ./. clone1 &&
>  	git clone ./. clone2 &&
> -	GIT_DIR=clone2/.git git branch new2 &&
> -	echo Data for commit1. >clone2/b &&
> -	GIT_DIR=clone2/.git git add clone2/b &&
> -	GIT_DIR=clone2/.git git commit -m new2
> +	(
> +		cd clone2 &&
> +		git branch new2 &&
> +		echo Data for commit1. >b &&
> +		git add b &&
> +		git commit -m new2
> +	)
>  '
>  
>  for clone in 1 2; do
> @@ -48,7 +51,7 @@ test_expect_success 'post-checkout runs as expected ' '
>  '
>  
>  test_expect_success 'post-checkout args are correct with git checkout -b ' '
> -	GIT_DIR=clone1/.git git checkout -b new1 &&
> +	( cd clone1 && git checkout -b new1 ) &&
>  	old=$(awk "{print \$1}" clone1/.git/post-checkout.args) &&
>  	new=$(awk "{print \$2}" clone1/.git/post-checkout.args) &&
>  	flag=$(awk "{print \$3}" clone1/.git/post-checkout.args) &&
> @@ -56,7 +59,7 @@ test_expect_success 'post-checkout args are correct with git checkout -b ' '
>  '
>  
>  test_expect_success 'post-checkout receives the right args with HEAD changed ' '
> -	GIT_DIR=clone2/.git git checkout new2 &&
> +	( cd clone2 && git checkout new2 ) &&
>  	old=$(awk "{print \$1}" clone2/.git/post-checkout.args) &&
>  	new=$(awk "{print \$2}" clone2/.git/post-checkout.args) &&
>  	flag=$(awk "{print \$3}" clone2/.git/post-checkout.args) &&
> @@ -64,7 +67,7 @@ test_expect_success 'post-checkout receives the right args with HEAD changed ' '
>  '
>  
>  test_expect_success 'post-checkout receives the right args when not switching branches ' '
> -	GIT_DIR=clone2/.git git checkout master b &&
> +	( cd clone2 && git checkout master b ) &&
>  	old=$(awk "{print \$1}" clone2/.git/post-checkout.args) &&
>  	new=$(awk "{print \$2}" clone2/.git/post-checkout.args) &&
>  	flag=$(awk "{print \$3}" clone2/.git/post-checkout.args) &&

Re: [PATCH/WIP 04/11] tree-walk.c: do not leak internal structure in tree_entry_len()

[Junio C Hamano]

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

> tree_entry_len() does not simply take two random arguments and return
> a tree length. The two pointers must point to a tree item structure,
> or struct name_entry. Passing random pointers will return incorrect
> value.
>
> Force callers to pass struct name_entry instead of two pointers (with
> hope that they don't manually construct struct name_entry themselves)

Makes quite a lot of sense.

Re: [PATCH/WIP 05/11] symbolize return values of tree_entry_interesting()

[Junio C Hamano]

Makes it a lot easier to read for first-time readers. Nice.

Just one minor formatting nit of lacking SP near ":", though.

Re: [PATCH/WIP 02/11] notes-merge: use opendir/readdir instead of using read_directory()

[Junio C Hamano]

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

> notes_merge_commit() only needs to list all entries (non-recursively)
> under a directory, which can be easily accomplished with
> opendir/readdir and would be more lightweight than read_directory().
>
> read_directory() is designed to list paths inside a working
> directory. Using it outside of its scope may lead to undesired effects.

Technically isn't the directory structure this codepath looks at a working
tree that has extract of a notes tree commit?

Looking at the result of the patch I do not have strong opinions either
way, though. It isn't like we care about gitignore or attributes rules in
the notes tree, so using read_directory() does feel like an overkill.

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  notes-merge.c |   45 +++++++++++++++++++++++++++------------------
>  1 files changed, 27 insertions(+), 18 deletions(-)
>
> diff --git a/notes-merge.c b/notes-merge.c
> index e9e4199..80d64a2 100644
> --- a/notes-merge.c
> +++ b/notes-merge.c
> @@ -680,48 +680,57 @@ int notes_merge_commit(struct notes_merge_options *o,
>  	 * commit message and parents from 'partial_commit'.
>  	 * Finally store the new commit object SHA1 into 'result_sha1'.
>  	 */
> -	struct dir_struct dir;
> -	char *path = xstrdup(git_path(NOTES_MERGE_WORKTREE "/"));
> -	int path_len = strlen(path), i;
> +	DIR *dir;
> +	struct dirent *e;
> +	struct strbuf path = STRBUF_INIT;
>  	const char *msg = strstr(partial_commit->buffer, "\n\n");
> +	int baselen;
>  
> -	OUTPUT(o, 3, "Committing notes in notes merge worktree at %.*s",
> -	       path_len - 1, path);
> +	strbuf_addstr(&path, git_path(NOTES_MERGE_WORKTREE));
> +	OUTPUT(o, 3, "Committing notes in notes merge worktree at %s", path.buf);
>  
>  	if (!msg || msg[2] == '\0')
>  		die("partial notes commit has empty message");
>  	msg += 2;
>  
> -	memset(&dir, 0, sizeof(dir));
> -	read_directory(&dir, path, path_len, NULL);
> -	for (i = 0; i < dir.nr; i++) {
> -		struct dir_entry *ent = dir.entries[i];
> +	dir = opendir(path.buf);
> +	if (!dir)
> +		die_errno("could not open %s", path.buf);
> +
> +	strbuf_addch(&path, '/');
> +	baselen = path.len;
> +	while ((e = readdir(dir)) != NULL) {
>  		struct stat st;
> -		const char *relpath = ent->name + path_len;
>  		unsigned char obj_sha1[20], blob_sha1[20];
>  
> -		if (ent->len - path_len != 40 || get_sha1_hex(relpath, obj_sha1)) {
> -			OUTPUT(o, 3, "Skipping non-SHA1 entry '%s'", ent->name);
> +		if (is_dot_or_dotdot(e->d_name))
> +			continue;
> +
> +		if (strlen(e->d_name) != 40 || get_sha1_hex(e->d_name, obj_sha1)) {
> +			OUTPUT(o, 3, "Skipping non-SHA1 entry '%s%s'", path.buf, e->d_name);
>  			continue;
>  		}
>  
> +		strbuf_addstr(&path, e->d_name);
>  		/* write file as blob, and add to partial_tree */
> -		if (stat(ent->name, &st))
> -			die_errno("Failed to stat '%s'", ent->name);
> -		if (index_path(blob_sha1, ent->name, &st, HASH_WRITE_OBJECT))
> -			die("Failed to write blob object from '%s'", ent->name);
> +		if (stat(path.buf, &st))
> +			die_errno("Failed to stat '%s'", path.buf);
> +		if (index_path(blob_sha1, path.buf, &st, HASH_WRITE_OBJECT))
> +			die("Failed to write blob object from '%s'", path.buf);
>  		if (add_note(partial_tree, obj_sha1, blob_sha1, NULL))
>  			die("Failed to add resolved note '%s' to notes tree",
> -			    ent->name);
> +			    path.buf);
>  		OUTPUT(o, 4, "Added resolved note for object %s: %s",
>  		       sha1_to_hex(obj_sha1), sha1_to_hex(blob_sha1));
> +		strbuf_setlen(&path, baselen);
>  	}
>  
>  	create_notes_commit(partial_tree, partial_commit->parents, msg,
>  			    result_sha1);
>  	OUTPUT(o, 4, "Finalized notes merge commit: %s",
>  	       sha1_to_hex(result_sha1));
> -	free(path);
> +	strbuf_release(&path);
> +	closedir(dir);
>  	return 0;
>  }

Re: [PATCH/WIP 02/11] notes-merge: use opendir/readdir instead of using read_directory()

[Nguyen Thai Ngoc Duy]

2011/10/26 Junio C Hamano <gitster@pobox.com>:
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> notes_merge_commit() only needs to list all entries (non-recursively)
>> under a directory, which can be easily accomplished with
>> opendir/readdir and would be more lightweight than read_directory().
>>
>> read_directory() is designed to list paths inside a working
>> directory. Using it outside of its scope may lead to undesired effects.
>
> Technically isn't the directory structure this codepath looks at a working
> tree that has extract of a notes tree commit?

Yes it's like a secondary working tree, only for notes, if I read the
code correctly. The thing is this space is inside ".git".

Current read_directory() treats given path separately from contents
inside the path. If the given path has ".git", it's ok (but it'll stop
at .git if during tree recursion). The new read_directory() does not
make this exception, so when note-merge call
read_directory(".git/NOTES_MERGE_WORKTREE"), read_directory() sees
".git" and stops immediately, assuming it's a gitlink.

One could say we should keep current behavior, but I don't really see
it's worth the effort.
-- 
Duy

Re: [PATCH/WIP 03/11] t5403: avoid doing "git add foo/bar" where foo/.git exists

[Nguyen Thai Ngoc Duy]

2011/10/26 Junio C Hamano <gitster@pobox.com>:
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> In this case, "foo" is considered a submodule and bar, if added,
>> belongs to foo/.git. "git add" should only allow "git add foo" in this
>> case, but it passes somehow.
>
> I do not think the above description is correct.
>
> The test:
>
>  - populates the current directory;
>  - makes a clone in ./clone2;
>  - creates a file clone2/b;
>  - runs "git add clone2/b" with GIT_DIR set to clone2/.git, without
>   setting GIT_WORK_TREE nor having core.worktree in clone2/.git/config.
>
> The last step should add a path "clone2/b" to $GIT_DIR/index (which is
> clone2/.git/index in this case).  The clone2 is not a submodule to the top
> level repository in this case, but even if it were, that would not change
> the definition of what the command should do when GIT_DIR is set without
> GIT_WORK_TREE nor core.worktree in $GIT_DIR/config.

Now look from "git add" perspective, it does not really care where
$GIT_DIR is. It assumes that $(pwd) is working directory's top. So it

 - reads content of current directory, it sees "clone2" as a directory
 - it descends in and see ".git" so "clone2" must be a git link
 - because clone2 is a separate repository (again $GIT_DIR is not
consulted), "b" should be managed by "clone2"
 - so it stops.

This is the only place I see a submodule (from the first glance) is
actually top level repository. Yes I guess we can support this, but
it's just too weird to be widely used in pratice..

> Running (cd clone2 && git add b) is a _more natural_ thing to do, and it
> will result in a path "b" added to the clone2 repository, so that the
> result is more useful if you are going to chdir to the repository and keep
> working on it.  But that does not mean the existing test is incorrect. It
> does not just pass somehow but the test passes by design.
>
> I did not check if later tests look at the contents of commit "new2" to
> make sure it contains "clone2/b", but if they do this change should break
> such tests.
>
> So I am puzzled by this change; what is this trying to achieve?
-- 
Duy

Re: [PATCH/WIP 03/11] t5403: avoid doing "git add foo/bar" where foo/.git exists

[Junio C Hamano]

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

> Now look from "git add" perspective, it does not really care where
> $GIT_DIR is.
> It assumes that $(pwd) is working directory's top. So it

Now you confused me.

Doesn't it use $GIT_DIR to find the index?  And it decides that it is at
the top level because it is given GIT_DIR but not GIT_WORKING_TREE which
is how working tree discovery is defined.

>  - reads content of current directory, it sees "clone2" as a directory
>  - it descends in and see ".git" so "clone2" must be a git link
>  - because clone2 is a separate repository (again $GIT_DIR is not
> consulted), "b" should be managed by "clone2"
>  - so it stops.
>
> This is the only place I see a submodule (from the first glance) is
> actually top level repository. Yes I guess we can support this, but
> it's just too weird to be widely used in pratice..

Where did you get this idea that submodule is somehow involved in this test?

I do not see there is any submodules involved; the test uses two
repositories 1 and 2 that appear in the working tree of the main
repository test infrastructure created, but otherwise there is no
sub/super relation among these three, and there are many other tests with
"clone" or "mkdir+init" or "init <newdir>" in the main test repository.

The clone2 repository tracks a path without having a corresponding file in
its working tree (i.e. it has "b" but tracks "clone2/b") which I already
is said unusual, but unusual does not mean it is bad or we want to remove
a test that covers the unusual case to let a change that regresses the
case go unnoticed.

>> Running (cd clone2 && git add b) is a _more natural_ thing to do, and it
>> will result in a path "b" added to the clone2 repository, so that the
>> result is more useful if you are going to chdir to the repository and keep
>> working on it. But that does not mean the existing test is incorrect. It
>> does not just pass somehow but the test passes by design.
>>
>> I did not check if later tests look at the contents of commit "new2" to
>> make sure it contains "clone2/b", but if they do this change should break
>> such tests.
>>
>> So I am puzzled by this change; what is this trying to achieve?

So again, what is this change trying to achieve?

Re: [PATCH/WIP 02/11] notes-merge: use opendir/readdir instead of using read_directory()

[Junio C Hamano]

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

> Current read_directory() treats given path separately from contents
> inside the path. If the given path has ".git", it's ok (but it'll stop
> at .git if during tree recursion). The new read_directory() does not
> make this exception, so when note-merge call
> read_directory(".git/NOTES_MERGE_WORKTREE"), read_directory() sees
> ".git" and stops immediately, assuming it's a gitlink.

When read_directory("where/ever") is called, what kind of paths does it
collect? Do the paths the function collects share "where/ever" as their
common prefix? I thought it collects the paths relative to whatever
top-level directory given to the function, so that "where/ever" could be
anything.

Why does it even have to look at the given path in the first place and
make a decision heavier than "can I opendir() and read from it"?  In other
words, if you see read_directory("some/thing/.git/more/stuff") and find a
substring ".git/" in there, what "magic" gitlink handling does the code
have to do?

I do not think it matters for _this_ particular case, but I can for
example imagine an alternative implementation of a merge that uses
temporary working tree somewhere other than the main working tree, and one
of the natural "temporary" places such a feature in the future may want to
use is inside .git/ somewhere. If you are planning to close the door by
breaking the behaviour of read_directory(".git/some_where") for such
callers with this series, we need to be aware of it, and that is why I am
not satisfied by your explanation.

Re: [PATCH/WIP 02/11] notes-merge: use opendir/readdir instead of using read_directory()

[Nguyen Thai Ngoc Duy]

On Thu, Oct 27, 2011 at 4:37 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
>
>> Current read_directory() treats given path separately from contents
>> inside the path. If the given path has ".git", it's ok (but it'll stop
>> at .git if during tree recursion). The new read_directory() does not
>> make this exception, so when note-merge call
>> read_directory(".git/NOTES_MERGE_WORKTREE"), read_directory() sees
>> ".git" and stops immediately, assuming it's a gitlink.
>
> When read_directory("where/ever") is called, what kind of paths does it
> collect? Do the paths the function collects share "where/ever" as their
> common prefix? I thought it collects the paths relative to whatever
> top-level directory given to the function, so that "where/ever" could be
> anything.

Correct. But read_directory() takes pathspec now so naturally it does
not treat "where/ever" a common prefix anymore. So it has to open(".")
and starts from there. Even current code does not trust "where/ever"
completely. treat_leading_path() may dismiss "where/ever" if it's
excluded by .gitignore.

> Why does it even have to look at the given path in the first place and
> make a decision heavier than "can I opendir() and read from it"?

Because opendir("wh*/*r") may fail.

> In other
> words, if you see read_directory("some/thing/.git/more/stuff") and find a
> substring ".git/" in there, what "magic" gitlink handling does the code
> have to do?

"some/thing/.git" can be considered a new entry in index, so it should
stop traversing at ".git". But because "some/thing/.git" does not
exacly match "some/thing/.git/more/stuff", it is filtered out.

git-add deals with this case especially in order to avoid accidentally
replace "some/thing/.git" in index with "some/thing/.git/more/stuff".
But I feel it should be handled by read_directory(), not git-add.

> I do not think it matters for _this_ particular case, but I can for
> example imagine an alternative implementation of a merge that uses
> temporary working tree somewhere other than the main working tree, and one
> of the natural "temporary" places such a feature in the future may want to
> use is inside .git/ somewhere. If you are planning to close the door by
> breaking the behaviour of read_directory(".git/some_where") for such
> callers with this series, we need to be aware of it, and that is why I am
> not satisfied by your explanation.

Maybe I should step back a bit. Instead of treating any input to
read_directory() as pathspec, callers may provide two sets: a prefix
set and a pathspec set. read_directory() starts from the prefix set
without any checks, then descends in using pathspec.

But then what about the "if (treat_one_path(..) == path_ignored)" in
treat_leading_path()? Remove it?
-- 
Duy

Re: [PATCH/WIP 03/11] t5403: avoid doing "git add foo/bar" where foo/.git exists

[Nguyen Thai Ngoc Duy]

On Thu, Oct 27, 2011 at 4:26 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>  - reads content of current directory, it sees "clone2" as a directory
>>  - it descends in and see ".git" so "clone2" must be a git link
>>  - because clone2 is a separate repository (again $GIT_DIR is not
>> consulted), "b" should be managed by "clone2"
>>  - so it stops.
>>
>> This is the only place I see a submodule (from the first glance) is
>> actually top level repository. Yes I guess we can support this, but
>> it's just too weird to be widely used in pratice..
>
> Where did you get this idea that submodule is somehow involved in this test?

Because "clone2" looks like a submodule (it has ".git" inside with valid HEAD)

> I do not see there is any submodules involved; the test uses two
> repositories 1 and 2 that appear in the working tree of the main
> repository test infrastructure created, but otherwise there is no
> sub/super relation among these three, and there are many other tests with
> "clone" or "mkdir+init" or "init <newdir>" in the main test repository.

If I tweak the test a bit

git clone ./. clone2 &&
GIT_DIR=clone2/.git git add clone2 &&
GIT_DIR=clone2/.git git add clone2/b

then the last command fails with "Path 'clone2/b' is in submodule
'clone2'". So clone2 could be a submodule from that perspective. Doing
the the other way around

git clone ./. clone2 &&
GIT_DIR=clone2/.git git add clone2/b &&
GIT_DIR=clone2/.git git add clone2

"clone2" is not just a normal path. Should we stick with one way only?
-- 
Duy

Re: [PATCH/WIP 02/11] notes-merge: use opendir/readdir instead of using read_directory()

[Junio C Hamano]

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

>> When read_directory("where/ever") is called, what kind of paths does it
>> collect? Do the paths the function collects share "where/ever" as their
>> common prefix? I thought it collects the paths relative to whatever
>> top-level directory given to the function, so that "where/ever" could be
>> anything.
>
> Correct. But read_directory() takes pathspec now so naturally it does
> not treat "where/ever" a common prefix anymore.  So it has to open(".")
> and starts from there.

That is a puzzling statement. The read_directory() function takes:

 - dir: use this struct to pass traversal status and collected paths;

 - path, len: this is the directory (not a pathspec) we start traversal
   from; and

 - pathspec: these are the patterns that specify which parts of the
   directory hierarchy under <path,len> are traversed.

I do not see any good reason for <path,len> to become a match pattern. Are
you trying to get it prepended to elements in pathspec[] and match the path
collected including the <path> part?

Why?

I could see that "open . and start from there, treating as if <path,len>
is also pathspec" could be made to work, but I do not see why that is
desirable.

In other words, are there existing callers that abuse read_directory()
to feed a pattern in <path,len>? Maybe they should be the one that needs
fixing instead?

Re: [PATCH/WIP 03/11] t5403: avoid doing "git add foo/bar" where foo/.git exists

[Junio C Hamano]

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

> On Thu, Oct 27, 2011 at 4:26 AM, Junio C Hamano <gitster@pobox.com> wrote:
> ...
>> Where did you get this idea that submodule is somehow involved in this test?
>
> Because "clone2" looks like a submodule (it has ".git" inside with valid HEAD)

But there is a crucial difference between "it looks like" and "it is". If
it is not a submodule, and the established behaviour is not to treat it
like a submodule, then we shouldn't suddenly change our behaviour to start
treating as one.

> ... Should we stick with one way only?

Whatever we have been doing should not change, especially in corner cases.

Re: [PATCH/WIP 01/11] Introduce "check-attr --excluded" as a replacement for "add --ignore-missing"

[Junio C Hamano]

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

> --ignore-missing is used by submodule to check if a path may be
> ignored by .gitignore files. It does not really fit in git-add (git
> add takes pathspec, but --ignore-missing takes only paths)
>
> Google reckons that --ignore-missing is not used anywhere but
> git-submodule.sh. Remove --ignore-missing and introduce "check-attr
> --excluded" as a replacement.

Hmm. "add --ignore-missing" somehow does not fit very well with other uses
of the option of the same name. In all other contexts, "ignore-missing"
means just that: ignore the fact that whatever _thing_ we were made to
expect to exist by the instruction from the user does not exist, which
usually results in an error or a report. "add --ignore-missing" does not
seem to be that (for one thing, it requires --dry-run).

It is unclear to me what is supposed to do to after reading the three-line
documentation in the manpage X-<.

So I am perfectly OK with the removal in the current form.

But I do not think "is this path ignored with the .gitignore rules" check
belongs to check-attr, either.

The pattern of having .scmignore files to list ignored paths were forced
upon us by historical version control systems, in the name of "the users
expect it". If there weren't such constraints, it would have been far
nicer---we could have just said "if you want to ignore paths, just use the
attributes mechanism and give them the 'ignored' attribute" without having
to have the exclude mechanism.

But we do not live in that ideal world.

Perhaps ls-files is a more suitable home for the feature?

Re: [PATCH/WIP 05/11] symbolize return values of tree_entry_interesting()

[Junio C Hamano]

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

> This helps extending the value later on for "interesting, but cannot
> decide if the entry truely matches yet" (ie. prefix matches)
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

Good change; it is a basic code hygiene to avoid magic constants anyway.

> diff --git a/tree-diff.c b/tree-diff.c
> index 6782484..25cc981 100644
> --- a/tree-diff.c
> +++ b/tree-diff.c
> @@ -114,12 +114,13 @@ static void show_entry(struct diff_options *opt, const char *prefix,
>  }
>  
>  static void skip_uninteresting(struct tree_desc *t, struct strbuf *base,
> -			       struct diff_options *opt, int *match)
> +			       struct diff_options *opt,
> +			       enum interesting *match)
>  {
>  	while (t->size) {
>  		*match = tree_entry_interesting(&t->entry, base, 0, &opt->pathspec);
>  		if (*match) {
> -			if (*match < 0)
> +			if (*match == all_entries_not_interesting)
>  				t->size = 0;
>  			break;
>  		}

The caller of this function needs to be updated as well.

But I have to wonder why this skip_uninteresting() does not peek the
original value of *match and skip, which is the loop structure the other
caller of tree_entry_interesting() in this file has.

diff --git a/tree-diff.c b/tree-diff.c
index 25cc981..de4ba28 100644
--- a/tree-diff.c
+++ b/tree-diff.c
@@ -133,7 +133,7 @@ int diff_tree(struct tree_desc *t1, struct tree_desc *t2,
 {
 	struct strbuf base;
 	int baselen = strlen(base_str);
-	int t1_match = 0, t2_match = 0;
+	enum interesting t1_match, t2_match;
 
 	/* Enable recursion indefinitely */
 	opt->pathspec.recursive = DIFF_OPT_TST(opt, RECURSIVE);
@@ -142,6 +142,9 @@ int diff_tree(struct tree_desc *t1, struct tree_desc *t2,
 	strbuf_init(&base, PATH_MAX);
 	strbuf_add(&base, base_str, baselen);
 
+	/* Initialize to something other than all_entries_not_interesting */
+	t1_match = t2_match = entry_not_interesting;
+
 	for (;;) {
 		if (diff_can_quit_early(opt))
 			break;

Re: [PATCH/WIP 02/11] notes-merge: use opendir/readdir instead of using read_directory()

[Nguyen Thai Ngoc Duy]

On Fri, Oct 28, 2011 at 4:23 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
>
>>> When read_directory("where/ever") is called, what kind of paths does it
>>> collect? Do the paths the function collects share "where/ever" as their
>>> common prefix? I thought it collects the paths relative to whatever
>>> top-level directory given to the function, so that "where/ever" could be
>>> anything.
>>
>> Correct. But read_directory() takes pathspec now so naturally it does
>> not treat "where/ever" a common prefix anymore.  So it has to open(".")
>> and starts from there.
>
> That is a puzzling statement. The read_directory() function takes:
>
>  - dir: use this struct to pass traversal status and collected paths;
>
>  - path, len: this is the directory (not a pathspec) we start traversal
>   from; and
>
>  - pathspec: these are the patterns that specify which parts of the
>   directory hierarchy under <path,len> are traversed.
>
> I do not see any good reason for <path,len> to become a match pattern. Are
> you trying to get it prepended to elements in pathspec[] and match the path
> collected including the <path> part?
>
> Why?
>
> I could see that "open . and start from there, treating as if <path,len>
> is also pathspec" could be made to work, but I do not see why that is
> desirable.
>
> In other words, are there existing callers that abuse read_directory()
> to feed a pattern in <path,len>? Maybe they should be the one that needs
> fixing instead?

fill_directory() tries to calculate a common prefix (i.e. <path,len>
to read_directory()) from pathspec and that may or may not work when
pathspec magic comes into play. But yes, I could just make
fill_directory() pass <"",0> to read_directory() and keep <path,len>
in read_directory() for notes-merge and future users.
-- 
Duy

Re: [PATCH/WIP 01/11] Introduce "check-attr --excluded" as a replacement for "add --ignore-missing"

[Nguyen Thai Ngoc Duy]

2011/10/28 Junio C Hamano <gitster@pobox.com>:
> Perhaps ls-files is a more suitable home for the feature?

ls-files sounds good. It does all kinds of file selection already.
I'll see if I can add -I (aka "show ignored files only) to it.
-- 
Duy

Re: [PATCH/WIP 03/11] t5403: avoid doing "git add foo/bar" where foo/.git exists

[Nguyen Thai Ngoc Duy]

On Fri, Oct 28, 2011 at 12:41 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> ... Should we stick with one way only?
>
> Whatever we have been doing should not change, especially in corner cases.

I disagree. If it's not right, then we should change it even though it
may face unpleasant consequences from misusing it. And I don't think
it's sane to behave like what we're doing now:

$ GIT_DIR=clone2/.git git ls-files --stage
100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0       1

$ ls -l clone2/2 3
-rw-r--r-- 1 pclouds users 0 Th10 30 12:40 3
-rw-r--r-- 1 pclouds users 0 Th10 30 12:40 clone2/2

$ GIT_DIR=clone2/.git git add clone2/2 3

$ GIT_DIR=clone2/.git git ls-files --stage
100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0       1
100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0       3

$ GIT_DIR=clone2/.git git add clone2/2

$ GIT_DIR=clone2/.git git ls-files --stage
100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0       1
100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0       3
100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0       clone2/2

"git add" behaves inconsistently when "clone2/2" and "3" are given and
when clone2/2 is given alone. This is just bad to me.

Note that this has nothing to do with read_directory() discussion we
had in the notes-merge patch, I agree we should keep the prefix. This
is about the calculating common prefix automatically from pathspec.
But prefix and pathspec are treated differently by read_directory().
In "git add clone2/2 3", common prefix is "" while in "git add
clone2/2" common prefix is "clone2".
-- 
Duy

Re: [PATCH/WIP 03/11] t5403: avoid doing "git add foo/bar" where foo/.git exists

[Junio C Hamano]

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

> Note that this has nothing to do with read_directory() discussion we
> had in the notes-merge patch...

I think we are in agreement on that point.

Going back to your example...

> $ GIT_DIR=clone2/.git git add clone2/2 3
>
> $ GIT_DIR=clone2/.git git ls-files --stage
> 100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0       1
> 100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0       3

You probably found a bug here. It is simply wrong to choose not to add
clone2/2, especially without telling the caller anything.

	Side note. I just did this and I am not getting what you saw above.

        $ mkdir -p /var/tmp/j/y && cd /var/tmp/j/y
        $ git init; git init clone2
        $ : >3; : >clone2/2
        $ GIT_DIR=clone2/.git git add clone2/2 3
        $ GIT_DIR=clone2/.git git ls-files
        3
	clone2/2

	The behavour is different when clone2/.git already has commit, and
        whatever codepath that gives these two different behaviour needs
        to be fixed.

By the way, I think I know where you are coming from.

If we think clone2/ and everything underneath belongs to a repository that
is _not_ governed by our GIT_DIR (which usually is .git), it may be nicer
when the user attempts to add clone2/2 (which would normally belong to
clone2/.git) to at least warn about it, or even error out. I would not be
entirely opposed to a change in the behaviour if the above example were
done without GIT_DIR and produced an error, like this:

    $ git add clone2/2 3; echo $?
    error: clone2/2 is outside our repository, possibly governed by clone2/.git
    1
    $ git ls-files
    1

After all, if clone2 were a submodule of our repository, we do notice and
error out an attempt to add clone2/2 to our repository, so if we changed
the way how "git add" behaves to do the above, I can buy an argument that
calls it a bugfix.

When GIT_DIR=clone2/.git is given, however, the caller explicitly declines
the repository discovery. We do not know how the repository we are dealing
with (which we were explicitly told with $GIT_DIR) and a directory whose
name is ".git" under "clone2" we happened to find in read_directory()
relates to each other, especially when our index does not have clone2 as
our submodule.

We however *do* know that our working tree is our current directory, so
it would be wrong to do this:

    $ GIT_DIR=clone2/.git git add clone2/2 3; echo $?
    error: 3 is outside our repository, possibly goverened by .git
    1

The command should just add clone2/2 and 3 as it was told to.

Re: [PATCH/WIP 05/11] symbolize return values of tree_entry_interesting()

[Nguyen Thai Ngoc Duy]

2011/10/28 Junio C Hamano <gitster@pobox.com>:
>>  static void skip_uninteresting(struct tree_desc *t, struct strbuf *base,
>> -                            struct diff_options *opt, int *match)
>> +                            struct diff_options *opt,
>> +                            enum interesting *match)
>>  {
>>       while (t->size) {
>>               *match = tree_entry_interesting(&t->entry, base, 0, &opt->pathspec);
>>               if (*match) {
>> -                     if (*match < 0)
>> +                     if (*match == all_entries_not_interesting)
>>                               t->size = 0;
>>                       break;
>>               }
>
> The caller of this function needs to be updated as well.

Yeah, thanks.

> But I have to wonder why this skip_uninteresting() does not peek the
> original value of *match and skip, which is the loop structure the other
> caller of tree_entry_interesting() in this file has.

Probably because no one asked that question before. I think it makes
sense for skip_uninteresting() to skip t_e_i() when *match == -1 or 2.
Thanks.
-- 
Duy

Re: [PATCH/WIP 03/11] t5403: avoid doing "git add foo/bar" where foo/.git exists

[Nguyen Thai Ngoc Duy]

On Sun, Oct 30, 2011 at 2:08 PM, Junio C Hamano <gitster@pobox.com> wrote:
>        Side note. I just did this and I am not getting what you saw above.
>
>        $ mkdir -p /var/tmp/j/y && cd /var/tmp/j/y
>        $ git init; git init clone2
>        $ : >3; : >clone2/2
>        $ GIT_DIR=clone2/.git git add clone2/2 3
>        $ GIT_DIR=clone2/.git git ls-files
>        3
>        clone2/2
>
>        The behavour is different when clone2/.git already has commit, and
>        whatever codepath that gives these two different behaviour needs
>        to be fixed.

It's resolve_gitlink_ref() in treat_directory(). I think replacing
that call() with is_git_directory() would fix this problem. We may
want to do the same with remove_dir_recursively().

> When GIT_DIR=clone2/.git is given, however, the caller explicitly declines
> the repository discovery. We do not know how the repository we are dealing
> with (which we were explicitly told with $GIT_DIR) and a directory whose
> name is ".git" under "clone2" we happened to find in read_directory()
> relates to each other, especially when our index does not have clone2 as
> our submodule.
>
> We however *do* know that our working tree is our current directory, so
> it would be wrong to do this:
>
>    $ GIT_DIR=clone2/.git git add clone2/2 3; echo $?
>    error: 3 is outside our repository, possibly goverened by .git
>    1
>
> The command should just add clone2/2 and 3 as it was told to.

I am concerned about clone2/2 in this case, not 3. I guess we can
check if clone2/.git is the repo we are using. If it is, skip it.
-- 
Duy

Re: [PATCH/WIP 03/11] t5403: avoid doing "git add foo/bar" where foo/.git exists

[Junio C Hamano]

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

>> We however *do* know that our working tree is our current directory, so
>> it would be wrong to do this:
>>
>>    $ GIT_DIR=clone2/.git git add clone2/2 3; echo $?
>>    error: 3 is outside our repository, possibly goverened by .git
>>    1
>>
>> The command should just add clone2/2 and 3 as it was told to.
>
> I am concerned about clone2/2 in this case, not 3. ...

Hmm... If that is the case, I am afraid that I failed to convey my point
in the previous message.

You are concerned about clone2/2 because you think GIT_DIR=clone2/.git
somehow implies that clone2/2 is a file at the toplevel of some repository
that should appear at "2" not at "clone2/2" in the index, no?

If that is the case, it means you are somehow getting the notion that
GIT_WORK_TREE is set to clone2 even though in the example we are
discussing it is _not_. Which in turn would mean "3" that is outside of
that directory should not even get into the picture.

In other words, the wish to register clone2/2 at "2" in the index by
stripping clone2/ and the wish to reject "3" as outside because it is
impossible to strip clone2/ from it are the same thing. You either should
be worrying about _both_ paths, or neither of them.

And I am saying that you should be worried about neither of them in this
case.  GIT_DIR=<some where> without GIT_WORK_TREE set has treated the
current directory as the top of the working tree from the beginning of
time, and both clone2/2 and 3 _should_ appear in the index in this
example, which is $GIT_DIR/index, which happens to be at a confusing
location that is clone2/.git/index.

> ... I guess we can
> check if clone2/.git is the repo we are using. If it is, skip it.

[PATCH 1/2] t3310: Add testcase demonstrating failure to --commit from within another dir

[Johan Herland]

Found-by: David Bremner <david@tethera.net>
Signed-off-by: Johan Herland <johan@herland.net>
---

This is a transcription of David's test script into a git test case.

Thanks to David for finding this issue.


Have fun! :)

...Johan

 t/t3310-notes-merge-manual-resolve.sh |   19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh
index 4367197..0c531c3 100755
--- a/t/t3310-notes-merge-manual-resolve.sh
+++ b/t/t3310-notes-merge-manual-resolve.sh
@@ -553,4 +553,23 @@ test_expect_success 'resolve situation by aborting the notes merge' '
 	verify_notes z
 '

+cat >expect_notes <<EOF
+foo
+bar
+EOF
+
+test_expect_failure 'switch cwd before committing notes merge' '
+	git notes add -m foo HEAD &&
+	git notes --ref=other add -m bar HEAD &&
+	test_must_fail git notes merge refs/notes/other &&
+	(
+		cd .git/NOTES_MERGE_WORKTREE &&
+		echo "foo" > $(git rev-parse HEAD) &&
+		echo "bar" >> $(git rev-parse HEAD) &&
+		git notes merge --commit
+	) &&
+	git notes show HEAD > actual_notes &&
+	test_cmp expect_notes actual_notes
+'
+
 test_done
--
1.7.9.2

[PATCH 2/2] notes-merge: use opendir/readdir instead of using read_directory()

[Johan Herland]

notes_merge_commit() only needs to list all entries (non-recursively)
under a directory, which can be easily accomplished with
opendir/readdir and would be more lightweight than read_directory().

read_directory() is designed to list paths inside a working
directory. Using it outside of its scope may lead to undesired effects.

Apparently, one of the undesired effects of read_directory() is that it
doesn't deal with being given absolute paths. This creates problems for
notes_merge_commit() when git_path() returns an absolute path, which
happens when the current working directory is in a subdirectory of the
.git directory.

Originally-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Updated-by:  Johan Herland <johan@herland.net>
Signed-off-by: Johan Herland <johan@herland.net>
---

This is a resurrection of pclouds' patch 2/11 in a patch series sent
last October for rewriting read_directory(). This patch doesn't
actually touch read_directory(), but instead rewrites
notes_merge_commit() to use opendir()/readdir() instead of
read_directory(). Since the usage of read_directory() is what caused
the bug that David found (in the previous patch), this rewrite happens
to fix that bug as well.


Have fun! :)

...Johan

 notes-merge.c                         |   50 ++++++++++++++++++++-------------
 t/t3310-notes-merge-manual-resolve.sh |    2 +-
 2 files changed, 31 insertions(+), 21 deletions(-)

diff --git a/notes-merge.c b/notes-merge.c
index fb0832f..3a16af2 100644
--- a/notes-merge.c
+++ b/notes-merge.c
@@ -687,51 +687,60 @@ int notes_merge_commit(struct notes_merge_options *o,
 {
 	/*
 	 * Iterate through files in .git/NOTES_MERGE_WORKTREE and add all
-	 * found notes to 'partial_tree'. Write the updates notes tree to
+	 * found notes to 'partial_tree'. Write the updated notes tree to
 	 * the DB, and commit the resulting tree object while reusing the
 	 * commit message and parents from 'partial_commit'.
 	 * Finally store the new commit object SHA1 into 'result_sha1'.
 	 */
-	struct dir_struct dir;
-	char *path = xstrdup(git_path(NOTES_MERGE_WORKTREE "/"));
-	int path_len = strlen(path), i;
+	DIR *dir;
+	struct dirent *e;
+	struct strbuf path = STRBUF_INIT;
 	char *msg = strstr(partial_commit->buffer, "\n\n");
 	struct strbuf sb_msg = STRBUF_INIT;
+	int baselen;

+	strbuf_addstr(&path, git_path(NOTES_MERGE_WORKTREE));
 	if (o->verbosity >= 3)
-		printf("Committing notes in notes merge worktree at %.*s\n",
-			path_len - 1, path);
+		printf("Committing notes in notes merge worktree at %s\n",
+			path.buf);

 	if (!msg || msg[2] == '\0')
 		die("partial notes commit has empty message");
 	msg += 2;

-	memset(&dir, 0, sizeof(dir));
-	read_directory(&dir, path, path_len, NULL);
-	for (i = 0; i < dir.nr; i++) {
-		struct dir_entry *ent = dir.entries[i];
+	dir = opendir(path.buf);
+	if (!dir)
+		die_errno("could not open %s", path.buf);
+
+	strbuf_addch(&path, '/');
+	baselen = path.len;
+	while ((e = readdir(dir)) != NULL) {
 		struct stat st;
-		const char *relpath = ent->name + path_len;
 		unsigned char obj_sha1[20], blob_sha1[20];

-		if (ent->len - path_len != 40 || get_sha1_hex(relpath, obj_sha1)) {
+		if (is_dot_or_dotdot(e->d_name))
+			continue;
+
+		if (strlen(e->d_name) != 40 || get_sha1_hex(e->d_name, obj_sha1)) {
 			if (o->verbosity >= 3)
-				printf("Skipping non-SHA1 entry '%s'\n",
-								ent->name);
+				printf("Skipping non-SHA1 entry '%s%s'\n",
+					path.buf, e->d_name);
 			continue;
 		}

+		strbuf_addstr(&path, e->d_name);
 		/* write file as blob, and add to partial_tree */
-		if (stat(ent->name, &st))
-			die_errno("Failed to stat '%s'", ent->name);
-		if (index_path(blob_sha1, ent->name, &st, HASH_WRITE_OBJECT))
-			die("Failed to write blob object from '%s'", ent->name);
+		if (stat(path.buf, &st))
+			die_errno("Failed to stat '%s'", path.buf);
+		if (index_path(blob_sha1, path.buf, &st, HASH_WRITE_OBJECT))
+			die("Failed to write blob object from '%s'", path.buf);
 		if (add_note(partial_tree, obj_sha1, blob_sha1, NULL))
 			die("Failed to add resolved note '%s' to notes tree",
-			    ent->name);
+			    path.buf);
 		if (o->verbosity >= 4)
 			printf("Added resolved note for object %s: %s\n",
 				sha1_to_hex(obj_sha1), sha1_to_hex(blob_sha1));
+		strbuf_setlen(&path, baselen);
 	}

 	strbuf_attach(&sb_msg, msg, strlen(msg), strlen(msg) + 1);
@@ -740,7 +749,8 @@ int notes_merge_commit(struct notes_merge_options *o,
 	if (o->verbosity >= 4)
 		printf("Finalized notes merge commit: %s\n",
 			sha1_to_hex(result_sha1));
-	free(path);
+	strbuf_release(&path);
+	closedir(dir);
 	return 0;
 }

diff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh
index 0c531c3..d6d6ac6 100755
--- a/t/t3310-notes-merge-manual-resolve.sh
+++ b/t/t3310-notes-merge-manual-resolve.sh
@@ -558,7 +558,7 @@ foo
 bar
 EOF

-test_expect_failure 'switch cwd before committing notes merge' '
+test_expect_success 'switch cwd before committing notes merge' '
 	git notes add -m foo HEAD &&
 	git notes --ref=other add -m bar HEAD &&
 	test_must_fail git notes merge refs/notes/other &&
--
1.7.9.2

Re: [PATCH 2/2] notes-merge: use opendir/readdir instead of using read_directory()

[Nguyen Thai Ngoc Duy]

On Mon, Mar 12, 2012 at 9:47 PM, Johan Herland <johan@herland.net> wrote:
> This is a resurrection of pclouds' patch 2/11 in a patch series sent
> last October for rewriting read_directory(). This patch doesn't
> actually touch read_directory(), but instead rewrites
> notes_merge_commit() to use opendir()/readdir() instead of
> read_directory(). Since the usage of read_directory() is what caused
> the bug that David found (in the previous patch), this rewrite happens
> to fix that bug as well.

Happy to help. And that reminds me I've got to revive the
read_directory() rewrite soon. Got stuck at git-add because I aimed
too high. Gaah...
-- 
Duy

[PATCH 1/2] t3310: Add testcase demonstrating failure to --commit from within another dir

[Johan Herland]

Found-by: David Bremner <david@tethera.net>
Signed-off-by: Johan Herland <johan@herland.net>
---

(sending again in the correct thread. Sorry for the screwup.)

This is a transcription of David's test script into a git test case.

Thanks to David for finding this issue.


Have fun! :)

...Johan

 t/t3310-notes-merge-manual-resolve.sh |   19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh
index 4367197..0c531c3 100755
--- a/t/t3310-notes-merge-manual-resolve.sh
+++ b/t/t3310-notes-merge-manual-resolve.sh
@@ -553,4 +553,23 @@ test_expect_success 'resolve situation by aborting the notes merge' '
 	verify_notes z
 '

+cat >expect_notes <<EOF
+foo
+bar
+EOF
+
+test_expect_failure 'switch cwd before committing notes merge' '
+	git notes add -m foo HEAD &&
+	git notes --ref=other add -m bar HEAD &&
+	test_must_fail git notes merge refs/notes/other &&
+	(
+		cd .git/NOTES_MERGE_WORKTREE &&
+		echo "foo" > $(git rev-parse HEAD) &&
+		echo "bar" >> $(git rev-parse HEAD) &&
+		git notes merge --commit
+	) &&
+	git notes show HEAD > actual_notes &&
+	test_cmp expect_notes actual_notes
+'
+
 test_done
--
1.7.9.2

Re: [PATCH 1/2] t3310: Add testcase demonstrating failure to --commit from within another dir

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> Found-by: David Bremner <david@tethera.net>
> Signed-off-by: Johan Herland <johan@herland.net>

Could you clarify what "from within another dir" means on the subject?

What was the expected usage?

	The 'git notes merge' command expected to be run from the
        working tree of the project being annotated, and did not
        anticipate getting run inside $GIT_DIR/.  However, because
        we use $GIT_DIR/NOTES_MERGE_WORKTREE as a temporary working
        space for the user to work on resolving conflicts, it is not
        unreasonable for a user to run "git notes merge --commit"
        there.

Is that the issue?

> ---
>
> (sending again in the correct thread. Sorry for the screwup.)
>
> This is a transcription of David's test script into a git test case.
>
> Thanks to David for finding this issue.
>
>
> Have fun! :)
>
> ...Johan
>
>  t/t3310-notes-merge-manual-resolve.sh |   19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>
> diff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh
> index 4367197..0c531c3 100755
> --- a/t/t3310-notes-merge-manual-resolve.sh
> +++ b/t/t3310-notes-merge-manual-resolve.sh
> @@ -553,4 +553,23 @@ test_expect_success 'resolve situation by aborting the notes merge' '
>  	verify_notes z
>  '
>
> +cat >expect_notes <<EOF
> +foo
> +bar
> +EOF
> +
> +test_expect_failure 'switch cwd before committing notes merge' '
> +	git notes add -m foo HEAD &&
> +	git notes --ref=other add -m bar HEAD &&
> +	test_must_fail git notes merge refs/notes/other &&
> +	(
> +		cd .git/NOTES_MERGE_WORKTREE &&
> +		echo "foo" > $(git rev-parse HEAD) &&
> +		echo "bar" >> $(git rev-parse HEAD) &&
> +		git notes merge --commit
> +	) &&
> +	git notes show HEAD > actual_notes &&
> +	test_cmp expect_notes actual_notes
> +'
> +
>  test_done
> --
> 1.7.9.2

Re: [PATCH 1/2] t3310: Add testcase demonstrating failure to --commit from within another dir

[Johan Herland]

On Mon, Mar 12, 2012 at 19:21, Junio C Hamano <gitster@pobox.com> wrote:
> Johan Herland <johan@herland.net> writes:
>
>> Found-by: David Bremner <david@tethera.net>
>> Signed-off-by: Johan Herland <johan@herland.net>
>
> Could you clarify what "from within another dir" means on the subject?
>
> What was the expected usage?
>
>        The 'git notes merge' command expected to be run from the
>        working tree of the project being annotated, and did not
>        anticipate getting run inside $GIT_DIR/.  However, because
>        we use $GIT_DIR/NOTES_MERGE_WORKTREE as a temporary working
>        space for the user to work on resolving conflicts, it is not
>        unreasonable for a user to run "git notes merge --commit"
>        there.
>
> Is that the issue?

That is exactly the issue. Thanks for the clear wording.

Feel free to update the commit message accordingly.


...Johan

>> ---
>>
>> (sending again in the correct thread. Sorry for the screwup.)
>>
>> This is a transcription of David's test script into a git test case.
>>
>> Thanks to David for finding this issue.
>>
>>
>> Have fun! :)
>>
>> ...Johan
>>
>>  t/t3310-notes-merge-manual-resolve.sh |   19 +++++++++++++++++++
>>  1 file changed, 19 insertions(+)
>>
>> diff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh
>> index 4367197..0c531c3 100755
>> --- a/t/t3310-notes-merge-manual-resolve.sh
>> +++ b/t/t3310-notes-merge-manual-resolve.sh
>> @@ -553,4 +553,23 @@ test_expect_success 'resolve situation by aborting the notes merge' '
>>       verify_notes z
>>  '
>>
>> +cat >expect_notes <<EOF
>> +foo
>> +bar
>> +EOF
>> +
>> +test_expect_failure 'switch cwd before committing notes merge' '
>> +     git notes add -m foo HEAD &&
>> +     git notes --ref=other add -m bar HEAD &&
>> +     test_must_fail git notes merge refs/notes/other &&
>> +     (
>> +             cd .git/NOTES_MERGE_WORKTREE &&
>> +             echo "foo" > $(git rev-parse HEAD) &&
>> +             echo "bar" >> $(git rev-parse HEAD) &&
>> +             git notes merge --commit
>> +     ) &&
>> +     git notes show HEAD > actual_notes &&
>> +     test_cmp expect_notes actual_notes
>> +'
>> +
>>  test_done
>> --
>> 1.7.9.2



-- 
Johan Herland, <johan@herland.net>
www.herland.net

[PATCH jh/notes-merge-in-git-dir-worktree] fixup! t3310 on Windows

[Johannes Sixt]

From: Johannes Sixt <j6t@kdbg.org>

On Windows, a directory cannot be removed while it is the working
directory of a process. "git notes merge --commit" attempts to remove
.git/NOTES_MERGE_WORKTREE, but during the test the directory was still
"occupied" by the shell. Move the command out of the subshell to release
the directory.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 Feel free to squash this into 1/2.

 t/t3310-notes-merge-manual-resolve.sh |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh
index d6d6ac6..6351877 100755
--- a/t/t3310-notes-merge-manual-resolve.sh
+++ b/t/t3310-notes-merge-manual-resolve.sh
@@ -565,9 +565,9 @@ test_expect_success 'switch cwd before committing notes merge' '
 	(
 		cd .git/NOTES_MERGE_WORKTREE &&
 		echo "foo" > $(git rev-parse HEAD) &&
-		echo "bar" >> $(git rev-parse HEAD) &&
-		git notes merge --commit
+		echo "bar" >> $(git rev-parse HEAD)
 	) &&
+	git notes merge --commit &&
 	git notes show HEAD > actual_notes &&
 	test_cmp expect_notes actual_notes
 '
-- 
1.7.10.rc0.1198.g85187

Re: [PATCH jh/notes-merge-in-git-dir-worktree] fixup! t3310 on Windows

[Johan Herland]

On Wed, Mar 14, 2012 at 09:39, Johannes Sixt <j.sixt@viscovery.net> wrote:
> From: Johannes Sixt <j6t@kdbg.org>
>
> On Windows, a directory cannot be removed while it is the working
> directory of a process. "git notes merge --commit" attempts to remove
> .git/NOTES_MERGE_WORKTREE, but during the test the directory was still
> "occupied" by the shell. Move the command out of the subshell to release
> the directory.
>
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> ---
>  Feel free to squash this into 1/2.
>
>  t/t3310-notes-merge-manual-resolve.sh |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh
> index d6d6ac6..6351877 100755
> --- a/t/t3310-notes-merge-manual-resolve.sh
> +++ b/t/t3310-notes-merge-manual-resolve.sh
> @@ -565,9 +565,9 @@ test_expect_success 'switch cwd before committing notes merge' '
>        (
>                cd .git/NOTES_MERGE_WORKTREE &&
>                echo "foo" > $(git rev-parse HEAD) &&
> -               echo "bar" >> $(git rev-parse HEAD) &&
> -               git notes merge --commit
> +               echo "bar" >> $(git rev-parse HEAD)
>        ) &&
> +       git notes merge --commit &&

NAK. This defeats the entire purpose of this test. The bug that we're
trying to solve is exactly the situation where the user has changed
into the .git/NOTES_MERGE_WORKTREE directory, and invokes 'git notes
merge --commit' from within. We need to find a different solution for
this on Windows. Maybe we should just abort 'git notes merge
--commit/--abort' if the current directory is within
.git/NOTES_MERGE_WORKTREE (and we're on Windows)?


...Johan


-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [PATCH jh/notes-merge-in-git-dir-worktree] fixup! t3310 on Windows

[Johannes Sixt]

Am 3/14/2012 12:39, schrieb Johan Herland:
> On Wed, Mar 14, 2012 at 09:39, Johannes Sixt <j.sixt@viscovery.net> wrote:
>> From: Johannes Sixt <j6t@kdbg.org>
>>
>> On Windows, a directory cannot be removed while it is the working
>> directory of a process. "git notes merge --commit" attempts to remove
>> .git/NOTES_MERGE_WORKTREE, but during the test the directory was still
>> "occupied" by the shell. Move the command out of the subshell to release
>> the directory.
>>
>> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
>> ---
>>  Feel free to squash this into 1/2.
>>
>>  t/t3310-notes-merge-manual-resolve.sh |    4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh
>> index d6d6ac6..6351877 100755
>> --- a/t/t3310-notes-merge-manual-resolve.sh
>> +++ b/t/t3310-notes-merge-manual-resolve.sh
>> @@ -565,9 +565,9 @@ test_expect_success 'switch cwd before committing notes merge' '
>>        (
>>                cd .git/NOTES_MERGE_WORKTREE &&
>>                echo "foo" > $(git rev-parse HEAD) &&
>> -               echo "bar" >> $(git rev-parse HEAD) &&
>> -               git notes merge --commit
>> +               echo "bar" >> $(git rev-parse HEAD)
>>        ) &&
>> +       git notes merge --commit &&
> 
> NAK. This defeats the entire purpose of this test. The bug that we're
> trying to solve is exactly the situation where the user has changed
> into the .git/NOTES_MERGE_WORKTREE directory, and invokes 'git notes
> merge --commit' from within. We need to find a different solution for
> this on Windows. Maybe we should just abort 'git notes merge
> --commit/--abort' if the current directory is within
> .git/NOTES_MERGE_WORKTREE (and we're on Windows)?

Isn't this an indication that something *VERY* wrong is happening? How do
you explain to POSIX people that you have just pulled the rug unter their
feet?

$ git notes merge --commit
$ git notes
fatal: Unable to read current working directory: No such file or directory

I doubt that the use-case that is tested here makes sense.

Or .git/NOTES_MERGE_WORKTREE should not be removed. Would it be an option
to clear it out only when it is needed, right before it is filled again?

-- Hannes

Re: [PATCH jh/notes-merge-in-git-dir-worktree] fixup! t3310 on Windows

[David Bremner]

> I doubt that the use-case that is tested here makes sense.

To me, the obvious workflow to resolve notes conflicts is is chdir into
this directory, edit files, and then call git notes merge --commit. The
(implicit) requirement that one cannot call commit from within the
directory you edited files was quite surprising to me. So in my opinion
the use case does make sense, which is why I submitted the bug in the
first place.

In any case, it should not fail silently, whether or not one is required
to chdir back to the worktree before calling git notes merge --commit.

d

Re: [PATCH jh/notes-merge-in-git-dir-worktree] fixup! t3310 on Windows

[Johan Herland]

On Wed, Mar 14, 2012 at 12:59, Johannes Sixt <j.sixt@viscovery.net> wrote:
> Am 3/14/2012 12:39, schrieb Johan Herland:
>> On Wed, Mar 14, 2012 at 09:39, Johannes Sixt <j.sixt@viscovery.net> wrote:
>>> From: Johannes Sixt <j6t@kdbg.org>
>>>
>>> On Windows, a directory cannot be removed while it is the working
>>> directory of a process. "git notes merge --commit" attempts to remove
>>> .git/NOTES_MERGE_WORKTREE, but during the test the directory was still
>>> "occupied" by the shell. Move the command out of the subshell to release
>>> the directory.
>>>
>>> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
>>> ---
>>>  Feel free to squash this into 1/2.
>>>
>>>  t/t3310-notes-merge-manual-resolve.sh |    4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh
>>> index d6d6ac6..6351877 100755
>>> --- a/t/t3310-notes-merge-manual-resolve.sh
>>> +++ b/t/t3310-notes-merge-manual-resolve.sh
>>> @@ -565,9 +565,9 @@ test_expect_success 'switch cwd before committing notes merge' '
>>>        (
>>>                cd .git/NOTES_MERGE_WORKTREE &&
>>>                echo "foo" > $(git rev-parse HEAD) &&
>>> -               echo "bar" >> $(git rev-parse HEAD) &&
>>> -               git notes merge --commit
>>> +               echo "bar" >> $(git rev-parse HEAD)
>>>        ) &&
>>> +       git notes merge --commit &&
>>
>> NAK. This defeats the entire purpose of this test. The bug that we're
>> trying to solve is exactly the situation where the user has changed
>> into the .git/NOTES_MERGE_WORKTREE directory, and invokes 'git notes
>> merge --commit' from within. We need to find a different solution for
>> this on Windows. Maybe we should just abort 'git notes merge
>> --commit/--abort' if the current directory is within
>> .git/NOTES_MERGE_WORKTREE (and we're on Windows)?
>
> Isn't this an indication that something *VERY* wrong is happening? How do
> you explain to POSIX people that you have just pulled the rug unter their
> feet?
>
> $ git notes merge --commit
> $ git notes
> fatal: Unable to read current working directory: No such file or directory

True.

> I doubt that the use-case that is tested here makes sense.

As David wrote, the use case is likely to pop up among regular users.
We can't simply ignore it.

> Or .git/NOTES_MERGE_WORKTREE should not be removed. Would it be an option
> to clear it out only when it is needed, right before it is filled again?

Maybe, but then we wouldn't be able to warn or abort in the case where
there is a previous unfinished notes merge, and the user tries to
start a new notes merge. Instead, we'd silently overwrite the previous
unfinished notes merge...

Maybe it's better to simply detect if cwd is inside
.git/NOTES_MERGE_WORKTREE, and then abort, telling the user to chdir
out before trying again?


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [PATCH jh/notes-merge-in-git-dir-worktree] fixup! t3310 on Windows

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> On Wed, Mar 14, 2012 at 12:59, Johannes Sixt <j.sixt@viscovery.net> wrote:
> ...
>> I doubt that the use-case that is tested here makes sense.
>
> As David wrote, the use case is likely to pop up among regular users.
> We can't simply ignore it.
>
>> Or .git/NOTES_MERGE_WORKTREE should not be removed. Would it be an option
>> to clear it out only when it is needed, right before it is filled again?
>
> Maybe, but then we wouldn't be able to warn or abort in the case where
> there is a previous unfinished notes merge, and the user tries to
> start a new notes merge. Instead, we'd silently overwrite the previous
> unfinished notes merge...

We cannot simply ignore it.

You _could_ do is perhaps to do something like:

 - when you notice that you need to ask the user to hand-merge a temporary
   file (i.e. when 'notes merge' is run), check if .git/N_M_W/done exists.

    - If you see that marker file, remove it everything in the
      directory, deposit the temporary file you want the user to edit,
      and expect the user to later tell you that he is done by "notes
      merge --commit";

    - If you don't, the user didn't give you "notes merge --commit"
      during the earlier run.  Tell him to first abandon the previous
      round with "notes merge --abandon" or something.

 - "notes merge --commit" will just mark .git/N_M_W/ with "done".

 - "notes merge --abandon" will remove .git/N_M_W/.

but it only supports the simplest "Run 'notes merge', cd to .git/N_M_W,
edit all, run 'notes merge --commit'" workflow.  If the user decides to
abandon instead of commit, he will be inside the problematic directory, so
trying to be more elaborate like the above does not really solve anything
fundamental.  The user has to come out of that temporary directory
eventually anyway.

So I think the following is not just an improvement over the current code,
but probably an acceptable solution, given its simplicity.

> Maybe it's better to simply detect if cwd is inside
> .git/NOTES_MERGE_WORKTREE, and then abort, telling the user to chdir
> out before trying again?

[PATCH 3/2] notes-merge: Don't remove .git/NOTES_MERGE_WORKTREE; it may be the user's cwd

[Johan Herland]

When a manual notes merge is committed or aborted, we need to remove the
temporary worktree at .git/NOTES_MERGE_WORKTREE. However, removing the
entire directory is not good if the user ran the 'git notes merge
--commit/--abort' from within that directory. On Windows, the directory
removal would simply fail, while on POSIX systems, users would suddenly
find themselves in an invalid current directory.

Therefore, instead of deleting the entire directory, we delete everything
_within_ the directory, and leave the (empty) directory in place.

This would cause a subsequent notes merge to abort, complaining about a
previous - unfinished - notes merge (due to the presence of
.git/NOTES_MERGE_WORKTREE), so we also need to adjust this check to only
trigger when .git/NOTES_MERGE_WORKTREE is non-empty.

Finally, adjust the t3310 manual notes merge testcases to correctly handle
the existence of an empty .git/NOTES_MERGE_WORKTREE directory.

Inspired-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Johan Herland <johan@herland.net>
---

How about this solution? I believe it should solve all the cases.

I'm torn about the new remove_everything_inside_dir(). Obviously it's a
copy-paste-modify of dir.c:remove_dir_recursively(), and could instead be
implemented by adding an extra flag to remove_dir_recursively(). However,
adding a "#define REMOVE_DIR_CONTENTS_BUT_NOT_DIR_ITSELF 04" seemed even
uglier to me...

What do you think?


...Johan


 notes-merge.c                         |   52 ++++++++++++++++++++++++++++++---
 t/t3310-notes-merge-manual-resolve.sh |    8 ++---
 2 files changed, 52 insertions(+), 8 deletions(-)

diff --git a/notes-merge.c b/notes-merge.c
index 3a16af2..bf080fb 100644
--- a/notes-merge.c
+++ b/notes-merge.c
@@ -267,7 +267,8 @@ static void check_notes_merge_worktree(struct notes_merge_options *o)
 		 * Must establish NOTES_MERGE_WORKTREE.
 		 * Abort if NOTES_MERGE_WORKTREE already exists
 		 */
-		if (file_exists(git_path(NOTES_MERGE_WORKTREE))) {
+		if (file_exists(git_path(NOTES_MERGE_WORKTREE)) &&
+		    !is_empty_dir(git_path(NOTES_MERGE_WORKTREE))) {
 			if (advice_resolve_conflict)
 				die("You have not concluded your previous "
 				    "notes merge (%s exists).\nPlease, use "
@@ -754,16 +755,59 @@ int notes_merge_commit(struct notes_merge_options *o,
 	return 0;
 }
 
+/* Based on dir.c:remove_dir_recursively() */
+static int remove_everything_inside_dir(struct strbuf *path)
+{
+	DIR *dir;
+	struct dirent *e;
+	int ret = 0, original_len = path->len, len;
+
+	dir = opendir(path->buf);
+	if (!dir)
+		return -1;
+	if (path->buf[original_len - 1] != '/')
+		strbuf_addch(path, '/');
+
+	len = path->len;
+	while ((e = readdir(dir)) != NULL) {
+		struct stat st;
+		if (is_dot_or_dotdot(e->d_name))
+			continue;
+
+		strbuf_setlen(path, len);
+		strbuf_addstr(path, e->d_name);
+		if (lstat(path->buf, &st))
+			; /* fall thru */
+		else if (S_ISDIR(st.st_mode)) {
+			if (!remove_dir_recursively(path, 0))
+				continue; /* happy */
+		} else if (!unlink(path->buf))
+			continue; /* happy, too */
+
+		/* path too long, stat fails, or non-directory still exists */
+		ret = -1;
+		break;
+	}
+	closedir(dir);
+
+	strbuf_setlen(path, original_len);
+	return ret;
+}
+
 int notes_merge_abort(struct notes_merge_options *o)
 {
-	/* Remove .git/NOTES_MERGE_WORKTREE directory and all files within */
+	/*
+	 * Remove all files within .git/NOTES_MERGE_WORKTREE. We do not remove
+	 * the .git/NOTES_MERGE_WORKTREE directory itself, since it might be
+	 * the current working directory of the user.
+	 */
 	struct strbuf buf = STRBUF_INIT;
 	int ret;
 
 	strbuf_addstr(&buf, git_path(NOTES_MERGE_WORKTREE));
 	if (o->verbosity >= 3)
-		printf("Removing notes merge worktree at %s\n", buf.buf);
-	ret = remove_dir_recursively(&buf, 0);
+		printf("Removing notes merge worktree at %s/*\n", buf.buf);
+	ret = remove_everything_inside_dir(&buf);
 	strbuf_release(&buf);
 	return ret;
 }
diff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh
index d6d6ac6..195bb97 100755
--- a/t/t3310-notes-merge-manual-resolve.sh
+++ b/t/t3310-notes-merge-manual-resolve.sh
@@ -324,7 +324,7 @@ y and z notes on 4th commit
 EOF
 	git notes merge --commit &&
 	# No .git/NOTES_MERGE_* files left
-	test_must_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&
+	test_might_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&
 	test_cmp /dev/null output &&
 	# Merge commit has pre-merge y and pre-merge z as parents
 	test "$(git rev-parse refs/notes/m^1)" = "$(cat pre_merge_y)" &&
@@ -386,7 +386,7 @@ test_expect_success 'redo merge of z into m (== y) with default ("manual") resol
 test_expect_success 'abort notes merge' '
 	git notes merge --abort &&
 	# No .git/NOTES_MERGE_* files left
-	test_must_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&
+	test_might_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&
 	test_cmp /dev/null output &&
 	# m has not moved (still == y)
 	test "$(git rev-parse refs/notes/m)" = "$(cat pre_merge_y)" &&
@@ -453,7 +453,7 @@ EOF
 	# Finalize merge
 	git notes merge --commit &&
 	# No .git/NOTES_MERGE_* files left
-	test_must_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&
+	test_might_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&
 	test_cmp /dev/null output &&
 	# Merge commit has pre-merge y and pre-merge z as parents
 	test "$(git rev-parse refs/notes/m^1)" = "$(cat pre_merge_y)" &&
@@ -542,7 +542,7 @@ EOF
 test_expect_success 'resolve situation by aborting the notes merge' '
 	git notes merge --abort &&
 	# No .git/NOTES_MERGE_* files left
-	test_must_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&
+	test_might_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&
 	test_cmp /dev/null output &&
 	# m has not moved (still == w)
 	test "$(git rev-parse refs/notes/m)" = "$(git rev-parse refs/notes/w)" &&
-- 
1.7.10.rc0.43.g35011

Re: [PATCH 3/2] notes-merge: Don't remove .git/NOTES_MERGE_WORKTREE; it may be the user's cwd

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> I'm torn about the new remove_everything_inside_dir(). Obviously it's a
> copy-paste-modify of dir.c:remove_dir_recursively(), and could instead be
> implemented by adding an extra flag to remove_dir_recursively(). However,
> adding a "#define REMOVE_DIR_CONTENTS_BUT_NOT_DIR_ITSELF 04" seemed even
> uglier to me...

Hmm, what ugliness am I missing when viewing the attached patch?  It looks
simple and straightforward enough, at least to me.

 dir.c |   14 ++++++++++----
 dir.h |    1 +
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/dir.c b/dir.c
index 0a78d00..6432728 100644
--- a/dir.c
+++ b/dir.c
@@ -1178,6 +1178,7 @@ int remove_dir_recursively(struct strbuf *path, int flag)
 	struct dirent *e;
 	int ret = 0, original_len = path->len, len;
 	int only_empty = (flag & REMOVE_DIR_EMPTY_ONLY);
+	int keep_toplevel = (flag & REMOVE_DIR_KEEP_TOPLEVEL);
 	unsigned char submodule_head[20];
 
 	if ((flag & REMOVE_DIR_KEEP_NESTED_GIT) &&
@@ -1185,9 +1186,14 @@ int remove_dir_recursively(struct strbuf *path, int flag)
 		/* Do not descend and nuke a nested git work tree. */
 		return 0;
 
+	flag &= ~REMOVE_DIR_KEEP_TOPLEVEL;
 	dir = opendir(path->buf);
-	if (!dir)
-		return rmdir(path->buf);
+	if (!dir) {
+		if (!keep_toplevel)
+			return rmdir(path->buf);
+		else
+			return -1;
+	}
 	if (path->buf[original_len - 1] != '/')
 		strbuf_addch(path, '/');
 
@@ -1202,7 +1208,7 @@ int remove_dir_recursively(struct strbuf *path, int flag)
 		if (lstat(path->buf, &st))
 			; /* fall thru */
 		else if (S_ISDIR(st.st_mode)) {
-			if (!remove_dir_recursively(path, only_empty))
+			if (!remove_dir_recursively(path, flag))
 				continue; /* happy */
 		} else if (!only_empty && !unlink(path->buf))
 			continue; /* happy, too */
@@ -1214,7 +1220,7 @@ int remove_dir_recursively(struct strbuf *path, int flag)
 	closedir(dir);
 
 	strbuf_setlen(path, original_len);
-	if (!ret)
+	if (!ret && !keep_toplevel)
 		ret = rmdir(path->buf);
 	return ret;
 }
diff --git a/dir.h b/dir.h
index dd6947e..58b6fc7 100644
--- a/dir.h
+++ b/dir.h
@@ -102,6 +102,7 @@ extern void setup_standard_excludes(struct dir_struct *dir);
 
 #define REMOVE_DIR_EMPTY_ONLY 01
 #define REMOVE_DIR_KEEP_NESTED_GIT 02
+#define REMOVE_DIR_KEEP_TOPLEVEL 04
 extern int remove_dir_recursively(struct strbuf *path, int flag);
 
 /* tries to remove the path with empty directories along it, ignores ENOENT */
-- 
1.7.10.rc1.22.g07e85

Re: [PATCH 3/2] notes-merge: Don't remove .git/NOTES_MERGE_WORKTREE; it may be the user's cwd

[Junio C Hamano]

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

> Johan Herland <johan@herland.net> writes:
>
>> I'm torn about the new remove_everything_inside_dir(). Obviously it's a
>> copy-paste-modify of dir.c:remove_dir_recursively(), and could instead be
>> implemented by adding an extra flag to remove_dir_recursively(). However,
>> adding a "#define REMOVE_DIR_CONTENTS_BUT_NOT_DIR_ITSELF 04" seemed even
>> uglier to me...
>
> Hmm, what ugliness am I missing when viewing the attached patch?  It looks
> simple and straightforward enough, at least to me.
>
>  dir.c |   14 ++++++++++----
>  dir.h |    1 +
>  2 files changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/dir.c b/dir.c
> index 0a78d00..6432728 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -1178,6 +1178,7 @@ int remove_dir_recursively(struct strbuf *path, int flag)
>  	struct dirent *e;
>  	int ret = 0, original_len = path->len, len;
>  	int only_empty = (flag & REMOVE_DIR_EMPTY_ONLY);
> +	int keep_toplevel = (flag & REMOVE_DIR_KEEP_TOPLEVEL);
>  	unsigned char submodule_head[20];
>  
>  	if ((flag & REMOVE_DIR_KEEP_NESTED_GIT) &&
> @@ -1185,9 +1186,14 @@ int remove_dir_recursively(struct strbuf *path, int flag)
>  		/* Do not descend and nuke a nested git work tree. */
>  		return 0;
>  
> +	flag &= ~REMOVE_DIR_KEEP_TOPLEVEL;

Nit. This needs to drop REMOVE_DIR_KEEP_NESTED_GIT as well in order to
preserve the current behaviour.

I actually suspect that the passing of "only_empty" in the original may be
a bug in a0f4afb (clean: require double -f options to nuke nested git
repository and work tree, 2009-06-30), and this patch might be a fix to
the bug, but I didn't think things through, and it is getting late, so...

>  	dir = opendir(path->buf);
> -	if (!dir)
> -		return rmdir(path->buf);
> +	if (!dir) {
> +		if (!keep_toplevel)
> +			return rmdir(path->buf);
> +		else
> +			return -1;
> +	}
>  	if (path->buf[original_len - 1] != '/')
>  		strbuf_addch(path, '/');
>  
> @@ -1202,7 +1208,7 @@ int remove_dir_recursively(struct strbuf *path, int flag)
>  		if (lstat(path->buf, &st))
>  			; /* fall thru */
>  		else if (S_ISDIR(st.st_mode)) {
> -			if (!remove_dir_recursively(path, only_empty))
> +			if (!remove_dir_recursively(path, flag))
>  				continue; /* happy */
>  		} else if (!only_empty && !unlink(path->buf))
>  			continue; /* happy, too */

Re: [PATCH 3/2] notes-merge: Don't remove .git/NOTES_MERGE_WORKTREE; it may be the user's cwd

[Johan Herland]

On Thu, Mar 15, 2012 at 08:16, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>> Johan Herland <johan@herland.net> writes:
>>> I'm torn about the new remove_everything_inside_dir(). Obviously it's a
>>> copy-paste-modify of dir.c:remove_dir_recursively(), and could instead be
>>> implemented by adding an extra flag to remove_dir_recursively(). However,
>>> adding a "#define REMOVE_DIR_CONTENTS_BUT_NOT_DIR_ITSELF 04" seemed even
>>> uglier to me...
>>
>> Hmm, what ugliness am I missing when viewing the attached patch?  It looks
>> simple and straightforward enough, at least to me.

Agreed, you found a much more palatable name than I did. The patch
below looks good to me, and should become patch #3 in this series,
with my "3/2" patch being adjusted accordingly and becoming patch #4.
Do you want me to send the whole series again, or is it easier for you
to simply fix it up yourself?

>>  dir.c |   14 ++++++++++----
>>  dir.h |    1 +
>>  2 files changed, 11 insertions(+), 4 deletions(-)
>>
>> diff --git a/dir.c b/dir.c
>> index 0a78d00..6432728 100644
>> --- a/dir.c
>> +++ b/dir.c
>> @@ -1178,6 +1178,7 @@ int remove_dir_recursively(struct strbuf *path, int flag)
>>       struct dirent *e;
>>       int ret = 0, original_len = path->len, len;
>>       int only_empty = (flag & REMOVE_DIR_EMPTY_ONLY);
>> +     int keep_toplevel = (flag & REMOVE_DIR_KEEP_TOPLEVEL);
>>       unsigned char submodule_head[20];
>>
>>       if ((flag & REMOVE_DIR_KEEP_NESTED_GIT) &&
>> @@ -1185,9 +1186,14 @@ int remove_dir_recursively(struct strbuf *path, int flag)
>>               /* Do not descend and nuke a nested git work tree. */
>>               return 0;
>>
>> +     flag &= ~REMOVE_DIR_KEEP_TOPLEVEL;
>
> Nit. This needs to drop REMOVE_DIR_KEEP_NESTED_GIT as well in order to
> preserve the current behaviour.
>
> I actually suspect that the passing of "only_empty" in the original may be
> a bug in a0f4afb (clean: require double -f options to nuke nested git
> repository and work tree, 2009-06-30), and this patch might be a fix to
> the bug, but I didn't think things through, and it is getting late, so...

I noticed the same while looking at this function, and I think your
analysis is correct. As it stands, REMOVE_DIR_KEEP_NESTED_GIT only
applies to .git folders located directly in the toplevel dir, and not
inside a subdirectory. That strikes me as odd given the name of the
flag.


Have fun! :)

...Johan

>>       dir = opendir(path->buf);
>> -     if (!dir)
>> -             return rmdir(path->buf);
>> +     if (!dir) {
>> +             if (!keep_toplevel)
>> +                     return rmdir(path->buf);
>> +             else
>> +                     return -1;
>> +     }
>>       if (path->buf[original_len - 1] != '/')
>>               strbuf_addch(path, '/');
>>
>> @@ -1202,7 +1208,7 @@ int remove_dir_recursively(struct strbuf *path, int flag)
>>               if (lstat(path->buf, &st))
>>                       ; /* fall thru */
>>               else if (S_ISDIR(st.st_mode)) {
>> -                     if (!remove_dir_recursively(path, only_empty))
>> +                     if (!remove_dir_recursively(path, flag))
>>                               continue; /* happy */
>>               } else if (!only_empty && !unlink(path->buf))
>>                       continue; /* happy, too */



-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re* [PATCH 3/2] notes-merge: Don't remove .git/NOTES_MERGE_WORKTREE; it may be the user's cwd

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> On Thu, Mar 15, 2012 at 08:16, Junio C Hamano <gitster@pobox.com> wrote:
> ...
>> I actually suspect that the passing of "only_empty" in the original may be
>> a bug in a0f4afb (clean: require double -f options to nuke nested git
>> repository and work tree, 2009-06-30), and this patch might be a fix to
>> the bug, but I didn't think things through, and it is getting late, so...
>
> I noticed the same while looking at this function, and I think your
> analysis is correct. As it stands, REMOVE_DIR_KEEP_NESTED_GIT only
> applies to .git folders located directly in the toplevel dir, and not
> inside a subdirectory. That strikes me as odd given the name of the
> flag.

This ended up to be totally unrelated to what you wanted to achieve, but
here is a potential fix. I only tested this by running the usual tests and
additional tests in this patch; we know the coverage of "git clean" is
spotty and its implementation is lower quality than others, so please take
it with a large grain of salt.

The basic idea is to report from the lower level of recursion if it
decided to leave something in the directory back to the upper level,
and have the upper level refrain from removing itself (and when this
happens, that upper level in turn reports that it kept the directory
it is in charge of to its own caller), to avoid returning -1 from the
remove_dir_recursively() to the caller when we deliberately not run
rmdir() on a directory to prevent "git clean" from issuing a warning()
or exiting with an error.

-- >8 --
Subject: clean: preserve nested git worktree in subdirectories

remove_dir_recursively() had a check to avoid removing the directory it
was asked to remove without recursing into it and report success when the
directory is the top level of a working tree of a nested git repository,
to protext such a repository from "clean -f" (without double -f). If a
working tree of a nested git repository is in a subdirectory of a toplevel
project, however, this protection did not apply.

Pass REMOVE_DIR_KEEP_NESTED_GIT flag down to the recursive removal
codepath, and also teach the higher level not to remove the directory it
is asked to remove, when the recursed invocation did not remove the
directory it was asked to remove due to a nested git repository, as it is
not an error to leave the parent directories of such a nested repository.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 dir.c            |   21 ++++++++++++++++-----
 t/t7300-clean.sh |   27 ++++++++++++++++++++++-----
 2 files changed, 38 insertions(+), 10 deletions(-)

diff --git a/dir.c b/dir.c
index 6432728..0e09556 100644
--- a/dir.c
+++ b/dir.c
@@ -1172,23 +1172,27 @@ int is_empty_dir(const char *path)
 	return ret;
 }
 
-int remove_dir_recursively(struct strbuf *path, int flag)
+static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up)
 {
 	DIR *dir;
 	struct dirent *e;
-	int ret = 0, original_len = path->len, len;
+	int ret = 0, original_len = path->len, len, kept_down = 0;
 	int only_empty = (flag & REMOVE_DIR_EMPTY_ONLY);
 	int keep_toplevel = (flag & REMOVE_DIR_KEEP_TOPLEVEL);
 	unsigned char submodule_head[20];
 
 	if ((flag & REMOVE_DIR_KEEP_NESTED_GIT) &&
-	    !resolve_gitlink_ref(path->buf, "HEAD", submodule_head))
+	    !resolve_gitlink_ref(path->buf, "HEAD", submodule_head)) {
 		/* Do not descend and nuke a nested git work tree. */
+		if (kept_up)
+			*kept_up = 1;
 		return 0;
+	}
 
 	flag &= ~REMOVE_DIR_KEEP_TOPLEVEL;
 	dir = opendir(path->buf);
 	if (!dir) {
+		/* an empty dir could be removed even if it is unreadble */
 		if (!keep_toplevel)
 			return rmdir(path->buf);
 		else
@@ -1208,7 +1212,7 @@ int remove_dir_recursively(struct strbuf *path, int flag)
 		if (lstat(path->buf, &st))
 			; /* fall thru */
 		else if (S_ISDIR(st.st_mode)) {
-			if (!remove_dir_recursively(path, flag))
+			if (!remove_dir_recurse(path, flag, &kept_down))
 				continue; /* happy */
 		} else if (!only_empty && !unlink(path->buf))
 			continue; /* happy, too */
@@ -1220,11 +1224,18 @@ int remove_dir_recursively(struct strbuf *path, int flag)
 	closedir(dir);
 
 	strbuf_setlen(path, original_len);
-	if (!ret && !keep_toplevel)
+	if (!ret && !keep_toplevel && !kept_down)
 		ret = rmdir(path->buf);
+	else if (kept_up)
+		*kept_up = 1;
 	return ret;
 }
 
+int remove_dir_recursively(struct strbuf *path, int flag)
+{
+	return remove_dir_recurse(path, flag, NULL);
+}
+
 void setup_standard_excludes(struct dir_struct *dir)
 {
 	const char *path;
diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh
index 800b536..ccfb54d 100755
--- a/t/t7300-clean.sh
+++ b/t/t7300-clean.sh
@@ -399,8 +399,8 @@ test_expect_success SANITY 'removal failure' '
 '
 
 test_expect_success 'nested git work tree' '
-	rm -fr foo bar &&
-	mkdir foo bar &&
+	rm -fr foo bar baz &&
+	mkdir -p foo bar baz/boo &&
 	(
 		cd foo &&
 		git init &&
@@ -412,15 +412,24 @@ test_expect_success 'nested git work tree' '
 		cd bar &&
 		>goodbye.people
 	) &&
+	(
+		cd baz/boo &&
+		git init &&
+		>deeper.world
+		git add . &&
+		git commit -a -m deeply.nested
+	) &&
 	git clean -f -d &&
 	test -f foo/.git/index &&
 	test -f foo/hello.world &&
+	test -f baz/boo/.git/index &&
+	test -f baz/boo/deeper.world &&
 	! test -d bar
 '
 
 test_expect_success 'force removal of nested git work tree' '
-	rm -fr foo bar &&
-	mkdir foo bar &&
+	rm -fr foo bar baz &&
+	mkdir -p foo bar baz/boo &&
 	(
 		cd foo &&
 		git init &&
@@ -432,9 +441,17 @@ test_expect_success 'force removal of nested git work tree' '
 		cd bar &&
 		>goodbye.people
 	) &&
+	(
+		cd baz/boo &&
+		git init &&
+		>deeper.world
+		git add . &&
+		git commit -a -m deeply.nested
+	) &&
 	git clean -f -f -d &&
 	! test -d foo &&
-	! test -d bar
+	! test -d bar &&
+	! test -d baz
 '
 
 test_expect_success 'git clean -e' '

Re: [PATCH 3/2] notes-merge: Don't remove .git/NOTES_MERGE_WORKTREE; it may be the user's cwd

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> ... The patch
> below looks good to me, and should become patch #3 in this series,
> with my "3/2" patch being adjusted accordingly and becoming patch #4.
> Do you want me to send the whole series again, or is it easier for you
> to simply fix it up yourself?

I'd rather not to do this myself, as this alleged #3 in v2 was written
merely as a response to the "refactoring dir.c:remove_dir_recursively is
ugly", without reading other parts of the patch (i.e. the logic you use to
decide when to call this function with what arguments) at all.

Thanks.

Re: [PATCH 3/2] notes-merge: Don't remove .git/NOTES_MERGE_WORKTREE; it may be the user's cwd

[Johannes Sixt]

Am 3/15/2012 0:55, schrieb Johan Herland:
> When a manual notes merge is committed or aborted, we need to remove the
> temporary worktree at .git/NOTES_MERGE_WORKTREE. However, removing the
> entire directory is not good if the user ran the 'git notes merge
> --commit/--abort' from within that directory. On Windows, the directory
> removal would simply fail, while on POSIX systems, users would suddenly
> find themselves in an invalid current directory.
> 
> Therefore, instead of deleting the entire directory, we delete everything
> _within_ the directory, and leave the (empty) directory in place.

Just a data point: With this patch, the test passes on Windows.

-- Hannes