Operations on unborn branch

4 messages, 1 author, 2016-06-15 · open the first message on its own page

Operations on unborn branch

From: Martin von Zweigbergk <hidden>
Date: 2016-06-15 22:55:25

While looking at how to handle "git rebase --root", I noticed that
"git cherry-pick" fails with the following when run on an unborn
branch:

error: You do not have a valid HEAD
fatal: cherry-pick failed

I can not see any reason that it shouldn't work. "git cherry-pick -n"
does work. (For rebase, "git cherry-pick --ff" would be used, and I
think that should also work on an unborn branch.)

Also, "git reset" doesn't work on an unborn branch and I can not see
any reason that it shouldn't work. This was also asked on stack
overflow [1], and of course the solution is to use "git rm --cached",
but doesn't mean that "git reset" shouldn't work.

I have very limited time to work on git these days, so if anyone else
would like to work on any of this, I would be very happy. I _might_
take some time to fix the cherry-pick issue.

Btw, every time I run into problems like these with the treatment of
root commits, I can't help but wonder how things would look if git had
always had a single root commit (naturally with some dummy user,
timestamp etc to ensure sameness across repos). With my limited
knowledge, it seems like that would complicate a few things, but
simplify a lot of things (maybe I'm biased because of the things I
have happened to work on?). Has anyone spent some time seriously
thinking about this? I suppose it would be hard to introduce
backward-compatibly, and maybe this is very unrealistic even for git
2.0, but I would be curious to hear what others think.

Martin

[1] http://stackoverflow.com/questions/3894808/new-git-repository-and-already-git-reset-does-not-work

[RFC/PATCH 0/2] Fix "git reset" on unborn branch

From: Martin von Zweigbergk <hidden>
Date: 2016-06-15 22:55:27

I decided to address this before "cherry-pick on unborn branch". RFC
mostly because I'm not sure about the user interface. When we have
agreed on that, I will add documentation.

Martin von Zweigbergk (2):
  reset: learn to reset to tree
  reset: learn to reset on unborn branch

 builtin/reset.c                     | 73 ++++++++++++++++++++++---------------
 t/t1512-rev-parse-disambiguation.sh |  4 --
 t/t7102-reset.sh                    | 26 +++++++++++++
 t/t7106-reset-unborn-branch.sh      | 52 ++++++++++++++++++++++++++
 4 files changed, 122 insertions(+), 33 deletions(-)
 create mode 100755 t/t7106-reset-unborn-branch.sh

-- 
1.8.0.1.240.ge8a1f5a

[RFC/PATCH 1/2] reset: learn to reset to tree

From: Martin von Zweigbergk <hidden>
Date: 2016-06-15 22:55:27

In cases where HEAD is not supposed to be updated, there is no reason
that "git reset" should require a commit, a tree should be enough. So
make "git reset $rev^{tree}" work just like "git reset $rev", except
that the former will not update HEAD (since there is no commit to
point it to).

Disallow --soft with trees, since that is about updating only HEAD.

By not updating HEAD, "git reset $rev^{tree}" behaves quite like "git
reset $rev .". One might therefore consider requiring a path when
using reset with a tree to make that similarity more obvious. However,
a future commit will make "git reset" work on an unborn branch by
interpreting it as "git reset $empty_tree" and it would seem
unintuitive to the user to say "git reset ." on an unborn
branch. Requiring a path would also make "git reset --hard $tree"
disallowed.

This commit effectively undoes some of commit 13243c2 (reset: the
command takes committish, 2012-07-03). The command line argument is
now required to be an unambiguous treeish.

---

My implementation of lookup_commit_or_tree looks a little clunky. I'm
not very familiar with the API. Suggestions welcome.

Why is the "HEAD is now at ..." message printed only for --hard reset?
After all, HEAD is updated for all types of reset not involving paths.

 builtin/reset.c                     | 67 +++++++++++++++++++++----------------
 t/t1512-rev-parse-disambiguation.sh |  4 ---
 t/t7102-reset.sh                    | 26 ++++++++++++++
 3 files changed, 65 insertions(+), 32 deletions(-)
diff --git a/builtin/reset.c b/builtin/reset.c
index 915cc9f..cec9874 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -225,6 +225,21 @@ static void die_if_unmerged_cache(int reset_type)
 
 }
 
+static struct object *lookup_commit_or_tree(const char *rev) {
+	unsigned char sha1[20];
+	struct commit *commit;
+	struct tree *tree;
+	if (get_sha1_treeish(rev, sha1))
+		die(_("Failed to resolve '%s' as a valid ref."), rev);
+	commit = lookup_commit_reference_gently(sha1, 1);
+	if (commit)
+		return (struct object *) commit;
+	tree = parse_tree_indirect(sha1);
+	if (tree)
+		return (struct object *) tree;
+	die(_("Could not parse object '%s'."), rev);
+}
+
 int cmd_reset(int argc, const char **argv, const char *prefix)
 {
 	int i = 0, reset_type = NONE, update_ref_status = 0, quiet = 0;
@@ -232,7 +247,8 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 	const char *rev = "HEAD";
 	unsigned char sha1[20], *orig = NULL, sha1_orig[20],
 				*old_orig = NULL, sha1_old_orig[20];
-	struct commit *commit;
+	struct object *object;
+	struct commit *commit = NULL;
 	struct strbuf msg = STRBUF_INIT;
 	const struct option options[] = {
 		OPT__QUIET(&quiet, N_("be quiet, only report errors")),
@@ -276,7 +292,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 		 * Otherwise, argv[i] could be either <rev> or <paths> and
 		 * has to be unambiguous.
 		 */
-		else if (!get_sha1_committish(argv[i], sha1)) {
+		else if (!get_sha1_treeish(argv[i], sha1)) {
 			/*
 			 * Ok, argv[i] looks like a rev; it should not
 			 * be a filename.
@@ -289,19 +305,12 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 		}
 	}
 
-	if (get_sha1_committish(rev, sha1))
-		die(_("Failed to resolve '%s' as a valid ref."), rev);
-
-	/*
-	 * NOTE: As "git reset $treeish -- $path" should be usable on
-	 * any tree-ish, this is not strictly correct. We are not
-	 * moving the HEAD to any commit; we are merely resetting the
-	 * entries in the index to that of a treeish.
-	 */
-	commit = lookup_commit_reference(sha1);
-	if (!commit)
-		die(_("Could not parse object '%s'."), rev);
-	hashcpy(sha1, commit->object.sha1);
+	object = lookup_commit_or_tree(rev);
+	if (object->type == OBJ_COMMIT)
+		commit = (struct commit*) object;
+	else if (reset_type == SOFT)
+		die(_("--soft requires a commit, which '%s' is not"), rev);
+	hashcpy(sha1, object->sha1);
 
 	if (patch_mode) {
 		if (reset_type != NONE)
@@ -347,23 +356,25 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 			die(_("Could not reset index file to revision '%s'."), rev);
 	}
 
-	/* Any resets update HEAD to the head being switched to,
-	 * saving the previous head in ORIG_HEAD before. */
-	if (!get_sha1("ORIG_HEAD", sha1_old_orig))
-		old_orig = sha1_old_orig;
-	if (!get_sha1("HEAD", sha1_orig)) {
-		orig = sha1_orig;
-		set_reflog_message(&msg, "updating ORIG_HEAD", NULL);
-		update_ref(msg.buf, "ORIG_HEAD", orig, old_orig, 0, MSG_ON_ERR);
+	if (commit) {
+		/* Any resets update HEAD to the head being switched to,
+		 * saving the previous head in ORIG_HEAD before. */
+		if (!get_sha1("ORIG_HEAD", sha1_old_orig))
+			old_orig = sha1_old_orig;
+		if (!get_sha1("HEAD", sha1_orig)) {
+			orig = sha1_orig;
+			set_reflog_message(&msg, "updating ORIG_HEAD", NULL);
+			update_ref(msg.buf, "ORIG_HEAD", orig, old_orig, 0, MSG_ON_ERR);
+		}
+		else if (old_orig)
+			delete_ref("ORIG_HEAD", old_orig, 0);
+		set_reflog_message(&msg, "updating HEAD", rev);
+		update_ref_status = update_ref(msg.buf, "HEAD", sha1, orig, 0, MSG_ON_ERR);
 	}
-	else if (old_orig)
-		delete_ref("ORIG_HEAD", old_orig, 0);
-	set_reflog_message(&msg, "updating HEAD", rev);
-	update_ref_status = update_ref(msg.buf, "HEAD", sha1, orig, 0, MSG_ON_ERR);
 
 	switch (reset_type) {
 	case HARD:
-		if (!update_ref_status && !quiet)
+		if (commit && !update_ref_status && !quiet)
 			print_new_head_line(commit);
 		break;
 	case SOFT: /* Nothing else to do. */
diff --git a/t/t1512-rev-parse-disambiguation.sh b/t/t1512-rev-parse-disambiguation.sh
index 6b3d797..bc1e40c 100755
--- a/t/t1512-rev-parse-disambiguation.sh
+++ b/t/t1512-rev-parse-disambiguation.sh
@@ -121,10 +121,6 @@ test_expect_success 'git log takes only commit-ish' '
 	git log 000000000
 '
 
-test_expect_success 'git reset takes only commit-ish' '
-	git reset 000000000
-'
-
 test_expect_success 'first tag' '
 	# create one tag 0000000000f8f
 	git tag -a -m j7cp83um v1.0.0
diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
index b096dc8..d723ef5 100755
--- a/t/t7102-reset.sh
+++ b/t/t7102-reset.sh
@@ -491,4 +491,30 @@ test_expect_success 'disambiguation (4)' '
 	test ! -f secondfile
 '
 
+test_expect_success 'reset to tree does not update HEAD' '
+	git reset --hard HEAD &&
+	rev_before=$(git rev-parse HEAD) &&
+	git reset HEAD^^{tree} &&
+	test $(git rev-parse HEAD) == $rev_before
+'
+
+test_expect_success 'reset to tree' '
+	# for simpler tests, drop last commit containing added files
+	git reset --hard HEAD^ &&
+	git reset HEAD^^{tree} &&
+	git diff --cached HEAD^ --exit-code &&
+	git diff HEAD --exit-code
+'
+
+test_expect_success 'reset --hard to tree' '
+	git reset --hard &&
+	git reset --hard HEAD^^{tree} &&
+	git diff --cached HEAD^ --exit-code &&
+	git diff HEAD^ --exit-code
+'
+
+test_expect_success 'reset to tree not allowed with --soft}' '
+	test_must_fail git reset --soft HEAD^^{tree}
+'
+
 test_done
-- 
1.8.0.1.240.ge8a1f5a

[RFC/PATCH 2/2] reset: learn to reset on unborn branch

From: Martin von Zweigbergk <hidden>
Date: 2016-06-15 22:55:27

When run on an unborn branch, "git reset" currently fails with:

  fatal: Failed to resolve 'HEAD' as a valid ref.

Fix this by interpreting it as a reset to the empty tree.

If --patch is given, we currently pass the revision specifier, as
given on the command line, to interactive_reset(). On an unborn
branch, HEAD can of course not be resolved, so we need to pass the
sha1 of the empty tree to interactive_reset() as well. This is fine
since interactive_reset only needs the parameter to be a treeish and
doesn't use it for display purposes.

---

Is it correct that interactive_reset does not use the revision
specifier for display purposes? Or, worse, that it requires it to be a
commit in some cases? I tried it and didn't see any problem.

 builtin/reset.c                | 10 +++++---
 t/t7106-reset-unborn-branch.sh | 52 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+), 3 deletions(-)
 create mode 100755 t/t7106-reset-unborn-branch.sh
diff --git a/builtin/reset.c b/builtin/reset.c
index cec9874..3845225 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -229,7 +229,10 @@ static struct object *lookup_commit_or_tree(const char *rev) {
 	unsigned char sha1[20];
 	struct commit *commit;
 	struct tree *tree;
-	if (get_sha1_treeish(rev, sha1))
+	if (!strcmp(rev, "HEAD") && get_sha1("HEAD", sha1)) {
+		// unborn branch: reset to empty tree
+		hashcpy(sha1, EMPTY_TREE_SHA1_BIN);
+	} else if (get_sha1_treeish(rev, sha1))
 		die(_("Failed to resolve '%s' as a valid ref."), rev);
 	commit = lookup_commit_reference_gently(sha1, 1);
 	if (commit)
@@ -292,7 +295,8 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 		 * Otherwise, argv[i] could be either <rev> or <paths> and
 		 * has to be unambiguous.
 		 */
-		else if (!get_sha1_treeish(argv[i], sha1)) {
+		else if (!strcmp(argv[i], "HEAD") ||
+			 !get_sha1_treeish(argv[i], sha1)) {
 			/*
 			 * Ok, argv[i] looks like a rev; it should not
 			 * be a filename.
@@ -315,7 +319,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 	if (patch_mode) {
 		if (reset_type != NONE)
 			die(_("--patch is incompatible with --{hard,mixed,soft}"));
-		return interactive_reset(rev, argv + i, prefix);
+		return interactive_reset(sha1_to_hex(sha1), argv + i, prefix);
 	}
 
 	/* git reset tree [--] paths... can be used to
diff --git a/t/t7106-reset-unborn-branch.sh b/t/t7106-reset-unborn-branch.sh
new file mode 100755
index 0000000..67d45be
--- /dev/null
+++ b/t/t7106-reset-unborn-branch.sh
@@ -0,0 +1,52 @@
+#!/bin/sh
+
+test_description='git reset should work on unborn branch'
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	echo a >a &&
+	echo b >b
+'
+
+test_expect_success 'reset' '
+	git add a b &&
+	git reset &&
+	test "$(git ls-files)" == ""
+'
+
+test_expect_success 'reset HEAD' '
+	rm .git/index &&
+	git add a b &&
+	git reset HEAD &&
+	test "$(git ls-files)" == ""
+'
+
+test_expect_success 'reset $file' '
+	rm .git/index &&
+	git add a b &&
+	git reset a &&
+	test "$(git ls-files)" == "b"
+'
+
+test_expect_success 'reset -p' '
+	rm .git/index &&
+	git add a &&
+	echo y | git reset -p &&
+	test "$(git ls-files)" == ""
+'
+
+test_expect_success 'reset --soft not allowed' '
+	rm .git/index &&
+	git add a &&
+	test_must_fail git reset --soft
+'
+
+test_expect_success 'reset --hard' '
+	rm .git/index &&
+	git add a &&
+	git reset --hard &&
+	test "$(git ls-files)" == "" &&
+	test_path_is_missing a
+'
+
+test_done
-- 
1.8.0.1.240.ge8a1f5a
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help