Re: [RFC PATCH 3/5] pack-objects: add delta-islands support
- From
Jeff King <peff@peff.net>
- Date
- Aug 6, 2018, 13:58 UTC
- Message-ID
- <20180806135855.GC31282@sigill.intra.peff.net>
- In-Reply-To
- <CAP8UFD3uruf94d7eV9Fy25h19y_KV8uZeaZN71F_xFM2XrspWQ@mail.gmail.com>
On Mon, Aug 06, 2018 at 10:44:14AM +0200, Christian Couder wrote:
Show 20 quoted lines
> Taking a look at how we use regexec() in our code base, it looks like
> it might be better to use regexec_buf() defined in
> "git-compat-util.h".
>
> I am not completely sure about that because apply.c has:
>
> status = regexec(stamp, timestamp, ARRAY_SIZE(m), m, 0);
> if (status) {
> if (status != REG_NOMATCH)
> warning(_("regexec returned %d for input: %s"),
> status, timestamp);
> return 0;
> }
>
> Though the above uses a regex that is defined in apply.c. The regex
> doesn't come from the config file.
>
> Actually except the above there is a mix of regexec() and
> regexec_buf() in our code base, which are used with only 0, 1 or 2
> capture groups, so it is not very clear what should be used.I don't think we need regexec_buf(). The advantage it has is that it can operate on strings that aren't NUL-terminated, but that isn't the case here.
> And anyway I still don't see how we could diagnose when the end user > input requires more captures than we support.
We can use the final element as a sentinel, and complain if it gets filled in:
diff --git a/delta-islands.c b/delta-islands.c index dcc6590cc1..18426ffb18 100644 --- a/delta-islands.c +++ b/delta-islands.c @@ -375,6 +375,10 @@ static int find_island_for_ref(const char *refname, const struct object_id *oid, if (i < 0) return 0; + if (matches[ARRAY_SIZE(matches)-1].rm_so != -1) + die("island regex had too many matches (max=%d)", + (int)ARRAY_SIZE(matches) - 2); + for (m = 1; m < ARRAY_SIZE(matches); m++) { regmatch_t *match = &matches[m]; The big downside is that it only kicks in when you actually successfully make a match. So you could have: [pack] island = refs/(one)/(two)/(three)/(four)/(five)/(six)/(seven) in your config for years, and then one day it blows up when somebody actually has a ref that matches it. I think it would be fine to just say "we only respect the first N capture groups". And maybe even issue a warning (based on the detection above). I'd also be fine with bumping the "matches" array to something more ridiculous, like 32. The current value of 8 was supposed to be ridiculous already (we've never used more than 2). -Peff