Re: git add -A fails in empty repository since 1.8.5
- From
Duy Nguyen <pclouds@gmail.com>
- Date
- Dec 19, 2013, 00:49 UTC
- Message-ID
- <CACsJy8BYDYUvpNwR4iSg+hiJGyZF2u75-y9XHg_qwJ4Xrxa+DA@mail.gmail.com>
- In-Reply-To
- <xmqqhaa5ho7x.fsf@gitster.dls.corp.google.com>
On Thu, Dec 19, 2013 at 3:57 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 38 quoted lines
> Duy Nguyen <pclouds@gmail.com> writes:
>
>> On Wed, Dec 18, 2013 at 3:44 PM, Antoine Pelisse <apelisse@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
>>> <tfnico@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.
Show 14 quoted lines
>> 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..
Show 8 quoted lines
> 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.
Show 11 quoted lines
> > >> 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@vger.kernel.org >>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
-- Duy