Re: [PATCH 1/8] Refactor parse_loc

[Junio C Hamano <gitster@pobox.com>]

Thomas Rast <trast@student.ethz.ch> writes:

> diff --git a/line.c b/line.c
> new file mode 100644
> index 0000000..29898ec
> --- /dev/null
> +++ b/line.c
> @@ -0,0 +1,106 @@
> +#include "git-compat-util.h"
> +#include "line.h"
> +
> +/*
> + * Parse one item in the -L option
> + */
> +const char *parse_loc(const char *spec, nth_line_fn_t nth_line,
> +		void *data, long lines, long begin, long *ret)
> +{
> +        char *term;
> +        const char *line;
> +        long num;
> +        int reg_error;
> +        regex_t regexp;
> +        regmatch_t match[1];
> +
> +        /* Catch the '$' matcher, now it is used to match the last
> +         * line of the file. */

"now"?  What now, as opposed to which then?

Ahh, is it an artifact of squashing multiple patches, one that moves the
function and then another that adds a new feature?

In any case, please fix the style of multi-line comment.  I wouldn't mind
if you fixed the other one you moved from blame.c to this function (I
omitted it from the context but you know which one I mean).

> ...
> +int parse_range_arg(const char *arg, nth_line_fn_t nth_line_cb,
> +		void *cb_data, long lines, long *begin, long *end)
> +{
> +	arg = parse_loc(arg, nth_line_cb, cb_data, lines, -1, begin);
> +
> +        if (*arg == ',') {
> +		arg = parse_loc(arg+1, nth_line_cb, cb_data, lines, *begin+1, end);
> +		if (*begin > *end) {
> +			long tmp = *begin;
> +			*begin = *end;
> +			*end = tmp;
> +		}

It is somewhat unfortunate that this "swap begin and end given -L9,4" is
done here not in the caller---for some callers 9,4 and 4,9 may mean
different things.  But for now this would do.

> diff --git a/t/t8003-blame.sh b/t/t8003-blame.sh
> index 230143c..51d313e 100755
> --- a/t/t8003-blame.sh
> +++ b/t/t8003-blame.sh
> @@ -175,6 +175,12 @@ test_expect_success 'blame -L with invalid end' '
>  	grep "has only 2 lines" errors
>  '
>  
> +test_expect_success 'blame -L parses end' '
> +	git blame -L1,1 tres >out &&
> +	cat out &&
> +	test $(wc -l < out) -eq 1
> +'

What does this test exactly?  "end"?