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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 26, 2016, 16:29 UTC
Message-ID: <xmqqshtrijdn.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CAFZEwPPfXvLUtcBR6cYAP2dT8FAePFPjDSnVm8BhpLN9cfR1uw@mail.gmail.com>

Pranit Bauva <pranit.bauva@gmail.com> writes:

Show 5 quoted lines

>> 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.

My bad.  I just compared your bisect_reset() implementation that had

    if (no specific commit) {
    	reset to the branch
    } else {
	reset to the commit
    }

with the original

    case $# in
    0)	reset to the branch ;;
    1)  reset to the commit ;;
    *)  give usage and die ;;
    esac

and took the difference and reacted "ah, excess parameters are not diagnosed in this function".

Your caller does complain about excess parameters without giving usage, and that is what I missed.

I am not sure if you intended to change the behaviour in this case to avoid giving the usage string; I tend to think it is a good change, but I didn't see it mentioned in the proposed commit log, which also contributed to my not noticing the test in the caller.