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

[Nika Layzell <nika@thelayzells.com>]

On Sun, Nov 24, 2019 at 12:54 AM Junio C Hamano <gitster@pobox.com> wrote:
> 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.

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