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

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

[Nika Layzell via GitGitGadget]

This allows passing a tree-ish git reset -p without specifying a pathspec.
Requiring a commit in this situation appears to be an oversight, and support
for a tree-ish is documented by git-reset's manpage. (
https://github.com/git/git/blob/d9f6f3b6195a0ca35642561e530798ad1469bd41/Documentation/git-reset.txt#L12
)

An alternative implementation of this change would move the if (patch_mode)
{ ... return; } check before the rev parsing logic, offloading validation of
the rev argument when in patch mode to the git-add--interactive logic. This
would be possible as the parsed oid is not passed to git-add--interactive. (
https://github.com/git/git/blob/d9f6f3b6195a0ca35642561e530798ad1469bd41/builtin/reset.c#L341-L346
)

Nika Layzell (1):
  reset: parse rev as tree-ish in patch mode

 builtin/reset.c        | 2 +-
 t/t7105-reset-patch.sh | 7 +++++++
 2 files changed, 8 insertions(+), 1 deletion(-)


base-commit: 5fa0f5238b0cd46cfe7f6fa76c3f526ea98148d9
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-474%2Fmystor%2Freset-interactive-treeish-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-474/mystor/reset-interactive-treeish-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/474
-- 
gitgitgadget

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

[Nika Layzell via GitGitGadget]

From: Nika Layzell <nika@thelayzells.com>

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 <nika@thelayzells.com>
---
 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);
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
-- 
gitgitgadget

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

[Junio C Hamano]

"Nika Layzell via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Nika Layzell <nika@thelayzells.com>
>
> 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 <nika@thelayzells.com>
> ---
>  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.

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

[Nika Layzell]

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.

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

[Junio C Hamano]

Nika Layzell <nika@thelayzells.com> 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 v2 0/1] reset: parse rev as tree-ish in patch mode

[Nika Layzell via GitGitGadget]

This allows passing a tree-ish git reset -p without specifying a pathspec.
Requiring a commit in this situation appears to be an oversight, and support
for a tree-ish is documented by git-reset's manpage. (
https://github.com/git/git/blob/d9f6f3b6195a0ca35642561e530798ad1469bd41/Documentation/git-reset.txt#L12
)

An alternative implementation of this change would move the if (patch_mode)
{ ... return; } check before the rev parsing logic, offloading validation of
the rev argument when in patch mode to the git-add--interactive logic. This
would be possible as the parsed oid is not passed to git-add--interactive. (
https://github.com/git/git/blob/d9f6f3b6195a0ca35642561e530798ad1469bd41/builtin/reset.c#L341-L346
)

Nika Layzell (1):
  reset: parse rev as tree-ish in patch mode

 builtin/reset.c        |  2 +-
 t/t7105-reset-patch.sh | 21 +++++++++++++++++++++
 2 files changed, 22 insertions(+), 1 deletion(-)


base-commit: 5fa0f5238b0cd46cfe7f6fa76c3f526ea98148d9
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-474%2Fmystor%2Freset-interactive-treeish-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-474/mystor/reset-interactive-treeish-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/474

Range-diff vs v1:

 1:  338c2777f7 ! 1:  a608dc8368 reset: parse rev as tree-ish in patch mode
     @@ -2,12 +2,22 @@
      
          reset: parse rev as tree-ish in patch mode
      
     -    Relaxes the commit requirement for the rev argument when running
     -    git-reset in patch mode without pathspec.
     +    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.
      
     -    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.
     +    Like resetting with paths, "git reset --patch" does not update HEAD, and
     +    need not require a commit. The path-filtered form, "git reset --patch
     +    $object -- $pathspec", has accepted a tree-ish since 2f328c3d.
     +
     +    "git reset --patch" is documented as accepting a <tree-ish> since
     +    bf44142f ("reset: update documentation to require only tree-ish with
     +    paths", 2013-01-16). Documentation changes are not required.
     +
     +    Loosen the restriction that requires a commit for the unfiltered "git
     +    reset --patch $object".
      
          Signed-off-by: Nika Layzell <nika@thelayzells.com>
      
     @@ -37,6 +47,20 @@
      +	verify_saved_state bar &&
      +	test_i18ngrep "Apply" output
      +'
     ++
     ++test_expect_success PERL 'git reset -p HEAD^:dir/foo (blob fails)' '
     ++	set_and_save_state dir/foo work work &&
     ++	test_must_fail git reset -p HEAD^:dir/foo &&
     ++	verify_saved_state dir/foo &&
     ++	verify_saved_state bar
     ++'
     ++
     ++test_expect_success PERL 'git reset -p aaaaaaaa (unknown fails)' '
     ++	set_and_save_state dir/foo work work &&
     ++	test_must_fail git reset -p aaaaaaaa &&
     ++	verify_saved_state dir/foo &&
     ++	verify_saved_state bar
     ++'
      +
       # 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

-- 
gitgitgadget

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

[Nika Layzell via GitGitGadget]

From: Nika Layzell <nika@thelayzells.com>

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.

Like resetting with paths, "git reset --patch" does not update HEAD, and
need not require a commit. The path-filtered form, "git reset --patch
$object -- $pathspec", has accepted a tree-ish since 2f328c3d.

"git reset --patch" is documented as accepting a <tree-ish> since
bf44142f ("reset: update documentation to require only tree-ish with
paths", 2013-01-16). Documentation changes are not required.

Loosen the restriction that requires a commit for the unfiltered "git
reset --patch $object".

Signed-off-by: Nika Layzell <nika@thelayzells.com>
---
 builtin/reset.c        |  2 +-
 t/t7105-reset-patch.sh | 21 +++++++++++++++++++++
 2 files changed, 22 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);
diff --git a/t/t7105-reset-patch.sh b/t/t7105-reset-patch.sh
index bd10a96727..fc2a6cf5c7 100755
--- a/t/t7105-reset-patch.sh
+++ b/t/t7105-reset-patch.sh
@@ -38,6 +38,27 @@ 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
+'
+
+test_expect_success PERL 'git reset -p HEAD^:dir/foo (blob fails)' '
+	set_and_save_state dir/foo work work &&
+	test_must_fail git reset -p HEAD^:dir/foo &&
+	verify_saved_state dir/foo &&
+	verify_saved_state bar
+'
+
+test_expect_success PERL 'git reset -p aaaaaaaa (unknown fails)' '
+	set_and_save_state dir/foo work work &&
+	test_must_fail git reset -p aaaaaaaa &&
+	verify_saved_state dir/foo &&
+	verify_saved_state bar
+'
+
 # 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
-- 
gitgitgadget

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

[Junio C Hamano]

"Nika Layzell via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Nika Layzell <nika@thelayzells.com>
>
> 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.
>
> Like resetting with paths, "git reset --patch" does not update HEAD, and
> need not require a commit. The path-filtered form, "git reset --patch
> $object -- $pathspec", has accepted a tree-ish since 2f328c3d.
>
> "git reset --patch" is documented as accepting a <tree-ish> since
> bf44142f ("reset: update documentation to require only tree-ish with
> paths", 2013-01-16). Documentation changes are not required.
>
> Loosen the restriction that requires a commit for the unfiltered "git
> reset --patch $object".
>
> Signed-off-by: Nika Layzell <nika@thelayzells.com>
> ---

Nicely explained.

> diff --git a/t/t7105-reset-patch.sh b/t/t7105-reset-patch.sh
> index bd10a96727..fc2a6cf5c7 100755
> --- a/t/t7105-reset-patch.sh
> +++ b/t/t7105-reset-patch.sh
> @@ -38,6 +38,27 @@ 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
> +'
> +
> +test_expect_success PERL 'git reset -p HEAD^:dir/foo (blob fails)' '
> +	set_and_save_state dir/foo work work &&
> +	test_must_fail git reset -p HEAD^:dir/foo &&
> +	verify_saved_state dir/foo &&
> +	verify_saved_state bar
> +'
> +
> +test_expect_success PERL 'git reset -p aaaaaaaa (unknown fails)' '
> +	set_and_save_state dir/foo work work &&
> +	test_must_fail git reset -p aaaaaaaa &&
> +	verify_saved_state dir/foo &&
> +	verify_saved_state bar
> +'

Thanks, will queue.