Alban Gruin <alban.gruin@xxxxxxxxx> writes:
> - if (command == SKIP_UNNECESSARY_PICKS && argc == 1)
> - return !!skip_unnecessary_picks();
> + if (command == SKIP_UNNECESSARY_PICKS && argc == 1) {
> + ret = !!skip_unnecessary_picks(&oid);
> + if (!ret && oid)
> + puts(oid);
Think why you wrote !! there; in other words, avoid getting into a
habit of blindly copying and pasting existing code without thinking.
The original is synthesizing an integer value that is to be used as
the final exit status from a process, which has to be a non negative
integer, out of a helper function, whose error return may be
nagative [*1*], and it knows that the caller cares only about one
bit "did we or did we not have an error?", so it turns any non-zero
return value into 1. That is why it uses !!.
Side note *1* The normal convention for helper functions is
to signal an error with negative return value, and success
with 0. There may be different negative values used to
signal different error conditions to the caller, depending
on how well the API is designed.
But realize that !! is an operation that loses information, so in
general you would want to delay its application til when you need
it. In this particular case, you do not care what kind of error
you got (or you didn't), so you can just say
ret = skip_unnecessary_picks(&oid);
if (!ret && oid)
puts(oid); /* happy */
without applying !! before receiving the return value from the
helper in "ret".
> + return ret;
And ensure that you return only 0 or 1 by applying !! here instead.
> diff --git a/sequencer.c b/sequencer.c
> index 2b6ddc6cf..676ac1320 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -4392,7 +4392,7 @@ static int rewrite_file(const char *path, const char *buf, size_t len)
> }
>
> /* skip picking commits whose parents are unchanged */
> -int skip_unnecessary_picks(void)
> +int skip_unnecessary_picks(const char **output_oid)
> {
> const char *todo_file = rebase_path_todo();
> struct strbuf buf = STRBUF_INIT;
> @@ -4467,7 +4467,7 @@ int skip_unnecessary_picks(void)
> }
>
> todo_list_release(&todo_list);
> - printf("%s\n", oid_to_hex(oid));
> + *output_oid = oid_to_hex(oid);
The return value of oid_to_hex() is not meant to be long-lived.
Consider writing into caller supplied buffer with oid_to_hex_r()
or something like that.
