| Message ID | pull.1137.git.1643802721612.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | forbid a hard reset before the initial commit | expand |
"Viaceslavus via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Viacelaus <vaceslavkozin619@gmail.com> > > Performing 'git reset --hard' on empty repo with staged files > may have the only one possible result - deleting all staged files. Sure. It has the only one possible result, which is a sign that the command is well designed to give a robust and predictable end user experience. I know you wanted to say "there is only one possible result, and that result cannot be anything but bad. You Git folks are stupid to design a command that only can have a bad result, so I'll fix that stupidity for you". But the thing is, not everybody agrees with your "deleting all files that added to the index when asked to 'reset --hard' is bad". It is the most obvious way to go back to the "pristine" state, and after all, that is what "reset --hard" is about. Many readers on the list are non-native speakers. You must be careful with your rhetorics, because they often will not be taken in the way you meant them to be taken by them. When you can say "doing X does Y" and convey the core of what you want to say, do so, instead of saying "doing X has only one possible result, which is Y". You may lose the "you Git folks are stupid" part of the message, but you're better off not to sound rude anyway ;-) > Such behaviour may be unexpected or even dangerous. With this > commit, when running 'git reset --hard', git will check for the > existence of commits in the repo; in case of absence of such, and > also if there are any files staged, git will die with an error. This directly contradicts with, and likely will regress the fix made by, what 166ec2e9 (reset: allow reset on unborn branch, 2013-01-14) wanted to do. I do not think we want this change in its current form. When starting a new project on a hosting provider like GitHub these days, you can have them create the initial commit that records the copy of the license file, and the first thing you do on your local machine after leaving the browser to create the repository over there is to clone from it. After that, you'd populate the working tree with the rest of the project files, and record the result. If you say "reset --hard" before committing, you'll equally lose all the newly added files, but because the history is not empty, the approach taken by this patch would not work to protect you, I suspect. It almost always is a mistake to special case an empty repository or an empty history. Having said all that, I am sympathetic to the cause to make it harder to discard a lot of work by mistake. It is just that disabling "reset --hard" only when it is trying to go back to an empty tree is not an effective way to do so. It is even less so when you do not give any escape hatch in case the user knew what they were doing and really meant to go back to the pristine state. Side note. Yes, "git diff --cached | git apply -R --index" or "git rm --cached -r ." as a workaround, but when the user wanted to do "reset --hard", we should have a way to let them do so. Off the top of my head, here are a couple of possible ways to improve the design of this change (note: I am not saying that I'll unconditionally take such a patch that implements any of these): * Detect if we are being interactive, and offer Yes/No choice to give an interactive user a chance to abort when we detect a "risky" situation. Don't do anything if we are not interactive, and don't make it impossible to do things that we may (mis)detect as risky. * Instead of "we are going back to the state without any commit yet", use a better heuristics, such as "we'd lose a newly added path (i.e. the path exists in the index and in the working tree but does not exist in HEAD)" as a sign to flag the situation as possibly risky. Or limit that further to protect only when we'd lose more than N-percent of the paths in the index that way. But both are hard problems. Many existing scripts do rely on "reset --hard" to be a robust and predictable way to go back to the pristine state, and they will be very upset if we misdetect and prompt the user who is not sitting in front of the keyboard.
diff --git a/builtin/reset.c b/builtin/reset.c index b97745ee94e..5a0e80d380f 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -301,6 +301,11 @@ static void die_if_unmerged_cache(int reset_type) } +static int check_commit_exists(const char *refname, const struct object_id *oid, int f, void *d) +{ + return is_branch(refname); +} + static void parse_args(struct pathspec *pathspec, const char **argv, const char *prefix, int patch_mode, @@ -474,6 +479,15 @@ int cmd_reset(int argc, const char **argv, const char *prefix) die(_("Cannot do %s reset with paths."), _(reset_type_names[reset_type])); } + + if (reset_type == HARD) { + int commits_exist = for_each_fullref_in("refs/heads", check_commit_exists, NULL); + if(!commits_exist) { + if(read_cache() == 1) + die(_("Hard reset isn`t allowed before the first commit.")); + } + } + if (reset_type == NONE) reset_type = MIXED; /* by default */ diff --git a/t/t7104-reset-hard.sh b/t/t7104-reset-hard.sh index cf9697eba9a..30fb71e6fb9 100755 --- a/t/t7104-reset-hard.sh +++ b/t/t7104-reset-hard.sh @@ -44,4 +44,22 @@ test_expect_success 'reset --hard did not corrupt index or cache-tree' ' ' +test_expect_success 'reset --hard on empty repo without staged changes works fine' ' + git reset --hard +' + +test_expect_success 'reset --hard on empty repo with staged changes results in an error' ' + touch n && + git add n && + test_must_fail git reet --hard +' + +test_expect_success 'reset --hard after a commit works fine' ' + touch new && + git add new && + git commit -m "initial" && + git reset --hard 2> error && + test_must_be_empty error +' + test_done diff --git a/t/t7106-reset-unborn-branch.sh b/t/t7106-reset-unborn-branch.sh index ecb85c3b823..8d45f640480 100755 --- a/t/t7106-reset-unborn-branch.sh +++ b/t/t7106-reset-unborn-branch.sh @@ -53,15 +53,4 @@ test_expect_success 'reset --soft is a no-op' ' test_cmp expect actual ' -test_expect_success 'reset --hard' ' - rm .git/index && - git add a && - test_when_finished "echo a >a" && - git reset --hard && - - git ls-files >actual && - test_must_be_empty actual && - test_path_is_missing a -' - test_done