{"thread":{"id":"45753","subject":"[PATCH] Add color slots for branch names in \"git status --short --branch\"","startedAt":"2017-04-20T05:57:22Z","lastAt":"2017-04-27T08:54:21Z","messageCount":5,"participants":["Stephen Kent","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"317310","messageId":"201704556286334.8b7dc718029e6dd189dadb3703bfa@localhost","threadId":"45753","inReplyTo":null,"subject":"[PATCH] Add color slots for branch names in \"git status --short --branch\"","fromName":"Stephen Kent","fromEmail":"smkent@smkent.net","sentAt":"2017-04-20T05:57:08Z","receivedAt":"2017-04-20T05:57:22Z","isPatch":true,"sender":{"key":"smkent@smkent.net","avatar":"https://avatars.githubusercontent.com/u/2831985?v=4"},"body":"Signed-off-by: Stephen Kent <smkent@smkent.net>\n---\n Documentation/config.txt               | 5 ++++-\n builtin/commit.c                       | 4 ++++\n contrib/completion/git-completion.bash | 2 ++\n 3 files changed, 10 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 475e874..96e9cf8 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1137,7 +1137,10 @@ color.status.<slot>::\n \t`untracked` (files which are not tracked by Git),\n \t`branch` (the current branch),\n \t`nobranch` (the color the 'no branch' warning is shown in, defaulting\n-\tto red), or\n+\tto red),\n+\t`localBranch` or `remoteBranch` (the local and remote branch names,\n+\trespectively, when branch and tracking information is displayed in the\n+\tstatus short-format), or\n \t`unmerged` (files which have unmerged changes).\n \n color.ui::\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 4e288bc..43846d5 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1263,6 +1263,10 @@ static int parse_status_slot(const char *slot)\n \t\treturn WT_STATUS_NOBRANCH;\n \tif (!strcasecmp(slot, \"unmerged\"))\n \t\treturn WT_STATUS_UNMERGED;\n+\tif (!strcasecmp(slot, \"localBranch\"))\n+\t\treturn WT_STATUS_LOCAL_BRANCH;\n+\tif (!strcasecmp(slot, \"remoteBranch\"))\n+\t\treturn WT_STATUS_REMOTE_BRANCH;\n \treturn -1;\n }\n \ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 1150164..f0542b6 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -2377,7 +2377,9 @@ _git_config ()\n \t\tcolor.status.added\n \t\tcolor.status.changed\n \t\tcolor.status.header\n+\t\tcolor.status.localBranch\n \t\tcolor.status.nobranch\n+\t\tcolor.status.remoteBranch\n \t\tcolor.status.unmerged\n \t\tcolor.status.untracked\n \t\tcolor.status.updated\n-- \n1.9.1\n\n"},{"id":"317311","messageId":"20170420061653.y3bhdzw6sx5zavfc@sigill.intra.peff.net","threadId":"45753","inReplyTo":"201704556286334.8b7dc718029e6dd189dadb3703bfa@localhost","subject":"Re: [PATCH] Add color slots for branch names in \"git status --short --branch\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-20T06:16:53Z","receivedAt":"2017-04-20T06:17:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Overall this looks good. A few minor nits:\n\nOn Wed, Apr 19, 2017 at 10:57:08PM -0700, Stephen Kent wrote:\n\n> Subject: Re: [PATCH] Add color slots for branch names in \"git status --short\n\nWe usually try to use \"subsystem: blah\" for our subjects, which makes\nthem easy to parse when you're looking through a oneline. So probably:\n\n  status: add color config slots for branch names\n\nor something.\n\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 475e874..96e9cf8 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -1137,7 +1137,10 @@ color.status.<slot>::\n>  \t`untracked` (files which are not tracked by Git),\n>  \t`branch` (the current branch),\n>  \t`nobranch` (the color the 'no branch' warning is shown in, defaulting\n> -\tto red), or\n> +\tto red),\n> +\t`localBranch` or `remoteBranch` (the local and remote branch names,\n> +\trespectively, when branch and tracking information is displayed in the\n> +\tstatus short-format), or\n\nI wondered if this \"short-format\" was accurate. But indeed, we do not\nseem to color the local/remote branch specially in long-format mode, so\nit really is only the short format that is affected.\n\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 4e288bc..43846d5 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -1263,6 +1263,10 @@ static int parse_status_slot(const char *slot)\n>  \t\treturn WT_STATUS_NOBRANCH;\n>  \tif (!strcasecmp(slot, \"unmerged\"))\n>  \t\treturn WT_STATUS_UNMERGED;\n> +\tif (!strcasecmp(slot, \"localBranch\"))\n> +\t\treturn WT_STATUS_LOCAL_BRANCH;\n> +\tif (!strcasecmp(slot, \"remoteBranch\"))\n> +\t\treturn WT_STATUS_REMOTE_BRANCH;\n\nNormally we match config names in the code as all lowercase, since the\nkey names we get from the config parser will be normalized. Here it\nworks with your mixed-case because you're using strcasecmp(). Obviously\nthat was picked up from the surrounding code, but I think those existing\nstrcasecmp() calls could (and perhaps should) just be strcmp().\n\nI don't know if it's worth converting them or not. If we leave them all\nas strcasecmp(), I don't mind your camelCase names, for readability.\n\n-Peff\n"},{"id":"317312","messageId":"xmqqzifb8ubt.fsf@gitster.mtv.corp.google.com","threadId":"45753","inReplyTo":"201704556286334.8b7dc718029e6dd189dadb3703bfa@localhost","subject":"Re: [PATCH] Add color slots for branch names in \"git status --short --branch\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-20T06:30:14Z","receivedAt":"2017-04-20T06:30:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stephen Kent <smkent@smkent.net> writes:\n\n> Subject: Re: [PATCH] Add color slots for branch names in \"git status --short --branch\"\n\nWe spell one-liner title of our commits as \"<area>: <summary>\"\ntypically.  In this case, this is about the output from the status\ncommand, so\n\n\tstatus: make the color used \"--shrot --branch\" output configurable\n\nor something, perhaps?\n\n> Signed-off-by: Stephen Kent <smkent@smkent.net>\n> ---\n>  Documentation/config.txt               | 5 ++++-\n>  builtin/commit.c                       | 4 ++++\n>  contrib/completion/git-completion.bash | 2 ++\n>  3 files changed, 10 insertions(+), 1 deletion(-)\n>\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 475e874..96e9cf8 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -1137,7 +1137,10 @@ color.status.<slot>::\n>  \t`untracked` (files which are not tracked by Git),\n>  \t`branch` (the current branch),\n>  \t`nobranch` (the color the 'no branch' warning is shown in, defaulting\n> -\tto red), or\n> +\tto red),\n> +\t`localBranch` or `remoteBranch` (the local and remote branch names,\n> +\trespectively, when branch and tracking information is displayed in the\n> +\tstatus short-format), or\n>  \t`unmerged` (files which have unmerged changes).\n\nOK.\n\n>  color.ui::\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 4e288bc..43846d5 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -1263,6 +1263,10 @@ static int parse_status_slot(const char *slot)\n>  \t\treturn WT_STATUS_NOBRANCH;\n>  \tif (!strcasecmp(slot, \"unmerged\"))\n>  \t\treturn WT_STATUS_UNMERGED;\n> +\tif (!strcasecmp(slot, \"localBranch\"))\n> +\t\treturn WT_STATUS_LOCAL_BRANCH;\n> +\tif (!strcasecmp(slot, \"remoteBranch\"))\n> +\t\treturn WT_STATUS_REMOTE_BRANCH;\n>  \treturn -1;\n>  }\n\nOK.\n\nI know we do not test color.status.<slot> at all (other than t4026\nthat makes sure a configuration from future version of Git that\nspecifies a slot that is not yet known to our version of Git is\nsafely ignored without triggering an error), but perhaps we would\nwant a new test or two at the end of t7508?\n\nThanks.\n"},{"id":"317535","messageId":"201704235826023.e27fbbda3fdd0930bd9ff5eaaba99@localhost","threadId":"45753","inReplyTo":"xmqqzifb8ubt.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] Add color slots for branch names in \"git status --short --branch\"","fromName":"Stephen Kent","fromEmail":"smkent@smkent.net","sentAt":"2017-04-22T05:40:30Z","receivedAt":"2017-04-22T05:40:53Z","isPatch":true,"sender":{"key":"smkent@smkent.net","avatar":"https://avatars.githubusercontent.com/u/2831985?v=4"},"body":"Hi Peff and Junio,\n\nI've updated the commit message and updated one of the existing unit \ntests for this feature. Patch version 2 will follow shortly after this \nemail.\n\nResponses to both of your comments:\n\n> I wondered if this \"short-format\" was accurate. But indeed, we do not \n> seem to color the local/remote branch specially in long-format mode, \n> so it really is only the short format that is affected.\n\nRight, the hardcoded red and green colors only seem to be used for the \nbranch and remote tracking branch names (and commit counts) in the \nstatus short-format.\n\nThere is an existing color slot \"color.status.branch\" for the branch \nname in the default (long) status format which is different than the new \ncolor config slots this patch adds.\n\nI'm wondering if it makes sense to also use color.status.branch for the \nlocal branch color in short-format. On the other hand, I have configured \ndifferent colors in the short and long status format for the local \nbranch name and I find it useful for them to be separate color slots.\n\n> Normally we match config names in the code as all lowercase, since the \n> key names we get from the config parser will be normalized. Here it \n> works with your mixed-case because you're using strcasecmp(). \n> Obviously that was picked up from the surrounding code, but I think \n> those existing strcasecmp() calls could (and perhaps should) just be \n> strcmp().\n\n> I don't know if it's worth converting them or not. If we leave them \n> all as strcasecmp(), I don't mind your camelCase names, for \n> readability.\n\nI chose the localBranch and remoteBranch camel case names for \nconsistency with the existing \"color.decorate.remoteBranch\" color config \nslot in log-tree.c. The documentation for color.decorate.remoteBranch \nuses that camel case name, but the config option is case-insensitive. \n\nI'm happy to use whatever names are best for the short status branch \nname color slots. Let me know if I should change them and what I should \nreplace them with.\n\n> I know we do not test color.status.<slot> at all (other than t4026 \n> that makes sure a configuration from future version of Git that \n> specifies a slot that is not yet known to our version of Git is safely \n> ignored without triggering an error), but perhaps we would want a new \n> test or two at the end of t7508?\n\nI see the existing tests for git status in t7508. The unit tests set up \nsome mock repository modifications to test git status output, so I've \nmodified one of the tests to include a custom color for the local branch \nin git status -sb.\n\nt7508 doesn't seem to contain any tests that include an ahead or behind \ncommit count, so I didn't make any test changes for the remote tracking \nbranch color. What's the best course of action here?\n\nThanks!\n\nStephen\n\n\n\nOn Wed, Apr 19, 2017 at 23:30, Junio C Hamano wrote:\n> Stephen Kent <smkent@smkent.net> writes:\n>\n>> Subject: Re: [PATCH] Add color slots for branch names in \"git status \n>> --short --branch\"\n>\n> We spell one-liner title of our commits as \"<area>: <summary>\" \n> typically.  In this case, this is about the output from the status \n> command, so\n>\n> \tstatus: make the color used \"--shrot --branch\" output configurable\n>\n> or something, perhaps?\n>\n>> Signed-off-by: Stephen Kent <smkent@smkent.net>\n>> ---\n>>  Documentation/config.txt               | 5 ++++-\n>>  builtin/commit.c                       | 4 ++++ \n>>  contrib/completion/git-completion.bash | 2 ++\n>>  3 files changed, 10 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/Documentation/config.txt b/Documentation/config.txt \n>> index 475e874..96e9cf8 100644\n>> --- a/Documentation/config.txt\n>> +++ b/Documentation/config.txt\n>> @@ -1137,7 +1137,10 @@ color.status.<slot>::\n>>  \t`untracked` (files which are not tracked by Git),\n>>  \t`branch` (the current branch),\n>>  \t`nobranch` (the color the 'no branch' warning is shown in, \n>>  \tdefaulting -\tto red), or\n>> +\tto red),\n>> +\t`localBranch` or `remoteBranch` (the local and remote branch\n>> names, +\trespectively, when branch and tracking information is \n>> displayed in the +\tstatus short-format), or\n>>  \t`unmerged` (files which have unmerged changes).\n>\n> OK.\n>\n>>  color.ui::\n>> diff --git a/builtin/commit.c b/builtin/commit.c\n>> index 4e288bc..43846d5 100644\n>> --- a/builtin/commit.c\n>> +++ b/builtin/commit.c\n>> @@ -1263,6 +1263,10 @@ static int parse_status_slot(const char *slot) \n>> return WT_STATUS_NOBRANCH;\n>>  \tif (!strcasecmp(slot, \"unmerged\"))\n>>  \t\treturn WT_STATUS_UNMERGED;\n>> +\tif (!strcasecmp(slot, \"localBranch\"))\n>> +\t\treturn WT_STATUS_LOCAL_BRANCH;\n>> +\tif (!strcasecmp(slot, \"remoteBranch\"))\n>> +\t\treturn WT_STATUS_REMOTE_BRANCH;\n>>  \treturn -1;\n>>  }\n>\n> OK.\n>\n> I know we do not test color.status.<slot> at all (other than t4026 \n> that makes sure a configuration from future version of Git that \n> specifies a slot that is not yet known to our version of Git is\n> safely ignored without triggering an error), but perhaps we would\n> want a new test or two at the end of t7508?\n>\n> Thanks.\n"},{"id":"318060","messageId":"20170427085408.ugbor7mb6hflalh2@sigill.intra.peff.net","threadId":"45753","inReplyTo":"201704235826023.e27fbbda3fdd0930bd9ff5eaaba99@localhost","subject":"Re: [PATCH] Add color slots for branch names in \"git status --short --branch\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-27T08:54:08Z","receivedAt":"2017-04-27T08:54:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 21, 2017 at 10:40:30PM -0700, Stephen Kent wrote:\n\n> I've updated the commit message and updated one of the existing unit tests\n> for this feature. Patch version 2 will follow shortly after this email.\n\nThanks, and sorry for the slow reply.\n\n> There is an existing color slot \"color.status.branch\" for the branch name in\n> the default (long) status format which is different than the new color\n> config slots this patch adds.\n> \n> I'm wondering if it makes sense to also use color.status.branch for the\n> local branch color in short-format. On the other hand, I have configured\n> different colors in the short and long status format for the local branch\n> name and I find it useful for them to be separate color slots.\n\nHrm. That does kind of make sense to me. But I'm not sure it is worth\nthe backwards-compatibility weirdness. E.g., if I have\ncolor.status.branch set to \"red\" right now, that may look OK in the long\nstatus. But if we started picking it up for the local branch in \"git\nstatus --short -b\", then you'd end up with two red names.\n\nSo I think we can probably just call it a historical wart that the short\nand long formats use two different color config schemes.\n\n> > I don't know if it's worth converting them or not. If we leave them all\n> > as strcasecmp(), I don't mind your camelCase names, for readability.\n> \n> I chose the localBranch and remoteBranch camel case names for consistency\n> with the existing \"color.decorate.remoteBranch\" color config slot in\n> log-tree.c. The documentation for color.decorate.remoteBranch uses that\n> camel case name, but the config option is case-insensitive.\n\nAh, I knew as soon as I said \"we usually\" that you would reveal a\ncounter-example from the code. :)\n\nI think it's OK to leave it as-is for your patch. If somebody wants to\nclean up the useless strcasecmps in the slot name parsers, we can do\nthat separately (but it's not really hurting anything, so it may not be\nworth caring about).\n\n> I see the existing tests for git status in t7508. The unit tests set up some\n> mock repository modifications to test git status output, so I've modified\n> one of the tests to include a custom color for the local branch in git\n> status -sb.\n\nWhat you have in your v2 makes sense. Usually we try to avoid modifying\nexisting tests (as opposed to adding new ones), because it mixes up what\nis actually being tested. But in this case, the test is exactly about\ntesting the colors, so that's the right place to put it.\n\n> t7508 doesn't seem to contain any tests that include an ahead or behind\n> commit count, so I didn't make any test changes for the remote tracking\n> branch color. What's the best course of action here?\n\nI think it would be reasonable to set up an ahead/behind situation at\nthe start of the script so that we exercise that code through the\nvarious invocations. So I tried that, and behold, it found a bug. :)\n\nI'll post the patch in a minute.\n\n-Peff\n"}]}