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

On Thu, Oct 27, 2011 at 4:37 AM, Junio C Hamano [off-list ref] wrote:

Nguyen Thai Ngoc Duy [off-list ref] writes:

quoted

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