[PATCH] fetch-pack: demonstrate --all failure when remote is empty

( Junio, please pick up the patch provided in the end )

On Tue, Jun 12, 2018 at 06:54:17PM +0000, Kirill Smelkov wrote:

On Tue, Jun 12, 2018 at 05:48:49AM -0400, Jeff King wrote:

quoted

On Mon, Jun 11, 2018 at 09:43:02AM +0000, Kirill Smelkov wrote:

[...]

quoted

quoted

I'm not sure, but I would say that `fetch-pack --all` from an empty
repository should not fail and should just give empty output as fetch
does.

Yeah, that seems reasonable to me. The die() that catches this dates
back to 2005-era, and we later taught the "fetch" porcelain to handle
this. I don't _think_ anybody would be upset that the plumbing learned
to treat this as a noop. It's probably a one-liner change in
fetch_pack() to return early instead of dying.

Ok, I will try to send related testcase, and it is indeed easy to find
- the fix itself.

I started doing it in full with the following

--- a/fetch-pack.c
+++ b/fetch-pack.c

@@ -1581,6 +1581,8 @@ struct ref *fetch_pack(struct fetch_pack_args *args,

        if (!ref) {
                packet_flush(fd[1]);
+               if (nr_sought == 0)	// XXX or better args->fetch_all
+                       return NULL; /* nothing to fetch */
                die(_("no matching remote head"));
        }
        prepare_shallow_info(&si, shallow);

but then came to the fact that !ref fetch_pack() return is analyzed in 2
places:

- in builtin/fetch-pack.c itself:

        ref = fetch_pack(&args, fd, conn, ref, dest, sought, nr_sought,
                 &shallow, pack_lockfile_ptr, protocol_v0);

	...

        ret = !ref;

- and in transport.c in fetch_refs_via_pack():

        case protocol_v1:
        case protocol_v0:
                refs = fetch_pack(&args, data->fd, data->conn,
                                  refs_tmp ? refs_tmp : transport->remote_refs,
                                  dest, to_fetch, nr_heads, &data->shallow,
                                  &transport->pack_lockfile, data->version);
                break;

	...

        if (refs == NULL)
                ret = -1;


As I don't know git codebase well-enough I don't see offhand how to
distinguish empty result from a real error when something was requested
and not fetched. If it would be only builtin/fetch-pack I could start to
play ugly games with analyzing too in the calling site args.fetch_all
and nr_though and if at that level we also know we requested nothing,
don't treat !refs as an error.

However with transport.c being there too, since I'm no longer using
`fetch-pack --all`, now it is best for me to not delve into this story
and just stop with attached patch.

Thanks,
Kirill

---- 8< ----
From 76d80ffcfd4574715545c62413d64d40af063d09 Mon Sep 17 00:00:00 2001
From: Kirill Smelkov <redacted>
Date: Wed, 13 Jun 2018 15:46:00 +0300
Subject: [PATCH] fetch-pack: demonstrate --all failure when remote is empty

Currently `fetch-pack --all` from an empty repository gives:

	fatal: no matching remote head

However it would be logical for this fetch operation to succeed with
empty result. Add test showing the failure.

Signed-off-by: Kirill Smelkov <redacted>

---
 t/t5500-fetch-pack.sh | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 82aee1c2d8..2234bad411 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh

@@ -528,6 +528,14 @@ test_expect_success 'test --all with tag to non-tip' '
 	)
 '
 
+test_expect_failure 'test --all wrt empty.git' '
+	git init --bare empty.git &&
+	(
+		cd client &&
+		git fetch-pack --all ../empty.git
+	)
+'
+
 test_expect_success 'shallow fetch with tags does not break the repository' '
 	mkdir repo1 &&
 	(

-- 
2.18.0.rc1.253.gf85a566b11.dirty