threads / patch / 28481

patchfast-import: Add drop command

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

## tl;dr

15 messages between Sep 24, 2011 and Nov 9, 2011. Diffs are folded; open one to read it.

replies: 14people: 4as markdown or json

Vitor Antunes· Sep 24, 2011, 15:27 UTC · lore

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
Vitor Antunes· Sep 24, 2011, 15:27 UTC · re: Vitor Antunes · lore

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(-)
Show changes to fast-import.c +34 −1
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
Jonathan Nieder· Sep 24, 2011, 19:37 UTC · re: Vitor Antunes · lore

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

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
Dmitry Ivankov· Sep 24, 2011, 21:19 UTC · re: Jonathan Nieder · lore

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

On Sun, Sep 25, 2011 at 1:37 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 7 quoted lines
> 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
>
Vitor Antunes· Sep 27, 2011, 08:57 UTC · re: Dmitry Ivankov · lore

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

On Sat, Sep 24, 2011 at 10:19 PM, Dmitry Ivankov <divanorama@gmail.com> wrote:
Show 7 quoted lines
> 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.

Show 14 quoted lines
> 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').

Show 7 quoted lines
> 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
Vitor Antunes· Oct 24, 2011, 16:37 UTC · re: Vitor Antunes · lore

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

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:
Show 51 quoted lines
> 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)
Sverre Rabbelier· Oct 24, 2011, 18:01 UTC · re: Vitor Antunes · lore

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

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
Vitor Antunes· Oct 25, 2011, 09:56 UTC · re: Sverre Rabbelier · lore

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

On Mon, Oct 24, 2011 at 7:01 PM, Sverre Rabbelier <srabbelier@gmail.com> wrote:
Show 5 quoted lines
> 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
Sverre Rabbelier· Oct 27, 2011, 11:06 UTC · re: Vitor Antunes · lore

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

Heya,
On Tue, Oct 25, 2011 at 11:56, Vitor Antunes <vitor.hda@gmail.com> wrote:
Show 21 quoted lines
> 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
Vitor Antunes· Oct 27, 2011, 11:22 UTC · re: Sverre Rabbelier · lore

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

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
Sverre Rabbelier· Oct 27, 2011, 14:36 UTC · re: Vitor Antunes · lore

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

Heya,
On Thu, Oct 27, 2011 at 13:22, Vitor Antunes <vitor.hda@gmail.com> wrote:
Show 11 quoted lines
> 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
Vitor Antunes· Nov 9, 2011, 00:24 UTC · re: Sverre Rabbelier · lore

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

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
Sverre Rabbelier· Nov 9, 2011, 00:27 UTC · re: Vitor Antunes · lore

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

Heya,
On Wed, Nov 9, 2011 at 01:24, Vitor Antunes <vitor.hda@gmail.com> wrote:
Show 10 quoted lines
> 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
Vitor Antunes· Nov 9, 2011, 11:29 UTC · re: Sverre Rabbelier · lore

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

On Wed, Nov 9, 2011 at 12:27 AM, Sverre Rabbelier <srabbelier@gmail.com> wrote:
Show 11 quoted lines
> 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
Vitor Antunes· Sep 24, 2011, 21:35 UTC · re: Jonathan Nieder · lore

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

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

← back to recent threads