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

[Kirill Smelkov <kirr@nexedi.com>]

On Wed, Jun 13, 2018 at 10:13:07AM -0700, Junio C Hamano wrote:
> Kirill Smelkov <kirr@nexedi.com> writes:
> 
> > ( 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:
> >> > On Mon, Jun 11, 2018 at 09:43:02AM +0000, Kirill Smelkov wrote:
> > [...]
> >
> >> > > 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.
> 
> I actually have a slight preference to the current "attempting to
> fetch from a total emptiness is so rare that it is worth grabbing
> attention of whoever does so" behaviour, to be honest.

I see.

> Oh, wait, is this specific to "fetch-pack" and the behaviour of
> end-user-facing "git fetch" is kept same as before?  If then, I'd be
> somewhat sympathetic to the cause---it would be more convenient for
> the calling Porcelain script if this turned into a silent noop (even
> though it would probably make it harder to diagnose when such a
> Porcelain is set up incorrectly e.g. pointing at an empty repository
> that is not the one the Porcelain writer intended to fetch from).

Yes, it is only for fetch-pack, and behaviour of porcelain fetch is kept
as it was before.

> > 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.
> 
> If we do not plan to change the behaviour later ourselves, I do not
> think it makes sense, nor it is fair to those future developers who
> inherit this project, to declare that the established behaviour is
> wrong with an 'expect-failure' test like this, to be honest.

I see. Let's please cancel this patch then.


> > +test_expect_failure 'test --all wrt empty.git' '
> > +	git init --bare empty.git &&
> > +	(
> > +		cd client &&
> > +		git fetch-pack --all ../empty.git
> > +	)
> > +'