Re: Bug#553296: gitignore broken completely

[cc'ing git@vger and quoting excessively to give those readers context]

On Fri, Oct 30, 2009 at 05:59:03PM +0100, Klaus Ethgen wrote:

Am Fr den 30. Okt 2009 um 17:28 schrieb Gerrit Pape:

quoted

quoted

The most recent git in debian has a broken ignore handling. Let me
show
it on a example:
 > mkdir gittest
 > cd gittest
 > git init
 > touch a
 > git add a
 > git commit -m commit
 > git ls-files -i --exclude-standard

The last command will show the file a (as it would show every file as
being ignored, every which is in the index!). But that command should
show nothing at that point.

Hi Klaus, it looks like upstream did that on purpose

 http://git.kernel.org/?p=git/git.git;a=commit;h=b5227d80

$ git describe --contains b5227d80
v1.6.5.2~2^2^2

It was not exactly on purpose. The point of that change was that "git
ls-files" should also show things mentioned in the .gitignore, because
.gitignore has nothing to do whatsoever with tracked files.

But I simply forgot about "git ls-files -i", so changing it was an
unintended side effect (and sadly, we don't seem to have any regression
tests for it).

That makes the whole sense of -i ad absurdum (I do not know how to tell
"ad absurdum" in english). With that patch there is no way anymore to
see if some ignored files are accidentally committed.

Yes, with the current code "-i" serves no purpose unless you are using
"-o".

I have to use git often as frontend for that broken design (svn). So to
hold the ignores up2date I use "git svn show-ignore > .gitignore" But
many, many repositories have broken ignores so I have to check it with
"git ls-files -i --exclude-standard" to see if there is accidentally
ignored files. Well, that patch makes that impossible at all!

Just to be clear: nothing is actually broken about ignores that cover
tracked files. Ignores are (and have been since long before this patch)
purely about untracked files. So there is no problem at all with:

  $ echo content >foo
  $ git add foo
  $ echo foo >.gitignore
  $ git commit

The _only_ thing that respected such ignores was "git ls-files", and the
point of the patch in question was to fix that.

So I think, this patch is a bug at all!

I add Jeff (and Junio as he did the commit) to the Cc. @Jeff and or
Junio: could you please revoke that patch?

I am not sure simply reverting is the best choice; the patch does do
something useful. And while it strictly breaks backwards compatibility
on the output without "-i", the old behavior was considered a bug. But
the "-i" behavior is useless now, so we need to figure out how to
proceed. We can:

  1. Revert and accept that the behavior is historical. Callers need to
     work around it by dropping --exclude* when invoking ls-files.

  2. Declare "-i" useless, deprecate and/or remove. Obviously this is
     also breaking existing behavior, but again, I don't think that
     using "-i" actually accomplishes anything.

  3. Revert for now, and then reinstate the patch during a major version
     bump when we are declaring some compatibility breakages.

  4. Re-work "-i" to show tracked but ignored files, but still show all
     files when "-i" is not given at all.

I think (4) preserves the benefit of the patch in question, but still
allows your usage ("git ls-files -i --exclude-standard"). I do question
whether that usage is worth supporting. Certainly I wouldn't implement
it if I were writing git-ls-files from scratch today, but we do in
general try to maintain backwards compatibility, especially for plumbing
like ls-files.

Junio, what do you want to do?

-Peff

On Fri, Oct 30, 2009 at 07:23:31PM +0100, Klaus Ethgen wrote:

Well ls-files is used to see such broken files. (Another example is if
you accidentally add a file which you (later) decide to be ignored. You
will have no change to find that files at all anymore.) With the patch
that use case of ls-files has been gone without a replacement.

I think your reasoning is a little bit circular. They are not actually
broken with respect to git. But they might represent an error on the
part of the programmer, and one which they would want to investigate.
You could also catch it by looking at diffs ("why is this file in my
diff, it is supposed to be ignored"), but I am not opposed to accessing
the information via git-ls-files (and I think you are right that the
query cannot be easily done any other way).

I have two more options:

   5. Revert the patch and rework it to have a option to ignore or
      respect the excluded files (Such as --use-excludes for example) or
      respect them anyway if a --exclude* option is given on command
      line.

I think that is basically equivalent to (1). There is already a way for
callers to avoid the bug, which is not to provide --exclude* at all. So
you are already setting that one bit of information in whether or not
you provide any excludes.

   6. Revert the patch and rework it so that it will only have effect if
      there is no -i option on the command line. (That is similiar to a
      mix of 3 and 4.)

Yeah, that would actually be the least invasive change, and would keep
"-i" just as it is. If we do anything except a simple, I think your (6)
makes the most sense.

Let me see if I can make a patch.

-Peff

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

Am Fr den 30. Okt 2009 um 18:38 schrieb Jeff King:

It was not exactly on purpose. The point of that change was that "git
ls-files" should also show things mentioned in the .gitignore, because
.gitignore has nothing to do whatsoever with tracked files.

(Mostly) true. It has when sending such patches that adds a ignored
file, to some other.

But I simply forgot about "git ls-files -i", so changing it was an
unintended side effect (and sadly, we don't seem to have any regression
tests for it).

Could happen. No Problem with that as such.

quoted

That makes the whole sense of -i ad absurdum (I do not know how to tell
"ad absurdum" in english). With that patch there is no way anymore to
see if some ignored files are accidentally committed.

Yes, with the current code "-i" serves no purpose unless you are using
"-o".

But this is an other use case.

quoted

I have to use git often as frontend for that broken design (svn). So to
hold the ignores up2date I use "git svn show-ignore > .gitignore" But
many, many repositories have broken ignores so I have to check it with
"git ls-files -i --exclude-standard" to see if there is accidentally
ignored files. Well, that patch makes that impossible at all!

Just to be clear: nothing is actually broken about ignores that cover
tracked files. Ignores are (and have been since long before this patch)
purely about untracked files. So there is no problem at all with:

See my comment above.

The _only_ thing that respected such ignores was "git ls-files", and the
point of the patch in question was to fix that.

Well ls-files is used to see such broken files. (Another example is if
you accidentally add a file which you (later) decide to be ignored. You
will have no change to find that files at all anymore.) With the patch
that use case of ls-files has been gone without a replacement.

quoted

So I think, this patch is a bug at all!

I add Jeff (and Junio as he did the commit) to the Cc. @Jeff and or
Junio: could you please revoke that patch?

I am not sure simply reverting is the best choice; the patch does do
something useful. And while it strictly breaks backwards compatibility
on the output without "-i", the old behavior was considered a bug. But
the "-i" behavior is useless now, so we need to figure out how to
proceed. We can:

  1. Revert and accept that the behavior is historical. Callers need to
     work around it by dropping --exclude* when invoking ls-files.

  2. Declare "-i" useless, deprecate and/or remove. Obviously this is
     also breaking existing behavior, but again, I don't think that
     using "-i" actually accomplishes anything.

  3. Revert for now, and then reinstate the patch during a major version
     bump when we are declaring some compatibility breakages.

  4. Re-work "-i" to show tracked but ignored files, but still show all
     files when "-i" is not given at all.

I have two more options:

   5. Revert the patch and rework it to have a option to ignore or
      respect the excluded files (Such as --use-excludes for example) or
      respect them anyway if a --exclude* option is given on command
      line.

   6. Revert the patch and rework it so that it will only have effect if
      there is no -i option on the command line. (That is similiar to a
      mix of 3 and 4.)

I have nothing against the patch as such. But in the current form it
breaks at least one frequent used use case of ls-files.

I think (4) preserves the benefit of the patch in question, but still
allows your usage ("git ls-files -i --exclude-standard"). I do question
whether that usage is worth supporting. Certainly I wouldn't implement
it if I were writing git-ls-files from scratch today,

Well I had to search explicit for this use case as I had several
problems with ignored files in combination with svn. So I would. (if I
know the code good enough. And this problem to list such files did made
enough pain to me that I would go into the code to get it implemented.)

Regards
   Klaus
- -- 
Klaus Ethgen                            http://www.ethgen.de/
pub  2048R/D1A4EDE5 2000-02-26 Klaus Ethgen [off-list ref]
Fingerprint: D7 67 71 C4 99 A6 D4 FE  EA 40 30 57 3C 88 26 2B
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)

iQEVAwUBSusvI5+OKpjRpO3lAQqMhQgAnM7w+VqvUB+zFCJj8sCqO6QgcI+oup1z
dMwsv+5QU+S5UH7xdm/L6AhFJEsWsbpHzytVg7Rv3wCp8OzFiXmnjfUw+3JEvuLJ
+ggWHvFeKkTReDaRY00dafAQCP/8h0Yar6hVmXfdqGeiOnK0LeN5OXx0T3K51U/2
r8YOeNPZOzbaITcRaeIi5ghAEpyAgdqEw++f8h1xCRGo6DyncUIoexmFSG0pZS8q
tsyksW7q02LscTEp6PinQa7jhUN0xWJFTvpuCBWlfNgNTTffWt1xDjXTebwRKsYR
cT1ygEiI2+aZfrE47Fn91G9I+JjF5KYo7jt5UFNFlck9YsEKGPf22g==
=2ZcU
-----END PGP SIGNATURE-----

On Fri, Oct 30, 2009 at 02:41:55PM -0400, Jeff King wrote:

quoted

   6. Revert the patch and rework it so that it will only have effect if
      there is no -i option on the command line. (That is similiar to a
      mix of 3 and 4.)

Yeah, that would actually be the least invasive change, and would keep
"-i" just as it is. If we do anything except a simple, I think your (6)
makes the most sense.

Let me see if I can make a patch.

Here it is. I think this is the right thing to do. Junio?

-- >8 --
Subject: [PATCH] ls-files: unbreak "ls-files -i"

Commit b5227d8 changed the behavior of "ls-files" with
respect to includes, but accidentally broke the "-i" option
The original behavior was:

  1. if no "-i" is given, cull all results according to --exclude*
  2. if "-i" is given, show the inverse of (1)

The broken behavior was:

  1. if no "-i" is given:
     a. for "-o", cull results according to --exclude*
     b. for index files, always show all
  2. if "-i" is given:
     a. for "-o", shows the inverse of (1a)
     b. for index files, always show all

The fixed behavior keeps the new (1b) behavior introduced
by b5227d8, but fixes the (2b) behavior to show only ignored
files, not all files.

This patch also tweaks the documentation. The original text
was somewhat obscure in the first place, but it is also now
inaccurate (the relationship between (1b) and (2b) is not
quite a "reverse").

Signed-off-by: Jeff King <redacted>
---
 Documentation/git-ls-files.txt |    6 ++++--
 builtin-ls-files.c             |    8 ++++++++
 t/t3003-ls-files-exclude.sh    |    8 ++++++++
 3 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
index 021066e..625723e 100644
--- a/Documentation/git-ls-files.txt
+++ b/Documentation/git-ls-files.txt

@@ -48,8 +48,10 @@ OPTIONS
 
 -i::
 --ignored::
-	Show ignored files in the output.
-	Note that this also reverses any exclude list present.
+	Show only ignored files in the output. When showing files in the
+	index, print only those matched by an exclude pattern. When
+	showing "other" files, show only those matched by an exclude
+	pattern.
 
 -s::
 --stage::

diff --git a/builtin-ls-files.c b/builtin-ls-files.c
index c5c0407..c9a03e5 100644
--- a/builtin-ls-files.c
+++ b/builtin-ls-files.c

@@ -170,6 +170,10 @@ static void show_files(struct dir_struct *dir, const char *prefix)
 	if (show_cached | show_stage) {
 		for (i = 0; i < active_nr; i++) {
 			struct cache_entry *ce = active_cache[i];
+			int dtype = ce_to_dtype(ce);
+			if (dir->flags & DIR_SHOW_IGNORED &&
+			    !excluded(dir, ce->name, &dtype))
+				continue;
 			if (show_unmerged && !ce_stage(ce))
 				continue;
 			if (ce->ce_flags & CE_UPDATE)

@@ -182,6 +186,10 @@ static void show_files(struct dir_struct *dir, const char *prefix)
 			struct cache_entry *ce = active_cache[i];
 			struct stat st;
 			int err;
+			int dtype = ce_to_dtype(ce);
+			if (dir->flags & DIR_SHOW_IGNORED &&
+			    !excluded(dir, ce->name, &dtype))
+				continue;
 			if (ce->ce_flags & CE_UPDATE)
 				continue;
 			err = lstat(ce->name, &st);

diff --git a/t/t3003-ls-files-exclude.sh b/t/t3003-ls-files-exclude.sh
index fc1e379..d5ec333 100755
--- a/t/t3003-ls-files-exclude.sh
+++ b/t/t3003-ls-files-exclude.sh

@@ -29,4 +29,12 @@ test_expect_success 'add file to gitignore' '
 '
 check_all_output
 
+test_expect_success 'ls-files -i lists only tracked-but-ignored files' '
+	echo content >other-file &&
+	git add other-file &&
+	echo file >expect &&
+	git ls-files -i --exclude-standard >output &&
+	test_cmp expect output
+'
+
 test_done

-- 
1.6.5.2.224.g22719.dirty