Re: [PATCH 1/1] reset: parse rev as tree-ish in patch mode

From: Nika Layzell <nika@thelayzells.com>
Date: Nov 24, 2019, 18:31 UTC
Message-ID: <CACwGqKgPYMEHhfOnAEuCA3sNiuV0Wk_JQwU+c=vF7Wkhf=V9iQ@mail.gmail.com>
In-Reply-To: <xmqq4kyt250e.fsf@gitster-ct.c.googlers.com>

On Sun, Nov 24, 2019 at 12:54 AM Junio C Hamano <gitster@pobox.com> wrote:

Show 16 quoted lines

> I notice that under the patch mode after this part of the code, we
> do not even use the oid computed in these pieces of code at all.
> Presumably, run_add_interactive() function would also be sanity
> checking the incoming "rev" and giving an appropriate error message
> when it is not of expected type.
>
> Which means that perhaps a cleaner fix may be
>
> -       if (unborn) {
> +       if (patch_mode) {
> +               /* do not compute or check &oid we will not use */
> +               ;
> +       } else if (unborn) {
>                 ...
>
> right?

I tried to make this change, however it has unfortunate side-effects. The "git-add--interactive" script does produce an error if it is not of the expected type, but it exits with a successful "0" status.

$ $(git --exec-path)/git-add--interactive --patch=reset HEAD~:README.md -- error: Bad tree object HEAD~:README.md No changes. $ echo $? 0

Given that the add--interactive script is undergoing a C rewrite, I am inclined to continue validating the argument in reset.c to avoid changes to the perl script.

Show 5 quoted lines

> This only tests a positive (i.e. "see what my change now allows you
> to do") without checking a negative (i.e. "I started allowing a
> tree, but I didn't inadvertently started allowing a blob or an
> object that does not exist"), which is an anti-pattern.  Please have
> another test to ensure that the parser fails when it should, too.

I have added negative tests for the next version of the patch.

Thanks, Nika

PS. Apologies for the duplicate email. I accidentally sent a non-plain-text version previously which was rejected from the mailing list.