Re: [PATCH 1/1] fast-import: show a warning for non-existent files.

["Felipe Contreras" <felipe.contreras@gmail.com>]

On Mon, Sep 1, 2008 at 10:25 PM, Shawn O. Pearce <spearce@spearce.org> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> 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