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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 11, 2012, 22:56 UTC
Message-ID: <7vmx35dhzo.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20120711214019.GF30213@beczulka>

Marcin Owsiany <marcin@owsiany.pl> writes:

Show 17 quoted lines

> 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.

Show 9 quoted lines

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

Show 13 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?

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

Show 13 quoted lines

>         return if $ENV{GIT_DIR} !~ m#^(?:.*/)?\.git$#;
>         my $index = $ENV{GIT_INDEX_FILE} || "$ENV{GIT_DIR}/index";
>         return if -f $index;
> 
>         return if command_oneline(qw/rev-parse --is-inside-work-tree/) eq 'false';
>         return if command_oneline(qw/rev-parse --is-inside-git-dir/) eq 'true';
>         command_noisy(qw/read-tree -m -u -v HEAD HEAD/);
>         print STDERR "Checked out HEAD:\n  ",
>                      $gs->full_url, " r", $gs->last_rev, "\n";
>         if (auto_create_empty_directories($gs)) {
>                 $gs->mkemptydirs($gs->last_rev);
>         }
>  }