Re: [PATCH/RFC] git-svn: don't create master if another head exists
Marcin Owsiany mar...@owsiany.pl wrote: On Wed, Jul 18, 2012 at 11:27:22AM +, Eric Wong wrote: Marcin Owsiany mar...@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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC] git-svn: don't create master if another head exists
Eric Wong normalper...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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:
Marcin Owsiany mar...@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 mar...@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 mar...@owsiany.pl
---
git-svn.perl |9 -
1 files changed, 4 insertions(+), 5
Re: [PATCH/RFC] git-svn: don't create master if another head exists
On Wed, Jul 18, 2012 at 11:27:22AM +, Eric Wong wrote: Marcin Owsiany mar...@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 mar...@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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC] git-svn: don't create master if another head exists
Marcin Owsiany mar...@owsiany.pl writes:
PTAL:
From: Marcin Owsiany mar...@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 mar...@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.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC] git-svn: don't create master if another head exists
On Wed, Jul 11, 2012 at 01:26:17AM +, Eric Wong wrote:
Junio C Hamano gits...@pobox.com wrote:
Marcin Owsiany mar...@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 mar...@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 mar...@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 mar...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC] git-svn: don't create master if another head exists
Marcin Owsiany mar...@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);
}
}
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC] git-svn: don't create master if another head exists
Junio C Hamano gits...@pobox.com wrote: Marcin Owsiany mar...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC] git-svn: don't create master if another head exists
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 mar...@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 mar...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC] git-svn: don't create master if another head exists
Marcin Owsiany mar...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
