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

Commit Message

Linus Arver via GitGitGadget Nov. 23, 2019, 7:55 p.m. UTC

From: Nika Layzell <[e-mail]>

Relaxes the commit requirement for the rev argument when running
git-reset in patch mode without pathspec.

The revision argument to git-reset is parsed as either a commit or
tree-ish depending on mode. Previously, if no pathspec was provided,
the rev argument was parsed as a commit unconditionally.

Signed-off-by: Nika Layzell <[e-mail]>
---
 builtin/reset.c        | 2 +-
 t/t7105-reset-patch.sh | 7 +++++++
 2 files changed, 8 insertions(+), 1 deletion(-)

Comments

Junio C Hamano Nov. 24, 2019, 5:54 a.m. UTC | #1

"Nika Layzell via GitGitGadget" <[e-mail]> writes:

> From: Nika Layzell <[e-mail]>
>
> Relaxes the commit requirement for the rev argument when running
> git-reset in patch mode without pathspec.
>
> The revision argument to git-reset is parsed as either a commit or
> tree-ish depending on mode. Previously, if no pathspec was provided,
> the rev argument was parsed as a commit unconditionally.

Swap the order of two paragraphs, i.e. explain what happens and what
you perceive as a problem in the current system in the present tense
(and do not say "Previously" if you are talking about the state of
the system without your change), and then propose what to change in
the imperative mood as if you are giving an order to the codebase to
"be like so".  Perhaps like this.

    Since 2f328c3d ("reset $sha1 $pathspec: require $sha1 only to be
    treeish", 2013-01-14), we allowed "git reset $object -- $path"
    to reset individual paths that match the pathspec to take the
    blob from a tree object, not necessarily a commit, while the
    form to reset the tip of the current branch to some other commit
    still must be given a commit.

    Sometimes, however, being able to give a tree object to "git
    reset -p $obj" when using the patch mode is useful FOR SUCH AND
    SUCH REASONS.

    Loosen the condition that requires a commit object to take a
    tree object under the patch mode.

>
> Signed-off-by: Nika Layzell <[e-mail]>
> ---
>  builtin/reset.c        | 2 +-
>  t/t7105-reset-patch.sh | 7 +++++++
>  2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/reset.c b/builtin/reset.c
> index fdd572168b..5cbfb21ec4 100644
> --- a/builtin/reset.c
> +++ b/builtin/reset.c
> @@ -320,7 +320,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
>  	if (unborn) {
>  		/* reset on unborn branch: treat as reset to empty tree */
>  		oidcpy(&oid, the_hash_algo->empty_tree);
> -	} else if (!pathspec.nr) {
> +	} else if (!pathspec.nr && !patch_mode) {
>  		struct commit *commit;
>  		if (get_oid_committish(rev, &oid))
>  			die(_("Failed to resolve '%s' as a valid revision."), rev);

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?

> diff --git a/t/t7105-reset-patch.sh b/t/t7105-reset-patch.sh
> index bd10a96727..2a6ecf515b 100755
> --- a/t/t7105-reset-patch.sh
> +++ b/t/t7105-reset-patch.sh
> @@ -38,6 +38,13 @@ test_expect_success PERL 'git reset -p HEAD^' '
>  	test_i18ngrep "Apply" output
>  '
>  
> +test_expect_success PERL 'git reset -p HEAD^^{tree}' '
> +	test_write_lines n y | git reset -p HEAD^^{tree} >output &&
> +	verify_state dir/foo work parent &&
> +	verify_saved_state bar &&
> +	test_i18ngrep "Apply" output
> +'

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.

Thanks.

Nika Layzell Nov. 24, 2019, 6:31 p.m. UTC | #2

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

Junio C Hamano Nov. 25, 2019, 1:58 a.m. UTC | #3

Nika Layzell <[e-mail]> writes:

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

Thanks for being diligent.  I love it when I see contributors do not
believe blindly what I suggest out of hunch and instead verify it
themselves.

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

Thanks.  That is a very reasonable decision.

Patch

diff --git a/builtin/reset.c b/builtin/reset.c
index fdd572168b..5cbfb21ec4 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -320,7 +320,7 @@  int cmd_reset(int argc, const char **argv, const char *prefix)
 	if (unborn) {
 		/* reset on unborn branch: treat as reset to empty tree */
 		oidcpy(&oid, the_hash_algo->empty_tree);
-	} else if (!pathspec.nr) {
+	} else if (!pathspec.nr && !patch_mode) {
 		struct commit *commit;
 		if (get_oid_committish(rev, &oid))
 			die(_("Failed to resolve '%s' as a valid revision."), rev);
diff --git a/t/t7105-reset-patch.sh b/t/t7105-reset-patch.sh
index bd10a96727..2a6ecf515b 100755
--- a/t/t7105-reset-patch.sh
+++ b/t/t7105-reset-patch.sh
@@ -38,6 +38,13 @@  test_expect_success PERL 'git reset -p HEAD^' '
 	test_i18ngrep "Apply" output
 '
 
+test_expect_success PERL 'git reset -p HEAD^^{tree}' '
+	test_write_lines n y | git reset -p HEAD^^{tree} >output &&
+	verify_state dir/foo work parent &&
+	verify_saved_state bar &&
+	test_i18ngrep "Apply" output
+'
+
 # The idea in the rest is that bar sorts first, so we always say 'y'
 # first and if the path limiter fails it'll apply to bar instead of
 # dir/foo.  There's always an extra 'n' to reject edits to dir/foo in