[PATCH v2 0/6] Partial clone part 1: object filtering

[PATCH v2 0/6] Partial clone part 1: object filtering

[Jeff Hostetler]

From: Jeff Hostetler <jeffhost@microsoft.com>

Here is V2 of the list-object filtering. It replaces [1]
and reflect a refactoring and simplification of the original.

After much discussion on the "list-object-filter-map" I've replaced
it with a regular oidset -- the only need for the map was to store
the first observed pathname for each blob, but that itself was of
questionable value.

I've extended oidmap and oidset to have iterators.  These 2 commits
could be pulled out and applied on their own, but for now I need
them here.

There were also several comments on the layout of the filtering
API and the layout of the filter source code.  I've restructured
the filtering routines to put them in the same source file, and
made them all static.  These are now hidden behind a "factory-like"
function with a vtable.  This greatly simplifies the code in
traverse_commit_list_filtered().

I've added "--filter-ignore-missing" parameter to rev-list and
pack-objects to ignore missing objects rather than error out.
This allows this patch series to better stand on its own eliminates
the need in part 1 for "patch 9" from V1.

This is a brute force ignore all missing objects.  Later, in part
2 or part 3 when --exclude-promisor-objects is introduced, we will
be able to ignore EXPECTED missing objects.

Finally, patch 1 in this series is the same [2] which is currently
cooking in next.

[1] https://public-inbox.org/git/20171024185332.57261-1-git@jeffhostetler.com/

[2] * jh/dir-add-exclude-from-blob (2017-10-27) 1 commit
    - dir: allow exclusions from blob in addition to file


Jeff Hostetler (6):
  dir: allow exclusions from blob in addition to file
  oidmap: add oidmap iterator methods
  oidset: add iterator methods to oidset
  list-objects: filter objects in traverse_commit_list
  rev-list: add list-objects filtering support
  pack-objects: add list-objects filtering

 Documentation/git-pack-objects.txt     |  12 +-
 Documentation/git-rev-list.txt         |   6 +-
 Documentation/rev-list-options.txt     |  34 +++
 Makefile                               |   2 +
 builtin/pack-objects.c                 |  28 ++-
 builtin/rev-list.c                     |  75 +++++-
 dir.c                                  | 132 ++++++++---
 dir.h                                  |   3 +
 list-objects-filter-options.c          | 119 ++++++++++
 list-objects-filter-options.h          |  55 +++++
 list-objects-filter.c                  | 408 +++++++++++++++++++++++++++++++++
 list-objects-filter.h                  |  84 +++++++
 list-objects.c                         |  95 ++++++--
 list-objects.h                         |   2 +-
 oidmap.h                               |  22 ++
 oidset.c                               |  10 +
 oidset.h                               |  36 +++
 t/t5317-pack-objects-filter-objects.sh | 369 +++++++++++++++++++++++++++++
 t/t6112-rev-list-filters-objects.sh    | 225 ++++++++++++++++++
 19 files changed, 1664 insertions(+), 53 deletions(-)
 create mode 100644 list-objects-filter-options.c
 create mode 100644 list-objects-filter-options.h
 create mode 100644 list-objects-filter.c
 create mode 100644 list-objects-filter.h
 create mode 100755 t/t5317-pack-objects-filter-objects.sh
 create mode 100755 t/t6112-rev-list-filters-objects.sh

-- 
2.9.3

[PATCH v2 1/6] dir: allow exclusions from blob in addition to file

[Jeff Hostetler]

From: Jeff Hostetler <jeffhost@microsoft.com>

Refactor add_excludes() to separate the reading of the
exclude file into a buffer and the parsing of the buffer
into exclude_list items.

Add add_excludes_from_blob_to_list() to allow an exclude
file be specified with an OID without assuming a local
worktree or index exists.

Refactor read_skip_worktree_file_from_index() and add
do_read_blob() to eliminate duplication of preliminary
processing of blob contents.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 dir.c | 132 ++++++++++++++++++++++++++++++++++++++++++++++++++----------------
 dir.h |   3 ++
 2 files changed, 104 insertions(+), 31 deletions(-)

diff --git a/dir.c b/dir.c
index 1d17b80..1962374 100644
--- a/dir.c
+++ b/dir.c
@@ -220,6 +220,57 @@ int within_depth(const char *name, int namelen,
 	return 1;
 }
 
+/*
+ * Read the contents of the blob with the given OID into a buffer.
+ * Append a trailing LF to the end if the last line doesn't have one.
+ *
+ * Returns:
+ *    -1 when the OID is invalid or unknown or does not refer to a blob.
+ *     0 when the blob is empty.
+ *     1 along with { data, size } of the (possibly augmented) buffer
+ *       when successful.
+ *
+ * Optionally updates the given sha1_stat with the given OID (when valid).
+ */
+static int do_read_blob(const struct object_id *oid,
+			struct sha1_stat *sha1_stat,
+			size_t *size_out,
+			char **data_out)
+{
+	enum object_type type;
+	unsigned long sz;
+	char *data;
+
+	*size_out = 0;
+	*data_out = NULL;
+
+	data = read_sha1_file(oid->hash, &type, &sz);
+	if (!data || type != OBJ_BLOB) {
+		free(data);
+		return -1;
+	}
+
+	if (sha1_stat) {
+		memset(&sha1_stat->stat, 0, sizeof(sha1_stat->stat));
+		hashcpy(sha1_stat->sha1, oid->hash);
+	}
+
+	if (sz == 0) {
+		free(data);
+		return 0;
+	}
+
+	if (data[sz - 1] != '\n') {
+		data = xrealloc(data, st_add(sz, 1));
+		data[sz++] = '\n';
+	}
+
+	*size_out = xsize_t(sz);
+	*data_out = data;
+
+	return 1;
+}
+
 #define DO_MATCH_EXCLUDE   (1<<0)
 #define DO_MATCH_DIRECTORY (1<<1)
 #define DO_MATCH_SUBMODULE (1<<2)
@@ -600,32 +651,22 @@ void add_exclude(const char *string, const char *base,
 	x->el = el;
 }
 
-static void *read_skip_worktree_file_from_index(const struct index_state *istate,
-						const char *path, size_t *size,
-						struct sha1_stat *sha1_stat)
+static int read_skip_worktree_file_from_index(const struct index_state *istate,
+					      const char *path,
+					      size_t *size_out,
+					      char **data_out,
+					      struct sha1_stat *sha1_stat)
 {
 	int pos, len;
-	unsigned long sz;
-	enum object_type type;
-	void *data;
 
 	len = strlen(path);
 	pos = index_name_pos(istate, path, len);
 	if (pos < 0)
-		return NULL;
+		return -1;
 	if (!ce_skip_worktree(istate->cache[pos]))
-		return NULL;
-	data = read_sha1_file(istate->cache[pos]->oid.hash, &type, &sz);
-	if (!data || type != OBJ_BLOB) {
-		free(data);
-		return NULL;
-	}
-	*size = xsize_t(sz);
-	if (sha1_stat) {
-		memset(&sha1_stat->stat, 0, sizeof(sha1_stat->stat));
-		hashcpy(sha1_stat->sha1, istate->cache[pos]->oid.hash);
-	}
-	return data;
+		return -1;
+
+	return do_read_blob(&istate->cache[pos]->oid, sha1_stat, size_out, data_out);
 }
 
 /*
@@ -739,6 +780,10 @@ static void invalidate_directory(struct untracked_cache *uc,
 		dir->dirs[i]->recurse = 0;
 }
 
+static int add_excludes_from_buffer(char *buf, size_t size,
+				    const char *base, int baselen,
+				    struct exclude_list *el);
+
 /*
  * Given a file with name "fname", read it (either from disk, or from
  * an index if 'istate' is non-null), parse it and store the
@@ -754,9 +799,10 @@ static int add_excludes(const char *fname, const char *base, int baselen,
 			struct sha1_stat *sha1_stat)
 {
 	struct stat st;
-	int fd, i, lineno = 1;
+	int r;
+	int fd;
 	size_t size = 0;
-	char *buf, *entry;
+	char *buf;
 
 	fd = open(fname, O_RDONLY);
 	if (fd < 0 || fstat(fd, &st) < 0) {
@@ -764,17 +810,13 @@ static int add_excludes(const char *fname, const char *base, int baselen,
 			warn_on_fopen_errors(fname);
 		else
 			close(fd);
-		if (!istate ||
-		    (buf = read_skip_worktree_file_from_index(istate, fname, &size, sha1_stat)) == NULL)
+		if (!istate)
 			return -1;
-		if (size == 0) {
-			free(buf);
-			return 0;
-		}
-		if (buf[size-1] != '\n') {
-			buf = xrealloc(buf, st_add(size, 1));
-			buf[size++] = '\n';
-		}
+		r = read_skip_worktree_file_from_index(istate, fname,
+						       &size, &buf,
+						       sha1_stat);
+		if (r != 1)
+			return r;
 	} else {
 		size = xsize_t(st.st_size);
 		if (size == 0) {
@@ -813,6 +855,17 @@ static int add_excludes(const char *fname, const char *base, int baselen,
 		}
 	}
 
+	add_excludes_from_buffer(buf, size, base, baselen, el);
+	return 0;
+}
+
+static int add_excludes_from_buffer(char *buf, size_t size,
+				    const char *base, int baselen,
+				    struct exclude_list *el)
+{
+	int i, lineno = 1;
+	char *entry;
+
 	el->filebuf = buf;
 
 	if (skip_utf8_bom(&buf, size))
@@ -841,6 +894,23 @@ int add_excludes_from_file_to_list(const char *fname, const char *base,
 	return add_excludes(fname, base, baselen, el, istate, NULL);
 }
 
+int add_excludes_from_blob_to_list(
+	struct object_id *oid,
+	const char *base, int baselen,
+	struct exclude_list *el)
+{
+	char *buf;
+	size_t size;
+	int r;
+
+	r = do_read_blob(oid, NULL, &size, &buf);
+	if (r != 1)
+		return r;
+
+	add_excludes_from_buffer(buf, size, base, baselen, el);
+	return 0;
+}
+
 struct exclude_list *add_exclude_list(struct dir_struct *dir,
 				      int group_type, const char *src)
 {
diff --git a/dir.h b/dir.h
index e371705..1bcf391 100644
--- a/dir.h
+++ b/dir.h
@@ -256,6 +256,9 @@ extern struct exclude_list *add_exclude_list(struct dir_struct *dir,
 extern int add_excludes_from_file_to_list(const char *fname, const char *base, int baselen,
 					  struct exclude_list *el, struct  index_state *istate);
 extern void add_excludes_from_file(struct dir_struct *, const char *fname);
+extern int add_excludes_from_blob_to_list(struct object_id *oid,
+					  const char *base, int baselen,
+					  struct exclude_list *el);
 extern void parse_exclude_pattern(const char **string, int *patternlen, unsigned *flags, int *nowildcardlen);
 extern void add_exclude(const char *string, const char *base,
 			int baselen, struct exclude_list *el, int srcpos);
-- 
2.9.3

[PATCH v2 2/6] oidmap: add oidmap iterator methods

[Jeff Hostetler]

From: Jeff Hostetler <jeffhost@microsoft.com>

Add the usual map iterator functions to oidmap.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 oidmap.h | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/oidmap.h b/oidmap.h
index 18f54cd..d3cd2bb 100644
--- a/oidmap.h
+++ b/oidmap.h
@@ -65,4 +65,26 @@ extern void *oidmap_put(struct oidmap *map, void *entry);
  */
 extern void *oidmap_remove(struct oidmap *map, const struct object_id *key);
 
+
+struct oidmap_iter {
+	struct hashmap_iter h_iter;
+};
+
+static inline void oidmap_iter_init(struct oidmap *map, struct oidmap_iter *iter)
+{
+	hashmap_iter_init(&map->map, &iter->h_iter);
+}
+
+static inline void *oidmap_iter_next(struct oidmap_iter *iter)
+{
+	return hashmap_iter_next(&iter->h_iter);
+}
+
+static inline void *oidmap_iter_first(struct oidmap *map,
+				      struct oidmap_iter *iter)
+{
+	oidmap_iter_init(map, iter);
+	return oidmap_iter_next(iter);
+}
+
 #endif
-- 
2.9.3

[PATCH v2 3/6] oidset: add iterator methods to oidset

[Jeff Hostetler]

From: Jeff Hostetler <jeffhost@microsoft.com>

Add the usual iterator methods to oidset.
Add oidset_remove().

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 oidset.c | 10 ++++++++++
 oidset.h | 36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+)

diff --git a/oidset.c b/oidset.c
index f1f874a..454c54f 100644
--- a/oidset.c
+++ b/oidset.c
@@ -24,6 +24,16 @@ int oidset_insert(struct oidset *set, const struct object_id *oid)
 	return 0;
 }
 
+int oidset_remove(struct oidset *set, const struct object_id *oid)
+{
+	struct oidmap_entry *entry;
+
+	entry = oidmap_remove(&set->map, oid);
+	free(entry);
+
+	return (entry != NULL);
+}
+
 void oidset_clear(struct oidset *set)
 {
 	oidmap_free(&set->map, 1);
diff --git a/oidset.h b/oidset.h
index f4c9e0f..783abce 100644
--- a/oidset.h
+++ b/oidset.h
@@ -24,6 +24,12 @@ struct oidset {
 
 #define OIDSET_INIT { OIDMAP_INIT }
 
+
+static inline void oidset_init(struct oidset *set, size_t initial_size)
+{
+	return oidmap_init(&set->map, initial_size);
+}
+
 /**
  * Returns true iff `set` contains `oid`.
  */
@@ -39,9 +45,39 @@ int oidset_contains(const struct oidset *set, const struct object_id *oid);
 int oidset_insert(struct oidset *set, const struct object_id *oid);
 
 /**
+ * Remove the oid from the set.
+ *
+ * Returns 1 if the oid was present in the set, 0 otherwise.
+ */
+int oidset_remove(struct oidset *set, const struct object_id *oid);
+
+/**
  * Remove all entries from the oidset, freeing any resources associated with
  * it.
  */
 void oidset_clear(struct oidset *set);
 
+struct oidset_iter {
+	struct oidmap_iter m_iter;
+};
+
+static inline void oidset_iter_init(struct oidset *set,
+				    struct oidset_iter *iter)
+{
+	oidmap_iter_init(&set->map, &iter->m_iter);
+}
+
+static inline struct object_id *oidset_iter_next(struct oidset_iter *iter)
+{
+	struct oidmap_entry *e = oidmap_iter_next(&iter->m_iter);
+	return e ? &e->oid : NULL;
+}
+
+static inline struct object_id *oidset_iter_first(struct oidset *set,
+						  struct oidset_iter *iter)
+{
+	oidset_iter_init(set, iter);
+	return oidset_iter_next(iter);
+}
+
 #endif /* OIDSET_H */
-- 
2.9.3

[PATCH v2 4/6] list-objects: filter objects in traverse_commit_list

[Jeff Hostetler]

From: Jeff Hostetler <jeffhost@microsoft.com>

Create traverse_commit_list_filtered() and add filtering
interface to allow certain objects to be omitted from the
traversal.

Update traverse_commit_list() to be a wrapper for the above
with a null filter to minimize the number of callers that
needed to be changed.

Object filtering will be used in a future commit by rev-list
and pack-objects for partial clone and fetch to omit unwanted
objects from the result.

traverse_bitmap_commit_list() does not work with filtering.

If a packfile bitmap is present, it will not be used.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 Makefile                      |   2 +
 list-objects-filter-options.c | 119 ++++++++++++
 list-objects-filter-options.h |  55 ++++++
 list-objects-filter.c         | 408 ++++++++++++++++++++++++++++++++++++++++++
 list-objects-filter.h         |  84 +++++++++
 list-objects.c                |  95 ++++++++--
 list-objects.h                |   2 +-
 7 files changed, 748 insertions(+), 17 deletions(-)
 create mode 100644 list-objects-filter-options.c
 create mode 100644 list-objects-filter-options.h
 create mode 100644 list-objects-filter.c
 create mode 100644 list-objects-filter.h

diff --git a/Makefile b/Makefile
index cd75985..ca378a4 100644
--- a/Makefile
+++ b/Makefile
@@ -807,6 +807,8 @@ LIB_OBJS += levenshtein.o
 LIB_OBJS += line-log.o
 LIB_OBJS += line-range.o
 LIB_OBJS += list-objects.o
+LIB_OBJS += list-objects-filter.o
+LIB_OBJS += list-objects-filter-options.o
 LIB_OBJS += ll-merge.o
 LIB_OBJS += lockfile.o
 LIB_OBJS += log-tree.o
diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c
new file mode 100644
index 0000000..31255e7
--- /dev/null
+++ b/list-objects-filter-options.c
@@ -0,0 +1,119 @@
+#include "cache.h"
+#include "commit.h"
+#include "config.h"
+#include "revision.h"
+#include "argv-array.h"
+#include "list-objects.h"
+#include "list-objects-filter.h"
+#include "list-objects-filter-options.h"
+
+/*
+ * Parse value of the argument to the "filter" keword.
+ * On the command line this looks like:
+ *       --filter=<arg>
+ * and in the pack protocol as:
+ *       "filter" SP <arg>
+ *
+ * <arg> ::= blob:none
+ *           blob:limit=<n>[kmg]
+ *           sparse:oid=<oid-expression>
+ *           sparse:path=<pathname>
+ */
+int parse_list_objects_filter(struct list_objects_filter_options *filter_options,
+			      const char *arg)
+{
+	struct object_context oc;
+	struct object_id sparse_oid;
+	const char *v0;
+	const char *v1;
+
+	if (filter_options->choice)
+		die(_("multiple object filter types cannot be combined"));
+
+	/*
+	 * TODO consider rejecting 'arg' if it contains any
+	 * TODO injection characters (since we might send this
+	 * TODO to a sub-command or to the server and we don't
+	 * TODO want to deal with legacy quoting/escaping for
+	 * TODO a new feature).
+	 */
+
+	filter_options->raw_value = strdup(arg);
+
+	if (skip_prefix(arg, "blob:", &v0) || skip_prefix(arg, "blobs:", &v0)) {
+		if (!strcmp(v0, "none")) {
+			filter_options->choice = LOFC_BLOB_NONE;
+			return 0;
+		}
+
+		if (skip_prefix(v0, "limit=", &v1) &&
+		    git_parse_ulong(v1, &filter_options->blob_limit_value)) {
+			filter_options->choice = LOFC_BLOB_LIMIT;
+			return 0;
+		}
+	}
+	else if (skip_prefix(arg, "sparse:", &v0)) {
+		if (skip_prefix(v0, "oid=", &v1)) {
+			filter_options->choice = LOFC_SPARSE_OID;
+			if (!get_oid_with_context(v1, GET_OID_BLOB,
+						  &sparse_oid, &oc)) {
+				/*
+				 * We successfully converted the <oid-expr>
+				 * into an actual OID.  Rewrite the raw_value
+				 * in canonoical form with just the OID.
+				 * (If we send this request to the server, we
+				 * want an absolute expression rather than a
+				 * local-ref-relative expression.)
+				 */
+				free((char *)filter_options->raw_value);
+				filter_options->raw_value =
+					xstrfmt("sparse:oid=%s",
+						oid_to_hex(&sparse_oid));
+				filter_options->sparse_oid_value =
+					oiddup(&sparse_oid);
+			} else {
+				/*
+				 * We could not turn the <oid-expr> into an
+				 * OID.  Leave the raw_value as is in case
+				 * the server can parse it.  (It may refer to
+				 * a branch, commit, or blob we don't have.)
+				 */
+			}
+			return 0;
+		}
+
+		if (skip_prefix(v0, "path=", &v1)) {
+			filter_options->choice = LOFC_SPARSE_PATH;
+			filter_options->sparse_path_value = strdup(v1);
+			return 0;
+		}
+	}
+
+	die(_("invalid filter expression '%s'"), arg);
+	return 0;
+}
+
+int opt_parse_list_objects_filter(const struct option *opt,
+				  const char *arg, int unset)
+{
+	struct list_objects_filter_options *filter_options = opt->value;
+
+	assert(arg);
+	assert(!unset);
+
+	return parse_list_objects_filter(filter_options, arg);
+}
+
+void arg_format_list_objects_filter(
+	struct argv_array *argv_array,
+	const struct list_objects_filter_options *filter_options)
+{
+	if (!filter_options->choice)
+		return;
+
+	/*
+	 * TODO Think about quoting the value.
+	 */
+	argv_array_pushf(argv_array, "--%s=%s", CL_ARG__FILTER,
+			 filter_options->raw_value);
+}
diff --git a/list-objects-filter-options.h b/list-objects-filter-options.h
new file mode 100644
index 0000000..c9c5052
--- /dev/null
+++ b/list-objects-filter-options.h
@@ -0,0 +1,55 @@
+#ifndef LIST_OBJECTS_FILTER_OPTIONS_H
+#define LIST_OBJECTS_FILTER_OPTIONS_H
+
+#include "parse-options.h"
+
+/*
+ * The list of defined filters for list-objects.
+ */
+enum list_objects_filter_choice {
+	LOFC_DISABLED = 0,
+	LOFC_BLOB_NONE,
+	LOFC_BLOB_LIMIT,
+	LOFC_SPARSE_OID,
+	LOFC_SPARSE_PATH,
+	LOFC__COUNT /* must be last */
+};
+
+struct list_objects_filter_options {
+	/*
+	 * The raw argument value given on the command line or
+	 * protocol request.  (The part after the "--keyword=".)
+	 */
+	char *raw_value;
+
+	/*
+	 * Parsed values. Only 1 will be set depending on the flags below.
+	 */
+	struct object_id *sparse_oid_value;
+	char *sparse_path_value;
+	unsigned long blob_limit_value;
+
+	enum list_objects_filter_choice choice;
+};
+
+/* Normalized command line arguments */
+#define CL_ARG__FILTER "filter"
+
+int parse_list_objects_filter(
+	struct list_objects_filter_options *filter_options,
+	const char *arg);
+
+int opt_parse_list_objects_filter(const struct option *opt,
+				  const char *arg, int unset);
+
+#define OPT_PARSE_LIST_OBJECTS_FILTER(fo) \
+	{ OPTION_CALLBACK, 0, CL_ARG__FILTER, fo, N_("args"), \
+	  N_("object filtering"), PARSE_OPT_NONEG, \
+	  opt_parse_list_objects_filter }
+
+struct argv_array;
+void arg_format_list_objects_filter(
+	struct argv_array *aa,
+	const struct list_objects_filter_options *filter_options);
+
+#endif /* LIST_OBJECTS_FILTER_OPTIONS_H */
diff --git a/list-objects-filter.c b/list-objects-filter.c
new file mode 100644
index 0000000..7f28425
--- /dev/null
+++ b/list-objects-filter.c
@@ -0,0 +1,408 @@
+#include "cache.h"
+#include "dir.h"
+#include "tag.h"
+#include "commit.h"
+#include "tree.h"
+#include "blob.h"
+#include "diff.h"
+#include "tree-walk.h"
+#include "revision.h"
+#include "list-objects.h"
+#include "list-objects-filter.h"
+#include "list-objects-filter-options.h"
+#include "oidset.h"
+
+/* See object.h and revision.h */
+#define FILTER_REVISIT (1<<25)
+
+/*
+ * A filter for list-objects to omit ALL blobs from the traversal.
+ * And to OPTIONALLY collect a list of the omitted OIDs.
+ */
+struct filter_blobs_none_data {
+	struct oidset *omits;
+};
+
+static enum list_objects_filter_result filter_blobs_none(
+	enum list_objects_filter_type filter_type,
+	struct object *obj,
+	const char *pathname,
+	const char *filename,
+	void *filter_data_)
+{
+	struct filter_blobs_none_data *filter_data = filter_data_;
+
+	switch (filter_type) {
+	default:
+		die("unkown filter_type");
+		return LOFR_ZERO;
+
+	case LOFT_BEGIN_TREE:
+		assert(obj->type == OBJ_TREE);
+		/* always include all tree objects */
+		return LOFR_MARK_SEEN | LOFR_SHOW;
+
+	case LOFT_END_TREE:
+		assert(obj->type == OBJ_TREE);
+		return LOFR_ZERO;
+
+	case LOFT_BLOB:
+		assert(obj->type == OBJ_BLOB);
+		assert((obj->flags & SEEN) == 0);
+
+		if (filter_data->omits)
+			oidset_insert(filter_data->omits, &obj->oid);
+		return LOFR_MARK_SEEN; /* but not LOFR_SHOW (hard omit) */
+	}
+}
+
+static void *filter_blobs_none__init(
+	struct oidset *omitted,
+	struct list_objects_filter_options *filter_options,
+	filter_object_fn *filter_fn,
+	filter_free_fn *filter_free_fn)
+{
+	struct filter_blobs_none_data *d = xcalloc(1, sizeof(*d));
+	d->omits = omitted;
+
+	*filter_fn = filter_blobs_none;
+	*filter_free_fn = free;
+	return d;
+}
+
+/*
+ * A filter for list-objects to omit large blobs,
+ * but always include ".git*" special files.
+ * And to OPTIONALLY collect a list of the omitted OIDs.
+ */
+struct filter_blobs_limit_data {
+	struct oidset *omits;
+	unsigned long max_bytes;
+};
+
+static enum list_objects_filter_result filter_blobs_limit(
+	enum list_objects_filter_type filter_type,
+	struct object *obj,
+	const char *pathname,
+	const char *filename,
+	void *filter_data_)
+{
+	struct filter_blobs_limit_data *filter_data = filter_data_;
+	unsigned long object_length;
+	enum object_type t;
+	int is_special_filename;
+
+	switch (filter_type) {
+	default:
+		die("unkown filter_type");
+		return LOFR_ZERO;
+
+	case LOFT_BEGIN_TREE:
+		assert(obj->type == OBJ_TREE);
+		/* always include all tree objects */
+		return LOFR_MARK_SEEN | LOFR_SHOW;
+
+	case LOFT_END_TREE:
+		assert(obj->type == OBJ_TREE);
+		return LOFR_ZERO;
+
+	case LOFT_BLOB:
+		assert(obj->type == OBJ_BLOB);
+		assert((obj->flags & SEEN) == 0);
+
+		is_special_filename = ((strncmp(filename, ".git", 4) == 0) &&
+				       filename[4]);
+		if (is_special_filename) {
+			/*
+			 * Alwayse include ".git*" special files (regardless
+			 * of size).
+			 *
+			 * (This may cause us to include blobs that we do
+			 * not have locally because we are only looking at
+			 * the filename and don't actually have to read
+			 * them.)
+			 */
+			goto include_it;
+		}
+
+		t = sha1_object_info(obj->oid.hash, &object_length);
+		if (t != OBJ_BLOB) { /* probably OBJ_NONE */
+			/*
+			 * We DO NOT have the blob locally, so we cannot
+			 * apply the size filter criteria.  Be conservative
+			 * and force show it (and let the caller deal with
+			 * the ambiguity).  (This matches the behavior above
+			 * when the special filename matches.)
+			 */
+			goto include_it;
+		}
+
+		if (object_length < filter_data->max_bytes)
+			goto include_it;
+
+		/*
+		 * Provisionally omit it.  We've already established
+		 * that this blob is too big and doesn't have a special
+		 * filename, so we *WANT* to omit it.  However, there
+		 * may be a special file elsewhere in the tree that
+		 * references this same blob, so we cannot reject it
+		 * just yet.  Leave the LOFR_ bits unset so that *IF*
+		 * the blob appears again in the traversal, we will
+		 * be asked again.
+		 *
+		 * If we are keeping a list of the ommitted objects,
+		 * provisionally add it to the list.
+		 */
+
+		if (filter_data->omits)
+			oidset_insert(filter_data->omits, &obj->oid);
+		return LOFR_ZERO;
+	}
+
+include_it:
+	if (filter_data->omits)
+		oidset_remove(filter_data->omits, &obj->oid);
+	return LOFR_MARK_SEEN | LOFR_SHOW;
+}
+
+static void *filter_blobs_limit__init(
+	struct oidset *omitted,
+	struct list_objects_filter_options *filter_options,
+	filter_object_fn *filter_fn,
+	filter_free_fn *filter_free_fn)
+{
+	struct filter_blobs_limit_data *d = xcalloc(1, sizeof(*d));
+	d->omits = omitted;
+	d->max_bytes = filter_options->blob_limit_value;
+
+	*filter_fn = filter_blobs_limit;
+	*filter_free_fn = free;
+	return d;
+}
+
+/*
+ * A filter driven by a sparse-checkout specification to only
+ * include blobs that a sparse checkout would populate.
+ *
+ * The sparse-checkout spec can be loaded from a blob with the
+ * given OID or from a local pathname.  We allow an OID because
+ * the repo may be bare or we may be doing the filtering on the
+ * server.
+ */
+struct frame {
+	int defval;
+	int child_prov_omit : 1;
+};
+
+struct filter_sparse_data {
+	struct oidset *omits;
+	struct exclude_list el;
+
+	size_t nr, alloc;
+	struct frame *array_frame;
+};
+
+static enum list_objects_filter_result filter_sparse(
+	enum list_objects_filter_type filter_type,
+	struct object *obj,
+	const char *pathname,
+	const char *filename,
+	void *filter_data_)
+{
+	struct filter_sparse_data *filter_data = filter_data_;
+	int val, dtype;
+	struct frame *frame;
+
+	switch (filter_type) {
+	default:
+		die("unkown filter_type");
+		return LOFR_ZERO;
+
+	case LOFT_BEGIN_TREE:
+		assert(obj->type == OBJ_TREE);
+		dtype = DT_DIR;
+		val = is_excluded_from_list(pathname, strlen(pathname),
+					    filename, &dtype, &filter_data->el,
+					    &the_index);
+		if (val < 0)
+			val = filter_data->array_frame[filter_data->nr].defval;
+
+		ALLOC_GROW(filter_data->array_frame, filter_data->nr + 1,
+			   filter_data->alloc);
+		filter_data->nr++;
+		filter_data->array_frame[filter_data->nr].defval = val;
+		filter_data->array_frame[filter_data->nr].child_prov_omit = 0;
+
+		/*
+		 * A directory with this tree OID may appear in multiple
+		 * places in the tree. (Think of a directory move, with
+		 * no other changes.)  And with a different pathname, the
+		 * is_excluded...() results for this directory and items
+		 * contained within it may be different.  So we cannot
+		 * mark it SEEN (yet), since that will prevent process_tree()
+		 * from revisiting this tree object with other pathnames.
+		 *
+		 * Only SHOW the tree object the first time we visit this
+		 * tree object.
+		 *
+		 * We always show all tree objects.  A future optimization
+		 * may want to attempt to narrow this.
+		 */
+		if (obj->flags & FILTER_REVISIT)
+			return LOFR_ZERO;
+		obj->flags |= FILTER_REVISIT;
+		return LOFR_SHOW;
+
+	case LOFT_END_TREE:
+		assert(obj->type == OBJ_TREE);
+		assert(filter_data->nr > 0);
+
+		frame = &filter_data->array_frame[filter_data->nr];
+		filter_data->nr--;
+
+		/*
+		 * Tell our parent directory if any of our children were
+		 * provisionally omitted.
+		 */
+		filter_data->array_frame[filter_data->nr].child_prov_omit |=
+			frame->child_prov_omit;
+
+		/*
+		 * If there are NO provisionally omitted child objects (ALL child
+		 * objects in this folder were INCLUDED), then we can mark the
+		 * folder as SEEN (so we will not have to revisit it again).
+		 */
+		if (!frame->child_prov_omit)
+			return LOFR_MARK_SEEN;
+		return LOFR_ZERO;
+
+	case LOFT_BLOB:
+		assert(obj->type == OBJ_BLOB);
+		assert((obj->flags & SEEN) == 0);
+
+		frame = &filter_data->array_frame[filter_data->nr];
+
+		dtype = DT_REG;
+		val = is_excluded_from_list(pathname, strlen(pathname),
+					    filename, &dtype, &filter_data->el,
+					    &the_index);
+		if (val < 0)
+			val = frame->defval;
+		if (val > 0) {
+			if (filter_data->omits)
+				oidset_remove(filter_data->omits, &obj->oid);
+			return LOFR_MARK_SEEN | LOFR_SHOW;
+		}
+
+		/*
+		 * Provisionally omit it.  We've already established that
+		 * this pathname is not in the sparse-checkout specification
+		 * with the CURRENT pathname, so we *WANT* to omit this blob.
+		 *
+		 * However, a pathname elsewhere in the tree may also
+		 * reference this same blob, so we cannot reject it yet.
+		 * Leave the LOFR_ bits unset so that if the blob appears
+		 * again in the traversal, we will be asked again.
+		 */
+		if (filter_data->omits)
+			oidset_insert(filter_data->omits, &obj->oid);
+
+		/*
+		 * Remember that at least 1 blob in this tree was
+		 * provisionally omitted.  This prevents us from short
+		 * cutting the tree in future iterations.
+		 */
+		frame->child_prov_omit = 1;
+		return LOFR_ZERO;
+	}
+}
+
+
+static void filter_sparse_free(void *filter_data)
+{
+	struct filter_sparse_data *d = filter_data;
+	/* TODO free contents of 'd' */
+	free(d);
+}
+
+static void *filter_sparse_oid__init(
+	struct oidset *omitted,
+	struct list_objects_filter_options *filter_options,
+	filter_object_fn *filter_fn,
+	filter_free_fn *filter_free_fn)
+{
+	struct filter_sparse_data *d = xcalloc(1, sizeof(*d));
+	d->omits = omitted;
+	if (add_excludes_from_blob_to_list(filter_options->sparse_oid_value,
+					   NULL, 0, &d->el) < 0)
+		die("could not load filter specification");
+
+	ALLOC_GROW(d->array_frame, d->nr + 1, d->alloc);
+	d->array_frame[d->nr].defval = 0; /* default to include */
+	d->array_frame[d->nr].child_prov_omit = 0;
+
+	*filter_fn = filter_sparse;
+	*filter_free_fn = filter_sparse_free;
+	return d;
+}
+
+static void *filter_sparse_path__init(
+	struct oidset *omitted,
+	struct list_objects_filter_options *filter_options,
+	filter_object_fn *filter_fn,
+	filter_free_fn *filter_free_fn)
+{
+	struct filter_sparse_data *d = xcalloc(1, sizeof(*d));
+	d->omits = omitted;
+	if (add_excludes_from_file_to_list(filter_options->sparse_path_value,
+					   NULL, 0, &d->el, NULL) < 0)
+		die("could not load filter specification");
+
+	ALLOC_GROW(d->array_frame, d->nr + 1, d->alloc);
+	d->array_frame[d->nr].defval = 0; /* default to include */
+	d->array_frame[d->nr].child_prov_omit = 0;
+
+	*filter_fn = filter_sparse;
+	*filter_free_fn = filter_sparse_free;
+	return d;
+}
+
+typedef void *(*filter_init_fn)(
+	struct oidset *omitted,
+	struct list_objects_filter_options *filter_options,
+	filter_object_fn *filter_fn,
+	filter_free_fn *filter_free_fn);
+
+/*
+ * Must match "enum list_objects_filter_choice".
+ */
+static filter_init_fn s_filters[] = {
+	NULL,
+	filter_blobs_none__init,
+	filter_blobs_limit__init,
+	filter_sparse_oid__init,
+	filter_sparse_path__init,
+};
+
+void *list_objects_filter__init(
+	struct oidset *omitted,
+	struct list_objects_filter_options *filter_options,
+	filter_object_fn *filter_fn,
+	filter_free_fn *filter_free_fn)
+{
+	filter_init_fn init_fn;
+
+	assert((sizeof(s_filters) / sizeof(s_filters[0])) == LOFC__COUNT);
+
+	if (filter_options->choice >= LOFC__COUNT)
+		die("invalid list-objects filter choice: %d",
+		    filter_options->choice);
+
+	init_fn = s_filters[filter_options->choice];
+	if (init_fn)
+		return init_fn(omitted, filter_options,
+			       filter_fn, filter_free_fn);
+	*filter_fn = NULL;
+	*filter_free_fn = NULL;
+	return NULL;
+}
diff --git a/list-objects-filter.h b/list-objects-filter.h
new file mode 100644
index 0000000..f30a514
--- /dev/null
+++ b/list-objects-filter.h
@@ -0,0 +1,84 @@
+#ifndef LIST_OBJECTS_FILTER_H
+#define LIST_OBJECTS_FILTER_H
+
+/*
+ * During list-object traversal we allow certain objects to be
+ * filtered (omitted) from the result.  The active filter uses
+ * these result values to guide list-objects.
+ *
+ * _ZERO      : Do nothing with the object at this time.  It may
+ *              be revisited if it appears in another place in
+ *              the tree or in another commit during the overall
+ *              traversal.
+ *
+ * _MARK_SEEN : Mark this object as "SEEN" in the object flags.
+ *              This will prevent it from being revisited during
+ *              the remainder of the traversal.  This DOES NOT
+ *              imply that it will be included in the results.
+ *
+ * _SHOW      : Show this object in the results (call show() on it).
+ *              In general, objects should only be shown once, but
+ *              this result DOES NOT imply that we mark it SEEN.
+ *
+ * Most of the time, you want the combination (_MARK_SEEN | _SHOW)
+ * but they can be used independently, such as when sparse-checkout
+ * pattern matching is being applied.
+ *
+ * A _MARK_SEEN without _SHOW can be called a hard-omit -- the
+ * object is not shown and will never be reconsidered (unless a
+ * previous iteration has already shown it).
+ *
+ * A _ZERO is can be called a provisional-omit -- the object is
+ * not shown, but *may* be revisited (if the object appears again
+ * in the traversal).  Therefore, it will be omitted from the
+ * results *unless* a later iteration causes it to be shown.
+ */
+enum list_objects_filter_result {
+	LOFR_ZERO      = 0,
+	LOFR_MARK_SEEN = 1<<0,
+	LOFR_SHOW      = 1<<1,
+};
+
+enum list_objects_filter_type {
+	LOFT_BEGIN_TREE,
+	LOFT_END_TREE,
+	LOFT_BLOB
+};
+
+typedef enum list_objects_filter_result (*filter_object_fn)(
+	enum list_objects_filter_type filter_type,
+	struct object *obj,
+	const char *pathname,
+	const char *filename,
+	void *filter_data);
+
+typedef void (*filter_free_fn)(void *filter_data);
+
+struct oidset;
+struct list_objects_filter_options;
+
+void traverse_commit_list_filtered(
+	struct list_objects_filter_options *filter_options,
+	struct rev_info *revs,
+	show_commit_fn show_commit,
+	show_object_fn show_object,
+	void *show_data,
+	struct oidset *omitted);
+
+/*
+ * Constructor for the set of defined list-objects filters.
+ * Returns a generic "void *filter_data".
+ *
+ * The returned "filter_fn" will be used by traverse_commit_list()
+ * to filter the results.
+ *
+ * The returned "filter_free_fn" is a destructor for the
+ * filter_data.
+ */
+void *list_objects_filter__init(
+	struct oidset *omitted,
+	struct list_objects_filter_options *filter_options,
+	filter_object_fn *filter_fn,
+	filter_free_fn *filter_free_fn);
+
+#endif /* LIST_OBJECTS_FILTER_H */
diff --git a/list-objects.c b/list-objects.c
index b3931fa..848b040 100644
--- a/list-objects.c
+++ b/list-objects.c
@@ -7,16 +7,21 @@
 #include "tree-walk.h"
 #include "revision.h"
 #include "list-objects.h"
+#include "list-objects-filter.h"
+#include "list-objects-filter-options.h"
 
 static void process_blob(struct rev_info *revs,
 			 struct blob *blob,
 			 show_object_fn show,
 			 struct strbuf *path,
 			 const char *name,
-			 void *cb_data)
+			 void *cb_data,
+			 filter_object_fn filter_fn,
+			 void *filter_data)
 {
 	struct object *obj = &blob->object;
 	size_t pathlen;
+	enum list_objects_filter_result r = LOFR_MARK_SEEN | LOFR_SHOW;
 
 	if (!revs->blob_objects)
 		return;
@@ -24,11 +29,17 @@ static void process_blob(struct rev_info *revs,
 		die("bad blob object");
 	if (obj->flags & (UNINTERESTING | SEEN))
 		return;
-	obj->flags |= SEEN;
 
 	pathlen = path->len;
 	strbuf_addstr(path, name);
-	show(obj, path->buf, cb_data);
+	if (filter_fn)
+		r = filter_fn(LOFT_BLOB, obj,
+			      path->buf, &path->buf[pathlen],
+			      filter_data);
+	if (r & LOFR_MARK_SEEN)
+		obj->flags |= SEEN;
+	if (r & LOFR_SHOW)
+		show(obj, path->buf, cb_data);
 	strbuf_setlen(path, pathlen);
 }
 
@@ -69,7 +80,9 @@ static void process_tree(struct rev_info *revs,
 			 show_object_fn show,
 			 struct strbuf *base,
 			 const char *name,
-			 void *cb_data)
+			 void *cb_data,
+			 filter_object_fn filter_fn,
+			 void *filter_data)
 {
 	struct object *obj = &tree->object;
 	struct tree_desc desc;
@@ -77,6 +90,7 @@ static void process_tree(struct rev_info *revs,
 	enum interesting match = revs->diffopt.pathspec.nr == 0 ?
 		all_entries_interesting: entry_not_interesting;
 	int baselen = base->len;
+	enum list_objects_filter_result r = LOFR_MARK_SEEN | LOFR_SHOW;
 
 	if (!revs->tree_objects)
 		return;
@@ -90,9 +104,15 @@ static void process_tree(struct rev_info *revs,
 		die("bad tree object %s", oid_to_hex(&obj->oid));
 	}
 
-	obj->flags |= SEEN;
 	strbuf_addstr(base, name);
-	show(obj, base->buf, cb_data);
+	if (filter_fn)
+		r = filter_fn(LOFT_BEGIN_TREE, obj,
+			      base->buf, &base->buf[baselen],
+			      filter_data);
+	if (r & LOFR_MARK_SEEN)
+		obj->flags |= SEEN;
+	if (r & LOFR_SHOW)
+		show(obj, base->buf, cb_data);
 	if (base->len)
 		strbuf_addch(base, '/');
 
@@ -112,7 +132,7 @@ static void process_tree(struct rev_info *revs,
 			process_tree(revs,
 				     lookup_tree(entry.oid),
 				     show, base, entry.path,
-				     cb_data);
+				     cb_data, filter_fn, filter_data);
 		else if (S_ISGITLINK(entry.mode))
 			process_gitlink(revs, entry.oid->hash,
 					show, base, entry.path,
@@ -121,8 +141,19 @@ static void process_tree(struct rev_info *revs,
 			process_blob(revs,
 				     lookup_blob(entry.oid),
 				     show, base, entry.path,
-				     cb_data);
+				     cb_data, filter_fn, filter_data);
 	}
+
+	if (filter_fn) {
+		r = filter_fn(LOFT_END_TREE, obj,
+			      base->buf, &base->buf[baselen],
+			      filter_data);
+		if (r & LOFR_MARK_SEEN)
+			obj->flags |= SEEN;
+		if (r & LOFR_SHOW)
+			show(obj, base->buf, cb_data);
+	}
+
 	strbuf_setlen(base, baselen);
 	free_tree_buffer(tree);
 }
@@ -183,10 +214,12 @@ static void add_pending_tree(struct rev_info *revs, struct tree *tree)
 	add_pending_object(revs, &tree->object, "");
 }
 
-void traverse_commit_list(struct rev_info *revs,
-			  show_commit_fn show_commit,
-			  show_object_fn show_object,
-			  void *data)
+static void do_traverse(struct rev_info *revs,
+			show_commit_fn show_commit,
+			show_object_fn show_object,
+			void *show_data,
+			filter_object_fn filter_fn,
+			void *filter_data)
 {
 	int i;
 	struct commit *commit;
@@ -200,7 +233,7 @@ void traverse_commit_list(struct rev_info *revs,
 		 */
 		if (commit->tree)
 			add_pending_tree(revs, commit->tree);
-		show_commit(commit, data);
+		show_commit(commit, show_data);
 	}
 	for (i = 0; i < revs->pending.nr; i++) {
 		struct object_array_entry *pending = revs->pending.objects + i;
@@ -211,19 +244,21 @@ void traverse_commit_list(struct rev_info *revs,
 			continue;
 		if (obj->type == OBJ_TAG) {
 			obj->flags |= SEEN;
-			show_object(obj, name, data);
+			show_object(obj, name, show_data);
 			continue;
 		}
 		if (!path)
 			path = "";
 		if (obj->type == OBJ_TREE) {
 			process_tree(revs, (struct tree *)obj, show_object,
-				     &base, path, data);
+				     &base, path, show_data,
+				     filter_fn, filter_data);
 			continue;
 		}
 		if (obj->type == OBJ_BLOB) {
 			process_blob(revs, (struct blob *)obj, show_object,
-				     &base, path, data);
+				     &base, path, show_data,
+				     filter_fn, filter_data);
 			continue;
 		}
 		die("unknown pending object %s (%s)",
@@ -232,3 +267,31 @@ void traverse_commit_list(struct rev_info *revs,
 	object_array_clear(&revs->pending);
 	strbuf_release(&base);
 }
+
+void traverse_commit_list(struct rev_info *revs,
+			  show_commit_fn show_commit,
+			  show_object_fn show_object,
+			  void *show_data)
+{
+	do_traverse(revs, show_commit, show_object, show_data, NULL, NULL);
+}
+
+void traverse_commit_list_filtered(
+	struct list_objects_filter_options *filter_options,
+	struct rev_info *revs,
+	show_commit_fn show_commit,
+	show_object_fn show_object,
+	void *show_data,
+	struct oidset *omitted)
+{
+	filter_object_fn filter_fn = NULL;
+	filter_free_fn filter_free_fn = NULL;
+	void *filter_data = NULL;
+
+	filter_data = list_objects_filter__init(omitted, filter_options,
+						&filter_fn, &filter_free_fn);
+	do_traverse(revs, show_commit, show_object, show_data,
+		    filter_fn, filter_data);
+	if (filter_data && filter_free_fn)
+		filter_free_fn(filter_data);
+}
diff --git a/list-objects.h b/list-objects.h
index 0cebf85..33c964c 100644
--- a/list-objects.h
+++ b/list-objects.h
@@ -8,4 +8,4 @@ void traverse_commit_list(struct rev_info *, show_commit_fn, show_object_fn, voi
 typedef void (*show_edge_fn)(struct commit *);
 void mark_edges_uninteresting(struct rev_info *, show_edge_fn);
 
-#endif
+#endif /* LIST_OBJECTS_H */
-- 
2.9.3

Re: [PATCH v2 4/6] list-objects: filter objects in traverse_commit_list

[Jonathan Tan]

On Thu,  2 Nov 2017 17:50:11 +0000
Jeff Hostetler <git@jeffhostetler.com> wrote:

> +int parse_list_objects_filter(struct list_objects_filter_options *filter_options,
> +			      const char *arg)

Returning void is fine, I think. It seems that all your code paths
either return 0 or die.

> +{
> +	struct object_context oc;
> +	struct object_id sparse_oid;
> +	const char *v0;
> +	const char *v1;
> +
> +	if (filter_options->choice)
> +		die(_("multiple object filter types cannot be combined"));
> +
> +	/*
> +	 * TODO consider rejecting 'arg' if it contains any
> +	 * TODO injection characters (since we might send this
> +	 * TODO to a sub-command or to the server and we don't
> +	 * TODO want to deal with legacy quoting/escaping for
> +	 * TODO a new feature).
> +	 */
> +
> +	filter_options->raw_value = strdup(arg);
> +
> +	if (skip_prefix(arg, "blob:", &v0) || skip_prefix(arg, "blobs:", &v0)) {
> +		if (!strcmp(v0, "none")) {
> +			filter_options->choice = LOFC_BLOB_NONE;
> +			return 0;
> +		}
> +
> +		if (skip_prefix(v0, "limit=", &v1) &&
> +		    git_parse_ulong(v1, &filter_options->blob_limit_value)) {
> +			filter_options->choice = LOFC_BLOB_LIMIT;
> +			return 0;
> +		}
> +	}
> +	else if (skip_prefix(arg, "sparse:", &v0)) {

Style: join the 2 lines above.

> +		if (skip_prefix(v0, "oid=", &v1)) {
> +			filter_options->choice = LOFC_SPARSE_OID;
> +			if (!get_oid_with_context(v1, GET_OID_BLOB,
> +						  &sparse_oid, &oc)) {
> +				/*
> +				 * We successfully converted the <oid-expr>
> +				 * into an actual OID.  Rewrite the raw_value
> +				 * in canonoical form with just the OID.
> +				 * (If we send this request to the server, we
> +				 * want an absolute expression rather than a
> +				 * local-ref-relative expression.)
> +				 */

I think this would lead to confusing behavior - for example, a fetch
with "--filter=oid=mybranch:sparseconfig" would have different results
depending on whether "mybranch" refers to a valid object locally.

The way I see it, this should either (i) only accept full 40-character
OIDs or (ii) retain the raw string to be interpreted only when the
filtering is done. (i) is simpler and safer, but is not so useful. In
both cases, if the user really wants client-side interpretation, they
can still use "$(git rev-parse foo)" to make it explicit.

> +				free((char *)filter_options->raw_value);
> +				filter_options->raw_value =
> +					xstrfmt("sparse:oid=%s",
> +						oid_to_hex(&sparse_oid));
> +				filter_options->sparse_oid_value =
> +					oiddup(&sparse_oid);
> +			} else {
> +				/*
> +				 * We could not turn the <oid-expr> into an
> +				 * OID.  Leave the raw_value as is in case
> +				 * the server can parse it.  (It may refer to
> +				 * a branch, commit, or blob we don't have.)
> +				 */
> +			}
> +			return 0;
> +		}
> +
> +		if (skip_prefix(v0, "path=", &v1)) {
> +			filter_options->choice = LOFC_SPARSE_PATH;
> +			filter_options->sparse_path_value = strdup(v1);
> +			return 0;
> +		}
> +	}
> +
> +	die(_("invalid filter expression '%s'"), arg);
> +	return 0;
> +}

[snip]

> +void arg_format_list_objects_filter(
> +	struct argv_array *argv_array,
> +	const struct list_objects_filter_options *filter_options)

Is this function used anywhere (in this patch or subsequent patches)?

> diff --git a/list-objects-filter.c b/list-objects-filter.c
> +/* See object.h and revision.h */
> +#define FILTER_REVISIT (1<<25)

Looking later in the code, this flag indicates that a tree has been
SHOWN, so it might be better to just call this FILTER_SHOWN.

Also document this flag in object.h in the table above FLAG_BITS.

> +static enum list_objects_filter_result filter_blobs_limit(
> +	enum list_objects_filter_type filter_type,
> +	struct object *obj,
> +	const char *pathname,
> +	const char *filename,
> +	void *filter_data_)
> +{
> +	struct filter_blobs_limit_data *filter_data = filter_data_;
> +	unsigned long object_length;
> +	enum object_type t;
> +	int is_special_filename;
> +
> +	switch (filter_type) {
> +	default:
> +		die("unkown filter_type");

Spelling of "unknown" (also elsewhere in this file).

> +		return LOFR_ZERO;
> +
> +	case LOFT_BEGIN_TREE:
> +		assert(obj->type == OBJ_TREE);
> +		/* always include all tree objects */
> +		return LOFR_MARK_SEEN | LOFR_SHOW;
> +
> +	case LOFT_END_TREE:
> +		assert(obj->type == OBJ_TREE);
> +		return LOFR_ZERO;
> +
> +	case LOFT_BLOB:
> +		assert(obj->type == OBJ_BLOB);
> +		assert((obj->flags & SEEN) == 0);
> +
> +		is_special_filename = ((strncmp(filename, ".git", 4) == 0) &&
> +				       filename[4]);

You can just use starts_with(). Also, the filename[4] check is probably
unnecessary.

> +		if (is_special_filename) {
> +			/*
> +			 * Alwayse include ".git*" special files (regardless
> +			 * of size).
> +			 *
> +			 * (This may cause us to include blobs that we do
> +			 * not have locally because we are only looking at
> +			 * the filename and don't actually have to read
> +			 * them.)
> +			 */
> +			goto include_it;
> +		}
> +
> +		t = sha1_object_info(obj->oid.hash, &object_length);
> +		if (t != OBJ_BLOB) { /* probably OBJ_NONE */
> +			/*
> +			 * We DO NOT have the blob locally, so we cannot
> +			 * apply the size filter criteria.  Be conservative
> +			 * and force show it (and let the caller deal with
> +			 * the ambiguity).  (This matches the behavior above
> +			 * when the special filename matches.)
> +			 */
> +			goto include_it;
> +		}
> +
> +		if (object_length < filter_data->max_bytes)
> +			goto include_it;
> +
> +		/*
> +		 * Provisionally omit it.  We've already established
> +		 * that this blob is too big and doesn't have a special
> +		 * filename, so we *WANT* to omit it.  However, there
> +		 * may be a special file elsewhere in the tree that
> +		 * references this same blob, so we cannot reject it
> +		 * just yet.  Leave the LOFR_ bits unset so that *IF*
> +		 * the blob appears again in the traversal, we will
> +		 * be asked again.
> +		 *
> +		 * If we are keeping a list of the ommitted objects,
> +		 * provisionally add it to the list.
> +		 */
> +
> +		if (filter_data->omits)
> +			oidset_insert(filter_data->omits, &obj->oid);
> +		return LOFR_ZERO;
> +	}
> +
> +include_it:
> +	if (filter_data->omits)
> +		oidset_remove(filter_data->omits, &obj->oid);
> +	return LOFR_MARK_SEEN | LOFR_SHOW;
> +}

[snip]

> +struct frame {
> +	int defval;

Document this variable?

> +	int child_prov_omit : 1;

I think it's clearer if we use "unsigned" here. Also, document this
(e.g. "1 if any descendant of this tree object was provisionally
omitted").

> +/*
> + * During list-object traversal we allow certain objects to be
> + * filtered (omitted) from the result.  The active filter uses
> + * these result values to guide list-objects.
> + *
> + * _ZERO      : Do nothing with the object at this time.  It may
> + *              be revisited if it appears in another place in
> + *              the tree or in another commit during the overall
> + *              traversal.
> + *
> + * _MARK_SEEN : Mark this object as "SEEN" in the object flags.
> + *              This will prevent it from being revisited during
> + *              the remainder of the traversal.  This DOES NOT
> + *              imply that it will be included in the results.
> + *
> + * _SHOW      : Show this object in the results (call show() on it).
> + *              In general, objects should only be shown once, but
> + *              this result DOES NOT imply that we mark it SEEN.
> + *
> + * Most of the time, you want the combination (_MARK_SEEN | _SHOW)
> + * but they can be used independently, such as when sparse-checkout
> + * pattern matching is being applied.
> + *
> + * A _MARK_SEEN without _SHOW can be called a hard-omit -- the
> + * object is not shown and will never be reconsidered (unless a
> + * previous iteration has already shown it).
> + *
> + * A _ZERO is can be called a provisional-omit -- the object is
> + * not shown, but *may* be revisited (if the object appears again
> + * in the traversal).  Therefore, it will be omitted from the
> + * results *unless* a later iteration causes it to be shown.
> + */
> +enum list_objects_filter_result {
> +	LOFR_ZERO      = 0,
> +	LOFR_MARK_SEEN = 1<<0,
> +	LOFR_SHOW      = 1<<1,
> +};

Thanks - this looks like a good explanation to me.

> +enum list_objects_filter_type {
> +	LOFT_BEGIN_TREE,
> +	LOFT_END_TREE,
> +	LOFT_BLOB
> +};

Optional: probably a better name would be list_objects_filter_situation.

> +void traverse_commit_list_filtered(
> +	struct list_objects_filter_options *filter_options,
> +	struct rev_info *revs,
> +	show_commit_fn show_commit,
> +	show_object_fn show_object,
> +	void *show_data,
> +	struct oidset *omitted)
> +{
> +	filter_object_fn filter_fn = NULL;
> +	filter_free_fn filter_free_fn = NULL;
> +	void *filter_data = NULL;
> +
> +	filter_data = list_objects_filter__init(omitted, filter_options,
> +						&filter_fn, &filter_free_fn);
> +	do_traverse(revs, show_commit, show_object, show_data,
> +		    filter_fn, filter_data);
> +	if (filter_data && filter_free_fn)
> +		filter_free_fn(filter_data);
> +}

This function traverse_commit_list_filtered() is in list-objects.c but
in list-objects-filter.h, if I'm reading the diff correctly?

Overall, this looks like a good change. Object traversal was upgraded
with the behaviors of MARK_SEEN and SHOW independently controllable and
with the ability to do things post-tree (in addition to pre-tree and
blob), and this was used to support a few types of filtering, which
subsequent patches will allow the user to invoke through "--filter=".

Re: [PATCH v2 4/6] list-objects: filter objects in traverse_commit_list

[Johannes Schindelin]

Hi Jonathan,

On Thu, 2 Nov 2017, Jonathan Tan wrote:

> On Thu,  2 Nov 2017 17:50:11 +0000
> Jeff Hostetler <git@jeffhostetler.com> wrote:
> 
> > +int parse_list_objects_filter(struct list_objects_filter_options *filter_options,
> > +			      const char *arg)
> 
> Returning void is fine, I think. It seems that all your code paths
> either return 0 or die.

Can we please start to encourage libified code, rather than discourage it?

Thank you,
Dscho

Re: [PATCH v2 4/6] list-objects: filter objects in traverse_commit_list

[Jeff Hostetler]

On 11/3/2017 7:54 AM, Johannes Schindelin wrote:
> Hi Jonathan,
> 
> On Thu, 2 Nov 2017, Jonathan Tan wrote:
> 
>> On Thu,  2 Nov 2017 17:50:11 +0000
>> Jeff Hostetler <git@jeffhostetler.com> wrote:
>>
>>> +int parse_list_objects_filter(struct list_objects_filter_options *filter_options,
>>> +			      const char *arg)
>>
>> Returning void is fine, I think. It seems that all your code paths
>> either return 0 or die.
> 
> Can we please start to encourage libified code, rather than discourage it?


I did that so that I could call it from the opt_parse_... version below
it that is used by the OPT_ macros.

And Johannes is right, it bothers me that there doesn't seem to be a hard
line where one should or should not call die() vs returning an error code.

Jeff

Re: [PATCH v2 4/6] list-objects: filter objects in traverse_commit_list

[Jeff Hostetler]

On 11/2/2017 3:32 PM, Jonathan Tan wrote:
> On Thu,  2 Nov 2017 17:50:11 +0000
> Jeff Hostetler <git@jeffhostetler.com> wrote:
> 
>> +		if (skip_prefix(v0, "oid=", &v1)) {
>> +			filter_options->choice = LOFC_SPARSE_OID;
>> +			if (!get_oid_with_context(v1, GET_OID_BLOB,
>> +						  &sparse_oid, &oc)) {
>> +				/*
>> +				 * We successfully converted the <oid-expr>
>> +				 * into an actual OID.  Rewrite the raw_value
>> +				 * in canonoical form with just the OID.
>> +				 * (If we send this request to the server, we
>> +				 * want an absolute expression rather than a
>> +				 * local-ref-relative expression.)
>> +				 */
> 
> I think this would lead to confusing behavior - for example, a fetch
> with "--filter=oid=mybranch:sparseconfig" would have different results
> depending on whether "mybranch" refers to a valid object locally.
> 
> The way I see it, this should either (i) only accept full 40-character
> OIDs or (ii) retain the raw string to be interpreted only when the
> filtering is done. (i) is simpler and safer, but is not so useful. In
> both cases, if the user really wants client-side interpretation, they
> can still use "$(git rev-parse foo)" to make it explicit.

Good point. I'll change it to always pass the original expression
so that it is evaluated wherever the filtering is actually performed.


> 
>> +				free((char *)filter_options->raw_value);
>> +				filter_options->raw_value =
>> +					xstrfmt("sparse:oid=%s",
>> +						oid_to_hex(&sparse_oid));
>> +				filter_options->sparse_oid_value =
>> +					oiddup(&sparse_oid);
>> +			} else {
>> +				/*
>> +				 * We could not turn the <oid-expr> into an
>> +				 * OID.  Leave the raw_value as is in case
>> +				 * the server can parse it.  (It may refer to
>> +				 * a branch, commit, or blob we don't have.)
>> +				 */
>> +			}
>> +			return 0;
>> +		}
>> +
>> +		if (skip_prefix(v0, "path=", &v1)) {
>> +			filter_options->choice = LOFC_SPARSE_PATH;
>> +			filter_options->sparse_path_value = strdup(v1);
>> +			return 0;
>> +		}
>> +	}
>> +
>> +	die(_("invalid filter expression '%s'"), arg);
>> +	return 0;
>> +}
> 
> [snip]
> 
>> +void arg_format_list_objects_filter(
>> +	struct argv_array *argv_array,
>> +	const struct list_objects_filter_options *filter_options)
> 
> Is this function used anywhere (in this patch or subsequent patches)?

It is used in upload-pack.c in part 3.  I'll remove it from part 1
and revisit in part 3.
  


>> diff --git a/list-objects-filter.c b/list-objects-filter.c
>> +/* See object.h and revision.h */
>> +#define FILTER_REVISIT (1<<25)
> 
> Looking later in the code, this flag indicates that a tree has been
> SHOWN, so it might be better to just call this FILTER_SHOWN.

I'll amend this. There are already several SHOWN bits that behave
slightly differently.  I'll update and document this better.  Thanks.


> 
> [snip]
> 
>> +struct frame {
>> +	int defval;
> 
> Document this variable?
> 
>> +	int child_prov_omit : 1;
> 
> I think it's clearer if we use "unsigned" here. Also, document this
> (e.g. "1 if any descendant of this tree object was provisionally
> omitted").

got it. thanks.


>> +enum list_objects_filter_type {
>> +	LOFT_BEGIN_TREE,
>> +	LOFT_END_TREE,
>> +	LOFT_BLOB
>> +};
> 
> Optional: probably a better name would be list_objects_filter_situation.

got it. thanks.

  
>> +void traverse_commit_list_filtered(
>> +	struct list_objects_filter_options *filter_options,
>> +	struct rev_info *revs,
>> +	show_commit_fn show_commit,
>> +	show_object_fn show_object,
>> +	void *show_data,
>> +	struct oidset *omitted)
>> +{
>> +	filter_object_fn filter_fn = NULL;
>> +	filter_free_fn filter_free_fn = NULL;
>> +	void *filter_data = NULL;
>> +
>> +	filter_data = list_objects_filter__init(omitted, filter_options,
>> +						&filter_fn, &filter_free_fn);
>> +	do_traverse(revs, show_commit, show_object, show_data,
>> +		    filter_fn, filter_data);
>> +	if (filter_data && filter_free_fn)
>> +		filter_free_fn(filter_data);
>> +}
> 
> This function traverse_commit_list_filtered() is in list-objects.c but
> in list-objects-filter.h, if I'm reading the diff correctly?

oops.  thanks.


> 
> Overall, this looks like a good change. Object traversal was upgraded
> with the behaviors of MARK_SEEN and SHOW independently controllable and
> with the ability to do things post-tree (in addition to pre-tree and
> blob), and this was used to support a few types of filtering, which
> subsequent patches will allow the user to invoke through "--filter=".
> 

thanks
Jeff

Re: [PATCH v2 4/6] list-objects: filter objects in traverse_commit_list

[Jeff Hostetler]

On 11/2/2017 1:50 PM, Jeff Hostetler wrote:
> From: Jeff Hostetler <jeffhost@microsoft.com>
> 
> Create traverse_commit_list_filtered() and add filtering
> interface to allow certain objects to be omitted from the
> traversal.
> ...
> diff --git a/list-objects-filter.c b/list-objects-filter.c
> new file mode 100644
> index 0000000..7f28425
> --- /dev/null
> +++ b/list-objects-filter.c
> ...
> +/*
> + * A filter for list-objects to omit large blobs,
> + * but always include ".git*" special files.
> + * And to OPTIONALLY collect a list of the omitted OIDs.
> + */

Jonathan and I were talking off-list about the performance
effects of inspecting the pathnames to identify the ".git*"
special files. I added it in my first draft back in the spring,
thinking that even if you set the blob-limit to a small
number (or zero), you'd probably still always want the
.gitattribute and .gitignore files.  But now with the addition
of the sparse filter and functional dynamic object fetching,
I'm not sure I see the need for this.

Also, if the primary use of the blob-limit is to filter out
giant binary assets, it is unlikely anyone is going to have
a 1MB+ .git* file, so it is unlikely that the is_special_file
would include anything that wouldn't already be included by
the size criteria.

So, if there's no objections, I think I'll remove this and
simplify the blob-limit filter function.  (That would let me
get rid of the provisional omit code here.)

Jeff

Re: [PATCH v2 4/6] list-objects: filter objects in traverse_commit_list

[Jonathan Tan]

On Mon, 6 Nov 2017 12:51:52 -0500
Jeff Hostetler <git@jeffhostetler.com> wrote:

> Jonathan and I were talking off-list about the performance
> effects of inspecting the pathnames to identify the ".git*"
> special files. I added it in my first draft back in the spring,
> thinking that even if you set the blob-limit to a small
> number (or zero), you'd probably still always want the
> .gitattribute and .gitignore files.  But now with the addition
> of the sparse filter and functional dynamic object fetching,
> I'm not sure I see the need for this.
> 
> Also, if the primary use of the blob-limit is to filter out
> giant binary assets, it is unlikely anyone is going to have
> a 1MB+ .git* file, so it is unlikely that the is_special_file
> would include anything that wouldn't already be included by
> the size criteria.
> 
> So, if there's no objections, I think I'll remove this and
> simplify the blob-limit filter function.  (That would let me
> get rid of the provisional omit code here.)

This sounds like a good idea to me. (For the record, one of the
performance impacts of checking the filename is that bitmaps can't be
used to obtain a whitelist of what is to be packed - instead, a regular
object walk must be used.)

[PATCH v2 5/6] rev-list: add list-objects filtering support

[Jeff Hostetler]

From: Jeff Hostetler <jeffhost@microsoft.com>

Teach rev-list to use the filtering provided by the
traverse_commit_list_filtered() interface to omit
unwanted objects from the result.  This feature is
intended to help with partial clone.

Object filtering is only allowed when one of the "--objects*"
options are used.

When the "--filter-print-omitted" option is used, the omitted
objects are printed at the end.  These are marked with a "~".
This option can be combined with "--quiet" to get a list of
just the omitted objects.

Normally, rev-list will stop with an error when there are
missing objects.

When the "--filter-print-missing" option is used, rev-list
will print a list of any missing objects that should have
been included in the output (rather than stopping).
These are marked with a "?".

When the "--filter-ignore-missing" option is used, rev-list
will silently ignore any missing objects and continue without
error.

Add t6112 test.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 Documentation/git-rev-list.txt      |   6 +-
 Documentation/rev-list-options.txt  |  34 ++++++
 builtin/rev-list.c                  |  75 +++++++++++-
 t/t6112-rev-list-filters-objects.sh | 225 ++++++++++++++++++++++++++++++++++++
 4 files changed, 337 insertions(+), 3 deletions(-)
 create mode 100755 t/t6112-rev-list-filters-objects.sh

diff --git a/Documentation/git-rev-list.txt b/Documentation/git-rev-list.txt
index ef22f17..b8a3a5b 100644
--- a/Documentation/git-rev-list.txt
+++ b/Documentation/git-rev-list.txt
@@ -47,7 +47,11 @@ SYNOPSIS
 	     [ --fixed-strings | -F ]
 	     [ --date=<format>]
 	     [ [ --objects | --objects-edge | --objects-edge-aggressive ]
-	       [ --unpacked ] ]
+	       [ --unpacked ]
+	       [ --filter=<filter-spec> ] ]
+	     [ --filter-print-missing ]
+	     [ --filter-print-omitted ]
+	     [ --filter-ignore-missing ]
 	     [ --pretty | --header ]
 	     [ --bisect ]
 	     [ --bisect-vars ]
diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 13501e1..9233134 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -706,6 +706,40 @@ ifdef::git-rev-list[]
 --unpacked::
 	Only useful with `--objects`; print the object IDs that are not
 	in packs.
+
+--filter=<filter-spec>::
+	Only useful with one of the `--objects*`; omits objects (usually
+	blobs) from the list of printed objects.  The '<filter-spec>'
+	may be one of the following:
++
+The form '--filter=blob:none' omits all blobs.
++
+The form '--filter=blob:limit=<n>[kmg]' omits blobs larger than n bytes
+or units.  The value may be zero.  Special files matching '.git*' are
+alwayse included, regardless of size.
++
+The form '--filter=sparse:oid=<oid-ish>' uses a sparse-checkout
+specification contained in the object (or the object that the expression
+evaluates to) to omit blobs not required by the corresponding sparse
+checkout.
++
+The form '--filter=sparse:path=<path>' similarly uses a sparse-checkout
+specification contained in <path>.
+
+--filter-print-missing::
+	Prints a list of the missing objects for the requested traversal.
+	Object IDs are prefixed with a ``?'' character.  The object type
+	is printed after the ID.  This may be used with or without any of
+	the above filtering options.
+
+--filter-ignore-missing::
+	Ignores missing objects encountered during the requested traversal.
+	This may be used with or without any of the above filtering options.
+
+--filter-print-omitted::
+	Only useful with one of the above `--filter*`; prints a list
+	of the omitted objects.  Object IDs are prefixed with a ``~''
+	character.
 endif::git-rev-list[]
 
 --no-walk[=(sorted|unsorted)]::
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index c1c74d4..cc9fa40 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -4,6 +4,8 @@
 #include "diff.h"
 #include "revision.h"
 #include "list-objects.h"
+#include "list-objects-filter.h"
+#include "list-objects-filter-options.h"
 #include "pack.h"
 #include "pack-bitmap.h"
 #include "builtin.h"
@@ -12,6 +14,7 @@
 #include "bisect.h"
 #include "progress.h"
 #include "reflog-walk.h"
+#include "oidset.h"
 
 static const char rev_list_usage[] =
 "git rev-list [OPTION] <commit-id>... [ -- paths... ]\n"
@@ -54,6 +57,15 @@ static const char rev_list_usage[] =
 
 static struct progress *progress;
 static unsigned progress_counter;
+static struct list_objects_filter_options filter_options;
+static struct oidset missing_objects;
+static struct oidset omitted_objects;
+static int arg_print_missing;
+static int arg_print_omitted;
+static int arg_ignore_missing;
+
+#define DEFAULT_OIDSET_SIZE     (16*1024)
+
 
 static void finish_commit(struct commit *commit, void *data);
 static void show_commit(struct commit *commit, void *data)
@@ -181,8 +193,16 @@ static void finish_commit(struct commit *commit, void *data)
 static void finish_object(struct object *obj, const char *name, void *cb_data)
 {
 	struct rev_list_info *info = cb_data;
-	if (obj->type == OBJ_BLOB && !has_object_file(&obj->oid))
+	if (obj->type == OBJ_BLOB && !has_object_file(&obj->oid)) {
+		if (arg_print_missing) {
+			oidset_insert(&missing_objects, &obj->oid);
+			return;
+		}
+		if (arg_ignore_missing)
+			return;
+
 		die("missing blob object '%s'", oid_to_hex(&obj->oid));
+	}
 	if (info->revs->verify_objects && !obj->parsed && obj->type != OBJ_COMMIT)
 		parse_object(&obj->oid);
 }
@@ -335,6 +355,30 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 			show_progress = arg;
 			continue;
 		}
+
+		if (skip_prefix(arg, ("--" CL_ARG__FILTER "="), &arg)) {
+			parse_list_objects_filter(&filter_options, arg);
+			if (filter_options.choice && !revs.blob_objects)
+				die(_("object filtering requires --objects"));
+			if (filter_options.choice == LOFC_SPARSE_OID &&
+			    !filter_options.sparse_oid_value)
+				die(_("invalid sparse value '%s'"),
+				    filter_options.raw_value);
+			continue;
+		}
+		if (!strcmp(arg, "--filter-print-missing")) {
+			arg_print_missing = 1;
+			continue;
+		}
+		if (!strcmp(arg, "--filter-print-omitted")) {
+			arg_print_omitted = 1;
+			continue;
+		}
+		if (!strcmp(arg, "--filter-ignore-missing")) {
+			arg_ignore_missing = 1;
+			continue;
+		}
+		
 		usage(rev_list_usage);
 
 	}
@@ -360,6 +404,9 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 	if (revs.show_notes)
 		die(_("rev-list does not support display of notes"));
 
+	if (filter_options.choice && use_bitmap_index)
+		die(_("cannot combine --use-bitmap-index with object filtering"));
+
 	save_commit_buffer = (revs.verbose_header ||
 			      revs.grep_filter.pattern_list ||
 			      revs.grep_filter.header_list);
@@ -404,7 +451,31 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 			return show_bisect_vars(&info, reaches, all);
 	}
 
-	traverse_commit_list(&revs, show_commit, show_object, &info);
+	if (arg_print_missing)
+		oidset_init(&missing_objects, DEFAULT_OIDSET_SIZE);
+	if (arg_print_omitted)
+		oidset_init(&omitted_objects, DEFAULT_OIDSET_SIZE);
+
+	traverse_commit_list_filtered(
+		&filter_options, &revs, show_commit, show_object, &info,
+		(arg_print_omitted ? &omitted_objects : NULL));
+
+	if (arg_print_omitted) {
+		struct oidset_iter iter;
+		struct object_id *oid;
+		oidset_iter_init(&omitted_objects, &iter);
+		while ((oid = oidset_iter_next(&iter)))
+			printf("~%s\n", oid_to_hex(oid));
+		oidset_clear(&omitted_objects);
+	}
+	if (arg_print_missing) {
+		struct oidset_iter iter;
+		struct object_id *oid;
+		oidset_iter_init(&missing_objects, &iter);
+		while ((oid = oidset_iter_next(&iter)))
+			printf("?%s\n", oid_to_hex(oid));
+		oidset_clear(&missing_objects);
+	}
 
 	stop_progress(&progress);
 
diff --git a/t/t6112-rev-list-filters-objects.sh b/t/t6112-rev-list-filters-objects.sh
new file mode 100755
index 0000000..9327974
--- /dev/null
+++ b/t/t6112-rev-list-filters-objects.sh
@@ -0,0 +1,225 @@
+#!/bin/sh
+
+test_description='git rev-list with object filtering for partial clone'
+
+. ./test-lib.sh
+
+# Test the blob:none filter.
+
+test_expect_success 'setup r1' '
+	echo "{print \$1}" >print_1.awk &&
+	echo "{print \$2}" >print_2.awk &&
+
+	git init r1 &&
+	for n in 1 2 3 4 5
+	do
+		echo "This is file: $n" > r1/file.$n
+		git -C r1 add file.$n
+		git -C r1 commit -m "$n"
+	done
+'
+
+test_expect_success 'verify blob:none omits all 5 blobs' '
+	git -C r1 ls-files -s file.1 file.2 file.3 file.4 file.5 \
+		| awk -f print_2.awk \
+		| sort >expected &&
+	git -C r1 rev-list HEAD --quiet --objects --filter-print-omitted --filter=blob:none \
+		| awk -f print_1.awk \
+		| sed "s/~//" \
+		| sort >observed &&
+	test_cmp observed expected
+'
+
+test_expect_success 'verify emitted+omitted == all' '
+	git -C r1 rev-list HEAD --objects \
+		| awk -f print_1.awk \
+		| sort >expected &&
+	git -C r1 rev-list HEAD --objects --filter-print-omitted --filter=blob:none \
+		| awk -f print_1.awk \
+		| sed "s/~//" \
+		| sort >observed &&
+	test_cmp observed expected
+'
+
+
+# Test blob:limit=<n>[kmg] filter.
+# We boundary test around the size parameter.  The filter is strictly less than
+# the value, so size 500 and 1000 should have the same results, but 1001 should
+# filter more.
+
+test_expect_success 'setup r2' '
+	git init r2 &&
+	for n in 1000 10000
+	do
+		printf "%"$n"s" X > r2/large.$n
+		git -C r2 add large.$n
+		git -C r2 commit -m "$n"
+	done
+'
+
+test_expect_success 'verify blob:limit=500 omits all blobs' '
+	git -C r2 ls-files -s large.1000 large.10000 \
+		| awk -f print_2.awk \
+		| sort >expected &&
+	git -C r2 rev-list HEAD --quiet --objects --filter-print-omitted --filter=blob:limit=500 \
+		| awk -f print_1.awk \
+		| sed "s/~//" \
+		| sort >observed &&
+	test_cmp observed expected
+'
+
+test_expect_success 'verify emitted+omitted == all' '
+	git -C r2 rev-list HEAD --objects \
+		| awk -f print_1.awk \
+		| sort >expected &&
+	git -C r2 rev-list HEAD --objects --filter-print-omitted --filter=blob:limit=500 \
+		| awk -f print_1.awk \
+		| sed "s/~//" \
+		| sort >observed &&
+	test_cmp observed expected
+'
+
+test_expect_success 'verify blob:limit=1000' '
+	git -C r2 ls-files -s large.1000 large.10000 \
+		| awk -f print_2.awk \
+		| sort >expected &&
+	git -C r2 rev-list HEAD --quiet --objects --filter-print-omitted --filter=blob:limit=1000 \
+		| awk -f print_1.awk \
+		| sed "s/~//" \
+		| sort >observed &&
+	test_cmp observed expected
+'
+
+test_expect_success 'verify blob:limit=1001' '
+	git -C r2 ls-files -s large.10000 \
+		| awk -f print_2.awk \
+		| sort >expected &&
+	git -C r2 rev-list HEAD --quiet --objects --filter-print-omitted --filter=blob:limit=1001 \
+		| awk -f print_1.awk \
+		| sed "s/~//" \
+		| sort >observed &&
+	test_cmp observed expected
+'
+
+test_expect_success 'verify blob:limit=1k' '
+	git -C r2 ls-files -s large.10000 \
+		| awk -f print_2.awk \
+		| sort >expected &&
+	git -C r2 rev-list HEAD --quiet --objects --filter-print-omitted --filter=blob:limit=1k \
+		| awk -f print_1.awk \
+		| sed "s/~//" \
+		| sort >observed &&
+	test_cmp observed expected
+'
+
+test_expect_success 'verify blob:limit=1m' '
+	cat </dev/null >expected &&
+	git -C r2 rev-list HEAD --quiet --objects --filter-print-omitted --filter=blob:limit=1m \
+		| awk -f print_1.awk \
+		| sed "s/~//" \
+		| sort >observed &&
+	test_cmp observed expected
+'
+
+# Test sparse:path=<path> filter.
+# Use a local file containing a sparse-checkout specification to filter
+# out blobs not required for the corresponding sparse-checkout.  We do not
+# require sparse-checkout to actually be enabled.
+
+test_expect_success 'setup r3' '
+	git init r3 &&
+	mkdir r3/dir1 &&
+	for n in sparse1 sparse2
+	do
+		echo "This is file: $n" > r3/$n
+		git -C r3 add $n
+		echo "This is file: dir1/$n" > r3/dir1/$n
+		git -C r3 add dir1/$n
+	done &&
+	git -C r3 commit -m "sparse" &&
+	echo dir1/ >pattern1 &&
+	echo sparse1 >pattern2
+'
+
+test_expect_success 'verify sparse:path=pattern1 omits top-level files' '
+	git -C r3 ls-files -s sparse1 sparse2 \
+		| awk -f print_2.awk \
+		| sort >expected &&
+	git -C r3 rev-list HEAD --quiet --objects --filter-print-omitted --filter=sparse:path=../pattern1 \
+		| awk -f print_1.awk \
+		| sed "s/~//" \
+		| sort >observed &&
+	test_cmp observed expected
+'
+
+test_expect_success 'verify sparse:path=pattern2 omits both sparse2 files' '
+	git -C r3 ls-files -s sparse2 dir1/sparse2 \
+		| awk -f print_2.awk \
+		| sort >expected &&
+	git -C r3 rev-list HEAD --quiet --objects --filter-print-omitted --filter=sparse:path=../pattern2 \
+		| awk -f print_1.awk \
+		| sed "s/~//" \
+		| sort >observed &&
+	test_cmp observed expected
+'
+
+# Test sparse:oid=<oid-ish> filter.
+# Like sparse:path, but we get the sparse-checkout specification from
+# a blob rather than a file on disk.
+
+test_expect_success 'setup r3 part 2' '
+	echo dir1/ >r3/pattern &&
+	git -C r3 add pattern &&
+	git -C r3 commit -m "pattern"
+'
+
+test_expect_success 'verify sparse:oid=OID omits top-level files' '
+	git -C r3 ls-files -s pattern sparse1 sparse2 \
+		| awk -f print_2.awk \
+		| sort >expected &&
+	oid=$(git -C r3 ls-files -s pattern | awk -f print_2.awk) &&
+	git -C r3 rev-list HEAD --quiet --objects --filter-print-omitted --filter=sparse:oid=$oid \
+		| awk -f print_1.awk \
+		| sed "s/~//" \
+		| sort >observed &&
+	test_cmp observed expected
+'
+
+test_expect_success 'verify sparse:oid=oid-ish omits top-level files' '
+	git -C r3 ls-files -s pattern sparse1 sparse2 \
+		| awk -f print_2.awk \
+		| sort >expected &&
+	git -C r3 rev-list HEAD --quiet --objects --filter-print-omitted --filter=sparse:oid=master:pattern \
+		| awk -f print_1.awk \
+		| sed "s/~//" \
+		| sort >observed &&
+	test_cmp observed expected
+'
+
+# Delete some loose objects and use rev-list, but WITHOUT any filtering.
+# This models previously omitted objects that we did not receive.
+
+test_expect_success 'rev-list W/ print-missing' '
+	git -C r1 ls-files -s file.1 file.2 file.3 file.4 file.5 \
+		| awk -f print_2.awk \
+		| sort >expected &&
+	for id in `cat expected | sed "s|..|&/|"`
+	do
+		rm r1/.git/objects/$id
+	done &&
+	git -C r1 rev-list --quiet HEAD --filter-print-missing --objects \
+		| awk -f print_1.awk \
+		| sed "s/?//" \
+		| sort >observed &&
+	test_cmp observed expected
+'
+
+test_expect_success 'rev-list W/O print-missing fails' '
+	test_must_fail git -C r1 rev-list --quiet --objects HEAD
+'
+
+test_expect_success 'rev-list W/ ignore-missing' '
+	git -C r1 rev-list --quiet --filter-ignore-missing --objects HEAD
+'
+
+test_done
-- 
2.9.3

[PATCH v2 6/6] pack-objects: add list-objects filtering

[Jeff Hostetler]

From: Jeff Hostetler <jeffhost@microsoft.com>

Teach pack-objects to use the filtering provided by the
traverse_commit_list_filtered() interface to omit unwanted
objects from the resulting packfile.

This feature is intended for partial clone/fetch.

Filtering requires the use of the "--stdout" option.

Add t5317 test.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 Documentation/git-pack-objects.txt     |  12 +-
 builtin/pack-objects.c                 |  28 ++-
 t/t5317-pack-objects-filter-objects.sh | 369 +++++++++++++++++++++++++++++++++
 3 files changed, 407 insertions(+), 2 deletions(-)
 create mode 100755 t/t5317-pack-objects-filter-objects.sh

diff --git a/Documentation/git-pack-objects.txt b/Documentation/git-pack-objects.txt
index 473a161..6786351 100644
--- a/Documentation/git-pack-objects.txt
+++ b/Documentation/git-pack-objects.txt
@@ -12,7 +12,8 @@ SYNOPSIS
 'git pack-objects' [-q | --progress | --all-progress] [--all-progress-implied]
 	[--no-reuse-delta] [--delta-base-offset] [--non-empty]
 	[--local] [--incremental] [--window=<n>] [--depth=<n>]
-	[--revs [--unpacked | --all]] [--stdout | base-name]
+	[--revs [--unpacked | --all]]
+	[--stdout [--filter=<filter-spec>] | base-name]
 	[--shallow] [--keep-true-parents] < object-list
 
 
@@ -236,6 +237,15 @@ So does `git bundle` (see linkgit:git-bundle[1]) when it creates a bundle.
 	With this option, parents that are hidden by grafts are packed
 	nevertheless.
 
+--filter=<filter-spec>::
+	Requires `--stdout`.  Omits certain objects (usually blobs) from
+	the resulting packfile.  See linkgit:git-rev-list[1] for valid
+	`<filter-spec>` forms.
+
+--filter-ignore-missing:
+	Ignore missing objects without error.  This may be used with
+	or without and of the above filtering.
+
 SEE ALSO
 --------
 linkgit:git-rev-list[1]
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 6e77dfd..e16722f 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -15,6 +15,8 @@
 #include "diff.h"
 #include "revision.h"
 #include "list-objects.h"
+#include "list-objects-filter.h"
+#include "list-objects-filter-options.h"
 #include "pack-objects.h"
 #include "progress.h"
 #include "refs.h"
@@ -79,6 +81,9 @@ static unsigned long cache_max_small_delta_size = 1000;
 
 static unsigned long window_memory_limit = 0;
 
+static struct list_objects_filter_options filter_options;
+static int arg_ignore_missing;
+
 /*
  * stats
  */
@@ -2547,6 +2552,15 @@ static void show_commit(struct commit *commit, void *data)
 
 static void show_object(struct object *obj, const char *name, void *data)
 {
+	/*
+	 * Quietly ignore missing objects when they are expected.  This
+	 * avoids staging them and getting an odd error later.  If we are
+	 * not expecting them, stage it and let the normal error handling
+	 * deal with it.
+	 */
+	if (arg_ignore_missing && !has_object_file(&obj->oid))
+		return;
+
 	add_preferred_base_object(name);
 	add_object_entry(obj->oid.hash, obj->type, name, 0);
 	obj->flags |= OBJECT_ADDED;
@@ -2816,7 +2830,10 @@ static void get_object_list(int ac, const char **av)
 	if (prepare_revision_walk(&revs))
 		die("revision walk setup failed");
 	mark_edges_uninteresting(&revs, show_edge);
-	traverse_commit_list(&revs, show_commit, show_object, NULL);
+
+	traverse_commit_list_filtered(&filter_options, &revs,
+				      show_commit, show_object, NULL,
+				      NULL);
 
 	if (unpack_unreachable_expiration) {
 		revs.ignore_missing_links = 1;
@@ -2952,6 +2969,9 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 			 N_("use a bitmap index if available to speed up counting objects")),
 		OPT_BOOL(0, "write-bitmap-index", &write_bitmap_index,
 			 N_("write a bitmap index together with the pack index")),
+		OPT_PARSE_LIST_OBJECTS_FILTER(&filter_options),
+		OPT_BOOL(0, "filter-ignore-missing", &arg_ignore_missing,
+			 N_("ignore and omit missing objects from packfile")),
 		OPT_END(),
 	};
 
@@ -3028,6 +3048,12 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 	if (!rev_list_all || !rev_list_reflog || !rev_list_index)
 		unpack_unreachable_expiration = 0;
 
+	if (filter_options.choice) {
+		if (!pack_to_stdout)
+			die("cannot use filtering with an indexable pack.");
+		use_bitmap_index = 0;
+	}
+
 	/*
 	 * "soft" reasons not to use bitmaps - for on-disk repack by default we want
 	 *
diff --git a/t/t5317-pack-objects-filter-objects.sh b/t/t5317-pack-objects-filter-objects.sh
new file mode 100755
index 0000000..4249557
--- /dev/null
+++ b/t/t5317-pack-objects-filter-objects.sh
@@ -0,0 +1,369 @@
+#!/bin/sh
+
+test_description='git pack-objects with object filtering for partial clone'
+
+. ./test-lib.sh
+
+# Test blob:none filter.
+
+test_expect_success 'setup r1' '
+	echo "{print \$1}" >print_1.awk &&
+	echo "{print \$2}" >print_2.awk &&
+
+	git init r1 &&
+	for n in 1 2 3 4 5
+	do
+		echo "This is file: $n" > r1/file.$n
+		git -C r1 add file.$n
+		git -C r1 commit -m "$n"
+	done
+'
+
+test_expect_success 'verify blob count in normal packfile' '
+	git -C r1 ls-files -s file.1 file.2 file.3 file.4 file.5 \
+		| awk -f print_2.awk \
+		| sort >expected &&
+	git -C r1 pack-objects --rev --stdout >all.pack <<-EOF &&
+	HEAD
+	EOF
+	git -C r1 index-pack ../all.pack &&
+	git -C r1 verify-pack -v ../all.pack \
+		| grep blob \
+		| awk -f print_1.awk \
+		| sort >observed &&
+	test_cmp observed expected
+'
+
+test_expect_success 'verify blob:none packfile has no blobs' '
+	git -C r1 pack-objects --rev --stdout --filter=blob:none >filter.pack <<-EOF &&
+	HEAD
+	EOF
+	git -C r1 index-pack ../filter.pack &&
+	git -C r1 verify-pack -v ../filter.pack \
+		| grep blob \
+		| awk -f print_1.awk \
+		| sort >observed &&
+	nr=$(wc -l <observed) &&
+	test 0 -eq $nr
+'
+
+test_expect_success 'verify normal and blob:none packfiles have same commits/trees' '
+	git -C r1 verify-pack -v ../all.pack \
+		| grep -E "commit|tree" \
+		| awk -f print_1.awk \
+		| sort >expected &&
+	git -C r1 verify-pack -v ../filter.pack \
+		| grep -E "commit|tree" \
+		| awk -f print_1.awk \
+		| sort >observed &&
+	test_cmp observed expected
+'
+
+# Test blob:limit=<n>[kmg] filter.
+# We boundary test around the size parameter.  The filter is strictly less than
+# the value, so size 500 and 1000 should have the same results, but 1001 should
+# filter more.
+
+test_expect_success 'setup r2' '
+	git init r2 &&
+	for n in 1000 10000
+	do
+		printf "%"$n"s" X > r2/large.$n
+		git -C r2 add large.$n
+		git -C r2 commit -m "$n"
+	done
+'
+
+test_expect_success 'verify blob count in normal packfile' '
+	git -C r2 ls-files -s large.1000 large.10000 \
+		| awk -f print_2.awk \
+		| sort >expected &&
+	git -C r2 pack-objects --rev --stdout >all.pack <<-EOF &&
+	HEAD
+	EOF
+	git -C r2 index-pack ../all.pack &&
+	git -C r2 verify-pack -v ../all.pack \
+		| grep blob \
+		| awk -f print_1.awk \
+		| sort >observed &&
+	test_cmp observed expected
+'
+
+test_expect_success 'verify blob:limit=500 omits all blobs' '
+	git -C r2 pack-objects --rev --stdout --filter=blob:limit=500 >filter.pack <<-EOF &&
+	HEAD
+	EOF
+	git -C r2 index-pack ../filter.pack &&
+	git -C r2 verify-pack -v ../filter.pack \
+		| grep blob \
+		| awk -f print_1.awk \
+		| sort >observed &&
+	nr=$(wc -l <observed) &&
+	test 0 -eq $nr
+'
+
+test_expect_success 'verify blob:limit=1000' '
+	git -C r2 pack-objects --rev --stdout --filter=blob:limit=1000 >filter.pack <<-EOF &&
+	HEAD
+	EOF
+	git -C r2 index-pack ../filter.pack &&
+	git -C r2 verify-pack -v ../filter.pack \
+		| grep blob \
+		| awk -f print_1.awk \
+		| sort >observed &&
+	nr=$(wc -l <observed) &&
+	test 0 -eq $nr
+'
+
+test_expect_success 'verify blob:limit=1001' '
+	git -C r2 ls-files -s large.1000 \
+		| awk -f print_2.awk \
+		| sort >expected &&
+	git -C r2 pack-objects --rev --stdout --filter=blob:limit=1001 >filter.pack <<-EOF &&
+	HEAD
+	EOF
+	git -C r2 index-pack ../filter.pack &&
+	git -C r2 verify-pack -v ../filter.pack \
+		| grep blob \
+		| awk -f print_1.awk \
+		| sort >observed &&
+	test_cmp observed expected
+'
+
+test_expect_success 'verify blob:limit=10001' '
+	git -C r2 ls-files -s large.1000 large.10000 \
+		| awk -f print_2.awk \
+		| sort >expected &&
+	git -C r2 pack-objects --rev --stdout --filter=blob:limit=10001 >filter.pack <<-EOF &&
+	HEAD
+	EOF
+	git -C r2 index-pack ../filter.pack &&
+	git -C r2 verify-pack -v ../filter.pack \
+		| grep blob \
+		| awk -f print_1.awk \
+		| sort >observed &&
+	test_cmp observed expected
+'
+
+test_expect_success 'verify blob:limit=1k' '
+	git -C r2 ls-files -s large.1000 \
+		| awk -f print_2.awk \
+		| sort >expected &&
+	git -C r2 pack-objects --rev --stdout --filter=blob:limit=1k >filter.pack <<-EOF &&
+	HEAD
+	EOF
+	git -C r2 index-pack ../filter.pack &&
+	git -C r2 verify-pack -v ../filter.pack \
+		| grep blob \
+		| awk -f print_1.awk \
+		| sort >observed &&
+	test_cmp observed expected
+'
+
+test_expect_success 'verify blob:limit=1m' '
+	git -C r2 ls-files -s large.1000 large.10000 \
+		| awk -f print_2.awk \
+		| sort >expected &&
+	git -C r2 pack-objects --rev --stdout --filter=blob:limit=1m >filter.pack <<-EOF &&
+	HEAD
+	EOF
+	git -C r2 index-pack ../filter.pack &&
+	git -C r2 verify-pack -v ../filter.pack \
+		| grep blob \
+		| awk -f print_1.awk \
+		| sort >observed &&
+	test_cmp observed expected
+'
+
+test_expect_success 'verify normal and blob:limit packfiles have same commits/trees' '
+	git -C r2 verify-pack -v ../all.pack \
+		| grep -E "commit|tree" \
+		| awk -f print_1.awk \
+		| sort >expected &&
+	git -C r2 verify-pack -v ../filter.pack \
+		| grep -E "commit|tree" \
+		| awk -f print_1.awk \
+		| sort >observed &&
+	test_cmp observed expected
+'
+
+# Test sparse:path=<path> filter.
+# Use a local file containing a sparse-checkout specification to filter
+# out blobs not required for the corresponding sparse-checkout.  We do not
+# require sparse-checkout to actually be enabled.
+
+test_expect_success 'setup r3' '
+	git init r3 &&
+	mkdir r3/dir1 &&
+	for n in sparse1 sparse2
+	do
+		echo "This is file: $n" > r3/$n
+		git -C r3 add $n
+		echo "This is file: dir1/$n" > r3/dir1/$n
+		git -C r3 add dir1/$n
+	done &&
+	git -C r3 commit -m "sparse" &&
+	echo dir1/ >pattern1 &&
+	echo sparse1 >pattern2
+'
+
+test_expect_success 'verify blob count in normal packfile' '
+	git -C r3 ls-files -s sparse1 sparse2 dir1/sparse1 dir1/sparse2 \
+		| awk -f print_2.awk \
+		| sort >expected &&
+	git -C r3 pack-objects --rev --stdout >all.pack <<-EOF &&
+	HEAD
+	EOF
+	git -C r3 index-pack ../all.pack &&
+	git -C r3 verify-pack -v ../all.pack \
+		| grep blob \
+		| awk -f print_1.awk \
+		| sort >observed &&
+	test_cmp observed expected
+'
+
+test_expect_success 'verify sparse:path=pattern1' '
+	git -C r3 ls-files -s dir1/sparse1 dir1/sparse2 \
+		| awk -f print_2.awk \
+		| sort >expected &&
+	git -C r3 pack-objects --rev --stdout --filter=sparse:path=../pattern1 >filter.pack <<-EOF &&
+	HEAD
+	EOF
+	git -C r3 index-pack ../filter.pack &&
+	git -C r3 verify-pack -v ../filter.pack \
+		| grep blob \
+		| awk -f print_1.awk \
+		| sort >observed &&
+	test_cmp observed expected
+'
+
+test_expect_success 'verify normal and sparse:path=pattern1 packfiles have same commits/trees' '
+	git -C r3 verify-pack -v ../all.pack \
+		| grep -E "commit|tree" \
+		| awk -f print_1.awk \
+		| sort >expected &&
+	git -C r3 verify-pack -v ../filter.pack \
+		| grep -E "commit|tree" \
+		| awk -f print_1.awk \
+		| sort >observed &&
+	test_cmp observed expected
+'
+
+test_expect_success 'verify sparse:path=pattern2' '
+	git -C r3 ls-files -s sparse1 dir1/sparse1 \
+		| awk -f print_2.awk \
+		| sort >expected &&
+	git -C r3 pack-objects --rev --stdout --filter=sparse:path=../pattern2 >filter.pack <<-EOF &&
+	HEAD
+	EOF
+	git -C r3 index-pack ../filter.pack &&
+	git -C r3 verify-pack -v ../filter.pack \
+		| grep blob \
+		| awk -f print_1.awk \
+		| sort >observed &&
+	test_cmp observed expected
+'
+
+test_expect_success 'verify normal and sparse:path=pattern2 packfiles have same commits/trees' '
+	git -C r3 verify-pack -v ../all.pack \
+		| grep -E "commit|tree" \
+		| awk -f print_1.awk \
+		| sort >expected &&
+	git -C r3 verify-pack -v ../filter.pack \
+		| grep -E "commit|tree" \
+		| awk -f print_1.awk \
+		| sort >observed &&
+	test_cmp observed expected
+'
+
+# Test sparse:oid=<oid-ish> filter.
+# Like sparse:path, but we get the sparse-checkout specification from
+# a blob rather than a file on disk.
+
+test_expect_success 'setup r4' '
+	git init r4 &&
+	mkdir r4/dir1 &&
+	for n in sparse1 sparse2
+	do
+		echo "This is file: $n" > r4/$n
+		git -C r4 add $n
+		echo "This is file: dir1/$n" > r4/dir1/$n
+		git -C r4 add dir1/$n
+	done &&
+	echo dir1/ >r4/pattern &&
+	git -C r4 add pattern &&
+	git -C r4 commit -m "pattern"
+'
+
+test_expect_success 'verify blob count in normal packfile' '
+	git -C r4 ls-files -s pattern sparse1 sparse2 dir1/sparse1 dir1/sparse2 \
+		| awk -f print_2.awk \
+		| sort >expected &&
+	git -C r4 pack-objects --rev --stdout >all.pack <<-EOF &&
+	HEAD
+	EOF
+	git -C r4 index-pack ../all.pack &&
+	git -C r4 verify-pack -v ../all.pack \
+		| grep blob \
+		| awk -f print_1.awk \
+		| sort >observed &&
+	test_cmp observed expected
+'
+
+test_expect_success 'verify sparse:oid=OID' '
+	git -C r4 ls-files -s dir1/sparse1 dir1/sparse2 \
+		| awk -f print_2.awk \
+		| sort >expected &&
+	oid=$(git -C r4 ls-files -s pattern | awk -f print_2.awk) &&
+	git -C r4 pack-objects --rev --stdout --filter=sparse:oid=$oid >filter.pack <<-EOF &&
+	HEAD
+	EOF
+	git -C r4 index-pack ../filter.pack &&
+	git -C r4 verify-pack -v ../filter.pack \
+		| grep blob \
+		| awk -f print_1.awk \
+		| sort >observed &&
+	test_cmp observed expected
+'
+
+test_expect_success 'verify sparse:oid=oid-ish' '
+	git -C r4 ls-files -s dir1/sparse1 dir1/sparse2 \
+		| awk -f print_2.awk \
+		| sort >expected &&
+	git -C r4 pack-objects --rev --stdout --filter=sparse:oid=master:pattern >filter.pack <<-EOF &&
+	HEAD
+	EOF
+	git -C r4 index-pack ../filter.pack &&
+	git -C r4 verify-pack -v ../filter.pack \
+		| grep blob \
+		| awk -f print_1.awk \
+		| sort >observed &&
+	test_cmp observed expected
+'
+
+# Delete some loose objects and use pack-objects, but WITHOUT any filtering.
+# This models previously omitted objects that we did not receive.
+
+test_expect_success 'setup r1 - delete loose blobs' '
+	git -C r1 ls-files -s file.1 file.2 file.3 file.4 file.5 \
+		| awk -f print_2.awk \
+		| sort >expected &&
+	for id in `cat expected | sed "s|..|&/|"`
+	do
+		rm r1/.git/objects/$id
+	done
+'
+
+test_expect_success 'verify pack-objects fails w/ missing objects' '
+	test_must_fail git -C r1 pack-objects --rev --stdout >miss.pack <<-EOF
+	HEAD
+	EOF
+'
+
+test_expect_success 'verify pack-objects w/ ignore-missing' '
+	git -C r1 pack-objects --rev --stdout --filter-ignore-missing >miss.pack <<-EOF
+	HEAD
+	EOF
+'
+
+test_done
-- 
2.9.3

Re: [PATCH v2 0/6] Partial clone part 1: object filtering

[Jonathan Tan]

On Thu,  2 Nov 2017 17:50:07 +0000
Jeff Hostetler <git@jeffhostetler.com> wrote:

> From: Jeff Hostetler <jeffhost@microsoft.com>
> 
> Here is V2 of the list-object filtering. It replaces [1]
> and reflect a refactoring and simplification of the original.

Thanks, overall this looks quite good. I reviewed patches 2-6 (skipping
1 since it's already in next), made my comments on 4, and don't have any
for the rest (besides what's below).

> I've added "--filter-ignore-missing" parameter to rev-list and
> pack-objects to ignore missing objects rather than error out.
> This allows this patch series to better stand on its own eliminates
> the need in part 1 for "patch 9" from V1.
> 
> This is a brute force ignore all missing objects.  Later, in part
> 2 or part 3 when --exclude-promisor-objects is introduced, we will
> be able to ignore EXPECTED missing objects.

(This is regarding patches 5 and 6.) Is the intention to support both
flags? (That is, --ignore-missing to ignore without checking whether the
object being missing is not unexpected, and --exclude-promisor-objects
to check and ignore.)

Re: [PATCH v2 0/6] Partial clone part 1: object filtering

[Jeff Hostetler]

On 11/2/2017 3:44 PM, Jonathan Tan wrote:
> On Thu,  2 Nov 2017 17:50:07 +0000
> Jeff Hostetler <git@jeffhostetler.com> wrote:
> 
>> From: Jeff Hostetler <jeffhost@microsoft.com>
>>
>> Here is V2 of the list-object filtering. It replaces [1]
>> and reflect a refactoring and simplification of the original.
> 
> Thanks, overall this looks quite good. I reviewed patches 2-6 (skipping
> 1 since it's already in next), made my comments on 4, and don't have any
> for the rest (besides what's below).
> 
>> I've added "--filter-ignore-missing" parameter to rev-list and
>> pack-objects to ignore missing objects rather than error out.
>> This allows this patch series to better stand on its own eliminates
>> the need in part 1 for "patch 9" from V1.
>>
>> This is a brute force ignore all missing objects.  Later, in part
>> 2 or part 3 when --exclude-promisor-objects is introduced, we will
>> be able to ignore EXPECTED missing objects.
> 
> (This is regarding patches 5 and 6.) Is the intention to support both
> flags? (That is, --ignore-missing to ignore without checking whether the
> object being missing is not unexpected, and --exclude-promisor-objects
> to check and ignore.)
> 

Yes, I thought we should have both (perhaps renamed or combined
into 1 parameter with value, such as --exclude=missing vs --exclude=promisor)
and let the user decide how strict they want to be.

Jeff

Re: [PATCH v2 0/6] Partial clone part 1: object filtering

[Junio C Hamano]

Jeff Hostetler <git@jeffhostetler.com> writes:

> Yes, I thought we should have both (perhaps renamed or combined
> into 1 parameter with value, such as --exclude=missing vs --exclude=promisor)
> and let the user decide how strict they want to be.

Assuming we eventually get promisor support working, would there be
any use case where "any missing is OK" mode would be useful in a
sense more reasonable than "because we could have such a mode" and
"it is not our business to prevent users from playing with fire"?

Re: [PATCH v2 0/6] Partial clone part 1: object filtering

[Jeff Hostetler]

On 11/3/2017 11:05 AM, Junio C Hamano wrote:
> Jeff Hostetler <git@jeffhostetler.com> writes:
> 
>> Yes, I thought we should have both (perhaps renamed or combined
>> into 1 parameter with value, such as --exclude=missing vs --exclude=promisor)
>> and let the user decide how strict they want to be.
> 
> Assuming we eventually get promisor support working, would there be
> any use case where "any missing is OK" mode would be useful in a
> sense more reasonable than "because we could have such a mode" and
> "it is not our business to prevent users from playing with fire"?
> 

For now, I'd like to keep my "any missing is OK" option.
I do think it has value all by itself.

We are essentially using something like that now with our GVFS
users on the gigantic Windows repo and haven't had any issues.

But yes, when we get promisor support working, we could revisit
the need for this parameter.

However, I do have some scaling concerns here.  If for example,
I have 100M missing blobs (because we did an only commits-and-trees
clone), the cost to compute "promisor missing" vs "just missing"
might be prohibitively expensive.  It could be something we want
fsck/gc to be aware of, but other commands may want to just assume
any missing objects are expected and continue.

Hopefully, we won't have a scale problem, but we just don't know
yet.

Jeff

Re: [PATCH v2 0/6] Partial clone part 1: object filtering

[Jonathan Tan]

On Fri, 3 Nov 2017 14:34:39 -0400
Jeff Hostetler <git@jeffhostetler.com> wrote:

> > Assuming we eventually get promisor support working, would there be
> > any use case where "any missing is OK" mode would be useful in a
> > sense more reasonable than "because we could have such a mode" and
> > "it is not our business to prevent users from playing with fire"?
> > 
> 
> For now, I'd like to keep my "any missing is OK" option.
> I do think it has value all by itself.
> 
> We are essentially using something like that now with our GVFS
> users on the gigantic Windows repo and haven't had any issues.
> 
> But yes, when we get promisor support working, we could revisit
> the need for this parameter.

Well, it's probably not a good idea to include this parameter, and then
subsequently remove it.

> However, I do have some scaling concerns here.  If for example,
> I have 100M missing blobs (because we did an only commits-and-trees
> clone), the cost to compute "promisor missing" vs "just missing"
> might be prohibitively expensive.  It could be something we want
> fsck/gc to be aware of, but other commands may want to just assume
> any missing objects are expected and continue.
> 
> Hopefully, we won't have a scale problem, but we just don't know
> yet.

I can see some use for this parameter - for example, when doing a report
for statistical purposes (percentage of objects missing, for example) or
for a background task that downloads missing objects into a cache. Also,
power users who know what they're doing (or normal users in an
emergency) can use this option when they have no network connection if
they really need to find something out from the local repo.

In these cases, the promisor check (after detecting that the object is
missing) is indeed not so useful, I think. (Or we can do the
--exclude=missing and --exclude=promisor idea that Jeff mentioned -
--exclude=missing now, and --exclude=promisor after we add promisor
support.)

This is conceptually different from gc's use of
--exclude-promisor-objects (in my patch set), which does not intend to
touch promisor objects (objects that are known to be in the promisor
remote), whether they are present or not.

Having said that, I would be OK if we didn't have tolerance (and/or
reporting) of missing objects right now. As far as I know, for the
initial implementation of partial clone, only the server performs any
filtering, and we assume that the server possesses all objects (so it
does not need to filter out any missing objects).

Re: [PATCH v2 0/6] Partial clone part 1: object filtering

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

> I can see some use for this parameter - for example, when doing a report
> for statistical purposes (percentage of objects missing, for example) or
> for a background task that downloads missing objects into a cache. Also,
> power users who know what they're doing (or normal users in an
> emergency) can use this option when they have no network connection if
> they really need to find something out from the local repo.
>
> In these cases, the promisor check (after detecting that the object is
> missing) is indeed not so useful, I think. (Or we can do the
> --exclude=missing and --exclude=promisor idea that Jeff mentioned -
> --exclude=missing now, and --exclude=promisor after we add promisor
> support.)

This sounds like a reasonable thing to have in the endgame state to
me.

> Having said that, I would be OK if we didn't have tolerance (and/or
> reporting) of missing objects right now. As far as I know, for the
> initial implementation of partial clone, only the server performs any
> filtering, and we assume that the server possesses all objects (so it
> does not need to filter out any missing objects).

True.  It does not have to exist in an early part, but I do not
think we would terribly mind if it does, if only to help debugging
and development.

Thanks for thinking it through.

Re: [PATCH v2 0/6] Partial clone part 1: object filtering

[Jeff Hostetler]

On 11/7/2017 7:54 PM, Junio C Hamano wrote:
> Jonathan Tan <jonathantanmy@google.com> writes:
> 
>> I can see some use for this parameter - for example, when doing a report
>> for statistical purposes (percentage of objects missing, for example) or
>> for a background task that downloads missing objects into a cache. Also,
>> power users who know what they're doing (or normal users in an
>> emergency) can use this option when they have no network connection if
>> they really need to find something out from the local repo.
>>
>> In these cases, the promisor check (after detecting that the object is
>> missing) is indeed not so useful, I think. (Or we can do the
>> --exclude=missing and --exclude=promisor idea that Jeff mentioned -
>> --exclude=missing now, and --exclude=promisor after we add promisor
>> support.)
> 
> This sounds like a reasonable thing to have in the endgame state to
> me.

OK thanks, I'll change it to --exclude=missing in my next version.
  
> 
>> Having said that, I would be OK if we didn't have tolerance (and/or
>> reporting) of missing objects right now. As far as I know, for the
>> initial implementation of partial clone, only the server performs any
>> filtering, and we assume that the server possesses all objects (so it
>> does not need to filter out any missing objects).
> 
> True.  It does not have to exist in an early part, but I do not
> think we would terribly mind if it does, if only to help debugging
> and development.
> 
> Thanks for thinking it through.
> 

Right, it could come later, but having it here in part 1 as part
of the initial series completes the pre-promisor portion of these
commands.  Having a print-missing option lets rev-list -- as is --
be used in a bulk-fetch-object pre-checkout hook or as part of a
"give me what I need before I go offline" command.  This is useful
by itself.  It does augment the dynamic fetch-object code added in
part 2 and the unpack-trees changes in part 3 to call fetch-object.

Jeff