Re: [PATCH 07/10] bisect--helper: reimplement `bisect_state` & `bisect_head` shell functions in C
Hi,
On Wed, 26 Feb 2020, Miriam Rubio wrote:
Show 14 quoted lines
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index f9b04bee23..fedcad4d9b 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -833,6 +835,74 @@ static int bisect_autostart(struct bisect_terms *terms)
> return BISECT_OK;
> }
>
> +static char *bisect_head(void)
> +{
> + if (is_empty_or_missing_file(git_path_bisect_head()))
> + return "HEAD";
> + else
> + return "BISECT_HEAD";It is relatively common, and makes it easier to read at least to me, to omit the `else` here: the conditional `return` already leaves the function early.
Show 16 quoted lines
> +}
> +
> +static enum bisect_error bisect_state(struct bisect_terms *terms, const char **argv,
> + int argc)
> +{
> + const char *state = argv[0];
> +
> + if (check_and_set_terms(terms, state))
> + return BISECT_FAILED;
> +
> + if (!argc)
> + return error(_("Please call `--bisect-state` with at least one argument"));
> +
> + if (argc == 1 && one_of(state, terms->term_good,
> + terms->term_bad, "skip", NULL)) {
> + const char *bisected_head = xstrdup(bisect_head());Do we really want to `strdup()` it? That would leak memory, no? And I don't think that we need this to be allocated anywhere. It's not like we are trying to modify the characters or that we need to take custody of the string: `bisect_head()` will return a static, immutable string.
But then, it is not very natural for C code to let `bisect_head()` return a string. Why not return the already-parsed object ID? That would also make that function more robust: instead of expecting `BISECT_HEAD` to be stored in the file `BISECT_HEAD` in the `.git/` directory, you could simply call `get_oid("BISECT_HEAD", &oid)` and if that fails, fall back to do the same with `HEAD`. That way, you tap into the refs machinery and the bisect code would be less dependent on implementation details.
Show 10 quoted lines
> + const char *hex[1];
> + struct object_id oid;
> +
> + if (get_oid(bisected_head, &oid))
> + return error(_("Bad rev input: %s"), bisected_head);
> + if (bisect_write(state, oid_to_hex(&oid), terms, 0))
> + return BISECT_FAILED;
> +
> + *hex = xstrdup(oid_to_hex(&oid));
> + check_expected_revs(hex, 1);Hmm. Do we expect `check_expected_revs()` to modify what is passed into it? If not, we could probably simplify this a lot (and avoid another memory leak) by something like this:
const char *hex;
[...]
hex = oid_to_hex(&oid);
check_expected_revs(&hex, 1);
However, taking a step back, I wonder whether it makes sense for `check_expected_revs()` to accept `const char **revs` instead of `struct object_id *oids`.
Looking at the definition of `check_expected_revs()`, I would think that it would be better to pass object IDs in, rather than strings:
static int is_expected_rev(const char *expected_hex)
{
struct strbuf actual_hex = STRBUF_INIT;
int res = 0;
if (strbuf_read_file(&actual_hex, git_path_bisect_expected_rev(), 0) >= 40) {
strbuf_trim(&actual_hex);
res = !strcmp(actual_hex.buf, expected_hex);
}
strbuf_release(&actual_hex);
return res;
}static void check_expected_revs(const char **revs, int rev_nr)
{
int i; for (i = 0; i < rev_nr; i++) {
if (!is_expected_rev(revs[i])) {
unlink_or_warn(git_path_bisect_ancestors_ok());
unlink_or_warn(git_path_bisect_expected_rev());
}
}
}There are a couple of observation that immediately come to my mind:
- Reading the bisect_expected_rev file for each rev, only to immediately
release it before the next iteration, is wasteful
- We can break out of the loop immediately once `is_expected_rev()`
returns 0 because we are then unlinking the very file that
`is_expected_rev()` checks against.
- As I suspected above, a much cleaner interface would use `struct
object_id **revs`.
- Keeping the code of `is_expected_rev()` separate from
`check_expected_revs()` does not make the code more readable for me, but
actually less readable instead.
- Why are we hard-coding the 40? At this time and age, we've _got_ to use
`the_hash_algo->hexsz` instead.
In other words, I would do something like this instead:
static void check_expected_revs(struct object_id **revs, int rev_nr)
{
struct object_id expected;
struct strbuf buf = STRBUF_INIT;
int i; if (!rev_nr)
return;
if (strbuf_read_file(&buf, git_path_bisect_expected_rev(), 0)
< the_hash_algo->hexsz ||
get_oid_hex(buf.buf, &expected) < 0)
return; /* Ignore invalid file contents */
for (i = 0; i < rev_nr; i++)
if (!oideq(revs[i], &expected)) {
... unlink ...
return;
}
}However, taking _yet_ another step back, it seems a bit silly to compare many revs (that need to be provided as 40-digit hex strings) to the contents of a file that we expect to contain exactly one 40-digit hex string. Surely, we do not want to take an arbitrary number of revisions, then expect all of them to be identical to the _same_ rev, and otherwise delete that file from where that rev came from?
We could even skip reading that file if two or more revisions were passed to `check_expected_revs()` unless _all_ of them are identical!
So let's look at the actual caller of this thing:
bisect_state() {
bisect_autostart
state=$1
git bisect--helper --check-and-set-terms $state $TERM_GOOD $TERM_BAD || exit
get_terms
case "$#,$state" in
0,*)
die "Please call 'bisect_state' with at least one argument." ;;
1,"$TERM_BAD"|1,"$TERM_GOOD"|1,skip)
bisected_head=$(bisect_head)
rev=$(git rev-parse --verify "$bisected_head") ||
die "$(eval_gettext "Bad rev input: \$bisected_head")"
git bisect--helper --bisect-write "$state" "$rev" "$TERM_GOOD" "$TERM_BAD" || exit
git bisect--helper --check-expected-revs "$rev" ;;
2,"$TERM_BAD"|*,"$TERM_GOOD"|*,skip)
shift
hash_list=''
for rev in "$@"
do
sha=$(git rev-parse --verify "$rev^{commit}") ||
die "$(eval_gettext "Bad rev input: \$rev")"
hash_list="$hash_list $sha"
done
for rev in $hash_list
do
git bisect--helper --bisect-write "$state" "$rev" "$TERM_GOOD" "$TERM_BAD" || exit
done
git bisect--helper --check-expected-revs $hash_list ;;
*,"$TERM_BAD")
die "$(eval_gettext "'git bisect \$TERM_BAD' can take only one argument.")" ;;
*)
usage ;;
esac
bisect_auto_next
}So there are actually two callers: the first one in the 1,* arm, the second one in the 2,* arm. The first one is easy: it only passes a single rev to `git bisect--helper --check-expected-revs`.
The second caller indeed can pass multiple arguments to `check_expected_revs()`, but only if the first argument was not `bad`.
But this whole shell function is super ugly, and I do not believe that we're served well by translating it verbatim to even super uglier C code.
I propose to instead rewrite it substantially, in the following way:
- Handle the `argc == 0` case first thing: in this case, die, complaining
that we need at least one argument.
(You already did that. Although `state` was already assigned to
`argv[0]` at that stage, and I think we don't really want that, just in
case that _one_ call chain ends up using `NULL, 0` for `argv, argc`.)
- Next, assign `state = argv[0]` and verify that it is one of `good`,
`bad` or `skip`, otherwise erroring out.
- At this stage, we should do the equivalent of a `shift`: `argv++;
argc--;`
- Now, we probably need to special-case the `bad` case and verify that
`argc <= 1`, otherwise error out.
Side note: I do _not_ understand this restriction. Why shouldn't it be
valid to pass several revisions to `git bisect bad <revisions>...`?
- At this point, if `argc == 0`, we should parse `bisect_head()` into a
`struct object_id` and use that as `revs`, setting `argc = 0`.
Otherwise, if `argc > 0`, we should populate a `struct object_array` and
use that as `revs`.
- And finally, we should iterate over these revs and call `bisect_write()`
with them, and in the same loop we can also take care of the expected
rev. It does not make sense to keep those separate from one another
(i.e. to have _two_ loops).
That would be a much more natural, and readable, flow, at least to me.
Show 11 quoted lines
> + }
> +
> + if ((argc == 2 && !strcmp(state, terms->term_bad)) ||
> + one_of(state, terms->term_good, "skip", NULL)) {
> + int i;
> + struct string_list hex = STRING_LIST_INIT_DUP;
> +
> + for (i = 1; i < argc; i++) {
> + struct object_id oid;
> +
> + if (get_oid(argv[i], &oid)) {The original does `git rev-parse --verify "$rev^{commit}"` here, so I think we really want `get_oid_commit()` here, not just `get_oid()`.
> + string_list_clear(&hex, 0);
> + return error(_("Bad rev input: %s"), argv[i]);
> + }
> + string_list_append(&hex, oid_to_hex(&oid));Converting the parsed object ID to hex and storing _that_ as a string and throwing away the parsed object ID looks not very much like C, but much more like shell script. Let's not do that. Let's store the parsed data in a proper `struct object_array`, and only resort to `oid_to_hex()` when we _have_ to.
Show 15 quoted lines
> + }
> + for (i = 0; i < hex.nr; i++) {
> + const char **hex_string = (const char **) &hex.items[i].string;
> + if (bisect_write(state, *hex_string, terms, 0)) {
> + string_list_clear(&hex, 0);
> + return BISECT_FAILED;
> + }
> + check_expected_revs(hex_string, 1);
> + string_list_clear(&hex, 0);
> + return bisect_auto_next(terms, NULL);
> + }
> +
> + if (!strcmp(state, terms->term_bad))
> + return error(_("'git bisect %s' can take only one argument."),
> + terms->term_bad);That is an awful late time in the function for a reviewer to read about a condition that should be an early error.
Show 15 quoted lines
> +
> + return BISECT_FAILED;
> +}
> +
> int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> {
> enum {
> [...]
> @@ -944,6 +1017,13 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> set_terms(&terms, "bad", "good");
> res = bisect_autostart(&terms);
> break;
> + case BISECT_STATE:
> + if (!argc)
> + return error(_("--bisect-state requires at least one revision"));Wait, we _already_ test for that here? Then we do not need the very same check in `bisect_state()`.
I'd rather have it in `bisect_state()`.
Besides, we're in a `cmd_*()` function, so we cannot return `error()` because that translates to `-1` and `cmd_*()` functions need to return exit codes, i.e. numbers between 0 and 127.
Show 6 quoted lines
> + set_terms(&terms, "bad", "good");
> + get_terms(&terms);
> + res = bisect_state(&terms, argv, argc);
> + break;
> default:
> return error("BUG: unknown subcommand '%d'", cmdmode);Granted, this incorrect `return error()` in a `cmd_*()` function was there before your patch. It is just as incorrect, though.
Show 13 quoted lines
> }
> diff --git a/git-bisect.sh b/git-bisect.sh
> index 049ffacdff..7f10187055 100755
> --- a/git-bisect.sh
> +++ b/git-bisect.sh
> [...]
> @@ -185,8 +139,7 @@ exit code \$res from '\$command' is < 0 or >= 128" >&2
> state="$TERM_GOOD"
> fi
>
> - # We have to use a subshell because "bisect_state" can exit.
> - ( bisect_state $state >"$GIT_DIR/BISECT_RUN" )
> + ( git bisect--helper --bisect-state $state >"$GIT_DIR/BISECT_RUN" )
This subshell looks pointless. Before, we wanted to be able to catch the `die` in `bisect_state`. But since the `bisect--helper` cannot cause the _shell script_ to exit, we can just drop the subshell here.
Phew, that was quite a lot, and the review of this patch also took two hours. I'll have to stop here, once again, and hope that I will find time soon to continue the review.
I left you quite a bit of work here, but I hope that all of my suggestions are clear enough to act on. If not, please holler.
Ciao, Dscho
Show 25 quoted lines
> res=$?
>
> cat "$GIT_DIR/BISECT_RUN"
> @@ -201,7 +154,7 @@ exit code \$res from '\$command' is < 0 or >= 128" >&2
> if [ $res -ne 0 ]
> then
> eval_gettextln "bisect run failed:
> -'bisect_state \$state' exited with error code \$res" >&2
> +'git bisect--helper --bisect-state \$state' exited with error code \$res" >&2
> exit $res
> fi
>
> @@ -242,7 +195,7 @@ case "$#" in
> start)
> git bisect--helper --bisect-start "$@" ;;
> bad|good|new|old|"$TERM_BAD"|"$TERM_GOOD")
> - bisect_state "$cmd" "$@" ;;
> + git bisect--helper --bisect-state "$cmd" "$@" ;;
> skip)
> bisect_skip "$@" ;;
> next)
> --
> 2.25.0
>
>
Miriam Rubio
