{"thread":{"id":"15152","subject":"[PATCH] Support \"core.excludesfile = ~/.gitignore\"","startedAt":"2008-08-22T04:14:48Z","lastAt":"2008-08-30T06:02:00Z","messageCount":45,"participants":["Karl Chen","Eric Raible","Bert Wesarg","Junio C Hamano","Jeff King","Miklos Vajna","Nguyen Thai Ngoc Duy","Johannes Sixt","Michael J Gruber","Nguyễn Thái Ngọc Duy"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"88082","messageId":"quack.20080821T2114.lthvdxtvg7b@roar.cs.berkeley.edu","threadId":"15152","inReplyTo":null,"subject":"[PATCH] Support \"core.excludesfile = ~/.gitignore\"","fromName":"Karl Chen","fromEmail":"quarl@cs.berkeley.edu","sentAt":"2008-08-22T04:14:48Z","receivedAt":"2008-08-22T04:14:48Z","isPatch":true,"sender":{"key":"quarl@cs.berkeley.edu","avatar":null},"body":"\nI keep my rc files, including .gitconfig and my default gitignore\nlist under version control and like to have the same contents\neverywhere. Unfortunately my home directory is at different\nlocations on different systems.\n\nI'd like to be able to put something like this in my ~/.gitconfig:\n\n[core]\n excludesfile = ~/.gitignore\n\nor\n excludesfile = $HOME/.gitignore\n\nAnother idea is to have a non-absolute path be interpreted\nrelative to the location of .gitconfig, i.e. $HOME, instead of the\ncurrent directory. $GIT_DIR/info/excludes is already for\nrepository-specific excludes so no functionality would be lost.\n\n\nBelow is a sample patch that works for me. We could also use\ngetpwuid(getuid()) instead of getenv(\"HOME\") to be consistent with\nuser_path() but this is simpler and arguably more likely what the\nuser wants when it matters.\n\n\n>From 6eb18f8ade791521bdad955e1da2b40399a426f0 Mon Sep 17 00:00:00 2001\nFrom: Karl Chen \nDate: Thu, 21 Aug 2008 21:00:26 -0700\nSubject: [PATCH] Support \"core.excludesfile = ~/.gitignore\"\n\nThe config variable core.excludesfile is parsed to substitute leading \"~/\"\nwith getenv(\"HOME\").\n\nSigned-off-by: Karl Chen \n\n---\n config.c | 20 ++++++++++++++++++--\n 1 files changed, 18 insertions(+), 2 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 53f04a0..41061d2 100644\n--- a/config.c\n+++ b/config.c\n@@ -334,6 +334,18 @@ int git_config_string(const char **dest, const char *var, const char *value)\n \treturn 0;\n }\n \n+static char const *git_config_subst_userdir(char const *value) {\n+\tif (value[0] == '~' && value[1] == '/') {\n+\t\tconst char *home = getenv(\"HOME\");\n+\t\tchar *userdir_excludes_file = malloc(strlen(home) + strlen(value)-1 + 1);\n+\t\tstrcpy(userdir_excludes_file, home);\n+\t\tstrcat(userdir_excludes_file, value+1);\n+\t\treturn userdir_excludes_file;\n+\t} else {\n+\t\treturn xstrdup(value);\n+\t}\n+}\n+\n static int git_default_core_config(const char *var, const char *value)\n {\n \t/* This needs a better name */\n@@ -456,8 +468,12 @@ static int git_default_core_config(const char *var, const char *value)\n \tif (!strcmp(var, \"core.editor\"))\n \t\treturn git_config_string(&editor_program, var, value);\n \n-\tif (!strcmp(var, \"core.excludesfile\"))\n-\t\treturn git_config_string(&excludes_file, var, value);\n+\tif (!strcmp(var, \"core.excludesfile\")) {\n+\t\tif (!value)\n+\t\t\treturn config_error_nonbool(var);\n+\t\texcludes_file = git_config_subst_userdir(value);\n+\t\treturn 0;\n+\t}\n \n \tif (!strcmp(var, \"core.whitespace\")) {\n \t\tif (!value)\n-- \n1.5.6.2\n"},{"id":"88160","messageId":"loom.20080822T165656-932@post.gmane.org","threadId":"15152","inReplyTo":"quack.20080821T2114.lthvdxtvg7b@roar.cs.berkeley.edu","subject":"Re: [PATCH] Support \"core.excludesfile = ~/.gitignore\"","fromName":"Eric Raible","fromEmail":"raible@gmail.com","sentAt":"2008-08-22T16:58:11Z","receivedAt":"2008-08-22T16:58:11Z","isPatch":true,"sender":{"key":"raible@gmail.com","avatar":null},"body":"Karl Chen cs.berkeley.edu> writes:\n\n> +static char const *git_config_subst_userdir(char const *value) {\n> +\tif (value[0] == '~' && value[1] == '/') {\n\nMight you want to check that strlen(value) is at least 2?\n\n- Eric\n"},{"id":"88168","messageId":"36ca99e90808221056i6d79b122occ4ae1e21da6a221@mail.gmail.com","threadId":"15152","inReplyTo":"loom.20080822T165656-932@post.gmane.org","subject":"Re: [PATCH] Support \"core.excludesfile = ~/.gitignore\"","fromName":"Bert Wesarg","fromEmail":"bert.wesarg@googlemail.com","sentAt":"2008-08-22T17:56:22Z","receivedAt":"2008-08-22T17:56:22Z","isPatch":true,"sender":{"key":"bert.wesarg@googlemail.com","avatar":"https://avatars.githubusercontent.com/u/111934?v=4"},"body":"On Fri, Aug 22, 2008 at 18:58, Eric Raible wrote:\n> Karl Chen cs.berkeley.edu> writes:\n>\n>> +static char const *git_config_subst_userdir(char const *value) {\n>> + if (value[0] == '~' && value[1] == '/') {\n>\n> Might you want to check that strlen(value) is at least 2?\nNo.\n\n swtich (strlen(value)) {\n case 0:\n /* value[0] == '\\0' => value[0] != '~'\n value[1] will never be dereferenced, because of lazy && */\n case 1:\n /* value[0] != '\\0'\n if (value[0] == '~')\n value[1] == '\\0' => value[1] != '/' */\n default:\n /* ... */\n }\n\nSo no invalid memory dereferences.\n\nRegards\nBert\n>\n> - Eric\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n>\n"},{"id":"88198","messageId":"7vsksw92nh.fsf@gitster.siamese.dyndns.org","threadId":"15152","inReplyTo":"quack.20080821T2114.lthvdxtvg7b@roar.cs.berkeley.edu","subject":"Re: [PATCH] Support \"core.excludesfile = ~/.gitignore\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-22T21:10:42Z","receivedAt":"2008-08-22T21:10:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karl Chen writes:\n\n> Another idea is to have a non-absolute path be interpreted\n> relative to the location of .gitconfig.\n\nIf we were to support relative paths, I think it would be useful and\nconsistent if a relative path found in \".git/config\" is relative to the\nwork tree root, in \"config\" in a bare repository relative to the bare\nrepository, and in \"$HOME/.gitconfig\" relative to $HOME. I am not sure\nwhat a relative path in \"/etc/gitconfig\" should be relative to, though.\n\nHowever, this has a technical difficulty. When configuration values are\nread, the code that knows what the value means does not in general know\nwhich configuration file is being read from.\n\n> Below is a sample patch that works for me. We could also use\n> getpwuid(getuid()) instead of getenv(\"HOME\") to be consistent with\n> user_path() but this is simpler and arguably more likely what the\n> user wants when it matters.\n\nIt is quite likely that somebody would want you to interpret \"~name/\" if\nyou advertize that you support \"~/\", so you would need to call getpwuid()\neventually if you go down this path. I wonder how this would affect\nWindows folks.\n\nWhat are the paths valued configuration variables other than excludesfile\nthat we would want to support? There was a topic to allow mail-aliases\nlookup for parameters given to the \"--author\" option today, and send-email\ntakes aliasfile configuration. Because the latter is a script, we would\nneed a \"--path\" option to \"git config\" (the idea is similar to existing\n\"--bool\" option) so that calling scripts can ask the same \"magic\"\nperformed to configuration variables' values before being reported.\n"},{"id":"88353","messageId":"quack.20080824T0140.lth3aku956e@roar.cs.berkeley.edu","threadId":"15152","inReplyTo":"7vsksw92nh.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Support \"core.excludesfile = ~/.gitignore\"","fromName":"Karl Chen","fromEmail":"quarl@cs.berkeley.edu","sentAt":"2008-08-24T08:40:41Z","receivedAt":"2008-08-24T08:40:41Z","isPatch":true,"sender":{"key":"quarl@cs.berkeley.edu","avatar":null},"body":">>>>> On 2008-08-22 14:10 PDT, Junio C Hamano writes:\n\n Junio> Karl Chen writes:\n >> Another idea is to have a non-absolute path be interpreted\n >> relative to the location of .gitconfig.\n\n Junio> If we were to support relative paths, I think it would\n Junio> be useful and consistent if a relative path found in\n Junio> \".git/config\" is relative to the work tree root, in\n Junio> \"config\" in a bare repository relative to the bare\n Junio> repository, and in \"$HOME/.gitconfig\" relative to\n Junio> $HOME.\n\nMakes sense to support it everywhere. For .git/config, isn't it\nmore consistent for it to be relative to .git?\n\n Junio> I am not sure what a relative path in \"/etc/gitconfig\"\n Junio> should be relative to, though.\n\nWhy not just relative to the location of that file? Normally\n/etc, but if some distro customizes the location of /etc/gitconfig\n(/etc/git/config), or on non-Linux/posix systems it's somewhere\nelse, or git is installed in /usr/local or /opt or $HOME, then\nit's still relative to the location of system gitconfig.\n\n Junio> However, this has a technical difficulty. When\n Junio> configuration values are read, the code that knows what\n Junio> the value means does not in general know which\n Junio> configuration file is being read from.\n\nSounds like a refactoring issue.\n\n Junio> It is quite likely that somebody would want you to\n Junio> interpret \"~name/\" if you advertize that you support\n Junio> \"~/\", so you would need to call getpwuid() eventually\n Junio> if you go down this path. I wonder how this would\n Junio> affect Windows folks.\n\nI would be happy either way. Though since git uses getenv(\"HOME\")\nto find ~/.gitconfig, I can see arguments for looking for the\nignore file there also, in case it's different.\n\n Junio> we would need a \"--path\" option to \"git config\" (the\n Junio> idea is similar to existing \"--bool\" option) so that\n Junio> calling scripts can ask the same \"magic\" performed to\n Junio> configuration variables' values before being reported.\n\nSounds fine.\n\nSo, being new to git development, am I correctly assessing your\nresponse as \"with refinement this can be included in git\"?\n\nRelative paths and ~ (and $HOME) are all mutually compatible so\nthey could all be implemented. If $HOME were supported directly\n(either just \"$HOME\" or parsing all $ENVVARS) then it'd be easier\nto decide to use getpwuid for ~. Personally I'd use: 1) relative\npath, 2) $HOME (as \"~\" or \"$HOME\"), 3) getpwuid (as \"~\")\n\nKarl\n"},{"id":"88374","messageId":"7vprnyqo59.fsf@gitster.siamese.dyndns.org","threadId":"15152","inReplyTo":"quack.20080824T0140.lth3aku956e@roar.cs.berkeley.edu","subject":"Re: [PATCH] Support \"core.excludesfile = ~/.gitignore\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-24T18:11:14Z","receivedAt":"2008-08-24T18:11:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karl Chen writes:\n\n>>>>>> On 2008-08-22 14:10 PDT, Junio C Hamano writes:\n>\n> Junio> If we were to support relative paths, I think it would\n> Junio> be useful and consistent if a relative path found in\n> Junio> \".git/config\" is relative to the work tree root, in\n> Junio> \"config\" in a bare repository relative to the bare\n> Junio> repository, and in \"$HOME/.gitconfig\" relative to\n> Junio> $HOME.\n>\n> Makes sense to support it everywhere. For .git/config, isn't it\n> more consistent for it to be relative to .git?\n\nConsistency and usefulness are different things. Suppose you want as the\nupstream of your project maintain and distribute a mail-alias list in-tree\n(say, the file is at the root level, CONTRIBUTORS), and you suggest\ncontributors to use it when using \"commit --author\".\n\nWhich one do you want to write in your README:\n\n\t[user]\n \tnicknamelistfile = ../CONTRIBUTORS\n\nor\n\n\t[user]\n \tnicknamelistfile = CONTRIBUTORS\n\nYou have to say the former if it is relative to .git/config.\n\n> So, being new to git development, am I correctly assessing your\n> response as \"with refinement this can be included in git\"?\n\nI do not have fundamental objection to what you are trying to achieve\n(i.e. being able to say \"relative to $HOME\"). I personally think the\napproach you took in your patch (i.e. only support \"~/\" and use $HOME,\nwithout any other fancy stuff) is a sensible first cut for that issue.\n\nI just pointed out possible design issues about the future direction after\nthat first cut. When I make comments on design-level issues, I rarely\nread the patch itself very carefully, so it is a different issue if your\nparticular implementation in the patch is the best implementation of that\nfirst cut approach.\n"},{"id":"88402","messageId":"20080824220854.GA27299@coredump.intra.peff.net","threadId":"15152","inReplyTo":"7vprnyqo59.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Support \"core.excludesfile = ~/.gitignore\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-08-24T22:08:54Z","receivedAt":"2008-08-24T22:08:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Aug 24, 2008 at 11:11:14AM -0700, Junio C Hamano wrote:\n\n> Consistency and usefulness are different things. Suppose you want as the\n> upstream of your project maintain and distribute a mail-alias list in-tree\n> (say, the file is at the root level, CONTRIBUTORS), and you suggest\n> contributors to use it when using \"commit --author\".\n> \n> Which one do you want to write in your README:\n> \n> \t[user]\n> \tnicknamelistfile = ../CONTRIBUTORS\n> \n> or\n> \n> \t[user]\n> \tnicknamelistfile = CONTRIBUTORS\n> \n> You have to say the former if it is relative to .git/config.\n\nCouldn't the exact opposite argument be made for \"suppose you want to\nput the mail-alias file in a repo-specific directory that was not\ntracked?\" I.e., you are trading off \"CONTRIBUTORS\" against\n\".git/CONTRIBUTORS\". So which one inconveniences the smallest number of\npeople is really a question of what people want to do with such pointers\n(and since we don't support any yet, we don't really know...).\n\nBut more worrisome to me is that the working directory and git directory\ndo not necessarily follow a \"../\" and \".git/\" relationship. How would\nyou resolve \"../foo\" with:\n\n GIT_DIR=/path/to/other/place git ...\n\nor\n\n git --work-tree=/path/to/other/place\n\n?\n\nIf you want to be able to point to either, I suspect we are better off\nsimply introducing some basic substitutions like $GIT_DIR and\n$GIT_WORK_TREE. Maybe even just allow environment variable expansion,\nand then promise to set those variables, which takes care of $HOME\nautomagically.\n\n-Peff\n"},{"id":"88405","messageId":"7vzln2j9y2.fsf@gitster.siamese.dyndns.org","threadId":"15152","inReplyTo":"20080824220854.GA27299@coredump.intra.peff.net","subject":"Re: [PATCH] Support \"core.excludesfile = ~/.gitignore\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-24T22:59:49Z","receivedAt":"2008-08-24T22:59:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Sun, Aug 24, 2008 at 11:11:14AM -0700, Junio C Hamano wrote:\n>\n>> Consistency and usefulness are different things. Suppose you want as the\n>> upstream of your project maintain and distribute a mail-alias list in-tree\n>> (say, the file is at the root level, CONTRIBUTORS), and you suggest\n>> contributors to use it when using \"commit --author\".\n>> \n>> Which one do you want to write in your README:\n>> \n>> \t[user]\n>> \tnicknamelistfile = ../CONTRIBUTORS\n>> \n>> or\n>> \n>> \t[user]\n>> \tnicknamelistfile = CONTRIBUTORS\n>> \n>> You have to say the former if it is relative to .git/config.\n>\n> Couldn't the exact opposite argument be made for \"suppose you want to\n> put the mail-alias file in a repo-specific directory that was not\n> tracked?\" I.e., you are trading off \"CONTRIBUTORS\" against\n> \".git/CONTRIBUTORS\".\n\nNo, I couldn't ;-)\n\nWhy would you write what you wrote in README?\n\nAnything you store in .git is not propagated, so the instruction would not\nlikely to be \"store it in .git/CONTRIBUTORS and point at it\". There is no\nmerit in forcing users to standardize on \"in .git\". The instruction would\nbe to \"store it anywhere you want, and point at it\".\n\nThe example I gave is very different. It points at an in-tree thing, and\nanybody who has worktree checked out will have it _at the location I as\nthe README writer expect it to be_. That is the difference that makes the\nexact opposite argument much weaker.\n\nThat's why I suggested \"relative to work tree if in .git/config, or gitdir\nif in config of a bare repository\", although honestly speaking I do not\nhave very strong preference either way.\n\n> If you want to be able to point to either, I suspect we are better off\n> simply introducing some basic substitutions like $GIT_DIR and\n> $GIT_WORK_TREE. Maybe even just allow environment variable expansion,\n> and then promise to set those variables, which takes care of $HOME\n> automagically.\n\nBecause we haven't deprecated core.worktree (or $GIT_WORK_TREE) yet, your\nsuggestion has an obvious chicken-and-egg problem, even though otherwise I\nthink it makes perfect sense and very much like it.\n\nPerhaps we should rid of the worktree that is separate and floats\nunrelated to where $GIT_DIR is.\n"},{"id":"88406","messageId":"20080824231343.GC27619@coredump.intra.peff.net","threadId":"15152","inReplyTo":"7vzln2j9y2.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Support \"core.excludesfile = ~/.gitignore\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-08-24T23:13:43Z","receivedAt":"2008-08-24T23:13:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Aug 24, 2008 at 03:59:49PM -0700, Junio C Hamano wrote:\n\n> > Couldn't the exact opposite argument be made for \"suppose you want to\n> > put the mail-alias file in a repo-specific directory that was not\n> > tracked?\" I.e., you are trading off \"CONTRIBUTORS\" against\n> > \".git/CONTRIBUTORS\".\n> \n> No, I couldn't ;-)\n> \n> Why would you write what you wrote in README?\n> \n> Anything you store in .git is not propagated, so the instruction would not\n> likely to be \"store it in .git/CONTRIBUTORS and point at it\". There is no\n> merit in forcing users to standardize on \"in .git\". The instruction would\n> be to \"store it anywhere you want, and point at it\".\n\nAh, right.\n\nI still think there is a little bit of convenience when you are doing\nsomething totally personal (i.e., not putting it in a README, but rather\njust wanting to store the referenced file in .git for the sake of\nsimplicity). But in that case, it is only slightly less convenient to\njust point to the full path. So your example trumps this, since you have\nno sane way of knowing the full path in README instructions.\n\n> Because we haven't deprecated core.worktree (or $GIT_WORK_TREE) yet, your\n> suggestion has an obvious chicken-and-egg problem, even though otherwise I\n> think it makes perfect sense and very much like it.\n\nYou might be able to get around that by lazily filling in the variable.\nIOW, expand it at the point-of-use rather than while reading the config.\nHowever, the point of use might easily have something to do with reading\nconfig, so that re-creates the cycle.\n\nIn general, I think we treat config as order-independent. We could make\nthe use of such variables order-dependent (i.e., if you haven't set\ncore.worktree, then we give you the value without having set it, and we\nrecalculate later. Confusing results, but at least a simple rule to\nunderstand).\n\nI think there actually are a few other order-dependent things in the\nconfig, like the order of multi-value keys like push and fetch refspecs.\n\nWould you want this expansion only for specially marked variables, or\nfor all variables? I like the concept of general templates for config\nvalues, but it will backwards compatibility, especially for alias.*.\n\n> Perhaps we should rid of the worktree that is separate and floats\n> unrelated to where $GIT_DIR is.\n\nI assumed people were actually using it, which is why it was\nimplemented.\n\n-Peff\n"},{"id":"88407","messageId":"7vhc9aj82i.fsf@gitster.siamese.dyndns.org","threadId":"15152","inReplyTo":"20080824231343.GC27619@coredump.intra.peff.net","subject":"Re: [PATCH] Support \"core.excludesfile = ~/.gitignore\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-24T23:40:21Z","receivedAt":"2008-08-24T23:40:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> Perhaps we should rid of the worktree that is separate and floats\n>> unrelated to where $GIT_DIR is.\n>\n> I assumed people were actually using it, which is why it was\n> implemented.\n\nJudging from the occasional \"I tried core.worktree but it does not work in\nthis and that situations\" I see here and on #git, my impression is that\nnew people try it, saying \"git is cool -- unlike cvs that sprinkles those\nugly CVS directories all over the place, it only contaminates my work tree\nwith a single directory '.git' and nothing else. Ah, wait --- what's this\ncore.worktree thing? Can I get rid of that last one as well? That sounds\neven cooler\".\n\nIOW, I do not think it is really _needed_ per-se as a feature, but it was\ndone because it was thought to be doable, which unfortunately turned out\nto involve hair-pulling complexity that the two attempts that led to the\ncurrent code still haven't resolved.\n\nI really wish we do not have to worry about that anymore.\n"},{"id":"88408","messageId":"20080824235124.GA28248@coredump.intra.peff.net","threadId":"15152","inReplyTo":"7vhc9aj82i.fsf@gitster.siamese.dyndns.org","subject":"limiting relationship of git dir and worktree (was Re: [PATCH] Support \"core.excludesfile = ~/.gitignore\")","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-08-24T23:51:25Z","receivedAt":"2008-08-24T23:51:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Aug 24, 2008 at 04:40:21PM -0700, Junio C Hamano wrote:\n\n> Judging from the occasional \"I tried core.worktree but it does not work in\n> this and that situations\" I see here and on #git, my impression is that\n> new people try it, saying \"git is cool -- unlike cvs that sprinkles those\n> ugly CVS directories all over the place, it only contaminates my work tree\n> with a single directory '.git' and nothing else. Ah, wait --- what's this\n> core.worktree thing? Can I get rid of that last one as well? That sounds\n> even cooler\".\n> \n> IOW, I do not think it is really _needed_ per-se as a feature, but it was\n> done because it was thought to be doable, which unfortunately turned out\n> to involve hair-pulling complexity that the two attempts that led to the\n> current code still haven't resolved.\n> \n> I really wish we do not have to worry about that anymore.\n\nWell, as a non-user of this feature, I certainly have no argument\nagainst taking it out. Maybe the subject line will pull some other\npeople into the discussion.\n\n-Peff\n"},{"id":"88411","messageId":"7v7ia6j5q9.fsf_-_@gitster.siamese.dyndns.org","threadId":"15152","inReplyTo":"20080824235124.GA28248@coredump.intra.peff.net","subject":"Dropping core.worktree and GIT_WORK_TREE support (was Re: limiting relationship of git dir and worktree)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-25T00:30:54Z","receivedAt":"2008-08-25T00:30:54Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Sun, Aug 24, 2008 at 04:40:21PM -0700, Junio C Hamano wrote:\n>\n>> Judging from the occasional \"I tried core.worktree but it does not work in\n>> this and that situations\" I see here and on #git, my impression is that\n>> new people try it, saying \"git is cool -- unlike cvs that sprinkles those\n>> ugly CVS directories all over the place, it only contaminates my work tree\n>> with a single directory '.git' and nothing else. Ah, wait --- what's this\n>> core.worktree thing? Can I get rid of that last one as well? That sounds\n>> even cooler\".\n>> \n>> IOW, I do not think it is really _needed_ per-se as a feature, but it was\n>> done because it was thought to be doable, which unfortunately turned out\n>> to involve hair-pulling complexity that the two attempts that led to the\n>> current code still haven't resolved.\n>> \n>> I really wish we do not have to worry about that anymore.\n>\n> Well, as a non-user of this feature, I certainly have no argument\n> against taking it out. Maybe the subject line will pull some other\n> people into the discussion.\n\nHeh, if we are to do the attention-getter, let's do so more strongly ;-)\n"},{"id":"88417","messageId":"20080825020054.GP23800@genesis.frugalware.org","threadId":"15152","inReplyTo":"7v7ia6j5q9.fsf_-_@gitster.siamese.dyndns.org","subject":"Re: Dropping core.worktree and GIT_WORK_TREE support (was Re: limiting relationship of git dir and worktree)","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2008-08-25T02:00:54Z","receivedAt":"2008-08-25T02:00:54Z","isPatch":false,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"On Sun, Aug 24, 2008 at 05:30:54PM -0700, Junio C Hamano wrote:\n> Heh, if we are to do the attention-getter, let's do so more strongly ;-)\n\nDoes this include removing of --work-tree as well?\n\nThe git backend of Pootle (http://translate.sourceforge.net/wiki/) uses\nit.\n\nAlso, here is a question:\n\n$ git --git-dir git/.git --work-tree git diff --stat|tail -n 1\n 1443 files changed, 0 insertions(+), 299668 deletions(-)\n\nSo, it's like it thinks every file is removed.\n\nBut then:\n\n$ cd git\n$ git diff --stat|wc -l\n0\n\nis this a bug, or a user error?\n\nThanks.\n"},{"id":"88421","messageId":"7v1w0dkd5s.fsf@gitster.siamese.dyndns.org","threadId":"15152","inReplyTo":"20080825020054.GP23800@genesis.frugalware.org","subject":"Re: Dropping core.worktree and GIT_WORK_TREE support","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-25T03:05:03Z","receivedAt":"2008-08-25T03:05:03Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Miklos Vajna writes:\n\n> On Sun, Aug 24, 2008 at 05:30:54PM -0700, Junio C Hamano wrote:\n>> Heh, if we are to do the attention-getter, let's do so more strongly ;-)\n>\n> Does this include removing of --work-tree as well?\n>\n> The git backend of Pootle (http://translate.sourceforge.net/wiki/) uses\n> it.\n\nInteresting. Does it use it because it can (meaning, --work-tree is\nsupposed to work), or because --work-tree is the cleanest way to do what\nit wants to do (if the feature worked properly, that is, which is not the\ncase)?\n\n> Also, here is a question:\n>\n> $ git --git-dir git/.git --work-tree git diff --stat|tail -n 1\n> 1443 files changed, 0 insertions(+), 299668 deletions(-)\n>\n> So, it's like it thinks every file is removed.\n>\n> But then:\n>\n> $ cd git\n> $ git diff --stat|wc -l\n> 0\n>\n> is this a bug, or a user error?\n\nI think it is among the many other things that falls into \"the two\nattempts still haven't resolved\" category.\n"},{"id":"88450","messageId":"20080825125205.GB23800@genesis.frugalware.org","threadId":"15152","inReplyTo":"7v1w0dkd5s.fsf@gitster.siamese.dyndns.org","subject":"Re: Dropping core.worktree and GIT_WORK_TREE support","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2008-08-25T12:52:05Z","receivedAt":"2008-08-25T12:52:05Z","isPatch":false,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"On Sun, Aug 24, 2008 at 08:05:03PM -0700, Junio C Hamano wrote:\n> > Does this include removing of --work-tree as well?\n> >\n> > The git backend of Pootle (http://translate.sourceforge.net/wiki/) uses\n> > it.\n> \n> Interesting. Does it use it because it can (meaning, --work-tree is\n> supposed to work), or because --work-tree is the cleanest way to do what\n> it wants to do (if the feature worked properly, that is, which is not the\n> case)?\n\nIt's like:\n\nThe current working directory is like\n/usr/lib/python2.5/site-packages/Pootle. The git repository is under\n/some/other/path/outside/usr.\n\nThen Pootle has two possibilities:\n\n1) save the current directory, change to /some/other, execute git, and\nchange the directory back\n\n2) use git --work-tree / --git-dir\n\nI guess the second form is more elegant. Of course if it is decided that\nthis option will be removed then the old form can be still used, but I\nthink that would be a step back.\n\n> > Also, here is a question:\n> >\n> > $ git --git-dir git/.git --work-tree git diff --stat|tail -n 1\n> > 1443 files changed, 0 insertions(+), 299668 deletions(-)\n> >\n> > So, it's like it thinks every file is removed.\n> >\n> > But then:\n> >\n> > $ cd git\n> > $ git diff --stat|wc -l\n> > 0\n> >\n> > is this a bug, or a user error?\n> \n> I think it is among the many other things that falls into \"the two\n> attempts still haven't resolved\" category.\n\nI'm unfamiliar with this part of the codebase, so in case somebody other\ncould look at it, that would be great, but I'm happy with write a\ntestcase for it. (Or in case nobody cares, I can try to fix it, but that\nmay take a bit more time.)\n"},{"id":"88456","messageId":"fcaeb9bf0808250652p3d0f483dt714cd68d3122d7c9@mail.gmail.com","threadId":"15152","inReplyTo":"20080825125205.GB23800@genesis.frugalware.org","subject":"Re: Dropping core.worktree and GIT_WORK_TREE support","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2008-08-25T13:52:11Z","receivedAt":"2008-08-25T13:52:11Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On 8/25/08, Miklos Vajna wrote:\n> On Sun, Aug 24, 2008 at 08:05:03PM -0700, Junio C Hamano wrote:\n> > > Does this include removing of --work-tree as well?\n> > >\n> > > The git backend of Pootle (http://translate.sourceforge.net/wiki/) uses\n> > > it.\n> >\n> > Interesting. Does it use it because it can (meaning, --work-tree is\n> > supposed to work), or because --work-tree is the cleanest way to do what\n> > it wants to do (if the feature worked properly, that is, which is not the\n> > case)?\n>\n>\n> It's like:\n>\n> The current working directory is like\n> /usr/lib/python2.5/site-packages/Pootle. The git repository is under\n> /some/other/path/outside/usr.\n>\n> Then Pootle has two possibilities:\n>\n> 1) save the current directory, change to /some/other, execute git, and\n> change the directory back\n>\n> 2) use git --work-tree / --git-dir\n>\n> I guess the second form is more elegant. Of course if it is decided that\n> this option will be removed then the old form can be still used, but I\n> think that would be a step back.\n>\n>\n> > > Also, here is a question:\n> > >\n> > > $ git --git-dir git/.git --work-tree git diff --stat|tail -n 1\n> > > 1443 files changed, 0 insertions(+), 299668 deletions(-)\n> > >\n> > > So, it's like it thinks every file is removed.\n> > >\n> > > But then:\n> > >\n> > > $ cd git\n> > > $ git diff --stat|wc -l\n> > > 0\n> > >\n> > > is this a bug, or a user error?\n> >\n> > I think it is among the many other things that falls into \"the two\n> > attempts still haven't resolved\" category.\n>\n>\n> I'm unfamiliar with this part of the codebase, so in case somebody other\n> could look at it, that would be great, but I'm happy with write a\n> testcase for it. (Or in case nobody cares, I can try to fix it, but that\n> may take a bit more time.)\n\nBecause \"git diff\" did not call setup_work_tree(). The same happens\nfor \"git diff-index\" that someone reported recently. IIRC \"git\ndiff-files\" has the same problem.\n-- \nDuy\n"},{"id":"88462","messageId":"1219675383-1717-1-git-send-email-vmiklos@frugalware.org","threadId":"15152","inReplyTo":"fcaeb9bf0808250652p3d0f483dt714cd68d3122d7c9@mail.gmail.com","subject":"[PATCH] git diff/diff-index/diff-files: call setup_work_tree()","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2008-08-25T14:43:03Z","receivedAt":"2008-08-25T14:43:03Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"This makes it possible to use git diff when we are outside the repo but\n--work-tree and --git-dir is used.\n\nSigned-off-by: Miklos Vajna \n---\n\nOn Mon, Aug 25, 2008 at 08:52:11PM +0700, Nguyen Thai Ngoc Duy wrote:\n> Because \"git diff\" did not call setup_work_tree(). The same happens\n> for \"git diff-index\" that someone reported recently. IIRC \"git\n> diff-files\" has the same problem.\n\nThanks, that was the problem.\n\n builtin-diff-files.c | 1 +\n builtin-diff-index.c | 1 +\n builtin-diff.c | 1 +\n 3 files changed, 3 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin-diff-files.c b/builtin-diff-files.c\nindex 9bf10bb..4802e00 100644\n--- a/builtin-diff-files.c\n+++ b/builtin-diff-files.c\n@@ -19,6 +19,7 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)\n \tint result;\n \tunsigned options = 0;\n \n+\tsetup_work_tree();\n \tinit_revisions(&rev, prefix);\n \tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n \trev.abbrev = 0;\ndiff --git a/builtin-diff-index.c b/builtin-diff-index.c\nindex 17d851b..b8e0656 100644\n--- a/builtin-diff-index.c\n+++ b/builtin-diff-index.c\n@@ -16,6 +16,7 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)\n \tint i;\n \tint result;\n \n+\tsetup_work_tree();\n \tinit_revisions(&rev, prefix);\n \tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n \trev.abbrev = 0;\ndiff --git a/builtin-diff.c b/builtin-diff.c\nindex 7ffea97..86f9255 100644\n--- a/builtin-diff.c\n+++ b/builtin-diff.c\n@@ -244,6 +244,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \tint nongit;\n \tint result = 0;\n \n+\tsetup_work_tree();\n \t/*\n \t * We could get N tree-ish in the rev.pending_objects list.\n \t * Also there could be M blobs there, and P pathspecs.\n-- \n1.6.0.rc3.17.gc14c8.dirty\n"},{"id":"88464","messageId":"fcaeb9bf0808250746q3366b9cap78c45718287fba80@mail.gmail.com","threadId":"15152","inReplyTo":"1219675383-1717-1-git-send-email-vmiklos@frugalware.org","subject":"Re: [PATCH] git diff/diff-index/diff-files: call setup_work_tree()","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2008-08-25T14:46:37Z","receivedAt":"2008-08-25T14:46:37Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On 8/25/08, Miklos Vajna wrote:\n> diff --git a/builtin-diff-index.c b/builtin-diff-index.c\n> index 17d851b..b8e0656 100644\n> --- a/builtin-diff-index.c\n> +++ b/builtin-diff-index.c\n> @@ -16,6 +16,7 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)\n> int i;\n> int result;\n>\n> + setup_work_tree();\n> init_revisions(&rev, prefix);\n> git_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n> rev.abbrev = 0;\n\nI think this is only needed when cached == 0\n\n> diff --git a/builtin-diff.c b/builtin-diff.c\n> index 7ffea97..86f9255 100644\n> --- a/builtin-diff.c\n> +++ b/builtin-diff.c\n> @@ -244,6 +244,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n> int nongit;\n> int result = 0;\n>\n> + setup_work_tree();\n> /*\n> * We could get N tree-ish in the rev.pending_objects list.\n> * Also there could be M blobs there, and P pathspecs.\n>\n\nNo. git-diff has too many modes, some does not need worktree. This\nforces worktree on all modes.\n-- \nDuy\n"},{"id":"88465","messageId":"20080825145044.GE23800@genesis.frugalware.org","threadId":"15152","inReplyTo":"fcaeb9bf0808250746q3366b9cap78c45718287fba80@mail.gmail.com","subject":"Re: [PATCH] git diff/diff-index/diff-files: call setup_work_tree()","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2008-08-25T14:50:44Z","receivedAt":"2008-08-25T14:50:44Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"On Mon, Aug 25, 2008 at 09:46:37PM +0700, Nguyen Thai Ngoc Duy wrote:\n> On 8/25/08, Miklos Vajna wrote:\n> > diff --git a/builtin-diff-index.c b/builtin-diff-index.c\n> > index 17d851b..b8e0656 100644\n> > --- a/builtin-diff-index.c\n> > +++ b/builtin-diff-index.c\n> > @@ -16,6 +16,7 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)\n> > int i;\n> > int result;\n> >\n> > + setup_work_tree();\n> > init_revisions(&rev, prefix);\n> > git_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n> > rev.abbrev = 0;\n> \n> I think this is only needed when cached == 0\n> \n> > diff --git a/builtin-diff.c b/builtin-diff.c\n> > index 7ffea97..86f9255 100644\n> > --- a/builtin-diff.c\n> > +++ b/builtin-diff.c\n> > @@ -244,6 +244,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n> > int nongit;\n> > int result = 0;\n> >\n> > + setup_work_tree();\n> > /*\n> > * We could get N tree-ish in the rev.pending_objects list.\n> > * Also there could be M blobs there, and P pathspecs.\n> >\n> \n> No. git-diff has too many modes, some does not need worktree. This\n> forces worktree on all modes.\n\nAh, yes. I just wanted to say that I forgot do a 'make test' and\nactually this breaks at least t0020-crlf.sh. I'll post a fixed patch in\na bit.\n\nSorry.\n"},{"id":"88466","messageId":"1219677095-21732-1-git-send-email-vmiklos@frugalware.org","threadId":"15152","inReplyTo":"20080825145044.GE23800@genesis.frugalware.org","subject":"[PATCH] git diff/diff-index/diff-files: call setup_work_tree()","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2008-08-25T15:11:35Z","receivedAt":"2008-08-25T15:11:35Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"This makes it possible to use git diff when we are outside the repo but\n--work-tree and --git-dir is used.\n\nSigned-off-by: Miklos Vajna \n---\n builtin-diff-files.c | 1 +\n builtin-diff-index.c | 2 ++\n builtin-diff.c | 1 +\n 3 files changed, 4 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin-diff-files.c b/builtin-diff-files.c\nindex 9bf10bb..4802e00 100644\n--- a/builtin-diff-files.c\n+++ b/builtin-diff-files.c\n@@ -19,6 +19,7 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)\n \tint result;\n \tunsigned options = 0;\n \n+\tsetup_work_tree();\n \tinit_revisions(&rev, prefix);\n \tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n \trev.abbrev = 0;\ndiff --git a/builtin-diff-index.c b/builtin-diff-index.c\nindex 17d851b..5510291 100644\n--- a/builtin-diff-index.c\n+++ b/builtin-diff-index.c\n@@ -29,6 +29,8 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)\n \t\telse\n \t\t\tusage(diff_cache_usage);\n \t}\n+\tif (!cached)\n+\t\tsetup_work_tree();\n \tif (!rev.diffopt.output_format)\n \t\trev.diffopt.output_format = DIFF_FORMAT_RAW;\n \ndiff --git a/builtin-diff.c b/builtin-diff.c\nindex 7ffea97..57da6ed 100644\n--- a/builtin-diff.c\n+++ b/builtin-diff.c\n@@ -279,6 +279,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \tdiff_no_index(&rev, argc, argv, nongit, prefix);\n \n \t/* Otherwise, we are doing the usual \"git\" diff */\n+\tsetup_work_tree();\n \trev.diffopt.skip_stat_unmatch = !!diff_auto_refresh_index;\n \n \tif (nongit)\n-- \n1.6.0.rc3.17.gc14c8.dirty\n"},{"id":"88467","messageId":"fcaeb9bf0808250826l2f1a0f3l94fff1b702e69c5d@mail.gmail.com","threadId":"15152","inReplyTo":"1219677095-21732-1-git-send-email-vmiklos@frugalware.org","subject":"Re: [PATCH] git diff/diff-index/diff-files: call setup_work_tree()","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2008-08-25T15:26:07Z","receivedAt":"2008-08-25T15:26:07Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On 8/25/08, Miklos Vajna wrote:\n> diff --git a/builtin-diff.c b/builtin-diff.c\n>\n> index 7ffea97..57da6ed 100644\n>\n> --- a/builtin-diff.c\n> +++ b/builtin-diff.c\n>\n> @@ -279,6 +279,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n> diff_no_index(&rev, argc, argv, nongit, prefix);\n>\n> /* Otherwise, we are doing the usual \"git\" diff */\n> + setup_work_tree();\n> rev.diffopt.skip_stat_unmatch = !!diff_auto_refresh_index;\n>\n> if (nongit)\n\nAt least builtin_diff_blobs() and builtin_diff_tree() won't need\nworktree, so NACK again. Anyway I'm not familiar with diff*. Junio\nshould know these better.\n\n-- \nDuy\n"},{"id":"88489","messageId":"quack.20080825T1207.lthk5e46hi4_-_@roar.cs.berkeley.edu","threadId":"15152","inReplyTo":"7vhc9aj82i.fsf@gitster.siamese.dyndns.org","subject":"[PATCH v2] Support \"core.excludesfile = ~/.gitignore\"","fromName":"Karl Chen","fromEmail":"quarl@cs.berkeley.edu","sentAt":"2008-08-25T19:07:15Z","receivedAt":"2008-08-25T19:07:15Z","isPatch":true,"sender":{"key":"quarl@cs.berkeley.edu","avatar":null},"body":"\nThe config variable core.excludesfile is parsed to substitute ~ and ~user with\ngetpw entries.\n\nSigned-off-by: Karl Chen \n---\n config.c | 41 +++++++++++++++++++++++++++++++++++++++--\n 1 files changed, 39 insertions(+), 2 deletions(-)\n\n\nBased on the discussion it sounds like there are complications to\nsupporting relative paths (due to worktree config), and \"$HOME\"\n(when generalized, due to bootstrapping issues with $GIT_*).\n\nSince ~ and ~user are orthogonal to these, can I suggest going\nforward with this, without blocking on those two?\n\nI have reworked the patch to use getpw to support ~user. $HOME\ncan eventually be supported via $ENVVARs.\n\n\ndiff --git a/config.c b/config.c\nindex 53f04a0..6a83c64 100644\n--- a/config.c\n+++ b/config.c\n@@ -334,6 +334,42 @@ int git_config_string(const char **dest, const char *var, const char *value)\n \treturn 0;\n }\n \n+/*\n+ * Expand ~ and ~user. Returns a newly malloced string. (If input does not\n+ * start with \"~\", equivalent to xstrdup.)\n+ */\n+static char *expand_userdir(const char *value) {\n+\tif (value[0] == '~') {\n+\t\tstruct passwd *pw;\n+\t\tchar *expanded_dir;\n+\t\tconst char *slash = strchr(value+1, '/');\n+\t\tconst char *after_username = slash ? slash : value+strlen(value);\n+\t\tif (after_username == value+1) {\n+\t\t\tpw = getpwuid(getuid());\n+\t\t\tif (!pw) die(\"You don't exist!\");\n+\t\t} else {\n+\t\t\tchar save = *after_username;\n+\t\t\t*(char*)after_username = '\\0';\n+\t\t\tpw = getpwnam(value+1);\n+\t\t\tif (!pw) die(\"No such user: '%s'\", value+1);\n+\t\t\t*(char*)after_username = save;\n+\t\t}\n+\t\texpanded_dir = xmalloc(strlen(pw->pw_dir) + strlen(after_username) + 1);\n+\t\tstrcpy(expanded_dir, pw->pw_dir);\n+\t\tstrcat(expanded_dir, after_username);\n+\t\treturn expanded_dir;\n+\t} else {\n+\t\treturn xstrdup(value);\n+\t}\n+}\n+\n+int git_config_userdir(const char **dest, const char *var, const char *value) {\n+\tif (!value)\n+\t\treturn config_error_nonbool(var);\n+\t*dest = expand_userdir(value);\n+\treturn 0;\n+}\n+\n static int git_default_core_config(const char *var, const char *value)\n {\n \t/* This needs a better name */\n@@ -456,8 +492,9 @@ static int git_default_core_config(const char *var, const char *value)\n \tif (!strcmp(var, \"core.editor\"))\n \t\treturn git_config_string(&editor_program, var, value);\n \n-\tif (!strcmp(var, \"core.excludesfile\"))\n-\t\treturn git_config_string(&excludes_file, var, value);\n+\tif (!strcmp(var, \"core.excludesfile\")) {\n+\t\treturn git_config_userdir(&excludes_file, var, value);\n+\t}\n \n \tif (!strcmp(var, \"core.whitespace\")) {\n \t\tif (!value)\n-- \n1.5.6.2\n"},{"id":"88513","messageId":"7vk5e4g58t.fsf@gitster.siamese.dyndns.org","threadId":"15152","inReplyTo":"20080825125205.GB23800@genesis.frugalware.org","subject":"Re: Dropping core.worktree and GIT_WORK_TREE support","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-25T21:21:54Z","receivedAt":"2008-08-25T21:21:54Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Miklos Vajna writes:\n\n> On Sun, Aug 24, 2008 at 08:05:03PM -0700, Junio C Hamano wrote:\n>> > Does this include removing of --work-tree as well?\n>> >\n>> > The git backend of Pootle (http://translate.sourceforge.net/wiki/) uses\n>> > it.\n>> \n>> Interesting. Does it use it because it can (meaning, --work-tree is\n>> supposed to work), or because --work-tree is the cleanest way to do what\n>> it wants to do (if the feature worked properly, that is, which is not the\n>> case)?\n>\n> It's like:\n>\n> The current working directory is like\n> /usr/lib/python2.5/site-packages/Pootle. The git repository is under\n> /some/other/path/outside/usr.\n>\n> Then Pootle has two possibilities:\n\nThe real question was about if/why that git repository _has to be_ outside\nof /usr/lib/*/Pootle/. Is that because --work-tree, if worked properly,\nwould have allowed it to be, or is that because for some external reason\nyou are not allowed to have .git under /usr/lib/*/Pootle/ directory?\n\nIf the latter, that shows the real requirement to keep supporting it as a\nfeature, and issues around it need to be fixed. Otherwise, i.e. if it\ndoes not require use of --work-tree but it uses it only because it could,\nthat gives us less incentive to keep --work-tree as a feature.\n\nI haven't read the breakage and fix around diff in this thread yet, but I\nwill when I get home.\n\nThanks to both Nguyen and you for trying to salvage --work-tree support.\n"},{"id":"88517","messageId":"20080825213748.GJ23800@genesis.frugalware.org","threadId":"15152","inReplyTo":"7vk5e4g58t.fsf@gitster.siamese.dyndns.org","subject":"Re: Dropping core.worktree and GIT_WORK_TREE support","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2008-08-25T21:37:48Z","receivedAt":"2008-08-25T21:37:48Z","isPatch":false,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"On Mon, Aug 25, 2008 at 02:21:54PM -0700, Junio C Hamano wrote:\n> The real question was about if/why that git repository _has to be_ outside\n> of /usr/lib/*/Pootle/. Is that because --work-tree, if worked properly,\n> would have allowed it to be, or is that because for some external reason\n> you are not allowed to have .git under /usr/lib/*/Pootle/ directory?\n\nI'm not 100% sure, but I think the reason is that Pootle runs as the\nuser 'pootle' which has write access to some /var/pootle or /home/pootle\ndir, but has no write access to /usr/lib/*/Pootle/.\n\n> If the latter, that shows the real requirement to keep supporting it as a\n> feature, and issues around it need to be fixed. Otherwise, i.e. if it\n> does not require use of --work-tree but it uses it only because it could,\n> that gives us less incentive to keep --work-tree as a feature.\n\nI think this is the latter, though as I mentioned previously - it can be\nstill worked around by a \"cd /other/path; git ; cd -\" (speaking\nin shell commands), so it is not a \"must\", it would be just ugly IMHO.\n"},{"id":"88547","messageId":"48B3A5CA.6090201@viscovery.net","threadId":"15152","inReplyTo":"quack.20080825T1207.lthk5e46hi4_-_@roar.cs.berkeley.edu","subject":"Re: [PATCH v2] Support \"core.excludesfile = ~/.gitignore\"","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-08-26T06:42:18Z","receivedAt":"2008-08-26T06:42:18Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Karl Chen schrieb:\n> +/*\n> + * Expand ~ and ~user. Returns a newly malloced string. (If input does not\n> + * start with \"~\", equivalent to xstrdup.)\n> + */\n> +static char *expand_userdir(const char *value) {\n\nThere is user_path() in path.c that does the same thing.\n\nWatch your style: The opening brace of functions is on the next line.\n\n-- Hannes\n"},{"id":"88553","messageId":"48B3B256.6010609@fastmail.fm","threadId":"15152","inReplyTo":"7v7ia6j5q9.fsf_-_@gitster.siamese.dyndns.org","subject":"Re: Dropping core.worktree and GIT_WORK_TREE support (was Re: limiting relationship of git dir and worktree)","fromName":"Michael J Gruber","fromEmail":"michaeljgruber+gmane@fastmail.fm","sentAt":"2008-08-26T07:35:50Z","receivedAt":"2008-08-26T07:35:50Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Junio C Hamano venit, vidit, dixit 25.08.2008 02:30:\n> Jeff King writes:\n> \n\n>> Well, as a non-user of this feature, I certainly have no argument\n>> against taking it out. Maybe the subject line will pull some other\n>> people into the discussion.\n> \n> Heh, if we are to do the attention-getter, let's do so more strongly ;-)\n\nSorry for being late to the discussion.\n\nI think there are many use cases or environments which differ\nsubstantially from those of the \"typical\" developer; this implies that\nthey differ from those of the typical git contributor, which naturally\nleads to a certain bias in discussions like this one.\n\n\"Typical\" developers track source code in the proper sense (somewhere in\n$HOME); on local file systems; mostly on machines where they have root\naccess, or least can get extra accounts (for gitosis) or a port for \"git\ndaemon\" etc; they collaborate with peers for whom basically the same\nassumptions apply.\n\nNow think of a user say in academics, who tracks \"source code\" for\nscientific papers (somewhere in $HOME) but also needs to track, e.g.,\ncentral web pages or other \"sources\" where he has partial write access\nbut can't have \".git\" in place (and shouldn't change ownership &\npermissions), but needs to be aware of changes and log own changes; on\nNFS; no extra accounts but in need of an authenticated protocol (papers\nin progress are private, public only when published); who collaborates\nwith peers for whom the same assumptions apply, except most certainly\nfor git usage...\n\nYes, that's me, but also many others, I would think and hope, at least\nincreasingly so. That second scenario is one where I have to cope with\nhow things are set up centrally, making the best possible use of git.\n\nI would imagine that many corporate environments are basically similar,\nif individual employees want to use git without central support.\n\nThese remarks apply to the discussion about an authenticated protocol\n(some way for secure, private pull&push for users with access to $HOME\nand maybe cgi-bins), but also here:\n\nI need to keep .git away from the work tree for several projects. Using\n--git-dir etc. leads to problems with some commands, especially\ngit{k,-gui,-citool}. I found the most robust solution to be an alias\n(shell) which guesses the work tree (from core.worktree etc.) and cd's\nthere before doing anything. This also solves the problems with diff.\n\nI would strongly advocate for keeping the possibility of separating\ngit-dir and work-tree, and possibly dropping the assumption that\neverything \"foo.git\" is a bare repo. There are config variables for\nthis. The Tcl/Tk family I mentioned makes even stronger assumptions. I\npromise to have a look at these when I find time (oh yeah...).\n\nMichael\n"},{"id":"88669","messageId":"7vprnvuy5q.fsf@gitster.siamese.dyndns.org","threadId":"15152","inReplyTo":"fcaeb9bf0808250826l2f1a0f3l94fff1b702e69c5d@mail.gmail.com","subject":"Re: [PATCH] git diff/diff-index/diff-files: call setup_work_tree()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-26T23:58:09Z","receivedAt":"2008-08-26T23:58:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Nguyen Thai Ngoc Duy\" writes:\n\n> On 8/25/08, Miklos Vajna wrote:\n>> diff --git a/builtin-diff.c b/builtin-diff.c\n>>\n>> index 7ffea97..57da6ed 100644\n>>\n>> --- a/builtin-diff.c\n>> +++ b/builtin-diff.c\n>>\n>> @@ -279,6 +279,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n>> diff_no_index(&rev, argc, argv, nongit, prefix);\n>>\n>> /* Otherwise, we are doing the usual \"git\" diff */\n>> + setup_work_tree();\n>> rev.diffopt.skip_stat_unmatch = !!diff_auto_refresh_index;\n>>\n>> if (nongit)\n>\n> At least builtin_diff_blobs() and builtin_diff_tree() won't need\n> worktree, so NACK again. Anyway I'm not familiar with diff*. Junio\n> should know these better.\n\nHow about doing it this way then?\n\n * diff-files is about comparing with work tree, so it obviously needs a\n work tree;\n\n * diff-index also does;\n\n * no-index is about random files outside git context, so it obviously\n doesn't need any work tree;\n\n * comparing two (or more) trees doesn't;\n\n * comparing two blobs doesn't;\n\n * comparing a blob with a random file doesn't;\n\nWhat could be problematic is \"git diff --cached\". Strictly speaking, it\ncompares the index and a tree so it shouldn't need any work tree. The\nsame obviously applies to \"git diff-index --cached\".\n\nWhile it is theoretically possible to have an index in a bare repository\nand build your history using it without using any worktree, I do not think\nit is a use case worth worrying about. As long as setup_work_tree() does\nnot complain and die in such a setup, \"diff --cached\" itself won't look at\nthe work tree (whereever random place setup_work_tree() sets it) at all,\nso probably it is a non issue. I dunno.\n\nI do not have a test environment that uses a separate worktree settings,\nso this is obviously untested.\n\nPerhaps people who are interested in keeping core.worktree alive can add\ntest scripts in t/ somewhere to help salvaging the feature?\n\n---\n\n builtin-diff.c | 3 +++\n git.c | 4 ++--\n 2 files changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git i/builtin-diff.c w/builtin-diff.c\nindex 7ffea97..06c85da 100644\n--- i/builtin-diff.c\n+++ w/builtin-diff.c\n@@ -114,6 +114,8 @@ static int builtin_diff_index(struct rev_info *revs,\n \t\t\t int argc, const char **argv)\n {\n \tint cached = 0;\n+\n+\tsetup_work_tree();\n \twhile (1 < argc) {\n \t\tconst char *arg = argv[1];\n \t\tif (!strcmp(arg, \"--cached\"))\n@@ -207,6 +209,7 @@ static int builtin_diff_files(struct rev_info *revs, int argc, const char **argv\n \tint result;\n \tunsigned int options = 0;\n \n+\tsetup_work_tree();\n \twhile (1 < argc && argv[1][0] == '-') {\n \t\tif (!strcmp(argv[1], \"--base\"))\n \t\t\trevs->max_count = 1;\ndiff --git i/git.c w/git.c\nindex 37b1d76..a8e730d 100644\n--- i/git.c\n+++ w/git.c\n@@ -286,8 +286,8 @@ static void handle_internal_command(int argc, const char **argv)\n \t\t{ \"count-objects\", cmd_count_objects, RUN_SETUP },\n \t\t{ \"describe\", cmd_describe, RUN_SETUP },\n \t\t{ \"diff\", cmd_diff },\n-\t\t{ \"diff-files\", cmd_diff_files, RUN_SETUP },\n-\t\t{ \"diff-index\", cmd_diff_index, RUN_SETUP },\n+\t\t{ \"diff-files\", cmd_diff_files, RUN_SETUP | NEED_WORK_TREE },\n+\t\t{ \"diff-index\", cmd_diff_index, RUN_SETUP | NEED_WORK_TREE },\n \t\t{ \"diff-tree\", cmd_diff_tree, RUN_SETUP },\n \t\t{ \"fast-export\", cmd_fast_export, RUN_SETUP },\n \t\t{ \"fetch\", cmd_fetch, RUN_SETUP },\n"},{"id":"88672","messageId":"20080827002506.GB7347@coredump.intra.peff.net","threadId":"15152","inReplyTo":"quack.20080825T1207.lthk5e46hi4_-_@roar.cs.berkeley.edu","subject":"Re: [PATCH v2] Support \"core.excludesfile = ~/.gitignore\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-08-27T00:25:06Z","receivedAt":"2008-08-27T00:25:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 25, 2008 at 12:07:15PM -0700, Karl Chen wrote:\n\n> Based on the discussion it sounds like there are complications to\n> supporting relative paths (due to worktree config), and \"$HOME\"\n> (when generalized, due to bootstrapping issues with $GIT_*).\n\nI think that is fine for now. One other simple possibility would be to\nexpand _just_ $HOME, and then if we later decided to do all environment\nvariables it would naturally encompass that. However, we might want to\nsupport \"~\" then anyway, so I think doing \"~\" first is fine.\n\nHowever, there are two problems with the patch:\n\n 1. It should probably re-use path.c:user_path, as Johannes mentioned.\n\n 2. There is no documentation update.\n\nAlso, are there any other config variables which would benefit from this\nsubstitution (I can't think of any off-hand, but there are quite a few I\ndon't use).\n\n-Peff\n"},{"id":"88676","messageId":"20080827004924.GA8204@coredump.intra.peff.net","threadId":"15152","inReplyTo":"48B3B256.6010609@fastmail.fm","subject":"Re: Dropping core.worktree and GIT_WORK_TREE support (was Re: limiting relationship of git dir and worktree)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-08-27T00:49:24Z","receivedAt":"2008-08-27T00:49:24Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"[resend: urgh, I somehow missed the git-list when replying, so it is\ncc'd here]\n\nOn Tue, Aug 26, 2008 at 09:35:50AM +0200, Michael J Gruber wrote:\n\n> \"Typical\" developers track source code in the proper sense (somewhere in\n> $HOME); on local file systems; mostly on machines where they have root\n> access, or least can get extra accounts (for gitosis) or a port for \"git\n> daemon\" etc; they collaborate with peers for whom basically the same\n> assumptions apply.\n> \n> Now think of a user say in academics, who tracks \"source code\" for\n> scientific papers (somewhere in $HOME) but also needs to track, e.g.,\n> central web pages or other \"sources\" where he has partial write access\n> but can't have \".git\" in place (and shouldn't change ownership &\n\nActually, I do all of those things, and I don't use the work-tree config\nvariable or command line options at all. :)\n\nI think the general advice with things like web access is \"don't just\ndump your git stuff into a production area; instead, build and/or\ninstall from your git work tree into your production area\". Because\nthings like merges _can_ leave your files in a broken state.\n\nBut I do recognize that there are some special circumstances where that\nisn't possible, and you are willing to accept the tradeoff. E.g., if the\ncheckout is extremely large and you can't afford another copy, if you\nhave clueless collaborators who can't understand a build procedure.\n\nAnd even though I expect those cases to be the exception, it seems a\nshame for git not to support split git-dir/work-tree setups because we\nreally are 99% there. This code has been the source of a number of\nproblems.\n\nI think what is really needed is somebody to look carefully at the git\nstartup sequence and figure out a sane set of rules for the order of:\n\n - looking at env variables\n - looking at config\n - figuring out GIT_DIR and GIT_WORK_TREE\n - chdir'ing to top level of work tree if necessary\n\nBecause we obviously have some corner cases where very confusing things\nare happening.\n\nThis is on my long term todo list, but my git time is very short at\nleast for the next few months. I think it would be great if somebody\nelse wanted to take the lead on this, and I would be happy to give\npointers about some of the corner cases we have already seen.\n\n-Peff\n"},{"id":"88680","messageId":"quack.20080826T2012.lthvdxn2ls4@roar.cs.berkeley.edu","threadId":"15152","inReplyTo":"20080827002506.GB7347@coredump.intra.peff.net","subject":"Re: [PATCH v2] Support \"core.excludesfile = ~/.gitignore\"","fromName":"Karl Chen","fromEmail":"quarl@cs.berkeley.edu","sentAt":"2008-08-27T03:12:59Z","receivedAt":"2008-08-27T03:12:59Z","isPatch":true,"sender":{"key":"quarl@cs.berkeley.edu","avatar":null},"body":">>>>> On 2008-08-26 17:25 PDT, Jeff King writes:\n\n Jeff> 1. It should probably re-use path.c:user_path, as\n Jeff> Johannes mentioned.\n\nThat function has the wrong interface for this task (requires\nextra strdup, imposes PATH_MAX, conflates all error conditions\ninto returning NULL, also returns NULL if input doesn't have \"~\").\nDo you still think it should re-use that function?\n"},{"id":"88681","messageId":"quack.20080826T2018.lthr68b2ljc@roar.cs.berkeley.edu","threadId":"15152","inReplyTo":"20080827002506.GB7347@coredump.intra.peff.net","subject":"Re: [PATCH v2] Support \"core.excludesfile = ~/.gitignore\"","fromName":"Karl Chen","fromEmail":"quarl@cs.berkeley.edu","sentAt":"2008-08-27T03:18:15Z","receivedAt":"2008-08-27T03:18:15Z","isPatch":true,"sender":{"key":"quarl@cs.berkeley.edu","avatar":null},"body":">>>>> On 2008-08-26 17:25 PDT, Jeff King writes:\n\n Jeff> 2. There is no documentation update.\n\nRelative paths and $ENVVARS would need explanation; not sure what\nneeds to be said about ~user since the new behavior is what people\nexpect to just work. Would it go in git-config.txt if something\nwere added?\n"},{"id":"88685","messageId":"7vabezt62n.fsf@gitster.siamese.dyndns.org","threadId":"15152","inReplyTo":"quack.20080826T2018.lthr68b2ljc@roar.cs.berkeley.edu","subject":"Re: [PATCH v2] Support \"core.excludesfile = ~/.gitignore\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-27T04:50:08Z","receivedAt":"2008-08-27T04:50:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karl Chen writes:\n\n>>>>>> On 2008-08-26 17:25 PDT, Jeff King writes:\n>\n> Jeff> 2. There is no documentation update.\n>\n> Relative paths and $ENVVARS would need explanation; not sure what\n> needs to be said about ~user since the new behavior is what people\n> expect to just work. Would it go in git-config.txt if something\n> were added?\n\nTraditionally, we never has interpret ~/ or ~user/. Users expected that\nthey need to spell it like:\n\n [core]\n excludesfile = \"/home/joe/.my-ignore-pattern\"\n\nEspecially since this is called \"variables\", it is natural, without\nexplanation, to expect this not to work, just like this use of a variable\nwon't work:\n\n $ EDITOR='~/bin/editor' ;# notice the single quote\n $ export EDITOR;\n $ git commit -a\n fatal: exec ~/bin/editor failed.\n\nYour patch improves the situation and now allow:\n\n [core]\n excludesfile = \"~/.my-ignore-pattern\" \n\nIf you do not document that it is now possible for this particular\nconfiguration variable, the users will never know.\n"},{"id":"88688","messageId":"7vy72jrr00.fsf@gitster.siamese.dyndns.org","threadId":"15152","inReplyTo":"quack.20080826T2012.lthvdxn2ls4@roar.cs.berkeley.edu","subject":"Re: [PATCH v2] Support \"core.excludesfile = ~/.gitignore\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-27T05:01:03Z","receivedAt":"2008-08-27T05:01:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karl Chen writes:\n\n>>>>>> On 2008-08-26 17:25 PDT, Jeff King writes:\n>\n> Jeff> 1. It should probably re-use path.c:user_path, as\n> Jeff> Johannes mentioned.\n>\n> That function has the wrong interface for this task (requires\n> extra strdup, imposes PATH_MAX, conflates all error conditions\n> into returning NULL, also returns NULL if input doesn't have \"~\").\n> Do you still think it should re-use that function?\n\nOn the other hand you have a pair of ugly \"casting a const pointer down,\ntemporarily modifying what is const\" in your version.\n\nIn any case, I think their point is that these two functions are meant to\ndo the same thing, and a unified interface would be desirable.\n\nYou do not necessarily have to use user_path() from path.c, but\nuser_path(), if its interface is coarser, could become a thin wrapper\naround yours, don't you think? Even better, perhaps the current callers\nof user_path() may benefit from finer distinction among error conditions\n(I didn't look, though).\n"},{"id":"88999","messageId":"quack.20080828T0209.lthmyixjyjx_-_@roar.cs.berkeley.edu","threadId":"15152","inReplyTo":"7vy72jrr00.fsf@gitster.siamese.dyndns.org","subject":"[PATCH v3] Expand ~ and ~user in core.excludesfile, commit.template","fromName":"Karl Chen","fromEmail":"quarl@cs.berkeley.edu","sentAt":"2008-08-28T09:09:38Z","receivedAt":"2008-08-28T09:09:38Z","isPatch":true,"sender":{"key":"quarl@cs.berkeley.edu","avatar":null},"body":"\nThese config variables are parsed to substitute ~ and ~user with getpw\nentries.\n\nuser_path() refactored into new function expand_user_path(), to allow\ndynamically allocating the return buffer.\n\nSigned-off-by: Karl Chen \n---\n\n>>>>> On 2008-08-26 22:01 PDT, Junio C Hamano writes:\n\n>>>>> On 2008-08-26 17:25 PDT, Jeff King writes:\n >> \n Jeff> 1. It should probably re-use path.c:user_path, as\n Jeff> Johannes mentioned.\n >> \n >> That function has the wrong interface for this task\n >> (requires extra strdup, imposes PATH_MAX, conflates all\n >> error conditions into returning NULL, also returns NULL if\n >> input doesn't have \"~\"). Do you still think it should\n >> re-use that function?\n\n Junio> On the other hand you have a pair of ugly \"casting a\n Junio> const pointer down, temporarily modifying what is\n Junio> const\" in your version.\n\nuser_path() does the same thing; it's just obscured by the lack of\ntype qualifiers. I rationalized it because git has much bigger\nproblems if threading support were added. But I agree it's ugly.\nThe new patch avoids it.\n\n Junio> In any case, I think their point is that these two\n Junio> functions are meant to do the same thing, and a unified\n Junio> interface would be desirable.\n\nThere is a lot of refactoring I would do were I maintaining the\ncode. I figured a minimally invasive change would be more easily\naccepted. Anyway here is a patch to unify \"~\" expansion.\n\n builtin-commit.c | 2 +-\n cache.h | 2 +\n config.c | 13 +++++++-\n path.c | 88 +++++++++++++++++++++++++++++++++--------------------\n 4 files changed, 69 insertions(+), 36 deletions(-)\n\ndiff --git a/builtin-commit.c b/builtin-commit.c\nindex 649c8be..e510207 100644\n--- a/builtin-commit.c\n+++ b/builtin-commit.c\n@@ -891,7 +891,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1)\n static int git_commit_config(const char *k, const char *v, void *cb)\n {\n \tif (!strcmp(k, \"commit.template\"))\n-\t\treturn git_config_string(&template_file, k, v);\n+\t\treturn git_config_userdir(&template_file, k, v);\n \n \treturn git_status_config(k, v, cb);\n }\ndiff --git a/cache.h b/cache.h\nindex ab9f97e..096fd9d 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -527,6 +527,7 @@ int git_config_perm(const char *var, const char *value);\n int adjust_shared_perm(const char *path);\n int safe_create_leading_directories(char *path);\n int safe_create_leading_directories_const(const char *path);\n+extern char *expand_user_path(char *buf, const char *path, int sz);\n char *enter_repo(char *path, int strict);\n static inline int is_absolute_path(const char *path)\n {\n@@ -748,6 +749,7 @@ extern unsigned long git_config_ulong(const char *, const char *);\n extern int git_config_bool_or_int(const char *, const char *, int *);\n extern int git_config_bool(const char *, const char *);\n extern int git_config_string(const char **, const char *, const char *);\n+extern int git_config_userdir(const char **, const char *, const char *);\n extern int git_config_set(const char *, const char *);\n extern int git_config_set_multivar(const char *, const char *, const char *, int);\n extern int git_config_rename_section(const char *, const char *);\ndiff --git a/config.c b/config.c\nindex 53f04a0..3395283 100644\n--- a/config.c\n+++ b/config.c\n@@ -334,6 +334,14 @@ int git_config_string(const char **dest, const char *var, const char *value)\n \treturn 0;\n }\n \n+int git_config_userdir(const char **dest, const char *var, const char *value) {\n+\tif (!value)\n+\t\treturn config_error_nonbool(var);\n+\t*dest = expand_user_path(NULL, value, 0);\n+\tif (!*dest || !**dest) die(\"Failed to expand user dir in: '%s'\", value);\n+\treturn 0;\n+}\n+\n static int git_default_core_config(const char *var, const char *value)\n {\n \t/* This needs a better name */\n@@ -456,8 +464,9 @@ static int git_default_core_config(const char *var, const char *value)\n \tif (!strcmp(var, \"core.editor\"))\n \t\treturn git_config_string(&editor_program, var, value);\n \n-\tif (!strcmp(var, \"core.excludesfile\"))\n-\t\treturn git_config_string(&excludes_file, var, value);\n+\tif (!strcmp(var, \"core.excludesfile\")) {\n+\t\treturn git_config_userdir(&excludes_file, var, value);\n+\t}\n \n \tif (!strcmp(var, \"core.whitespace\")) {\n \t\tif (!value)\ndiff --git a/path.c b/path.c\nindex 76e8872..ef6dbaa 100644\n--- a/path.c\n+++ b/path.c\n@@ -137,43 +137,65 @@ int validate_headref(const char *path)\n \treturn -1;\n }\n \n-static char *user_path(char *buf, char *path, int sz)\n+static inline struct passwd *getpw_strspan(const char *begin_username,\n+\t\t\t\t\t\t\t\t\t\t const char *end_username)\n {\n-\tstruct passwd *pw;\n-\tchar *slash;\n-\tint len, baselen;\n-\n-\tif (!path || path[0] != '~')\n-\t\treturn NULL;\n-\tpath++;\n-\tslash = strchr(path, '/');\n-\tif (path[0] == '/' || !path[0]) {\n-\t\tpw = getpwuid(getuid());\n+\tif (begin_username == end_username) {\n+\t\treturn getpwuid(getuid());\n+\t} else {\n+\t\tsize_t username_len = end_username - begin_username;\n+\t\tchar *username = alloca(username_len + 1);\n+\t\tmemcpy(username, begin_username, username_len);\n+\t\tusername[username_len] = '\\0';\n+\t\treturn getpwnam(username);\n \t}\n-\telse {\n-\t\tif (slash) {\n-\t\t\t*slash = 0;\n-\t\t\tpw = getpwnam(path);\n-\t\t\t*slash = '/';\n-\t\t}\n-\t\telse\n-\t\t\tpw = getpwnam(path);\n+}\n+\n+static inline char *concatstr(char *buf, const char *str1, const char *str2,\n+\t\t\t\t\t\t\t size_t bufsz)\n+{\n+\tsize_t len1 = strlen(str1);\n+\tsize_t len2 = strlen(str2);\n+\tsize_t needbuflen = len1 + len2 + 1;\n+\tif (buf) {\n+\t\tif (needbuflen > bufsz) return NULL;\n+\t} else {\n+\t\tbuf = xmalloc(needbuflen);\n \t}\n-\tif (!pw || !pw->pw_dir || sz <= strlen(pw->pw_dir))\n+\tmemcpy(buf, str1, len1);\n+\tmemcpy(buf+len1, str2, len2+1);\n+\treturn buf;\n+}\n+\n+static inline const char *strchr_or_end(const char *str, char c)\n+{\n+\twhile (*str && *str != c) ++str;\n+\treturn str;\n+}\n+\n+/*\n+ * Return a string with ~ and ~user expanded via getpw*. If buf != NULL, then\n+ * it is the output buffer with size sz (including terminator); else the\n+ * return buffer is xmalloced. Returns NULL on getpw failure or if the input\n+ * buffer is too small.\n+ */\n+char *expand_user_path(char *buf, const char *path, int sz)\n+{\n+\tif (path == NULL) {\n \t\treturn NULL;\n-\tbaselen = strlen(pw->pw_dir);\n-\tmemcpy(buf, pw->pw_dir, baselen);\n-\twhile ((1 < baselen) && (buf[baselen-1] == '/')) {\n-\t\tbuf[baselen-1] = 0;\n-\t\tbaselen--;\n-\t}\n-\tif (slash && slash[1]) {\n-\t\tlen = strlen(slash);\n-\t\tif (sz <= baselen + len)\n-\t\t\treturn NULL;\n-\t\tmemcpy(buf + baselen, slash, len + 1);\n+\t} else if (path[0] != '~') {\n+\t\tif (buf == NULL) {\n+\t\t\treturn xstrdup(path);\n+\t\t} else {\n+\t\t\tif (strlen(path)+1 > sz) return NULL;\n+\t\t\treturn strcpy(buf, path);\n+\t\t}\n+\t} else {\n+\t\tconst char *after_username = strchr_or_end(path+1, '/');\n+\t\tstruct passwd *pw = getpw_strspan(path+1, after_username);\n+\t\tif (!pw) return NULL;\n+\t\treturn concatstr(buf, pw->pw_dir, after_username, sz);\n \t}\n-\treturn buf;\n }\n \n /*\n@@ -221,7 +243,7 @@ char *enter_repo(char *path, int strict)\n \t\tif (PATH_MAX <= len)\n \t\t\treturn NULL;\n \t\tif (path[0] == '~') {\n-\t\t\tif (!user_path(used_path, path, PATH_MAX))\n+\t\t\tif (!expand_user_path(used_path, path, PATH_MAX))\n \t\t\t\treturn NULL;\n \t\t\tstrcpy(validated_path, path);\n \t\t\tpath = used_path;\n-- \n1.5.6.3\n"},{"id":"88878","messageId":"1219928532-25087-1-git-send-email-pclouds@gmail.com","threadId":"15152","inReplyTo":"7vprnvuy5q.fsf@gitster.siamese.dyndns.org","subject":"[PATCH] diff*: fix worktree setup","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2008-08-28T13:02:12Z","receivedAt":"2008-08-28T13:02:12Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This fixes \"git diff\", \"git diff-files\" and \"git diff-index\" to work\ncorrectly under worktree setup. Because diff* family works in many modes\nand not all of them require worktree, Junio made a nice summary\n(with a little modification from me):\n\n * diff-files is about comparing with work tree, so it obviously needs a\n work tree;\n\n * diff-index also does, except \"diff-index --cached\" or \"diff --cached TREE\"\n\n * no-index is about random files outside git context, so it obviously\n doesn't need any work tree;\n\n * comparing two (or more) trees doesn't;\n\n * comparing two blobs doesn't;\n\n * comparing a blob with a random file doesn't;\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin-diff-index.c | 2 +\n builtin-diff.c | 3 ++\n git.c | 2 +-\n t/t1501-worktree.sh | 59 ++++++++++++++++++++++++++++++++++++++++++++++++-\n 4 files changed, 63 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin-diff-index.c b/builtin-diff-index.c\nindex 17d851b..0483749 100644\n--- a/builtin-diff-index.c\n+++ b/builtin-diff-index.c\n@@ -39,6 +39,8 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)\n \tif (rev.pending.nr != 1 ||\n \t rev.max_count != -1 || rev.min_age != -1 || rev.max_age != -1)\n \t\tusage(diff_cache_usage);\n+\tif (!cached)\n+\t\tsetup_work_tree();\n \tif (read_cache() < 0) {\n \t\tperror(\"read_cache\");\n \t\treturn -1;\ndiff --git a/builtin-diff.c b/builtin-diff.c\nindex 7ffea97..037c303 100644\n--- a/builtin-diff.c\n+++ b/builtin-diff.c\n@@ -122,6 +122,8 @@ static int builtin_diff_index(struct rev_info *revs,\n \t\t\tusage(builtin_diff_usage);\n \t\targv++; argc--;\n \t}\n+\tif (!cached)\n+\t\tsetup_work_tree();\n \t/*\n \t * Make sure there is one revision (i.e. pending object),\n \t * and there is no revision filtering parameters.\n@@ -225,6 +227,7 @@ static int builtin_diff_files(struct rev_info *revs, int argc, const char **argv\n \t (revs->diffopt.output_format & DIFF_FORMAT_PATCH))\n \t\trevs->combine_merges = revs->dense_combined_merges = 1;\n \n+\tsetup_work_tree();\n \tif (read_cache() < 0) {\n \t\tperror(\"read_cache\");\n \t\treturn -1;\ndiff --git a/git.c b/git.c\nindex 37b1d76..fdb0f71 100644\n--- a/git.c\n+++ b/git.c\n@@ -286,7 +286,7 @@ static void handle_internal_command(int argc, const char **argv)\n \t\t{ \"count-objects\", cmd_count_objects, RUN_SETUP },\n \t\t{ \"describe\", cmd_describe, RUN_SETUP },\n \t\t{ \"diff\", cmd_diff },\n-\t\t{ \"diff-files\", cmd_diff_files, RUN_SETUP },\n+\t\t{ \"diff-files\", cmd_diff_files, RUN_SETUP | NEED_WORK_TREE },\n \t\t{ \"diff-index\", cmd_diff_index, RUN_SETUP },\n \t\t{ \"diff-tree\", cmd_diff_tree, RUN_SETUP },\n \t\t{ \"fast-export\", cmd_fast_export, RUN_SETUP },\ndiff --git a/t/t1501-worktree.sh b/t/t1501-worktree.sh\nindex 2ee88d8..e9e352c 100755\n--- a/t/t1501-worktree.sh\n+++ b/t/t1501-worktree.sh\n@@ -28,6 +28,7 @@ test_rev_parse() {\n \t[ $# -eq 0 ] && return\n }\n \n+EMPTY_TREE=$(git write-tree)\n mkdir -p work/sub/dir || exit 1\n mv .git repo.git || exit 1\n \n@@ -106,12 +107,66 @@ test_expect_success 'repo finds its work tree from work tree, too' '\n '\n \n test_expect_success '_gently() groks relative GIT_DIR & GIT_WORK_TREE' '\n-\tcd repo.git/work/sub/dir &&\n+\t(cd repo.git/work/sub/dir &&\n \tGIT_DIR=../../.. GIT_WORK_TREE=../.. GIT_PAGER= \\\n \t\tgit diff --exit-code tracked &&\n \techo changed > tracked &&\n \t! GIT_DIR=../../.. GIT_WORK_TREE=../.. GIT_PAGER= \\\n-\t\tgit diff --exit-code tracked\n+\t\tgit diff --exit-code tracked)\n+'\n+cat > diff-index-cached.expected <<\\EOF\n+:000000 100644 0000000000000000000000000000000000000000 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 A\tsub/dir/tracked\n+EOF\n+cat > diff-index.expected <<\\EOF\n+:000000 100644 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 A\tsub/dir/tracked\n+EOF\n+\n+\n+test_expect_success 'git diff-index' '\n+\tGIT_DIR=repo.git GIT_WORK_TREE=repo.git/work git diff-index $EMPTY_TREE > result &&\n+\tcmp diff-index.expected result &&\n+\tGIT_DIR=repo.git git diff-index --cached $EMPTY_TREE > result &&\n+\tcmp diff-index-cached.expected result\n+'\n+cat >diff-files.expected <<\\EOF\n+:100644 100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0000000000000000000000000000000000000000 M\tsub/dir/tracked\n+EOF\n+\n+test_expect_success 'git diff-files' '\n+\tGIT_DIR=repo.git GIT_WORK_TREE=repo.git/work git diff-files > result &&\n+\tcmp diff-files.expected result\n+'\n+\n+cat >diff-TREE.expected <<\\EOF\n+diff --git a/sub/dir/tracked b/sub/dir/tracked\n+new file mode 100644\n+index 0000000..5ea2ed4\n+--- /dev/null\n++++ b/sub/dir/tracked\n+@@ -0,0 +1 @@\n++changed\n+EOF\n+cat >diff-TREE-cached.expected <<\\EOF\n+diff --git a/sub/dir/tracked b/sub/dir/tracked\n+new file mode 100644\n+index 0000000..e69de29\n+EOF\n+cat >diff-FILES.expected <<\\EOF\n+diff --git a/sub/dir/tracked b/sub/dir/tracked\n+index e69de29..5ea2ed4 100644\n+--- a/sub/dir/tracked\n++++ b/sub/dir/tracked\n+@@ -0,0 +1 @@\n++changed\n+EOF\n+\n+test_expect_success 'git diff' '\n+\tGIT_DIR=repo.git GIT_WORK_TREE=repo.git/work git diff $EMPTY_TREE > result &&\n+\tcmp diff-TREE.expected result &&\n+\tGIT_DIR=repo.git git diff --cached $EMPTY_TREE > result &&\n+\tcmp diff-TREE-cached.expected result &&\n+\tGIT_DIR=repo.git GIT_WORK_TREE=repo.git/work git diff > result &&\n+\tcmp diff-FILES.expected result\n '\n \n test_done\n-- \n1.6.0.96.g2fad1.dirty\n"},{"id":"89077","messageId":"20080829032630.GA7024@coredump.intra.peff.net","threadId":"15152","inReplyTo":"quack.20080828T0209.lthmyixjyjx_-_@roar.cs.berkeley.edu","subject":"Re: [PATCH v3] Expand ~ and ~user in core.excludesfile, commit.template","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-08-29T03:26:31Z","receivedAt":"2008-08-29T03:26:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 28, 2008 at 02:09:38AM -0700, Karl Chen wrote:\n\n> builtin-commit.c | 2 +-\n> cache.h | 2 +\n> config.c | 13 +++++++-\n> path.c | 88 +++++++++++++++++++++++++++++++++--------------------\n> 4 files changed, 69 insertions(+), 36 deletions(-)\n\nDocumentation?\n\n> \tif (!strcmp(k, \"commit.template\"))\n> -\t\treturn git_config_string(&template_file, k, v);\n> +\t\treturn git_config_userdir(&template_file, k, v);\n\nI like this.\n\n> +int git_config_userdir(const char **dest, const char *var, const char *value) {\n> +\tif (!value)\n> +\t\treturn config_error_nonbool(var);\n> +\t*dest = expand_user_path(NULL, value, 0);\n> +\tif (!*dest || !**dest) die(\"Failed to expand user dir in: '%s'\", value);\n> +\treturn 0;\n> +}\n\nI am not sure about !**dest here. This precludes somebody from using \"\".\nWhile it might not matter here, if there are other users of\ngit_config_userdir(), they might want to allow a blank entry.\n\nAlso, style: there should be a newline after conditional but before\nexecuted code. IOW, replace\n\n if (cond) code;\n\nwith\n\n if (cond)\n code;\n\n> +static inline struct passwd *getpw_strspan(const char *begin_username,\n> +\t\t\t\t\t\t\t\t\t\t const char *end_username) \n\n1. There seem to be extra tabs in the second line, pushing the\n end_username argument way too far to the right.\n\n2. I'm not sure \"strspan\" is a good name for this helper, since it calls\n to mind the strspn C function, which is not really related to this at\n all.\n\n3. Usually helper functions that take a non-terminated string like this\n in git use the combination of (char *begin, int len) instead of two\n pointers. While you are currently the only user of the helper, I\n think it makes sense to follow that convention for future users.\n\n> +\tif (begin_username == end_username) {\n> +\t\treturn getpwuid(getuid());\n> +\t} else {\n\nStyle: omit braces on one-liner conditionals:\n\n if (begin_username == end_username)\n return getwpuid(getuid());\n\nAlso, you do a lot of early returns in your code. I think this is good,\nbecause it makes it more readable. But that means you don't have to\nworry about \"else\"ing the other half of the conditional, because you\nhave already returned. Which makes it even easier to read.\n\n> +\t\tsize_t username_len = end_username - begin_username;\n\nSee, here you end up converting back from two pointers to a pointer and\na length. Which is why I think we tend to use the other representation.\n\n> +\t\tchar *username = alloca(username_len + 1);\n\nI don't think we use alloca() anywhere else. I don't know if there are\nportability issues.\n\n> +static inline char *concatstr(char *buf, const char *str1, const char *str2,\n> +\t\t\t\t\t\t\t size_t bufsz)\n> +{\n> +\tsize_t len1 = strlen(str1);\n> +\tsize_t len2 = strlen(str2);\n> +\tsize_t needbuflen = len1 + len2 + 1;\n> +\tif (buf) {\n> +\t\tif (needbuflen > bufsz) return NULL;\n> +\t} else {\n> +\t\tbuf = xmalloc(needbuflen);\n\nStyle: more braces which can be omitted.\n\nThis function seems a little superfluous, since its semantics are so\nspecific to this usage. I am all for splitting into little functions,\nbut I think it would be quite confusing for somebody to try reusing\nthis. Perhaps it at least needs a comment explaining the semantics of\nbuf?\n\n> +static inline const char *strchr_or_end(const char *str, char c)\n> +{\n> +\twhile (*str && *str != c) ++str;\n> +\treturn str;\n> +}\n\nThis really seems like premature optimization to me. The only advantage\nthis has over\n\n p = strchr(s);\n if (!p)\n p = s + strlen(s);\n\nis that we avoid traversing the string once. But balance that against an\nassembler-optimized strchr provided by your libc. And then wonder if it\nis even worth it, since this is not even remotely a critical path.\n\n> +{\n> +\tif (path == NULL) {\n> \t\treturn NULL;\n> [...]\n> +\t} else if (path[0] != '~') {\n> +\t\tif (buf == NULL) {\n> +\t\t\treturn xstrdup(path);\n> +\t\t} else {\n> +\t\t\tif (strlen(path)+1 > sz) return NULL;\n> +\t\t\treturn strcpy(buf, path);\n> +\t\t}\n\nMore early returns which can be removed from conditionals.\n\nAlso, some of this code seems duplicated with concatstr. Wouldn't it\njust be simpler to let concatstr take a NULL for one of the arguments,\nand then just use it again here? IOW, something like:\n\n if (!path)\n return NULL;\n if (path[0] != '~')\n return concatstr(path, NULL);\n\n> -\t\t\tif (!user_path(used_path, path, PATH_MAX))\n> +\t\t\tif (!expand_user_path(used_path, path, PATH_MAX))\n\nBut these functions don't have the same semantics, do they? user_path\nused to return NULL if the path didn't start with ~, right?\n\n-Peff\n"},{"id":"89080","messageId":"7vod3ca2ey.fsf@gitster.siamese.dyndns.org","threadId":"15152","inReplyTo":"20080829032630.GA7024@coredump.intra.peff.net","subject":"Re: [PATCH v3] Expand ~ and ~user in core.excludesfile, commit.template","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-29T04:08:37Z","receivedAt":"2008-08-29T04:08:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Aug 28, 2008 at 02:09:38AM -0700, Karl Chen wrote:\n>\n>> builtin-commit.c | 2 +-\n>> cache.h | 2 +\n>> config.c | 13 +++++++-\n>> path.c | 88 +++++++++++++++++++++++++++++++++--------------------\n>> 4 files changed, 69 insertions(+), 36 deletions(-)\n>\n> Documentation?\n>\n>> \tif (!strcmp(k, \"commit.template\"))\n>> -\t\treturn git_config_string(&template_file, k, v);\n>> +\t\treturn git_config_userdir(&template_file, k, v);\n>\n> I like this.\n\nLikewise, except for the name. It is more like \"pathname\"; \"userdir\" is\nstressing only one aspect of the magic we would do to a value that is a\npathname compared to a value that is a string without any magic.\n\n>> +int git_config_userdir(const char **dest, const char *var, const char *value) {\n>> +\tif (!value)\n>> +\t\treturn config_error_nonbool(var);\n>> +\t*dest = expand_user_path(NULL, value, 0);\n>> +\tif (!*dest || !**dest) die(\"Failed to expand user dir in: '%s'\", value);\n>> +\treturn 0;\n>> +}\n>\n> I am not sure about !**dest here. This precludes somebody from using \"\".\n> While it might not matter here, if there are other users of\n> git_config_userdir(), they might want to allow a blank entry.\n\nTrue again.\n\n>> +\tif (begin_username == end_username) {\n>> +\t\treturn getpwuid(getuid());\n>> +\t} else {\n>\n> Style: omit braces on one-liner conditionals:\n\n... except in cases like this where \"else\" side is a multi-statement\nblock, in which case the above is fine. But as you pointed out, the early\nreturn from here makes the else block unnecessary so you do not need the\nbraces around \"if\" side.\n\n>> +\t\tchar *username = alloca(username_len + 1);\n>\n> I don't think we use alloca() anywhere else. I don't know if there are\n> portability issues.\n\nAvoidance of alloca() and c99 dynamic array on stack is deliberate in the\ncurrent codebase. Portable use of alloca() is quite hard to get right.\n\n>> +static inline const char *strchr_or_end(const char *str, char c)\n>> +{\n>> +\twhile (*str && *str != c) ++str;\n>> +\treturn str;\n>> +}\n>\n> This really seems like premature optimization to me.\n\nSo is overuse of inline, it seems.\n"},{"id":"89092","messageId":"48B79E9D.9000308@viscovery.net","threadId":"15152","inReplyTo":"quack.20080828T0209.lthmyixjyjx_-_@roar.cs.berkeley.edu","subject":"Re: [PATCH v3] Expand ~ and ~user in core.excludesfile, commit.template","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-08-29T07:00:45Z","receivedAt":"2008-08-29T07:00:45Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Karl Chen schrieb:\n> +\t\tconst char *after_username = strchr_or_end(path+1, '/');\n\nUse strchrnul() instead of a home-grown strchr_or_end().\n\n> +\t\tstruct passwd *pw = getpw_strspan(path+1, after_username);\n> +\t\tif (!pw) return NULL;\n> +\t\treturn concatstr(buf, pw->pw_dir, after_username, sz);\n\nYou really should use the strbuf API here. Look for strbuf_detach() in the\nexisting code.\n\n-- Hannes\n"},{"id":"89108","messageId":"quack.20080829T0229.lthhc94rwyr_-_@roar.cs.berkeley.edu","threadId":"15152","inReplyTo":"7vod3ca2ey.fsf@gitster.siamese.dyndns.org","subject":"[PATCH v4] Expand ~ and ~user in core.excludesfile, commit.template","fromName":"Karl Chen","fromEmail":"quarl@cs.berkeley.edu","sentAt":"2008-08-29T09:29:00Z","receivedAt":"2008-08-29T09:29:00Z","isPatch":true,"sender":{"key":"quarl@cs.berkeley.edu","avatar":null},"body":"\nThese config variables are parsed to substitute ~ and ~user with getpw\nentries.\n\nuser_path() refactored into new function expand_user_path(), to allow\ndynamically allocating the return buffer.\n\nSigned-off-by: Karl Chen \n---\n\n>>>>> On 2008-08-28 20:26 PDT, Jeff King writes:\n\n Peff> Documentation?\n\nDocumentation added.\n\n Peff> I am not sure about !**dest here. This precludes\n Peff> somebody from using \"\". While it might not matter here,\n Peff> if there are other users of git_config_userdir(), they\n Peff> might want to allow a blank entry.\n\nPoint taken.\n\n Peff> 1. There seem to be extra tabs in the second line,\n Peff> pushing the end_username argument way too far to the\n Peff> right.\n\nI had assumed that you guys use tab-width 4 instead of\nc-basic-offset 8. This is one of the reasons I never use tabs\nwhen I can help it.\n\n Peff> 2. I'm not sure \"strspan\" is a good name for this\n Peff> helper, since it calls to mind the strspn C function,\n Peff> which is not really related to this at all.\n\nChanged to getpw_str().\n\n Peff> 3. Usually helper functions that take a non-terminated\n Peff> string like this in git use the combination of (char\n Peff> *begin, int len) instead of two pointers. While you\n Peff> are currently the only user of the helper, I think it\n Peff> makes sense to follow that convention for future\n Peff> users.\n\nPoint taken, changed as suggested.\n\n Peff> Also, you do a lot of early returns in your code. I\n Peff> think this is good, because it makes it more\n Peff> readable. But that means you don't have to worry about\n Peff> \"else\"ing the other half of the conditional, because you\n Peff> have already returned. Which makes it even easier to\n Peff> read.\n\nPersonally I think early returns without \"else\"ing is appropriate\nfor error conditions but not when it's a \"do this\" or \"do that\"\nswitch. (I wonder if indenting 8 spaces has a long-term effect on\nthings like this?) Anyway, I accept the color you picked for this\nbikeshed.\n\n Peff> This function seems a little superfluous, since its\n Peff> semantics are so specific to this usage. I am all for\n Peff> splitting into little functions, but I think it would be\n Peff> quite confusing for somebody to try reusing\n Peff> this. Perhaps it at least needs a comment explaining the\n Peff> semantics of buf?\n\nComment added.\n\n Peff> Also, some of this code seems duplicated with\n Peff> concatstr. Wouldn't it just be simpler to let concatstr\n Peff> take a NULL for one of the arguments, and then just use\n Peff> it again here? IOW, something like:\n\n Peff> if (!path)\n Peff> return NULL;\n Peff> if (path[0] != '~')\n Peff> return concatstr(path, NULL);\n\nRefactored as suggested.\n\n >> - if (!user_path(used_path, path, PATH_MAX))\n >> + if (!expand_user_path(used_path, path, PATH_MAX))\n\n Peff> But these functions don't have the same semantics, do\n Peff> they? user_path used to return NULL if the path didn't\n Peff> start with ~, right?\n\nYes, but user_path was only called when the input starts with ~.\n\n>>>>> On 2008-08-28 21:08 PDT, Junio C Hamano writes:\n >>> + return git_config_userdir(&template_file, k, v);\n\n Junio> It is more like \"pathname\"; \"userdir\" is stressing only\n Junio> one aspect of the magic we would do to a value that is\n Junio> a pathname compared to a value that is a string without\n Junio> any magic.\n\nRenamed as suggested.\n\n Junio> Avoidance of alloca() and c99 dynamic array on stack is\n Junio> deliberate in the current codebase. Portable use of\n Junio> alloca() is quite hard to get right.\n\nAlloca replaced with xmalloc+free.\n\n>>>>> On 2008-08-29 00:00 PDT, Johannes Sixt writes:\n\n Hannes> Use strchrnul() instead of a home-grown\n Hannes> strchr_or_end().\n\nDidn't know about strchrnul, thanks.\n\n Hannes> You really should use the strbuf API here. Look for\n Hannes> strbuf_detach() in the existing code.\n\nUnfortunately expand_user_path() needs to support both a fixed\nbuffer and mallocing return. I don't think the strbuf API can do\nthat easily?\n\n\n Documentation/config.txt | 4 ++-\n builtin-commit.c | 2 +-\n cache.h | 2 +\n config.c | 11 +++++-\n path.c | 86 +++++++++++++++++++++++++++++-----------------\n 5 files changed, 70 insertions(+), 35 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex af57d94..05e846d 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -346,7 +346,8 @@ Common unit suffixes of 'k', 'm', or 'g' are supported.\n core.excludesfile::\n \tIn addition to '.gitignore' (per-directory) and\n \t'.git/info/exclude', git looks into this file for patterns\n-\tof files which are not meant to be tracked. See\n+\tof files which are not meant to be tracked. \"~\" and \"~user\"\n+\tare expanded to the user's home directory. See\n \tlinkgit:gitignore[5].\n \n core.editor::\n@@ -554,6 +555,7 @@ color.status.::\n \n commit.template::\n \tSpecify a file to use as the template for new commit messages.\n+\t\"~\" and \"~user\" are expanded to the user's home directory.\n \n color.ui::\n \tWhen set to `always`, always use colors in all git commands which\ndiff --git a/builtin-commit.c b/builtin-commit.c\nindex 649c8be..905ebde 100644\n--- a/builtin-commit.c\n+++ b/builtin-commit.c\n@@ -891,7 +891,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1)\n static int git_commit_config(const char *k, const char *v, void *cb)\n {\n \tif (!strcmp(k, \"commit.template\"))\n-\t\treturn git_config_string(&template_file, k, v);\n+\t\treturn git_config_pathname(&template_file, k, v);\n \n \treturn git_status_config(k, v, cb);\n }\ndiff --git a/cache.h b/cache.h\nindex ab9f97e..3e04794 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -527,6 +527,7 @@ int git_config_perm(const char *var, const char *value);\n int adjust_shared_perm(const char *path);\n int safe_create_leading_directories(char *path);\n int safe_create_leading_directories_const(const char *path);\n+extern char *expand_user_path(char *buf, const char *path, int sz);\n char *enter_repo(char *path, int strict);\n static inline int is_absolute_path(const char *path)\n {\n@@ -748,6 +749,7 @@ extern unsigned long git_config_ulong(const char *, const char *);\n extern int git_config_bool_or_int(const char *, const char *, int *);\n extern int git_config_bool(const char *, const char *);\n extern int git_config_string(const char **, const char *, const char *);\n+extern int git_config_pathname(const char **, const char *, const char *);\n extern int git_config_set(const char *, const char *);\n extern int git_config_set_multivar(const char *, const char *, const char *, int);\n extern int git_config_rename_section(const char *, const char *);\ndiff --git a/config.c b/config.c\nindex 53f04a0..55353d9 100644\n--- a/config.c\n+++ b/config.c\n@@ -334,6 +334,15 @@ int git_config_string(const char **dest, const char *var, const char *value)\n \treturn 0;\n }\n \n+int git_config_pathname(const char **dest, const char *var, const char *value) {\n+\tif (!value)\n+\t\treturn config_error_nonbool(var);\n+\t*dest = expand_user_path(NULL, value, 0);\n+\tif (!*dest)\n+\t\tdie(\"Failed to expand user dir in: '%s'\", value);\n+\treturn 0;\n+}\n+\n static int git_default_core_config(const char *var, const char *value)\n {\n \t/* This needs a better name */\n@@ -457,7 +466,7 @@ static int git_default_core_config(const char *var, const char *value)\n \t\treturn git_config_string(&editor_program, var, value);\n \n \tif (!strcmp(var, \"core.excludesfile\"))\n-\t\treturn git_config_string(&excludes_file, var, value);\n+\t\treturn git_config_pathname(&excludes_file, var, value);\n \n \tif (!strcmp(var, \"core.whitespace\")) {\n \t\tif (!value)\ndiff --git a/path.c b/path.c\nindex 76e8872..016d072 100644\n--- a/path.c\n+++ b/path.c\n@@ -137,46 +137,68 @@ int validate_headref(const char *path)\n \treturn -1;\n }\n \n-static char *user_path(char *buf, char *path, int sz)\n+static inline struct passwd *getpw_str(const char *username, size_t len)\n {\n+\tif (len == 0)\n+\t\treturn getpwuid(getuid());\n+\n \tstruct passwd *pw;\n-\tchar *slash;\n-\tint len, baselen;\n+\tchar *username_z = xmalloc(len + 1);\n+\tmemcpy(username_z, username, len);\n+\tusername_z[len] = '\\0';\n+\tpw = getpwnam(username_z);\n+\tfree(username_z);\n+\treturn pw;\n+}\n \n-\tif (!path || path[0] != '~')\n-\t\treturn NULL;\n-\tpath++;\n-\tslash = strchr(path, '/');\n-\tif (path[0] == '/' || !path[0]) {\n-\t\tpw = getpwuid(getuid());\n-\t}\n-\telse {\n-\t\tif (slash) {\n-\t\t\t*slash = 0;\n-\t\t\tpw = getpwnam(path);\n-\t\t\t*slash = '/';\n-\t\t}\n-\t\telse\n-\t\t\tpw = getpwnam(path);\n-\t}\n-\tif (!pw || !pw->pw_dir || sz <= strlen(pw->pw_dir))\n-\t\treturn NULL;\n-\tbaselen = strlen(pw->pw_dir);\n-\tmemcpy(buf, pw->pw_dir, baselen);\n-\twhile ((1 < baselen) && (buf[baselen-1] == '/')) {\n-\t\tbuf[baselen-1] = 0;\n-\t\tbaselen--;\n-\t}\n-\tif (slash && slash[1]) {\n-\t\tlen = strlen(slash);\n-\t\tif (sz <= baselen + len)\n+/*\n+ * Return a string with input strings concatenated. If buf != NULL, then\n+ * it is the output buffer with size bufsz (including terminator); else the\n+ * return buffer is xmalloced. Second string can be NULL, in which case the\n+ * first string is simply strduped/strcpyed. Returns NULL if bufsz is too\n+ * small.\n+ */\n+static inline char *concatstr(char *buf, const char *str1, const char *str2,\n+\t\t\t size_t bufsz)\n+{\n+\tsize_t len1 = strlen(str1);\n+\tsize_t len2 = str ? strlen(str2) : 0;\n+\tsize_t needbuflen = len1 + len2 + 1;\n+\tif (buf) {\n+\t\tif (needbuflen > bufsz)\n \t\t\treturn NULL;\n-\t\tmemcpy(buf + baselen, slash, len + 1);\n+\t} else {\n+\t\tbuf = xmalloc(needbuflen);\n \t}\n+\tmemcpy(buf, str1, len1);\n+\tif (str2)\n+\t\tmemcpy(buf+len1, str2, len2+1);\n \treturn buf;\n }\n \n /*\n+ * Return a string with ~ and ~user expanded via getpw*. If buf != NULL, then\n+ * it is the output buffer with size bufsz (including terminator); else the\n+ * return buffer is xmalloced. Returns NULL on getpw failure or if the input\n+ * buffer is too small.\n+ */\n+char *expand_user_path(char *buf, const char *path, int bufsz)\n+{\n+\tif (path == NULL)\n+\t\treturn NULL;\n+\n+\tif (path[0] != '~')\n+\t\treturn concatstr(buf, path, NULL, bufsz);\n+\n+\tconst char *username = path + 1;\n+\tsize_t username_len = strchrnul(username, '/') - username;\n+\tstruct passwd *pw = getpw_str(username, username_len);\n+\tif (!pw)\n+\t\treturn NULL;\n+\treturn concatstr(buf, pw->pw_dir, username+username_len, bufsz);\n+}\n+\n+/*\n * First, one directory to try is determined by the following algorithm.\n *\n * (0) If \"strict\" is given, the path is used as given and no DWIM is\n@@ -221,7 +243,7 @@ char *enter_repo(char *path, int strict)\n \t\tif (PATH_MAX <= len)\n \t\t\treturn NULL;\n \t\tif (path[0] == '~') {\n-\t\t\tif (!user_path(used_path, path, PATH_MAX))\n+\t\t\tif (!expand_user_path(used_path, path, PATH_MAX))\n \t\t\t\treturn NULL;\n \t\t\tstrcpy(validated_path, path);\n \t\t\tpath = used_path;\n-- \n1.5.6.3\n"},{"id":"89138","messageId":"7vsksn4xdo.fsf@gitster.siamese.dyndns.org","threadId":"15152","inReplyTo":"quack.20080829T0229.lthhc94rwyr_-_@roar.cs.berkeley.edu","subject":"Re: [PATCH v4] Expand ~ and ~user in core.excludesfile, commit.template","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-29T16:08:35Z","receivedAt":"2008-08-29T16:08:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karl Chen writes:\n\n> These config variables are parsed to substitute ~ and ~user with getpw\n> entries.\n>\n> user_path() refactored into new function expand_user_path(), to allow\n> dynamically allocating the return buffer.\n>\n> Signed-off-by: Karl Chen \n\nThanks.\n\n> ... Anyway, I accept the color you picked for this\n> bikeshed.\n\nI do not think Documentation/CodingStyle is bikesheding but just behaving\nlike Romans do while in Rome, so that the end result will blend in better.\n\n> Hannes> You really should use the strbuf API here. Look for\n> Hannes> strbuf_detach() in the existing code.\n>\n> Unfortunately expand_user_path() needs to support both a fixed\n> buffer and mallocing return. I don't think the strbuf API can do\n> that easily?\n\nI do not see any strong reason why the single caller of user_path() has to\nkeep using the static allocation. Would it help to reduce the complexity\nof your expand_user_path() implementation, if we fixed the caller along\nthe lines of this patch (untested, but just to illustrate the point)?\n\n path.c | 15 +++++++++------\n 1 files changed, 9 insertions(+), 6 deletions(-)\n\ndiff --git i/path.c w/path.c\nindex 76e8872..c5b253c 100644\n--- i/path.c\n+++ w/path.c\n@@ -221,19 +221,22 @@ char *enter_repo(char *path, int strict)\n \t\tif (PATH_MAX <= len)\n \t\t\treturn NULL;\n \t\tif (path[0] == '~') {\n-\t\t\tif (!user_path(used_path, path, PATH_MAX))\n+\t\t\tchar *newpath = expand_user_path(path);\n+\t\t\tif (!newpath || (PATH_MAX <= strlen(newpath))) {\n+\t\t\t\tif (path != newpath)\n+\t\t\t\t\tfree(newpath);\n \t\t\t\treturn NULL;\n+\t\t\t}\n \t\t\tstrcpy(validated_path, path);\n-\t\t\tpath = used_path;\n-\t\t}\n-\t\telse if (PATH_MAX - 10 < len)\n-\t\t\treturn NULL;\n-\t\telse {\n+\t\t\tpath = newpath;\n+\t\t} else {\n \t\t\tpath = strcpy(used_path, path);\n \t\t\tstrcpy(validated_path, path);\n \t\t}\n \t\tlen = strlen(path);\n \t\tfor (i = 0; suffix[i]; i++) {\n+\t\t\tif (PATH_MAX <= strlen(suffix[i] + len))\n+\t\t\t\tcontinue;\n \t\t\tstrcpy(path + len, suffix[i]);\n \t\t\tif (!access(path, F_OK)) {\n \t\t\t\tstrcat(validated_path, suffix[i]);\n"},{"id":"89147","messageId":"quack.20080829T1201.lthsksnir1u@roar.cs.berkeley.edu","threadId":"15152","inReplyTo":"7vsksn4xdo.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH v4] Expand ~ and ~user in core.excludesfile, commit.template","fromName":"Karl Chen","fromEmail":"quarl@cs.berkeley.edu","sentAt":"2008-08-29T19:01:33Z","receivedAt":"2008-08-29T19:01:33Z","isPatch":true,"sender":{"key":"quarl@cs.berkeley.edu","avatar":null},"body":">>>>> On 2008-08-29 09:08 PDT, Junio C Hamano writes:\n\n Junio> I do not see any strong reason why the single caller of\n Junio> user_path() has to keep using the static allocation.\n Junio> Would it help to reduce the complexity of your\n Junio> expand_user_path() implementation, if we fixed the\n Junio> caller along the lines of this patch (untested, but\n Junio> just to illustrate the point)?\n\nYes, expand_user_path() would be much simpler, it would basically\nbe me original implementation except for returning NULL on error.\n"},{"id":"89150","messageId":"7vk5dz4o3t.fsf@gitster.siamese.dyndns.org","threadId":"15152","inReplyTo":"quack.20080829T1201.lthsksnir1u@roar.cs.berkeley.edu","subject":"Re: [PATCH v4] Expand ~ and ~user in core.excludesfile, commit.template","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-29T19:28:54Z","receivedAt":"2008-08-29T19:28:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karl Chen writes:\n\n>>>>>> On 2008-08-29 09:08 PDT, Junio C Hamano writes:\n>\n> Junio> I do not see any strong reason why the single caller of\n> Junio> user_path() has to keep using the static allocation.\n> Junio> Would it help to reduce the complexity of your\n> Junio> expand_user_path() implementation, if we fixed the\n> Junio> caller along the lines of this patch (untested, but\n> Junio> just to illustrate the point)?\n>\n> Yes, expand_user_path() would be much simpler, it would basically\n> be me original implementation except for returning NULL on error.\n\nYeah, modulo those styles issues your v3 and v4 addressed, and use of\nstrbuf.\n\nIt might feel that we went full circles, wasting your time. But it's not.\nWe found out that the final series would look like this:\n\n [1/3] Introduce expand_user_path();\n\n [2/3] Using #1, introduce git_config_pathname() and use it to parse your\n two variables;\n\n [3/3] Update the sole caller of user_path() to use expand_user_path().\n\nPatch #1 and #2 can be squashed into one if you want. Also you do not\nhave to do #3 yourself if you do not feel like it (but now we know how the\ncode would look like, why not?).\n\nThanks to these three initial rounds, we know whoever implements #1 knows\nwhat kind of interface the (to-be-rewritten) user of user_path() would\nwant, so #3 will become much cleaner. We made progress.\n\nThanks.\n"},{"id":"89168","messageId":"quack.20080829T1534.lthd4jr30xq@roar.cs.berkeley.edu","threadId":"15152","inReplyTo":"7vk5dz4o3t.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH v4] Expand ~ and ~user in core.excludesfile, commit.template","fromName":"Karl Chen","fromEmail":"quarl@cs.berkeley.edu","sentAt":"2008-08-29T22:34:41Z","receivedAt":"2008-08-29T22:34:41Z","isPatch":true,"sender":{"key":"quarl@cs.berkeley.edu","avatar":null},"body":">>>>> On 2008-08-29 12:28 PDT, Junio C Hamano writes:\n\n Junio> [3/3] Update the sole caller of user_path() to use\n Junio> expand_user_path().\n\nActually I just looked closer at enter_repo() and it's not quite\nas simple as your proposed patch, because enter_repo() wants to\nconcatenate suffixes like \".git\". So either the malloced string\nwould have to be copied to the static buffer again, or return a\nstrbuf, or take an argument for allocating extra chars.\n\nWow, I'd forgotten how much work it is to do string manipulation\nin C.\n"},{"id":"89189","messageId":"7v8wuf2hmg.fsf@gitster.siamese.dyndns.org","threadId":"15152","inReplyTo":"quack.20080829T1534.lthd4jr30xq@roar.cs.berkeley.edu","subject":"Re: [PATCH v4] Expand ~ and ~user in core.excludesfile, commit.template","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-30T05:31:51Z","receivedAt":"2008-08-30T05:31:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karl Chen writes:\n\n>>>>>> On 2008-08-29 12:28 PDT, Junio C Hamano writes:\n>\n> Junio> [3/3] Update the sole caller of user_path() to use\n> Junio> expand_user_path().\n>\n> Actually I just looked closer at enter_repo() and it's not quite\n> as simple as your proposed patch, because enter_repo() wants to\n> concatenate suffixes like \".git\".\n\nI thought the \"just an illustration\" patch at least took care of that\npart.\n\nThe thing is that enter_repo() is not performance critical, it is where\nserver side programs validate the repository path received from the other\nend of the network, and we can be stricter than necessary about path\nlengths. I do not particularly think it is necessary to convert it and\nuse strbuf to allow arbitrarily long paths --- rejecting requests to an\nabsurdly deep directory is not an end of the world there.\n"},{"id":"89190","messageId":"20080830060159.GA6826@coredump.intra.peff.net","threadId":"15152","inReplyTo":"quack.20080829T0229.lthhc94rwyr_-_@roar.cs.berkeley.edu","subject":"Re: [PATCH v4] Expand ~ and ~user in core.excludesfile, commit.template","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-08-30T06:02:00Z","receivedAt":"2008-08-30T06:02:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 29, 2008 at 02:29:00AM -0700, Karl Chen wrote:\n\n> core.excludesfile::\n> \tIn addition to '.gitignore' (per-directory) and\n> \t'.git/info/exclude', git looks into this file for patterns\n> -\tof files which are not meant to be tracked. See\n> +\tof files which are not meant to be tracked. \"~\" and \"~user\"\n> +\tare expanded to the user's home directory. See\n> \tlinkgit:gitignore[5].\n\nHow about:\n\n A leading \"~\" or \"~user\" is expanded to the home directory of the\n current user or \"user\", as in the shell.\n\nSpecifically:\n\n 1. We obviously handle only leading cases, so /path/to/~file~with~tildes\n is ok.\n\n 2. It was a little unclear to me whether both \"~\" and \"~user\" expande\n to the same thing. I.e., can one use this for arbitrary \"user\" (and\n the answer of course is yes).\n\n> +char *expand_user_path(char *buf, const char *path, int bufsz)\n> +{\n> +\tif (path == NULL)\n> +\t\treturn NULL;\n> +\n> +\tif (path[0] != '~')\n> +\t\treturn concatstr(buf, path, NULL, bufsz);\n> +\n> +\tconst char *username = path + 1;\n> +\tsize_t username_len = strchrnul(username, '/') - username;\n> +\tstruct passwd *pw = getpw_str(username, username_len);\n\nDeclaration after statement (we try to remain portable to non-C99\nsystems).\n\nOther than that, I think the patch is fine (though I am not opposed to\nthe improvements Junio has mentioned).\n\n-Peff\n"}]}