On Mon, Sep 1, 2008 at 10:25 PM, Shawn O. Pearce <spearce@xxxxxxxxxxx> wrote:
> Felipe Contreras <felipe.contreras@xxxxxxxxx> wrote:
>> This is useful in certain SCMs like monotone, where each 'merge revision' has
>> the changes of all the micro-branches merged. So it appears as duplicated commands.
>>
>> The delete command was ignoring the issue completely. The rename/copy commands
>> where throwing a fatal exception.
>
> Signed-off-by line? See Documentation/SubmittingPatches.
All right, read.
>> diff --git a/fast-import.c b/fast-import.c
>> index 7089e6f..3dd2ab6 100644
>> --- a/fast-import.c
>> +++ b/fast-import.c
>> @@ -1952,7 +1953,13 @@ static void file_change_d(struct branch *b)
>> die("Garbage after path in: %s", command_buf.buf);
>> p = uq.buf;
>> }
>> - tree_content_remove(&b->branch_tree, p, NULL);
>> + memset(&leaf, 0, sizeof(leaf));
>> + tree_content_remove(&b->branch_tree, p, &leaf);
>> + if (!leaf.versions[1].mode)
>> + {
>> + warning("Path %s not in branch", p);
>> + return;
>> + }
>> }
>>
>> static void file_change_cr(struct branch *b, int rename)
>
> This is going to leak memory unless you add this before the
> if (..mode) condition:
>
> if (leaf->tree)
> release_tree_content_recursive(e->tree);
Hmm, ok.
> We didn't worry about deleting a path that doesn't exist because
> the importer clearly wants it gone. If it wants it gone and it is
> already gone then it should be fine to ignore the delete command.
>
> But as I point out below some import front-ends should be accurate
> enough that they should not send a 'D' command unless the path is
> already in the tree. Thus this can be an error condition for some
> types of frontends, but can be ignored for others.
I'm sending the patch again with this behavior as an option.
>> @@ -1994,7 +2001,10 @@ static void file_change_cr(struct branch *b, int rename)
>> else
>> tree_content_get(&b->branch_tree, s, &leaf);
>> if (!leaf.versions[1].mode)
>> - die("Path %s not in branch", s);
>> + {
>> + warning("Path %s not in branch", s);
>> + return;
>> + }
>> tree_content_set(&b->branch_tree, d,
>> leaf.versions[1].sha1,
>> leaf.versions[1].mode,
>
> Normally we consider invalid paths to be an error. I wonder if this
> should still be an error, unless the front-end passes an option on
> the command line. Then monotone based importers can make these
> warnings, but other importers that don't have this problem can
> still treat them what they are, which is a fatal error.
>
> Did you run the test suite (t/t9300-fast-import.sh) after your patch?
> I would have thought a few of the bad path errors should be caught
> there.
I didn't initially, now I just did and it doesn't seem to be checking
for such things.
Best regards.
--
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
