Thanks for the review! On Wed, Jul 11, 2012 at 03:56:43PM -0700, Junio C Hamano wrote: > Marcin Owsiany <marcin@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx> --- 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@xxxxxxxxxx> 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html