threads / patch / 33919

v2, 3 partsTowards a useable git-branch

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

## tl;dr

31 messages between May 24, 2013 and Jun 4, 2013. Diffs are folded; open one to read it.

replies: 30people: 8as markdown or json

Ramkumar Ramachandra· May 24, 2013, 14:19 UTC · lore

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
Ramkumar Ramachandra· May 24, 2013, 14:19 UTC · re: Ramkumar Ramachandra · lore

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

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(-)
Show changes to builtin/for-each-ref.c +22 −4
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
Antoine Pelisse· May 24, 2013, 20:56 UTC · re: Ramkumar Ramachandra · lore

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

On Fri, May 24, 2013 at 4:19 PM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:

Show 19 quoted lines
> @@ -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 ?
Show 5 quoted lines
> +                       print_value(info, parse_atom(sp + 2, ep), quote_style);
> +               }
>         }
>         if (*cp) {
>                 sp = cp + strlen(cp);
Ramkumar Ramachandra· May 25, 2013, 11:50 UTC · re: Antoine Pelisse · lore

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

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?

John Keeping· May 25, 2013, 12:20 UTC · re: Ramkumar Ramachandra · lore

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

On Sat, May 25, 2013 at 05:20:29PM +0530, Ramkumar Ramachandra wrote:
Show 5 quoted lines
> 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.

Ramkumar Ramachandra· May 25, 2013, 12:54 UTC · re: John Keeping · lore

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

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.
Show 5 quoted lines
>> 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.
Antoine Pelisse· May 25, 2013, 12:35 UTC · re: Ramkumar Ramachandra · lore

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

On Sat, May 25, 2013 at 1:50 PM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:

Show 5 quoted lines
> 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 ?

David Aguilar· May 24, 2013, 23:41 UTC · re: Ramkumar Ramachandra · lore

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

On Fri, May 24, 2013 at 7:19 AM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:

Show 16 quoted lines
> 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

Ramkumar Ramachandra· May 25, 2013, 11:51 UTC · re: David Aguilar · lore

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

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.

Eric Sunshine· May 25, 2013, 06:29 UTC · re: Ramkumar Ramachandra · lore

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

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/
Show 8 quoted lines
> 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>
Ramkumar Ramachandra· May 24, 2013, 14:19 UTC · re: Ramkumar Ramachandra · lore

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

'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(-)
Show changes to builtin/for-each-ref.c +11 −2
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
Philip Oakley· May 24, 2013, 20:28 UTC · re: Ramkumar Ramachandra · lore

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

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.

Show 45 quoted lines
>
> 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
Ramkumar Ramachandra· May 24, 2013, 14:19 UTC · re: Ramkumar Ramachandra · lore

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

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(-)
Show changes to builtin/for-each-ref.c +40 −2
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
Duy Nguyen· May 24, 2013, 15:27 UTC · re: Ramkumar Ramachandra · lore

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

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

Ramkumar Ramachandra· May 24, 2013, 15:58 UTC · re: Duy Nguyen · lore

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

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.

Junio C Hamano· May 24, 2013, 17:52 UTC · re: Duy Nguyen · lore

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

Duy Nguyen <pclouds@gmail.com> writes:
Show 10 quoted lines
> 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.
Ramkumar Ramachandra· May 24, 2013, 18:01 UTC · re: Junio C Hamano · lore

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

Junio C Hamano wrote:
Show 7 quoted lines
> 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.

Ramkumar Ramachandra· May 24, 2013, 18:08 UTC · re: Junio C Hamano · lore

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

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.

Duy Nguyen· May 24, 2013, 22:51 UTC · re: Ramkumar Ramachandra · lore

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

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

Duy Nguyen· May 25, 2013, 06:26 UTC · re: Duy Nguyen · lore

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

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

Duy Nguyen· May 25, 2013, 11:35 UTC · re: Duy Nguyen · lore

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

On Sat, May 25, 2013 at 1:26 PM, Duy Nguyen <pclouds@gmail.com> wrote:
Show 9 quoted lines
> 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

Ramkumar Ramachandra· May 25, 2013, 11:48 UTC · re: Duy Nguyen · lore

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

Duy Nguyen wrote:
Show 7 quoted lines
> 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)
Ramkumar Ramachandra· May 28, 2013, 14:01 UTC · re: Ramkumar Ramachandra · lore

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

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.

Ramkumar Ramachandra· May 28, 2013, 14:24 UTC · re: Ramkumar Ramachandra · lore

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

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?
Duy Nguyen· May 30, 2013, 04:19 UTC · re: Ramkumar Ramachandra · lore

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

On Tue, May 28, 2013 at 9:24 PM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:

Show 23 quoted lines
> 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
Ramkumar Ramachandra· Jun 4, 2013, 12:52 UTC · re: Duy Nguyen · lore

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

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.
Duy Nguyen· Jun 4, 2013, 13:11 UTC · re: Ramkumar Ramachandra · lore

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

On Tue, Jun 4, 2013 at 7:52 PM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
Show 10 quoted lines
> 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?

Show 5 quoted lines
>> 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..

Show 5 quoted lines
>> 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.
Show 5 quoted lines
>> 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

Ramkumar Ramachandra· May 28, 2013, 14:28 UTC · re: Ramkumar Ramachandra · lore

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

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.
Duy Nguyen· May 29, 2013, 03:04 UTC · re: Ramkumar Ramachandra · lore

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

On Tue, May 28, 2013 at 9:28 PM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:

Show 11 quoted lines
> 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

Ramkumar Ramachandra· May 29, 2013, 21:12 UTC · re: Duy Nguyen · lore

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

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.
Duy Nguyen· May 29, 2013, 03:22 UTC · re: Ramkumar Ramachandra · lore

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

On Tue, May 28, 2013 at 9:01 PM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:

Show 6 quoted lines
> 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

← back to recent threads