Re: [PATCH] connected.c: reprepare packs for corner cases

From: Derrick Stolee <stolee@gmail.com>
Date: Mar 13, 2020, 00:54 UTC
Message-ID: <7378a863-7e2a-455e-4635-e07938ef3381@gmail.com>
In-Reply-To: <20200312212613.GB872402@coredump.intra.peff.net>

On 3/12/2020 5:26 PM, Jeff King wrote:

Show 5 quoted lines

> On Thu, Mar 12, 2020 at 05:16:38PM -0400, Jeff King wrote:
> 
>> There we see that same reprepare happen in 882839, which is the child
>> fetch-pack. The parent fetch probably needs to reprepare itself after
>> fetch-pack completes.

I agree with you and Junio that where I put the reprepare was non- optimal. The initial reason to put it there was that I found where the error was happening, and thought that placing the reprepare there was the best way to prevent this error from popping up in another case.

The result of a fetch failing and saying the remote did something wrong is quite alarming, and I wanted to avoid that from happening again in the future from some other path.

But you're right, it's better to be as correct as possible.

> Actually, it's not fetch which is running fetch-pack, but rather the
> remote helper itself. So I think the simplest thing is for the
> remote-helper layer to do something like this:

I appreciate your root-causing this into the multi-process nature of
fetch. I will update the commit message to include your details,
especially about how it does not reproduce over file or ssh protocol.

Show 42 quoted lines

> diff --git a/transport-helper.c b/transport-helper.c
> index 20a7185ec4..25957e9a05 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -14,6 +14,7 @@
>  #include "refspec.h"
>  #include "transport-internal.h"
>  #include "protocol.h"
> +#include "packfile.h"
>  
>  static int debug;
>  
> @@ -672,6 +673,7 @@ static int fetch(struct transport *transport,
>  {
>  	struct helper_data *data = transport->data;
>  	int i, count;
> +	int ret;
>  
>  	get_helper(transport);
>  
> @@ -710,13 +712,18 @@ static int fetch(struct transport *transport,
>  	if (data->transport_options.negotiation_tips)
>  		warning("Ignoring --negotiation-tip because the protocol does not support it.");
>  
> -	if (data->fetch)
> -		return fetch_with_fetch(transport, nr_heads, to_fetch);
> +	ret = data->fetch ? fetch_with_fetch(transport, nr_heads, to_fetch) :
> +	      data->import ? fetch_with_import(transport, nr_heads, to_fetch) :
> +	      -1;
>  
> -	if (data->import)
> -		return fetch_with_import(transport, nr_heads, to_fetch);
> +	/*
> +	 * We may have just received a pack through the helper sub-process;
> +	 * refresh the pack list.
> +	 */
> +	if (!ret)
> +		reprepare_packed_git(the_repository);
>  
> -	return -1;
> +	return ret;
>  }

This code looks correct, and should be the fix for the short-term.

I wonder if we could do something more complicated in the long-term, which was recommended to me by Jeff Hostetler: add the pack to the packed_git list once we've indexed it. That way, we don't reprepare and scan the packs one-by-one, but instead we insert to the list a single pack that we already know about.

Thanks, -Stolee