On Thu, Dec 19, 2013 at 3:57 AM, Junio C Hamano <gits...@pobox.com> wrote:
> Duy Nguyen <pclo...@gmail.com> writes:
>
>> On Wed, Dec 18, 2013 at 3:44 PM, Antoine Pelisse <apeli...@gmail.com> wrote:
>>> FWIW, git-bisect points to 84b8b5d (that is $gmane/230349).
>>>
>>> On Wed, Dec 18, 2013 at 9:06 AM, Thomas Ferris Nicolaisen
>>> <tfn...@gmail.com> wrote:
>>>> This was discussed on the Git user list recently [1].
>>>>
>>>> #in a repo with no files
>>>>> git add -A
>>>> fatal: pathspec '.' did not match any files
>>>>
>>>> The same goes for git add . (and -u).
>>>>
>>>> Whereas I think some warning feedback is useful, we are curious
>>>> whether this is an intentional change or not.
>>
>> I was not aware of this case when I made the change. It's caused by
>> this change that removes pathspec.raw[i][0] check in builtin/add.c in
>> 84b8b5d .
>>
>> - for (i = 0; pathspec.raw[i]; i++) {
>> - if (!seen[i] && pathspec.raw[i][0]
>> - && !file_exists(pathspec.raw[i])) {
>> + for (i = 0; i < pathspec.nr; i++) {
>> + const char *path = pathspec.items[i].match;
>> + if (!seen[i] && !file_exists(path)) {
>
> Isn't that pathspec.raw[i][0] check merely an attempt to work around
> the combination of
>
> (1) "the current directory" pathspec "." is sanitized down to an
> empty string by the pathspec code; and
>
> (2) even though file_exists() is willing to say "yes" to a non-file
> (namely, a directory), it is not prepared to take an empty
> string resulting from (1) to mean "the directory .".
Yeah, and it was added so intentionally in 07d7bed (add: don't
complain when adding empty project root - 2009-04-28). So this is a
regression.
>> Adding it back requires some thinking because "path" in the new code
>> could be something magic..
>
> Ehh, why? Shouldn't "something magic" that did _not_ match
> (i.e. not in seen[]) diagnosed as such?
>
> I am wondering why we even need !file_exists(path) check there in
> the first place. We run fill_directory() and then let
> prune_directory() report which pathspec did not have any match via
> the seen[] array. We also match pathspec against the index to see
> if there are pathspec that does not match anything. So at that
> point of the codeflow, we ought to be able to make sure that seen[]
> is the _only_ thing we need to consult to see if there are any
> pathspec elements that did not match.
See e96980e (builtin-add: simplify (and increase accuracy of) exclude
handling - 2007-06-12). It has something to do with directory check
originally, then we don't care about S_ISDIR() any more and turn it to
file_exists(). Maybe it's safe to remove it now. Need to check
fill_directory() again..
> Stepping back even further, I wonder if this "yes, I found a
> matching entity and know this is not an end-user typo" bit actually
> should be _in_ "struct pathspec". Traditionally we implemented that
> bit as a separate seen[] array parallel to "const char **pathspec"
> array, but that was merely because we only had the list of strings.
> Now we express a pathspec as a list of "struct pathspec" elements,
> I think seen[] can and should become part of the pathspec. Am I
> missing something?
Yes it probably better belongs to struct pathspec. Turning it into 1
flag would simplify seen[] memory management too.
>
>
>> and the new behavior makes sense, so I'm
>> inclined to keep it as is, unless people have other opinions.
>>
>>>>
>>>> [1] https://groups.google.com/d/topic/git-users/Qs4YSPhTsqE/discussion
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe git" in
>>>> the body of a message to majord...@vger.kernel.org
>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
