Re: [PATCH] Switch receive.denyCurrentBranch to "refuse"

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jan 30, 2009, 14:11 UTC
Message-ID: <alpine.DEB.1.00.0901301429010.3586@pacific.mpi-cbg.de>
In-Reply-To: <20090130025546.GA18257@coredump.intra.peff.net>

Hi,

On Thu, 29 Jan 2009, Jeff King wrote:

Show 12 quoted lines

> On Fri, Jan 30, 2009 at 01:34:28AM +0100, Johannes Schindelin wrote:
> 
> > 	Let's be honest here, I have not much respect for users who fail 
> > 	to read up enough to understand what they are doing.
> > 
> > 	But hearing from those users constantly is really unnerving.  And 
> > 	it would be a one-time cost to old-timers.
> 
> I am not personally opposed to changing this default. I seem to
> recall some opposition when this was brought up initially, but I don't
> recall any specific reason besides "change is bad". Maybe those who
> oppose want to summarize their arguments here.

We like to play it safe when changing behavior that does not meet expectations of old-timers.

For example, all those early adopters who have forks of the linux-2.6 repository (and probably that repository itself, too) do not have core.bare set.

So whenever an old-timer would upgrade to a new Git _with_ my patch, they would need to change their setup.

A one-time cost.

And far easier to accomodate than the push for non-dashed commands (which people still seem to grumble about, even if they should have realized by now that calling Git through the wrapper exclusively brings so many advantages).

Show 5 quoted lines

> I was hoping that introducing the warning would cause new users to "get 
> it". But since this warning was put in place, I think we have still 
> gotten a few questions on the list about this. I don't know if it simply 
> because they are on older versions, or if the warning is insufficient. 
> If the former, then perhaps that argues for leaving it a little longer.

I would argue it is because users cannot read :-)

Show 5 quoted lines

> >  	case DENY_REFUSE:
> > -		if (!is_ref_checked_out(name))
> > +		if (is_bare_repository() || !is_ref_checked_out(name))
> 
> Now what is this change about?

I missed the fact that is_ref_checked_out() already checked for that.

Show 14 quoted lines

> > --- a/t/t5701-clone-local.sh
> > +++ b/t/t5701-clone-local.sh
> > @@ -119,7 +119,7 @@ test_expect_success 'bundle clone with nonexistent HEAD' '
> >  test_expect_success 'clone empty repository' '
> >  	cd "$D" &&
> >  	mkdir empty &&
> > -	(cd empty && git init) &&
> > +	(cd empty && git init && git config receive.denyCurrentBranch false) &&
> >  	git clone empty empty-clone &&
> >  	test_tick &&
> >  	(cd empty-clone
> 
> Perhaps some of these tests would do better to actually just use a bare
> repository.

Right. I just ran out of time, but did not want to hide the patch from the community.

> That would better match the expected workflow for cloning empty, anyway.

Well, I did not want to mix up the two of them. Besides, I have this patch in my personal tree for quite some time now, always wanting to clean it up enough to send it...)

Ciao, Dscho