Re: [PATCH 1/2] refspec: consolidate ref-prefix generation logic

From: Jonathan Nieder <jrnieder@gmail.com>
Date: May 31, 2018, 01:07 UTC
Message-ID: <20180531010739.GB36515@aiede.svl.corp.google.com>
In-Reply-To: <20180531004309.GA36515@aiede.svl.corp.google.com>

Jonathan Nieder wrote:

> Brandon Williams wrote:

Show 17 quoted lines

>> When using protocol v2 a client constructs a list of ref-prefixes which
>> are sent across the wire so that the server can do server-side filtering
>> of the ref-advertisement.  The logic that does this exists for both
>> fetch and push (even though no push support for v2 currently exists yet)
>> and is roughly the same so lets consolidate this logic and make it
>> general enough that it can be used for both the push and fetch cases.
>>
>> Signed-off-by: Brandon Williams <bmwill@google.com>
>> ---
>>  builtin/fetch.c | 13 +------------
>>  refspec.c       | 29 +++++++++++++++++++++++++++++
>>  refspec.h       |  4 ++++
>>  transport.c     | 21 +--------------------
>>  4 files changed, 35 insertions(+), 32 deletions(-)
>
> I assume this is meant to be a refactoring with no functional change.
> Alas, it's causing fetch-by-SHA-1 to fail for me:

[...]

>  $ bin-wrappers/git -C /tmp/r -c protocol.version=2 fetch \
> 	https://kernel.googlesource.com/pub/scm/git/git \
> 	6373cb598e1a4e0340583ad75d5abba01ff79774
>  fatal: no matching remote head

Backtrace:

 #0  0x0000555555671e09 in fetch_pack (args=0x7fffffffda80, fd=0x555555a6a9b0, conn=0x555555a65780, ref=0x0, 
     dest=0x555555a64900 "https://kernel.googlesource.com/pub/scm/git/git", sought=0x555555a69a80, nr_sought=1, shallow=0x555555a6a9d8, 
     pack_lockfile=0x555555a631c8, version=protocol_v2) at fetch-pack.c:1562
 #1  0x00005555557290b9 in fetch_refs_via_pack (transport=0x555555a63190, nr_heads=1, to_fetch=0x555555a69a80) at transport.c:326
 #2  0x000055555572b712 in transport_fetch_refs (transport=0x555555a63190, refs=0x555555a65520) at transport.c:1247
 #3  0x000055555559a2a6 in fetch_refs (transport=0x555555a63190, ref_map=0x555555a65520) at builtin/fetch.c:942
 #4  0x000055555559ac3e in do_fetch (transport=0x555555a63190, rs=0x7fffffffdc30) at builtin/fetch.c:1157
 #5  0x000055555559b535 in fetch_one (remote=0x555555a63870, argc=1, argv=0x7fffffffdff8, prune_tags_ok=0) at builtin/fetch.c:1398
 #6  0x000055555559b9a3 in cmd_fetch (argc=1, argv=0x7fffffffdff8, prefix=0x0) at builtin/fetch.c:1487
 #7  0x000055555556764b in run_builtin (p=0x555555a0fa30 <commands+816>, argc=3, argv=0x7fffffffdff0) at git.c:350
 #8  0x000055555556795f in handle_builtin (argc=3, argv=0x7fffffffdff0) at git.c:565
 #9  0x0000555555567b07 in run_argv (argcp=0x7fffffffde8c, argv=0x7fffffffde80) at git.c:617
 #10 0x0000555555567cbc in cmd_main (argc=3, argv=0x7fffffffdff0) at git.c:694
 #11 0x000055555560774c in main (argc=8, argv=0x7fffffffdfc8) at common-main.c:45

Both refs_tmp and transport->remote_refs are NULL.

When refspec_ref_prefixes gets called, it produces the usual argv_array with 6373cb598e1a4e0340583ad75d5abba01ff79774, refs/6373cb598e1a4e0340583ad75d5abba01ff79774, etc.

In the old code, this doesn't happen because we trigger the item->exact_sha1 test.

Would something like the following make sense? It passes tests and appears to avoid trouble but I haven't looked any closer than that.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

--- i/refspec.c +++ w/refspec.c

@@ -202,6 +202,8 @@ void refspec_ref_prefixes(const struct refspec *rs,
 		const struct refspec_item *item = &rs->items[i];
 		const char *prefix = NULL;
 
+		if (item->exact_sha1)
+			continue;
 		if (rs->fetch == REFSPEC_FETCH)
 			prefix = item->src;
 		else if (item->dst)