[PATCH v2 0/3] Towards a useable git-branch

[PATCH v2 0/3] Towards a useable git-branch

[Ramkumar Ramachandra]

So, while investigating alignment operators in pretty-formats, I found
out that it's way too much effort and totally not worth it (atleast
not immediately; we can add it later if we want).  What I want now is
a useable git-branch output.  And I think I can say that I've achieved
it.

I currently have hot aliased to

for-each-ref --format='%C(red)%(HEAD)%C(reset) %C(green)%(refname:short)%C(reset)%(upstream:trackshort)' --count 10 --sort='-committerdate' refs/heads

and it works beautifully for me.  Sample output:

% git hot
* hot-branch<>
  pickaxe-doc>
  publish-rev=
  publish-rev-test
  upstream-error=
  push-current-head=
  master=
  prompt=
  autostash-stash=
  rebase.autostash=

The asterisk is red, the branch names are in green, and the tracking
marker is white.

I'm very happy with the implementation too:

1. color only kicks in at the parsing layer.
2. HEAD is a new atom.
3. :track[short] is a formatp like :short.

There is no need to use a hammer and coerce everything into an atom,
or throw everything out the window and start from scratch to conform
to pretty-formats perfectly.  Let's extend the existing format to be
_useful_ sensibly.

Thanks.

Ramkumar Ramachandra (3):
  for-each-ref: introduce %C(...) for color
  for-each-ref: introduce %(HEAD) marker
  for-each-ref: introduce %(upstream:track[short])

 builtin/for-each-ref.c | 81 +++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 73 insertions(+), 8 deletions(-)

-- 
1.8.3.rc3.2.g99b8f3f.dirty

[PATCH 1/3] for-each-ref: introduce %C(...) for color

[Ramkumar Ramachandra]

Since 'git branch' misses important options like --sort, --count, and
--format that are present in 'git for-each-ref'.  Until we are in a
position to fix 'git branch', let us enhance the 'git for-each-ref'
format so it can atleast colorize output.

You can use the following format in for-each-ref:

  %C(green)%(refname:short)%C(reset)

To display refs in green.  Future patches will attempt to extend the
format even more to get useful output.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/for-each-ref.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 7f059c3..1563b25 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -9,6 +9,7 @@
 #include "quote.h"
 #include "parse-options.h"
 #include "remote.h"
+#include "color.h"
 
 /* Quoting styles */
 #define QUOTE_NONE 0
@@ -155,10 +156,13 @@ static const char *find_next(const char *cp)
 	while (*cp) {
 		if (*cp == '%') {
 			/*
+			 * %C( is the start of a color;
 			 * %( is the start of an atom;
 			 * %% is a quoted per-cent.
 			 */
-			if (cp[1] == '(')
+			if (cp[1] == 'C' && cp[2] == '(')
+				return cp;
+			else if (cp[1] == '(')
 				return cp;
 			else if (cp[1] == '%')
 				cp++; /* skip over two % */
@@ -180,8 +184,12 @@ static int verify_format(const char *format)
 		const char *ep = strchr(sp, ')');
 		if (!ep)
 			return error("malformed format string %s", sp);
-		/* sp points at "%(" and ep points at the closing ")" */
-		parse_atom(sp + 2, ep);
+		/*
+		 * Ignore color specifications: %C(
+		 * sp points at "%(" and ep points at the closing ")"
+		 */
+		if (prefixcmp(sp, "%C("))
+			parse_atom(sp + 2, ep);
 		cp = ep + 1;
 	}
 	return 0;
@@ -928,12 +936,22 @@ static void emit(const char *cp, const char *ep)
 static void show_ref(struct refinfo *info, const char *format, int quote_style)
 {
 	const char *cp, *sp, *ep;
+	char color[COLOR_MAXLEN];
 
 	for (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {
 		ep = strchr(sp, ')');
 		if (cp < sp)
 			emit(cp, sp);
-		print_value(info, parse_atom(sp + 2, ep), quote_style);
+
+		/* Do we have a color specification? */
+		if (!prefixcmp(sp, "%C("))
+			color_parse_mem(sp + 3,
+					ep - sp - 3,
+					"--format ", color);
+		else {
+			printf("%s", color);
+			print_value(info, parse_atom(sp + 2, ep), quote_style);
+		}
 	}
 	if (*cp) {
 		sp = cp + strlen(cp);
-- 
1.8.3.rc3.2.g99b8f3f.dirty

[PATCH 2/3] for-each-ref: introduce %(HEAD) marker

[Ramkumar Ramachandra]

'git branch' shows which branch you are currently on with an '*', but
'git for-each-ref' misses this feature.  So, extend the format with
%(HEAD) to do exactly the same thing.

Now you can use the following format in for-each-ref:

  %C(red)%(HEAD)%C(reset) %C(green)%(refname:short)%C(reset)

to display a red asterisk next to the current ref.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/for-each-ref.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 1563b25..63d3a85 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -76,6 +76,7 @@ static struct {
 	{ "upstream" },
 	{ "symref" },
 	{ "flag" },
+	{ "HEAD" },
 };
 
 /*
@@ -683,8 +684,16 @@ static void populate_value(struct refinfo *ref)
 				v->s = xstrdup(buf + 1);
 			}
 			continue;
-		}
-		else
+		} else if (!strcmp(name, "HEAD")) {
+			const char *head;
+			unsigned char sha1[20];
+			head = resolve_ref_unsafe("HEAD", sha1, 1, NULL);
+			if (!strcmp(ref->refname, head))
+				v->s = "*";
+			else
+				v->s = " ";
+			continue;
+		} else
 			continue;
 
 		formatp = strchr(name, ':');
-- 
1.8.3.rc3.2.g99b8f3f.dirty

[PATCH 3/3] for-each-ref: introduce %(upstream:track[short])

[Ramkumar Ramachandra]

Introduce %(upstream:track) to display "[ahead M, behind N]" and
%(upstream:trackshort) to display "=", ">", "<", or "<>"
appropriately (inspired by the contrib/completion/git-prompt.sh).

Now you can use the following format in for-each-ref:

  %C(red)%(HEAD)%C(reset) %C(green)%(refname:short)%C(reset)%(upstream:trackshort)

to display refs with terse tracking information.

Note that :track and :trackshort only works with upstream, and errors
out when used with anything else.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/for-each-ref.c | 42 ++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 40 insertions(+), 2 deletions(-)

diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 63d3a85..838b125 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -632,6 +632,7 @@ static void populate_value(struct refinfo *ref)
 	int eaten, i;
 	unsigned long size;
 	const unsigned char *tagged;
+	int upstream_present = 0;
 
 	ref->value = xcalloc(sizeof(struct atom_value), used_atom_cnt);
 
@@ -649,6 +650,7 @@ static void populate_value(struct refinfo *ref)
 		int deref = 0;
 		const char *refname;
 		const char *formatp;
+		struct branch *branch;
 
 		if (*name == '*') {
 			deref = 1;
@@ -660,7 +662,6 @@ static void populate_value(struct refinfo *ref)
 		else if (!prefixcmp(name, "symref"))
 			refname = ref->symref ? ref->symref : "";
 		else if (!prefixcmp(name, "upstream")) {
-			struct branch *branch;
 			/* only local branches may have an upstream */
 			if (prefixcmp(ref->refname, "refs/heads/"))
 				continue;
@@ -670,6 +671,7 @@ static void populate_value(struct refinfo *ref)
 			    !branch->merge[0]->dst)
 				continue;
 			refname = branch->merge[0]->dst;
+			upstream_present = 1;
 		}
 		else if (!strcmp(name, "flag")) {
 			char buf[256], *cp = buf;
@@ -687,6 +689,7 @@ static void populate_value(struct refinfo *ref)
 		} else if (!strcmp(name, "HEAD")) {
 			const char *head;
 			unsigned char sha1[20];
+
 			head = resolve_ref_unsafe("HEAD", sha1, 1, NULL);
 			if (!strcmp(ref->refname, head))
 				v->s = "*";
@@ -699,11 +702,46 @@ static void populate_value(struct refinfo *ref)
 		formatp = strchr(name, ':');
 		/* look for "short" refname format */
 		if (formatp) {
+			int num_ours, num_theirs;
+
 			formatp++;
 			if (!strcmp(formatp, "short"))
 				refname = shorten_unambiguous_ref(refname,
 						      warn_ambiguous_refs);
-			else
+			else if (!strcmp(formatp, "track") &&
+				!prefixcmp(name, "upstream")) {
+				char buf[40];
+
+				if (!upstream_present)
+					continue;
+				if (!stat_tracking_info(branch, &num_ours, &num_theirs))
+					v->s = "";
+				else if (!num_ours) {
+					sprintf(buf, "[behind %d]", num_theirs);
+					v->s = xstrdup(buf);
+				} else if (!num_theirs) {
+					sprintf(buf, "[ahead %d]", num_ours);
+					v->s = xstrdup(buf);
+				} else {
+					sprintf(buf, "[ahead %d, behind %d]",
+						num_ours, num_theirs);
+					v->s = xstrdup(buf);
+				}
+				continue;
+			} else if (!strcmp(formatp, "trackshort") &&
+				!prefixcmp(name, "upstream")) {
+				if (!upstream_present)
+					continue;
+				if (!stat_tracking_info(branch, &num_ours, &num_theirs))
+					v->s = "=";
+				else if (!num_ours)
+					v->s = "<";
+				else if (!num_theirs)
+					v->s = ">";
+				else
+					v->s = "<>";
+				continue;
+			} else
 				die("unknown %.*s format %s",
 				    (int)(formatp - name), name, formatp);
 		}
-- 
1.8.3.rc3.2.g99b8f3f.dirty

Re: [PATCH v2 0/3] Towards a useable git-branch

[Duy Nguyen]

On Fri, May 24, 2013 at 9:19 PM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> There is no need to use a hammer and coerce everything into an atom,
> or throw everything out the window and start from scratch to conform
> to pretty-formats perfectly.  Let's extend the existing format to be
> _useful_ sensibly.

Usefulness is one thing. Another is maintenance and in that regard I
still think we should be able to remove -v and -vv code (not the
functionality) with this topic.
--
Duy

Re: [PATCH v2 0/3] Towards a useable git-branch

[Ramkumar Ramachandra]

Duy Nguyen wrote:
> Usefulness is one thing. Another is maintenance and in that regard I
> still think we should be able to remove -v and -vv code (not the
> functionality) with this topic.

Why?  The topic adds good functionality, doesn't break anything,
doesn't paint us into any corner, and has users.  Replacing -v and -vv
can be done eventually, after we get alignment.

Re: [PATCH v2 0/3] Towards a useable git-branch

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Fri, May 24, 2013 at 9:19 PM, Ramkumar Ramachandra
> <artagnon@gmail.com> wrote:
>> There is no need to use a hammer and coerce everything into an atom,
>> or throw everything out the window and start from scratch to conform
>> to pretty-formats perfectly.  Let's extend the existing format to be
>> _useful_ sensibly.
>
> Usefulness is one thing. Another is maintenance and in that regard I
> still think we should be able to remove -v and -vv code (not the
> functionality) with this topic.

Yes, the aim of the topic should be to make the machinery flexible
enough so that we can lose -v/-vv implementation and replace them
with calls to the new machinery with canned set of output format
templates.

By the way, I do not think "useable git-branch" is a good title,
though.  The expression has unnecessary venom in it, as if what it
currently does is not usable.  More flexible would be fine.

I am having this feeling that we see more and more of this line of
bad behaviours from some on the list recently to call something that
is working in which they find itch using unnecessarily derogatory
terms, and it makes people simply avoid any discussion that starts
with such an attitude.

Unnecessary negativity is not productive and it has to stop.

Re: [PATCH v2 0/3] Towards a useable git-branch

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> I am having this feeling that we see more and more of this line of
> bad behaviours from some on the list recently to call something that
> is working in which they find itch using unnecessarily derogatory
> terms, and it makes people simply avoid any discussion that starts
> with such an attitude.
>
> Unnecessary negativity is not productive and it has to stop.

My apologies.  After all, we have all been using 'git branch' for so
many years without complaining.  I only noticed the itch recently:
it's a burning itch that I want it to be fixed very badly (hence the
series).  If anything I intended to convey the importance of the patch
to me personally, not about some "general truth" on the broken-ness of
git-branch.

Ofcourse I will take your suggestion and tone down, because I don't
want the git community to feel bad about the software they're
developing.

Re: [PATCH v2 0/3] Towards a useable git-branch

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> Yes, the aim of the topic should be to make the machinery flexible
> enough so that we can lose -v/-vv implementation and replace them
> with calls to the new machinery with canned set of output format
> templates.

Definitely.  I don't want to keep my ugly alias around forever, and I
certainly want more users to have easy access to this (configurable
git-branch output formats).  However, the series is not the
theoretical exercise of prettifying the code underlying -v and -vv.
Supporting -v and -vv is something we _have_ to do to preserve
backward compatibility, and I would consider it a side-effect of the
series rather than the "aim of the topic".  The aim of the topic is to
get more useful output from git-branch.

As long as the topic doesn't paint us into a corner after which it
will be impossible to implement -v and -vv on top of the format, I
think we're good.

Re: [PATCH 2/3] for-each-ref: introduce %(HEAD) marker

[Philip Oakley]

From: "Ramkumar Ramachandra" <artagnon@gmail.com>
Sent: Friday, May 24, 2013 3:19 PM
> 'git branch' shows which branch you are currently on with an '*', but
> 'git for-each-ref' misses this feature.  So, extend the format with
> %(HEAD) to do exactly the same thing.

Maybe 'isHEAD'  as a better name, or 'ifHEAD', or something to indicate 
its boolean nature.

>
> Now you can use the following format in for-each-ref:
>
>  %C(red)%(HEAD)%C(reset) %C(green)%(refname:short)%C(reset)
>
> to display a red asterisk next to the current ref.
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
> builtin/for-each-ref.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
> index 1563b25..63d3a85 100644
> --- a/builtin/for-each-ref.c
> +++ b/builtin/for-each-ref.c
> @@ -76,6 +76,7 @@ static struct {
>  { "upstream" },
>  { "symref" },
>  { "flag" },
> + { "HEAD" },
> };
>
> /*
> @@ -683,8 +684,16 @@ static void populate_value(struct refinfo *ref)
>  v->s = xstrdup(buf + 1);
>  }
>  continue;
> - }
> - else
> + } else if (!strcmp(name, "HEAD")) {
> + const char *head;
> + unsigned char sha1[20];
> + head = resolve_ref_unsafe("HEAD", sha1, 1, NULL);
> + if (!strcmp(ref->refname, head))
> + v->s = "*";
> + else
> + v->s = " ";
> + continue;
> + } else
>  continue;
>
>  formatp = strchr(name, ':');
> -- 
> 1.8.3.rc3.2.g99b8f3f.dirty

Re: [PATCH 1/3] for-each-ref: introduce %C(...) for color

[Antoine Pelisse]

On Fri, May 24, 2013 at 4:19 PM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> @@ -928,12 +936,22 @@ static void emit(const char *cp, const char *ep)
>  static void show_ref(struct refinfo *info, const char *format, int quote_style)
>  {
>         const char *cp, *sp, *ep;
> +       char color[COLOR_MAXLEN];
>
>         for (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {
>                 ep = strchr(sp, ')');
>                 if (cp < sp)
>                         emit(cp, sp);
> -               print_value(info, parse_atom(sp + 2, ep), quote_style);
> +
> +               /* Do we have a color specification? */
> +               if (!prefixcmp(sp, "%C("))
> +                       color_parse_mem(sp + 3,
> +                                       ep - sp - 3,
> +                                       "--format ", color);
> +               else {
> +                       printf("%s", color);

Is it not possible for "color" to be used uninitialized here ?

> +                       print_value(info, parse_atom(sp + 2, ep), quote_style);
> +               }
>         }
>         if (*cp) {
>                 sp = cp + strlen(cp);

Re: [PATCH v2 0/3] Towards a useable git-branch

[Duy Nguyen]

On Fri, May 24, 2013 at 9:19 PM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> So, while investigating alignment operators in pretty-formats, I found
> out that it's way too much effort and totally not worth it (atleast
> not immediately; we can add it later if we want).

I just had an idea that might bring pretty stuff to for-each-ref with
probably reasonable effort, making for-each-ref format a superset of
pretty. But I need to clean up my backlog first. Give me a few days, I
will show you something (or give up ;)
--
Duy

Re: [PATCH 1/3] for-each-ref: introduce %C(...) for color

[David Aguilar]

On Fri, May 24, 2013 at 7:19 AM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> Since 'git branch' misses important options like --sort, --count, and
> --format that are present in 'git for-each-ref'.  Until we are in a
> position to fix 'git branch', let us enhance the 'git for-each-ref'
> format so it can atleast colorize output.
>
> You can use the following format in for-each-ref:
>
>   %C(green)%(refname:short)%C(reset)
>
> To display refs in green.  Future patches will attempt to extend the
> format even more to get useful output.
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  builtin/for-each-ref.c | 26 ++++++++++++++++++++++----
>  1 file changed, 22 insertions(+), 4 deletions(-)

Can you please also update Documentation/?
--
David

Re: [PATCH v2 0/3] Towards a useable git-branch

[Duy Nguyen]

On Sat, May 25, 2013 at 5:51 AM, Duy Nguyen <pclouds@gmail.com> wrote:
> I just had an idea that might bring pretty stuff to for-each-ref with
> probably reasonable effort, making for-each-ref format a superset of
> pretty. But I need to clean up my backlog first. Give me a few days, I
> will show you something (or give up ;)

And I can't get it out of my head. Might as well write it down. Check out

https://github.com/pclouds/git.git for-each-ref-pretty

It opens a hook in format_commit_message() to plug f-e-r's atom syntax
in. I didn't do extensive test or anything, just t6300. The %xx syntax
in for-each-ref might override some placeholders in pretty, I did not
check. You can add extra %(xx) on top as you have done. I still need
one more hook to support %>(*) with automatic width detection. After
that I'm quite confident we can kill -v/-vv code.
--
Duy

Re: [PATCH 1/3] for-each-ref: introduce %C(...) for color

[Eric Sunshine]

On Fri, May 24, 2013 at 10:19 AM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> Since 'git branch' misses important options like --sort, --count, and
> --format that are present in 'git for-each-ref'.  Until we are in a
> position to fix 'git branch', let us enhance the 'git for-each-ref'
> format so it can atleast colorize output.

s/atleast/at least/

> You can use the following format in for-each-ref:
>
>   %C(green)%(refname:short)%C(reset)
>
> To display refs in green.  Future patches will attempt to extend the
> format even more to get useful output.
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>

Re: [PATCH v2 0/3] Towards a useable git-branch

[Duy Nguyen]

On Sat, May 25, 2013 at 1:26 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Sat, May 25, 2013 at 5:51 AM, Duy Nguyen <pclouds@gmail.com> wrote:
>> I just had an idea that might bring pretty stuff to for-each-ref with
>> probably reasonable effort, making for-each-ref format a superset of
>> pretty. But I need to clean up my backlog first. Give me a few days, I
>> will show you something (or give up ;)
>
> And I can't get it out of my head. Might as well write it down. Check out
>
> https://github.com/pclouds/git.git for-each-ref-pretty

Ram, fetch the url above again. Its tip now is 5b4aa27 (for-each-ref:
introduce format specifier %>(*) and %<(*) - 2013-05-25). Those
changes make for-each-ref --format a superset of pretty. You can add
new %(xxx) on top and resend the whole thing to the list for review.
You can remove "branch -v/-vv" code as well or I'll add some patches
on top to do that later. I have some compatibility concerns about the
"superset" thing. But let's wait until the series hits git@vger.
--
Duy

Re: [PATCH v2 0/3] Towards a useable git-branch

[Ramkumar Ramachandra]

Duy Nguyen wrote:
> Ram, fetch the url above again. Its tip now is 5b4aa27 (for-each-ref:
> introduce format specifier %>(*) and %<(*) - 2013-05-25). Those
> changes make for-each-ref --format a superset of pretty. You can add
> new %(xxx) on top and resend the whole thing to the list for review.
> You can remove "branch -v/-vv" code as well or I'll add some patches
> on top to do that later. I have some compatibility concerns about the
> "superset" thing. But let's wait until the series hits git@vger.

Great work Duy!  I see that you've used format_commit_message(), but
there are some concerns about pretty-formats conflicting with f-e-r's
format.  We'll iron it out slowly, but I like the overall approach.

Thanks.

(Very sleep deprived at the moment; will review and collaborate after I wake up)

Re: [PATCH 1/3] for-each-ref: introduce %C(...) for color

[Ramkumar Ramachandra]

Antoine Pelisse wrote:
> Is it not possible for "color" to be used uninitialized here ?

My compiler didn't complain; what am I missing?  Doesn't the
declaration char color[COLOR_MAXLEN]; initialize an empty string?
More importantly, aren't there numerous instances of this in the
codebase?

Re: [PATCH 1/3] for-each-ref: introduce %C(...) for color

[Ramkumar Ramachandra]

David Aguilar wrote:
> Can you please also update Documentation/?

Yeah, will do in the re-roll.  Duy is bringing in pretty-formats.
We'll probably need a separate document called pretty-ref-formats or
some such thing.

Re: [PATCH 1/3] for-each-ref: introduce %C(...) for color

[John Keeping]

On Sat, May 25, 2013 at 05:20:29PM +0530, Ramkumar Ramachandra wrote:
> Antoine Pelisse wrote:
> > Is it not possible for "color" to be used uninitialized here ?
> 
> My compiler didn't complain; what am I missing?  Doesn't the
> declaration char color[COLOR_MAXLEN]; initialize an empty string?

Why would it?  The variable's begin allocated on the stack and the C
standard only zero-initializes variables with static storage duration;
Section 6.7.9 of the C11 standard says:

    If an object that has automatic storage duration is not initialized
    explicitly, its value is indeterminate.


I suspect the compiler doesn't complain because there is a path through
the function that initializes color before reading it (if we hit the
"if" branch in the loop before the "else" branch) and the compile
assumes that there is something in the function's contract that
guarantees that we follow this path.  But I don't think that's correct
so you do need to initialize color to the empty string.

> More importantly, aren't there numerous instances of this in the
> codebase?

Care to point at one?  I had a quick look and all places I inspected are
either static or write to the array before reading it.

Re: [PATCH 1/3] for-each-ref: introduce %C(...) for color

[Antoine Pelisse]

On Sat, May 25, 2013 at 1:50 PM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> Antoine Pelisse wrote:
>> Is it not possible for "color" to be used uninitialized here ?
>
> My compiler didn't complain; what am I missing?  Doesn't the
> declaration char color[COLOR_MAXLEN]; initialize an empty string?

As John said, this is allocated on the stack.
What do you want it to be initialized to ?
Filled with zeros ? That's overkill because only the first char needs
to be zeroed.
The compiler can't know what to do and it lets you to take the best action.

> More importantly, aren't there numerous instances of this in the
> codebase?

I think Valgrind would be mad at us if there was many instances of
this. By the way Ramkumar, could you check if Valgrind complains with
your patch ?

Re: [PATCH 1/3] for-each-ref: introduce %C(...) for color

[Ramkumar Ramachandra]

John Keeping wrote:
> Section 6.7.9 of the C11 standard says:
>
>     If an object that has automatic storage duration is not initialized
>     explicitly, its value is indeterminate.

Ah, thanks.  I'll initialize it to an empty string.

>> More importantly, aren't there numerous instances of this in the
>> codebase?
>
> Care to point at one?  I had a quick look and all places I inspected are
> either static or write to the array before reading it.

I'll run some Valgrind tests to see what it yields.

Re: [PATCH v2 0/3] Towards a useable git-branch

[Ramkumar Ramachandra]

Hi Duy,

I just woke up and started looking at the series: it's rather well
done, and I'm confident that this is the way forward.  To reciprocate,
I've done some work at gh:artagnon/git for-each-ref-pretty.  See:

https://github.com/artagnon/git/commits/for-each-ref-pretty

There is one major problem though:

%>(N) doesn't work properly with f-e-r, and I'm not sure why.  I'm not
talking about your last patch where you compute * -- that works fine;
it's just that %>(N) doesn't when N is a concrete number.

Also, a couple of minor annoyances:

1. When f-e-r is invoked with refs/tags, we get stray output.  Atleast
it doesn't segfault, thanks to your ignore-commit patch.  Maybe
printing stray output is the right thing to do, as opposed to erroring
out.

2. %>(*) only works with f-e-r atoms, not with pretty-format atoms.
This is ofcourse obvious from the implementation, but isn't it a
little consistent?

Should we start off a new pretty-ref-formats document, where we
explicitly exclude things like %ae (because of the hex overriding
thing)?  I don't think it's a problem if documented properly.

Re: [PATCH v2 0/3] Towards a useable git-branch

[Ramkumar Ramachandra]

Oh, and by the way:

We're pretty close we are to replacing branch -v and branch -vv.

brv = for-each-ref --format='%(HEAD)
%C(green)%<(*)%(refname:short)%C(reset) %<(*)%(objectname:short)
%(subject)' refs/heads

brvv = for-each-ref --format='%(HEAD)
%C(green)%<(*)%(refname:short)%C(reset) %<(*)%(objectname:short)
%C(blue)%(upstream:short)%C(reset) %(subject)' refs/heads

There are small differences:

1. In branch -v, the green-color of the branch name is dependent on
%(HEAD).  Not worth ironing out, in my opinion.

2. In branch -vv, there are dependent square braces that come on when
%(refname:short) is set.  We might want to introduce an undocumented
%(refname:branchvv) for internal use by branch -vv, for backward
compatibility.

What do you think?

Re: [PATCH v2 0/3] Towards a useable git-branch

[Ramkumar Ramachandra]

Ramkumar Ramachandra wrote:
> %>(N) doesn't work properly with f-e-r, and I'm not sure why.  I'm not
> talking about your last patch where you compute * -- that works fine;
> it's just that %>(N) doesn't when N is a concrete number.

Try this:

%(refname:short)%>(30)%(upstream:short)

(assuming that you have lots of branches).  I'm noticing random
alignment problems.

However, %<(N) doesn't seem to have this problem:

%<(30)%(refname:short)%(upstream:short)

I'm not able to figure this out.

Re: [PATCH v2 0/3] Towards a useable git-branch

[Duy Nguyen]

On Tue, May 28, 2013 at 9:28 PM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> Ramkumar Ramachandra wrote:
>> %>(N) doesn't work properly with f-e-r, and I'm not sure why.  I'm not
>> talking about your last patch where you compute * -- that works fine;
>> it's just that %>(N) doesn't when N is a concrete number.
>
> Try this:
>
> %(refname:short)%>(30)%(upstream:short)
>
> (assuming that you have lots of branches).  I'm noticing random
> alignment problems.

It's because you don't pad enough spaces after %(refname:short) so the
starting point of %(upstream:short) on each line is already unaligned,
I think. Try this:

%<(*)%(refname:short)%>(30)%(upstream:short)

or if you prefer at specific column (e.g. align upstream close to the
60th column, regardless of refname's length):

%(refname:short)%>|(60)%(upstream:short)
--
Duy

Re: [PATCH v2 0/3] Towards a useable git-branch

[Duy Nguyen]

On Tue, May 28, 2013 at 9:01 PM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> Also, a couple of minor annoyances:
>
> 1. When f-e-r is invoked with refs/tags, we get stray output.  Atleast
> it doesn't segfault, thanks to your ignore-commit patch.  Maybe
> printing stray output is the right thing to do, as opposed to erroring
> out.

What format did you use?

> 2. %>(*) only works with f-e-r atoms, not with pretty-format atoms.
> This is ofcourse obvious from the implementation, but isn't it a
> little consistent?

It is not. I think it's documented as well as a known implementation
limitation. It's not hard to be fixed (we could call
format_commit_message again for all entries, this time with a single
placeholder, to collect the best width). If anybody does need %>(*)%H
to work, we could do it. BTW, the way I implement get_atom_width() is
not really optimal. For n lines, we call print_value() in
get_atom_width n^2 times. We could cache the calculated width instead
and reduce that to n times.

> Should we start off a new pretty-ref-formats document, where we
> explicitly exclude things like %ae (because of the hex overriding
> thing)?  I don't think it's a problem if documented properly.

And this one is doucmented as well, I think. I'd really like to
introduce a new --pretty option (or something) that does not accept
%xx as a hex notion, so %ae and friends won't be hidden. It's also a
good thing for compatibility because currently %H in --format produces
%H. After this, %H produces something else. It's unlikely that anybody
has done that. But it's even better if we avoid that possibility
entirely with a new option.
--
Duy

Re: [PATCH v2 0/3] Towards a useable git-branch

[Ramkumar Ramachandra]

Duy Nguyen wrote:
> It's because you don't pad enough spaces after %(refname:short) so the
> starting point of %(upstream:short) on each line is already unaligned,
> I think.

Yeah, my stupidity.  I really meant %>|(30), and that works fine.

Re: [PATCH v2 0/3] Towards a useable git-branch

[Duy Nguyen]

On Tue, May 28, 2013 at 9:24 PM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> Oh, and by the way:
>
> We're pretty close we are to replacing branch -v and branch -vv.
>
> brv = for-each-ref --format='%(HEAD)
> %C(green)%<(*)%(refname:short)%C(reset) %<(*)%(objectname:short)
> %(subject)' refs/heads
>
> brvv = for-each-ref --format='%(HEAD)
> %C(green)%<(*)%(refname:short)%C(reset) %<(*)%(objectname:short)
> %C(blue)%(upstream:short)%C(reset) %(subject)' refs/heads
>
> There are small differences:
>
> 1. In branch -v, the green-color of the branch name is dependent on
> %(HEAD).  Not worth ironing out, in my opinion.
>
> 2. In branch -vv, there are dependent square braces that come on when
> %(refname:short) is set.  We might want to introduce an undocumented
> %(refname:branchvv) for internal use by branch -vv, for backward
> compatibility.
>
> What do you think?

I think we can change -v and -vv format slightly as long as the
information remains the same. Nobody should ever parse these output
with scripts. The color can be generated from color.branch.*. A bigger
problem is show_detached(), --[no-]merged, --with and --contains. We
need to move some of those code over to for-each-ref. Another problem
is the new "branch -v" seems to less responsive than old "branch -v"
because (I think) of sorting, even if we don't need it. I checked out
your branch, made some more updates and pushed out to my
for-each-ref-pretty again. Changes are:

 - fix segfault with "for-each-ref --format=%something refs/tags/'
 - add --pretty for new format syntax and keep --format unchanged
 - add --[no-]column to for-each-ref (for "git branch" with no arguments)
 - remove branch listing code and use for-each-ref instead (41
inserts, 378 deletes, beautiful).
 - add --sort and --count to git-branch
--
Duy

Re: [PATCH v2 0/3] Towards a useable git-branch

[Ramkumar Ramachandra]

Duy Nguyen wrote:
> Nobody should ever parse these output
> with scripts. The color can be generated from color.branch.*.

How do we implement color.branch.(current|local|remote|plain)?  In the
current code, we take a crude approach by hand-constructing argc, argv
strings and passing it to cmd_for_each_ref().  There are no
conditionals in the format syntax (and introducing one is probably not
a good idea either): so, I'm guessing these configuration variables
have to be read by for-each-ref?

> A bigger
> problem is show_detached(), --[no-]merged, --with and --contains. We
> need to move some of those code over to for-each-ref.

I saw that you fixed this.

> Another problem
> is the new "branch -v" seems to less responsive than old "branch -v"
> because (I think) of sorting, even if we don't need it.

Does your track-responsiveness patch fix this?

> I checked out
> your branch, made some more updates and pushed out to my
> for-each-ref-pretty again. Changes are:

*pants* Sorry, I'm finding it hard to keep up.

Re: [PATCH v2 0/3] Towards a useable git-branch

[Duy Nguyen]

On Tue, Jun 4, 2013 at 7:52 PM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
> Duy Nguyen wrote:
>> Nobody should ever parse these output
>> with scripts. The color can be generated from color.branch.*.
>
> How do we implement color.branch.(current|local|remote|plain)?  In the
> current code, we take a crude approach by hand-constructing argc, argv
> strings and passing it to cmd_for_each_ref().  There are no
> conditionals in the format syntax (and introducing one is probably not
> a good idea either): so, I'm guessing these configuration variables
> have to be read by for-each-ref?

Maybe. I don't really like the idea that for-each-ref reads _branch_
config. We could introduce the same set of keys but in
color.for-each-ref namespace. %C(auto) will take care of the logic and
choose the correct color key. When we replace branch's listing code
with for-each-ref, I think we could somehow override for-each-ref keys
with branch ones in-core. Too hacky?

>> A bigger
>> problem is show_detached(), --[no-]merged, --with and --contains. We
>> need to move some of those code over to for-each-ref.
>
> I saw that you fixed this.

Nope. --[no-]merged and --contains seem easy. show_detached is still
WIP, mostly because detached HEAD may or may not show when you provide
a pattern to git-branch (e.g. git branch --list 'foo*') and because
HEAD is always the first item, regardless of sorting order.
get_head_description also seems more porcelain-ish than a plumbing
feature..

>> Another problem
>> is the new "branch -v" seems to less responsive than old "branch -v"
>> because (I think) of sorting, even if we don't need it.
>
> Does your track-responsiveness patch fix this?

Yes.

>> I checked out
>> your branch, made some more updates and pushed out to my
>> for-each-ref-pretty again. Changes are:
>
> *pants* Sorry, I'm finding it hard to keep up.

Sorry that branch was in a better shape the day I sent my previous
email. Then I kind of used it as a playground with --[no-]merged,
--contains and stuff :-P
--
Duy