Phillip Wood [off-list ref] writes:
quoted
+static int is_range(const char *range)
+{
+ return !!strstr(range, "..");
+}
If the user wrongly passes two arguments referring to single commits
with `:/<text>` or `@{/<text>}` where text contains ".." this will
give a false positive.
True. I do not think this aims to be complete revision parser in
the first place, though.
It is tempting to at least idly speculate if an approach to run
setup_revisions() on argument is_range() takes and checking the
result would yield a more practical solution. I would imagine that
we would want to see in the resulting revs.pending has at least one
positive and one negative, and none of them have SYMMETRIC_LEFT set
in their .flags word.
Side note: Strictly speaking, people could wish "rev" to mean
"everything reachable from the rev, down to root", so
requiring one negative may technically be a wrong
thing, but in the context of "range-diff", I am not
sure how useful such a behaviour would be.
Hi Junio
On 22/01/2021 21:59, Junio C Hamano wrote:
Phillip Wood [off-list ref] writes:
quoted
quoted
+static int is_range(const char *range)
+{
+ return !!strstr(range, "..");
+}
If the user wrongly passes two arguments referring to single commits
with `:/<text>` or `@{/<text>}` where text contains ".." this will
give a false positive.
True. I do not think this aims to be complete revision parser in
the first place, though.
Yes but it affects the error message given to the user. If I run
git range-diff $(git rev-parse HEAD^{/..q}) $(git rev-parse HEAD^{/..x})
It fails immediately with
fatal: no .. in range: 'b60863619cf47b2e1e891c2376bd4f6da8111ab1'
This patch improves the error message to say it is not a range
but
git range-diff HEAD^{/..q} HEAD^{/..x}
runs for several minutes without producing any output and eventually
fails with
fatal: Out of memory, malloc failed (tried to allocate 33846432676 bytes)
So I think it would be helpful to be more careful here.
Best Wishes
Phillip
It is tempting to at least idly speculate if an approach to run
setup_revisions() on argument is_range() takes and checking the
result would yield a more practical solution. I would imagine that
we would want to see in the resulting revs.pending has at least one
positive and one negative, and none of them have SYMMETRIC_LEFT set
in their .flags word.
Side note: Strictly speaking, people could wish "rev" to mean
"everything reachable from the rev, down to root", so
requiring one negative may technically be a wrong
thing, but in the context of "range-diff", I am not
sure how useful such a behaviour would be.
Hi Phillip,
On Sat, 23 Jan 2021, Phillip Wood wrote:
On 22/01/2021 21:59, Junio C Hamano wrote:
quoted
Phillip Wood [off-list ref] writes:
quoted
quoted
+static int is_range(const char *range)
+{
+ return !!strstr(range, "..");
+}
If the user wrongly passes two arguments referring to single commits
with `:/<text>` or `@{/<text>}` where text contains ".." this will
give a false positive.
True. I do not think this aims to be complete revision parser in
the first place, though.
Yes but it affects the error message given to the user.
True. But my patch series does not try to fix that (it is not an issue
_introduced_ by this patch series, it was there all along).
Ciao,
Dscho