This series cleans up populate_value() in ref-filter, by moving out
the parsing part of atoms to separate parsing functions. This ensures
that parsing is only done once and also improves the modularity of the
code.
v1: http://thread.gmane.org/gmane.comp.version-control.git/281180
v2: http://thread.gmane.org/gmane.comp.version-control.git/282563
v3: http://thread.gmane.org/gmane.comp.version-control.git/283350
v4: http://thread.gmane.org/gmane.comp.version-control.git/285158
Changes:
* Fixed small errors in multiple 'die(..)' messages.
* Removed unecessary braces.
* In parse_align_position() use 'v', 's' to denote the vector of arguments
and individual argument respectively rather than re-using 'arg'.
* Fix error in parse_ref_filter_atom() where length of current atom wasn't
accurately calculated.
* Small code and indentation fixes
Thanks to Eric, Junio, Ramsay and Andreas for their comments on the previous
version. And everyone else who helped review the previous patch series.
Karthik Nayak (12):
strbuf: introduce strbuf_split_str_omit_term()
ref-filter: use strbuf_split_str_omit_term()
ref-filter: bump 'used_atom' and related code to the top
ref-filter: introduce struct used_atom
ref-filter: introduce parsing functions for each valid atom
ref-filter: introduce color_atom_parser()
ref-filter: introduce parse_align_position()
ref-filter: introduce align_atom_parser()
ref-filter: align: introduce long-form syntax
ref-filter: introduce remote_ref_atom_parser()
ref-filter: introduce contents_atom_parser()
ref-filter: introduce objectname_atom_parser()
Documentation/git-for-each-ref.txt | 20 +-
ref-filter.c | 434 +++++++++++++++++++++----------------
strbuf.c | 14 +-
strbuf.h | 25 ++-
t/t6302-for-each-ref-filter.sh | 42 ++++
5 files changed, 329 insertions(+), 206 deletions(-)
Interdiff:
@@ -74,14 +74,14 @@ static void remote_ref_atom_parser(struct used_atom *atom, const char *arg)staticvoidbody_atom_parser(structused_atom*atom,constchar*arg){if(arg)-die("%%(body) atom does not take arguments");+die("%%(body) does not take arguments");atom->u.contents.option=C_BODY_DEP;}staticvoidsubject_atom_parser(structused_atom*atom,constchar*arg){if(arg)-die("%%(subject) atom does not take arguments");+die("%%(subject) does not take arguments");atom->u.contents.option=C_SUB;}
@@ -253,6 +253,7 @@ int parse_ref_filter_atom(const char *atom, const char *ep)/* Is the atom a valid one? */for(i=0;i<ARRAY_SIZE(valid_atom);i++){intlen=strlen(valid_atom[i].name);+/**Iftheatomnamehasacolon,stripitandeverythingafter*itoff-itspecifiestheformatforthisentry,and
@@ -149,8 +149,8 @@ test_align_permutations() {whileread-roptiondotest_expect_success"align:$option"'-gitfor-each-ref--format="|%(align:$option)refname is %(refname)%(end)|%(refname)">actual&&-test_cmpexpectactual+gitfor-each-ref--format="|%(align:$option)refname is %(refname)%(end)|%(refname)">actual&&+test_cmpexpectactual'done}
The current implementation of 'strbuf_split_buf()' includes the
terminator at the end of each strbuf post splitting. Add an option
wherein we can drop the terminator if desired. In this context
introduce a wrapper function 'strbuf_split_str_omit_term()' which
splits a given string into strbufs without including the terminator.
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Karthik Nayak <redacted>
---
strbuf.c | 14 +++++++++-----
strbuf.h | 25 ++++++++++++++++---------
2 files changed, 25 insertions(+), 14 deletions(-)
Parsing atoms is done in populate_value(), this is repetitive and
hence expensive. Introduce a parsing function which would let us parse
atoms beforehand and store the required details into the 'used_atom'
structure for further usage.
Helped-by: Eric Sunshine [off-list ref]
Helped-by: Ramsay Jones [off-list ref]
Helped-by: Andreas Schwab [off-list ref]
Signed-off-by: Karthik Nayak <redacted>
---
ref-filter.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
@@ -132,16 +134,16 @@ int parse_ref_filter_atom(const char *atom, const char *ep)/* Is the atom a valid one? */for(i=0;i<ARRAY_SIZE(valid_atom);i++){intlen=strlen(valid_atom[i].name);+/**Iftheatomnamehasacolon,stripitandeverythingafter*itoff-itspecifiestheformatforthisentry,and*shouldn'tbeusedforcheckingagainstthevalid_atom*table.*/-constchar*formatp=strchr(sp,':');-if(!formatp||ep<formatp)-formatp=ep;-if(len==formatp-sp&&!memcmp(valid_atom[i].name,sp,len))+arg=memchr(sp,':',ep-sp);+if(len==(arg?arg:ep)-sp&&+!memcmp(valid_atom[i].name,sp,len))break;}
Use the newly introduced strbuf_split_str_omit_term() rather than
using strbuf_split_str() and manually removing the ',' terminator.
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Karthik Nayak <redacted>
---
ref-filter.c | 9 +--------
1 file changed, 1 insertion(+), 8 deletions(-)
Introduce the 'used_atom' structure to replace the existing
implementation of 'used_atom' (which is a list of atoms). This helps
us parse atoms beforehand and store required details into the
'used_atom' for future usage.
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Karthik Nayak <redacted>
---
ref-filter.c | 35 ++++++++++++++++++-----------------
1 file changed, 18 insertions(+), 17 deletions(-)
@@ -122,8 +124,8 @@ int parse_ref_filter_atom(const char *atom, const char *ep)/* Do we have the atom already used elsewhere? */for(i=0;i<used_atom_cnt;i++){-intlen=strlen(used_atom[i]);-if(len==ep-atom&&!memcmp(used_atom[i],atom,len))+intlen=strlen(used_atom[i].name);+if(len==ep-atom&&!memcmp(used_atom[i].name,atom,len))returni;}
@@ -809,7 +810,7 @@ static void populate_value(struct ref_array_item *ref)/* Fill in specials first */for(i=0;i<used_atom_cnt;i++){-constchar*name=used_atom[i];+constchar*name=used_atom[i].name;structatom_value*v=&ref->value[i];intderef=0;constchar*refname;
Bump code to the top for usage in further patches.
Signed-off-by: Karthik Nayak <redacted>
---
ref-filter.c | 30 +++++++++++++++---------------
1 file changed, 15 insertions(+), 15 deletions(-)
From populate_value() extract parse_align_position() which given a
string would give us the alignment position. This is a preparatory
patch as to introduce prefixes for the %(align) atom and avoid
redundancy in the code.
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Karthik Nayak <redacted>
---
ref-filter.c | 21 +++++++++++++++------
1 file changed, 15 insertions(+), 6 deletions(-)
Introduce align_atom_parser() which will parse an 'align' atom and
store the required alignment position and width in the 'used_atom'
structure for further usage in populate_value().
Since this patch removes the last usage of match_atom_name(), remove
the function from ref-filter.c.
Helped-by: Eric Sunshine [off-list ref]
Helped-by: Ramsay Jones [off-list ref]
Signed-off-by: Karthik Nayak <redacted>
---
ref-filter.c | 91 ++++++++++++++++++++++++++----------------------------------
1 file changed, 40 insertions(+), 51 deletions(-)
@@ -288,22 +320,6 @@ static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_stapop_stack_element(&state->stack);}-staticintmatch_atom_name(constchar*name,constchar*atom_name,constchar**val)-{-constchar*body;--if(!skip_prefix(name,atom_name,&body))-return0;/* doesn't even begin with "atom_name" */-if(!body[0]){-*val=NULL;/* %(atom_name) and no customization */-return1;-}-if(body[0]!=':')-return0;/* "atom_namefoo" is not "atom_name" or "atom_name:..." */-*val=body+1;/* "atom_name:val" */-return1;-}-/**Inaformatstring,findthenextoccurrenceof%(atom).*/
Introduce color_atom_parser() which will parse a "color" atom and
store its color in the "used_atom" structure for further usage in
populate_value().
Helped-by: Ramsay Jones [off-list ref]
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Karthik Nayak <redacted>
---
ref-filter.c | 25 ++++++++++++++++---------
1 file changed, 16 insertions(+), 9 deletions(-)
@@ -816,6 +828,7 @@ static void populate_value(struct ref_array_item *ref)/* Fill in specials first */for(i=0;i<used_atom_cnt;i++){+structused_atom*atom=&used_atom[i];constchar*name=used_atom[i].name;structatom_value*v=&ref->value[i];intderef=0;
Introduce remote_ref_atom_parser() which will parse the '%(upstream)'
and '%(push)' atoms and store information into the 'used_atom'
structure based on the modifiers used along with the corresponding
atom.
Helped-by: Ramsay Jones [off-list ref]
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Karthik Nayak <redacted>
---
ref-filter.c | 103 +++++++++++++++++++++++++++++++++++------------------------
1 file changed, 61 insertions(+), 42 deletions(-)
Introduce contents_atom_parser() which will parse the '%(contents)'
atom and store information into the 'used_atom' structure based on the
modifiers used along with the atom. Also introduce body_atom_parser()
and subject_atom_parser() for parsing atoms '%(body)' and '%(subject)'
respectively.
Helped-by: Ramsay Jones [off-list ref]
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Karthik Nayak <redacted>
---
ref-filter.c | 79 ++++++++++++++++++++++++++++++++++++++----------------------
1 file changed, 50 insertions(+), 29 deletions(-)
@@ -66,6 +70,38 @@ static void remote_ref_atom_parser(struct used_atom *atom, const char *arg)die(_("unrecognized format: %%(%s)"),atom->name);}+staticvoidbody_atom_parser(structused_atom*atom,constchar*arg)+{+if(arg)+die("%%(body) does not take arguments");+atom->u.contents.option=C_BODY_DEP;+}++staticvoidsubject_atom_parser(structused_atom*atom,constchar*arg)+{+if(arg)+die("%%(subject) does not take arguments");+atom->u.contents.option=C_SUB;+}++staticvoidcontents_atom_parser(structused_atom*atom,constchar*arg)+{+if(!arg)+atom->u.contents.option=C_BARE;+elseif(!strcmp(arg,"body"))+atom->u.contents.option=C_BODY;+elseif(!strcmp(arg,"signature"))+atom->u.contents.option=C_SIG;+elseif(!strcmp(arg,"subject"))+atom->u.contents.option=C_SUB;+elseif(skip_prefix(arg,"lines=",&arg)){+atom->u.contents.option=C_LINES;+if(strtoul_ui(arg,10,&atom->u.contents.nlines))+die(_("positive value expected contents:lines=%s"),arg);+}else+die(_("unrecognized %%(contents) argument: %s"),arg);+}+staticalign_typeparse_align_position(constchar*s){if(!strcmp(s,"right"))
@@ -181,7 +212,6 @@ struct atom_value {constchar*s;union{structalignalign;-structcontentscontents;}u;void(*handler)(structatom_value*atomv,structref_formatting_state*state);unsignedlongul;/* used for sorting when not FIELD_STR */
@@ -754,28 +780,23 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj&bodypos,&bodylen,&nonsiglen,&sigpos,&siglen);-if(!strcmp(name,"subject"))-v->s=copy_subject(subpos,sublen);-elseif(!strcmp(name,"contents:subject"))+if(atom->u.contents.option==C_SUB)v->s=copy_subject(subpos,sublen);-elseif(!strcmp(name,"body"))+elseif(atom->u.contents.option==C_BODY_DEP)v->s=xmemdupz(bodypos,bodylen);-elseif(!strcmp(name,"contents:body"))+elseif(atom->u.contents.option==C_BODY)v->s=xmemdupz(bodypos,nonsiglen);-elseif(!strcmp(name,"contents:signature"))+elseif(atom->u.contents.option==C_SIG)v->s=xmemdupz(sigpos,siglen);-elseif(!strcmp(name,"contents"))-v->s=xstrdup(subpos);-elseif(skip_prefix(name,"contents:lines=",&valp)){+elseif(atom->u.contents.option==C_LINES){structstrbufs=STRBUF_INIT;constchar*contents_end=bodylen+bodypos-siglen;-if(strtoul_ui(valp,10,&v->u.contents.lines))-die(_("positive value expected contents:lines=%s"),valp);/* Size is the length of the message after removing the signature */-append_lines(&s,subpos,contents_end-subpos,v->u.contents.lines);+append_lines(&s,subpos,contents_end-subpos,atom->u.contents.nlines);v->s=strbuf_detach(&s,NULL);-}+}elseif(atom->u.contents.option==C_BARE)+v->s=xstrdup(subpos);}}
Introduce optional prefixes "width=" and "position=" for the align atom
so that the atom can be used as "%(align:width=<width>,position=<position>)".
Add Documentation and tests for the same.
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Karthik Nayak <redacted>
---
Documentation/git-for-each-ref.txt | 20 ++++++++++--------
ref-filter.c | 10 ++++++++-
t/t6302-for-each-ref-filter.sh | 42 ++++++++++++++++++++++++++++++++++++++
3 files changed, 63 insertions(+), 9 deletions(-)
@@ -133,14 +133,18 @@ color:: align:: Left-, middle-, or right-align the content between- %(align:...) and %(end). The "align:" is followed by `<width>`- and `<position>` in any order separated by a comma, where the- `<position>` is either left, right or middle, default being- left and `<width>` is the total length of the content with- alignment. If the contents length is more than the width then- no alignment is performed. If used with '--quote' everything- in between %(align:...) and %(end) is quoted, but if nested- then only the topmost level performs quoting.+ %(align:...) and %(end). The "align:" is followed by+ `width=<width>` and `position=<position>` in any order+ separated by a comma, where the `<position>` is either left,+ right or middle, default being left and `<width>` is the total+ length of the content with alignment. For brevity, the+ "width=" and/or "position=" prefixes may be omitted, and bare+ <width> and <position> used instead. For instance,+ `%(align:<width>,<position>)`. If the contents length is more+ than the width then no alignment is performed. If used with+ '--quote' everything in between %(align:...) and %(end) is+ quoted, but if nested then only the topmost level performs+ quoting. In addition to the above, for commit and tag objects, the header field names (`tree`, `parent`, `object`, `type`, and `tag`) can
@@ -133,6 +133,48 @@ test_expect_success 'right alignment' 'test_cmpexpectactual'+cat>expect<<-\EOF+|refnameisrefs/heads/master|refs/heads/master+|refnameisrefs/heads/side|refs/heads/side+|refnameisrefs/odd/spot|refs/odd/spot+|refnameisrefs/tags/double-tag|refs/tags/double-tag+|refnameisrefs/tags/four|refs/tags/four+|refnameisrefs/tags/one|refs/tags/one+|refnameisrefs/tags/signed-tag|refs/tags/signed-tag+|refnameisrefs/tags/three|refs/tags/three+|refnameisrefs/tags/two|refs/tags/two+EOF++test_align_permutations(){+whileread-roption+do+test_expect_success"align:$option"'+gitfor-each-ref--format="|%(align:$option)refname is %(refname)%(end)|%(refname)">actual&&+test_cmpexpectactual+'+done+}++test_align_permutations<<-\EOF+middle,42+42,middle+position=middle,42+42,position=middle+middle,width=42+width=42,middle+position=middle,width=42+width=42,position=middle+EOF++# Last one wins (silently) when multiple arguments of the same type are given++test_align_permutations<<-\EOF+32,width=42,middle+width=30,42,middle+width=42,position=right,middle+42,right,position=middle+EOF+# Individual atoms inside %(align:...) and %(end) must not be quoted. test_expect_success'alignment with format quote'"
Introduce objectname_atom_parser() which will parse the
'%(objectname)' atom and store information into the 'used_atom'
structure based on the modifiers used along with the atom.
Helped-by: Ramsay Jones [off-list ref]
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Karthik Nayak <redacted>
---
ref-filter.c | 35 ++++++++++++++++++++++++-----------
1 file changed, 24 insertions(+), 11 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 23:08:16
On Wed, Feb 17, 2016 at 12:30:05AM +0530, Karthik Nayak wrote:
Use the newly introduced strbuf_split_str_omit_term() rather than
using strbuf_split_str() and manually removing the ',' terminator.
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Karthik Nayak <redacted>
---
ref-filter.c | 9 +--------
1 file changed, 1 insertion(+), 8 deletions(-)
Did you consider just using string_list_split for this? AFAICT, you
don't care about the results being strbufs themselves, and it would do
what you want without having to bother with patch 1. The result would
look something like the patch below.
Sorry to waltz into a review of v5 with a suggestion to throw out all
the work done in previous iterations. :-/ I just think the strbuf_split
interface is kind of clunky and I'd be happy if we could slowly get rid
of it rather than growing it. Maybe that's not realistic, though (some
of the callsites _do_ want to do things like strbuf_trim() after
splitting).
-Peff
From: Jeff King <hidden> Date: 2016-06-15 23:08:16
On Tue, Feb 16, 2016 at 02:22:32PM -0500, Jeff King wrote:
On Wed, Feb 17, 2016 at 12:30:05AM +0530, Karthik Nayak wrote:
quoted
Use the newly introduced strbuf_split_str_omit_term() rather than
using strbuf_split_str() and manually removing the ',' terminator.
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Karthik Nayak <redacted>
---
ref-filter.c | 9 +--------
1 file changed, 1 insertion(+), 8 deletions(-)
Did you consider just using string_list_split for this? AFAICT, you
don't care about the results being strbufs themselves, and it would do
what you want without having to bother with patch 1. The result would
look something like the patch below.
Probably help if I actually included the patch.
---
From: Eric Sunshine <hidden> Date: 2016-06-15 23:08:16
On Tue, Feb 16, 2016 at 2:22 PM, Jeff King [off-list ref] wrote:
On Wed, Feb 17, 2016 at 12:30:05AM +0530, Karthik Nayak wrote:
quoted
Use the newly introduced strbuf_split_str_omit_term() rather than
using strbuf_split_str() and manually removing the ',' terminator.
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Karthik Nayak <redacted>
---
Did you consider just using string_list_split for this? AFAICT, you
don't care about the results being strbufs themselves, and it would do
what you want without having to bother with patch 1. [...]
Sorry to waltz into a review of v5 with a suggestion to throw out all
the work done in previous iterations. :-/ I just think the strbuf_split
interface is kind of clunky and I'd be happy if we could slowly get rid
of it rather than growing it. [...]
That's a nice idea, however, I'm not sure if making it part of this
series this late in the game is a good idea. The series has gone
through major changes and heavy review in each of the preceding
versions, and turnaround time has been consequently quite slow (due
both to the amount of work required by Karthik for each version, and
to the amount of time needed by reviewers to digest all the new
changes). v4 was the first one which had settled to the point where
only minor changes were needed, and we were hoping to land the series
with v5. (A few larger changes were also discussed in v4 reviews, but
we concluded that they could be done as follow-up patches.)
With that in mind, it might be better to make this change as a
followup to this series. On the other hand, as you say, waiting would
expand the strbuf_split interface undesirably, so the alternative
would be for Karthik to submit v6 with this change only (to wit: drop
patch 1 and rewrite patch 2 as you've shown). While such a change will
again require careful review, at least it is well localized, and
Karthik's turnaround time shouldn't be too bad. So...
From: Jeff King <hidden> Date: 2016-06-15 23:08:16
On Tue, Feb 16, 2016 at 03:12:29PM -0500, Eric Sunshine wrote:
quoted
Did you consider just using string_list_split for this? AFAICT, you
don't care about the results being strbufs themselves, and it would do
what you want without having to bother with patch 1. [...]
Sorry to waltz into a review of v5 with a suggestion to throw out all
the work done in previous iterations. :-/ I just think the strbuf_split
interface is kind of clunky and I'd be happy if we could slowly get rid
of it rather than growing it. [...]
That's a nice idea, however, I'm not sure if making it part of this
series this late in the game is a good idea. The series has gone
through major changes and heavy review in each of the preceding
versions, and turnaround time has been consequently quite slow (due
both to the amount of work required by Karthik for each version, and
to the amount of time needed by reviewers to digest all the new
changes). v4 was the first one which had settled to the point where
only minor changes were needed, and we were hoping to land the series
with v5. (A few larger changes were also discussed in v4 reviews, but
we concluded that they could be done as follow-up patches.)
With that in mind, it might be better to make this change as a
followup to this series. On the other hand, as you say, waiting would
expand the strbuf_split interface undesirably, so the alternative
would be for Karthik to submit v6 with this change only (to wit: drop
patch 1 and rewrite patch 2 as you've shown). While such a change will
again require careful review, at least it is well localized, and
Karthik's turnaround time shouldn't be too bad. So...
Yeah, I don't insist, and like I said, I'm not 100% sure we can get rid
of the strbuf_split interface anyway. I thought it might actually make
things easier by making the series _shorter_ (so my regret was that
mentioning earlier could have saved reviewing effort on patch 1).
It does mean extra review of the patch I posted, but my hope was that
it's small and localized, and wouldn't impact the later stuff seriously
(there are some textual tweaks to carry it forward, though).
Anyway, I've said my piece, and you guys can do what you will with it.
-Peff
From: Eric Sunshine <hidden> Date: 2016-06-15 23:08:16
On Tue, Feb 16, 2016 at 3:49 PM, Jeff King [off-list ref] wrote:
On Tue, Feb 16, 2016 at 03:12:29PM -0500, Eric Sunshine wrote:
quoted
quoted
Did you consider just using string_list_split for this? AFAICT, you
don't care about the results being strbufs themselves, and it would do
what you want without having to bother with patch 1. [...]
That's a nice idea, however, I'm not sure if making it part of this
series this late in the game is a good idea. The series has gone
through major changes and heavy review in each of the preceding
versions, and turnaround time has been consequently quite slow (due
both to the amount of work required by Karthik for each version, and
to the amount of time needed by reviewers to digest all the new
changes). v4 was the first one which had settled to the point where
only minor changes were needed, and we were hoping to land the series
with v5. [...]
With that in mind, it might be better to make this change as a
followup to this series. On the other hand, as you say, waiting would
expand the strbuf_split interface undesirably, so the alternative
would be for Karthik to submit v6 with this change only (to wit: drop
patch 1 and rewrite patch 2 as you've shown). While such a change will
again require careful review, at least it is well localized, and
Karthik's turnaround time shouldn't be too bad. So...
Yeah, I don't insist, and like I said, I'm not 100% sure we can get rid
of the strbuf_split interface anyway. I thought it might actually make
things easier by making the series _shorter_ (so my regret was that
mentioning earlier could have saved reviewing effort on patch 1).
It does mean extra review of the patch I posted, but my hope was that
it's small and localized, and wouldn't impact the later stuff seriously
(there are some textual tweaks to carry it forward, though).
My initial reaction was negative due to the heavy review burden this
series has demanded thus far, however, my mind was changing even as I
composed the above response. In retrospect, I think I'd be okay seeing
a v6, for the following reasons:
- I already ended up reviewing the the suggested new changes pretty
closely as a side-effect of reading your proposal.
- It would indeed be nice to avoid introducing
strbuf_split_str_omit_term() in the first place; thus one less thing
to worry about if someone ever takes on the task of retiring the
strbuf_split interface.
- It should be only a minimal amount of work for Karthik, thus
turnaround time should be short.
So, I think I'm fine with it, if Karthik is game.
From: Jeff King <hidden> Date: 2016-06-15 23:08:16
On Tue, Feb 16, 2016 at 04:09:53PM -0500, Eric Sunshine wrote:
My initial reaction was negative due to the heavy review burden this
series has demanded thus far, however, my mind was changing even as I
composed the above response. In retrospect, I think I'd be okay seeing
a v6, for the following reasons:
- I already ended up reviewing the the suggested new changes pretty
closely as a side-effect of reading your proposal.
- It would indeed be nice to avoid introducing
strbuf_split_str_omit_term() in the first place; thus one less thing
to worry about if someone ever takes on the task of retiring the
strbuf_split interface.
- It should be only a minimal amount of work for Karthik, thus
turnaround time should be short.
So, I think I'm fine with it, if Karthik is game.
I started to write up a commit message for my proposed change. But it
did make me think of a counter-argument. Right now we parse
"%(align:10,middle)" but do not allow "%(align: 10, middle)".
Should we? Or perhaps: might we? If the answer is yes, we are likely
better off with strbuf_split, because then we are only a strbuf_trim()
away from making that work.
-Peff
From: Eric Sunshine <hidden> Date: 2016-06-15 23:08:16
On Tue, Feb 16, 2016 at 5:34 PM, Jeff King [off-list ref] wrote:
On Tue, Feb 16, 2016 at 04:09:53PM -0500, Eric Sunshine wrote:
quoted
My initial reaction was negative due to the heavy review burden this
series has demanded thus far, however, my mind was changing even as I
composed the above response. In retrospect, I think I'd be okay seeing
a v6, for the following reasons:
- I already ended up reviewing the the suggested new changes pretty
closely as a side-effect of reading your proposal.
- It would indeed be nice to avoid introducing
strbuf_split_str_omit_term() in the first place; thus one less thing
to worry about if someone ever takes on the task of retiring the
strbuf_split interface.
- It should be only a minimal amount of work for Karthik, thus
turnaround time should be short.
So, I think I'm fine with it, if Karthik is game.
I started to write up a commit message for my proposed change. But it
did make me think of a counter-argument. Right now we parse
"%(align:10,middle)" but do not allow "%(align: 10, middle)".
Should we? Or perhaps: might we? If the answer is yes, we are likely
better off with strbuf_split, because then we are only a strbuf_trim()
away from making that work.
I also considered the issue of embedded whitespace very early on when
reading your initial proposal, but didn't mention anything about it
due to a vague recollection from one of the early reviews (or possibly
a review of one of Karthik's other patch series) of someone (possibly
Junio) saying or implying that embedded whitespace would not be
supported. Unfortunately, I can't locate that message (assuming it
even exists and wasn't a figment of my imagination).
From: Jeff King <hidden> Date: 2016-06-15 23:08:16
On Tue, Feb 16, 2016 at 05:49:19PM -0500, Eric Sunshine wrote:
On Tue, Feb 16, 2016 at 5:34 PM, Jeff King [off-list ref] wrote:
quoted
On Tue, Feb 16, 2016 at 04:09:53PM -0500, Eric Sunshine wrote:
quoted
My initial reaction was negative due to the heavy review burden this
series has demanded thus far, however, my mind was changing even as I
composed the above response. In retrospect, I think I'd be okay seeing
a v6, for the following reasons:
- I already ended up reviewing the the suggested new changes pretty
closely as a side-effect of reading your proposal.
- It would indeed be nice to avoid introducing
strbuf_split_str_omit_term() in the first place; thus one less thing
to worry about if someone ever takes on the task of retiring the
strbuf_split interface.
- It should be only a minimal amount of work for Karthik, thus
turnaround time should be short.
So, I think I'm fine with it, if Karthik is game.
I started to write up a commit message for my proposed change. But it
did make me think of a counter-argument. Right now we parse
"%(align:10,middle)" but do not allow "%(align: 10, middle)".
Should we? Or perhaps: might we? If the answer is yes, we are likely
better off with strbuf_split, because then we are only a strbuf_trim()
away from making that work.
I also considered the issue of embedded whitespace very early on when
reading your initial proposal, but didn't mention anything about it
due to a vague recollection from one of the early reviews (or possibly
a review of one of Karthik's other patch series) of someone (possibly
Junio) saying or implying that embedded whitespace would not be
supported. Unfortunately, I can't locate that message (assuming it
even exists and wasn't a figment of my imagination).
Yeah, I could not find any relevant reference (though I didn't spend all
that long digging).
For reference, I rebuilt Karthik's series on top of my proposal, and the
changes are fairly minor. I pushed it to:
git://github.com/peff/git.git jk/tweaked-ref-filter
The tbdiff is below. Hopefully having that done makes it easier to
decide based on the outcome, rather than the pain of rebasing. :)
To be honest, though, I am now on the fence, considering the possible
whitespace issue.
1: 92de9c7 < --: ------- strbuf: introduce strbuf_split_str_omit_term()
2: 4845dc5 < --: ------- ref-filter: use strbuf_split_str_omit_term()
--: ------- > 1: 29177cc ref-filter: use string_list_split over strbuf_split
3: 040e9ce = 2: ed284bc ref-filter: bump 'used_atom' and related code to the top
4: c7eb061 = 3: 2a99777 ref-filter: introduce struct used_atom
5: c3e24cf = 4: b18f23b ref-filter: introduce parsing functions for each valid atom
6: 0b7fe83 = 5: e5221cc ref-filter: introduce color_atom_parser()
7: ffb3afe ! 6: 454af9c ref-filter: introduce parse_align_position()
@@ -32,21 +32,21 @@
const char *name;
cmp_type cmp_type;
@@
- align->position = ALIGN_LEFT;
-
- while (*s) {
+ string_list_split(¶ms, valp, ',', -1);
+ for (i = 0; i < params.nr; i++) {
+ const char *s = params.items[i].string;
+ int position;
+
- if (!strtoul_ui(s[0]->buf, 10, (unsigned int *)&width))
+ if (!strtoul_ui(s, 10, (unsigned int *)&width))
;
-- else if (!strcmp(s[0]->buf, "left"))
+- else if (!strcmp(s, "left"))
- align->position = ALIGN_LEFT;
-- else if (!strcmp(s[0]->buf, "right"))
+- else if (!strcmp(s, "right"))
- align->position = ALIGN_RIGHT;
-- else if (!strcmp(s[0]->buf, "middle"))
+- else if (!strcmp(s, "middle"))
- align->position = ALIGN_MIDDLE;
-+ else if ((position = parse_align_position(s[0]->buf)) >= 0)
++ else if ((position = parse_align_position(s)) >= 0)
+ align->position = position;
else
- die(_("improper format entered align:%s"), s[0]->buf);
- s++;
+ die(_("improper format entered align:%s"), s);
+ }
8: 0f0e596 ! 7: 0779954 ref-filter: introduce align_atom_parser()
@@ -43,18 +43,19 @@
+static void align_atom_parser(struct used_atom *atom, const char *arg)
+{
+ struct align *align = &atom->u.align;
-+ struct strbuf **v, **to_free;
++ struct string_list params = STRING_LIST_INIT_DUP;
++ int i;
+ unsigned int width = ~0U;
+
+ if (!arg)
+ die(_("expected format: %%(align:<width>,<position>)"));
-+ v = to_free = strbuf_split_str_omit_term(arg, ',', 0);
+
+ align->position = ALIGN_LEFT;
+
-+ while (*v) {
++ string_list_split(¶ms, arg, ',', -1);
++ for (i = 0; i < params.nr; i++) {
++ const char *s = params.items[i].string;
+ int position;
-+ const char *s = v[0]->buf;
+
+ if (!strtoul_ui(s, 10, &width))
+ ;
@@ -62,13 +63,12 @@
+ align->position = position;
+ else
+ die(_("unrecognized %%(align) argument: %s"), s);
-+ v++;
+ }
+
+ if (width == ~0U)
+ die(_("positive width expected with the %%(align) atom"));
+ align->width = width;
-+ strbuf_list_free(to_free);
++ string_list_clear(¶ms, 0);
+}
+
static struct {
@@ -130,32 +130,32 @@
continue;
- } else if (match_atom_name(name, "align", &valp)) {
- struct align *align = &v->u.align;
-- struct strbuf **s, **to_free;
+- struct string_list params = STRING_LIST_INIT_DUP;
+- int i;
- int width = -1;
-
- if (!valp)
- die(_("expected format: %%(align:<width>,<position>)"));
-
-- s = to_free = strbuf_split_str_omit_term(valp, ',', 0);
--
- align->position = ALIGN_LEFT;
-
-- while (*s) {
+- string_list_split(¶ms, valp, ',', -1);
+- for (i = 0; i < params.nr; i++) {
+- const char *s = params.items[i].string;
- int position;
-
-- if (!strtoul_ui(s[0]->buf, 10, (unsigned int *)&width))
+- if (!strtoul_ui(s, 10, (unsigned int *)&width))
- ;
-- else if ((position = parse_align_position(s[0]->buf)) >= 0)
+- else if ((position = parse_align_position(s)) >= 0)
- align->position = position;
- else
-- die(_("improper format entered align:%s"), s[0]->buf);
-- s++;
+- die(_("improper format entered align:%s"), s);
- }
-
- if (width < 0)
- die(_("positive width expected with the %%(align) atom"));
- align->width = width;
-- strbuf_list_free(to_free);
+- string_list_clear(¶ms, 0);
+ } else if (starts_with(name, "align")) {
+ v->u.align = atom->u.align;
v->handler = align_atom_handler;
9: d3dc384 ! 8: 792c89a ref-filter: align: introduce long-form syntax
@@ -45,8 +45,8 @@
--- a/ref-filter.c
+++ b/ref-filter.c
@@
+ const char *s = params.items[i].string;
int position;
- const char *s = v[0]->buf;
- if (!strtoul_ui(s, 10, &width))
+ if (skip_prefix(s, "position=", &s)) {
10: 3ae28b5 = 9: 019fee7 ref-filter: introduce remote_ref_atom_parser()
11: 06c70af = 10: f6e4f5a ref-filter: introduce contents_atom_parser()
12: c9db181 = 11: 0a84b70 ref-filter: introduce objectname_atom_parser()
On Wed, Feb 17, 2016 at 12:52 AM, Jeff King [off-list ref] wrote:
On Wed, Feb 17, 2016 at 12:30:05AM +0530, Karthik Nayak wrote:
quoted
Use the newly introduced strbuf_split_str_omit_term() rather than
using strbuf_split_str() and manually removing the ',' terminator.
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Karthik Nayak <redacted>
---
ref-filter.c | 9 +--------
1 file changed, 1 insertion(+), 8 deletions(-)
Did you consider just using string_list_split for this? AFAICT, you
don't care about the results being strbufs themselves, and it would do
what you want without having to bother with patch 1. The result would
look something like the patch below.
I haven't, thanks for bringing it up :)
Sorry to waltz into a review of v5 with a suggestion to throw out all
the work done in previous iterations. :-/ I just think the strbuf_split
interface is kind of clunky and I'd be happy if we could slowly get rid
of it rather than growing it. Maybe that's not realistic, though (some
of the callsites _do_ want to do things like strbuf_trim() after
splitting).
-Peff
That's fine, as I see it, it's better to wait a while and get a better version
of something.
--
Regards,
Karthik Nayak
On Wed, Feb 17, 2016 at 2:39 AM, Eric Sunshine [off-list ref] wrote:
On Tue, Feb 16, 2016 at 3:49 PM, Jeff King [off-list ref] wrote:
quoted
On Tue, Feb 16, 2016 at 03:12:29PM -0500, Eric Sunshine wrote:
quoted
quoted
Did you consider just using string_list_split for this? AFAICT, you
don't care about the results being strbufs themselves, and it would do
what you want without having to bother with patch 1. [...]
That's a nice idea, however, I'm not sure if making it part of this
series this late in the game is a good idea. The series has gone
through major changes and heavy review in each of the preceding
versions, and turnaround time has been consequently quite slow (due
both to the amount of work required by Karthik for each version, and
to the amount of time needed by reviewers to digest all the new
changes). v4 was the first one which had settled to the point where
only minor changes were needed, and we were hoping to land the series
with v5. [...]
With that in mind, it might be better to make this change as a
followup to this series. On the other hand, as you say, waiting would
expand the strbuf_split interface undesirably, so the alternative
would be for Karthik to submit v6 with this change only (to wit: drop
patch 1 and rewrite patch 2 as you've shown). While such a change will
again require careful review, at least it is well localized, and
Karthik's turnaround time shouldn't be too bad. So...
Yeah, I don't insist, and like I said, I'm not 100% sure we can get rid
of the strbuf_split interface anyway. I thought it might actually make
things easier by making the series _shorter_ (so my regret was that
mentioning earlier could have saved reviewing effort on patch 1).
It does mean extra review of the patch I posted, but my hope was that
it's small and localized, and wouldn't impact the later stuff seriously
(there are some textual tweaks to carry it forward, though).
My initial reaction was negative due to the heavy review burden this
series has demanded thus far, however, my mind was changing even as I
composed the above response. In retrospect, I think I'd be okay seeing
a v6, for the following reasons:
- I already ended up reviewing the the suggested new changes pretty
closely as a side-effect of reading your proposal.
- It would indeed be nice to avoid introducing
strbuf_split_str_omit_term() in the first place; thus one less thing
to worry about if someone ever takes on the task of retiring the
strbuf_split interface.
- It should be only a minimal amount of work for Karthik, thus
turnaround time should be short.
So, I think I'm fine with it, if Karthik is game.
Sounds good to me.
I just read the conversation between Jeff, Junio and You about the whitespace
counter-argument and I think its good to go ahead with v6 with Jeff's suggested
change.
Since he's already pushed the changes on top of my changes to:
git://github.com/peff/git.git jk/tweaked-ref-filter
I'll just have a look and push that to the list as v6.
--
Regards,
Karthik Nayak
From: Eric Sunshine <hidden> Date: 2016-06-15 23:08:17
On Wed, Feb 17, 2016 at 12:04 PM, Karthik Nayak [off-list ref] wrote:
On Wed, Feb 17, 2016 at 2:39 AM, Eric Sunshine [off-list ref] wrote:
quoted
My initial reaction was negative due to the heavy review burden this
series has demanded thus far, however, my mind was changing even as I
composed the above response. [...]
So, I think I'm fine with it, if Karthik is game.
Sounds good to me.
I just read the conversation between Jeff, Junio and You about the whitespace
counter-argument and I think its good to go ahead with v6 with Jeff's suggested
change.
Since he's already pushed the changes on top of my changes to:
git://github.com/peff/git.git jk/tweaked-ref-filter
I'll just have a look and push that to the list as v6.
I reviewed the entire series again, including Peff's changes, so this
entire series is:
Reviewed-by: Eric Sunshine [off-list ref]
Karthik, feel free to include my Reviewed-by: in all the patches
(including Peff's) when you post v6.
Thanks.
On Wed, Feb 17, 2016 at 11:09 PM, Eric Sunshine [off-list ref] wrote:
On Wed, Feb 17, 2016 at 12:04 PM, Karthik Nayak [off-list ref] wrote:
quoted
On Wed, Feb 17, 2016 at 2:39 AM, Eric Sunshine [off-list ref] wrote:
quoted
My initial reaction was negative due to the heavy review burden this
series has demanded thus far, however, my mind was changing even as I
composed the above response. [...]
So, I think I'm fine with it, if Karthik is game.
Sounds good to me.
I just read the conversation between Jeff, Junio and You about the whitespace
counter-argument and I think its good to go ahead with v6 with Jeff's suggested
change.
Since he's already pushed the changes on top of my changes to:
git://github.com/peff/git.git jk/tweaked-ref-filter
I'll just have a look and push that to the list as v6.
I reviewed the entire series again, including Peff's changes, so this
entire series is:
Reviewed-by: Eric Sunshine [off-list ref]
Karthik, feel free to include my Reviewed-by: in all the patches
(including Peff's) when you post v6.
Thanks.
Oops! I just pushed v6 before I even saw this mail.
--
Regards,
Karthik Nayak
From: Eric Sunshine <hidden> Date: 2016-06-15 23:08:17
On Wed, Feb 17, 2016 at 1:07 PM, Karthik Nayak [off-list ref] wrote:
On Wed, Feb 17, 2016 at 11:09 PM, Eric Sunshine [off-list ref] wrote:
quoted
I reviewed the entire series again, including Peff's changes, so this
entire series is:
Reviewed-by: Eric Sunshine [off-list ref]
Karthik, feel free to include my Reviewed-by: in all the patches
(including Peff's) when you post v6.
Oops! I just pushed v6 before I even saw this mail.
No problem. Junio can add my Reviewed-by: if he wants when he picks up
the series.
On Wed, Feb 17, 2016 at 11:47 PM, Eric Sunshine [off-list ref] wrote:
On Wed, Feb 17, 2016 at 1:07 PM, Karthik Nayak [off-list ref] wrote:
quoted
On Wed, Feb 17, 2016 at 11:09 PM, Eric Sunshine [off-list ref] wrote:
quoted
I reviewed the entire series again, including Peff's changes, so this
entire series is:
Reviewed-by: Eric Sunshine [off-list ref]
Karthik, feel free to include my Reviewed-by: in all the patches
(including Peff's) when you post v6.
Oops! I just pushed v6 before I even saw this mail.
No problem. Junio can add my Reviewed-by: if he wants when he picks up
the series.
That would be great :) Thanks for reviewing this series.
--
Regards,
Karthik Nayak