Re: Bug Report: .gitignore behavior is not matching in git clean and git status

[Junio C Hamano]

Samuel Lijin  writes:

> After some more digging (and familiarizing myself with the
> behind-the-scenes logic) the issue is that dir.c has this implicit
> assumption that a directory which contains only untracked and ignored
> files should itself be considered untracked. While that works fine for
> use cases where we're asking if a directory should be added to the git
> database, that decidedly does not make sense when we're asking if a
> directory can be removed from the working tree.

Thanks for digging.

> I'm not sure where to proceed from here. I see two ways forward: one,
> builtin/clean.c can collect ignored files when it calls
> dir.c:fill_directory(), and then clean -d can prune out directories
> that contain ignored files; two, path_treatment can learn about
> untracked directories which contain excluded (ignored) files.

My gut feeling is that the former approach would be of lesser
impact.  Directory A/ can be removed "clean" without "-x" when there
is nothing tracked in there in the index and there is no ignored
paths.  Having zero untracked files there or one or more untracked
file there do not matter---they are all subject to removal by
"clean".

Re: Bug Report: .gitignore behavior is not matching in git clean and git status

[Samuel Lijin]

After some more digging (and familiarizing myself with the
behind-the-scenes logic) the issue is that dir.c has this implicit
assumption that a directory which contains only untracked and ignored
files should itself be considered untracked. While that works fine for
use cases where we're asking if a directory should be added to the git
database, that decidedly does not make sense when we're asking if a
directory can be removed from the working tree.

I'm not sure where to proceed from here. I see two ways forward: one,
builtin/clean.c can collect ignored files when it calls
dir.c:fill_directory(), and then clean -d can prune out directories
that contain ignored files; two, path_treatment can learn about
untracked directories which contain excluded (ignored) files.

On Mon, May 1, 2017 at 8:51 AM, Samuel Lijin  wrote:
> On Sun, Apr 30, 2017 at 8:56 PM, Chris Johnson  
> wrote:
>> Good assessment/understanding of the issue. git clean -n  does not
>> report anything as being targeted for removal, and git clean -f
>> matches that behavior. I agree with it probably being related
>> specifically to the -d flag.
>>
>> As another experiment I modified .gitignore to ignore /A/B/C instead
>> of /A/B/ and the same result occurs (-n reports nothing, -dn reports
>> removing A/)
>>
>> Lastly, I changed .gitignore to just be /A/, and in doing so, clean
>> -dn stops reporting that it will remove A/. I’m not exactly sure if
>> this last one is surprising or not.
>
> It doesn't seem so to me, since a trailing slash in .gitignore is an
> explicit directive to ignore the entire directory, so when pruning the
> tree of files/dirs to ignore, it drops everything in A/ before even
> getting to A/B/C. The issue is that ignoring A/B/C shouldn't leave A/
> to be cleaned.
>
>> Also, and sorry for the noise, but I did a reply-all here, but will a
>> reply automatically include the rest of the list? Or was reply-all the
>> right move?
>>
>> On Sun, Apr 30, 2017 at 9:41 PM, Junio C Hamano  wrote:
>>> Chris Johnson  writes:
>>>
 I am a mailing list noob so I’m sorry if this is the wrong format or
 the wrong please.

 Here’s the setup for the bug (I will call it a bug but I half expect
 somebody to tell me I’m an idiot):

 git init
 echo "/A/B/" > .gitignore
>>>
>>> You tell Git that anything in A/B/ are uninteresting.
>>>
 git add .gitignore && git commit -m 'Add ignore'
 mkdir -p A/B
 touch A/B/C
>>>
>>> And create an uninteresting cruft.
>>>
 git status
>>>
>>> And Git does not bug you about it.
>>>
 git clean -dn
>>>
>>> This incorrectly reports "Would remove A/" and if you gave 'f'
>>> instead of 'n', it does remove A/, A/B, and A/B/C.
>>>
>>> Despite that "git clean --help" says 'only files unknown to Git are
>>> removed' (with an undefined term 'unknown to Git').  What it wants
>>> the term mean can be guessed by seeing 'if the -x option is
>>> specified, ignored files are also removed'---so 'unknown to Git'
>>> does not include what you told .gitignore that they are
>>> uninteresting.  IOW, Git knows they are not interesting.
>>>
>>> It looks like a bug in "git clean -d" to me.
>
> I may be wrong (I'm not very familiar with the codebase), but I don't
> think it's a bug in specifically git clean -d.
>
> Throwing gdb at it, when builtin/clean.c:cmd_clean() calls
> dir.c:fill_directory(), A/ gets appended to dir->entries, regardless
> of whether or not git clean is called with or without -d. The
> difference is that if git clean is called without -d, the loop that
> strips out directories strips out the A/ entry.
>
> When dir.c:fill_directory() is invoked through git status, A/ does
> not, however, get appended to dir->entries. As best as I can tell,
> this seems to be because git status sets the
> DIR_HIDE_EMPTY_DIRECTORIES flag, whereas git clean does not (which
> makes sense), but the fact that DIR_HIDE_EMPTY_DIRECTORIES is
> responsible for not adding A/ to dir->entries seems to be the issue.

Re: Bug Report: .gitignore behavior is not matching in git clean and git status

[Samuel Lijin]

On Sun, Apr 30, 2017 at 8:56 PM, Chris Johnson  wrote:
> Good assessment/understanding of the issue. git clean -n  does not
> report anything as being targeted for removal, and git clean -f
> matches that behavior. I agree with it probably being related
> specifically to the -d flag.
>
> As another experiment I modified .gitignore to ignore /A/B/C instead
> of /A/B/ and the same result occurs (-n reports nothing, -dn reports
> removing A/)
>
> Lastly, I changed .gitignore to just be /A/, and in doing so, clean
> -dn stops reporting that it will remove A/. I’m not exactly sure if
> this last one is surprising or not.

It doesn't seem so to me, since a trailing slash in .gitignore is an
explicit directive to ignore the entire directory, so when pruning the
tree of files/dirs to ignore, it drops everything in A/ before even
getting to A/B/C. The issue is that ignoring A/B/C shouldn't leave A/
to be cleaned.

> Also, and sorry for the noise, but I did a reply-all here, but will a
> reply automatically include the rest of the list? Or was reply-all the
> right move?
>
> On Sun, Apr 30, 2017 at 9:41 PM, Junio C Hamano  wrote:
>> Chris Johnson  writes:
>>
>>> I am a mailing list noob so I’m sorry if this is the wrong format or
>>> the wrong please.
>>>
>>> Here’s the setup for the bug (I will call it a bug but I half expect
>>> somebody to tell me I’m an idiot):
>>>
>>> git init
>>> echo "/A/B/" > .gitignore
>>
>> You tell Git that anything in A/B/ are uninteresting.
>>
>>> git add .gitignore && git commit -m 'Add ignore'
>>> mkdir -p A/B
>>> touch A/B/C
>>
>> And create an uninteresting cruft.
>>
>>> git status
>>
>> And Git does not bug you about it.
>>
>>> git clean -dn
>>
>> This incorrectly reports "Would remove A/" and if you gave 'f'
>> instead of 'n', it does remove A/, A/B, and A/B/C.
>>
>> Despite that "git clean --help" says 'only files unknown to Git are
>> removed' (with an undefined term 'unknown to Git').  What it wants
>> the term mean can be guessed by seeing 'if the -x option is
>> specified, ignored files are also removed'---so 'unknown to Git'
>> does not include what you told .gitignore that they are
>> uninteresting.  IOW, Git knows they are not interesting.
>>
>> It looks like a bug in "git clean -d" to me.

I may be wrong (I'm not very familiar with the codebase), but I don't
think it's a bug in specifically git clean -d.

Throwing gdb at it, when builtin/clean.c:cmd_clean() calls
dir.c:fill_directory(), A/ gets appended to dir->entries, regardless
of whether or not git clean is called with or without -d. The
difference is that if git clean is called without -d, the loop that
strips out directories strips out the A/ entry.

When dir.c:fill_directory() is invoked through git status, A/ does
not, however, get appended to dir->entries. As best as I can tell,
this seems to be because git status sets the
DIR_HIDE_EMPTY_DIRECTORIES flag, whereas git clean does not (which
makes sense), but the fact that DIR_HIDE_EMPTY_DIRECTORIES is
responsible for not adding A/ to dir->entries seems to be the issue.

Re: Bug Report: .gitignore behavior is not matching in git clean and git status

[Junio C Hamano]

Chris Johnson  writes:

> Also, and sorry for the noise, but I did a reply-all here, but will a
> reply automatically include the rest of the list? Or was reply-all the
> right move?

The convention around here is to do reply-all (in other words, make
sure that Cc: line has git@vger.kernel.org and others involved in
the discussion and mentioned on To: or Cc: line of the message you
are responding to).  Your message (i.e. the one I am responding to)
has correct addresses on To:/Cc: lines AFAICS.

Re: Bug Report: .gitignore behavior is not matching in git clean and git status

[Chris Johnson]

Good assessment/understanding of the issue. git clean -n  does not
report anything as being targeted for removal, and git clean -f
matches that behavior. I agree with it probably being related
specifically to the -d flag.

As another experiment I modified .gitignore to ignore /A/B/C instead
of /A/B/ and the same result occurs (-n reports nothing, -dn reports
removing A/)

Lastly, I changed .gitignore to just be /A/, and in doing so, clean
-dn stops reporting that it will remove A/. I’m not exactly sure if
this last one is surprising or not.

Also, and sorry for the noise, but I did a reply-all here, but will a
reply automatically include the rest of the list? Or was reply-all the
right move?

On Sun, Apr 30, 2017 at 9:41 PM, Junio C Hamano  wrote:
> Chris Johnson  writes:
>
>> I am a mailing list noob so I’m sorry if this is the wrong format or
>> the wrong please.
>>
>> Here’s the setup for the bug (I will call it a bug but I half expect
>> somebody to tell me I’m an idiot):
>>
>> git init
>> echo "/A/B/" > .gitignore
>
> You tell Git that anything in A/B/ are uninteresting.
>
>> git add .gitignore && git commit -m 'Add ignore'
>> mkdir -p A/B
>> touch A/B/C
>
> And create an uninteresting cruft.
>
>> git status
>
> And Git does not bug you about it.
>
>> git clean -dn
>
> This incorrectly reports "Would remove A/" and if you gave 'f'
> instead of 'n', it does remove A/, A/B, and A/B/C.
>
> Despite that "git clean --help" says 'only files unknown to Git are
> removed' (with an undefined term 'unknown to Git').  What it wants
> the term mean can be guessed by seeing 'if the -x option is
> specified, ignored files are also removed'---so 'unknown to Git'
> does not include what you told .gitignore that they are
> uninteresting.  IOW, Git knows they are not interesting.
>
> It looks like a bug in "git clean -d" to me.  Do you see the same
> issue if you use "git clean" without "-d"?  IOW, does it offer to
> remove A/B/C?

Re: Bug Report: .gitignore behavior is not matching in git clean and git status

[Junio C Hamano]

Chris Johnson  writes:

> I am a mailing list noob so I’m sorry if this is the wrong format or
> the wrong please.
>
> Here’s the setup for the bug (I will call it a bug but I half expect
> somebody to tell me I’m an idiot):
>
> git init
> echo "/A/B/" > .gitignore

You tell Git that anything in A/B/ are uninteresting.

> git add .gitignore && git commit -m 'Add ignore'
> mkdir -p A/B
> touch A/B/C

And create an uninteresting cruft.

> git status

And Git does not bug you about it.

> git clean -dn

This incorrectly reports "Would remove A/" and if you gave 'f'
instead of 'n', it does remove A/, A/B, and A/B/C.

Despite that "git clean --help" says 'only files unknown to Git are
removed' (with an undefined term 'unknown to Git').  What it wants
the term mean can be guessed by seeing 'if the -x option is
specified, ignored files are also removed'---so 'unknown to Git'
does not include what you told .gitignore that they are
uninteresting.  IOW, Git knows they are not interesting.

It looks like a bug in "git clean -d" to me.  Do you see the same
issue if you use "git clean" without "-d"?  IOW, does it offer to
remove A/B/C?

Bug Report: .gitignore behavior is not matching in git clean and git status

[Chris Johnson]

I am a mailing list noob so I’m sorry if this is the wrong format or
the wrong please.

Here’s the setup for the bug (I will call it a bug but I half expect
somebody to tell me I’m an idiot):

git init
echo "/A/B/" > .gitignore
git add .gitignore && git commit -m 'Add ignore'
mkdir -p A/B
touch A/B/C
git status
git clean -dn

(my client may have screwed up the quotes, please bear with me)

Now, this may just be a case of me misunderstanding the behavior of
.gitignore, and that’s fine, but to me, git clean -dn should roughly
reflect the same behavior as git status as far as ignored files go. As
it is, git status does NOT report A/B/C as an untracked file, but git
clean will remove the entire A dir. It seems to me that one or the
other’s behavior ought to be tweaked. Which one is correct I am not
sure.

This happens on 2.5.1 as well as 2.9.2

Chris