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

[Marcin Owsiany <marcin@owsiany.pl>]

Thanks for the review!

On Wed, Jul 11, 2012 at 03:56:43PM -0700, Junio C Hamano wrote:
> 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?

Hm, good point, nothing between these two returns seems to modify
on-disk state.

> >         # 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?

I don't know if there can ever be some other unborn branch other than
"master", but I guess your proposal makes it more robust.

> 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!!!";

Turns out that command_noisy()
 - has a meaningless return value
 - throws an exception on command failure
so the "||" bit does not work.
Also, for some reason command_noisy does not check for the command being
killed by a signal, so I'd prefer to leave the verify_ref there.

PTAL:

From: Marcin Owsiany <marcin@owsiany.pl>
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 already points at a valid head.
This means "master" does get created on initial "clone" but does not get
recreated once a user deletes it.

Also, make post_fetch_checkout work with any head that is pointed to by HEAD,
not just "master".

Also, use fatal error handling consistent with the rest of the program for
post_fetch_checkout.

Signed-off-by: Marcin Owsiany <marcin@owsiany.pl>
---
 git-svn.perl |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/git-svn.perl b/git-svn.perl
index 0b074c4..6673d21 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -367,9 +367,9 @@ Git::SVN::init_vars();
 eval {
 	Git::SVN::verify_remotes_sanity();
 	$cmd{$cmd}->[0]->(@ARGV);
+	post_fetch_checkout();
 };
 fatal $@ if $@;
-post_fetch_checkout();
 exit 0;
 
 ####################### primary functions ######################
@@ -1598,8 +1598,8 @@ sub rebase_cmd {
 
 sub post_fetch_checkout {
 	return if $_no_checkout;
+	return if verify_ref('HEAD^0');
 	my $gs = $Git::SVN::_head or return;
-	return if verify_ref('refs/heads/master^0');
 
 	# look for "trunk" ref if it exists
 	my $remote = Git::SVN::read_all_remotes()->{$gs->{repo_id}};
@@ -1612,9 +1612,8 @@ sub post_fetch_checkout {
 		}
 	}
 
-	my $valid_head = verify_ref('HEAD^0');
-	command_noisy(qw(update-ref refs/heads/master), $gs->refname);
-	return if ($valid_head || !verify_ref('HEAD^0'));
+	command_noisy(qw(update-ref HEAD), $gs->refname);
+	return unless verify_ref('HEAD^0');
 
 	return if $ENV{GIT_DIR} !~ m#^(?:.*/)?\.git$#;
 	my $index = $ENV{GIT_INDEX_FILE} || "$ENV{GIT_DIR}/index";
-- 
1.7.7.3

-- 
Marcin Owsiany <marcin@owsiany.pl>              http://marcin.owsiany.pl/
GnuPG: 2048R/02F946FC  35E9 1344 9F77 5F43 13DD  6423 DBF4 80C6 02F9 46FC

"Every program in development at MIT expands until it can read mail."
                                                              -- Unknown