Re: [PATCH 1/8] Refactor parse_loc
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Dec 14, 2010, 22:57 UTC
- Message-ID
- <7v8vzsrmhk.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <ff9def9ac3f207b9f9be9c59ec3a313c9b3d4d84.1292291624.git.trast@student.ethz.ch>
Thomas Rast <trast@student.ethz.ch> writes:
Show 24 quoted lines
> 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).
Show 13 quoted lines
> ...
> +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.
Show 13 quoted lines
> 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"?