threads / rfc / 20522

RFC v2, 4 partsSparse checkout

Subject: [RFC PATCH v2 0/4] Sparse checkout

## tl;dr

19 messages between Aug 10, 2009 and Aug 11, 2009. Diffs are folded; open one to read it.

replies: 18people: 3as markdown or json

Nguyễn Thái Ngọc Duy· Aug 10, 2009, 15:19 UTC · lore

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
Nguyễn Thái Ngọc Duy· Aug 10, 2009, 15:19 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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
Show changes to 2 files +34 −2

diff-lib.c, 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
Nguyễn Thái Ngọc Duy· Aug 10, 2009, 15:19 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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(-)
Show changes to 6 files +96 −25

Documentation/technical/api-directory-listing.txt, builtin-clean.c, builtin-ls-files.c, dir.c, t/t3001-ls-files-others-exclude.sh, t/t7300-clean.sh

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
Nguyễn Thái Ngọc Duy· Aug 10, 2009, 15:19 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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
Show changes to 4 files +193 −6

cache.h, t/t1009-read-tree-sparse.sh, unpack-trees.c, unpack-trees.h

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
Nguyễn Thái Ngọc Duy· Aug 10, 2009, 15:19 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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(-)
Show changes to 4 files +14 −3

builtin-read-tree.c, t/t1009-read-tree-sparse.sh, unpack-trees.c, unpack-trees.h

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
Johannes Schindelin· Aug 10, 2009, 16:46 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

Hi,
On Mon, 10 Aug 2009, Nguyễn Thái Ngọc Duy wrote:
Show 15 quoted lines
> 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);
Show 10 quoted lines
> 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

Nguyen Thai Ngoc Duy· Aug 11, 2009, 01:38 UTC · re: Johannes Schindelin · lore

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

2009/8/10 Johannes Schindelin <Johannes.Schindelin@gmx.de>:
Show 24 quoted lines
> 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.
Show 15 quoted lines
>> 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
Junio C Hamano· Aug 11, 2009, 05:13 UTC · re: Nguyen Thai Ngoc Duy · lore

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

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
Show 6 quoted lines
>> 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.

Johannes Schindelin· Aug 11, 2009, 06:50 UTC · re: Junio C Hamano · lore

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

Hi,
On Mon, 10 Aug 2009, Junio C Hamano wrote:
Show 8 quoted lines
> 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.
Show 6 quoted lines
> 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

Nguyen Thai Ngoc Duy· Aug 11, 2009, 07:08 UTC · re: Johannes Schindelin · lore

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

On Tue, Aug 11, 2009 at 1:50 PM, Johannes Schindelin<Johannes.Schindelin@gmx.de> wrote:

Show 12 quoted lines
>> 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
Johannes Schindelin· Aug 10, 2009, 16:41 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

Hi,
On Mon, 10 Aug 2009, Nguyễn Thái Ngọc Duy wrote:
Show 9 quoted lines
> 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

Nguyen Thai Ngoc Duy· Aug 11, 2009, 01:47 UTC · re: Johannes Schindelin · lore

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

2009/8/10 Johannes Schindelin <Johannes.Schindelin@gmx.de>:
Show 28 quoted lines
> 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
Johannes Schindelin· Aug 11, 2009, 07:02 UTC · re: Nguyen Thai Ngoc Duy · lore

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

Hi,
On Tue, 11 Aug 2009, Nguyen Thai Ngoc Duy wrote:
Show 36 quoted lines
> 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

Johannes Schindelin· Aug 10, 2009, 16:33 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

Hi,
On Mon, 10 Aug 2009, Nguyễn Thái Ngọc Duy wrote:
Show 9 quoted lines
> 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...

Show 15 quoted lines
> @@ -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?
Show 16 quoted lines
> @@ -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).

Show 25 quoted lines
>  		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

Nguyen Thai Ngoc Duy· Aug 11, 2009, 01:57 UTC · re: Johannes Schindelin · lore

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

2009/8/10 Johannes Schindelin <Johannes.Schindelin@gmx.de>:
Show 17 quoted lines
> 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.
Show 17 quoted lines
>> @@ -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.

Show 20 quoted lines
>> @@ -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.

Show 17 quoted lines
>> @@ -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
Johannes Schindelin· Aug 11, 2009, 08:12 UTC · re: Nguyen Thai Ngoc Duy · lore

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

Hi,
On Tue, 11 Aug 2009, Nguyen Thai Ngoc Duy wrote:
Show 23 quoted lines
> 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.

Show 23 quoted lines
> >> @@ -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.

Show 19 quoted lines
> >> @@ -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

Johannes Schindelin· Aug 10, 2009, 16:20 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

Hi,
On Mon, 10 Aug 2009, Nguyễn Thái Ngọc Duy wrote:
Show 21 quoted lines
> 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

Show 26 quoted lines
> 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?
Show 6 quoted lines
> +
> +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?
Show 5 quoted lines
> +test_expect_success 'diff-files does not examine assume-unchanged entries' '
> +	/usr/bin/git diff-files -- one
> +'
> +
> +test_done

Thanks, Dscho

Nguyen Thai Ngoc Duy· Aug 11, 2009, 01:34 UTC · re: Johannes Schindelin · lore

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

2009/8/10 Johannes Schindelin <Johannes.Schindelin@gmx.de>:
Show 14 quoted lines
>> +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.
Show 9 quoted lines
>
>> +
>> +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
Johannes Schindelin· Aug 11, 2009, 06:45 UTC · re: Nguyen Thai Ngoc Duy · lore

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

Hi,
On Tue, 11 Aug 2009, Nguyen Thai Ngoc Duy wrote:
Show 11 quoted lines
> >> +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

← back to recent threads