Skip to contentSkip to conversation

Partial clone part 1: object filtering

PATCH Superseded Jeff Hostetler 4 participants 16 messages Nov 16, 2017 – Nov 17, 2017

Subsystems (2)

v4 of 7 Superseded Latest is v7 →

lore
Share & tools
lore
#0 lore raw

v4 cover Partial clone part 1: object filtering

From: Jeff Hostetler <[email protected]>

Here is V4 of the list-object filtering, rev-list, and pack-objects.

This version addresses comments on the V3 version series.

This version replaces the code to scan and reject the filter-spec for injection characters with a new hex-encoding technique. The purpose of this is only to guard against injection attacks containing characters like semicolon, quotes, spaces, and etc. when a filter-spec is handed to a subordinate command. It does not eliminate the need for the recipient to validate the contents.

This version also combines the various command line flags for handling missing objects into a single --missing={error,print,allow-any} flag.

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         |   4 +-
 Documentation/rev-list-options.txt     |  37 +++
 Makefile                               |   2 +
 builtin/pack-objects.c                 |  64 +++++-
 builtin/rev-list.c                     | 108 ++++++++-
 dir.c                                  | 132 ++++++++---
 dir.h                                  |   3 +
 list-objects-filter-options.c          | 149 ++++++++++++
 list-objects-filter-options.h          |  57 +++++
 list-objects-filter.c                  | 401 +++++++++++++++++++++++++++++++++
 list-objects-filter.h                  |  77 +++++++
 list-objects.c                         |  95 ++++++--
 list-objects.h                         |  13 +-
 object.h                               |   1 +
 oidmap.h                               |  22 ++
 oidset.c                               |  10 +
 oidset.h                               |  36 +++
 t/t5317-pack-objects-filter-objects.sh | 375 ++++++++++++++++++++++++++++++
 t/t6112-rev-list-filters-objects.sh    | 225 ++++++++++++++++++
 20 files changed, 1770 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
Signature · 1 line
2.9.3
#1 lore raw

PATCH v4 1/6 dir: allow exclusions from blob in addition to file

From: Jeff Hostetler <[email protected]>

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.

---
 dir.c | 132 ++++++++++++++++++++++++++++++++++++++++++++++++++----------------
 dir.h |   3 ++
 2 files changed, 104 insertions(+), 31 deletions(-)
dir.c +101 -31
index 1d17b80..1962374 100644
@@ -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)
 {
dir.h +3 -0
index e371705..1bcf391 100644
@@ -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);
Signature · 1 line
2.9.3
#2 lore raw

PATCH v4 2/6 oidmap: add oidmap iterator methods

In reply to Jeff Hostetler

From: Jeff Hostetler <[email protected]>

Add the usual map iterator functions to oidmap.

---
 oidmap.h | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)
oidmap.h +22 -0
index 18f54cd..d3cd2bb 100644
@@ -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
Signature · 1 line
2.9.3
#3 lore raw

PATCH v4 3/6 oidset: add iterator methods to oidset

In reply to Jeff Hostetler

From: Jeff Hostetler <[email protected]>

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

---
 oidset.c | 10 ++++++++++
 oidset.h | 36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+)
oidset.c +10 -0
index f1f874a..454c54f 100644
@@ -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);
oidset.h +36 -0
index f4c9e0f..783abce 100644
@@ -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 */
Signature · 1 line
2.9.3
#4 lore raw

PATCH v4 4/6 list-objects: filter objects in traverse_commit_list

In reply to Jeff Hostetler

From: Jeff Hostetler <[email protected]>

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. It should be possible to extend such support in the future (at least to simple filters that do not require object pathnames), but that is beyond the scope of this patch series.

---
 Makefile                      |   2 +
 list-objects-filter-options.c | 149 ++++++++++++++++
 list-objects-filter-options.h |  57 ++++++
 list-objects-filter.c         | 401 ++++++++++++++++++++++++++++++++++++++++++
 list-objects-filter.h         |  77 ++++++++
 list-objects.c                |  95 ++++++++--
 list-objects.h                |  13 +-
 object.h                      |   1 +
 8 files changed, 778 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
Makefile +2 -0
index cd75985..ca378a4 100644
@@ -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
list-objects-filter-options.c new file +149 -0

149 lines not on this page · raw message

list-objects-filter-options.h new file +57 -0

57 lines not on this page · raw message

list-objects-filter.c new file +401 -0

401 lines not on this page · raw message

list-objects-filter.h new file +77 -0

77 lines not on this page · raw message

list-objects.c +79 -16

182 lines not on this page · raw message

list-objects.h +12 -1
index 0cebf85..aa618d7 100644
@@ -8,4 +8,15 @@ 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
+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);
+
+#endif /* LIST_OBJECTS_H */
object.h +1 -0
index df8abe9..f34461d 100644
@@ -38,6 +38,7 @@ struct object_array {
  * http-push.c:                            16-----19
  * commit.c:                               16-----19
  * sha1_name.c:                                     20
+ * list-objects-filter.c:                             21
  * builtin/fsck.c:  0--3
  */
 #define FLAG_BITS  27
Signature · 1 line
2.9.3
#5 lore raw

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

list-objects-filter-options.c line 18

Quoted 9 lines · On Thu, 16 Nov 2017 18:07:41 +0000 Jeff Hostetler <[email protected]> wrote:
list-objects-filter-options.c new file +9 -0
+/*
+ * Return 1 if the given string needs armoring because of "special"
+ * characters that may cause injection problems when a command passes
+ * the argument to a subordinate command (such as when upload-pack
+ * launches pack-objects).
+ *
+ * The usual alphanumeric and key punctuation do not trigger it.
+ */ 
+static int arg_needs_armor(const char *arg)

First of all, about the injection problem, replying to your previous e-mail [1]:

https://public-inbox.org/git/[email protected]/

Quoted 4 lines

I couldn't use quote.[ch] because it is more concerned with quoting pathnames because of LF and CR characters within them -- rather than semicolons and quotes and the like which I was concerned about.

sq_quote_buf() (or one of the other similarly-named functions) should solve this problem, right? The single quotes around the argument takes care of LF, CR, and semicolons, and things like backslashes and quotes are taken care of as documented.

I don't think we need to invent another encoding to solve this.

list-objects-filter-options.c line 30

Quoted 12 lines
list-objects-filter-options.c new file +12 -0
+{
+	const unsigned char *p;
+
+	for (p = (const unsigned char *)arg; *p; p++) {
+		if (*p >= 'a' && *p <= 'z')
+			continue;
+		if (*p >= 'A' && *p <= 'Z')
+			continue;
+		if (*p >= '0' && *p <= '9')
+			continue;
+		if (*p == '-' || *p == '_' || *p == '.' || *p == '/')
+			continue;

If we do take this approach, can ':' also be included?

Quoted 7 lines
list-objects-filter-options.c new file +7 -0
+	if (skip_prefix(arg, "sparse:oid=", &v0)) {
+		struct object_context oc;
+		struct object_id sparse_oid;
+
+		/*
+		 * Try to parse <oid-expression> into an OID for the current
+		 * command, but DO NOT complain if we don't have the blob or

list-objects-filter-options.c line 116

Quoted 10 lines
list-objects-filter-options.c new file +10 -0
+		 * ref locally.
+		 */
+		if (!get_oid_with_context(v0, GET_OID_BLOB,
+					  &sparse_oid, &oc))
+			filter_options->sparse_oid_value = oiddup(&sparse_oid);
+		filter_options->choice = LOFC_SPARSE_OID;
+		if (arg_needs_armor(v0))
+			filter_options->requires_armor = v0 - arg;
+		return 0;
+	}

In your previous e-mail, you mentioned:

Quoted 4 lines

yes. I always pass filter_options.raw_value over the wire. The code above tries to parse it and put it in an OID for private use by the current process -- just like the size limit value in the blob:limit filter.

So I think this function should complain if you don't have the blob or ref locally. (I envision that if a filter string is to be directly sent to a server, it should be stored as a string, not processed by this function first.)

#6 lore raw

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

Quoted 23 lines · Jonathan Tan

list-objects-filter-options.c line 18

Quoted 9 lines · On Thu, 16 Nov 2017 18:07:41 +0000 Jeff Hostetler <[email protected]> wrote:
list-objects-filter-options.c new file +9 -0
+/*
+ * Return 1 if the given string needs armoring because of "special"
+ * characters that may cause injection problems when a command passes
+ * the argument to a subordinate command (such as when upload-pack
+ * launches pack-objects).
+ *
+ * The usual alphanumeric and key punctuation do not trigger it.
+ */
+static int arg_needs_armor(const char *arg)

First of all, about the injection problem, replying to your previous e-mail [1]:

https://public-inbox.org/git/[email protected]/

Quoted 4 lines

I couldn't use quote.[ch] because it is more concerned with quoting pathnames because of LF and CR characters within them -- rather than semicolons and quotes and the like which I was concerned about.

Quoted 6 lines

sq_quote_buf() (or one of the other similarly-named functions) should solve this problem, right? The single quotes around the argument takes care of LF, CR, and semicolons, and things like backslashes and quotes are taken care of as documented.

I don't think we need to invent another encoding to solve this.

I'll take another look, sq_quote_buf() looks like it might work. I was looking at quote_c_style() and that didn't seem right for my needs. Thanks.

Quoted 15 lines

list-objects-filter-options.c line 30

Quoted 13 lines
list-objects-filter-options.c new file +12 -0
+{
+	const unsigned char *p;
+
+	for (p = (const unsigned char *)arg; *p; p++) {
+		if (*p >= 'a' && *p <= 'z')
+			continue;
+		if (*p >= 'A' && *p <= 'Z')
+			continue;
+		if (*p >= '0' && *p <= '9')
+			continue;
+		if (*p == '-' || *p == '_' || *p == '.' || *p == '/')
+			continue;

If we do take this approach, can ':' also be included?

Sure. I just picked the common ones.

Quoted 26 lines
Quoted 8 lines
list-objects-filter-options.c new file +7 -0
+	if (skip_prefix(arg, "sparse:oid=", &v0)) {
+		struct object_context oc;
+		struct object_id sparse_oid;
+
+		/*
+		 * Try to parse <oid-expression> into an OID for the current
+		 * command, but DO NOT complain if we don't have the blob or

list-objects-filter-options.c line 116

Quoted 10 lines
list-objects-filter-options.c new file +10 -0
+		 * ref locally.
+		 */
+		if (!get_oid_with_context(v0, GET_OID_BLOB,
+					  &sparse_oid, &oc))
+			filter_options->sparse_oid_value = oiddup(&sparse_oid);
+		filter_options->choice = LOFC_SPARSE_OID;
+		if (arg_needs_armor(v0))
+			filter_options->requires_armor = v0 - arg;
+		return 0;
+	}

In your previous e-mail, you mentioned:

Quoted 4 lines

yes. I always pass filter_options.raw_value over the wire. The code above tries to parse it and put it in an OID for private use by the current process -- just like the size limit value in the blob:limit filter.

Quoted 4 lines

So I think this function should complain if you don't have the blob or ref locally. (I envision that if a filter string is to be directly sent to a server, it should be stored as a string, not processed by this function first.)

The whole point was for clone to be able to ask the server to use a known sparse-checkout spec, based on a branch name and a pathname within the tree. That's more usable. My expectation was that users could keep one or more sparse-checkout specs in the tree based upon the area/subset of the tree they want to work on. Then do a partial clone based upon the desired subset. Granted, we make them publish OIDs (outside of git) and then make the user request the partial clone by OID, but that's awkward.

There are 2 uses of the "--filter=..." parser -- one to actually use the data within the current process (rev-list and pack-objects) and one to capture the value and pass it on to another process (fetch-pack and upload-pack). The former want the fields fully decoded into actual variables and the latter want the raw string to pass on. The parser routine provides both values for its callers. (In an earlier draft, the parser would replace the raw_value of a sparse spec with the actual OID when it had it locally, but I took that out based on your earlier comments. Now, the raw_value is always passed by the second type of usages.)

Jeff

#7 lore raw

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

Quoted 15 lines · Jeff Hostetler
Quoted 15 lines

First of all, about the injection problem, replying to your previous e-mail [1]:

https://public-inbox.org/git/[email protected]/

Quoted 4 lines

I couldn't use quote.[ch] because it is more concerned with quoting pathnames because of LF and CR characters within them -- rather than semicolons and quotes and the like which I was concerned about.

sq_quote_buf() (or one of the other similarly-named functions) should solve this problem, right? The single quotes around the argument takes care of LF, CR, and semicolons, and things like backslashes and quotes are taken care of as documented.

Quoted 5 lines
Quoted 1 line

I don't think we need to invent another encoding to solve this.

I'll take another look, sq_quote_buf() looks like it might work. I was looking at quote_c_style() and that didn't seem right for my needs. Thanks.

I admit I haven't been following this thread closely, but I couldn't seem to find any indication of exactly which interfaces need quoting, or who is expected to unquote (here or in the previous iterations).

It sounds like you're worried about shell injection, but shouldn't we worry about that the actual shell boundary? Likewise, if these values are being passed over the git protocol, shouldn't that part of the protocol be designed to encode arbitrary bytes?

Those encodings don't necessarily need to be the same, because they're about transport. Inside each process we'd have the raw bytes, and encode them as appropriate to whatever sub-program we're going to pass to (or not at all if we skip the shell for sub-processes, which is usually a good idea).

I have the feeling I'm missing something.

-Peff

#8 lore raw

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

Quoted 5 lines · Jeff King

Those encodings don't necessarily need to be the same, because they're about transport. Inside each process we'd have the raw bytes, and encode them as appropriate to whatever sub-program we're going to pass to (or not at all if we skip the shell for sub-processes, which is usually a good idea).

Yes, I share the same feeling. It does not help that the series defines its own notion of arg_needs_armor() and uses it to set a field called requires_armor that is not yet used, the definition of "armor"ing being each byte getting encoded as two hexadecimal digits without any sign (which makes me wonder what a receiver of "deadbeef" would do---did it receive an armored string or a plain one???). I do not understand why these strings are not passed as opaque sequences of bytes and instead converted at this low a layer.

#9 lore raw

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

Quoted 8 lines · Junio C Hamano
Quoted 5 lines · Jeff King

Those encodings don't necessarily need to be the same, because they're about transport. Inside each process we'd have the raw bytes, and encode them as appropriate to whatever sub-program we're going to pass to (or not at all if we skip the shell for sub-processes, which is usually a good idea).

Quoted 8 lines

Yes, I share the same feeling. It does not help that the series defines its own notion of arg_needs_armor() and uses it to set a field called requires_armor that is not yet used, the definition of "armor"ing being each byte getting encoded as two hexadecimal digits without any sign (which makes me wonder what a receiver of "deadbeef" would do---did it receive an armored string or a plain one???). I do not understand why these strings are not passed as opaque sequences of bytes and instead converted at this low a layer.

I'm probably being too paranoid. My fear is that a client could pass an expression to clone/fetch/fetch-pack that would be sent to the server and evaluated by the interface between upload-pack and pack-objects. I'm not worried about the pack-protocol transport. I'm mainly concerned in how upload-pack passes that *client-expression* to pack-objects and are there ways for that to go south on the server with a carefully crafted expression.

Even if we assume that upload-pack on the server directly invokes pack-objects (rather than a shell), there still might be issues. For platforms like Linux which have a native execve() and can pass args in an argv (and which the sub-process also receives in an argv in their main()), my paranoia is probably overkill.

But on Windows, where the native interface takes a command-line string rather than an argv, I was concerned. Yes, there is code in compat/mingw.c to quote args when building a command line from an argv (and I'm *not* saying there are bugs in that), but again maybe I am being paranoid.

I'll take another look and the existing quoting mechanisms and re-eval.

Thanks,
Jeff

#10 lore raw

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

Quoted 9 lines · Jeff Hostetler
Quoted 8 lines

Yes, I share the same feeling. It does not help that the series defines its own notion of arg_needs_armor() and uses it to set a field called requires_armor that is not yet used, the definition of "armor"ing being each byte getting encoded as two hexadecimal digits without any sign (which makes me wonder what a receiver of "deadbeef" would do---did it receive an armored string or a plain one???). I do not understand why these strings are not passed as opaque sequences of bytes and instead converted at this low a layer.

Quoted 7 lines

I'm probably being too paranoid. My fear is that a client could pass an expression to clone/fetch/fetch-pack that would be sent to the server and evaluated by the interface between upload-pack and pack-objects. I'm not worried about the pack-protocol transport. I'm mainly concerned in how upload-pack passes that *client-expression* to pack-objects and are there ways for that to go south on the server with a carefully crafted expression.

I think you have to trust that those interfaces are capable of passing raw bytes, whether directly via execve() or because we got the quoting right. If there's a bug there, it's going to be a bigger problem than just this code path (and the fix needs to be there, not second-guessing it in the callers).

So I'd say that yeah, you are being too paranoid.

As an aside, though, I wonder if these client expressions should be fed over stdin to pack-objects. That removes any argv limits we might run into on the server side. It also removes shell injections as a possibility, though of course we'd need quoting in that layer to avoid argument-injection to pack-objects.

-Peff

#11 lore raw

PATCH v4 5/6 rev-list: add list-objects filtering support

In reply to Jeff Hostetler

From: Jeff Hostetler <[email protected]>

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.

Added "--missing=(error|print|omit)" argument to specify how rev-list should behave when it encounters a missing object (presumably from a prior partial clone).

When "--missing=print" is used, rev-list will print a list of any missing objects that should have been included in the output. These are marked with a "?".

Add t6112 test.

---
 Documentation/git-rev-list.txt      |   4 +-
 Documentation/rev-list-options.txt  |  37 ++++++
 builtin/rev-list.c                  | 108 ++++++++++++++++-
 t/t6112-rev-list-filters-objects.sh | 225 ++++++++++++++++++++++++++++++++++++
 4 files changed, 371 insertions(+), 3 deletions(-)
 create mode 100755 t/t6112-rev-list-filters-objects.sh
Documentation/git-rev-list.txt +3 -1
index ef22f17..397a0dd 100644
@@ -47,7 +47,9 @@ SYNOPSIS
 	     [ --fixed-strings | -F ]
 	     [ --date=<format>]
 	     [ [ --objects | --objects-edge | --objects-edge-aggressive ]
-	       [ --unpacked ] ]
+	       [ --unpacked ]
+	       [ --filter=<filter-spec> [ --filter-print-omitted ] ] ]
+	     [ --missing=(error|allow-any|print) ]
 	     [ --pretty | --header ]
 	     [ --bisect ]
 	     [ --bisect-vars ]
Documentation/rev-list-options.txt +37 -0
index 13501e1..c84e465 100644
@@ -706,6 +706,43 @@ 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-omitted::
+	Only useful with `--filter=`; prints a list of the omitted objects.
+	Object IDs are prefixed with a ``~'' character.
+
+--missing=(error|allow-any|print)::
+	Specifies how missing objects are handled.  The repository may
+	have missing objects after a partial clone, for example.
++
+The value 'error' requests that rev-list stop with an error if a missing
+object is encountered.  This is the default action.
++
+The value 'allow-any' will allow object traversal to continue if a
+missing object is encountered.  Missing objects will silently be omitted
+from the results.
++
+The value 'print' is like 'allow-any', but will also print a list of the
+missing objects.  Object IDs are prefixed with a ``?'' character.
 endif::git-rev-list[]
 
 --no-walk[=(sorted|unsorted)]::
builtin/rev-list.c +106 -2

160 lines not on this page · raw message

t/t6112-rev-list-filters-objects.sh new file +225 -0

225 lines not on this page · raw message

Signature · 1 line
2.9.3
#12 lore raw

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

Quoted 19 lines · On Thu, 16 Nov 2017 18:07:42 +0000 Jeff Hostetler <[email protected]> wrote:

From: Jeff Hostetler <[email protected]>

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.

Added "--missing=(error|print|omit)" argument to specify how rev-list should behave when it encounters a missing object (presumably from a prior partial clone).

Quoted 5 lines

When "--missing=print" is used, rev-list will print a list of any missing objects that should have been included in the output. These are marked with a "?".

Add t6112 test.

The patch itself looks good, except that I have a nagging feeling about the usage of the term "partial clone" in the commit message, documentation, and test description. I feel that the usage here leads one to believe that partial clones haphazardly leave repositories without random objects (and at the point that this patch is merged, there will not be any patch in the main repo contradicting this viewpoint), contrary to the fact that we will have a tracking mechanism to track which missing objects are expected to be missing. (If I'm the only one feeling this way, though, then I'll just let it slide.)

If it were up to me, I would remove all existing mentions of "partial clone" and explain the presence of the "--missing" argument as follows:

In the future, we will introduce a "partial clone" mechanism wherein an object in a repo, obtained from a remote, may reference a missing object that can be dynamically fetched from that remote once needed. This "partial clone" mechanism will have a way, sometimes slow, of determining if a missing link is one of the links expected to be produced by this mechanism.

This patch introduces handling of missing objects to help debugging and development of the "partial clone" mechanism, and once the mechanism is implemented, for a power user to perform operations that are missing-object-aware without incurring the cost of checking if a missing link is expected.

#13 lore raw

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

Quoted 10 lines · Jonathan Tan

If it were up to me, I would remove all existing mentions of "partial clone" and explain the presence of the "--missing" argument as follows:

In the future, we will introduce a "partial clone" mechanism wherein an object in a repo, obtained from a remote, may reference a missing object that can be dynamically fetched from that remote once needed. This "partial clone" mechanism will have a way, sometimes slow, of determining if a missing link is one of the links expected to be produced by this mechanism.

Quoted 5 lines

This patch introduces handling of missing objects to help debugging and development of the "partial clone" mechanism, and once the mechanism is implemented, for a power user to perform operations that are missing-object-aware without incurring the cost of checking if a missing link is expected.

That sounds quite sensible.

#14 lore raw

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

Quoted 12 lines · Junio C Hamano
Quoted 10 lines · Jonathan Tan

If it were up to me, I would remove all existing mentions of "partial clone" and explain the presence of the "--missing" argument as follows:

In the future, we will introduce a "partial clone" mechanism wherein an object in a repo, obtained from a remote, may reference a missing object that can be dynamically fetched from that remote once needed. This "partial clone" mechanism will have a way, sometimes slow, of determining if a missing link is one of the links expected to be produced by this mechanism.

Quoted 8 lines
Quoted 5 lines

This patch introduces handling of missing objects to help debugging and development of the "partial clone" mechanism, and once the mechanism is implemented, for a power user to perform operations that are missing-object-aware without incurring the cost of checking if a missing link is expected.

That sounds quite sensible.

will do. thanks.
Jeff

#15 lore raw

PATCH v4 6/6 pack-objects: add list-objects filtering

In reply to Jeff Hostetler

From: Jeff Hostetler <[email protected]>

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.

---
 Documentation/git-pack-objects.txt     |  12 +-
 builtin/pack-objects.c                 |  64 +++++-
 t/t5317-pack-objects-filter-objects.sh | 375 +++++++++++++++++++++++++++++++++
 3 files changed, 449 insertions(+), 2 deletions(-)
 create mode 100755 t/t5317-pack-objects-filter-objects.sh
Documentation/git-pack-objects.txt +11 -1
index 473a161..5fad696 100644
@@ -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.
+
+--missing=(error|allow-any):
+	Specifies how missing objects are handled.  This is useful, for
+	example, when there are missing objects from a prior partial clone.
+
 SEE ALSO
 --------
 linkgit:git-rev-list[1]
builtin/pack-objects.c +63 -1

100 lines not on this page · raw message

t/t5317-pack-objects-filter-objects.sh new file +375 -0

375 lines not on this page · raw message

Signature · 1 line
2.9.3