{"thread":{"id":"40896","subject":"[PATCH] clean: new option --exclude-from","startedAt":"2015-11-26T14:44:25Z","lastAt":"2015-12-07T22:53:33Z","messageCount":21,"participants":["James","Eric Sunshine","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"273784","messageId":"1448549065-16337-1-git-send-email-rouzier@gmail.com","threadId":"40896","inReplyTo":null,"subject":"[PATCH] clean: new option --exclude-from","fromName":"James","fromEmail":"rouzier@gmail.com","sentAt":"2015-11-26T14:44:25Z","receivedAt":"2015-11-26T14:44:25Z","isPatch":true,"sender":{"key":"rouzier@gmail.com","avatar":null},"body":"From: James Rouzier \n\nSpecify a file to read for exclude patterns.\n---\n Documentation/git-clean.txt | 5 ++++-\n builtin/clean.c | 15 ++++++++++++++-\n t/t7300-clean.sh | 35 +++++++++++++++++++++++++++++++++++\n 3 files changed, 53 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-clean.txt b/Documentation/git-clean.txt\nindex 641681f..ccd0aa4 100644\n--- a/Documentation/git-clean.txt\n+++ b/Documentation/git-clean.txt\n@@ -8,7 +8,7 @@ git-clean - Remove untracked files from the working tree\n SYNOPSIS\n --------\n [verse]\n-'git clean' [-d] [-f] [-i] [-n] [-q] [-e ] [-x | -X] [--] ...\n+'git clean' [-d] [-f] [-i] [-n] [-q] [-e ] [--exclude-from ] [-x | -X] [--] ...\n \n DESCRIPTION\n -----------\n@@ -61,6 +61,9 @@ OPTIONS\n \t$GIT_DIR/info/exclude, also consider these patterns to be in the\n \tset of the ignore rules in effect.\n \n+--exclude-from=::\n+\tRead exclude patterns from ; 1 per line.\n+\n -x::\n \tDon't use the standard ignore rules read from .gitignore (per\n \tdirectory) and $GIT_DIR/info/exclude, but do still use the ignore\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex d7acb94..e4995a3 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -875,6 +875,16 @@ static void interactive_main_loop(void)\n \t}\n }\n \n+static int option_parse_exclude_from(const struct option *opt,\n+\t\t\t\t const char *arg, int unset)\n+{\n+\tstruct dir_struct *dir = opt->value;\n+\n+\tadd_excludes_from_file(dir, arg);\n+\n+\treturn 0;\n+}\n+\n int cmd_clean(int argc, const char **argv, const char *prefix)\n {\n \tint i, res;\n@@ -898,11 +908,15 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \t\t\t\tN_(\"remove whole directories\")),\n \t\t{ OPTION_CALLBACK, 'e', \"exclude\", &exclude_list, N_(\"pattern\"),\n \t\t N_(\"add to ignore rules\"), PARSE_OPT_NONEG, exclude_cb },\n+\t\t{ OPTION_CALLBACK, 0, \"exclude-from\", &dir, N_(\"file\"),\n+\t\t\tN_(\"exclude patterns are read from \"),\n+\t\t\t0, option_parse_exclude_from },\n \t\tOPT_BOOL('x', NULL, &ignored, N_(\"remove ignored files, too\")),\n \t\tOPT_BOOL('X', NULL, &ignored_only,\n \t\t\t\tN_(\"remove only ignored files\")),\n \t\tOPT_END()\n \t};\n+\tmemset(&dir, 0, sizeof(dir));\n \n \tgit_config(git_clean_config, NULL);\n \tif (force < 0)\n@@ -913,7 +927,6 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, options, builtin_clean_usage,\n \t\t\t 0);\n \n-\tmemset(&dir, 0, sizeof(dir));\n \tif (ignored_only)\n \t\tdir.flags |= DIR_SHOW_IGNORED;\n \ndiff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\nindex 86ceb38..9b648b9 100755\n--- a/t/t7300-clean.sh\n+++ b/t/t7300-clean.sh\n@@ -630,6 +630,41 @@ test_expect_success 'git clean -e' '\n \t)\n '\n \n+test_expect_success 'git clean --exclude-from' '\n+\trm -fr repo &&\n+\tmkdir repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit init &&\n+\t\ttouch known 1 2 3 &&\n+\t\tgit add known &&\n+\t\techo 1 >> .git/clean-exclude &&\n+\t\techo 2 >> .git/clean-exclude &&\n+\t\tgit clean -f --exclude-from=.git/clean-exclude &&\n+\t\ttest -e 1 &&\n+\t\ttest -e 2 &&\n+\t\t! (test -e 3) &&\n+\t\ttest -e known\n+\t)\n+'\n+\n+test_expect_success 'git clean -e --exclude-from' '\n+\trm -fr repo &&\n+\tmkdir repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit init &&\n+\t\ttouch known 1 2 3 &&\n+\t\tgit add known &&\n+\t\techo 1 >> .git/clean-exclude &&\n+\t\tgit clean -f -e 2 --exclude-from=.git/clean-exclude &&\n+\t\ttest -e 1 &&\n+\t\ttest -e 2 &&\n+\t\t! (test -e 3) &&\n+\t\ttest -e known\n+\t)\n+'\n+\n test_expect_success SANITY 'git clean -d with an unreadable empty directory' '\n \tmkdir foo &&\n \tchmod a= foo &&\n-- \n2.3.6\n"},{"id":"273822","messageId":"CAPig+cQMXgan0Hkm+HJt836Xm9AObxr8EW0DXb=puPzWD+v=Dg@mail.gmail.com","threadId":"40896","inReplyTo":"1448549065-16337-1-git-send-email-rouzier@gmail.com","subject":"Re: [PATCH] clean: new option --exclude-from","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-11-30T02:24:04Z","receivedAt":"2015-11-30T02:24:04Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Nov 26, 2015 at 9:44 AM, James wrote:\n> From: James Rouzier \n>\n> Specify a file to read for exclude patterns.\n> ---\n> diff --git a/Documentation/git-clean.txt b/Documentation/git-clean.txt\n> @@ -8,7 +8,7 @@ git-clean - Remove untracked files from the working tree\n> SYNOPSIS\n> --------\n> [verse]\n> -'git clean' [-d] [-f] [-i] [-n] [-q] [-e ] [-x | -X] [--] ...\n> +'git clean' [-d] [-f] [-i] [-n] [-q] [-e ] [--exclude-from ] [-x | -X] [--] ...\n\nCommon practice seems to be to include the '=' in the synopsis:\n[--exclude-from=]\n\n> DESCRIPTION\n> -----------\n> @@ -61,6 +61,9 @@ OPTIONS\n> $GIT_DIR/info/exclude, also consider these patterns to be in the\n> set of the ignore rules in effect.\n>\n> +--exclude-from=::\n> + Read exclude patterns from ; 1 per line.\n\ns/;/,/ maybe?\n\n> diff --git a/builtin/clean.c b/builtin/clean.c\n> @@ -875,6 +875,16 @@ static void interactive_main_loop(void)\n> +static int option_parse_exclude_from(const struct option *opt,\n> + const char *arg, int unset)\n\nFor consistency with the other callback function in this file, you'd\nprobably want to name this function exclude_from_cb().\n\n> +{\n> + struct dir_struct *dir = opt->value;\n> +\n> + add_excludes_from_file(dir, arg);\n> +\n> + return 0;\n\nStyle: You can drop the blank line before 'return'.\n\n> +}\n> @@ -898,11 +908,15 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n> N_(\"remove whole directories\")),\n> { OPTION_CALLBACK, 'e', \"exclude\", &exclude_list, N_(\"pattern\"),\n> N_(\"add to ignore rules\"), PARSE_OPT_NONEG, exclude_cb },\n> + { OPTION_CALLBACK, 0, \"exclude-from\", &dir, N_(\"file\"),\n> + N_(\"exclude patterns are read from \"),\n\nThe existing descriptions all use imperative mood. This probably ought\nto follow suit:\n\n N_(\"read exclude patterns from file\"),\n\n> + 0, option_parse_exclude_from },\n> OPT_BOOL('x', NULL, &ignored, N_(\"remove ignored files, too\")),\n> OPT_BOOL('X', NULL, &ignored_only,\n> N_(\"remove only ignored files\")),\n> OPT_END()\n> };\n> + memset(&dir, 0, sizeof(dir));\n\nStyle: Leave a blank line after the final declaration (before the memset).\n\nAlso, why move the memset() all the way up here as opposed, say, to\nmoving it just before the parse_options() invocation? Is it just to\nmake it easier for the next person who comes along wanting to\nmanipulate 'dir' early on (before git_config(), for instance)?\n\n> git_config(git_clean_config, NULL);\n> if (force < 0)\n> @@ -913,7 +927,6 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n> argc = parse_options(argc, argv, prefix, options, builtin_clean_usage,\n> 0);\n>\n> - memset(&dir, 0, sizeof(dir));\n> if (ignored_only)\n> dir.flags |= DIR_SHOW_IGNORED;\n>\n> diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\n> @@ -630,6 +630,41 @@ test_expect_success 'git clean -e' '\n> +test_expect_success 'git clean --exclude-from' '\n> + rm -fr repo &&\n> + mkdir repo &&\n> + (\n> + cd repo &&\n> + git init &&\n> + touch known 1 2 3 &&\n\nUnless the timestamp is significant, we normally avoid using 'touch'\nand instead just use redirection to create empty files:\n\n >1\n >2\n >3\n\nIn this case, though, you're merely matching existing style in this\nscript, so 'touch' may be okay.\n\n> + git add known &&\n> + echo 1 >> .git/clean-exclude &&\n> + echo 2 >> .git/clean-exclude &&\n\nStyle: no space after redirection operator.\n\nA here-doc may be cleaner and easier to maintain:\n\n cat >.git/clean-exclude <<-\\EOF\n 1\n 2\n EOF\n\n> + git clean -f --exclude-from=.git/clean-exclude &&\n> + test -e 1 &&\n> + test -e 2 &&\n> + ! (test -e 3) &&\n\nI see that you copied this from the \"git clean -e\" test, but it's not\nobvious why parentheses are needed or wanted, and none of the other\ntests use parentheses when negating the return of 'test', thus they\nprobably ought to be dropped.\n\n> + test -e known\n\nModern scripts would normally use test_path_is_file() and\ntest_path_is_missing() instead of 'test -e', however, you are again\nmatching existing style in this script, so 'test -e' may be\nreasonable.\n\n> + )\n> +'\n> +\n> +test_expect_success 'git clean -e --exclude-from' '\n> + rm -fr repo &&\n> + mkdir repo &&\n> + (\n> + cd repo &&\n> + git init &&\n> + touch known 1 2 3 &&\n> + git add known &&\n> + echo 1 >> .git/clean-exclude &&\n> + git clean -f -e 2 --exclude-from=.git/clean-exclude &&\n> + test -e 1 &&\n> + test -e 2 &&\n> + ! (test -e 3) &&\n> + test -e known\n> + )\n> +'\n\nShould a test be added which uses --exclude-from multiple times in the\nsame git-clean invocation?\n\nWould it make sense add a test checking the behavior when the file\nnamed by --exclude-from doesn't exist or is otherwise unusable as an\nexclusion file?\n\n> test_expect_success SANITY 'git clean -d with an unreadable empty directory' '\n> mkdir foo &&\n> chmod a= foo &&\n> --\n> 2.3.6\n"},{"id":"273880","messageId":"CAPig+cRa31uriO4qkZUydooNx0V+dNrUgFvTUxoLL9gCjq9AHQ@mail.gmail.com","threadId":"40896","inReplyTo":"CAGjXF72PgdjBw03ERVYxj+atvsBXK0LeJ6O3zTZgi3-kv9BWsw@mail.gmail.com","subject":"Re: [PATCH] clean: new option --exclude-from","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-12-01T22:25:38Z","receivedAt":"2015-12-01T22:25:38Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Dec 1, 2015 at 4:36 PM, James Rouzier wrote:\n> Eric thank you for the feedback.\n\n[re-adding git@vger.kernel.org to recipient list since this response\nwas likely intended to be public]\n\n> On Sun, Nov 29, 2015 at 9:24 PM, Eric Sunshine \n> wrote:\n>> On Thu, Nov 26, 2015 at 9:44 AM, James wrote:\n>> > From: James Rouzier \n>> >\n>> > Specify a file to read for exclude patterns.\n>> > ---\n>> > @@ -61,6 +61,9 @@ OPTIONS\n>> > $GIT_DIR/info/exclude, also consider these patterns to be in the\n>> > set of the ignore rules in effect.\n>> >\n>> > +--exclude-from=::\n>> > + Read exclude patterns from ; 1 per line.\n>>\n>> s/;/,/ maybe?\n>\n> I copied this from Documentation/git-ls-files.txt to try and keep the\n> documentation style consistent.\n> However if it is believed to be better I will change it here and also in a\n> separate patch for Documentation/git-ls-files.txt\n\nI don't feel strongly about it. Existing precedence may be a good\nargument in its favor.\n\n>> Also, why move the memset() all the way up here as opposed, say, to\n>> moving it just before the parse_options() invocation? Is it just to\n>> make it easier for the next person who comes along wanting to\n>> manipulate 'dir' early on (before git_config(), for instance)?\n>\n> Yes I want to make sure that the 'dir' is initialized before any usage.\n>\n>> > + git clean -f --exclude-from=.git/clean-exclude &&\n>> > + test -e 1 &&\n>> > + test -e 2 &&\n>> > + ! (test -e 3) &&\n>>\n>> I see that you copied this from the \"git clean -e\" test, but it's not\n>> obvious why parentheses are needed or wanted, and none of the other\n>> tests use parentheses when negating the return of 'test', thus they\n>> probably ought to be dropped.\n>\n> Ok will do\n>\n>> > + test -e known\n>>\n>> Modern scripts would normally use test_path_is_file() and\n>> test_path_is_missing() instead of 'test -e', however, you are again\n>> matching existing style in this script, so 'test -e' may be\n>> reasonable.\n>\n> Since it is the standard I could just take the time to upgrade 'test -e' in\n> this test file to use newer standard.\n\nThis test script is probably relatively quiescent right now, so such\ncleanup may be reasonable. Since it is conceptually distinct from the\npurpose of the current patch, you would want to do the cleanup as a\npreparatory patch, thus making this a 2-patch series.\n\n>> > +test_expect_success 'git clean -e --exclude-from' '\n>> > + rm -fr repo &&\n>> > + mkdir repo &&\n>> > + (\n>> > + cd repo &&\n>> > + git init &&\n>> > + touch known 1 2 3 &&\n>> > + git add known &&\n>> > + echo 1 >> .git/clean-exclude &&\n>> > + git clean -f -e 2 --exclude-from=.git/clean-exclude &&\n>> > + test -e 1 &&\n>> > + test -e 2 &&\n>> > + ! (test -e 3) &&\n>> > + test -e known\n>> > + )\n>> > +'\n>>\n>> Should a test be added which uses --exclude-from multiple times in the\n>> same git-clean invocation?\n>\n> That does make sense will do.\n>\n>> Would it make sense add a test checking the behavior when the file\n>> named by --exclude-from doesn't exist or is otherwise unusable as an\n>> exclusion file?\n>\n> At the moment the add_excludes_from_file function will exit the program if\n> there is a problem loading the exclude file.\n> I could add a test for that behavior. In case in the future this behavior is\n> changed.\n"},{"id":"273891","messageId":"20151202005338.GD28197@sigill.intra.peff.net","threadId":"40896","inReplyTo":"1448549065-16337-1-git-send-email-rouzier@gmail.com","subject":"Re: [PATCH] clean: new option --exclude-from","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-02T00:53:38Z","receivedAt":"2015-12-02T00:53:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 26, 2015 at 09:44:25AM -0500, James wrote:\n\n> From: James Rouzier \n> \n> Specify a file to read for exclude patterns.\n> ---\n\nLots of commands care about excludes (e.g., \"add\", \"status\").\n\nShould this perhaps be an option to the main \"git\" to append to the set\nof excludes?\n\nYou can kind-of do this already with:\n\n git -c core.excludesfile=/path/to/whatever clean ...\n\nbut of course you might be using core.excludesfile already. I wonder if\nthat config option should take multiple values and respect all of them,\nrather than last-one-wins.\n\n-Peff\n"},{"id":"273895","messageId":"xmqq37vl7fux.fsf@gitster.mtv.corp.google.com","threadId":"40896","inReplyTo":"20151202005338.GD28197@sigill.intra.peff.net","subject":"Re: [PATCH] clean: new option --exclude-from","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-02T02:18:30Z","receivedAt":"2015-12-02T02:18:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Nov 26, 2015 at 09:44:25AM -0500, James wrote:\n>\n>> From: James Rouzier \n>> \n>> Specify a file to read for exclude patterns.\n>> ---\n>\n> Lots of commands care about excludes (e.g., \"add\", \"status\").\n>\n> Should this perhaps be an option to the main \"git\" to append to the set\n> of excludes?\n>\n> You can kind-of do this already with:\n>\n> git -c core.excludesfile=/path/to/whatever clean ...\n>\n> but of course you might be using core.excludesfile already. I wonder if\n> that config option should take multiple values and respect all of them,\n> rather than last-one-wins.\n\nIt is likely that existing users are already using $HOME/.gitconfig\nthat sets core.excludesfile=$HOME/.gitconfig as the personal\nfallback, that is overriden, not tweaked, by project specific\nsettings of the same variable in .git/config, so that would not fly\nvery well, I suspect.\n"},{"id":"273899","messageId":"20151202024403.GA29329@sigill.intra.peff.net","threadId":"40896","inReplyTo":"xmqq37vl7fux.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] clean: new option --exclude-from","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-02T02:44:03Z","receivedAt":"2015-12-02T02:44:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 01, 2015 at 06:18:30PM -0800, Junio C Hamano wrote:\n\n> > Should this perhaps be an option to the main \"git\" to append to the set\n> > of excludes?\n> >\n> > You can kind-of do this already with:\n> >\n> > git -c core.excludesfile=/path/to/whatever clean ...\n> >\n> > but of course you might be using core.excludesfile already. I wonder if\n> > that config option should take multiple values and respect all of them,\n> > rather than last-one-wins.\n> \n> It is likely that existing users are already using $HOME/.gitconfig\n> that sets core.excludesfile=$HOME/.gitconfig as the personal\n> fallback, that is overriden, not tweaked, by project specific\n> settings of the same variable in .git/config, so that would not fly\n> very well, I suspect.\n\nMaybe. I would think the more common setup is:\n\n 1. Personal exclude files (e.g., your editor's backup files) come from\n ~/.gitconfig.\n\n 2. Per-project personal excludes go directly into .git/info/exclude.\n\nBut you're right that it would be a backwards-incompatible change.\n\n-Peff\n"},{"id":"273911","messageId":"xmqqlh9c6byy.fsf@gitster.mtv.corp.google.com","threadId":"40896","inReplyTo":"20151202024403.GA29329@sigill.intra.peff.net","subject":"Re: [PATCH] clean: new option --exclude-from","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-02T16:40:05Z","receivedAt":"2015-12-02T16:40:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Dec 01, 2015 at 06:18:30PM -0800, Junio C Hamano wrote:\n>\n>> It is likely that existing users are already using $HOME/.gitconfig\n>> that sets core.excludesfile=$HOME/.gitconfig as the personal\n>> fallback, that is overriden, not tweaked, by project specific\n>> settings of the same variable in .git/config, so that would not fly\n>> very well, I suspect.\n>\n> Maybe. I would think the more common setup is:\n>\n> 1. Personal exclude files (e.g., your editor's backup files) come from\n> ~/.gitconfig.\n>\n> 2. Per-project personal excludes go directly into .git/info/exclude.\n>\n> But you're right that it would be a backwards-incompatible change.\n\nNo question about it, but at the same time I can sort of see how\nuseful being able to read from more than one would be.\n\nBut for this particular one, I viewed the topic as adding a new\noption as a shorter way for passing multiple -e options on\nthe command line. When viewed that way, even if core.excludesfile\nwere multi-valued, I wouldn't have imagined that people would try to\nuse that mechanism for such a purpose--for one thing, the precedence\norder is wrong for that purpose, isn't it?\n"},{"id":"273912","messageId":"20151202164712.GA16636@sigill.intra.peff.net","threadId":"40896","inReplyTo":"xmqqlh9c6byy.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] clean: new option --exclude-from","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-02T16:47:12Z","receivedAt":"2015-12-02T16:47:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 02, 2015 at 08:40:05AM -0800, Junio C Hamano wrote:\n\n> But for this particular one, I viewed the topic as adding a new\n> option as a shorter way for passing multiple -e options on\n> the command line. When viewed that way, even if core.excludesfile\n> were multi-valued, I wouldn't have imagined that people would try to\n> use that mechanism for such a purpose--for one thing, the precedence\n> order is wrong for that purpose, isn't it?\n\nGood point. I didn't think about precedence at all.\n\nI perhaps shouldn't have brought up core.excludesfile as all. I only\nmeant it as \"you maybe can hack your way through to this without adding\nany code\".\n\nI think a real option to do git-wide excludes would probably be more\nlike:\n\n git --exclude-from=/path/to/file clean ...\n\nand that in turn would probably set GIT_EXCLUDE_FROM to a\ncolon-separated list of paths (or similar) so that sub-processes would\nrespect it, too.\n\nOn the other hand, one could make the same argument about the existing\n\"-e\". You cannot do:\n\n git add --exclude='*.o' .\n\nright now. So maybe \"clean\" is really the only place where people care\nabout such ad-hoc exclusion. Or maybe this an opportunity to add:\n\n git --exclude='*.o' clean\n\nI dunno. I cannot think of a time when I would have used any of those\noptions myself.\n\n-Peff\n"},{"id":"273915","messageId":"xmqqd1uo69vf.fsf@gitster.mtv.corp.google.com","threadId":"40896","inReplyTo":"20151202164712.GA16636@sigill.intra.peff.net","subject":"Re: [PATCH] clean: new option --exclude-from","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-02T17:25:24Z","receivedAt":"2015-12-02T17:25:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> .... So maybe \"clean\" is really the only place where people care\n> about such ad-hoc exclusion. Or maybe this an opportunity to add:\n>\n> git --exclude='*.o' clean\n>\n> I dunno. I cannot think of a time when I would have used any of those\n> options myself.\n\nMe either and I do not think I ever wanted to use -e to \"clean\".\n\nI do not think we should liberally add options that apply to\nanything \"git\" in the first place [*1*]. Limit them to ones that\nare really special and fundamental that changes the way Git\noperates, i.e. \"Where is our $GIT_DIR?\" is a good thing for users to\nbe able to tell \"git\" itself. Compared to that, the ignore patterns\nis a fringe that is used only by commands that care about the\nworking tree (e.g. the global option in \"git --exclude='*.o'\nls-tree\" would be meaningless).\n\n\n[Footnote]\n\n*1* It would add unnecessary confusion to the end users; they have\n to decide if they need to pass an option before or after the\n subcommand name. If the motivation behind the \"git --option cmd\"\n is to share code and semantics for common \"--option\", we should\n instead further refactor command line option handling, just like\n the code for config handling allows us to share config_default.\n"},{"id":"273916","messageId":"20151202175144.GA18550@sigill.intra.peff.net","threadId":"40896","inReplyTo":"xmqqd1uo69vf.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] clean: new option --exclude-from","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-02T17:51:45Z","receivedAt":"2015-12-02T17:51:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 02, 2015 at 09:25:24AM -0800, Junio C Hamano wrote:\n\n> I do not think we should liberally add options that apply to\n> anything \"git\" in the first place [*1*]. Limit them to ones that\n> are really special and fundamental that changes the way Git\n> operates, i.e. \"Where is our $GIT_DIR?\" is a good thing for users to\n> be able to tell \"git\" itself. Compared to that, the ignore patterns\n> is a fringe that is used only by commands that care about the\n> working tree (e.g. the global option in \"git --exclude='*.o'\n> ls-tree\" would be meaningless).\n> \n> \n> [Footnote]\n> \n> *1* It would add unnecessary confusion to the end users; they have\n> to decide if they need to pass an option before or after the\n> subcommand name. If the motivation behind the \"git --option cmd\"\n> is to share code and semantics for common \"--option\", we should\n> instead further refactor command line option handling, just like\n> the code for config handling allows us to share config_default.\n\nMy motivation isn't exactly code sharing. It is that you sometimes want\nto affect sub-commands of a program, and cannot pass command line\noptions to them yourself.\n\nFor instance, \"git-stash --include-untracked\" will call \"git clean\"\nunder the hood. There is no way to say \"...and treat foo.* as ignored\nfor this invocation\". It could grow its own \"-e\" option, but that does\nnot help any other third-party scripts which call \"git clean\".\n\nSo IMHO this is not really about command-line options, but about the\nenvironment in which a command is executed. Environment variables are\nthe obvious way to do that; \"git --foo\" options are just syntactic sugar\nto set the variables. We could just add variables without matching\noptions.\n\nI agree that we could end up proliferating such options (or environment\nvariables). Using the logic above, you could argue that I should be able\nto affect any option of any sub-command in a script, which just gets\nsilly. My rule of thumb would be that if there is some implicit state in\nthe on-disk repo (e.g., what is in your .git/config) that you\nmight want to override for a one-shot invocation, then it may be a\nreasonable candidate. The \"git -c\" config override is such an example.\n\nIn this case, it is basically adding to \".git/info/exclude\", which\nfollows the same rule.\n\nBut like I said, I do not feel all that strongly about this particular\noption. I would not use it myself. Just trying to make my reasoning\nclear. :)\n\n-Peff\n"},{"id":"274059","messageId":"xmqqr3j0nsk8.fsf@gitster.mtv.corp.google.com","threadId":"40896","inReplyTo":"20151202175144.GA18550@sigill.intra.peff.net","subject":"Re: [PATCH] clean: new option --exclude-from","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-06T03:51:03Z","receivedAt":"2015-12-06T03:51:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> My motivation isn't exactly code sharing. It is that you sometimes want\n> to affect sub-commands of a program, and cannot pass command line\n> options to them yourself.\n>\n> For instance, \"git-stash --include-untracked\" will call \"git clean\"\n> under the hood. There is no way to say \"...and treat foo.* as ignored\n> for this invocation\". It could grow its own \"-e\" option, but that does\n> not help any other third-party scripts which call \"git clean\".\n>\n> So IMHO this is not really about command-line options, but about the\n> environment in which a command is executed. Environment variables are\n> the obvious way to do that; \"git --foo\" options are just syntactic sugar\n> to set the variables. We could just add variables without matching\n> options.\n\nI'd be even more wary of that, as different commands use ignore\npatterns for different purposes. A script may invoke \"clean\" and\n\"add\" and would want to use different sets of ignore patterns to\nemulate \"precious\" class (which we do not have), for example, by\ngiving a wider ignore pattern for \"add\" to prevent a file that must\nbe kept untracked outside the index while telling \"clean -X\" that\nthat file is not expendable with a narrower ignore pattern. That is\njust one example that comes to me without thinking about the issues\ntoo hard, so I am reasonably sure that it would hurt the ecosystem\nto promote that the ignore pattern can be used for specifying\nimportant per-invocation input to a script.\n\nIn any case, what we've been discussing may be an interesting and\npotentially important tangent, but it still is a tangent while\nevaluating the patch in question. I do not think I'd be using the\nnew \"--exclude-from=\" option to \"clean\" (simply because I do\nnot think I've ever used the existing \"-e\" option to it unless I am\nmerely testing the command to make sure it works as advertised)\nmyself, but I do not immediately see how it would hurt us in the\nfuture to add it now. So...\n"},{"id":"274072","messageId":"1449413906-23256-1-git-send-email-rouzier@gmail.com","threadId":"40896","inReplyTo":"CAPig+cRa31uriO4qkZUydooNx0V+dNrUgFvTUxoLL9gCjq9AHQ@mail.gmail.com","subject":"[PATCH v2 1/2] modernize t7300","fromName":"James","fromEmail":"rouzier@gmail.com","sentAt":"2015-12-06T14:58:25Z","receivedAt":"2015-12-06T14:58:25Z","isPatch":true,"sender":{"key":"rouzier@gmail.com","avatar":null},"body":"From: James Rouzier \n\n---\n t/t7300-clean.sh | 382 +++++++++++++++++++++++++++----------------------------\n 1 file changed, 190 insertions(+), 192 deletions(-)\n\ndiff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\nindex 86ceb38..d555bb6 100755\n--- a/t/t7300-clean.sh\n+++ b/t/t7300-clean.sh\n@@ -28,15 +28,15 @@ test_expect_success 'git clean with skip-worktree .gitignore' '\n \tmkdir -p build docs &&\n \ttouch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&\n \tgit clean &&\n-\ttest -f Makefile &&\n-\ttest -f README &&\n-\ttest -f src/part1.c &&\n-\ttest -f src/part2.c &&\n-\ttest ! -f a.out &&\n-\ttest ! -f src/part3.c &&\n-\ttest -f docs/manual.txt &&\n-\ttest -f obj.o &&\n-\ttest -f build/lib.so &&\n+\ttest_path_is_file Makefile &&\n+\ttest_path_is_file README &&\n+\ttest_path_is_file src/part1.c &&\n+\ttest_path_is_file src/part2.c &&\n+\ttest_path_is_missing a.out &&\n+\ttest_path_is_missing src/part3.c &&\n+\ttest_path_is_file docs/manual.txt &&\n+\ttest_path_is_file obj.o &&\n+\ttest_path_is_file build/lib.so &&\n \tgit update-index --no-skip-worktree .gitignore &&\n \tgit checkout .gitignore\n '\n@@ -46,15 +46,15 @@ test_expect_success 'git clean' '\n \tmkdir -p build docs &&\n \ttouch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&\n \tgit clean &&\n-\ttest -f Makefile &&\n-\ttest -f README &&\n-\ttest -f src/part1.c &&\n-\ttest -f src/part2.c &&\n-\ttest ! -f a.out &&\n-\ttest ! -f src/part3.c &&\n-\ttest -f docs/manual.txt &&\n-\ttest -f obj.o &&\n-\ttest -f build/lib.so\n+\ttest_path_is_file Makefile &&\n+\ttest_path_is_file README &&\n+\ttest_path_is_file src/part1.c &&\n+\ttest_path_is_file src/part2.c &&\n+\ttest_path_is_missing a.out &&\n+\ttest_path_is_missing src/part3.c &&\n+\ttest_path_is_file docs/manual.txt &&\n+\ttest_path_is_file obj.o &&\n+\ttest_path_is_file build/lib.so\n \n '\n \n@@ -63,15 +63,15 @@ test_expect_success 'git clean src/' '\n \tmkdir -p build docs &&\n \ttouch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&\n \tgit clean src/ &&\n-\ttest -f Makefile &&\n-\ttest -f README &&\n-\ttest -f src/part1.c &&\n-\ttest -f src/part2.c &&\n-\ttest -f a.out &&\n-\ttest ! -f src/part3.c &&\n-\ttest -f docs/manual.txt &&\n-\ttest -f obj.o &&\n-\ttest -f build/lib.so\n+\ttest_path_is_file Makefile &&\n+\ttest_path_is_file README &&\n+\ttest_path_is_file src/part1.c &&\n+\ttest_path_is_file src/part2.c &&\n+\ttest_path_is_file a.out &&\n+\ttest_path_is_missing src/part3.c &&\n+\ttest_path_is_file docs/manual.txt &&\n+\ttest_path_is_file obj.o &&\n+\ttest_path_is_file build/lib.so\n \n '\n \n@@ -80,15 +80,15 @@ test_expect_success 'git clean src/ src/' '\n \tmkdir -p build docs &&\n \ttouch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&\n \tgit clean src/ src/ &&\n-\ttest -f Makefile &&\n-\ttest -f README &&\n-\ttest -f src/part1.c &&\n-\ttest -f src/part2.c &&\n-\ttest -f a.out &&\n-\ttest ! -f src/part3.c &&\n-\ttest -f docs/manual.txt &&\n-\ttest -f obj.o &&\n-\ttest -f build/lib.so\n+\ttest_path_is_file Makefile &&\n+\ttest_path_is_file README &&\n+\ttest_path_is_file src/part1.c &&\n+\ttest_path_is_file src/part2.c &&\n+\ttest_path_is_file a.out &&\n+\ttest_path_is_missing src/part3.c &&\n+\ttest_path_is_file docs/manual.txt &&\n+\ttest_path_is_file obj.o &&\n+\ttest_path_is_file build/lib.so\n \n '\n \n@@ -97,16 +97,16 @@ test_expect_success 'git clean with prefix' '\n \tmkdir -p build docs src/test &&\n \ttouch a.out src/part3.c docs/manual.txt obj.o build/lib.so src/test/1.c &&\n \t(cd src/ && git clean) &&\n-\ttest -f Makefile &&\n-\ttest -f README &&\n-\ttest -f src/part1.c &&\n-\ttest -f src/part2.c &&\n-\ttest -f a.out &&\n-\ttest ! -f src/part3.c &&\n-\ttest -f src/test/1.c &&\n-\ttest -f docs/manual.txt &&\n-\ttest -f obj.o &&\n-\ttest -f build/lib.so\n+\ttest_path_is_file Makefile &&\n+\ttest_path_is_file README &&\n+\ttest_path_is_file src/part1.c &&\n+\ttest_path_is_file src/part2.c &&\n+\ttest_path_is_file a.out &&\n+\ttest_path_is_missing src/part3.c &&\n+\ttest_path_is_file src/test/1.c &&\n+\ttest_path_is_file docs/manual.txt &&\n+\ttest_path_is_file obj.o &&\n+\ttest_path_is_file build/lib.so\n \n '\n \n@@ -160,16 +160,16 @@ test_expect_success 'git clean -d with prefix and path' '\n \tmkdir -p build docs src/feature &&\n \ttouch a.out src/part3.c src/feature/file.c docs/manual.txt obj.o build/lib.so &&\n \t(cd src/ && git clean -d feature/) &&\n-\ttest -f Makefile &&\n-\ttest -f README &&\n-\ttest -f src/part1.c &&\n-\ttest -f src/part2.c &&\n-\ttest -f a.out &&\n-\ttest -f src/part3.c &&\n-\ttest ! -f src/feature/file.c &&\n-\ttest -f docs/manual.txt &&\n-\ttest -f obj.o &&\n-\ttest -f build/lib.so\n+\ttest_path_is_file Makefile &&\n+\ttest_path_is_file README &&\n+\ttest_path_is_file src/part1.c &&\n+\ttest_path_is_file src/part2.c &&\n+\ttest_path_is_file a.out &&\n+\ttest_path_is_file src/part3.c &&\n+\ttest_path_is_missing src/feature/file.c &&\n+\ttest_path_is_file docs/manual.txt &&\n+\ttest_path_is_file obj.o &&\n+\ttest_path_is_file build/lib.so\n \n '\n \n@@ -179,16 +179,16 @@ test_expect_success SYMLINKS 'git clean symbolic link' '\n \ttouch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&\n \tln -s docs/manual.txt src/part4.c &&\n \tgit clean &&\n-\ttest -f Makefile &&\n-\ttest -f README &&\n-\ttest -f src/part1.c &&\n-\ttest -f src/part2.c &&\n-\ttest ! -f a.out &&\n-\ttest ! -f src/part3.c &&\n-\ttest ! -f src/part4.c &&\n-\ttest -f docs/manual.txt &&\n-\ttest -f obj.o &&\n-\ttest -f build/lib.so\n+\ttest_path_is_file Makefile &&\n+\ttest_path_is_file README &&\n+\ttest_path_is_file src/part1.c &&\n+\ttest_path_is_file src/part2.c &&\n+\ttest_path_is_missing a.out &&\n+\ttest_path_is_missing src/part3.c &&\n+\ttest_path_is_missing src/part4.c &&\n+\ttest_path_is_file docs/manual.txt &&\n+\ttest_path_is_file obj.o &&\n+\ttest_path_is_file build/lib.so\n \n '\n \n@@ -196,13 +196,13 @@ test_expect_success 'git clean with wildcard' '\n \n \ttouch a.clean b.clean other.c &&\n \tgit clean \"*.clean\" &&\n-\ttest -f Makefile &&\n-\ttest -f README &&\n-\ttest -f src/part1.c &&\n-\ttest -f src/part2.c &&\n-\ttest ! -f a.clean &&\n-\ttest ! -f b.clean &&\n-\ttest -f other.c\n+\ttest_path_is_file Makefile &&\n+\ttest_path_is_file README &&\n+\ttest_path_is_file src/part1.c &&\n+\ttest_path_is_file src/part2.c &&\n+\ttest_path_is_missing a.clean &&\n+\ttest_path_is_missing b.clean &&\n+\ttest_path_is_file other.c\n \n '\n \n@@ -211,15 +211,15 @@ test_expect_success 'git clean -n' '\n \tmkdir -p build docs &&\n \ttouch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&\n \tgit clean -n &&\n-\ttest -f Makefile &&\n-\ttest -f README &&\n-\ttest -f src/part1.c &&\n-\ttest -f src/part2.c &&\n-\ttest -f a.out &&\n-\ttest -f src/part3.c &&\n-\ttest -f docs/manual.txt &&\n-\ttest -f obj.o &&\n-\ttest -f build/lib.so\n+\ttest_path_is_file Makefile &&\n+\ttest_path_is_file README &&\n+\ttest_path_is_file src/part1.c &&\n+\ttest_path_is_file src/part2.c &&\n+\ttest_path_is_file a.out &&\n+\ttest_path_is_file src/part3.c &&\n+\ttest_path_is_file docs/manual.txt &&\n+\ttest_path_is_file obj.o &&\n+\ttest_path_is_file build/lib.so\n \n '\n \n@@ -228,15 +228,15 @@ test_expect_success 'git clean -d' '\n \tmkdir -p build docs &&\n \ttouch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&\n \tgit clean -d &&\n-\ttest -f Makefile &&\n-\ttest -f README &&\n-\ttest -f src/part1.c &&\n-\ttest -f src/part2.c &&\n-\ttest ! -f a.out &&\n-\ttest ! -f src/part3.c &&\n-\ttest ! -d docs &&\n-\ttest -f obj.o &&\n-\ttest -f build/lib.so\n+\ttest_path_is_file Makefile &&\n+\ttest_path_is_file README &&\n+\ttest_path_is_file src/part1.c &&\n+\ttest_path_is_file src/part2.c &&\n+\ttest_path_is_missing a.out &&\n+\ttest_path_is_missing src/part3.c &&\n+\ttest_path_is_missing docs &&\n+\ttest_path_is_file obj.o &&\n+\ttest_path_is_file build/lib.so\n \n '\n \n@@ -245,16 +245,16 @@ test_expect_success 'git clean -d src/ examples/' '\n \tmkdir -p build docs examples &&\n \ttouch a.out src/part3.c docs/manual.txt obj.o build/lib.so examples/1.c &&\n \tgit clean -d src/ examples/ &&\n-\ttest -f Makefile &&\n-\ttest -f README &&\n-\ttest -f src/part1.c &&\n-\ttest -f src/part2.c &&\n-\ttest -f a.out &&\n-\ttest ! -f src/part3.c &&\n-\ttest ! -f examples/1.c &&\n-\ttest -f docs/manual.txt &&\n-\ttest -f obj.o &&\n-\ttest -f build/lib.so\n+\ttest_path_is_file Makefile &&\n+\ttest_path_is_file README &&\n+\ttest_path_is_file src/part1.c &&\n+\ttest_path_is_file src/part2.c &&\n+\ttest_path_is_file a.out &&\n+\ttest_path_is_missing src/part3.c &&\n+\ttest_path_is_missing examples/1.c &&\n+\ttest_path_is_file docs/manual.txt &&\n+\ttest_path_is_file obj.o &&\n+\ttest_path_is_file build/lib.so\n \n '\n \n@@ -263,15 +263,15 @@ test_expect_success 'git clean -x' '\n \tmkdir -p build docs &&\n \ttouch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&\n \tgit clean -x &&\n-\ttest -f Makefile &&\n-\ttest -f README &&\n-\ttest -f src/part1.c &&\n-\ttest -f src/part2.c &&\n-\ttest ! -f a.out &&\n-\ttest ! -f src/part3.c &&\n-\ttest -f docs/manual.txt &&\n-\ttest ! -f obj.o &&\n-\ttest -f build/lib.so\n+\ttest_path_is_file Makefile &&\n+\ttest_path_is_file README &&\n+\ttest_path_is_file src/part1.c &&\n+\ttest_path_is_file src/part2.c &&\n+\ttest_path_is_missing a.out &&\n+\ttest_path_is_missing src/part3.c &&\n+\ttest_path_is_file docs/manual.txt &&\n+\ttest_path_is_missing obj.o &&\n+\ttest_path_is_file build/lib.so\n \n '\n \n@@ -280,15 +280,15 @@ test_expect_success 'git clean -d -x' '\n \tmkdir -p build docs &&\n \ttouch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&\n \tgit clean -d -x &&\n-\ttest -f Makefile &&\n-\ttest -f README &&\n-\ttest -f src/part1.c &&\n-\ttest -f src/part2.c &&\n-\ttest ! -f a.out &&\n-\ttest ! -f src/part3.c &&\n-\ttest ! -d docs &&\n-\ttest ! -f obj.o &&\n-\ttest ! -d build\n+\ttest_path_is_file Makefile &&\n+\ttest_path_is_file README &&\n+\ttest_path_is_file src/part1.c &&\n+\ttest_path_is_file src/part2.c &&\n+\ttest_path_is_missing a.out &&\n+\ttest_path_is_missing src/part3.c &&\n+\ttest_path_is_missing docs &&\n+\ttest_path_is_missing obj.o &&\n+\ttest_path_is_missing build\n \n '\n \n@@ -297,15 +297,15 @@ test_expect_success 'git clean -d -x with ignored tracked directory' '\n \tmkdir -p build docs &&\n \ttouch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&\n \tgit clean -d -x -e src &&\n-\ttest -f Makefile &&\n-\ttest -f README &&\n-\ttest -f src/part1.c &&\n-\ttest -f src/part2.c &&\n-\ttest ! -f a.out &&\n-\ttest -f src/part3.c &&\n-\ttest ! -d docs &&\n-\ttest ! -f obj.o &&\n-\ttest ! -d build\n+\ttest_path_is_file Makefile &&\n+\ttest_path_is_file README &&\n+\ttest_path_is_file src/part1.c &&\n+\ttest_path_is_file src/part2.c &&\n+\ttest_path_is_missing a.out &&\n+\ttest_path_is_file src/part3.c &&\n+\ttest_path_is_missing docs &&\n+\ttest_path_is_missing obj.o &&\n+\ttest_path_is_missing build\n \n '\n \n@@ -314,15 +314,15 @@ test_expect_success 'git clean -X' '\n \tmkdir -p build docs &&\n \ttouch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&\n \tgit clean -X &&\n-\ttest -f Makefile &&\n-\ttest -f README &&\n-\ttest -f src/part1.c &&\n-\ttest -f src/part2.c &&\n-\ttest -f a.out &&\n-\ttest -f src/part3.c &&\n-\ttest -f docs/manual.txt &&\n-\ttest ! -f obj.o &&\n-\ttest -f build/lib.so\n+\ttest_path_is_file Makefile &&\n+\ttest_path_is_file README &&\n+\ttest_path_is_file src/part1.c &&\n+\ttest_path_is_file src/part2.c &&\n+\ttest_path_is_file a.out &&\n+\ttest_path_is_file src/part3.c &&\n+\ttest_path_is_file docs/manual.txt &&\n+\ttest_path_is_missing obj.o &&\n+\ttest_path_is_file build/lib.so\n \n '\n \n@@ -331,15 +331,15 @@ test_expect_success 'git clean -d -X' '\n \tmkdir -p build docs &&\n \ttouch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&\n \tgit clean -d -X &&\n-\ttest -f Makefile &&\n-\ttest -f README &&\n-\ttest -f src/part1.c &&\n-\ttest -f src/part2.c &&\n-\ttest -f a.out &&\n-\ttest -f src/part3.c &&\n-\ttest -f docs/manual.txt &&\n-\ttest ! -f obj.o &&\n-\ttest ! -d build\n+\ttest_path_is_file Makefile &&\n+\ttest_path_is_file README &&\n+\ttest_path_is_file src/part1.c &&\n+\ttest_path_is_file src/part2.c &&\n+\ttest_path_is_file a.out &&\n+\ttest_path_is_file src/part3.c &&\n+\ttest_path_is_file docs/manual.txt &&\n+\ttest_path_is_missing obj.o &&\n+\ttest_path_is_missing build\n \n '\n \n@@ -348,15 +348,15 @@ test_expect_success 'git clean -d -X with ignored tracked directory' '\n \tmkdir -p build docs &&\n \ttouch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&\n \tgit clean -d -X -e src &&\n-\ttest -f Makefile &&\n-\ttest -f README &&\n-\ttest -f src/part1.c &&\n-\ttest -f src/part2.c &&\n-\ttest -f a.out &&\n-\ttest ! -f src/part3.c &&\n-\ttest -f docs/manual.txt &&\n-\ttest ! -f obj.o &&\n-\ttest ! -d build\n+\ttest_path_is_file Makefile &&\n+\ttest_path_is_file README &&\n+\ttest_path_is_file src/part1.c &&\n+\ttest_path_is_file src/part2.c &&\n+\ttest_path_is_file a.out &&\n+\ttest_path_is_missing src/part3.c &&\n+\ttest_path_is_file docs/manual.txt &&\n+\ttest_path_is_missing obj.o &&\n+\ttest_path_is_missing build\n \n '\n \n@@ -379,29 +379,29 @@ test_expect_success 'clean.requireForce and -n' '\n \tmkdir -p build docs &&\n \ttouch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&\n \tgit clean -n &&\n-\ttest -f Makefile &&\n-\ttest -f README &&\n-\ttest -f src/part1.c &&\n-\ttest -f src/part2.c &&\n-\ttest -f a.out &&\n-\ttest -f src/part3.c &&\n-\ttest -f docs/manual.txt &&\n-\ttest -f obj.o &&\n-\ttest -f build/lib.so\n+\ttest_path_is_file Makefile &&\n+\ttest_path_is_file README &&\n+\ttest_path_is_file src/part1.c &&\n+\ttest_path_is_file src/part2.c &&\n+\ttest_path_is_file a.out &&\n+\ttest_path_is_file src/part3.c &&\n+\ttest_path_is_file docs/manual.txt &&\n+\ttest_path_is_file obj.o &&\n+\ttest_path_is_file build/lib.so\n \n '\n \n test_expect_success 'clean.requireForce and -f' '\n \n \tgit clean -f &&\n-\ttest -f README &&\n-\ttest -f src/part1.c &&\n-\ttest -f src/part2.c &&\n-\ttest ! -f a.out &&\n-\ttest ! -f src/part3.c &&\n-\ttest -f docs/manual.txt &&\n-\ttest -f obj.o &&\n-\ttest -f build/lib.so\n+\ttest_path_is_file README &&\n+\ttest_path_is_file src/part1.c &&\n+\ttest_path_is_file src/part2.c &&\n+\ttest_path_is_missing a.out &&\n+\ttest_path_is_missing src/part3.c &&\n+\ttest_path_is_file docs/manual.txt &&\n+\ttest_path_is_file obj.o &&\n+\ttest_path_is_file build/lib.so\n \n '\n \n@@ -444,11 +444,11 @@ test_expect_success 'nested git work tree' '\n \t\ttest_commit deeply.nested deeper.world\n \t) &&\n \tgit clean -f -d &&\n-\ttest -f foo/.git/index &&\n-\ttest -f foo/hello.world &&\n-\ttest -f baz/boo/.git/index &&\n-\ttest -f baz/boo/deeper.world &&\n-\t! test -d bar\n+\ttest_path_is_file foo/.git/index &&\n+\ttest_path_is_file foo/hello.world &&\n+\ttest_path_is_file baz/boo/.git/index &&\n+\ttest_path_is_file baz/boo/deeper.world &&\n+\ttest_path_is_missing bar\n '\n \n test_expect_success 'should clean things that almost look like git but are not' '\n@@ -609,32 +609,30 @@ test_expect_success 'force removal of nested git work tree' '\n \t\ttest_commit deeply.nested deeper.world\n \t) &&\n \tgit clean -f -f -d &&\n-\t! test -d foo &&\n-\t! test -d bar &&\n-\t! test -d baz\n+\ttest_path_is_missing foo &&\n+\ttest_path_is_missing bar &&\n+\ttest_path_is_missing baz\n '\n \n test_expect_success 'git clean -e' '\n \trm -fr repo &&\n \tmkdir repo &&\n-\t(\n-\t\tcd repo &&\n-\t\tgit init &&\n-\t\ttouch known 1 2 3 &&\n-\t\tgit add known &&\n-\t\tgit clean -f -e 1 -e 2 &&\n-\t\ttest -e 1 &&\n-\t\ttest -e 2 &&\n-\t\t! (test -e 3) &&\n-\t\ttest -e known\n-\t)\n+\tcd repo &&\n+\tgit init &&\n+\ttouch known 1 2 3 &&\n+\tgit add known &&\n+\tgit clean -f -e 1 -e 2 &&\n+\ttest_path_is_file 1 &&\n+\ttest_path_is_file 2 &&\n+\ttest_path_is_missing 3 &&\n+\ttest_path_is_file known\n '\n \n test_expect_success SANITY 'git clean -d with an unreadable empty directory' '\n \tmkdir foo &&\n \tchmod a= foo &&\n \tgit clean -dfx foo &&\n-\t! test -d foo\n+\ttest_path_is_missing foo\n '\n \n test_expect_success 'git clean -d respects pathspecs (dir is prefix of pathspec)' '\n-- \n2.3.6\n"},{"id":"274071","messageId":"1449413906-23256-2-git-send-email-rouzier@gmail.com","threadId":"40896","inReplyTo":"1449413906-23256-1-git-send-email-rouzier@gmail.com","subject":"[PATCH v2 2/2] clean: new option --exclude-from","fromName":"James","fromEmail":"rouzier@gmail.com","sentAt":"2015-12-06T14:58:26Z","receivedAt":"2015-12-06T14:58:26Z","isPatch":true,"sender":{"key":"rouzier@gmail.com","avatar":null},"body":"From: James Rouzier \n\nSpecify a file to read for exclude patterns.\n---\n Documentation/git-clean.txt | 5 +++-\n builtin/clean.c | 15 ++++++++++--\n t/t7300-clean.sh | 60 +++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 77 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git-clean.txt b/Documentation/git-clean.txt\nindex 641681f..ef5dc41 100644\n--- a/Documentation/git-clean.txt\n+++ b/Documentation/git-clean.txt\n@@ -8,7 +8,7 @@ git-clean - Remove untracked files from the working tree\n SYNOPSIS\n --------\n [verse]\n-'git clean' [-d] [-f] [-i] [-n] [-q] [-e ] [-x | -X] [--] ...\n+'git clean' [-d] [-f] [-i] [-n] [-q] [-e ] [--exclude-from=] [-x | -X] [--] ...\n \n DESCRIPTION\n -----------\n@@ -61,6 +61,9 @@ OPTIONS\n \t$GIT_DIR/info/exclude, also consider these patterns to be in the\n \tset of the ignore rules in effect.\n \n+--exclude-from=::\n+\tRead exclude patterns from ; 1 per line.\n+\n -x::\n \tDon't use the standard ignore rules read from .gitignore (per\n \tdirectory) and $GIT_DIR/info/exclude, but do still use the ignore\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex d7acb94..8e652f8 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -22,7 +22,7 @@ static struct string_list del_list = STRING_LIST_INIT_DUP;\n static unsigned int colopts;\n \n static const char *const builtin_clean_usage[] = {\n-\tN_(\"git clean [-d] [-f] [-i] [-n] [-q] [-e ] [-x | -X] [--] ...\"),\n+\tN_(\"git clean [-d] [-f] [-i] [-n] [-q] [-e ] [--exclude-from=] [-x | -X] [--] ...\"),\n \tNULL\n };\n \n@@ -875,6 +875,14 @@ static void interactive_main_loop(void)\n \t}\n }\n \n+static int exclude_from_cb(const struct option *opt,\n+\t\t\t\t const char *arg, int unset)\n+{\n+\tstruct dir_struct *dir = opt->value;\n+\tadd_excludes_from_file(dir, arg);\n+\treturn 0;\n+}\n+\n int cmd_clean(int argc, const char **argv, const char *prefix)\n {\n \tint i, res;\n@@ -898,12 +906,16 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \t\t\t\tN_(\"remove whole directories\")),\n \t\t{ OPTION_CALLBACK, 'e', \"exclude\", &exclude_list, N_(\"pattern\"),\n \t\t N_(\"add to ignore rules\"), PARSE_OPT_NONEG, exclude_cb },\n+\t\t{ OPTION_CALLBACK, 0, \"exclude-from\", &dir, N_(\"file\"),\n+\t\t\tN_(\"read exclude patterns from \"),\n+\t\t\t0, exclude_from_cb },\n \t\tOPT_BOOL('x', NULL, &ignored, N_(\"remove ignored files, too\")),\n \t\tOPT_BOOL('X', NULL, &ignored_only,\n \t\t\t\tN_(\"remove only ignored files\")),\n \t\tOPT_END()\n \t};\n \n+\tmemset(&dir, 0, sizeof(dir));\n \tgit_config(git_clean_config, NULL);\n \tif (force < 0)\n \t\tforce = 0;\n@@ -913,7 +925,6 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, options, builtin_clean_usage,\n \t\t\t 0);\n \n-\tmemset(&dir, 0, sizeof(dir));\n \tif (ignored_only)\n \t\tdir.flags |= DIR_SHOW_IGNORED;\n \ndiff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\nindex d555bb6..c6bfdda 100755\n--- a/t/t7300-clean.sh\n+++ b/t/t7300-clean.sh\n@@ -628,6 +628,66 @@ test_expect_success 'git clean -e' '\n \ttest_path_is_file known\n '\n \n+test_expect_success 'git clean --exclude-from' '\n+\trm -fr repo &&\n+\tmkdir repo &&\n+\tcd repo &&\n+\tgit init &&\n+\ttouch known 1 2 3 &&\n+\tgit add known &&\n+\tcat >.git/clean-exclude <<-\\EOF &&\n+\t1\n+\t2\n+\tEOF\n+\tgit clean -f --exclude-from=.git/clean-exclude &&\n+\ttest_path_is_file 1 &&\n+\ttest_path_is_file 2 &&\n+\ttest_path_is_missing 3 &&\n+\ttest_path_is_file known\n+'\n+\n+test_expect_success 'git clean -e --exclude-from' '\n+\trm -fr repo &&\n+\tmkdir repo &&\n+\tcd repo &&\n+\tgit init &&\n+\ttouch known 1 2 3 &&\n+\tgit add known &&\n+\techo 1 >> .git/clean-exclude &&\n+\tgit clean -f -e 2 --exclude-from=.git/clean-exclude &&\n+\ttest_path_is_file 1 &&\n+\ttest_path_is_file 2 &&\n+\ttest_path_is_missing 3 &&\n+\ttest_path_is_file known\n+'\n+\n+test_expect_success 'git clean --exclude-from --exclude-from' '\n+\trm -fr repo &&\n+\tmkdir repo &&\n+\tgit init &&\n+\ttouch known 1 2 3 &&\n+\tgit add known &&\n+\tcat >.git/clean-exclude1 <<-\\EOF &&\n+\t1\n+\tEOF\n+\tcat >.git/clean-exclude2 <<-\\EOF &&\n+\t2\n+\tEOF\n+\tgit clean -f --exclude-from=.git/clean-exclude1 --exclude-from=.git/clean-exclude2 &&\n+\ttest_path_is_file 1 &&\n+\ttest_path_is_file 2 &&\n+\ttest_path_is_missing 3 &&\n+\ttest_path_is_file known\n+'\n+\n+test_expect_success 'git clean --exclude-from=BADFILE' '\n+\trm -fr repo &&\n+\tmkdir repo &&\n+\tcd repo &&\n+\tgit init &&\n+\ttest_expect_code 128 git clean -f --exclude-from=.git/clean-exclude-not-there\n+'\n+\n test_expect_success SANITY 'git clean -d with an unreadable empty directory' '\n \tmkdir foo &&\n \tchmod a= foo &&\n-- \n2.3.6\n"},{"id":"274127","messageId":"20151207193907.GA29673@sigill.intra.peff.net","threadId":"40896","inReplyTo":"xmqqr3j0nsk8.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] clean: new option --exclude-from","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-07T19:39:07Z","receivedAt":"2015-12-07T19:39:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Dec 05, 2015 at 07:51:03PM -0800, Junio C Hamano wrote:\n\n> In any case, what we've been discussing may be an interesting and\n> potentially important tangent, but it still is a tangent while\n> evaluating the patch in question. I do not think I'd be using the\n> new \"--exclude-from=\" option to \"clean\" (simply because I do\n> not think I've ever used the existing \"-e\" option to it unless I am\n> merely testing the command to make sure it works as advertised)\n> myself, but I do not immediately see how it would hurt us in the\n> future to add it now. So...\n\nI think that is OK. The reason I brought it up was \"let's stop and make\nsure we don't want to go this other route before we add more potentially\nredundant options that we cannot get rid of later\". But based on this\ndiscussion it is not at all clear that \"clean --exclude-from\" would\nbecome redundant, so let's cross that off as an objection.\n\n-Peff\n"},{"id":"274139","messageId":"20151207205619.GE30203@sigill.intra.peff.net","threadId":"40896","inReplyTo":"1449413906-23256-2-git-send-email-rouzier@gmail.com","subject":"Re: [PATCH v2 2/2] clean: new option --exclude-from","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-07T20:56:19Z","receivedAt":"2015-12-07T20:56:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Dec 06, 2015 at 09:58:26AM -0500, James wrote:\n\n> +test_expect_success 'git clean -e --exclude-from' '\n> +\trm -fr repo &&\n> +\tmkdir repo &&\n> +\tcd repo &&\n> +\tgit init &&\n> +\ttouch known 1 2 3 &&\n> +\tgit add known &&\n> +\techo 1 >> .git/clean-exclude &&\n> +\tgit clean -f -e 2 --exclude-from=.git/clean-exclude &&\n> +\ttest_path_is_file 1 &&\n> +\ttest_path_is_file 2 &&\n> +\ttest_path_is_missing 3 &&\n> +\ttest_path_is_file known\n> +'\n\nThis checks that we exclude the union of the \"-e\" and \"--exclude-from\"\nparameters. What happens if one excludes a path and the other negates\nthe exclusion? How is the precedence handled? Is it based on order on\nthe command-line, or something else?\n\nI do not have much of a preference myself, but it probably makes sense\nto document and test it.\n\n> +test_expect_success 'git clean --exclude-from --exclude-from' '\n> +\trm -fr repo &&\n> +\tmkdir repo &&\n> +\tgit init &&\n> +\ttouch known 1 2 3 &&\n> +\tgit add known &&\n> +\tcat >.git/clean-exclude1 <<-\\EOF &&\n> +\t1\n> +\tEOF\n> +\tcat >.git/clean-exclude2 <<-\\EOF &&\n> +\t2\n> +\tEOF\n> +\tgit clean -f --exclude-from=.git/clean-exclude1 --exclude-from=.git/clean-exclude2 &&\n> +\ttest_path_is_file 1 &&\n> +\ttest_path_is_file 2 &&\n> +\ttest_path_is_missing 3 &&\n> +\ttest_path_is_file known\n> +'\n\nDitto here (as the same \"type\", the only precedence that would make any\nsense is command-line order here).\n\n> +test_expect_success 'git clean --exclude-from=BADFILE' '\n> +\trm -fr repo &&\n> +\tmkdir repo &&\n> +\tcd repo &&\n> +\tgit init &&\n> +\ttest_expect_code 128 git clean -f --exclude-from=.git/clean-exclude-not-there\n> +'\n\nDo you actually care about the 128 here, or is just \"it should exit with\nfailure\"? If the latter, we usually use test_must_fail.\n\n-Peff\n"},{"id":"274146","messageId":"xmqq610alyy5.fsf@gitster.mtv.corp.google.com","threadId":"40896","inReplyTo":"1449413906-23256-1-git-send-email-rouzier@gmail.com","subject":"Re: [PATCH v2 1/2] modernize t7300","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-07T21:40:34Z","receivedAt":"2015-12-07T21:40:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"James writes:\n\n> From: James Rouzier \n>\n> ---\n\nMissing sign-off.\n\n> t/t7300-clean.sh | 382 +++++++++++++++++++++++++++----------------------------\n> 1 file changed, 190 insertions(+), 192 deletions(-)\n>\n> diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\n> index 86ceb38..d555bb6 100755\n> --- a/t/t7300-clean.sh\n> +++ b/t/t7300-clean.sh\n> @@ -28,15 +28,15 @@ test_expect_success 'git clean with skip-worktree .gitignore' '\n> \tmkdir -p build docs &&\n> \ttouch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&\n> \tgit clean &&\n> -\ttest -f Makefile &&\n> -\ttest -f README &&\n> -\ttest -f src/part1.c &&\n> -\ttest -f src/part2.c &&\n> -\ttest ! -f a.out &&\n> -\ttest ! -f src/part3.c &&\n> -\ttest -f docs/manual.txt &&\n> -\ttest -f obj.o &&\n> -\ttest -f build/lib.so &&\n> +\ttest_path_is_file Makefile &&\n> +\ttest_path_is_file README &&\n> +\ttest_path_is_file src/part1.c &&\n> +\ttest_path_is_file src/part2.c &&\n\nOK.\n\n> +\ttest_path_is_missing a.out &&\n> +\ttest_path_is_missing src/part3.c &&\n\nThis is better than the original, which may have said \"OK\" upon\nseeing a directory called \"a.out\".\n\n> +\ttest_path_is_file docs/manual.txt &&\n> +\ttest_path_is_file obj.o &&\n> +\ttest_path_is_file build/lib.so &&\n\nOK.\n\n> \tgit update-index --no-skip-worktree .gitignore &&\n> \tgit checkout .gitignore\n> '\n> @@ -46,15 +46,15 @@ test_expect_success 'git clean' '\n> \tmkdir -p build docs &&\n> \ttouch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&\n> \tgit clean &&\n> -\ttest -f Makefile &&\n> -\ttest -f README &&\n> -\ttest -f src/part1.c &&\n> -\ttest -f src/part2.c &&\n> -\ttest ! -f a.out &&\n> -\ttest ! -f src/part3.c &&\n> -\ttest -f docs/manual.txt &&\n> -\ttest -f obj.o &&\n> -\ttest -f build/lib.so\n> +\ttest_path_is_file Makefile &&\n> +\ttest_path_is_file README &&\n> +\ttest_path_is_file src/part1.c &&\n> +\ttest_path_is_file src/part2.c &&\n> +\ttest_path_is_missing a.out &&\n> +\ttest_path_is_missing src/part3.c &&\n> +\ttest_path_is_file docs/manual.txt &&\n> +\ttest_path_is_file obj.o &&\n> +\ttest_path_is_file build/lib.so\n\nThe verbosity of this conversion makes me wonder if we want to have\n\"test_paths_are_files\" and \"test_paths_are_missing\". For that\nmatter, this test does not really care about the distinction between\nfiles and directories (e.g. some tests said \"test ! -d docs\" and\nwould have passed if there were a 'docs' regular file, but what we\nreally care about is the path 'docs' is _gone_), so what we want may\nbe test_paths_exist and test_paths_are_missing. With that, the\nabove hunk would become\n\n\ttest_paths_exist Makefile README src/part1.c src/part2.c \\\n\t\tobj.o build/lib.so &&\n test_paths_are_missing a.out src/part3.c\n\nI dunno.\n\n> test_expect_success 'git clean -e' '\n> \trm -fr repo &&\n> \tmkdir repo &&\n> -\t(\n> -\t\tcd repo &&\n> -\t\tgit init &&\n> -\t\ttouch known 1 2 3 &&\n> -\t\tgit add known &&\n> -\t\tgit clean -f -e 1 -e 2 &&\n> -\t\ttest -e 1 &&\n> -\t\ttest -e 2 &&\n> -\t\t! (test -e 3) &&\n> -\t\ttest -e known\n> -\t)\n> +\tcd repo &&\n> +\tgit init &&\n> +\ttouch known 1 2 3 &&\n> +\tgit add known &&\n> +\tgit clean -f -e 1 -e 2 &&\n> +\ttest_path_is_file 1 &&\n> +\ttest_path_is_file 2 &&\n> +\ttest_path_is_missing 3 &&\n> +\ttest_path_is_file known\n> '\n\nI think this is wrong. The next test piece will be run inside\n\"repo\" directory with this patch applied. Don't lose the subshell\nhere.\n"},{"id":"274148","messageId":"xmqq1taylyr8.fsf@gitster.mtv.corp.google.com","threadId":"40896","inReplyTo":"1449413906-23256-2-git-send-email-rouzier@gmail.com","subject":"Re: [PATCH v2 2/2] clean: new option --exclude-from","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-07T21:44:43Z","receivedAt":"2015-12-07T21:44:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"James writes:\n\n> +static int exclude_from_cb(const struct option *opt,\n> +\t\t\t\t const char *arg, int unset)\n> +{\n> +\tstruct dir_struct *dir = opt->value;\n> +\tadd_excludes_from_file(dir, arg);\n\nI suspect this is wrong. add_excludes_from_file() creates a\nnew excludes_list that is separate from the command line level and\npushes that down to the exclude stack. You'd instead need to add\neach line of the input at the same EXC_CMDL level like -e \noption does from the command line, I would think.\n"},{"id":"274151","messageId":"CAPig+cRqG3mG89Htmjd3a=YzMG36V3an1qUC4siJNBqHzDHQFA@mail.gmail.com","threadId":"40896","inReplyTo":"1449413906-23256-1-git-send-email-rouzier@gmail.com","subject":"Re: [PATCH v2 1/2] modernize t7300","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-12-07T22:43:48Z","receivedAt":"2015-12-07T22:43:48Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Dec 6, 2015 at 9:58 AM, James wrote:\n> From: James Rouzier \n\nThis would be a good place to explain how you are modernizing the test\nscript. Right now, you're just updating to take advantage of\ntest_path_is_foo(), but some other modernizations you could do\ninclude:\n\n* using '>' rather than 'touch' to create empty files when the\ntimestamp doesn't matter (which is all cases in this script)\n\n* (optional) replace unnecessarily complex 'mkdir -p foo' with simpler\n'mkdir foo' (but leave \"mkdir -p foo/bar\" as is)\n\n* drop blank lines before and after test body; for instance:\n\n test_expect_success 'foo' '\n\n blah &&\n bloo\n\n '\n\nbecomes:\n\n test_expect_success 'foo' '\n blah &&\n bloo\n '\n\n> ---\n> diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\n> @@ -609,32 +609,30 @@ test_expect_success 'force removal of nested git work tree' '\n> test_expect_success 'git clean -e' '\n> rm -fr repo &&\n> mkdir repo &&\n> - (\n> - cd repo &&\n> - git init &&\n> - touch known 1 2 3 &&\n> - git add known &&\n> - git clean -f -e 1 -e 2 &&\n> - test -e 1 &&\n> - test -e 2 &&\n> - ! (test -e 3) &&\n> - test -e known\n> - )\n> + cd repo &&\n\nThe previous working directory is not automatically restored at the\nend of the test, so this \"cd\" without corresponding \"cd ..\" causes\nfollowing tests to execute at the incorrect location. Unfortunately,\nyou can't just place \"cd ..\" at the end of a test since it will be\nskipped if something before the \"cd ..\" fails. The way the existing\ncode deals with this is by using a subshell:\n\n mkdir repo &&\n (\n cd repo &&\n ...\n )\n\nThe \"cd repo\" is inside the subshell, so it doesn't affect the parent\nshell, and when the subshell exits, the parent shell remains at the\ncorrect working directory (regardless of whether the test succeeded or\nfailed). Therefore, you don't want to drop the subshell.\n\n> + git init &&\n> + touch known 1 2 3 &&\n> + git add known &&\n> + git clean -f -e 1 -e 2 &&\n> + test_path_is_file 1 &&\n> + test_path_is_file 2 &&\n> + test_path_is_missing 3 &&\n> + test_path_is_file known\n> '\n"},{"id":"274152","messageId":"CAPig+cQOW69-YNaVvqaVYyNt6cT-u92a=i0V6YufnVEmYHy3LA@mail.gmail.com","threadId":"40896","inReplyTo":"xmqq610alyy5.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 1/2] modernize t7300","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-12-07T22:46:00Z","receivedAt":"2015-12-07T22:46:00Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Dec 7, 2015 at 4:40 PM, Junio C Hamano wrote:\n> James writes:\n>> @@ -46,15 +46,15 @@ test_expect_success 'git clean' '\n>> mkdir -p build docs &&\n>> touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&\n>> git clean &&\n>> - test -f Makefile &&\n>> - test -f README &&\n>> - test -f src/part1.c &&\n>> - test -f src/part2.c &&\n>> - test ! -f a.out &&\n>> - test ! -f src/part3.c &&\n>> - test -f docs/manual.txt &&\n>> - test -f obj.o &&\n>> - test -f build/lib.so\n>> + test_path_is_file Makefile &&\n>> + test_path_is_file README &&\n>> + test_path_is_file src/part1.c &&\n>> + test_path_is_file src/part2.c &&\n>> + test_path_is_missing a.out &&\n>> + test_path_is_missing src/part3.c &&\n>> + test_path_is_file docs/manual.txt &&\n>> + test_path_is_file obj.o &&\n>> + test_path_is_file build/lib.so\n>\n> The verbosity of this conversion makes me wonder if we want to have\n> \"test_paths_are_files\" and \"test_paths_are_missing\". For that\n> matter, this test does not really care about the distinction between\n> files and directories (e.g. some tests said \"test ! -d docs\" and\n> would have passed if there were a 'docs' regular file, but what we\n> really care about is the path 'docs' is _gone_), so what we want may\n> be test_paths_exist and test_paths_are_missing. With that, the\n> above hunk would become\n>\n> test_paths_exist Makefile README src/part1.c src/part2.c \\\n> obj.o build/lib.so &&\n> test_paths_are_missing a.out src/part3.c\n>\n> I dunno.\n\nAlternately, update test_path_foo() functions to accept multiple\npathnames, or is that too ugly?\n"},{"id":"274153","messageId":"xmqqr3ixlvpt.fsf@gitster.mtv.corp.google.com","threadId":"40896","inReplyTo":"CAPig+cQOW69-YNaVvqaVYyNt6cT-u92a=i0V6YufnVEmYHy3LA@mail.gmail.com","subject":"Re: [PATCH v2 1/2] modernize t7300","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-07T22:50:22Z","receivedAt":"2015-12-07T22:50:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> Alternately, update test_path_foo() functions to accept multiple\n> pathnames, or is that too ugly?\n\nThat actually would have been my first choice, except (1) that\npath_is_missing has a cruft whose usefulness is dubious, and (2)\nthat \"path_exists A B C\" and \"path_is_missing D E F\" would be\ngramatically incorrect.\n\nI think we should first see if we can remove that \"customized\nmessage\" that appears only in path_is_missing and remove it if\nwe can. Extending \"path_is_missing A B C\" and friends to work\nwould then become trivial.\n\nThanks.\n"},{"id":"274154","messageId":"CAPig+cQb754exY9KTTyohTqoin_uzRspQwrmeoDeJ5bQ=0jzLQ@mail.gmail.com","threadId":"40896","inReplyTo":"1449413906-23256-2-git-send-email-rouzier@gmail.com","subject":"Re: [PATCH v2 2/2] clean: new option --exclude-from","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-12-07T22:53:33Z","receivedAt":"2015-12-07T22:53:33Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"In addition to Peff's and Junio's review comments...\n\nOn Sun, Dec 6, 2015 at 9:58 AM, James wrote:\n> From: James Rouzier \n>\n> Specify a file to read for exclude patterns.\n\nMissing Signed-off-by:.\n\n> ---\n> diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\n> @@ -628,6 +628,66 @@ test_expect_success 'git clean -e' '\n> +test_expect_success 'git clean --exclude-from' '\n> + rm -fr repo &&\n> + mkdir repo &&\n> + cd repo &&\n\nSee my review comments for patch 1/2 as to why you want to wrap 'cd'\nand remaining statements in a subshell.\n\n> + git init &&\n> + touch known 1 2 3 &&\n\nLikewise, use '>' rather than 'touch' to create empty files when the\ntimestamp isn't significant.\n\n >1 &&\n >2 &&\n >3 &&\n\n> + git add known &&\n> + cat >.git/clean-exclude <<-\\EOF &&\n> + 1\n> + 2\n> + EOF\n> + git clean -f --exclude-from=.git/clean-exclude &&\n> + test_path_is_file 1 &&\n> + test_path_is_file 2 &&\n> + test_path_is_missing 3 &&\n> + test_path_is_file known\n> +'\n> +\n> +test_expect_success 'git clean -e --exclude-from' '\n> + rm -fr repo &&\n> + mkdir repo &&\n> + cd repo &&\n> + git init &&\n> + touch known 1 2 3 &&\n> + git add known &&\n> + echo 1 >> .git/clean-exclude &&\n> + git clean -f -e 2 --exclude-from=.git/clean-exclude &&\n> + test_path_is_file 1 &&\n> + test_path_is_file 2 &&\n> + test_path_is_missing 3 &&\n> + test_path_is_file known\n> +'\n> +\n> +test_expect_success 'git clean --exclude-from --exclude-from' '\n> + rm -fr repo &&\n> + mkdir repo &&\n> + git init &&\n> + touch known 1 2 3 &&\n> + git add known &&\n> + cat >.git/clean-exclude1 <<-\\EOF &&\n> + 1\n> + EOF\n> + cat >.git/clean-exclude2 <<-\\EOF &&\n> + 2\n> + EOF\n\nCreation of these single-line files probably would be more readable\nusing 'echo', as you do in the test just above (for\n.git/clean-exclude):\n\n echo 1 >.git/clean-exclude1 &&\n echo 2 >.git/clean-exclude2 &&\n\n> + git clean -f --exclude-from=.git/clean-exclude1 --exclude-from=.git/clean-exclude2 &&\n> + test_path_is_file 1 &&\n> + test_path_is_file 2 &&\n> + test_path_is_missing 3 &&\n> + test_path_is_file known\n> +'\n> +\n> +test_expect_success 'git clean --exclude-from=BADFILE' '\n> + rm -fr repo &&\n> + mkdir repo &&\n> + cd repo &&\n> + git init &&\n> + test_expect_code 128 git clean -f --exclude-from=.git/clean-exclude-not-there\n> +'\n> +\n> test_expect_success SANITY 'git clean -d with an unreadable empty directory' '\n> mkdir foo &&\n> chmod a= foo &&\n> --\n> 2.3.6\n"}]}