From 79ef3be449c9d81dd0b37a30999563aa92e4679e Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Wed, 15 May 2013 14:50:05 -0700 Subject: [PATCH 1/5] Fix diff crash when last item is untracked dir When the last item in a diff was an untracked directory that only contained ignored items, the loop to scan the contents would run off the end of the iterator and dereference a NULL pointer. This includes a test that reproduces the problem and a fix. --- src/diff.c | 6 ++++-- tests-clar/diff/workdir.c | 25 +++++++++++++++++++++++++ 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/src/diff.c b/src/diff.c index f466546bb..d93506984 100644 --- a/src/diff.c +++ b/src/diff.c @@ -747,7 +747,8 @@ static int diff_scan_inside_untracked_dir( } /* look for actual untracked file */ - while (!diff->pfxcomp(info->nitem->path, git_buf_cstr(&base))) { + while (info->nitem != NULL && + !diff->pfxcomp(info->nitem->path, git_buf_cstr(&base))) { is_ignored = git_iterator_current_is_ignored(info->new_iter); /* need to recurse into non-ignored directories */ @@ -769,7 +770,8 @@ static int diff_scan_inside_untracked_dir( } /* finish off scan */ - while (!diff->pfxcomp(info->nitem->path, git_buf_cstr(&base))) { + while (info->nitem != NULL && + !diff->pfxcomp(info->nitem->path, git_buf_cstr(&base))) { if ((error = git_iterator_advance(&info->nitem, info->new_iter)) < 0) break; } diff --git a/tests-clar/diff/workdir.c b/tests-clar/diff/workdir.c index 94fd7165d..18182ea96 100644 --- a/tests-clar/diff/workdir.c +++ b/tests-clar/diff/workdir.c @@ -1220,3 +1220,28 @@ void test_diff_workdir__untracked_directory_scenarios(void) git_diff_list_free(diff); } + + +void test_diff_workdir__untracked_directory_comes_last(void) +{ + git_diff_options opts = GIT_DIFF_OPTIONS_INIT; + git_diff_list *diff = NULL; + + g_repo = cl_git_sandbox_init("renames"); + + cl_git_mkfile("renames/.gitignore", "*.ign\n"); + cl_git_pass(p_mkdir("renames/zzz_untracked", 0777)); + cl_git_mkfile("renames/zzz_untracked/an.ign", "ignore me please"); + cl_git_mkfile("renames/zzz_untracked/skip.ign", "ignore me really"); + cl_git_mkfile("renames/zzz_untracked/test.ign", "ignore me now"); + + opts.context_lines = 3; + opts.interhunk_lines = 1; + opts.flags |= GIT_DIFF_INCLUDE_IGNORED | GIT_DIFF_INCLUDE_UNTRACKED; + + cl_git_pass(git_diff_index_to_workdir(&diff, g_repo, NULL, &opts)); + + cl_assert(diff != NULL); + + git_diff_list_free(diff); +} From 55d3a39098bfc513b12ad6cb56658cb2f87e6a91 Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Wed, 15 May 2013 14:52:12 -0700 Subject: [PATCH 2/5] Remove old symlinks before updating Unlike blob updates, symlink updates cannot be done "in place" writing over an old symlink. This means that in checkout when we realize that we can safely update a symlink, we still need to remove the old one before writing the new. --- src/checkout.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/src/checkout.c b/src/checkout.c index 68028dfef..4d019dbd1 100644 --- a/src/checkout.c +++ b/src/checkout.c @@ -189,6 +189,10 @@ static int checkout_action_common( action = (action & ~CHECKOUT_ACTION__UPDATE_BLOB) | CHECKOUT_ACTION__UPDATE_SUBMODULE; + /* to "update" a symlink, we must remove the old one first */ + if (delta->new_file.mode == GIT_FILEMODE_LINK && wd != NULL) + action |= CHECKOUT_ACTION__REMOVE; + notify = GIT_CHECKOUT_NOTIFY_UPDATED; } @@ -764,20 +768,24 @@ cleanup: } static int blob_content_to_link( - struct stat *st, git_blob *blob, const char *path, mode_t dir_mode, int can_symlink) + struct stat *st, + git_blob *blob, + const char *path, + mode_t dir_mode, + int can_symlink) { git_buf linktarget = GIT_BUF_INIT; int error; if ((error = git_futils_mkpath2file(path, dir_mode)) < 0) return error; - + if ((error = git_blob__getbuf(&linktarget, blob)) < 0) return error; if (can_symlink) { if ((error = p_symlink(git_buf_cstr(&linktarget), path)) < 0) - giterr_set(GITERR_CHECKOUT, "Could not create symlink %s\n", path); + giterr_set(GITERR_OS, "Could not create symlink %s\n", path); } else { error = git_futils_fake_symlink(git_buf_cstr(&linktarget), path); } From dcb0f7c061554de06db5879361b22eab3517a4ee Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Wed, 15 May 2013 14:54:02 -0700 Subject: [PATCH 3/5] Fix checkout of submodules with no .gitmodules It is possible for there to be a submodule in a repository with no .gitmodules file (for example, if the user forgot to commit the .gitmodules file). In this case, core Git will just create an empty directory as a placeholder for the submodule but otherwise ignore it. We were generating an error and stopping the checkout. This makes our behavior match that of core git. --- include/git2/index.h | 2 ++ src/checkout.c | 57 ++++++++++++++++++++++++++++---------------- src/index.c | 2 +- 3 files changed, 39 insertions(+), 22 deletions(-) diff --git a/include/git2/index.h b/include/git2/index.h index d23c3a8ea..bde38a9dd 100644 --- a/include/git2/index.h +++ b/include/git2/index.h @@ -57,6 +57,8 @@ GIT_BEGIN_DECL #define GIT_IDXENTRY_EXTENDED_FLAGS (GIT_IDXENTRY_INTENT_TO_ADD | GIT_IDXENTRY_SKIP_WORKTREE) +#define GIT_IDXENTRY_STAGE(E) (((E)->flags & GIT_IDXENTRY_STAGEMASK) >> GIT_IDXENTRY_STAGESHIFT) + /** Time used in a git index entry */ typedef struct { git_time_t seconds; diff --git a/src/checkout.c b/src/checkout.c index 4d019dbd1..5820f626a 100644 --- a/src/checkout.c +++ b/src/checkout.c @@ -820,6 +820,31 @@ static int checkout_update_index( return git_index_add(data->index, &entry); } +static int checkout_submodule_update_index( + checkout_data *data, + const git_diff_file *file) +{ + struct stat st; + + /* update the index unless prevented */ + if ((data->strategy & GIT_CHECKOUT_DONT_UPDATE_INDEX) != 0) + return 0; + + git_buf_truncate(&data->path, data->workdir_len); + if (git_buf_puts(&data->path, file->path) < 0) + return -1; + + if (p_stat(git_buf_cstr(&data->path), &st) < 0) { + giterr_set( + GITERR_CHECKOUT, "Could not stat submodule %s\n", file->path); + return GIT_ENOTFOUND; + } + + st.st_mode = GIT_FILEMODE_COMMIT; + + return checkout_update_index(data, file, &st); +} + static int checkout_submodule( checkout_data *data, const git_diff_file *file) @@ -836,8 +861,17 @@ static int checkout_submodule( data->opts.dir_mode, GIT_MKDIR_PATH)) < 0) return error; - if ((error = git_submodule_lookup(&sm, data->repo, file->path)) < 0) + if ((error = git_submodule_lookup(&sm, data->repo, file->path)) < 0) { + /* I've observed repos with submodules in the tree that do not + * have a .gitmodules - core Git just makes an empty directory + */ + if (error == GIT_ENOTFOUND) { + giterr_clear(); + return checkout_submodule_update_index(data, file); + } + return error; + } /* TODO: Support checkout_strategy options. Two circumstances: * 1 - submodule already checked out, but we need to move the HEAD @@ -848,26 +882,7 @@ static int checkout_submodule( * command should probably be able to. Do we need a submodule callback? */ - /* update the index unless prevented */ - if ((data->strategy & GIT_CHECKOUT_DONT_UPDATE_INDEX) == 0) { - struct stat st; - - git_buf_truncate(&data->path, data->workdir_len); - if (git_buf_puts(&data->path, file->path) < 0) - return -1; - - if ((error = p_stat(git_buf_cstr(&data->path), &st)) < 0) { - giterr_set( - GITERR_CHECKOUT, "Could not stat submodule %s\n", file->path); - return error; - } - - st.st_mode = GIT_FILEMODE_COMMIT; - - error = checkout_update_index(data, file, &st); - } - - return error; + return checkout_submodule_update_index(data, file); } static void report_progress( diff --git a/src/index.c b/src/index.c index 53edb4874..bdbe545bf 100644 --- a/src/index.c +++ b/src/index.c @@ -106,7 +106,7 @@ static void index_entry_reuc_free(git_index_reuc_entry *reuc); GIT_INLINE(int) index_entry_stage(const git_index_entry *entry) { - return (entry->flags & GIT_IDXENTRY_STAGEMASK) >> GIT_IDXENTRY_STAGESHIFT; + return GIT_IDXENTRY_STAGE(entry); } static int index_srch(const void *key, const void *array_member) From 09fae31d45c3b9112cb7ce7d5c7d1d63840e407d Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Wed, 15 May 2013 14:58:26 -0700 Subject: [PATCH 4/5] Improve robustness of diff rename detection Under some strange circumstances, diffs can end up listing files that we can't actually open successfully. Instead of aborting the git_diff_find_similar, this makes it so that those files just won't be considered as valid rename/copy targets instead. --- src/diff_tform.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/src/diff_tform.c b/src/diff_tform.c index 201a0e896..de2d5f616 100644 --- a/src/diff_tform.c +++ b/src/diff_tform.c @@ -386,8 +386,12 @@ static int similarity_calc( /* TODO: apply wd-to-odb filters to file data if necessary */ - if (!(error = git_buf_joinpath( - &path, git_repository_workdir(diff->repo), file->path))) + if ((error = git_buf_joinpath( + &path, git_repository_workdir(diff->repo), file->path)) < 0) + return error; + + /* if path is not a regular file, just skip this item */ + if (git_path_isfile(path.ptr)) error = opts->metric->file_signature( &cache[file_idx], file, path.ptr, opts->metric->payload); @@ -398,8 +402,11 @@ static int similarity_calc( /* TODO: add max size threshold a la diff? */ - if ((error = git_blob_lookup(&blob, diff->repo, &file->oid)) < 0) - return error; + if (git_blob_lookup(&blob, diff->repo, &file->oid) < 0) { + /* if lookup fails, just skip this item in similarity calc */ + giterr_clear(); + return 0; + } blobsize = git_blob_rawsize(blob); if (!git__is_sizet(blobsize)) /* ? what to do ? */ @@ -437,7 +444,7 @@ static int similarity_measure( return -1; if (!cache[b_idx] && similarity_calc(diff, opts, b_idx, cache) < 0) return -1; - + /* some metrics may not wish to process this file (too big / too small) */ if (!cache[a_idx] || !cache[b_idx]) return 0; From 72b3dd4a5ca2f6572e741c243cd973963d0ef419 Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Wed, 15 May 2013 15:23:33 -0700 Subject: [PATCH 5/5] Use GIT_IDXENTRY_STAGE macro Since I added the GIT_IDXENTRY_STAGE macro to extract the stage from a git_index_entry, we probably don't need an internal inline function to do the same thing. --- src/index.c | 27 +++++++++++---------------- 1 file changed, 11 insertions(+), 16 deletions(-) diff --git a/src/index.c b/src/index.c index bdbe545bf..35f282b35 100644 --- a/src/index.c +++ b/src/index.c @@ -104,11 +104,6 @@ static int index_find(size_t *at_pos, git_index *index, const char *path, int st static void index_entry_free(git_index_entry *entry); static void index_entry_reuc_free(git_index_reuc_entry *reuc); -GIT_INLINE(int) index_entry_stage(const git_index_entry *entry) -{ - return GIT_IDXENTRY_STAGE(entry); -} - static int index_srch(const void *key, const void *array_member) { const struct entry_srch_key *srch_key = key; @@ -118,7 +113,7 @@ static int index_srch(const void *key, const void *array_member) ret = strcmp(srch_key->path, entry->path); if (ret == 0) - ret = srch_key->stage - index_entry_stage(entry); + ret = srch_key->stage - GIT_IDXENTRY_STAGE(entry); return ret; } @@ -132,7 +127,7 @@ static int index_isrch(const void *key, const void *array_member) ret = strcasecmp(srch_key->path, entry->path); if (ret == 0) - ret = srch_key->stage - index_entry_stage(entry); + ret = srch_key->stage - GIT_IDXENTRY_STAGE(entry); return ret; } @@ -170,7 +165,7 @@ static int index_cmp(const void *a, const void *b) diff = strcmp(entry_a->path, entry_b->path); if (diff == 0) - diff = (index_entry_stage(entry_a) - index_entry_stage(entry_b)); + diff = (GIT_IDXENTRY_STAGE(entry_a) - GIT_IDXENTRY_STAGE(entry_b)); return diff; } @@ -184,7 +179,7 @@ static int index_icmp(const void *a, const void *b) diff = strcasecmp(entry_a->path, entry_b->path); if (diff == 0) - diff = (index_entry_stage(entry_a) - index_entry_stage(entry_b)); + diff = (GIT_IDXENTRY_STAGE(entry_a) - GIT_IDXENTRY_STAGE(entry_b)); return diff; } @@ -721,7 +716,7 @@ static int index_insert(git_index *index, git_index_entry *entry, int replace) entry->flags |= GIT_IDXENTRY_NAMEMASK; /* look if an entry with this path already exists */ - if (!index_find(&position, index, entry->path, index_entry_stage(entry))) { + if (!index_find(&position, index, entry->path, GIT_IDXENTRY_STAGE(entry))) { existing = (git_index_entry **)&index->entries.contents[position]; /* update filemode to existing values if stat is not trusted */ @@ -869,7 +864,7 @@ int git_index_remove_directory(git_index *index, const char *dir, int stage) if (!entry || git__prefixcmp(entry->path, pfx.ptr) != 0) break; - if (index_entry_stage(entry) != stage) { + if (GIT_IDXENTRY_STAGE(entry) != stage) { ++pos; continue; } @@ -1008,7 +1003,7 @@ int git_index_conflict_get(git_index_entry **ancestor_out, if (index->entries_cmp_path(conflict_entry->path, path) != 0) break; - stage = index_entry_stage(conflict_entry); + stage = GIT_IDXENTRY_STAGE(conflict_entry); switch (stage) { case 3: @@ -1050,7 +1045,7 @@ int git_index_conflict_remove(git_index *index, const char *path) if (index->entries_cmp_path(conflict_entry->path, path) != 0) break; - if (index_entry_stage(conflict_entry) == 0) { + if (GIT_IDXENTRY_STAGE(conflict_entry) == 0) { pos++; continue; } @@ -1069,7 +1064,7 @@ static int index_conflicts_match(const git_vector *v, size_t idx) { git_index_entry *entry = git_vector_get(v, idx); - if (index_entry_stage(entry) > 0) { + if (GIT_IDXENTRY_STAGE(entry) > 0) { index_entry_free(entry); return 1; } @@ -1091,7 +1086,7 @@ int git_index_has_conflicts(const git_index *index) assert(index); git_vector_foreach(&index->entries, i, entry) { - if (index_entry_stage(entry) > 0) + if (GIT_IDXENTRY_STAGE(entry) > 0) return 1; } @@ -1858,7 +1853,7 @@ static int write_index(git_index *index, git_filebuf *file) int git_index_entry_stage(const git_index_entry *entry) { - return index_entry_stage(entry); + return GIT_IDXENTRY_STAGE(entry); } typedef struct read_tree_data {