Carried over
http://thread.gmane.org/gmane.comp.version-control.git/279226/focus=279352. Where
we were talking about pre-parsing most of the atoms so that we do not
have to parse them in ref-filter:populate_value(), where we could now
instead only fill in necessary values. This series aims to introduce
parsing functions for atoms so that they maybe parsed before hand and
the necessary values maybe stored the introduce used_atom structure.
Karthik Nayak (10):
ref-filter: introduce a parsing function for each atom in valid_atom
ref-filter: introduce struct used_atom
ref-fitler: bump match_atom() name to the top
ref-filter: skip deref specifier in match_atom_name()
ref-filter: introduce color_atom_parser()
strbuf: introduce strbuf_split_str_without_term()
ref-filter: introduce align_atom_parser()
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 | 18 +-
ref-filter.c | 483 ++++++++++++++++++++++---------------
strbuf.c | 17 +-
strbuf.h | 14 +-
t/t6302-for-each-ref-filter.sh | 4 +-
5 files changed, 328 insertions(+), 208 deletions(-)
--
2.6.2
Introduce a parsing function for each atom in valid_atom. Using this
we can define special parsing functions for each of the atoms. Since
we have a third field in valid_atom structure, we now fill out missing
cmp_type values.
Signed-off-by: Karthik Nayak <redacted>
---
ref-filter.c | 59 ++++++++++++++++++++++++++++++-----------------------------
1 file changed, 30 insertions(+), 29 deletions(-)
Introduce color_atom_parser() which will parse a "color" atom and
store its color in the "use_atom" structure for further usage in
'populate_value()'.
Signed-off-by: Karthik Nayak <redacted>
---
ref-filter.c | 20 ++++++++++++++++----
1 file changed, 16 insertions(+), 4 deletions(-)
Introduce the 'used_array' structure which would replace the existing
implementation of 'used_array' (which a list of atoms). This helps us
parse atom's before hand and store required details into the
'used_array' for future usage.
Signed-off-by: Karthik Nayak <redacted>
---
ref-filter.c | 62 +++++++++++++++++++++++++++++++-----------------------------
1 file changed, 32 insertions(+), 30 deletions(-)
@@ -123,8 +125,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].str);+if(len==ep-atom&&!memcmp(used_atom[i].str,atom,len))returni;}
@@ -787,7 +788,8 @@ 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];+structused_atom*atom=&used_atom[i];+constchar*name=atom->str;structatom_value*v=&ref->value[i];intderef=0;constchar*refname;
@@ -37,6 +37,10 @@ static int match_atom_name(const char *name, const char *atom_name, const char *{constchar*body;+/* skip the deref specifier*/+if(name[0]=='*')+name++;+if(!skip_prefix(name,atom_name,&body))return0;/* doesn't even begin with "atom_name" */if(!body[0]){
Bump match_atom() to the top for usage in further patches.
Signed-off-by: Karthik Nayak <redacted>
---
ref-filter.c | 32 ++++++++++++++++----------------
1 file changed, 16 insertions(+), 16 deletions(-)
@@ -33,6 +33,22 @@ static struct used_atom {staticintused_atom_cnt,need_tagged,need_symref;staticintneed_color_reset_at_eol;+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;+}+staticstruct{constchar*name;cmp_typecmp_type;
@@ -260,22 +276,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 align_atom_parser() which will parse 'align' atoms and store
the required width and position into the 'used_atom' structure. While
we're here, add support for the usage of 'width=' and 'position=' when
using the 'align' atom (e.g. %(align:position=middle,width=30)).
Add documentation and modify the existing tests in t6302 to reflect
the same.
Signed-off-by: Karthik Nayak <redacted>
---
Documentation/git-for-each-ref.txt | 18 ++++---
ref-filter.c | 102 +++++++++++++++++++++++--------------
t/t6302-for-each-ref-filter.sh | 4 +-
3 files changed, 75 insertions(+), 49 deletions(-)
@@ -129,14 +129,16 @@ 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. The prefix for the+ arguments is not mandatory. 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
@@ -63,6 +69,61 @@ void color_atom_parser(struct used_atom *atom)die(_("expected format: %%(color:<color>)"));}+staticalign_typeget_align_position(constchar*type)+{+if(!strcmp(type,"right"))+returnALIGN_RIGHT;+elseif(!strcmp(type,"middle"))+returnALIGN_MIDDLE;+elseif(!strcmp(type,"left"))+returnALIGN_LEFT;+return-1;+}++voidalign_atom_parser(structused_atom*atom)+{+structalign*align=&atom->u.align;+constchar*buf=NULL;+structstrbuf**s,**to_free;+intwidth=-1;++match_atom_name(atom->str,"align",&buf);+if(!buf)+die(_("expected format: %%(align:<width>,<position>)"));+s=to_free=strbuf_split_str_without_term(buf,',',0);++/* By default align to ALGIN_LEFT */+align->position=ALIGN_LEFT;++while(*s){+intposition;+buf=s[0]->buf;++position=get_align_position(buf);++if(skip_prefix(buf,"position=",&buf)){+position=get_align_position(buf);+if(position==-1)+die(_("improper format entered align:%s"),s[0]->buf);+align->position=position;+}elseif(skip_prefix(buf,"width=",&buf)){+if(strtoul_ui(buf,10,(unsignedint*)&width))+die(_("improper format entered align:%s"),s[0]->buf);+}elseif(!strtoul_ui(buf,10,(unsignedint*)&width))+;+elseif(position!=-1)+align->position=position;+else+die(_("improper format entered align:%s"),s[0]->buf);+s++;+}++if(width<0)+die(_("positive width expected with the %%(align) atom"));+align->width=width;+strbuf_list_free(to_free);+}+staticstruct{constchar*name;cmp_typecmp_type;
@@ -97,7 +97,7 @@ test_expect_success 'left alignment is default' 'refnameisrefs/tags/three|refs/tags/threerefnameisrefs/tags/two|refs/tags/twoEOF-gitfor-each-ref--format="%(align:30)refname is %(refname)%(end)|%(refname)">actual&&+gitfor-each-ref--format="%(align:width=30)refname is %(refname)%(end)|%(refname)">actual&&test_cmpexpectactual'
@@ -113,7 +113,7 @@ test_expect_success 'middle alignment' '|refnameisrefs/tags/three|refs/tags/three|refnameisrefs/tags/two|refs/tags/twoEOF-gitfor-each-ref--format="|%(align:middle,30)refname is %(refname)%(end)|%(refname)">actual&&+gitfor-each-ref--format="|%(align:position=middle,30)refname is %(refname)%(end)|%(refname)">actual&&test_cmpexpectactual'
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.
Signed-off-by: Karthik Nayak <redacted>
---
ref-filter.c | 107 ++++++++++++++++++++++++++++++++++++-----------------------
1 file changed, 66 insertions(+), 41 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.
Signed-off-by: Karthik Nayak <redacted>
---
ref-filter.c | 66 ++++++++++++++++++++++++++++++++++++++----------------------
1 file changed, 42 insertions(+), 24 deletions(-)
@@ -212,7 +238,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 */
@@ -784,26 +805,23 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct objif(!strcmp(name,"subject"))v->s=copy_subject(subpos,sublen);-elseif(!strcmp(name,"contents:subject"))+elseif(atom->u.contents.subject)v->s=copy_subject(subpos,sublen);elseif(!strcmp(name,"body"))v->s=xmemdupz(bodypos,bodylen);-elseif(!strcmp(name,"contents:body"))+elseif(atom->u.contents.body)v->s=xmemdupz(bodypos,nonsiglen);-elseif(!strcmp(name,"contents:signature"))+elseif(atom->u.contents.signature)v->s=xmemdupz(sigpos,siglen);-elseif(!strcmp(name,"contents"))-v->s=xstrdup(subpos);-elseif(skip_prefix(name,"contents:lines=",&valp)){+elseif(atom->u.contents.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.no_lines);v->s=strbuf_detach(&s,NULL);-}+}else/* For %(contents) without modifiers */+v->s=xstrdup(subpos);}}
The current implementation of 'strbuf_split_buf()' includes the
terminator at the end of each strbuf post splitting. Include an option
wherein we can drop the terminator if required. In this context
introduce a wrapper function 'strbuf_split_str_without_term()' which
splits a given string into strbufs without including the terminator.
Signed-off-by: Karthik Nayak <redacted>
---
ref-filter.c | 5 +----
strbuf.c | 17 +++++++++++++----
strbuf.h | 14 ++++++++++----
3 files changed, 24 insertions(+), 12 deletions(-)
@@ -123,18 +123,27 @@ struct strbuf **strbuf_split_buf(const char *str, size_t slen,while(slen){intlen=slen;+intterm=with_term;if(max<=0||nr+1<max){constchar*end=memchr(str,terminator,slen);if(end)-len=end-str+1;+len=end-str+term;+else+/* When no terminator present, we must add the last character */+term=1;}t=xmalloc(sizeof(structstrbuf));strbuf_init(t,len);strbuf_add(t,str,len);ALLOC_GROW(ret,nr+2,alloc);ret[nr++]=t;-str+=len;-slen-=len;+if(!term){+str+=len+1;+slen-=len+1;+}else{+str+=len;+slen-=len;+}}ALLOC_GROW(ret,nr+1,alloc);/* In case string was empty */ret[nr]=NULL;
From: Eric Sunshine <hidden> Date: 2016-06-15 23:07:18
On Wed, Nov 11, 2015 at 2:44 PM, Karthik Nayak [off-list ref] wrote:
Introduce a parsing function for each atom in valid_atom. Using this
we can define special parsing functions for each of the atoms. Since
we have a third field in valid_atom structure, we now fill out missing
cmp_type values.
I don't get it. Why do you need to "fill out missing cmp_type values"
considering that you're never assigning the third field in this patch?
Are you planning on filling in the third field in a future patch?
Compiler diagnostic:
warning: declaration of 'struct used_atom' will not be
visible outside of this function [-Wvisibility]
Indeed, it seems rather odd to introduce the new field in this patch
but never actually do anything with it. It's difficult to understand
the intention.
From: Jeff King <hidden> Date: 2016-06-15 23:07:19
On Thu, Nov 12, 2015 at 01:14:26AM +0530, Karthik Nayak wrote:
Karthik Nayak (10):
ref-filter: introduce a parsing function for each atom in valid_atom
ref-filter: introduce struct used_atom
ref-fitler: bump match_atom() name to the top
ref-filter: skip deref specifier in match_atom_name()
ref-filter: introduce color_atom_parser()
strbuf: introduce strbuf_split_str_without_term()
ref-filter: introduce align_atom_parser()
ref-filter: introduce remote_ref_atom_parser()
ref-filter: introduce contents_atom_parser()
ref-filter: introduce objectname_atom_parser()
Hmm, your patch 10 does not seem to have made it to the list (at least I
did not ever get it, and gmane seems to be down, so I cannot check there).
-Peff
On Tue, Nov 24, 2015 at 5:14 AM, Eric Sunshine [off-list ref] wrote:
On Wed, Nov 11, 2015 at 2:44 PM, Karthik Nayak [off-list ref] wrote:
quoted
Introduce a parsing function for each atom in valid_atom. Using this
we can define special parsing functions for each of the atoms. Since
we have a third field in valid_atom structure, we now fill out missing
cmp_type values.
I don't get it. Why do you need to "fill out missing cmp_type values"
considering that you're never assigning the third field in this patch?
Are you planning on filling in the third field in a future patch?
I plan on filling that in upcoming patches. Probably, should mention that in
the commit message.
Compiler diagnostic:
warning: declaration of 'struct used_atom' will not be
visible outside of this function [-Wvisibility]
Indeed, it seems rather odd to introduce the new field in this patch
but never actually do anything with it. It's difficult to understand
the intention.
This is to make way for upcoming patches. But the compiler error is
accurate used_atom only becomes a structure in the next patch.
Should change that.
--
Regards,
Karthik Nayak
On Wed, Nov 25, 2015 at 3:18 AM, Jeff King [off-list ref] wrote:
On Thu, Nov 12, 2015 at 01:14:26AM +0530, Karthik Nayak wrote:
quoted
Karthik Nayak (10):
ref-filter: introduce a parsing function for each atom in valid_atom
ref-filter: introduce struct used_atom
ref-fitler: bump match_atom() name to the top
ref-filter: skip deref specifier in match_atom_name()
ref-filter: introduce color_atom_parser()
strbuf: introduce strbuf_split_str_without_term()
ref-filter: introduce align_atom_parser()
ref-filter: introduce remote_ref_atom_parser()
ref-filter: introduce contents_atom_parser()
ref-filter: introduce objectname_atom_parser()
Hmm, your patch 10 does not seem to have made it to the list (at least I
did not ever get it, and gmane seems to be down, so I cannot check there).
-Peff
That's weird, I'll reply to this mail with patch 10.
--
Regards,
Karthik Nayak
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.
Signed-off-by: Karthik Nayak <redacted>
---
ref-filter.c | 42 +++++++++++++++++++++++++++++++-----------
1 file changed, 31 insertions(+), 11 deletions(-)
From: Eric Sunshine <hidden> Date: 2016-06-15 23:07:20
On Wed, Nov 25, 2015 at 7:10 AM, Karthik Nayak [off-list ref] wrote:
On Tue, Nov 24, 2015 at 5:14 AM, Eric Sunshine [off-list ref] wrote:
quoted
On Wed, Nov 11, 2015 at 2:44 PM, Karthik Nayak [off-list ref] wrote:
quoted
Introduce a parsing function for each atom in valid_atom. Using this
we can define special parsing functions for each of the atoms. Since
we have a third field in valid_atom structure, we now fill out missing
cmp_type values.
I don't get it. Why do you need to "fill out missing cmp_type values"
considering that you're never assigning the third field in this patch?
Are you planning on filling in the third field in a future patch?
I plan on filling that in upcoming patches. Probably, should mention that in
the commit message.
Making it clear that this patch is preparatory for introduction of
'valid_atom' is a good idea, however, adding the unused 'valid_atom'
field in this patch is not recommended. It would be better to
introduce 'valid_atom' in the patch which actually needs it.
Compiler diagnostic:
warning: declaration of 'struct used_atom' will not be
visible outside of this function [-Wvisibility]
Indeed, it seems rather odd to introduce the new field in this patch
but never actually do anything with it. It's difficult to understand
the intention.
This is to make way for upcoming patches. But the compiler error is
accurate used_atom only becomes a structure in the next patch.
Should change that.
This problem will go away if you introduce the 'valid_atom' field in
the patch which actually needs it (as suggested above) rather than in
this patch.
On Thu, Nov 26, 2015 at 1:11 AM, Eric Sunshine [off-list ref] wrote:
On Wed, Nov 25, 2015 at 7:10 AM, Karthik Nayak [off-list ref] wrote:
quoted
On Tue, Nov 24, 2015 at 5:14 AM, Eric Sunshine [off-list ref] wrote:
quoted
On Wed, Nov 11, 2015 at 2:44 PM, Karthik Nayak [off-list ref] wrote:
quoted
Introduce a parsing function for each atom in valid_atom. Using this
we can define special parsing functions for each of the atoms. Since
we have a third field in valid_atom structure, we now fill out missing
cmp_type values.
I don't get it. Why do you need to "fill out missing cmp_type values"
considering that you're never assigning the third field in this patch?
Are you planning on filling in the third field in a future patch?
I plan on filling that in upcoming patches. Probably, should mention that in
the commit message.
Making it clear that this patch is preparatory for introduction of
'valid_atom' is a good idea, however, adding the unused 'valid_atom'
field in this patch is not recommended. It would be better to
introduce 'valid_atom' in the patch which actually needs it.
Compiler diagnostic:
warning: declaration of 'struct used_atom' will not be
visible outside of this function [-Wvisibility]
Indeed, it seems rather odd to introduce the new field in this patch
but never actually do anything with it. It's difficult to understand
the intention.
This is to make way for upcoming patches. But the compiler error is
accurate used_atom only becomes a structure in the next patch.
Should change that.
This problem will go away if you introduce the 'valid_atom' field in
the patch which actually needs it (as suggested above) rather than in
this patch.
Yup, agreed.
Thanks for your suggestions.
--
Regards,
Karthik Nayak
From: Eric Sunshine <hidden> Date: 2016-06-15 23:07:21
On Wed, Nov 11, 2015 at 2:44 PM, Karthik Nayak [off-list ref] wrote:
Introduce the 'used_array' structure which would replace the existing
I guess you meant s/used_array/used_atom/ or something?
Also, s/which would/to/
implementation of 'used_array' (which a list of atoms). This helps us
s/which a/which is a/
parse atom's before hand and store required details into the
s/atom's/atoms/
s/before hand/beforehand/
quoted hunk
'used_array' for future usage.
Signed-off-by: Karthik Nayak <redacted>
---
diff --git a/ref-filter.c b/ref-filter.c
@@ -16,6 +16,23 @@+/*+ * An atom is a valid field atom listed below, possibly prefixed with+ * a "*" to denote deref_tag().+ *+ * We parse given format string and sort specifiers, and make a list+ * of properties that we need to extract out of objects. ref_array_item+ * structure will hold an array of values extracted that can be+ * indexed with the "atom number", which is an index into this+ * array.+ */+static struct used_atom {+ const char *str;
This is really the atom's name, isn't it? If so, perhaps "name" would
be a better field name.
@@ -93,21 +110,6 @@ struct atom_value { }; /*- * An atom is a valid field atom listed above, possibly prefixed with- * a "*" to denote deref_tag().- *- * We parse given format string and sort specifiers, and make a list- * of properties that we need to extract out of objects. ref_array_item- * structure will hold an array of values extracted that can be- * indexed with the "atom number", which is an index into this- * array.- */-static const char **used_atom;-static cmp_type *used_atom_type;-static int used_atom_cnt, need_tagged, need_symref;-static int need_color_reset_at_eol;
You're moving this block of declarations up above the valid_atom[]
array because the previous patch added a new field named "parser" to
valid_atom[] which references 'struct used_atom' added by this patch
(2). I wonder if this movement should be done as a separate
preparatory patch to make it easier to review since, as it stands, the
reviewer has to read much more carefully to detect changes in the
moved block.
quoted hunk
-/*
* Used to parse format string and sort specifiers
*/
int parse_ref_filter_atom(const char *atom, const char *ep)
@@ -787,7 +788,8 @@ static void populate_value(struct ref_array_item *ref) /* Fill in specials first */ for (i = 0; i < used_atom_cnt; i++) {- const char *name = used_atom[i];+ struct used_atom *atom = &used_atom[i];+ const char *name = atom->str;
Why not just:
const char *name = used_atom[i].str;
?
From: Eric Sunshine <hidden> Date: 2016-06-15 23:07:21
On Wed, Nov 11, 2015 at 2:44 PM, Karthik Nayak [off-list ref] wrote:
Signed-off-by: Karthik Nayak <redacted>
A bit of explanation about why this change is desirable would be
welcome. I'm guessing it's because a future patch is going to make
calls to match_atom_name() with the '*' deref indicator still attached
to the name, whereas existing code does not do so.
quoted hunk
---
diff --git a/ref-filter.c b/ref-filter.c
@@ -37,6 +37,10 @@ static int match_atom_name(const char *name, const char *atom_name, const char * { const char *body;+ /* skip the deref specifier*/+ if (name[0] == '*')+ name++;+ if (!skip_prefix(name, atom_name, &body)) return 0; /* doesn't even begin with "atom_name" */ if (!body[0]) {--
From: Eric Sunshine <hidden> Date: 2016-06-15 23:07:21
On Wed, Nov 11, 2015 at 2:44 PM, Karthik Nayak [off-list ref] wrote:
The current implementation of 'strbuf_split_buf()' includes the
terminator at the end of each strbuf post splitting. Include an option
s/Include an/Add an/
wherein we can drop the terminator if required. In this context
s/required/desired/
quoted hunk
introduce a wrapper function 'strbuf_split_str_without_term()' which
splits a given string into strbufs without including the terminator.
Signed-off-by: Karthik Nayak <redacted>
---
diff --git a/ref-filter.c b/ref-filter.c
@@ -892,14 +892,11 @@ static void populate_value(struct ref_array_item *ref) * TODO: Implement a function similar to strbuf_split_str() * which would omit the separator from the end of each value. */- s = to_free = strbuf_split_str(valp, ',', 0);+ s = to_free = strbuf_split_str_without_term(valp, ',', 0); align->position = ALIGN_LEFT; while (*s) {- /* Strip trailing comma */- if (s[1])- strbuf_setlen(s[0], s[0]->len - 1);
I'd prefer to see this ref-filter.c change split out as a separate
patch so as not to pollute the otherwise single-purpose change
introduced by this patch (i.e. capability to omit the terminator).
Also, it might make sense to move this patch to the head of the
series, since it's conceptually distinct from the rest of the patches,
and could conceivably prove useful on its own, regardless of how the
rest of the series fares.
quoted hunk
if (!strtoul_ui(s[0]->buf, 10, (unsigned int *)&width))
;
else if (!strcmp(s[0]->buf, "left"))
diff --git a/strbuf.c b/strbuf.c
@@ -115,7 +115,7 @@ void strbuf_tolower(struct strbuf *sb) } struct strbuf **strbuf_split_buf(const char *str, size_t slen,- int terminator, int max)+ int terminator, int max, int with_term)
"with_term" might undesirably be interpreted as meaning "use this
particular term". Perhaps a different name, such as "include_term",
"drop_term", or "omit_term" would be a bit less ambiguous. (I think I
prefer "omit_term".)
@@ -123,18 +123,27 @@ struct strbuf **strbuf_split_buf(const char *str, size_t slen, while (slen) { int len = slen;+ int term = with_term;
"term" is not a great variable name, and is easily confused with the
existing "terminator" input argument. This is really being used as a
length adjustment, so perhaps a name such as 'term_adjust' or
'len_adjust' or something better would be preferable.
Also, since the value of 'with_term' never changes, then 'term' will
have the same value each time through the loop, thus you could
(should) hoist the declaration and initialization of 'term' outside of
the loop.
Due to the way you're using this variable ('term'), you want its value
always to be 0 or 1 but you don't do anything to ensure that. What if
the user passes in 42 rather than 0 or 1? That would mess up your
(below) calculations. Worse, what if the user passes in -42? That
would be particularly alarming. To turn this into a boolean value (0
or 1), do this instead:
int term = !!with_term;
if (max <= 0 || nr + 1 < max) {
const char *end = memchr(str, terminator, slen);
if (end)
- len = end - str + 1;
+ len = end - str + term;
+ else
+ /* When no terminator present, we must add the last character */
+ term = 1;
}
t = xmalloc(sizeof(struct strbuf));
strbuf_init(t, len);
strbuf_add(t, str, len);
ALLOC_GROW(ret, nr + 2, alloc);
ret[nr++] = t;
- str += len;
- slen -= len;
+ if (!term) {
+ str += len + 1;
+ slen -= len + 1;
+ } else {
+ str += len;
+ slen -= len;
+ }
This new logic is complex and confusing, thus difficult to review for
correctness. Rather than messing with 'len' and the existing logic,
how about instead, just adjusting the amount you store in the strbuf?
That is, instead of all the above changes, you might be able to get by
with one little change, something like this (untested):
- strbuf_add(t, str, len);
+ strbuf_add(t, str, len - !!end * !!with_term);
quoted hunk
}
ALLOC_GROW(ret, nr + 1, alloc); /* In case string was empty */
ret[nr] = NULL;
diff --git a/strbuf.h b/strbuf.h
@@ -465,19 +465,25 @@ static inline int strbuf_strip_suffix(struct strbuf *sb, const char *suffix) * For lighter-weight alternatives, see string_list_split() and * string_list_split_in_place(). */-extern struct strbuf **strbuf_split_buf(const char *, size_t,- int terminator, int max);+extern struct strbuf **strbuf_split_buf(const char *str, size_t slen,+ int terminator, int max, int with_term);
You also need to update the comment block above this declaration since
it still says that each substring includes the terminator. It also
fails to mention the new 'with_term' argument added by this patch.
+static inline struct strbuf **strbuf_split_str_without_term(const char *str,
+ int terminator, int max)
This is an uncomfortably long function name. Unfortunately, short and
sweet strbuf_split() is already taken. Perhaps
strbuf_split_str_drop_term()? strbuf_split_str_omit_term()?
strbuf_split_str_no_term()? strbuf_split_noterm()?
From: Eric Sunshine <hidden> Date: 2016-06-15 23:07:22
On Wed, Nov 11, 2015 at 2:44 PM, Karthik Nayak [off-list ref] wrote:
Introduce align_atom_parser() which will parse 'align' atoms and store
the required width and position into the 'used_atom' structure. While
we're here, add support for the usage of 'width=' and 'position=' when
using the 'align' atom (e.g. %(align:position=middle,width=30)).
This patch is doing too much by both moving code around and modifying
that code (somewhat dramatically), thus it is difficult for reviewers
to compare the old and new behaviors. It deserves to be split apart
into at least two patches. First, the code movement patch which
introduces align_atom_parser() (and possibly get_align_position())
without any behavior or logical change; then the patch which changes
behavior to recognize the spelled-out forms "width=" and "position=".
You may even want to spilt it into more patches, for instance by doing
the get_align_position() extraction in its own patch.
quoted hunk
Add documentation and modify the existing tests in t6302 to reflect
the same.
Signed-off-by: Karthik Nayak <redacted>
---
@@ -129,14 +129,16 @@ 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. The prefix for the+ arguments is not mandatory. If the contents length is more
This paragraph is so bulky that it's very easy to overlook the bit
about the "prefix for the arguments" being optional, and it's not
necessarily even clear to the casual reader what that means. It might,
therefore, be a good idea to spell it out explicitly. For instance,
you might say something like:
For brevity, the "width=" and/or "position=" prefixes may be
omitted, and bare <width> and <position> used instead.
For instance, `%(align:<width>,<position>)`.
quoted hunk
+ 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.
Taken in context of the callers, this isn't a great function name, as
it implies that it is retrieving some value, when in fact it is
parsing the input argument. A better name might be
parse_align_position().
Likewise, 'type' isn't necessarily a great argument name. You might
instead call it 'pos' or even just short and sweet 's'.
Is this still the way you want this error message to appear, or should
it show the long-form of the arguments? (I don't care strongly.)
+ s = to_free = strbuf_split_str_without_term(buf, ',', 0);
+
+ /* By default align to ALGIN_LEFT */
What is ALGIN? Regardless of the answer, this comment is not
particularly useful since it merely repeats what the code itself
already states clearly.
+ align->position = ALIGN_LEFT;
+
+ while (*s) {
+ int position;
+ buf = s[0]->buf;
+
+ position = get_align_position(buf);
Why is this assignment way up here rather than down below in the
penultimate 'else' arm where its result is actually being checked? By
moving it closer to the point of use, the logic becomes easier to
understand.
+ if (skip_prefix(buf, "position=", &buf)) {
+ position = get_align_position(buf);
+ if (position == -1)
+ die(_("improper format entered align:%s"), s[0]->buf);
At this point, you can give a better error message since you know that
you were parsing a "position=" argument. Maybe something like
"unrecognized position: %s".
+ align->position = position;
+ } else if (skip_prefix(buf, "width=", &buf)) {
+ if (strtoul_ui(buf, 10, (unsigned int *)&width))
+ die(_("improper format entered align:%s"), s[0]->buf);
Ditto regarding better error message.
+ } else if (!strtoul_ui(buf, 10, (unsigned int *)&width))
+ ;
+ else if (position != -1)
+ align->position = position;
+ else
+ die(_("improper format entered align:%s"), s[0]->buf);
Here too, it would be more user-friendly to say "unrecognized
%%(align) argument: %s".
quoted hunk
+ s++;
+ }
+
+ if (width < 0)
+ die(_("positive width expected with the %%(align) atom"));
+ align->width = width;
+ strbuf_list_free(to_free);
+}
@@ -97,7 +97,7 @@ test_expect_success 'left alignment is default' ' refname is refs/tags/three |refs/tags/three refname is refs/tags/two |refs/tags/two EOF- git for-each-ref --format="%(align:30)refname is %(refname)%(end)|%(refname)" >actual &&+ git for-each-ref --format="%(align:width=30)refname is %(refname)%(end)|%(refname)" >actual && test_cmp expect actual '
@@ -113,7 +113,7 @@ test_expect_success 'middle alignment' ' | refname is refs/tags/three |refs/tags/three | refname is refs/tags/two |refs/tags/two EOF- git for-each-ref --format="|%(align:middle,30)refname is %(refname)%(end)|%(refname)" >actual &&+ git for-each-ref --format="|%(align:position=middle,30)refname is %(refname)%(end)|%(refname)" >actual && test_cmp expect actual '
While it may sometimes be reasonable to re-purpose existing tests like
this, this probably is not one of those cases. Instead, you should be
adding new tests to check all the permutations of the new argument
handling. For instance:
%(align:42)
%(align:middle,42)
%(align:42,middle)
%(align:position=middle,42)
%(align:42,position=middle)
%(align:middle,width=42)
%(align:width=42,middle)
%(align:position=middle,width=42)
%(align:width=42,position=middle)
And, it wouldn't hurt to test handling of redundant or extra position
and width arguments. Should multiple arguments of the same type result
in an error, or should "last one wins (sliently)" be the policy? Once
you decide upon a policy, add tests to check that that policy works as
expected.
In this case, "last one wins (silently)" may be more friendly to
script writers, so it might be the better choice. You'd want to add
appropriate tests, using the various permutations. For instance:
%(align:42,width=43)
%(align:width=43,42)
%(align:42,position=middle,right)
%(align:42,right,position=middle)
On Wed, Dec 2, 2015 at 4:41 AM, Eric Sunshine [off-list ref] wrote:
On Wed, Nov 11, 2015 at 2:44 PM, Karthik Nayak [off-list ref] wrote:
quoted
Signed-off-by: Karthik Nayak <redacted>
A bit of explanation about why this change is desirable would be
welcome. I'm guessing it's because a future patch is going to make
calls to match_atom_name() with the '*' deref indicator still attached
to the name, whereas existing code does not do so.
Yes, you're correct!
Will add this in, thanks.
--
Regards,
Karthik Nayak
On Wed, Dec 2, 2015 at 1:34 PM, Eric Sunshine [off-list ref] wrote:
On Wed, Nov 11, 2015 at 2:44 PM, Karthik Nayak [off-list ref] wrote:
quoted
The current implementation of 'strbuf_split_buf()' includes the
terminator at the end of each strbuf post splitting. Include an option
s/Include an/Add an/
quoted
wherein we can drop the terminator if required. In this context
s/required/desired/
will change.
quoted
introduce a wrapper function 'strbuf_split_str_without_term()' which
splits a given string into strbufs without including the terminator.
Signed-off-by: Karthik Nayak <redacted>
---
diff --git a/ref-filter.c b/ref-filter.c
@@ -892,14 +892,11 @@ static void populate_value(struct ref_array_item *ref) * TODO: Implement a function similar to strbuf_split_str() * which would omit the separator from the end of each value. */- s = to_free = strbuf_split_str(valp, ',', 0);+ s = to_free = strbuf_split_str_without_term(valp, ',', 0); align->position = ALIGN_LEFT; while (*s) {- /* Strip trailing comma */- if (s[1])- strbuf_setlen(s[0], s[0]->len - 1);
I'd prefer to see this ref-filter.c change split out as a separate
patch so as not to pollute the otherwise single-purpose change
introduced by this patch (i.e. capability to omit the terminator).
Also, it might make sense to move this patch to the head of the
series, since it's conceptually distinct from the rest of the patches,
and could conceivably prove useful on its own, regardless of how the
rest of the series fares.
I guess it makes sense to split this into two separate patches. I'll do that and
push it to the top of the series.
quoted
if (!strtoul_ui(s[0]->buf, 10, (unsigned int *)&width))
;
else if (!strcmp(s[0]->buf, "left"))
diff --git a/strbuf.c b/strbuf.c
@@ -115,7 +115,7 @@ void strbuf_tolower(struct strbuf *sb) } struct strbuf **strbuf_split_buf(const char *str, size_t slen,- int terminator, int max)+ int terminator, int max, int with_term)
"with_term" might undesirably be interpreted as meaning "use this
particular term". Perhaps a different name, such as "include_term",
"drop_term", or "omit_term" would be a bit less ambiguous. (I think I
prefer "omit_term".)
True. I too prefer "omit_term" from what you mentioned I'll stick to that.
@@ -123,18 +123,27 @@ struct strbuf **strbuf_split_buf(const char *str, size_t slen, while (slen) { int len = slen;+ int term = with_term;
"term" is not a great variable name, and is easily confused with the
existing "terminator" input argument. This is really being used as a
length adjustment, so perhaps a name such as 'term_adjust' or
'len_adjust' or something better would be preferable.
'len_adjust' seems better, will change.
Also, since the value of 'with_term' never changes, then 'term' will
have the same value each time through the loop, thus you could
(should) hoist the declaration and initialization of 'term' outside of
the loop.
True, will move it out of the loop.
Due to the way you're using this variable ('term'), you want its value
always to be 0 or 1 but you don't do anything to ensure that. What if
the user passes in 42 rather than 0 or 1? That would mess up your
(below) calculations. Worse, what if the user passes in -42? That
would be particularly alarming. To turn this into a boolean value (0
or 1), do this instead:
int term = !!with_term;
Makes sense, will change.
quoted
if (max <= 0 || nr + 1 < max) {
const char *end = memchr(str, terminator, slen);
if (end)
- len = end - str + 1;
+ len = end - str + term;
+ else
+ /* When no terminator present, we must add the last character */
+ term = 1;
}
t = xmalloc(sizeof(struct strbuf));
strbuf_init(t, len);
strbuf_add(t, str, len);
ALLOC_GROW(ret, nr + 2, alloc);
ret[nr++] = t;
- str += len;
- slen -= len;
+ if (!term) {
+ str += len + 1;
+ slen -= len + 1;
+ } else {
+ str += len;
+ slen -= len;
+ }
This new logic is complex and confusing, thus difficult to review for
correctness. Rather than messing with 'len' and the existing logic,
how about instead, just adjusting the amount you store in the strbuf?
That is, instead of all the above changes, you might be able to get by
with one little change, something like this (untested):
- strbuf_add(t, str, len);
+ strbuf_add(t, str, len - !!end * !!with_term);
That seems about right, this should be easier to understand.
quoted
}
ALLOC_GROW(ret, nr + 1, alloc); /* In case string was empty */
ret[nr] = NULL;
diff --git a/strbuf.h b/strbuf.h
@@ -465,19 +465,25 @@ static inline int strbuf_strip_suffix(struct strbuf *sb, const char *suffix) * For lighter-weight alternatives, see string_list_split() and * string_list_split_in_place(). */-extern struct strbuf **strbuf_split_buf(const char *, size_t,- int terminator, int max);+extern struct strbuf **strbuf_split_buf(const char *str, size_t slen,+ int terminator, int max, int with_term);
You also need to update the comment block above this declaration since
it still says that each substring includes the terminator. It also
fails to mention the new 'with_term' argument added by this patch.
Will do :)
quoted
+static inline struct strbuf **strbuf_split_str_without_term(const char *str,
+ int terminator, int max)
This is an uncomfortably long function name. Unfortunately, short and
sweet strbuf_split() is already taken. Perhaps
strbuf_split_str_drop_term()? strbuf_split_str_omit_term()?
strbuf_split_str_no_term()? strbuf_split_noterm()?
strbuf_split_str_omit_term() would be nice keeping it consistent with the
variable name omit_term.
--
Regards,
Karthik Nayak
On Thu, Dec 3, 2015 at 2:53 AM, Eric Sunshine [off-list ref] wrote:
On Wed, Nov 11, 2015 at 2:44 PM, Karthik Nayak [off-list ref] wrote:
quoted
Introduce align_atom_parser() which will parse 'align' atoms and store
the required width and position into the 'used_atom' structure. While
we're here, add support for the usage of 'width=' and 'position=' when
using the 'align' atom (e.g. %(align:position=middle,width=30)).
This patch is doing too much by both moving code around and modifying
that code (somewhat dramatically), thus it is difficult for reviewers
to compare the old and new behaviors. It deserves to be split apart
into at least two patches. First, the code movement patch which
introduces align_atom_parser() (and possibly get_align_position())
without any behavior or logical change; then the patch which changes
behavior to recognize the spelled-out forms "width=" and "position=".
You may even want to spilt it into more patches, for instance by doing
the get_align_position() extraction in its own patch.
I split it into two separate patches:
1. add align_atom_parser()
2. introduce "width=" and "position=" prefixes.
I think now it seems easier to follow.
quoted
Add documentation and modify the existing tests in t6302 to reflect
the same.
Signed-off-by: Karthik Nayak <redacted>
---
@@ -129,14 +129,16 @@ 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. The prefix for the+ arguments is not mandatory. If the contents length is more
This paragraph is so bulky that it's very easy to overlook the bit
about the "prefix for the arguments" being optional, and it's not
necessarily even clear to the casual reader what that means. It might,
therefore, be a good idea to spell it out explicitly. For instance,
you might say something like:
For brevity, the "width=" and/or "position=" prefixes may be
omitted, and bare <width> and <position> used instead.
For instance, `%(align:<width>,<position>)`.
Added this in.
quoted
+ 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.
Taken in context of the callers, this isn't a great function name, as
it implies that it is retrieving some value, when in fact it is
parsing the input argument. A better name might be
parse_align_position().
Likewise, 'type' isn't necessarily a great argument name. You might
instead call it 'pos' or even just short and sweet 's'.
Is this still the way you want this error message to appear, or should
it show the long-form of the arguments? (I don't care strongly.)
I think it still holds good, not too keen on changing it either.
quoted
+ s = to_free = strbuf_split_str_without_term(buf, ',', 0);
+
+ /* By default align to ALGIN_LEFT */
What is ALGIN? Regardless of the answer, this comment is not
particularly useful since it merely repeats what the code itself
already states clearly.
will do.
quoted
+ align->position = ALIGN_LEFT;
+
+ while (*s) {
+ int position;
+ buf = s[0]->buf;
+
+ position = get_align_position(buf);
Why is this assignment way up here rather than down below in the
penultimate 'else' arm where its result is actually being checked? By
moving it closer to the point of use, the logic becomes easier to
understand.
makes sense, will do.
quoted
+ if (skip_prefix(buf, "position=", &buf)) {
+ position = get_align_position(buf);
+ if (position == -1)
+ die(_("improper format entered align:%s"), s[0]->buf);
At this point, you can give a better error message since you know that
you were parsing a "position=" argument. Maybe something like
"unrecognized position: %s".
thanks, will add this in.
quoted
+ align->position = position;
+ } else if (skip_prefix(buf, "width=", &buf)) {
+ if (strtoul_ui(buf, 10, (unsigned int *)&width))
+ die(_("improper format entered align:%s"), s[0]->buf);
Ditto regarding better error message.
will do.
quoted
+ } else if (!strtoul_ui(buf, 10, (unsigned int *)&width))
+ ;
+ else if (position != -1)
+ align->position = position;
+ else
+ die(_("improper format entered align:%s"), s[0]->buf);
Here too, it would be more user-friendly to say "unrecognized
%%(align) argument: %s".
will change.
quoted
+ s++;
+ }
+
+ if (width < 0)
+ die(_("positive width expected with the %%(align) atom"));
+ align->width = width;
+ strbuf_list_free(to_free);
+}
@@ -97,7 +97,7 @@ test_expect_success 'left alignment is default' ' refname is refs/tags/three |refs/tags/three refname is refs/tags/two |refs/tags/two EOF- git for-each-ref --format="%(align:30)refname is %(refname)%(end)|%(refname)" >actual &&+ git for-each-ref --format="%(align:width=30)refname is %(refname)%(end)|%(refname)" >actual && test_cmp expect actual '
@@ -113,7 +113,7 @@ test_expect_success 'middle alignment' ' | refname is refs/tags/three |refs/tags/three | refname is refs/tags/two |refs/tags/two EOF- git for-each-ref --format="|%(align:middle,30)refname is %(refname)%(end)|%(refname)" >actual &&+ git for-each-ref --format="|%(align:position=middle,30)refname is %(refname)%(end)|%(refname)" >actual && test_cmp expect actual '
While it may sometimes be reasonable to re-purpose existing tests like
this, this probably is not one of those cases. Instead, you should be
adding new tests to check all the permutations of the new argument
handling. For instance:
%(align:42)
%(align:middle,42)
%(align:42,middle)
%(align:position=middle,42)
%(align:42,position=middle)
%(align:middle,width=42)
%(align:width=42,middle)
%(align:position=middle,width=42)
%(align:width=42,position=middle)
And, it wouldn't hurt to test handling of redundant or extra position
and width arguments. Should multiple arguments of the same type result
in an error, or should "last one wins (sliently)" be the policy? Once
you decide upon a policy, add tests to check that that policy works as
expected.
Currently like you said, it works on a "last one wins(silently)" policy.
I think this is what I'd stick with.
So like you said I'll implement the combinations of tests as suggested
and those suggested below too.
In this case, "last one wins (silently)" may be more friendly to
script writers, so it might be the better choice. You'd want to add
appropriate tests, using the various permutations. For instance:
%(align:42,width=43)
%(align:width=43,42)
%(align:42,position=middle,right)
%(align:42,right,position=middle)
Thanks for the review :)
--
Regards,
Karthik Nayak
From: Eric Sunshine <hidden> Date: 2016-06-15 23:07:26
On Wed, Nov 11, 2015 at 2:44 PM, Karthik Nayak [off-list ref] wrote:
quoted hunk
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.
Signed-off-by: Karthik Nayak <redacted>
---
Are 'shorten', 'track', and 'trackshort' mutually exclusive? If so, a
simple enum would be clearer than bitfields:
union {
const char *color;
struct align align;
enum { RR_PLAIN, RR_SHORTEN, RR_TRACK, RR_TRACKSHORT }
remote_ref;
};
Or something.
quoted hunk
} u;
} *used_atom;
static int used_atom_cnt, need_tagged, need_symref;
@@ -69,6 +74,24 @@ void color_atom_parser(struct used_atom *atom)+void remote_ref_atom_parser(struct used_atom *atom)+{+ const char *buf;++ buf = strchr(atom->str, ':');+ if (!buf)+ return;+ buf++;+ if (!strcmp(buf, "short"))+ atom->u.remote_ref.shorten = 1;+ else if (!strcmp(buf, "track"))+ atom->u.remote_ref.track = 1;+ else if (!strcmp(buf, "trackshort"))+ atom->u.remote_ref.trackshort = 1;+ else+ die(_("improper format entered align:%s"), buf);
"align:"? Also, how about a more grammatically-friendly error message?
Tangent: These xstrfmt()'d strings are getting leaked, right? Is that
something that we need to worry about (if, for instance, a repository
contains a lot of tracking refs)? Should there be a NEEDSWORK comment
here regarding the issue?
quoted hunk
+ } else if (atom->u.remote_ref.trackshort) {
+ if (stat_tracking_info(branch, &num_ours,
+ &num_theirs, NULL))
+ return;
+
+ if (!num_ours && !num_theirs)
+ *s = "=";
+ else if (!num_ours)
+ *s = "<";
+ else if (!num_theirs)
+ *s = ">";
+ else
+ *s = "<>";
+ } else
+ *s = refname;
+}
+
/*
* Parse the object referred by ref, and grab needed value.
*/
@@ -948,49 +1011,11 @@ static void populate_value(struct ref_array_item *ref) formatp = strchr(name, ':'); if (formatp) {- int num_ours, num_theirs;- formatp++; if (!strcmp(formatp, "short")) refname = shorten_unambiguous_ref(refname, warn_ambiguous_refs);
Is this duplicating work already done by fill_remote_ref_details()?
From: Eric Sunshine <hidden> Date: 2016-06-15 23:07:26
On Wed, Nov 11, 2015 at 2:44 PM, Karthik Nayak [off-list ref] wrote:
quoted hunk
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.
Signed-off-by: Karthik Nayak <redacted>
---
Same question as patch 8/10: With the exception of non-bitfield
'no_lines', are these 'contents' flags mutually exclusive? If so, an
enum would be a more natural representation than bitfields.
quoted hunk
} u;
} *used_atom;
static int used_atom_cnt, need_tagged, need_symref;
@@ -784,26 +805,23 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj if (!strcmp(name, "subject")) v->s = copy_subject(subpos, sublen);- else if (!strcmp(name, "contents:subject"))+ else if (atom->u.contents.subject) v->s = copy_subject(subpos, sublen);
With the disclaimer that I haven't fully digested the existing logic,
is there a reason that you don't also preprocess bare "subject" as you
preprocess "contents:subject"? Isn't "subject" just historic aliases
for "contents:subject"?
A similar observation may be made about "body" and "contents:body",
although I see they mean slightly different things (for, I suppose,
historical reasons).
else if (!strcmp(name, "body"))
v->s = xmemdupz(bodypos, bodylen);
- else if (!strcmp(name, "contents:body"))
+ else if (atom->u.contents.body)
v->s = xmemdupz(bodypos, nonsiglen);
- else if (!strcmp(name, "contents:signature"))
+ else if (atom->u.contents.signature)
v->s = xmemdupz(sigpos, siglen);
- else if (!strcmp(name, "contents"))
- v->s = xstrdup(subpos);
- else if (skip_prefix(name, "contents:lines=", &valp)) {
+ else if (atom->u.contents.lines) {
struct strbuf s = STRBUF_INIT;
const char *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.no_lines);
v->s = strbuf_detach(&s, NULL);
- }
+ } else /* For %(contents) without modifiers */
Too many blanks following '/*'; downcase 'for' or drop it altogether:
/* bare %(contents) */
Also, isn't this logic somewhat bogus? u.contents.all is set to 1 for
both bare %(contents) and decorated %(contents:whatever). Then, below,
you also set one of .body, .signature, .subject, or .lines if a
decoration is specified. So, now you have both .all and one of the
other attributes set to 1, which is rather nonsensical (if I
understand correctly).
If you change this to an enum as suggested in my previous email, then
the problem goes away.
quoted
+ if (!strcmp(buf, "body"))
+ atom->u.contents.body = 1;
+ else if (!strcmp(buf, "signature"))
+ atom->u.contents.signature = 1;
+ else if (!strcmp(buf, "subject"))
+ atom->u.contents.subject = 1;
+ else if (skip_prefix(buf, "lines=", &buf)) {
+ atom->u.contents.lines = 1;
+ if (strtoul_ui(buf, 10, &atom->u.contents.no_lines))
+ die(_("positive value expected contents:lines=%s"), buf);
+ } else
+ die(_("improper format entered contents:%s"), buf);
+}
From: Eric Sunshine <hidden> Date: 2016-06-15 23:07:26
On Wed, Nov 25, 2015 at 8:44 AM, Karthik Nayak [off-list ref] wrote:
quoted hunk
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.
Signed-off-by: Karthik Nayak <redacted>
---
Same comment as in my patch 8 and 9 reviews: If 'shorten' and 'full'
are mutually exclusive, then an enum would be clearer. In fact, if
there are only these two states (full and short), then this could be a
simple boolean named 'shorten'.
quoted hunk
} u;
} *used_atom;
static int used_atom_cnt, need_tagged, need_symref;
On Sun, Dec 13, 2015 at 6:23 AM, Eric Sunshine [off-list ref] wrote:
On Wed, Nov 11, 2015 at 2:44 PM, Karthik Nayak [off-list ref] wrote:
quoted
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.
Signed-off-by: Karthik Nayak <redacted>
---
Are 'shorten', 'track', and 'trackshort' mutually exclusive? If so, a
simple enum would be clearer than bitfields:
union {
const char *color;
struct align align;
enum { RR_PLAIN, RR_SHORTEN, RR_TRACK, RR_TRACKSHORT }
remote_ref;
};
Or something.
Sure, will do that.
quoted
} u;
} *used_atom;
static int used_atom_cnt, need_tagged, need_symref;
@@ -69,6 +74,24 @@ void color_atom_parser(struct used_atom *atom)+void remote_ref_atom_parser(struct used_atom *atom)+{+ const char *buf;++ buf = strchr(atom->str, ':');+ if (!buf)+ return;+ buf++;+ if (!strcmp(buf, "short"))+ atom->u.remote_ref.shorten = 1;+ else if (!strcmp(buf, "track"))+ atom->u.remote_ref.track = 1;+ else if (!strcmp(buf, "trackshort"))+ atom->u.remote_ref.trackshort = 1;+ else+ die(_("improper format entered align:%s"), buf);
"align:"? Also, how about a more grammatically-friendly error message?
Tangent: These xstrfmt()'d strings are getting leaked, right? Is that
something that we need to worry about (if, for instance, a repository
contains a lot of tracking refs)? Should there be a NEEDSWORK comment
here regarding the issue?
This is sort of a problem with most of the values in ref-filter, we dynamically
allocate memory and do not free it, since the program exits soon after and
we leave it to the Operating System to do the garbage collection.
Not sure if we'd want to work on this though.
quoted
+ } else if (atom->u.remote_ref.trackshort) {
+ if (stat_tracking_info(branch, &num_ours,
+ &num_theirs, NULL))
+ return;
+
+ if (!num_ours && !num_theirs)
+ *s = "=";
+ else if (!num_ours)
+ *s = "<";
+ else if (!num_theirs)
+ *s = ">";
+ else
+ *s = "<>";
+ } else
+ *s = refname;
+}
+
/*
* Parse the object referred by ref, and grab needed value.
*/
@@ -948,49 +1011,11 @@ static void populate_value(struct ref_array_item *ref) formatp = strchr(name, ':'); if (formatp) {- int num_ours, num_theirs;- formatp++; if (!strcmp(formatp, "short")) refname = shorten_unambiguous_ref(refname, warn_ambiguous_refs);
Is this duplicating work already done by fill_remote_ref_details()?
No, this is only activated when using "refname". fill_remote_ref_details()
is used by the %(upstream) and %(push) atoms, both of which skip the
loop using "continue" in populate_value().
--
Regards,
Karthik Nayak
From: Eric Sunshine <hidden> Date: 2016-06-15 23:07:26
On Thu, Dec 3, 2015 at 8:35 AM, Karthik Nayak [off-list ref] wrote:
On Wed, Dec 2, 2015 at 4:57 AM, Eric Sunshine [off-list ref] wrote:
quoted
On Wed, Nov 11, 2015 at 2:44 PM, Karthik Nayak [off-list ref] wrote:
quoted
@@ -833,11 +846,10 @@ static void populate_value(struct ref_array_item *ref) refname = branch_get_push(branch, NULL); if (!refname) continue;- } else if (match_atom_name(name, "color", &valp)) {+ } else if (starts_with(name, "color")) {
Hmm, so this will also match "colorize". Is that desirable?
Well the error checking is done when we parse the atom in color_atom_parser()
so here we don't need to worry about something like this.
I'm not sure that I understand your response. Let's say that, in the
future, someone adds a new atom named "colorize" (which may or may not
have its own parser in the valid_atom[] table). color_atom_parser()
will never see that atom, thus error checking in color_atom_parser()
is not relevant to this case. What is relevant is that the original
code:
} if (match_atom_name(name, "color", &valp)) {
only matched %(color) or %(color:whatever). It did not match
%(colorize). However, the new code:
} else if (starts_with(name, "color")) {
is far looser and will match %(colorize) and %(color) and
%(color:whatever) and %(coloranything), which is potentially
undesirable. It's true that the person adding %(colorize) could be
careful and ensure that the if/else chain checks %(colorize) first:
} else if (!strcmp(name, "colorize") {
...
} else if (starts_with(name, "color")) {
...
} else ...
but that places a certain extra burden on that person. Alternately,
you can tighten the matching so that it is as strict as the original:
} else if (!strcmp(name, "color") || starts_with(name, "color:")) {
...
} else ...
Or perhaps upgrade match_atom_name() to make the 'val' argument
optional, in which case you might be able to do something like this:
} else if (match_atom_name(name, "color", NULL) {
(However, if you introduce an 'enum atom_type' as suggested in my
response to the cover letter, then this problem goes away because
you'd be switching on the enum value, which is determinate, rather
than on a partial string, which may be ambiguous, as illustrated.)
Tangent: These xstrfmt()'d strings are getting leaked, right? Is that
something that we need to worry about (if, for instance, a repository
contains a lot of tracking refs)? Should there be a NEEDSWORK comment
here regarding the issue?
This is sort of a problem with most of the values in ref-filter, we dynamically
allocate memory and do not free it, since the program exits soon after and
we leave it to the Operating System to do the garbage collection.
I'm not worried about memory dynamically allocated for the used_atom[]
array being leaked (and cleaned up automatically at program exit), but
rather about memory being leaked for each processed reference, which
might become substantial for a project with a lot of references.
Not sure if we'd want to work on this though.
It's likely outside the scope of the current patch series anyhow, and
probably not something that needs to be tackled right away (or perhaps
ever), which is why a NEEDSWORK comment might be appropriate, as a
reminder that the situation could be improved.
On Sun, Dec 13, 2015 at 11:32 AM, Karthik Nayak [off-list ref] wrote:
On Sun, Dec 13, 2015 at 6:23 AM, Eric Sunshine [off-list ref] wrote:
quoted
On Wed, Nov 11, 2015 at 2:44 PM, Karthik Nayak [off-list ref] wrote:
quoted
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.
Signed-off-by: Karthik Nayak <redacted>
---
Are 'shorten', 'track', and 'trackshort' mutually exclusive? If so, a
simple enum would be clearer than bitfields:
union {
const char *color;
struct align align;
enum { RR_PLAIN, RR_SHORTEN, RR_TRACK, RR_TRACKSHORT }
remote_ref;
};
Or something.
Sure, will do that.
There's also a slight problem with using enum's with the current implementation.
The problem is the enum is set to 0 by default (since we use memset).
so the first
value is set by default, not something we'd want. So either we stick
to the structure
with unsigned bits or we introduce a pseudo value in the enum. I
prefer the former.
On Sun, Dec 13, 2015 at 11:45 AM, Eric Sunshine [off-list ref] wrote:
On Sun, Dec 13, 2015 at 1:02 AM, Karthik Nayak [off-list ref] wrote:
quoted
On Sun, Dec 13, 2015 at 6:23 AM, Eric Sunshine [off-list ref] wrote:
quoted
On Wed, Nov 11, 2015 at 2:44 PM, Karthik Nayak [off-list ref] wrote:
Tangent: These xstrfmt()'d strings are getting leaked, right? Is that
something that we need to worry about (if, for instance, a repository
contains a lot of tracking refs)? Should there be a NEEDSWORK comment
here regarding the issue?
This is sort of a problem with most of the values in ref-filter, we dynamically
allocate memory and do not free it, since the program exits soon after and
we leave it to the Operating System to do the garbage collection.
I'm not worried about memory dynamically allocated for the used_atom[]
array being leaked (and cleaned up automatically at program exit), but
rather about memory being leaked for each processed reference, which
might become substantial for a project with a lot of references.
quoted
Not sure if we'd want to work on this though.
It's likely outside the scope of the current patch series anyhow, and
probably not something that needs to be tackled right away (or perhaps
ever), which is why a NEEDSWORK comment might be appropriate, as a
reminder that the situation could be improved.
Yes I got what you're saying. I'm talking about other values which are
dynamically
allocated in ref-filter itself.
For e.g.
When we use the deref option
v->s = xstrfmt("%s^{}", refname);
or the color option
v->s = xstrdup(color);
So seems like we need a way to go around all of these.
--
Regards,
Karthik Nayak
From: Eric Sunshine <hidden> Date: 2016-06-15 23:07:26
On Sun, Dec 13, 2015 at 3:32 AM, Karthik Nayak [off-list ref] wrote:
quoted
On Sun, Dec 13, 2015 at 6:23 AM, Eric Sunshine [off-list ref] wrote:
quoted
On Wed, Nov 11, 2015 at 2:44 PM, Karthik Nayak [off-list ref] wrote:
quoted
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.
Signed-off-by: Karthik Nayak <redacted>
---
Are 'shorten', 'track', and 'trackshort' mutually exclusive? If so, a
simple enum would be clearer than bitfields:
union {
const char *color;
struct align align;
enum { RR_PLAIN, RR_SHORTEN, RR_TRACK, RR_TRACKSHORT }
remote_ref;
};
Or something.
There's also a slight problem with using enum's with the current implementation.
The problem is the enum is set to 0 by default (since we use memset).
so the first value is set by default, not something we'd want.
I'm afraid I don't see the problem. Doesn't the RR_PLAIN in the
example cover this case?
So either we stick to the structure
with unsigned bits or we introduce a pseudo value in the enum. I
prefer the former.
It's not a pseudo-value, but rather just one of the mutually exclusive states.
On Sun, Dec 13, 2015 at 2:15 PM, Eric Sunshine [off-list ref] wrote:
On Sun, Dec 13, 2015 at 3:32 AM, Karthik Nayak [off-list ref] wrote:
quoted
quoted
On Sun, Dec 13, 2015 at 6:23 AM, Eric Sunshine [off-list ref] wrote:
quoted
On Wed, Nov 11, 2015 at 2:44 PM, Karthik Nayak [off-list ref] wrote:
quoted
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.
Signed-off-by: Karthik Nayak <redacted>
---
Are 'shorten', 'track', and 'trackshort' mutually exclusive? If so, a
simple enum would be clearer than bitfields:
union {
const char *color;
struct align align;
enum { RR_PLAIN, RR_SHORTEN, RR_TRACK, RR_TRACKSHORT }
remote_ref;
};
Or something.
There's also a slight problem with using enum's with the current implementation.
The problem is the enum is set to 0 by default (since we use memset).
so the first value is set by default, not something we'd want.
I'm afraid I don't see the problem. Doesn't the RR_PLAIN in the
example cover this case?
quoted
So either we stick to the structure
with unsigned bits or we introduce a pseudo value in the enum. I
prefer the former.
It's not a pseudo-value, but rather just one of the mutually exclusive states.
This example is actually fine, the next one "%(contents)" is more of
the problem,
the check is done in grab_sub_body_contents() where previously "contents.all"
would be enough to check if we need to add contents value. Now the first enum
value is selected. Maybe have a "NOT_VALID" field in the enum.
--
Regards,
Karthik Nayak
On Sun, Dec 13, 2015 at 11:35 AM, Eric Sunshine [off-list ref] wrote:
On Thu, Dec 3, 2015 at 8:35 AM, Karthik Nayak [off-list ref] wrote:
quoted
On Wed, Dec 2, 2015 at 4:57 AM, Eric Sunshine [off-list ref] wrote:
quoted
On Wed, Nov 11, 2015 at 2:44 PM, Karthik Nayak [off-list ref] wrote:
quoted
@@ -833,11 +846,10 @@ static void populate_value(struct ref_array_item *ref) refname = branch_get_push(branch, NULL); if (!refname) continue;- } else if (match_atom_name(name, "color", &valp)) {+ } else if (starts_with(name, "color")) {
Hmm, so this will also match "colorize". Is that desirable?
Well the error checking is done when we parse the atom in color_atom_parser()
so here we don't need to worry about something like this.
I'm not sure that I understand your response. Let's say that, in the
future, someone adds a new atom named "colorize" (which may or may not
have its own parser in the valid_atom[] table). color_atom_parser()
will never see that atom, thus error checking in color_atom_parser()
is not relevant to this case. What is relevant is that the original
code:
} if (match_atom_name(name, "color", &valp)) {
only matched %(color) or %(color:whatever). It did not match
%(colorize). However, the new code:
} else if (starts_with(name, "color")) {
is far looser and will match %(colorize) and %(color) and
%(color:whatever) and %(coloranything), which is potentially
undesirable. It's true that the person adding %(colorize) could be
careful and ensure that the if/else chain checks %(colorize) first:
} else if (!strcmp(name, "colorize") {
...
} else if (starts_with(name, "color")) {
...
} else ...
but that places a certain extra burden on that person. Alternately,
you can tighten the matching so that it is as strict as the original:
} else if (!strcmp(name, "color") || starts_with(name, "color:")) {
...
} else ...
Or perhaps upgrade match_atom_name() to make the 'val' argument
optional, in which case you might be able to do something like this:
} else if (match_atom_name(name, "color", NULL) {
(However, if you introduce an 'enum atom_type' as suggested in my
response to the cover letter, then this problem goes away because
you'd be switching on the enum value, which is determinate, rather
than on a partial string, which may be ambiguous, as illustrated.)
Ah, Thanks for putting it out. I understand, I'll work on the enum atom_type
now. that should take care of this :)
--
Regards,
Karthik Nayak
On Sun, Dec 13, 2015 at 8:40 AM, Eric Sunshine [off-list ref] wrote:
On Wed, Nov 11, 2015 at 2:44 PM, Karthik Nayak [off-list ref] wrote:
quoted
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.
Signed-off-by: Karthik Nayak <redacted>
---
Same question as patch 8/10: With the exception of non-bitfield
'no_lines', are these 'contents' flags mutually exclusive? If so, an
enum would be a more natural representation than bitfields.
Like I said, an enum would cause problems here.
If you see the code flow when we check for contents we currently (as
per this patch)
use the 'all' bit to denote usage of contents, but when using an enum we would
set the enum to the first value when using memset.
But this should go away with the 'enum atom_type' implementation which
you suggested.
quoted
} u;
} *used_atom;
static int used_atom_cnt, need_tagged, need_symref;
@@ -784,26 +805,23 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj if (!strcmp(name, "subject")) v->s = copy_subject(subpos, sublen);- else if (!strcmp(name, "contents:subject"))+ else if (atom->u.contents.subject) v->s = copy_subject(subpos, sublen);
With the disclaimer that I haven't fully digested the existing logic,
is there a reason that you don't also preprocess bare "subject" as you
preprocess "contents:subject"? Isn't "subject" just historic aliases
for "contents:subject"?
A similar observation may be made about "body" and "contents:body",
although I see they mean slightly different things (for, I suppose,
historical reasons).
Actually I missed that out, now that you mention it, i'll see how I
can come around this.
quoted
else if (!strcmp(name, "body"))
v->s = xmemdupz(bodypos, bodylen);
- else if (!strcmp(name, "contents:body"))
+ else if (atom->u.contents.body)
v->s = xmemdupz(bodypos, nonsiglen);
- else if (!strcmp(name, "contents:signature"))
+ else if (atom->u.contents.signature)
v->s = xmemdupz(sigpos, siglen);
- else if (!strcmp(name, "contents"))
- v->s = xstrdup(subpos);
- else if (skip_prefix(name, "contents:lines=", &valp)) {
+ else if (atom->u.contents.lines) {
struct strbuf s = STRBUF_INIT;
const char *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.no_lines);
v->s = strbuf_detach(&s, NULL);
- }
+ } else /* For %(contents) without modifiers */
Too many blanks following '/*'; downcase 'for' or drop it altogether:
/* bare %(contents) */
Also, isn't this logic somewhat bogus? u.contents.all is set to 1 for
both bare %(contents) and decorated %(contents:whatever). Then, below,
you also set one of .body, .signature, .subject, or .lines if a
decoration is specified. So, now you have both .all and one of the
other attributes set to 1, which is rather nonsensical (if I
understand correctly).
The problem is its not mutually exclusive here, the 'all' was supposed to
act as a way of checking if its a contents atom, since populate_value() doesn't
really check that again.
So if any of the others were selected we would implement %(contents:<value>)
else the bare %(contents:<value>) would be selected.
If you change this to an enum as suggested in my previous email, then
the problem goes away.
On Sun, Dec 13, 2015 at 10:19 AM, Eric Sunshine [off-list ref] wrote:
On Wed, Nov 25, 2015 at 8:44 AM, Karthik Nayak [off-list ref] wrote:
quoted
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.
Signed-off-by: Karthik Nayak <redacted>
---
Same comment as in my patch 8 and 9 reviews: If 'shorten' and 'full'
are mutually exclusive, then an enum would be clearer. In fact, if
there are only these two states (full and short), then this could be a
simple boolean named 'shorten'.
quoted
} u;
} *used_atom;
static int used_atom_cnt, need_tagged, need_symref;