Hi Denton
On 03/04/2020 11:31, Denton Liu wrote:
Hi Phillip,
Thanks for your detailed review on the series. I've updated everything
according to your comments except for the parts below:
That's great, I've added a few more comments below
On Thu, Apr 02, 2020 at 04:24:54PM +0100, Phillip Wood wrote:
diff --git a/branch.c b/branch.c
index 579494738a..bf2536c70d 100644
--- a/branch.c
+++ b/branch.c
@@ -344,6 +344,7 @@ void remove_merge_branch_state(struct repository *r)
unlink(git_path_merge_rr(r));
unlink(git_path_merge_msg(r));
unlink(git_path_merge_mode(r));
+ apply_autostash(git_path_merge_autostash(r));
}
void remove_branch_state(struct repository *r, int verbose)
diff --git a/builtin/commit.c b/builtin/commit.c
index 7ba33a3bec..c11894423a 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1687,6 +1687,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
unlink(git_path_merge_mode(the_repository));
unlink(git_path_squash_msg(the_repository));
+ apply_autostash(git_path_merge_autostash(the_repository));
+
It's hard to tell from the limited context but do we want to run
commit_index_files() before applying the autostash?
I don't think it really matters which order we run it in. When we run
apply_autostash(), we only ever touch the working tree, not the index so
it doesn't matter if it's run before or after. I'd prefer to keep it
here because if we ever refactor this to use
remove_merge_branch_state(), the person working on this will be able to
perform the refactor more easily without having to worry about implicit
ordering dependencies.
Thinking a bit more about this we definitely want to commit the index
files before applying the stash as that is done in a separate process so
we want our index written to disk first.
'git stash apply' does touch the index while it's merging and then tries
to reset it to the pre-merge state if the merge has no conflicts. If the
user runs
git add new-file
git merge --autostash branch
and new-file is not in branch then 'git stash apply' will add new-file
to the index
I wonder if this should
be using remove_merge_branch_state() instead of duplicating it as well.
We can _almost_ use remove_branch_state() even! Unfortunately,
remove_merge_branch_state() calls `unlink(git_path_merge_rr(r))` which
we cannot do since repo_rerere() relies on it later. Perhaps we can
can move repo_rerere() earlier?
I think we should move apply_autostash() down so that repo_rerere() is
called with the index that we've committed before we apply the stash
which might add conflicts or new files. We should probably run the
post-commit and post-rewrite hooks before we apply the autostash as well
to avoid changing the existing behaviour.
If we aim for something like
commit_index_files()
repo_rerere()
stash_oid = read_autostash_oid()
cleanup_branch_state()
run_post_commit_hook()
run_post_rewrite_hook()
print_commit_summary()
apply_autostash(stash_oid)
Then we keep the existing behaviour for rerere and the hooks, and the
commit summary is printed before all the status junk that
apply_autostash() spews out
We probably need to check the where 'git merge' applies the autostash as
well to make sure it is writing the index to disk, calling the
post-merge hook and printing any messages before applying the stash
Best Wishes
Phillip
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?
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;
}
