[RFC PATCH v2 0/4] Sparse checkout

[RFC PATCH v2 0/4] Sparse checkout

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

Another RFC, which is more usable than the last series I sent out.
Compared to the last one, a few more patches have gone the way of
Dodo; 'git shape-workdir' has become a hook and should be easier
to write too. All you need is a few
"git update-index --[no-]assume-unchanged" in .git/hooks/sparse.

Nguyễn Thái Ngọc Duy (4):
  Prevent diff machinery from examining assume-unchanged entries on
    worktree
  gitignore: read from index if .gitignore is assume-unchanged
  unpack_trees(): add support for sparse checkout
  read-tree: add --no-sparse to turn off sparse hook

 Documentation/technical/api-directory-listing.txt |    3 +
 builtin-clean.c                                   |    5 +-
 builtin-ls-files.c                                |    4 +-
 builtin-read-tree.c                               |    4 +-
 cache.h                                           |    3 +
 diff-lib.c                                        |    5 +-
 dir.c                                             |   70 +++++++---
 t/t1009-read-tree-sparse.sh                       |   48 +++++++
 t/t3001-ls-files-others-exclude.sh                |   20 +++
 t/t4039-diff-assume-unchanged.sh                  |   31 ++++
 t/t7300-clean.sh                                  |   19 +++
 unpack-trees.c                                    |  154 ++++++++++++++++++++-
 unpack-trees.h                                    |    3 +
 13 files changed, 335 insertions(+), 34 deletions(-)
 create mode 100755 t/t1009-read-tree-sparse.sh
 create mode 100755 t/t4039-diff-assume-unchanged.sh

[RFC PATCH v2 1/4] Prevent diff machinery from examining assume-unchanged entries on worktree

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

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 diff-lib.c                       |    5 +++--
 t/t4039-diff-assume-unchanged.sh |   31 +++++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+), 2 deletions(-)
 create mode 100755 t/t4039-diff-assume-unchanged.sh

diff --git a/diff-lib.c b/diff-lib.c
index b7813af..f5787f6 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -159,7 +159,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 				continue;
 		}
 
-		if (ce_uptodate(ce))
+		if (ce_uptodate(ce) || (ce->ce_flags & CE_VALID))
 			continue;
 
 		changed = check_removed(ce, &st);
@@ -337,6 +337,8 @@ static void do_oneway_diff(struct unpack_trees_options *o,
 	struct rev_info *revs = o->unpack_data;
 	int match_missing, cached;
 
+	/* if the entry is not checked out, don't examine work tree */
+	cached = o->index_only || (idx && (idx->ce_flags & CE_VALID));
 	/*
 	 * Backward compatibility wart - "diff-index -m" does
 	 * not mean "do not ignore merges", but "match_missing".
@@ -344,7 +346,6 @@ static void do_oneway_diff(struct unpack_trees_options *o,
 	 * But with the revision flag parsing, that's found in
 	 * "!revs->ignore_merges".
 	 */
-	cached = o->index_only;
 	match_missing = !revs->ignore_merges;
 
 	if (cached && idx && ce_stage(idx)) {
diff --git a/t/t4039-diff-assume-unchanged.sh b/t/t4039-diff-assume-unchanged.sh
new file mode 100755
index 0000000..d0e46a7
--- /dev/null
+++ b/t/t4039-diff-assume-unchanged.sh
@@ -0,0 +1,31 @@
+#!/bin/sh
+
+test_description='diff with assume-unchanged entries'
+
+. ./test-lib.sh
+
+# external diff has been tested in t4020-diff-external.sh
+
+test_expect_success 'setup' '
+	echo zero > zero &&
+	git add zero &&
+	git commit -m zero &&
+	echo one > one &&
+	echo two > two &&
+	git add one two &&
+	git commit -m onetwo &&
+	git update-index --assume-unchanged one &&
+	echo borked >> one &&
+	test "$(git ls-files -v one)" = "h one"
+'
+
+test_expect_success 'diff-index does not examine assume-unchanged entries' '
+	git diff-index HEAD^ -- one | grep -q 5626abf0f72e58d7a153368ba57db4c673c0e171 
+'
+
+# TODO ced_uptodate()
+test_expect_success 'diff-files does not examine assume-unchanged entries' '
+	/usr/bin/git diff-files -- one
+'
+
+test_done
-- 
1.6.3.GIT

[RFC PATCH v2 2/4] gitignore: read from index if .gitignore is assume-unchanged

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

In sparse checkout mode (aka CE_VALID or assume-unchanged) some files
may be missing from working directory. If some of those files are
.gitignore, it will affect how git excludes files.

Because those files are by definition "assume unchanged" we can
instead read them from index. This adds index as a prerequisite for
directory listing. At the moment directory listing is used by "git
clean", "git add", "git ls-files" and "git status"/"git commit" and
unpack_trees()-related commands.  These commands have been
checked/modified to populate index before doing directory listing.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/technical/api-directory-listing.txt |    3 +
 builtin-clean.c                                   |    5 +-
 builtin-ls-files.c                                |    4 +-
 dir.c                                             |   70 ++++++++++++++-------
 t/t3001-ls-files-others-exclude.sh                |   20 ++++++
 t/t7300-clean.sh                                  |   19 ++++++
 6 files changed, 96 insertions(+), 25 deletions(-)

diff --git a/Documentation/technical/api-directory-listing.txt b/Documentation/technical/api-directory-listing.txt
index 5bbd18f..7d0e282 100644
--- a/Documentation/technical/api-directory-listing.txt
+++ b/Documentation/technical/api-directory-listing.txt
@@ -58,6 +58,9 @@ The result of the enumeration is left in these fields::
 Calling sequence
 ----------------
 
+* Ensure the_index is populated as it may have CE_VALID entries that
+  affect directory listing.
+
 * Prepare `struct dir_struct dir` and clear it with `memset(&dir, 0,
   sizeof(dir))`.
 
diff --git a/builtin-clean.c b/builtin-clean.c
index 2d8c735..d917472 100644
--- a/builtin-clean.c
+++ b/builtin-clean.c
@@ -71,8 +71,11 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 
 	dir.flags |= DIR_SHOW_OTHER_DIRECTORIES;
 
-	if (!ignored)
+	if (!ignored) {
+		if (read_cache() < 0)
+			die("index file corrupt");
 		setup_standard_excludes(&dir);
+	}
 
 	pathspec = get_pathspec(prefix, argv);
 	read_cache();
diff --git a/builtin-ls-files.c b/builtin-ls-files.c
index f473220..d1a23c4 100644
--- a/builtin-ls-files.c
+++ b/builtin-ls-files.c
@@ -481,6 +481,9 @@ int cmd_ls_files(int argc, const char **argv, const char *prefix)
 		prefix_offset = strlen(prefix);
 	git_config(git_default_config, NULL);
 
+	if (read_cache() < 0)
+		die("index file corrupt");
+
 	argc = parse_options(argc, argv, prefix, builtin_ls_files_options,
 			ls_files_usage, 0);
 	if (show_tag || show_valid_bit) {
@@ -508,7 +511,6 @@ int cmd_ls_files(int argc, const char **argv, const char *prefix)
 	pathspec = get_pathspec(prefix, argv);
 
 	/* be nice with submodule paths ending in a slash */
-	read_cache();
 	if (pathspec)
 		strip_trailing_slash_from_submodules();
 
diff --git a/dir.c b/dir.c
index e05b850..e55344f 100644
--- a/dir.c
+++ b/dir.c
@@ -200,11 +200,36 @@ void add_exclude(const char *string, const char *base,
 	which->excludes[which->nr++] = x;
 }
 
+static void *read_index_data(const char *path, size_t *size)
+{
+	int pos, len;
+	unsigned long sz;
+	enum object_type type;
+	void *data;
+	struct index_state *istate = &the_index;
+
+	len = strlen(path);
+	pos = index_name_pos(istate, path, len);
+	if (pos < 0)
+		return NULL;
+	/* only applies to CE_VALID entries */
+	if (!(istate->cache[pos]->ce_flags & CE_VALID))
+		return NULL;
+	data = read_sha1_file(istate->cache[pos]->sha1, &type, &sz);
+	if (!data || type != OBJ_BLOB) {
+		free(data);
+		return NULL;
+	}
+	*size = xsize_t(sz);
+	return data;
+}
+
 static int add_excludes_from_file_1(const char *fname,
 				    const char *base,
 				    int baselen,
 				    char **buf_p,
-				    struct exclude_list *which)
+				    struct exclude_list *which,
+				    int check_index)
 {
 	struct stat st;
 	int fd, i;
@@ -212,27 +237,31 @@ static int add_excludes_from_file_1(const char *fname,
 	char *buf, *entry;
 
 	fd = open(fname, O_RDONLY);
-	if (fd < 0 || fstat(fd, &st) < 0)
-		goto err;
-	size = xsize_t(st.st_size);
-	if (size == 0) {
-		close(fd);
-		return 0;
+	if (fd < 0 || fstat(fd, &st) < 0) {
+		if (0 <= fd)
+			close(fd);
+		if (!check_index || (buf = read_index_data(fname, &size)) == NULL)
+			return -1;
 	}
-	buf = xmalloc(size+1);
-	if (read_in_full(fd, buf, size) != size)
-	{
-		free(buf);
-		goto err;
+	else {
+		size = xsize_t(st.st_size);
+		if (size == 0) {
+			close(fd);
+			return 0;
+		}
+		buf = xmalloc(size);
+		if (read_in_full(fd, buf, size) != size) {
+			close(fd);
+			return -1;
+		}
+		close(fd);
 	}
-	close(fd);
 
 	if (buf_p)
 		*buf_p = buf;
-	buf[size++] = '\n';
 	entry = buf;
-	for (i = 0; i < size; i++) {
-		if (buf[i] == '\n') {
+	for (i = 0; i <= size; i++) {
+		if (i == size || buf[i] == '\n') {
 			if (entry != buf + i && entry[0] != '#') {
 				buf[i - (i && buf[i-1] == '\r')] = 0;
 				add_exclude(entry, base, baselen, which);
@@ -241,17 +270,12 @@ static int add_excludes_from_file_1(const char *fname,
 		}
 	}
 	return 0;
-
- err:
-	if (0 <= fd)
-		close(fd);
-	return -1;
 }
 
 void add_excludes_from_file(struct dir_struct *dir, const char *fname)
 {
 	if (add_excludes_from_file_1(fname, "", 0, NULL,
-				     &dir->exclude_list[EXC_FILE]) < 0)
+				     &dir->exclude_list[EXC_FILE], 0) < 0)
 		die("cannot use %s as an exclude file", fname);
 }
 
@@ -302,7 +326,7 @@ static void prep_exclude(struct dir_struct *dir, const char *base, int baselen)
 		strcpy(dir->basebuf + stk->baselen, dir->exclude_per_dir);
 		add_excludes_from_file_1(dir->basebuf,
 					 dir->basebuf, stk->baselen,
-					 &stk->filebuf, el);
+					 &stk->filebuf, el, 1);
 		dir->exclude_stack = stk;
 		current = stk->baselen;
 	}
diff --git a/t/t3001-ls-files-others-exclude.sh b/t/t3001-ls-files-others-exclude.sh
index c65bca8..88b69bc 100755
--- a/t/t3001-ls-files-others-exclude.sh
+++ b/t/t3001-ls-files-others-exclude.sh
@@ -85,6 +85,26 @@ test_expect_success \
        >output &&
      test_cmp expect output'
 
+test_expect_success 'setup sparse gitignore' '
+	git add .gitignore one/.gitignore one/two/.gitignore &&
+	git update-index --assume-unchanged .gitignore one/.gitignore one/two/.gitignore &&
+	rm .gitignore one/.gitignore one/two/.gitignore
+'
+
+test_expect_success \
+    'git ls-files --others with various exclude options.' \
+    'git ls-files --others \
+       --exclude=\*.6 \
+       --exclude-per-directory=.gitignore \
+       --exclude-from=.git/ignore \
+       >output &&
+     test_cmp expect output'
+
+test_expect_success 'restore gitignore' '
+	git checkout .gitignore one/.gitignore one/two/.gitignore &&
+	rm .git/index
+'
+
 cat > excludes-file <<\EOF
 *.[1-8]
 e*
diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh
index 929d5d4..4886d5f 100755
--- a/t/t7300-clean.sh
+++ b/t/t7300-clean.sh
@@ -22,6 +22,25 @@ test_expect_success 'setup' '
 
 '
 
+test_expect_success 'git clean with assume-unchanged .gitignore' '
+	git update-index --assume-unchanged .gitignore &&
+	rm .gitignore &&
+	mkdir -p build docs &&
+	touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
+	git clean &&
+	test -f Makefile &&
+	test -f README &&
+	test -f src/part1.c &&
+	test -f src/part2.c &&
+	test ! -f a.out &&
+	test ! -f src/part3.c &&
+	test -f docs/manual.txt &&
+	test -f obj.o &&
+	test -f build/lib.so &&
+	git update-index --no-assume-unchanged .gitignore &&
+	git checkout .gitignore
+'
+
 test_expect_success 'git clean' '
 
 	mkdir -p build docs &&
-- 
1.6.3.GIT

[RFC PATCH v2 3/4] unpack_trees(): add support for sparse checkout

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

This patch teaches unpack_trees() to checkout/remove entries on
working directories appropriately when sparse checkout area is
changed. Hook "sparse" is needed to help determine which entry will be
checked out, which will not be.

When the hook is run, it is prepared with a pseudo index. The hook then
can use "git update-index --[no-]assume-unchanged" to manipulate the index.
It should not do anything else on the index. Assume unchanged information
from the index will be used to shape working directory.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 cache.h                     |    3 +
 t/t1009-read-tree-sparse.sh |   42 ++++++++++++
 unpack-trees.c              |  152 +++++++++++++++++++++++++++++++++++++++++--
 unpack-trees.h              |    2 +
 4 files changed, 193 insertions(+), 6 deletions(-)
 create mode 100755 t/t1009-read-tree-sparse.sh

diff --git a/cache.h b/cache.h
index 1a2a3c9..dfad54a 100644
--- a/cache.h
+++ b/cache.h
@@ -177,6 +177,9 @@ struct cache_entry {
 #define CE_HASHED    (0x100000)
 #define CE_UNHASHED  (0x200000)
 
+/* Only remove in work directory, not index */
+#define CE_WT_REMOVE (0x400000)
+
 /*
  * Extended on-disk flags
  */
diff --git a/t/t1009-read-tree-sparse.sh b/t/t1009-read-tree-sparse.sh
new file mode 100755
index 0000000..b613a89
--- /dev/null
+++ b/t/t1009-read-tree-sparse.sh
@@ -0,0 +1,42 @@
+#!/bin/sh
+
+test_description='sparse hook tests'
+
+. ./test-lib.sh
+
+make_hook() {
+	echo "#!/bin/sh" > .git/hooks/sparse &&
+	echo "$1" >> .git/hooks/sparse &&
+	chmod u+x .git/hooks/sparse
+}
+
+test_expect_success setup '
+	echo one > one &&
+	echo two > two &&
+	git add one two &&
+	git commit -m onetwo &&
+	echo three > three &&
+	git add three &&
+	git commit -m three
+'
+
+mkdir .git/hooks
+
+test_expect_success 'failed hook' '
+	make_hook "exit 1" &&
+	test_must_fail git read-tree -u -m HEAD
+'
+
+test_expect_success 'remove one' '
+	make_hook "git update-index --assume-unchanged one"
+	git read-tree -u -m HEAD &&
+	test ! -f one
+'
+
+test_expect_success 're-add one' '
+	make_hook "git update-index --no-assume-unchanged one" &&
+	git read-tree -u -m HEAD &&
+	test -f one
+'
+
+test_done
diff --git a/unpack-trees.c b/unpack-trees.c
index 720f7a1..f407bf5 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -8,6 +8,7 @@
 #include "progress.h"
 #include "refs.h"
 #include "attr.h"
+#include "run-command.h"
 
 /*
  * Error messages expected by scripts out of plumbing commands such as
@@ -32,6 +33,12 @@ static struct unpack_trees_error_msgs unpack_plumbing_errors = {
 
 	/* bind_overlap */
 	"Entry '%s' overlaps with '%s'.  Cannot bind.",
+
+	/* sparse_not_uptodate_file */
+	"Entry '%s' not uptodate. Cannot update sparse checkout.",
+
+	/* would_lose_orphaned */
+	"Orphaned working tree file '%s' would be %s by sparse checkout update.",
 };
 
 #define ERRORMSG(o,fld) \
@@ -78,7 +85,7 @@ static int check_updates(struct unpack_trees_options *o)
 	if (o->update && o->verbose_update) {
 		for (total = cnt = 0; cnt < index->cache_nr; cnt++) {
 			struct cache_entry *ce = index->cache[cnt];
-			if (ce->ce_flags & (CE_UPDATE | CE_REMOVE))
+			if (ce->ce_flags & (CE_UPDATE | CE_REMOVE | CE_WT_REMOVE))
 				total++;
 		}
 
@@ -92,6 +99,13 @@ static int check_updates(struct unpack_trees_options *o)
 	for (i = 0; i < index->cache_nr; i++) {
 		struct cache_entry *ce = index->cache[i];
 
+		if (ce->ce_flags & CE_WT_REMOVE) {
+			display_progress(progress, ++cnt);
+			if (o->update)
+				unlink_entry(ce);
+			continue;
+		}
+
 		if (ce->ce_flags & CE_REMOVE) {
 			display_progress(progress, ++cnt);
 			if (o->update)
@@ -118,6 +132,106 @@ static int check_updates(struct unpack_trees_options *o)
 	return errs != 0;
 }
 
+static int verify_uptodate_sparse(struct cache_entry *ce, struct unpack_trees_options *o);
+static int verify_absent_sparse(struct cache_entry *ce, const char *action, struct unpack_trees_options *o);
+static int run_sparse_hook(struct unpack_trees_options *o)
+{
+	struct index_state *index = &o->result;
+	struct index_state sparse_index = *index;
+	struct cache_entry *ce, *sparse_ce;
+	char sparse_index_file[PATH_MAX];
+	char sparse_index_env[PATH_MAX];
+	const char *argv[2], *env[2];
+	struct child_process cp;
+	int fd, i, j;
+
+	if (access(git_path("hooks/sparse"), X_OK) < 0)
+		return 0;
+
+	strcpy(sparse_index_file, git_path("sparse"));
+	fd = open(sparse_index_file, O_WRONLY | O_CREAT, 0600);
+	if (fd < 0) {
+		error("Unable to open %s for writing", sparse_index_file);
+		return -1;
+	}
+	/* FIXME: write_index may change something */
+	if (write_index(&sparse_index, fd)) {
+		error("Unable to write index to %s", sparse_index_file);
+		close(fd);
+		return -1;
+	}
+	close(fd);
+
+	memset(&cp, 0, sizeof(cp));
+	argv[0] = git_path("hooks/sparse");
+	argv[1] = NULL;
+	cp.argv = argv;
+	cp.no_stdin = 1;
+	cp.stdout_to_stderr = 1;
+	snprintf(sparse_index_env, sizeof(sparse_index_env), "GIT_INDEX_FILE=%s", sparse_index_file);
+	env[0] = sparse_index_env;
+	env[1] = NULL;
+	cp.env = env;
+	if (run_command(&cp)) {
+		error("Failed to run hook 'sparse'");
+		unlink(sparse_index_file);
+		return -1;
+	}
+
+	discard_index(&sparse_index);
+	read_index_from(&sparse_index, sparse_index_file);
+	unlink(sparse_index_file);
+
+	ce = index->cache[0];
+	sparse_ce = sparse_index.cache[0];
+	for (i = j = 0; i < index->cache_nr; i++, ce++) {
+		int was_checkout = !(ce->ce_flags & CE_VALID);
+
+		if (ce_stage(ce))
+			continue;
+
+		/*
+		 * We only care about files getting into the checkout area
+		 * If merge strategies want to remove some, go ahead
+		 */
+		if (ce->ce_flags & CE_REMOVE)
+			continue;
+
+		while (j < sparse_index.cache_nr &&
+		       cache_name_compare(sparse_ce->name, sparse_ce->ce_flags, ce->name, ce->ce_flags) < 0) {
+			sparse_ce++;
+			j++;
+		}
+		if (j < sparse_index.cache_nr &&
+		    !cache_name_compare(sparse_ce->name, sparse_ce->ce_flags, ce->name, ce->ce_flags))
+			ce->ce_flags = (ce->ce_flags & ~CE_VALID) | (sparse_ce->ce_flags & CE_VALID);
+
+		/* Update worktree, add/remove entries if needed */
+
+		if (was_checkout && ce->ce_flags & CE_VALID) {
+			/*
+			 * If CE_UPDATE is set, verify_uptodate() must be called already
+			 * also stat info may have lost after merged_entry() so calling
+			 * verify_uptodate() again may fail
+			 */
+			if (!(ce->ce_flags & CE_UPDATE) && verify_uptodate_sparse(ce, o))
+				return -1;
+			ce->ce_flags |= CE_WT_REMOVE;
+		}
+		if (!was_checkout && !(ce->ce_flags & CE_VALID)) {
+			if (verify_absent_sparse(ce, "overwritten", o))
+				return -1;
+			ce->ce_flags |= CE_UPDATE;
+		}
+
+		/* merge strategies may set CE_UPDATE outside checkout area */
+		if (ce->ce_flags & CE_VALID)
+			ce->ce_flags &= ~CE_UPDATE;
+
+	}
+	return 0;
+}
+
 static inline int call_unpack_fn(struct cache_entry **src, struct unpack_trees_options *o)
 {
 	int ret = o->fn(src, o);
@@ -416,6 +530,9 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 	if (o->trivial_merges_only && o->nontrivial_merge)
 		return unpack_failed(o, "Merge requires file-level merging");
 
+	if (run_sparse_hook(o))
+		return unpack_failed(o, NULL);
+
 	o->src_index = NULL;
 	ret = check_updates(o) ? (-2) : 0;
 	if (o->dst_index)
@@ -445,8 +562,9 @@ static int same(struct cache_entry *a, struct cache_entry *b)
  * When a CE gets turned into an unmerged entry, we
  * want it to be up-to-date
  */
-static int verify_uptodate(struct cache_entry *ce,
-		struct unpack_trees_options *o)
+static int verify_uptodate_generic(struct cache_entry *ce,
+				   struct unpack_trees_options *o,
+				   const char *error_msg)
 {
 	struct stat st;
 
@@ -471,7 +589,18 @@ static int verify_uptodate(struct cache_entry *ce,
 	if (errno == ENOENT)
 		return 0;
 	return o->gently ? -1 :
-		error(ERRORMSG(o, not_uptodate_file), ce->name);
+		error(error_msg, ce->name);
+}
+
+static int verify_uptodate(struct cache_entry *ce,
+			   struct unpack_trees_options *o)
+{
+	return verify_uptodate_generic(ce, o, ERRORMSG(o, not_uptodate_file));
+}
+static int verify_uptodate_sparse(struct cache_entry *ce,
+				  struct unpack_trees_options *o)
+{
+	return verify_uptodate_generic(ce, o, ERRORMSG(o, sparse_not_uptodate_file));
 }
 
 static void invalidate_ce_path(struct cache_entry *ce, struct unpack_trees_options *o)
@@ -579,8 +708,9 @@ static int icase_exists(struct unpack_trees_options *o, struct cache_entry *dst,
  * We do not want to remove or overwrite a working tree file that
  * is not tracked, unless it is ignored.
  */
-static int verify_absent(struct cache_entry *ce, const char *action,
-			 struct unpack_trees_options *o)
+static int verify_absent_generic(struct cache_entry *ce, const char *action,
+				 struct unpack_trees_options *o,
+				 const char *error_msg)
 {
 	struct stat st;
 
@@ -660,6 +790,16 @@ static int verify_absent(struct cache_entry *ce, const char *action,
 	}
 	return 0;
 }
+static int verify_absent(struct cache_entry *ce, const char *action,
+			 struct unpack_trees_options *o)
+{
+	return verify_absent_generic(ce, action, o, ERRORMSG(o, would_lose_untracked));
+}
+static int verify_absent_sparse(struct cache_entry *ce, const char *action,
+			 struct unpack_trees_options *o)
+{
+	return verify_absent_generic(ce, action, o, ERRORMSG(o, would_lose_orphaned));
+}
 
 static int merged_entry(struct cache_entry *merge, struct cache_entry *old,
 		struct unpack_trees_options *o)
diff --git a/unpack-trees.h b/unpack-trees.h
index d19df44..ad21823 100644
--- a/unpack-trees.h
+++ b/unpack-trees.h
@@ -14,6 +14,8 @@ struct unpack_trees_error_msgs {
 	const char *not_uptodate_dir;
 	const char *would_lose_untracked;
 	const char *bind_overlap;
+	const char *sparse_not_uptodate_file;
+	const char *would_lose_orphaned;
 };
 
 struct unpack_trees_options {
-- 
1.6.3.GIT

[RFC PATCH v2 4/4] read-tree: add --no-sparse to turn off sparse hook

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

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin-read-tree.c         |    4 +++-
 t/t1009-read-tree-sparse.sh |    6 ++++++
 unpack-trees.c              |    6 ++++--
 unpack-trees.h              |    1 +
 4 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/builtin-read-tree.c b/builtin-read-tree.c
index 9c2d634..4c1061e 100644
--- a/builtin-read-tree.c
+++ b/builtin-read-tree.c
@@ -31,7 +31,7 @@ static int list_tree(unsigned char *sha1)
 }
 
 static const char * const read_tree_usage[] = {
-	"git read-tree [[-m [--trivial] [--aggressive] | --reset | --prefix=<prefix>] [-u [--exclude-per-directory=<gitignore>] | -i]]  [--index-output=<file>] <tree-ish1> [<tree-ish2> [<tree-ish3>]]",
+	"git read-tree [[-m [--trivial] [--aggressive] | --reset | --prefix=<prefix>] [-u [--exclude-per-directory=<gitignore>] | -i]] [--no-sparse] [--index-output=<file>] <tree-ish1> [<tree-ish2> [<tree-ish3>]]",
 	NULL
 };
 
@@ -98,6 +98,8 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
 		  PARSE_OPT_NONEG, exclude_per_directory_cb },
 		OPT_SET_INT('i', NULL, &opts.index_only,
 			    "don't check the working tree after merging", 1),
+		OPT_SET_INT(0, "no-sparse", &opts.no_sparse_hook,
+			    "do not run sparse hook", 1),
 		OPT_END()
 	};
 
diff --git a/t/t1009-read-tree-sparse.sh b/t/t1009-read-tree-sparse.sh
index b613a89..18115b1 100755
--- a/t/t1009-read-tree-sparse.sh
+++ b/t/t1009-read-tree-sparse.sh
@@ -39,4 +39,10 @@ test_expect_success 're-add one' '
 	test -f one
 '
 
+test_expect_success 'read-tree --no-sparse' '
+	make_hook "git update-index --assume-unchanged one"
+	git read-tree -u -m --no-sparse HEAD &&
+	test -f one
+'
+
 test_done
diff --git a/unpack-trees.c b/unpack-trees.c
index f407bf5..d087112 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -530,8 +530,10 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 	if (o->trivial_merges_only && o->nontrivial_merge)
 		return unpack_failed(o, "Merge requires file-level merging");
 
-	if (run_sparse_hook(o))
-		return unpack_failed(o, NULL);
+	if (!o->no_sparse_hook) {
+		if (run_sparse_hook(o))
+			return unpack_failed(o, NULL);
+	}
 
 	o->src_index = NULL;
 	ret = check_updates(o) ? (-2) : 0;
diff --git a/unpack-trees.h b/unpack-trees.h
index ad21823..81eb2ef 100644
--- a/unpack-trees.h
+++ b/unpack-trees.h
@@ -30,6 +30,7 @@ struct unpack_trees_options {
 		     skip_unmerged,
 		     initial_checkout,
 		     diff_index_cached,
+		     no_sparse_hook,
 		     gently;
 	const char *prefix;
 	int pos;
-- 
1.6.3.GIT

Re: [RFC PATCH v2 4/4] read-tree: add --no-sparse to turn off sparse hook

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1238 bytes --]

Hi,

On Mon, 10 Aug 2009, Nguyễn Thái Ngọc Duy wrote:

> diff --git a/unpack-trees.c b/unpack-trees.c
> index f407bf5..d087112 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -530,8 +530,10 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
>  	if (o->trivial_merges_only && o->nontrivial_merge)
>  		return unpack_failed(o, "Merge requires file-level merging");
>  
> -	if (run_sparse_hook(o))
> -		return unpack_failed(o, NULL);
> +	if (!o->no_sparse_hook) {
> +		if (run_sparse_hook(o))
> +			return unpack_failed(o, NULL);
> +	}
>  

IMHO this would read nicelier as

	if (!o->no_sparse_hook && run_sparse_hook(o))
		return unpack_failed(o, NULL);

> diff --git a/unpack-trees.h b/unpack-trees.h
> index ad21823..81eb2ef 100644
> --- a/unpack-trees.h
> +++ b/unpack-trees.h
> @@ -30,6 +30,7 @@ struct unpack_trees_options {
>  		     skip_unmerged,
>  		     initial_checkout,
>  		     diff_index_cached,
> +		     no_sparse_hook,
>  		     gently;

Hmm.  I understand that the assumption is that memset(&opts, 0, 
sizeof(opts)); should give you a sensible default, but I cannot avoid 
noticing that "no_sparse_hook = 0" is a double negation, something to be 
avoided...

Thanks,
Dscho

Re: [RFC PATCH v2 4/4] read-tree: add --no-sparse to turn off sparse hook

[Nguyen Thai Ngoc Duy]

2009/8/10 Johannes Schindelin <Johannes.Schindelin@gmx.de>:
> Hi,
>
> On Mon, 10 Aug 2009, Nguyễn Thái Ngọc Duy wrote:
>
>> diff --git a/unpack-trees.c b/unpack-trees.c
>> index f407bf5..d087112 100644
>> --- a/unpack-trees.c
>> +++ b/unpack-trees.c
>> @@ -530,8 +530,10 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
>>       if (o->trivial_merges_only && o->nontrivial_merge)
>>               return unpack_failed(o, "Merge requires file-level merging");
>>
>> -     if (run_sparse_hook(o))
>> -             return unpack_failed(o, NULL);
>> +     if (!o->no_sparse_hook) {
>> +             if (run_sparse_hook(o))
>> +                     return unpack_failed(o, NULL);
>> +     }
>>
>
> IMHO this would read nicelier as
>
>        if (!o->no_sparse_hook && run_sparse_hook(o))
>                return unpack_failed(o, NULL);

Right.

>> diff --git a/unpack-trees.h b/unpack-trees.h
>> index ad21823..81eb2ef 100644
>> --- a/unpack-trees.h
>> +++ b/unpack-trees.h
>> @@ -30,6 +30,7 @@ struct unpack_trees_options {
>>                    skip_unmerged,
>>                    initial_checkout,
>>                    diff_index_cached,
>> +                  no_sparse_hook,
>>                    gently;
>
> Hmm.  I understand that the assumption is that memset(&opts, 0,
> sizeof(opts)); should give you a sensible default, but I cannot avoid
> noticing that "no_sparse_hook = 0" is a double negation, something to be
> avoided...

skip_sparse_hook then? :-)
-- 
Duy

Re: [RFC PATCH v2 4/4] read-tree: add --no-sparse to turn off sparse hook

[Junio C Hamano]

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

>> Hmm.  I understand that the assumption is that memset(&opts, 0,
>> sizeof(opts)); should give you a sensible default, but I cannot avoid
>> noticing that "no_sparse_hook = 0" is a double negation, something to be
>> avoided...
>
> skip_sparse_hook then? :-)

Why not making the hook to be skipped by default, and pass an explicit
option to trigger the hook?

I like Dscho's other suggestion to use patterns like .gitignore instead of
using hook scripts that needs to be ported across platforms, by the way.

Re: [RFC PATCH v2 4/4] read-tree: add --no-sparse to turn off sparse hook

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1018 bytes --]

Hi,

On Mon, 10 Aug 2009, Junio C Hamano wrote:

> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
> 
> >> Hmm.  I understand that the assumption is that memset(&opts, 0,
> >> sizeof(opts)); should give you a sensible default, but I cannot avoid
> >> noticing that "no_sparse_hook = 0" is a double negation, something to be
> >> avoided...
> >
> > skip_sparse_hook then? :-)

It nicely avoids the double negation, indeed.

> Why not making the hook to be skipped by default, and pass an explicit 
> option to trigger the hook?
> 
> I like Dscho's other suggestion to use patterns like .gitignore instead 
> of using hook scripts that needs to be ported across platforms, by the 
> way.

I forgot to mention that I checked dir.c to verify that 
add_excludes_from_file_1() (which is static, so you'll either need to use 
it in the same file, or even better, export it renaming it to something 
like add_excludes_from_file_to_list()) and excluded_1() (same here) 
already do a large part of what you need.

Ciao,
Dscho

Re: [RFC PATCH v2 4/4] read-tree: add --no-sparse to turn off sparse hook

[Nguyen Thai Ngoc Duy]

On Tue, Aug 11, 2009 at 1:50 PM, Johannes
Schindelin<Johannes.Schindelin@gmx.de> wrote:
>> Why not making the hook to be skipped by default, and pass an explicit
>> option to trigger the hook?
>>
>> I like Dscho's other suggestion to use patterns like .gitignore instead
>> of using hook scripts that needs to be ported across platforms, by the
>> way.
>
> I forgot to mention that I checked dir.c to verify that
> add_excludes_from_file_1() (which is static, so you'll either need to use
> it in the same file, or even better, export it renaming it to something
> like add_excludes_from_file_to_list()) and excluded_1() (same here)
> already do a large part of what you need.

Indeed (and I have that code too). I still like the hook though
because you have more freedom in defining your worktree (and yes, less
portable). I wanted to make something generic enough that you can
build higher "strategies" on top, like .gitignore-based patterns.
Anyway I'll send out another patch for .gitignore patterns tonight and
see how it goes.
-- 
Duy

Re: [RFC PATCH v2 3/4] unpack_trees(): add support for sparse checkout

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1057 bytes --]

Hi,

On Mon, 10 Aug 2009, Nguyễn Thái Ngọc Duy wrote:

> This patch teaches unpack_trees() to checkout/remove entries on working 
> directories appropriately when sparse checkout area is changed. Hook 
> "sparse" is needed to help determine which entry will be checked out, 
> which will not be.
> 
> When the hook is run, it is prepared with a pseudo index. The hook then 
> can use "git update-index --[no-]assume-unchanged" to manipulate the 
> index. It should not do anything else on the index. Assume unchanged 
> information from the index will be used to shape working directory.

If I understand correctly, the purpose of the hook is to allow the user to 
mark something as unwanted preemptively, right?

If that is the sole reason for the hook, would it not be better to add 
support for a file .git/info/sparse which has the same syntax as 
.git/info/exclude, and which is used to determine if an 
added/modified/deleted file is supposed to be in the "sparse" area or not?

Something like

	*
	!/Documentation/

comes to mind.

Thanks,
Dscho

Re: [RFC PATCH v2 3/4] unpack_trees(): add support for sparse checkout

[Nguyen Thai Ngoc Duy]

2009/8/10 Johannes Schindelin <Johannes.Schindelin@gmx.de>:
> Hi,
>
> On Mon, 10 Aug 2009, Nguyễn Thái Ngọc Duy wrote:
>
>> This patch teaches unpack_trees() to checkout/remove entries on working
>> directories appropriately when sparse checkout area is changed. Hook
>> "sparse" is needed to help determine which entry will be checked out,
>> which will not be.
>>
>> When the hook is run, it is prepared with a pseudo index. The hook then
>> can use "git update-index --[no-]assume-unchanged" to manipulate the
>> index. It should not do anything else on the index. Assume unchanged
>> information from the index will be used to shape working directory.
>
> If I understand correctly, the purpose of the hook is to allow the user to
> mark something as unwanted preemptively, right?
>
> If that is the sole reason for the hook, would it not be better to add
> support for a file .git/info/sparse which has the same syntax as
> .git/info/exclude, and which is used to determine if an
> added/modified/deleted file is supposed to be in the "sparse" area or not?
>
> Something like
>
>        *
>        !/Documentation/
>
> comes to mind.

That was what the original series was about (although I used git
config instead of .git/info/sparse). The hook has two advantages:

 - flexibility: you can set things differently based on branch, for
example, or filter files based on certain file content.
 - less code bloat (and less intrusive too), compare ~1000 lines of
change of the original series with this series.
-- 
Duy

Re: [RFC PATCH v2 3/4] unpack_trees(): add support for sparse checkout

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2650 bytes --]

Hi,

On Tue, 11 Aug 2009, Nguyen Thai Ngoc Duy wrote:

> 2009/8/10 Johannes Schindelin <Johannes.Schindelin@gmx.de>:
>
> > On Mon, 10 Aug 2009, Nguyễn Thái Ngọc Duy wrote:
> >
> >> This patch teaches unpack_trees() to checkout/remove entries on 
> >> working directories appropriately when sparse checkout area is 
> >> changed. Hook "sparse" is needed to help determine which entry will 
> >> be checked out, which will not be.
> >>
> >> When the hook is run, it is prepared with a pseudo index. The hook 
> >> then can use "git update-index --[no-]assume-unchanged" to manipulate 
> >> the index. It should not do anything else on the index. Assume 
> >> unchanged information from the index will be used to shape working 
> >> directory.
> >
> > If I understand correctly, the purpose of the hook is to allow the 
> > user to mark something as unwanted preemptively, right?
> >
> > If that is the sole reason for the hook, would it not be better to add 
> > support for a file .git/info/sparse which has the same syntax as 
> > .git/info/exclude, and which is used to determine if an 
> > added/modified/deleted file is supposed to be in the "sparse" area or 
> > not?
> >
> > Something like
> >
> >        *
> >        !/Documentation/
> >
> > comes to mind.
> 
> That was what the original series was about (although I used git
> config instead of .git/info/sparse). The hook has two advantages:
> 
>  - flexibility: you can set things differently based on branch, for 
>    example, or filter files based on certain file content.

True, it is flexible.  The question is: is it not too flexible for its own 
good?

Having to introduce a hook means that every user either must trust another 
person and install the unmodified hook, or write the hook herself for 
every narrow work tree.

The branch use case you mentioned is maybe a good example what I mean by 
"too flexible":

- it is easy to get confused which parts are narrow or not if the hook can 
  change that depending on, say, the phase of the moon (okay, okay, 
  depending on the branch, but it is still confusing),

- the whole purpose of narrow checkout is to avoid having to check out 
  stuff that does not affect you.  Chances are that 99.99% of all "narrow" 
  users do not need to modify which slice of the files they want to see, 
  and those who do are probably better off with two work trees, methinks.

>  - less code bloat (and less intrusive too), compare ~1000 lines of 
>    change of the original series with this series.

Heh, see my other mail.  I'm sorry I forgot to mention that the similarity 
to .gitignore spares you a lot of code...

Ciao,
Dscho

Re: [RFC PATCH v2 2/4] gitignore: read from index if .gitignore is assume-unchanged

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2887 bytes --]

Hi,

On Mon, 10 Aug 2009, Nguyễn Thái Ngọc Duy wrote:

> diff --git a/dir.c b/dir.c
> index e05b850..e55344f 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -200,11 +200,36 @@ void add_exclude(const char *string, const char *base,
>  	which->excludes[which->nr++] = x;
>  }
>  
> +static void *read_index_data(const char *path, size_t *size)

How about calling it "read_assume_unchanged_from_index()" instead?  I 
suggest this because it does not read the index from the data if the path 
is not marked assume unchanged...

> @@ -212,27 +237,31 @@ static int add_excludes_from_file_1(const char *fname,
>
> [...]
>  
>  	if (buf_p)
>  		*buf_p = buf;
> -	buf[size++] = '\n';
>  	entry = buf;
> -	for (i = 0; i < size; i++) {
> -		if (buf[i] == '\n') {
> +	for (i = 0; i <= size; i++) {
> +		if (i == size || buf[i] == '\n') {
>  			if (entry != buf + i && entry[0] != '#') {
>  				buf[i - (i && buf[i-1] == '\r')] = 0;
>  				add_exclude(entry, base, baselen, which);

Should this change not rather be a separate one?

> @@ -241,17 +270,12 @@ static int add_excludes_from_file_1(const char *fname,
>  		}
>  	}
>  	return 0;
> -
> - err:
> -	if (0 <= fd)
> -		close(fd);
> -	return -1;
>  }
>  
>  void add_excludes_from_file(struct dir_struct *dir, const char *fname)
>  {
>  	if (add_excludes_from_file_1(fname, "", 0, NULL,
> -				     &dir->exclude_list[EXC_FILE]) < 0)
> +				     &dir->exclude_list[EXC_FILE], 0) < 0)

Could you mention in the commit message why this function does not want to 
check the index (I _guess_ it is because this code path only tries to read 
.git/info/exclude, but it would be better to be sure).

>  		die("cannot use %s as an exclude file", fname);
>  }
>  
> @@ -302,7 +326,7 @@ static void prep_exclude(struct dir_struct *dir, const char *base, int baselen)
>  		strcpy(dir->basebuf + stk->baselen, dir->exclude_per_dir);
>  		add_excludes_from_file_1(dir->basebuf,
>  					 dir->basebuf, stk->baselen,
> -					 &stk->filebuf, el);
> +					 &stk->filebuf, el, 1);
>  		dir->exclude_stack = stk;
>  		current = stk->baselen;
>  	}
> diff --git a/t/t3001-ls-files-others-exclude.sh b/t/t3001-ls-files-others-exclude.sh
> index c65bca8..88b69bc 100755
> --- a/t/t3001-ls-files-others-exclude.sh
> +++ b/t/t3001-ls-files-others-exclude.sh
> @@ -85,6 +85,26 @@ test_expect_success \
>         >output &&
>       test_cmp expect output'
>  
> +test_expect_success 'setup sparse gitignore' '
> +	git add .gitignore one/.gitignore one/two/.gitignore &&
> +	git update-index --assume-unchanged .gitignore one/.gitignore one/two/.gitignore &&
> +	rm .gitignore one/.gitignore one/two/.gitignore
> +'

You're probably less sloppy than me; I'd have defined a variable like 
this:

	ignores=".gitignore one/.gitignore one/two/.gitignore"

and used it for the three calls, just to make sure that I do not fsck 
anything up there due to fat fingers.

Thanks,
Dscho

Re: [RFC PATCH v2 2/4] gitignore: read from index if .gitignore is assume-unchanged

[Nguyen Thai Ngoc Duy]

2009/8/10 Johannes Schindelin <Johannes.Schindelin@gmx.de>:
> Hi,
>
> On Mon, 10 Aug 2009, Nguyễn Thái Ngọc Duy wrote:
>
>> diff --git a/dir.c b/dir.c
>> index e05b850..e55344f 100644
>> --- a/dir.c
>> +++ b/dir.c
>> @@ -200,11 +200,36 @@ void add_exclude(const char *string, const char *base,
>>       which->excludes[which->nr++] = x;
>>  }
>>
>> +static void *read_index_data(const char *path, size_t *size)
>
> How about calling it "read_assume_unchanged_from_index()" instead?  I
> suggest this because it does not read the index from the data if the path
> is not marked assume unchanged...

Agree.

>> @@ -212,27 +237,31 @@ static int add_excludes_from_file_1(const char *fname,
>>
>> [...]
>>
>>       if (buf_p)
>>               *buf_p = buf;
>> -     buf[size++] = '\n';
>>       entry = buf;
>> -     for (i = 0; i < size; i++) {
>> -             if (buf[i] == '\n') {
>> +     for (i = 0; i <= size; i++) {
>> +             if (i == size || buf[i] == '\n') {
>>                       if (entry != buf + i && entry[0] != '#') {
>>                               buf[i - (i && buf[i-1] == '\r')] = 0;
>>                               add_exclude(entry, base, baselen, which);
>
> Should this change not rather be a separate one?

You meant a separate patch? It is tied to this patch, because if bus
is read from read_index_data, it does not have extra space for '\n' at
the end.

>> @@ -241,17 +270,12 @@ static int add_excludes_from_file_1(const char *fname,
>>               }
>>       }
>>       return 0;
>> -
>> - err:
>> -     if (0 <= fd)
>> -             close(fd);
>> -     return -1;
>>  }
>>
>>  void add_excludes_from_file(struct dir_struct *dir, const char *fname)
>>  {
>>       if (add_excludes_from_file_1(fname, "", 0, NULL,
>> -                                  &dir->exclude_list[EXC_FILE]) < 0)
>> +                                  &dir->exclude_list[EXC_FILE], 0) < 0)
>
> Could you mention in the commit message why this function does not want to
> check the index (I _guess_ it is because this code path only tries to read
> .git/info/exclude, but it would be better to be sure).

To retain old behaviour. But I have to check its callers. Maybe we
want to check the index too.

>> @@ -85,6 +85,26 @@ test_expect_success \
>>         >output &&
>>       test_cmp expect output'
>>
>> +test_expect_success 'setup sparse gitignore' '
>> +     git add .gitignore one/.gitignore one/two/.gitignore &&
>> +     git update-index --assume-unchanged .gitignore one/.gitignore one/two/.gitignore &&
>> +     rm .gitignore one/.gitignore one/two/.gitignore
>> +'
>
> You're probably less sloppy than me; I'd have defined a variable like
> this:
>
>        ignores=".gitignore one/.gitignore one/two/.gitignore"
>
> and used it for the three calls, just to make sure that I do not fsck
> anything up there due to fat fingers.

I have slim ones :-) But "git add $ignores && git update-index
$ignores && rm $ignores" is easier to read.
-- 
Duy

Re: [RFC PATCH v2 2/4] gitignore: read from index if .gitignore is assume-unchanged

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3211 bytes --]

Hi,

On Tue, 11 Aug 2009, Nguyen Thai Ngoc Duy wrote:

> 2009/8/10 Johannes Schindelin <Johannes.Schindelin@gmx.de>:
>
> > On Mon, 10 Aug 2009, Nguyễn Thái Ngọc Duy wrote:
> >
> >> @@ -212,27 +237,31 @@ static int add_excludes_from_file_1(const char *fname,
> >>
> >> [...]
> >>
> >>       if (buf_p)
> >>               *buf_p = buf;
> >> -     buf[size++] = '\n';
> >>       entry = buf;
> >> -     for (i = 0; i < size; i++) {
> >> -             if (buf[i] == '\n') {
> >> +     for (i = 0; i <= size; i++) {
> >> +             if (i == size || buf[i] == '\n') {
> >>                       if (entry != buf + i && entry[0] != '#') {
> >>                               buf[i - (i && buf[i-1] == '\r')] = 0;
> >>                               add_exclude(entry, base, baselen, which);
> >
> > Should this change not rather be a separate one?
> 
> You meant a separate patch?

Yes, sorry, I meant exactly that.

> It is tied to this patch, because if bus is read from read_index_data, 
> it does not have extra space for '\n' at the end.

But you could separate it out as a patch that just avoids writing to buf.  
IMHO this would make following the patch series easier.

> >> @@ -241,17 +270,12 @@ static int add_excludes_from_file_1(const char *fname,
> >>               }
> >>       }
> >>       return 0;
> >> -
> >> - err:
> >> -     if (0 <= fd)
> >> -             close(fd);
> >> -     return -1;
> >>  }
> >>
> >>  void add_excludes_from_file(struct dir_struct *dir, const char *fname)
> >>  {
> >>       if (add_excludes_from_file_1(fname, "", 0, NULL,
> >> -                                  &dir->exclude_list[EXC_FILE]) < 0)
> >> +                                  &dir->exclude_list[EXC_FILE], 0) < 0)
> >
> > Could you mention in the commit message why this function does not 
> > want to check the index (I _guess_ it is because this code path only 
> > tries to read .git/info/exclude, but it would be better to be sure).
> 
> To retain old behaviour. But I have to check its callers. Maybe we
> want to check the index too.

Yes, it would be good to illustrate in the commit message which code paths 
want to check the index, and why.

> >> @@ -85,6 +85,26 @@ test_expect_success \
> >>         >output &&
> >>       test_cmp expect output'
> >>
> >> +test_expect_success 'setup sparse gitignore' '
> >> +     git add .gitignore one/.gitignore one/two/.gitignore &&
> >> +     git update-index --assume-unchanged .gitignore one/.gitignore one/two/.gitignore &&
> >> +     rm .gitignore one/.gitignore one/two/.gitignore
> >> +'
> >
> > You're probably less sloppy than me; I'd have defined a variable like
> > this:
> >
> >        ignores=".gitignore one/.gitignore one/two/.gitignore"
> >
> > and used it for the three calls, just to make sure that I do not fsck
> > anything up there due to fat fingers.
> 
> I have slim ones :-)

Oh, don't get me wrong: my fingers are slim enough to play piano, and even 
to write pretty fast on my EeePC 701.

But darn, there are days when they feel as if they were thick like a 
brick.

Ciao,
Dscho

Re: [RFC PATCH v2 1/4] Prevent diff machinery from examining assume-unchanged entries on worktree

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2072 bytes --]

Hi,

On Mon, 10 Aug 2009, Nguyễn Thái Ngọc Duy wrote:

> diff --git a/diff-lib.c b/diff-lib.c
> index b7813af..f5787f6 100644
> --- a/diff-lib.c
> +++ b/diff-lib.c
> @@ -337,6 +337,8 @@ static void do_oneway_diff(struct unpack_trees_options *o,
>  	struct rev_info *revs = o->unpack_data;
>  	int match_missing, cached;
>  
> +	/* if the entry is not checked out, don't examine work tree */
> +	cached = o->index_only || (idx && (idx->ce_flags & CE_VALID));
>  	/*
>  	 * Backward compatibility wart - "diff-index -m" does
>  	 * not mean "do not ignore merges", but "match_missing".
> @@ -344,7 +346,6 @@ static void do_oneway_diff(struct unpack_trees_options *o,
>  	 * But with the revision flag parsing, that's found in
>  	 * "!revs->ignore_merges".
>  	 */
> -	cached = o->index_only;
>  	match_missing = !revs->ignore_merges;
>  
>  	if (cached && idx && ce_stage(idx)) {

Out of curiosity: why did that line have to move up?

Ciao,
Dscho

> diff --git a/t/t4039-diff-assume-unchanged.sh b/t/t4039-diff-assume-unchanged.sh
> new file mode 100755
> index 0000000..d0e46a7
> --- /dev/null
> +++ b/t/t4039-diff-assume-unchanged.sh
> @@ -0,0 +1,31 @@
> +#!/bin/sh
> +
> +test_description='diff with assume-unchanged entries'
> +
> +. ./test-lib.sh
> +
> +# external diff has been tested in t4020-diff-external.sh
> +
> +test_expect_success 'setup' '
> +	echo zero > zero &&
> +	git add zero &&
> +	git commit -m zero &&
> +	echo one > one &&
> +	echo two > two &&
> +	git add one two &&
> +	git commit -m onetwo &&
> +	git update-index --assume-unchanged one &&
> +	echo borked >> one &&
> +	test "$(git ls-files -v one)" = "h one"
> +'

Maybe use test_commit, to make it more readable?

> +
> +test_expect_success 'diff-index does not examine assume-unchanged entries' '
> +	git diff-index HEAD^ -- one | grep -q 5626abf0f72e58d7a153368ba57db4c673c0e171 
> +'
> +
> +# TODO ced_uptodate()

What is this about?

> +test_expect_success 'diff-files does not examine assume-unchanged entries' '
> +	/usr/bin/git diff-files -- one
> +'
> +
> +test_done

Thanks,
Dscho

Re: [RFC PATCH v2 1/4] Prevent diff machinery from examining assume-unchanged entries on worktree

[Nguyen Thai Ngoc Duy]

2009/8/10 Johannes Schindelin <Johannes.Schindelin@gmx.de>:
>> +test_expect_success 'setup' '
>> +     echo zero > zero &&
>> +     git add zero &&
>> +     git commit -m zero &&
>> +     echo one > one &&
>> +     echo two > two &&
>> +     git add one two &&
>> +     git commit -m onetwo &&
>> +     git update-index --assume-unchanged one &&
>> +     echo borked >> one &&
>> +     test "$(git ls-files -v one)" = "h one"
>> +'
>
> Maybe use test_commit, to make it more readable?

Thanks. I did not know that (or forgot already). Will use it.

>
>> +
>> +test_expect_success 'diff-index does not examine assume-unchanged entries' '
>> +     git diff-index HEAD^ -- one | grep -q 5626abf0f72e58d7a153368ba57db4c673c0e171
>> +'
>> +
>> +# TODO ced_uptodate()
>
> What is this about?

It tests "if (ce_uptodate(ce) || (ce->ce_flags & CE_VALID))" and I was
pretty sure it hit ce_uptodate() first, so the second expression was
not tested.
-- 
Duy

Re: [RFC PATCH v2 1/4] Prevent diff machinery from examining assume-unchanged entries on worktree

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 661 bytes --]

Hi,

On Tue, 11 Aug 2009, Nguyen Thai Ngoc Duy wrote:

> >> +test_expect_success 'diff-index does not examine assume-unchanged entries' '
> >> +     git diff-index HEAD^ -- one | grep -q 5626abf0f72e58d7a153368ba57db4c673c0e171
> >> +'
> >> +
> >> +# TODO ced_uptodate()
> >
> > What is this about?
> 
> It tests "if (ce_uptodate(ce) || (ce->ce_flags & CE_VALID))" and I was 
> pretty sure it hit ce_uptodate() first, so the second expression was not 
> tested.

Ah.  I was distracted by the "d" before the underscore.

ce_uptodate(ce) checks the time stamp amongst other things, right?  How 
about using test-chmtime to force an mtime mismatch?

Ciao,
Dscho