Re: [PATCH 08/15] ref-filter: make "%(symref)" atom work with the ':short' modifier
On Wed, Mar 9, 2016 at 1:49 AM, Junio C Hamanowrote: > Karthik Nayak writes: > >> On Tue, Mar 8, 2016 at 7:26 AM, Jacob Keller wrote: >>> On Mon, Mar 7, 2016 at 3:08 PM, Junio C Hamano wrote: Karthik Nayak writes: > The "%(symref)" atom doesn't work when used with the ':short' modifier > because we strictly match only 'symref' for setting the 'need_symref' > indicator. Fix this by using 'starts_with()' rather than 'strcmp()'. Does that mean you also accept %(symrefgarbage) without complaining? >>> >>> Looks like patch 9 fixes this by introducing symref_atom_parser. >>> >> >> There are two ways this kinda errors can occur: >> 1. %(symrefgarbage) : This is handled by parse_ref_filter_atom() which would >> print a "fatal: unknown field name: symrefgarbage". >> 2. %(symref:garbage): This is handled by populate_value() which would print >> a "fatal: unknown symref: format garbage". >> >> Either ways we do not need to worry about this as existing code would handle >> it. Also like Jacob mentioned Patch 09 would ensure this error checking would >> happen within symref_atom_parser(). > > You forgot to mention that there is 3., which is that you just > closed the door for a new valid_atom[] that begins with substring > "symref" which does not need to flip need_symref on, I think. > > You can check valid_atom[i].name with strcmp() to achieve what you > are trying to do here, instead of checking used_atom[at].name, and I > think that would be a cleaner way to avoid all three problems. That's actually a very good point, will incorporate that, thanks! -- Regards, Karthik Nayak -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/15] ref-filter: make "%(symref)" atom work with the ':short' modifier
Karthik Nayakwrites: > On Tue, Mar 8, 2016 at 7:26 AM, Jacob Keller wrote: >> On Mon, Mar 7, 2016 at 3:08 PM, Junio C Hamano wrote: >>> Karthik Nayak writes: >>> The "%(symref)" atom doesn't work when used with the ':short' modifier because we strictly match only 'symref' for setting the 'need_symref' indicator. Fix this by using 'starts_with()' rather than 'strcmp()'. >>> >>> Does that mean you also accept %(symrefgarbage) without complaining? >>> >>> >> >> Looks like patch 9 fixes this by introducing symref_atom_parser. >> > > There are two ways this kinda errors can occur: > 1. %(symrefgarbage) : This is handled by parse_ref_filter_atom() which would > print a "fatal: unknown field name: symrefgarbage". > 2. %(symref:garbage): This is handled by populate_value() which would print > a "fatal: unknown symref: format garbage". > > Either ways we do not need to worry about this as existing code would handle > it. Also like Jacob mentioned Patch 09 would ensure this error checking would > happen within symref_atom_parser(). You forgot to mention that there is 3., which is that you just closed the door for a new valid_atom[] that begins with substring "symref" which does not need to flip need_symref on, I think. You can check valid_atom[i].name with strcmp() to achieve what you are trying to do here, instead of checking used_atom[at].name, and I think that would be a cleaner way to avoid all three problems. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/15] ref-filter: make "%(symref)" atom work with the ':short' modifier
On Tue, Mar 8, 2016 at 7:26 AM, Jacob Kellerwrote: > On Mon, Mar 7, 2016 at 3:08 PM, Junio C Hamano wrote: >> Karthik Nayak writes: >> >>> The "%(symref)" atom doesn't work when used with the ':short' modifier >>> because we strictly match only 'symref' for setting the 'need_symref' >>> indicator. Fix this by using 'starts_with()' rather than 'strcmp()'. >> >> Does that mean you also accept %(symrefgarbage) without complaining? >> >> > > Looks like patch 9 fixes this by introducing symref_atom_parser. > There are two ways this kinda errors can occur: 1. %(symrefgarbage) : This is handled by parse_ref_filter_atom() which would print a "fatal: unknown field name: symrefgarbage". 2. %(symref:garbage): This is handled by populate_value() which would print a "fatal: unknown symref: format garbage". Either ways we do not need to worry about this as existing code would handle it. Also like Jacob mentioned Patch 09 would ensure this error checking would happen within symref_atom_parser(). -- Regards, Karthik Nayak -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/15] ref-filter: make "%(symref)" atom work with the ':short' modifier
On Mon, Mar 7, 2016 at 3:08 PM, Junio C Hamanowrote: > Karthik Nayak writes: > >> The "%(symref)" atom doesn't work when used with the ':short' modifier >> because we strictly match only 'symref' for setting the 'need_symref' >> indicator. Fix this by using 'starts_with()' rather than 'strcmp()'. > > Does that mean you also accept %(symrefgarbage) without complaining? > > Looks like patch 9 fixes this by introducing symref_atom_parser. Regards, Jake -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/15] ref-filter: make "%(symref)" atom work with the ':short' modifier
Karthik Nayakwrites: > The "%(symref)" atom doesn't work when used with the ':short' modifier > because we strictly match only 'symref' for setting the 'need_symref' > indicator. Fix this by using 'starts_with()' rather than 'strcmp()'. Does that mean you also accept %(symrefgarbage) without complaining? > > Add tests for %(symref) and %(symref:short) while we're here. > > Signed-off-by: Karthik Nayak > --- > ref-filter.c| 2 +- > t/t6300-for-each-ref.sh | 24 > 2 files changed, 25 insertions(+), 1 deletion(-) > > diff --git a/ref-filter.c b/ref-filter.c > index 0e6ab75..ab860a3 100644 > --- a/ref-filter.c > +++ b/ref-filter.c > @@ -323,7 +323,7 @@ int parse_ref_filter_atom(const char *atom, const char > *ep) > valid_atom[i].parser(_atom[at], arg); > if (*atom == '*') > need_tagged = 1; > - if (!strcmp(used_atom[at].name, "symref")) > + if (starts_with(used_atom[at].name, "symref")) > need_symref = 1; > return at; > } > diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh > index 2c5f177..b06ea1c 100755 > --- a/t/t6300-for-each-ref.sh > +++ b/t/t6300-for-each-ref.sh > @@ -38,6 +38,7 @@ test_atom() { > case "$1" in > head) ref=refs/heads/master ;; >tag) ref=refs/tags/testtag ;; > + sym) ref=refs/heads/sym ;; > *) ref=$1 ;; > esac > printf '%s\n' "$3" >expected > @@ -565,4 +566,27 @@ test_expect_success 'Verify sort with multiple keys' ' > refs/tags/bogo refs/tags/master > actual && > test_cmp expected actual > ' > + > +test_expect_success 'Add symbolic ref for the following tests' ' > + git symbolic-ref refs/heads/sym refs/heads/master > +' > + > +cat >expected < +refs/heads/master > +EOF > + > +test_expect_success 'Verify usage of %(symref) atom' ' > + git for-each-ref --format="%(symref)" refs/heads/sym > actual && > + test_cmp expected actual > +' > + > +cat >expected < +heads/master > +EOF > + > +test_expect_success 'Verify usage of %(symref:short) atom' ' > + git for-each-ref --format="%(symref:short)" refs/heads/sym > actual && > + test_cmp expected actual > +' > + > test_done -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html