Re: [PATCH v7 03/11] ref-filter: add option to pad atoms to the right
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Jul 30, 2015, 17:56 UTC
- Message-ID
- <CAPig+cR709U7zy3+F4eoiyUBhA=c2ZCxPc9uUoDQLOuooFxvzQ@mail.gmail.com>
- In-Reply-To
- <1438271332-10615-3-git-send-email-Karthik.188@gmail.com>
On Thu, Jul 30, 2015 at 11:48 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
Show 32 quoted lines
> Add a new atom "padright" and support %(padright:X) where X is a > number. This will align the succeeding atom or string to the left > followed by spaces for a total length of X characters. If X is less > than the atom or string length then no padding is done. > > Add tests and documentation for the same. > > Signed-off-by: Karthik Nayak <karthik.188@gmail.com> > --- > diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh > index 505a360..48caac9 100755 > --- a/t/t6302-for-each-ref-filter.sh > +++ b/t/t6302-for-each-ref-filter.sh > @@ -81,4 +81,36 @@ test_expect_success 'filtering with --contains' ' > test_cmp expect actual > ' > > +test_expect_success 'padding to the right using `padright`' ' > + cat >expect <<-\EOF && > + master| master | > + side| side | > + odd/spot| odd/spot | > + double-tag| double-tag| > + four| four | > + one| one | > + signed-tag| signed-tag| > + three| three | > + two| two | > + EOF > + git for-each-ref --format="%(refname:short)%(padright:5)|%(padright:10)%(refname:short)|" >actual && > + test_cmp expect actual > +'
In an earlier review, Matthieu pointed out that this test failed to ensure that the 'padright' value did not leak into the next atom. In a subsequent version, you fixed the test to check that condition, but now you've somewhat lost it again, at least visually. That is, because whitespace is "invisible" and because 'padright' now also affects literal strings, someone reading this test can't tell if those trailing |'s in the expected output are padded or not. You could use a different format string to prove that the 'padright' value doesn't leak. For instance:
%(padright:10)%(refname:short)|%(padright:5)|%(refname:short)
This way, as long as the two |'s are side-by-side, then you've proved that the first one wasn't affected by the preceding 'padright:10'. You could also add back the %(refname:short) at the front of the pattern, as you currently have it, if you want to prove that padding is not enabled at the start of format.
Show 19 quoted lines
> +test_expect_success 'no padding when `padright` length is smaller than atom length' ' > + cat >expect <<-\EOF && > + refs/heads/master| > + refs/heads/side| > + refs/odd/spot| > + refs/tags/double-tag| > + refs/tags/four| > + refs/tags/one| > + refs/tags/signed-tag| > + refs/tags/three| > + refs/tags/two| > + EOF > + git for-each-ref --format="%(padright:5)%(refname)|" >actual && > + test_cmp expect actual > +' > + > test_done > -- > 2.4.6