Re: [PATCH 3/3] fetch: die on invalid --negotiation-tip hash

[Emily Shaffer <emilyshaffer@google.com>]

On Wed, Jun 23, 2021 at 03:30:53PM -0700, Jonathan Tan wrote:
> 
> If a full hexadecimal hash is given as a --negotiation-tip to "git
> fetch", and that hash does not correspond to an object, "git fetch" will
> segfault if --negotiate-only is given and will silently ignore that hash
> otherwise. Make these cases fatal errors, just like the case when an
> invalid ref name or abbreviated hash is given.
> 
> While at it, mark the error messages as translatable.
I don't usually like this kind of "while we're at it" change, but in
this case it's very very small, so it doesn't bother me much.
> 
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
>  builtin/fetch.c  | 4 +++-
>  t/t5510-fetch.sh | 9 +++++++++
>  2 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 9191620e50..2c50465cff 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -1428,7 +1428,9 @@ static void add_negotiation_tips(struct git_transport_options *smart_options)
>  		if (!has_glob_specials(s)) {
>  			struct object_id oid;
>  			if (get_oid(s, &oid))
> -				die("%s is not a valid object", s);
> +				die(_("%s is not a valid object"), s);
> +			if (!has_object(the_repository, &oid, 0))
> +				die(_("%s is not a valid object"), s);
Any reason not to consolidate these, e.g. if (get_oid || !has_object)?
Then we wouldn't dup the err string.

>  			oid_array_append(oids, &oid);
>  			continue;
>  		}
> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
> index e83b2a6506..5fc5750d8d 100755
> --- a/t/t5510-fetch.sh
> +++ b/t/t5510-fetch.sh
> @@ -1214,6 +1214,15 @@ test_expect_success '--negotiation-tip understands abbreviated SHA-1' '
>  	check_negotiation_tip
>  '
>  
> +test_expect_success '--negotiation-tip rejects missing OIDs' '
> +	setup_negotiation_tip server server 0 &&
> +	test_must_fail git -C client fetch \
> +		--negotiation-tip=alpha_1 \
This one's okay...

> +		--negotiation-tip=$(test_oid zero) \
...and this one's junk. Ok.

> +		origin alpha_s beta_s 2>err &&
> +	test_i18ngrep "is not a valid object" err
> +'
> +
>  . "$TEST_DIRECTORY"/lib-httpd.sh
>  start_httpd
>  
> -- 
> 2.32.0.288.g62a8d224e6-goog

Other than the little nit, this seems OK to me.

 - Emily