threads / patch / 12675

patchgit-cvsimport: fix initial checkout

Subject: [PATCH] git-cvsimport: fix initial checkout

## tl;dr

9 messages between Mar 13, 2008 and Mar 13, 2008. Diffs are folded; open one to read it.

replies: 8people: 3as markdown or json

Marc-Andre Lureau· Mar 13, 2008, 19:09 UTC · lore
git-symbolic-ref HEAD returns master reference, even if the file does
not exists. That prevents the initial checkout and fails in
git-rev-parse. The patch checks the existence of the reference file
before assuming an original branch exists. There might be better
solutions than checking file existence.
---
 git-cvsimport.perl |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)
Show changes to git-cvsimport.perl +8 −3
diff --git a/git-cvsimport.perl b/git-cvsimport.perl
index 95c5eec..1512fe4 100755
--- a/git-cvsimport.perl
+++ b/git-cvsimport.perl
@@ -570,12 +570,16 @@ unless (-d $git_dir) {
 	open(F, "git-symbolic-ref HEAD |") or
 		die "Cannot run git-symbolic-ref: $!\n";
 	chomp ($last_branch = <F>);
-	$last_branch = basename($last_branch);
-	close(F);
-	unless ($last_branch) {
+	if (-f "$git_dir/$last_branch") {
+	    $last_branch = basename($last_branch);
+	    unless ($last_branch) {
 		warn "Cannot read the last branch name: $! -- assuming 'master'\n";
 		$last_branch = "master";
+	    }
+	} else {
+	    $last_branch = "";
 	}
+	close(F);
 	$orig_branch = $last_branch;
 	$tip_at_start = `git-rev-parse --verify HEAD`;
 
@@ -953,6 +957,7 @@ while (<CVS>) {
 		print "* UNKNOWN LINE * $_\n";
 	}
 }
+
 commit() if $branch and $state != 11;
 
 unless ($opt_P) {

-- 
1.5.4.4.534.gfb90c.dirty
Junio C Hamano· Mar 13, 2008, 21:05 UTC · re: Marc-Andre Lureau · lore

Re: [PATCH] git-cvsimport: fix initial checkout

Marc-Andre Lureau <marcandre.lureau@gmail.com> writes:
> git-symbolic-ref HEAD returns master reference, even if the file does
> not exists. That prevents the initial checkout and fails in
> git-rev-parse.

I had an impression that this check was deliberately done, but I do not recall the details. Martin?

Show 17 quoted lines
> diff --git a/git-cvsimport.perl b/git-cvsimport.perl
> index 95c5eec..1512fe4 100755
> --- a/git-cvsimport.perl
> +++ b/git-cvsimport.perl
> @@ -570,12 +570,16 @@ unless (-d $git_dir) {
>  	open(F, "git-symbolic-ref HEAD |") or
>  		die "Cannot run git-symbolic-ref: $!\n";
>  	chomp ($last_branch = <F>);
> -	$last_branch = basename($last_branch);
> -	close(F);
> -	unless ($last_branch) {
> +	if (-f "$git_dir/$last_branch") {
> +	    $last_branch = basename($last_branch);
> +	    unless ($last_branch) {
>  		warn "Cannot read the last branch name: $! -- assuming 'master'\n";
>  		$last_branch = "master";
> +	    }

In any case, what if last_branch is a branch with hierarchical name, I have to wonder. If you are on a branch whose name has a slash in it, like "frotz/nitfol" when you start cvsimport, doesn't this (before or after the patch) code break your repository?

Martin Langhoff· Mar 13, 2008, 22:18 UTC · re: Marc-Andre Lureau · lore

Re: [PATCH] git-cvsimport: fix initial checkout

Marc-Andre Lureau wrote:
> git-symbolic-ref HEAD returns master reference, even if the file does
> not exists. That prevents the initial checkout and fails in
> git-rev-parse.

But you are patching the block that gets triggered on subsequent imports, this code does not deal with "initial checkout" unless something else is wrong. The line right above the open() is an else that has the block that matters.

> The patch checks the existence of the reference file
> before assuming an original branch exists. There might be better
> solutions than checking file existence.

There are indeed. If we need this patch -- then you can call git ref-parse right to see if you get a sha1.

> -	unless ($last_branch) {
> +	if (-f "$git_dir/$last_branch") {

Note that the file won't exist there in any modern git. It will be in $git_dir/refs/heads/$last_branch. Did you test this patch?

What's your workflow with cvsimport? Perhaps you are doing something strange with it... :-)

cheers,
martin
-- 
-----------------------------------------------------------------------
Martin @ Catalyst .Net .NZ  Ltd, PO Box 11-053, Manners St,  Wellington
WEB: http://catalyst.net.nz/           PHYS: Level 2, 150-154 Willis St
NZ: +64(4)916-7224    MOB: +64(21)364-017    UK: 0845 868 5733 ext 7224
      Make things as simple as possible, but no simpler - Einstein
-----------------------------------------------------------------------
Marc-André Lureau· Mar 13, 2008, 23:04 UTC · re: Martin Langhoff · lore

Re: [PATCH] git-cvsimport: fix initial checkout

Hi

On Fri, Mar 14, 2008 at 12:18 AM, Martin Langhoff <martin@catalyst.net.nz> wrote:

Show 10 quoted lines
> Marc-Andre Lureau wrote:
>  > git-symbolic-ref HEAD returns master reference, even if the file does
>  > not exists. That prevents the initial checkout and fails in
>  > git-rev-parse.
>
>  But you are patching the block that gets triggered on subsequent
>  imports, this code does not deal with "initial checkout" unless
>  something else is wrong. The line right above the open() is an else that
>  has the block that matters.
>
Yeah, it failed in the middle of a ~4h import, I did not restart it.

git-cvsimport -r cvs -p b,HEAD -k -m -a -v -d :pserver:anoncvs@anoncvs.freedesktop.org:/cvs/gstreamer -C gst-plugins-good gst-plugins-good

This is a quite problematic CVS, btw. (missing patches/files in the end, branch merge fail ... see my previous patch)

Show 6 quoted lines
>  > The patch checks the existence of the reference file
>  > before assuming an original branch exists. There might be better
>  > solutions than checking file existence.
>
>  There are indeed. If we need this patch -- then you can call git
>  ref-parse right to see if you get a sha1.

Ok, which one is prefered? ref-parse I guess? I am mostly ignorant of all the plumbing stuff.

Show 6 quoted lines
>  > -     unless ($last_branch) {
>  > +     if (-f "$git_dir/$last_branch") {
>
>  Note that the file won't exist there in any modern git. It will be in
>  $git_dir/refs/heads/$last_branch. Did you test this patch?
>

Crap. The patch indeed worked, because the file did not exist. The second time it also worked:

skip patchset 5825: 1205277122 before 1205418644 skip patchset 5826: 1205418644 before 1205418644 DONE. Already up-to-date. *** Building gst-plugins-good *** [1/145] make -j2

Result is here: http://git.infradead.org/users/elmarco/gst-plugins-good.git
Thanks for the review!!
-- 
Marc-André Lureau
Martin Langhoff· Mar 13, 2008, 23:29 UTC · re: Marc-André Lureau · lore

Re: [PATCH] git-cvsimport: fix initial checkout

Marc-André Lureau wrote:
> Yeah, it failed in the middle of a ~4h import, I did not restart it.

there are no guarantees then of correctness (there seldom are, but a failed import can leave your repo in a number of odd states...).

I'd suggest parsecvs for the initial import of a messy repo. You can continue to do incrementas with cvsimport after the initial import.

cheers,
m
-- 
-----------------------------------------------------------------------
Martin @ Catalyst .Net .NZ  Ltd, PO Box 11-053, Manners St,  Wellington
WEB: http://catalyst.net.nz/           PHYS: Level 2, 150-154 Willis St
NZ: +64(4)916-7224    MOB: +64(21)364-017    UK: 0845 868 5733 ext 7224
      Make things as simple as possible, but no simpler - Einstein
-----------------------------------------------------------------------
Junio C Hamano· Mar 13, 2008, 23:15 UTC · re: Martin Langhoff · lore

Re: [PATCH] git-cvsimport: fix initial checkout

Martin Langhoff <martin@catalyst.net.nz> writes:
Show 22 quoted lines
> Marc-Andre Lureau wrote:
>> git-symbolic-ref HEAD returns master reference, even if the file does
>> not exists. That prevents the initial checkout and fails in
>> git-rev-parse.
>
> But you are patching the block that gets triggered on subsequent
> imports, this code does not deal with "initial checkout" unless
> something else is wrong. The line right above the open() is an else that
> has the block that matters.
>
>> The patch checks the existence of the reference file
>> before assuming an original branch exists. There might be better
>> solutions than checking file existence.
>
> There are indeed. If we need this patch -- then you can call git
> ref-parse right to see if you get a sha1.
>
>> -	unless ($last_branch) {
>> +	if (-f "$git_dir/$last_branch") {
>
> Note that the file won't exist there in any modern git. It will be in
> $git_dir/refs/heads/$last_branch. Did you test this patch?

Martin, it may not even be in $git_dir/refs/heads/$last_branch ;-) The refs can be packed.

By the way, doesn't cvsimport fail when your HEAD is detached with this code?

I always have cvsimport update the pristine upstream branch and rebase my work against it, so I never have the branch cvsimport updates checked out, and for meit seems to work wonderfully (well, at least as wonderful as a workflow that involves any CVS in it could be). I do not see a reason why it should not to work similarly well when my HEAD is detached..

Martin Langhoff· Mar 13, 2008, 23:27 UTC · re: Junio C Hamano · lore

Re: [PATCH] git-cvsimport: fix initial checkout

Junio C Hamano wrote:
> Martin, it may not even be in $git_dir/refs/heads/$last_branch ;-)  The
> refs can be packed.

Yes. I was suggesting git-ref-parse to support packed refs, but this slipped ;-)

> By the way, doesn't cvsimport fail when your HEAD is detached with this
> code?

Perhaps. Importing on a working repo with -i is a bit messy, and detached heads may well break it. I'm not that conversant on how detached heads look ;-)

Show 5 quoted lines
> I always have cvsimport update the pristine upstream branch and rebase my
> work against it, so I never have the branch cvsimport updates checked
> out, and  for meit seems to work wonderfully (well, at least as wonderful
> as a workflow that involves any CVS in it could be).  I do not see a
> reason why it should not to work similarly well when my HEAD is detached..
sounds reasonable.
m
-- 
-----------------------------------------------------------------------
Martin @ Catalyst .Net .NZ  Ltd, PO Box 11-053, Manners St,  Wellington
WEB: http://catalyst.net.nz/           PHYS: Level 2, 150-154 Willis St
NZ: +64(4)916-7224    MOB: +64(21)364-017    UK: 0845 868 5733 ext 7224
      Make things as simple as possible, but no simpler - Einstein
-----------------------------------------------------------------------
Marc-André Lureau· Mar 13, 2008, 23:22 UTC · re: Martin Langhoff · lore

Re: [PATCH] git-cvsimport: fix initial checkout

Hi

On Fri, Mar 14, 2008 at 12:18 AM, Martin Langhoff <martin@catalyst.net.nz> wrote:

Show 6 quoted lines
>  > -     unless ($last_branch) {
>  > +     if (-f "$git_dir/$last_branch") {
>
>  Note that the file won't exist there in any modern git. It will be in
>  $git_dir/refs/heads/$last_branch. Did you test this patch?
>

git-symbolic-ref HEAD do return "refs/heads/master" on my initial in-the-middle checkout (I still have a copy),

So it seems correct for now, but i'll change it to use rev-parse instead as it seems more correct.

Regards
-- 
Marc-André Lureau

← back to recent threads