restore: allow --staged on unborn branch

Commit Message

Ghanshyam Thakkar Feb. 6, 2024, 11:03 p.m. UTC

Some users expect that being on an unborn branch is similar to having an
empty tree checked out. However, when running "git restore --staged ."
on unborn branch having staged changes, the follwing error gets printed,

    fatal: could not resolve HEAD

Therefore, teach "git restore --staged ." without a source option, to
take empty tree as source on unborn branch. Note that, this assumption
is already taken by "git reset" (166ec2e9). However, still disallow
explicitly referring to HEAD on unborn branch.

Signed-off-by: Ghanshyam Thakkar <[e-mail]>
---
 builtin/checkout.c        | 27 +++++++++++++++++++-------
 t/t2073-restore-unborn.sh | 40 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 60 insertions(+), 7 deletions(-)
 create mode 100755 t/t2073-restore-unborn.sh

Comments

Junio C Hamano Feb. 7, 2024, 4:06 p.m. UTC | #1

Ghanshyam Thakkar <[e-mail]> writes:

> Some users expect that being on an unborn branch is similar to having an
> empty tree checked out. However, when running "git restore --staged ."
> on unborn branch having staged changes, the follwing error gets printed,
>
>     fatal: could not resolve HEAD

Sounds like a sensible behaviour---there is no HEAD so there is
nothing to resolve.  With "git resetore --staged ." in such a state,
what did the user try to do?  "git reset" (no other arguments)?

BTW, "follwing" -> "following".

Ghanshyam Thakkar Feb. 7, 2024, 4:39 p.m. UTC | #2

On Wed Feb 7, 2024 at 9:36 PM IST, Junio C Hamano wrote:
> Ghanshyam Thakkar <[e-mail]> writes:
>
> > Some users expect that being on an unborn branch is similar to having an
> > empty tree checked out. However, when running "git restore --staged ."
> > on unborn branch having staged changes, the follwing error gets printed,
> >
> >     fatal: could not resolve HEAD
>
> Sounds like a sensible behaviour---there is no HEAD so there is
> nothing to resolve.  With "git resetore --staged ." in such a state,
> what did the user try to do?  "git reset" (no other arguments)?

Yeah, I did "git reset" (I am that user btw). But I suppose this is a
case of UX. If a user is using "git restore --staged ." every time they
want to unstage something, then why would they expect it to fail on an
unborn branch when something similar (e.g "git reset") does not?

Also that HEAD, I suppose, is our assumption of the user's intent. And
intent of the user using "git restore --staged ." on unborn branch is
to unstage the changes relative to empty tree. Besides "git reset" already
does this so the matter of assuming empty tree on unborn, I suppose, would
already be settled. Therefore, wouldn't assuming empty tree on unborn
when also using "git restore --staged ." be reasonable and consistent?

Patch

diff --git a/builtin/checkout.c b/builtin/checkout.c
index a6e30931b5..1258ae0a59 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1691,6 +1691,7 @@  static int checkout_main(int argc, const char **argv, const char *prefix,
 			 struct branch_info *new_branch_info)
 {
 	int parseopt_flags = 0;
+	int unborn_and_unspecified = 0;
 
 	opts->overwrite_ignore = 1;
 	opts->prefix = prefix;
@@ -1754,12 +1755,6 @@  static int checkout_main(int argc, const char **argv, const char *prefix,
 	}
 	if (opts->checkout_index < 0 || opts->checkout_worktree < 0)
 		BUG("these flags should be non-negative by now");
-	/*
-	 * convenient shortcut: "git restore --staged [--worktree]" equals
-	 * "git restore --staged [--worktree] --source HEAD"
-	 */
-	if (!opts->from_treeish && opts->checkout_index)
-		opts->from_treeish = "HEAD";
 
 	/*
 	 * From here on, new_branch will contain the branch to be checked out,
@@ -1785,6 +1780,18 @@  static int checkout_main(int argc, const char **argv, const char *prefix,
 		opts->new_branch = argv0 + 1;
 	}
 
+	/*
+	 * convenient shortcut: "git restore --staged [--worktree]" equals
+	 * "git restore --staged [--worktree] --source HEAD"
+	 */
+	if (!opts->from_treeish && opts->checkout_index) {
+		struct object_id oid;
+		opts->from_treeish = "HEAD";
+
+		if(repo_get_oid(the_repository, opts->from_treeish, &oid))
+			unborn_and_unspecified = 1;
+	}
+
 	/*
 	 * Extract branch name from command line arguments, so
 	 * all that is left is pathspecs.
@@ -1812,7 +1819,13 @@  static int checkout_main(int argc, const char **argv, const char *prefix,
 	} else if (!opts->accept_ref && opts->from_treeish) {
 		struct object_id rev;
 
-		if (repo_get_oid_mb(the_repository, opts->from_treeish, &rev))
+		/*
+		 * when the branch is unborn and no revision is given, use
+		 * empty tree as source
+		 */
+		if(unborn_and_unspecified)
+			oidcpy(&rev, the_hash_algo->empty_tree);
+		else if (repo_get_oid_mb(the_repository, opts->from_treeish, &rev))
 			die(_("could not resolve %s"), opts->from_treeish);
 
 		setup_new_branch_info_and_source_tree(new_branch_info,
diff --git a/t/t2073-restore-unborn.sh b/t/t2073-restore-unborn.sh
new file mode 100755
index 0000000000..fbd8b2df5f
--- /dev/null
+++ b/t/t2073-restore-unborn.sh
@@ -0,0 +1,40 @@ 
+#!/bin/sh
+
+test_description='restore --staged should work on unborn branch'
+. ./test-lib.sh
+
+test_expect_success 'explicitly naming HEAD on unborn should fail' '
+	echo a >foo &&
+	echo b >bar &&
+	git add foo bar &&
+	test_must_fail git restore --staged --source=HEAD .
+'
+
+test_expect_success 'restore --staged .' '
+	rm .git/index &&
+	git add foo bar &&
+	git restore --staged . &&
+	git diff --cached --name-only >actual &&
+	test_must_be_empty actual
+'
+
+test_expect_success 'restore --staged $file' '
+	rm .git/index &&
+	git add foo bar &&
+	git restore --staged foo &&
+	git diff --cached --name-only >actual &&
+	echo bar >expected &&
+	test_cmp expected actual
+'
+
+test_expect_success 'restore -p --staged' '
+	rm .git/index &&
+	git add foo bar &&
+	test_write_lines y n | git restore -p --staged >output &&
+	git diff --cached --name-only >actual &&
+	echo foo >expected &&
+	test_cmp expected actual &&
+	test_grep "Unstage" output
+'
+
+test_done