From: Phillip Wood <phillip.wood123@gmail.com>
Date: Fri, 22 Jan 2021 19:12:54 GMT
Subject: Re: [PATCH 1/3] range-diff: refactor check for commit range
Message-ID: <0ce0953a-92b2-2ce6-1e13-6f6240933ce1@gmail.com>
In-Reply-To: <5839ba4f7615819ed6f9a0fcb6de1855cd2e89e1.1611267638.git.gitgitgadget@gmail.com>

Hi Dscho

On 21/01/2021 22:20, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> Currently, when called with exactly two arguments, we test for a literal
> `..` in each of the two.
> 
> However, `<commit>^!` is a perfectly valid commit range, equivalent to
> `<commit>^..<commit>` according to the `SPECIFYING RANGES` section of
> gitrevisions[7].
> 
> In preparation for allowing more sophisticated ways to specify commit
> ranges, let's refactor the conditional into its own function.
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>   builtin/range-diff.c | 13 +++++++++----
>   1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/builtin/range-diff.c b/builtin/range-diff.c
> index 24c4162f744..551d3e689cb 100644
> --- a/builtin/range-diff.c
> +++ b/builtin/range-diff.c
> @@ -11,6 +11,11 @@ N_("git range-diff [<options>] <base> <old-tip> <new-tip>"),
>   NULL
>   };
>   
> +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.

Best Wishes

Phillip

>   int cmd_range_diff(int argc, const char **argv, const char *prefix)
>   {
>   	int creation_factor = RANGE_DIFF_CREATION_FACTOR_DEFAULT;
> @@ -46,12 +51,12 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix)
>   		diffopt.use_color = 1;
>   
>   	if (argc == 2) {
> -		if (!strstr(argv[0], ".."))
> -			die(_("no .. in range: '%s'"), argv[0]);
> +		if (!is_range(argv[0]))
> +			die(_("not a commit range: '%s'"), argv[0]);
>   		strbuf_addstr(&range1, argv[0]);
>   
> -		if (!strstr(argv[1], ".."))
> -			die(_("no .. in range: '%s'"), argv[1]);
> +		if (!is_range(argv[1]))
> +			die(_("not a commit range: '%s'"), argv[1]);
>   		strbuf_addstr(&range2, argv[1]);
>   	} else if (argc == 3) {
>   		strbuf_addf(&range1, "%s..%s", argv[0], argv[1]);
>