Re: [PATCH v6 0/7] Support %(trailers) arguments in for-each-ref(1)
Jeff King writes: > Out of curiosity, do you frequently test with GETTEXT_POISON, or did you > just guess at a potential problem after reading the tests? Proper use > of test_i18ncmp is definitely something we ought to be looking for > during review, but I confess it's something I often miss. I learn of breakages after the fact, when Travis reports.
Re: [PATCH v6 0/7] Support %(trailers) arguments in for-each-ref(1)
On Tue, Oct 03, 2017 at 03:24:41PM +0900, Junio C Hamano wrote: > Jeff King writes: > > > Since that's the only thing I noticed, let's hold off on a reroll for > > the moment to see if there are any more comments (and I won't be > > surprised if Junio just picks it up with the tweak, but we'll see). > > > > Please do make sure that "make test" runs clean before posting (I > > usually run it on each commit to catch any "oops, we broke this and then > > refixed it" in the middle of the series, too). > > OK, I think fixes for both the "flipped" and the "gettext-poison" > breakages are solved locally in my tree already, so I guess this is > ready to be merged to 'next'. Yes, I think so. Out of curiosity, do you frequently test with GETTEXT_POISON, or did you just guess at a potential problem after reading the tests? Proper use of test_i18ncmp is definitely something we ought to be looking for during review, but I confess it's something I often miss. -Peff
Re: [PATCH v6 0/7] Support %(trailers) arguments in for-each-ref(1)
Jeff King writes: > Since that's the only thing I noticed, let's hold off on a reroll for > the moment to see if there are any more comments (and I won't be > surprised if Junio just picks it up with the tweak, but we'll see). > > Please do make sure that "make test" runs clean before posting (I > usually run it on each commit to catch any "oops, we broke this and then > refixed it" in the middle of the series, too). OK, I think fixes for both the "flipped" and the "gettext-poison" breakages are solved locally in my tree already, so I guess this is ready to be merged to 'next'.
Re: [PATCH v6 0/7] Support %(trailers) arguments in for-each-ref(1)
On Mon, Oct 02, 2017 at 09:15:00PM +0900, Junio C Hamano wrote: > Junio C Hamano writes: > > > Thanks. t6300 seems to show that %(contents:trailers:unfold) does > > not quite work, among other things. > > > > https://travis-ci.org/git/git/jobs/282126607#L3658 > > > > I didn't have a chance to look into it myself. > > Peff's "oops, your logic is backwards" fixes the above failure. > > We also need this on top to pass the gettext-poison build. > > diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh > index 872973b954..3bdfa02559 100755 > --- a/t/t6300-for-each-ref.sh > +++ b/t/t6300-for-each-ref.sh > @@ -685,19 +685,21 @@ test_expect_success '%(contents:trailers:only) and > %(contents:trailers:unfold) w > ' > > test_expect_success '%(trailers) rejects unknown trailers arguments' ' > + # error message cannot be checked under i18n > cat >expect <<-EOF && > fatal: unknown %(trailers) argument: unsupported > EOF > test_must_fail git for-each-ref --format="%(trailers:unsupported)" > 2>actual && > - test_cmp expect actual > + test_i18ncmp expect actual > ' > > test_expect_success '%(contents:trailers) rejects unknown trailers > arguments' ' > + # error message cannot be checked under i18n > cat >expect <<-EOF && > fatal: unknown %(trailers) argument: unsupported > EOF > test_must_fail git for-each-ref > --format="%(contents:trailers:unsupported)" 2>actual && > - test_cmp expect actual > + test_i18ncmp expect actual > ' > > test_expect_success 'basic atom: head contents:trailers' ' Thank you for pointing this out. I am not well-versed on gettext, and its usage within Git. I am happy to send out v7 of this series, or you can apply these changes in queueing. Whichever is easier :-). -- - Taylor
Re: [PATCH v6 0/7] Support %(trailers) arguments in for-each-ref(1)
Junio C Hamano writes: > Thanks. t6300 seems to show that %(contents:trailers:unfold) does > not quite work, among other things. > > https://travis-ci.org/git/git/jobs/282126607#L3658 > > I didn't have a chance to look into it myself. Peff's "oops, your logic is backwards" fixes the above failure. We also need this on top to pass the gettext-poison build. diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh index 872973b954..3bdfa02559 100755 --- a/t/t6300-for-each-ref.sh +++ b/t/t6300-for-each-ref.sh @@ -685,19 +685,21 @@ test_expect_success '%(contents:trailers:only) and %(contents:trailers:unfold) w ' test_expect_success '%(trailers) rejects unknown trailers arguments' ' + # error message cannot be checked under i18n cat >expect <<-EOF && fatal: unknown %(trailers) argument: unsupported EOF test_must_fail git for-each-ref --format="%(trailers:unsupported)" 2>actual && - test_cmp expect actual + test_i18ncmp expect actual ' test_expect_success '%(contents:trailers) rejects unknown trailers arguments' ' + # error message cannot be checked under i18n cat >expect <<-EOF && fatal: unknown %(trailers) argument: unsupported EOF test_must_fail git for-each-ref --format="%(contents:trailers:unsupported)" 2>actual && - test_cmp expect actual + test_i18ncmp expect actual ' test_expect_success 'basic atom: head contents:trailers' '
Re: [PATCH v6 0/7] Support %(trailers) arguments in for-each-ref(1)
Taylor Blau writes: > Attached is the sixth revision of my patch-set "Support %(trailers) > arguments in for-each-ref(1)". > > In includes the following changes since v5: > > * Added an additional patch to change t4205 to harden `unfold()` > against multi-line trailer folding. > > * Added a missing parameter call in ref-filter.c to > `trailers_atom_parser` through `contents_atom_parser`. > > I believe that this version of the series should be ready for queueing. > > I am going to address Peff's follow-up for teaching > `parse_ref_atom_filter` to accept "empty" argument lists as > `%(refname:)` in a follow-up series later this evening. > > Thanks again :-). Thanks. t6300 seems to show that %(contents:trailers:unfold) does not quite work, among other things. https://travis-ci.org/git/git/jobs/282126607#L3658 I didn't have a chance to look into it myself.
Re: [PATCH v6 0/7] Support %(trailers) arguments in for-each-ref(1)
On Sun, Oct 01, 2017 at 10:23:26PM -0700, Taylor Blau wrote: > Attached is the sixth revision of my patch-set "Support %(trailers) > arguments in for-each-ref(1)". > > In includes the following changes since v5: > > * Added an additional patch to change t4205 to harden `unfold()` > against multi-line trailer folding. > > * Added a missing parameter call in ref-filter.c to > `trailers_atom_parser` through `contents_atom_parser`. > > I believe that this version of the series should be ready for queueing. This looks good to me, modulo the flipped logic in the final patch. Since that's the only thing I noticed, let's hold off on a reroll for the moment to see if there are any more comments (and I won't be surprised if Junio just picks it up with the tweak, but we'll see). Please do make sure that "make test" runs clean before posting (I usually run it on each commit to catch any "oops, we broke this and then refixed it" in the middle of the series, too). -Peff
Re: [PATCH v6 0/7] Support %(trailers) arguments in for-each-ref(1)
Hi, Attached is the sixth revision of my patch-set "Support %(trailers) arguments in for-each-ref(1)". In includes the following changes since v5: * Added an additional patch to change t4205 to harden `unfold()` against multi-line trailer folding. * Added a missing parameter call in ref-filter.c to `trailers_atom_parser` through `contents_atom_parser`. I believe that this version of the series should be ready for queueing. I am going to address Peff's follow-up for teaching `parse_ref_atom_filter` to accept "empty" argument lists as `%(refname:)` in a follow-up series later this evening. Thanks again :-). -- - Taylor
