Re: [PATCH] fetch: remove fetch_if_missing=0

From: Jonathan Tan <jonathantanmy@google.com>
Date: Nov 5, 2019, 18:53 UTC
Message-ID: <20191105185322.205317-1-jonathantanmy@google.com>
In-Reply-To: <20191101220537.GA249573@google.com>

Thanks for your review.

Show 11 quoted lines

> > Note that commits and tags may still be lazy-fetched. I limited myself
> > to objects that could be trees or blobs here because Git does not
> > support creating such commit- and tag-excluding clones yet, and even if
> > such a clone were manually created, Git does not have good support for
> > fetching a single commit (when fetching a commit, it and all its
> > ancestors would be sent).
> 
> Is there a place we could put a NEEDSWORK comment to avoid confusion
> when debugging if this gets introduced later?
> 
> Even if not, this seems like a sensible choice.

Done in the test.

Show 8 quoted lines

> > I've verified that this also solves the bug explained in:
> > https://public-inbox.org/git/20191007181825.13463-1-jonathantanmy@google.com/
> 
> Might be worth mentioning the example from there in the commit message
> as well, to help explain the context behind the change.
> 
> I would still be in favor of applying that more conservative change to
> "master", even this late in the -rc cycle.

If we're applying that change first, then this no longer fixes any bug, but is just a code cleanup. (If we don't apply that change, then I'll include that example in the commit message.)

> Should we make OBJECT_INFO_QUICK always imply
> OBJECT_INFO_SKIP_FETCH_OBJECT?  I would suspect that if we are willing to
> avoid checking thoroughly locally, checking remotely would be even more
> undesirable.

As I wrote in [1], the implication does not really go both ways. I think it's better to keep them separate. (Or, at least, we don't need to make the decision in this patch.)

[1] https://public-inbox.org/git/20191011220822.154063-1-jonathantanmy@google.com/

Show 18 quoted lines

> > +test_expect_success 'fetch lazy-fetches only to resolve deltas' '
> > +	setup_triangle &&
> > +
> > +	# Exercise to make sure it works. Git will not fetch anything from the
> > +	# promisor remote other than for the big blob (because it needs to
> > +	# resolve the delta).
> > +	GIT_TRACE_PACKET="$(pwd)/trace" git -C client \
> > +		fetch "file://$(pwd)/server" master &&
> > +
> > +	# Verify the assumption that the client needed to fetch the delta base
> > +	# to resolve the delta.
> > +	git hash-object big-blob.txt >hash &&
> > +	grep "want $(cat hash)" trace
> 
> nit: can avoid using cat:
> 
> 	hash=$(git hash-object big-blob.txt) &&
> 	grep "want $hash" trace

I think it's less error-prone if we always have a "git" command on its own on a line, to avoid losing its error code. When piped into another invocation, or when command-substituted into an argument (e.g. "echo $(git hash-object foo)"), we lose its error code.