{"thread":{"id":"21184","subject":"[PATCH] git-add--interactive: never skip files included in index","startedAt":"2009-10-10T15:51:45Z","lastAt":"2009-10-12T23:42:14Z","messageCount":8,"participants":["Pauli Virtanen","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"124575","messageId":"1255189906-16049-1-git-send-email-pav@iki.fi","threadId":"21184","inReplyTo":null,"subject":"[PATCH] git-add--interactive: never skip files included in index","fromName":"Pauli Virtanen","fromEmail":"pav@iki.fi","sentAt":"2009-10-10T15:51:45Z","receivedAt":"2009-10-10T15:51:45Z","isPatch":true,"sender":{"key":"pav@iki.fi","avatar":null},"body":"Make \"git add -p\" to not skip files that are in index even if they are\nexcluded (by .gitignore etc.). This fixes the contradictory behavior\nthat \"git status\" and \"git commit -a\" listed such files as modified, but\n\"git add -p FILENAME\" ignored them.\n\nSigned-off-by: Pauli Virtanen <pav@iki.fi>\n---\n git-add--interactive.perl  |    2 +-\n t/t3701-add-interactive.sh |   18 ++++++++++++++++++\n 2 files changed, 19 insertions(+), 1 deletions(-)\n\ndiff --git a/git-add--interactive.perl b/git-add--interactive.perl\nindex 392efb9..69aeaf0 100755\n--- a/git-add--interactive.perl\n+++ b/git-add--interactive.perl\n@@ -259,7 +259,7 @@ sub list_modified {\n \t\t@tracked = map {\n \t\t\tchomp $_;\n \t\t\tunquote_path($_);\n-\t\t} run_cmd_pipe(qw(git ls-files --exclude-standard --), @ARGV);\n+\t\t} run_cmd_pipe(qw(git ls-files --), @ARGV);\n \t\treturn if (!@tracked);\n \t}\n \ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 62fd65e..89bfdcb 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -138,6 +138,24 @@ test_expect_success 'real edit works' '\n \ttest_cmp expected output\n '\n \n+cat >.gitignore <<EOF\n+file\n+EOF\n+cat >file <<EOF\n+changed\n+EOF\n+test_expect_success 'skip files similarly as commit -a' '\n+\tgit reset\n+\techo y | git add -p file &&\n+\tgit diff >output &&\n+\tgit reset &&\n+\tgit commit -am commit &&\n+\tgit diff >expected &&\n+\ttest_cmp expected output &&\n+\tgit reset --hard HEAD^\n+'\n+rm -f .gitignore\n+\n if test \"$(git config --bool core.filemode)\" = false\n then\n \tsay 'skipping filemode tests (filesystem does not properly support modes)'\n-- \n1.6.3.3\n"},{"id":"124630","messageId":"7viqelwyp1.fsf@alter.siamese.dyndns.org","threadId":"21184","inReplyTo":"1255189906-16049-1-git-send-email-pav@iki.fi","subject":"Re: [PATCH] git-add--interactive: never skip files included in index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-10-11T18:52:10Z","receivedAt":"2009-10-11T18:52:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks.\n\nThe change looks innocent enough and I do not expect to see any unexpected\nregressions from it, but it is a bit too late for 1.6.5 cycle, so let's\nqueue this fix and aim for 1.6.5.1.\n"},{"id":"124632","messageId":"20091011191440.GA2532@coredump.intra.peff.net","threadId":"21184","inReplyTo":"7viqelwyp1.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] git-add--interactive: never skip files included in index","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-10-11T19:14:40Z","receivedAt":"2009-10-11T19:14:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Oct 11, 2009 at 11:52:10AM -0700, Junio C Hamano wrote:\n\n> Thanks.\n> \n> The change looks innocent enough and I do not expect to see any unexpected\n> regressions from it, but it is a bit too late for 1.6.5 cycle, so let's\n> queue this fix and aim for 1.6.5.1.\n\nI think this patch is good to apply, as there is no conceivable reason\nto even look at excludes when listing modified files.\n\nBut this triggered my spider sense; shouldn't --exclude-standard simply\nbe a no-op for ls-files when we are not listing untracked files? And\nbisecting, it seems that it is a very old regression caused by 63d285c\n(per-directory-exclude: lazily read .gitignore files, 2007-11-29).\n\nI don't know if it is worth fixing now or not. It does seem a bit\ninconsistent to me (since everything else is very clear that .gitignore\nis only about untracked files), but nobody seems to have been\ncomplaining for the last two years (and they may have, in fact, been\ncoding to the new behavior).\n\n-Peff\n"},{"id":"124647","messageId":"7vpr8tr2pe.fsf@alter.siamese.dyndns.org","threadId":"21184","inReplyTo":"20091011191440.GA2532@coredump.intra.peff.net","subject":"Re: [PATCH] git-add--interactive: never skip files included in index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-10-11T22:22:05Z","receivedAt":"2009-10-11T22:22:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Sun, Oct 11, 2009 at 11:52:10AM -0700, Junio C Hamano wrote:\n>\n>> Thanks.\n>> \n>> The change looks innocent enough and I do not expect to see any unexpected\n>> regressions from it, but it is a bit too late for 1.6.5 cycle, so let's\n>> queue this fix and aim for 1.6.5.1.\n>\n> I think this patch is good to apply, as there is no conceivable reason\n> to even look at excludes when listing modified files.\n>\n> But this triggered my spider sense; shouldn't --exclude-standard simply\n> be a no-op for ls-files when we are not listing untracked files? And\n> bisecting, it seems that it is a very old regression caused by 63d285c\n> (per-directory-exclude: lazily read .gitignore files, 2007-11-29).\n>\n> I don't know if it is worth fixing now or not. It does seem a bit\n> inconsistent to me (since everything else is very clear that .gitignore\n> is only about untracked files), but nobody seems to have been\n> complaining for the last two years (and they may have, in fact, been\n> coding to the new behavior).\n\nThis is one of those moments when I feel very blessed to have competent\nand diligent people around me ;-)\n\nI think you are right; that we shouldn't filter the output with gitignore\nentries when showing what is _in_ the index.\n"},{"id":"124652","messageId":"20091012014007.GA7674@coredump.intra.peff.net","threadId":"21184","inReplyTo":"7vpr8tr2pe.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] git-add--interactive: never skip files included in index","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-10-12T01:40:07Z","receivedAt":"2009-10-12T01:40:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Oct 11, 2009 at 03:22:05PM -0700, Junio C Hamano wrote:\n\n> > I don't know if it is worth fixing now or not. It does seem a bit\n> > inconsistent to me (since everything else is very clear that .gitignore\n> > is only about untracked files), but nobody seems to have been\n> > complaining for the last two years (and they may have, in fact, been\n> > coding to the new behavior).\n> \n> This is one of those moments when I feel very blessed to have competent\n> and diligent people around me ;-)\n\nWell, I have to do _something_ to make up for all the brown paper bag\nbugs. ;)\n\n> I think you are right; that we shouldn't filter the output with gitignore\n> entries when showing what is _in_ the index.\n\nSo being the diligent and competent soul that I am, I started to prepare\na patch for this, which is included below. But the plot thickens.\n\nI bisected the change of behavior to 63d285c, as I mentioned. But when I\nfound the code that needed to be tweaked, it actually git-blames to a\nmuch earlier date in \"git show-files\", which later became ls-files.  See\n9ff768e.\n\nSo now I am doubly confused. Did this feature exist, and was broken, and\nyou actually fixed it in 63d285c, creating what looked like a regression\nbut was actually intentional?\n\nAnd more importantly, what do you want to do with it now?\n\n---\n builtin-ls-files.c          |    8 --------\n t/t3003-ls-files-exclude.sh |   32 ++++++++++++++++++++++++++++++++\n 2 files changed, 32 insertions(+), 8 deletions(-)\n create mode 100755 t/t3003-ls-files-exclude.sh\n\ndiff --git a/builtin-ls-files.c b/builtin-ls-files.c\nindex 2c95ca6..c5c0407 100644\n--- a/builtin-ls-files.c\n+++ b/builtin-ls-files.c\n@@ -170,10 +170,6 @@ static void show_files(struct dir_struct *dir, const char *prefix)\n \tif (show_cached | show_stage) {\n \t\tfor (i = 0; i < active_nr; i++) {\n \t\t\tstruct cache_entry *ce = active_cache[i];\n-\t\t\tint dtype = ce_to_dtype(ce);\n-\t\t\tif (excluded(dir, ce->name, &dtype) !=\n-\t\t\t\t\t!!(dir->flags & DIR_SHOW_IGNORED))\n-\t\t\t\tcontinue;\n \t\t\tif (show_unmerged && !ce_stage(ce))\n \t\t\t\tcontinue;\n \t\t\tif (ce->ce_flags & CE_UPDATE)\n@@ -186,10 +182,6 @@ static void show_files(struct dir_struct *dir, const char *prefix)\n \t\t\tstruct cache_entry *ce = active_cache[i];\n \t\t\tstruct stat st;\n \t\t\tint err;\n-\t\t\tint dtype = ce_to_dtype(ce);\n-\t\t\tif (excluded(dir, ce->name, &dtype) !=\n-\t\t\t\t\t!!(dir->flags & DIR_SHOW_IGNORED))\n-\t\t\t\tcontinue;\n \t\t\tif (ce->ce_flags & CE_UPDATE)\n \t\t\t\tcontinue;\n \t\t\terr = lstat(ce->name, &st);\ndiff --git a/t/t3003-ls-files-exclude.sh b/t/t3003-ls-files-exclude.sh\nnew file mode 100755\nindex 0000000..fc1e379\n--- /dev/null\n+++ b/t/t3003-ls-files-exclude.sh\n@@ -0,0 +1,32 @@\n+#!/bin/sh\n+\n+test_description='ls-files --exclude does not affect index files'\n+. ./test-lib.sh\n+\n+test_expect_success 'create repo with file' '\n+\techo content >file &&\n+\tgit add file &&\n+\tgit commit -m file &&\n+\techo modification >file\n+'\n+\n+check_output() {\n+test_expect_success \"ls-files output contains file ($1)\" \"\n+\techo '$2' >expect &&\n+\tgit ls-files --exclude-standard --$1 >output &&\n+\ttest_cmp expect output\n+\"\n+}\n+\n+check_all_output() {\n+\tcheck_output 'cached' 'file'\n+\tcheck_output 'modified' 'file'\n+}\n+\n+check_all_output\n+test_expect_success 'add file to gitignore' '\n+\techo file >.gitignore\n+'\n+check_all_output\n+\n+test_done\n-- \n1.6.5.rc3.240.g77692\n"},{"id":"124654","messageId":"7v63alpbwx.fsf@alter.siamese.dyndns.org","threadId":"21184","inReplyTo":"20091012014007.GA7674@coredump.intra.peff.net","subject":"Re: [PATCH] git-add--interactive: never skip files included in index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-10-12T02:46:06Z","receivedAt":"2009-10-12T02:46:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> So now I am doubly confused. Did this feature exist, and was broken, and\n> you actually fixed it in 63d285c, creating what looked like a regression\n> but was actually intentional?\n\nI do not think it was an intentional change.  I _think_ the comment at the\nbeginning of show_files() tells us quite a bit---we don't do read-dir when\nshowing the indexed files, and I suspect that we used to rely on the fact\nthat not doing the read-dir made exclusion mechanism a no-op.  After the\nlazy .gitignore reading, I suspect that excluded() call started to\ninitialize the exclude mechanism lazily, and that is how the bug was\nintroduced, isn't it?\n\nRegarding your patch, the loops deal with paths that are already in the\nindex, so removing the conditional skip looks like a sane thing to do.\n"},{"id":"124661","messageId":"20091012051157.GA23007@coredump.intra.peff.net","threadId":"21184","inReplyTo":"7v63alpbwx.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] git-add--interactive: never skip files included in index","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-10-12T05:11:57Z","receivedAt":"2009-10-12T05:11:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Oct 11, 2009 at 07:46:06PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > So now I am doubly confused. Did this feature exist, and was broken, and\n> > you actually fixed it in 63d285c, creating what looked like a regression\n> > but was actually intentional?\n> \n> I do not think it was an intentional change.  I _think_ the comment at the\n> beginning of show_files() tells us quite a bit---we don't do read-dir when\n> showing the indexed files, and I suspect that we used to rely on the fact\n> that not doing the read-dir made exclusion mechanism a no-op.  After the\n> lazy .gitignore reading, I suspect that excluded() call started to\n> initialize the exclude mechanism lazily, and that is how the bug was\n> introduced, isn't it?\n\nI did a bit more looking, and the situation is a bit more complex than\nthat. Hopefully the commit message below explains it.\n\n-- >8 --\nSubject: [PATCH] ls-files: excludes should not impact tracked files\n\nIn all parts of git, .gitignore and other exclude files\nimpact only how we treat untracked files; they should have\nno effect on files listed in the index.\n\nThis behavior was originally implemented very early on in\n9ff768e, but only for --exclude-from. Later, commit 63d285c\naccidentally caused us to trigger the behavior for\n--exclude-per-directory.\n\nThis patch totally ignores excludes for files found in the\nindex. This means we are reversing the original intent of\n9ff768e, while at the same time fixing the accidental\nbehavior of 63d285c. This is a good thing, though, as the\nway that 9ff768e behaved does not really make sense with the\nway exclusions are used in modern git.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nOne other option would be to retain the --exclude-from behavior but\neliminate the --exclude-per-directory behavior. I don't think this is\nvery easy, though, as it involves separating out which excludes came\nfrom which file. And I think expecting excludes to impact the list of\nindex files is crazy, anyway, since no other part of git does so. But we\nshould recognize that we are changing existing behavior; I consider it a\nbug fix, though.\n\nI also still think that Pauli's patch makes sense; there is no point in\npassing --exclude-standard there. It should be a no-op.\n\n builtin-ls-files.c          |    8 --------\n t/t3003-ls-files-exclude.sh |   32 ++++++++++++++++++++++++++++++++\n 2 files changed, 32 insertions(+), 8 deletions(-)\n create mode 100755 t/t3003-ls-files-exclude.sh\n\ndiff --git a/builtin-ls-files.c b/builtin-ls-files.c\nindex 2c95ca6..c5c0407 100644\n--- a/builtin-ls-files.c\n+++ b/builtin-ls-files.c\n@@ -170,10 +170,6 @@ static void show_files(struct dir_struct *dir, const char *prefix)\n \tif (show_cached | show_stage) {\n \t\tfor (i = 0; i < active_nr; i++) {\n \t\t\tstruct cache_entry *ce = active_cache[i];\n-\t\t\tint dtype = ce_to_dtype(ce);\n-\t\t\tif (excluded(dir, ce->name, &dtype) !=\n-\t\t\t\t\t!!(dir->flags & DIR_SHOW_IGNORED))\n-\t\t\t\tcontinue;\n \t\t\tif (show_unmerged && !ce_stage(ce))\n \t\t\t\tcontinue;\n \t\t\tif (ce->ce_flags & CE_UPDATE)\n@@ -186,10 +182,6 @@ static void show_files(struct dir_struct *dir, const char *prefix)\n \t\t\tstruct cache_entry *ce = active_cache[i];\n \t\t\tstruct stat st;\n \t\t\tint err;\n-\t\t\tint dtype = ce_to_dtype(ce);\n-\t\t\tif (excluded(dir, ce->name, &dtype) !=\n-\t\t\t\t\t!!(dir->flags & DIR_SHOW_IGNORED))\n-\t\t\t\tcontinue;\n \t\t\tif (ce->ce_flags & CE_UPDATE)\n \t\t\t\tcontinue;\n \t\t\terr = lstat(ce->name, &st);\ndiff --git a/t/t3003-ls-files-exclude.sh b/t/t3003-ls-files-exclude.sh\nnew file mode 100755\nindex 0000000..fc1e379\n--- /dev/null\n+++ b/t/t3003-ls-files-exclude.sh\n@@ -0,0 +1,32 @@\n+#!/bin/sh\n+\n+test_description='ls-files --exclude does not affect index files'\n+. ./test-lib.sh\n+\n+test_expect_success 'create repo with file' '\n+\techo content >file &&\n+\tgit add file &&\n+\tgit commit -m file &&\n+\techo modification >file\n+'\n+\n+check_output() {\n+test_expect_success \"ls-files output contains file ($1)\" \"\n+\techo '$2' >expect &&\n+\tgit ls-files --exclude-standard --$1 >output &&\n+\ttest_cmp expect output\n+\"\n+}\n+\n+check_all_output() {\n+\tcheck_output 'cached' 'file'\n+\tcheck_output 'modified' 'file'\n+}\n+\n+check_all_output\n+test_expect_success 'add file to gitignore' '\n+\techo file >.gitignore\n+'\n+check_all_output\n+\n+test_done\n-- \n1.6.5.rc3.240.g77692\n"},{"id":"124779","messageId":"7vskdo5gdl.fsf@alter.siamese.dyndns.org","threadId":"21184","inReplyTo":"20091012051157.GA23007@coredump.intra.peff.net","subject":"Re: [PATCH] git-add--interactive: never skip files included in index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-10-12T23:42:14Z","receivedAt":"2009-10-12T23:42:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Subject: [PATCH] ls-files: excludes should not impact tracked files\n>\n> In all parts of git, .gitignore and other exclude files\n> impact only how we treat untracked files; they should have\n> no effect on files listed in the index.\n>\n> This behavior was originally implemented very early on in\n> 9ff768e, but only for --exclude-from. Later, commit 63d285c\n> accidentally caused us to trigger the behavior for\n> --exclude-per-directory.\n>\n> This patch totally ignores excludes for files found in the\n> index. This means we are reversing the original intent of\n> 9ff768e, while at the same time fixing the accidental\n> behavior of 63d285c. This is a good thing, though, as the\n> way that 9ff768e behaved does not really make sense with the\n> way exclusions are used in modern git.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n\nMakes sense; thanks.\n\n> I also still think that Pauli's patch makes sense; there is no point in\n> passing --exclude-standard there. It should be a no-op.\n\nYeah, that is also queued independent from this one.\n"}]}