{"thread":{"id":"28046","subject":"[PATCH] Support specific color for a specific remote branches","startedAt":"2011-08-08T15:49:13Z","lastAt":"2011-08-10T12:16:12Z","messageCount":6,"participants":["Aviv Eyal","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"173136","messageId":"1312818553-25042-1-git-send-email-avivey@gmail.com","threadId":"28046","inReplyTo":null,"subject":"[PATCH] Support specific color for a specific remote branches","fromName":"Aviv Eyal","fromEmail":"avivey@gmail.com","sentAt":"2011-08-08T15:49:13Z","receivedAt":"2011-08-08T15:49:13Z","isPatch":true,"sender":{"key":"avivey@gmail.com","avatar":"https://gravatar.com/avatar/93416169042bb1ee4af6a7b7d313a18252bab4b25aeca338137f2948d2d6c26a?d=mp&s=160"},"body":"Program 'show branch -a' supports different colors for 'local' and 'remote' branches.\nWhen tracking more then one remote, all branches have the same color.\nThis change lets the user define a color for each remote, providing better visual distinction for different remotes.\n\nSigned-off-by: Aviv Eyal <avivey@gmail.com>\n---\n\nSince my C days are now ancient history, I'm worried I might leak some char*s down there. Also, \nI might have missed some apis that would have made this patch smaller.\n\nI'm using a sorted list as a dictionary (Remote name -> Color), which is not too elegant but\nprobably 'good enough', considering I expect it to be a very small (<3) list.\n\nAviv.\n\n Documentation/config.txt |    6 ++\n builtin/branch.c         |  119 +++++++++++++++++++++++++++++++++++++---------\n 2 files changed, 102 insertions(+), 23 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 0658ffb..efdb61f 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -700,6 +700,7 @@ color.branch.<slot>::\n \t`current` (the current branch), `local` (a local branch),\n \t`remote` (a remote-tracking branch in refs/remotes/), `plain` (other\n \trefs).\n+\tSee also `remote.<name>.color`.\n +\n The value for these configuration variables is a list of colors (at most\n two) and attributes (at most one), separated by spaces.  The colors\n@@ -1658,6 +1659,11 @@ remote.<name>.mirror::\n \tIf true, pushing to this remote will automatically behave\n \tas if the `\\--mirror` option was given on the command line.\n \n+remote.<name>.color:\n+\tIf set, the `branch` command will use the colors specified here\n+\tfor the names of all branches tracking this remote. Colors\n+\tspecified here take precedent over `color.branch.remote` config.\n+\n remote.<name>.skipDefaultUpdate::\n \tIf true, this remote will be skipped by default when updating\n \tusing linkgit:git-fetch[1] or the `update` subcommand of\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 3142daa..85e6920 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -15,6 +15,7 @@\n #include \"branch.h\"\n #include \"diff.h\"\n #include \"revision.h\"\n+#include \"string-list.h\"\n \n static const char * const builtin_branch_usage[] = {\n \t\"git branch [options] [-r | -a] [--merged | --no-merged]\",\n@@ -46,6 +47,9 @@ enum color_branch {\n \tBRANCH_COLOR_CURRENT = 4\n };\n \n+// this string_list is used as a basic dictionary.\n+struct string_list custom_colors = STRING_LIST_INIT_DUP;\n+\n static enum merge_filter {\n \tNO_FILTER = 0,\n \tSHOW_NOT_MERGED,\n@@ -68,6 +72,52 @@ static int parse_branch_color_slot(const char *var, int ofs)\n \treturn -1;\n }\n \n+static char *strclone(const char *start, int len)\n+{\n+\tchar *result = malloc((len + 1) * sizeof(char));\n+\tif (!result)\n+\t\treturn NULL;\n+\tstrncpy(result, start, len);\n+\tresult[len] = '\\0';\n+\treturn result;\n+}\n+\n+static int git_branch_config_custom_color_remote(const char *var, const char *value)\n+{\n+\tstruct string_list_item *item;\n+\tchar *name, *color;\n+\tname = strclone(var + 7, strlen(var) - 13); // \"remote.\"=7, \"remote..color\"=13.\n+\tif (!name)\n+\t\treturn 0;\n+\tcolor =  malloc(COLOR_MAXLEN * sizeof(char));\n+\tif (!color) {\n+\t\tfree(name);\n+\t\treturn 0;\n+\t}\n+\titem = string_list_insert(&custom_colors, name);\n+\tcolor_parse(value, var, color);\n+\titem->util = color;\n+\treturn 0;\n+}\n+\n+static char *git_branch_get_custom_color_remote(const char *name)\n+{\n+\tint name_len;\n+\tchar* repo_name;\n+\tstruct string_list_item *custom;\n+\tname_len = strchr(name, '/') - name;\n+\trepo_name = strclone(name, name_len);\n+\tif (!repo_name)\n+\t\treturn NULL;\n+\n+\tcustom = string_list_lookup(&custom_colors, repo_name);\n+\tfree(repo_name);\n+\tif (custom)\n+\t\treturn (char*) custom->util;\n+\treturn NULL;\n+}\n+\n+\n static int git_branch_config(const char *var, const char *value, void *cb)\n {\n \tif (!strcmp(var, \"color.branch\")) {\n@@ -83,14 +133,10 @@ static int git_branch_config(const char *var, const char *value, void *cb)\n \t\tcolor_parse(value, var, branch_colors[slot]);\n \t\treturn 0;\n \t}\n-\treturn git_color_default_config(var, value, cb);\n-}\n+\tif (!prefixcmp(var, \"remote.\") && !suffixcmp(var, \".color\"))\n+\t\treturn git_branch_config_custom_color_remote(var, value);\n \n-static const char *branch_get_color(enum color_branch ix)\n-{\n-\tif (branch_use_color > 0)\n-\t\treturn branch_colors[ix];\n-\treturn \"\";\n+\treturn git_color_default_config(var, value, cb);\n }\n \n static int branch_merged(int kind, const char *name,\n@@ -354,6 +400,43 @@ static int ref_cmp(const void *r1, const void *r2)\n \treturn strcmp(c1->name, c2->name);\n }\n \n+static const char *branch_get_color(struct ref_item *item, int current)\n+{\n+\tint index;\n+\tchar *color;\n+\n+\tif (branch_use_color <= 0)\n+\t\treturn \"\";\n+\n+\tif (!item)\n+\t\tindex = BRANCH_COLOR_RESET;\n+\telse if (current)\n+\t\tindex = BRANCH_COLOR_CURRENT;\n+\telse switch (item->kind) {\n+\t\tcase REF_LOCAL_BRANCH:\n+\t\t\tindex = BRANCH_COLOR_LOCAL;\n+\t\t\tbreak;\n+\t\tcase REF_REMOTE_BRANCH:\n+\t\t\tindex = BRANCH_COLOR_REMOTE;\n+\t\t\tcolor = git_branch_get_custom_color_remote(item->name);\n+\t\t\tif (color)\n+\t\t\t\treturn color;\n+\t\t\tbreak;\n+\t\tdefault:\n+\t\t\tindex = BRANCH_COLOR_PLAIN;\n+\t\t\tbreak;\n+\t}\n+\n+\treturn branch_colors[index];\n+}\n+\n+static const char *branch_get_color_reset()\n+{\n+\tif (branch_use_color <= 0)\n+\t\treturn \"\";\n+\treturn branch_colors[BRANCH_COLOR_RESET];\n+}\n+\n static void fill_tracking_info(struct strbuf *stat, const char *branch_name,\n \t\tint show_upstream_ref)\n {\n@@ -424,18 +507,6 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,\n \tif (!matches_merge_filter(commit))\n \t\treturn;\n \n-\tswitch (item->kind) {\n-\tcase REF_LOCAL_BRANCH:\n-\t\tcolor = BRANCH_COLOR_LOCAL;\n-\t\tbreak;\n-\tcase REF_REMOTE_BRANCH:\n-\t\tcolor = BRANCH_COLOR_REMOTE;\n-\t\tbreak;\n-\tdefault:\n-\t\tcolor = BRANCH_COLOR_PLAIN;\n-\t\tbreak;\n-\t}\n-\n \tc = ' ';\n \tif (current) {\n \t\tc = '*';\n@@ -444,12 +515,12 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,\n \n \tstrbuf_addf(&name, \"%s%s\", prefix, item->name);\n \tif (verbose)\n-\t\tstrbuf_addf(&out, \"%c %s%-*s%s\", c, branch_get_color(color),\n+\t\tstrbuf_addf(&out, \"%c %s%-*s%s\", c, branch_get_color(item, current),\n \t\t\t    maxwidth, name.buf,\n-\t\t\t    branch_get_color(BRANCH_COLOR_RESET));\n+\t\t\t    branch_get_color_reset());\n \telse\n-\t\tstrbuf_addf(&out, \"%c %s%s%s\", c, branch_get_color(color),\n-\t\t\t    name.buf, branch_get_color(BRANCH_COLOR_RESET));\n+\t\tstrbuf_addf(&out, \"%c %s%s%s\", c, branch_get_color(item, current),\n+\t\t\t    name.buf, branch_get_color_reset());\n \n \tif (item->dest)\n \t\tstrbuf_addf(&out, \" -> %s\", item->dest);\n@@ -712,5 +783,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t} else\n \t\tusage_with_options(builtin_branch_usage, options);\n \n+\tstring_list_clear(&custom_colors, 1);\n+\n \treturn 0;\n }\n-- \n1.7.4.1\n"},{"id":"173144","messageId":"7v8vr3zsh1.fsf@alter.siamese.dyndns.org","threadId":"28046","inReplyTo":"1312818553-25042-1-git-send-email-avivey@gmail.com","subject":"Re: [PATCH] Support specific color for a specific remote branches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-08T18:08:26Z","receivedAt":"2011-08-08T18:08:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Aviv Eyal <avivey@gmail.com> writes:\n\n> +remote.<name>.color:\n> +\tIf set, the `branch` command will use the colors specified here\n> +\tfor the names of all branches tracking this remote. Colors\n> +\tspecified here take precedent over `color.branch.remote` config.\n\n> +static char *strclone(const char *start, int len)\n> +{\n\nHmm, don't we have xmemdupz() for this?\n\n> +\tchar *result = malloc((len + 1) * sizeof(char));\n> +\tif (!result)\n> +\t\treturn NULL;\n> +\tstrncpy(result, start, len);\n> +\tresult[len] = '\\0';\n> +\treturn result;\n> +}\n> +\n> +static int git_branch_config_custom_color_remote(const char *var, const char *value)\n> +{\n> +\tstruct string_list_item *item;\n> +\tchar *name, *color;\n> +\tname = strclone(var + 7, strlen(var) - 13); // \"remote.\"=7, \"remote..color\"=13.\n\nNo // comments please.\n\n> +\tif (!name)\n> +\t\treturn 0;\n> +\tcolor =  malloc(COLOR_MAXLEN * sizeof(char));\n\nAt least use xmalloc(); also isn't sizeof(char) by definition 1?\n\n> +\tif (!color) {\n> +\t\tfree(name);\n> +\t\treturn 0;\n> +\t}\n> +\titem = string_list_insert(&custom_colors, name);\n> +\tcolor_parse(value, var, color);\n> +\titem->util = color;\n\nIt may be just a style thing, but I'd rather see this caller call\ncolor_parse() first, and after letting that function validate the end-user\ninput, call string_list_insert().\n\nAlso what happens when there are two entries that talk about the same\nremote branch in the configuration? Such a configuration is not an error;\nyour ~/.gitconfig may say one thing for remote.origin.master and your\nrepository specific .git/config may override it for a particular\nrepository.\n\n> +\treturn 0;\n> +}\n> +\n> +static char *git_branch_get_custom_color_remote(const char *name)\n> +{\n> +\tint name_len;\n> +\tchar* repo_name;\n> +\tstruct string_list_item *custom;\n> +\tname_len = strchr(name, '/') - name;\n\nWho said a remote name is terminated with (and cannot contain) a slash?\n\nShouldn't this code be consulting the configuration file to learn the\nremote mapping, e.g.\n\n    [remote \"frotz\"]\n        fetch = +refs/heads/*:refs/remotes/nitfol/*\n\nso that remote branches from \"frotz\" remote, that happen to be stored\nunder refs/remotes/nitfol/ hierarchy, are painted in the correct color?\n"},{"id":"173157","messageId":"20110808205214.GG18294@sigill.intra.peff.net","threadId":"28046","inReplyTo":"7v8vr3zsh1.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Support specific color for a specific remote branches","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-08-08T20:52:14Z","receivedAt":"2011-08-08T20:52:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 08, 2011 at 11:08:26AM -0700, Junio C Hamano wrote:\n\n> > +\treturn 0;\n> > +}\n> > +\n> > +static char *git_branch_get_custom_color_remote(const char *name)\n> > +{\n> > +\tint name_len;\n> > +\tchar* repo_name;\n> > +\tstruct string_list_item *custom;\n> > +\tname_len = strchr(name, '/') - name;\n> \n> Who said a remote name is terminated with (and cannot contain) a slash?\n> \n> Shouldn't this code be consulting the configuration file to learn the\n> remote mapping, e.g.\n> \n>     [remote \"frotz\"]\n>         fetch = +refs/heads/*:refs/remotes/nitfol/*\n> \n> so that remote branches from \"frotz\" remote, that happen to be stored\n> under refs/remotes/nitfol/ hierarchy, are painted in the correct color?\n\nThis seems related to the recent thread about showing branches only for\na specific remote:\n\n  http://article.gmane.org/gmane.comp.version-control.git/178668\n\nMaybe the two should share code.\n\nRight now, \"git branch -r\" means \"show everything under refs/remotes\ninstead of refs/heads\". This would be easy to implement if it instead\nmeant \"show all refs created by the RHS of a fetch refspec in a\nconfigured remote\". The two are equivalent in the default config, but\nthe latter may make more sense in a complex case.\n\n-Peff\n"},{"id":"173166","messageId":"7v62m7wpxg.fsf@alter.siamese.dyndns.org","threadId":"28046","inReplyTo":"20110808205214.GG18294@sigill.intra.peff.net","subject":"Re: [PATCH] Support specific color for a specific remote branches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-08T21:31:39Z","receivedAt":"2011-08-08T21:31:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Mon, Aug 08, 2011 at 11:08:26AM -0700, Junio C Hamano wrote:\n>\n> This seems related to the recent thread about showing branches only for\n> a specific remote:\n>\n>   http://article.gmane.org/gmane.comp.version-control.git/178668\n\nYeah, that one I do recall.\n\n> Right now, \"git branch -r\" means \"show everything under refs/remotes\n> instead of refs/heads\". This would be easy to implement if it instead\n> meant \"show all refs created by the RHS of a fetch refspec in a\n> configured remote\". The two are equivalent in the default config, but\n> the latter may make more sense in a complex case.\n\nI actually am a bit ambivalent about this. I do not necessarily consider\nthe contrived \"remote.frotz.fetch = refs/heads/*:refs/remotes/nitfol/*\"\nexample something that we _must_ solve. It is unlikely people would do\nthat, and if we give them an unexpected result, they deserve it ;-).\n\nBut in real-life, it is entirely plausible that people with multiple\nintegration branches are taking advantage of the simplicity of the old\nlayout, i.e.\n\n    [remote \"origin\"]\n    \tfetch = refs/heads/master:refs/heads/origin\n        fetch = refs/heads/next:refs/heads/next\n        fetch = refs/heads/maint:refs/heads/maint\n        fetch = +refs/heads/pu:refs/heads/pu\n\nI have many repositories of this style, and it is very convenient to be\nable to say:\n\n    $ git checkout master && git pull --ff-only\n    $ for b in master next maint pu\n      do\n          git checkout $b && make install || break\n      done\n\nI do not think I want to ever switch them to new layout, and I suspect\nthat many others do feel the same.\n\nNow, for these repositories, is \"next\" a local branch or a remote one? I\nhave a feeling that it might be easier to understand if we label anything\nthat you can update with \"checkout && commit\" a local one for the purpose\nof \"branch -r\" listing; IOW, the current \"git branch -r\" classification\nwould match this use pattern better, even though refs/heads/next _is_ an\nRHS of a rule to follow others.\n\nIn that sense, I would be entirely happy if the configuration variable\nused in this series were branch.<namepattern>.color and let you specify\n\n\t[branch \"refs/heads\"] color = yellow\n        [branch \"refs/remotes/origin\"] color = purple\n        [branch \"refs/remotes/nitfol\"] color = cyan\n\nIt becomes complicated (and for no good reason, in my opinion; see the\n\"next\" example above) if you try to tie this with remote.<name> hierarchy,\nas it obviously becomes illogical not to use the \"RHS of a fetch refspec\"\nlogic when we are talking about remote.<name>.\n"},{"id":"173218","messageId":"CAGHT48LHOi=TL6eSJKiFoao55quPn+nsK+etCmWPQsZB7ydLmQ@mail.gmail.com","threadId":"28046","inReplyTo":"7v8vr3zsh1.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Support specific color for a specific remote branches","fromName":"Aviv Eyal","fromEmail":"avivey@gmail.com","sentAt":"2011-08-09T18:59:47Z","receivedAt":"2011-08-09T18:59:47Z","isPatch":true,"sender":{"key":"avivey@gmail.com","avatar":"https://gravatar.com/avatar/93416169042bb1ee4af6a7b7d313a18252bab4b25aeca338137f2948d2d6c26a?d=mp&s=160"},"body":"On 8 August 2011 21:08, Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Who said a remote name is terminated with (and cannot contain) a slash?\n...\n>\n> Shouldn't this code be consulting the configuration file to learn the\n...\n> so that remote branches from \"frotz\" remote, that happen to be stored\n> under refs/remotes/nitfol/ hierarchy, are painted in the correct color?\n>\n\nSorry, that's too dip & complex for me... Consider this patch 'withdrawn' then.\n\n-- Aviv\n"},{"id":"173238","messageId":"20110810121611.GA17071@sigill.intra.peff.net","threadId":"28046","inReplyTo":"7v62m7wpxg.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Support specific color for a specific remote branches","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-08-10T12:16:12Z","receivedAt":"2011-08-10T12:16:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 08, 2011 at 02:31:39PM -0700, Junio C Hamano wrote:\n\n> But in real-life, it is entirely plausible that people with multiple\n> integration branches are taking advantage of the simplicity of the old\n> layout, i.e.\n> \n>     [remote \"origin\"]\n>     \tfetch = refs/heads/master:refs/heads/origin\n>         fetch = refs/heads/next:refs/heads/next\n>         fetch = refs/heads/maint:refs/heads/maint\n>         fetch = +refs/heads/pu:refs/heads/pu\n>\n> [...]\n>\n> Now, for these repositories, is \"next\" a local branch or a remote one? I\n> have a feeling that it might be easier to understand if we label anything\n> that you can update with \"checkout && commit\" a local one for the purpose\n> of \"branch -r\" listing; IOW, the current \"git branch -r\" classification\n> would match this use pattern better, even though refs/heads/next _is_ an\n> RHS of a rule to follow others.\n\nAgreed, that really muddies the idea of what is a \"remote branch\" and\nwhat is not. I think there is a sense among users that \"local branches\"\nand \"remote branches\" are mutually exclusive sets, even though by some\ndefinitions they may not be (i.e., if you define the former by where in\nthe refs/ hierarchy they exist, and the latter by being the RHS of a\nremote fetch refspec).\n\n> In that sense, I would be entirely happy if the configuration variable\n> used in this series were branch.<namepattern>.color and let you specify\n> \n> \t[branch \"refs/heads\"] color = yellow\n>         [branch \"refs/remotes/origin\"] color = purple\n>         [branch \"refs/remotes/nitfol\"] color = cyan\n\nThere are two issues I see with that:\n\n  1. Until now, \"branch\" sections like this have always been about local\n     branches.  What are the rules for defining something like\n     \"branch.refs/remotes/origin/foo.merge\"? Knowing how the code works,\n     it is easy to say it is a noop, as we will never look at it. But I\n     wonder how it looks from the perspective if a recent git user.\n\n  2. Until now, \"branch\" sections specify full refs, not subsets or\n     wildcard patterns. What does \"branch.refs/heads.merge\" mean?\n     A noop? A wildcard with slightly lesser precedence than\n     \"branch.refs/heads/foo.merge\"?\n\nIf we are going to go this route, I think it is really about introducing\nproperties not on branches, but on subsets of the ref namespace. So\nmight say:\n\n  [ref \"refs/heads/*\"] color = yellow\n\nwhich says \"whenever you are dealing with this part of the refs\nnamespace, my preferred color is yellow\". And \"git branch\" happens to\nrespect that (and we could just as easily have a \"%(refcolor)\" marker in\ngit-for-each-ref).\n\nI know the difference is subtle, but I just think it removes entirely\nthe question about what is a branch and what is not. Furthermore, it\nnaturally extends to other commands (e.g., you could color subsets of\nthe tag namespace differently), to more complex layouts (e.g., if we\nend up moving fetched tags into the refs/remotes namespace eventually),\nand to other properties besides color (though I haven't though up any\napplications).\n\n> It becomes complicated (and for no good reason, in my opinion; see the\n> \"next\" example above) if you try to tie this with remote.<name> hierarchy,\n> as it obviously becomes illogical not to use the \"RHS of a fetch refspec\"\n> logic when we are talking about remote.<name>.\n\nWe've been discussing the coloring issue here. But the other thread I\npointed out was about asking \"which fetched branches do we have locally\nfor this remote?\".  Which is a very reasonable thing to ask, and which\ndon't do a good job of answering right now. And I think it has to\ndo the \"RHS of a fetch refspec thing\".\n\nRight now you can do \"git remote show\". But:\n\n  1. It's very heavyweight. It shows you way more than you want in most\n     cases, and it touches the network by default (there is a \"-n\"\n     option, but touching the network by default makes it pretty\n     un-git).\n\n  2. It's not as easily discoverable as \"git branch -r\". It's not\n     unreasonable for users to mentally go through the sequence:\n\n       a. \"git branch\" shows me branches\n\n       b. oh, it has an \"-r\" option for remotes\n\n       c. how do I limit to one remote?\n\nI'm not sure what the right solution is. Going from 2b to 2c is a very\nnatural thing for a user to want to do. But it means jumping from one\ndefinition of \"remote\" (i.e., everything under \"refs/remotes\") to\nanother (the config defined by remote \"foo\"). In the default config,\nthat is a natural jump, as they are semantically connected. But they\ndon't have to be.\n\n-Peff\n"}]}