All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] forbid a hard reset before the initial commit
@ 2022-02-02 11:52 Viaceslavus via GitGitGadget
  2022-02-02 21:05 ` Junio C Hamano
  2022-02-11 22:26 ` [PATCH v2] hard reset safe mode Viaceslavus via GitGitGadget
  0 siblings, 2 replies; 6+ messages in thread
From: Viaceslavus via GitGitGadget @ 2022-02-02 11:52 UTC (permalink / raw)
  To: git; +Cc: Viaceslavus, Viacelaus

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.
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.

Signed-off-by: Viacelaus <vaceslavkozin619@gmail.com>
---
    Forbid a hard reset on empty repo with staged files.
    
    Performing git reset --hard on empty repo (initialized repository
    without any commits) with staged files may have the only one possible
    result - deleting all staged files. Such behaviour may be unexpected or
    even dangerous, as it is possible to permanently delete the whole
    project. It is also absolutely useless. So with this patch, when running
    git reset --hard, git will check for the existence of commits in the
    repository; in case of absence of such, and also if there are files
    staged, git will return an error. All the tests were added into
    t/t7104-reset-hard.sh file.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1137%2FViaceslavus%2Fhard-reset-safety-on-empty-repo-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1137/Viaceslavus/hard-reset-safety-on-empty-repo-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1137

 builtin/reset.c                | 14 ++++++++++++++
 t/t7104-reset-hard.sh          | 18 ++++++++++++++++++
 t/t7106-reset-unborn-branch.sh | 11 -----------
 3 files changed, 32 insertions(+), 11 deletions(-)

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

base-commit: 5d01301f2b865aa8dba1654d3f447ce9d21db0b5
-- 
gitgitgadget

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] forbid a hard reset before the initial commit
  2022-02-02 11:52 [PATCH] forbid a hard reset before the initial commit Viaceslavus via GitGitGadget
@ 2022-02-02 21:05 ` Junio C Hamano
  2022-02-11 22:26 ` [PATCH v2] hard reset safe mode Viaceslavus via GitGitGadget
  1 sibling, 0 replies; 6+ messages in thread
From: Junio C Hamano @ 2022-02-02 21:05 UTC (permalink / raw)
  To: Viaceslavus via GitGitGadget; +Cc: git, Viaceslavus

"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.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v2] hard reset safe mode
  2022-02-02 11:52 [PATCH] forbid a hard reset before the initial commit Viaceslavus via GitGitGadget
  2022-02-02 21:05 ` Junio C Hamano
@ 2022-02-11 22:26 ` Viaceslavus via GitGitGadget
  2022-02-11 23:03   ` Junio C Hamano
  1 sibling, 1 reply; 6+ messages in thread
From: Viaceslavus via GitGitGadget @ 2022-02-11 22:26 UTC (permalink / raw)
  To: git; +Cc: Viaceslavus, Viacelaus

From: Viacelaus <vaceslavkozin619@gmail.com>

The power of hard reset may frequently be inconvinient for a common user. So
this is an implementation of safe mode for hard reset. It can be switched on
by setting 'reset.safe' config variable to true. When running 'reset --hard'
with 'reset.safe' enabled git will check if there are any staged changes
that may be discarded by this reset. If there is a chance of deleting the
changes, git will ask the user for a confirmation with Yes/No choice.

Signed-off-by: Viacelaus <vaceslavkozin619@gmail.com>
---
    Hard reset safe mode
    
    Preventing unexpected code-deletion with hard reset 'safe mode'.
    
    Considering the recomendations for patch v1 and in order to preserve the
    current robustness of hard reset, I have made the following
    modifications to the original version (which has completely disallowed
    hard reset on unborn branch with staged files):
    
    Changes since v1:
    
     * Described security measures aren't enabled by default. Safe mode can
       be turned on with 'reset.safe' config variable.
     * Replaced the resulting error with Yes/No choice so hard reset on
       unborn branch isn`t impossible now.
     * Detection of staged changes that are going to be deleted by the reset
       isn't limited to unborn-branch state now. Git will warn you and ask
       for a confirmation if there are commits on the branch also.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1137%2FViaceslavus%2Fhard-reset-safety-on-empty-repo-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1137/Viaceslavus/hard-reset-safety-on-empty-repo-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1137

Range-diff vs v1:

 1:  13cc01b7de7 ! 1:  e6eec1bfda2 forbid a hard reset before the initial commit
     @@ Metadata
      Author: Viacelaus <vaceslavkozin619@gmail.com>
      
       ## Commit message ##
     -    forbid a hard reset before the initial commit
     +    hard reset safe mode
      
     -    Performing 'git reset --hard' on empty repo with staged files
     -    may have the only one possible result - deleting all staged files.
     -    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.
     +    The power of hard reset may frequently be inconvinient for a common user. So
     +    this is an implementation of safe mode for hard reset. It can be switched on
     +    by setting 'reset.safe' config variable to true. When running 'reset --hard'
     +    with 'reset.safe' enabled git will check if there are any staged changes
     +    that may be discarded by this reset. If there is a chance of deleting the
     +    changes, git will ask the user for a confirmation with Yes/No choice.
      
          Signed-off-by: Viacelaus <vaceslavkozin619@gmail.com>
      
       ## builtin/reset.c ##
     +@@
     + #include "submodule.h"
     + #include "submodule-config.h"
     + #include "dir.h"
     ++#include "wt-status.h"
     + 
     + #define REFRESH_INDEX_DELAY_WARNING_IN_MS (2 * 1000)
     + 
      @@ builtin/reset.c: static void die_if_unmerged_cache(int reset_type)
       
       }
     @@ builtin/reset.c: static void die_if_unmerged_cache(int reset_type)
      +{
      +	return is_branch(refname);
      +}
     ++
     ++static void accept_discarding_changes(void) {
     ++	int answer = getc(stdin);
     ++	printf(_("Some staged changes may be discarded by this reset. Continue? [Y/n]"));
     ++
     ++	if (answer != 'y' && answer != 'Y') {
     ++		printf(_("aborted\n"));
     ++		exit(1);
     ++	}
     ++}
     ++
     ++static void detect_risky_reset(int commits_exist) {
     ++	int cache = read_cache();
     ++	if(!commits_exist) {
     ++		if(cache == 1) {
     ++			accept_discarding_changes();
     ++		}
     ++	}
     ++	else {
     ++		if(has_uncommitted_changes(the_repository, 1)) {
     ++			accept_discarding_changes();
     ++		}
     ++	}
     ++}
      +
       static void parse_args(struct pathspec *pathspec,
       		       const char **argv, const char *prefix,
     @@ builtin/reset.c: int cmd_reset(int argc, const char **argv, const char *prefix)
       	}
      +
      +	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."));
     ++		int safe = 0;
     ++		git_config_get_bool("reset.safe", &safe);
     ++		if (safe) {
     ++			int commits_exist = for_each_fullref_in("refs/heads", check_commit_exists, NULL);
     ++			detect_risky_reset(commits_exist);
      +		}
      +	}
      +
     @@ t/t7104-reset-hard.sh: test_expect_success 'reset --hard did not corrupt index o
       
       '
       
     -+test_expect_success 'reset --hard on empty repo without staged changes works fine' '
     -+	git reset --hard
     ++test_expect_success 'reset --hard in safe mode on unborn branch with staged files results in a warning' '
     ++	git config reset.safe on &&
     ++	touch a &&
     ++	git add a &&
     ++	test_must_fail 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 in safe mode after a commit without staged changes works fine' '
     ++	git config reset.safe on &&
     ++	touch b &&
     ++	git add b &&
     ++	git commit -m "initial" &&
     ++	git reset --hard
     ++
      +'
      +
     -+test_expect_success 'reset --hard after a commit works fine' '
     -+	touch new &&
     -+	git add new &&
     ++test_expect_success 'reset --hard in safe mode after a commit with staged changes results in a warning' '
     ++	git config reset.safe on &&
     ++	touch c d &&
     ++	git add c &&
      +	git commit -m "initial" &&
     -+	git reset --hard 2> error &&
     -+	test_must_be_empty error
     ++	git add d &&
     ++	test_must_fail git reset --hard
     ++
      +'
      +
       test_done
     -
     - ## t/t7106-reset-unborn-branch.sh ##
     -@@ t/t7106-reset-unborn-branch.sh: 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


 builtin/reset.c       | 40 ++++++++++++++++++++++++++++++++++++++++
 t/t7104-reset-hard.sh | 27 +++++++++++++++++++++++++++
 2 files changed, 67 insertions(+)

diff --git a/builtin/reset.c b/builtin/reset.c
index b97745ee94e..997e2adf8d8 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -26,6 +26,7 @@
 #include "submodule.h"
 #include "submodule-config.h"
 #include "dir.h"
+#include "wt-status.h"
 
 #define REFRESH_INDEX_DELAY_WARNING_IN_MS (2 * 1000)
 
@@ -301,6 +302,35 @@ 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 accept_discarding_changes(void) {
+	int answer = getc(stdin);
+	printf(_("Some staged changes may be discarded by this reset. Continue? [Y/n]"));
+
+	if (answer != 'y' && answer != 'Y') {
+		printf(_("aborted\n"));
+		exit(1);
+	}
+}
+
+static void detect_risky_reset(int commits_exist) {
+	int cache = read_cache();
+	if(!commits_exist) {
+		if(cache == 1) {
+			accept_discarding_changes();
+		}
+	}
+	else {
+		if(has_uncommitted_changes(the_repository, 1)) {
+			accept_discarding_changes();
+		}
+	}
+}
+
 static void parse_args(struct pathspec *pathspec,
 		       const char **argv, const char *prefix,
 		       int patch_mode,
@@ -474,6 +504,16 @@ 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 safe = 0;
+		git_config_get_bool("reset.safe", &safe);
+		if (safe) {
+			int commits_exist = for_each_fullref_in("refs/heads", check_commit_exists, NULL);
+			detect_risky_reset(commits_exist);
+		}
+	}
+
 	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..c962c706bed 100755
--- a/t/t7104-reset-hard.sh
+++ b/t/t7104-reset-hard.sh
@@ -44,4 +44,31 @@ test_expect_success 'reset --hard did not corrupt index or cache-tree' '
 
 '
 
+test_expect_success 'reset --hard in safe mode on unborn branch with staged files results in a warning' '
+	git config reset.safe on &&
+	touch a &&
+	git add a &&
+	test_must_fail git reset --hard
+
+'
+
+test_expect_success 'reset --hard in safe mode after a commit without staged changes works fine' '
+	git config reset.safe on &&
+	touch b &&
+	git add b &&
+	git commit -m "initial" &&
+	git reset --hard
+
+'
+
+test_expect_success 'reset --hard in safe mode after a commit with staged changes results in a warning' '
+	git config reset.safe on &&
+	touch c d &&
+	git add c &&
+	git commit -m "initial" &&
+	git add d &&
+	test_must_fail git reset --hard
+
+'
+
 test_done

base-commit: 5d01301f2b865aa8dba1654d3f447ce9d21db0b5
-- 
gitgitgadget

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] hard reset safe mode
  2022-02-11 22:26 ` [PATCH v2] hard reset safe mode Viaceslavus via GitGitGadget
@ 2022-02-11 23:03   ` Junio C Hamano
  2022-02-14  3:14     ` Bagas Sanjaya
  0 siblings, 1 reply; 6+ messages in thread
From: Junio C Hamano @ 2022-02-11 23:03 UTC (permalink / raw)
  To: Viaceslavus via GitGitGadget; +Cc: git, Viaceslavus

"Viaceslavus via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Viacelaus <vaceslavkozin619@gmail.com>
>
> The power of hard reset may frequently be inconvinient for a common user. So
> this is an implementation of safe mode for hard reset. It can be switched on
> by setting 'reset.safe' config variable to true. When running 'reset --hard'
> with 'reset.safe' enabled git will check if there are any staged changes
> that may be discarded by this reset. If there is a chance of deleting the
> changes, git will ask the user for a confirmation with Yes/No choice.

There needs an explanation on how this avoids breaking scripts that
trust that "git reset --hard HEAD" reliably matches the index and
the working tree files to what is recorded in HEAD without getting
stuck waiting for any user input.  "They can turn off reset.safe" is
not an acceptable answer.

> +static int check_commit_exists(const char *refname, const struct object_id *oid, int f, void *d)
> +{
> +	return is_branch(refname);
> +}

The returned value from a for_each_ref() callback is used to tell
the caller "stop here, no need to further iterate and call me with
other refs".  I think this wants to say "if I ever get called even
once, tell the caller to stop, so that it can tell its caller that
it was stopped".

> +static void accept_discarding_changes(void) {
> +	int answer = getc(stdin);
> +	printf(_("Some staged changes may be discarded by this reset. Continue? [Y/n]"));
> +
> +	if (answer != 'y' && answer != 'Y') {
> +		printf(_("aborted\n"));
> +		exit(1);
> +	}
> +}

I'd think at least we should use git_prompt(), instead of
hand-rolled prompt routine like this one that assumes that an
end-user is sitting in front of the terminal waiting to be prompted.

If updating "git reset" like this patch does were a good idea to
begin with, that is.

> +static void detect_risky_reset(int commits_exist) {
> +	int cache = read_cache();
> +	if(!commits_exist) {
> +		if(cache == 1) {
> +			accept_discarding_changes();
> +		}
> +	}
> +	else {
> +		if(has_uncommitted_changes(the_repository, 1)) {
> +			accept_discarding_changes();
> +		}
> +	}
> +}

Style (too many to list---see Documentation/CodingGuidelines).

> +	if (reset_type == HARD) {
> +		int safe = 0;
> +		git_config_get_bool("reset.safe", &safe);
> +		if (safe) {
> +			int commits_exist = for_each_fullref_in("refs/heads", check_commit_exists, NULL);

The callback is called for each and every ref inside "refs/heads/",
so by definition, shouldn't any of them pass "is_branch(refname)"?

In any case, why does this have to be done by the caller?  If the
helper claims to be capable of detecting a "risky reset" (if such a
thing exists, that is), and if the helper behaves differently when
there is any commit on any branch or not as its implementation
detail, shouldn't it figure out if there is a commit _inside_ the
helper itself, not forcing the caller to compute it for it?

> +			detect_risky_reset(commits_exist);
> +		}
> +	}
> +
>  	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..c962c706bed 100755
> --- a/t/t7104-reset-hard.sh
> +++ b/t/t7104-reset-hard.sh
> @@ -44,4 +44,31 @@ test_expect_success 'reset --hard did not corrupt index or cache-tree' '
>  
>  '
>  
> +test_expect_success 'reset --hard in safe mode on unborn branch with staged files results in a warning' '
> +	git config reset.safe on &&

Use either "test_when_finished" or "test_config", which is a good
way to isolate each test from side effects of running the test that
comes before it.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] hard reset safe mode
  2022-02-11 23:03   ` Junio C Hamano
@ 2022-02-14  3:14     ` Bagas Sanjaya
  2022-02-14 11:44       ` Junio C Hamano
  0 siblings, 1 reply; 6+ messages in thread
From: Bagas Sanjaya @ 2022-02-14  3:14 UTC (permalink / raw)
  To: Junio C Hamano, Viaceslavus via GitGitGadget; +Cc: git, Viaceslavus

On 12/02/22 06.03, Junio C Hamano wrote:
> "Viaceslavus via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Viacelaus <vaceslavkozin619@gmail.com>
>>
>> The power of hard reset may frequently be inconvinient for a common user. So
>> this is an implementation of safe mode for hard reset. It can be switched on
>> by setting 'reset.safe' config variable to true. When running 'reset --hard'
>> with 'reset.safe' enabled git will check if there are any staged changes
>> that may be discarded by this reset. If there is a chance of deleting the
>> changes, git will ask the user for a confirmation with Yes/No choice.
> 
> There needs an explanation on how this avoids breaking scripts that
> trust that "git reset --hard HEAD" reliably matches the index and
> the working tree files to what is recorded in HEAD without getting
> stuck waiting for any user input.  "They can turn off reset.safe" is
> not an acceptable answer.
> 
>> +static int check_commit_exists(const char *refname, const struct object_id *oid, int f, void *d)
>> +{
>> +	return is_branch(refname);
>> +}
> 
> The returned value from a for_each_ref() callback is used to tell
> the caller "stop here, no need to further iterate and call me with
> other refs".  I think this wants to say "if I ever get called even
> once, tell the caller to stop, so that it can tell its caller that
> it was stopped".
> 
>> +static void accept_discarding_changes(void) {
>> +	int answer = getc(stdin);
>> +	printf(_("Some staged changes may be discarded by this reset. Continue? [Y/n]"));
>> +
>> +	if (answer != 'y' && answer != 'Y') {
>> +		printf(_("aborted\n"));
>> +		exit(1);
>> +	}
>> +}
> 
> I'd think at least we should use git_prompt(), instead of
> hand-rolled prompt routine like this one that assumes that an
> end-user is sitting in front of the terminal waiting to be prompted.
> 
> If updating "git reset" like this patch does were a good idea to
> begin with, that is.
> 

I think it's better just to error out when there are staged changes.
The message will be something like:
"error: there are staged changes that will be hard-reset. Please
commit or stash them before resetting. If you want to remove all
changes from index, you can do so by running 'git reset'."

-- 
An old man doll... just what I always wanted! - Clara

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] hard reset safe mode
  2022-02-14  3:14     ` Bagas Sanjaya
@ 2022-02-14 11:44       ` Junio C Hamano
  0 siblings, 0 replies; 6+ messages in thread
From: Junio C Hamano @ 2022-02-14 11:44 UTC (permalink / raw)
  To: Bagas Sanjaya; +Cc: Viaceslavus via GitGitGadget, git, Viaceslavus

Bagas Sanjaya <bagasdotme@gmail.com> writes:

> I think it's better just to error out when there are staged changes.
> The message will be something like:
> "error: there are staged changes that will be hard-reset. Please
> commit or stash them before resetting. If you want to remove all
> changes from index, you can do so by running 'git reset'."

I do prefer not to have interactive prompt at all, as we _will_ get
it wrong when we shouldn't be doing the warning or prompting (i.e.
when no user is sitting in front of the terminal).

Another thing that I would be very opposed to in the posted patch is
the special casing of an initial commit.  If typing 10,000 lines of
material to create a new file, doing "git add", and changing mind
and saying "git reset --hard" to lose the new file is painful before
creating the first commit, doing exactly the same in a project after
10 years with 10,000 commits will hurt exactly the same way.  You'd
"lose" the 10,000 lines of material you prepared to the "accident"
either way.

One more thing the posted patch is designed in a wrong way is that
it cares about the state of _other_ branches; even if you "git reset
--hard" on an unborn branch, if there are some commits on other
branches that has nothing to do with your current state, it will do
what it was told to do regardless of "safe mode".  The "damage"
caused by such an "accident" would be the same for the end-user,
whether there is an unrelated branch with a commit or there isn't.
You'd "lose" the 10,000 lines of material you prepared to the
"accident" regardless.

So, I am strongly opposed to the posted patch.  "git reset --hard"
is designed to bring the index and the working tree to the pristine
state in a predictable way, and making it work differently depending
on a setting, or if it is on an unborn branch, or if there are
commits on any branch, goes very against its spirit.

After all, "reset --hard" is called *HARD* for a reason.  It is not
"unsafe".  It is designed to give you pristine state no matter what
and it tries very hard to do so, by even removing an untracked file
that is squatting the path at which the pristine state wants a
directory.

In other words, those who type "git reset --hard" WITHOUT wishing to
go back to the pristine state by clearing the deck may be using a
wrong command.

I wonder if "git reset" (i.e. mixed, not hard) is what they want to
use, instead.  It is the command to use when: "I've made a mess in
the index after doing 'add -p' and other things, and want to get the
index into pristine state, while keeping the working tree files".

After doing so, the first thing they can do is to "git diff" to
compare what they truly do not want to have anywhere and "git
checkout -- <path>" to update the whole file to the state in the
index, or "git checkout -p -- <path>" to update them piecemeal.

Or perhaps they want "git stash".  That would clear the deck and
give them the pristine state justlike "git reset --hard" would, but
if they found that they did so by mistake, they can immediately
unstash.

Having said all that, because "reset" is mostly about the index and
not about the working tree (the core of the Git's philosophy is that
the index is all that matters and the working tree files are just
ephemeral means to the end, which is to update the index with good
contents and to make tree out of it), I can see us adding another
mode, perhaps called "reset --clear", that is almost like "reset
--hard" with one distinction, for end-user's interactive use.

If you did this sequence:

    $ git reset --hard    ;# start from a pristine state

    $ date >new-file-a    ;# create a new file
    $ date >new-file-b    ;# create another new file
    $ git add new-file-a  ;# just add one of them

    $ git reset --hard    ;# get back to the pristine state

the second reset will make sure that neither of these two files are
in the index, but it also removes only one of the files in the
working tree, while leaving the other file in the working tree.

The fact that new-file-a has been "git add"ed means that after
losing new-file-a, its contents *can* be recovered (i.e. it is one
of the loose objects that is no longer reachable, and "git fsck
--lost-found" could theoretically give it back to you).

But new-file-b has never been added, so losing it is a more grave
loss.  So the behaviour to remove new-file-a that was in the index
while leaving new-file-b alone is somewhat justifiable, but not by a
large margin.  A new "reset --clear" mode _could_ make paths in the
index that do not appear in the resetted-to treeish (either HEAD or
an empty tree if the branch is unborn) untracked, while leaving their
working tree files alone.

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2022-02-14 11:47 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-02 11:52 [PATCH] forbid a hard reset before the initial commit Viaceslavus via GitGitGadget
2022-02-02 21:05 ` Junio C Hamano
2022-02-11 22:26 ` [PATCH v2] hard reset safe mode Viaceslavus via GitGitGadget
2022-02-11 23:03   ` Junio C Hamano
2022-02-14  3:14     ` Bagas Sanjaya
2022-02-14 11:44       ` Junio C Hamano

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.