Re: [PATCH 1/3] range-diff: refactor check for commit range

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 22, 2021, 21:59 UTC
Message-ID: <xmqqbldg7hvb.fsf@gitster.c.googlers.com>
In-Reply-To: <0ce0953a-92b2-2ce6-1e13-6f6240933ce1@gmail.com>

Phillip Wood <phillip.wood123@gmail.com> writes:

Show 8 quoted lines

>>   +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.