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

On Tue, Jul 13 2021, Emily Shaffer wrote:

On Wed, Jun 23, 2021 at 03:30:53PM -0700, Jonathan Tan wrote:

quoted

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.

quoted

Signed-off-by: Jonathan Tan <redacted>
---
 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.

Generally I'd agree, but aren't we explicitly conflating cases where
something is a valid way no name an object v.s. being certain that such
an object does not exist? I.e. this should be something like:

    if can't get_get():
        error "couldn't get the OID of revision '%s'"
    if can't look up fully-qualified OID:
        error "the OID '%s' does not exist"

Or something...