From mboxrd@z Thu Jan  1 00:00:00 1970
From: peter at lekensteyn.nl (Peter Wu)
Date: Tue, 10 Jun 2014 23:07:20 +0200
Subject: Error when searching for a bogus range
In-Reply-To: <20140610205939.GA28807@serenity.lan>
References: <539748DA.5080804@kernel.org> <20140610205939.GA28807@serenity.lan>
Message-ID: <1776285.4k9HZBe05R@al>

On Tuesday 10 June 2014 21:59:39 John Keeping wrote:
> On Tue, Jun 10, 2014 at 02:05:14PM -0400, Konstantin Ryabitsev wrote:
> > cgit-0.10.1-1.el6 (EPEL version)
> > 
> > If you search for a bogus range string here:
> > 
> > http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/log/
> > 
> > Using something like "range" and "qwerty123456", it returns an "Internal
> > Server Error" and the following in the logs:
> > 
> > > [Tue Jun 10 17:45:32 2014] [error] [client 172.21.1.6] fatal: ambiguous argument 'qwerty123456': unknown revision or path not in the working tree., referer: http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/
> > > [Tue Jun 10 17:45:32 2014] [error] [client 172.21.1.6] Use '--' to separate paths from revisions, like this:, referer: http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/
> > > [Tue Jun 10 17:45:32 2014] [error] [client 172.21.1.6] 'git <command> [<revision>...] -- [<file>...]', referer: http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/
> > > [Tue Jun 10 17:45:32 2014] [error] [client 172.21.1.6] Premature end of script headers: cgit, referer: http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/
> 
> This is because we just pass the arguments straight to Git's revision
> parsing machinery which die()s if it cannot parse an argument, printing
> the above to stderr and exiting.
> 
> The patch below makes it a bit friendlier by just ignoring unhandled
> arguments, but I can't see an easy way to report errors when we can't
> parse revision arguments without losing the flexibility of supporting
> all of the revision specifiers supported by Git.

The patch looks good to me, it reduces the error to just the "fatal: ..."
message. The error can be reproduced with:

echo scan-path=. > cgitrc
CGIT_CONFIG=cgitrc PATH_INFO=/.git/log/ QUERY_STRING=qt=range\&q=foo ./cgit
(assuming .git/ in $PWD)

The accepted argument is the same as the one passed to `git rev-list`.
Unless you manually check for the possible formats (v3.15~10..v3.15,
v3.13.., ..next, foo...bar, you won't be able to catch the error (as
John noted).

Regards,
Peter

> -- >8 --
> diff --git a/ui-log.c b/ui-log.c
> index 499534c..4cb26bc 100644
> --- a/ui-log.c
> +++ b/ui-log.c
> @@ -337,16 +337,16 @@ void cgit_print_log(const char *tip, int ofs, int cnt, char *grep, char *pattern
>  	else if (commit_sort == 2)
>  		argv_array_push(&rev_argv, "--topo-order");
>  
> -	if (path) {
> -		argv_array_push(&rev_argv, "--");
> +	argv_array_push(&rev_argv, "--");
> +	if (path)
>  		argv_array_push(&rev_argv, path);
> -	}
>  
>  	init_revisions(&rev, NULL);
>  	rev.abbrev = DEFAULT_ABBREV;
>  	rev.commit_format = CMIT_FMT_DEFAULT;
>  	rev.verbose_header = 1;
>  	rev.show_root_diff = 0;
> +	rev.ignore_missing = 1;
>  	setup_revisions(rev_argv.argc, rev_argv.argv, &rev, NULL);
>  	load_ref_decorations(DECORATE_FULL_REFS);
>  	rev.show_decorations = 1;