From: Phillip Wood <phillip.wood123@gmail.com>
Date: Sat, 23 Jan 2021 15:59:23 GMT
Subject: Re: [PATCH 1/3] range-diff: refactor check for commit range
Message-ID: <a0b100e7-57b8-df46-de2b-d54fdbe6a255@gmail.com>
In-Reply-To: <xmqqbldg7hvb.fsf@gitster.c.googlers.com>

Hi Junio

On 22/01/2021 21:59, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>>>    +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.
>