From: Nguyen Thai Ngoc Duy Date: Tue, 16 Nov 2010 02:18:04 GMT Subject: Re: [PATCH 01/10] add: do not rely on dtype being NULL behavior Message-ID: In-Reply-To: <20101115121415.GB14729@burratino> On Mon, Nov 15, 2010 at 7:14 PM, Jonathan Nieder 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