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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 24, 2019, 05:54 UTC
Message-ID: <xmqq4kyt250e.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <338c2777f711ac21a30a7f890a8a11708e9a4fa6.1574538937.git.gitgitgadget@gmail.com>

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

Show 8 quoted lines

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

Show 20 quoted lines

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

Show 14 quoted lines

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