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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 10, 2022, 17:27 UTC
Message-ID: <xmqqbkqj4mj7.fsf@gitster.g>
In-Reply-To: <83f2cd893a4c47c947a93fe99d202d67f540e963.1665417859.git.gitgitgadget@gmail.com>

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 13 quoted lines

> From: Derrick Stolee <derrickstolee@github.com>
>
> 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.

Show 6 quoted lines

> 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.

Show 17 quoted lines

> 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.