On Thu, Dec 17, 2015 at 2:29 PM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
> On Wed, Dec 16, 2015 at 10:29 AM, Karthik Nayak <karthik.188@xxxxxxxxx> wrote:
>> ref-filter: introduce prefixes for the align atom
>
> The prefixes are actually for the arguments to the 'align' atom, not
> for the atom itself. However, it might be better to describe this at a
> bit higher level. Perhaps:
>
> ref-filter: align: introduce long-form syntax
>
> or something.
Makes sense, thanks.
>
>> Introduce optional prefixes "width=" and "position=" for the align atom
>> so that the atom can be used as "%(align:width=<width>,position=<position>)".
>>
>> Add Documetation and tests for the same.
>>
>> Signed-off-by: Karthik Nayak <Karthik.188@xxxxxxxxx>
>> ---
>> diff --git a/ref-filter.c b/ref-filter.c
>> @@ -96,10 +96,19 @@ static void align_atom_parser(struct used_atom *atom)
>>
>> while (*s) {
>> int position;
>> + buf = s[0]->buf;
>
> It probably would be better to do this assignment in the previous
> patch (7/11) since its presence here introduces unwanted noise
> (textual replacement of "s[0]->buf" with "buf") in several locations
> below which slightly obscure the real changes of this patch.
>
This makes sense from a reviewers perspective, will do.
>> - if (!strtoul_ui(s[0]->buf, 10, (unsigned int *)&width))
>> + if (skip_prefix(buf, "position=", &buf)) {
>> + position = parse_align_position(buf);
>> + if (position == -1)
>
> It may be more idiomatic in this codebase to detect errors via '< 0'
> rather than explicit '== -1'. Ditto below.
>
I think Junio also mentioned this once. Thanks for reminding.
>> + die(_("unrecognized position:%s"), buf);
>> + align->position = position;
>> + } else if (skip_prefix(buf, "width=", &buf)) {
>> + if (strtoul_ui(buf, 10, (unsigned int *)&width))
>> + die(_("unrecognized width:%s"), buf);
>> + } else if (!strtoul_ui(buf, 10, (unsigned int *)&width))
>> ;
>> - else if ((position = parse_align_position(s[0]->buf)) > 0)
>> + else if ((position = parse_align_position(buf)) != -1)
>> align->position = position;
>> else
>> die(_("unrecognized %%(align) argument: %s"), s[0]->buf);
>
> s/s[0]->//
>
Thanks.
> More below...
>
>> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
>> @@ -133,6 +133,168 @@ test_expect_success 'right alignment' '
>> test_cmp expect actual
>> '
>>
>> +test_expect_success 'alignment with "position" prefix' '
>> + cat >expect <<-\EOF &&
>> + | refname is refs/heads/master|refs/heads/master
>> + | refname is refs/heads/side|refs/heads/side
>> + | refname is refs/odd/spot|refs/odd/spot
>> + |refname is refs/tags/double-tag|refs/tags/double-tag
>> + | refname is refs/tags/four|refs/tags/four
>> + | refname is refs/tags/one|refs/tags/one
>> + |refname is refs/tags/signed-tag|refs/tags/signed-tag
>> + | refname is refs/tags/three|refs/tags/three
>> + | refname is refs/tags/two|refs/tags/two
>> + EOF
>> + git for-each-ref --format="|%(align:30,position=right)refname is %(refname)%(end)|%(refname)" >actual &&
>> + test_cmp expect actual
>> +'
>> +
>> +test_expect_success 'alignment with "position" prefix' '
>> + cat >expect <<-\EOF &&
>> + | refname is refs/heads/master|refs/heads/master
>> + | refname is refs/heads/side|refs/heads/side
>> + | refname is refs/odd/spot|refs/odd/spot
>> + |refname is refs/tags/double-tag|refs/tags/double-tag
>> + | refname is refs/tags/four|refs/tags/four
>> + | refname is refs/tags/one|refs/tags/one
>> + |refname is refs/tags/signed-tag|refs/tags/signed-tag
>> + | refname is refs/tags/three|refs/tags/three
>> + | refname is refs/tags/two|refs/tags/two
>> + EOF
>> + git for-each-ref --format="|%(align:position=right,30)refname is %(refname)%(end)|%(refname)" >actual &&
>> + test_cmp expect actual
>> +'
>
> This (and below) is a lot of copy/paste code which makes it difficult
> to read the tests and maintain (change) them. Since 'expect' doesn't
> appear to change from test to test, one way to eliminate some of this
> noisiness would be to create 'expect' once outside of the tests.
>
> However, even better, especially from a comprehension,
> maintainability, and extensibility standpoints would be to make this
> all table-driven. In particular, I'd expect to see a table with
> exactly the list of test inputs mentioned in my earlier review[1], and
> have that table passed to a shell function which performs the test for
> each element of the table. For instance, something like:
>
> 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
>
> where test_align_permutations is the name of the shell function which
> reads each line of it stdin and performs the "git for-each-ref
> --format=..." test with the given %(align:) arguments.
>
> Ditto regarding the below "last one wins (silently) tests".
>
> [1]: http://article.gmane.org/gmane.comp.version-control.git/281916
>
This seems like a good idea, I implemented both of those together.
test_align_permutations() {
while read -r option; do
test_expect_success 'align permutations' '
git for-each-ref --format="|%(align:$option)refname is
%(refname)%(end)|%(refname)" >actual &&
test_cmp expect actual
'
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
Thanks :)
--
Regards,
Karthik Nayak
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html