Re: [RFC/PATCH 07/11] branch: move 'current' check down to the presentation layer
Karthik Nayak karthik@gmail.com writes:
Even more generic would be an %(ifeq:x:y) conditional and a
%(currentbranch) atom:
%(ifeq:refname:currentbranch)*%(endif)
Those are just a couple ideas. Other variations are possible and
likely preferable to the specialized %(starifcurrent).
This makes sense, thanks. But implementing something like
%(if:atom) seems to not be as easy as I thought it would be.
First we need to parse that inner atom, but the used_atom_cnt is based
on how many atoms there are initially, which doesn't count this inner atom.
Although we could have a way around that, we'd need to again call
populate_value
from itself to get that inner atom's value. This causes more problems.
Either ways
I'm looking at ways around this.
A simple solution would be to do :
%(if)%(atom)%(then).%(endif)
or just
%(if)%(atom).%(endif)
My knee-jerk reaction to the former was Eeww, the users is forced
to keep verbosely typing unnecessarily '%(', ')%(then)' forever,
only because the implementor was too lazy to do the job properly in
parse_atom(). I do not think the latter a good idea at all.
But I think the former is worth considering, as it will later allow
us to extend it to various forms, e.g.
%(if:notempty)%(atom)%(then)...%(end)
%(if)%(atom)%(then)...%(elif)%(atom1)%(atom2)%(then)...%(end)
Two points being
(1) the default string is not empty does not have to be the only
test criteria, by leaving the door to add %(if:condition)
later;
(2) what is tested does not have to be a single atom (that is why
I do not think the one without %(then) good at all) but can be
a string that can later be interpreted.
And syntactically, something like this even may want to be
introduced later:
%(if:expr)master == %(refname:short)%(then)...%(end)
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 07/11] branch: move 'current' check down to the presentation layer
On Sat, Aug 1, 2015 at 2:33 PM, Eric Sunshine sunsh...@sunshineco.com wrote: On Sat, Aug 1, 2015 at 2:48 AM, Karthik Nayak karthik@gmail.com wrote: On Thu, Jul 30, 2015 at 2:57 AM, Matthieu Moy matthieu@grenoble-inp.fr wrote: Good point! I just was wondering if we need another atom just to print a star. But your points certainly are valid. I'll implement this. Thanks :) This would remove the need of making the printing of the * to be needed by ref-filter. As this is only needed in branch.c If going on what you're saying we could have a %(starifcurrent) atom or something, but I don't see a general use for this. To have a really customizeable format, you would want to allow e.g. git branch --format '%(starifcurrent) %(objectname) %(refname)' if the user wants to get the sha1 before the refname, and still have the star in front. It's a bit frustrating to have a hardcoded format that the user can't reproduce with the --format option, since it means you can't easily make a small variation on it. Agreed. will have a starifcurrent atom :) Please don't. It's better to avoid these highly specialized solutions and instead seek general purpose ones. For instance, utilizing the %(if:) atom suggested elsewhere, you might add an %(iscurrentbranch) which would allow a format like this: %(if:iscurrentbranch)*%(endif) Even more generic would be an %(ifeq:x:y) conditional and a %(currentbranch) atom: %(ifeq:refname:currentbranch)*%(endif) Those are just a couple ideas. Other variations are possible and likely preferable to the specialized %(starifcurrent). This makes sense, thanks. But implementing something like %(if:atom) seems to not be as easy as I thought it would be. First we need to parse that inner atom, but the used_atom_cnt is based on how many atoms there are initially, which doesn't count this inner atom. Although we could have a way around that, we'd need to again call populate_value from itself to get that inner atom's value. This causes more problems. Either ways I'm looking at ways around this. A simple solution would be to do : %(if)%(atom)%(then).%(endif) or just %(if)%(atom).%(endif) -- Regards, Karthik Nayak -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 07/11] branch: move 'current' check down to the presentation layer
On Thu, Jul 30, 2015 at 2:57 AM, Matthieu Moy matthieu@grenoble-inp.fr wrote: Karthik Nayak karthik@gmail.com writes: What I was thinking of was something like this : struct strbuf format = STRBUF_INIT; char c = ' '; if (current) c = '*'; strbuf_addf(format, %c, c, other format options...); show_ref_array_item(item, format.buf, quote_style, 0); strbuf_release(format); I think that would interact badly with verify_ref_format(). Usually, you have just one format string and call verify_ref_format() on it, not a different format string for each ref_array_item. That would probably be solvable. Good point! I just was wondering if we need another atom just to print a star. But your points certainly are valid. I'll implement this. Thanks :) This would remove the need of making the printing of the * to be needed by ref-filter. As this is only needed in branch.c If going on what you're saying we could have a %(starifcurrent) atom or something, but I don't see a general use for this. To have a really customizeable format, you would want to allow e.g. git branch --format '%(starifcurrent) %(objectname) %(refname)' if the user wants to get the sha1 before the refname, and still have the star in front. It's a bit frustrating to have a hardcoded format that the user can't reproduce with the --format option, since it means you can't easily make a small variation on it. Agreed. will have a starifcurrent atom :) -- Regards, Karthik Nayak -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 07/11] branch: move 'current' check down to the presentation layer
On Fri, Jul 31, 2015 at 11:48 PM, Karthik Nayak karthik@gmail.com wrote: On Thu, Jul 30, 2015 at 2:57 AM, Matthieu Moy matthieu@grenoble-inp.fr wrote: Karthik Nayak karthik@gmail.com writes: What I was thinking of was something like this : struct strbuf format = STRBUF_INIT; char c = ' '; if (current) c = '*'; strbuf_addf(format, %c, c, other format options...); show_ref_array_item(item, format.buf, quote_style, 0); strbuf_release(format); I think that would interact badly with verify_ref_format(). Usually, you have just one format string and call verify_ref_format() on it, not a different format string for each ref_array_item. That would probably be solvable. Good point! I just was wondering if we need another atom just to print a star. But your points certainly are valid. I'll implement this. Thanks :) This would remove the need of making the printing of the * to be needed by ref-filter. As this is only needed in branch.c If going on what you're saying we could have a %(starifcurrent) atom or something, but I don't see a general use for this. To have a really customizeable format, you would want to allow e.g. git branch --format '%(starifcurrent) %(objectname) %(refname)' if the user wants to get the sha1 before the refname, and still have the star in front. It's a bit frustrating to have a hardcoded format that the user can't reproduce with the --format option, since it means you can't easily make a small variation on it. Agreed. will have a starifcurrent atom :) -- Wonder if there is some sort of more generic atom that would work? I can't think of anything obvious at all though... it may be worth having this even though it definitely seems less useful generically, as the reason above where --format should be at least as expressive as the builtin formatting. Regards, Jake -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 07/11] branch: move 'current' check down to the presentation layer
On Sat, Aug 1, 2015 at 2:48 AM, Karthik Nayak karthik@gmail.com wrote: On Thu, Jul 30, 2015 at 2:57 AM, Matthieu Moy matthieu@grenoble-inp.fr wrote: Good point! I just was wondering if we need another atom just to print a star. But your points certainly are valid. I'll implement this. Thanks :) This would remove the need of making the printing of the * to be needed by ref-filter. As this is only needed in branch.c If going on what you're saying we could have a %(starifcurrent) atom or something, but I don't see a general use for this. To have a really customizeable format, you would want to allow e.g. git branch --format '%(starifcurrent) %(objectname) %(refname)' if the user wants to get the sha1 before the refname, and still have the star in front. It's a bit frustrating to have a hardcoded format that the user can't reproduce with the --format option, since it means you can't easily make a small variation on it. Agreed. will have a starifcurrent atom :) Please don't. It's better to avoid these highly specialized solutions and instead seek general purpose ones. For instance, utilizing the %(if:) atom suggested elsewhere, you might add an %(iscurrentbranch) which would allow a format like this: %(if:iscurrentbranch)*%(endif) Even more generic would be an %(ifeq:x:y) conditional and a %(currentbranch) atom: %(ifeq:refname:currentbranch)*%(endif) Those are just a couple ideas. Other variations are possible and likely preferable to the specialized %(starifcurrent). -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 07/11] branch: move 'current' check down to the presentation layer
On Wed, Jul 29, 2015 at 3:31 PM, Matthieu Moy matthieu@grenoble-inp.fr wrote: Karthik Nayak karthik@gmail.com writes: On Tue, Jul 28, 2015 at 6:39 PM, Matthieu Moy matthieu@grenoble-inp.fr wrote: Karthik Nayak karthik@gmail.com writes: We check if given ref is the current branch in print_ref_list(). Move this check to print_ref_item() where it is checked right before printing. This means that the '*' and the different color are coded in C, hence it's not possible to mimick this using git for-each-ref --format I do not consider this as blocking, but I think the ultimate goal should be to allow this, so that all the goodies of git branch can be made available to other ref-listing commands. Not sure what you mean here. What you already know, but probably badly explained ;-). Eventually, the output of git branch should correspond to a format string (so git branch would be almost an alias for git for-each-ref refs/heads/ --format '...'). Internally, this would mean using show_ref_array_item instead of print_ref_item. This is what you managed to do for git tag. You already identified one difficulty with sha1 alignment in git branch -v. I'm pointing out another which is that displaying the * in front of the current branch is currently not possible with a format string. You would need an atom like %(displayAStarIfTheBranchIsTheCurrentOne) (for which you'd need to find a short-and-sweet name ;-) ). What I was thinking of was something like this : struct strbuf format = STRBUF_INIT; char c = ' '; if (current) c = '*'; strbuf_addf(format, %c, c, other format options...); show_ref_array_item(item, format.buf, quote_style, 0); strbuf_release(format); This would remove the need of making the printing of the * to be needed by ref-filter. As this is only needed in branch.c If going on what you're saying we could have a %(starifcurrent) atom or something, but I don't see a general use for this. -- Regards, Karthik Nayak -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 07/11] branch: move 'current' check down to the presentation layer
Karthik Nayak karthik@gmail.com writes: What I was thinking of was something like this : struct strbuf format = STRBUF_INIT; char c = ' '; if (current) c = '*'; strbuf_addf(format, %c, c, other format options...); show_ref_array_item(item, format.buf, quote_style, 0); strbuf_release(format); I think that would interact badly with verify_ref_format(). Usually, you have just one format string and call verify_ref_format() on it, not a different format string for each ref_array_item. That would probably be solvable. This would remove the need of making the printing of the * to be needed by ref-filter. As this is only needed in branch.c If going on what you're saying we could have a %(starifcurrent) atom or something, but I don't see a general use for this. To have a really customizeable format, you would want to allow e.g. git branch --format '%(starifcurrent) %(objectname) %(refname)' if the user wants to get the sha1 before the refname, and still have the star in front. It's a bit frustrating to have a hardcoded format that the user can't reproduce with the --format option, since it means you can't easily make a small variation on it. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 07/11] branch: move 'current' check down to the presentation layer
On Wed, Jul 29, 2015 at 6:16 AM, Jacob Keller jacob.kel...@gmail.com wrote: On Tue, Jul 28, 2015 at 1:12 PM, Karthik Nayak karthik@gmail.com wrote: On Tue, Jul 28, 2015 at 6:39 PM, Matthieu Moy matthieu@grenoble-inp.fr wrote: Karthik Nayak karthik@gmail.com writes: We check if given ref is the current branch in print_ref_list(). Move this check to print_ref_item() where it is checked right before printing. This means that the '*' and the different color are coded in C, hence it's not possible to mimick this using git for-each-ref --format I do not consider this as blocking, but I think the ultimate goal should be to allow this, so that all the goodies of git branch can be made available to other ref-listing commands. Not sure what you mean here. He means to make sure that any feature that was in tag, branch, for-each-ref, etc should be available as part of ref-filter and in all of them Maybe he misunderstood code or maybe you misunderstood the comment here? Regards, Jake Ah, I didn't quite understand the first time. Thanks :) -- Regards, Karthik Nayak -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 07/11] branch: move 'current' check down to the presentation layer
Karthik Nayak karthik@gmail.com writes: On Tue, Jul 28, 2015 at 6:39 PM, Matthieu Moy matthieu@grenoble-inp.fr wrote: Karthik Nayak karthik@gmail.com writes: We check if given ref is the current branch in print_ref_list(). Move this check to print_ref_item() where it is checked right before printing. This means that the '*' and the different color are coded in C, hence it's not possible to mimick this using git for-each-ref --format I do not consider this as blocking, but I think the ultimate goal should be to allow this, so that all the goodies of git branch can be made available to other ref-listing commands. Not sure what you mean here. What you already know, but probably badly explained ;-). Eventually, the output of git branch should correspond to a format string (so git branch would be almost an alias for git for-each-ref refs/heads/ --format '...'). Internally, this would mean using show_ref_array_item instead of print_ref_item. This is what you managed to do for git tag. You already identified one difficulty with sha1 alignment in git branch -v. I'm pointing out another which is that displaying the * in front of the current branch is currently not possible with a format string. You would need an atom like %(displayAStarIfTheBranchIsTheCurrentOne) (for which you'd need to find a short-and-sweet name ;-) ). -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC/PATCH 07/11] branch: move 'current' check down to the presentation layer
We check if given ref is the current branch in print_ref_list(). Move
this check to print_ref_item() where it is checked right before
printing.
Based-on-patch-by: Jeff King p...@peff.net
Mentored-by: Christian Couder christian.cou...@gmail.com
Mentored-by: Matthieu Moy matthieu@grenoble-inp.fr
Signed-off-by: Karthik Nayak karthik@gmail.com
---
builtin/branch.c | 29 -
1 file changed, 12 insertions(+), 17 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index f3d83d0..ba9cbad 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -534,9 +534,9 @@ static char *get_head_description(void)
}
static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
- int abbrev, int current, const char *remote_prefix)
+ int abbrev, int detached, const char *remote_prefix)
{
- char c;
+ char c = ' ';
int color;
struct strbuf out = STRBUF_INIT, name = STRBUF_INIT;
const char *prefix = ;
@@ -547,7 +547,11 @@ static void print_ref_item(struct ref_item *item, int
maxwidth, int verbose,
switch (item-kind) {
case REF_LOCAL_BRANCH:
- color = BRANCH_COLOR_LOCAL;
+ if (!detached !strcmp(item-name, head)) {
+ color = BRANCH_COLOR_CURRENT;
+ c = '*';
+ } else
+ color = BRANCH_COLOR_LOCAL;
break;
case REF_REMOTE_BRANCH:
color = BRANCH_COLOR_REMOTE;
@@ -556,18 +560,13 @@ static void print_ref_item(struct ref_item *item, int
maxwidth, int verbose,
case REF_DETACHED_HEAD:
color = BRANCH_COLOR_CURRENT;
desc = get_head_description();
+ c = '*';
break;
default:
color = BRANCH_COLOR_PLAIN;
break;
}
- c = ' ';
- if (current) {
- c = '*';
- color = BRANCH_COLOR_CURRENT;
- }
-
strbuf_addf(name, %s%s, prefix, desc);
if (verbose) {
int utf8_compensation = strlen(name.buf) -
utf8_strwidth(name.buf);
@@ -677,21 +676,17 @@ static int print_ref_list(int kinds, int detached, int
verbose, int abbrev, stru
index = ref_list.index;
/* Print detached heads before sorting and printing the rest */
- if (detached (ref_list.list[index - 1].kind == REF_DETACHED_HEAD)
- !strcmp(ref_list.list[index - 1].name, head)) {
+ if (detached) {
print_ref_item(ref_list.list[index - 1], maxwidth, verbose,
abbrev,
- 1, remote_prefix);
+ detached, remote_prefix);
index -= 1;
}
qsort(ref_list.list, index, sizeof(struct ref_item), ref_cmp);
- for (i = 0; i index; i++) {
- int current = !detached (ref_list.list[i].kind ==
REF_LOCAL_BRANCH)
- !strcmp(ref_list.list[i].name, head);
+ for (i = 0; i index; i++)
print_ref_item(ref_list.list[i], maxwidth, verbose,
- abbrev, current, remote_prefix);
- }
+ abbrev, detached, remote_prefix);
free_ref_list(ref_list);
--
2.4.6
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 07/11] branch: move 'current' check down to the presentation layer
On Tue, Jul 28, 2015 at 1:12 PM, Karthik Nayak karthik@gmail.com wrote: On Tue, Jul 28, 2015 at 6:39 PM, Matthieu Moy matthieu@grenoble-inp.fr wrote: Karthik Nayak karthik@gmail.com writes: We check if given ref is the current branch in print_ref_list(). Move this check to print_ref_item() where it is checked right before printing. This means that the '*' and the different color are coded in C, hence it's not possible to mimick this using git for-each-ref --format I do not consider this as blocking, but I think the ultimate goal should be to allow this, so that all the goodies of git branch can be made available to other ref-listing commands. Not sure what you mean here. He means to make sure that any feature that was in tag, branch, for-each-ref, etc should be available as part of ref-filter and in all of them Maybe he misunderstood code or maybe you misunderstood the comment here? Regards, Jake -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 07/11] branch: move 'current' check down to the presentation layer
Karthik Nayak karthik@gmail.com writes:
We check if given ref is the current branch in print_ref_list(). Move
this check to print_ref_item() where it is checked right before
printing.
This means that the '*' and the different color are coded in C, hence
it's not possible to mimick this using git for-each-ref --format
I do not consider this as blocking, but I think the ultimate goal should
be to allow this, so that all the goodies of git branch can be made
available to other ref-listing commands.
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -534,9 +534,9 @@ static char *get_head_description(void)
}
static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
-int abbrev, int current, const char *remote_prefix)
+int abbrev, int detached, const char *remote_prefix)
{
- char c;
+ char c = ' ';
int color;
struct strbuf out = STRBUF_INIT, name = STRBUF_INIT;
const char *prefix = ;
@@ -547,7 +547,11 @@ static void print_ref_item(struct ref_item *item, int
maxwidth, int verbose,
switch (item-kind) {
case REF_LOCAL_BRANCH:
- color = BRANCH_COLOR_LOCAL;
+ if (!detached !strcmp(item-name, head)) {
+ color = BRANCH_COLOR_CURRENT;
+ c = '*';
+ } else
+ color = BRANCH_COLOR_LOCAL;
break;
case REF_REMOTE_BRANCH:
color = BRANCH_COLOR_REMOTE;
@@ -556,18 +560,13 @@ static void print_ref_item(struct ref_item *item, int
maxwidth, int verbose,
case REF_DETACHED_HEAD:
color = BRANCH_COLOR_CURRENT;
desc = get_head_description();
+ c = '*';
break;
default:
color = BRANCH_COLOR_PLAIN;
break;
}
- c = ' ';
- if (current) {
- c = '*';
- color = BRANCH_COLOR_CURRENT;
- }
I actually prefered the old way: current is a Boolean that says
semantically this is the current branch, and the Boolean is turned
into presentation directives in a separate piece of code.
I'd write
char c;
int current = 0;
...
if (...)
current = 1;
...
case REF_DETACHED_HEAD:
current = 1;
...
and keep the last hunk.
(IOW, turn current from a parameter into a local variable)
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 07/11] branch: move 'current' check down to the presentation layer
On Tue, Jul 28, 2015 at 6:39 PM, Matthieu Moy
matthieu@grenoble-inp.fr wrote:
Karthik Nayak karthik@gmail.com writes:
We check if given ref is the current branch in print_ref_list(). Move
this check to print_ref_item() where it is checked right before
printing.
This means that the '*' and the different color are coded in C, hence
it's not possible to mimick this using git for-each-ref --format
I do not consider this as blocking, but I think the ultimate goal should
be to allow this, so that all the goodies of git branch can be made
available to other ref-listing commands.
Not sure what you mean here.
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -534,9 +534,9 @@ static char *get_head_description(void)
}
static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
-int abbrev, int current, const char *remote_prefix)
+int abbrev, int detached, const char *remote_prefix)
{
- char c;
+ char c = ' ';
int color;
struct strbuf out = STRBUF_INIT, name = STRBUF_INIT;
const char *prefix = ;
@@ -547,7 +547,11 @@ static void print_ref_item(struct ref_item *item, int
maxwidth, int verbose,
switch (item-kind) {
case REF_LOCAL_BRANCH:
- color = BRANCH_COLOR_LOCAL;
+ if (!detached !strcmp(item-name, head)) {
+ color = BRANCH_COLOR_CURRENT;
+ c = '*';
+ } else
+ color = BRANCH_COLOR_LOCAL;
break;
case REF_REMOTE_BRANCH:
color = BRANCH_COLOR_REMOTE;
@@ -556,18 +560,13 @@ static void print_ref_item(struct ref_item *item, int
maxwidth, int verbose,
case REF_DETACHED_HEAD:
color = BRANCH_COLOR_CURRENT;
desc = get_head_description();
+ c = '*';
break;
default:
color = BRANCH_COLOR_PLAIN;
break;
}
- c = ' ';
- if (current) {
- c = '*';
- color = BRANCH_COLOR_CURRENT;
- }
I actually prefered the old way: current is a Boolean that says
semantically this is the current branch, and the Boolean is turned
into presentation directives in a separate piece of code.
I'd write
char c;
int current = 0;
...
if (...)
current = 1;
...
case REF_DETACHED_HEAD:
current = 1;
...
and keep the last hunk.
(IOW, turn current from a parameter into a local variable)
Thanks will do this.
--
Regards,
Karthik Nayak
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
