Re: Minor annoyance with git push

From: Jeff King <peff@peff.net>
Date: Feb 20, 2008, 18:15 UTC
Message-ID: <20080220181513.GA16289@sigill.intra.peff.net>
In-Reply-To: <7vk5kz7btw.fsf@gitster.siamese.dyndns.org>

On Wed, Feb 20, 2008 at 09:54:35AM -0800, Junio C Hamano wrote:

Show 13 quoted lines

> > +++ b/builtin-push.c
> > @@ -68,8 +68,7 @@ static int do_push(const char *repo, int flags)
> >  	if (!refspec
> >  		&& !(flags & TRANSPORT_PUSH_ALL)
> >  		&& remote->push_refspec_nr) {
> > -		refspec = remote->push_refspec;
> > -		refspec_nr = remote->push_refspec_nr;
> > +		set_refspecs(remote->push_refspec, remote->push_refspec_nr);
> >  	}
> >  	errs = 0;
> >  	for (i = 0; i < remote->url_nr; i++) {
> 
> Yeah, we are on the same page.  See the patch I sent out last night ;-)

Meaning you will take this patch? (And yes, I think the "+HEAD" patch you sent is a good fix, regardless of everything else we've talked about).

Show 10 quoted lines

> > How are you detecting that the remote is a shared repository?
> 
> I am not.  I only said "... may even be able to" ;-).
> 
> The autodetection is not even the first step to tackle this
> issue anyway.  The "HEAD" magic (or if somebody comes up with a
> better design, that one) comes first, deciding if such an
> autodetection is even a good idea comes next, and then iff we
> decide that it is a good idea finally comes the task of finding
> out how we do so.

Didn't the "HEAD" magic just come? :) Is there some part of that that you are unhappy with?

> I'd presume that the upload-pack side can internally check "int
> shared_repository" and the protocol extension that conveys that
> information to the other end is easy enough.  As always, dumb
> transports are second class citizens and need their own hacks.

Reasonable.

Show 6 quoted lines

> > By the core.sharedrepository config option? I use several
> > shared repositories, and I never set that variable; instead
> > ...
> 
> It is _your_ problem that you do not use published interface,
> isn't it?

I would agree with you if core.sharedRepository had ever been introduced as a signal flag for a particular workflow, and not simply as a way to set up the umask.

If you want to introduce those semantics to sharedRepository, that is not unreasonable, but:

  - recognize that you don't automagically turn on this feature for
    repositories using the shared workflow, since they may or may not
    even have this flag enabled

  - recognize that people who _do_ want this new behavior might not want
    the umask side effects (and in fact, those side effects can reduce
    security if the users are in a group with untrusted users)

so I think that a new config option is probably safer, and not necessarily less likely to work automatically.

That being said, I think this sort of automatic detection should wait at least one release cycle. Once the behavior is configurable at all, we can see how people adopt it into their workflows and if there is really any desire for detection at all.

-Peff