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