From 83ba5e3654631eb186c8b820def459b0680509df Mon Sep 17 00:00:00 2001 From: Edward Thomson Date: Mon, 22 Jun 2015 18:43:13 -0400 Subject: [PATCH 1/4] diff_tform: remove reversed copy of delta merger Drop `git_diff__merge_like_cgit_reversed`, since it's a copy and paste mess of slightly incompatible changes. --- src/diff_tform.c | 53 +++++++----------------------------------------- 1 file changed, 7 insertions(+), 46 deletions(-) diff --git a/src/diff_tform.c b/src/diff_tform.c index 041592fbf..cc809c159 100644 --- a/src/diff_tform.c +++ b/src/diff_tform.c @@ -80,10 +80,11 @@ static git_diff_delta *diff_delta__merge_like_cgit( return NULL; /* If 'a' status is uninteresting, then we're done */ - if (a->status == GIT_DELTA_UNMODIFIED) + if (a->status == GIT_DELTA_UNMODIFIED || + a->status == GIT_DELTA_UNTRACKED || + a->status == GIT_DELTA_UNREADABLE) return dup; - assert(a->status != GIT_DELTA_UNMODIFIED); assert(b->status != GIT_DELTA_UNMODIFIED); /* A cgit exception is that the diff of a file that is only in the @@ -108,47 +109,6 @@ static git_diff_delta *diff_delta__merge_like_cgit( return dup; } -static git_diff_delta *diff_delta__merge_like_cgit_reversed( - const git_diff_delta *a, - const git_diff_delta *b, - git_pool *pool) -{ - git_diff_delta *dup; - - /* reversed version of above logic */ - - if (a->status == GIT_DELTA_CONFLICTED) - return diff_delta__dup(a, pool); - if (b->status == GIT_DELTA_CONFLICTED) - return diff_delta__dup(b, pool); - - if (a->status == GIT_DELTA_UNMODIFIED) - return diff_delta__dup(b, pool); - - if ((dup = diff_delta__dup(a, pool)) == NULL) - return NULL; - - if (b->status == GIT_DELTA_UNMODIFIED || b->status == GIT_DELTA_UNTRACKED || b->status == GIT_DELTA_UNREADABLE) - return dup; - - if (dup->status == GIT_DELTA_DELETED) { - if (b->status == GIT_DELTA_ADDED) { - dup->status = GIT_DELTA_UNMODIFIED; - dup->nfiles = 2; - } - } else { - dup->status = b->status; - dup->nfiles = b->nfiles; - } - - git_oid_cpy(&dup->old_file.id, &b->old_file.id); - dup->old_file.mode = b->old_file.mode; - dup->old_file.size = b->old_file.size; - dup->old_file.flags = b->old_file.flags; - - return dup; -} - int git_diff_merge(git_diff *onto, const git_diff *from) { int error = 0; @@ -191,9 +151,10 @@ int git_diff_merge(git_diff *onto, const git_diff *from) delta = diff_delta__dup(f, &onto_pool); j++; } else { - delta = reversed ? - diff_delta__merge_like_cgit_reversed(o, f, &onto_pool) : - diff_delta__merge_like_cgit(o, f, &onto_pool); + const git_diff_delta *left = reversed ? f : o; + const git_diff_delta *right = reversed ? o : f; + + delta = diff_delta__merge_like_cgit(left, right, &onto_pool); i++; j++; } From 5ef43d41b036d4178c6d886c2222db9dd7f32fc1 Mon Sep 17 00:00:00 2001 From: Edward Thomson Date: Tue, 23 Jun 2015 10:29:59 -0400 Subject: [PATCH 2/4] git_diff__merge: allow pluggable diff merges --- src/diff.h | 10 ++++++++++ src/diff_tform.c | 12 +++++++++--- 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/src/diff.h b/src/diff.h index 2a35fd9ac..39d15fa6d 100644 --- a/src/diff.h +++ b/src/diff.h @@ -123,6 +123,16 @@ extern int git_diff_find_similar__calc_similarity( extern int git_diff__commit( git_diff **diff, git_repository *repo, const git_commit *commit, const git_diff_options *opts); +/* Merge two `git_diff`s according to the callback given by `cb`. */ + +typedef git_diff_delta *(*git_diff__merge_cb)( + const git_diff_delta *left, + const git_diff_delta *right, + git_pool *pool); + +extern int git_diff__merge( + git_diff *onto, const git_diff *from, git_diff__merge_cb cb); + /* * Sometimes a git_diff_file will have a zero size; this attempts to * fill in the size without loading the blob if possible. If that is diff --git a/src/diff_tform.c b/src/diff_tform.c index cc809c159..52a7b528b 100644 --- a/src/diff_tform.c +++ b/src/diff_tform.c @@ -109,7 +109,8 @@ static git_diff_delta *diff_delta__merge_like_cgit( return dup; } -int git_diff_merge(git_diff *onto, const git_diff *from) +int git_diff__merge_deltas( + git_diff *onto, const git_diff *from, git_diff__merge_cb cb) { int error = 0; git_pool onto_pool; @@ -154,7 +155,7 @@ int git_diff_merge(git_diff *onto, const git_diff *from) const git_diff_delta *left = reversed ? f : o; const git_diff_delta *right = reversed ? o : f; - delta = diff_delta__merge_like_cgit(left, right, &onto_pool); + delta = cb(left, right, &onto_pool); i++; j++; } @@ -162,7 +163,7 @@ int git_diff_merge(git_diff *onto, const git_diff *from) /* the ignore rules for the target may not match the source * or the result of a merged delta could be skippable... */ - if (git_diff_delta__should_skip(&onto->opts, delta)) { + if (delta && git_diff_delta__should_skip(&onto->opts, delta)) { git__free(delta); continue; } @@ -193,6 +194,11 @@ int git_diff_merge(git_diff *onto, const git_diff *from) return error; } +int git_diff_merge(git_diff *onto, const git_diff *from) +{ + return git_diff__merge_deltas(onto, from, diff_delta__merge_like_cgit); +} + int git_diff_find_similar__hashsig_for_file( void **out, const git_diff_file *f, const char *path, void *p) { From 14304b0e87e92291a1265d5e692bd2d21ba59a62 Mon Sep 17 00:00:00 2001 From: Edward Thomson Date: Tue, 23 Jun 2015 16:27:17 -0400 Subject: [PATCH 3/4] stash tests: ensure we save the workdir file Ensure that when a file is added in the index and subsequently modified in the working directory, the stashed working directory tree contains the actual working directory contents. --- tests/stash/foreach.c | 20 +++++++++++--------- tests/stash/save.c | 12 ++++++++++-- tests/stash/stash_helpers.c | 5 +++++ 3 files changed, 26 insertions(+), 11 deletions(-) diff --git a/tests/stash/foreach.c b/tests/stash/foreach.c index f1983625f..57dc8eeb4 100644 --- a/tests/stash/foreach.c +++ b/tests/stash/foreach.c @@ -69,16 +69,16 @@ void test_stash_foreach__enumerating_a_empty_repository_doesnt_fail(void) void test_stash_foreach__can_enumerate_a_repository(void) { char *oids_default[] = { - "1d91c842a7cdfc25872b3a763e5c31add8816c25", NULL }; + "493568b7a2681187aaac8a58d3f1eab1527cba84", NULL }; char *oids_untracked[] = { "7f89a8b15c878809c5c54d1ff8f8c9674154017b", - "1d91c842a7cdfc25872b3a763e5c31add8816c25", NULL }; + "493568b7a2681187aaac8a58d3f1eab1527cba84", NULL }; char *oids_ignored[] = { "c95599a8fef20a7e57582c6727b1a0d02e0a5828", "7f89a8b15c878809c5c54d1ff8f8c9674154017b", - "1d91c842a7cdfc25872b3a763e5c31add8816c25", NULL }; + "493568b7a2681187aaac8a58d3f1eab1527cba84", NULL }; cl_git_pass(git_repository_init(&repo, REPO_NAME, 0)); @@ -96,9 +96,7 @@ void test_stash_foreach__can_enumerate_a_repository(void) cl_git_pass(git_stash_foreach(repo, callback_cb, &data)); cl_assert_equal_i(1, data.invokes); - data.oids = oids_untracked; - data.invokes = 0; - + /* ensure stash_foreach operates with INCLUDE_UNTRACKED */ cl_git_pass(git_stash_save( &stash_tip_oid, repo, @@ -106,12 +104,13 @@ void test_stash_foreach__can_enumerate_a_repository(void) NULL, GIT_STASH_INCLUDE_UNTRACKED)); + data.oids = oids_untracked; + data.invokes = 0; + cl_git_pass(git_stash_foreach(repo, callback_cb, &data)); cl_assert_equal_i(2, data.invokes); - data.oids = oids_ignored; - data.invokes = 0; - + /* ensure stash_foreach operates with INCLUDE_IGNORED */ cl_git_pass(git_stash_save( &stash_tip_oid, repo, @@ -119,6 +118,9 @@ void test_stash_foreach__can_enumerate_a_repository(void) NULL, GIT_STASH_INCLUDE_IGNORED)); + data.oids = oids_ignored; + data.invokes = 0; + cl_git_pass(git_stash_foreach(repo, callback_cb, &data)); cl_assert_equal_i(3, data.invokes); } diff --git a/tests/stash/save.c b/tests/stash/save.c index e07877516..edcee820f 100644 --- a/tests/stash/save.c +++ b/tests/stash/save.c @@ -100,12 +100,18 @@ $ git status --short assert_blob_oid("refs/stash:how", "e6d64adb2c7f3eb8feb493b556cc8070dca379a3"); /* not so small and */ assert_blob_oid("refs/stash:who", "a0400d4954659306a976567af43125a0b1aa8595"); /* funky world */ assert_blob_oid("refs/stash:when", NULL); + assert_blob_oid("refs/stash:why", "88c2533e21f098b89c91a431d8075cbdbe422a51"); /* would anybody use stash? */ + assert_blob_oid("refs/stash:where", "e3d6434ec12eb76af8dfa843a64ba6ab91014a0b"); /* .... */ + assert_blob_oid("refs/stash:.gitignore", "ac4d88de61733173d9959e4b77c69b9f17a00980"); assert_blob_oid("refs/stash:just.ignore", NULL); assert_blob_oid("refs/stash^2:what", "dd7e1c6f0fefe118f0b63d9f10908c460aa317a6"); /* goodbye */ assert_blob_oid("refs/stash^2:how", "e6d64adb2c7f3eb8feb493b556cc8070dca379a3"); /* not so small and */ assert_blob_oid("refs/stash^2:who", "cc628ccd10742baea8241c5924df992b5c019f71"); /* world */ assert_blob_oid("refs/stash^2:when", NULL); + assert_blob_oid("refs/stash^2:why", "88c2533e21f098b89c91a431d8075cbdbe422a51"); /* would anybody use stash? */ + assert_blob_oid("refs/stash^2:where", "e08f7fbb9a42a0c5367cf8b349f1f08c3d56bd72"); /* ???? */ + assert_blob_oid("refs/stash^2:.gitignore", "ac4d88de61733173d9959e4b77c69b9f17a00980"); assert_blob_oid("refs/stash^2:just.ignore", NULL); assert_blob_oid("refs/stash^3", NULL); @@ -243,11 +249,13 @@ void test_stash_save__cannot_stash_when_there_are_no_local_change(void) cl_git_pass(git_repository_index(&index, repo)); /* - * 'what' and 'who' are being committed. - * 'when' remain untracked. + * 'what', 'where' and 'who' are being committed. + * 'when' remains untracked. */ cl_git_pass(git_index_add_bypath(index, "what")); + cl_git_pass(git_index_add_bypath(index, "where")); cl_git_pass(git_index_add_bypath(index, "who")); + cl_repo_commit_from_index(NULL, repo, signature, 0, "Initial commit"); git_index_free(index); diff --git a/tests/stash/stash_helpers.c b/tests/stash/stash_helpers.c index ff683eced..0398757c2 100644 --- a/tests/stash/stash_helpers.c +++ b/tests/stash/stash_helpers.c @@ -26,12 +26,17 @@ void setup_stash(git_repository *repo, git_signature *signature) cl_git_rewritefile("stash/what", "goodbye\n"); /* dd7e1c6f0fefe118f0b63d9f10908c460aa317a6 */ cl_git_rewritefile("stash/how", "not so small and\n"); /* e6d64adb2c7f3eb8feb493b556cc8070dca379a3 */ cl_git_rewritefile("stash/who", "funky world\n"); /* a0400d4954659306a976567af43125a0b1aa8595 */ + cl_git_mkfile("stash/why", "would anybody use stash?\n"); /* 88c2533e21f098b89c91a431d8075cbde422a51 */ + cl_git_mkfile("stash/where", "????\n"); /* e08f7fbb9a42a0c5367cf8b349f1f08c3d56bd72 */ cl_git_pass(git_index_add_bypath(index, "what")); cl_git_pass(git_index_add_bypath(index, "how")); + cl_git_pass(git_index_add_bypath(index, "why")); + cl_git_pass(git_index_add_bypath(index, "where")); cl_git_pass(git_index_write(index)); cl_git_rewritefile("stash/what", "see you later\n"); /* bc99dc98b3eba0e9157e94769cd4d49cb49de449 */ + cl_git_mkfile("stash/where", "....\n"); /* e3d6434ec12eb76af8dfa843a64ba6ab91014a0b */ git_index_free(index); } From 9017711143ed106ec75df8fc007e383b0c90e18e Mon Sep 17 00:00:00 2001 From: Edward Thomson Date: Tue, 23 Jun 2015 16:27:33 -0400 Subject: [PATCH 4/4] stash: save the workdir file when deleted in index When stashing the workdir tree, examine the index as well. Using a mechanism similar to `git_diff_tree_to_workdir_with_index` allows us to determine that a file was added in the index and subsequently modified in the working directory. Without examining the index, we would erroneously believe that this file was untracked and fail to include it in the working directory tree. Use a slightly modified `git_diff_tree_to_workdir_with_index` in order to avoid some of the behavior custom to `git diff`. In particular, be sure to include the working directory side of a file when it was deleted in the index. --- src/diff.h | 9 +++++++++ src/diff_tform.c | 22 +++++++++++----------- src/stash.c | 29 ++++++++++++++++++++++++++--- 3 files changed, 46 insertions(+), 14 deletions(-) diff --git a/src/diff.h b/src/diff.h index 39d15fa6d..a202a086c 100644 --- a/src/diff.h +++ b/src/diff.h @@ -133,6 +133,15 @@ typedef git_diff_delta *(*git_diff__merge_cb)( extern int git_diff__merge( git_diff *onto, const git_diff *from, git_diff__merge_cb cb); +extern git_diff_delta *git_diff__merge_like_cgit( + const git_diff_delta *a, + const git_diff_delta *b, + git_pool *pool); + +/* Duplicate a `git_diff_delta` out of the `git_pool` */ +extern git_diff_delta *git_diff__delta_dup( + const git_diff_delta *d, git_pool *pool); + /* * Sometimes a git_diff_file will have a zero size; this attempts to * fill in the size without loading the blob if possible. If that is diff --git a/src/diff_tform.c b/src/diff_tform.c index 52a7b528b..92647e330 100644 --- a/src/diff_tform.c +++ b/src/diff_tform.c @@ -15,7 +15,7 @@ #include "fileops.h" #include "config.h" -static git_diff_delta *diff_delta__dup( +git_diff_delta *git_diff__delta_dup( const git_diff_delta *d, git_pool *pool) { git_diff_delta *delta = git__malloc(sizeof(git_diff_delta)); @@ -46,7 +46,7 @@ fail: return NULL; } -static git_diff_delta *diff_delta__merge_like_cgit( +git_diff_delta *git_diff__merge_like_cgit( const git_diff_delta *a, const git_diff_delta *b, git_pool *pool) @@ -67,16 +67,16 @@ static git_diff_delta *diff_delta__merge_like_cgit( /* If one of the diffs is a conflict, just dup it */ if (b->status == GIT_DELTA_CONFLICTED) - return diff_delta__dup(b, pool); + return git_diff__delta_dup(b, pool); if (a->status == GIT_DELTA_CONFLICTED) - return diff_delta__dup(a, pool); + return git_diff__delta_dup(a, pool); /* if f2 == f3 or f2 is deleted, then just dup the 'a' diff */ if (b->status == GIT_DELTA_UNMODIFIED || a->status == GIT_DELTA_DELETED) - return diff_delta__dup(a, pool); + return git_diff__delta_dup(a, pool); /* otherwise, base this diff on the 'b' diff */ - if ((dup = diff_delta__dup(b, pool)) == NULL) + if ((dup = git_diff__delta_dup(b, pool)) == NULL) return NULL; /* If 'a' status is uninteresting, then we're done */ @@ -109,7 +109,7 @@ static git_diff_delta *diff_delta__merge_like_cgit( return dup; } -int git_diff__merge_deltas( +int git_diff__merge( git_diff *onto, const git_diff *from, git_diff__merge_cb cb) { int error = 0; @@ -146,10 +146,10 @@ int git_diff__merge_deltas( STRCMP_CASESELECT(ignore_case, o->old_file.path, f->old_file.path); if (cmp < 0) { - delta = diff_delta__dup(o, &onto_pool); + delta = git_diff__delta_dup(o, &onto_pool); i++; } else if (cmp > 0) { - delta = diff_delta__dup(f, &onto_pool); + delta = git_diff__delta_dup(f, &onto_pool); j++; } else { const git_diff_delta *left = reversed ? f : o; @@ -196,7 +196,7 @@ int git_diff__merge_deltas( int git_diff_merge(git_diff *onto, const git_diff *from) { - return git_diff__merge_deltas(onto, from, diff_delta__merge_like_cgit); + return git_diff__merge(onto, from, git_diff__merge_like_cgit); } int git_diff_find_similar__hashsig_for_file( @@ -347,7 +347,7 @@ static int insert_delete_side_of_split( git_diff *diff, git_vector *onto, const git_diff_delta *delta) { /* make new record for DELETED side of split */ - git_diff_delta *deleted = diff_delta__dup(delta, &diff->pool); + git_diff_delta *deleted = git_diff__delta_dup(delta, &diff->pool); GITERR_CHECK_ALLOC(deleted); deleted->status = GIT_DELTA_DELETED; diff --git a/src/stash.c b/src/stash.c index 8f512d4ad..9010c476d 100644 --- a/src/stash.c +++ b/src/stash.c @@ -22,6 +22,7 @@ #include "signature.h" #include "iterator.h" #include "merge.h" +#include "diff.h" static int create_error(int error, const char *msg) { @@ -292,6 +293,25 @@ cleanup: return error; } +static git_diff_delta *stash_delta_merge( + const git_diff_delta *a, + const git_diff_delta *b, + git_pool *pool) +{ + /* Special case for stash: if a file is deleted in the index, but exists + * in the working tree, we need to stash the workdir copy for the workdir. + */ + if (a->status == GIT_DELTA_DELETED && b->status == GIT_DELTA_UNTRACKED) { + git_diff_delta *dup = git_diff__delta_dup(b, pool); + + if (dup) + dup->status = GIT_DELTA_MODIFIED; + return dup; + } + + return git_diff__merge_like_cgit(a, b, pool); +} + static int build_workdir_tree( git_tree **tree_out, git_index *index, @@ -299,17 +319,19 @@ static int build_workdir_tree( { git_repository *repo = git_index_owner(index); git_tree *b_tree = NULL; - git_diff *diff = NULL; + git_diff *diff = NULL, *idx_to_wd = NULL; git_diff_options opts = GIT_DIFF_OPTIONS_INIT; struct stash_update_rules data = {0}; int error; - opts.flags = GIT_DIFF_IGNORE_SUBMODULES; + opts.flags = GIT_DIFF_IGNORE_SUBMODULES | GIT_DIFF_INCLUDE_UNTRACKED; if ((error = git_commit_tree(&b_tree, b_commit)) < 0) goto cleanup; - if ((error = git_diff_tree_to_workdir(&diff, repo, b_tree, &opts)) < 0) + if ((error = git_diff_tree_to_index(&diff, repo, b_tree, index, &opts)) < 0 || + (error = git_diff_index_to_workdir(&idx_to_wd, repo, index, &opts)) < 0 || + (error = git_diff__merge(diff, idx_to_wd, stash_delta_merge)) < 0) goto cleanup; data.include_changed = true; @@ -320,6 +342,7 @@ static int build_workdir_tree( error = build_tree_from_index(tree_out, index); cleanup: + git_diff_free(idx_to_wd); git_diff_free(diff); git_tree_free(b_tree);