* [PATCH] log -L: improve error message on malformed argument
@ 2015-04-16 14:43 Matthieu Moy
2015-04-16 20:59 ` Junio C Hamano
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Matthieu Moy @ 2015-04-16 14:43 UTC (permalink / raw)
To: gitster; +Cc: git, Matthieu Moy
The old message did not mention the :regex:file form.
To avoid overly long lines, split the message into two lines (in case
item->string is long, it will be the only part truncated in a narrow
terminal).
Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
line-log.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/line-log.c b/line-log.c
index a490efe..e725248 100644
--- a/line-log.c
+++ b/line-log.c
@@ -575,7 +575,8 @@ parse_lines(struct commit *commit, const char *prefix, struct string_list *args)
name_part = skip_range_arg(item->string);
if (!name_part || *name_part != ':' || !name_part[1])
- die("-L argument '%s' not of the form start,end:file",
+ die("invalid -L argument '%s'.\n"
+ "It should be of the form start,end:file or :regex:file.",
item->string);
range_part = xstrndup(item->string, name_part - item->string);
name_part++;
--
2.4.0.rc1.42.g9642cc6
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] log -L: improve error message on malformed argument
2015-04-16 14:43 [PATCH] log -L: improve error message on malformed argument Matthieu Moy
@ 2015-04-16 20:59 ` Junio C Hamano
2015-04-16 21:45 ` Junio C Hamano
2015-04-17 20:38 ` Junio C Hamano
2 siblings, 0 replies; 7+ messages in thread
From: Junio C Hamano @ 2015-04-16 20:59 UTC (permalink / raw)
To: Matthieu Moy; +Cc: git
Matthieu Moy <Matthieu.Moy@imag.fr> writes:
> The old message did not mention the :regex:file form.
>
> To avoid overly long lines, split the message into two lines (in case
> item->string is long, it will be the only part truncated in a narrow
> terminal).
>
> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
> ---
> line-log.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/line-log.c b/line-log.c
> index a490efe..e725248 100644
> --- a/line-log.c
> +++ b/line-log.c
> @@ -575,7 +575,8 @@ parse_lines(struct commit *commit, const char *prefix, struct string_list *args)
>
> name_part = skip_range_arg(item->string);
> if (!name_part || *name_part != ':' || !name_part[1])
> - die("-L argument '%s' not of the form start,end:file",
> + die("invalid -L argument '%s'.\n"
> + "It should be of the form start,end:file or :regex:file.",
> item->string);
> range_part = xstrndup(item->string, name_part - item->string);
> name_part++;
Hmm.
While I understand and even agree with the reasoning behind chomping
the line after a potentially long user-supplied argument, it
somewhat bothers me that the output of subsequent lines would begin
without the prefix.
Do we have any other multi-line die/error/warning message?
I am tempted to suggest doing something along the lines of the
attached, if we were to start using multi-line die/error/warning
like this one. Obviously we would need something similar on the
vwritef() side as well.
usage.c | 23 ++++++++++++++++++++---
1 file changed, 20 insertions(+), 3 deletions(-)
diff --git a/usage.c b/usage.c
index ed14645..09710fa 100644
--- a/usage.c
+++ b/usage.c
@@ -8,9 +8,26 @@
void vreportf(const char *prefix, const char *err, va_list params)
{
- char msg[4096];
- vsnprintf(msg, sizeof(msg), err, params);
- fprintf(stderr, "%s%s\n", prefix, msg);
+ char msg[4096], *bol;
+ int len;
+
+ len = vsnprintf(msg, sizeof(msg), err, params);
+ if (sizeof(msg) < len)
+ len = sizeof(msg);
+ bol = msg;
+ while (1) {
+ int linelen;
+ char *eol = memchr(bol, '\n', len);
+ if (!eol)
+ linelen = len;
+ else
+ linelen = eol - bol;
+ fprintf(stderr, "%s%.*s\n", prefix, linelen, bol);
+ if (!eol)
+ break;
+ bol = eol + 1;
+ len -= linelen + 1;
+ }
}
void vwritef(int fd, const char *prefix, const char *err, va_list params)
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] log -L: improve error message on malformed argument
2015-04-16 14:43 [PATCH] log -L: improve error message on malformed argument Matthieu Moy
2015-04-16 20:59 ` Junio C Hamano
@ 2015-04-16 21:45 ` Junio C Hamano
2015-04-16 21:49 ` Eric Sunshine
2015-04-17 20:38 ` Junio C Hamano
2 siblings, 1 reply; 7+ messages in thread
From: Junio C Hamano @ 2015-04-16 21:45 UTC (permalink / raw)
To: Matthieu Moy; +Cc: git
Matthieu Moy <Matthieu.Moy@imag.fr> writes:
> The old message did not mention the :regex:file form.
>
> To avoid overly long lines, split the message into two lines (in case
> item->string is long, it will be the only part truncated in a narrow
> terminal).
>
> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
> ---
> line-log.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/line-log.c b/line-log.c
> index a490efe..e725248 100644
> --- a/line-log.c
> +++ b/line-log.c
> @@ -575,7 +575,8 @@ parse_lines(struct commit *commit, const char *prefix, struct string_list *args)
>
> name_part = skip_range_arg(item->string);
> if (!name_part || *name_part != ':' || !name_part[1])
> - die("-L argument '%s' not of the form start,end:file",
> + die("invalid -L argument '%s'.\n"
> + "It should be of the form start,end:file or :regex:file.",
> item->string);
> range_part = xstrndup(item->string, name_part - item->string);
> name_part++;
You forgot to update tests to match their expectations? 4211.20
wants to see 'argument.*not of the form', it seems.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] log -L: improve error message on malformed argument
2015-04-16 21:45 ` Junio C Hamano
@ 2015-04-16 21:49 ` Eric Sunshine
0 siblings, 0 replies; 7+ messages in thread
From: Eric Sunshine @ 2015-04-16 21:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Matthieu Moy, Git List
On Thu, Apr 16, 2015 at 5:45 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Matthieu Moy <Matthieu.Moy@imag.fr> writes:
>
>> The old message did not mention the :regex:file form.
>>
>> To avoid overly long lines, split the message into two lines (in case
>> item->string is long, it will be the only part truncated in a narrow
>> terminal).
>>
>> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
>> ---
>> line-log.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/line-log.c b/line-log.c
>> index a490efe..e725248 100644
>> --- a/line-log.c
>> +++ b/line-log.c
>> @@ -575,7 +575,8 @@ parse_lines(struct commit *commit, const char *prefix, struct string_list *args)
>>
>> name_part = skip_range_arg(item->string);
>> if (!name_part || *name_part != ':' || !name_part[1])
>> - die("-L argument '%s' not of the form start,end:file",
>> + die("invalid -L argument '%s'.\n"
>> + "It should be of the form start,end:file or :regex:file.",
>> item->string);
>> range_part = xstrndup(item->string, name_part - item->string);
>> name_part++;
>
> You forgot to update tests to match their expectations? 4211.20
> wants to see 'argument.*not of the form', it seems.
An alternate wording might be:
"-L argument not 'start,end:file' or ':regex:file': %s"
This can still wrap onscreen but the important information (the two
forms accepted by -L) is at the head of the error message, and the
variable (user-supplied) bit is at the end after the colon, in typical
error message form.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] log -L: improve error message on malformed argument
2015-04-16 14:43 [PATCH] log -L: improve error message on malformed argument Matthieu Moy
2015-04-16 20:59 ` Junio C Hamano
2015-04-16 21:45 ` Junio C Hamano
@ 2015-04-17 20:38 ` Junio C Hamano
2015-04-19 17:29 ` [PATCH] Documentation: change -L:<regex> to -L:<funcname> Matthieu Moy
2 siblings, 1 reply; 7+ messages in thread
From: Junio C Hamano @ 2015-04-17 20:38 UTC (permalink / raw)
To: Matthieu Moy; +Cc: git, Eric Sunshine
Matthieu Moy <Matthieu.Moy@imag.fr> writes:
> The old message did not mention the :regex:file form.
>
> To avoid overly long lines, split the message into two lines (in case
> item->string is long, it will be the only part truncated in a narrow
> terminal).
>
> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
> ---
> line-log.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/line-log.c b/line-log.c
> index a490efe..e725248 100644
> --- a/line-log.c
> +++ b/line-log.c
> @@ -575,7 +575,8 @@ parse_lines(struct commit *commit, const char *prefix, struct string_list *args)
>
> name_part = skip_range_arg(item->string);
> if (!name_part || *name_part != ':' || !name_part[1])
> - die("-L argument '%s' not of the form start,end:file",
> + die("invalid -L argument '%s'.\n"
> + "It should be of the form start,end:file or :regex:file.",
> item->string);
> range_part = xstrndup(item->string, name_part - item->string);
> name_part++;
I actually think "start,end:file" is more correct than your updated
text.
By adding :regex:file as a possibility, you are hinting that 'start'
and 'end' are *not* regular expressions but numbers, but
$ git log -L'/^int main/,/^}/:git.c'
is a perfectly fine way to specify start (i.e. the first line that
matches '^int main') and end (i.e. the first line that matches '^}'
after that). Perhaps rephrase it as :funcname:file to avoid giving
false impression to the other one, and use Eric's suggestion on top?
die("-L argument not 'start,end:file' or ':funcname:file': %s",
item->string);
With the matching update to tests, here is what I'll queue on top of
this patch for now, but please send in objections and improvements.
Thanks.
line-log.c | 3 +--
t/t4211-line-log.sh | 8 ++++----
2 files changed, 5 insertions(+), 6 deletions(-)
diff --git a/line-log.c b/line-log.c
index 5d4cb7c..51d9f7c 100644
--- a/line-log.c
+++ b/line-log.c
@@ -543,8 +543,7 @@ parse_lines(struct commit *commit, const char *prefix, struct string_list *args)
name_part = skip_range_arg(item->string);
if (!name_part || *name_part != ':' || !name_part[1])
- die("invalid -L argument '%s'.\n"
- "It should be of the form start,end:file or :regex:file.",
+ die("-L argument not 'start,end:file' or ':funcname:file': %s",
item->string);
range_part = xstrndup(item->string, name_part - item->string);
name_part++;
diff --git a/t/t4211-line-log.sh b/t/t4211-line-log.sh
index 426a828..edd5ed3 100755
--- a/t/t4211-line-log.sh
+++ b/t/t4211-line-log.sh
@@ -40,14 +40,14 @@ canned_test "-L 24,+1:a.c simple" vanishes-early
canned_test "-L '/long f/,/^}/:b.c' move-support" move-support-f
test_bad_opts "-L" "switch.*requires a value"
-test_bad_opts "-L b.c" "argument.*not of the form"
-test_bad_opts "-L 1:" "argument.*not of the form"
+test_bad_opts "-L b.c" "argument not .start,end:file"
+test_bad_opts "-L 1:" "argument not .start,end:file"
test_bad_opts "-L 1:nonexistent" "There is no path"
test_bad_opts "-L 1:simple" "There is no path"
-test_bad_opts "-L '/foo:b.c'" "argument.*not of the form"
+test_bad_opts "-L '/foo:b.c'" "argument not .start,end:file"
test_bad_opts "-L 1000:b.c" "has only.*lines"
test_bad_opts "-L 1,1000:b.c" "has only.*lines"
-test_bad_opts "-L :b.c" "argument.*not of the form"
+test_bad_opts "-L :b.c" "argument not .start,end:file"
test_bad_opts "-L :foo:b.c" "no match"
test_done
--
2.4.0-rc2-183-g70401ab
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH] Documentation: change -L:<regex> to -L:<funcname>
2015-04-17 20:38 ` Junio C Hamano
@ 2015-04-19 17:29 ` Matthieu Moy
2015-04-20 11:31 ` Ramsay Jones
0 siblings, 1 reply; 7+ messages in thread
From: Matthieu Moy @ 2015-04-19 17:29 UTC (permalink / raw)
To: gitster; +Cc: git, sunshine, Matthieu Moy
The old wording was somehow implying that <start> and <end> were not
regular expressions. Also, the common case is to use a plain function
name here so <funcname> makes sense (the fact that it is a regular
expression is documented in line-range-format.txt).
Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
Junio C Hamano <gitster@pobox.com> writes:
> By adding :regex:file as a possibility, you are hinting that 'start'
> and 'end' are *not* regular expressions but numbers, but
>
> $ git log -L'/^int main/,/^}/:git.c'
>
> is a perfectly fine way to specify start (i.e. the first line that
> matches '^int main') and end (i.e. the first line that matches '^}'
> after that).
OK, but the same argument applies to the documentation (where I
cut-and-pasted from actually). So I suggest this patch in addition
(I'd apply it right before the patch on the code).
> false impression to the other one, and use Eric's suggestion on top?
>
> die("-L argument not 'start,end:file' or ':funcname:file': %s",
> item->string);
>
> With the matching update to tests, here is what I'll queue on top of
> this patch for now, but please send in objections and improvements.
Very good.
Let me know if you want me to resend the 2-patch series.
Documentation/blame-options.txt | 2 +-
Documentation/git-log.txt | 2 +-
Documentation/gitk.txt | 4 ++--
Documentation/line-range-format.txt | 11 ++++++-----
4 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt
index b299b59..a09969b 100644
--- a/Documentation/blame-options.txt
+++ b/Documentation/blame-options.txt
@@ -10,7 +10,7 @@
Include additional statistics at the end of blame output.
-L <start>,<end>::
--L :<regex>::
+-L :<funcname>::
Annotate only the given line range. May be specified multiple times.
Overlapping ranges are allowed.
+
diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt
index 18bc716..f0ec283 100644
--- a/Documentation/git-log.txt
+++ b/Documentation/git-log.txt
@@ -62,7 +62,7 @@ produced by `--stat`, etc.
output by allowing them to allocate space in advance.
-L <start>,<end>:<file>::
--L :<regex>:<file>::
+-L :<funcname>:<file>::
Trace the evolution of the line range given by "<start>,<end>"
(or the funcname regex <regex>) within the <file>. You may
not give any pathspec limiters. This is currently limited to
diff --git a/Documentation/gitk.txt b/Documentation/gitk.txt
index 7ae50aa..d3b91ca 100644
--- a/Documentation/gitk.txt
+++ b/Documentation/gitk.txt
@@ -99,10 +99,10 @@ linkgit:git-rev-list[1] for a complete list.
detailed explanation.)
-L<start>,<end>:<file>::
--L:<regex>:<file>::
+-L:<funcname>:<file>::
Trace the evolution of the line range given by "<start>,<end>"
- (or the funcname regex <regex>) within the <file>. You may
+ (or the funcname regex <funcname>) within the <file>. You may
not give any pathspec limiters. This is currently limited to
a walk starting from a single revision, i.e., you may only
give zero or one positive revision arguments.
diff --git a/Documentation/line-range-format.txt b/Documentation/line-range-format.txt
index d7f2603..829676f 100644
--- a/Documentation/line-range-format.txt
+++ b/Documentation/line-range-format.txt
@@ -22,8 +22,9 @@ This is only valid for <end> and will specify a number
of lines before or after the line given by <start>.
+
-If ``:<regex>'' is given in place of <start> and <end>, it denotes the range
-from the first funcname line that matches <regex>, up to the next
-funcname line. ``:<regex>'' searches from the end of the previous `-L` range,
-if any, otherwise from the start of file.
-``^:<regex>'' searches from the start of file.
+If ``:<funcname>'' is given in place of <start> and <end>, it is a
+regular expression that denotes the range from the first funcname line
+that matches <funcname>, up to the next funcname line. ``:<funcname>''
+searches from the end of the previous `-L` range, if any, otherwise
+from the start of file. ``^:<funcname>'' searches from the start of
+file.
--
2.4.0.rc1.42.g9642cc6
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] Documentation: change -L:<regex> to -L:<funcname>
2015-04-19 17:29 ` [PATCH] Documentation: change -L:<regex> to -L:<funcname> Matthieu Moy
@ 2015-04-20 11:31 ` Ramsay Jones
0 siblings, 0 replies; 7+ messages in thread
From: Ramsay Jones @ 2015-04-20 11:31 UTC (permalink / raw)
To: Matthieu Moy, gitster; +Cc: git, sunshine
On 19/04/15 18:29, Matthieu Moy wrote:
> The old wording was somehow implying that <start> and <end> were not
> regular expressions. Also, the common case is to use a plain function
> name here so <funcname> makes sense (the fact that it is a regular
> expression is documented in line-range-format.txt).
>
> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
> ---
> Junio C Hamano <gitster@pobox.com> writes:
>
>> By adding :regex:file as a possibility, you are hinting that 'start'
>> and 'end' are *not* regular expressions but numbers, but
>>
>> $ git log -L'/^int main/,/^}/:git.c'
>>
>> is a perfectly fine way to specify start (i.e. the first line that
>> matches '^int main') and end (i.e. the first line that matches '^}'
>> after that).
>
> OK, but the same argument applies to the documentation (where I
> cut-and-pasted from actually). So I suggest this patch in addition
> (I'd apply it right before the patch on the code).
>
>> false impression to the other one, and use Eric's suggestion on top?
>>
>> die("-L argument not 'start,end:file' or ':funcname:file': %s",
>> item->string);
>>
>> With the matching update to tests, here is what I'll queue on top of
>> this patch for now, but please send in objections and improvements.
>
> Very good.
>
> Let me know if you want me to resend the 2-patch series.
>
> Documentation/blame-options.txt | 2 +-
> Documentation/git-log.txt | 2 +-
> Documentation/gitk.txt | 4 ++--
> Documentation/line-range-format.txt | 11 ++++++-----
> 4 files changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt
> index b299b59..a09969b 100644
> --- a/Documentation/blame-options.txt
> +++ b/Documentation/blame-options.txt
> @@ -10,7 +10,7 @@
> Include additional statistics at the end of blame output.
>
> -L <start>,<end>::
> --L :<regex>::
> +-L :<funcname>::
> Annotate only the given line range. May be specified multiple times.
> Overlapping ranges are allowed.
> +
> diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt
> index 18bc716..f0ec283 100644
> --- a/Documentation/git-log.txt
> +++ b/Documentation/git-log.txt
> @@ -62,7 +62,7 @@ produced by `--stat`, etc.
> output by allowing them to allocate space in advance.
>
> -L <start>,<end>:<file>::
> --L :<regex>:<file>::
> +-L :<funcname>:<file>::
> Trace the evolution of the line range given by "<start>,<end>"
> (or the funcname regex <regex>) within the <file>. You may
perhaps this should read the same as the hunk below, namely:
(or the funcname regex <funcname>) ...
[I haven't actually given it any thought, I just noticed the difference ...]
Thanks!
ATB,
Ramsay Jones
> not give any pathspec limiters. This is currently limited to
> diff --git a/Documentation/gitk.txt b/Documentation/gitk.txt
> index 7ae50aa..d3b91ca 100644
> --- a/Documentation/gitk.txt
> +++ b/Documentation/gitk.txt
> @@ -99,10 +99,10 @@ linkgit:git-rev-list[1] for a complete list.
> detailed explanation.)
>
> -L<start>,<end>:<file>::
> --L:<regex>:<file>::
> +-L:<funcname>:<file>::
>
> Trace the evolution of the line range given by "<start>,<end>"
> - (or the funcname regex <regex>) within the <file>. You may
> + (or the funcname regex <funcname>) within the <file>. You may
> not give any pathspec limiters. This is currently limited to
> a walk starting from a single revision, i.e., you may only
> give zero or one positive revision arguments.
> diff --git a/Documentation/line-range-format.txt b/Documentation/line-range-format.txt
> index d7f2603..829676f 100644
> --- a/Documentation/line-range-format.txt
> +++ b/Documentation/line-range-format.txt
> @@ -22,8 +22,9 @@ This is only valid for <end> and will specify a number
> of lines before or after the line given by <start>.
>
> +
> -If ``:<regex>'' is given in place of <start> and <end>, it denotes the range
> -from the first funcname line that matches <regex>, up to the next
> -funcname line. ``:<regex>'' searches from the end of the previous `-L` range,
> -if any, otherwise from the start of file.
> -``^:<regex>'' searches from the start of file.
> +If ``:<funcname>'' is given in place of <start> and <end>, it is a
> +regular expression that denotes the range from the first funcname line
> +that matches <funcname>, up to the next funcname line. ``:<funcname>''
> +searches from the end of the previous `-L` range, if any, otherwise
> +from the start of file. ``^:<funcname>'' searches from the start of
> +file.
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-04-20 11:31 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-04-16 14:43 [PATCH] log -L: improve error message on malformed argument Matthieu Moy
2015-04-16 20:59 ` Junio C Hamano
2015-04-16 21:45 ` Junio C Hamano
2015-04-16 21:49 ` Eric Sunshine
2015-04-17 20:38 ` Junio C Hamano
2015-04-19 17:29 ` [PATCH] Documentation: change -L:<regex> to -L:<funcname> Matthieu Moy
2015-04-20 11:31 ` Ramsay Jones
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.