{"thread":{"id":"43131","subject":"[PATCH] Add support to git-branch to show local and remote branches","startedAt":"2006-11-02T11:11:09Z","lastAt":"2006-11-03T23:05:24Z","messageCount":23,"participants":["Andy Parkins","Junio C Hamano","Alex Riesen","Andreas Ericsson"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"295427","messageId":"200611021111.09434.andyparkins@gmail.com","threadId":"43131","inReplyTo":"bec6ab7849e3fcacac23cca44a0ba93282af5fca.1162465753.git.andyparkins@gmail.com","subject":"[PATCH 2/4] Rename remote_only to display_mode","fromName":"Andy Parkins","fromEmail":"andyparkins@gmail.com","sentAt":"2006-11-02T11:11:09Z","receivedAt":"2006-11-02T11:11:09Z","isPatch":true,"sender":{"key":"andyparkins@gmail.com","avatar":null},"body":"Signed-off-by: Andy Parkins <andyparkins@gmail.com>\n---\n builtin-branch.c |   10 +++++-----\n 1 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin-branch.c b/builtin-branch.c\nindex 368b68e..85b7007 100644\n--- a/builtin-branch.c\n+++ b/builtin-branch.c\n@@ -100,12 +100,12 @@ static int ref_cmp(const void *r1, const\n \treturn strcmp(*(char **)r1, *(char **)r2);\n }\n \n-static void print_ref_list(int remote_only)\n+static void print_ref_list(int display_mode)\n {\n \tint i;\n \tchar c;\n \n-\tif (remote_only)\n+\tif (display_mode)\n \t\tfor_each_remote_ref(append_ref, NULL);\n \telse\n \t\tfor_each_branch_ref(append_ref, NULL);\n@@ -160,7 +160,7 @@ static void create_branch(const char *na\n \n int cmd_branch(int argc, const char **argv, const char *prefix)\n {\n-\tint delete = 0, force_delete = 0, force_create = 0, remote_only = 0;\n+\tint delete = 0, force_delete = 0, force_create = 0, display_mode = 0;\n \tint reflog = 0;\n \tint i;\n \n@@ -189,7 +189,7 @@ int cmd_branch(int argc, const char **ar\n \t\t\tcontinue;\n \t\t}\n \t\tif (!strcmp(arg, \"-r\")) {\n-\t\t\tremote_only = 1;\n+\t\t\tdisplay_mode = 1;\n \t\t\tcontinue;\n \t\t}\n \t\tif (!strcmp(arg, \"-l\")) {\n@@ -209,7 +209,7 @@ int cmd_branch(int argc, const char **ar\n \tif (delete)\n \t\tdelete_branches(argc - i, argv + i, force_delete);\n \telse if (i == argc)\n-\t\tprint_ref_list(remote_only);\n+\t\tprint_ref_list(display_mode);\n \telse if (i == argc - 1)\n \t\tcreate_branch(argv[i], head, force_create, reflog);\n \telse if (i == argc - 2)\n-- \n1.4.3.2\n\n"},{"id":"294664","messageId":"200611021111.22604.andyparkins@gmail.com","threadId":"43131","inReplyTo":"bec6ab7849e3fcacac23cca44a0ba93282af5fca.1162465753.git.andyparkins@gmail.com","subject":"[PATCH 3/4] Default to displaying /all/ non-tag refs, not just locals","fromName":"Andy Parkins","fromEmail":"andyparkins@gmail.com","sentAt":"2006-11-02T11:11:22Z","receivedAt":"2006-11-02T11:11:22Z","isPatch":true,"sender":{"key":"andyparkins@gmail.com","avatar":null},"body":"Adds support for display_mode == 2; which shows all non-tag refs.  I've set\ndisplay_mode = 2 by default so it's easily reverted if needed - or if a switch\nfor the old mode is wanted it can be easily added\n\nSigned-off-by: Andy Parkins <andyparkins@gmail.com>\n---\n builtin-branch.c |   32 +++++++++++++++++++++++++-------\n 1 files changed, 25 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin-branch.c b/builtin-branch.c\nindex 85b7007..b88413a 100644\n--- a/builtin-branch.c\n+++ b/builtin-branch.c\n@@ -104,20 +104,38 @@ static void print_ref_list(int display_m\n {\n \tint i;\n \tchar c;\n+\tchar *p;\n \n-\tif (display_mode)\n-\t\tfor_each_remote_ref(append_ref, NULL);\n-\telse\n-\t\tfor_each_branch_ref(append_ref, NULL);\n+\tswitch (display_mode) {\n+\t\tcase 0:\n+\t\t\tfor_each_branch_ref(append_ref, NULL);\n+\t\t\tbreak;\n+\t\tcase 1:\n+\t\t\tfor_each_remote_ref(append_ref, NULL);\n+\t\t\tbreak;\n+\t\tcase 2:\n+\t\t\tfor_each_ref(append_ref, NULL);\n+\t\t\tbreak;\n+\t}\n \n \tqsort(ref_list, ref_index, sizeof(char *), ref_cmp);\n \n \tfor (i = 0; i < ref_index; i++) {\n+\t\tp = ref_list[i];\n+\t\tif (display_mode == 2) {\n+\t\t\tif (!strncmp( p, \"refs/\", 5 ))\n+\t\t\t\tp += 5;\n+\t\t\tif (!strncmp( p, \"tags/\", 5 ))\n+\t\t\t\tcontinue;\n+\t\t\tif (!strncmp( p, \"heads/\", 6 ))\n+\t\t\t\tp += 6;\n+\t\t}\n+\n \t\tc = ' ';\n-\t\tif (!strcmp(ref_list[i], head))\n+\t\tif (!strcmp(p, head))\n \t\t\tc = '*';\n \n-\t\tprintf(\"%c %s\\n\", c, ref_list[i]);\n+\t\tprintf(\"%c %s\\n\", c, p);\n \t}\n }\n \n@@ -160,7 +178,7 @@ static void create_branch(const char *na\n \n int cmd_branch(int argc, const char **argv, const char *prefix)\n {\n-\tint delete = 0, force_delete = 0, force_create = 0, display_mode = 0;\n+\tint delete = 0, force_delete = 0, force_create = 0, display_mode = 2;\n \tint reflog = 0;\n \tint i;\n \n-- \n1.4.3.2\n\n"},{"id":"297476","messageId":"200611021111.32759.andyparkins@gmail.com","threadId":"43131","inReplyTo":"bec6ab7849e3fcacac23cca44a0ba93282af5fca.1162465753.git.andyparkins@gmail.com","subject":"[PATCH 4/4] Show the branch type after the branch name for remotes","fromName":"Andy Parkins","fromEmail":"andyparkins@gmail.com","sentAt":"2006-11-02T11:11:32Z","receivedAt":"2006-11-02T11:11:32Z","isPatch":true,"sender":{"key":"andyparkins@gmail.com","avatar":null},"body":"Instead of prefixing the remote branches with \"remotes/\" suffix them with\n\"[read only]\"\n\nSigned-off-by: Andy Parkins <andyparkins@gmail.com>\n---\n builtin-branch.c |    8 +++++++-\n 1 files changed, 7 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-branch.c b/builtin-branch.c\nindex b88413a..6736882 100644\n--- a/builtin-branch.c\n+++ b/builtin-branch.c\n@@ -105,6 +105,7 @@ static void print_ref_list(int display_m\n \tint i;\n \tchar c;\n \tchar *p;\n+\tconst char *suffix;\n \n \tswitch (display_mode) {\n \t\tcase 0:\n@@ -122,6 +123,7 @@ static void print_ref_list(int display_m\n \n \tfor (i = 0; i < ref_index; i++) {\n \t\tp = ref_list[i];\n+\t\tsuffix = \"\";\n \t\tif (display_mode == 2) {\n \t\t\tif (!strncmp( p, \"refs/\", 5 ))\n \t\t\t\tp += 5;\n@@ -129,13 +131,17 @@ static void print_ref_list(int display_m\n \t\t\t\tcontinue;\n \t\t\tif (!strncmp( p, \"heads/\", 6 ))\n \t\t\t\tp += 6;\n+\t\t\tif (!strncmp( p, \"remotes/\", 8 )) {\n+\t\t\t\tsuffix = \" [read only]\";\n+\t\t\t\tp += 8;\n+\t\t\t}\n \t\t}\n \n \t\tc = ' ';\n \t\tif (!strcmp(p, head))\n \t\t\tc = '*';\n \n-\t\tprintf(\"%c %s\\n\", c, p);\n+\t\tprintf(\"%c %s%s\\n\", c, p, suffix);\n \t}\n }\n \n-- \n1.4.3.2\n"},{"id":"294683","messageId":"7vac39mew2.fsf@assigned-by-dhcp.cox.net","threadId":"43131","inReplyTo":"200611021111.22604.andyparkins@gmail.com","subject":"Re: [PATCH 3/4] Default to displaying /all/ non-tag refs, not just locals","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-11-03T02:40:29Z","receivedAt":"2006-11-03T02:40:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andy Parkins <andyparkins@gmail.com> writes:\n\n> Adds support for display_mode == 2; which shows all non-tag refs.\n\nThat is a change in behaviour and given that we introduced\nremotes for the explicit purpose of not to clutter the local\nbranch namespace, I doubt defaulting to _show_ remotes is a good\nchange.  See the 'bitfield' comment in my other reply.\n\n"},{"id":"296619","messageId":"7vy7qtl0bk.fsf@assigned-by-dhcp.cox.net","threadId":"43131","inReplyTo":"200611021111.32759.andyparkins@gmail.com","subject":"Re: [PATCH 4/4] Show the branch type after the branch name for remotes","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-11-03T02:40:31Z","receivedAt":"2006-11-03T02:40:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andy Parkins <andyparkins@gmail.com> writes:\n\n> Instead of prefixing the remote branches with \"remotes/\" suffix them with\n> \"[read only]\"\n\nThis is a change in UI and while I understand why you want to\nsay r/o instead of remotes/, I think this needs a bit more\nthought and discussion.  People should not be feeding the output\nof \"git branch\" Porcelainish to their scripts, but you'll never\nknow...\n\nBy the way, does \"git branch -r\" (without any of your patches)\neven say \"remotes/\"?\n"},{"id":"298092","messageId":"7v64dxl0bf.fsf@assigned-by-dhcp.cox.net","threadId":"43131","inReplyTo":"200611021111.09434.andyparkins@gmail.com","subject":"Re: [PATCH 2/4] Rename remote_only to display_mode","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-11-03T02:40:36Z","receivedAt":"2006-11-03T02:40:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andy Parkins <andyparkins@gmail.com> writes:\n\n> -static void print_ref_list(int remote_only)\n> +static void print_ref_list(int display_mode)\n>  {\n>  \tint i;\n>  \tchar c;\n>  \n> -\tif (remote_only)\n> +\tif (display_mode)\n>  \t\tfor_each_remote_ref(append_ref, NULL);\n>  \telse\n>  \t\tfor_each_branch_ref(append_ref, NULL);\n\nIf you make this a \"mode\", it probably is better to make 1 and 0\ninto symbolic constants.  This patch taken alone is regression\nin readability.\n\nBy the way, it might make sense to make it bitfields; that would\nallow you to show either one kind or both.\n\nSomething like this untested patch, that is...\n\n\ndiff --git a/builtin-branch.c b/builtin-branch.c\nindex 368b68e..182648c 100644\n--- a/builtin-branch.c\n+++ b/builtin-branch.c\n@@ -79,45 +79,73 @@ static void delete_branches(int argc, co\n \t}\n }\n \n+#define REF_LOCAL_BRANCH\t01\n+#define REF_REMOTE_BRANCH\t02\n+\n static int ref_index, ref_alloc;\n-static char **ref_list;\n+static struct ref_list {\n+\tint kind;\n+\tchar name[FLEX_ARRAY];\n+} **ref_list;\n \n-static int append_ref(const char *refname, const unsigned char *sha1, int flags,\n-\t\tvoid *cb_data)\n+static int append_ref(const char *refname, const unsigned char *sha1,\n+\t\t      int flags, void *cb_data)\n {\n+\tint kinds = *((int*)cb_data);\n+\tint this_kind, strip;\n+\tstruct ref_list *elem;\n+\n+\tif (!strncmp(refname, \"refs/heads/\", 11)) {\n+\t\tthis_kind = REF_LOCAL_BRANCH;\n+\t\tstrip = 11;\n+\t}\n+\telse if (!strncmp(refname, \"refs/remotes/\", 13)) {\n+\t\tthis_kind = REF_REMOTE_BRANCH;\n+\t\tstrip = 13;\n+\t}\n+\telse\n+\t\tthis_kind = 0;\n+\n+\tif ((this_kind & kinds) == 0)\n+\t\treturn 0;\n+\n \tif (ref_index >= ref_alloc) {\n \t\tref_alloc = alloc_nr(ref_alloc);\n \t\tref_list = xrealloc(ref_list, ref_alloc * sizeof(char *));\n \t}\n-\n-\tref_list[ref_index++] = xstrdup(refname);\n+\t\n+\telem = xcalloc(1, sizeof(*elem) + strlen(refname) - strip);\n+\tstrcpy(elem->name, refname + strip); \n+\telem->kind = this_kind;\n+\tref_list[ref_index++] = elem;\n \n \treturn 0;\n }\n \n-static int ref_cmp(const void *r1, const void *r2)\n+static int ref_cmp(const void *r1_, const void *r2_)\n {\n-\treturn strcmp(*(char **)r1, *(char **)r2);\n+\tconst struct ref_list *r1 = *((const struct ref_list **)r1_);\n+\tconst struct ref_list *r2 = *((const struct ref_list **)r2_);\n+\n+\tif (r1->kind != r2->kind)\n+\t\treturn r1->kind - r2->kind;\n+\treturn strcmp(r1->name, r2->name);\n }\n \n-static void print_ref_list(int remote_only)\n+static void print_ref_list(int kinds)\n {\n \tint i;\n \tchar c;\n \n-\tif (remote_only)\n-\t\tfor_each_remote_ref(append_ref, NULL);\n-\telse\n-\t\tfor_each_branch_ref(append_ref, NULL);\n-\n+\tfor_each_ref(append_ref, &kinds);\n \tqsort(ref_list, ref_index, sizeof(char *), ref_cmp);\n \n \tfor (i = 0; i < ref_index; i++) {\n \t\tc = ' ';\n-\t\tif (!strcmp(ref_list[i], head))\n+\t\tif (ref_list[i]->kind == REF_LOCAL_BRANCH &&\n+\t\t    !strcmp(ref_list[i]->name, head))\n \t\t\tc = '*';\n-\n-\t\tprintf(\"%c %s\\n\", c, ref_list[i]);\n+\t\tprintf(\"%c %s\\n\", c, ref_list[i]->name);\n \t}\n }\n \n@@ -160,8 +188,9 @@ static void create_branch(const char *na\n \n int cmd_branch(int argc, const char **argv, const char *prefix)\n {\n-\tint delete = 0, force_delete = 0, force_create = 0, remote_only = 0;\n+\tint delete = 0, force_delete = 0, force_create = 0;\n \tint reflog = 0;\n+\tint kinds = REF_LOCAL_BRANCH;\n \tint i;\n \n \tgit_config(git_default_config);\n@@ -189,7 +218,11 @@ int cmd_branch(int argc, const char **ar\n \t\t\tcontinue;\n \t\t}\n \t\tif (!strcmp(arg, \"-r\")) {\n-\t\t\tremote_only = 1;\n+\t\t\tkinds = REF_REMOTE_BRANCH;\n+\t\t\tcontinue;\n+\t\t}\n+\t\tif (!strcmp(arg, \"-a\")) {\n+\t\t\tkinds = REF_REMOTE_BRANCH | REF_LOCAL_BRANCH;\n \t\t\tcontinue;\n \t\t}\n \t\tif (!strcmp(arg, \"-l\")) {\n@@ -209,7 +242,7 @@ int cmd_branch(int argc, const char **ar\n \tif (delete)\n \t\tdelete_branches(argc - i, argv + i, force_delete);\n \telse if (i == argc)\n-\t\tprint_ref_list(remote_only);\n+\t\tprint_ref_list(kinds);\n \telse if (i == argc - 1)\n \t\tcreate_branch(argv[i], head, force_create, reflog);\n \telse if (i == argc - 2)\n"},{"id":"294619","messageId":"200611030833.42596.andyparkins@gmail.com","threadId":"43131","inReplyTo":"7vy7qtl0bk.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH 4/4] Show the branch type after the branch name for remotes","fromName":"Andy Parkins","fromEmail":"andyparkins@gmail.com","sentAt":"2006-11-03T08:33:40Z","receivedAt":"2006-11-03T08:33:40Z","isPatch":true,"sender":{"key":"andyparkins@gmail.com","avatar":null},"body":"On Friday 2006 November 03 02:40, Junio C Hamano wrote:\n> Andy Parkins <andyparkins@gmail.com> writes:\n> > Instead of prefixing the remote branches with \"remotes/\" suffix them with\n> > \"[read only]\"\n>\n> This is a change in UI and while I understand why you want to\n> say r/o instead of remotes/, I think this needs a bit more\n> thought and discussion.  People should not be feeding the output\n> of \"git branch\" Porcelainish to their scripts, but you'll never\n> know...\n\nI intentionally made this patch in such a way as to leave the original form \navailable.  I haven't added a switch to show the original form, but it's \nthere if it's needed.\n\nThe reason I thought it would be acceptable is that the output changed fairly \nsignificantly when git-branch went builtin.\n\nHere's the original and my \"git-branch -r\" run on my git repository\n$ git-branch -r\nremotes/up/maint\nremotes/up/master\nremotes/up/next\nremotes/up/pu\n$ ./git-branch -r\n  momentum/master\n  up/maint\n  up/master\n  up/next\n  up/pu\n\nI've not touched the \"-r\" path, so this is the same as the unpatched builtin \nbranch.  The \"remotes/\" prefix is removed in refs.c by for_each_remote_ref() \nwith do_for_each_ref(\"refs/remotes/\", fn, 13, cb_data); and the spaces are \nadded because print_ref_list() has only one printing path and that always \nincludes spaces.\n\nFor me personally, I find my git-branch output more useful because in it's \nunswitched form it shows me all branches.  However, I can easily put this \nbehaviour under a switch or, and this would get my vote, put the original \nbehaviour (i.e. show local branches only) under a switch.\n\nWhich would you like?  If any.\n\n> By the way, does \"git branch -r\" (without any of your patches)\n> even say \"remotes/\"?\n\nI haven't touched the \"-r\" path, so that output should be unaffected.  As \nmentioned above, git-branch.sh did include \"remotes/\", new builtin branch \ndoes not, but does include \"  \" at the beginning of every line.\n\n\nAndy\n\n-- \nDr Andy Parkins, M Eng (hons), MIEE\n"},{"id":"296979","messageId":"200611030841.05888.andyparkins@gmail.com","threadId":"43131","inReplyTo":"7v64dxl0bf.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH 2/4] Rename remote_only to display_mode","fromName":"Andy Parkins","fromEmail":"andyparkins@gmail.com","sentAt":"2006-11-03T08:40:55Z","receivedAt":"2006-11-03T08:40:55Z","isPatch":true,"sender":{"key":"andyparkins@gmail.com","avatar":null},"body":"On Friday 2006 November 03 02:40, Junio C Hamano wrote:\n\n> If you make this a \"mode\", it probably is better to make 1 and 0\n> into symbolic constants.  This patch taken alone is regression\n> in readability.\n\nIn my own code I would have done exactly that; however I've been trying to \nkeep my patches as minimal as possible.\n\nDigressing a little: what is the polite form of patches for git?  My strategy \nwith this set was to make each patch as small as possible to reach my end \npoint.  If those patches were okayed on the list, I could then do a \"make \nmore beautiful\" patch, which is really nothing to do with the original \nchanges to functionality but would make the code prettier.  Really I'm asking \nwhat level of intrusiveness of patch is not considered rude?  In making my \npatches, should I ride rough-shod over current implementation and just do it \nhow I'd do it or should I try to fit in (as I did in this case)?\n\n> Something like this untested patch, that is...\n\nI'm very much in favour; I shall make changes of this form soon.\n\n\nAndy\n\n-- \nDr Andy Parkins, M Eng (hons), MIEE\n"},{"id":"295444","messageId":"200611030847.22252.andyparkins@gmail.com","threadId":"43131","inReplyTo":"7vac39mew2.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH 3/4] Default to displaying /all/ non-tag refs, not just locals","fromName":"Andy Parkins","fromEmail":"andyparkins@gmail.com","sentAt":"2006-11-03T08:47:20Z","receivedAt":"2006-11-03T08:47:20Z","isPatch":true,"sender":{"key":"andyparkins@gmail.com","avatar":null},"body":"On Friday 2006 November 03 02:40, Junio C Hamano wrote:\n\n> That is a change in behaviour and given that we introduced\n> remotes for the explicit purpose of not to clutter the local\n> branch namespace, I doubt defaulting to _show_ remotes is a good\n\nReally?  I had imagined it was to prevent accidental checking out of an \nupstream-tracking branch.  Also; I don't think \"not cluttering the namespace\" \nis the same as \"not showing multiple namespaces\".  The local namespace \nremains as uncluttered as it ever was.  This is a question of what to \ndisplay.\n\nAssuming my \"mixed mode\" display thing were in place, doesn't that make the \ntwo choices of UI\n\n1)\n git-branch            : show local branches\n git-branch --all      : show local and remote branches\n git-branch -r         : show remote branches\n2)\n git-branch            : show local and remote branches\n git-branch --local    : show local branches\n git-branch -r         : show remote branches\n\nIn case 2) the switch is simply selecting a filter, and so fits in with \nthe \"-r\" better.\n\n\n\nAndy\n\n-- \nDr Andy Parkins, M Eng (hons), MIEE\n"},{"id":"297350","messageId":"454B1F3B.1020603@op5.se","threadId":"43131","inReplyTo":"200611030841.05888.andyparkins@gmail.com","subject":"Re: [PATCH 2/4] Rename remote_only to display_mode","fromName":"Andreas Ericsson","fromEmail":"ae@op5.se","sentAt":"2006-11-03T10:51:39Z","receivedAt":"2006-11-03T10:51:39Z","isPatch":true,"sender":{"key":"ae@op5.se","avatar":"https://gravatar.com/avatar/426e89595c75a8f5252dd0c989e5fabe5bcac616e68557427ad9aef6b0ca342a?d=mp&s=160"},"body":"Andy Parkins wrote:\n> Digressing a little: what is the polite form of patches for git?  My strategy \n> with this set was to make each patch as small as possible to reach my end \n> point.  If those patches were okayed on the list, I could then do a \"make \n> more beautiful\" patch, which is really nothing to do with the original \n> changes to functionality but would make the code prettier.\n\nI believe the order of preferrence goes: tested, concise, short.\n\nLinus has a nasty habit of ending his mails with \"totally untested \nofcourse\", which is not a good strategy to adopt if you want your \npatches included.\n\n>  Really I'm asking \n> what level of intrusiveness of patch is not considered rude?  In making my \n> patches, should I ride rough-shod over current implementation and just do it \n> how I'd do it or should I try to fit in (as I did in this case)?\n> \n\nIf you *need* to change something, change it. If you *want* to change \nsomething just because it's not written the way you would write it, back \naway. If you think some interface you're using needs clearing up \n(codewise or with extra comments), send a separate patch for that so the \nactual feature/bugfix you're sending in doesn't drown in cosmetic \nchanges to the interfaces the patch uses/touches.\n\n-- \nAndreas Ericsson                   andreas.ericsson@op5.se\nOP5 AB                             www.op5.se\n"},{"id":"293892","messageId":"200611031052.16095.andyparkins@gmail.com","threadId":"43131","inReplyTo":"7v64dxl0bf.fsf@assigned-by-dhcp.cox.net","subject":"[PATCH] Add support to git-branch to show local and remote branches","fromName":"Andy Parkins","fromEmail":"andyparkins@gmail.com","sentAt":"2006-11-03T10:52:15Z","receivedAt":"2006-11-03T10:52:15Z","isPatch":true,"sender":{"key":"andyparkins@gmail.com","avatar":null},"body":"Instead of storing a list of refnames in append_ref, a list of structures is\ncreated.  Each of these stores the refname and a symbolic constant representing\nits type.\n\nThe creation of the list is filtered based on a command line switch; no switch\nmeans \"local branches only\", \"-r\" means \"remote branches only\" (as they always\ndid); but now \"-a\" means \"local branches or remote branches\".\n\nAs a side effect, the list is now not global, but allocated in print_ref_list()\nwhere it used.\n\nAlso a memory leak is plugged, the memory allocated during the list creation\nwas never freed.  This is now done in the new function, tidy_ref_list()\n\nSigned-off-by: Andy Parkins <andyparkins@gmail.com>\n---\n builtin-branch.c |   95 +++++++++++++++++++++++++++++++++++++++++++----------\n 1 files changed, 77 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin-branch.c b/builtin-branch.c\nindex 368b68e..6dd33ee 100644\n--- a/builtin-branch.c\n+++ b/builtin-branch.c\n@@ -79,46 +79,100 @@ static void delete_branches(int argc, co\n \t}\n }\n \n-static int ref_index, ref_alloc;\n-static char **ref_list;\n+#define REF_UNKNOWN_TYPE    0x00\n+#define REF_LOCAL_BRANCH    0x01\n+#define REF_REMOTE_BRANCH   0x02\n+#define REF_TAG             0x04\n+\n+struct ref_item {\n+\tchar *name;\n+\tunsigned int type;\n+};\n+\n+struct ref_list {\n+\tint index, alloc;\n+\tstruct ref_item *list;\n+\tint type_wanted;\n+};\n \n static int append_ref(const char *refname, const unsigned char *sha1, int flags,\n \t\tvoid *cb_data)\n {\n-\tif (ref_index >= ref_alloc) {\n-\t\tref_alloc = alloc_nr(ref_alloc);\n-\t\tref_list = xrealloc(ref_list, ref_alloc * sizeof(char *));\n+\tstruct ref_list *ref_list = (struct ref_list*)(cb_data);\n+\tstruct ref_item *newitem;\n+\tint type = REF_UNKNOWN_TYPE;\n+\n+\t/* Detect type */\n+\tif (!strncmp(refname, \"refs/heads/\", 11)) {\n+\t\ttype = REF_LOCAL_BRANCH;\n+\t\trefname += 11;\n+\t} else if (!strncmp(refname, \"refs/remotes/\", 13)) {\n+\t\ttype = REF_REMOTE_BRANCH;\n+\t\trefname += 13;\n+\t} else if (!strncmp(refname, \"refs/tags/\", 10)) {\n+\t\ttype = REF_TAG;\n+\t\trefname += 10;\n+\t}\n+\n+\t/* Don't add type the caller doesn't want */\n+\tif ((type & ref_list->type_wanted) == 0) {\n+\t\treturn 0;\n+\t}\n+\n+\t/* Resize buffer */\n+\tif (ref_list->index >= ref_list->alloc) {\n+\t\tref_list->alloc = alloc_nr(ref_list->alloc);\n+\t\tref_list->list = xrealloc(ref_list->list,\n+\t\t\t\tref_list->alloc * sizeof(struct ref_item));\n \t}\n \n-\tref_list[ref_index++] = xstrdup(refname);\n+\t/* Record the new item */\n+\tnewitem = &(ref_list->list[ref_list->index++]);\n+\tnewitem->name = xstrdup(refname);\n+\tnewitem->type = type;\n \n \treturn 0;\n }\n \n+static int tidy_ref_list( struct ref_list *ref_list )\n+{\n+\tint i;\n+\tfor (i = 0; i < ref_list->index; i++) {\n+\t\tfree( ref_list->list[i].name );\n+\t}\n+\tfree( ref_list->list );\n+}\n+\n static int ref_cmp(const void *r1, const void *r2)\n {\n+\tstruct ref_item *c1 = (struct ref_item*)(r1),\n+\t\t\t\t\t*c2 = (struct ref_item*)(r2);\n+\tif( c1->type != c2->type )\n+\t\treturn c1->type - c2->type;\n \treturn strcmp(*(char **)r1, *(char **)r2);\n }\n \n-static void print_ref_list(int remote_only)\n+static void print_ref_list( int type_wanted )\n {\n \tint i;\n \tchar c;\n+\tstruct ref_list ref_list;\n \n-\tif (remote_only)\n-\t\tfor_each_remote_ref(append_ref, NULL);\n-\telse\n-\t\tfor_each_branch_ref(append_ref, NULL);\n+\tmemset( &ref_list, 0, sizeof( ref_list ) );\n+\tref_list.type_wanted = type_wanted;\n+\tfor_each_ref(append_ref, &ref_list);\n \n-\tqsort(ref_list, ref_index, sizeof(char *), ref_cmp);\n+\tqsort(ref_list.list, ref_list.index, sizeof(struct ref_item), ref_cmp);\n \n-\tfor (i = 0; i < ref_index; i++) {\n+\tfor (i = 0; i < ref_list.index; i++) {\n \t\tc = ' ';\n-\t\tif (!strcmp(ref_list[i], head))\n+\t\tif (!strcmp(ref_list.list[i].name, head))\n \t\t\tc = '*';\n \n-\t\tprintf(\"%c %s\\n\", c, ref_list[i]);\n+\t\tprintf(\"%c %s\\n\", c, ref_list.list[i].name);\n \t}\n+\n+\ttidy_ref_list( &ref_list );\n }\n \n static void create_branch(const char *name, const char *start,\n@@ -160,9 +214,10 @@ static void create_branch(const char *na\n \n int cmd_branch(int argc, const char **argv, const char *prefix)\n {\n-\tint delete = 0, force_delete = 0, force_create = 0, remote_only = 0;\n+\tint delete = 0, force_delete = 0, force_create = 0;\n \tint reflog = 0;\n \tint i;\n+\tint type_wanted = REF_LOCAL_BRANCH;\n \n \tgit_config(git_default_config);\n \n@@ -189,7 +244,11 @@ int cmd_branch(int argc, const char **ar\n \t\t\tcontinue;\n \t\t}\n \t\tif (!strcmp(arg, \"-r\")) {\n-\t\t\tremote_only = 1;\n+\t\t\ttype_wanted = REF_REMOTE_BRANCH;\n+\t\t\tcontinue;\n+\t\t}\n+\t\tif (!strcmp(arg, \"-a\")) {\n+\t\t\ttype_wanted = REF_LOCAL_BRANCH | REF_REMOTE_BRANCH;\n \t\t\tcontinue;\n \t\t}\n \t\tif (!strcmp(arg, \"-l\")) {\n@@ -209,7 +268,7 @@ int cmd_branch(int argc, const char **ar\n \tif (delete)\n \t\tdelete_branches(argc - i, argv + i, force_delete);\n \telse if (i == argc)\n-\t\tprint_ref_list(remote_only);\n+\t\tprint_ref_list(type_wanted);\n \telse if (i == argc - 1)\n \t\tcreate_branch(argv[i], head, force_create, reflog);\n \telse if (i == argc - 2)\n-- \n1.4.3.2\n"},{"id":"297053","messageId":"454B203C.9040006@op5.se","threadId":"43131","inReplyTo":"200611030847.22252.andyparkins@gmail.com","subject":"Re: [PATCH 3/4] Default to displaying /all/ non-tag refs, not just locals","fromName":"Andreas Ericsson","fromEmail":"ae@op5.se","sentAt":"2006-11-03T10:55:56Z","receivedAt":"2006-11-03T10:55:56Z","isPatch":true,"sender":{"key":"ae@op5.se","avatar":"https://gravatar.com/avatar/426e89595c75a8f5252dd0c989e5fabe5bcac616e68557427ad9aef6b0ca342a?d=mp&s=160"},"body":"Andy Parkins wrote:\n> On Friday 2006 November 03 02:40, Junio C Hamano wrote:\n> \n>> That is a change in behaviour and given that we introduced\n>> remotes for the explicit purpose of not to clutter the local\n>> branch namespace, I doubt defaulting to _show_ remotes is a good\n> \n> Really?  I had imagined it was to prevent accidental checking out of an \n> upstream-tracking branch.  Also; I don't think \"not cluttering the namespace\" \n> is the same as \"not showing multiple namespaces\".  The local namespace \n> remains as uncluttered as it ever was.  This is a question of what to \n> display.\n> \n> Assuming my \"mixed mode\" display thing were in place, doesn't that make the \n> two choices of UI\n> \n> 1)\n>  git-branch            : show local branches\n>  git-branch --all      : show local and remote branches\n>  git-branch -r         : show remote branches\n> 2)\n>  git-branch            : show local and remote branches\n>  git-branch --local    : show local branches\n>  git-branch -r         : show remote branches\n> \n> In case 2) the switch is simply selecting a filter, and so fits in with \n> the \"-r\" better.\n> \n\nI think it'd make more sense if git-branch could instead take a --filter \nparameter that does a simple strncmp(filter, branch, strlen(filter)) to \nsee if it should show a branch or not. That way, \"--filter=remotes\" \nwould work splendidly. \"local\" as keyword to \"--filter\" could possibly \nbe a special case and need documentation.\n\n-- \nAndreas Ericsson                   andreas.ericsson@op5.se\nOP5 AB                             www.op5.se\n"},{"id":"294314","messageId":"7v4ptgagth.fsf@assigned-by-dhcp.cox.net","threadId":"43131","inReplyTo":"454B1F3B.1020603@op5.se","subject":"Re: [PATCH 2/4] Rename remote_only to display_mode","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-11-03T11:51:54Z","receivedAt":"2006-11-03T11:51:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andreas Ericsson <ae@op5.se> writes:\n\n> Andy Parkins wrote:\n>> Digressing a little: what is the polite form of patches for git?  My\n>> strategy with this set was to make each patch as small as possible\n>> to reach my end point.  If those patches were okayed on the list, I\n>> could then do a \"make more beautiful\" patch, which is really nothing\n>> to do with the original changes to functionality but would make the\n>> code prettier.\n>\n> I believe the order of preferrence goes: tested, concise, short.\n>\n> Linus has a nasty habit of ending his mails with \"totally untested\n> ofcourse\", which is not a good strategy to adopt if you want your\n> patches included.\n\nI've picked it up as well.  Consider it a privilege for being\nthe toplevel maintainer ;-).\n\nSeriously, it is perfectly Ok to send \"for discussion\" feelers\nthat are untested or messy, but marking them clearly as \"for\ndiscussion only -- will clean-up after discussion\" would be very\nmuch appreciated.\n\nThe organization of our four series was almost perfect, except\nyou went a bit too far with [2/4].  I said \"taken alone this is\nregression in readability\", with the full knowledge that the\nreal reason of the change was that it would not make any sense\nto call the variable \"remote_only\" in [3/4].  IOW, I would have\nrolled 2 and 3 into a single change.\n\n> If you *need* to change something, change it. If you *want* to change\n> something just because it's not written the way you would write it,\n> back away. If you think some interface you're using needs clearing up\n> (codewise or with extra comments), send a separate patch for that so\n> the actual feature/bugfix you're sending in doesn't drown in cosmetic\n> changes to the interfaces the patch uses/touches.\n\nThis is a very good advice.  I fully agree with Andreas.\n\nWhen I was an active contributor, somebody (I do not remember\nwho) asked me privately: \"you seem to be getting along pretty\nwell with Linus; I have these changes I want to send in, but can\nyou suggest a good strategy to get patches accepted?\"  I recall\nsaying something along this line:\n\n - Make sure the patches apply cleanly to upstream (rebase if\n   necessary).\n\n - When making a series, make clean-ups and obviously correct\n   ones early, then follow them with bigger and potentially more\n   controversial ones.  Doing it the other way around takes the\n   \"obviously correct\" ones hostage to the latter; IOW, you\n   would be effectively saying \"if you swallow this big change,\n   you would get these clean-ups and obviously correct\n   bugfixes\", which is not nice to the maintainer.\n\n - When some of your changes are accepted but others were not,\n   do not give up, if you believe in the cause, try to come up\n   with convincing examples to explain why your change helps\n   certain workflows.\n\n - When doing so, make sure the patches apply cleanly to the\n   updated upstream that now has some of your changes.  It might\n   have been applied with fix-ups, and other people may have\n   made clean-ups in neighbouring area.  You may need to re-roll\n   the patch.\n\n - Linus is not doing this full-time and is a busy person.\n   Although \"not giving up\" is important, do not push him too\n   hard.  Try to guess how busy he is and what area his\n   attention currently is.  The latter is important for two\n   reasons.  (1) If you have to touch the same part of code but\n   for a different reason, that would add more work on him.  It\n   is better wait until the upstream code settles in the part of\n   the code and base your patch on that.  (2) If your change\n   needs a deep thinking to swallow, it is likely to be dropped\n   when Linus's attention is in completely different area.\n\n"},{"id":"294444","messageId":"200611031200.27275.andyparkins@gmail.com","threadId":"43131","inReplyTo":"454B1F3B.1020603@op5.se","subject":"Re: [PATCH 2/4] Rename remote_only to display_mode","fromName":"Andy Parkins","fromEmail":"andyparkins@gmail.com","sentAt":"2006-11-03T12:00:25Z","receivedAt":"2006-11-03T12:00:25Z","isPatch":true,"sender":{"key":"andyparkins@gmail.com","avatar":null},"body":"On Friday 2006 November 03 10:51, Andreas Ericsson wrote:\n\n> If you *need* to change something, change it. If you *want* to change\n> something just because it's not written the way you would write it, back\n> away. If you think some interface you're using needs clearing up\n> (codewise or with extra comments), send a separate patch for that so the\n> actual feature/bugfix you're sending in doesn't drown in cosmetic\n> changes to the interfaces the patch uses/touches.\n\nThank you for the excellent advice.  What then would you suggest in the case \nin point?  I made as minimal a change as I could make; but that left the code \na little bit bitty - I had press-ganged a variable into taking on another \nfunction and was using numeric literals that should really have been given \nmeaning with #define?\n\nMy question is perhaps different from simply git-etiquette; it's should I \nprefer my patches to be minimal or neat?  If there is a more appropriate way \nof doing something should I do it or should I favour minimalism?\n\nI've actually rewritten it now as per Junio's request, and while I'm happier \nwith the code, it was much bigger change, that didn't really lend itself to \nbeing broken into smaller patches as did my first attempt.\n\nI guess in the end it's a judgement call and the best thing to do is post it \nand see who shoots it down :-)\n\n\nAndy\n\n-- \nDr Andy Parkins, M Eng (hons), MIEE\n"},{"id":"295878","messageId":"200611031206.12515.andyparkins@gmail.com","threadId":"43131","inReplyTo":"200611031052.16095.andyparkins@gmail.com","subject":"[PATCH] Colourise git-branch output","fromName":"Andy Parkins","fromEmail":"andyparkins@gmail.com","sentAt":"2006-11-03T12:06:12Z","receivedAt":"2006-11-03T12:06:12Z","isPatch":true,"sender":{"key":"andyparkins@gmail.com","avatar":null},"body":"I wanted to have a visual indication of which branches are local and which are\nremote in git-branch -a output; however Junio was concerned that someone might\nbe using the output in a script.  This patch addresses the problem by colouring\nthe git-branch output - which in \"auto\" mode won't be activated.\n\nI've based it off the colouring code for builtin-diff.c; which means there is a\nbranch.color configuration variable that needs setting to something before the\ncolor will appear.\n\nThis patch chooses green for local, red for remote and bold green for current.\n\nAs yet, there is no support for changing the colors using the config file; but\nit wouldn't be hard to add.\n\nSigned-off-by: Andy Parkins <andyparkins@gmail.com>\n---\n builtin-branch.c |   57 +++++++++++++++++++++++++++++++++++++++++++++++++++--\n 1 files changed, 54 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin-branch.c b/builtin-branch.c\nindex 6dd33ee..de7f81e 100644\n--- a/builtin-branch.c\n+++ b/builtin-branch.c\n@@ -5,6 +5,7 @@\n  * Based on git-branch.sh by Junio C Hamano.\n  */\n \n+#include \"color.h\"\n #include \"cache.h\"\n #include \"refs.h\"\n #include \"commit.h\"\n@@ -17,6 +18,38 @@ static const char builtin_branch_usage[]\n static const char *head;\n static unsigned char head_sha1[20];\n \n+static int branch_use_color;\n+static char branch_colors[][COLOR_MAXLEN] = {\n+\t\"\\033[m\",\t/* reset */\n+\t\"\",\t\t/* PLAIN (normal) */\n+\t\"\\033[31m\",\t/* REMOTE (red) */\n+\t\"\\033[32m\",\t/* LOCAL (green) */\n+\t\"\\033[1;32m\",\t/* CURRENT (boldgreen) */\n+};\n+enum color_branch {\n+\tCOLOR_BRANCH_RESET = 0,\n+\tCOLOR_BRANCH_PLAIN = 1,\n+\tCOLOR_BRANCH_REMOTE = 2,\n+\tCOLOR_BRANCH_LOCAL = 3,\n+\tCOLOR_BRANCH_CURRENT = 4,\n+};\n+\n+int git_branch_config(const char *var, const char *value)\n+{\n+\tif (!strcmp(var, \"branch.color\")) {\n+\t\tbranch_use_color = git_config_colorbool(var, value);\n+\t\treturn 0;\n+\t}\n+\treturn git_default_config(var, value);\n+}\n+\n+const char *branch_get_color(enum color_branch ix)\n+{\n+\tif (branch_use_color)\n+\t\treturn branch_colors[ix];\n+\treturn \"\";\n+}\n+\n static int in_merge_bases(const unsigned char *sha1,\n \t\t\t  struct commit *rev1,\n \t\t\t  struct commit *rev2)\n@@ -157,6 +190,7 @@ static void print_ref_list( int type_wan\n \tint i;\n \tchar c;\n \tstruct ref_list ref_list;\n+\tint color;\n \n \tmemset( &ref_list, 0, sizeof( ref_list ) );\n \tref_list.type_wanted = type_wanted;\n@@ -165,11 +199,28 @@ static void print_ref_list( int type_wan\n \tqsort(ref_list.list, ref_list.index, sizeof(struct ref_item), ref_cmp);\n \n \tfor (i = 0; i < ref_list.index; i++) {\n+\t\tswitch( ref_list.list[i].type ) {\n+\t\t\tcase REF_LOCAL_BRANCH:\n+\t\t\t\tcolor = COLOR_BRANCH_LOCAL;\n+\t\t\t\tbreak;\n+\t\t\tcase REF_REMOTE_BRANCH:\n+\t\t\t\tcolor = COLOR_BRANCH_REMOTE;\n+\t\t\t\tbreak;\n+\t\t\tdefault:\n+\t\t\t\tcolor = COLOR_BRANCH_PLAIN;\n+\t\t\t\tbreak;\n+\t\t}\n+\n \t\tc = ' ';\n-\t\tif (!strcmp(ref_list.list[i].name, head))\n+\t\tif (!strcmp(ref_list.list[i].name, head)) {\n \t\t\tc = '*';\n+\t\t\tcolor = COLOR_BRANCH_CURRENT;\n+\t\t}\n \n-\t\tprintf(\"%c %s\\n\", c, ref_list.list[i].name);\n+\t\tprintf(\"%c %s%s%s\\n\", c,\n+\t\t\t\tbranch_get_color(color),\n+\t\t\t\tref_list.list[i].name,\n+\t\t\t\tbranch_get_color(COLOR_BRANCH_RESET));\n \t}\n \n \ttidy_ref_list( &ref_list );\n@@ -219,7 +270,7 @@ int cmd_branch(int argc, const char **ar\n \tint i;\n \tint type_wanted = REF_LOCAL_BRANCH;\n \n-\tgit_config(git_default_config);\n+\tgit_config(git_branch_config);\n \n \tfor (i = 1; i < argc; i++) {\n \t\tconst char *arg = argv[i];\n-- \n1.4.3.2\n"},{"id":"297847","messageId":"7vhcxg91gq.fsf@assigned-by-dhcp.cox.net","threadId":"43131","inReplyTo":"200611031052.16095.andyparkins@gmail.com","subject":"Re: [PATCH] Add support to git-branch to show local and remote branches","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-11-03T12:08:53Z","receivedAt":"2006-11-03T12:08:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andy Parkins <andyparkins@gmail.com> writes:\n\n> Instead of storing a list of refnames in append_ref, a list of\n> structures is created.  Each of these stores the refname and a\n> symbolic constant representing its type.\n>\n> The creation of the list is filtered based on a command line\n> switch; no switch means \"local branches only\", \"-r\" means\n> \"remote branches only\" (as they always did); but now \"-a\"\n> means \"local branches or remote branches\".\n>\n> As a side effect, the list is now not global, but allocated in\n> print_ref_list() where it used.\n>\n> Also a memory leak is plugged, the memory allocated during the\n> list creation was never freed.  This is now done in the new\n> function, tidy_ref_list()\n\nI would not call that a \"leak\" given that print_ref_list() is\nthe last thing to be called before the command exits.  I'd\nrather not to spend cycles calling free().\n\n> +static int tidy_ref_list( struct ref_list *ref_list )\n\nStyle.  No spaces before or after parameter list.\n\nI see you already parse \"refs/tags\" prefix. \"git branch\" would\nnot print tags, but that part might be useful when we want to\nredo git-tag in C.\n\nOther than that, I think it is equivalent to what I have in \"pu\"\nright now.\n\n"},{"id":"297466","messageId":"200611031237.35290.andyparkins@gmail.com","threadId":"43131","inReplyTo":"7v4ptgagth.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH 2/4] Rename remote_only to display_mode","fromName":"Andy Parkins","fromEmail":"andyparkins@gmail.com","sentAt":"2006-11-03T12:37:33Z","receivedAt":"2006-11-03T12:37:33Z","isPatch":true,"sender":{"key":"andyparkins@gmail.com","avatar":null},"body":"On Friday 2006 November 03 11:51, Junio C Hamano wrote:\n\n> Seriously, it is perfectly Ok to send \"for discussion\" feelers\n\nThank you for all the excellent advice.  Being new here I'm basically trying \nnot to make a nuisance of myself :-)\n\nI'll worry less about getting these things right first time; no one here goes \nmad if a patch is wrong and I'm quite enjoying myself really :-)\n\n\n\nAndy\n\n-- \nDr Andy Parkins, M Eng (hons), MIEE\n"},{"id":"295578","messageId":"200611031240.34177.andyparkins@gmail.com","threadId":"43131","inReplyTo":"7vhcxg91gq.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Add support to git-branch to show local and remote branches","fromName":"Andy Parkins","fromEmail":"andyparkins@gmail.com","sentAt":"2006-11-03T12:40:30Z","receivedAt":"2006-11-03T12:40:30Z","isPatch":true,"sender":{"key":"andyparkins@gmail.com","avatar":null},"body":"On Friday 2006 November 03 12:08, Junio C Hamano wrote:\n\n> > +static int tidy_ref_list( struct ref_list *ref_list )\n>\n> Style.  No spaces before or after parameter list.\n\nBah!  It's so hard getting my fingers to remember what style goes in which \nproject :-)\n\n> I see you already parse \"refs/tags\" prefix. \"git branch\" would\n> not print tags, but that part might be useful when we want to\n> redo git-tag in C.\n\nI'm going to have a look at that soon; I've got enough of a hang of things \nwith this git-branch work that I think I could cope with writing git-tag in \nC.\n\n\nAndy\n-- \nDr Andy Parkins, M Eng (hons), MIEE\n"},{"id":"296585","messageId":"454B42E2.8070001@op5.se","threadId":"43131","inReplyTo":"200611031200.27275.andyparkins@gmail.com","subject":"Re: [PATCH 2/4] Rename remote_only to display_mode","fromName":"Andreas Ericsson","fromEmail":"ae@op5.se","sentAt":"2006-11-03T13:23:46Z","receivedAt":"2006-11-03T13:23:46Z","isPatch":true,"sender":{"key":"ae@op5.se","avatar":"https://gravatar.com/avatar/426e89595c75a8f5252dd0c989e5fabe5bcac616e68557427ad9aef6b0ca342a?d=mp&s=160"},"body":"Andy Parkins wrote:\n> On Friday 2006 November 03 10:51, Andreas Ericsson wrote:\n> \n>> If you *need* to change something, change it. If you *want* to change\n>> something just because it's not written the way you would write it, back\n>> away. If you think some interface you're using needs clearing up\n>> (codewise or with extra comments), send a separate patch for that so the\n>> actual feature/bugfix you're sending in doesn't drown in cosmetic\n>> changes to the interfaces the patch uses/touches.\n> \n> Thank you for the excellent advice.  What then would you suggest in the case \n> in point?  I made as minimal a change as I could make; but that left the code \n> a little bit bitty - I had press-ganged a variable into taking on another \n> function and was using numeric literals that should really have been given \n> meaning with #define?\n> \n> My question is perhaps different from simply git-etiquette; it's should I \n> prefer my patches to be minimal or neat?  If there is a more appropriate way \n> of doing something should I do it or should I favour minimalism?\n> \n\nNeat, imo. Re-using old variables might be appropriate if the name of \nthe variable still makes sense, but rename it if there's a better name \nfor it.\n\n> I've actually rewritten it now as per Junio's request, and while I'm happier \n> with the code, it was much bigger change, that didn't really lend itself to \n> being broken into smaller patches as did my first attempt.\n> \n> I guess in the end it's a judgement call and the best thing to do is post it \n> and see who shoots it down :-)\n> \n\nProbably the most sensible approach. Even though the list is pretty \ntrigger-happy, the guns are more of the playful water-squirt type than \nthe high-powered big-calibre kind.\n\n-- \nAndreas Ericsson                   andreas.ericsson@op5.se\nOP5 AB                             www.op5.se\n"},{"id":"297772","messageId":"7vac388hig.fsf@assigned-by-dhcp.cox.net","threadId":"43131","inReplyTo":"200611031240.34177.andyparkins@gmail.com","subject":"Re: [PATCH] Add support to git-branch to show local and remote branches","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-11-03T19:19:51Z","receivedAt":"2006-11-03T19:19:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andy Parkins <andyparkins@gmail.com> writes:\n\n> On Friday 2006 November 03 12:08, Junio C Hamano wrote:\n>\n>> > +static int tidy_ref_list( struct ref_list *ref_list )\n>>\n>> Style.  No spaces before or after parameter list.\n>\n> Bah!  It's so hard getting my fingers to remember what style goes in which \n> project :-)\n>\n>> I see you already parse \"refs/tags\" prefix. \"git branch\" would\n>> not print tags, but that part might be useful when we want to\n>> redo git-tag in C.\n>\n> I'm going to have a look at that soon;...\n\nJust to make sure you do not misunderstand.  I do not see much\nneed for rewriting git-tag in C right now, and often it is more\nconvenient to leave higher layer commands in shell scripts to\nallow people to try out new things and flush out UI issues.  I\nwas merely pointing out that part of the code would be usable if\nwe were to do it.\n\n \n"},{"id":"298102","messageId":"7v3b908h88.fsf@assigned-by-dhcp.cox.net","threadId":"43131","inReplyTo":"200611031206.12515.andyparkins@gmail.com","subject":"Re: [PATCH] Colourise git-branch output","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-11-03T19:25:59Z","receivedAt":"2006-11-03T19:25:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andy Parkins <andyparkins@gmail.com> writes:\n\n> I wanted to have a visual indication of which branches are\n> local and which are remote in git-branch -a output; however\n> Junio was concerned that someone might be using the output in\n> a script.  This patch addresses the problem by colouring the\n> git-branch output - which in \"auto\" mode won't be activated.\n\nYuck.  We are getting more and more color happy.  As long as\nthis stays optional I'm Ok with it; we'll see if people find it\nuseful soon enough.\n\n> @@ -165,11 +199,28 @@ static void print_ref_list( int type_wan\n>  \tqsort(ref_list.list, ref_list.index, sizeof(struct ref_item), ref_cmp);\n>  \n>  \tfor (i = 0; i < ref_list.index; i++) {\n> +\t\tswitch( ref_list.list[i].type ) {\n> +\t\t\tcase REF_LOCAL_BRANCH:\n> +\t\t\t\tcolor = COLOR_BRANCH_LOCAL;\n> +\t\t\t\tbreak;\n\nStyle.  SP between \"switch\" and open parenthesis, no SP after\nthat open parenthesis.  We tend to align \"switch\", \"case\", and\n\"default\" on the same column.\n"},{"id":"298585","messageId":"7vmz785m4e.fsf@assigned-by-dhcp.cox.net","threadId":"43131","inReplyTo":"200611030833.42596.andyparkins@gmail.com","subject":"Re: [PATCH 4/4] Show the branch type after the branch name for remotes","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-11-03T20:08:33Z","receivedAt":"2006-11-03T20:08:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andy Parkins <andyparkins@gmail.com> writes:\n\n> For me personally, I find my git-branch output more useful because in it's \n> unswitched form it shows me all branches.  However, I can easily put this \n> behaviour under a switch or, and this would get my vote, put the original \n> behaviour (i.e. show local branches only) under a switch.\n>\n> Which would you like?  If any.\n\nI am a wrong person to ask, since I do not usually work in\nrepositories with refs/remotes/ myself and the choice would not\naffect me either way.\n\nFor people who work with multiple remotes (e.g. following both\nLinus and Jeff) and work on many separate topic branches of\ntheir own, I would suspect that showing only local branches by\ndefault, with an option to show remote branches from only named\nremotes would make things less cluttered, and we can always have\n\"-a\" to mean everything under the sun.\n\nThis kind of limiting can be done more flexibly with globbing\nthan just fixed set of flags.\n\nWe could have:\n\n\tgit branch --list 'heads/'\n        git branch --list 'remotes/jgarzik/'\n        git branch --list 'heads/??/*'\n\nand that command, crazily enough, would let you do:\n\n        git branch --list 'tags/v2.6.19-rc*'\n\nHmm?\n"},{"id":"294521","messageId":"20061103230524.GA4429@steel.home","threadId":"43131","inReplyTo":"7v3b908h88.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Colourise git-branch output","fromName":"Alex Riesen","fromEmail":"fork0@t-online.de","sentAt":"2006-11-03T23:05:24Z","receivedAt":"2006-11-03T23:05:24Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Junio C Hamano, Fri, Nov 03, 2006 20:25:59 +0100:\n> > I wanted to have a visual indication of which branches are\n> > local and which are remote in git-branch -a output; however\n> > Junio was concerned that someone might be using the output in\n> > a script.  This patch addresses the problem by colouring the\n> > git-branch output - which in \"auto\" mode won't be activated.\n> \n> Yuck.  We are getting more and more color happy.  As long as\n> this stays optional I'm Ok with it; we'll see if people find it\n> useful soon enough.\n> \n\nAs long as the output stays stable one can always colorize it with\nan external program, using pseudo terminals, if needed. Wont work\nfor the other platform though. As usual.\n"}]}