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

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

On Thu, Oct 27, 2011 at 4:37 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

> 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