Skip to content

Commit 4c96a77

Browse files
peffgitster
peff
authored and
gitster
committed
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>
1 parent 72de589 commit 4c96a77

File tree

5 files changed

+14
-21
lines changed

5 files changed

+14
-21
lines changed

‎builtin/rev-list.c‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -471,10 +471,6 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
471471
parse_list_objects_filter(&filter_options, arg);
472472
if (filter_options.choice && !revs.blob_objects)
473473
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);
478474
continue;
479475
}
480476
if (!strcmp(arg, ("--no-" CL_ARG__FILTER))) {

‎list-objects-filter-options.c‎

Lines changed: 2 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -63,17 +63,7 @@ static int gently_parse_list_objects_filter(
6363
return 0;
6464

6565
} 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);
7767
filter_options->choice = LOFC_SPARSE_OID;
7868
return 0;
7969

@@ -138,7 +128,7 @@ void list_objects_filter_release(
138128
struct list_objects_filter_options *filter_options)
139129
{
140130
free(filter_options->filter_spec);
141-
free(filter_options->sparse_oid_value);
131+
free(filter_options->sparse_oid_name);
142132
memset(filter_options, 0, sizeof(*filter_options));
143133
}
144134

‎list-objects-filter-options.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ struct list_objects_filter_options {
4242
* choice-specific; not all values will be defined for any given
4343
* choice.
4444
*/
45-
struct object_id *sparse_oid_value;
45+
char *sparse_oid_name;
4646
unsigned long blob_limit_value;
4747
unsigned long tree_exclude_depth;
4848
};

‎list-objects-filter.c‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -463,9 +463,16 @@ static void *filter_sparse_oid__init(
463463
filter_free_fn *filter_free_fn)
464464
{
465465
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);
466474
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)
469476
die("could not load filter specification");
470477

471478
ALLOC_GROW(d->array_frame, d->nr + 1, d->alloc);

‎t/t5616-partial-clone.sh‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -252,7 +252,7 @@ test_expect_success 'setup src repo for sparse filter' '
252252
git -C sparse-src commit -m "add sparse checkout files"
253253
'
254254

255-
test_expect_failure 'partial clone with sparse filter succeeds' '
255+
test_expect_success 'partial clone with sparse filter succeeds' '
256256
rm -rf dst.git &&
257257
git clone --no-local --bare \
258258
--filter=sparse:oid=master:only-one \
@@ -265,7 +265,7 @@ test_expect_failure 'partial clone with sparse filter succeeds' '
265265
)
266266
'
267267

268-
test_expect_failure 'partial clone with unresolvable sparse filter fails cleanly' '
268+
test_expect_success 'partial clone with unresolvable sparse filter fails cleanly' '
269269
rm -rf dst.git &&
270270
test_must_fail git clone --no-local --bare \
271271
--filter=sparse:oid=master:no-such-name \

0 commit comments

Comments
 (0)