Re: [PATCH] connected: distinguish local/remote bad objects

[Junio C Hamano <gitster@pobox.com>]

> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index ac29c2b1ae..6f43b2bf8d 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -1133,7 +1133,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
>  
>  		rm = ref_map;
>  		if (check_connected(iterate_ref_map, &rm, &opt)) {
> -			rc = error(_("%s did not send all necessary objects\n"), url);
> +			rc = error(_("connectivity check failed for %s\n"), url);
>  			goto abort;
>  		}
>  	}

Clever.

I was wondering how you are going to deal with the different exit
condition from the rev-list that is only expressed with the two
different messages.  You _could_ grep for "from local" vs "from
remote", but you just let the human user who is staring at the error
message to read it, and stop mentioning the details in this message.

OK.

> diff --git a/connected.c b/connected.c
> index ed3025e7a2..ea773f25db 100644
> --- a/connected.c
> +++ b/connected.c
> @@ -94,6 +94,7 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
>  		strvec_push(&rev_list.args, opt->shallow_file);
>  	}
>  	strvec_push(&rev_list.args,"rev-list");
> +	strvec_push(&rev_list.args, "--detailed-bad-object");
>  	strvec_push(&rev_list.args, "--objects");
>  	strvec_push(&rev_list.args, "--stdin");
>  	if (has_promisor_remote())
> diff --git a/revision.c b/revision.c
> index 090a967bf4..777e762373 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -367,6 +367,16 @@ void add_head_to_pending(struct rev_info *revs)
>  	add_pending_object(revs, obj, "HEAD");
>  }
>  
> +static void NORETURN bad_object(struct rev_info *revs, const char *name,
> +				unsigned int flags)
> +{
> +	if (!revs->detailed_bad_object)
> +		die("bad object %s", name);
> +	if (flags & UNINTERESTING)
> +		die("bad object %s (from local object store)", name);
> +	die("bad object %s (from remote)", name);
> +}

If the missing object you found is reachable from existing tips
(i.e. local aka UNINTERESTING) and also from what they should have
sent (i.e. remote tips), when we discover that the object does not
exist locally (but we can have an in-core shell object whose object
name is already known because child objects that are closer to the
tips than the missing object do exist and point at it), does this
new heuristic work reliably?  

Do we always die and report bad_object() with UNINTERESTING in the
the flags variable, or only when we are lucky?

IOW, if our current branch pionts at A, while the other side says
they are updating it to B,

    X-----o-----A
     \
      x---B

we try to traverse "rev-list ^A B" and make sure everything exists.
If we find objects 'o' missing, it is clear that it was something we
were supposed to have on the local side even before we started the
fetch.  But if 'X' is missing, by the time we notice and report a
missing object, do we always reliably know that it ought to be
reachable from both?  Or do we have 50/50 chance that the traversal
comes from 'o' earlier than from 'x' (in which case X is found to be
missing when we try to see who is the parent of 'o', and flags have
UNINTERESTING bit), or later than from 'x' (in which case X is found
when trying to see who the parents of 'x' is, and we may not know
and we may not bother to find out if X is also a parent of 'o',
hence we'd still say 'You do not have X, and we were looking at 'x',
which we got from the other end, so they were supposed to have sent
it', which would be a misdiagnosis)?

Thanks.