Re: [PATCH v2] hard reset safe mode
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 11, 2022, 23:03 UTC
- Message-ID
- <xmqqee4980qz.fsf@gitster.g>
- In-Reply-To
- <pull.1137.v2.git.1644618404948.gitgitgadget@gmail.com>
"Viaceslavus via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 8 quoted lines
> 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".
Show 9 quoted lines
> +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.
Show 13 quoted lines
> +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).
Show 5 quoted lines
> + 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?
Show 7 quoted lines
> + detect_risky_reset(commits_exist); > + } > + } > + > if (reset_type == NONE) > reset_type = MIXED; /* by default */ >
Show 10 quoted lines
> 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.