Wed, Oct 14, 2015 at 12:24 AM, Matthieu Moy
<Matthieu.Moy@xxxxxxxxxxxxxxx> wrote:
> Yes, but I still think that this was a bad idea. If you want nobracket
> to apply to "track", then the syntax should be
> %(upstream:track=nobracket). I think the "nobracket" should apply to
> "upstream" (i.e. be global to the atom), hence
> %(upstream:nobracket,track) and %(upstream:track,nobracket) should both
> be accepted. Possibly %(upstream:<not track>,nobracket) could complain,
> but just ignoring "nobracket" in this case would make sense to me.
>
Oh okay, was thinking only WRT the "track" option.
> Special-casing the implementation of "nobracket" also means you're
> special-casing its user-visible behavior. And as a user, I hate it when
> the behavior is subtely different for things that look like each other
> (in this case, %(align:...) and %(upstream:...) ).
>
Makes sense, was just looking for opinions.
>> %(upstream:nobracket,track) to be supported then, I think we'll have
>> to change this whole layout and have the detection done up where we
>> locat "upstream" / "push", what would be a nice way to go around this?
>
> You mean, below
>
> else if (starts_with(name, "upstream")) {
>
> within populate_value()?
>
> I think it would, yes.
>
Yes, that's what I meant.
>> What I could think of:
>> 1. Do the cleanup that Junio and Matthieu suggested, where we
>> basically parse the
>> atoms and store them into a used_atom struct. I could either work on
>> those patches
>> before this and then rebase this on top.
>> 2. Let this be and come back on it when implementing the above series.
>> After reading Matthieu's and Junio's discussion, I lean towards the latter.
>
> Leaving it as-is does not fit in my arguments to do the refactoring
> later. It's not introducing "another instance of an existing pattern",
> but actually a new pattern.
>
I meant after changing whatever we discussed above.
--
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