[PATCH v0] fast-import: Add drop command

First try in implementing the drop command that was discussed in the
past. Please be gentle, as this is the first time I touch C in many
years. The git internals are also rather new to me.

No documentation and test cases were added at this point. It would be
great if someone could implement the test case for me.

Vitor Antunes (1):
  fast-import: Add drop command

 fast-import.c |   35 ++++++++++++++++++++++++++++++++++-
 1 files changed, 34 insertions(+), 1 deletions(-)

-- 
1.7.7.rc2.11.g4aecf.dirty

The drop command deletes the given branch reference, allowing
fast-import to actively ignore it in the final checks.

Signed-off-by: Vitor Antunes <redacted>
---
 fast-import.c |   35 ++++++++++++++++++++++++++++++++++-
 1 files changed, 34 insertions(+), 1 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index 742e7da..906bbf4 100644
--- a/fast-import.c
+++ b/fast-import.c

@@ -743,6 +743,29 @@ static struct branch *new_branch(const char *name)
 	return b;
 }
 
+static void release_tree_entry(struct tree_entry *e);
+static void drop_branch(const char *name)
+{
+	unsigned int hc = hc_str(name, strlen(name)) % branch_table_sz;
+	struct branch *b_prev = NULL, *b = NULL;
+	struct ref_lock *lock;
+	unsigned char old_sha1[20];
+
+	for (b = branch_table[hc]; b; b = b->table_next_branch) {
+		if (!strcmp(name, b->name)) {
+			release_tree_entry(&b->branch_tree);
+			if (b_prev)
+				b_prev->table_next_branch = b->table_next_branch;
+			branch_table[hc] = NULL;
+			branch_count--;
+		}
+		b_prev = b;
+	}
+
+	if (!read_ref(name, old_sha1))
+		delete_ref(name, old_sha1, 0)
+}
+
 static unsigned int hc_entries(unsigned int cnt)
 {
 	cnt = cnt & 7 ? (cnt / 8) + 1 : cnt / 8;

@@ -776,7 +799,6 @@ static struct tree_content *new_tree_content(unsigned int cnt)
 	return t;
 }
 
-static void release_tree_entry(struct tree_entry *e);
 static void release_tree_content(struct tree_content *t)
 {
 	struct avail_tree_content *f = (struct avail_tree_content*)t;

@@ -2793,6 +2815,15 @@ static void parse_reset_branch(void)
 		unread_command_buf = 1;
 }
 
+static void parse_drop_branch(void)
+{
+	char *sp;
+
+	/* Obtain the branch name from the rest of our command */
+	sp = strchr(command_buf.buf, ' ') + 1;
+	drop_branch(sp);
+}
+
 static void cat_blob_write(const char *buf, unsigned long size)
 {
 	if (write_in_full(cat_blob_fd, buf, size) != size)

@@ -3332,6 +3363,8 @@ int main(int argc, const char **argv)
 			parse_new_tag();
 		else if (!prefixcmp(command_buf.buf, "reset "))
 			parse_reset_branch();
+		else if (!prefixcmp(command_buf.buf, "drop "))
+			parse_drop_branch();
 		else if (!strcmp("checkpoint", command_buf.buf))
 			parse_checkpoint();
 		else if (!strcmp("done", command_buf.buf))

-- 
1.7.7.rc2.11.g4aecf.dirty

Vitor Antunes wrote:

The drop command deletes the given branch reference, allowing
fast-import to actively ignore it in the final checks.

Thanks.  I must have missed the earlier discussion.  What are the
semantics of this command and its intended purpose?  For example, what
happens if the branch already existed or if there is a checkpoint
(perhaps triggered by the impatient user sending SIGUSR1 to
fast-import) before the "drop" command is processed?

Jonathan

On Sun, Sep 25, 2011 at 1:37 AM, Jonathan Nieder [off-list ref] wrote:

Vitor Antunes wrote:

quoted

The drop command deletes the given branch reference, allowing
fast-import to actively ignore it in the final checks.

Thanks.  I must have missed the earlier discussion.  What are the
semantics of this command and its intended purpose?

My guess is that if fast-import is used to manage a set of "remote"
branches, it should be able to delete branches. Then, it should
be allowed to do non-fastforward updates too (--force). Why can't
it just ignore branches deletion (considering --force)?

Random thoughts:
1. once 'drop' is executed, fast-import can't tell if the branch was
actually deleted. And moreover any attempt to read this branch
head becomes illegal (either it's missing in .git or fast-import is
instructed to use a dropped branch).
2. 'reset' command is a bit like proposed 'drop' but it never deletes
a branch ref. Consider following imports:
1) import branch topic
2) reset topic
3) import branch topic2 starting at topic (incorrect import)
If 1-3) is done in one fast-import process, the error is reported.
If 3) is done separately, it succeeds but the result is strange:
topic2 isn't started from scratch but from old "erased" topic.
So, maybe, reset should be fixed to erase branches on --force.

One more scenario is:
1) import topic
2) reset topic
3) import topic
If 1-3) go together - no error
If 3) goes separate - no error, but non-fastforward update.
Much more harmless, but still may look strange.

For example, what
happens if the branch already existed or if there is a checkpoint
(perhaps triggered by the impatient user sending SIGUSR1 to
fast-import) before the "drop" command is processed?

I think that actual ref deletion should take place in update_branch().
So all the cases would be handled as usual.

Jonathan

On Sep 24, 2011 8:37 PM, "Jonathan Nieder" [off-list ref] wrote:

Thanks.  I must have missed the earlier discussion.  What are the
semantics of this command and its intended purpose?  For example, what
happens if the branch already existed or if there is a checkpoint
(perhaps triggered by the impatient user sending SIGUSR1 to
fast-import) before the "drop" command is processed?

In the tests I made there are checkpoints triggered before using the
command. I tried to remove the branch within fast-import variables as well
as the already processed objects in git.

This command is required because I need to reset a given branch multiple
times in order to be able to "guess" its origin commit in the parent branch.
To make this analysis I also need to use "checkpoint" at each try.

(Resending... apparently gmail Android app sends a HTML attachment)

On Sat, Sep 24, 2011 at 10:19 PM, Dmitry Ivankov [off-list ref] wrote:

On Sun, Sep 25, 2011 at 1:37 AM, Jonathan Nieder [off-list ref] wrote:

quoted

Thanks.  I must have missed the earlier discussion.  What are the
semantics of this command and its intended purpose?

My guess is that if fast-import is used to manage a set of "remote"
branches, it should be able to delete branches. Then, it should
be allowed to do non-fastforward updates too (--force). Why can't
it just ignore branches deletion (considering --force)?

I started by using --force, but I did not want to completely disable
these checks. The idea of the drop command is to add support to the
exceptions that require non-fastforward updates.

Random thoughts:
1. once 'drop' is executed, fast-import can't tell if the branch was
actually deleted. And moreover any attempt to read this branch
head becomes illegal (either it's missing in .git or fast-import is
instructed to use a dropped branch).
2. 'reset' command is a bit like proposed 'drop' but it never deletes
a branch ref. Consider following imports:
1) import branch topic
2) reset topic
3) import branch topic2 starting at topic (incorrect import)
If 1-3) is done in one fast-import process, the error is reported.
If 3) is done separately, it succeeds but the result is strange:
topic2 isn't started from scratch but from old "erased" topic.
So, maybe, reset should be fixed to erase branches on --force.

I think you are not considering the possibility that checkpoints could
have been done along the way. I use them frequently to be able to
analyse branches with diff-tree. As soon as a checkpoint is done,
update-branches will issue an error (commit A is not part of branch A').

One more scenario is:
1) import topic
2) reset topic
3) import topic
If 1-3) go together - no error
If 3) goes separate - no error, but non-fastforward update.
Much more harmless, but still may look strange.

Not exactly true if there is a checkpoint done after step 1.

My scenario is:

1) import topic
2) checkpoint
3) diff-tree and processing
4) exit if processing returns ok
5) reset topic to another HEAD
6) goto 1)

-- 
Vitor Antunes

Hi,

This thread did not receive any updates for a long time.
Could someone provide some feedback?

Is this feasible? Does it make sense to add this command? If not, why?

Thanks,
Vitor

On Tue, Sep 27, 2011 at 9:57 AM, Vitor Antunes [off-list ref] wrote:

On Sat, Sep 24, 2011 at 10:19 PM, Dmitry Ivankov [off-list ref] wrote:

quoted

On Sun, Sep 25, 2011 at 1:37 AM, Jonathan Nieder [off-list ref] wrote:

quoted

Thanks.  I must have missed the earlier discussion.  What are the
semantics of this command and its intended purpose?

My guess is that if fast-import is used to manage a set of "remote"
branches, it should be able to delete branches. Then, it should
be allowed to do non-fastforward updates too (--force). Why can't
it just ignore branches deletion (considering --force)?

I started by using --force, but I did not want to completely disable
these checks. The idea of the drop command is to add support to the
exceptions that require non-fastforward updates.

quoted

Random thoughts:
1. once 'drop' is executed, fast-import can't tell if the branch was
actually deleted. And moreover any attempt to read this branch
head becomes illegal (either it's missing in .git or fast-import is
instructed to use a dropped branch).
2. 'reset' command is a bit like proposed 'drop' but it never deletes
a branch ref. Consider following imports:
1) import branch topic
2) reset topic
3) import branch topic2 starting at topic (incorrect import)
If 1-3) is done in one fast-import process, the error is reported.
If 3) is done separately, it succeeds but the result is strange:
topic2 isn't started from scratch but from old "erased" topic.
So, maybe, reset should be fixed to erase branches on --force.

I think you are not considering the possibility that checkpoints could
have been done along the way. I use them frequently to be able to
analyse branches with diff-tree. As soon as a checkpoint is done,
update-branches will issue an error (commit A is not part of branch A').

quoted

One more scenario is:
1) import topic
2) reset topic
3) import topic
If 1-3) go together - no error
If 3) goes separate - no error, but non-fastforward update.
Much more harmless, but still may look strange.

Not exactly true if there is a checkpoint done after step 1.

My scenario is:

1) import topic
2) checkpoint
3) diff-tree and processing
4) exit if processing returns ok
5) reset topic to another HEAD
6) goto 1)

Heya,

On Mon, Oct 24, 2011 at 18:37, Vitor Antunes [off-list ref] wrote:

This thread did not receive any updates for a long time.
Could someone provide some feedback?

Is this feasible? Does it make sense to add this command? If not, why?

I for one welcome our new branch deleting overlords :).

You mention that checkpointing solves some of the concerns raised by
others in this thread, would automatic checkpointing be way to make
sure everything is as it should be?

-- 
Cheers,

Sverre Rabbelier

On Mon, Oct 24, 2011 at 7:01 PM, Sverre Rabbelier [off-list ref] wrote:

I for one welcome our new branch deleting overlords :).

You mention that checkpointing solves some of the concerns raised by
others in this thread, would automatic checkpointing be way to make
sure everything is as it should be?

Apparently I did not explain myself correctly. Let me try again :)

This is what I am doing:

1) import topic
2) checkpoint
3) diff-tree and processing
4) exit if processing returns ok
5) reset topic to another HEAD
6) goto 1)

In this scenario it is the checkpoint that "breaks" everything because
it will write the original tree to disk. When fast-import exits it will
find the old tree on disk but not within "topic" tree.

So, no, I don't think that automatic checkpointing would make anything
easier. Quite the opposite!

-- 
Vitor Antunes

Heya,

On Tue, Oct 25, 2011 at 11:56, Vitor Antunes [off-list ref] wrote:

On Mon, Oct 24, 2011 at 7:01 PM, Sverre Rabbelier [off-list ref] wrote:

quoted

I for one welcome our new branch deleting overlords :).

You mention that checkpointing solves some of the concerns raised by
others in this thread, would automatic checkpointing be way to make
sure everything is as it should be?

Apparently I did not explain myself correctly. Let me try again :)

This is what I am doing:

1) import topic
2) checkpoint
3) diff-tree and processing
4) exit if processing returns ok
5) reset topic to another HEAD
6) goto 1)

In this scenario it is the checkpoint that "breaks" everything because
it will write the original tree to disk. When fast-import exits it will
find the old tree on disk but not within "topic" tree.

I'm afraid I don't understand why it's a bad thing that fast-import
will find the old tree on disk, won't it just be gc-ed if it is no
longer used?

-- 
Cheers,

Sverre Rabbelier

On Thu, Oct 27, 2011 at 12:06 PM, Sverre Rabbelier [off-list ref] wrote:

I'm afraid I don't understand why it's a bad thing that fast-import
will find the old tree on disk, won't it just be gc-ed if it is no
longer used?

No, because fast-import actively checks this to make sure the frontend
script did not do anything wrong during the import. I think the check
makes sense and may help debugging a corner case the frontend script
does not support. So, using "--force" is also not a solution because it
ignores everything and not only the specific commits I want to leave
behind.

Vitor

Heya,

On Thu, Oct 27, 2011 at 13:22, Vitor Antunes [off-list ref] wrote:

On Thu, Oct 27, 2011 at 12:06 PM, Sverre Rabbelier [off-list ref] wrote:

quoted

I'm afraid I don't understand why it's a bad thing that fast-import
will find the old tree on disk, won't it just be gc-ed if it is no
longer used?

No, because fast-import actively checks this to make sure the frontend
script did not do anything wrong during the import. I think the check
makes sense and may help debugging a corner case the frontend script
does not support. So, using "--force" is also not a solution because it
ignores everything and not only the specific commits I want to leave
behind.

Ok, so the problem is that fast-import notices that a tree that was
written out as part of a checkpoint is later removed and doesn't like
that? Shouldn't we just teach the check about trees deleted by the
drop command?

-- 
Cheers,

Sverre Rabbelier

Hi Sverre,

Sorry for the late reply.

On Thu, Oct 27, 2011 at 3:36 PM, Sverre Rabbelier [off-list ref] wrote:

Ok, so the problem is that fast-import notices that a tree that was
written out as part of a checkpoint is later removed and doesn't like
that? Shouldn't we just teach the check about trees deleted by the
drop command?

That was exactly my intention when I used release_tree_entry(). But I
guess I'm doing it wrong, because without the delete_ref() part this
does not work (just noticed there's a missing semicolon there...
sorry). Any advices/guidance, please? :)

-- 
Vitor Antunes

Heya,

On Wed, Nov 9, 2011 at 01:24, Vitor Antunes [off-list ref] wrote:

On Thu, Oct 27, 2011 at 3:36 PM, Sverre Rabbelier [off-list ref] wrote:

quoted

Ok, so the problem is that fast-import notices that a tree that was
written out as part of a checkpoint is later removed and doesn't like
that? Shouldn't we just teach the check about trees deleted by the
drop command?

That was exactly my intention when I used release_tree_entry(). But I
guess I'm doing it wrong, because without the delete_ref() part this
does not work (just noticed there's a missing semicolon there...
sorry). Any advices/guidance, please? :)

ENODATA. What do you mean with "does not work"? Can you run it through
gdb and see what's going on?

-- 
Cheers,

Sverre Rabbelier

On Wed, Nov 9, 2011 at 12:27 AM, Sverre Rabbelier [off-list ref] wrote:

Heya,

On Wed, Nov 9, 2011 at 01:24, Vitor Antunes [off-list ref] wrote:

quoted

That was exactly my intention when I used release_tree_entry(). But I
guess I'm doing it wrong, because without the delete_ref() part this
does not work (just noticed there's a missing semicolon there...
sorry). Any advices/guidance, please? :)

ENODATA. What do you mean with "does not work"? Can you run it through
gdb and see what's going on?

Calm down! It's not that bad to require gdb :)

It just means that even using the drop() command from the patch I
posted before, I still get the "new tip ... does not contain
..." error from fast-import.

-- 
Vitor Antunes