Re: [PATCH v14 07/27] bisect--helper: `bisect_reset` shell function in C
- From
Pranit Bauva <pranit.bauva@gmail.com>
- Date
- Aug 26, 2016, 13:46 UTC
- Message-ID
- <CAFZEwPPfXvLUtcBR6cYAP2dT8FAePFPjDSnVm8BhpLN9cfR1uw@mail.gmail.com>
- In-Reply-To
- <xmqq4m69q3b2.fsf@gitster.mtv.corp.google.com>
Hey Junio,
On Thu, Aug 25, 2016 at 2:42 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 43 quoted lines
>
> Pranit Bauva <pranit.bauva@gmail.com> 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);
> > + }
>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