Duy Nguyen [off-list ref] writes:
quoted
quoted
+ if (skip_prefix_mem(cond, cond_len, "gitdir:", &cond, &cond_len))
+ return include_by_gitdir(cond, cond_len, 0);
+ else if (skip_prefix_mem(cond, cond_len, "gitdir/i:", &cond, &cond_len))
+ return include_by_gitdir(cond, cond_len, 1);
This may be OK for now, but it should be trivial to start from a
table with two entries, i.e.
struct include_cond {
const char *keyword;
int (*fn)(const char *, size_t);
};
and will show a better way to do things to those who follow your
footsteps.
Yeah I don't see a third include coming soon and did not go with that.
Let's way for it and refactor then.
I would have said exactly that in my message if you already had
include_by_gitdir() and include_by_gitdir_i() as separate functions.
But I didn't, because the above code gives an excuse to the person
who adds the third one to be lazy and add another "else if" for
expediency, because making it table-driven would require an extra
work to add two wrapper functions that do not have anything to do
with the third one being added.
On Fri, Feb 24, 2017 at 09:46:17AM -0800, Junio C Hamano wrote:
Duy Nguyen [off-list ref] writes:
quoted
quoted
quoted
+ if (skip_prefix_mem(cond, cond_len, "gitdir:", &cond, &cond_len))
+ return include_by_gitdir(cond, cond_len, 0);
+ else if (skip_prefix_mem(cond, cond_len, "gitdir/i:", &cond, &cond_len))
+ return include_by_gitdir(cond, cond_len, 1);
This may be OK for now, but it should be trivial to start from a
table with two entries, i.e.
struct include_cond {
const char *keyword;
int (*fn)(const char *, size_t);
};
and will show a better way to do things to those who follow your
footsteps.
Yeah I don't see a third include coming soon and did not go with that.
Let's way for it and refactor then.
I would have said exactly that in my message if you already had
include_by_gitdir() and include_by_gitdir_i() as separate functions.
But I didn't, because the above code gives an excuse to the person
who adds the third one to be lazy and add another "else if" for
expediency, because making it table-driven would require an extra
work to add two wrapper functions that do not have anything to do
with the third one being added.
I don't think driving that with a two-entry table is the right thing
here. We are as likely to add another "foobar:" entry as we are to add
another modifier "/i" modifier to "gitdir:", and it is unclear whether
that modifier would be mutually exclusive with "/i".
E.g., imagine we add "/re" for a regex, but allow a case-insensitive
regex with an "i", giving something like "gitdir/i/re:". Now you would
want to drive it by matching "gitdir" first, and then collecting the
"/i/re" independently, to avoid an explosion of matches.
Driving that with a table is much more complex. I'd just as soon keep
things as simple as possible for now and worry about flagging it in
review when something new gets added.
-Peff