Re: [PATCH v2 00/10] port branch.c to use ref-filter's printing options

Karthik Nayak [off-list ref] writes:

On Thu, Oct 8, 2015 at 5:57 PM, Matthieu Moy
[off-list ref] wrote:

quoted

quoted

+                             unsigned int nobracket = 0;
+
+                             if (!strcmp(valp, ",nobracket"))
+                                     nobracket = 1;

The code to parse comma-separated values is different here and
elsewhere. I'd rather have the same code (possibly factored into a
helper function), both to get consistent behavior (you're not allowing
%(upstream:nobracket,track) for example, right?) and consistent code.

Speaking of comma-separated values, the only other place we use that is
in the align atom. Also I find this very specific to get a function out of.
Somehow I think this is the simplest way to go about this.

Well, most pieces of code start with one instance, then two, then
more ;-). When the second instance starts being different from the
first, it doesn't give a good example for the future third instance.

This particular piece of code is so important and I won't fight for a
better factored one, but in general "there are only two instances" is a
dubious argument to avoid refactoring.

Still, I find it weird to force the nobracket to be always at the same
position.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

On Thu, Oct 8, 2015 at 10:40 PM, Matthieu Moy
[off-list ref] wrote:

Karthik Nayak [off-list ref] writes:

quoted

On Thu, Oct 8, 2015 at 5:57 PM, Matthieu Moy
[off-list ref] wrote:

quoted

quoted

+                             unsigned int nobracket = 0;
+
+                             if (!strcmp(valp, ",nobracket"))
+                                     nobracket = 1;

The code to parse comma-separated values is different here and
elsewhere. I'd rather have the same code (possibly factored into a
helper function), both to get consistent behavior (you're not allowing
%(upstream:nobracket,track) for example, right?) and consistent code.

Speaking of comma-separated values, the only other place we use that is
in the align atom. Also I find this very specific to get a function out of.
Somehow I think this is the simplest way to go about this.

Well, most pieces of code start with one instance, then two, then
more ;-). When the second instance starts being different from the
first, it doesn't give a good example for the future third instance.

Totally agree with you here.

This particular piece of code is so important and I won't fight for a
better factored one, but in general "there are only two instances" is a
dubious argument to avoid refactoring.

Still, I find it weird to force the nobracket to be always at the same
position.

No i mean I could follow up with the way we use it in align, but I don't see
how I can make a function out of this.

-- 
Regards,
Karthik Nayak