[1/3] parse_insn_line(): improve error message when parsing failed
diff mbox series

Message ID 2ae2e435b0ef6888e72defc7abee1909b29aa914.1579209506.git.gitgitgadget@gmail.com
State New
Headers show
Series
  • Re-fix rebase -i with SHA-1 collisions
Related show

Commit Message

Markus Klein via GitGitGadget Jan. 16, 2020, 9:18 p.m. UTC
From: Johannes Schindelin <johannes.schindelin@gmx.de>

In the case that a `get_oid()` call failed, we showed some rather bogus
part of the line instead of the precise string we sent to said function.
That makes it rather hard for users to understand what is going wrong,
so let's fix that.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Junio C Hamano Jan. 17, 2020, 6 p.m. UTC | #1
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> In the case that a `get_oid()` call failed, we showed some rather bogus
> part of the line instead of the precise string we sent to said function.
> That makes it rather hard for users to understand what is going wrong,
> so let's fix that.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  sequencer.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index b9dbf1adb0..7c30dad59c 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2118,6 +2118,8 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,
>  	saved = *end_of_object_name;
>  	*end_of_object_name = '\0';
>  	status = get_oid(bol, &commit_oid);
> +	if (status < 0)
> +		error(_("could not parse '%s'"), bol); /* return later */
>  	*end_of_object_name = saved;

OK, so after making sure the line begins a command that takes an
object name, we first NUL terminated the token after the command
but it turns out the string is not a valid oid.  We show the part
we thought was the object name before reverting the NUL termination.

Makes sense.  And then later...

>  	bol = end_of_object_name + strspn(end_of_object_name, " \t");
> @@ -2125,11 +2127,10 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,
>  	item->arg_len = (int)(eol - bol);
>  
>  	if (status < 0)
> -		return error(_("could not parse '%.*s'"),
> -			     (int)(end_of_object_name - bol), bol);
> +		return status;

...this is the *only* code that uses the status that was assigned to
the return value of get_oid().  

Because the implementation of this function (mis)uses "bol", which
stands for "beginning of line", as a moving pointer of "beginning of
the token we are looking at", the value of "bol" at this point of
the control in the original code is not where the "bol" pointer used
to be when end-of-object-name was computed (if it were, the '%.*s'
argument would have been correct) and shows a token after where the
object name would have been, which may help the user identify the
line but does not directly show what token we had trouble with
parsing.

And the updated code will avoid the issue by using the "bol" pointer
when it is still pointing at the right place.


>  	item->commit = lookup_commit_reference(r, &commit_oid);
> -	return !item->commit;
> +	return item->commit ? 0 : -1;

This changes the polarity of the error exit from positive 1 to
negative 1.  The only caller of this function takes anything
non-zero as a failure so this would not cause behaviour change, but
returning negative is more in line with the practice so it is an
improvement (it is unrelated to the theme of this patch and is not
explained, though).

OK.

Patch
diff mbox series

diff --git a/sequencer.c b/sequencer.c
index b9dbf1adb0..7c30dad59c 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2118,6 +2118,8 @@  static int parse_insn_line(struct repository *r, struct todo_item *item,
 	saved = *end_of_object_name;
 	*end_of_object_name = '\0';
 	status = get_oid(bol, &commit_oid);
+	if (status < 0)
+		error(_("could not parse '%s'"), bol); /* return later */
 	*end_of_object_name = saved;
 
 	bol = end_of_object_name + strspn(end_of_object_name, " \t");
@@ -2125,11 +2127,10 @@  static int parse_insn_line(struct repository *r, struct todo_item *item,
 	item->arg_len = (int)(eol - bol);
 
 	if (status < 0)
-		return error(_("could not parse '%.*s'"),
-			     (int)(end_of_object_name - bol), bol);
+		return status;
 
 	item->commit = lookup_commit_reference(r, &commit_oid);
-	return !item->commit;
+	return item->commit ? 0 : -1;
 }
 
 int sequencer_get_last_command(struct repository *r, enum replay_action *action)