Re: git add -A fails in empty repository since 1.8.5

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Dec 19, 2013 at 3:57 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Duy Nguyen <pclouds@xxxxxxxxx> writes:
>
>> On Wed, Dec 18, 2013 at 3:44 PM, Antoine Pelisse <apelisse@xxxxxxxxx> wrote:
>>> FWIW, git-bisect points to 84b8b5d (that is $gmane/230349).
>>>
>>> On Wed, Dec 18, 2013 at 9:06 AM, Thomas Ferris Nicolaisen
>>> <tfnico@xxxxxxxxx> 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 majordomo@xxxxxxxxxxxxxxx
>>>> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]