Subject: Re: [PATCH v4 08/11] bundle: add flags to verify_bundle(), skip walk

From: Junio C Hamano <[email-masked]>
Date: Mon, 10 Oct 2022 10:27:08 -0700

"Derrick Stolee via GitGitGadget" <[email-masked]> writes:

> From: Derrick Stolee <[email-masked]>
>
> The verify_bundle() method checks if a bundle can be applied to a given
> repository. This not only verifies that certain commits exist in the
> repository, but Git also checks that these commits are reachable.
>
> This behavior dates back to the original git-bundle builtin written in
> 2e0afafebd8 (Add git-bundle: move objects and references by archive,
> 2007-02-22), but the message does not go into detail why the
> reachability check is important.
>
> Since verify_bundle() is called from unbundle(), we need to add an
> option to pipe the flags through that method.

All makes sense.

> When unbundling from a list of bundles, Git will create refs that point
> to the tips of the latest bundle, which makes this reachability walk
> succeed, in theory. However, the loose refs cache does not get
> invalidated and hence the reachability walk fails. By disabling the
> reachability walk in the bundle URI code, we can get around this
> reachability check.

The above makes it sound like the real culprit is that cache goes
out of sync and the presented solution is a workaround; readers are
left in suspense if the "real" solution (as opposed to a workaround)
would come in a later step or in a future series.

> diff --git a/bundle-uri.c b/bundle-uri.c
> index 8a7c11c6393..ad5baabdd94 100644
> --- a/bundle-uri.c
> +++ b/bundle-uri.c
> @@ -301,7 +301,13 @@ static int unbundle_from_file(struct repository *r, const char *file)
>  	if ((bundle_fd = read_bundle_header(file, &header)) < 0)
>  		return 1;
>  
> -	if ((result = unbundle(r, &header, bundle_fd, NULL)))
> +	/*
> +	 * Skip the reachability walk here, since we will be adding
> +	 * a reachable ref pointing to the new tips, which will reach
> +	 * the prerequisite commits.
> +	 */
> +	if ((result = unbundle(r, &header, bundle_fd, NULL,
> +			       VERIFY_BUNDLE_SKIP_REACHABLE)))
>  		return 1;

This is not a new problem introduced in this new round, but if we
are updating this, can we fix it to omit assignment inside if
condition?

 * result is initialized to 0.

 * when unbundle returns non-zero, it is assigned to result and the
   function returns immediately, discarding whatever was assigned to
   the variable.

 * if unbundle returns zero, it is assigned to result and the
   control continues from here.  We know result is set to 0, but
   then that is what it was initialized earlier.