{"thread":{"id":"57357","subject":"[PATCH] forbid a hard reset before the initial commit","startedAt":"2022-02-02T11:52:08Z","lastAt":"2022-02-14T11:47:30Z","messageCount":6,"participants":["Viaceslavus via GitGitGadget","Junio C Hamano","Bagas Sanjaya"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"447534","messageId":"pull.1137.git.1643802721612.gitgitgadget@gmail.com","threadId":"57357","inReplyTo":null,"subject":"[PATCH] forbid a hard reset before the initial commit","fromName":"Viaceslavus via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-02T11:52:01Z","receivedAt":"2022-02-02T11:52:08Z","isPatch":true,"sender":{"key":"name:Viaceslavus","avatar":null},"body":"From: Viacelaus \n\nPerforming 'git reset --hard' on empty repo with staged files\nmay have the only one possible result - deleting all staged files.\nSuch behaviour may be unexpected or even dangerous. With this\ncommit, when running 'git reset --hard', git will check for the\nexistence of commits in the repo; in case of absence of such, and\nalso if there are any files staged, git will die with an error.\n\nSigned-off-by: Viacelaus \n---\n Forbid a hard reset on empty repo with staged files.\n \n Performing git reset --hard on empty repo (initialized repository\n without any commits) with staged files may have the only one possible\n result - deleting all staged files. Such behaviour may be unexpected or\n even dangerous, as it is possible to permanently delete the whole\n project. It is also absolutely useless. So with this patch, when running\n git reset --hard, git will check for the existence of commits in the\n repository; in case of absence of such, and also if there are files\n staged, git will return an error. All the tests were added into\n t/t7104-reset-hard.sh file.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1137%2FViaceslavus%2Fhard-reset-safety-on-empty-repo-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1137/Viaceslavus/hard-reset-safety-on-empty-repo-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1137\n\n builtin/reset.c | 14 ++++++++++++++\n t/t7104-reset-hard.sh | 18 ++++++++++++++++++\n t/t7106-reset-unborn-branch.sh | 11 -----------\n 3 files changed, 32 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex b97745ee94e..5a0e80d380f 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -301,6 +301,11 @@ static void die_if_unmerged_cache(int reset_type)\n \n }\n \n+static int check_commit_exists(const char *refname, const struct object_id *oid, int f, void *d)\n+{\n+\treturn is_branch(refname);\n+}\n+\n static void parse_args(struct pathspec *pathspec,\n \t\t const char **argv, const char *prefix,\n \t\t int patch_mode,\n@@ -474,6 +479,15 @@ int cmd_reset(int argc, const char **argv, const char *prefix)\n \t\t\tdie(_(\"Cannot do %s reset with paths.\"),\n \t\t\t\t\t_(reset_type_names[reset_type]));\n \t}\n+\n+\tif (reset_type == HARD) {\n+\t\tint commits_exist = for_each_fullref_in(\"refs/heads\", check_commit_exists, NULL);\n+\t\tif(!commits_exist) {\n+\t\t\tif(read_cache() == 1)\n+\t\t\t\tdie(_(\"Hard reset isn`t allowed before the first commit.\"));\n+\t\t}\n+\t}\n+\n \tif (reset_type == NONE)\n \t\treset_type = MIXED; /* by default */\n \ndiff --git a/t/t7104-reset-hard.sh b/t/t7104-reset-hard.sh\nindex cf9697eba9a..30fb71e6fb9 100755\n--- a/t/t7104-reset-hard.sh\n+++ b/t/t7104-reset-hard.sh\n@@ -44,4 +44,22 @@ test_expect_success 'reset --hard did not corrupt index or cache-tree' '\n \n '\n \n+test_expect_success 'reset --hard on empty repo without staged changes works fine' '\n+\tgit reset --hard\n+'\n+\n+test_expect_success 'reset --hard on empty repo with staged changes results in an error' '\n+\ttouch n &&\n+\tgit add n &&\n+\ttest_must_fail git reet --hard\n+'\n+\n+test_expect_success 'reset --hard after a commit works fine' '\n+\ttouch new &&\n+\tgit add new &&\n+\tgit commit -m \"initial\" &&\n+\tgit reset --hard 2> error &&\n+\ttest_must_be_empty error\n+'\n+\n test_done\ndiff --git a/t/t7106-reset-unborn-branch.sh b/t/t7106-reset-unborn-branch.sh\nindex ecb85c3b823..8d45f640480 100755\n--- a/t/t7106-reset-unborn-branch.sh\n+++ b/t/t7106-reset-unborn-branch.sh\n@@ -53,15 +53,4 @@ test_expect_success 'reset --soft is a no-op' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'reset --hard' '\n-\trm .git/index &&\n-\tgit add a &&\n-\ttest_when_finished \"echo a >a\" &&\n-\tgit reset --hard &&\n-\n-\tgit ls-files >actual &&\n-\ttest_must_be_empty actual &&\n-\ttest_path_is_missing a\n-'\n-\n test_done\n\nbase-commit: 5d01301f2b865aa8dba1654d3f447ce9d21db0b5\n-- \ngitgitgadget\n"},{"id":"447562","messageId":"xmqq4k5hyo76.fsf@gitster.g","threadId":"57357","inReplyTo":"pull.1137.git.1643802721612.gitgitgadget@gmail.com","subject":"Re: [PATCH] forbid a hard reset before the initial commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-02T21:05:01Z","receivedAt":"2022-02-02T21:05:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Viaceslavus via GitGitGadget\" writes:\n\n> From: Viacelaus \n>\n> Performing 'git reset --hard' on empty repo with staged files\n> may have the only one possible result - deleting all staged files.\n\nSure. It has the only one possible result, which is a sign that the\ncommand is well designed to give a robust and predictable end user\nexperience.\n\nI know you wanted to say \"there is only one possible result, and\nthat result cannot be anything but bad. You Git folks are stupid to\ndesign a command that only can have a bad result, so I'll fix that\nstupidity for you\".\n\nBut the thing is, not everybody agrees with your \"deleting all files\nthat added to the index when asked to 'reset --hard' is bad\". It is\nthe most obvious way to go back to the \"pristine\" state, and after\nall, that is what \"reset --hard\" is about.\n\nMany readers on the list are non-native speakers. You must be\ncareful with your rhetorics, because they often will not be taken in\nthe way you meant them to be taken by them. When you can say \"doing\nX does Y\" and convey the core of what you want to say, do so,\ninstead of saying \"doing X has only one possible result, which is\nY\". You may lose the \"you Git folks are stupid\" part of the message,\nbut you're better off not to sound rude anyway ;-)\n\n> Such behaviour may be unexpected or even dangerous. With this\n> commit, when running 'git reset --hard', git will check for the\n> existence of commits in the repo; in case of absence of such, and\n> also if there are any files staged, git will die with an error.\n\nThis directly contradicts with, and likely will regress the fix made\nby, what 166ec2e9 (reset: allow reset on unborn branch, 2013-01-14)\nwanted to do. I do not think we want this change in its current\nform.\n\nWhen starting a new project on a hosting provider like GitHub these\ndays, you can have them create the initial commit that records the\ncopy of the license file, and the first thing you do on your local\nmachine after leaving the browser to create the repository over\nthere is to clone from it. After that, you'd populate the working\ntree with the rest of the project files, and record the result. If\nyou say \"reset --hard\" before committing, you'll equally lose all\nthe newly added files, but because the history is not empty, the\napproach taken by this patch would not work to protect you, I\nsuspect. It almost always is a mistake to special case an empty\nrepository or an empty history.\n\nHaving said all that, I am sympathetic to the cause to make it\nharder to discard a lot of work by mistake. It is just that\ndisabling \"reset --hard\" only when it is trying to go back to an\nempty tree is not an effective way to do so. It is even less so\nwhen you do not give any escape hatch in case the user knew what\nthey were doing and really meant to go back to the pristine state.\n\n Side note. Yes, \"git diff --cached | git apply -R --index\" or\n \"git rm --cached -r .\" as a workaround, but when the user wanted\n to do \"reset --hard\", we should have a way to let them do so.\n\nOff the top of my head, here are a couple of possible ways to\nimprove the design of this change (note: I am not saying that I'll\nunconditionally take such a patch that implements any of these):\n\n * Detect if we are being interactive, and offer Yes/No choice to\n give an interactive user a chance to abort when we detect a\n \"risky\" situation. Don't do anything if we are not interactive,\n and don't make it impossible to do things that we may (mis)detect\n as risky.\n\n * Instead of \"we are going back to the state without any commit\n yet\", use a better heuristics, such as \"we'd lose a newly added\n path (i.e. the path exists in the index and in the working tree\n but does not exist in HEAD)\" as a sign to flag the situation as\n possibly risky. Or limit that further to protect only when we'd\n lose more than N-percent of the paths in the index that way.\n\nBut both are hard problems.\n\nMany existing scripts do rely on \"reset --hard\" to be a robust and\npredictable way to go back to the pristine state, and they will be\nvery upset if we misdetect and prompt the user who is not sitting in\nfront of the keyboard.\n"},{"id":"448288","messageId":"pull.1137.v2.git.1644618404948.gitgitgadget@gmail.com","threadId":"57357","inReplyTo":"pull.1137.git.1643802721612.gitgitgadget@gmail.com","subject":"[PATCH v2] hard reset safe mode","fromName":"Viaceslavus via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-11T22:26:44Z","receivedAt":"2022-02-11T22:26:51Z","isPatch":true,"sender":{"key":"name:Viaceslavus","avatar":null},"body":"From: Viacelaus \n\nThe power of hard reset may frequently be inconvinient for a common user. So\nthis is an implementation of safe mode for hard reset. It can be switched on\nby setting 'reset.safe' config variable to true. When running 'reset --hard'\nwith 'reset.safe' enabled git will check if there are any staged changes\nthat may be discarded by this reset. If there is a chance of deleting the\nchanges, git will ask the user for a confirmation with Yes/No choice.\n\nSigned-off-by: Viacelaus \n---\n Hard reset safe mode\n \n Preventing unexpected code-deletion with hard reset 'safe mode'.\n \n Considering the recomendations for patch v1 and in order to preserve the\n current robustness of hard reset, I have made the following\n modifications to the original version (which has completely disallowed\n hard reset on unborn branch with staged files):\n \n Changes since v1:\n \n * Described security measures aren't enabled by default. Safe mode can\n be turned on with 'reset.safe' config variable.\n * Replaced the resulting error with Yes/No choice so hard reset on\n unborn branch isn`t impossible now.\n * Detection of staged changes that are going to be deleted by the reset\n isn't limited to unborn-branch state now. Git will warn you and ask\n for a confirmation if there are commits on the branch also.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1137%2FViaceslavus%2Fhard-reset-safety-on-empty-repo-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1137/Viaceslavus/hard-reset-safety-on-empty-repo-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1137\n\nRange-diff vs v1:\n\n 1: 13cc01b7de7 ! 1: e6eec1bfda2 forbid a hard reset before the initial commit\n @@ Metadata\n Author: Viacelaus \n \n ## Commit message ##\n - forbid a hard reset before the initial commit\n + hard reset safe mode\n \n - Performing 'git reset --hard' on empty repo with staged files\n - may have the only one possible result - deleting all staged files.\n - Such behaviour may be unexpected or even dangerous. With this\n - commit, when running 'git reset --hard', git will check for the\n - existence of commits in the repo; in case of absence of such, and\n - also if there are any files staged, git will die with an error.\n + The power of hard reset may frequently be inconvinient for a common user. So\n + this is an implementation of safe mode for hard reset. It can be switched on\n + by setting 'reset.safe' config variable to true. When running 'reset --hard'\n + with 'reset.safe' enabled git will check if there are any staged changes\n + that may be discarded by this reset. If there is a chance of deleting the\n + changes, git will ask the user for a confirmation with Yes/No choice.\n \n Signed-off-by: Viacelaus \n \n ## builtin/reset.c ##\n +@@\n + #include \"submodule.h\"\n + #include \"submodule-config.h\"\n + #include \"dir.h\"\n ++#include \"wt-status.h\"\n + \n + #define REFRESH_INDEX_DELAY_WARNING_IN_MS (2 * 1000)\n + \n @@ builtin/reset.c: static void die_if_unmerged_cache(int reset_type)\n \n }\n @@ builtin/reset.c: static void die_if_unmerged_cache(int reset_type)\n +{\n +\treturn is_branch(refname);\n +}\n ++\n ++static void accept_discarding_changes(void) {\n ++\tint answer = getc(stdin);\n ++\tprintf(_(\"Some staged changes may be discarded by this reset. Continue? [Y/n]\"));\n ++\n ++\tif (answer != 'y' && answer != 'Y') {\n ++\t\tprintf(_(\"aborted\\n\"));\n ++\t\texit(1);\n ++\t}\n ++}\n ++\n ++static void detect_risky_reset(int commits_exist) {\n ++\tint cache = read_cache();\n ++\tif(!commits_exist) {\n ++\t\tif(cache == 1) {\n ++\t\t\taccept_discarding_changes();\n ++\t\t}\n ++\t}\n ++\telse {\n ++\t\tif(has_uncommitted_changes(the_repository, 1)) {\n ++\t\t\taccept_discarding_changes();\n ++\t\t}\n ++\t}\n ++}\n +\n static void parse_args(struct pathspec *pathspec,\n \t\t const char **argv, const char *prefix,\n @@ builtin/reset.c: int cmd_reset(int argc, const char **argv, const char *prefix)\n \t}\n +\n +\tif (reset_type == HARD) {\n -+\t\tint commits_exist = for_each_fullref_in(\"refs/heads\", check_commit_exists, NULL);\n -+\t\tif(!commits_exist) {\n -+\t\t\tif(read_cache() == 1)\n -+\t\t\t\tdie(_(\"Hard reset isn`t allowed before the first commit.\"));\n ++\t\tint safe = 0;\n ++\t\tgit_config_get_bool(\"reset.safe\", &safe);\n ++\t\tif (safe) {\n ++\t\t\tint commits_exist = for_each_fullref_in(\"refs/heads\", check_commit_exists, NULL);\n ++\t\t\tdetect_risky_reset(commits_exist);\n +\t\t}\n +\t}\n +\n @@ t/t7104-reset-hard.sh: test_expect_success 'reset --hard did not corrupt index o\n \n '\n \n -+test_expect_success 'reset --hard on empty repo without staged changes works fine' '\n -+\tgit reset --hard\n ++test_expect_success 'reset --hard in safe mode on unborn branch with staged files results in a warning' '\n ++\tgit config reset.safe on &&\n ++\ttouch a &&\n ++\tgit add a &&\n ++\ttest_must_fail git reset --hard\n ++\n +'\n +\n -+test_expect_success 'reset --hard on empty repo with staged changes results in an error' '\n -+\ttouch n &&\n -+\tgit add n &&\n -+\ttest_must_fail git reet --hard\n ++test_expect_success 'reset --hard in safe mode after a commit without staged changes works fine' '\n ++\tgit config reset.safe on &&\n ++\ttouch b &&\n ++\tgit add b &&\n ++\tgit commit -m \"initial\" &&\n ++\tgit reset --hard\n ++\n +'\n +\n -+test_expect_success 'reset --hard after a commit works fine' '\n -+\ttouch new &&\n -+\tgit add new &&\n ++test_expect_success 'reset --hard in safe mode after a commit with staged changes results in a warning' '\n ++\tgit config reset.safe on &&\n ++\ttouch c d &&\n ++\tgit add c &&\n +\tgit commit -m \"initial\" &&\n -+\tgit reset --hard 2> error &&\n -+\ttest_must_be_empty error\n ++\tgit add d &&\n ++\ttest_must_fail git reset --hard\n ++\n +'\n +\n test_done\n -\n - ## t/t7106-reset-unborn-branch.sh ##\n -@@ t/t7106-reset-unborn-branch.sh: test_expect_success 'reset --soft is a no-op' '\n - \ttest_cmp expect actual\n - '\n - \n --test_expect_success 'reset --hard' '\n --\trm .git/index &&\n --\tgit add a &&\n --\ttest_when_finished \"echo a >a\" &&\n --\tgit reset --hard &&\n --\n --\tgit ls-files >actual &&\n --\ttest_must_be_empty actual &&\n --\ttest_path_is_missing a\n --'\n --\n - test_done\n\n\n builtin/reset.c | 40 ++++++++++++++++++++++++++++++++++++++++\n t/t7104-reset-hard.sh | 27 +++++++++++++++++++++++++++\n 2 files changed, 67 insertions(+)\n\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex b97745ee94e..997e2adf8d8 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -26,6 +26,7 @@\n #include \"submodule.h\"\n #include \"submodule-config.h\"\n #include \"dir.h\"\n+#include \"wt-status.h\"\n \n #define REFRESH_INDEX_DELAY_WARNING_IN_MS (2 * 1000)\n \n@@ -301,6 +302,35 @@ static void die_if_unmerged_cache(int reset_type)\n \n }\n \n+static int check_commit_exists(const char *refname, const struct object_id *oid, int f, void *d)\n+{\n+\treturn is_branch(refname);\n+}\n+\n+static void accept_discarding_changes(void) {\n+\tint answer = getc(stdin);\n+\tprintf(_(\"Some staged changes may be discarded by this reset. Continue? [Y/n]\"));\n+\n+\tif (answer != 'y' && answer != 'Y') {\n+\t\tprintf(_(\"aborted\\n\"));\n+\t\texit(1);\n+\t}\n+}\n+\n+static void detect_risky_reset(int commits_exist) {\n+\tint cache = read_cache();\n+\tif(!commits_exist) {\n+\t\tif(cache == 1) {\n+\t\t\taccept_discarding_changes();\n+\t\t}\n+\t}\n+\telse {\n+\t\tif(has_uncommitted_changes(the_repository, 1)) {\n+\t\t\taccept_discarding_changes();\n+\t\t}\n+\t}\n+}\n+\n static void parse_args(struct pathspec *pathspec,\n \t\t const char **argv, const char *prefix,\n \t\t int patch_mode,\n@@ -474,6 +504,16 @@ int cmd_reset(int argc, const char **argv, const char *prefix)\n \t\t\tdie(_(\"Cannot do %s reset with paths.\"),\n \t\t\t\t\t_(reset_type_names[reset_type]));\n \t}\n+\n+\tif (reset_type == HARD) {\n+\t\tint safe = 0;\n+\t\tgit_config_get_bool(\"reset.safe\", &safe);\n+\t\tif (safe) {\n+\t\t\tint commits_exist = for_each_fullref_in(\"refs/heads\", check_commit_exists, NULL);\n+\t\t\tdetect_risky_reset(commits_exist);\n+\t\t}\n+\t}\n+\n \tif (reset_type == NONE)\n \t\treset_type = MIXED; /* by default */\n \ndiff --git a/t/t7104-reset-hard.sh b/t/t7104-reset-hard.sh\nindex cf9697eba9a..c962c706bed 100755\n--- a/t/t7104-reset-hard.sh\n+++ b/t/t7104-reset-hard.sh\n@@ -44,4 +44,31 @@ test_expect_success 'reset --hard did not corrupt index or cache-tree' '\n \n '\n \n+test_expect_success 'reset --hard in safe mode on unborn branch with staged files results in a warning' '\n+\tgit config reset.safe on &&\n+\ttouch a &&\n+\tgit add a &&\n+\ttest_must_fail git reset --hard\n+\n+'\n+\n+test_expect_success 'reset --hard in safe mode after a commit without staged changes works fine' '\n+\tgit config reset.safe on &&\n+\ttouch b &&\n+\tgit add b &&\n+\tgit commit -m \"initial\" &&\n+\tgit reset --hard\n+\n+'\n+\n+test_expect_success 'reset --hard in safe mode after a commit with staged changes results in a warning' '\n+\tgit config reset.safe on &&\n+\ttouch c d &&\n+\tgit add c &&\n+\tgit commit -m \"initial\" &&\n+\tgit add d &&\n+\ttest_must_fail git reset --hard\n+\n+'\n+\n test_done\n\nbase-commit: 5d01301f2b865aa8dba1654d3f447ce9d21db0b5\n-- \ngitgitgadget\n"},{"id":"448290","messageId":"xmqqee4980qz.fsf@gitster.g","threadId":"57357","inReplyTo":"pull.1137.v2.git.1644618404948.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] hard reset safe mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-11T23:03:16Z","receivedAt":"2022-02-11T23:03:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Viaceslavus via GitGitGadget\" writes:\n\n> From: Viacelaus \n>\n> The power of hard reset may frequently be inconvinient for a common user. So\n> this is an implementation of safe mode for hard reset. It can be switched on\n> by setting 'reset.safe' config variable to true. When running 'reset --hard'\n> with 'reset.safe' enabled git will check if there are any staged changes\n> that may be discarded by this reset. If there is a chance of deleting the\n> changes, git will ask the user for a confirmation with Yes/No choice.\n\nThere needs an explanation on how this avoids breaking scripts that\ntrust that \"git reset --hard HEAD\" reliably matches the index and\nthe working tree files to what is recorded in HEAD without getting\nstuck waiting for any user input. \"They can turn off reset.safe\" is\nnot an acceptable answer.\n\n> +static int check_commit_exists(const char *refname, const struct object_id *oid, int f, void *d)\n> +{\n> +\treturn is_branch(refname);\n> +}\n\nThe returned value from a for_each_ref() callback is used to tell\nthe caller \"stop here, no need to further iterate and call me with\nother refs\". I think this wants to say \"if I ever get called even\nonce, tell the caller to stop, so that it can tell its caller that\nit was stopped\".\n\n> +static void accept_discarding_changes(void) {\n> +\tint answer = getc(stdin);\n> +\tprintf(_(\"Some staged changes may be discarded by this reset. Continue? [Y/n]\"));\n> +\n> +\tif (answer != 'y' && answer != 'Y') {\n> +\t\tprintf(_(\"aborted\\n\"));\n> +\t\texit(1);\n> +\t}\n> +}\n\nI'd think at least we should use git_prompt(), instead of\nhand-rolled prompt routine like this one that assumes that an\nend-user is sitting in front of the terminal waiting to be prompted.\n\nIf updating \"git reset\" like this patch does were a good idea to\nbegin with, that is.\n\n> +static void detect_risky_reset(int commits_exist) {\n> +\tint cache = read_cache();\n> +\tif(!commits_exist) {\n> +\t\tif(cache == 1) {\n> +\t\t\taccept_discarding_changes();\n> +\t\t}\n> +\t}\n> +\telse {\n> +\t\tif(has_uncommitted_changes(the_repository, 1)) {\n> +\t\t\taccept_discarding_changes();\n> +\t\t}\n> +\t}\n> +}\n\nStyle (too many to list---see Documentation/CodingGuidelines).\n\n> +\tif (reset_type == HARD) {\n> +\t\tint safe = 0;\n> +\t\tgit_config_get_bool(\"reset.safe\", &safe);\n> +\t\tif (safe) {\n> +\t\t\tint commits_exist = for_each_fullref_in(\"refs/heads\", check_commit_exists, NULL);\n\nThe callback is called for each and every ref inside \"refs/heads/\",\nso by definition, shouldn't any of them pass \"is_branch(refname)\"?\n\nIn any case, why does this have to be done by the caller? If the\nhelper claims to be capable of detecting a \"risky reset\" (if such a\nthing exists, that is), and if the helper behaves differently when\nthere is any commit on any branch or not as its implementation\ndetail, shouldn't it figure out if there is a commit _inside_ the\nhelper itself, not forcing the caller to compute it for it?\n\n> +\t\t\tdetect_risky_reset(commits_exist);\n> +\t\t}\n> +\t}\n> +\n> \tif (reset_type == NONE)\n> \t\treset_type = MIXED; /* by default */\n> \n\n> diff --git a/t/t7104-reset-hard.sh b/t/t7104-reset-hard.sh\n> index cf9697eba9a..c962c706bed 100755\n> --- a/t/t7104-reset-hard.sh\n> +++ b/t/t7104-reset-hard.sh\n> @@ -44,4 +44,31 @@ test_expect_success 'reset --hard did not corrupt index or cache-tree' '\n> \n> '\n> \n> +test_expect_success 'reset --hard in safe mode on unborn branch with staged files results in a warning' '\n> +\tgit config reset.safe on &&\n\nUse either \"test_when_finished\" or \"test_config\", which is a good\nway to isolate each test from side effects of running the test that\ncomes before it.\n"},{"id":"448347","messageId":"c81e1226-2184-0f16-b81d-c11d894b4654@gmail.com","threadId":"57357","inReplyTo":"xmqqee4980qz.fsf@gitster.g","subject":"Re: [PATCH v2] hard reset safe mode","fromName":"Bagas Sanjaya","fromEmail":"bagasdotme@gmail.com","sentAt":"2022-02-14T03:14:29Z","receivedAt":"2022-02-14T03:14:52Z","isPatch":true,"sender":{"key":"bagasdotme@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40219486?v=4"},"body":"On 12/02/22 06.03, Junio C Hamano wrote:\n> \"Viaceslavus via GitGitGadget\" writes:\n> \n>> From: Viacelaus \n>>\n>> The power of hard reset may frequently be inconvinient for a common user. So\n>> this is an implementation of safe mode for hard reset. It can be switched on\n>> by setting 'reset.safe' config variable to true. When running 'reset --hard'\n>> with 'reset.safe' enabled git will check if there are any staged changes\n>> that may be discarded by this reset. If there is a chance of deleting the\n>> changes, git will ask the user for a confirmation with Yes/No choice.\n> \n> There needs an explanation on how this avoids breaking scripts that\n> trust that \"git reset --hard HEAD\" reliably matches the index and\n> the working tree files to what is recorded in HEAD without getting\n> stuck waiting for any user input. \"They can turn off reset.safe\" is\n> not an acceptable answer.\n> \n>> +static int check_commit_exists(const char *refname, const struct object_id *oid, int f, void *d)\n>> +{\n>> +\treturn is_branch(refname);\n>> +}\n> \n> The returned value from a for_each_ref() callback is used to tell\n> the caller \"stop here, no need to further iterate and call me with\n> other refs\". I think this wants to say \"if I ever get called even\n> once, tell the caller to stop, so that it can tell its caller that\n> it was stopped\".\n> \n>> +static void accept_discarding_changes(void) {\n>> +\tint answer = getc(stdin);\n>> +\tprintf(_(\"Some staged changes may be discarded by this reset. Continue? [Y/n]\"));\n>> +\n>> +\tif (answer != 'y' && answer != 'Y') {\n>> +\t\tprintf(_(\"aborted\\n\"));\n>> +\t\texit(1);\n>> +\t}\n>> +}\n> \n> I'd think at least we should use git_prompt(), instead of\n> hand-rolled prompt routine like this one that assumes that an\n> end-user is sitting in front of the terminal waiting to be prompted.\n> \n> If updating \"git reset\" like this patch does were a good idea to\n> begin with, that is.\n> \n\nI think it's better just to error out when there are staged changes.\nThe message will be something like:\n\"error: there are staged changes that will be hard-reset. Please\ncommit or stash them before resetting. If you want to remove all\nchanges from index, you can do so by running 'git reset'.\"\n\n-- \nAn old man doll... just what I always wanted! - Clara\n"},{"id":"448353","messageId":"xmqqleyd4qqu.fsf@gitster.g","threadId":"57357","inReplyTo":"c81e1226-2184-0f16-b81d-c11d894b4654@gmail.com","subject":"Re: [PATCH v2] hard reset safe mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-14T11:44:25Z","receivedAt":"2022-02-14T11:47:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Bagas Sanjaya writes:\n\n> I think it's better just to error out when there are staged changes.\n> The message will be something like:\n> \"error: there are staged changes that will be hard-reset. Please\n> commit or stash them before resetting. If you want to remove all\n> changes from index, you can do so by running 'git reset'.\"\n\nI do prefer not to have interactive prompt at all, as we _will_ get\nit wrong when we shouldn't be doing the warning or prompting (i.e.\nwhen no user is sitting in front of the terminal).\n\nAnother thing that I would be very opposed to in the posted patch is\nthe special casing of an initial commit. If typing 10,000 lines of\nmaterial to create a new file, doing \"git add\", and changing mind\nand saying \"git reset --hard\" to lose the new file is painful before\ncreating the first commit, doing exactly the same in a project after\n10 years with 10,000 commits will hurt exactly the same way. You'd\n\"lose\" the 10,000 lines of material you prepared to the \"accident\"\neither way.\n\nOne more thing the posted patch is designed in a wrong way is that\nit cares about the state of _other_ branches; even if you \"git reset\n--hard\" on an unborn branch, if there are some commits on other\nbranches that has nothing to do with your current state, it will do\nwhat it was told to do regardless of \"safe mode\". The \"damage\"\ncaused by such an \"accident\" would be the same for the end-user,\nwhether there is an unrelated branch with a commit or there isn't.\nYou'd \"lose\" the 10,000 lines of material you prepared to the\n\"accident\" regardless.\n\nSo, I am strongly opposed to the posted patch. \"git reset --hard\"\nis designed to bring the index and the working tree to the pristine\nstate in a predictable way, and making it work differently depending\non a setting, or if it is on an unborn branch, or if there are\ncommits on any branch, goes very against its spirit.\n\nAfter all, \"reset --hard\" is called *HARD* for a reason. It is not\n\"unsafe\". It is designed to give you pristine state no matter what\nand it tries very hard to do so, by even removing an untracked file\nthat is squatting the path at which the pristine state wants a\ndirectory.\n\nIn other words, those who type \"git reset --hard\" WITHOUT wishing to\ngo back to the pristine state by clearing the deck may be using a\nwrong command.\n\nI wonder if \"git reset\" (i.e. mixed, not hard) is what they want to\nuse, instead. It is the command to use when: \"I've made a mess in\nthe index after doing 'add -p' and other things, and want to get the\nindex into pristine state, while keeping the working tree files\".\n\nAfter doing so, the first thing they can do is to \"git diff\" to\ncompare what they truly do not want to have anywhere and \"git\ncheckout -- \" to update the whole file to the state in the\nindex, or \"git checkout -p -- \" to update them piecemeal.\n\nOr perhaps they want \"git stash\". That would clear the deck and\ngive them the pristine state justlike \"git reset --hard\" would, but\nif they found that they did so by mistake, they can immediately\nunstash.\n\nHaving said all that, because \"reset\" is mostly about the index and\nnot about the working tree (the core of the Git's philosophy is that\nthe index is all that matters and the working tree files are just\nephemeral means to the end, which is to update the index with good\ncontents and to make tree out of it), I can see us adding another\nmode, perhaps called \"reset --clear\", that is almost like \"reset\n--hard\" with one distinction, for end-user's interactive use.\n\nIf you did this sequence:\n\n $ git reset --hard ;# start from a pristine state\n\n $ date >new-file-a ;# create a new file\n $ date >new-file-b ;# create another new file\n $ git add new-file-a ;# just add one of them\n\n $ git reset --hard ;# get back to the pristine state\n\nthe second reset will make sure that neither of these two files are\nin the index, but it also removes only one of the files in the\nworking tree, while leaving the other file in the working tree.\n\nThe fact that new-file-a has been \"git add\"ed means that after\nlosing new-file-a, its contents *can* be recovered (i.e. it is one\nof the loose objects that is no longer reachable, and \"git fsck\n--lost-found\" could theoretically give it back to you).\n\nBut new-file-b has never been added, so losing it is a more grave\nloss. So the behaviour to remove new-file-a that was in the index\nwhile leaving new-file-b alone is somewhat justifiable, but not by a\nlarge margin. A new \"reset --clear\" mode _could_ make paths in the\nindex that do not appear in the resetted-to treeish (either HEAD or\nan empty tree if the branch is unborn) untracked, while leaving their\nworking tree files alone.\n"}]}