| Message ID | 2c2744333ecf5662d4198bdddeee80ff4adf6acd.1611339373.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Range diff with ranges lacking dotdot | expand |
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes: > + i = strlen(range); > + c = i > 2 ? range[--i] : 0; > + if (c == '!') > + i--; /* might be ...^! */ > + else if (isdigit(c)) { > + /* handle ...^-<n> */ > + while (i > 2 && isdigit(range[--i])) > + ; /* keep trimming trailing digits */ > + if (i < 2 || range[i--] != '-') > + return 0; > + } else > + return 0; > + > + /* Before the `!` or the `-<n>`, we expect `<rev>^` */ > + return i > 0 && range[i] == '^'; This is still way too complex for my liking, but at least I cannot immediately spot a glaring off-by-one like the previous round ;-) This is a tangent ([*1*]), but we often equate an omission to implicitly specified HEAD; e.g. "git log @{u}.." is what we did since we started building on top of our upstream. I wonder if it makes sense to allow similar short-hand so that ^! alone would mean HEAD^!, ^@ alone would mean HEAD^@, and so on. Thanks. [Footnote] *1* read: this has nothing to do with how ready I think this patch is, but the topic reminds me of it strongly enough that I raise it here, because I know the opinions on this unrelated thing on recipients of this response are worth listening to.
Hi Junio, On Fri, 22 Jan 2021, Junio C Hamano wrote: > "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> > writes: > > > + i = strlen(range); > > + c = i > 2 ? range[--i] : 0; > > + if (c == '!') > > + i--; /* might be ...^! */ > > + else if (isdigit(c)) { > > + /* handle ...^-<n> */ > > + while (i > 2 && isdigit(range[--i])) > > + ; /* keep trimming trailing digits */ > > + if (i < 2 || range[i--] != '-') > > + return 0; > > + } else > > + return 0; > > + > > + /* Before the `!` or the `-<n>`, we expect `<rev>^` */ > > + return i > 0 && range[i] == '^'; > > This is still way too complex for my liking, but at least I cannot > immediately spot a glaring off-by-one like the previous round ;-) Maybe I am too stuck with the idea of avoiding regular expressions after that StackOverflow incident... Maybe static regex_t *regex; if (strstr(range, "..")) return 1; if (!regex) { regex = xmalloc(sizeof(*regex)); if (regcomp(regex, "\\^(!|-[0-9]*)$", REG_EXTENDED)) BUG("could not compile regex"); } return !regexec(regex, range, 0, NULL, 0); is more readable, and acceptable in this context? > This is a tangent ([*1*]), but we often equate an omission to > implicitly specified HEAD; e.g. "git log @{u}.." is what we did > since we started building on top of our upstream. I wonder if it > makes sense to allow similar short-hand so that ^! alone would mean > HEAD^!, ^@ alone would mean HEAD^@, and so on. I don't know. On one hand, it would make things more consistent. On the other hand, it would be relatively hard to explain, I think. And we already have the shortcut `@!^`, which is hard enough to explain as it is. Ciao, Dscho
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > Maybe I am too stuck with the idea of avoiding regular expressions after > that StackOverflow incident... Maybe > > static regex_t *regex; > > if (strstr(range, "..")) > return 1; > > if (!regex) { > regex = xmalloc(sizeof(*regex)); > if (regcomp(regex, "\\^(!|-[0-9]*)$", REG_EXTENDED)) > BUG("could not compile regex"); > } > > return !regexec(regex, range, 0, NULL, 0); > > is more readable, and acceptable in this context? Readable, yes, acceptable, I don't know, and I am not even sure if I want to be the one to judge to be honest ;-) Have you tried the approach to feed the thing to setup_revisions() and inspect what objects are in revs.pending? When you got a valid range, you should find one or more positive and one or more negative commits , and the approach won't be fooled by strings like "HEAD^{/other than A..B/}". Or does the revision parsing machinery too eager to "die" when we find a syntax error? If so, scratch that idea, but in the longer haul, fixing these die's would also be something we'd want to do to make more parts of libgit.a callable as a proper library. Thanks.
Hi Junio, On Wed, 27 Jan 2021, Junio C Hamano wrote: > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > > > Maybe I am too stuck with the idea of avoiding regular expressions after > > that StackOverflow incident... Maybe > > > > static regex_t *regex; > > > > if (strstr(range, "..")) > > return 1; > > > > if (!regex) { > > regex = xmalloc(sizeof(*regex)); > > if (regcomp(regex, "\\^(!|-[0-9]*)$", REG_EXTENDED)) > > BUG("could not compile regex"); > > } > > > > return !regexec(regex, range, 0, NULL, 0); > > > > is more readable, and acceptable in this context? > > Readable, yes, acceptable, I don't know, and I am not even sure if I > want to be the one to judge to be honest ;-) > > Have you tried the approach to feed the thing to setup_revisions() > and inspect what objects are in revs.pending? I hadn't thought of that. > When you got a valid range, you should find one or more positive and > one or more negative commits , and the approach won't be fooled by > strings like "HEAD^{/other than A..B/}". > > Or does the revision parsing machinery too eager to "die" when we > find a syntax error? If so, scratch that idea, but in the longer > haul, fixing these die's would also be something we'd want to do to > make more parts of libgit.a callable as a proper library. It should probably be libified anyway if it calles `die()` (I don't know off the top of my head). Worse than the `die()` is probably the allocated memory; IIRC there is no way to release the memory allocated by `setup_revisions()` unless one performs a revwalk. Will try to find some time to play with the `setup_revisions()` idea. Ciao, Dscho
Hi Junio, On Thu, 28 Jan 2021, Junio C Hamano wrote: > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > > > Worse than the `die()` is probably the allocated memory; IIRC there is no > > way to release the memory allocated by `setup_revisions()` unless one > > performs a revwalk. > > Hmph, do you mean the singleton instances of "struct object" and > those specific types that contain it? I am concerned when I see what `add_rev_cmdline()` does, for example. Ciao, Dscho
diff --git a/revision.c b/revision.c index 00675f598a3..9ee063a2c03 100644 --- a/revision.c +++ b/revision.c @@ -4209,5 +4209,25 @@ void put_revision_mark(const struct rev_info *revs, const struct commit *commit) int specifies_commit_range(const char *range) { - return !!strstr(range, ".."); + size_t i; + char c; + + if (strstr(range, "..")) + return 1; + + i = strlen(range); + c = i > 2 ? range[--i] : 0; + if (c == '!') + i--; /* might be ...^! */ + else if (isdigit(c)) { + /* handle ...^-<n> */ + while (i > 2 && isdigit(range[--i])) + ; /* keep trimming trailing digits */ + if (i < 2 || range[i--] != '-') + return 0; + } else + return 0; + + /* Before the `!` or the `-<n>`, we expect `<rev>^` */ + return i > 0 && range[i] == '^'; } diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh index 6eb344be031..e217cecac9e 100755 --- a/t/t3206-range-diff.sh +++ b/t/t3206-range-diff.sh @@ -150,6 +150,14 @@ test_expect_success 'simple A B C (unmodified)' ' test_cmp expect actual ' +test_expect_success 'A^! and A^-<n> (unmodified)' ' + git range-diff --no-color topic^! unmodified^-1 >actual && + cat >expect <<-EOF && + 1: $(test_oid t4) = 1: $(test_oid u4) s/12/B/ + EOF + test_cmp expect actual +' + test_expect_success 'trivial reordering' ' git range-diff --no-color master topic reordered >actual && cat >expect <<-EOF &&