diff mbox series

[2/2] list-objects-filter: handle unresolved sparse filter OID

Message ID 20190828201824.1255-3-jon@jonsimons.org (mailing list archive)
State New, archived
Headers show
Series partial-clone: fix two issues with sparse filter handling | expand

Commit Message

Jon Simons Aug. 28, 2019, 8:18 p.m. UTC 
Handle a potential NULL 'sparse_oid_value' when attempting to load
sparse filter exclusions by blob, to avoid segfaulting later during
'add_excludes_from_blob_to_list'.

While here, uniquify the errors emitted to distinguish between the
case that a given OID is NULL due to an earlier failure to resolve it,
and when an OID resolves but parsing the sparse filter spec fails.

t5616 is updated to demonstrate the change.

Co-authored-by: Jeff King <peff@peff.net>
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Jon Simons <jon@jonsimons.org>
---
 list-objects-filter.c    | 6 +++++-
 t/t5616-partial-clone.sh | 7 +++++++
 2 files changed, 12 insertions(+), 1 deletion(-)

Comments

Derrick Stolee Aug. 29, 2019, 1:12 p.m. UTC | #1 
On 8/28/2019 4:18 PM, Jon Simons wrote:
> Handle a potential NULL 'sparse_oid_value' when attempting to load
> sparse filter exclusions by blob, to avoid segfaulting later during
> 'add_excludes_from_blob_to_list'.
> 
> While here, uniquify the errors emitted to distinguish between the
> case that a given OID is NULL due to an earlier failure to resolve it,
> and when an OID resolves but parsing the sparse filter spec fails.

Adding localization here also seems like a good idea. Thanks!

-Stolee
> +test_expect_success 'partial clone with unresolvable sparse filter fails cleanly' '
> +	test_must_fail git clone --no-local --no-checkout --filter=sparse:oid=master:sparse-filter "file://$(pwd)/sparse-src" sc1 2>err &&
> +	test_i18ngrep "unable to read sparse filter specification from sparse:oid=master:sparse-filter" err &&
> +	test_must_fail git clone --no-local --no-checkout --filter=sparse:oid=master "file://$(pwd)/sparse-src" sc2 2>err &&
> +	test_i18ngrep "unable to parse sparse filter data in $(git -C sparse-src rev-parse master)" err

Just as a sanity check: when we use test_i18ngrep, how does it know how to
separate the part that is translated and which part is not?

	translated: "unable to read sparse filter specification from"
	not translated: "sparse:oid=master"

Thanks,
-Stolee
Jeff King Aug. 29, 2019, 1:44 p.m. UTC | #2 
On Thu, Aug 29, 2019 at 09:12:38AM -0400, Derrick Stolee wrote:

> > +test_expect_success 'partial clone with unresolvable sparse filter fails cleanly' '
> > +	test_must_fail git clone --no-local --no-checkout --filter=sparse:oid=master:sparse-filter "file://$(pwd)/sparse-src" sc1 2>err &&
> > +	test_i18ngrep "unable to read sparse filter specification from sparse:oid=master:sparse-filter" err &&
> > +	test_must_fail git clone --no-local --no-checkout --filter=sparse:oid=master "file://$(pwd)/sparse-src" sc2 2>err &&
> > +	test_i18ngrep "unable to parse sparse filter data in $(git -C sparse-src rev-parse master)" err
> 
> Just as a sanity check: when we use test_i18ngrep, how does it know how to
> separate the part that is translated and which part is not?
> 
> 	translated: "unable to read sparse filter specification from"
> 	not translated: "sparse:oid=master"

It doesn't know. By default we run the suite in LOCALE=C and it checks
the whole string. Under a GETTEXT_POISON build, it checks nothing at
all.

The poison stuff is really about helping people not accidentally mark a
plumbing string (that we expect to get parsed by a machine) as
translatable. So the idea is you'd build with GETTEXT_POISON and then
run the test suite to see if anything breaks. But that means we also
have to annotate the test suite with "yes, I know this will be gibberish
in a poison build, but that's OK because it's meant for humans". And
that's what test_i18ngrep is.

test_i18ngrep could be more clever about matching the gibberish, but
there's not much point. The LOCALE=C run already covered the correctness
of checking the message.

-Peff
Derrick Stolee Aug. 29, 2019, 2:28 p.m. UTC | #3 
On 8/29/2019 9:44 AM, Jeff King wrote:
> On Thu, Aug 29, 2019 at 09:12:38AM -0400, Derrick Stolee wrote:
> 
>>> +test_expect_success 'partial clone with unresolvable sparse filter fails cleanly' '
>>> +	test_must_fail git clone --no-local --no-checkout --filter=sparse:oid=master:sparse-filter "file://$(pwd)/sparse-src" sc1 2>err &&
>>> +	test_i18ngrep "unable to read sparse filter specification from sparse:oid=master:sparse-filter" err &&
>>> +	test_must_fail git clone --no-local --no-checkout --filter=sparse:oid=master "file://$(pwd)/sparse-src" sc2 2>err &&
>>> +	test_i18ngrep "unable to parse sparse filter data in $(git -C sparse-src rev-parse master)" err
>>
>> Just as a sanity check: when we use test_i18ngrep, how does it know how to
>> separate the part that is translated and which part is not?
>>
>> 	translated: "unable to read sparse filter specification from"
>> 	not translated: "sparse:oid=master"
> 
> It doesn't know. By default we run the suite in LOCALE=C and it checks
> the whole string. Under a GETTEXT_POISON build, it checks nothing at
> all.
> 
> The poison stuff is really about helping people not accidentally mark a
> plumbing string (that we expect to get parsed by a machine) as
> translatable. So the idea is you'd build with GETTEXT_POISON and then
> run the test suite to see if anything breaks. But that means we also
> have to annotate the test suite with "yes, I know this will be gibberish
> in a poison build, but that's OK because it's meant for humans". And
> that's what test_i18ngrep is.
> 
> test_i18ngrep could be more clever about matching the gibberish, but
> there's not much point. The LOCALE=C run already covered the correctness
> of checking the message.

Thanks for clearing this up for me!

-Stolee
diff mbox series

Patch

diff --git a/list-objects-filter.c b/list-objects-filter.c
index 36e1f774bc..252fae5d4e 100644
--- a/list-objects-filter.c
+++ b/list-objects-filter.c
@@ -464,9 +464,13 @@  static void *filter_sparse_oid__init(
 {
 	struct filter_sparse_data *d = xcalloc(1, sizeof(*d));
 	d->omits = omitted;
+	if (!filter_options->sparse_oid_value)
+		die(_("unable to read sparse filter specification from %s"),
+		      filter_options->filter_spec);
 	if (add_excludes_from_blob_to_list(filter_options->sparse_oid_value,
 					   NULL, 0, &d->el) < 0)
-		die("could not load filter specification");
+		die(_("unable to parse sparse filter data in %s"),
+		      oid_to_hex(filter_options->sparse_oid_value));
 
 	ALLOC_GROW(d->array_frame, d->nr + 1, d->alloc);
 	d->array_frame[d->nr].defval = 0; /* default to include */
diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
index 6c3aa06973..0adb11f17b 100755
--- a/t/t5616-partial-clone.sh
+++ b/t/t5616-partial-clone.sh
@@ -264,6 +264,13 @@  test_expect_success 'partial clone with sparse filter succeeds' '
 	git clone --no-local --no-checkout --filter=sparse:oid=master:odd-files "file://$(pwd)/sparse-src" pc-odd
 '
 
+test_expect_success 'partial clone with unresolvable sparse filter fails cleanly' '
+	test_must_fail git clone --no-local --no-checkout --filter=sparse:oid=master:sparse-filter "file://$(pwd)/sparse-src" sc1 2>err &&
+	test_i18ngrep "unable to read sparse filter specification from sparse:oid=master:sparse-filter" err &&
+	test_must_fail git clone --no-local --no-checkout --filter=sparse:oid=master "file://$(pwd)/sparse-src" sc2 2>err &&
+	test_i18ngrep "unable to parse sparse filter data in $(git -C sparse-src rev-parse master)" err
+'
+
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd