list-objects-filter: delay parsing of sparse oid

The list-objects-filter code has two steps to its initialization: 1. parse_list_objects_filter() makes sure the spec is a filter we know about and is syntactically correct. This step is done by "rev-list" or "upload-pack" that is going to apply a filter, but also by "git clone" or "git fetch" before they send the spec across the wire. 2. list_objects_filter__init() runs the type-specific initialization (using function pointers established in step 1). This happens at the start of traverse_commit_list_filtered(), when we're about to actually use the filter. It's a good idea to parse as much as we can in step 1, in order to catch problems early (e.g., a blob size limit that isn't a number). But one thing we _shouldn't_ do is resolve any oids at that step (e.g., for sparse-file contents specified by oid). In the case of a fetch, the oid has to be resolved on the remote side. The current code does resolve the oid during the parse phase, but ignores any error (which we must do, because we might just be sending the spec across the wire). This leads to two bugs: - if we're not in a repository (e.g., because it's git-clone parsing the spec), then we trigger a BUG() trying to resolve the name - if we did hit the error case, we still have to notice that later and bail. The code path in rev-list handles this, but the one in upload-pack does not, leading to a segfault. We can fix both by moving the oid resolution into the sparse-oid init function. At that point we know we have a repository (because we're about to traverse), and handling the error there fixes the segfault. As a bonus, we can drop the NULL sparse_oid_value check in rev-list, since this is now handled in the sparse-oid-filter init function. Signed-off-by: Jeff King <peff@peff.net> Acked-by: Jeff Hostetler <jeffhost@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jeff King committed Sep 15, 2019 at 12:12 UTC 4c96a775945d0299e39b982ab9cb32c5132e877d
5 files changed +14 -21
builtin/rev-list.c
-4
@@ -471,10 +471,6 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
471 parse_list_objects_filter(&filter_options, arg);
472 if (filter_options.choice && !revs.blob_objects)
473 die(_("object filtering requires --objects"));
474 - if (filter_options.choice == LOFC_SPARSE_OID &&
475 - !filter_options.sparse_oid_value)
476 - die(_("invalid sparse value '%s'"),
477 - filter_options.filter_spec);
474 continue;
475 }
476 if (!strcmp(arg, ("--no-" CL_ARG__FILTER))) {
list-objects-filter-options.c
+2 -12
@@ -63,17 +63,7 @@ static int gently_parse_list_objects_filter(
63 return 0;
64
65 } else if (skip_prefix(arg, "sparse:oid=", &v0)) {
66 - struct object_context oc;
67 - struct object_id sparse_oid;
68 -
69 - /*
70 - * Try to parse <oid-expression> into an OID for the current
71 - * command, but DO NOT complain if we don't have the blob or
72 - * ref locally.
73 - */
74 - if (!get_oid_with_context(the_repository, v0, GET_OID_BLOB,
75 - &sparse_oid, &oc))
76 - filter_options->sparse_oid_value = oiddup(&sparse_oid);
66 + filter_options->sparse_oid_name = xstrdup(v0);
67 filter_options->choice = LOFC_SPARSE_OID;
68 return 0;
69
@@ -138,7 +128,7 @@ void list_objects_filter_release(
128 struct list_objects_filter_options *filter_options)
129 {
130 free(filter_options->filter_spec);
141 - free(filter_options->sparse_oid_value);
131 + free(filter_options->sparse_oid_name);
132 memset(filter_options, 0, sizeof(*filter_options));
133 }
134
list-objects-filter-options.h
+1 -1
@@ -42,7 +42,7 @@ struct list_objects_filter_options {
42 * choice-specific; not all values will be defined for any given
43 * choice.
44 */
45 - struct object_id *sparse_oid_value;
45 + char *sparse_oid_name;
46 unsigned long blob_limit_value;
47 unsigned long tree_exclude_depth;
48 };
list-objects-filter.c
+9 -2
@@ -463,9 +463,16 @@ static void *filter_sparse_oid__init(
463 filter_free_fn *filter_free_fn)
464 {
465 struct filter_sparse_data *d = xcalloc(1, sizeof(*d));
466 + struct object_context oc;
467 + struct object_id sparse_oid;
468 +
469 + if (get_oid_with_context(the_repository,
470 + filter_options->sparse_oid_name,
471 + GET_OID_BLOB, &sparse_oid, &oc))
472 + die("unable to access sparse blob in '%s'",
473 + filter_options->sparse_oid_name);
474 d->omits = omitted;
467 - if (add_excludes_from_blob_to_list(filter_options->sparse_oid_value,
468 - NULL, 0, &d->el) < 0)
475 + if (add_excludes_from_blob_to_list(&sparse_oid, NULL, 0, &d->el) < 0)
476 die("could not load filter specification");
477
478 ALLOC_GROW(d->array_frame, d->nr + 1, d->alloc);
t/t5616-partial-clone.sh
+2 -2
@@ -252,7 +252,7 @@ test_expect_success 'setup src repo for sparse filter' '
252 git -C sparse-src commit -m "add sparse checkout files"
253 '
254
255 -test_expect_failure 'partial clone with sparse filter succeeds' '
255 +test_expect_success 'partial clone with sparse filter succeeds' '
256 rm -rf dst.git &&
257 git clone --no-local --bare \
258 --filter=sparse:oid=master:only-one \
@@ -265,7 +265,7 @@ test_expect_failure 'partial clone with sparse filter succeeds' '
265 )
266 '
267
268 -test_expect_failure 'partial clone with unresolvable sparse filter fails cleanly' '
268 +test_expect_success 'partial clone with unresolvable sparse filter fails cleanly' '
269 rm -rf dst.git &&
270 test_must_fail git clone --no-local --bare \
271 --filter=sparse:oid=master:no-such-name \