From 351888cf3dc3c3525faccb010adcf5c130ac5b16 Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Thu, 13 Jun 2013 15:37:06 -0700 Subject: [PATCH] Improve case handling in git_diff__paired_foreach This commit reinstates some changes to git_diff__paired_foreach that were discarded during the rebase (because the diff_output.c file had gone away), and also adjusts the case insensitively logic slightly to hopefully deal with either mismatched icase diffs and other case insensitivity scenarios. --- src/diff.c | 78 ++++++++++----- src/diff.h | 1 + src/status.c | 173 ++++++++++++++++------------------ tests-clar/status/worktree.c | 48 ++++++++++ tests-clar/submodule/status.c | 7 +- 5 files changed, 193 insertions(+), 114 deletions(-) diff --git a/src/diff.c b/src/diff.c index feb77b59b..87189a49c 100644 --- a/src/diff.c +++ b/src/diff.c @@ -254,6 +254,13 @@ int git_diff_delta__cmp(const void *a, const void *b) return val ? val : ((int)da->status - (int)db->status); } +int git_diff_delta__casecmp(const void *a, const void *b) +{ + const git_diff_delta *da = a, *db = b; + int val = strcasecmp(diff_delta__path(da), diff_delta__path(db)); + return val ? val : ((int)da->status - (int)db->status); +} + bool git_diff_delta__should_skip( const git_diff_options *opts, const git_diff_delta *delta) { @@ -1197,51 +1204,78 @@ size_t git_diff_num_deltas_of_type(git_diff_list *diff, git_delta_t type) } int git_diff__paired_foreach( - git_diff_list *idx2head, - git_diff_list *wd2idx, - int (*cb)(git_diff_delta *i2h, git_diff_delta *w2i, void *payload), + git_diff_list *head2idx, + git_diff_list *idx2wd, + int (*cb)(git_diff_delta *h2i, git_diff_delta *i2w, void *payload), void *payload) { int cmp; - git_diff_delta *i2h, *w2i; + git_diff_delta *h2i, *i2w; size_t i, j, i_max, j_max; - int (*strcomp)(const char *, const char *); + int (*strcomp)(const char *, const char *) = git__strcmp; + bool icase_mismatch; - i_max = idx2head ? idx2head->deltas.length : 0; - j_max = wd2idx ? wd2idx->deltas.length : 0; + i_max = head2idx ? head2idx->deltas.length : 0; + j_max = idx2wd ? idx2wd->deltas.length : 0; - /* Get appropriate strcmp function */ - strcomp = idx2head ? idx2head->strcomp : wd2idx ? wd2idx->strcomp : NULL; + /* At some point, tree-to-index diffs will probably never ignore case, + * even if that isn't true now. Index-to-workdir diffs may or may not + * ignore case, but the index filename for the idx2wd diff should + * still be using the canonical case-preserving name. + * + * Therefore the main thing we need to do here is make sure the diffs + * are traversed in a compatible order. To do this, we temporarily + * resort a mismatched diff to get the order correct. + */ + icase_mismatch = + (head2idx != NULL && idx2wd != NULL && + ((head2idx->opts.flags ^ idx2wd->opts.flags) & GIT_DIFF_DELTAS_ARE_ICASE)); - /* Assert both iterators use matching ignore-case. If this function ever - * supports merging diffs that are not sorted by the same function, then - * it will need to spool and sort on one of the results before merging - */ - if (idx2head && wd2idx) { - assert(idx2head->strcomp == wd2idx->strcomp); + /* force case-sensitive delta sort */ + if (icase_mismatch) { + if (head2idx->opts.flags & GIT_DIFF_DELTAS_ARE_ICASE) { + head2idx->deltas._cmp = git_diff_delta__cmp; + git_vector_sort(&head2idx->deltas); + } else { + idx2wd->deltas._cmp = git_diff_delta__cmp; + git_vector_sort(&idx2wd->deltas); + } } + else if (head2idx->opts.flags & GIT_DIFF_DELTAS_ARE_ICASE) + strcomp = git__strcasecmp; for (i = 0, j = 0; i < i_max || j < j_max; ) { - i2h = idx2head ? GIT_VECTOR_GET(&idx2head->deltas,i) : NULL; - w2i = wd2idx ? GIT_VECTOR_GET(&wd2idx->deltas,j) : NULL; + h2i = head2idx ? GIT_VECTOR_GET(&head2idx->deltas, i) : NULL; + i2w = idx2wd ? GIT_VECTOR_GET(&idx2wd->deltas, j) : NULL; - cmp = !w2i ? -1 : !i2h ? 1 : - strcomp(i2h->old_file.path, w2i->old_file.path); + cmp = !i2w ? -1 : !h2i ? 1 : + strcomp(h2i->new_file.path, i2w->old_file.path); if (cmp < 0) { - if (cb(i2h, NULL, payload)) + if (cb(h2i, NULL, payload)) return GIT_EUSER; i++; } else if (cmp > 0) { - if (cb(NULL, w2i, payload)) + if (cb(NULL, i2w, payload)) return GIT_EUSER; j++; } else { - if (cb(i2h, w2i, payload)) + if (cb(h2i, i2w, payload)) return GIT_EUSER; i++; j++; } } + /* restore case-insensitive delta sort */ + if (icase_mismatch) { + if (head2idx->opts.flags & GIT_DIFF_DELTAS_ARE_ICASE) { + head2idx->deltas._cmp = git_diff_delta__casecmp; + git_vector_sort(&head2idx->deltas); + } else { + idx2wd->deltas._cmp = git_diff_delta__casecmp; + git_vector_sort(&idx2wd->deltas); + } + } + return 0; } diff --git a/src/diff.h b/src/diff.h index ad12e7731..1536f79d7 100644 --- a/src/diff.h +++ b/src/diff.h @@ -74,6 +74,7 @@ extern void git_diff__cleanup_modes( extern void git_diff_list_addref(git_diff_list *diff); extern int git_diff_delta__cmp(const void *a, const void *b); +extern int git_diff_delta__casecmp(const void *a, const void *b); extern bool git_diff_delta__should_skip( const git_diff_options *opts, const git_diff_delta *delta); diff --git a/src/status.c b/src/status.c index ba4eef895..9c4aead74 100644 --- a/src/status.c +++ b/src/status.c @@ -130,12 +130,12 @@ static int status_collect( git_diff_delta *idx2wd, void *payload) { - git_status_list *statuslist = payload; + git_status_list *status = payload; git_status_entry *status_entry; - - if (!status_is_included(statuslist, head2idx, idx2wd)) + + if (!status_is_included(status, head2idx, idx2wd)) return 0; - + status_entry = git__malloc(sizeof(git_status_entry)); GITERR_CHECK_ALLOC(status_entry); @@ -143,9 +143,7 @@ static int status_collect( status_entry->head_to_index = head2idx; status_entry->index_to_workdir = idx2wd; - git_vector_insert(&statuslist->paired, status_entry); - - return 0; + return git_vector_insert(&status->paired, status_entry); } GIT_INLINE(int) status_entry_cmp_base( @@ -184,18 +182,23 @@ static int status_entry_cmp(const void *a, const void *b) static git_status_list *git_status_list_alloc(git_index *index) { - git_status_list *statuslist = NULL; + git_status_list *status = NULL; int (*entrycmp)(const void *a, const void *b); + if (!(status = git__calloc(1, sizeof(git_status_list)))) + return NULL; + entrycmp = index->ignore_case ? status_entry_icmp : status_entry_cmp; - if ((statuslist = git__calloc(1, sizeof(git_status_list))) == NULL || - git_vector_init(&statuslist->paired, 0, entrycmp) < 0) + if (git_vector_init(&status->paired, 0, entrycmp) < 0) { + git__free(status); return NULL; + } - return statuslist; + return status; } +/* static int newfile_cmp(const void *a, const void *b) { const git_diff_delta *delta_a = a; @@ -211,6 +214,7 @@ static int newfile_casecmp(const void *a, const void *b) return git__strcasecmp(delta_a->new_file.path, delta_b->new_file.path); } +*/ int git_status_list_new( git_status_list **out, @@ -218,13 +222,14 @@ int git_status_list_new( const git_status_options *opts) { git_index *index = NULL; - git_status_list *statuslist = NULL; + git_status_list *status = NULL; git_diff_options diffopt = GIT_DIFF_OPTIONS_INIT; git_diff_find_options findopts_i2w = GIT_DIFF_FIND_OPTIONS_INIT; git_tree *head = NULL; git_status_show_t show = opts ? opts->show : GIT_STATUS_SHOW_INDEX_AND_WORKDIR; int error = 0; + unsigned int flags = opts ? opts->flags : GIT_STATUS_OPT_DEFAULTS; assert(show <= GIT_STATUS_SHOW_INDEX_THEN_WORKDIR); @@ -238,123 +243,121 @@ int git_status_list_new( /* if there is no HEAD, that's okay - we'll make an empty iterator */ if (((error = git_repository_head_tree(&head, repo)) < 0) && - !(error == GIT_ENOTFOUND || error == GIT_EORPHANEDHEAD)) + error != GIT_ENOTFOUND && error != GIT_EORPHANEDHEAD) { + git_index_free(index); /* release index */ return error; + } - statuslist = git_status_list_alloc(index); - GITERR_CHECK_ALLOC(statuslist); + status = git_status_list_alloc(index); + GITERR_CHECK_ALLOC(status); - memcpy(&statuslist->opts, opts, sizeof(git_status_options)); - - memcpy(&diffopt.pathspec, &opts->pathspec, sizeof(diffopt.pathspec)); + if (opts) { + memcpy(&status->opts, opts, sizeof(git_status_options)); + memcpy(&diffopt.pathspec, &opts->pathspec, sizeof(diffopt.pathspec)); + } diffopt.flags = GIT_DIFF_INCLUDE_TYPECHANGE; - if ((opts->flags & GIT_STATUS_OPT_INCLUDE_UNTRACKED) != 0) + if ((flags & GIT_STATUS_OPT_INCLUDE_UNTRACKED) != 0) diffopt.flags = diffopt.flags | GIT_DIFF_INCLUDE_UNTRACKED; - if ((opts->flags & GIT_STATUS_OPT_INCLUDE_IGNORED) != 0) + if ((flags & GIT_STATUS_OPT_INCLUDE_IGNORED) != 0) diffopt.flags = diffopt.flags | GIT_DIFF_INCLUDE_IGNORED; - if ((opts->flags & GIT_STATUS_OPT_INCLUDE_UNMODIFIED) != 0) + if ((flags & GIT_STATUS_OPT_INCLUDE_UNMODIFIED) != 0) diffopt.flags = diffopt.flags | GIT_DIFF_INCLUDE_UNMODIFIED; - if ((opts->flags & GIT_STATUS_OPT_RECURSE_UNTRACKED_DIRS) != 0) + if ((flags & GIT_STATUS_OPT_RECURSE_UNTRACKED_DIRS) != 0) diffopt.flags = diffopt.flags | GIT_DIFF_RECURSE_UNTRACKED_DIRS; - if ((opts->flags & GIT_STATUS_OPT_DISABLE_PATHSPEC_MATCH) != 0) + if ((flags & GIT_STATUS_OPT_DISABLE_PATHSPEC_MATCH) != 0) diffopt.flags = diffopt.flags | GIT_DIFF_DISABLE_PATHSPEC_MATCH; - if ((opts->flags & GIT_STATUS_OPT_RECURSE_IGNORED_DIRS) != 0) + if ((flags & GIT_STATUS_OPT_RECURSE_IGNORED_DIRS) != 0) diffopt.flags = diffopt.flags | GIT_DIFF_RECURSE_IGNORED_DIRS; - if ((opts->flags & GIT_STATUS_OPT_EXCLUDE_SUBMODULES) != 0) + if ((flags & GIT_STATUS_OPT_EXCLUDE_SUBMODULES) != 0) diffopt.flags = diffopt.flags | GIT_DIFF_IGNORE_SUBMODULES; findopts_i2w.flags |= GIT_DIFF_FIND_FOR_UNTRACKED; if (show != GIT_STATUS_SHOW_WORKDIR_ONLY) { - if ((error = git_diff_tree_to_index(&statuslist->head2idx, repo, head, NULL, &diffopt)) < 0) - goto on_error; + if ((error = git_diff_tree_to_index( + &status->head2idx, repo, head, NULL, &diffopt)) < 0) + goto done; - if ((opts->flags & GIT_STATUS_OPT_RENAMES_HEAD_TO_INDEX) != 0 && - (error = git_diff_find_similar(statuslist->head2idx, NULL)) < 0) - goto on_error; + if ((flags & GIT_STATUS_OPT_RENAMES_HEAD_TO_INDEX) != 0 && + (error = git_diff_find_similar(status->head2idx, NULL)) < 0) + goto done; } if (show != GIT_STATUS_SHOW_INDEX_ONLY) { - if ((error = git_diff_index_to_workdir(&statuslist->idx2wd, repo, NULL, &diffopt)) < 0) - goto on_error; + if ((error = git_diff_index_to_workdir( + &status->idx2wd, repo, NULL, &diffopt)) < 0) + goto done; - if ((opts->flags & GIT_STATUS_OPT_RENAMES_INDEX_TO_WORKDIR) != 0 && - (error = git_diff_find_similar(statuslist->idx2wd, &findopts_i2w)) < 0) - goto on_error; + if ((flags & GIT_STATUS_OPT_RENAMES_INDEX_TO_WORKDIR) != 0 && + (error = git_diff_find_similar(status->idx2wd, &findopts_i2w)) < 0) + goto done; } if (show == GIT_STATUS_SHOW_INDEX_THEN_WORKDIR) { - if ((error = git_diff__paired_foreach(statuslist->head2idx, NULL, status_collect, statuslist)) < 0) - goto on_error; + if ((error = git_diff__paired_foreach( + status->head2idx, NULL, status_collect, status)) < 0) + goto done; - git_diff_list_free(statuslist->head2idx); - statuslist->head2idx = NULL; + git_diff_list_free(status->head2idx); + status->head2idx = NULL; } - if ((opts->flags & GIT_STATUS_OPT_RENAMES_HEAD_TO_INDEX) != 0) { - statuslist->head2idx->deltas._cmp = - (statuslist->head2idx->opts.flags & GIT_DIFF_DELTAS_ARE_ICASE) != 0 ? - newfile_casecmp : newfile_cmp; + if ((error = git_diff__paired_foreach( + status->head2idx, status->idx2wd, status_collect, status)) < 0) + goto done; - git_vector_sort(&statuslist->head2idx->deltas); - } - - if ((error = git_diff__paired_foreach(statuslist->head2idx, statuslist->idx2wd, - status_collect, statuslist)) < 0) - goto on_error; - - if ((opts->flags & GIT_STATUS_OPT_RENAMES_HEAD_TO_INDEX) != 0 || - (opts->flags & GIT_STATUS_OPT_RENAMES_INDEX_TO_WORKDIR) != 0) - git_vector_sort(&statuslist->paired); - - *out = statuslist; - goto done; - -on_error: - git_status_list_free(statuslist); + if ((flags & GIT_STATUS_OPT_RENAMES_HEAD_TO_INDEX) != 0 || + (flags & GIT_STATUS_OPT_RENAMES_INDEX_TO_WORKDIR) != 0) + git_vector_sort(&status->paired); done: + if (error < 0) { + git_status_list_free(status); + status = NULL; + } + + *out = status; + git_tree_free(head); git_index_free(index); return error; } -size_t git_status_list_entrycount(git_status_list *statuslist) +size_t git_status_list_entrycount(git_status_list *status) { - assert(statuslist); + assert(status); - return statuslist->paired.length; + return status->paired.length; } -const git_status_entry *git_status_byindex( - git_status_list *statuslist, - size_t i) +const git_status_entry *git_status_byindex(git_status_list *status, size_t i) { - assert(statuslist); + assert(status); - return git_vector_get(&statuslist->paired, i); + return git_vector_get(&status->paired, i); } -void git_status_list_free(git_status_list *statuslist) +void git_status_list_free(git_status_list *status) { git_status_entry *status_entry; size_t i; - if (statuslist == NULL) + if (status == NULL) return; - git_diff_list_free(statuslist->head2idx); - git_diff_list_free(statuslist->idx2wd); + git_diff_list_free(status->head2idx); + git_diff_list_free(status->idx2wd); - git_vector_foreach(&statuslist->paired, i, status_entry) + git_vector_foreach(&status->paired, i, status_entry) git__free(status_entry); - git_vector_free(&statuslist->paired); + git_vector_free(&status->paired); - git__free(statuslist); + git__memzero(status, sizeof(*status)); + git__free(status); } int git_status_foreach_ext( @@ -363,15 +366,15 @@ int git_status_foreach_ext( git_status_cb cb, void *payload) { - git_status_list *statuslist; + git_status_list *status; const git_status_entry *status_entry; size_t i; int error = 0; - if ((error = git_status_list_new(&statuslist, repo, opts)) < 0) + if ((error = git_status_list_new(&status, repo, opts)) < 0) return error; - git_vector_foreach(&statuslist->paired, i, status_entry) { + git_vector_foreach(&status->paired, i, status_entry) { const char *path = status_entry->head_to_index ? status_entry->head_to_index->old_file.path : status_entry->index_to_workdir->old_file.path; @@ -383,24 +386,14 @@ int git_status_foreach_ext( } } - git_status_list_free(statuslist); + git_status_list_free(status); return error; } -int git_status_foreach( - git_repository *repo, - git_status_cb callback, - void *payload) +int git_status_foreach(git_repository *repo, git_status_cb cb, void *payload) { - git_status_options opts = GIT_STATUS_OPTIONS_INIT; - - opts.show = GIT_STATUS_SHOW_INDEX_AND_WORKDIR; - opts.flags = GIT_STATUS_OPT_INCLUDE_IGNORED | - GIT_STATUS_OPT_INCLUDE_UNTRACKED | - GIT_STATUS_OPT_RECURSE_UNTRACKED_DIRS; - - return git_status_foreach_ext(repo, &opts, callback, payload); + return git_status_foreach_ext(repo, NULL, cb, payload); } struct status_file_info { diff --git a/tests-clar/status/worktree.c b/tests-clar/status/worktree.c index 13335843b..7c27ee588 100644 --- a/tests-clar/status/worktree.c +++ b/tests-clar/status/worktree.c @@ -695,3 +695,51 @@ void test_status_worktree__file_status_honors_case_ignorecase_regarding_untracke /* Actually returns GIT_STATUS_IGNORED on Windows */ cl_git_fail_with(git_status_file(&status, repo, "NEW_FILE"), GIT_ENOTFOUND); } + +void test_status_worktree__simple_delete(void) +{ + git_repository *repo = cl_git_sandbox_init("renames"); + git_status_options opts = GIT_STATUS_OPTIONS_INIT; + int count; + + opts.flags = GIT_STATUS_OPT_INCLUDE_UNTRACKED | + GIT_STATUS_OPT_DISABLE_PATHSPEC_MATCH | + GIT_STATUS_OPT_EXCLUDE_SUBMODULES | + GIT_STATUS_OPT_RECURSE_UNTRACKED_DIRS; + + count = 0; + cl_git_pass( + git_status_foreach_ext(repo, &opts, cb_status__count, &count) ); + cl_assert_equal_i(0, count); + + cl_must_pass(p_unlink("renames/untimely.txt")); + + count = 0; + cl_git_pass( + git_status_foreach_ext(repo, &opts, cb_status__count, &count) ); + cl_assert_equal_i(1, count); +} + +void test_status_worktree__simple_delete_indexed(void) +{ + git_repository *repo = cl_git_sandbox_init("renames"); + git_status_options opts = GIT_STATUS_OPTIONS_INIT; + git_status_list *status; + + opts.flags = GIT_STATUS_OPT_INCLUDE_UNTRACKED | + GIT_STATUS_OPT_DISABLE_PATHSPEC_MATCH | + GIT_STATUS_OPT_EXCLUDE_SUBMODULES | + GIT_STATUS_OPT_RECURSE_UNTRACKED_DIRS; + + cl_git_pass(git_status_list_new(&status, repo, &opts)); + cl_assert_equal_sz(0, git_status_list_entrycount(status)); + git_status_list_free(status); + + cl_must_pass(p_unlink("renames/untimely.txt")); + + cl_git_pass(git_status_list_new(&status, repo, &opts)); + cl_assert_equal_sz(1, git_status_list_entrycount(status)); + cl_assert_equal_i( + GIT_STATUS_WT_DELETED, git_status_byindex(status, 0)->status); + git_status_list_free(status); +} diff --git a/tests-clar/submodule/status.c b/tests-clar/submodule/status.c index 39c83a0b7..68110bdd5 100644 --- a/tests-clar/submodule/status.c +++ b/tests-clar/submodule/status.c @@ -376,9 +376,12 @@ void test_submodule_status__iterator(void) git_iterator_free(iter); - opts.flags = GIT_STATUS_OPT_INCLUDE_UNTRACKED | GIT_STATUS_OPT_INCLUDE_UNMODIFIED | GIT_STATUS_OPT_RECURSE_UNTRACKED_DIRS; + opts.flags = GIT_STATUS_OPT_INCLUDE_UNTRACKED | + GIT_STATUS_OPT_INCLUDE_UNMODIFIED | + GIT_STATUS_OPT_RECURSE_UNTRACKED_DIRS; - cl_git_pass(git_status_foreach_ext(g_repo, &opts, confirm_submodule_status, &exp)); + cl_git_pass(git_status_foreach_ext( + g_repo, &opts, confirm_submodule_status, &exp)); } void test_submodule_status__untracked_dirs_containing_ignored_files(void)