{"thread":{"id":"8096","subject":"[PATCH 0/3] Remotes library, take 3","startedAt":"2007-05-12T02:39:19Z","lastAt":"2007-05-12T07:25:02Z","messageCount":3,"participants":["Daniel Barkalow","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"41890","messageId":"Pine.LNX.4.64.0705112234520.18541@iabervon.org","threadId":"8096","inReplyTo":null,"subject":"[PATCH 0/3] Remotes library, take 3","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2007-05-12T02:39:19Z","receivedAt":"2007-05-12T02:39:19Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"This series is the same as the previous version, except that it matches \nthe current behavior of builtin-push with respect to treating names as \nliteral URIs.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"41896","messageId":"7viray5xfj.fsf@assigned-by-dhcp.cox.net","threadId":"8096","inReplyTo":"Pine.LNX.4.64.0705112234520.18541@iabervon.org","subject":"Re: [PATCH 0/3] Remotes library, take 3","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-05-12T07:10:56Z","receivedAt":"2007-05-12T07:10:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Daniel Barkalow <barkalow@iabervon.org> writes:\n\n> This series is the same as the previous version, except that it matches \n> the current behavior of builtin-push with respect to treating names as \n> literal URIs.\n\nThanks.\n\n        diff --git a/remote.c b/remote.c\n        index 32a0acf..1dd2e77 100644\n        --- a/remote.c\n        +++ b/remote.c\n        @@ -189,12 +189,14 @@ struct remote *remote_get(const char *name)\n                if (!name)\n                        name = default_remote_name;\n                ret = make_remote(name, 0);\n        -\tif (*name == '/')\n        -\t\tadd_uri(ret, name);\n        -\tif (!ret->uri)\n        -\t\tread_remotes_file(ret);\n        +\tif (name[0] != '/') {\n        +\t\tif (!ret->uri)\n        +\t\t\tread_remotes_file(ret);\n        +\t\tif (!ret->uri)\n        +\t\t\tread_branches_file(ret);\n        +\t}\n                if (!ret->uri)\n        -\t\tread_branches_file(ret);\n        +\t\tadd_uri(ret, name);\n                if (!ret->uri)\n                        return NULL;\n                return ret;\n\nThis is more similar to the original from builtin-push.c than\nyour previous round, but it is still not identical.\n\nThe differences should not matter in real life, but I think we\nneed to make it clear what the differences are to warn users.\nHere is my reading of the change (please correct me).\n\nEarlier.\n\n  - A name that does not begin with a slash could be a remote\n    shorthand.  Check remotes, config and branches in this order\n    and stop once a match is found.\n\n  - Otherwise use the name as a literal URI.\n\nThis patch.\n\n  - Config always wins.\n\n  - A name that does not begin with a slash could be found in\n    remotes or branches; check them in this order.\n\n  - Otherwise use it as is.\n\nTheoretically people _could_ have had a config like\n\n\t[remote \"/pub\"]\n\t\turl = blah\n\nbut it would never have matched.  This ``broken'' config file\nsuddenly start to interfere when somebody does:\n\n\t$ git push /pub\n\nAlso people may have had a remotes and config of the same name,\nand currently what is defined in config is ignored, but with the\nnew code, config takes precedence.  Which is unarguably good,\nbut still a change I should remember to write down in the\nrelease notes, hence prefer to have it clearly described in the\ncommit log message.\n\nWe probably would not care about the first difference, but it is\neasy enough to guard against, I think.  Perhaps with this patch?\n\n-- >8 --\nparsing remotes: forbid \"remote./foo.variable\" and fix segfault\n\nHistorically we did not pay attention to a remote shorthand\ndefined in the config file whose name starts with a slash\n(because we always took such a string as a literal localfile\nURL), but the new organization of the parsing code makes config\nalways take precedence.  It does not make much sense to define\nsuch a remote shorthand, so protect ourselves against it.\n\nAlso, the code forgot that a config variable could be spelled\nwithout a value to denote a boolean set to true, in which case\nthe config parser passes a NULL in value parameter, and run\nxstrdup() on it without checking.  Fix this.\n\nSigned-off-by: Junio C Hamano <junkio@cox.net>\n---\n remote.c |   17 +++++++++++++++++\n 1 files changed, 17 insertions(+), 0 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex 1dd2e77..05df196 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -149,7 +149,24 @@ static int handle_config(const char *key, const char *value)\n \tsubkey = strrchr(name, '.');\n \tif (!subkey)\n \t\treturn error(\"Config with no key for remote %s\", name);\n+\tif (*subkey == '/')\n+\t\treturn error(\"Config remote shorthand cannot begin with '/': %s\", name);\n \tremote = make_remote(name, subkey - name);\n+\tif (!value) {\n+\t\t/* if we ever have a boolean variable, e.g. \"remote.*.disabled\"\n+\t\t * [remote \"frotz\"]\n+\t\t *\tdisabled\n+\t\t * is a valid way to set it to true; we get NULL in value so\n+\t\t * we need to handle it here.\n+\t\t *\n+\t\t * if (!strcmp(subkey, \".disabled\")) {\n+\t\t * \tval = git_config_bool(key, value);\n+\t\t *\treturn 0;\n+\t\t * } else\n+\t\t *\n+\t\t */\n+\t\treturn error(\"Config with no value for remote %s\", name);\n+\t}\n \tif (!strcmp(subkey, \".url\")) {\n \t\tadd_uri(remote, xstrdup(value));\n \t} else if (!strcmp(subkey, \".push\")) {\n"},{"id":"41899","messageId":"Pine.LNX.4.64.0705120312570.18541@iabervon.org","threadId":"8096","inReplyTo":"7viray5xfj.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH 0/3] Remotes library, take 3","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2007-05-12T07:25:02Z","receivedAt":"2007-05-12T07:25:02Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Sat, 12 May 2007, Junio C Hamano wrote:\n\n> Daniel Barkalow <barkalow@iabervon.org> writes:\n> \n> > This series is the same as the previous version, except that it matches \n> > the current behavior of builtin-push with respect to treating names as \n> > literal URIs.\n> \n> Thanks.\n> \n>         diff --git a/remote.c b/remote.c\n>         index 32a0acf..1dd2e77 100644\n>         --- a/remote.c\n>         +++ b/remote.c\n>         @@ -189,12 +189,14 @@ struct remote *remote_get(const char *name)\n>                 if (!name)\n>                         name = default_remote_name;\n>                 ret = make_remote(name, 0);\n>         -\tif (*name == '/')\n>         -\t\tadd_uri(ret, name);\n>         -\tif (!ret->uri)\n>         -\t\tread_remotes_file(ret);\n>         +\tif (name[0] != '/') {\n>         +\t\tif (!ret->uri)\n>         +\t\t\tread_remotes_file(ret);\n>         +\t\tif (!ret->uri)\n>         +\t\t\tread_branches_file(ret);\n>         +\t}\n>                 if (!ret->uri)\n>         -\t\tread_branches_file(ret);\n>         +\t\tadd_uri(ret, name);\n>                 if (!ret->uri)\n>                         return NULL;\n>                 return ret;\n> \n> This is more similar to the original from builtin-push.c than\n> your previous round, but it is still not identical.\n> \n> The differences should not matter in real life, but I think we\n> need to make it clear what the differences are to warn users.\n> Here is my reading of the change (please correct me).\n> \n> Earlier.\n> \n>   - A name that does not begin with a slash could be a remote\n>     shorthand.  Check remotes, config and branches in this order\n>     and stop once a match is found.\n> \n>   - Otherwise use the name as a literal URI.\n> \n> This patch.\n> \n>   - Config always wins.\n> \n>   - A name that does not begin with a slash could be found in\n>     remotes or branches; check them in this order.\n> \n>   - Otherwise use it as is.\n> \n> Theoretically people _could_ have had a config like\n> \n> \t[remote \"/pub\"]\n> \t\turl = blah\n> \n> but it would never have matched.  This ``broken'' config file\n> suddenly start to interfere when somebody does:\n> \n> \t$ git push /pub\n\nThis is true, and I missed it before. Feel free to mention it in the \nappropriate commit message.\n\n> Also people may have had a remotes and config of the same name,\n> and currently what is defined in config is ignored, but with the\n> new code, config takes precedence.  Which is unarguably good,\n> but still a change I should remember to write down in the\n> release notes, hence prefer to have it clearly described in the\n> commit log message.\n\nThis is in the message for [1/3] already, along with the other change: it \nwill use the current branch's remote, if there is one, instead of \"origin\" \nif no repository is given on the command line.\n\nI was only claiming here (incorrectly, it turns out) that the configured \nremote vs. literal URI behavior is the same with this series.\n\n> We probably would not care about the first difference, but it is\n> easy enough to guard against, I think.  Perhaps with this patch?\n> \n> -- >8 --\n> diff --git a/remote.c b/remote.c\n> index 1dd2e77..05df196 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -149,7 +149,24 @@ static int handle_config(const char *key, const char *value)\n>  \tsubkey = strrchr(name, '.');\n>  \tif (!subkey)\n>  \t\treturn error(\"Config with no key for remote %s\", name);\n> +\tif (*subkey == '/')\n> +\t\treturn error(\"Config remote shorthand cannot begin with '/': %s\", name);\n\nMaybe just return? If we change the behavior to give an error in this \nsituation, we might as well make the config file actually take effect \ninstead.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"}]}