{"thread":{"id":"11711","subject":"git-clean buglet","startedAt":"2008-01-23T15:14:52Z","lastAt":"2008-03-07T15:24:34Z","messageCount":47,"participants":["Johannes Sixt","Johannes Schindelin","Shawn Bohrer","Junio C Hamano","しらいしななこ","Robin Rosenberg","Karl Hasselström"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"66402","messageId":"479759EC.4010002@viscovery.net","threadId":"11711","inReplyTo":null,"subject":"git-clean buglet","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-01-23T15:14:52Z","receivedAt":"2008-01-23T15:14:52Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Try this in your favorite git repo:\n\n git clean -n /\n\nHere, it responds with:\n\n fatal: oops in prep_exclude\n\nI don't know what the expected behavior should be, but certainly not this.\nMaybe just do nothing like 'git ls-files /'.\n\nI'm not familiar with either builtin-clean.c nor dir.c; perhaps someone\nelse can fix this?\n\n-- Hannes\n"},{"id":"66403","messageId":"47975C11.7050800@viscovery.net","threadId":"11711","inReplyTo":"479759EC.4010002@viscovery.net","subject":"Re: git-clean buglet","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-01-23T15:24:01Z","receivedAt":"2008-01-23T15:24:01Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Johannes Sixt schrieb:\n> Try this in your favorite git repo:\n> \n> git clean -n /\n> \n> Here, it responds with:\n> \n> fatal: oops in prep_exclude\n> \n> I don't know what the expected behavior should be, but certainly not this.\n> Maybe just do nothing like 'git ls-files /'.\n\nWell, 'git ls-files -o /' would be more similar, but that one lists the\nentire disk contents - not what I would have expected, either. Hmm...\n\n-- Hannes\n"},{"id":"66404","messageId":"alpine.LSU.1.00.0801231528520.5731@racer.site","threadId":"11711","inReplyTo":"479759EC.4010002@viscovery.net","subject":"Re: git-clean buglet","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-01-23T15:29:36Z","receivedAt":"2008-01-23T15:29:36Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 23 Jan 2008, Johannes Sixt wrote:\n\n> Try this in your favorite git repo:\n> \n> git clean -n /\n\nThat's an absolute path. Like almost _all_ git commands, clean only \ntakes relative ones. You probably meant \"git clean -n\".\n\nHth,\nDscho\n"},{"id":"66405","messageId":"47975FE6.4050709@viscovery.net","threadId":"11711","inReplyTo":"alpine.LSU.1.00.0801231528520.5731@racer.site","subject":"Re: git-clean buglet","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-01-23T15:40:22Z","receivedAt":"2008-01-23T15:40:22Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Johannes Schindelin schrieb:\n> Hi,\n> \n> On Wed, 23 Jan 2008, Johannes Sixt wrote:\n> \n>> Try this in your favorite git repo:\n>>\n>> git clean -n /\n> \n> That's an absolute path. Like almost _all_ git commands, clean only \n> takes relative ones. You probably meant \"git clean -n\".\n\nI know it's an absolute path, but, no, I said\n\n git clean -n \\*.vcproj\n\non Windows, which the MinGW port internally transforms into\n\n git clean -n /*.vcproj\n\n(which was not exactly what I meant, but that's a different story).\nAnd this also reports, just like on Linux:\n\n fatal: oops in prep_exclude\n\nThere remain the questions whether we want to do something about absolute\npaths in general or this oops in particular.\n\n-- Hannes\n"},{"id":"66685","messageId":"1201463731-1963-1-git-send-email-shawn.bohrer@gmail.com","threadId":"11711","inReplyTo":"47975FE6.4050709@viscovery.net","subject":"[PATCH] Fix off by one error in prep_exclude.","fromName":"Shawn Bohrer","fromEmail":"shawn.bohrer@gmail.com","sentAt":"2008-01-27T19:55:31Z","receivedAt":"2008-01-27T19:55:31Z","isPatch":true,"sender":{"key":"shawn.bohrer@gmail.com","avatar":"https://gravatar.com/avatar/6eb093ef7d276306d18366254e0c95ff6a5db58231ac7e82fe78c2800aaae1b6?d=mp&s=160"},"body":"base + current already includes the trailing slash so adding\none removes the first character of the next directory.\n\nSigned-off-by: Shawn Bohrer \n---\n\nThis fixes the oops part of the issue Johannes found, but doesn't\naddress the fact that we probably should remove files that aren't\na part of the repository at in the first place.\n\n dir.c | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 3e345c2..9e5879a 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -237,7 +237,7 @@ static void prep_exclude(struct dir_struct *dir, const char *base, int baselen)\n \t\t\tcurrent = 0;\n \t\t}\n \t\telse {\n-\t\t\tcp = strchr(base + current + 1, '/');\n+\t\t\tcp = strchr(base + current, '/');\n \t\t\tif (!cp)\n \t\t\t\tdie(\"oops in prep_exclude\");\n \t\t\tcp++;\n-- \n1.5.4-rc2.GIT\n"},{"id":"66687","messageId":"alpine.LSU.1.00.0801272043040.23907@racer.site","threadId":"11711","inReplyTo":"1201463731-1963-1-git-send-email-shawn.bohrer@gmail.com","subject":"Re: [PATCH] Fix off by one error in prep_exclude.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-01-27T20:44:57Z","receivedAt":"2008-01-27T20:44:57Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 27 Jan 2008, Shawn Bohrer wrote:\n\n> base + current already includes the trailing slash so adding\n> one removes the first character of the next directory.\n> \n> Signed-off-by: Shawn Bohrer \n> ---\n> \n> This fixes the oops part of the issue Johannes found,\n\nhave I?\n\n> but doesn't address the fact that we probably should remove files that \n> aren't a part of the repository at in the first place.\n\nI am sorry, but I cannot begin to see what this commit tries to \naccomplish. Yes, sure, there is an off-by-one error, and your commit \nmessage says how that was fixed. But I miss a description what usage it \nwould affect, i.e. when this bug triggers.\n\nI imagine that you would be as lost as me, reading that commit message 6 \nmonths from now, trying to understand why that change was made.\n\nCiao,\nDscho\n"},{"id":"66690","messageId":"20080127211539.GA10993@lintop","threadId":"11711","inReplyTo":"alpine.LSU.1.00.0801272043040.23907@racer.site","subject":"Re: [PATCH] Fix off by one error in prep_exclude.","fromName":"Shawn Bohrer","fromEmail":"shawn.bohrer@gmail.com","sentAt":"2008-01-27T21:15:39Z","receivedAt":"2008-01-27T21:15:39Z","isPatch":true,"sender":{"key":"shawn.bohrer@gmail.com","avatar":"https://gravatar.com/avatar/6eb093ef7d276306d18366254e0c95ff6a5db58231ac7e82fe78c2800aaae1b6?d=mp&s=160"},"body":"On Sun, Jan 27, 2008 at 08:44:57PM +0000, Johannes Schindelin wrote:\n> Hi,\n> \n> On Sun, 27 Jan 2008, Shawn Bohrer wrote:\n> \n> > base + current already includes the trailing slash so adding\n> > one removes the first character of the next directory.\n> > \n> > Signed-off-by: Shawn Bohrer \n> > ---\n> > \n> > This fixes the oops part of the issue Johannes found,\n> \n> have I?\n \nSorry I should have been more explicit Johannes Sixt reported the issue,\nyou were included simply because you had been involved in the thread.\n\n> > but doesn't address the fact that we probably should remove files that \n> > aren't a part of the repository at in the first place.\n> \n> I am sorry, but I cannot begin to see what this commit tries to \n> accomplish. Yes, sure, there is an off-by-one error, and your commit \n> message says how that was fixed. But I miss a description what usage it \n> would affect, i.e. when this bug triggers.\n\nAs far as I can see there are two protential cases that could trigger\nthis bug, but there may be more. This first was the arguably invalid\ncase the Johannes Sixt reported.\n\ngit clean -n /\n\nThe other case that could trigger this bug and potentially others is if\nsomeone makes their root dircetory a git repository and then uses \"/\" as\nan absolute path. For example:\n\ncd /\ngit init\ngit clean -n /\n\nYou may argue that both of these cases are invalid and that is fine by\nme, but since I noticed this bug I thought I would send a patch. If you\nwouldlike I can add these two use cases to the commit message.\n\n--\nShawn\n"},{"id":"66693","messageId":"7v3asiyk2i.fsf@gitster.siamese.dyndns.org","threadId":"11711","inReplyTo":"alpine.LSU.1.00.0801272043040.23907@racer.site","subject":"Re: [PATCH] Fix off by one error in prep_exclude.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-27T22:34:13Z","receivedAt":"2008-01-27T22:34:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> but doesn't address the fact that we probably should remove files that \n>> aren't a part of the repository at in the first place.\n>\n> I am sorry, but I cannot begin to see what this commit tries to \n> accomplish. Yes, sure, there is an off-by-one error, and your commit \n> message says how that was fixed. But I miss a description what usage it \n> would affect, i.e. when this bug triggers.\n>\n> I imagine that you would be as lost as me, reading that commit message 6 \n> months from now, trying to understand why that change was made.\n\nLikewise. The message has somewhat to be desired...\n\nIn \"struct exclude_stack\", prep_exclude() and excluded(), the\nconvention for a path is to express the length of directory part\nincluding the trailing slash (e.g. \"foo\" and \"bar/baz\" will get\nbaselen=0 and baselen=4 respectively).\n\nThe variable current and parameter baselen follow that\nconvention in the codepath the patch touches.\n\n\t\telse {\n\t\t\tcp = strchr(base + current + 1, '/');\n\t\t\tif (!cp)\n\t\t\t\tdie(\"oops in prep_exclude\");\n\t\t\tcp++;\n\t\t}\n\t\tstk->prev = dir->exclude_stack;\n\t\tstk->baselen = cp - base;\n\nis about coming up with the next value for current (which is\ntaken from stk->baselen) to dig one more level.\n\nIf base=\"foo/a/boo\" and current=4 (i.e. we are looking at\n\"foo/\"), at the point, scanning from (base+current) as Shawn\nBohrer's patch suggests means the scan begins at \"a/boo\" to find\nthe next slash. The existing code skips one letter ('a') and\nstarts scanning from \"/boo\".\n\nThe only case this microoptimization makes difference is when an\ninput is malformed and has double-slash (i.e. path component\nwhose length is zero), like \"foo//boo\".\n\nPerhaps the \"oops part of the issue Johannes found\" had a caller\nthat feeds such an incorrect input?\n"},{"id":"66698","messageId":"20080128003404.GA18276@lintop","threadId":"11711","inReplyTo":"7v3asiyk2i.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Fix off by one error in prep_exclude.","fromName":"Shawn Bohrer","fromEmail":"shawn.bohrer@gmail.com","sentAt":"2008-01-28T00:34:04Z","receivedAt":"2008-01-28T00:34:04Z","isPatch":true,"sender":{"key":"shawn.bohrer@gmail.com","avatar":"https://gravatar.com/avatar/6eb093ef7d276306d18366254e0c95ff6a5db58231ac7e82fe78c2800aaae1b6?d=mp&s=160"},"body":"On Sun, Jan 27, 2008 at 02:34:13PM -0800, Junio C Hamano wrote:\n> Johannes Schindelin writes:\n> \n> >> but doesn't address the fact that we probably should remove files that \n> >> aren't a part of the repository at in the first place.\n> >\n> > I am sorry, but I cannot begin to see what this commit tries to \n> > accomplish. Yes, sure, there is an off-by-one error, and your commit \n> > message says how that was fixed. But I miss a description what usage it \n> > would affect, i.e. when this bug triggers.\n> >\n> > I imagine that you would be as lost as me, reading that commit message 6 \n> > months from now, trying to understand why that change was made.\n> \n> Likewise. The message has somewhat to be desired...\n\nAgreed I'll resend with a improved message.\n \n> In \"struct exclude_stack\", prep_exclude() and excluded(), the\n> convention for a path is to express the length of directory part\n> including the trailing slash (e.g. \"foo\" and \"bar/baz\" will get\n> baselen=0 and baselen=4 respectively).\n> \n> The variable current and parameter baselen follow that\n> convention in the codepath the patch touches.\n> \n> \t\telse {\n> \t\t\tcp = strchr(base + current + 1, '/');\n> \t\t\tif (!cp)\n> \t\t\t\tdie(\"oops in prep_exclude\");\n> \t\t\tcp++;\n> \t\t}\n> \t\tstk->prev = dir->exclude_stack;\n> \t\tstk->baselen = cp - base;\n> \n> is about coming up with the next value for current (which is\n> taken from stk->baselen) to dig one more level.\n> \n> If base=\"foo/a/boo\" and current=4 (i.e. we are looking at\n> \"foo/\"), at the point, scanning from (base+current) as Shawn\n> Bohrer's patch suggests means the scan begins at \"a/boo\" to find\n> the next slash. The existing code skips one letter ('a') and\n> starts scanning from \"/boo\".\n> \n> The only case this microoptimization makes difference is when an\n> input is malformed and has double-slash (i.e. path component\n> whose length is zero), like \"foo//boo\".\n\nGood catch, I didn't think of this case but this indeed will cause\nthe same issue.\n \n> Perhaps the \"oops part of the issue Johannes found\" had a caller\n> that feeds such an incorrect input?\n\nNope the problem Johannes Sixt was having was that he mistakenly ran\n\ngit clean -n /*foo\n\nNow that isn't what he meant to do, but I figured it might be possible\nthat someone has their whole filesystem in a git repository, or maybe\nis using some sort of chroot on their repository. Your malformed\npaths guess is probably much more likely to occur.\n\n--\nShawn\n"},{"id":"66699","messageId":"1201480650-19716-1-git-send-email-shawn.bohrer@gmail.com","threadId":"11711","inReplyTo":"20080128003404.GA18276@lintop","subject":"[PATCH] Fix off by one error in prep_exclude.","fromName":"Shawn Bohrer","fromEmail":"shawn.bohrer@gmail.com","sentAt":"2008-01-28T00:37:30Z","receivedAt":"2008-01-28T00:37:30Z","isPatch":true,"sender":{"key":"shawn.bohrer@gmail.com","avatar":"https://gravatar.com/avatar/6eb093ef7d276306d18366254e0c95ff6a5db58231ac7e82fe78c2800aaae1b6?d=mp&s=160"},"body":"In \"struct exclude_stack\", prep_exclude() and excluded(), the\nconvention for a path is to express the length of directory part\nincluding the trailing slash (e.g. \"foo\" and \"bar/baz\" will get\nbaselen=0 and baselen=4 respectively).\n\nThe variable current and parameter baselen follow that\nconvention in the codepath the following patch touches.\n\n else {\n cp = strchr(base + current + 1, '/');\n if (!cp)\n die(\"oops in prep_exclude\");\n cp++;\n }\n stk->prev = dir->exclude_stack;\n stk->baselen = cp - base;\n\nis about coming up with the next value for current (which is\ntaken from stk->baselen) to dig one more level.\n\nIf base=\"foo/bar/boo\" and current=4 (i.e. we are looking at\n\"foo/\"), the current code (base + current + 1) will begin scanning\nfor the next slash at ar/boo skipping one letter ('b'). This\npatch starts the scanning at bar/boo/\n\nThis only causes a problem when a path component has a length of\nzero which can happen when the user provides an absolute path to\na file or directory in the root directory (i.e. \"/\", or \"/foo\"),\nor if the input is malformed and contains a double-slash such\nas \"foo//boo\".\n\nSigned-off-by: Shawn Bohrer \n---\n dir.c | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 3e345c2..9e5879a 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -237,7 +237,7 @@ static void prep_exclude(struct dir_struct *dir, const char *base, int baselen)\n \t\t\tcurrent = 0;\n \t\t}\n \t\telse {\n-\t\t\tcp = strchr(base + current + 1, '/');\n+\t\t\tcp = strchr(base + current, '/');\n \t\t\tif (!cp)\n \t\t\t\tdie(\"oops in prep_exclude\");\n \t\t\tcp++;\n-- \n1.5.4.rc5.1.g813e\n"},{"id":"66704","messageId":"7vodb6wtix.fsf@gitster.siamese.dyndns.org","threadId":"11711","inReplyTo":"20080128003404.GA18276@lintop","subject":"Re: [PATCH] Fix off by one error in prep_exclude.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-28T02:52:54Z","receivedAt":"2008-01-28T02:52:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shawn Bohrer writes:\n\n> Nope the problem Johannes Sixt was having was that he mistakenly ran\n>\n> git clean -n /*foo\n>\n> Now that isn't what he meant to do, but I figured it might be possible\n> that someone has their whole filesystem in a git repository, or maybe\n> is using some sort of chroot on their repository. Your malformed\n> paths guess is probably much more likely to occur.\n\nThat is not a user error from the syntax point of view (although\nit might be from the semantics point of view). I think the\ncaller of the excluded() function (that is probably somewhere in\nbuiltin-clean.c -- I did not check) is responsible for not\nsupplying such a path to the called function.\n"},{"id":"66712","messageId":"479D805E.3000209@viscovery.net","threadId":"11711","inReplyTo":"7vodb6wtix.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Fix off by one error in prep_exclude.","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-01-28T07:12:30Z","receivedAt":"2008-01-28T07:12:30Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Junio C Hamano schrieb:\n> Shawn Bohrer writes:\n> \n>> Nope the problem Johannes Sixt was having was that he mistakenly ran\n>>\n>> git clean -n /*foo\n>>\n>> Now that isn't what he meant to do, but I figured it might be possible\n>> that someone has their whole filesystem in a git repository, or maybe\n>> is using some sort of chroot on their repository. Your malformed\n>> paths guess is probably much more likely to occur.\n> \n> That is not a user error from the syntax point of view (although\n> it might be from the semantics point of view). I think the\n> caller of the excluded() function (that is probably somewhere in\n> builtin-clean.c -- I did not check) is responsible for not\n> supplying such a path to the called function.\n\nThe \"problem\" is not only with git-clean, but also in others, like\ngit-ls-files. Try this in you favorite repository:\n\n $ git ls-files -o /*bin\n\nThe output does not make a lot of sense. (Here it lists the contents of\n/bin and /sbin.) Not that it hurts with ls-files, but\n\n $ git clean -f /\n\nis basically a synonym for\n\n $ rm -rf /\n\n-- Hannes\n"},{"id":"66721","messageId":"7vprvmuykw.fsf@gitster.siamese.dyndns.org","threadId":"11711","inReplyTo":"479D805E.3000209@viscovery.net","subject":"Re: [PATCH] Fix off by one error in prep_exclude.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-28T08:46:39Z","receivedAt":"2008-01-28T08:46:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> The \"problem\" is not only with git-clean, but also in others, like\n> git-ls-files. Try this in you favorite repository:\n>\n> $ git ls-files -o /*bin\n>\n> The output does not make a lot of sense. (Here it lists the contents of\n> /bin and /sbin.) Not that it hurts with ls-files, but\n>\n> $ git clean -f /\n>\n> is basically a synonym for\n>\n> $ rm -rf /\n\nYeah, /*bin is not inside the repository so it should not even\nbe reported as \"others\". Shouldn't the commands detect this and\nreject feeding such paths outside the work tree to the core,\nwhich always expect you to talk about paths inside?\n"},{"id":"66724","messageId":"479D9ADE.6010003@viscovery.net","threadId":"11711","inReplyTo":"7vprvmuykw.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Fix off by one error in prep_exclude.","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-01-28T09:05:34Z","receivedAt":"2008-01-28T09:05:34Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Junio C Hamano schrieb:\n> Johannes Sixt writes:\n> \n>> The \"problem\" is not only with git-clean, but also in others, like\n>> git-ls-files. Try this in you favorite repository:\n>>\n>> $ git ls-files -o /*bin\n>>\n>> The output does not make a lot of sense. (Here it lists the contents of\n>> /bin and /sbin.) Not that it hurts with ls-files, but\n>>\n>> $ git clean -f /\n>>\n>> is basically a synonym for\n>>\n>> $ rm -rf /\n> \n> Yeah, /*bin is not inside the repository so it should not even\n> be reported as \"others\". Shouldn't the commands detect this and\n> reject feeding such paths outside the work tree to the core,\n> which always expect you to talk about paths inside?\n\nThat's what I had expected. But look:\n\n $ git ls-files -o /\n [... tons of file names ...]\n\n $ git ls-files -o ..\n fatal: '..' is outside repository\n\n $ git clean -n / # with Shawn's patch\n Would remove /bin/\n [... etc ...]\n\n $ git clean -n ..\n fatal: '..' is outside repository\n\nSome mechanism for this is already there; it's just not complete enough.\n\n-- Hannes\n"},{"id":"66725","messageId":"7vlk6auwxs.fsf@gitster.siamese.dyndns.org","threadId":"11711","inReplyTo":"479D9ADE.6010003@viscovery.net","subject":"Re: [PATCH] Fix off by one error in prep_exclude.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-28T09:22:07Z","receivedAt":"2008-01-28T09:22:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> Junio C Hamano schrieb:\n> ...\n>> Yeah, /*bin is not inside the repository so it should not even\n>> be reported as \"others\". Shouldn't the commands detect this and\n>> reject feeding such paths outside the work tree to the core,\n>> which always expect you to talk about paths inside?\n>\n> That's what I had expected. But look:\n> ...\n> Some mechanism for this is already there; it's just not complete enough.\n\nExactly. That was what I've been getting at from the beginning\nof this thread --- fix for that incompleteness is what's needed.\n"},{"id":"66738","messageId":"alpine.LSU.1.00.0801281159150.23907@racer.site","threadId":"11711","inReplyTo":"1201480650-19716-1-git-send-email-shawn.bohrer@gmail.com","subject":"Re: [PATCH] Fix off by one error in prep_exclude.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-01-28T11:59:50Z","receivedAt":"2008-01-28T11:59:50Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 27 Jan 2008, Shawn Bohrer wrote:\n\n> In \"struct exclude_stack\", prep_exclude() and excluded(), the\n> convention for a path is to express the length of directory part\n> including the trailing slash (e.g. \"foo\" and \"bar/baz\" will get\n> baselen=0 and baselen=4 respectively).\n> \n> The variable current and parameter baselen follow that\n> convention in the codepath the following patch touches.\n> \n> else {\n> cp = strchr(base + current + 1, '/');\n> if (!cp)\n> die(\"oops in prep_exclude\");\n> cp++;\n> }\n> stk->prev = dir->exclude_stack;\n> stk->baselen = cp - base;\n> \n> is about coming up with the next value for current (which is\n> taken from stk->baselen) to dig one more level.\n> \n> If base=\"foo/bar/boo\" and current=4 (i.e. we are looking at\n> \"foo/\"), the current code (base + current + 1) will begin scanning\n> for the next slash at ar/boo skipping one letter ('b'). This\n> patch starts the scanning at bar/boo/\n> \n> This only causes a problem when a path component has a length of\n> zero which can happen when the user provides an absolute path to\n> a file or directory in the root directory (i.e. \"/\", or \"/foo\"),\n> or if the input is malformed and contains a double-slash such\n> as \"foo//boo\".\n> \n> Signed-off-by: Shawn Bohrer \n\nI'll try to remember even 6 months from now that this was the \"git clean \n-n /\" problem ;-)\n\nCiao,\nDscho\n"},{"id":"66739","messageId":"7vd4rmtavb.fsf@gitster.siamese.dyndns.org","threadId":"11711","inReplyTo":"alpine.LSU.1.00.0801281159150.23907@racer.site","subject":"Re: [PATCH] Fix off by one error in prep_exclude.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-28T12:04:08Z","receivedAt":"2008-01-28T12:04:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> ...\n>> This only causes a problem when a path component has a length of\n>> zero which can happen when the user provides an absolute path to\n>> a file or directory in the root directory (i.e. \"/\", or \"/foo\"),\n>> or if the input is malformed and contains a double-slash such\n>> as \"foo//boo\".\n>> \n>> Signed-off-by: Shawn Bohrer \n>\n> I'll try to remember even 6 months from now that this was the \"git clean \n> -n /\" problem ;-)\n\nActually the quoted part of the message clearly tells that the\npatch is touching the wrong code. It should not blame the user\nbut the caller of the function that did not check such an input,\nwhich is where the fix should be in.\n"},{"id":"66741","messageId":"alpine.LSU.1.00.0801281210440.23907@racer.site","threadId":"11711","inReplyTo":"479D9ADE.6010003@viscovery.net","subject":"[RFH/PATCH] prefix_path(): disallow absolute paths","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-01-28T12:33:48Z","receivedAt":"2008-01-28T12:33:48Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"\nWithout this fix, \"git ls-files --others /\" would list _all_ files,\nexcept for those tracked in the current repository. Worse, \"git clean /\"\nwould start removing them.\n\nNoticed by Johannes Sixt.\n\nIncidentally, it fixes some strange code in builtin-mv.c by yours truly,\nwhere a slash was added to \"dst\" but then ignored, and instead taken from\nthe source path. This triggered the new check for absolute paths.\n\nSigned-off-by: Johannes Schindelin \n---\n\tOn Mon, 28 Jan 2008, Johannes Sixt wrote:\n\n\t> Junio C Hamano schrieb:\n\t> > Johannes Sixt writes:\n\t> > \n\t> >> The \"problem\" is not only with git-clean, but also in others, \n\t> >> like git-ls-files. Try this in you favorite repository:\n\t> >>\n\t> >> $ git ls-files -o /*bin\n\t> >>\n\t> >> The output does not make a lot of sense. (Here it lists the \n\t> >> contents of /bin and /sbin.) Not that it hurts with ls-files, \n\t> >> but\n\t> >>\n\t> >> $ git clean -f /\n\t> >>\n\t> >> is basically a synonym for\n\t> >>\n\t> >> $ rm -rf /\n\t> > \n\t> > Yeah, /*bin is not inside the repository so it should not even \n\t> > be reported as \"others\". Shouldn't the commands detect this \n\t> > and reject feeding such paths outside the work tree to the \n\t> > core, which always expect you to talk about paths inside?\n\t> \n\t> That's what I had expected. But look:\n\t> \n\t> $ git ls-files -o /\n\t> [... tons of file names ...]\n\t> \n\t> $ git ls-files -o ..\n\t> fatal: '..' is outside repository\n\t> \n\t> $ git clean -n / # with Shawn's patch\n\t> Would remove /bin/\n\t> [... etc ...]\n\t> \n\t> $ git clean -n ..\n\t> fatal: '..' is outside repository\n\t> \n\t> Some mechanism for this is already there; it's just not complete \n\t> enough.\n\n\tThis patch cannot be applied as-is: t3101 is failing (t7001 is \n\tfixed by the builtin-mv.c part).\n\n\tThe failure of t3101 has something to do with ls-tree filtering \n\tout invalid paths; I maintain that this behaviour is wrong to \n\tbegin with.\n\n\tSo the help I am requesting is this: so late in the game for 1.5.4 \n\tI would hate to introduce a change in prefix_path(), because it \n\taffects apparently too much. However, the \"git clean /\" bug is a \n\treal one, and should at least be prevented. What to do?\n\n builtin-mv.c | 4 ++--\n setup.c | 2 ++\n 2 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin-mv.c b/builtin-mv.c\nindex 990e213..94f6dd2 100644\n--- a/builtin-mv.c\n+++ b/builtin-mv.c\n@@ -164,7 +164,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \t\t\t\t}\n \n \t\t\t\tdst = add_slash(dst);\n-\t\t\t\tdst_len = strlen(dst) - 1;\n+\t\t\t\tdst_len = strlen(dst);\n \n \t\t\t\tfor (j = 0; j < last - first; j++) {\n \t\t\t\t\tconst char *path =\n@@ -172,7 +172,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \t\t\t\t\tsource[argc + j] = path;\n \t\t\t\t\tdestination[argc + j] =\n \t\t\t\t\t\tprefix_path(dst, dst_len,\n-\t\t\t\t\t\t\tpath + length);\n+\t\t\t\t\t\t\tpath + length + 1);\n \t\t\t\t\tmodes[argc + j] = INDEX;\n \t\t\t\t}\n \t\t\t\targc += last - first;\ndiff --git a/setup.c b/setup.c\nindex 2174e78..5a4aadc 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -13,6 +13,8 @@ const char *get_current_prefix()\n const char *prefix_path(const char *prefix, int len, const char *path)\n {\n \tconst char *orig = path;\n+\tif (is_absolute_path(path))\n+\t\tdie(\"no absolute paths allowed: '%s'\", path);\n \tfor (;;) {\n \t\tchar c;\n \t\tif (*path != '.')\n-- \n1.5.4.rc5.15.g8231f\n"},{"id":"66750","messageId":"alpine.LSU.1.00.0801281503350.23907@racer.site","threadId":"11711","inReplyTo":"alpine.LSU.1.00.0801281210440.23907@racer.site","subject":"[PATCH] prefix_path(): disallow absolute paths","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-01-28T15:05:42Z","receivedAt":"2008-01-28T15:05:42Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"\nWithout this fix, \"git ls-files --others /\" would list _all_ files,\nexcept for those tracked in the current repository. Worse, \"git clean /\"\nwould start removing them.\n\nNoticed by Johannes Sixt.\n\nIncidentally, it fixes some strange code in builtin-mv.c by yours truly,\nwhere a slash was added to \"dst\" but then ignored, and instead taken from\nthe source path. This triggered the new check for absolute paths.\n\nA test in t3101 started failing, too, because it tested ls-tree with\nnot-really-absolute paths (expecting the leading \"/\" to be ignored).\nThose paths were changed to relative paths.\n\nSigned-off-by: Johannes Schindelin \n---\n\n\tOn Mon, 28 Jan 2008, Johannes Schindelin wrote:\n\n\t> The failure of t3101 has something to do with ls-tree filtering \n\t> out invalid paths; I maintain that this behaviour is wrong to \n\t> begin with.\n\n\tThis patch fixes the test.\n\n\tBut as this fix illustrates, it is a change in semantics: where \n\tearlier\n\n\t\tgit ls-tree /README\n\n\twas allowed, it is no longer.\n\n\tComments?\n\n builtin-mv.c | 4 ++--\n setup.c | 2 ++\n t/t3101-ls-tree-dirname.sh | 2 +-\n 3 files changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin-mv.c b/builtin-mv.c\nindex 990e213..94f6dd2 100644\n--- a/builtin-mv.c\n+++ b/builtin-mv.c\n@@ -164,7 +164,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \t\t\t\t}\n \n \t\t\t\tdst = add_slash(dst);\n-\t\t\t\tdst_len = strlen(dst) - 1;\n+\t\t\t\tdst_len = strlen(dst);\n \n \t\t\t\tfor (j = 0; j < last - first; j++) {\n \t\t\t\t\tconst char *path =\n@@ -172,7 +172,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \t\t\t\t\tsource[argc + j] = path;\n \t\t\t\t\tdestination[argc + j] =\n \t\t\t\t\t\tprefix_path(dst, dst_len,\n-\t\t\t\t\t\t\tpath + length);\n+\t\t\t\t\t\t\tpath + length + 1);\n \t\t\t\t\tmodes[argc + j] = INDEX;\n \t\t\t\t}\n \t\t\t\targc += last - first;\ndiff --git a/setup.c b/setup.c\nindex 2174e78..5a4aadc 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -13,6 +13,8 @@ const char *get_current_prefix()\n const char *prefix_path(const char *prefix, int len, const char *path)\n {\n \tconst char *orig = path;\n+\tif (is_absolute_path(path))\n+\t\tdie(\"no absolute paths allowed: '%s'\", path);\n \tfor (;;) {\n \t\tchar c;\n \t\tif (*path != '.')\ndiff --git a/t/t3101-ls-tree-dirname.sh b/t/t3101-ls-tree-dirname.sh\nindex 39fe267..cc5b982 100755\n--- a/t/t3101-ls-tree-dirname.sh\n+++ b/t/t3101-ls-tree-dirname.sh\n@@ -120,7 +120,7 @@ EOF\n # having 1.txt and path3\n test_expect_success \\\n 'ls-tree filter odd names' \\\n- 'git ls-tree $tree 1.txt /1.txt //1.txt path3/1.txt /path3/1.txt //path3//1.txt path3 /path3/ path3// >current &&\n+ 'git ls-tree $tree 1.txt ./1.txt .//1.txt path3/1.txt ./path3/1.txt .//path3//1.txt path3 ./path3/ path3// >current &&\n cat >expected <<\\EOF &&\n 100644 blob X\t1.txt\n 100644 blob X\tpath3/1.txt\n-- \n1.5.4.rc5.15.g8231f\n"},{"id":"66768","messageId":"7vwspts9vj.fsf@gitster.siamese.dyndns.org","threadId":"11711","inReplyTo":"alpine.LSU.1.00.0801281210440.23907@racer.site","subject":"Re: [RFH/PATCH] prefix_path(): disallow absolute paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-29T01:23:12Z","receivedAt":"2008-01-29T01:23:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Without this fix, \"git ls-files --others /\" would list _all_ files,\n> except for those tracked in the current repository. Worse, \"git clean /\"\n> would start removing them.\n> ...\n> \tThis patch cannot be applied as-is: t3101 is failing (t7001 is \n> \tfixed by the builtin-mv.c part).\n>\n> \tThe failure of t3101 has something to do with ls-tree filtering \n> \tout invalid paths; I maintain that this behaviour is wrong to \n> \tbegin with.\n>\n> \tSo the help I am requesting is this: so late in the game for 1.5.4 \n> \tI would hate to introduce a change in prefix_path(), because it \n> \taffects apparently too much. However, the \"git clean /\" bug is a \n> \treal one, and should at least be prevented. What to do?\n> ...\n> diff --git a/setup.c b/setup.c\n> index 2174e78..5a4aadc 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -13,6 +13,8 @@ const char *get_current_prefix()\n> const char *prefix_path(const char *prefix, int len, const char *path)\n> {\n> \tconst char *orig = path;\n> +\tif (is_absolute_path(path))\n> +\t\tdie(\"no absolute paths allowed: '%s'\", path);\n> \tfor (;;) {\n> \t\tchar c;\n> \t\tif (*path != '.')\n\nIf we are touching the prefix_path(), I think we should try to\nmake its \"ambiguous path rejection\" more complete.\n\nCurrently, we:\n\n - Remove \".\" path component (i.e. the directory leading part\n specified) from the input;\n\n - Remove \"..\" path component and strip one level of the prefix;\n\nonly from the beginning. So if you give nonsense pathspec from\nthe command line, you can end up calling prefix_path() with things\nlike \"/README\", \"/absolute/path/to//repository/tracked/file\", and\n\"fo//o/../o\".\n\nAnd not passing such ambiguous path like \"fo//o\" to the core\nlevel but sanitizing matters. Then core level can always do\nmemcmp() with \"fo/o\" to see they are talking about the same\npath.\n\nI suspect that the right approach might be something like the\nattached patch. It introduces a version of prefix_path() that\nsanitizes path (but not prefix part, which comes from git itself\nand hopefully there should not be a need to sanitize it) while\ndoing the prefixing. It also strips the leading absolute path\nto the repository by comparing it with the value of work_tree.\n\nA few things to note.\n\n * Your mv fix is rolled in.\n\n * This allows you to name a in-repository file as `pwd`/file,\n or `pwd`//file (iow, double-slash is also sanitized). It may\n kill the bird in another thread nearby.\n\n * get_pathspec() drops paths outside of repository, so the\n caller may end up getting a smaller number of paths than it\n originally gave it. If an existing caller expects the same\n number of paths to come back, it needs to be adjusted (I did\n not check). We could alternatively die() but I couldn't\n decide which one is a better behaviour.\n\nThis is not to be applied (especially before auditing the\ncallers), but to be thought about. Although it passes all the\ntests...\n\n\n\n builtin-mv.c | 4 +-\n setup.c | 152 ++++++++++++++++++++++++++++++++++++++++++----------------\n 2 files changed, 112 insertions(+), 44 deletions(-)\n\ndiff --git a/builtin-mv.c b/builtin-mv.c\nindex 990e213..94f6dd2 100644\n--- a/builtin-mv.c\n+++ b/builtin-mv.c\n@@ -164,7 +164,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \t\t\t\t}\n \n \t\t\t\tdst = add_slash(dst);\n-\t\t\t\tdst_len = strlen(dst) - 1;\n+\t\t\t\tdst_len = strlen(dst);\n \n \t\t\t\tfor (j = 0; j < last - first; j++) {\n \t\t\t\t\tconst char *path =\n@@ -172,7 +172,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \t\t\t\t\tsource[argc + j] = path;\n \t\t\t\t\tdestination[argc + j] =\n \t\t\t\t\t\tprefix_path(dst, dst_len,\n-\t\t\t\t\t\t\tpath + length);\n+\t\t\t\t\t\t\tpath + length + 1);\n \t\t\t\t\tmodes[argc + j] = INDEX;\n \t\t\t\t}\n \t\t\t\targc += last - first;\ndiff --git a/setup.c b/setup.c\nindex adede16..fdc6459 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -4,51 +4,114 @@\n static int inside_git_dir = -1;\n static int inside_work_tree = -1;\n \n-const char *prefix_path(const char *prefix, int len, const char *path)\n+static int sanitary_path_copy(char *dst, const char *src)\n {\n-\tconst char *orig = path;\n+\tchar *dst0 = dst;\n+\n+\tif (*src == '/') {\n+\t\t*dst++ = '/';\n+\t\twhile (*src == '/')\n+\t\t\tsrc++;\n+\t}\n+\n \tfor (;;) {\n-\t\tchar c;\n-\t\tif (*path != '.')\n-\t\t\tbreak;\n-\t\tc = path[1];\n-\t\t/* \".\" */\n-\t\tif (!c) {\n-\t\t\tpath++;\n-\t\t\tbreak;\n+\t\tchar c = *src;\n+\n+\t\t/*\n+\t\t * A path component that begins with . could be\n+\t\t * special:\n+\t\t * (1) \".\" and ends -- ignore and terminate.\n+\t\t * (2) \"./\" -- ignore them, eat slash and continue.\n+\t\t * (3) \"..\" and ends -- strip one and terminate.\n+\t\t * (4) \"../\" -- strip one, eat slash and continue.\n+\t\t */\n+\t\tif (c == '.') {\n+\t\t\tswitch (src[1]) {\n+\t\t\tcase '\\0':\n+\t\t\t\t/* (1) */\n+\t\t\t\tsrc++;\n+\t\t\t\tbreak;\n+\t\t\tcase '/':\n+\t\t\t\t/* (2) */\n+\t\t\t\tsrc += 2;\n+\t\t\t\twhile (*src == '/')\n+\t\t\t\t\tsrc++;\n+\t\t\t\tcontinue;\n+\t\t\tcase '.':\n+\t\t\t\tswitch (src[2]) {\n+\t\t\t\tcase '\\0':\n+\t\t\t\t\t/* (3) */\n+\t\t\t\t\tsrc += 2;\n+\t\t\t\t\tgoto up_one;\n+\t\t\t\tcase '/':\n+\t\t\t\t\t/* (4) */\n+\t\t\t\t\tsrc += 3;\n+\t\t\t\t\twhile (*src == '/')\n+\t\t\t\t\t\tsrc++;\n+\t\t\t\t\tgoto up_one;\n+\t\t\t\t}\n+\t\t\t}\n \t\t}\n-\t\t/* \"./\" */\n+\n+\t\t/* copy up to the next '/', and eat all '/' */\n+\t\twhile ((c = *src++) != '\\0' && c != '/')\n+\t\t\t*dst++ = c;\n \t\tif (c == '/') {\n-\t\t\tpath += 2;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (c != '.')\n+\t\t\t*dst++ = c;\n+\t\t\twhile (c == '/')\n+\t\t\t\tc = *src++;\n+\t\t\tsrc--;\n+\t\t} else if (!c)\n \t\t\tbreak;\n-\t\tc = path[2];\n-\t\tif (!c)\n-\t\t\tpath += 2;\n-\t\telse if (c == '/')\n-\t\t\tpath += 3;\n-\t\telse\n-\t\t\tbreak;\n-\t\t/* \"..\" and \"../\" */\n-\t\t/* Remove last component of the prefix */\n-\t\tdo {\n-\t\t\tif (!len)\n-\t\t\t\tdie(\"'%s' is outside repository\", orig);\n-\t\t\tlen--;\n-\t\t} while (len && prefix[len-1] != '/');\n \t\tcontinue;\n+\n+\tup_one:\n+\t\t/*\n+\t\t * dst0..dst is prefix portion, and dst[-1] is '/';\n+\t\t * go up one level.\n+\t\t */\n+\t\tdst -= 2; /* go past trailing '/' if any */\n+\t\tif (dst < dst0)\n+\t\t\treturn -1;\n+\t\twhile (1) {\n+\t\t\tif (dst <= dst0)\n+\t\t\t\tbreak;\n+\t\t\tc = *dst--;\n+\t\t\tif (c == '/') {\n+\t\t\t\tdst += 2;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n \t}\n-\tif (len) {\n-\t\tint speclen = strlen(path);\n-\t\tchar *n = xmalloc(speclen + len + 1);\n+\t*dst = '\\0';\n+\treturn 0;\n+}\n \n-\t\tmemcpy(n, prefix, len);\n-\t\tmemcpy(n + len, path, speclen+1);\n-\t\tpath = n;\n+const char *prefix_path(const char *prefix, int len, const char *path)\n+{\n+\tconst char *orig = path;\n+\tchar *sanitized = xmalloc(len + strlen(path) + 1);\n+\tif (*orig == '/')\n+\t\tstrcpy(sanitized, path);\n+\telse {\n+\t\tif (len)\n+\t\t\tmemcpy(sanitized, prefix, len);\n+\t\tstrcpy(sanitized + len, path);\t\t\n \t}\n-\treturn path;\n+\tif (sanitary_path_copy(sanitized, sanitized))\n+\t\tgoto error_out;\n+\tif (*orig == '/') {\n+\t\tconst char *work_tree = get_git_work_tree();\n+\t\tsize_t len = strlen(work_tree);\n+\t\tif (strncmp(sanitized, work_tree, len) ||\n+\t\t (sanitized[len] != '\\0' && sanitized[len] != '/')) {\n+\t\terror_out:\n+\t\t\terror(\"'%s' is outside repository\", orig);\n+\t\t\tfree(sanitized);\n+\t\t\treturn NULL;\n+\t\t}\n+\t}\n+\treturn sanitized;\n }\n \n /*\n@@ -114,7 +177,7 @@ void verify_non_filename(const char *prefix, const char *arg)\n const char **get_pathspec(const char *prefix, const char **pathspec)\n {\n \tconst char *entry = *pathspec;\n-\tconst char **p;\n+\tconst char **src, **dst;\n \tint prefixlen;\n \n \tif (!prefix && !entry)\n@@ -128,12 +191,17 @@ const char **get_pathspec(const char *prefix, const char **pathspec)\n \t}\n \n \t/* Otherwise we have to re-write the entries.. */\n-\tp = pathspec;\n+\tsrc = pathspec;\n+\tdst = pathspec;\n \tprefixlen = prefix ? strlen(prefix) : 0;\n-\tdo {\n-\t\t*p = prefix_path(prefix, prefixlen, entry);\n-\t} while ((entry = *++p) != NULL);\n-\treturn (const char **) pathspec;\n+\twhile (*src) {\n+\t\tconst char *p = prefix_path(prefix, prefixlen, *src);\n+\t\tif (p)\n+\t\t\t*(dst++) = p;\n+\t\tsrc++;\n+\t}\n+\t*dst = NULL;\n+\treturn pathspec;\n }\n \n /*\n"},{"id":"66772","messageId":"7vd4rls817.fsf@gitster.siamese.dyndns.org","threadId":"11711","inReplyTo":"7vwspts9vj.fsf@gitster.siamese.dyndns.org","subject":"Re: [RFH/PATCH] prefix_path(): disallow absolute paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-29T02:03:00Z","receivedAt":"2008-01-29T02:03:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This is on top of the original \"sanitary_path_copy()\" patch to\nfix the case where the user gives `pwd`/foobar (or just `pwd`)\nto us. After making sure the leading part matches with the work\ntree, we need to strip that to make the result relative to the\nwork tree.\n\n setup.c | 5 +++++\n 1 files changed, 5 insertions(+), 0 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex 156d417..0688e7b 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -110,6 +110,9 @@ const char *prefix_path(const char *prefix, int len, const char *path)\n \t\t\tfree(sanitized);\n \t\t\treturn NULL;\n \t\t}\n+\t\tsanitized += len;\n+\t\tif (*sanitized == '/')\n+\t\t\tsanitized++;\n \t}\n \treturn sanitized;\n }\n@@ -201,6 +204,8 @@ const char **get_pathspec(const char *prefix, const char **pathspec)\n \t\tsrc++;\n \t}\n \t*dst = NULL;\n+\tif (!*pathspec)\n+\t\treturn NULL;\n \treturn pathspec;\n }\n \n"},{"id":"66773","messageId":"7vbq75s80t.fsf@gitster.siamese.dyndns.org","threadId":"11711","inReplyTo":"7vwspts9vj.fsf@gitster.siamese.dyndns.org","subject":"Re: [RFH/PATCH] prefix_path(): disallow absolute paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-29T02:03:14Z","receivedAt":"2008-01-29T02:03:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> I suspect that the right approach might be something like the\n> attached patch. It introduces a version of prefix_path() that\n> sanitizes path (but not prefix part, which comes from git itself\n> and hopefully there should not be a need to sanitize it) while\n> doing the prefixing. It also strips the leading absolute path\n> to the repository by comparing it with the value of work_tree.\n>\n> A few things to note.\n>\n> * Your mv fix is rolled in.\n>\n> * This allows you to name a in-repository file as `pwd`/file,\n> or `pwd`//file (iow, double-slash is also sanitized). It may\n> kill the bird in another thread nearby.\n>\n> * get_pathspec() drops paths outside of repository, so the\n> caller may end up getting a smaller number of paths than it\n> originally gave it. If an existing caller expects the same\n> number of paths to come back, it needs to be adjusted (I did\n> not check). We could alternatively die() but I couldn't\n> decide which one is a better behaviour.\n\nThis is the kind of \"fixing existing callers\" that would be\nneeded if we take this approach.\n\n builtin-ls-files.c | 11 ++++++++++-\n 1 files changed, 10 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-ls-files.c b/builtin-ls-files.c\nindex 0f0ab2d..3801cf4 100644\n--- a/builtin-ls-files.c\n+++ b/builtin-ls-files.c\n@@ -572,8 +572,17 @@ int cmd_ls_files(int argc, const char **argv, const char *prefix)\n \tpathspec = get_pathspec(prefix, argv + i);\n \n \t/* Verify that the pathspec matches the prefix */\n-\tif (pathspec)\n+\tif (pathspec) {\n+\t\tif (argc != i) {\n+\t\t\tint cnt;\n+\t\t\tfor (cnt = 0; pathspec[cnt]; cnt++)\n+\t\t\t\t;\n+\t\t\tif (cnt != (argc - i))\n+\t\t\t\texit(1); /* error message already given */\n+\t\t}\n \t\tprefix = verify_pathspec(prefix);\n+\t} else if (argc != i)\n+\t\texit(1); /* error message already given */\n \n \t/* Treat unmatching pathspec elements as errors */\n \tif (pathspec && error_unmatch) {\n"},{"id":"66777","messageId":"alpine.LSU.1.00.0801290234590.23907@racer.site","threadId":"11711","inReplyTo":"7vwspts9vj.fsf@gitster.siamese.dyndns.org","subject":"Re: [RFH/PATCH] prefix_path(): disallow absolute paths","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-01-29T02:37:59Z","receivedAt":"2008-01-29T02:37:59Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 28 Jan 2008, Junio C Hamano wrote:\n\n> If we are touching the prefix_path(), I think we should try to make its \n> \"ambiguous path rejection\" more complete.\n\nI should have made more clear that I tried to avoid exactly that before \n1.5.4, I guess.\n\n> This is not to be applied (especially before auditing the callers), but \n> to be thought about. Although it passes all the tests...\n\nIt certainly is tempting.\n\n\n> +\t\t\twhile (c == '/')\n> +\t\t\t\tc = *src++;\n> +\t\t\tsrc--;\n\nThis is ugly. I would like this better:\n\n\t\t\twhile (src[1] == '/')\n\t\t\t\tsrc++;\n\n> +const char *prefix_path(const char *prefix, int len, const char *path)\n> +{\n> +\tconst char *orig = path;\n> +\tchar *sanitized = xmalloc(len + strlen(path) + 1);\n\nThere _has_ to be a way to avoid malloc()ing things that will _never_ be \nfree()d again with every second patch ;-)\n\nCiao,\nDscho\n"},{"id":"66779","messageId":"7v7ihts61v.fsf@gitster.siamese.dyndns.org","threadId":"11711","inReplyTo":"alpine.LSU.1.00.0801290234590.23907@racer.site","subject":"Re: [RFH/PATCH] prefix_path(): disallow absolute paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-29T02:45:48Z","receivedAt":"2008-01-29T02:45:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> This is not to be applied (especially before auditing the callers), but \n>> to be thought about. Although it passes all the tests...\n>\n> It certainly is tempting.\n>\n>\n>> +\t\t\twhile (c == '/')\n>> +\t\t\t\tc = *src++;\n>> +\t\t\tsrc--;\n>\n> This is ugly. I would like this better:\n>\n> \t\t\twhile (src[1] == '/')\n> \t\t\t\tsrc++;\n\nWhatever. That was just for discussion.\n\n>> +const char *prefix_path(const char *prefix, int len, const char *path)\n>> +{\n>> +\tconst char *orig = path;\n>> +\tchar *sanitized = xmalloc(len + strlen(path) + 1);\n>\n> There _has_ to be a way to avoid malloc()ing things that will _never_ be \n> free()d again with every second patch ;-)\n\nHuh? prefix_path() already allocates for rewritten pathspec\nentries; this is nothing new.\n"},{"id":"66783","messageId":"alpine.LSU.1.00.0801290255050.23907@racer.site","threadId":"11711","inReplyTo":"7v7ihts61v.fsf@gitster.siamese.dyndns.org","subject":"Re: [RFH/PATCH] prefix_path(): disallow absolute paths","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-01-29T02:59:58Z","receivedAt":"2008-01-29T02:59:58Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 28 Jan 2008, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> >> +const char *prefix_path(const char *prefix, int len, const char *path)\n> >> +{\n> >> +\tconst char *orig = path;\n> >> +\tchar *sanitized = xmalloc(len + strlen(path) + 1);\n> >\n> > There _has_ to be a way to avoid malloc()ing things that will _never_ \n> > be free()d again with every second patch ;-)\n> \n> Huh? prefix_path() already allocates for rewritten pathspec entries; \n> this is nothing new.\n\nRight. It is nothing new. Except that we now allocate for more paths \nthan before.\n\nAt the same time as introducing a new feature (path normalisation), we \ncould introduce another change, which would introduce a function \ncleanup_prefixed_pathspecs(), which would free all path that were \nmalloc()ed.\n\nCiao,\nDscho\n"},{"id":"66815","messageId":"7v7ihtqfm8.fsf@gitster.siamese.dyndns.org","threadId":"11711","inReplyTo":"7vbq75s80t.fsf@gitster.siamese.dyndns.org","subject":"Re: [RFH/PATCH] prefix_path(): disallow absolute paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-29T07:02:07Z","receivedAt":"2008-01-29T07:02:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ok, the three patches from this afternoon, squashed together\ninto one, seems to give us quite a nice property.\n\nHere is an demonstration.\n\n t/t7010-setup.sh | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 117 insertions(+), 0 deletions(-)\n create mode 100755 t/t7010-setup.sh\n\ndiff --git a/t/t7010-setup.sh b/t/t7010-setup.sh\nnew file mode 100755\nindex 0000000..da20ba5\n--- /dev/null\n+++ b/t/t7010-setup.sh\n@@ -0,0 +1,117 @@\n+#!/bin/sh\n+\n+test_description='setup taking and sanitizing funny paths'\n+\n+. ./test-lib.sh\n+\n+test_expect_success setup '\n+\n+\tmkdir -p a/b/c a/e &&\n+\tD=$(pwd) &&\n+\t>a/b/c/d &&\n+\t>a/e/f\n+\n+'\n+\n+test_expect_success 'git add (absolute)' '\n+\n+\tgit add \"$D/a/b/c/d\" &&\n+\tgit ls-files >current &&\n+\techo a/b/c/d >expect &&\n+\tdiff -u expect current\n+\n+'\n+\n+\n+test_expect_success 'git add (funny relative)' '\n+\n+\trm -f .git/index &&\n+\t(\n+\t\tcd a/b &&\n+\t\tgit add \"../e/./f\"\n+\t) &&\n+\tgit ls-files >current &&\n+\techo a/e/f >expect &&\n+\tdiff -u expect current\n+\n+'\n+\n+test_expect_success 'git rm (absolute)' '\n+\n+\trm -f .git/index &&\n+\tgit add a &&\n+\tgit rm -f --cached \"$D/a/b/c/d\" &&\n+\tgit ls-files >current &&\n+\techo a/e/f >expect &&\n+\tdiff -u expect current\n+\n+'\n+\n+test_expect_success 'git rm (funny relative)' '\n+\n+\trm -f .git/index &&\n+\tgit add a &&\n+\t(\n+\t\tcd a/b &&\n+\t\tgit rm -f --cached \"../e/./f\"\n+\t) &&\n+\tgit ls-files >current &&\n+\techo a/b/c/d >expect &&\n+\tdiff -u expect current\n+\n+'\n+\n+test_expect_success 'git ls-files (absolute)' '\n+\n+\trm -f .git/index &&\n+\tgit add a &&\n+\tgit ls-files \"$D/a/e/../b\" >current &&\n+\techo a/b/c/d >expect &&\n+\tdiff -u expect current\n+\n+'\n+\n+test_expect_success 'git ls-files (relative #1)' '\n+\n+\trm -f .git/index &&\n+\tgit add a &&\n+\t(\n+\t\tcd a/b &&\n+\t\tgit ls-files \"../b/c\"\n+\t) >current &&\n+\techo c/d >expect &&\n+\tdiff -u expect current\n+\n+'\n+\n+test_expect_success 'git ls-files (relative #2)' '\n+\n+\trm -f .git/index &&\n+\tgit add a &&\n+\t(\n+\t\tcd a/b &&\n+\t\tgit ls-files --full-name \"../e/f\"\n+\t) >current &&\n+\techo a/e/f >expect &&\n+\tdiff -u expect current\n+\n+'\n+\n+test_expect_success 'git ls-files (relative #3)' '\n+\n+\trm -f .git/index &&\n+\tgit add a &&\n+\t(\n+\t\tcd a/b &&\n+\t\tif git ls-files \"../e/f\"\n+\t\tthen\n+\t\t\techo Gaah, should have failed\n+\t\t\texit 1\n+\t\telse\n+\t\t\t: happy\n+\t\tfi\n+\t)\n+\n+'\n+\n+test_done\n"},{"id":"66816","messageId":"479ED3AE.5000403@viscovery.net","threadId":"11711","inReplyTo":"7vwspts9vj.fsf@gitster.siamese.dyndns.org","subject":"Re: [RFH/PATCH] prefix_path(): disallow absolute paths","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-01-29T07:20:14Z","receivedAt":"2008-01-29T07:20:14Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Junio C Hamano schrieb:\n> +static int sanitary_path_copy(char *dst, const char *src)\n> {\n> -\tconst char *orig = path;\n> +\tchar *dst0 = dst;\n> +\n> +\tif (*src == '/') {\n> +\t\t*dst++ = '/';\n> +\t\twhile (*src == '/')\n> +\t\t\tsrc++;\n> +\t}\n\nAdvance notice: In this function, tests of the kind *src == '/' need to be\nturned into is_dir_sep(*src) when we port to Windows.\n\n> +\t\t/* copy up to the next '/', and eat all '/' */\n> +\t\twhile ((c = *src++) != '\\0' && c != '/')\n> +\t\t\t*dst++ = c;\n> \t\tif (c == '/') {\n> -\t\t\tpath += 2;\n> -\t\t\tcontinue;\n> -\t\t}\n> -\t\tif (c != '.')\n> +\t\t\t*dst++ = c;\n\n\t\t\t*dst++ = '/';\n\nwill be needed on Windows to sanitize all is_dir_sep(c) to '/'.\n\n> +\t\t\twhile (c == '/')\n> +\t\t\t\tc = *src++;\n> +\t\t\tsrc--;\n> +\t\t} else if (!c)\n> \t\t\tbreak;\n...\n> +const char *prefix_path(const char *prefix, int len, const char *path)\n> +{\n> +\tconst char *orig = path;\n> +\tchar *sanitized = xmalloc(len + strlen(path) + 1);\n> +\tif (*orig == '/')\n\n\tif (is_absolute_path(*orig))\n\n> +\t\tstrcpy(sanitized, path);\n> +\telse {\n> +\t\tif (len)\n> +\t\t\tmemcpy(sanitized, prefix, len);\n> +\t\tstrcpy(sanitized + len, path);\t\t\n> \t}\n> -\treturn path;\n> +\tif (sanitary_path_copy(sanitized, sanitized))\n> +\t\tgoto error_out;\n> +\tif (*orig == '/') {\n\nDitto.\n\n> +\t\tconst char *work_tree = get_git_work_tree();\n> +\t\tsize_t len = strlen(work_tree);\n> +\t\tif (strncmp(sanitized, work_tree, len) ||\n> +\t\t (sanitized[len] != '\\0' && sanitized[len] != '/')) {\n> +\t\terror_out:\n> +\t\t\terror(\"'%s' is outside repository\", orig);\n> +\t\t\tfree(sanitized);\n> +\t\t\treturn NULL;\n> +\t\t}\n> +\t}\n> +\treturn sanitized;\n> }\n\nI appreciate this new sanitary_copy_path() because I expect that we will\nneed at least one less #ifdef __MINGW32__/#endif compared to our current\nWindows port.\n\n-- Hannes\n"},{"id":"66818","messageId":"7v3ashqedx.fsf@gitster.siamese.dyndns.org","threadId":"11711","inReplyTo":"479ED3AE.5000403@viscovery.net","subject":"Re: [RFH/PATCH] prefix_path(): disallow absolute paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-29T07:28:42Z","receivedAt":"2008-01-29T07:28:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> I appreciate this new sanitary_copy_path() because I expect that we will\n> need at least one less #ifdef __MINGW32__/#endif compared to our current\n> Windows port.\n\nI would have expected that the whole function would have\nplatform specific implementation to deal with Windows, not just\nifdef sprinkled everywhere that says \"Oh, on this platform\ndirectory separator is a backslash\".\n\nEspecially I have no idea how would that drive lettter stuff\nwould/should work. When you are in C:\\Documents\\Panda\\, how\nwould you express D:\\Movie\\Porn\\My Favorite.mpg as a relative\npath?\n"},{"id":"66819","messageId":"479ED92E.4020709@viscovery.net","threadId":"11711","inReplyTo":"7v3ashqedx.fsf@gitster.siamese.dyndns.org","subject":"Re: [RFH/PATCH] prefix_path(): disallow absolute paths","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-01-29T07:43:42Z","receivedAt":"2008-01-29T07:43:42Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Junio C Hamano schrieb:\n> Johannes Sixt writes:\n> \n>> I appreciate this new sanitary_copy_path() because I expect that we will\n>> need at least one less #ifdef __MINGW32__/#endif compared to our current\n>> Windows port.\n> \n> I would have expected that the whole function would have\n> platform specific implementation to deal with Windows, not just\n> ifdef sprinkled everywhere that says \"Oh, on this platform\n> directory separator is a backslash\".\n\nI agree. Therefore, we would have *one* conditionalized definition of\nis_dir_sep() upfront, and use that in place of c == '/'.\n\nThe #ifdef I'm addressing above is one that we had to introduce because in\nthe old implementation of prefix_path() on Windows we had to rewrite the\npath more often than on *nix due to '\\\\' => '/' conversion. In your new\nimplementation this rewriting now always takes place, but on *nix it more\noften turns out to be an identity operation.\n\n> Especially I have no idea how would that drive lettter stuff\n> would/should work. When you are in C:\\Documents\\Panda\\, how\n> would you express D:\\Movie\\Porn\\My Favorite.mpg as a relative\n> path?\n\nYou can't. But when would this be necessary?\n\n-- Hannes\n"},{"id":"66821","messageId":"7vve5dox0o.fsf_-_@gitster.siamese.dyndns.org","threadId":"11711","inReplyTo":"7v7ihtqfm8.fsf@gitster.siamese.dyndns.org","subject":"[PATCH] setup: sanitize absolute and funny paths in get_pathspec()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-29T08:29:11Z","receivedAt":"2008-01-29T08:29:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The prefix_path() function called from get_pathspec() is\nresponsible for translating list of user-supplied pathspecs to\nlist of pathspecs that is relative to the root of the work\ntree. When working inside a subdirectory, the user-supplied\npathspecs are taken to be relative to the current subdirectory.\n\nAmong special path components in pathspecs, we used to accept\nand interpret only \".\" (\"the directory\", meaning a no-op) and\n\"..\" (\"up one level\") at the beginning. Everything else was\npassed through as-is.\n\nFor example, if you are in Documentation/ directory of the\nproject, you can name Documentation/howto/maintain-git.txt as:\n\n howto/maintain-git.txt\n ../Documentation/howto/maitain-git.txt\n ../././Documentation/howto/maitain-git.txt\n\nbut not as:\n\n howto/./maintain-git.txt\n $(pwd)/howto/maintain-git.txt\n\nThis patch updates prefix_path() in several ways:\n\n - If the pathspec is not absolute, prefix (i.e. the current\n subdirectory relative to the root of the work tree, with\n terminating slash, if not empty) and the pathspec is\n concatenated first and used in the next step. Otherwise,\n that absolute pathspec is used in the next step.\n\n - Then special path components \".\" (no-op) and \"..\" (up one\n level) are interpreted to simplify the path. It is an error\n to have too many \"..\" to cause the intermediate result to\n step outside of the input to this step.\n\n - If the original pathspec was not absolute, the result from\n the previous step is the resulting \"sanitized\" pathspec.\n Otherwise, the result from the previous step is still\n absolute, and it is an error if it does not begin with the\n directory that corresponds to the root of the work tree. The\n directory is stripped away from the result and is returned.\n\n - In any case, the resulting pathspec in the array\n get_pathspec() returns omit the ones that caused errors.\n\nWith this patch, the last two examples also behave as expected.\n\nSigned-off-by: Junio C Hamano \n---\n\n * This is a squash of the handful patches I sent out with some\n clean-ups, to conclude the day for me. Perhaps this will\n appear in 'pu' before 1.5.4 if I really am bored but not very\n likely. I am pretty much bogged down with day-job these days\n during the week...\n\n builtin-ls-files.c | 11 +++-\n builtin-mv.c | 4 +-\n setup.c | 158 ++++++++++++++++++++++++++++++++++++++--------------\n t/t7010-setup.sh | 117 ++++++++++++++++++++++++++++++++++++++\n 4 files changed, 245 insertions(+), 45 deletions(-)\n create mode 100755 t/t7010-setup.sh\n\ndiff --git a/builtin-ls-files.c b/builtin-ls-files.c\nindex 0f0ab2d..3801cf4 100644\n--- a/builtin-ls-files.c\n+++ b/builtin-ls-files.c\n@@ -572,8 +572,17 @@ int cmd_ls_files(int argc, const char **argv, const char *prefix)\n \tpathspec = get_pathspec(prefix, argv + i);\n \n \t/* Verify that the pathspec matches the prefix */\n-\tif (pathspec)\n+\tif (pathspec) {\n+\t\tif (argc != i) {\n+\t\t\tint cnt;\n+\t\t\tfor (cnt = 0; pathspec[cnt]; cnt++)\n+\t\t\t\t;\n+\t\t\tif (cnt != (argc - i))\n+\t\t\t\texit(1); /* error message already given */\n+\t\t}\n \t\tprefix = verify_pathspec(prefix);\n+\t} else if (argc != i)\n+\t\texit(1); /* error message already given */\n \n \t/* Treat unmatching pathspec elements as errors */\n \tif (pathspec && error_unmatch) {\ndiff --git a/builtin-mv.c b/builtin-mv.c\nindex 990e213..94f6dd2 100644\n--- a/builtin-mv.c\n+++ b/builtin-mv.c\n@@ -164,7 +164,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \t\t\t\t}\n \n \t\t\t\tdst = add_slash(dst);\n-\t\t\t\tdst_len = strlen(dst) - 1;\n+\t\t\t\tdst_len = strlen(dst);\n \n \t\t\t\tfor (j = 0; j < last - first; j++) {\n \t\t\t\t\tconst char *path =\n@@ -172,7 +172,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \t\t\t\t\tsource[argc + j] = path;\n \t\t\t\t\tdestination[argc + j] =\n \t\t\t\t\t\tprefix_path(dst, dst_len,\n-\t\t\t\t\t\t\tpath + length);\n+\t\t\t\t\t\t\tpath + length + 1);\n \t\t\t\t\tmodes[argc + j] = INDEX;\n \t\t\t\t}\n \t\t\t\targc += last - first;\ndiff --git a/setup.c b/setup.c\nindex adede16..23c9a11 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -4,51 +4,118 @@\n static int inside_git_dir = -1;\n static int inside_work_tree = -1;\n \n-const char *prefix_path(const char *prefix, int len, const char *path)\n+static int sanitary_path_copy(char *dst, const char *src)\n {\n-\tconst char *orig = path;\n+\tchar *dst0 = dst;\n+\n+\tif (*src == '/') {\n+\t\t*dst++ = '/';\n+\t\twhile (*src == '/')\n+\t\t\tsrc++;\n+\t}\n+\n \tfor (;;) {\n-\t\tchar c;\n-\t\tif (*path != '.')\n-\t\t\tbreak;\n-\t\tc = path[1];\n-\t\t/* \".\" */\n-\t\tif (!c) {\n-\t\t\tpath++;\n-\t\t\tbreak;\n+\t\tchar c = *src;\n+\n+\t\t/*\n+\t\t * A path component that begins with . could be\n+\t\t * special:\n+\t\t * (1) \".\" and ends -- ignore and terminate.\n+\t\t * (2) \"./\" -- ignore them, eat slash and continue.\n+\t\t * (3) \"..\" and ends -- strip one and terminate.\n+\t\t * (4) \"../\" -- strip one, eat slash and continue.\n+\t\t */\n+\t\tif (c == '.') {\n+\t\t\tswitch (src[1]) {\n+\t\t\tcase '\\0':\n+\t\t\t\t/* (1) */\n+\t\t\t\tsrc++;\n+\t\t\t\tbreak;\n+\t\t\tcase '/':\n+\t\t\t\t/* (2) */\n+\t\t\t\tsrc += 2;\n+\t\t\t\twhile (*src == '/')\n+\t\t\t\t\tsrc++;\n+\t\t\t\tcontinue;\n+\t\t\tcase '.':\n+\t\t\t\tswitch (src[2]) {\n+\t\t\t\tcase '\\0':\n+\t\t\t\t\t/* (3) */\n+\t\t\t\t\tsrc += 2;\n+\t\t\t\t\tgoto up_one;\n+\t\t\t\tcase '/':\n+\t\t\t\t\t/* (4) */\n+\t\t\t\t\tsrc += 3;\n+\t\t\t\t\twhile (*src == '/')\n+\t\t\t\t\t\tsrc++;\n+\t\t\t\t\tgoto up_one;\n+\t\t\t\t}\n+\t\t\t}\n \t\t}\n-\t\t/* \"./\" */\n+\n+\t\t/* copy up to the next '/', and eat all '/' */\n+\t\twhile ((c = *src++) != '\\0' && c != '/')\n+\t\t\t*dst++ = c;\n \t\tif (c == '/') {\n-\t\t\tpath += 2;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (c != '.')\n+\t\t\t*dst++ = c;\n+\t\t\twhile (c == '/')\n+\t\t\t\tc = *src++;\n+\t\t\tsrc--;\n+\t\t} else if (!c)\n \t\t\tbreak;\n-\t\tc = path[2];\n-\t\tif (!c)\n-\t\t\tpath += 2;\n-\t\telse if (c == '/')\n-\t\t\tpath += 3;\n-\t\telse\n-\t\t\tbreak;\n-\t\t/* \"..\" and \"../\" */\n-\t\t/* Remove last component of the prefix */\n-\t\tdo {\n-\t\t\tif (!len)\n-\t\t\t\tdie(\"'%s' is outside repository\", orig);\n-\t\t\tlen--;\n-\t\t} while (len && prefix[len-1] != '/');\n \t\tcontinue;\n+\n+\tup_one:\n+\t\t/*\n+\t\t * dst0..dst is prefix portion, and dst[-1] is '/';\n+\t\t * go up one level.\n+\t\t */\n+\t\tdst -= 2; /* go past trailing '/' if any */\n+\t\tif (dst < dst0)\n+\t\t\treturn -1;\n+\t\twhile (1) {\n+\t\t\tif (dst <= dst0)\n+\t\t\t\tbreak;\n+\t\t\tc = *dst--;\n+\t\t\tif (c == '/') {\n+\t\t\t\tdst += 2;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n \t}\n-\tif (len) {\n-\t\tint speclen = strlen(path);\n-\t\tchar *n = xmalloc(speclen + len + 1);\n+\t*dst = '\\0';\n+\treturn 0;\n+}\n \n-\t\tmemcpy(n, prefix, len);\n-\t\tmemcpy(n + len, path, speclen+1);\n-\t\tpath = n;\n+const char *prefix_path(const char *prefix, int len, const char *path)\n+{\n+\tconst char *orig = path;\n+\tchar *sanitized = xmalloc(len + strlen(path) + 1);\n+\tif (*orig == '/')\n+\t\tstrcpy(sanitized, path);\n+\telse {\n+\t\tif (len)\n+\t\t\tmemcpy(sanitized, prefix, len);\n+\t\tstrcpy(sanitized + len, path);\n \t}\n-\treturn path;\n+\tif (sanitary_path_copy(sanitized, sanitized))\n+\t\tgoto error_out;\n+\tif (*orig == '/') {\n+\t\tconst char *work_tree = get_git_work_tree();\n+\t\tsize_t len = strlen(work_tree);\n+\t\tsize_t total = strlen(sanitized) + 1;\n+\t\tif (strncmp(sanitized, work_tree, len) ||\n+\t\t (sanitized[len] != '\\0' && sanitized[len] != '/')) {\n+\t\terror_out:\n+\t\t\terror(\"'%s' is outside repository\", orig);\n+\t\t\tfree(sanitized);\n+\t\t\treturn NULL;\n+\t\t}\n+\t\tif (sanitized[len] == '/')\n+\t\t\tlen++;\n+\t\tmemmove(sanitized, sanitized + len, total - len);\n+\t}\n+\treturn sanitized;\n }\n \n /*\n@@ -114,7 +181,7 @@ void verify_non_filename(const char *prefix, const char *arg)\n const char **get_pathspec(const char *prefix, const char **pathspec)\n {\n \tconst char *entry = *pathspec;\n-\tconst char **p;\n+\tconst char **src, **dst;\n \tint prefixlen;\n \n \tif (!prefix && !entry)\n@@ -128,12 +195,19 @@ const char **get_pathspec(const char *prefix, const char **pathspec)\n \t}\n \n \t/* Otherwise we have to re-write the entries.. */\n-\tp = pathspec;\n+\tsrc = pathspec;\n+\tdst = pathspec;\n \tprefixlen = prefix ? strlen(prefix) : 0;\n-\tdo {\n-\t\t*p = prefix_path(prefix, prefixlen, entry);\n-\t} while ((entry = *++p) != NULL);\n-\treturn (const char **) pathspec;\n+\twhile (*src) {\n+\t\tconst char *p = prefix_path(prefix, prefixlen, *src);\n+\t\tif (p)\n+\t\t\t*(dst++) = p;\n+\t\tsrc++;\n+\t}\n+\t*dst = NULL;\n+\tif (!*pathspec)\n+\t\treturn NULL;\n+\treturn pathspec;\n }\n \n /*\ndiff --git a/t/t7010-setup.sh b/t/t7010-setup.sh\nnew file mode 100755\nindex 0000000..da20ba5\n--- /dev/null\n+++ b/t/t7010-setup.sh\n@@ -0,0 +1,117 @@\n+#!/bin/sh\n+\n+test_description='setup taking and sanitizing funny paths'\n+\n+. ./test-lib.sh\n+\n+test_expect_success setup '\n+\n+\tmkdir -p a/b/c a/e &&\n+\tD=$(pwd) &&\n+\t>a/b/c/d &&\n+\t>a/e/f\n+\n+'\n+\n+test_expect_success 'git add (absolute)' '\n+\n+\tgit add \"$D/a/b/c/d\" &&\n+\tgit ls-files >current &&\n+\techo a/b/c/d >expect &&\n+\tdiff -u expect current\n+\n+'\n+\n+\n+test_expect_success 'git add (funny relative)' '\n+\n+\trm -f .git/index &&\n+\t(\n+\t\tcd a/b &&\n+\t\tgit add \"../e/./f\"\n+\t) &&\n+\tgit ls-files >current &&\n+\techo a/e/f >expect &&\n+\tdiff -u expect current\n+\n+'\n+\n+test_expect_success 'git rm (absolute)' '\n+\n+\trm -f .git/index &&\n+\tgit add a &&\n+\tgit rm -f --cached \"$D/a/b/c/d\" &&\n+\tgit ls-files >current &&\n+\techo a/e/f >expect &&\n+\tdiff -u expect current\n+\n+'\n+\n+test_expect_success 'git rm (funny relative)' '\n+\n+\trm -f .git/index &&\n+\tgit add a &&\n+\t(\n+\t\tcd a/b &&\n+\t\tgit rm -f --cached \"../e/./f\"\n+\t) &&\n+\tgit ls-files >current &&\n+\techo a/b/c/d >expect &&\n+\tdiff -u expect current\n+\n+'\n+\n+test_expect_success 'git ls-files (absolute)' '\n+\n+\trm -f .git/index &&\n+\tgit add a &&\n+\tgit ls-files \"$D/a/e/../b\" >current &&\n+\techo a/b/c/d >expect &&\n+\tdiff -u expect current\n+\n+'\n+\n+test_expect_success 'git ls-files (relative #1)' '\n+\n+\trm -f .git/index &&\n+\tgit add a &&\n+\t(\n+\t\tcd a/b &&\n+\t\tgit ls-files \"../b/c\"\n+\t) >current &&\n+\techo c/d >expect &&\n+\tdiff -u expect current\n+\n+'\n+\n+test_expect_success 'git ls-files (relative #2)' '\n+\n+\trm -f .git/index &&\n+\tgit add a &&\n+\t(\n+\t\tcd a/b &&\n+\t\tgit ls-files --full-name \"../e/f\"\n+\t) >current &&\n+\techo a/e/f >expect &&\n+\tdiff -u expect current\n+\n+'\n+\n+test_expect_success 'git ls-files (relative #3)' '\n+\n+\trm -f .git/index &&\n+\tgit add a &&\n+\t(\n+\t\tcd a/b &&\n+\t\tif git ls-files \"../e/f\"\n+\t\tthen\n+\t\t\techo Gaah, should have failed\n+\t\t\texit 1\n+\t\telse\n+\t\t\t: happy\n+\t\tfi\n+\t)\n+\n+'\n+\n+test_done\n-- \n1.5.4.rc5\n"},{"id":"66823","messageId":"7vprvlowwg.fsf@gitster.siamese.dyndns.org","threadId":"11711","inReplyTo":"479ED92E.4020709@viscovery.net","subject":"Re: [RFH/PATCH] prefix_path(): disallow absolute paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-29T08:31:43Z","receivedAt":"2008-01-29T08:31:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> The #ifdef I'm addressing above is one that we had to introduce because in\n> the old implementation of prefix_path() on Windows we had to rewrite the\n> path more often than on *nix due to '\\\\' => '/' conversion. In your new\n> implementation this rewriting now always takes place, but on *nix it more\n> often turns out to be an identity operation.\n\nI am not sure what you mean by \"\\\\\" => '/', but you are right.\nWe should be able to optimize the common case of prefix=none and\nno special path components found in path, which should be an\nidentity function.\n\n>> Especially I have no idea how would that drive lettter stuff\n>> would/should work. When you are in C:\\Documents\\Panda\\, how\n>> would you express D:\\Movie\\Porn\\My Favorite.mpg as a relative\n>> path?\n>\n> You can't. But when would this be necessary?\n\nI don't know. That's why I asked.\n"},{"id":"66860","messageId":"200801292158.m0TLwA7u019321@mi0.bluebottle.com","threadId":"11711","inReplyTo":"7vwspts9vj.fsf@gitster.siamese.dyndns.org","subject":"Re: [RFH/PATCH] prefix_path(): disallow absolute paths","fromName":"しらいしななこ","fromEmail":"nanako3@bluebottle.com","sentAt":"2008-01-29T21:53:48Z","receivedAt":"2008-01-29T21:53:48Z","isPatch":true,"sender":{"key":"nanako3@lavabit.com","avatar":"https://gravatar.com/avatar/3777b9e201c5883a62b1a6fdf7c53f2d712d1d80989146063ea861e33aad72a8?d=mp&s=160"},"body":"Quoting Junio C Hamano :\n\n> Currently, we:\n>\n> - Remove \".\" path component (i.e. the directory leading part\n> specified) from the input;\n>\n> - Remove \"..\" path component and strip one level of the prefix;\n>\n> only from the beginning. So if you give nonsense pathspec from\n> the command line, you can end up calling prefix_path() with things\n> like \"/README\", \"/absolute/path/to//repository/tracked/file\", and\n> \"fo//o/../o\".\n>\n> And not passing such ambiguous path like \"fo//o\" to the core\n> level but sanitizing matters. Then core level can always do\n> memcmp() with \"fo/o\" to see they are talking about the same\n> path.\n\nI may be mistaken but I think \"fo//o\" and \"fo//o/\" are returned as two different strings \"fo/o\" and \"fo/o/\" from your patch. Shouldn't you clean-up the second one to \"fo/o\", too?\n\n-- \nNanako Shiraishi\nhttp://ivory.ap.teacup.com/nanako3/\n\n----------------------------------------------------------------------\nFree pop3 email with a spam filter.\nhttp://www.bluebottle.com/tag/5\n"},{"id":"66875","messageId":"7vhcgwkurj.fsf@gitster.siamese.dyndns.org","threadId":"11711","inReplyTo":"200801292158.m0TLwA7u019321@mi0.bluebottle.com","subject":"Re: [RFH/PATCH] prefix_path(): disallow absolute paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-30T00:43:44Z","receivedAt":"2008-01-30T00:43:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"しらいしななこ writes:\n\n> Quoting Junio C Hamano :\n>...\n>> And not passing such ambiguous path like \"fo//o\" to the core\n>> level but sanitizing matters. Then core level can always do\n>> memcmp() with \"fo/o\" to see they are talking about the same\n>> path.\n>\n> I may be mistaken but I think \"fo//o\" and \"fo//o/\" are\n> returned as two different strings \"fo/o\" and \"fo/o/\" from your\n> patch. Shouldn't you clean-up the second one to \"fo/o\", too?\n\nThat certainly is a potentially possible sanitization, and I\nactually thought about it when I did these patches.\n\nHowever, I chose not to because I was not sure if some core\nfunctions want to differentiate pathspecs \"foo/\" and \"foo\". The\nformer says \"I want to make sure that 'foo' is a directory, and\nwant to affect everything under it\", the latter says \"I do not\ncare if 'foo' is a blob or a directory, but I want to affect it\n(if a blob) or everything under it (if a directory)\".\n\nIn fact, stripping trailing slashes off would break this pair:\n\n\tgit ls-files --error-unmatch Makefile/\n\tgit ls-files --error-unmatch Makefile\n\nThings like \"git add Makefile/\" relies on the former to fail\nloudly. So the answer is no, we do not want to clean it up.\n\nIncidentally, I notice that in addition to the squashed patch\nfrom yesterday, we would need to teach \"error-unmatch\" code that\nit should trigger when get_pathspec() returns a pathspec that\nhas fewer number of paths than its input.\n\nIt should be a pretty straightforward patch, but I haven't\nlooked into fixing it. I'm lazy and I'd rather have other\npeople to do the fixing for me. Hint, hint... ;-)\n\nBy the way, please keep your lines to reasonable length by\nwrapping in your e-mailed messages.\n"},{"id":"67003","messageId":"200802010507.05421.robin.rosenberg.lists@dewire.com","threadId":"11711","inReplyTo":"7vve5dox0o.fsf_-_@gitster.siamese.dyndns.org","subject":"[PATCH] Make blame accept absolute paths","fromName":"Robin Rosenberg","fromEmail":"robin.rosenberg.lists@dewire.com","sentAt":"2008-02-01T04:07:04Z","receivedAt":"2008-02-01T04:07:04Z","isPatch":true,"sender":{"key":"robin.rosenberg@dewire.com","avatar":"https://avatars.githubusercontent.com/u/46357?v=4"},"body":"\nBlame did not always use prefix_path.\n\nSigned-off-by: Robin Rosenberg \n---\n builtin-blame.c | 4 +---\n 1 files changed, 1 insertions(+), 3 deletions(-)\n\nThis is a followup to \"setup: sanitize absolute and funny paths in get_pathspec()\"\nThanks Junio for fixing this wish of mine.\n\ndiff --git a/builtin-blame.c b/builtin-blame.c\nindex c7e6887..f88c32a 100644\n--- a/builtin-blame.c\n+++ b/builtin-blame.c\n@@ -1894,9 +1894,7 @@ static unsigned parse_score(const char *arg)\n \n static const char *add_prefix(const char *prefix, const char *path)\n {\n-\tif (!prefix || !prefix[0])\n-\t\treturn path;\n-\treturn prefix_path(prefix, strlen(prefix), path);\n+\treturn prefix_path(prefix, prefix ? strlen(prefix) : 0, path);\n }\n \n /*\n-- \n1.5.4.rc4.25.g81cc\n"},{"id":"67004","messageId":"200802010534.55925.robin.rosenberg.lists@dewire.com","threadId":"11711","inReplyTo":"7vve5dox0o.fsf_-_@gitster.siamese.dyndns.org","subject":"[PATCH] More test cases for sanitized path names","fromName":"Robin Rosenberg","fromEmail":"robin.rosenberg.lists@dewire.com","sentAt":"2008-02-01T04:34:55Z","receivedAt":"2008-02-01T04:34:55Z","isPatch":true,"sender":{"key":"robin.rosenberg@dewire.com","avatar":"https://avatars.githubusercontent.com/u/46357?v=4"},"body":"Verify a few more commands and pathname variants.\n\nSigned-off-by: Robin Rosenberg \n---\n t/t7010-setup.sh | 39 +++++++++++++++++++++++++++++++++++++++\n 1 files changed, 39 insertions(+), 0 deletions(-)\n\nThese are a few testcases from my earlier attempt at this. The\nlog and commit cases succeeded with Junios version, but not \nblame and some of the nastier versions for git add (same\nprinciple for all commands, just that I use add as an example)\n\n-- robin\n\ndiff --git a/t/t7010-setup.sh b/t/t7010-setup.sh\nindex da20ba5..60c4a46 100755\n--- a/t/t7010-setup.sh\n+++ b/t/t7010-setup.sh\n@@ -114,4 +114,43 @@ test_expect_success 'git ls-files (relative #3)' '\n \n '\n \n+test_expect_success 'commit using absolute path names' '\n+\tgit commit -m \"foo\" &&\n+\techo aa >>a/b/c/d &&\n+\tgit commit -m \"aa\" \"$(pwd)/a/b/c/d\"\n+'\n+\n+test_expect_success 'log using absolute path names' '\n+\techo bb >>a/b/c/d &&\n+\tgit commit -m \"bb\" $(pwd)/a/b/c/d &&\n+\n+\tgit log a/b/c/d >f1.txt &&\n+\tgit log \"$(pwd)/a/b/c/d\" >f2.txt &&\n+\tdiff -u f1.txt f2.txt\n+'\n+\n+test_expect_success 'blame using absolute path names' '\n+\tgit blame a/b/c/d >f1.txt &&\n+\tgit blame \"$(pwd)/a/b/c/d\" >f2.txt &&\n+\tdiff -u f1.txt f2.txt\n+'\n+\n+test_expect_failure 'add a directory outside the work tree' '\n+\td1=\"$(cd .. ; pwd)\" &&\n+\tgit add \"$d1\"\n+\techo $?\n+'\n+\n+test_expect_failure 'add a file outside the work tree, nasty case 1' '(\n+\tf=\"$(pwd)x\" &&\n+\ttouch \"$f\" &&\n+\tgit add \"$f\"\n+)'\n+\n+test_expect_failure 'add a file outside the work tree, nasty case 2' '(\n+\tf=\"$(pwd|sed \"s/.$//\")x\" &&\n+\ttouch \"$f\" &&\n+\tgit add \"$f\"\n+)'\n+\n test_done\n-- \n1.5.4.rc4.25.g81cc\n"},{"id":"67009","messageId":"7vabmlb0y0.fsf@gitster.siamese.dyndns.org","threadId":"11711","inReplyTo":"200802010534.55925.robin.rosenberg.lists@dewire.com","subject":"Re: [PATCH] More test cases for sanitized path names","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-01T07:17:11Z","receivedAt":"2008-02-01T07:17:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Robin Rosenberg writes:\n\n> +test_expect_failure 'add a directory outside the work tree' '\n> +\td1=\"$(cd .. ; pwd)\" &&\n> +\tgit add \"$d1\"\n> +\techo $?\n> +'\n\nThis test will always fail as the final exit status is that of\n\"echo\", which will exit with success and you are expecting a\nfailure.\n\n> +test_expect_failure 'add a file outside the work tree, nasty case 1' '(\n> +\tf=\"$(pwd)x\" &&\n> +\ttouch \"$f\" &&\n> +\tgit add \"$f\"\n> +)'\n\nYou are in the directory \"t/trash\", and try to add t/trashx, so\nthis should fail and you would want to make sure it fails.\n\nBut this has a few problems:\n\n * First, the obvious one. You are creating a garbage file\n outside of t/trash directory. Don't. If you need to, dig a\n test directory one level lower inside t/trash and play around\n there.\n\n * In general you should stay away from test_expect_failure. If\n any of the command in && chain fails, it fails the whole\n thing, but you cannot tell if the sequence failed at the\n command you expected to fail or something else that is much\n earlier. For example, it may be that somebody created t/trashx\n file in the source tree that is read-only, and the comand\n that failed in the sequence could be 'touch' before the\n command you are testing.\n\n Instead, write it like (after fixing it not to go outside\n t/trash):\n\n\ttest_expect_success 'add a path outside repo (1)' '\n\n\t\tfile=path_to_outside_repo &&\n touch \"$file\" &&\n\t\t! git add \"$f\"\n\n\t'\n\nI'd like to make the _first_ patch after 1.5.4 to be a fix-up\nfor tests that misuse test_expect_failure. After that, we can\nuse test_expect_failure to mark tests that ought to pass but\ndon't because of bugs in the commands. That way, people who are\nabsolutely bored can grep for test_expect_failure to see what\nexisting issues to tackle ;-).\n"},{"id":"67016","messageId":"200802011010.41938.robin.rosenberg.lists@dewire.com","threadId":"11711","inReplyTo":"7vabmlb0y0.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] More test cases for sanitized path names","fromName":"Robin Rosenberg","fromEmail":"robin.rosenberg.lists@dewire.com","sentAt":"2008-02-01T09:10:40Z","receivedAt":"2008-02-01T09:10:40Z","isPatch":true,"sender":{"key":"robin.rosenberg@dewire.com","avatar":"https://avatars.githubusercontent.com/u/46357?v=4"},"body":"fredagen den 1 februari 2008 skrev Junio C Hamano:\n> Robin Rosenberg writes:\n> \n> > +test_expect_failure 'add a directory outside the work tree' '\n> > +\td1=\"$(cd .. ; pwd)\" &&\n> > +\tgit add \"$d1\"\n> > +\techo $?\n> > +'\nOops. Remove the echo $?. It still fails, i.e. git add succeeds when\nit shouldn't. I was double checking it just before sending the patch.\n\n> > +test_expect_failure 'add a file outside the work tree, nasty case 1' '(\n> > +\tf=\"$(pwd)x\" &&\n> > +\ttouch \"$f\" &&\n> > +\tgit add \"$f\"\n> > +)'\n> \n> You are in the directory \"t/trash\", and try to add t/trashx, so\n> this should fail and you would want to make sure it fails.\n> \n> But this has a few problems:\n> \n> * First, the obvious one. You are creating a garbage file\n> outside of t/trash directory. Don't. If you need to, dig a\n> test directory one level lower inside t/trash and play around\n> there.\nCan we move the default trash one level down for all tests? That\nwould give us one free level to play with.\n\n> * In general you should stay away from test_expect_failure. If\nI respect that.\n[...]\n> I'd like to make the _first_ patch after 1.5.4 to be a fix-up\n> for tests that misuse test_expect_failure. After that, we can\n> use test_expect_failure to mark tests that ought to pass but\n> don't because of bugs in the commands. That way, people who are\n> absolutely bored can grep for test_expect_failure to see what\n> existing issues to tackle ;-).\n\nIf I recall things properly there are lots of test that test for success\nrather than checking that the command does what it should.\n\nUpdate follows.\n\n-- robin\n\n>From 11a52821ca81096987f53c29bf1b9ce373fe7fd4 Mon Sep 17 00:00:00 2001\nFrom: Robin Rosenberg \nDate: Fri, 1 Feb 2008 05:29:38 +0100\nSubject: [PATCH] More test cases for sanitized path names\n\nVerify a few more commands and pathname variants.\n\nSigned-off-by: Robin Rosenberg \n---\n t/t7010-setup.sh | 42 ++++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 42 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t7010-setup.sh b/t/t7010-setup.sh\nindex da20ba5..ef30099 100755\n--- a/t/t7010-setup.sh\n+++ b/t/t7010-setup.sh\n@@ -4,6 +4,10 @@ test_description='setup taking and sanitizing funny paths'\n \n . ./test-lib.sh\n \n+rm -rf .git\n+test_create_repo repo\n+cd repo\n+\n test_expect_success setup '\n \n \tmkdir -p a/b/c a/e &&\n@@ -114,4 +118,42 @@ test_expect_success 'git ls-files (relative #3)' '\n \n '\n \n+test_expect_success 'commit using absolute path names' '\n+\tgit commit -m \"foo\" &&\n+\techo aa >>a/b/c/d &&\n+\tgit commit -m \"aa\" \"$(pwd)/a/b/c/d\"\n+'\n+\n+test_expect_success 'log using absolute path names' '\n+\techo bb >>a/b/c/d &&\n+\tgit commit -m \"bb\" $(pwd)/a/b/c/d &&\n+\n+\tgit log a/b/c/d >f1.txt &&\n+\tgit log \"$(pwd)/a/b/c/d\" >f2.txt &&\n+\tdiff -u f1.txt f2.txt\n+'\n+\n+test_expect_success 'blame using absolute path names' '\n+\tgit blame a/b/c/d >f1.txt &&\n+\tgit blame \"$(pwd)/a/b/c/d\" >f2.txt &&\n+\tdiff -u f1.txt f2.txt\n+'\n+\n+test_expect_success 'add a directory outside the work tree' '\n+\td1=\"$(cd .. ; pwd)\" &&\n+\t! git add \"$d1\"\n+'\n+\n+test_expect_success 'add a file outside the work tree, nasty case 1' '(\n+\tf=\"$(pwd)x\" &&\n+\ttouch \"$f\" &&\n+\t! git add \"$f\"\n+)'\n+\n+test_expect_success 'add a file outside the work tree, nasty case 2' '(\n+\tf=\"$(pwd|sed \"s/.$//\")x\" &&\n+\ttouch \"$f\" &&\n+\t! git add \"$f\"\n+)'\n+\n test_done\n-- \n1.5.4.rc4.25.g81cc\n"},{"id":"67017","messageId":"20080201091618.GA32734@diana.vm.bytemark.co.uk","threadId":"11711","inReplyTo":"7vabmlb0y0.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] More test cases for sanitized path names","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2008-02-01T09:16:18Z","receivedAt":"2008-02-01T09:16:18Z","isPatch":true,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"On 2008-01-31 23:17:11 -0800, Junio C Hamano wrote:\n\n> I'd like to make the _first_ patch after 1.5.4 to be a fix-up for\n> tests that misuse test_expect_failure. After that, we can use\n> test_expect_failure to mark tests that ought to pass but don't\n> because of bugs in the commands.\n\nThis is precisely what StGit's test suite does, and it has a very nice\nproperty (beyond looking tidy): it's possible to first commit a new\n(failing) test, and then commit a fix for the bug that made the test\nfail, and have the test suite pass at every step.\n\n-- \nKarl Hasselström, kha@treskal.com\n www.treskal.com/kalle\n"},{"id":"67018","messageId":"7vwspp9f9e.fsf_-_@gitster.siamese.dyndns.org","threadId":"11711","inReplyTo":"7vabmlb0y0.fsf@gitster.siamese.dyndns.org","subject":"[PATCH for post 1.5.4] Sane use of test_expect_failure","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-01T09:50:53Z","receivedAt":"2008-02-01T09:50:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Originally, test_expect_failure was designed to be the opposite\nof test_expect_success, but this was a bad decision. Most tests\nrun a series of commands that leads to the single command that\nneeds to be tested, like this:\n\n test_expect_{success,failure} 'test title' '\n\tsetup1 &&\n setup2 &&\n setup3 &&\n what is to be tested\n '\n\nAnd expecting a failure exit from the whole sequence misses the\npoint of writing tests. Your setup$N that are supposed to\nsucceed may have failed without even reaching what you are\ntrying to test. The only valid use of test_expect_failure is to\ncheck a trivial single command that is expected to fail, which\nis a minority in tests of Porcelain-ish commands.\n\nThis large-ish patch rewrites all uses of test_expect_failure to\nuse test_expect_success and rewrites the condition of what is\ntested, like this:\n\n test_expect_success 'test title' '\n\tsetup1 &&\n setup2 &&\n setup3 &&\n ! this command should fail\n '\n\ntest_expect_failure is redefined to serve as a reminder that\nthat test *should* succeed but due to a known breakage in git it\ncurrently does not pass. So if git-foo command should create a\nfile 'bar' but you discovered a bug that it doesn't, you can\nwrite a test like this:\n\n test_expect_failure 'git-foo should create bar' '\n rm -f bar &&\n git foo &&\n test -f bar\n '\n\nThis construct acts similar to test_expect_success, but instead\nof reporting \"ok/FAIL\" like test_expect_success does, the\noutcome is reported as \"FIXED/still broken\".\n\nSigned-off-by: Junio C Hamano \n---\n Junio C Hamano writes:\n\n > I'd like to make the _first_ patch after 1.5.4 to be a fix-up\n > for tests that misuse test_expect_failure. After that, we can\n > use test_expect_failure to mark tests that ought to pass but\n > don't because of bugs in the commands. That way, people who are\n > absolutely bored can grep for test_expect_failure to see what\n > existing issues to tackle ;-).\n\n This turned out to be a huge patch. I tried to be careful by\n keeping the conversion mostly mechanical, but I am not sure\n about some of the git-svn and git-cvsserver tests, some of which\n may already have been using test_expect_failure to mark a known\n breakage.\n\n Eyeballing by area experts are very much appreciated.\n\n t/README | 14 +--\n t/t0000-basic.sh | 30 ++++--\n t/t0030-stripspace.sh | 40 ++++----\n t/t0040-parse-options.sh | 4 +-\n t/t1000-read-tree-m-3way.sh | 161 ++++++++++++++++-------------\n t/t1200-tutorial.sh | 8 +-\n t/t1300-repo-config.sh | 39 ++++---\n t/t1302-repo-version.sh | 5 +-\n t/t1400-update-ref.sh | 24 ++--\n t/t2000-checkout-cache-clash.sh | 4 +-\n t/t2002-checkout-cache-u.sh | 4 +-\n t/t2008-checkout-subdir.sh | 16 ++--\n t/t2100-update-cache-badpath.sh | 4 +-\n t/t3020-ls-files-error-unmatch.sh | 4 +-\n t/t3200-branch.sh | 36 ++++---\n t/t3210-pack-refs.sh | 26 +++---\n t/t3400-rebase.sh | 4 +-\n t/t3403-rebase-skip.sh | 6 +-\n t/t3600-rm.sh | 17 ++--\n t/t4103-apply-binary.sh | 68 +++++++------\n t/t4113-apply-ending.sh | 8 +-\n t/t5300-pack-object.sh | 4 +-\n t/t5302-pack-index.sh | 32 +++---\n t/t5401-update-hooks.sh | 8 +-\n t/t5402-post-merge-hook.sh | 4 +-\n t/t5500-fetch-pack.sh | 4 +-\n t/t5510-fetch.sh | 12 +-\n t/t5530-upload-pack-error.sh | 14 +--\n t/t5600-clone-fail-cleanup.sh | 12 +-\n t/t5710-info-alternate.sh | 14 ++-\n t/t6023-merge-file.sh | 12 +-\n t/t6024-recursive-merge.sh | 2 +-\n t/t6025-merge-symlinks.sh | 21 ++--\n t/t6101-rev-parse-parents.sh | 2 +-\n t/t6300-for-each-ref.sh | 8 +-\n t/t7001-mv.sh | 4 +-\n t/t7002-grep.sh | 4 +-\n t/t7004-tag.sh | 36 +++---\n t/t7101-reset.sh | 24 ++--\n t/t7501-commit.sh | 40 ++++----\n t/t7503-pre-commit-hook.sh | 4 +-\n t/t7504-commit-msg-hook.sh | 8 +-\n t/t9100-git-svn-basic.sh | 28 +++---\n t/t9106-git-svn-commit-diff-clobber.sh | 18 ++--\n t/t9106-git-svn-dcommit-clobber-series.sh | 4 +-\n t/t9300-fast-import.sh | 24 ++--\n t/t9400-git-cvsserver-server.sh | 30 +++--\n t/test-lib.sh | 30 +++++-\n 48 files changed, 498 insertions(+), 427 deletions(-)\n\ndiff --git a/t/README b/t/README\nindex 36f2517..73ed11b 100644\n--- a/t/README\n+++ b/t/README\n@@ -160,14 +160,12 @@ library for your script to use.\n \n - test_expect_failure