Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sparse index: integrate with git stash #428

Merged
merged 6 commits into from
Sep 21, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion builtin/am.c
Original file line number Diff line number Diff line change
Expand Up @@ -1585,7 +1585,7 @@ static int fall_back_threeway(const struct am_state *state, const char *index_pa
if (state->quiet)
o.verbosity = 0;

if (merge_recursive_generic(&o, &our_tree, &their_tree, 1, bases, &result)) {
if (merge_recursive_generic(&o, &our_tree, &their_tree, 1, bases, merge_recursive, &result)) {
repo_rerere(the_repository, state->allow_rerere_autoupdate);
free(their_tree_name);
return error(_("Failed to merge in the changes."));
Expand Down
2 changes: 1 addition & 1 deletion builtin/merge-recursive.c
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ int cmd_merge_recursive(int argc, const char **argv, const char *prefix)
if (o.verbosity >= 3)
printf(_("Merging %s with %s\n"), o.branch1, o.branch2);

failed = merge_recursive_generic(&o, &h1, &h2, bases_count, bases, &result);
failed = merge_recursive_generic(&o, &h1, &h2, bases_count, bases, merge_recursive, &result);

free(better1);
free(better2);
Expand Down
6 changes: 5 additions & 1 deletion builtin/stash.c
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include "cache-tree.h"
#include "unpack-trees.h"
#include "merge-recursive.h"
#include "merge-ort-wrappers.h"
#include "strvec.h"
#include "run-command.h"
#include "dir.h"
Expand Down Expand Up @@ -541,7 +542,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,
bases[0] = &info->b_tree;

ret = merge_recursive_generic(&o, &c_tree, &info->w_tree, 1, bases,
&result);
merge_ort_recursive, &result);
if (ret) {
rerere(0);

Expand Down Expand Up @@ -1703,6 +1704,9 @@ int cmd_stash(int argc, const char **argv, const char *prefix)
argc = parse_options(argc, argv, prefix, options, git_stash_usage,
PARSE_OPT_KEEP_UNKNOWN | PARSE_OPT_KEEP_DASHDASH);

prepare_repo_settings(the_repository);
the_repository->settings.command_requires_full_index = 0;

index_file = get_index_file();
strbuf_addf(&stash_index_path, "%s.stash.%" PRIuMAX, index_file,
(uintmax_t)pid);
Expand Down
3 changes: 2 additions & 1 deletion merge-ort.c
Original file line number Diff line number Diff line change
Expand Up @@ -4652,7 +4652,8 @@ void merge_incore_recursive(struct merge_options *opt,
trace2_region_enter("merge", "incore_recursive", opt->repo);

/* We set the ancestor label based on the merge_bases */
assert(opt->ancestor == NULL);
assert(opt->ancestor == NULL ||
!strcmp(opt->ancestor, "constructed merge base"));

trace2_region_enter("merge", "merge_start", opt->repo);
merge_start(opt, result);
Expand Down
4 changes: 2 additions & 2 deletions merge-recursive.c
Original file line number Diff line number Diff line change
Expand Up @@ -3785,6 +3785,7 @@ int merge_recursive_generic(struct merge_options *opt,
const struct object_id *merge,
int num_merge_bases,
const struct object_id **merge_bases,
recursive_merge_fn_t merge_fn,
struct commit **result)
{
int clean;
Expand All @@ -3808,8 +3809,7 @@ int merge_recursive_generic(struct merge_options *opt,
}

repo_hold_locked_index(opt->repo, &lock, LOCK_DIE_ON_ERROR);
clean = merge_recursive(opt, head_commit, next_commit, ca,
result);
clean = merge_fn(opt, head_commit, next_commit, ca, result);
Comment on lines 3811 to +3812
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is odd that this doesn't already allow the ORT strategy if configured to do so.

I went hunting for how this was configured in builtin/merge.c, but that seems like a very complicated procedure.

We could use something like the pull.twohead config here as a way to allow a user to modify the merge strategy. That gives us a safety valve in case the ORT strategy has a problem in one of these consumers.

The other approach could be to just use the ORT algorithm here no matter what. It's a big change, but it might improve things more rapidly.

After having all of these thoughts I think you've found a good middle ground: the function pointer presents a way to use ORT only when we really need it. This works especially well for our early adoption of sparse index in microsoft/git, but we should call special attention to this when upstreaming the work.

if (clean < 0) {
rollback_lock_file(&lock);
return clean;
Expand Down
9 changes: 8 additions & 1 deletion merge-recursive.h
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,12 @@ struct merge_options {
struct merge_options_internal *priv;
};

typedef int (*recursive_merge_fn_t)(struct merge_options *opt,
struct commit *h1,
struct commit *h2,
struct commit_list *merge_bases,
struct commit **result);

void init_merge_options(struct merge_options *opt, struct repository *repo);

/* parse the option in s and update the relevant field of opt */
Expand Down Expand Up @@ -103,7 +109,7 @@ int merge_recursive(struct merge_options *opt,

/*
* merge_recursive_generic can operate on trees instead of commits, by
* wrapping the trees into virtual commits, and calling merge_recursive().
* wrapping the trees into virtual commits, and calling the provided merge_fn.
* It also writes out the in-memory index to disk if the merge is successful.
*
* Outputs:
Expand All @@ -118,6 +124,7 @@ int merge_recursive_generic(struct merge_options *opt,
const struct object_id *merge,
int num_merge_bases,
const struct object_id **merge_bases,
recursive_merge_fn_t merge_fn,
struct commit **result);

#endif
1 change: 1 addition & 0 deletions t/perf/p2000-sparse-operations.sh
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ test_perf_on_all () {
}

test_perf_on_all git status
test_perf_on_all 'git stash && git stash pop'
test_perf_on_all git add -A
test_perf_on_all git add .
test_perf_on_all git commit -a -m A
Expand Down
2 changes: 1 addition & 1 deletion t/perf/run
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ set_git_test_installed () {
mydir=$1

mydir_abs=$(cd $mydir && pwd)
mydir_abs_wrappers="$mydir_abs_wrappers/bin-wrappers"
mydir_abs_wrappers="$mydir_abs/bin-wrappers"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just an FYI: I sent this upstream via gitgitgadget#1044, since it is so simple.

See https://lore.kernel.org/git/[email protected]/T/#u for the discussion.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, should I remove it from this branch? Or will it disappear once vfs-2.33.0 is rebased?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will disappear during the rebase onto v2.34.0 (assuming it gets in quickly).

if test -d "$mydir_abs_wrappers"
then
GIT_TEST_INSTALLED=$mydir_abs_wrappers
Expand Down
15 changes: 14 additions & 1 deletion t/t1092-sparse-checkout-compatibility.sh
Original file line number Diff line number Diff line change
Expand Up @@ -621,7 +621,7 @@ test_expect_success 'reset with wildcard pathspec' '

test_all_match git checkout -b reset-test update-deep &&
test_all_match git reset --hard update-folder1 &&
test_all_match git reset base -- */a &&
test_all_match git reset base -- \*/a &&
test_all_match git status --porcelain=v2
'

Expand Down Expand Up @@ -1159,6 +1159,19 @@ test_expect_success 'sparse-index is not expanded' '
echo >>sparse-index/untracked.txt &&
ensure_not_expanded add . &&

echo >>sparse-index/a &&
ensure_not_expanded stash &&
ensure_not_expanded stash list &&
ensure_not_expanded stash show stash@{0} &&
ensure_not_expanded stash apply stash@{0} &&
ensure_not_expanded stash drop stash@{0} &&

ensure_not_expanded stash create &&
oid=$(git -C sparse-index stash create) &&
ensure_not_expanded stash store -m "test" $oid &&
ensure_not_expanded reset --hard &&
ensure_not_expanded stash pop &&

ensure_not_expanded checkout-index -f a &&
ensure_not_expanded checkout-index -f --all &&
for ref in update-deep update-folder1 update-folder2 update-deep
Expand Down