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

On Sun, Nov 24, 2019 at 12:54 AM Junio C Hamano [off-list ref] 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.