[PATCH/RFC] fast-import: allow "reset" without "from" to delete a branch

[PATCH/RFC] fast-import: allow "reset" without "from" to delete a branch

[Eyvind Bernhardsen]

Resetting a branch without "from" and not making any further commits
to it currently causes fast-import to fail with an error message.

This patch prevents the error, allowing "reset" to be used to delete
a branch.

Signed-off-by: Eyvind Bernhardsen <eyvind-git@orakel.ntnu.no>
---
Since commit c3b0dec ("Be more careful about updating refs"), git fast- 
import has given the following error message on every import from  
cvs2svn:

	error: Trying to write ref refs/heads/TAG.FIXUP with nonexistant  
object 0000000000000000000000000000000000000000
	error: Unable to update refs/heads/TAG.FIXUP

The imported repository is fine, but the error message finally bugged  
me enough to figure out what was going on, and the explanation is  
simple.  If a branch is reset in fast-import, and no further commits  
are made on that branch, the final dump_branches() call in fast- 
import.c fails.

cvs2svn creates a TAG.FIXUP branch for every tag and then resets it  
after the tag has been set. The intent is that TAG.FIXUP should be  
deleted, and this patch makes that work without error (the branch is  
actually deleted even without this patch).

It's a small change and the test suite passes, but I'm not sure if  
using reset to delete a branch is desired behaviour, so I would  
appreciate it if someone who actually knows what they are doing could  
take a look at it :)

  fast-import.c |    5 +++--
  1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index 655913d..989ba94 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1539,8 +1539,9 @@ static int update_branch(struct branch *b)
  			return -1;
  		}
  	}
-	if (write_ref_sha1(lock, b->sha1, msg) < 0)
-		return error("Unable to update %s", b->name);
+	if (!is_null_sha1(b->sha1))
+		if (write_ref_sha1(lock, b->sha1, msg) < 0)
+			return error("Unable to update %s", b->name);
  	return 0;
  }

-- 
1.5.4.4.555.ga98c.dirty

Re: [PATCH/RFC] fast-import: allow "reset" without "from" to delete a branch

[Shawn O. Pearce]

Eyvind Bernhardsen <eyvind-git@orakel.ntnu.no> wrote:
> It's a small change and the test suite passes, but I'm not sure if  
> using reset to delete a branch is desired behaviour, so I would  
> appreciate it if someone who actually knows what they are doing could  
> take a look at it :)

I think this is a slightly better patch, as it avoids creating a
lock file around the ref if we aren't going to actually alter it.

At present fast-import does not allow an application to delete a
branch that existed when fast-import started, but if the branch
was strictly transient within the fast-import process (like the
cvs2svn TAG.FIXUP) then there is no problem.

Is this patch acceptable?  Note it is from you, I carried in your
commit message, SBO, etc.
 
--8>--
From: Eyvind Bernhardsen <eyvind-git@orakel.ntnu.no>
Subject: [PATCH] fast-import: allow "reset" without "from" to delete temporary branch

Resetting a branch without "from" and not making any further commits
to it currently causes fast-import to fail with an error message.

This patch prevents the error, allowing "reset" to be used to delete
a branch.

Signed-off-by: Eyvind Bernhardsen <eyvind-git@orakel.ntnu.no>
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 fast-import.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index 655913d..73e5439 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1516,6 +1516,8 @@ static int update_branch(struct branch *b)
 	struct ref_lock *lock;
 	unsigned char old_sha1[20];
 
+	if (is_null_sha1(b->sha1))
+		return 0;
 	if (read_ref(b->name, old_sha1))
 		hashclr(old_sha1);
 	lock = lock_any_ref_for_update(b->name, old_sha1, 0);
-- 
1.5.4.4.640.g8ae62

-- 
Shawn.

[PATCH] fast-import: Allow "reset" to delete a new branch without error

[Eyvind Bernhardsen]

Creating a branch in fast-import and then resetting it without making
any further commits to it currently causes an error message at the
end of the import.

This error is triggered by cvs2svn's git backend, which uses a
temporary fixup branch when it creates tags, because the fixup branch
is reset after each tag.

This patch prevents the error, allowing "reset" to be used to delete
temporary branches.

Signed-off-by: Eyvind Bernhardsen <eyvind-git@orakel.ntnu.no>
---
On 16. mars. 2008, at 05.12, Shawn O. Pearce wrote:

> I think this is a slightly better patch, as it avoids creating a
> lock file around the ref if we aren't going to actually alter it.


Yes, that's a much better patch, and since you pointed out that  
existing branches won't be deleted, here it is again with a better  
commit message.  Thanks!

  fast-import.c |    2 ++
  1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index 655913d..73e5439 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1516,6 +1516,8 @@ static int update_branch(struct branch *b)
  	struct ref_lock *lock;
  	unsigned char old_sha1[20];

+	if (is_null_sha1(b->sha1))
+		return 0;
  	if (read_ref(b->name, old_sha1))
  		hashclr(old_sha1);
  	lock = lock_any_ref_for_update(b->name, old_sha1, 0);
-- 
1.5.4.4.608.gc20d.dirty

Re: [PATCH] fast-import: Allow "reset" to delete a new branch without error

[Junio C Hamano]

Eyvind Bernhardsen <eyvind-git@orakel.ntnu.no> writes:

> On 16. mars. 2008, at 05.12, Shawn O. Pearce wrote:
>
>> I think this is a slightly better patch, as it avoids creating a
>> lock file around the ref if we aren't going to actually alter it.
>
> Yes, that's a much better patch, and since you pointed out that
> existing branches won't be deleted, here it is again with a better
> commit message.  Thanks!

I'll add Acked-by from Shawn and apply.  Thanks, both.