Re: [PATCH/RFC] git-svn: don't create master if another head exists
Thanks for the review!
On Wed, Jul 11, 2012 at 03:56:43PM -0700, Junio C Hamano wrote:
Show 43 quoted lines
> 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.
Show 31 quoted lines
> > # 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.
Show 5 quoted lines
> 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(-)
Show changes to git-svn.perl +4 −5
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
Marcin Owsiany