Re: [RFC/PATCH 5/5] fetch: fix regression with transport helpers

From: Jeff King <peff@peff.net>
Date: Jun 4, 2019, 14:32 UTC
Message-ID: <20190604143240.GC10598@sigill.intra.peff.net>
In-Reply-To: <20190604021330.16130-6-felipe.contreras@gmail.com>

On Mon, Jun 03, 2019 at 09:13:30PM -0500, Felipe Contreras wrote:

Show 11 quoted lines

> Commit e198b3a740 changed the behavior of fetch with regards to tags.
> Before, null oids where not ignored, now they are, regardless of whether
> the refs have been explicitly cleared or not.
> 
>   e198b3a740 (fetch: replace string-list used as a look-up table with a hashmap)
> 
> When using a transport helper the oids can certainly be null. So now
> tags are ignored and fetching them is impossible.
> 
> This patch fixes that by having a specific flag that is set only when we
> explicitly want to ignore the refs, restoring the original behavior.

Makes sense. Prior to e198b3a740, we indicated "ignored" by setting the oid (in item->util) to NULL. But since it's no longer a pointer after that commit, we can't do so.

So the obvious alternative to this is going back to having it as a pointer. I think I prefer your solution, though, since it keeps the memory management a bit simpler, and more clearly expresses the intent.

If we add any new code that has to similarly ignore an item in the hash, it will have to remember to use clear_item() rather than oidclr(). But I don't think that's a big risk.

>  builtin/fetch.c           | 5 +++--
>  t/t5801-remote-helpers.sh | 2 +-
>  2 files changed, 4 insertions(+), 3 deletions(-)

The code change itself looks obviously correct.

-Peff