[PATCH v0] fast-import: Add drop command

[PATCH v0] fast-import: Add drop command

[Vitor Antunes]

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

[PATCH v0] fast-import: Add drop command

[Vitor Antunes]

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

Signed-off-by: Vitor Antunes <vitor.hda@gmail.com>
---
 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

Re: [PATCH v0] fast-import: Add drop command

[Jonathan Nieder]

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

Re: [PATCH v0] fast-import: Add drop command

[Dmitry Ivankov]

On Sun, Sep 25, 2011 at 1:37 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> 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?
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
>

Re: [PATCH v0] fast-import: Add drop command

[Vitor Antunes]

On Sep 24, 2011 8:37 PM, "Jonathan Nieder" <jrnieder@gmail.com> 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)

Re: [PATCH v0] fast-import: Add drop command

[Vitor Antunes]

On Sat, Sep 24, 2011 at 10:19 PM, Dmitry Ivankov <divanorama@gmail.com> wrote:
> On Sun, Sep 25, 2011 at 1:37 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> 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

Re: [PATCH v0] fast-import: Add drop command

[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 <vitor.hda@gmail.com> wrote:
> On Sat, Sep 24, 2011 at 10:19 PM, Dmitry Ivankov <divanorama@gmail.com> wrote:
>> On Sun, Sep 25, 2011 at 1:37 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>>> 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)

Re: [PATCH v0] fast-import: Add drop command

[Sverre Rabbelier]

Heya,

On Mon, Oct 24, 2011 at 18:37, Vitor Antunes <vitor.hda@gmail.com> 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

Re: [PATCH v0] fast-import: Add drop command

[Vitor Antunes]

On Mon, Oct 24, 2011 at 7:01 PM, Sverre Rabbelier <srabbelier@gmail.com> 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

Re: [PATCH v0] fast-import: Add drop command

[Sverre Rabbelier]

Heya,

On Tue, Oct 25, 2011 at 11:56, Vitor Antunes <vitor.hda@gmail.com> wrote:
> On Mon, Oct 24, 2011 at 7:01 PM, Sverre Rabbelier <srabbelier@gmail.com> 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.

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

Re: [PATCH v0] fast-import: Add drop command

[Vitor Antunes]

On Thu, Oct 27, 2011 at 12:06 PM, Sverre Rabbelier <srabbelier@gmail.com> 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

Re: [PATCH v0] fast-import: Add drop command

[Sverre Rabbelier]

Heya,

On Thu, Oct 27, 2011 at 13:22, Vitor Antunes <vitor.hda@gmail.com> wrote:
> On Thu, Oct 27, 2011 at 12:06 PM, Sverre Rabbelier <srabbelier@gmail.com> 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.

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

Re: [PATCH v0] fast-import: Add drop command

[Vitor Antunes]

Hi Sverre,

Sorry for the late reply.

On Thu, Oct 27, 2011 at 3:36 PM, Sverre Rabbelier <srabbelier@gmail.com> 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

Re: [PATCH v0] fast-import: Add drop command

[Sverre Rabbelier]

Heya,

On Wed, Nov 9, 2011 at 01:24, Vitor Antunes <vitor.hda@gmail.com> wrote:
> On Thu, Oct 27, 2011 at 3:36 PM, Sverre Rabbelier <srabbelier@gmail.com> 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? :)

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

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH v0] fast-import: Add drop command

[Vitor Antunes]

On Wed, Nov 9, 2011 at 12:27 AM, Sverre Rabbelier <srabbelier@gmail.com> wrote:
> Heya,
>
> On Wed, Nov 9, 2011 at 01:24, Vitor Antunes <vitor.hda@gmail.com> wrote:
>>
>> 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