git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
* [PATCH] Add color slots for branch names in "git status --short --branch"
@ 2017-04-20  5:57 Stephen Kent
  2017-04-20  6:16 ` Jeff King
  2017-04-20  6:30 ` Junio C Hamano
  0 siblings, 2 replies; 4+ messages in thread
From: Stephen Kent @ 2017-04-20  5:57 UTC (permalink / raw)
  To: git; +Cc: Jeff King

Signed-off-by: Stephen Kent <smkent@smkent.net>
---
 Documentation/config.txt               | 5 ++++-
 builtin/commit.c                       | 4 ++++
 contrib/completion/git-completion.bash | 2 ++
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 475e874..96e9cf8 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1137,7 +1137,10 @@ color.status.<slot>::
 	`untracked` (files which are not tracked by Git),
 	`branch` (the current branch),
 	`nobranch` (the color the 'no branch' warning is shown in, defaulting
-	to red), or
+	to red),
+	`localBranch` or `remoteBranch` (the local and remote branch names,
+	respectively, when branch and tracking information is displayed in the
+	status short-format), or
 	`unmerged` (files which have unmerged changes).
 
 color.ui::
diff --git a/builtin/commit.c b/builtin/commit.c
index 4e288bc..43846d5 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1263,6 +1263,10 @@ static int parse_status_slot(const char *slot)
 		return WT_STATUS_NOBRANCH;
 	if (!strcasecmp(slot, "unmerged"))
 		return WT_STATUS_UNMERGED;
+	if (!strcasecmp(slot, "localBranch"))
+		return WT_STATUS_LOCAL_BRANCH;
+	if (!strcasecmp(slot, "remoteBranch"))
+		return WT_STATUS_REMOTE_BRANCH;
 	return -1;
 }
 
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 1150164..f0542b6 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2377,7 +2377,9 @@ _git_config ()
 		color.status.added
 		color.status.changed
 		color.status.header
+		color.status.localBranch
 		color.status.nobranch
+		color.status.remoteBranch
 		color.status.unmerged
 		color.status.untracked
 		color.status.updated
-- 
1.9.1


^ permalink raw reply	[flat|threaded] 4+ messages in thread

* Re: [PATCH] Add color slots for branch names in "git status --short --branch"
  2017-04-20  5:57 [PATCH] Add color slots for branch names in "git status --short --branch" Stephen Kent
@ 2017-04-20  6:16 ` Jeff King
  2017-04-20  6:30 ` Junio C Hamano
  1 sibling, 0 replies; 4+ messages in thread
From: Jeff King @ 2017-04-20  6:16 UTC (permalink / raw)
  To: Stephen Kent; +Cc: git

Overall this looks good. A few minor nits:

On Wed, Apr 19, 2017 at 10:57:08PM -0700, Stephen Kent wrote:

> Subject: Re: [PATCH] Add color slots for branch names in "git status --short

We usually try to use "subsystem: blah" for our subjects, which makes
them easy to parse when you're looking through a oneline. So probably:

  status: add color config slots for branch names

or something.

> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 475e874..96e9cf8 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1137,7 +1137,10 @@ color.status.<slot>::
>  	`untracked` (files which are not tracked by Git),
>  	`branch` (the current branch),
>  	`nobranch` (the color the 'no branch' warning is shown in, defaulting
> -	to red), or
> +	to red),
> +	`localBranch` or `remoteBranch` (the local and remote branch names,
> +	respectively, when branch and tracking information is displayed in the
> +	status short-format), or

I wondered if this "short-format" was accurate. But indeed, we do not
seem to color the local/remote branch specially in long-format mode, so
it really is only the short format that is affected.

> diff --git a/builtin/commit.c b/builtin/commit.c
> index 4e288bc..43846d5 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1263,6 +1263,10 @@ static int parse_status_slot(const char *slot)
>  		return WT_STATUS_NOBRANCH;
>  	if (!strcasecmp(slot, "unmerged"))
>  		return WT_STATUS_UNMERGED;
> +	if (!strcasecmp(slot, "localBranch"))
> +		return WT_STATUS_LOCAL_BRANCH;
> +	if (!strcasecmp(slot, "remoteBranch"))
> +		return WT_STATUS_REMOTE_BRANCH;

Normally we match config names in the code as all lowercase, since the
key names we get from the config parser will be normalized. Here it
works with your mixed-case because you're using strcasecmp(). Obviously
that was picked up from the surrounding code, but I think those existing
strcasecmp() calls could (and perhaps should) just be strcmp().

I don't know if it's worth converting them or not. If we leave them all
as strcasecmp(), I don't mind your camelCase names, for readability.

-Peff

^ permalink raw reply	[flat|threaded] 4+ messages in thread

* Re: [PATCH] Add color slots for branch names in "git status --short --branch"
  2017-04-20  5:57 [PATCH] Add color slots for branch names in "git status --short --branch" Stephen Kent
  2017-04-20  6:16 ` Jeff King
@ 2017-04-20  6:30 ` Junio C Hamano
  2017-04-22  5:40   ` Stephen Kent
  1 sibling, 1 reply; 4+ messages in thread
From: Junio C Hamano @ 2017-04-20  6:30 UTC (permalink / raw)
  To: Stephen Kent; +Cc: git, Jeff King

Stephen Kent <smkent@smkent.net> writes:

> Subject: Re: [PATCH] Add color slots for branch names in "git status --short --branch"

We spell one-liner title of our commits as "<area>: <summary>"
typically.  In this case, this is about the output from the status
command, so

	status: make the color used "--shrot --branch" output configurable

or something, perhaps?

> Signed-off-by: Stephen Kent <smkent@smkent.net>
> ---
>  Documentation/config.txt               | 5 ++++-
>  builtin/commit.c                       | 4 ++++
>  contrib/completion/git-completion.bash | 2 ++
>  3 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 475e874..96e9cf8 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1137,7 +1137,10 @@ color.status.<slot>::
>  	`untracked` (files which are not tracked by Git),
>  	`branch` (the current branch),
>  	`nobranch` (the color the 'no branch' warning is shown in, defaulting
> -	to red), or
> +	to red),
> +	`localBranch` or `remoteBranch` (the local and remote branch names,
> +	respectively, when branch and tracking information is displayed in the
> +	status short-format), or
>  	`unmerged` (files which have unmerged changes).

OK.

>  color.ui::
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 4e288bc..43846d5 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1263,6 +1263,10 @@ static int parse_status_slot(const char *slot)
>  		return WT_STATUS_NOBRANCH;
>  	if (!strcasecmp(slot, "unmerged"))
>  		return WT_STATUS_UNMERGED;
> +	if (!strcasecmp(slot, "localBranch"))
> +		return WT_STATUS_LOCAL_BRANCH;
> +	if (!strcasecmp(slot, "remoteBranch"))
> +		return WT_STATUS_REMOTE_BRANCH;
>  	return -1;
>  }

OK.

I know we do not test color.status.<slot> at all (other than t4026
that makes sure a configuration from future version of Git that
specifies a slot that is not yet known to our version of Git is
safely ignored without triggering an error), but perhaps we would
want a new test or two at the end of t7508?

Thanks.

^ permalink raw reply	[flat|threaded] 4+ messages in thread

* Re: [PATCH] Add color slots for branch names in "git status --short --branch"
  2017-04-20  6:30 ` Junio C Hamano
@ 2017-04-22  5:40   ` Stephen Kent
  0 siblings, 0 replies; 4+ messages in thread
From: Stephen Kent @ 2017-04-22  5:40 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Jeff King

Hi Peff and Junio,

I've updated the commit message and updated one of the existing unit 
tests for this feature. Patch version 2 will follow shortly after this 
email.

Responses to both of your comments:

> I wondered if this "short-format" was accurate. But indeed, we do not 
> seem to color the local/remote branch specially in long-format mode, 
> so it really is only the short format that is affected.

Right, the hardcoded red and green colors only seem to be used for the 
branch and remote tracking branch names (and commit counts) in the 
status short-format.

There is an existing color slot "color.status.branch" for the branch 
name in the default (long) status format which is different than the new 
color config slots this patch adds.

I'm wondering if it makes sense to also use color.status.branch for the 
local branch color in short-format. On the other hand, I have configured 
different colors in the short and long status format for the local 
branch name and I find it useful for them to be separate color slots.

> Normally we match config names in the code as all lowercase, since the 
> key names we get from the config parser will be normalized. Here it 
> works with your mixed-case because you're using strcasecmp(). 
> Obviously that was picked up from the surrounding code, but I think 
> those existing strcasecmp() calls could (and perhaps should) just be 
> strcmp().

> I don't know if it's worth converting them or not. If we leave them 
> all as strcasecmp(), I don't mind your camelCase names, for 
> readability.

I chose the localBranch and remoteBranch camel case names for 
consistency with the existing "color.decorate.remoteBranch" color config 
slot in log-tree.c. The documentation for color.decorate.remoteBranch 
uses that camel case name, but the config option is case-insensitive. 

I'm happy to use whatever names are best for the short status branch 
name color slots. Let me know if I should change them and what I should 
replace them with.

> I know we do not test color.status.<slot> at all (other than t4026 
> that makes sure a configuration from future version of Git that 
> specifies a slot that is not yet known to our version of Git is safely 
> ignored without triggering an error), but perhaps we would want a new 
> test or two at the end of t7508?

I see the existing tests for git status in t7508. The unit tests set up 
some mock repository modifications to test git status output, so I've 
modified one of the tests to include a custom color for the local branch 
in git status -sb.

t7508 doesn't seem to contain any tests that include an ahead or behind 
commit count, so I didn't make any test changes for the remote tracking 
branch color. What's the best course of action here?

Thanks!

Stephen



On Wed, Apr 19, 2017 at 23:30, Junio C Hamano wrote:
> Stephen Kent <smkent@smkent.net> writes:
>
>> Subject: Re: [PATCH] Add color slots for branch names in "git status 
>> --short --branch"
>
> We spell one-liner title of our commits as "<area>: <summary>" 
> typically.  In this case, this is about the output from the status 
> command, so
>
> 	status: make the color used "--shrot --branch" output configurable
>
> or something, perhaps?
>
>> Signed-off-by: Stephen Kent <smkent@smkent.net>
>> ---
>>  Documentation/config.txt               | 5 ++++-
>>  builtin/commit.c                       | 4 ++++ 
>>  contrib/completion/git-completion.bash | 2 ++
>>  3 files changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/config.txt b/Documentation/config.txt 
>> index 475e874..96e9cf8 100644
>> --- a/Documentation/config.txt
>> +++ b/Documentation/config.txt
>> @@ -1137,7 +1137,10 @@ color.status.<slot>::
>>  	`untracked` (files which are not tracked by Git),
>>  	`branch` (the current branch),
>>  	`nobranch` (the color the 'no branch' warning is shown in, 
>>  	defaulting -	to red), or
>> +	to red),
>> +	`localBranch` or `remoteBranch` (the local and remote branch
>> names, +	respectively, when branch and tracking information is 
>> displayed in the +	status short-format), or
>>  	`unmerged` (files which have unmerged changes).
>
> OK.
>
>>  color.ui::
>> diff --git a/builtin/commit.c b/builtin/commit.c
>> index 4e288bc..43846d5 100644
>> --- a/builtin/commit.c
>> +++ b/builtin/commit.c
>> @@ -1263,6 +1263,10 @@ static int parse_status_slot(const char *slot) 
>> return WT_STATUS_NOBRANCH;
>>  	if (!strcasecmp(slot, "unmerged"))
>>  		return WT_STATUS_UNMERGED;
>> +	if (!strcasecmp(slot, "localBranch"))
>> +		return WT_STATUS_LOCAL_BRANCH;
>> +	if (!strcasecmp(slot, "remoteBranch"))
>> +		return WT_STATUS_REMOTE_BRANCH;
>>  	return -1;
>>  }
>
> OK.
>
> I know we do not test color.status.<slot> at all (other than t4026 
> that makes sure a configuration from future version of Git that 
> specifies a slot that is not yet known to our version of Git is
> safely ignored without triggering an error), but perhaps we would
> want a new test or two at the end of t7508?
>
> Thanks.

^ permalink raw reply	[flat|threaded] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-20  5:57 [PATCH] Add color slots for branch names in "git status --short --branch" Stephen Kent
2017-04-20  6:16 ` Jeff King
2017-04-20  6:30 ` Junio C Hamano
2017-04-22  5:40   ` Stephen Kent

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox