Re: [PATCH/WIP 02/11] notes-merge: use opendir/readdir instead of using read_directory()

From: Nguyen Thai Ngoc Duy <pclouds@gmail.com>
Date: Oct 27, 2011, 07:51 UTC
Message-ID: <CACsJy8C4iHffr4UYP9SvCU0OPC-LocUORwAQ492LqaV_tyvFQA@mail.gmail.com>
In-Reply-To: <7vmxcn3b8w.fsf@alter.siamese.dyndns.org>

On Thu, Oct 27, 2011 at 4:37 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 14 quoted lines

> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
>
>> Current read_directory() treats given path separately from contents
>> inside the path. If the given path has ".git", it's ok (but it'll stop
>> at .git if during tree recursion). The new read_directory() does not
>> make this exception, so when note-merge call
>> read_directory(".git/NOTES_MERGE_WORKTREE"), read_directory() sees
>> ".git" and stops immediately, assuming it's a gitlink.
>
> When read_directory("where/ever") is called, what kind of paths does it
> collect? Do the paths the function collects share "where/ever" as their
> common prefix? I thought it collects the paths relative to whatever
> top-level directory given to the function, so that "where/ever" could be
> anything.

Correct. But read_directory() takes pathspec now so naturally it does not treat "where/ever" a common prefix anymore. So it has to open(".") and starts from there. Even current code does not trust "where/ever" completely. treat_leading_path() may dismiss "where/ever" if it's excluded by .gitignore.

> Why does it even have to look at the given path in the first place and
> make a decision heavier than "can I opendir() and read from it"?

Because opendir("wh*/*r") may fail.

> In other
> words, if you see read_directory("some/thing/.git/more/stuff") and find a
> substring ".git/" in there, what "magic" gitlink handling does the code
> have to do?

"some/thing/.git" can be considered a new entry in index, so it should stop traversing at ".git". But because "some/thing/.git" does not exacly match "some/thing/.git/more/stuff", it is filtered out.

git-add deals with this case especially in order to avoid accidentally replace "some/thing/.git" in index with "some/thing/.git/more/stuff". But I feel it should be handled by read_directory(), not git-add.

Show 8 quoted lines

> I do not think it matters for _this_ particular case, but I can for
> example imagine an alternative implementation of a merge that uses
> temporary working tree somewhere other than the main working tree, and one
> of the natural "temporary" places such a feature in the future may want to
> use is inside .git/ somewhere. If you are planning to close the door by
> breaking the behaviour of read_directory(".git/some_where") for such
> callers with this series, we need to be aware of it, and that is why I am
> not satisfied by your explanation.

Maybe I should step back a bit. Instead of treating any input to read_directory() as pathspec, callers may provide two sets: a prefix set and a pathspec set. read_directory() starts from the prefix set without any checks, then descends in using pathspec.

But then what about the "if (treat_one_path(..) == path_ignored)" in treat_leading_path()? Remove it?

-- 
Duy