On Tue, Feb 16, 2016 at 05:49:19PM -0500, Eric Sunshine wrote:
> On Tue, Feb 16, 2016 at 5:34 PM, Jeff King <peff@xxxxxxxx> wrote:
> > On Tue, Feb 16, 2016 at 04:09:53PM -0500, Eric Sunshine wrote:
> >> My initial reaction was negative due to the heavy review burden this
> >> series has demanded thus far, however, my mind was changing even as I
> >> composed the above response. In retrospect, I think I'd be okay seeing
> >> a v6, for the following reasons:
> >>
> >> - I already ended up reviewing the the suggested new changes pretty
> >> closely as a side-effect of reading your proposal.
> >>
> >> - It would indeed be nice to avoid introducing
> >> strbuf_split_str_omit_term() in the first place; thus one less thing
> >> to worry about if someone ever takes on the task of retiring the
> >> strbuf_split interface.
> >>
> >> - It should be only a minimal amount of work for Karthik, thus
> >> turnaround time should be short.
> >>
> >> So, I think I'm fine with it, if Karthik is game.
> >
> > I started to write up a commit message for my proposed change. But it
> > did make me think of a counter-argument. Right now we parse
> > "%(align:10,middle)" but do not allow "%(align: 10, middle)".
> >
> > Should we? Or perhaps: might we? If the answer is yes, we are likely
> > better off with strbuf_split, because then we are only a strbuf_trim()
> > away from making that work.
>
> I also considered the issue of embedded whitespace very early on when
> reading your initial proposal, but didn't mention anything about it
> due to a vague recollection from one of the early reviews (or possibly
> a review of one of Karthik's other patch series) of someone (possibly
> Junio) saying or implying that embedded whitespace would not be
> supported. Unfortunately, I can't locate that message (assuming it
> even exists and wasn't a figment of my imagination).
Yeah, I could not find any relevant reference (though I didn't spend all
that long digging).
For reference, I rebuilt Karthik's series on top of my proposal, and the
changes are fairly minor. I pushed it to:
git://github.com/peff/git.git jk/tweaked-ref-filter
The tbdiff is below. Hopefully having that done makes it easier to
decide based on the outcome, rather than the pain of rebasing. :)
To be honest, though, I am now on the fence, considering the possible
whitespace issue.
1: 92de9c7 < --: ------- strbuf: introduce strbuf_split_str_omit_term()
2: 4845dc5 < --: ------- ref-filter: use strbuf_split_str_omit_term()
--: ------- > 1: 29177cc ref-filter: use string_list_split over strbuf_split
3: 040e9ce = 2: ed284bc ref-filter: bump 'used_atom' and related code to the top
4: c7eb061 = 3: 2a99777 ref-filter: introduce struct used_atom
5: c3e24cf = 4: b18f23b ref-filter: introduce parsing functions for each valid atom
6: 0b7fe83 = 5: e5221cc ref-filter: introduce color_atom_parser()
7: ffb3afe ! 6: 454af9c ref-filter: introduce parse_align_position()
@@ -32,21 +32,21 @@
const char *name;
cmp_type cmp_type;
@@
- align->position = ALIGN_LEFT;
-
- while (*s) {
+ string_list_split(¶ms, valp, ',', -1);
+ for (i = 0; i < params.nr; i++) {
+ const char *s = params.items[i].string;
+ int position;
+
- if (!strtoul_ui(s[0]->buf, 10, (unsigned int *)&width))
+ if (!strtoul_ui(s, 10, (unsigned int *)&width))
;
-- else if (!strcmp(s[0]->buf, "left"))
+- else if (!strcmp(s, "left"))
- align->position = ALIGN_LEFT;
-- else if (!strcmp(s[0]->buf, "right"))
+- else if (!strcmp(s, "right"))
- align->position = ALIGN_RIGHT;
-- else if (!strcmp(s[0]->buf, "middle"))
+- else if (!strcmp(s, "middle"))
- align->position = ALIGN_MIDDLE;
-+ else if ((position = parse_align_position(s[0]->buf)) >= 0)
++ else if ((position = parse_align_position(s)) >= 0)
+ align->position = position;
else
- die(_("improper format entered align:%s"), s[0]->buf);
- s++;
+ die(_("improper format entered align:%s"), s);
+ }
8: 0f0e596 ! 7: 0779954 ref-filter: introduce align_atom_parser()
@@ -43,18 +43,19 @@
+static void align_atom_parser(struct used_atom *atom, const char *arg)
+{
+ struct align *align = &atom->u.align;
-+ struct strbuf **v, **to_free;
++ struct string_list params = STRING_LIST_INIT_DUP;
++ int i;
+ unsigned int width = ~0U;
+
+ if (!arg)
+ die(_("expected format: %%(align:<width>,<position>)"));
-+ v = to_free = strbuf_split_str_omit_term(arg, ',', 0);
+
+ align->position = ALIGN_LEFT;
+
-+ while (*v) {
++ string_list_split(¶ms, arg, ',', -1);
++ for (i = 0; i < params.nr; i++) {
++ const char *s = params.items[i].string;
+ int position;
-+ const char *s = v[0]->buf;
+
+ if (!strtoul_ui(s, 10, &width))
+ ;
@@ -62,13 +63,12 @@
+ align->position = position;
+ else
+ die(_("unrecognized %%(align) argument: %s"), s);
-+ v++;
+ }
+
+ if (width == ~0U)
+ die(_("positive width expected with the %%(align) atom"));
+ align->width = width;
-+ strbuf_list_free(to_free);
++ string_list_clear(¶ms, 0);
+}
+
static struct {
@@ -130,32 +130,32 @@
continue;
- } else if (match_atom_name(name, "align", &valp)) {
- struct align *align = &v->u.align;
-- struct strbuf **s, **to_free;
+- struct string_list params = STRING_LIST_INIT_DUP;
+- int i;
- int width = -1;
-
- if (!valp)
- die(_("expected format: %%(align:<width>,<position>)"));
-
-- s = to_free = strbuf_split_str_omit_term(valp, ',', 0);
--
- align->position = ALIGN_LEFT;
-
-- while (*s) {
+- string_list_split(¶ms, valp, ',', -1);
+- for (i = 0; i < params.nr; i++) {
+- const char *s = params.items[i].string;
- int position;
-
-- if (!strtoul_ui(s[0]->buf, 10, (unsigned int *)&width))
+- if (!strtoul_ui(s, 10, (unsigned int *)&width))
- ;
-- else if ((position = parse_align_position(s[0]->buf)) >= 0)
+- else if ((position = parse_align_position(s)) >= 0)
- align->position = position;
- else
-- die(_("improper format entered align:%s"), s[0]->buf);
-- s++;
+- die(_("improper format entered align:%s"), s);
- }
-
- if (width < 0)
- die(_("positive width expected with the %%(align) atom"));
- align->width = width;
-- strbuf_list_free(to_free);
+- string_list_clear(¶ms, 0);
+ } else if (starts_with(name, "align")) {
+ v->u.align = atom->u.align;
v->handler = align_atom_handler;
9: d3dc384 ! 8: 792c89a ref-filter: align: introduce long-form syntax
@@ -45,8 +45,8 @@
--- a/ref-filter.c
+++ b/ref-filter.c
@@
+ const char *s = params.items[i].string;
int position;
- const char *s = v[0]->buf;
- if (!strtoul_ui(s, 10, &width))
+ if (skip_prefix(s, "position=", &s)) {
10: 3ae28b5 = 9: 019fee7 ref-filter: introduce remote_ref_atom_parser()
11: 06c70af = 10: f6e4f5a ref-filter: introduce contents_atom_parser()
12: c9db181 = 11: 0a84b70 ref-filter: introduce objectname_atom_parser()
--
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