[prev in list] [next in list] [prev in thread] [next in thread] 

List:       git
Subject:    Re: [PATCH v2 08/11] ref-filter: introduce prefixes for the align atom
From:       Karthik Nayak <karthik.188 () gmail ! com>
Date:       2015-12-31 13:31:50
Message-ID: CAOLa=ZRrv26j4kHT3tQZ-ch8aycAKG4cQp4DcTTXEwOvMtZx7w () mail ! gmail ! com
[Download RAW message or body]

On Thu, Dec 17, 2015 at 2:29 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Wed, Dec 16, 2015 at 10:29 AM, Karthik Nayak <karthik.188@gmail.com> 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@gmail.com>
> > ---
> > 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[prev in list] [next in list] [prev in thread] [next in thread]