From 10746a361689aaa1aa98b8d4e7fb3b8463391864 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Mon, 14 Jan 2013 21:47:33 -0800 Subject: [PATCH 01/20] reset $pathspec: no need to discard index Since 34110cd (Make 'unpack_trees()' have a separate source and destination index, 2008-03-06), the index no longer gets clobbered by do_diff_cache() and we can remove the code for discarding and re-reading it. There are two paths to update_index_refresh() from cmd_reset(), but on both paths, either read_cache() or read_cache_unmerged() will have been called, so the call to read_cache() in this method is redundant (although practically free). This speeds up "git reset -- ." a little on the linux-2.6 repo (best of five, warm cache): Before After real 0m0.093s 0m0.080s user 0m0.040s 0m0.020s sys 0m0.050s 0m0.050s Signed-off-by: Martin von Zweigbergk Signed-off-by: Junio C Hamano --- builtin/reset.c | 16 +--------------- 1 file changed, 1 insertion(+), 15 deletions(-) diff --git a/builtin/reset.c b/builtin/reset.c index 915cc9f86f..8cc7c722ef 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -126,9 +126,6 @@ static int update_index_refresh(int fd, struct lock_file *index_lock, int flags) fd = hold_locked_index(index_lock, 1); } - if (read_cache() < 0) - return error(_("Could not read index")); - result = refresh_index(&the_index, (flags), NULL, NULL, _("Unstaged changes after reset:")) ? 1 : 0; if (write_cache(fd, active_cache, active_nr) || @@ -141,12 +138,6 @@ static void update_index_from_diff(struct diff_queue_struct *q, struct diff_options *opt, void *data) { int i; - int *discard_flag = data; - - /* do_diff_cache() mangled the index */ - discard_cache(); - *discard_flag = 1; - read_cache(); for (i = 0; i < q->nr; i++) { struct diff_filespec *one = q->queue[i]->one; @@ -179,17 +170,15 @@ static int read_from_tree(const char *prefix, const char **argv, unsigned char *tree_sha1, int refresh_flags) { struct lock_file *lock = xcalloc(1, sizeof(struct lock_file)); - int index_fd, index_was_discarded = 0; + int index_fd; struct diff_options opt; memset(&opt, 0, sizeof(opt)); diff_tree_setup_paths(get_pathspec(prefix, (const char **)argv), &opt); opt.output_format = DIFF_FORMAT_CALLBACK; opt.format_callback = update_index_from_diff; - opt.format_callback_data = &index_was_discarded; index_fd = hold_locked_index(lock, 1); - index_was_discarded = 0; read_cache(); if (do_diff_cache(tree_sha1, &opt)) return 1; @@ -197,9 +186,6 @@ static int read_from_tree(const char *prefix, const char **argv, diff_flush(&opt); diff_tree_release_paths(&opt); - if (!index_was_discarded) - /* The index is still clobbered from do_diff_cache() */ - discard_cache(); return update_index_refresh(index_fd, lock, refresh_flags); } From d94c5e2fa24dce13a3dc1ba178f381cb09bb0853 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Mon, 14 Jan 2013 21:47:34 -0800 Subject: [PATCH 02/20] reset $pathspec: exit with code 0 if successful "git reset $pathspec" currently exits with a non-zero exit code if the worktree is dirty after resetting, which is inconsistent with reset without pathspec, and it makes it harder to know whether the command really failed. Change it to exit with code 0 regardless of whether the worktree is dirty so that non-zero indicates an error. This makes the 4 "disambiguation" test cases in t7102 clearer since they all used to "fail", 3 of which "failed" due to changes in the work tree. Now only the ambiguous one fails. Signed-off-by: Martin von Zweigbergk Signed-off-by: Junio C Hamano --- builtin/reset.c | 8 +++----- t/t2013-checkout-submodule.sh | 2 +- t/t7102-reset.sh | 18 ++++++++++++------ 3 files changed, 16 insertions(+), 12 deletions(-) diff --git a/builtin/reset.c b/builtin/reset.c index 8cc7c722ef..65413d0a67 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -119,19 +119,17 @@ static void print_new_head_line(struct commit *commit) static int update_index_refresh(int fd, struct lock_file *index_lock, int flags) { - int result; - if (!index_lock) { index_lock = xcalloc(1, sizeof(struct lock_file)); fd = hold_locked_index(index_lock, 1); } - result = refresh_index(&the_index, (flags), NULL, NULL, - _("Unstaged changes after reset:")) ? 1 : 0; + refresh_index(&the_index, (flags), NULL, NULL, + _("Unstaged changes after reset:")); if (write_cache(fd, active_cache, active_nr) || commit_locked_index(index_lock)) return error ("Could not refresh index"); - return result; + return 0; } static void update_index_from_diff(struct diff_queue_struct *q, diff --git a/t/t2013-checkout-submodule.sh b/t/t2013-checkout-submodule.sh index 70edbb33e2..06b18f8bc1 100755 --- a/t/t2013-checkout-submodule.sh +++ b/t/t2013-checkout-submodule.sh @@ -23,7 +23,7 @@ test_expect_success '"reset " updates the index' ' git update-index --refresh && git diff-files --quiet && git diff-index --quiet --cached HEAD && - test_must_fail git reset HEAD^ submodule && + git reset HEAD^ submodule && test_must_fail git diff-files --quiet && git reset submodule && git diff-files --quiet diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh index b096dc88c2..81b2570a3e 100755 --- a/t/t7102-reset.sh +++ b/t/t7102-reset.sh @@ -388,7 +388,8 @@ test_expect_success 'test --mixed ' ' echo 4 > file4 && echo 5 > file1 && git add file1 file3 file4 && - test_must_fail git reset HEAD -- file1 file2 file3 && + git reset HEAD -- file1 file2 file3 && + test_must_fail git diff --quiet && git diff > output && test_cmp output expect && git diff --cached > output && @@ -402,7 +403,8 @@ test_expect_success 'test resetting the index at give paths' ' >sub/file2 && git update-index --add sub/file1 sub/file2 && T=$(git write-tree) && - test_must_fail git reset HEAD sub/file2 && + git reset HEAD sub/file2 && + test_must_fail git diff --quiet && U=$(git write-tree) && echo "$T" && echo "$U" && @@ -440,7 +442,8 @@ test_expect_success 'resetting specific path that is unmerged' ' echo "100644 $F3 3 file2" } | git update-index --index-info && git ls-files -u && - test_must_fail git reset HEAD file2 && + git reset HEAD file2 && + test_must_fail git diff --quiet && git diff-index --exit-code --cached HEAD ' @@ -449,7 +452,8 @@ test_expect_success 'disambiguation (1)' ' git reset --hard && >secondfile && git add secondfile && - test_must_fail git reset secondfile && + git reset secondfile && + test_must_fail git diff --quiet -- secondfile && test -z "$(git diff --cached --name-only)" && test -f secondfile && test ! -s secondfile @@ -474,7 +478,8 @@ test_expect_success 'disambiguation (3)' ' >secondfile && git add secondfile && rm -f secondfile && - test_must_fail git reset HEAD secondfile && + git reset HEAD secondfile && + test_must_fail git diff --quiet && test -z "$(git diff --cached --name-only)" && test ! -f secondfile @@ -486,7 +491,8 @@ test_expect_success 'disambiguation (4)' ' >secondfile && git add secondfile && rm -f secondfile && - test_must_fail git reset -- secondfile && + git reset -- secondfile && + test_must_fail git diff --quiet && test -z "$(git diff --cached --name-only)" && test ! -f secondfile ' From 18648e89e78662e3a6a9a38adfca3e6001cfaf85 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Mon, 14 Jan 2013 21:47:35 -0800 Subject: [PATCH 03/20] reset.c: pass pathspec around instead of (prefix, argv) pair We use the path arguments in two places in reset.c: in interactive_reset() and read_from_tree(). Both of these call get_pathspec(), so we pass the (prefix, argv) pair to both functions. Move the call to get_pathspec() out of these methods, for two reasons: 1) One argument is simpler than two. 2) It lets us use the (arguably clearer) "if (pathspec)" in place of "if (i < argc)". Signed-off-by: Martin von Zweigbergk Signed-off-by: Junio C Hamano --- builtin/reset.c | 27 ++++++++++----------------- 1 file changed, 10 insertions(+), 17 deletions(-) diff --git a/builtin/reset.c b/builtin/reset.c index 65413d0a67..045c9609f2 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -153,26 +153,15 @@ static void update_index_from_diff(struct diff_queue_struct *q, } } -static int interactive_reset(const char *revision, const char **argv, - const char *prefix) -{ - const char **pathspec = NULL; - - if (*argv) - pathspec = get_pathspec(prefix, argv); - - return run_add_interactive(revision, "--patch=reset", pathspec); -} - -static int read_from_tree(const char *prefix, const char **argv, - unsigned char *tree_sha1, int refresh_flags) +static int read_from_tree(const char **pathspec, unsigned char *tree_sha1, + int refresh_flags) { struct lock_file *lock = xcalloc(1, sizeof(struct lock_file)); int index_fd; struct diff_options opt; memset(&opt, 0, sizeof(opt)); - diff_tree_setup_paths(get_pathspec(prefix, (const char **)argv), &opt); + diff_tree_setup_paths(pathspec, &opt); opt.output_format = DIFF_FORMAT_CALLBACK; opt.format_callback = update_index_from_diff; @@ -216,6 +205,7 @@ 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]; + const char **pathspec = NULL; struct commit *commit; struct strbuf msg = STRBUF_INIT; const struct option options[] = { @@ -287,22 +277,25 @@ int cmd_reset(int argc, const char **argv, const char *prefix) die(_("Could not parse object '%s'."), rev); hashcpy(sha1, commit->object.sha1); + if (i < argc) + pathspec = get_pathspec(prefix, argv + i); + if (patch_mode) { if (reset_type != NONE) die(_("--patch is incompatible with --{hard,mixed,soft}")); - return interactive_reset(rev, argv + i, prefix); + return run_add_interactive(rev, "--patch=reset", pathspec); } /* git reset tree [--] paths... can be used to * load chosen paths from the tree into the index without * affecting the working tree nor HEAD. */ - if (i < argc) { + if (pathspec) { if (reset_type == MIXED) warning(_("--mixed with paths is deprecated; use 'git reset -- ' instead.")); else if (reset_type != NONE) die(_("Cannot do %s reset with paths."), _(reset_type_names[reset_type])); - return read_from_tree(prefix, argv + i, sha1, + return read_from_tree(pathspec, sha1, quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN); } if (reset_type == NONE) From 4f4ad3d938beafd785c319664348268bbef11a00 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Mon, 14 Jan 2013 21:47:36 -0800 Subject: [PATCH 04/20] reset: don't allow "git reset -- $pathspec" in bare repo Running e.g. "git reset ." in a bare repo results in an index file being created from the HEAD commit. The differences compared to the index are then printed as usual, but since there is no worktree, it will appear as if all files are deleted. For example, in a bare clone of git.git: Unstaged changes after reset: D .gitattributes D .gitignore D .mailmap ... This happens because the check for is_bare_repository() happens after we branch off into read_from_tree() to reset with paths. Fix by moving the branching point after the check. Signed-off-by: Martin von Zweigbergk Signed-off-by: Junio C Hamano --- builtin/reset.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/builtin/reset.c b/builtin/reset.c index 045c9609f2..664fad9fc5 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -295,8 +295,6 @@ int cmd_reset(int argc, const char **argv, const char *prefix) else if (reset_type != NONE) die(_("Cannot do %s reset with paths."), _(reset_type_names[reset_type])); - return read_from_tree(pathspec, sha1, - quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN); } if (reset_type == NONE) reset_type = MIXED; /* by default */ @@ -308,6 +306,10 @@ int cmd_reset(int argc, const char **argv, const char *prefix) die(_("%s reset is not allowed in a bare repository"), _(reset_type_names[reset_type])); + if (pathspec) + return read_from_tree(pathspec, sha1, + quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN); + /* Soft reset does not touch the index file nor the working tree * at all, but requires them in a good order. Other resets reset * the index file to the tree object we are switching to. */ From 39ea722d829ddc5bb253fbe12333d768965f4fe7 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Mon, 14 Jan 2013 21:47:37 -0800 Subject: [PATCH 05/20] reset.c: extract function for parsing arguments Declutter cmd_reset() a bit by moving out the argument parsing to its own function. Signed-off-by: Martin von Zweigbergk Signed-off-by: Junio C Hamano --- builtin/reset.c | 86 ++++++++++++++++++++++++++----------------------- 1 file changed, 46 insertions(+), 40 deletions(-) diff --git a/builtin/reset.c b/builtin/reset.c index 664fad9fc5..58f0f6116b 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -198,11 +198,54 @@ static void die_if_unmerged_cache(int reset_type) } +static const char **parse_args(int argc, const char **argv, const char *prefix, const char **rev_ret) +{ + int i = 0; + const char *rev = "HEAD"; + unsigned char unused[20]; + /* + * Possible arguments are: + * + * git reset [-opts] ... + * git reset [-opts] -- ... + * git reset [-opts] -- ... + * git reset [-opts] ... + * + * At this point, argv[i] points immediately after [-opts]. + */ + + if (i < argc) { + if (!strcmp(argv[i], "--")) { + i++; /* reset to HEAD, possibly with paths */ + } else if (i + 1 < argc && !strcmp(argv[i+1], "--")) { + rev = argv[i]; + i += 2; + } + /* + * Otherwise, argv[i] could be either or and + * has to be unambiguous. + */ + else if (!get_sha1_committish(argv[i], unused)) { + /* + * Ok, argv[i] looks like a rev; it should not + * be a filename. + */ + verify_non_filename(prefix, argv[i]); + rev = argv[i++]; + } else { + /* Otherwise we treat this as a filename */ + verify_filename(prefix, argv[i], 1); + } + } + *rev_ret = rev; + return i < argc ? get_pathspec(prefix, argv + i) : NULL; +} + int cmd_reset(int argc, const char **argv, const char *prefix) { - int i = 0, reset_type = NONE, update_ref_status = 0, quiet = 0; + int reset_type = NONE, update_ref_status = 0, quiet = 0; int patch_mode = 0; - const char *rev = "HEAD"; + const char *rev; unsigned char sha1[20], *orig = NULL, sha1_orig[20], *old_orig = NULL, sha1_old_orig[20]; const char **pathspec = NULL; @@ -227,41 +270,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix) argc = parse_options(argc, argv, prefix, options, git_reset_usage, PARSE_OPT_KEEP_DASHDASH); - - /* - * Possible arguments are: - * - * git reset [-opts] ... - * git reset [-opts] -- ... - * git reset [-opts] -- ... - * git reset [-opts] ... - * - * At this point, argv[i] points immediately after [-opts]. - */ - - if (i < argc) { - if (!strcmp(argv[i], "--")) { - i++; /* reset to HEAD, possibly with paths */ - } else if (i + 1 < argc && !strcmp(argv[i+1], "--")) { - rev = argv[i]; - i += 2; - } - /* - * Otherwise, argv[i] could be either or and - * has to be unambiguous. - */ - else if (!get_sha1_committish(argv[i], sha1)) { - /* - * Ok, argv[i] looks like a rev; it should not - * be a filename. - */ - verify_non_filename(prefix, argv[i]); - rev = argv[i++]; - } else { - /* Otherwise we treat this as a filename */ - verify_filename(prefix, argv[i], 1); - } - } + pathspec = parse_args(argc, argv, prefix, &rev); if (get_sha1_committish(rev, sha1)) die(_("Failed to resolve '%s' as a valid ref."), rev); @@ -277,9 +286,6 @@ int cmd_reset(int argc, const char **argv, const char *prefix) die(_("Could not parse object '%s'."), rev); hashcpy(sha1, commit->object.sha1); - if (i < argc) - pathspec = get_pathspec(prefix, argv + i); - if (patch_mode) { if (reset_type != NONE) die(_("--patch is incompatible with --{hard,mixed,soft}")); From dca48cf520933c028f807f65175c29482a209f23 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Mon, 14 Jan 2013 21:47:38 -0800 Subject: [PATCH 06/20] reset.c: remove unnecessary variable 'i' Throughout most of parse_args(), the variable 'i' remains at 0. Many references are still made to the variable even when it could only have the value 0. This made at least me, who has relatively little experience with C programming styles, think that parts of the function was meant to be part of a loop. To avoid such confusion, remove the variable and also the 'argc' parameter and check for NULL trailing argv instead. Signed-off-by: Martin von Zweigbergk Signed-off-by: Junio C Hamano --- builtin/reset.c | 33 ++++++++++++++++----------------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/builtin/reset.c b/builtin/reset.c index 58f0f6116b..d89cf4d113 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -198,9 +198,8 @@ static void die_if_unmerged_cache(int reset_type) } -static const char **parse_args(int argc, const char **argv, const char *prefix, const char **rev_ret) +static const char **parse_args(const char **argv, const char *prefix, const char **rev_ret) { - int i = 0; const char *rev = "HEAD"; unsigned char unused[20]; /* @@ -211,34 +210,34 @@ static const char **parse_args(int argc, const char **argv, const char *prefix, * git reset [-opts] -- ... * git reset [-opts] ... * - * At this point, argv[i] points immediately after [-opts]. + * At this point, argv points immediately after [-opts]. */ - if (i < argc) { - if (!strcmp(argv[i], "--")) { - i++; /* reset to HEAD, possibly with paths */ - } else if (i + 1 < argc && !strcmp(argv[i+1], "--")) { - rev = argv[i]; - i += 2; + if (argv[0]) { + if (!strcmp(argv[0], "--")) { + argv++; /* reset to HEAD, possibly with paths */ + } else if (argv[1] && !strcmp(argv[1], "--")) { + rev = argv[0]; + argv += 2; } /* - * Otherwise, argv[i] could be either or and + * Otherwise, argv[0] could be either or and * has to be unambiguous. */ - else if (!get_sha1_committish(argv[i], unused)) { + else if (!get_sha1_committish(argv[0], unused)) { /* - * Ok, argv[i] looks like a rev; it should not + * Ok, argv[0] looks like a rev; it should not * be a filename. */ - verify_non_filename(prefix, argv[i]); - rev = argv[i++]; + verify_non_filename(prefix, argv[0]); + rev = *argv++; } else { /* Otherwise we treat this as a filename */ - verify_filename(prefix, argv[i], 1); + verify_filename(prefix, argv[0], 1); } } *rev_ret = rev; - return i < argc ? get_pathspec(prefix, argv + i) : NULL; + return argv[0] ? get_pathspec(prefix, argv) : NULL; } int cmd_reset(int argc, const char **argv, const char *prefix) @@ -270,7 +269,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix) argc = parse_options(argc, argv, prefix, options, git_reset_usage, PARSE_OPT_KEEP_DASHDASH); - pathspec = parse_args(argc, argv, prefix, &rev); + pathspec = parse_args(argv, prefix, &rev); if (get_sha1_committish(rev, sha1)) die(_("Failed to resolve '%s' as a valid ref."), rev); From 7bca0e451b8e502fa882b458d6fc825da80034cb Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Mon, 14 Jan 2013 21:47:39 -0800 Subject: [PATCH 07/20] reset.c: extract function for updating {ORIG_,}HEAD By extracting the code for updating the HEAD and ORIG_HEAD symbolic references to a separate function, we declutter cmd_reset() a bit and we make it clear that e.g. the four variables {,sha1_}{,old_}orig are only used by this code. Signed-off-by: Martin von Zweigbergk Signed-off-by: Junio C Hamano --- builtin/reset.c | 39 +++++++++++++++++++++++---------------- 1 file changed, 23 insertions(+), 16 deletions(-) diff --git a/builtin/reset.c b/builtin/reset.c index d89cf4d113..2187d6453d 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -240,16 +240,35 @@ static const char **parse_args(const char **argv, const char *prefix, const char return argv[0] ? get_pathspec(prefix, argv) : NULL; } +static int update_refs(const char *rev, const unsigned char *sha1) +{ + int update_ref_status; + struct strbuf msg = STRBUF_INIT; + unsigned char *orig = NULL, sha1_orig[20], + *old_orig = NULL, sha1_old_orig[20]; + + 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); + strbuf_release(&msg); + return update_ref_status; +} + int cmd_reset(int argc, const char **argv, const char *prefix) { int reset_type = NONE, update_ref_status = 0, quiet = 0; int patch_mode = 0; const char *rev; - unsigned char sha1[20], *orig = NULL, sha1_orig[20], - *old_orig = NULL, sha1_old_orig[20]; + unsigned char sha1[20]; const char **pathspec = NULL; struct commit *commit; - struct strbuf msg = STRBUF_INIT; const struct option options[] = { OPT__QUIET(&quiet, N_("be quiet, only report errors")), OPT_SET_INT(0, "mixed", &reset_type, @@ -333,17 +352,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix) /* 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); + update_ref_status = update_refs(rev, sha1); switch (reset_type) { case HARD: @@ -360,7 +369,5 @@ int cmd_reset(int argc, const char **argv, const char *prefix) remove_branch_state(); - strbuf_release(&msg); - return update_ref_status; } From 352f58a57ba3050adbdc2dcdcd3839e584f1431b Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Mon, 14 Jan 2013 21:47:40 -0800 Subject: [PATCH 08/20] reset.c: share call to die_if_unmerged_cache() Use a single condition to guard the call to die_if_unmerged_cache for both --soft and --keep. This avoids the small distraction of the precondition check from the logic following it. Also change an instance of if (e) err = err || f(); to the almost as short, but clearer if (e && !err) err = f(); (which is equivalent since we only care whether exit code is 0) Signed-off-by: Martin von Zweigbergk Signed-off-by: Junio C Hamano --- builtin/reset.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/builtin/reset.c b/builtin/reset.c index 2187d6453d..4e341950a1 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -337,15 +337,13 @@ int cmd_reset(int argc, const char **argv, const char *prefix) /* Soft reset does not touch the index file nor the working tree * at all, but requires them in a good order. Other resets reset * the index file to the tree object we are switching to. */ - if (reset_type == SOFT) + if (reset_type == SOFT || reset_type == KEEP) die_if_unmerged_cache(reset_type); - else { - int err; - if (reset_type == KEEP) - die_if_unmerged_cache(reset_type); - err = reset_index_file(sha1, reset_type, quiet); - if (reset_type == KEEP) - err = err || reset_index_file(sha1, MIXED, quiet); + + if (reset_type != SOFT) { + int err = reset_index_file(sha1, reset_type, quiet); + if (reset_type == KEEP && !err) + err = reset_index_file(sha1, MIXED, quiet); if (err) die(_("Could not reset index file to revision '%s'."), rev); } From b7099a06e8ffe61a06d3e32632e832e59f23bd4d Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Mon, 14 Jan 2013 21:47:41 -0800 Subject: [PATCH 09/20] reset --keep: only write index file once "git reset --keep" calls reset_index_file() twice, first doing a two-way merge to the target revision, updating the index and worktree, and then resetting the index. After each call, we write the index file. In the unlikely event that the second call to reset_index_file() fails, the index will have been merged to the target revision, but HEAD will not be updated, leaving the user with a dirty index. By moving the locking, writing and committing out of reset_index_file() and into the caller, we can avoid writing the index twice, thereby making the sure we don't end up in the half-way reset state. As a bonus, we speed up "git reset --keep" a little on the linux-2.6 repo (best of five, warm cache): Before After real 0m0.315s 0m0.296s user 0m0.290s 0m0.280s sys 0m0.020s 0m0.010s Signed-off-by: Martin von Zweigbergk Signed-off-by: Junio C Hamano --- builtin/reset.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/builtin/reset.c b/builtin/reset.c index 4e341950a1..7c440add07 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -38,14 +38,12 @@ static inline int is_merge(void) return !access(git_path("MERGE_HEAD"), F_OK); } -static int reset_index_file(const unsigned char *sha1, int reset_type, int quiet) +static int reset_index(const unsigned char *sha1, int reset_type, int quiet) { int nr = 1; - int newfd; struct tree_desc desc[2]; struct tree *tree; struct unpack_trees_options opts; - struct lock_file *lock = xcalloc(1, sizeof(struct lock_file)); memset(&opts, 0, sizeof(opts)); opts.head_idx = 1; @@ -67,8 +65,6 @@ static int reset_index_file(const unsigned char *sha1, int reset_type, int quiet opts.reset = 1; } - newfd = hold_locked_index(lock, 1); - read_cache_unmerged(); if (reset_type == KEEP) { @@ -91,10 +87,6 @@ static int reset_index_file(const unsigned char *sha1, int reset_type, int quiet prime_cache_tree(&active_cache_tree, tree); } - if (write_cache(newfd, active_cache, active_nr) || - commit_locked_index(lock)) - return error(_("Could not write new index file.")); - return 0; } @@ -341,9 +333,16 @@ int cmd_reset(int argc, const char **argv, const char *prefix) die_if_unmerged_cache(reset_type); if (reset_type != SOFT) { - int err = reset_index_file(sha1, reset_type, quiet); + struct lock_file *lock = xcalloc(1, sizeof(struct lock_file)); + int newfd = hold_locked_index(lock, 1); + int err = reset_index(sha1, reset_type, quiet); if (reset_type == KEEP && !err) - err = reset_index_file(sha1, MIXED, quiet); + err = reset_index(sha1, MIXED, quiet); + if (!err && + (write_cache(newfd, active_cache, active_nr) || + commit_locked_index(lock))) { + err = error(_("Could not write new index file.")); + } if (err) die(_("Could not reset index file to revision '%s'."), rev); } From 1ca38f85860b33ddc79b1494baf29aecde8616cd Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Mon, 14 Jan 2013 21:47:42 -0800 Subject: [PATCH 10/20] reset: avoid redundant error message If writing or committing the new index file fails, we print "Could not write new index file." followed by "Could not reset index file to revision $rev.". The first message seems to imply the second, so print only the first message. Signed-off-by: Martin von Zweigbergk Signed-off-by: Junio C Hamano --- builtin/reset.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/builtin/reset.c b/builtin/reset.c index 7c440add07..97fa9f7859 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -338,13 +338,11 @@ int cmd_reset(int argc, const char **argv, const char *prefix) int err = reset_index(sha1, reset_type, quiet); if (reset_type == KEEP && !err) err = reset_index(sha1, MIXED, quiet); - if (!err && - (write_cache(newfd, active_cache, active_nr) || - commit_locked_index(lock))) { - err = error(_("Could not write new index file.")); - } if (err) die(_("Could not reset index file to revision '%s'."), rev); + if (write_cache(newfd, active_cache, active_nr) || + commit_locked_index(lock)) + die(_("Could not write new index file.")); } /* Any resets update HEAD to the head being switched to, From b489097e1dbff7764e35ee260f8397d62a70fbb5 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Mon, 14 Jan 2013 21:47:43 -0800 Subject: [PATCH 11/20] reset.c: replace switch by if-else The switch statement towards the end of reset.c is missing case arms for KEEP and MERGE for no obvious reason, and soon the only non-empty case arm will be the one for HARD. So let's proactively replace it by if-else, which will let us move one if statement out without leaving funny-looking left-overs. Signed-off-by: Martin von Zweigbergk Signed-off-by: Junio C Hamano --- builtin/reset.c | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/builtin/reset.c b/builtin/reset.c index 97fa9f7859..c3eb2eb48d 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -349,18 +349,11 @@ int cmd_reset(int argc, const char **argv, const char *prefix) * saving the previous head in ORIG_HEAD before. */ update_ref_status = update_refs(rev, sha1); - switch (reset_type) { - case HARD: - if (!update_ref_status && !quiet) - print_new_head_line(commit); - break; - case SOFT: /* Nothing else to do. */ - break; - case MIXED: /* Report what has not been updated. */ + if (reset_type == HARD && !update_ref_status && !quiet) + print_new_head_line(commit); + else if (reset_type == MIXED) /* Report what has not been updated. */ update_index_refresh(0, NULL, quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN); - break; - } remove_branch_state(); From bf883f3006f719ba1ade0180f1764f42cc20b529 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Mon, 14 Jan 2013 21:47:44 -0800 Subject: [PATCH 12/20] reset.c: move update_index_refresh() call out of read_from_tree() The final part of cmd_reset() essentially looks like: if (pathspec) { ... read_from_tree(...); } else { ... reset_index(...); update_index_refresh(...); ... } where read_from_tree() internally also calls update_index_refresh(). Move the call to update_index_refresh() out of read_from_tree for symmetry with the 'else' block, making read_from_tree() and reset_index() closer in functionality. Signed-off-by: Martin von Zweigbergk Signed-off-by: Junio C Hamano --- builtin/reset.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/builtin/reset.c b/builtin/reset.c index c3eb2eb48d..70733c271d 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -145,11 +145,8 @@ static void update_index_from_diff(struct diff_queue_struct *q, } } -static int read_from_tree(const char **pathspec, unsigned char *tree_sha1, - int refresh_flags) +static int read_from_tree(const char **pathspec, unsigned char *tree_sha1) { - struct lock_file *lock = xcalloc(1, sizeof(struct lock_file)); - int index_fd; struct diff_options opt; memset(&opt, 0, sizeof(opt)); @@ -157,7 +154,6 @@ static int read_from_tree(const char **pathspec, unsigned char *tree_sha1, opt.output_format = DIFF_FORMAT_CALLBACK; opt.format_callback = update_index_from_diff; - index_fd = hold_locked_index(lock, 1); read_cache(); if (do_diff_cache(tree_sha1, &opt)) return 1; @@ -165,7 +161,7 @@ static int read_from_tree(const char **pathspec, unsigned char *tree_sha1, diff_flush(&opt); diff_tree_release_paths(&opt); - return update_index_refresh(index_fd, lock, refresh_flags); + return 0; } static void set_reflog_message(struct strbuf *sb, const char *action, @@ -322,9 +318,13 @@ int cmd_reset(int argc, const char **argv, const char *prefix) die(_("%s reset is not allowed in a bare repository"), _(reset_type_names[reset_type])); - if (pathspec) - return read_from_tree(pathspec, sha1, - quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN); + if (pathspec) { + struct lock_file *lock = xcalloc(1, sizeof(struct lock_file)); + int index_fd = hold_locked_index(lock, 1); + return read_from_tree(pathspec, sha1) || + update_index_refresh(index_fd, lock, + quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN); + } /* Soft reset does not touch the index file nor the working tree * at all, but requires them in a good order. Other resets reset From 01a19dfc1ac53011deef492b21e52bf7840cef49 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Mon, 14 Jan 2013 21:47:45 -0800 Subject: [PATCH 13/20] reset.c: move lock, write and commit out of update_index_refresh() In preparation for the/a following patch, move the locking, writing and committing of the index file out of update_index_refresh(). The code duplication caused will soon be taken care of. What remains of update_index_refresh() is just one line, but it is still called from two places, so let's leave it for now. In the process, we expose and fix the minor UI bug that makes us print "Could not refresh index" when we fail to write the index file when invoked with a pathspec. Copy the error message from the pathspec-less codepath ("Could not write new index file."). Signed-off-by: Martin von Zweigbergk Signed-off-by: Junio C Hamano --- builtin/reset.c | 34 ++++++++++++++++++---------------- 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/builtin/reset.c b/builtin/reset.c index 70733c271d..c1d6ef2609 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -109,19 +109,10 @@ static void print_new_head_line(struct commit *commit) printf("\n"); } -static int update_index_refresh(int fd, struct lock_file *index_lock, int flags) +static void update_index_refresh(int flags) { - if (!index_lock) { - index_lock = xcalloc(1, sizeof(struct lock_file)); - fd = hold_locked_index(index_lock, 1); - } - refresh_index(&the_index, (flags), NULL, NULL, _("Unstaged changes after reset:")); - if (write_cache(fd, active_cache, active_nr) || - commit_locked_index(index_lock)) - return error ("Could not refresh index"); - return 0; } static void update_index_from_diff(struct diff_queue_struct *q, @@ -321,9 +312,14 @@ int cmd_reset(int argc, const char **argv, const char *prefix) if (pathspec) { struct lock_file *lock = xcalloc(1, sizeof(struct lock_file)); int index_fd = hold_locked_index(lock, 1); - return read_from_tree(pathspec, sha1) || - update_index_refresh(index_fd, lock, - quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN); + if (read_from_tree(pathspec, sha1)) + return 1; + update_index_refresh( + quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN); + if (write_cache(index_fd, active_cache, active_nr) || + commit_locked_index(lock)) + return error("Could not write new index file."); + return 0; } /* Soft reset does not touch the index file nor the working tree @@ -351,9 +347,15 @@ int cmd_reset(int argc, const char **argv, const char *prefix) if (reset_type == HARD && !update_ref_status && !quiet) print_new_head_line(commit); - else if (reset_type == MIXED) /* Report what has not been updated. */ - update_index_refresh(0, NULL, - quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN); + else if (reset_type == MIXED) { /* Report what has not been updated. */ + struct lock_file *index_lock = xcalloc(1, sizeof(struct lock_file)); + int fd = hold_locked_index(index_lock, 1); + update_index_refresh( + quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN); + if (write_cache(fd, active_cache, active_nr) || + commit_locked_index(index_lock)) + error("Could not refresh index"); + } remove_branch_state(); From bc41bf422e15209860bfc6c898dbd3cd89d5da34 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Mon, 14 Jan 2013 21:47:46 -0800 Subject: [PATCH 14/20] reset [--mixed]: only write index file once When doing a mixed reset without paths, the index is locked, read, reset, and written back as part of the actual reset operation (in reset_index()). Then, when showing the list of worktree modifications, we lock the index again, refresh it, and write it. Change this so we only write the index once, making "git reset" a little faster. It does mean that the index lock will be held a little longer, but the difference is small compared to the time spent refreshing the index. There is one minor functional difference: We used to say "Could not write new index file." if the first write failed, and "Could not refresh index" if the second write failed. Now, we will only use the first message. This speeds up "git reset" a little on the linux-2.6 repo (best of five, warm cache): Before After real 0m0.239s 0m0.214s user 0m0.160s 0m0.130s sys 0m0.070s 0m0.080s Signed-off-by: Martin von Zweigbergk Signed-off-by: Junio C Hamano --- builtin/reset.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/builtin/reset.c b/builtin/reset.c index c1d6ef2609..e8a3e41531 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -336,6 +336,11 @@ int cmd_reset(int argc, const char **argv, const char *prefix) err = reset_index(sha1, MIXED, quiet); if (err) die(_("Could not reset index file to revision '%s'."), rev); + + if (reset_type == MIXED) /* Report what has not been updated. */ + update_index_refresh( + quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN); + if (write_cache(newfd, active_cache, active_nr) || commit_locked_index(lock)) die(_("Could not write new index file.")); @@ -347,15 +352,6 @@ int cmd_reset(int argc, const char **argv, const char *prefix) if (reset_type == HARD && !update_ref_status && !quiet) print_new_head_line(commit); - else if (reset_type == MIXED) { /* Report what has not been updated. */ - struct lock_file *index_lock = xcalloc(1, sizeof(struct lock_file)); - int fd = hold_locked_index(index_lock, 1); - update_index_refresh( - quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN); - if (write_cache(fd, active_cache, active_nr) || - commit_locked_index(index_lock)) - error("Could not refresh index"); - } remove_branch_state(); From 3bbf2f20f62ec6aa7998d9f7acc942ded6639870 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Mon, 14 Jan 2013 21:47:47 -0800 Subject: [PATCH 15/20] reset.c: finish entire cmd_reset() whether or not pathspec is given By not returning from inside the "if (pathspec)" block, we can let the pathspec-aware and pathspec-less code share a bit more, making it easier to make future changes that should affect both cases. This also highlights the similarity between read_from_tree() and reset_index(). Signed-off-by: Martin von Zweigbergk Signed-off-by: Junio C Hamano --- builtin/reset.c | 42 ++++++++++++++++++------------------------ 1 file changed, 18 insertions(+), 24 deletions(-) diff --git a/builtin/reset.c b/builtin/reset.c index e8a3e41531..c316d9b589 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -309,19 +309,6 @@ int cmd_reset(int argc, const char **argv, const char *prefix) die(_("%s reset is not allowed in a bare repository"), _(reset_type_names[reset_type])); - if (pathspec) { - struct lock_file *lock = xcalloc(1, sizeof(struct lock_file)); - int index_fd = hold_locked_index(lock, 1); - if (read_from_tree(pathspec, sha1)) - return 1; - update_index_refresh( - quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN); - if (write_cache(index_fd, active_cache, active_nr) || - commit_locked_index(lock)) - return error("Could not write new index file."); - return 0; - } - /* Soft reset does not touch the index file nor the working tree * at all, but requires them in a good order. Other resets reset * the index file to the tree object we are switching to. */ @@ -331,11 +318,16 @@ int cmd_reset(int argc, const char **argv, const char *prefix) if (reset_type != SOFT) { struct lock_file *lock = xcalloc(1, sizeof(struct lock_file)); int newfd = hold_locked_index(lock, 1); - int err = reset_index(sha1, reset_type, quiet); - if (reset_type == KEEP && !err) - err = reset_index(sha1, MIXED, quiet); - if (err) - die(_("Could not reset index file to revision '%s'."), rev); + if (pathspec) { + if (read_from_tree(pathspec, sha1)) + return 1; + } else { + int err = reset_index(sha1, reset_type, quiet); + if (reset_type == KEEP && !err) + err = reset_index(sha1, MIXED, quiet); + if (err) + die(_("Could not reset index file to revision '%s'."), rev); + } if (reset_type == MIXED) /* Report what has not been updated. */ update_index_refresh( @@ -346,14 +338,16 @@ int cmd_reset(int argc, const char **argv, const char *prefix) die(_("Could not write new index file.")); } - /* Any resets update HEAD to the head being switched to, - * saving the previous head in ORIG_HEAD before. */ - update_ref_status = update_refs(rev, sha1); + if (!pathspec) { + /* Any resets without paths update HEAD to the head being + * switched to, saving the previous head in ORIG_HEAD before. */ + update_ref_status = update_refs(rev, sha1); - if (reset_type == HARD && !update_ref_status && !quiet) - print_new_head_line(commit); + if (reset_type == HARD && !update_ref_status && !quiet) + print_new_head_line(commit); - remove_branch_state(); + remove_branch_state(); + } return update_ref_status; } From 7637df131ec9939b737ca284874478cb5c899348 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Mon, 14 Jan 2013 21:47:48 -0800 Subject: [PATCH 16/20] reset.c: inline update_index_refresh() Now that there is only one caller left to the single-line method update_index_refresh(), inline it. Signed-off-by: Martin von Zweigbergk Signed-off-by: Junio C Hamano --- builtin/reset.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/builtin/reset.c b/builtin/reset.c index c316d9b589..520c1a56ea 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -109,12 +109,6 @@ static void print_new_head_line(struct commit *commit) printf("\n"); } -static void update_index_refresh(int flags) -{ - refresh_index(&the_index, (flags), NULL, NULL, - _("Unstaged changes after reset:")); -} - static void update_index_from_diff(struct diff_queue_struct *q, struct diff_options *opt, void *data) { @@ -329,9 +323,11 @@ int cmd_reset(int argc, const char **argv, const char *prefix) die(_("Could not reset index file to revision '%s'."), rev); } - if (reset_type == MIXED) /* Report what has not been updated. */ - update_index_refresh( - quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN); + if (reset_type == MIXED) { /* Report what has not been updated. */ + int flags = quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN; + refresh_index(&the_index, flags, NULL, NULL, + _("Unstaged changes after reset:")); + } if (write_cache(newfd, active_cache, active_nr) || commit_locked_index(lock)) From 2f328c3d2e88230a236e3d84d2bd6de59aea578d Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Mon, 14 Jan 2013 21:47:49 -0800 Subject: [PATCH 17/20] reset $sha1 $pathspec: require $sha1 only to be treeish Resetting with paths does not update HEAD and there is nothing else that a commit should be needed for. Relax the argument parsing so only a tree is required. The sha1 is only passed to read_from_tree(), which already only requires a tree. The "rev" variable we pass to run_add_interactive() will resolve to a tree. This is fine since interactive_reset only needs the parameter to be a treeish and doesn't use it for display purposes. Signed-off-by: Martin von Zweigbergk Signed-off-by: Junio C Hamano --- builtin/reset.c | 48 +++++++++++++++++++++++++++--------------------- t/t7102-reset.sh | 8 ++++++++ 2 files changed, 35 insertions(+), 21 deletions(-) diff --git a/builtin/reset.c b/builtin/reset.c index 520c1a56ea..b776867ad2 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -178,9 +178,10 @@ static const char **parse_args(const char **argv, const char *prefix, const char /* * Possible arguments are: * - * git reset [-opts] ... - * git reset [-opts] -- ... - * git reset [-opts] -- ... + * git reset [-opts] [] + * git reset [-opts] [...] + * git reset [-opts] -- [...] + * git reset [-opts] -- [...] * git reset [-opts] ... * * At this point, argv points immediately after [-opts]. @@ -195,11 +196,13 @@ static const char **parse_args(const char **argv, const char *prefix, const char } /* * Otherwise, argv[0] could be either or and - * has to be unambiguous. + * has to be unambiguous. If there is a single argument, it + * can not be a tree */ - else if (!get_sha1_committish(argv[0], unused)) { + else if ((!argv[1] && !get_sha1_committish(argv[0], unused)) || + (argv[1] && !get_sha1_treeish(argv[0], unused))) { /* - * Ok, argv[0] looks like a rev; it should not + * Ok, argv[0] looks like a commit/tree; it should not * be a filename. */ verify_non_filename(prefix, argv[0]); @@ -241,7 +244,6 @@ int cmd_reset(int argc, const char **argv, const char *prefix) const char *rev; unsigned char sha1[20]; const char **pathspec = NULL; - struct commit *commit; const struct option options[] = { OPT__QUIET(&quiet, N_("be quiet, only report errors")), OPT_SET_INT(0, "mixed", &reset_type, @@ -263,19 +265,23 @@ int cmd_reset(int argc, const char **argv, const char *prefix) PARSE_OPT_KEEP_DASHDASH); pathspec = parse_args(argv, prefix, &rev); - 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); + if (!pathspec) { + struct commit *commit; + if (get_sha1_committish(rev, sha1)) + die(_("Failed to resolve '%s' as a valid revision."), rev); + commit = lookup_commit_reference(sha1); + if (!commit) + die(_("Could not parse object '%s'."), rev); + hashcpy(sha1, commit->object.sha1); + } else { + struct tree *tree; + if (get_sha1_treeish(rev, sha1)) + die(_("Failed to resolve '%s' as a valid tree."), rev); + tree = parse_tree_indirect(sha1); + if (!tree) + die(_("Could not parse object '%s'."), rev); + hashcpy(sha1, tree->object.sha1); + } if (patch_mode) { if (reset_type != NONE) @@ -340,7 +346,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix) update_ref_status = update_refs(rev, sha1); if (reset_type == HARD && !update_ref_status && !quiet) - print_new_head_line(commit); + print_new_head_line(lookup_commit_reference(sha1)); remove_branch_state(); } diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh index 81b2570a3e..1fa2a5fc23 100755 --- a/t/t7102-reset.sh +++ b/t/t7102-reset.sh @@ -497,4 +497,12 @@ test_expect_success 'disambiguation (4)' ' test ! -f secondfile ' +test_expect_success 'reset with paths accepts 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_done From 166ec2e96e31bac913f44823dadd6c732d353172 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Mon, 14 Jan 2013 21:47:50 -0800 Subject: [PATCH 18/20] reset: allow reset on unborn branch Some users seem to think, knowingly or not, that being on an unborn branch is like having a commit with an empty tree checked out, but when run on an unborn branch, "git reset" currently fails with: fatal: Failed to resolve 'HEAD' as a valid ref. Instead of making users figure out that they should run git rm --cached -r . , let's teach "git reset" without a revision argument, when on an unborn branch, to behave as if the user asked to reset to an empty tree. Don't take the analogy with an empty commit too far, though, but still disallow explictly referring to HEAD in "git reset HEAD". Signed-off-by: Martin von Zweigbergk Signed-off-by: Junio C Hamano --- builtin/reset.c | 16 +++++++---- t/t7106-reset-unborn-branch.sh | 52 ++++++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+), 6 deletions(-) create mode 100755 t/t7106-reset-unborn-branch.sh diff --git a/builtin/reset.c b/builtin/reset.c index b776867ad2..45b01ebbcc 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -240,7 +240,7 @@ static int update_refs(const char *rev, const unsigned char *sha1) int cmd_reset(int argc, const char **argv, const char *prefix) { int reset_type = NONE, update_ref_status = 0, quiet = 0; - int patch_mode = 0; + int patch_mode = 0, unborn; const char *rev; unsigned char sha1[20]; const char **pathspec = NULL; @@ -265,7 +265,11 @@ int cmd_reset(int argc, const char **argv, const char *prefix) PARSE_OPT_KEEP_DASHDASH); pathspec = parse_args(argv, prefix, &rev); - if (!pathspec) { + unborn = !strcmp(rev, "HEAD") && get_sha1("HEAD", sha1); + if (unborn) { + /* reset on unborn branch: treat as reset to empty tree */ + hashcpy(sha1, EMPTY_TREE_SHA1_BIN); + } else if (!pathspec) { struct commit *commit; if (get_sha1_committish(rev, sha1)) die(_("Failed to resolve '%s' as a valid revision."), rev); @@ -286,7 +290,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 run_add_interactive(rev, "--patch=reset", pathspec); + return run_add_interactive(sha1_to_hex(sha1), "--patch=reset", pathspec); } /* git reset tree [--] paths... can be used to @@ -340,16 +344,16 @@ int cmd_reset(int argc, const char **argv, const char *prefix) die(_("Could not write new index file.")); } - if (!pathspec) { + if (!pathspec && !unborn) { /* Any resets without paths update HEAD to the head being * switched to, saving the previous head in ORIG_HEAD before. */ update_ref_status = update_refs(rev, sha1); if (reset_type == HARD && !update_ref_status && !quiet) print_new_head_line(lookup_commit_reference(sha1)); - - remove_branch_state(); } + if (!pathspec) + remove_branch_state(); return update_ref_status; } diff --git a/t/t7106-reset-unborn-branch.sh b/t/t7106-reset-unborn-branch.sh new file mode 100755 index 0000000000..8062cf502b --- /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 && + test_must_fail git reset HEAD +' + +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 is a no-op' ' + rm .git/index && + git add a && + git reset --soft + test "$(git ls-files)" = "a" +' + +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 From 3fde386a40f38dbaa684c17603e71909b862d021 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Mon, 14 Jan 2013 21:47:51 -0800 Subject: [PATCH 19/20] reset [--mixed]: use diff-based reset whether or not pathspec was given Thanks to b65982b (Optimize "diff-index --cached" using cache-tree, 2009-05-20), resetting with paths is much faster than resetting without paths. Some timings for the linux-2.6 repo to illustrate this (best of five, warm cache): reset reset . real 0m0.219s 0m0.080s user 0m0.140s 0m0.040s sys 0m0.070s 0m0.030s These two commands should do the same thing, so instead of having the user type the trailing " ." to get the faster do_diff_cache()-based implementation, always use it when doing a mixed reset, with or without paths (so "git reset $rev" would also be faster). Timing "git reset" shows that it indeed becomes as fast as "git reset ." after this patch. Signed-off-by: Martin von Zweigbergk Signed-off-by: Junio C Hamano --- builtin/reset.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/reset.c b/builtin/reset.c index 45b01ebbcc..921afbe62e 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -322,7 +322,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix) if (reset_type != SOFT) { struct lock_file *lock = xcalloc(1, sizeof(struct lock_file)); int newfd = hold_locked_index(lock, 1); - if (pathspec) { + if (reset_type == MIXED) { if (read_from_tree(pathspec, sha1)) return 1; } else { From bf44142f5464e913d99f9544787e59fc630f6cc9 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Wed, 16 Jan 2013 10:00:35 -0800 Subject: [PATCH 20/20] reset: update documentation to require only tree-ish with paths When resetting with paths, we no longer require a commit argument, but only a tree-ish. Update the documentation and synopsis accordingly. Signed-off-by: Martin von Zweigbergk Signed-off-by: Junio C Hamano --- Documentation/git-reset.txt | 18 +++++++++--------- builtin/reset.c | 4 ++-- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt index 978d8da50c..a404b47b7b 100644 --- a/Documentation/git-reset.txt +++ b/Documentation/git-reset.txt @@ -8,20 +8,20 @@ git-reset - Reset current HEAD to the specified state SYNOPSIS -------- [verse] -'git reset' [-q] [] [--] ... -'git reset' (--patch | -p) [] [--] [...] +'git reset' [-q] [] [--] ... +'git reset' (--patch | -p) [] [--] [...] 'git reset' [--soft | --mixed | --hard | --merge | --keep] [-q] [] DESCRIPTION ----------- -In the first and second form, copy entries from to the index. +In the first and second form, copy entries from to the index. In the third form, set the current branch head (HEAD) to , optionally -modifying index and working tree to match. The defaults to HEAD -in all forms. +modifying index and working tree to match. The / defaults +to HEAD in all forms. -'git reset' [-q] [] [--] ...:: +'git reset' [-q] [] [--] ...:: This form resets the index entries for all to their - state at . (It does not affect the working tree, nor + state at . (It does not affect the working tree, nor the current branch.) + This means that `git reset ` is the opposite of `git add @@ -34,9 +34,9 @@ Alternatively, using linkgit:git-checkout[1] and specifying a commit, you can copy the contents of a path out of a commit to the index and to the working tree in one go. -'git reset' (--patch | -p) [] [--] [...]:: +'git reset' (--patch | -p) [] [--] [...]:: Interactively select hunks in the difference between the index - and (defaults to HEAD). The chosen hunks are applied + and (defaults to HEAD). The chosen hunks are applied in reverse to the index. + This means that `git reset -p` is the opposite of `git add -p`, i.e. diff --git a/builtin/reset.c b/builtin/reset.c index 921afbe62e..6032131a90 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -23,8 +23,8 @@ static const char * const git_reset_usage[] = { N_("git reset [--mixed | --soft | --hard | --merge | --keep] [-q] []"), - N_("git reset [-q] [--] ..."), - N_("git reset --patch [] [--] [...]"), + N_("git reset [-q] [--] ..."), + N_("git reset --patch [] [--] [...]"), NULL };