* [PATCH v0] fast-import: Add drop command @ 2011-09-24 15:27 Vitor Antunes 2011-09-24 15:27 ` Vitor Antunes 0 siblings, 1 reply; 15+ messages in thread From: Vitor Antunes @ 2011-09-24 15:27 UTC (permalink / raw) To: git; +Cc: Jonathan Nieder, 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v0] fast-import: Add drop command 2011-09-24 15:27 [PATCH v0] fast-import: Add drop command Vitor Antunes @ 2011-09-24 15:27 ` Vitor Antunes 2011-09-24 19:37 ` Jonathan Nieder 0 siblings, 1 reply; 15+ messages in thread From: Vitor Antunes @ 2011-09-24 15:27 UTC (permalink / raw) To: git; +Cc: Jonathan Nieder, 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v0] fast-import: Add drop command 2011-09-24 15:27 ` Vitor Antunes @ 2011-09-24 19:37 ` Jonathan Nieder 2011-09-24 21:19 ` Dmitry Ivankov 2011-09-24 21:35 ` Vitor Antunes 0 siblings, 2 replies; 15+ messages in thread From: Jonathan Nieder @ 2011-09-24 19:37 UTC (permalink / raw) To: Vitor Antunes; +Cc: git, Sverre Rabbelier, Dmitry Ivankov, David Barr 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v0] fast-import: Add drop command 2011-09-24 19:37 ` Jonathan Nieder @ 2011-09-24 21:19 ` Dmitry Ivankov 2011-09-27 8:57 ` Vitor Antunes 2011-09-24 21:35 ` Vitor Antunes 1 sibling, 1 reply; 15+ messages in thread From: Dmitry Ivankov @ 2011-09-24 21:19 UTC (permalink / raw) To: Jonathan Nieder; +Cc: Vitor Antunes, git, Sverre Rabbelier, David Barr 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 > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v0] fast-import: Add drop command 2011-09-24 21:19 ` Dmitry Ivankov @ 2011-09-27 8:57 ` Vitor Antunes 2011-10-24 16:37 ` Vitor Antunes 0 siblings, 1 reply; 15+ messages in thread From: Vitor Antunes @ 2011-09-27 8:57 UTC (permalink / raw) To: Dmitry Ivankov; +Cc: Jonathan Nieder, git, Sverre Rabbelier, David Barr 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v0] fast-import: Add drop command 2011-09-27 8:57 ` Vitor Antunes @ 2011-10-24 16:37 ` Vitor Antunes 2011-10-24 18:01 ` Sverre Rabbelier 0 siblings, 1 reply; 15+ messages in thread From: Vitor Antunes @ 2011-10-24 16:37 UTC (permalink / raw) To: Dmitry Ivankov, Jonathan Nieder; +Cc: git, Sverre Rabbelier, David Barr 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) ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v0] fast-import: Add drop command 2011-10-24 16:37 ` Vitor Antunes @ 2011-10-24 18:01 ` Sverre Rabbelier 2011-10-25 9:56 ` Vitor Antunes 0 siblings, 1 reply; 15+ messages in thread From: Sverre Rabbelier @ 2011-10-24 18:01 UTC (permalink / raw) To: Vitor Antunes; +Cc: Dmitry Ivankov, Jonathan Nieder, git, David Barr 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v0] fast-import: Add drop command 2011-10-24 18:01 ` Sverre Rabbelier @ 2011-10-25 9:56 ` Vitor Antunes 2011-10-27 11:06 ` Sverre Rabbelier 0 siblings, 1 reply; 15+ messages in thread From: Vitor Antunes @ 2011-10-25 9:56 UTC (permalink / raw) To: Sverre Rabbelier; +Cc: Dmitry Ivankov, Jonathan Nieder, git, David Barr 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v0] fast-import: Add drop command 2011-10-25 9:56 ` Vitor Antunes @ 2011-10-27 11:06 ` Sverre Rabbelier 2011-10-27 11:22 ` Vitor Antunes 0 siblings, 1 reply; 15+ messages in thread From: Sverre Rabbelier @ 2011-10-27 11:06 UTC (permalink / raw) To: Vitor Antunes; +Cc: Dmitry Ivankov, Jonathan Nieder, git, David Barr 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v0] fast-import: Add drop command 2011-10-27 11:06 ` Sverre Rabbelier @ 2011-10-27 11:22 ` Vitor Antunes 2011-10-27 14:36 ` Sverre Rabbelier 0 siblings, 1 reply; 15+ messages in thread From: Vitor Antunes @ 2011-10-27 11:22 UTC (permalink / raw) To: Sverre Rabbelier; +Cc: Dmitry Ivankov, Jonathan Nieder, git, David Barr 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v0] fast-import: Add drop command 2011-10-27 11:22 ` Vitor Antunes @ 2011-10-27 14:36 ` Sverre Rabbelier 2011-11-09 0:24 ` Vitor Antunes 0 siblings, 1 reply; 15+ messages in thread From: Sverre Rabbelier @ 2011-10-27 14:36 UTC (permalink / raw) To: Vitor Antunes; +Cc: Dmitry Ivankov, Jonathan Nieder, git, David Barr 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v0] fast-import: Add drop command 2011-10-27 14:36 ` Sverre Rabbelier @ 2011-11-09 0:24 ` Vitor Antunes 2011-11-09 0:27 ` Sverre Rabbelier 0 siblings, 1 reply; 15+ messages in thread From: Vitor Antunes @ 2011-11-09 0:24 UTC (permalink / raw) To: Sverre Rabbelier; +Cc: Dmitry Ivankov, Jonathan Nieder, git, David Barr 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v0] fast-import: Add drop command 2011-11-09 0:24 ` Vitor Antunes @ 2011-11-09 0:27 ` Sverre Rabbelier 2011-11-09 11:29 ` Vitor Antunes 0 siblings, 1 reply; 15+ messages in thread From: Sverre Rabbelier @ 2011-11-09 0:27 UTC (permalink / raw) To: Vitor Antunes; +Cc: Dmitry Ivankov, Jonathan Nieder, git, David Barr 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v0] fast-import: Add drop command 2011-11-09 0:27 ` Sverre Rabbelier @ 2011-11-09 11:29 ` Vitor Antunes 0 siblings, 0 replies; 15+ messages in thread From: Vitor Antunes @ 2011-11-09 11:29 UTC (permalink / raw) To: Sverre Rabbelier; +Cc: Dmitry Ivankov, Jonathan Nieder, git, David Barr 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v0] fast-import: Add drop command 2011-09-24 19:37 ` Jonathan Nieder 2011-09-24 21:19 ` Dmitry Ivankov @ 2011-09-24 21:35 ` Vitor Antunes 1 sibling, 0 replies; 15+ messages in thread From: Vitor Antunes @ 2011-09-24 21:35 UTC (permalink / raw) To: Jonathan Nieder; +Cc: Sverre Rabbelier, git, David Barr, Dmitry Ivankov 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) ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2011-11-09 11:30 UTC | newest] Thread overview: 15+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-09-24 15:27 [PATCH v0] fast-import: Add drop command Vitor Antunes 2011-09-24 15:27 ` Vitor Antunes 2011-09-24 19:37 ` Jonathan Nieder 2011-09-24 21:19 ` Dmitry Ivankov 2011-09-27 8:57 ` Vitor Antunes 2011-10-24 16:37 ` Vitor Antunes 2011-10-24 18:01 ` Sverre Rabbelier 2011-10-25 9:56 ` Vitor Antunes 2011-10-27 11:06 ` Sverre Rabbelier 2011-10-27 11:22 ` Vitor Antunes 2011-10-27 14:36 ` Sverre Rabbelier 2011-11-09 0:24 ` Vitor Antunes 2011-11-09 0:27 ` Sverre Rabbelier 2011-11-09 11:29 ` Vitor Antunes 2011-09-24 21:35 ` Vitor Antunes
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).