Hi Denton
On 03/04/2020 11:56, Denton Liu wrote:
On Fri, Apr 03, 2020 at 06:31:26AM -0400, Denton Liu wrote:
diff --git a/builtin/reset.c b/builtin/reset.c
index 18228c312e..038c8532eb 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -25,6 +25,7 @@
#include "cache-tree.h"
#include "submodule.h"
#include "submodule-config.h"
+#include "sequencer.h"
#define REFRESH_INDEX_DELAY_WARNING_IN_MS (2 * 1000)
@@ -437,8 +438,12 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
if (reset_type == HARD && !update_ref_status && !quiet)
print_new_head_line(lookup_commit_reference(the_repository, &oid));
}
- if (!pathspec.nr)
+ if (!pathspec.nr) {
+ if (reset_type == HARD)
+ save_autostash(git_path_merge_autostash(the_repository));
+
remove_branch_state(the_repository, 0);
This removes the autostash file for all reset types but we only keep the
stash in the case of 'reset --hard' which is confusing.
I was worried that this change would be controversial... The rationale
behind this change was that with `reset --hard`, we want to leave a
clean working tree behind so we save it into the stash reflog. In all
other cases, remove_branch_state() will apply the saved stash entry
which should be fine since users don't expect a clean worktree with the
other reset types.
I considered saving the autostash in all cases of reset but
`git merge --abort` invokes `git reset --merge` behind the scenes so
we'd have to consider that. Perhaps we can make all resets save the
stash entry and, in the case of `merge --abort`, we can add some extra
logic to subvert this so that the stash entry is applied?
Perhaps something like this?
-- >8 --
commit 14d0b569cb7675f00d32d3d7fad7564fcaeca458
Author: Denton Liu <liu.denton@xxxxxxxxx>
Date: Fri Apr 3 06:50:34 2020 -0400
squash! merge: teach --autostash option
Stash is saved when any reset is run, when git merge --abort is run,
stash is applied.
I think this is the easiest behavior to understand, it avoids changing
the behavior of reset, in particular it stops 'reset --mixed/--soft'
from suddenly starting to touch the working tree.
diff --git a/builtin/merge.c b/builtin/merge.c
index 9573d77096..31b82d614c 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1242,6 +1242,8 @@ static int merging_a_throwaway_tag(struct commit *commit)
return is_throwaway_tag;
}
+static GIT_PATH_FUNC(git_path_merge_autostash_saved, "MERGE_AUTOSTASH_SAVED")
+
int cmd_merge(int argc, const char **argv, const char *prefix)
{
struct object_id result_tree, stash, head_oid;
@@ -1295,9 +1297,16 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
if (!file_exists(git_path_merge_head(the_repository)))
die(_("There is no merge to abort (MERGE_HEAD missing)."));
+ if (file_exists(git_path_merge_autostash(the_repository))) {
+ if (rename(git_path_merge_autostash(the_repository),
+ git_path_merge_autostash_saved()))
+ die_errno(_("failed to rename autostash"));
This is a bit of a performance, can't we just remember the stash oid in
a variable and tweak the apply code?
+ }
+
/* Invoke 'git reset --merge' */
ret = cmd_reset(nargc, nargv, prefix);
- apply_autostash(git_path_merge_autostash(the_repository));
+
+ apply_autostash(git_path_merge_autostash_saved());
Calling cmd_reset() was already a bit dodgy by our normal rules, now
we're calling other functions after it though I guess given the current
autostash implementation it's mostly in a separate process.
I think this is a good direction to go in
BTW what message gets printed when the stash is saved?
Best Wishes
Phillip
goto done;
}
diff --git a/builtin/reset.c b/builtin/reset.c
index 038c8532eb..060470c455 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -439,9 +439,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
print_new_head_line(lookup_commit_reference(the_repository, &oid));
}
if (!pathspec.nr) {
- if (reset_type == HARD)
- save_autostash(git_path_merge_autostash(the_repository));
-
+ save_autostash(git_path_merge_autostash(the_repository));
remove_branch_state(the_repository, 0);
}
I'm not sure about what the most intuitive behaviour is. I thought that
this implementation would be the best but I'm not entirely sure. I'd
appreciate some more discussion on this.
Thanks,
Denton
Best Wishes
Phillip
+ }
return update_ref_status;
}
