Re: [PATCH v14 07/27] bisect--helper: `bisect_reset` shell function in C

Pranit Bauva [off-list ref] writes:

+static int bisect_reset(const char *commit)
+{
+	struct strbuf branch = STRBUF_INIT;
+
+	if (!commit) {
+		if (strbuf_read_file(&branch, git_path_bisect_start(), 0) < 1) {

Hmm, tricky but correct to do the "< 1" comparison.  If the file
does not exist, you'd get -1; if it fails to read it, you'd get -1;
if it turns out to be empty, you'd get 0.

+			printf("We are not bisecting.\n");
+			return 0;
+		}
+		strbuf_rtrim(&branch);
+	} else {
+		struct object_id oid;
+		if (get_oid(commit, &oid))
+			return error(_("'%s' is not a valid commit"), commit);

The original is

	rev-parse --quiet --verify "$1^{commit}"

Unless the caller of this function already appended "^{commit}" to
whatever the user gave "bisect--helper bisect-reset", this
conversion is not equivalent.  If you said

    git bisect reset HEAD:

get_oid() would tell you that the top-level tree object of the
current commit exists in the object store, but the original is
meant to catch "That's not a commit, it's a tree!" before attempting
to run "git checkout" on it.

I think get_sha1_committish() is what you want to use here.

+		strbuf_addstr(&branch, commit);
+	}

Also this version fails to catch "bisect reset a b c" as an error, I
suspect.

Hey Junio,

On Thu, Aug 25, 2016 at 2:42 AM, Junio C Hamano [off-list ref] wrote:

Pranit Bauva [off-list ref] writes:

quoted

+static int bisect_reset(const char *commit)
+{
+     struct strbuf branch = STRBUF_INIT;
+
+     if (!commit) {
+             if (strbuf_read_file(&branch, git_path_bisect_start(), 0) < 1) {

Hmm, tricky but correct to do the "< 1" comparison.  If the file
does not exist, you'd get -1; if it fails to read it, you'd get -1;
if it turns out to be empty, you'd get 0.

quoted

+                     printf("We are not bisecting.\n");
+                     return 0;
+             }
+             strbuf_rtrim(&branch);
+     } else {
+             struct object_id oid;
+             if (get_oid(commit, &oid))
+                     return error(_("'%s' is not a valid commit"), commit);

The original is

        rev-parse --quiet --verify "$1^{commit}"

Unless the caller of this function already appended "^{commit}" to
whatever the user gave "bisect--helper bisect-reset", this
conversion is not equivalent.  If you said

    git bisect reset HEAD:

get_oid() would tell you that the top-level tree object of the
current commit exists in the object store, but the original is
meant to catch "That's not a commit, it's a tree!" before attempting
to run "git checkout" on it.

I think get_sha1_committish() is what you want to use here.

quoted

+             strbuf_addstr(&branch, commit);
+     }

Ya! get_sha1_committish() would be better. Thanks!

Also this version fails to catch "bisect reset a b c" as an error, I
suspect.

It didn't when I tried it right now. Could you please elaborate on why
you think it can fail? There might be a thing which I haven't tested.

Regards,
Pranit Bauva