[RFC PATCH 0/5] Sparse checkout resurrection

[RFC PATCH 0/5] Sparse checkout resurrection

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

Hi,

I rip most of the code from my sparse checkout series,
leaving only basic stuff. Now there is no CE_NO_CHECKOUT bit (CE_VALID
will be used), no sparse pattterns. You will need to provide a script,
namely "git-shape-workdir", to specify how you want to shape your
workdir. That's all. No porcelain, only plumbing. Need to add some
options to read-tree and checkout in order to enable/disable this, but
I haven't got that far.

The first four patches is more or less CE_VALID fixups. Real stuff is
in the last patch. Sparse patterns may return as a separate
script. Currently no idea how the porcelain UI should be. The only
thing that is probably impossible this way is "git clone
--sparse". You may need to "clone -n" first, then apply sparse
checkout later.

Comments?

Nguyễn Thái Ngọc Duy (5):
  Prevent diff machinery from examining worktree outside sparse checkout
  grep: skip files outside sparse checkout area
  gitignore: read from index if .gitignore is not in worktree
  unpack_trees(): keep track of unmerged entries
  unpack_trees(): add support for sparse checkout

 Documentation/git-grep.txt         |    4 +-
 builtin-grep.c                     |    7 ++-
 builtin-read-tree.c                |    6 ++-
 cache.h                            |    4 +
 diff-lib.c                         |    5 +-
 diff.c                             |    4 +-
 dir.c                              |   70 ++++++++++++------
 t/t3001-ls-files-others-exclude.sh |   20 +++++
 unpack-trees.c                     |  140 ++++++++++++++++++++++++++++++++++--
 unpack-trees.h                     |    4 +
 10 files changed, 229 insertions(+), 35 deletions(-)

[PATCH 1/5] Prevent diff machinery from examining worktree outside sparse checkout

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

---
 diff-lib.c |    5 +++--
 diff.c     |    4 +++-
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/diff-lib.c b/diff-lib.c
index b7813af..4094f18 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/diff.c b/diff.c
index 91d6ea2..de1bd87 100644
--- a/diff.c
+++ b/diff.c
@@ -1810,8 +1810,10 @@ static int reuse_worktree_file(const char *name, const unsigned char *sha1, int
 
 	/*
 	 * If ce matches the file in the work tree, we can reuse it.
+	 * For sparse checkout case, ce_uptodate() may be true although
+	 * the file may or may not exist in the work tree.
 	 */
-	if (ce_uptodate(ce) ||
+	if ((ce_uptodate(ce) && !(ce->ce_flags & CE_VALID)) ||
 	    (!lstat(name, &st) && !ce_match_stat(ce, &st, 0)))
 		return 1;
 
-- 
1.6.3.2.448.gdf8b6

[PATCH 2/5] grep: skip files outside sparse checkout area

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

---
 Documentation/git-grep.txt |    4 +++-
 builtin-grep.c             |    7 ++++++-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index 8c70020..8d9cba7 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -27,7 +27,9 @@ SYNOPSIS
 DESCRIPTION
 -----------
 Look for specified patterns in the working tree files, blobs
-registered in the index file, or given tree objects.
+registered in the index file, or given tree objects. By default
+it will search in the working tree files. When in sparse checkout
+mode, it only searches checked-out files.
 
 
 OPTIONS
diff --git a/builtin-grep.c b/builtin-grep.c
index ad0e0a5..55078cb 100644
--- a/builtin-grep.c
+++ b/builtin-grep.c
@@ -455,6 +455,8 @@ static int external_grep(struct grep_opt *opt, const char **paths, int cached)
 			continue;
 		if (!pathspec_matches(paths, ce->name, opt->max_depth))
 			continue;
+		if (ce->ce_flags & CE_VALID)
+			continue;
 		name = ce->name;
 		if (name[0] == '-') {
 			int len = ce_namelen(ce);
@@ -522,8 +524,11 @@ static int grep_cache(struct grep_opt *opt, const char **paths, int cached,
 				continue;
 			hit |= grep_sha1(opt, ce->sha1, ce->name, 0);
 		}
-		else
+		else {
+			if (ce->ce_flags & CE_VALID)
+				continue;
 			hit |= grep_file(opt, ce->name);
+		}
 		if (ce_stage(ce)) {
 			do {
 				nr++;
-- 
1.6.3.2.448.gdf8b6

[PATCH 3/5] gitignore: read from index if .gitignore is not in worktree

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

this allows to use .gitignore in sparse mode, where .gitignore may or
may not be checked out (CE_VALID)
---
 dir.c                              |   70 ++++++++++++++++++++++++------------
 t/t3001-ls-files-others-exclude.sh |   20 ++++++++++
 2 files changed, 67 insertions(+), 23 deletions(-)

diff --git a/dir.c b/dir.c
index e05b850..86bc90c 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 no-checkout items */
+	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..e8b25d3 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 --no-checkout .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*
-- 
1.6.3.2.448.gdf8b6

[PATCH 4/5] unpack_trees(): keep track of unmerged entries

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

"git read-tree --reset" will currently remove all unmerged entries in
index before feeding the index to unpack_trees(). Because the lack of
unmerged entries, these entries, when read from tree, will be seen as
"new entries" by {one,two,three}way_merge().

This is fine for now. But for sparse checkout, it needs to know
whether an entry is new entry, because it will handle it different way
than already-in entry.

So the patch moves "unmerged entries removal" part into
unpack_trees(), actually unpack_callback(). The function then can turn
on o->has_unmerged flag, which can be utilized by sparse checkout.

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

diff --git a/builtin-read-tree.c b/builtin-read-tree.c
index 9c2d634..17ca265 100644
--- a/builtin-read-tree.c
+++ b/builtin-read-tree.c
@@ -113,7 +113,11 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
 	argc = parse_options(argc, argv, unused_prefix, read_tree_options,
 			     read_tree_usage, 0);
 
-	if (read_cache_unmerged() && (opts.prefix || opts.merge))
+	if (opts.reset) {
+		opts.prune_unmerged = 1;
+		read_cache();
+	}
+	else if (read_cache_unmerged() && (opts.prefix || opts.merge))
 		die("You need to resolve your current index first");
 
 	prefix_set = opts.prefix ? 1 : 0;
diff --git a/unpack-trees.c b/unpack-trees.c
index 720f7a1..c67eed8 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -289,6 +289,7 @@ static int unpack_callback(int n, unsigned long mask, unsigned long dirmask, str
 
 	/* Are we supposed to look at the index too? */
 	if (o->merge) {
+		o->has_unmerged = 0;
 		while (o->pos < o->src_index->cache_nr) {
 			struct cache_entry *ce = o->src_index->cache[o->pos];
 			int cmp = compare_entry(ce, info, p);
@@ -308,6 +309,12 @@ static int unpack_callback(int n, unsigned long mask, unsigned long dirmask, str
 						add_entry(o, ce, 0, 0);
 						return mask;
 					}
+
+					if (o->prune_unmerged) {
+						o->has_unmerged = 1;
+						/* leave src[0] as NULL and go over all other staged entries */
+						continue;
+					}
 				}
 				src[0] = ce;
 			}
diff --git a/unpack-trees.h b/unpack-trees.h
index d19df44..aa3ac17 100644
--- a/unpack-trees.h
+++ b/unpack-trees.h
@@ -26,6 +26,8 @@ struct unpack_trees_options {
 		     verbose_update,
 		     aggressive,
 		     skip_unmerged,
+		     prune_unmerged,
+		     has_unmerged,
 		     initial_checkout,
 		     diff_index_cached,
 		     gently;
-- 
1.6.3.2.448.gdf8b6

[PATCH 5/5] 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. A helper "git shape-workdir" is needed to help determine
which entry will be checked out, which will be not.

"git shape-workdir" will receive from stdin in this format

X\tpathname

where X is either
 - '!' current entry is already CE_VALID
 - 'N' current entry is "new" (it has not been in index before)
 - '-' current entry is "normal" entry

"git shape-workdir" is expected to return either "1" or "0"
immediately. "1" means the entry should be in workdir. "0" means
setting CE_VALID and get rid of it from workdir.
---
 cache.h        |    4 ++
 unpack-trees.c |  133 +++++++++++++++++++++++++++++++++++++++++++++++++++++---
 unpack-trees.h |    2 +
 3 files changed, 133 insertions(+), 6 deletions(-)

diff --git a/cache.h b/cache.h
index 1a2a3c9..eac3151 100644
--- a/cache.h
+++ b/cache.h
@@ -177,6 +177,10 @@ struct cache_entry {
 #define CE_HASHED    (0x100000)
 #define CE_UNHASHED  (0x200000)
 
+/* Only remove in work directory, not index */
+#define CE_WT_REMOVE (0x400000)
+#define CE_NEW       (0x800000)
+
 /*
  * Extended on-disk flags
  */
diff --git a/unpack-trees.c b/unpack-trees.c
index c67eed8..125a662 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,81 @@ 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 apply_narrow_spec(struct unpack_trees_options *o)
+{
+	struct index_state *index = &o->result;
+	struct child_process cp;
+	const char *argv[] = { "shape-workdir", NULL };
+	int i, ret;
+
+	memset(&cp, 0, sizeof(cp));
+	cp.in = -1;
+	cp.out = -1;
+	cp.git_cmd = 1;
+	cp.argv = argv;
+
+	ret = start_command(&cp);
+	if (ret)
+		return -1;
+
+	for (i = 0; i < index->cache_nr; i++) {
+		struct cache_entry *ce = index->cache[i];
+		int was_checkout = !(ce->ce_flags & CE_VALID);
+		char buf[PATH_MAX+3];
+		char result[2];
+
+		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;
+
+		snprintf(buf, PATH_MAX+3, "%c\t%s\n",
+			 (ce->ce_flags & CE_NEW) ? 'N' : (was_checkout ? '!' : '-'),
+			 ce->name);
+		xwrite(cp.in, buf, strlen(buf));
+		xread(cp.out, result, 2);
+
+		if (result[0] == '1')
+			ce->ce_flags &= ~CE_VALID;
+		else
+			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;
+
+	}
+	close(cp.in);
+	finish_command(&cp);
+	close(cp.out);
+	return 0;
+}
+
 static inline int call_unpack_fn(struct cache_entry **src, struct unpack_trees_options *o)
 {
 	int ret = o->fn(src, o);
@@ -423,6 +512,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 (apply_narrow_spec(o))
+		return unpack_failed(o, NULL);
+
 	o->src_index = NULL;
 	ret = check_updates(o) ? (-2) : 0;
 	if (o->dst_index)
@@ -452,8 +544,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;
 
@@ -478,7 +571,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)
@@ -586,8 +690,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;
 
@@ -667,6 +772,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)
@@ -689,10 +804,14 @@ static int merged_entry(struct cache_entry *merge, struct cache_entry *old,
 				return -1;
 			invalidate_ce_path(old, o);
 		}
+		if (old->ce_flags & CE_VALID)
+			update |= CE_VALID;
 	}
 	else {
 		if (verify_absent(merge, "overwritten", o))
 			return -1;
+		if (!o->has_unmerged)
+			update |= CE_NEW;
 		invalidate_ce_path(merge, o);
 	}
 
@@ -1017,6 +1136,8 @@ int oneway_merge(struct cache_entry **src, struct unpack_trees_options *o)
 			    ie_match_stat(o->src_index, old, &st, CE_MATCH_IGNORE_VALID))
 				update |= CE_UPDATE;
 		}
+		if (old->ce_flags & CE_VALID)
+			update |= CE_VALID;
 		add_entry(o, old, update, 0);
 		return 0;
 	}
diff --git a/unpack-trees.h b/unpack-trees.h
index aa3ac17..8838517 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.2.448.gdf8b6

Re: [PATCH 5/5] unpack_trees(): add support for sparse checkout

[Jakub Narebski]

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

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

Wouldn't "git update-index --index-info" (perhaps extended) be enough?

> 
> "git shape-workdir" will receive from stdin in this format
> 
> X\tpathname
> 
> where X is either
>  - '!' current entry is already CE_VALID
>  - 'N' current entry is "new" (it has not been in index before)
>  - '-' current entry is "normal" entry
> 
> "git shape-workdir" is expected to return either "1" or "0"
> immediately. "1" means the entry should be in workdir. "0" means
> setting CE_VALID and get rid of it from workdir.

Just in case it would be better IMVHO if it returned "1\tpathname" or
"2\tpathname".  By the way, is 'pathname' quoted if necessary, and
does git-shape-workdir support -z/--null option?

Signoff (also in some other patches in this series)?

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: [PATCH 5/5] unpack_trees(): add support for sparse checkout

[Nguyen Thai Ngoc Duy]

2009/7/29 Jakub Narebski <jnareb@gmail.com>:
> Nguyễn Thái Ngọc Duy    <pclouds@gmail.com> writes:
>
>> This patch teaches unpack_trees() to checkout/remove entries
>> on working directories appropriately when sparse checkout area is
>> changed. A helper "git shape-workdir" is needed to help determine
>> which entry will be checked out, which will be not.
>
> Wouldn't "git update-index --index-info" (perhaps extended) be enough?

It's a bit more complicated because the in-memory index is filled with
various information and can't just be dumped out to be manipulated
with "git update-index".

>>
>> "git shape-workdir" will receive from stdin in this format
>>
>> X\tpathname
>>
>> where X is either
>>  - '!' current entry is already CE_VALID
>>  - 'N' current entry is "new" (it has not been in index before)
>>  - '-' current entry is "normal" entry
>>
>> "git shape-workdir" is expected to return either "1" or "0"
>> immediately. "1" means the entry should be in workdir. "0" means
>> setting CE_VALID and get rid of it from workdir.
>
> Just in case it would be better IMVHO if it returned "1\tpathname" or
> "2\tpathname".  By the way, is 'pathname' quoted if necessary, and
> does git-shape-workdir support -z/--null option?

It doesn't currently. Thanks for the suggestion.

> Signoff (also in some other patches in this series)?

This series is mainly for taking input on how git-shape-workdir should
behave, what format is good... That's why I did not sign off. Anyway
the series is not really well tested (and obvious lacks tests)
-- 
Duy

Re: [PATCH 5/5] unpack_trees(): add support for sparse checkout

[Jakub Narebski]

On Thu, 30 July 2009, Nguyen Thai Ngoc Duy wrote:
> 2009/7/29 Jakub Narebski <jnareb@gmail.com>:

> > Signoff (also in some other patches in this series)?
> 
> This series is mainly for taking input on how git-shape-workdir should
> behave, what format is good... That's why I did not sign off. Anyway
> the series is not really well tested (and obvious lacks tests)

In that case it would be IMHO a good idea to mark those patches also
by using "[PATCH/RFC m/n] ..." or "[RFC/PATCH m/n] ..." in subject.

-- 
Jakub Narebski
Poland

Re: [PATCH 1/5] Prevent diff machinery from examining worktree outside sparse checkout

[Junio C Hamano]

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

> ---

Sign off?  Oh, this is an RFC... Ok...

> diff --git a/diff-lib.c b/diff-lib.c
> index b7813af..4094f18 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)) {

The big comment is about match_missing and not about cached, so the code
movement is good.  When we pass NULL idx to this function, it means that
the tree has it but the index doesn't, so it cannot possibly be marked as
"exists in the index but is not checked out".  So the hunk makes sense.

> diff --git a/diff.c b/diff.c
> index 91d6ea2..de1bd87 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -1810,8 +1810,10 @@ static int reuse_worktree_file(const char *name, const unsigned char *sha1, int
>  
>  	/*
>  	 * If ce matches the file in the work tree, we can reuse it.
> +	 * For sparse checkout case, ce_uptodate() may be true although
> +	 * the file may or may not exist in the work tree.
>  	 */
> -	if (ce_uptodate(ce) ||
> +	if ((ce_uptodate(ce) && !(ce->ce_flags & CE_VALID)) ||
>  	    (!lstat(name, &st) && !ce_match_stat(ce, &st, 0)))
>  		return 1;

Why is this hunk necessary?  A few lines above this we return 0 for any
CE_VALID cache entry.

Re: [PATCH 2/5] grep: skip files outside sparse checkout area

[Junio C Hamano]

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

> ---
>  Documentation/git-grep.txt |    4 +++-
>  builtin-grep.c             |    7 ++++++-
>  2 files changed, 9 insertions(+), 2 deletions(-)

I do not think you can call this "CE_VALID fixup".  This is a possibly
useful but nevertheless a new, different behaviour.

We probably want opt->ignoreassumeunchanged in struct grep_opt, and
control it via grep.ignoreassumeunchanged configuration, that perhaps
defaults to true (i.e. "ignore"), with a command line override.

Oh, and perhaps a shorter-and-sweeter name than "ignoreAssumeUnchanged".

> diff --git a/builtin-grep.c b/builtin-grep.c
> index ad0e0a5..55078cb 100644
> --- a/builtin-grep.c
> +++ b/builtin-grep.c
> @@ -455,6 +455,8 @@ static int external_grep(struct grep_opt *opt, const char **paths, int cached)
>  			continue;
>  		if (!pathspec_matches(paths, ce->name, opt->max_depth))
>  			continue;
> +		if (ce->ce_flags & CE_VALID)
> +			continue;

The test probably should come before pathspec_matches().

> @@ -522,8 +524,11 @@ static int grep_cache(struct grep_opt *opt, const char **paths, int cached,
>  				continue;
>  			hit |= grep_sha1(opt, ce->sha1, ce->name, 0);
>  		}
> -		else
> +		else {
> +			if (ce->ce_flags & CE_VALID)
> +				continue;
>  			hit |= grep_file(opt, ce->name);
> +		}
>  		if (ce_stage(ce)) {
>  			do {
>  				nr++;

I do not think the new code in this hunk will be reached as the other arm
of the "else" greps in the index when CE_VALID is set.

Has this series been tested or self-reviewed yet?

Re: [PATCH 3/5] gitignore: read from index if .gitignore is not in worktree

[Junio C Hamano]

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

> this allows to use .gitignore in sparse mode, where .gitignore may or
> may not be checked out (CE_VALID)

Hmm, do we have a guarantee that we have read the index when .gitignore is
used in all the codepath in the current codebase?

"git add" used to do the fill_directory() traversal before reading the
index in some cases back in v1.6.0 days for a reason.  "git add" does not
do so anymore, but I am wondering if there are others.

Re: [RFC PATCH 0/5] Sparse checkout resurrection

[skillzero]

2009/7/28 Nguyễn Thái Ngọc Duy <pclouds@gmail.com>:
> Hi,
>
> I rip most of the code from my sparse checkout series,
> leaving only basic stuff. Now there is no CE_NO_CHECKOUT bit (CE_VALID
> will be used), no sparse pattterns. You will need to provide a script,
> namely "git-shape-workdir", to specify how you want to shape your
> workdir. That's all. No porcelain, only plumbing. Need to add some
> options to read-tree and checkout in order to enable/disable this, but
> I haven't got that far.
>
> The first four patches is more or less CE_VALID fixups. Real stuff is
> in the last patch. Sparse patterns may return as a separate
> script. Currently no idea how the porcelain UI should be. The only
> thing that is probably impossible this way is "git clone
> --sparse". You may need to "clone -n" first, then apply sparse
> checkout later.
>
> Comments?

I don't know the git code enough to really comment on the details yet,
but I just wanted to say that I'd love to see sparse checkout support
in git. I tried your other patch series using a config variable
instead of the "git-shape-workdir" script and I liked it (although I
didn't thoroughly test it). It shouldn't be hard to write a
git-shape-workdir script to parse a modules file to decide which files
to include/exclude (I'm hoping to do that soon).

Judging by the surprisingly large number of people commenting in the
recent git survey that they want some form of sparse/partial checkout
support (myself included), I hope something like this patch series
gets some traction.

BTW...Is there a repository I can clone from that has these changes
and possibly future enhancements? I checked
<http://repo.or.cz/w/git/pclouds.git>, but I didn't see them.

Re: [PATCH 2/5] grep: skip files outside sparse checkout area

[Nguyen Thai Ngoc Duy]

2009/7/31 Junio C Hamano <gitster@pobox.com>:
>> @@ -522,8 +524,11 @@ static int grep_cache(struct grep_opt *opt, const char **paths, int cached,
>>                               continue;
>>                       hit |= grep_sha1(opt, ce->sha1, ce->name, 0);
>>               }
>> -             else
>> +             else {
>> +                     if (ce->ce_flags & CE_VALID)
>> +                             continue;
>>                       hit |= grep_file(opt, ce->name);
>> +             }
>>               if (ce_stage(ce)) {
>>                       do {
>>                               nr++;
>
> I do not think the new code in this hunk will be reached as the other arm
> of the "else" greps in the index when CE_VALID is set.
>
> Has this series been tested or self-reviewed yet?

Yes and no. It had been tested and self-reviewed back when it was part
of original series. There the new code was CE_NO_CHECKOUT check, not
CE_VALID. I grepped through and replaced with CE_VALID to form a new
draft in order to get comments about this direction, this time it was
not well tested because my focus was in the last patch.
-- 
Duy

Re: [RFC PATCH 0/5] Sparse checkout resurrection

[Nguyen Thai Ngoc Duy]

2009/8/1  <skillzero@gmail.com>:
> BTW...Is there a repository I can clone from that has these changes
> and possibly future enhancements? I checked
> <http://repo.or.cz/w/git/pclouds.git>, but I didn't see them.

No. This is far from usable. I think I'll make "git-shape-workdir" a
hook, put an example hook that should cover basic cases and repost.
-- 
Duy