Re: [PATCH/RFC] git-svn: don't create master if another head exists

> Marcin Owsiany <marcin@owsiany.pl> writes:
> 
> > Date: Sun, 24 Jun 2012 22:40:05 +0100
> > Subject: [PATCH] git-svn: don't create master if another head exists
> >
> > git-svn insists on creating the "master" head (unless it exists) on every
> > "fetch". It is useful that it gets created initially, when no head exists
> > - users expect this git convention of having a "master" branch on initial
> > clone.
> >
> > However creating it when there already is another head does not provide any
> > value - the ref is never updated, so it just gets stale after a while.  Also,
> > some users find it annoying that it gets recreated, especially when they would
> > like the git branch names to follow SVN repository branch names. More
> > background in http://thread.gmane.org/gmane.comp.version-control.git/115030
> >
> > Make git-svn skip the "master" creation if HEAD points at a valid head. This
> > means "master" does get created on initial "clone" but does not get recreated
> > once a user deletes it.
> 
> The above description makes sense to me, but the code updated with
> this patch doesn't quite make sense to me.
> 
> This is your patch with a bit wider context.
> 
> > diff --git a/git-svn.perl b/git-svn.perl
> > index 0b074c4..2379a71 100755
> > --- a/git-svn.perl
> > +++ b/git-svn.perl
> > @@ -1599,35 +1599,35 @@ sub rebase_cmd {
> >  sub post_fetch_checkout {
> >         return if $_no_checkout;
> >         my $gs = $Git::SVN::_head or return;
> >         return if verify_ref('refs/heads/master^0');
> 
> Does "master" matter here?
> 
> I am wondering why this is not
> 
> 	return if verify_ref("HEAD^0");
> 
> Moreover, since the code will check verify_ref("HEAD^0") anyway in
> the place you updated, is this early return still necessary?

> >         # look for "trunk" ref if it exists
> >         my $remote = Git::SVN::read_all_remotes()->{$gs->{repo_id}};
> >         my $fetch = $remote->{fetch};
> >         if ($fetch) {
> >                 foreach my $p (keys %$fetch) {
> >                         basename($fetch->{$p}) eq 'trunk' or next;
> >                         $gs = Git::SVN->new($fetch->{$p}, $gs->{repo_id}, $p);
> >                         last;
> >                 }
> >         }
> > 
> > -	my $valid_head = verify_ref('HEAD^0');
> > +	return if verify_ref('HEAD^0');
> 
> This one matches the description.  When post_fetch_checkout() is
> called, if HEAD is already pointing at a valid commit, we do not
> want to run checkout (or create a ref).
> 
> >         command_noisy(qw(update-ref refs/heads/master), $gs->refname);
> > -	return if ($valid_head || !verify_ref('HEAD^0'));
> > +	return unless verify_ref('HEAD^0');
> 
> I do not understand these three lines.  Why aren't they like this?
> 
> 	command_noisy(qw(update-ref HEAD), $gs->refname) || return;
> 
> That is, in a fresh repository whose HEAD points at an unborn
> 'master', nothing changes from the current behaviour.  If a fresh
> repository whose HEAD points at some other unborn branch, should the
> code still want to update 'master'?  Wouldn't we rather want to
> update that branch?

> If the caller does not handle errors, it could be even clearer to
> write it like
> 
> 	command_noisy(qw(update-ref HEAD), $gs->refname) ||
> 		die "Cannot update HEAD!!!";