Re: [PATCH 01/10] add: do not rely on dtype being NULL behavior

[Nguyen Thai Ngoc Duy <pclouds@gmail.com>]

On Mon, Nov 15, 2010 at 7:14 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Nguyễn Thái Ngọc Duy wrote:
>
>> Commit c84de70 (excluded_1(): support exclude files in index -
>> 2009-08-20) added support for excluded() where dtype can be NULL. It
>> was designed specifically for index matching because there was no
>> other way to extract dtype information from index. It did not support
>> wildcard matching (for example, "a*/" pattern would fail to match).
>>
>> The code was probably misread when commit 108da0d (git add: Add the
>> "--ignore-missing" option for the dry run - 2010-07-10) was made
>> because DT_UNKNOWN happens to be zero (NULL) too.
>>
>> Do not pass DT_UNKNOWN/NULL to excluded(), instead pass a pointer to a
>> variable that contains DT_UNKNOWN. The real dtype will be extracted
>> from worktree by excluded(), as expected.
>
> Could you rephrase this in a way that contrasts current and desired
> behavior?  Is it like this?
>
>        The "git add --ignore-missing --dry-run" codepath is
>        interpreting .gitignore incorrectly, unlike "git add".  For
>        example:
>
>                $ test -e foo || echo missing
>                missing
>                $ echo foo/ >>.gitignore
>                $ mkdir bar
>                $ git add --ignore-missing --dry-run foo; echo $?
>                The following paths are ignored by one of your .gitignore files:
>                foo/
>                Use -f if you really want to add them.
>                fatal: no files added
>                128
>                $ git add --ignore-missing --dry-run bar/foo; echo $?
>                0
>
>        In the original use case (preparing to add a submodule) the
>        behavior of the first command is correct, second incorrect.
>        If the entry to be added was a regular file, it would be the
>        other way around.
>
>        The cause: the --ignore-missing code passes DT_UNKNOWN as the
>        dtype_ptr argument to excluded() which happens to equal zero
>        (NULL) and accidentally triggers the "match pathspecs in index
>        only" codepath (see c84de70, excluded_1(): support exclude
>        files in index, 2009-08-20) that is unfortunately a bit
>        primitive.
>
>        Surely what was really wanted is to check paths against the
>        index and work tree, defaulting to "regular file".
>
> Wait --- that's not true.  In the "git submodule add" case, we really
> want to default to (or even better, force) "directory".

Hmm.. get_index_dtype() would return DT_DIR if the submodule exists in
index. If it does not it must be a directory in worktree, right?
Call flow: excluded_from_list() -> get_dtype() -> get_index_dtype()
-- 
Duy