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

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

[Marcin Owsiany]

git-svn insists on creating the "master" head (unless it exists) on every
"fetch". While it is useful that it gets created initially (users expect this
git convention), 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 there is another head ref pointing
to the same place. This means "master" does get created on initial "clone" but
does not get recreated once a user deletes it.

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

diff --git a/git-svn.perl b/git-svn.perl
index 0b074c4..90f3d06 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -1613,6 +1613,8 @@ sub post_fetch_checkout {
 	}
 
 	my $valid_head = verify_ref('HEAD^0');
+	my @heads_commits = eval { command(qw(show-ref --heads --hash)) };
+	return if $valid_head and grep { $_ eq $valid_head } @heads_commits;
 	command_noisy(qw(update-ref refs/heads/master), $gs->refname);
 	return if ($valid_head || !verify_ref('HEAD^0'));
 
-- 
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

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

[Eric Wong]

Marcin Owsiany <marcin@owsiany.pl> wrote:
> git-svn insists on creating the "master" head (unless it exists) on every
> "fetch". While it is useful that it gets created initially (users expect this
> git convention), 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 there is another head ref pointing
> to the same place. This means "master" does get created on initial "clone" but
> does not get recreated once a user deletes it.

Sounds reasonable to me.  Thanks for following up on this after all this
time :)

> Signed-off-by: Marcin Owsiany <marcin@owsiany.pl>
> ---
>  git-svn.perl |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/git-svn.perl b/git-svn.perl
> index 0b074c4..90f3d06 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -1613,6 +1613,8 @@ sub post_fetch_checkout {
>  	}
>  
>  	my $valid_head = verify_ref('HEAD^0');
> +	my @heads_commits = eval { command(qw(show-ref --heads --hash)) };
> +	return if $valid_head and grep { $_ eq $valid_head } @heads_commits;

I (and I believe most git hackers) prefer C-style "&&" for boolean
expressions:

	return if $valid_head && grep { $_ eq $valid_head } @heads_commits;

"and" is lower precedence and best reserved for control flow:

	-f $file and print "File: $file exists!\n";

There's no logical difference in this case, but "&&" is probably easier for
C programmers to read.

I'll just swap "and" for "&&" and push unless there's any objection from
you.

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

[Junio C Hamano]

Marcin Owsiany <marcin@owsiany.pl> writes:

> git-svn insists on creating the "master" head (unless it exists) on every
> "fetch". While it is useful that it gets created initially (users expect this
> git convention), 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 there is another head ref pointing
> to the same place. This means "master" does get created on initial "clone" but
> does not get recreated once a user deletes it.
>
> Signed-off-by: Marcin Owsiany <marcin@owsiany.pl>
> ---
>  git-svn.perl |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/git-svn.perl b/git-svn.perl
> index 0b074c4..90f3d06 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -1613,6 +1613,8 @@ sub post_fetch_checkout {
>  	}
>  
>  	my $valid_head = verify_ref('HEAD^0');
> +	my @heads_commits = eval { command(qw(show-ref --heads --hash)) };
> +	return if $valid_head and grep { $_ eq $valid_head } @heads_commits;

This is strange.  Much earlier in the code there is this use of
master.

    sub post_fetch_checkout {
            return if $_no_checkout;
            my $gs = $Git::SVN::_head or return;
            return if verify_ref('refs/heads/master^0');

If your goal is to get rid of "master" (because you have a different
branch that serves the role of the primary branch), shouldn't this
code be killed?  Otherwise, if you have a stray "master" that you
are not even using, you would end up skipping checkout for your true
primary branch, no?

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

[Junio C Hamano]

Marcin Owsiany <marcin@owsiany.pl> writes:

> diff --git a/git-svn.perl b/git-svn.perl
> index 0b074c4..90f3d06 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -1613,6 +1613,8 @@ sub post_fetch_checkout {
>  	}
>  
>  	my $valid_head = verify_ref('HEAD^0');
> +	my @heads_commits = eval { command(qw(show-ref --heads --hash)) };
> +	return if $valid_head and grep { $_ eq $valid_head } @heads_commits;
>  	command_noisy(qw(update-ref refs/heads/master), $gs->refname);
>  	return if ($valid_head || !verify_ref('HEAD^0'));

This looks like a typical XY solution.  What are you really trying
to validate?  "HEAD" points at an existing branch and you do not
care what branch it is?  HEAD may not even point at a valid branch
but can be detached as long as it happens to point at a commit that
is at the tip of some branch (hence building further commit on HEAD
will break the condition you are checking in the above code)?

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

[Junio C Hamano]

Eric Wong <normalperson@yhbt.net> writes:

> Marcin Owsiany <marcin@owsiany.pl> wrote:
>> git-svn insists on creating the "master" head (unless it exists) on every
>> "fetch". While it is useful that it gets created initially (users expect this
>> git convention), 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 there is another head ref pointing
>> to the same place. This means "master" does get created on initial "clone" but
>> does not get recreated once a user deletes it.
>
> Sounds reasonable to me.  Thanks for following up on this after all this
> time :)
>
>> Signed-off-by: Marcin Owsiany <marcin@owsiany.pl>
>> ---
>>  git-svn.perl |    2 ++
>>  1 files changed, 2 insertions(+), 0 deletions(-)
>> 
>> diff --git a/git-svn.perl b/git-svn.perl
>> index 0b074c4..90f3d06 100755
>> --- a/git-svn.perl
>> +++ b/git-svn.perl
>> @@ -1613,6 +1613,8 @@ sub post_fetch_checkout {
>>  	}
>>  
>>  	my $valid_head = verify_ref('HEAD^0');
>> +	my @heads_commits = eval { command(qw(show-ref --heads --hash)) };
>> +	return if $valid_head and grep { $_ eq $valid_head } @heads_commits;
>
> I (and I believe most git hackers) prefer C-style "&&" for boolean
> expressions:
>
> 	return if $valid_head && grep { $_ eq $valid_head } @heads_commits;
>
> "and" is lower precedence and best reserved for control flow:
>
> 	-f $file and print "File: $file exists!\n";
>
> There's no logical difference in this case, but "&&" is probably easier for
> C programmers to read.
>
> I'll just swap "and" for "&&" and push unless there's any objection from
> you.

I personally do not like either (I would generally avoid the
statement modifiers unless the condition is really trivial and it is
about an exceptional case) from the style point of view, but more
importantly, I do not understand what the new check is trying to do,
and I am not convinced if it is the whole solution to "I do not want
'master', as I am using something else" problem (please see my two
review messages).

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

[Marcin Owsiany]

On Mon, Jun 25, 2012 at 04:16:59AM +0000, Eric Wong wrote:
> Marcin Owsiany <marcin@owsiany.pl> wrote:
> > git-svn insists on creating the "master" head (unless it exists) on every
> > "fetch". While it is useful that it gets created initially (users expect this
> > git convention), 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 there is another head ref pointing
> > to the same place. This means "master" does get created on initial "clone" but
> > does not get recreated once a user deletes it.
> 
> Sounds reasonable to me.  Thanks for following up on this after all this
> time :)

My TODO list is really long, and there is so little time :-)

> > Signed-off-by: Marcin Owsiany <marcin@owsiany.pl>
> > ---
> >  git-svn.perl |    2 ++
> >  1 files changed, 2 insertions(+), 0 deletions(-)
> > 
> > diff --git a/git-svn.perl b/git-svn.perl
> > index 0b074c4..90f3d06 100755
> > --- a/git-svn.perl
> > +++ b/git-svn.perl
> > @@ -1613,6 +1613,8 @@ sub post_fetch_checkout {
> >  	}
> >  
> >  	my $valid_head = verify_ref('HEAD^0');
> > +	my @heads_commits = eval { command(qw(show-ref --heads --hash)) };
> > +	return if $valid_head and grep { $_ eq $valid_head } @heads_commits;
> 
> I (and I believe most git hackers) prefer C-style "&&" for boolean
> expressions:
> 
> 	return if $valid_head && grep { $_ eq $valid_head } @heads_commits;
> 
> "and" is lower precedence and best reserved for control flow:
> 
> 	-f $file and print "File: $file exists!\n";
> 
> There's no logical difference in this case, but "&&" is probably easier for
> C programmers to read.
> 
> I'll just swap "and" for "&&" and push unless there's any objection from
> you.

Sounds fine.

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

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

[Marcin Owsiany]

On Sun, Jun 24, 2012 at 10:44:57PM -0700, Junio C Hamano wrote:
> Marcin Owsiany <marcin@owsiany.pl> writes:
> 
> > git-svn insists on creating the "master" head (unless it exists) on every
> > "fetch". While it is useful that it gets created initially (users expect this
> > git convention), 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 there is another head ref pointing
> > to the same place. This means "master" does get created on initial "clone" but
> > does not get recreated once a user deletes it.
> >
> > Signed-off-by: Marcin Owsiany <marcin@owsiany.pl>
> > ---
> >  git-svn.perl |    2 ++
> >  1 files changed, 2 insertions(+), 0 deletions(-)
> >
> > diff --git a/git-svn.perl b/git-svn.perl
> > index 0b074c4..90f3d06 100755
> > --- a/git-svn.perl
> > +++ b/git-svn.perl
> > @@ -1613,6 +1613,8 @@ sub post_fetch_checkout {
> >  	}
> >  
> >  	my $valid_head = verify_ref('HEAD^0');
> > +	my @heads_commits = eval { command(qw(show-ref --heads --hash)) };
> > +	return if $valid_head and grep { $_ eq $valid_head } @heads_commits;
> 
> This is strange.  Much earlier in the code there is this use of
> master.
> 
>     sub post_fetch_checkout {
>             return if $_no_checkout;
>             my $gs = $Git::SVN::_head or return;
>             return if verify_ref('refs/heads/master^0');
> 
> If your goal is to get rid of "master" (because you have a different
> branch that serves the role of the primary branch), shouldn't this
> code be killed?  Otherwise, if you have a stray "master" that you
> are not even using, you would end up skipping checkout for your true
> primary branch, no?

This is my first attempt at modifying git code and I don't pretend to
understand everything this function does. Even less obvious to me is why
it does that. E.g. "return if ($valid_head || ..." - skips the checkout
if HEAD was pointing at tip of _any_ branch, right? But why?

So I'm just trying to change as little as possible.

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

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

[Marcin Owsiany]

On Sun, Jun 24, 2012 at 10:47:04PM -0700, Junio C Hamano wrote:
> Marcin Owsiany <marcin@owsiany.pl> writes:
> 
> > diff --git a/git-svn.perl b/git-svn.perl
> > index 0b074c4..90f3d06 100755
> > --- a/git-svn.perl
> > +++ b/git-svn.perl
> > @@ -1613,6 +1613,8 @@ sub post_fetch_checkout {
> >  	}
> >  
> >  	my $valid_head = verify_ref('HEAD^0');
> > +	my @heads_commits = eval { command(qw(show-ref --heads --hash)) };
> > +	return if $valid_head and grep { $_ eq $valid_head } @heads_commits;
> >  	command_noisy(qw(update-ref refs/heads/master), $gs->refname);
> >  	return if ($valid_head || !verify_ref('HEAD^0'));
> 
> This looks like a typical XY solution.

Can you please explain wha an "XY solution" is? I'm not familiar with
this expression.

>  What are you really trying
> to validate?  "HEAD" points at an existing branch and you do not
> care what branch it is?

Yes. I think.

>  HEAD may not even point at a valid branch
> but can be detached as long as it happens to point at a commit that
> is at the tip of some branch (hence building further commit on HEAD
> will break the condition you are checking in the above code)?

The more questions you ask, the less I feel I know about how git works
:-)

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

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

[Junio C Hamano]

Marcin Owsiany <marcin@owsiany.pl> writes:

>>  What are you really trying
>> to validate?  "HEAD" points at an existing branch and you do not
>> care what branch it is?
>
> Yes. I think.

Why do you even care about the value of HEAD, i.e. the output from
"rev-parse HEAD", if that is the case?  Wouldn't you rather be
reading from the output "symbolic-ref HEAD" to see if it points at
any branch?

>>  HEAD may not even point at a valid branch
>> but can be detached as long as it happens to point at a commit that
>> is at the tip of some branch (hence building further commit on HEAD
>> will break the condition you are checking in the above code)?
>
> The more questions you ask, the less I feel I know about how git works
> :-)

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

[Marcin Owsiany]

On Mon, Jun 25, 2012 at 10:01:06AM -0700, Junio C Hamano wrote:
> Marcin Owsiany <marcin@owsiany.pl> writes:
> 
> >>  What are you really trying
> >> to validate?  "HEAD" points at an existing branch and you do not
> >> care what branch it is?
> >
> > Yes. I think.
> 
> Why do you even care about the value of HEAD, i.e. the output from
> "rev-parse HEAD", if that is the case?

I don't!

> Wouldn't you rather be
> reading from the output "symbolic-ref HEAD" to see if it points at
> any branch?

Sure, I was simply not aware of its existence.

However after actually trying this approach I have found out that when
post_fetch_checkout runs at initial "clone", HEAD points at
refs/heads/master, but refs/heads/master does not exist! So just
checking HEAD is not enough, I need to verify that it points to
something valid. How about this:

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". While it is useful that it gets created initially (users expect this
git convention), 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.

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

diff --git a/git-svn.perl b/git-svn.perl
index 0b074c4..2379a71 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -1612,9 +1612,9 @@ sub post_fetch_checkout {
 		}
 	}
 
-	my $valid_head = verify_ref('HEAD^0');
+	return if verify_ref('HEAD^0');
 	command_noisy(qw(update-ref refs/heads/master), $gs->refname);
-	return if ($valid_head || !verify_ref('HEAD^0'));
+	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

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

[Junio C Hamano]

Marcin Owsiany <marcin@owsiany.pl> writes:

> diff --git a/git-svn.perl b/git-svn.perl
> index 0b074c4..2379a71 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -1612,9 +1612,9 @@ sub post_fetch_checkout {
>  		}
>  	}
>  
> -	my $valid_head = verify_ref('HEAD^0');
> +	return if verify_ref('HEAD^0');
>  	command_noisy(qw(update-ref refs/heads/master), $gs->refname);

Given that your original motivation was "I do not want master, I am
using something else for my primary branch", I change that still
shows "update-ref refs/heads/master" smells like sweeping something
under the rug (i.e. repeated "oh, running this verison still fails
here, so I'll patch it up" hackery without a solid grand design).

> -	return if ($valid_head || !verify_ref('HEAD^0'));
> +	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

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

[Marcin Owsiany]

On Tue, Jun 26, 2012 at 03:03:07PM -0700, Junio C Hamano wrote:
> Marcin Owsiany <marcin@owsiany.pl> writes:
> 
> > diff --git a/git-svn.perl b/git-svn.perl
> > index 0b074c4..2379a71 100755
> > --- a/git-svn.perl
> > +++ b/git-svn.perl
> > @@ -1612,9 +1612,9 @@ sub post_fetch_checkout {
> >  		}
> >  	}
> >  
> > -	my $valid_head = verify_ref('HEAD^0');
> > +	return if verify_ref('HEAD^0');
> >  	command_noisy(qw(update-ref refs/heads/master), $gs->refname);
> 
> Given that your original motivation was "I do not want master, I am
> using something else for my primary branch", I change that still
> shows "update-ref refs/heads/master" smells like sweeping something
> under the rug

I'm not so sure... With this change, git-svn will only create master on
the initial "clone" and I think that's fine. It's consistent with what
"git clone" does when cloning a regular git repository.

It seems that I have slightly misinterpreted git-svn's actions in my
initial post in 2009. I thought it always updated "master" to the
most recent upstream commit. In reality it only every _creates_ it at
the most recent commit. But never fast-forwards it if it pre-exists.

This makes my idea to do the same to "my something else instead of
master" much less attractive. In fact I don't think such behaviour would
be useful.

I think with the suggested patch git-svn works as I would like it to:
 - creates "master" at initial checkout - consistent with git clone
 - using a different "tracking-like" branch is possible with "dcommit"
 - does not re-create "master" on fetch - so the annoying part is gone

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

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

[Marcin Owsiany]

On Tue, Jun 26, 2012 at 11:32:15PM +0100, Marcin Owsiany wrote:
> On Tue, Jun 26, 2012 at 03:03:07PM -0700, Junio C Hamano wrote:
> > Marcin Owsiany <marcin@owsiany.pl> writes:
> > 
> > > diff --git a/git-svn.perl b/git-svn.perl
> > > index 0b074c4..2379a71 100755
> > > --- a/git-svn.perl
> > > +++ b/git-svn.perl
> > > @@ -1612,9 +1612,9 @@ sub post_fetch_checkout {
> > >  		}
> > >  	}
> > >  
> > > -	my $valid_head = verify_ref('HEAD^0');
> > > +	return if verify_ref('HEAD^0');
> > >  	command_noisy(qw(update-ref refs/heads/master), $gs->refname);
> > 
> > Given that your original motivation was "I do not want master, I am
> > using something else for my primary branch", I change that still
> > shows "update-ref refs/heads/master" smells like sweeping something
> > under the rug
> 
> I'm not so sure... With this change, git-svn will only create master on
> the initial "clone" and I think that's fine. It's consistent with what
> "git clone" does when cloning a regular git repository.
> 
> It seems that I have slightly misinterpreted git-svn's actions in my
> initial post in 2009. I thought it always updated "master" to the
> most recent upstream commit. In reality it only every _creates_ it at
> the most recent commit. But never fast-forwards it if it pre-exists.
> 
> This makes my idea to do the same to "my something else instead of
> master" much less attractive. In fact I don't think such behaviour would
> be useful.
> 
> I think with the suggested patch git-svn works as I would like it to:
>  - creates "master" at initial checkout - consistent with git clone
>  - using a different "tracking-like" branch is possible with "dcommit"
>  - does not re-create "master" on fetch - so the annoying part is gone

Any comments?

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

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

[Junio C Hamano]

Marcin Owsiany <marcin@owsiany.pl> writes:

>> This makes my idea to do the same to "my something else instead of
>> master" much less attractive. In fact I don't think such behaviour would
>> be useful.
>> 
>> I think with the suggested patch git-svn works as I would like it to:
>>  - creates "master" at initial checkout - consistent with git clone
>>  - using a different "tracking-like" branch is possible with "dcommit"
>>  - does not re-create "master" on fetch - so the annoying part is gone
>
> Any comments?

Not from me.  Even though I'd love to hear Eric's opinion, your "I
don't think such behaviour would be useful." gave me an impression
that you would justify the change in a different way (i.e. a rewrite
of proposed log message) or tweak the patch (i.e. a modified
behaviour), or perhaps both, in your re-roll, the ball was in your
court, and we were waiting for such a rerolled patch.

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

[Eric Wong]

Junio C Hamano <gitster@pobox.com> wrote:
> Marcin Owsiany <marcin@owsiany.pl> writes:
> 
> >> This makes my idea to do the same to "my something else instead of
> >> master" much less attractive. In fact I don't think such behaviour would
> >> be useful.
> >> 
> >> I think with the suggested patch git-svn works as I would like it to:
> >>  - creates "master" at initial checkout - consistent with git clone
> >>  - using a different "tracking-like" branch is possible with "dcommit"
> >>  - does not re-create "master" on fetch - so the annoying part is gone
> >
> > Any comments?
> 
> Not from me.  Even though I'd love to hear Eric's opinion, your "I
> don't think such behaviour would be useful." gave me an impression
> that you would justify the change in a different way (i.e. a rewrite
> of proposed log message) or tweak the patch (i.e. a modified
> behaviour), or perhaps both, in your re-roll, the ball was in your
> court, and we were waiting for such a rerolled patch.

Sorry, I keep forgetting this topic.  But yes, I thought you would tweak
your patch.

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

[Marcin Owsiany]

On Wed, Jul 11, 2012 at 01:26:17AM +0000, Eric Wong wrote:
> Junio C Hamano <gitster@pobox.com> wrote:
> > Marcin Owsiany <marcin@owsiany.pl> writes:
> > 
> > >> This makes my idea to do the same to "my something else instead of
> > >> master" much less attractive. In fact I don't think such behaviour would
> > >> be useful.
> > >> 
> > >> I think with the suggested patch git-svn works as I would like it to:
> > >>  - creates "master" at initial checkout - consistent with git clone
> > >>  - using a different "tracking-like" branch is possible with "dcommit"
> > >>  - does not re-create "master" on fetch - so the annoying part is gone
> > >
> > > Any comments?
> > 
> > Not from me.  Even though I'd love to hear Eric's opinion, your "I
> > don't think such behaviour would be useful." gave me an impression
> > that you would justify the change in a different way (i.e. a rewrite
> > of proposed log message) or tweak the patch (i.e. a modified
> > behaviour), or perhaps both, in your re-roll, the ball was in your
> > court, and we were waiting for such a rerolled patch.
> 
> Sorry, I keep forgetting this topic.  But yes, I thought you would tweak
> your patch.

Oh, I guess I got used to projects where people pay no attention to
patch comments. How about this:

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 points at a valid head. This
means "master" does get created on initial "clone" but does not get recreated
once a user deletes it.

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

diff --git a/git-svn.perl b/git-svn.perl
index 0b074c4..2379a71 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -1612,9 +1612,9 @@ sub post_fetch_checkout {
 		}
 	}
 
-	my $valid_head = verify_ref('HEAD^0');
+	return if verify_ref('HEAD^0');
 	command_noisy(qw(update-ref refs/heads/master), $gs->refname);
-	return if ($valid_head || !verify_ref('HEAD^0'));
+	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

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

[Junio C Hamano]

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

>         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);
>         }
>  }

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

[Marcin Owsiany]

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

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

[Eric Wong]

Marcin Owsiany <marcin@owsiany.pl> wrote:
> On Wed, Jul 11, 2012 at 03:56:43PM -0700, Junio C Hamano wrote:
> > 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.

Ugh, I always forget the Git.pm API, too.  Perhaps command_noisy should
be made to respect signals in exit codes (the rest of git-svn is
compromised by this behavior in command_noisy, too, it turns out... :x)

I'm not sure what else would break if command_noisy were changed,
git-svn appears to be the only user in git.git.

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

[Marcin Owsiany]

On Wed, Jul 18, 2012 at 11:27:22AM +0000, Eric Wong wrote:
> Marcin Owsiany <marcin@owsiany.pl> wrote:
> > On Wed, Jul 11, 2012 at 03:56:43PM -0700, Junio C Hamano wrote:
> > > 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.
> 
> Ugh, I always forget the Git.pm API, too.  Perhaps command_noisy should
> be made to respect signals in exit codes (the rest of git-svn is
> compromised by this behavior in command_noisy, too, it turns out... :x)
> 
> I'm not sure what else would break if command_noisy were changed,
> git-svn appears to be the only user in git.git.

Other "command" flavours should probably also be changed to match?

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

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

[Junio C Hamano]

Marcin Owsiany <marcin@owsiany.pl> writes:

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

I am happy with this version, as long as Eric is happy ;-)

Thanks.

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

[Eric Wong]

Marcin Owsiany <marcin@owsiany.pl> wrote:
> On Wed, Jul 18, 2012 at 11:27:22AM +0000, Eric Wong wrote:
> > Marcin Owsiany <marcin@owsiany.pl> wrote:
> > > 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.
> > 
> > Ugh, I always forget the Git.pm API, too.  Perhaps command_noisy should
> > be made to respect signals in exit codes (the rest of git-svn is
> > compromised by this behavior in command_noisy, too, it turns out... :x)
> > 
> > I'm not sure what else would break if command_noisy were changed,
> > git-svn appears to be the only user in git.git.
> 
> Other "command" flavours should probably also be changed to match?

Probably, I'm not sure if it'd break existing uses.  Anyways, that's a
separate issue we can deal with another day.

I've added my Signed-off-by: to your latest patch and pushed
to "master" of git://bogomips.org/git-svn.git
(commit e3bd4ddaa9a60fa4e70efdb143b434b440d6cec4)

Marcin Owsiany (1):
      git-svn: don't create master if another head exists

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

[Junio C Hamano]

Eric Wong <normalperson@yhbt.net> writes:

> Probably, I'm not sure if it'd break existing uses.  Anyways, that's a
> separate issue we can deal with another day.
>
> I've added my Signed-off-by: to your latest patch and pushed
> to "master" of git://bogomips.org/git-svn.git
> (commit e3bd4ddaa9a60fa4e70efdb143b434b440d6cec4)

Thanks.