From 240f4af321612a0fe4cf01aed75a8cb44173feb8 Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Mon, 28 Apr 2014 14:04:29 -0700 Subject: [PATCH 01/13] Add build option for diff internal statistics --- CMakeLists.txt | 5 +++ src/checkout.c | 3 +- src/diff.c | 79 +++++++++++++++++++++++++------------------- src/diff.h | 7 +++- src/diff_tform.c | 8 ++--- src/iterator.c | 10 ++++-- src/iterator.h | 4 +++ src/status.c | 8 ++--- src/util.h | 8 +++++ tests/diff/workdir.c | 12 +++++++ 10 files changed, 96 insertions(+), 48 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 884c9bcf1..83e03d6cd 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -37,6 +37,7 @@ OPTION( ANDROID "Build for android NDK" OFF ) OPTION( USE_ICONV "Link with and use iconv library" OFF ) OPTION( USE_SSH "Link with libssh to enable SSH support" ON ) OPTION( VALGRIND "Configure build for valgrind" OFF ) +OPTION( PERF_STATS "Internally track performance data" OFF ) IF(${CMAKE_SYSTEM_NAME} MATCHES "Darwin") SET( USE_ICONV ON ) @@ -352,6 +353,10 @@ IF (THREADSAFE) ADD_DEFINITIONS(-DGIT_THREADS) ENDIF() +IF (PERF_STATS) + ADD_DEFINITIONS(-DGIT_PERF) +ENDIF() + ADD_DEFINITIONS(-D_FILE_OFFSET_BITS=64) # Collect sourcefiles diff --git a/src/checkout.c b/src/checkout.c index bc976b854..93d6bc9c5 100644 --- a/src/checkout.c +++ b/src/checkout.c @@ -185,8 +185,7 @@ static bool checkout_is_workdir_modified( return true; if (git_diff__oid_for_file( - data->repo, wditem->path, wditem->mode, - wditem->file_size, &oid) < 0) + &oid, data->diff, wditem->path, wditem->mode, wditem->file_size) < 0) return false; return (git_oid__cmp(&baseitem->id, &oid) != 0); diff --git a/src/diff.c b/src/diff.c index 4b6fbe25a..aa880650a 100644 --- a/src/diff.c +++ b/src/diff.c @@ -510,26 +510,31 @@ void git_diff_addref(git_diff *diff) } int git_diff__oid_for_file( - git_repository *repo, + git_oid *out, + git_diff *diff, const char *path, uint16_t mode, - git_off_t size, - git_oid *oid) + git_off_t size) { - int result = 0; + int error = 0; git_buf full_path = GIT_BUF_INIT; + git_filter_list *fl = NULL; + + memset(out, 0, sizeof(*out)); if (git_buf_joinpath( - &full_path, git_repository_workdir(repo), path) < 0) + &full_path, git_repository_workdir(diff->repo), path) < 0) return -1; if (!mode) { struct stat st; - if (p_stat(path, &st) < 0) { - giterr_set(GITERR_OS, "Could not stat '%s'", path); - result = -1; - goto cleanup; + GIT_PERF_INC(diff->stat_calls); + + if (p_stat(full_path.ptr, &st) < 0) { + error = git_path_set_error(errno, path, "stat"); + git_buf_free(&full_path); + return error; } mode = st.st_mode; @@ -540,46 +545,43 @@ int git_diff__oid_for_file( if (S_ISGITLINK(mode)) { git_submodule *sm; - memset(oid, 0, sizeof(*oid)); + GIT_PERF_INC(diff->submodule_lookups); - if (!git_submodule_lookup(&sm, repo, path)) { + if (!git_submodule_lookup(&sm, diff->repo, path)) { const git_oid *sm_oid = git_submodule_wd_id(sm); if (sm_oid) - git_oid_cpy(oid, sm_oid); + git_oid_cpy(out, sm_oid); git_submodule_free(sm); } else { /* if submodule lookup failed probably just in an intermediate * state where some init hasn't happened, so ignore the error */ giterr_clear(); - memset(oid, 0, sizeof(*oid)); } } else if (S_ISLNK(mode)) { - result = git_odb__hashlink(oid, full_path.ptr); + GIT_PERF_INC(diff->oid_calculations); + error = git_odb__hashlink(out, full_path.ptr); } else if (!git__is_sizet(size)) { giterr_set(GITERR_OS, "File size overflow (for 32-bits) on '%s'", path); - result = -1; - } else { - git_filter_list *fl = NULL; - - result = git_filter_list_load(&fl, repo, NULL, path, GIT_FILTER_TO_ODB); - if (!result) { - int fd = git_futils_open_ro(full_path.ptr); - if (fd < 0) - result = fd; - else { - result = git_odb__hashfd_filtered( - oid, fd, (size_t)size, GIT_OBJ_BLOB, fl); - p_close(fd); - } - - git_filter_list_free(fl); + error = -1; + } else if (!(error = git_filter_list_load( + &fl, diff->repo, NULL, path, GIT_FILTER_TO_ODB))) + { + int fd = git_futils_open_ro(full_path.ptr); + if (fd < 0) + error = fd; + else { + GIT_PERF_INC(diff->oid_calculations); + error = git_odb__hashfd_filtered( + out, fd, (size_t)size, GIT_OBJ_BLOB, fl); + p_close(fd); } + + git_filter_list_free(fl); } -cleanup: git_buf_free(&full_path); - return result; + return error; } static bool diff_time_eq( @@ -617,6 +619,8 @@ static int maybe_modified_submodule( ign == GIT_SUBMODULE_IGNORE_ALL) return 0; + GIT_PERF_INC(diff->submodule_lookups); + if ((error = git_submodule_lookup( &sub, diff->repo, info->nitem->path)) < 0) { @@ -748,8 +752,8 @@ static int maybe_modified( */ if (status == GIT_DELTA_MODIFIED && git_oid_iszero(&nitem->id)) { if (git_oid_iszero(&noid)) { - if ((error = git_diff__oid_for_file(diff->repo, - nitem->path, nitem->mode, nitem->file_size, &noid)) < 0) + if ((error = git_diff__oid_for_file(&noid, + diff, nitem->path, nitem->mode, nitem->file_size)) < 0) return error; } @@ -914,6 +918,8 @@ static int handle_unmatched_new_item( delta_type = GIT_DELTA_ADDED; else if (nitem->mode == GIT_FILEMODE_COMMIT) { + GIT_PERF_INC(diff->submodule_lookups); + /* ignore things that are not actual submodules */ if (git_submodule_lookup(NULL, info->repo, nitem->path) != 0) { giterr_clear(); @@ -1066,6 +1072,11 @@ int git_diff__from_iterators( error = 0; } + GIT_PERF_ADD(diff->stat_calls, old_iter->stat_calls); + GIT_PERF_ADD(diff->stat_calls, new_iter->stat_calls); + GIT_PERF_ADD(diff->submodule_lookups, old_iter->submodule_lookups); + GIT_PERF_ADD(diff->submodule_lookups, new_iter->submodule_lookups); + cleanup: if (!error) *diff_ptr = diff; diff --git a/src/diff.h b/src/diff.h index aae8fbff1..491fc4667 100644 --- a/src/diff.h +++ b/src/diff.h @@ -62,6 +62,11 @@ struct git_diff { git_iterator_type_t old_src; git_iterator_type_t new_src; uint32_t diffcaps; +#ifdef GIT_PERF + size_t stat_calls; + size_t oid_calculations; + size_t submodule_lookups; +#endif int (*strcomp)(const char *, const char *); int (*strncomp)(const char *, const char *, size_t); @@ -90,7 +95,7 @@ extern int git_diff_delta__format_file_header( int oid_strlen); extern int git_diff__oid_for_file( - git_repository *, const char *, uint16_t, git_off_t, git_oid *); + git_oid *oit, git_diff *, const char *, uint16_t, git_off_t); extern int git_diff__from_iterators( git_diff **diff_ptr, diff --git a/src/diff_tform.c b/src/diff_tform.c index 97fbc2883..a2dab0ae2 100644 --- a/src/diff_tform.c +++ b/src/diff_tform.c @@ -574,14 +574,14 @@ static int similarity_measure( if (exact_match) { if (git_oid_iszero(&a_file->id) && diff->old_src == GIT_ITERATOR_TYPE_WORKDIR && - !git_diff__oid_for_file(diff->repo, a_file->path, - a_file->mode, a_file->size, &a_file->id)) + !git_diff__oid_for_file(&a_file->id, + diff, a_file->path, a_file->mode, a_file->size)) a_file->flags |= GIT_DIFF_FLAG_VALID_ID; if (git_oid_iszero(&b_file->id) && diff->new_src == GIT_ITERATOR_TYPE_WORKDIR && - !git_diff__oid_for_file(diff->repo, b_file->path, - b_file->mode, b_file->size, &b_file->id)) + !git_diff__oid_for_file(&b_file->id, + diff, b_file->path, b_file->mode, b_file->size)) b_file->flags |= GIT_DIFF_FLAG_VALID_ID; } diff --git a/src/iterator.c b/src/iterator.c index ef27fa71f..5e668b50c 100644 --- a/src/iterator.c +++ b/src/iterator.c @@ -1017,6 +1017,8 @@ static int fs_iterator__expand_dir(fs_iterator *fi) return GIT_ENOTFOUND; } + GIT_PERF_ADD(fi->base.stat_calls, ff->entries.length); + fs_iterator__seek_frame_start(fi, ff); ff->next = fi->stack; @@ -1304,9 +1306,11 @@ static int workdir_iterator__enter_dir(fs_iterator *fi) /* convert submodules to GITLINK and remove trailing slashes */ git_vector_foreach(&ff->entries, pos, entry) { - if (S_ISDIR(entry->st.st_mode) && - git_submodule__is_submodule(fi->base.repo, entry->path)) - { + if (!S_ISDIR(entry->st.st_mode)) + continue; + + GIT_PERF_INC(fi->base.submodule_lookups); + if (git_submodule__is_submodule(fi->base.repo, entry->path)) { entry->st.st_mode = GIT_FILEMODE_COMMIT; entry->path_len--; entry->path[entry->path_len] = '\0'; diff --git a/src/iterator.h b/src/iterator.h index ba9c1e486..2968b8c0c 100644 --- a/src/iterator.h +++ b/src/iterator.h @@ -53,6 +53,10 @@ struct git_iterator { char *end; int (*prefixcomp)(const char *str, const char *prefix); unsigned int flags; +#ifdef GIT_PERF + size_t stat_calls; + size_t submodule_lookups; +#endif }; extern int git_iterator_for_nothing( diff --git a/src/status.c b/src/status.c index c4b990a84..e1f8e06ae 100644 --- a/src/status.c +++ b/src/status.c @@ -81,15 +81,15 @@ static unsigned int workdir_delta2status( if (git_oid_iszero(&idx2wd->old_file.id) && diff->old_src == GIT_ITERATOR_TYPE_WORKDIR && !git_diff__oid_for_file( - diff->repo, idx2wd->old_file.path, idx2wd->old_file.mode, - idx2wd->old_file.size, &idx2wd->old_file.id)) + &idx2wd->old_file.id, diff, idx2wd->old_file.path, + idx2wd->old_file.mode, idx2wd->old_file.size)) idx2wd->old_file.flags |= GIT_DIFF_FLAG_VALID_ID; if (git_oid_iszero(&idx2wd->new_file.id) && diff->new_src == GIT_ITERATOR_TYPE_WORKDIR && !git_diff__oid_for_file( - diff->repo, idx2wd->new_file.path, idx2wd->new_file.mode, - idx2wd->new_file.size, &idx2wd->new_file.id)) + &idx2wd->new_file.id, diff, idx2wd->new_file.path, + idx2wd->new_file.mode, idx2wd->new_file.size)) idx2wd->new_file.flags |= GIT_DIFF_FLAG_VALID_ID; if (!git_oid_equal(&idx2wd->old_file.id, &idx2wd->new_file.id)) diff --git a/src/util.h b/src/util.h index 6fb2dc0f4..be7a16ef8 100644 --- a/src/util.h +++ b/src/util.h @@ -436,4 +436,12 @@ GIT_INLINE(double) git__timer(void) #endif +#ifdef GIT_PERF +# define GIT_PERF_INC(counter) (counter)++ +# define GIT_PERF_ADD(counter,val) (counter) += (val) +#else +# define GIT_PERF_INC(counter) 0 +# define GIT_PERF_ADD(counter,val) 0 +#endif + #endif /* INCLUDE_util_h__ */ diff --git a/tests/diff/workdir.c b/tests/diff/workdir.c index 6128e820e..03a3ff418 100644 --- a/tests/diff/workdir.c +++ b/tests/diff/workdir.c @@ -2,6 +2,11 @@ #include "diff_helpers.h" #include "repository.h" +#ifdef GIT_PERF +/* access to diff usage statistics */ +# include "diff.h" +#endif + static git_repository *g_repo = NULL; void test_diff_workdir__initialize(void) @@ -58,6 +63,13 @@ void test_diff_workdir__to_index(void) cl_assert_equal_i(5, exp.line_ctxt); cl_assert_equal_i(4, exp.line_adds); cl_assert_equal_i(5, exp.line_dels); + +#ifdef GIT_PERF + cl_assert_equal_sz( + 13 /* in root */ + 3 /* in subdir */, diff->stat_calls); + cl_assert_equal_sz(9, diff->oid_calculations); + cl_assert_equal_sz(2, diff->submodule_lookups); +#endif } git_diff_free(diff); From 8ef4e11a76599111b98682d235e7a4df921b2597 Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Mon, 28 Apr 2014 14:16:26 -0700 Subject: [PATCH 02/13] Skip diff oid calc when size definitely changed When we think the stat cache in the index seems valid and the size or mode of a file has definitely changed, then don't bother trying to recalculate the OID of the workdir bits to confirm that it is modified - just accept that it is modified. This can result in files that show as modified with no actual diff, but the behavior actually appears to match Git on the command line. This also includes a minor optimization to not perform a submodule lookup on the ".git" directory itself. --- src/diff.c | 15 +++++++++++---- src/iterator.c | 2 +- tests/diff/workdir.c | 4 ++-- tests/status/worktree.c | 6 +++++- 4 files changed, 19 insertions(+), 8 deletions(-) diff --git a/src/diff.c b/src/diff.c index aa880650a..4c028ca4e 100644 --- a/src/diff.c +++ b/src/diff.c @@ -664,6 +664,7 @@ static int maybe_modified( unsigned int omode = oitem->mode; unsigned int nmode = nitem->mode; bool new_is_workdir = (info->new_iter->type == GIT_ITERATOR_TYPE_WORKDIR); + bool modified_uncertain = false; const char *matched_pathspec; int error = 0; @@ -731,15 +732,21 @@ static int maybe_modified( /* if the stat data looks different, then mark modified - this just * means that the OID will be recalculated below to confirm change */ - else if (omode != nmode || - oitem->file_size != nitem->file_size || - !diff_time_eq(&oitem->mtime, &nitem->mtime, use_nanos) || + else if (omode != nmode || oitem->file_size != nitem->file_size) { + status = GIT_DELTA_MODIFIED; + modified_uncertain = + (oitem->file_size <= 0 && nitem->file_size > 0); + } + else if (!diff_time_eq(&oitem->mtime, &nitem->mtime, use_nanos) || (use_ctime && !diff_time_eq(&oitem->ctime, &nitem->ctime, use_nanos)) || oitem->ino != nitem->ino || oitem->uid != nitem->uid || oitem->gid != nitem->gid) + { status = GIT_DELTA_MODIFIED; + modified_uncertain = true; + } } /* if mode is GITLINK and submodules are ignored, then skip */ @@ -750,7 +757,7 @@ static int maybe_modified( /* if we got here and decided that the files are modified, but we * haven't calculated the OID of the new item, then calculate it now */ - if (status == GIT_DELTA_MODIFIED && git_oid_iszero(&nitem->id)) { + if (modified_uncertain && git_oid_iszero(&nitem->id)) { if (git_oid_iszero(&noid)) { if ((error = git_diff__oid_for_file(&noid, diff, nitem->path, nitem->mode, nitem->file_size)) < 0) diff --git a/src/iterator.c b/src/iterator.c index 5e668b50c..03058b956 100644 --- a/src/iterator.c +++ b/src/iterator.c @@ -1306,7 +1306,7 @@ static int workdir_iterator__enter_dir(fs_iterator *fi) /* convert submodules to GITLINK and remove trailing slashes */ git_vector_foreach(&ff->entries, pos, entry) { - if (!S_ISDIR(entry->st.st_mode)) + if (!S_ISDIR(entry->st.st_mode) || !strcmp(GIT_DIR, entry->path)) continue; GIT_PERF_INC(fi->base.submodule_lookups); diff --git a/tests/diff/workdir.c b/tests/diff/workdir.c index 03a3ff418..84c8866e0 100644 --- a/tests/diff/workdir.c +++ b/tests/diff/workdir.c @@ -67,8 +67,8 @@ void test_diff_workdir__to_index(void) #ifdef GIT_PERF cl_assert_equal_sz( 13 /* in root */ + 3 /* in subdir */, diff->stat_calls); - cl_assert_equal_sz(9, diff->oid_calculations); - cl_assert_equal_sz(2, diff->submodule_lookups); + cl_assert_equal_sz(5, diff->oid_calculations); + cl_assert_equal_sz(1, diff->submodule_lookups); #endif } diff --git a/tests/status/worktree.c b/tests/status/worktree.c index def3d60f0..f51c61864 100644 --- a/tests/status/worktree.c +++ b/tests/status/worktree.c @@ -578,7 +578,11 @@ void test_status_worktree__line_endings_dont_count_as_changes_with_autocrlf(void cl_git_pass(git_status_file(&status, repo, "current_file")); - cl_assert_equal_i(GIT_STATUS_CURRENT, status); + /* stat data on file should no longer match stat cache, even though + * file diff will be empty because of line-ending conversion - matches + * the Git command-line behavior here. + */ + cl_assert_equal_i(GIT_STATUS_WT_MODIFIED, status); } void test_status_worktree__line_endings_dont_count_as_changes_with_autocrlf_issue_1397(void) From 0fc8e1f6bd9a5148d3a262142e9a70126f5c3a42 Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Mon, 28 Apr 2014 14:34:55 -0700 Subject: [PATCH 03/13] Lay groundwork for updating stat cache in diff This reorganized the diff OID calculation to make it easier to correctly update the stat cache during a diff once the flags to do so are enabled. This includes marking the path of a git_index_entry as const so we can make a "fake" git_index_entry with a "const char *" path and not get warnings. I was a little surprised at how unobtrusive this change was, but I think it's probably a good thing. --- include/git2/index.h | 2 +- src/checkout.c | 3 +-- src/diff.c | 44 ++++++++++++++++++++++++++++++-------------- src/diff.h | 4 +++- src/index.c | 6 ++---- 5 files changed, 37 insertions(+), 22 deletions(-) diff --git a/include/git2/index.h b/include/git2/index.h index 05e58a632..cdb87282c 100644 --- a/include/git2/index.h +++ b/include/git2/index.h @@ -61,7 +61,7 @@ typedef struct git_index_entry { unsigned short flags; unsigned short flags_extended; - char *path; + const char *path; } git_index_entry; /** diff --git a/src/checkout.c b/src/checkout.c index 93d6bc9c5..d94cb0c7d 100644 --- a/src/checkout.c +++ b/src/checkout.c @@ -184,8 +184,7 @@ static bool checkout_is_workdir_modified( if (baseitem->size && wditem->file_size != baseitem->size) return true; - if (git_diff__oid_for_file( - &oid, data->diff, wditem->path, wditem->mode, wditem->file_size) < 0) + if (git_diff__oid_for_entry(&oid, data->diff, wditem) < 0) return false; return (git_oid__cmp(&baseitem->id, &oid) != 0); diff --git a/src/diff.c b/src/diff.c index 4c028ca4e..eae4543fc 100644 --- a/src/diff.c +++ b/src/diff.c @@ -515,39 +515,53 @@ int git_diff__oid_for_file( const char *path, uint16_t mode, git_off_t size) +{ + git_index_entry entry; + + memset(&entry, 0, sizeof(entry)); + entry.mode = mode; + entry.file_size = size; + entry.path = (char *)path; + + return git_diff__oid_for_entry(out, diff, &entry); +} + +int git_diff__oid_for_entry( + git_oid *out, git_diff *diff, const git_index_entry *src) { int error = 0; git_buf full_path = GIT_BUF_INIT; + git_index_entry entry = *src; git_filter_list *fl = NULL; memset(out, 0, sizeof(*out)); if (git_buf_joinpath( - &full_path, git_repository_workdir(diff->repo), path) < 0) + &full_path, git_repository_workdir(diff->repo), entry.path) < 0) return -1; - if (!mode) { + if (!entry.mode) { struct stat st; GIT_PERF_INC(diff->stat_calls); if (p_stat(full_path.ptr, &st) < 0) { - error = git_path_set_error(errno, path, "stat"); + error = git_path_set_error(errno, entry.path, "stat"); git_buf_free(&full_path); return error; } - mode = st.st_mode; - size = st.st_size; + git_index_entry__init_from_stat( + &entry, &st, (diff->diffcaps & GIT_DIFFCAPS_TRUST_MODE_BITS) != 0); } /* calculate OID for file if possible */ - if (S_ISGITLINK(mode)) { + if (S_ISGITLINK(entry.mode)) { git_submodule *sm; GIT_PERF_INC(diff->submodule_lookups); - if (!git_submodule_lookup(&sm, diff->repo, path)) { + if (!git_submodule_lookup(&sm, diff->repo, entry.path)) { const git_oid *sm_oid = git_submodule_wd_id(sm); if (sm_oid) git_oid_cpy(out, sm_oid); @@ -558,14 +572,15 @@ int git_diff__oid_for_file( */ giterr_clear(); } - } else if (S_ISLNK(mode)) { + } else if (S_ISLNK(entry.mode)) { GIT_PERF_INC(diff->oid_calculations); error = git_odb__hashlink(out, full_path.ptr); - } else if (!git__is_sizet(size)) { - giterr_set(GITERR_OS, "File size overflow (for 32-bits) on '%s'", path); + } else if (!git__is_sizet(entry.file_size)) { + giterr_set(GITERR_OS, "File size overflow (for 32-bits) on '%s'", + entry.path); error = -1; } else if (!(error = git_filter_list_load( - &fl, diff->repo, NULL, path, GIT_FILTER_TO_ODB))) + &fl, diff->repo, NULL, entry.path, GIT_FILTER_TO_ODB))) { int fd = git_futils_open_ro(full_path.ptr); if (fd < 0) @@ -573,13 +588,15 @@ int git_diff__oid_for_file( else { GIT_PERF_INC(diff->oid_calculations); error = git_odb__hashfd_filtered( - out, fd, (size_t)size, GIT_OBJ_BLOB, fl); + out, fd, (size_t)entry.file_size, GIT_OBJ_BLOB, fl); p_close(fd); } git_filter_list_free(fl); } + /* TODO: update index for entry if requested */ + git_buf_free(&full_path); return error; } @@ -759,8 +776,7 @@ static int maybe_modified( */ if (modified_uncertain && git_oid_iszero(&nitem->id)) { if (git_oid_iszero(&noid)) { - if ((error = git_diff__oid_for_file(&noid, - diff, nitem->path, nitem->mode, nitem->file_size)) < 0) + if ((error = git_diff__oid_for_entry(&noid, diff, nitem)) < 0) return error; } diff --git a/src/diff.h b/src/diff.h index 491fc4667..8fa3c9b7b 100644 --- a/src/diff.h +++ b/src/diff.h @@ -95,7 +95,9 @@ extern int git_diff_delta__format_file_header( int oid_strlen); extern int git_diff__oid_for_file( - git_oid *oit, git_diff *, const char *, uint16_t, git_off_t); + git_oid *out, git_diff *, const char *, uint16_t, git_off_t); +extern int git_diff__oid_for_entry( + git_oid *out, git_diff *, const git_index_entry *entry); extern int git_diff__from_iterators( git_diff **diff_ptr, diff --git a/src/index.c b/src/index.c index c044af402..8a7f29279 100644 --- a/src/index.c +++ b/src/index.c @@ -842,7 +842,7 @@ static int index_entry_reuc_init(git_index_reuc_entry **reuc_out, static void index_entry_cpy(git_index_entry *tgt, const git_index_entry *src) { - char *tgt_path = tgt->path; + const char *tgt_path = tgt->path; memcpy(tgt, src, sizeof(*tgt)); tgt->path = tgt_path; /* reset to existing path data */ } @@ -2282,9 +2282,7 @@ static int read_tree_cb( entry->mode == old_entry->mode && git_oid_equal(&entry->id, &old_entry->id)) { - char *oldpath = entry->path; - memcpy(entry, old_entry, sizeof(*entry)); - entry->path = oldpath; + index_entry_cpy(entry, old_entry); entry->flags_extended = 0; } From 94fb4aadc80c927a59696dc01db03f3a0629dae7 Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Mon, 28 Apr 2014 14:48:41 -0700 Subject: [PATCH 04/13] Add diff option to update index stat cache When diff is scanning the working directory, if it finds a file where it is not sure if the index entry matches the working dir, it will recalculate the OID (which is pretty expensive). This adds a new flag to diff so that if the OID calculation finds that the file actually has not changed (i.e. just the modified time was altered or such), then it will refresh the stat cache in the index so that future calls to diff will not have to check the oid again. --- include/git2/diff.h | 7 ++++ src/checkout.c | 2 +- src/diff.c | 36 ++++++++++++++++-- src/diff.h | 2 +- tests/diff/workdir.c | 87 ++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 128 insertions(+), 6 deletions(-) diff --git a/include/git2/diff.h b/include/git2/diff.h index 273f471b6..afdb2c981 100644 --- a/include/git2/diff.h +++ b/include/git2/diff.h @@ -145,6 +145,13 @@ typedef enum { */ GIT_DIFF_ENABLE_FAST_UNTRACKED_DIRS = (1u << 14), + /** When diff finds a file in the working directory with stat + * information different from the index, but the OID ends up being the + * same, write the correct stat information into the index. Note: + * without this flag, diff will always leave the index untouched. + */ + GIT_DIFF_UPDATE_INDEX = (1u << 15), + /* * Options controlling how output will be generated */ diff --git a/src/checkout.c b/src/checkout.c index d94cb0c7d..727911694 100644 --- a/src/checkout.c +++ b/src/checkout.c @@ -184,7 +184,7 @@ static bool checkout_is_workdir_modified( if (baseitem->size && wditem->file_size != baseitem->size) return true; - if (git_diff__oid_for_entry(&oid, data->diff, wditem) < 0) + if (git_diff__oid_for_entry(&oid, data->diff, wditem, NULL) < 0) return false; return (git_oid__cmp(&baseitem->id, &oid) != 0); diff --git a/src/diff.c b/src/diff.c index eae4543fc..caed8bf40 100644 --- a/src/diff.c +++ b/src/diff.c @@ -442,6 +442,14 @@ static int diff_list_apply_options( diff->new_src = tmp_src; } + /* Unset UPDATE_INDEX unless diffing workdir and index */ + if (DIFF_FLAG_IS_SET(diff, GIT_DIFF_UPDATE_INDEX) && + (!(diff->old_src == GIT_ITERATOR_TYPE_WORKDIR || + diff->new_src == GIT_ITERATOR_TYPE_WORKDIR) || + !(diff->old_src == GIT_ITERATOR_TYPE_INDEX || + diff->new_src == GIT_ITERATOR_TYPE_INDEX))) + diff->opts.flags &= ~GIT_DIFF_UPDATE_INDEX; + /* if ignore_submodules not explicitly set, check diff config */ if (diff->opts.ignore_submodules <= 0) { const git_config_entry *entry; @@ -523,11 +531,14 @@ int git_diff__oid_for_file( entry.file_size = size; entry.path = (char *)path; - return git_diff__oid_for_entry(out, diff, &entry); + return git_diff__oid_for_entry(out, diff, &entry, NULL); } int git_diff__oid_for_entry( - git_oid *out, git_diff *diff, const git_index_entry *src) + git_oid *out, + git_diff *diff, + const git_index_entry *src, + const git_oid *update_match) { int error = 0; git_buf full_path = GIT_BUF_INIT; @@ -595,7 +606,16 @@ int git_diff__oid_for_entry( git_filter_list_free(fl); } - /* TODO: update index for entry if requested */ + /* update index for entry if requested */ + if (!error && update_match && git_oid_equal(out, update_match)) { + git_index *idx; + + if (!(error = git_repository_index(&idx, diff->repo))) { + memcpy(&entry.id, out, sizeof(entry.id)); + error = git_index_add(idx, &entry); + git_index_free(idx); + } + } git_buf_free(&full_path); return error; @@ -776,7 +796,12 @@ static int maybe_modified( */ if (modified_uncertain && git_oid_iszero(&nitem->id)) { if (git_oid_iszero(&noid)) { - if ((error = git_diff__oid_for_entry(&noid, diff, nitem)) < 0) + const git_oid *update_check = + DIFF_FLAG_IS_SET(diff, GIT_DIFF_UPDATE_INDEX) ? + &oitem->id : NULL; + + if ((error = git_diff__oid_for_entry( + &noid, diff, nitem, update_check)) < 0) return error; } @@ -1208,6 +1233,9 @@ int git_diff_index_to_workdir( &b, repo, GIT_ITERATOR_DONT_AUTOEXPAND, pfx, pfx) ); + if (!error && DIFF_FLAG_IS_SET(*diff, GIT_DIFF_UPDATE_INDEX)) + error = git_index_write(index); + return error; } diff --git a/src/diff.h b/src/diff.h index 8fa3c9b7b..b2b7dba70 100644 --- a/src/diff.h +++ b/src/diff.h @@ -97,7 +97,7 @@ extern int git_diff_delta__format_file_header( extern int git_diff__oid_for_file( git_oid *out, git_diff *, const char *, uint16_t, git_off_t); extern int git_diff__oid_for_entry( - git_oid *out, git_diff *, const git_index_entry *entry); + git_oid *out, git_diff *, const git_index_entry *, const git_oid *update); extern int git_diff__from_iterators( git_diff **diff_ptr, diff --git a/tests/diff/workdir.c b/tests/diff/workdir.c index 84c8866e0..9e4608e9d 100644 --- a/tests/diff/workdir.c +++ b/tests/diff/workdir.c @@ -1502,3 +1502,90 @@ void test_diff_workdir__with_stale_index(void) git_index_free(idx); } + +static int touch_file(void *payload, git_buf *path) +{ + int fd; + char b; + + GIT_UNUSED(payload); + if (git_path_isdir(path->ptr)) + return 0; + + cl_assert((fd = p_open(path->ptr, O_RDWR)) >= 0); + cl_assert_equal_i(1, p_read(fd, &b, 1)); + cl_must_pass(p_lseek(fd, 0, SEEK_SET)); + cl_must_pass(p_write(fd, &b, 1)); + cl_must_pass(p_close(fd)); + + return 0; +} + +static void basic_diff_status(git_diff **out, const git_diff_options *opts) +{ + diff_expects exp; + + cl_git_pass(git_diff_index_to_workdir(out, g_repo, NULL, opts)); + + memset(&exp, 0, sizeof(exp)); + + cl_git_pass(git_diff_foreach( + *out, diff_file_cb, diff_hunk_cb, diff_line_cb, &exp)); + + cl_assert_equal_i(13, exp.files); + cl_assert_equal_i(0, exp.file_status[GIT_DELTA_ADDED]); + cl_assert_equal_i(4, exp.file_status[GIT_DELTA_DELETED]); + cl_assert_equal_i(4, exp.file_status[GIT_DELTA_MODIFIED]); + cl_assert_equal_i(1, exp.file_status[GIT_DELTA_IGNORED]); + cl_assert_equal_i(4, exp.file_status[GIT_DELTA_UNTRACKED]); +} + +void test_diff_workdir__can_update_index(void) +{ + git_diff_options opts = GIT_DIFF_OPTIONS_INIT; + git_diff *diff = NULL; + + g_repo = cl_git_sandbox_init("status"); + + /* touch all the files so stat times are different */ + { + git_buf path = GIT_BUF_INIT; + cl_git_pass(git_buf_sets(&path, "status")); + cl_git_pass(git_path_direach(&path, 0, touch_file, NULL)); + git_buf_free(&path); + } + + opts.flags |= GIT_DIFF_INCLUDE_IGNORED | GIT_DIFF_INCLUDE_UNTRACKED; + + basic_diff_status(&diff, &opts); +#ifdef GIT_PERF + cl_assert_equal_sz(diff->stat_calls, 13 + 3); + cl_assert_equal_sz(diff->oid_calculations, 5); + cl_assert_equal_sz(diff->submodule_lookups, 1); +#endif + + git_diff_free(diff); + + /* now allow diff to update stat cache */ + opts.flags |= GIT_DIFF_UPDATE_INDEX; + + basic_diff_status(&diff, &opts); +#ifdef GIT_PERF + cl_assert_equal_sz(diff->stat_calls, 13 + 3); + cl_assert_equal_sz(diff->oid_calculations, 5); + cl_assert_equal_sz(diff->submodule_lookups, 1); +#endif + + git_diff_free(diff); + + /* now if we do it again, we should see fewer OID calculations */ + + basic_diff_status(&diff, &opts); +#ifdef GIT_PERF + cl_assert_equal_sz(diff->stat_calls, 13 + 3); + cl_assert_equal_sz(diff->oid_calculations, 0); /* Yay */ + cl_assert_equal_sz(diff->submodule_lookups, 1); +#endif + + git_diff_free(diff); +} From cd424ad5518c7cfbba10a764d7bc097377ec3995 Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Mon, 28 Apr 2014 16:39:53 -0700 Subject: [PATCH 05/13] Add GIT_STATUS_OPT_UPDATE_INDEX and use trace API This adds an option to refresh the stat cache while generating status. It also rips out the GIT_PERF stuff I had an makes use of the trace API to keep statistics about what happens during diff. --- CMakeLists.txt | 5 --- include/git2/status.h | 6 +++ src/diff.c | 20 ++++----- src/diff.h | 5 --- src/iterator.c | 5 ++- src/iterator.h | 4 -- src/status.c | 29 +++++++++++-- src/util.h | 8 ---- tests/diff/workdir.c | 66 ++++++++++++++++++++---------- tests/status/worktree.c | 91 +++++++++++++++++++++++++++++++++++++---- 10 files changed, 171 insertions(+), 68 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 83e03d6cd..884c9bcf1 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -37,7 +37,6 @@ OPTION( ANDROID "Build for android NDK" OFF ) OPTION( USE_ICONV "Link with and use iconv library" OFF ) OPTION( USE_SSH "Link with libssh to enable SSH support" ON ) OPTION( VALGRIND "Configure build for valgrind" OFF ) -OPTION( PERF_STATS "Internally track performance data" OFF ) IF(${CMAKE_SYSTEM_NAME} MATCHES "Darwin") SET( USE_ICONV ON ) @@ -353,10 +352,6 @@ IF (THREADSAFE) ADD_DEFINITIONS(-DGIT_THREADS) ENDIF() -IF (PERF_STATS) - ADD_DEFINITIONS(-DGIT_PERF) -ENDIF() - ADD_DEFINITIONS(-D_FILE_OFFSET_BITS=64) # Collect sourcefiles diff --git a/include/git2/status.h b/include/git2/status.h index 6af45c7dd..ee2f33287 100644 --- a/include/git2/status.h +++ b/include/git2/status.h @@ -121,6 +121,11 @@ typedef enum { * - GIT_STATUS_OPT_NO_REFRESH bypasses the default status behavior of * doing a "soft" index reload (i.e. reloading the index data if the * file on disk has been modified outside libgit2). + * - GIT_STATUS_OPT_UPDATE_INDEX tells libgit2 to refresh the stat cache + * in the index for files that are unchanged but have out of date stat + * information in the index. It will result in less work being done on + * subsequent calls to get status. This is mutually exclusive with the + * NO_REFRESH option. * * Calling `git_status_foreach()` is like calling the extended version * with: GIT_STATUS_OPT_INCLUDE_IGNORED, GIT_STATUS_OPT_INCLUDE_UNTRACKED, @@ -141,6 +146,7 @@ typedef enum { GIT_STATUS_OPT_SORT_CASE_INSENSITIVELY = (1u << 10), GIT_STATUS_OPT_RENAMES_FROM_REWRITES = (1u << 11), GIT_STATUS_OPT_NO_REFRESH = (1u << 12), + GIT_STATUS_OPT_UPDATE_INDEX = (1u << 13), } git_status_opt_t; #define GIT_STATUS_OPT_DEFAULTS \ diff --git a/src/diff.c b/src/diff.c index caed8bf40..8b7433c62 100644 --- a/src/diff.c +++ b/src/diff.c @@ -14,6 +14,7 @@ #include "index.h" #include "odb.h" #include "submodule.h" +#include "trace.h" #define DIFF_FLAG_IS_SET(DIFF,FLAG) (((DIFF)->opts.flags & (FLAG)) != 0) #define DIFF_FLAG_ISNT_SET(DIFF,FLAG) (((DIFF)->opts.flags & (FLAG)) == 0) @@ -554,8 +555,7 @@ int git_diff__oid_for_entry( if (!entry.mode) { struct stat st; - GIT_PERF_INC(diff->stat_calls); - + git_trace(GIT_TRACE_TRACE, "stat=1"); if (p_stat(full_path.ptr, &st) < 0) { error = git_path_set_error(errno, entry.path, "stat"); git_buf_free(&full_path); @@ -570,8 +570,7 @@ int git_diff__oid_for_entry( if (S_ISGITLINK(entry.mode)) { git_submodule *sm; - GIT_PERF_INC(diff->submodule_lookups); - + git_trace(GIT_TRACE_TRACE, "submodule_lookup=1"); if (!git_submodule_lookup(&sm, diff->repo, entry.path)) { const git_oid *sm_oid = git_submodule_wd_id(sm); if (sm_oid) @@ -584,7 +583,7 @@ int git_diff__oid_for_entry( giterr_clear(); } } else if (S_ISLNK(entry.mode)) { - GIT_PERF_INC(diff->oid_calculations); + git_trace(GIT_TRACE_TRACE, "oid_calculation=1"); error = git_odb__hashlink(out, full_path.ptr); } else if (!git__is_sizet(entry.file_size)) { giterr_set(GITERR_OS, "File size overflow (for 32-bits) on '%s'", @@ -597,7 +596,7 @@ int git_diff__oid_for_entry( if (fd < 0) error = fd; else { - GIT_PERF_INC(diff->oid_calculations); + git_trace(GIT_TRACE_TRACE, "oid_calculation=1"); error = git_odb__hashfd_filtered( out, fd, (size_t)entry.file_size, GIT_OBJ_BLOB, fl); p_close(fd); @@ -656,7 +655,7 @@ static int maybe_modified_submodule( ign == GIT_SUBMODULE_IGNORE_ALL) return 0; - GIT_PERF_INC(diff->submodule_lookups); + git_trace(GIT_TRACE_TRACE, "submodule_lookup=1"); if ((error = git_submodule_lookup( &sub, diff->repo, info->nitem->path)) < 0) { @@ -966,7 +965,7 @@ static int handle_unmatched_new_item( delta_type = GIT_DELTA_ADDED; else if (nitem->mode == GIT_FILEMODE_COMMIT) { - GIT_PERF_INC(diff->submodule_lookups); + git_trace(GIT_TRACE_TRACE, "submodule_lookup=1"); /* ignore things that are not actual submodules */ if (git_submodule_lookup(NULL, info->repo, nitem->path) != 0) { @@ -1120,11 +1119,6 @@ int git_diff__from_iterators( error = 0; } - GIT_PERF_ADD(diff->stat_calls, old_iter->stat_calls); - GIT_PERF_ADD(diff->stat_calls, new_iter->stat_calls); - GIT_PERF_ADD(diff->submodule_lookups, old_iter->submodule_lookups); - GIT_PERF_ADD(diff->submodule_lookups, new_iter->submodule_lookups); - cleanup: if (!error) *diff_ptr = diff; diff --git a/src/diff.h b/src/diff.h index b2b7dba70..2e7ce0b7d 100644 --- a/src/diff.h +++ b/src/diff.h @@ -62,11 +62,6 @@ struct git_diff { git_iterator_type_t old_src; git_iterator_type_t new_src; uint32_t diffcaps; -#ifdef GIT_PERF - size_t stat_calls; - size_t oid_calculations; - size_t submodule_lookups; -#endif int (*strcomp)(const char *, const char *); int (*strncomp)(const char *, const char *, size_t); diff --git a/src/iterator.c b/src/iterator.c index 03058b956..bebdeba84 100644 --- a/src/iterator.c +++ b/src/iterator.c @@ -11,6 +11,7 @@ #include "ignore.h" #include "buffer.h" #include "submodule.h" +#include "trace.h" #include #define ITERATOR_SET_CB(P,NAME_LC) do { \ @@ -1017,7 +1018,7 @@ static int fs_iterator__expand_dir(fs_iterator *fi) return GIT_ENOTFOUND; } - GIT_PERF_ADD(fi->base.stat_calls, ff->entries.length); + git_trace(GIT_TRACE_TRACE, "stat=%ld", (long)ff->entries.length); fs_iterator__seek_frame_start(fi, ff); @@ -1309,7 +1310,7 @@ static int workdir_iterator__enter_dir(fs_iterator *fi) if (!S_ISDIR(entry->st.st_mode) || !strcmp(GIT_DIR, entry->path)) continue; - GIT_PERF_INC(fi->base.submodule_lookups); + git_trace(GIT_TRACE_TRACE, "submodule_lookup=1"); if (git_submodule__is_submodule(fi->base.repo, entry->path)) { entry->st.st_mode = GIT_FILEMODE_COMMIT; entry->path_len--; diff --git a/src/iterator.h b/src/iterator.h index 2968b8c0c..ba9c1e486 100644 --- a/src/iterator.h +++ b/src/iterator.h @@ -53,10 +53,6 @@ struct git_iterator { char *end; int (*prefixcomp)(const char *str, const char *prefix); unsigned int flags; -#ifdef GIT_PERF - size_t stat_calls; - size_t submodule_lookups; -#endif }; extern int git_iterator_for_nothing( diff --git a/src/status.c b/src/status.c index e1f8e06ae..e418cf7b6 100644 --- a/src/status.c +++ b/src/status.c @@ -225,6 +225,28 @@ static git_status_list *git_status_list_alloc(git_index *index) return status; } +static int status_validate_options(const git_status_options *opts) +{ + if (!opts) + return 0; + + GITERR_CHECK_VERSION(opts, GIT_STATUS_OPTIONS_VERSION, "git_status_options"); + + if (opts->show > GIT_STATUS_SHOW_WORKDIR_ONLY) { + giterr_set(GITERR_INVALID, "Unknown status 'show' option"); + return -1; + } + + if ((opts->flags & GIT_STATUS_OPT_NO_REFRESH) != 0 && + (opts->flags & GIT_STATUS_OPT_UPDATE_INDEX) != 0) { + giterr_set(GITERR_INVALID, "Updating index from status " + "is not allowed when index refresh is disabled"); + return -1; + } + + return 0; +} + int git_status_list_new( git_status_list **out, git_repository *repo, @@ -240,11 +262,10 @@ int git_status_list_new( int error = 0; unsigned int flags = opts ? opts->flags : GIT_STATUS_OPT_DEFAULTS; - assert(show <= GIT_STATUS_SHOW_WORKDIR_ONLY); - *out = NULL; - GITERR_CHECK_VERSION(opts, GIT_STATUS_OPTIONS_VERSION, "git_status_options"); + if (status_validate_options(opts) < 0) + return -1; if ((error = git_repository__ensure_not_bare(repo, "status")) < 0 || (error = git_repository_index(&index, repo)) < 0) @@ -287,6 +308,8 @@ int git_status_list_new( diffopt.flags = diffopt.flags | GIT_DIFF_RECURSE_IGNORED_DIRS; if ((flags & GIT_STATUS_OPT_EXCLUDE_SUBMODULES) != 0) diffopt.flags = diffopt.flags | GIT_DIFF_IGNORE_SUBMODULES; + if ((flags & GIT_STATUS_OPT_UPDATE_INDEX) != 0) + diffopt.flags = diffopt.flags | GIT_DIFF_UPDATE_INDEX; if ((flags & GIT_STATUS_OPT_RENAMES_FROM_REWRITES) != 0) findopt.flags = findopt.flags | diff --git a/src/util.h b/src/util.h index be7a16ef8..6fb2dc0f4 100644 --- a/src/util.h +++ b/src/util.h @@ -436,12 +436,4 @@ GIT_INLINE(double) git__timer(void) #endif -#ifdef GIT_PERF -# define GIT_PERF_INC(counter) (counter)++ -# define GIT_PERF_ADD(counter,val) (counter) += (val) -#else -# define GIT_PERF_INC(counter) 0 -# define GIT_PERF_ADD(counter,val) 0 -#endif - #endif /* INCLUDE_util_h__ */ diff --git a/tests/diff/workdir.c b/tests/diff/workdir.c index 9e4608e9d..a225ebc89 100644 --- a/tests/diff/workdir.c +++ b/tests/diff/workdir.c @@ -1,21 +1,43 @@ #include "clar_libgit2.h" #include "diff_helpers.h" #include "repository.h" - -#ifdef GIT_PERF -/* access to diff usage statistics */ -# include "diff.h" -#endif +#include static git_repository *g_repo = NULL; +static struct { + size_t stat_calls; + size_t oid_calcs; + size_t submodule_lookups; +} g_diff_perf; + +static void add_stats(git_trace_level_t level, const char *msg) +{ + const char *assign = strchr(msg, '='); + + GIT_UNUSED(level); + + if (!assign) + return; + + if (!strncmp("stat", msg, (assign - msg))) + g_diff_perf.stat_calls += atoi(assign + 1); + else if (!strncmp("submodule_lookup", msg, (assign - msg))) + g_diff_perf.submodule_lookups += atoi(assign + 1); + else if (!strncmp("oid_calculation", msg, (assign - msg))) + g_diff_perf.oid_calcs += atoi(assign + 1); +} + void test_diff_workdir__initialize(void) { + memset(&g_diff_perf, 0, sizeof(g_diff_perf)); + cl_git_pass(git_trace_set(GIT_TRACE_TRACE, add_stats)); } void test_diff_workdir__cleanup(void) { cl_git_sandbox_cleanup(); + cl_git_pass(git_trace_set(0, NULL)); } void test_diff_workdir__to_index(void) @@ -64,11 +86,11 @@ void test_diff_workdir__to_index(void) cl_assert_equal_i(4, exp.line_adds); cl_assert_equal_i(5, exp.line_dels); -#ifdef GIT_PERF +#ifdef GIT_TRACE cl_assert_equal_sz( - 13 /* in root */ + 3 /* in subdir */, diff->stat_calls); - cl_assert_equal_sz(5, diff->oid_calculations); - cl_assert_equal_sz(1, diff->submodule_lookups); + 13 /* in root */ + 3 /* in subdir */, g_diff_perf.stat_calls); + cl_assert_equal_sz(5, g_diff_perf.oid_calcs); + cl_assert_equal_sz(1, g_diff_perf.submodule_lookups); #endif } @@ -1525,6 +1547,8 @@ static void basic_diff_status(git_diff **out, const git_diff_options *opts) { diff_expects exp; + memset(&g_diff_perf, 0, sizeof(g_diff_perf)); + cl_git_pass(git_diff_index_to_workdir(out, g_repo, NULL, opts)); memset(&exp, 0, sizeof(exp)); @@ -1558,10 +1582,10 @@ void test_diff_workdir__can_update_index(void) opts.flags |= GIT_DIFF_INCLUDE_IGNORED | GIT_DIFF_INCLUDE_UNTRACKED; basic_diff_status(&diff, &opts); -#ifdef GIT_PERF - cl_assert_equal_sz(diff->stat_calls, 13 + 3); - cl_assert_equal_sz(diff->oid_calculations, 5); - cl_assert_equal_sz(diff->submodule_lookups, 1); +#ifdef GIT_TRACE + cl_assert_equal_sz(13 + 3, g_diff_perf.stat_calls); + cl_assert_equal_sz(5, g_diff_perf.oid_calcs); + cl_assert_equal_sz(1, g_diff_perf.submodule_lookups); #endif git_diff_free(diff); @@ -1570,10 +1594,10 @@ void test_diff_workdir__can_update_index(void) opts.flags |= GIT_DIFF_UPDATE_INDEX; basic_diff_status(&diff, &opts); -#ifdef GIT_PERF - cl_assert_equal_sz(diff->stat_calls, 13 + 3); - cl_assert_equal_sz(diff->oid_calculations, 5); - cl_assert_equal_sz(diff->submodule_lookups, 1); +#ifdef GIT_TRACE + cl_assert_equal_sz(13 + 3, g_diff_perf.stat_calls); + cl_assert_equal_sz(5, g_diff_perf.oid_calcs); + cl_assert_equal_sz(1, g_diff_perf.submodule_lookups); #endif git_diff_free(diff); @@ -1581,10 +1605,10 @@ void test_diff_workdir__can_update_index(void) /* now if we do it again, we should see fewer OID calculations */ basic_diff_status(&diff, &opts); -#ifdef GIT_PERF - cl_assert_equal_sz(diff->stat_calls, 13 + 3); - cl_assert_equal_sz(diff->oid_calculations, 0); /* Yay */ - cl_assert_equal_sz(diff->submodule_lookups, 1); +#ifdef GIT_TRACE + cl_assert_equal_sz(13 + 3, g_diff_perf.stat_calls); + cl_assert_equal_sz(0, g_diff_perf.oid_calcs); /* Yay */ + cl_assert_equal_sz(1, g_diff_perf.submodule_lookups); #endif git_diff_free(diff); diff --git a/tests/status/worktree.c b/tests/status/worktree.c index f51c61864..0b94fb1a0 100644 --- a/tests/status/worktree.c +++ b/tests/status/worktree.c @@ -5,6 +5,36 @@ #include "posix.h" #include "util.h" #include "path.h" +#include + +static struct { + size_t stat_calls; + size_t oid_calcs; + size_t submodule_lookups; +} g_diff_perf; + +static void add_stats(git_trace_level_t level, const char *msg) +{ + const char *assign = strchr(msg, '='); + + GIT_UNUSED(level); + + if (!assign) + return; + + if (!strncmp("stat", msg, (assign - msg))) + g_diff_perf.stat_calls += atoi(assign + 1); + else if (!strncmp("submodule_lookup", msg, (assign - msg))) + g_diff_perf.submodule_lookups += atoi(assign + 1); + else if (!strncmp("oid_calculation", msg, (assign - msg))) + g_diff_perf.oid_calcs += atoi(assign + 1); +} + +void test_status_worktree__initialize(void) +{ + memset(&g_diff_perf, 0, sizeof(g_diff_perf)); + cl_git_pass(git_trace_set(GIT_TRACE_TRACE, add_stats)); +} /** * Cleanup @@ -15,6 +45,7 @@ void test_status_worktree__cleanup(void) { cl_git_sandbox_cleanup(); + cl_git_pass(git_trace_set(0, NULL)); } /** @@ -40,11 +71,15 @@ void test_status_worktree__whole_repository(void) cl_assert_equal_i(0, counts.wrong_sorted_path); } -void assert_show(const int entry_counts, const char *entry_paths[], - const unsigned int entry_statuses[], git_status_show_t show) +void assert_show( + const int entry_counts, + const char *entry_paths[], + const unsigned int entry_statuses[], + git_repository *repo, + git_status_show_t show, + unsigned int extra_flags) { status_entry_counts counts; - git_repository *repo = cl_git_sandbox_init("status"); git_status_options opts = GIT_STATUS_OPTIONS_INIT; memset(&counts, 0x0, sizeof(status_entry_counts)); @@ -52,7 +87,7 @@ void assert_show(const int entry_counts, const char *entry_paths[], counts.expected_paths = entry_paths; counts.expected_statuses = entry_statuses; - opts.flags = GIT_STATUS_OPT_DEFAULTS; + opts.flags = GIT_STATUS_OPT_DEFAULTS | extra_flags; opts.show = show; cl_git_pass( @@ -67,19 +102,19 @@ void assert_show(const int entry_counts, const char *entry_paths[], void test_status_worktree__show_index_and_workdir(void) { assert_show(entry_count0, entry_paths0, entry_statuses0, - GIT_STATUS_SHOW_INDEX_AND_WORKDIR); + cl_git_sandbox_init("status"), GIT_STATUS_SHOW_INDEX_AND_WORKDIR, 0); } void test_status_worktree__show_index_only(void) { assert_show(entry_count5, entry_paths5, entry_statuses5, - GIT_STATUS_SHOW_INDEX_ONLY); + cl_git_sandbox_init("status"), GIT_STATUS_SHOW_INDEX_ONLY, 0); } void test_status_worktree__show_workdir_only(void) { assert_show(entry_count6, entry_paths6, entry_statuses6, - GIT_STATUS_SHOW_WORKDIR_ONLY); + cl_git_sandbox_init("status"), GIT_STATUS_SHOW_WORKDIR_ONLY, 0); } /* this test is equivalent to t18-status.c:statuscb1 */ @@ -877,3 +912,45 @@ void test_status_worktree__long_filenames(void) cl_assert_equal_i(0, counts.wrong_sorted_path); } +/* The update stat cache tests mostly just mirror other tests and try + * to make sure that updating the stat cache doesn't change the results + * while reducing the amount of work that needs to be done + */ + +void test_status_worktree__update_stat_cache_0(void) +{ + git_repository *repo = cl_git_sandbox_init("status"); + + assert_show(entry_count0, entry_paths0, entry_statuses0, + repo, GIT_STATUS_SHOW_INDEX_AND_WORKDIR, 0); + +#ifdef GIT_TRACE + cl_assert_equal_sz(13 + 3, g_diff_perf.stat_calls); + cl_assert_equal_sz(5, g_diff_perf.oid_calcs); + cl_assert_equal_sz(1, g_diff_perf.submodule_lookups); + + memset(&g_diff_perf, 0, sizeof(g_diff_perf)); +#endif + + assert_show(entry_count0, entry_paths0, entry_statuses0, + repo, GIT_STATUS_SHOW_INDEX_AND_WORKDIR, GIT_STATUS_OPT_UPDATE_INDEX); + +#ifdef GIT_TRACE + cl_assert_equal_sz(13 + 3, g_diff_perf.stat_calls); + cl_assert_equal_sz(5, g_diff_perf.oid_calcs); + cl_assert_equal_sz(1, g_diff_perf.submodule_lookups); + + memset(&g_diff_perf, 0, sizeof(g_diff_perf)); +#endif + + assert_show(entry_count0, entry_paths0, entry_statuses0, + repo, GIT_STATUS_SHOW_INDEX_AND_WORKDIR, 0); + +#ifdef GIT_TRACE + cl_assert_equal_sz(13 + 3, g_diff_perf.stat_calls); + cl_assert_equal_sz(0, g_diff_perf.oid_calcs); + cl_assert_equal_sz(1, g_diff_perf.submodule_lookups); + + memset(&g_diff_perf, 0, sizeof(g_diff_perf)); +#endif +} From 225aab5d6a611076b22f00ae5a28184d92b5259c Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Mon, 28 Apr 2014 16:47:39 -0700 Subject: [PATCH 06/13] Don't use trace if GIT_TRACE not defined --- tests/diff/workdir.c | 8 ++++++++ tests/status/worktree.c | 6 ++++++ 2 files changed, 14 insertions(+) diff --git a/tests/diff/workdir.c b/tests/diff/workdir.c index a225ebc89..0fd41d3e0 100644 --- a/tests/diff/workdir.c +++ b/tests/diff/workdir.c @@ -5,6 +5,7 @@ static git_repository *g_repo = NULL; +#ifdef GIT_TRACE static struct { size_t stat_calls; size_t oid_calcs; @@ -27,17 +28,22 @@ static void add_stats(git_trace_level_t level, const char *msg) else if (!strncmp("oid_calculation", msg, (assign - msg))) g_diff_perf.oid_calcs += atoi(assign + 1); } +#endif void test_diff_workdir__initialize(void) { +#ifdef GIT_TRACE memset(&g_diff_perf, 0, sizeof(g_diff_perf)); cl_git_pass(git_trace_set(GIT_TRACE_TRACE, add_stats)); +#endif } void test_diff_workdir__cleanup(void) { cl_git_sandbox_cleanup(); +#ifdef GIT_TRACE cl_git_pass(git_trace_set(0, NULL)); +#endif } void test_diff_workdir__to_index(void) @@ -1547,7 +1553,9 @@ static void basic_diff_status(git_diff **out, const git_diff_options *opts) { diff_expects exp; +#ifdef GIT_TRACE memset(&g_diff_perf, 0, sizeof(g_diff_perf)); +#endif cl_git_pass(git_diff_index_to_workdir(out, g_repo, NULL, opts)); diff --git a/tests/status/worktree.c b/tests/status/worktree.c index 0b94fb1a0..c1d6be982 100644 --- a/tests/status/worktree.c +++ b/tests/status/worktree.c @@ -7,6 +7,7 @@ #include "path.h" #include +#ifdef GIT_TRACE static struct { size_t stat_calls; size_t oid_calcs; @@ -29,11 +30,14 @@ static void add_stats(git_trace_level_t level, const char *msg) else if (!strncmp("oid_calculation", msg, (assign - msg))) g_diff_perf.oid_calcs += atoi(assign + 1); } +#endif void test_status_worktree__initialize(void) { +#ifdef GIT_TRACE memset(&g_diff_perf, 0, sizeof(g_diff_perf)); cl_git_pass(git_trace_set(GIT_TRACE_TRACE, add_stats)); +#endif } /** @@ -45,7 +49,9 @@ void test_status_worktree__initialize(void) void test_status_worktree__cleanup(void) { cl_git_sandbox_cleanup(); +#ifdef GIT_TRACE cl_git_pass(git_trace_set(0, NULL)); +#endif } /** From b23b112dfe8eceb39eaaea2d5e60d971c4371aa0 Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Tue, 29 Apr 2014 11:29:49 -0700 Subject: [PATCH 07/13] Add payloads, bitmaps to trace API This is a proposed adjustment to the trace APIs. This makes the trace levels into a bitmask so that they can be selectively enabled and adds a callback-level payload, plus a message-level payload. This makes it easier for me to a GIT_TRACE_PERF callbacks that are simply bypassed if the PERF level is not set. --- include/git2/trace.h | 43 +++++++++++++++++++++++++++------------ src/diff.c | 12 +++++------ src/iterator.c | 4 ++-- src/trace.c | 15 ++++++++------ src/trace.h | 19 +++++++++-------- tests/diff/diff_helpers.c | 19 +++++++++++++++++ tests/diff/diff_helpers.h | 14 +++++++++++++ tests/diff/workdir.c | 29 ++++---------------------- tests/status/worktree.c | 32 +++++------------------------ tests/trace/trace.c | 39 +++++++++++++++++++---------------- 10 files changed, 122 insertions(+), 104 deletions(-) diff --git a/include/git2/trace.h b/include/git2/trace.h index f9b4d6ff6..867b34612 100644 --- a/include/git2/trace.h +++ b/include/git2/trace.h @@ -20,47 +20,64 @@ GIT_BEGIN_DECL /** - * Available tracing levels. When tracing is set to a particular level, - * callers will be provided tracing at the given level and all lower levels. + * Available tracing messages. Each tracing level can be enabled + * independently or pass GIT_TRACE_ALL to enable all levels. */ typedef enum { /** No tracing will be performed. */ - GIT_TRACE_NONE = 0, + GIT_TRACE_NONE = 0x0000u, + + /** All tracing messages will be sent. */ + GIT_TRACE_ALL = 0xFFFFu, /** Severe errors that may impact the program's execution */ - GIT_TRACE_FATAL = 1, + GIT_TRACE_FATAL = 0x0001u, /** Errors that do not impact the program's execution */ - GIT_TRACE_ERROR = 2, + GIT_TRACE_ERROR = 0x0002u, + GIT_TRACE_ERROR_AND_BELOW = 0x0003u, /** Warnings that suggest abnormal data */ - GIT_TRACE_WARN = 3, + GIT_TRACE_WARN = 0x0004u, + GIT_TRACE_WARN_AND_BELOW = 0x0007u, /** Informational messages about program execution */ - GIT_TRACE_INFO = 4, + GIT_TRACE_INFO = 0x0008u, + GIT_TRACE_INFO_AND_BELOW = 0x000Fu, /** Detailed data that allows for debugging */ - GIT_TRACE_DEBUG = 5, + GIT_TRACE_DEBUG = 0x0010u, /** Exceptionally detailed debugging data */ - GIT_TRACE_TRACE = 6 + GIT_TRACE_TRACE = 0x0020u, + + /** Performance tracking related traces */ + GIT_TRACE_PERF = 0x0040u, } git_trace_level_t; /** * An instance for a tracing function */ -typedef void (*git_trace_callback)(git_trace_level_t level, const char *msg); +typedef void (*git_trace_callback)( + git_trace_level_t level, /* just one bit will be sent */ + void *cb_payload, + void *msg_payload, + const char *msg); /** * Sets the system tracing configuration to the specified level with the * specified callback. When system events occur at a level equal to, or * lower than, the given level they will be reported to the given callback. * - * @param level Level to set tracing to - * @param cb Function to call with trace data + * @param level Bitmask of all enabled trace levels + * @param cb Function to call with trace messages + * @param cb_payload Payload to pass when callback is invoked * @return 0 or an error code */ -GIT_EXTERN(int) git_trace_set(git_trace_level_t level, git_trace_callback cb); +GIT_EXTERN(int) git_trace_set( + git_trace_level_t level, + git_trace_callback cb, + void *cb_payload); /** @} */ GIT_END_DECL diff --git a/src/diff.c b/src/diff.c index 8b7433c62..5a6b127a1 100644 --- a/src/diff.c +++ b/src/diff.c @@ -555,7 +555,7 @@ int git_diff__oid_for_entry( if (!entry.mode) { struct stat st; - git_trace(GIT_TRACE_TRACE, "stat=1"); + git_trace(GIT_TRACE_PERF, NULL, "stat"); if (p_stat(full_path.ptr, &st) < 0) { error = git_path_set_error(errno, entry.path, "stat"); git_buf_free(&full_path); @@ -570,7 +570,7 @@ int git_diff__oid_for_entry( if (S_ISGITLINK(entry.mode)) { git_submodule *sm; - git_trace(GIT_TRACE_TRACE, "submodule_lookup=1"); + git_trace(GIT_TRACE_PERF, NULL, "submodule_lookup"); if (!git_submodule_lookup(&sm, diff->repo, entry.path)) { const git_oid *sm_oid = git_submodule_wd_id(sm); if (sm_oid) @@ -583,7 +583,7 @@ int git_diff__oid_for_entry( giterr_clear(); } } else if (S_ISLNK(entry.mode)) { - git_trace(GIT_TRACE_TRACE, "oid_calculation=1"); + git_trace(GIT_TRACE_PERF, NULL, "oid_calculation"); error = git_odb__hashlink(out, full_path.ptr); } else if (!git__is_sizet(entry.file_size)) { giterr_set(GITERR_OS, "File size overflow (for 32-bits) on '%s'", @@ -596,7 +596,7 @@ int git_diff__oid_for_entry( if (fd < 0) error = fd; else { - git_trace(GIT_TRACE_TRACE, "oid_calculation=1"); + git_trace(GIT_TRACE_PERF, NULL, "oid_calculation"); error = git_odb__hashfd_filtered( out, fd, (size_t)entry.file_size, GIT_OBJ_BLOB, fl); p_close(fd); @@ -655,7 +655,7 @@ static int maybe_modified_submodule( ign == GIT_SUBMODULE_IGNORE_ALL) return 0; - git_trace(GIT_TRACE_TRACE, "submodule_lookup=1"); + git_trace(GIT_TRACE_PERF, NULL, "submodule_lookup"); if ((error = git_submodule_lookup( &sub, diff->repo, info->nitem->path)) < 0) { @@ -965,7 +965,7 @@ static int handle_unmatched_new_item( delta_type = GIT_DELTA_ADDED; else if (nitem->mode == GIT_FILEMODE_COMMIT) { - git_trace(GIT_TRACE_TRACE, "submodule_lookup=1"); + git_trace(GIT_TRACE_PERF, NULL, "submodule_lookup"); /* ignore things that are not actual submodules */ if (git_submodule_lookup(NULL, info->repo, nitem->path) != 0) { diff --git a/src/iterator.c b/src/iterator.c index bebdeba84..0d7e5918d 100644 --- a/src/iterator.c +++ b/src/iterator.c @@ -1018,7 +1018,7 @@ static int fs_iterator__expand_dir(fs_iterator *fi) return GIT_ENOTFOUND; } - git_trace(GIT_TRACE_TRACE, "stat=%ld", (long)ff->entries.length); + git_trace(GIT_TRACE_PERF, &ff->entries.length, "stat"); fs_iterator__seek_frame_start(fi, ff); @@ -1310,7 +1310,7 @@ static int workdir_iterator__enter_dir(fs_iterator *fi) if (!S_ISDIR(entry->st.st_mode) || !strcmp(GIT_DIR, entry->path)) continue; - git_trace(GIT_TRACE_TRACE, "submodule_lookup=1"); + git_trace(GIT_TRACE_PERF, entry->path, "submodule_lookup"); if (git_submodule__is_submodule(fi->base.repo, entry->path)) { entry->st.st_mode = GIT_FILEMODE_COMMIT; entry->path_len--; diff --git a/src/trace.c b/src/trace.c index ee5039f56..6ee2cf2ce 100644 --- a/src/trace.c +++ b/src/trace.c @@ -17,22 +17,25 @@ struct git_trace_data git_trace__data = {0}; #endif -int git_trace_set(git_trace_level_t level, git_trace_callback callback) +int git_trace_set( + git_trace_level_t level, git_trace_callback cb, void *cb_payload) { #ifdef GIT_TRACE - assert(level == 0 || callback != NULL); + assert(level == 0 || cb != NULL); git_trace__data.level = level; - git_trace__data.callback = callback; + git_trace__data.callback = cb; + git_trace__data.callback_payload = cb_payload; GIT_MEMORY_BARRIER; return 0; #else GIT_UNUSED(level); - GIT_UNUSED(callback); + GIT_UNUSED(cb); + GIT_UNUSED(cb_payload); - giterr_set(GITERR_INVALID, - "This version of libgit2 was not built with tracing."); + giterr_set( + GITERR_INVALID, "This version of libgit2 was not built with tracing."); return -1; #endif } diff --git a/src/trace.h b/src/trace.h index 4d4e3bf53..b35e3808f 100644 --- a/src/trace.h +++ b/src/trace.h @@ -15,13 +15,16 @@ struct git_trace_data { git_trace_level_t level; git_trace_callback callback; + void *callback_payload; }; extern struct git_trace_data git_trace__data; GIT_INLINE(void) git_trace__write_fmt( git_trace_level_t level, - const char *fmt, ...) + void *message_payload, + const char *fmt, + ...) { git_trace_callback callback = git_trace__data.callback; git_buf message = GIT_BUF_INIT; @@ -31,18 +34,18 @@ GIT_INLINE(void) git_trace__write_fmt( git_buf_vprintf(&message, fmt, ap); va_end(ap); - callback(level, git_buf_cstr(&message)); + callback( + level, git_trace__data.callback_payload, message_payload, + git_buf_cstr(&message)); git_buf_free(&message); } #define git_trace_level() (git_trace__data.level) -#define git_trace(l, ...) { \ - if (git_trace__data.level >= l && \ - git_trace__data.callback != NULL) { \ - git_trace__write_fmt(l, __VA_ARGS__); \ - } \ - } +#define git_trace(l, p, ...) do { \ + if ((git_trace__data.level & (l)) != 0 && git_trace__data.callback) { \ + git_trace__write_fmt((l), (p), __VA_ARGS__); \ + } } while (0) #else diff --git a/tests/diff/diff_helpers.c b/tests/diff/diff_helpers.c index 279cb20c5..5de9834ba 100644 --- a/tests/diff/diff_helpers.c +++ b/tests/diff/diff_helpers.c @@ -229,3 +229,22 @@ void diff_print_raw(FILE *fp, git_diff *diff) git_diff_print(diff, GIT_DIFF_FORMAT_RAW, git_diff_print_callback__to_file_handle, fp ? fp : stderr)); } + +void diff_perf_track_stats( + git_trace_level_t level, + void *cb_payload, + void *msg_payload, + const char *msg) +{ + diff_perf *data = cb_payload; + + if (!(level & GIT_TRACE_PERF)) + return; + + if (!strcmp("stat", msg)) + data->stat_calls += msg_payload ? *((size_t *)msg_payload) : 1; + else if (!strcmp("submodule_lookup", msg)) + data->submodule_lookups++; + else if (!strcmp("oid_calculation", msg)) + data->oid_calcs++; +} diff --git a/tests/diff/diff_helpers.h b/tests/diff/diff_helpers.h index bf21f4b1f..3ed538702 100644 --- a/tests/diff/diff_helpers.h +++ b/tests/diff/diff_helpers.h @@ -62,3 +62,17 @@ extern int diff_foreach_via_iterator( extern void diff_print(FILE *fp, git_diff *diff); extern void diff_print_raw(FILE *fp, git_diff *diff); + +#include "git2/trace.h" + +typedef struct { + size_t stat_calls; + size_t oid_calcs; + size_t submodule_lookups; +} diff_perf; + +extern void diff_perf_track_stats( + git_trace_level_t level, + void *cb_payload, + void *msg_payload, + const char *msg); diff --git a/tests/diff/workdir.c b/tests/diff/workdir.c index 0fd41d3e0..952c9022b 100644 --- a/tests/diff/workdir.c +++ b/tests/diff/workdir.c @@ -1,40 +1,19 @@ #include "clar_libgit2.h" #include "diff_helpers.h" #include "repository.h" -#include static git_repository *g_repo = NULL; #ifdef GIT_TRACE -static struct { - size_t stat_calls; - size_t oid_calcs; - size_t submodule_lookups; -} g_diff_perf; - -static void add_stats(git_trace_level_t level, const char *msg) -{ - const char *assign = strchr(msg, '='); - - GIT_UNUSED(level); - - if (!assign) - return; - - if (!strncmp("stat", msg, (assign - msg))) - g_diff_perf.stat_calls += atoi(assign + 1); - else if (!strncmp("submodule_lookup", msg, (assign - msg))) - g_diff_perf.submodule_lookups += atoi(assign + 1); - else if (!strncmp("oid_calculation", msg, (assign - msg))) - g_diff_perf.oid_calcs += atoi(assign + 1); -} +static diff_perf g_diff_perf; #endif void test_diff_workdir__initialize(void) { #ifdef GIT_TRACE memset(&g_diff_perf, 0, sizeof(g_diff_perf)); - cl_git_pass(git_trace_set(GIT_TRACE_TRACE, add_stats)); + cl_git_pass(git_trace_set( + GIT_TRACE_PERF, diff_perf_track_stats, &g_diff_perf)); #endif } @@ -42,7 +21,7 @@ void test_diff_workdir__cleanup(void) { cl_git_sandbox_cleanup(); #ifdef GIT_TRACE - cl_git_pass(git_trace_set(0, NULL)); + cl_git_pass(git_trace_set(GIT_TRACE_NONE, NULL, NULL)); #endif } diff --git a/tests/status/worktree.c b/tests/status/worktree.c index c1d6be982..06864ad59 100644 --- a/tests/status/worktree.c +++ b/tests/status/worktree.c @@ -5,38 +5,18 @@ #include "posix.h" #include "util.h" #include "path.h" -#include +#include "../diff/diff_helpers.h" #ifdef GIT_TRACE -static struct { - size_t stat_calls; - size_t oid_calcs; - size_t submodule_lookups; -} g_diff_perf; - -static void add_stats(git_trace_level_t level, const char *msg) -{ - const char *assign = strchr(msg, '='); - - GIT_UNUSED(level); - - if (!assign) - return; - - if (!strncmp("stat", msg, (assign - msg))) - g_diff_perf.stat_calls += atoi(assign + 1); - else if (!strncmp("submodule_lookup", msg, (assign - msg))) - g_diff_perf.submodule_lookups += atoi(assign + 1); - else if (!strncmp("oid_calculation", msg, (assign - msg))) - g_diff_perf.oid_calcs += atoi(assign + 1); -} +static diff_perf g_diff_perf; #endif void test_status_worktree__initialize(void) { #ifdef GIT_TRACE memset(&g_diff_perf, 0, sizeof(g_diff_perf)); - cl_git_pass(git_trace_set(GIT_TRACE_TRACE, add_stats)); + cl_git_pass(git_trace_set( + GIT_TRACE_PERF, diff_perf_track_stats, &g_diff_perf)); #endif } @@ -50,7 +30,7 @@ void test_status_worktree__cleanup(void) { cl_git_sandbox_cleanup(); #ifdef GIT_TRACE - cl_git_pass(git_trace_set(0, NULL)); + cl_git_pass(git_trace_set(GIT_TRACE_NONE, NULL, NULL)); #endif } @@ -956,7 +936,5 @@ void test_status_worktree__update_stat_cache_0(void) cl_assert_equal_sz(13 + 3, g_diff_perf.stat_calls); cl_assert_equal_sz(0, g_diff_perf.oid_calcs); cl_assert_equal_sz(1, g_diff_perf.submodule_lookups); - - memset(&g_diff_perf, 0, sizeof(g_diff_perf)); #endif } diff --git a/tests/trace/trace.c b/tests/trace/trace.c index 87b325378..328539379 100644 --- a/tests/trace/trace.c +++ b/tests/trace/trace.c @@ -3,44 +3,49 @@ static int written = 0; -static void trace_callback(git_trace_level_t level, const char *message) +static void trace_callback( + git_trace_level_t level, + void *cb_payload, + void *msg_payload, + const char *msg) { - GIT_UNUSED(level); + GIT_UNUSED(level); GIT_UNUSED(msg_payload); - cl_assert(strcmp(message, "Hello world!") == 0); + cl_assert(strcmp(msg, "Hello world!") == 0); - written = 1; + if (cb_payload) + *((int *)cb_payload) = 1; } void test_trace_trace__initialize(void) { - git_trace_set(GIT_TRACE_INFO, trace_callback); + git_trace_set(GIT_TRACE_INFO_AND_BELOW, trace_callback, &written); written = 0; } void test_trace_trace__cleanup(void) { - git_trace_set(GIT_TRACE_NONE, NULL); + git_trace_set(GIT_TRACE_NONE, NULL, NULL); } void test_trace_trace__sets(void) { #ifdef GIT_TRACE - cl_assert(git_trace_level() == GIT_TRACE_INFO); + cl_assert(git_trace_level() == GIT_TRACE_INFO_AND_BELOW); #endif } void test_trace_trace__can_reset(void) { #ifdef GIT_TRACE - cl_assert(git_trace_level() == GIT_TRACE_INFO); - cl_git_pass(git_trace_set(GIT_TRACE_ERROR, trace_callback)); + cl_assert(git_trace_level() == GIT_TRACE_INFO_AND_BELOW); + cl_git_pass(git_trace_set(GIT_TRACE_ERROR, trace_callback, &written)); cl_assert(written == 0); - git_trace(GIT_TRACE_INFO, "Hello %s!", "world"); + git_trace(GIT_TRACE_INFO, NULL, "Hello %s!", "world"); cl_assert(written == 0); - git_trace(GIT_TRACE_ERROR, "Hello %s!", "world"); + git_trace(GIT_TRACE_ERROR, NULL, "Hello %s!", "world"); cl_assert(written == 1); #endif } @@ -48,13 +53,13 @@ void test_trace_trace__can_reset(void) void test_trace_trace__can_unset(void) { #ifdef GIT_TRACE - cl_assert(git_trace_level() == GIT_TRACE_INFO); - cl_git_pass(git_trace_set(GIT_TRACE_NONE, NULL)); + cl_assert(git_trace_level() == GIT_TRACE_INFO_AND_BELOW); + cl_git_pass(git_trace_set(GIT_TRACE_NONE, NULL, NULL)); cl_assert(git_trace_level() == GIT_TRACE_NONE); cl_assert(written == 0); - git_trace(GIT_TRACE_FATAL, "Hello %s!", "world"); + git_trace(GIT_TRACE_FATAL, NULL, "Hello %s!", "world"); cl_assert(written == 0); #endif } @@ -63,7 +68,7 @@ void test_trace_trace__skips_higher_level(void) { #ifdef GIT_TRACE cl_assert(written == 0); - git_trace(GIT_TRACE_DEBUG, "Hello %s!", "world"); + git_trace(GIT_TRACE_DEBUG, NULL, "Hello %s!", "world"); cl_assert(written == 0); #endif } @@ -72,7 +77,7 @@ void test_trace_trace__writes(void) { #ifdef GIT_TRACE cl_assert(written == 0); - git_trace(GIT_TRACE_INFO, "Hello %s!", "world"); + git_trace(GIT_TRACE_INFO, NULL, "Hello %s!", "world"); cl_assert(written == 1); #endif } @@ -81,7 +86,7 @@ void test_trace_trace__writes_lower_level(void) { #ifdef GIT_TRACE cl_assert(written == 0); - git_trace(GIT_TRACE_ERROR, "Hello %s!", "world"); + git_trace(GIT_TRACE_ERROR, NULL, "Hello %s!", "world"); cl_assert(written == 1); #endif } From 7a2e56a3f6115c3a145e4f73d0aa8bea6dded899 Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Tue, 29 Apr 2014 14:30:15 -0700 Subject: [PATCH 08/13] Get rid of redundant git_diff_options_init fn Since git_diff_init_options was introduced, remove this old fn. --- include/git2/diff.h | 17 ----------------- src/diff.c | 14 -------------- tests/diff/blob.c | 2 +- tests/diff/tree.c | 2 +- 4 files changed, 2 insertions(+), 33 deletions(-) diff --git a/include/git2/diff.h b/include/git2/diff.h index afdb2c981..18880889c 100644 --- a/include/git2/diff.h +++ b/include/git2/diff.h @@ -811,23 +811,6 @@ GIT_EXTERN(int) git_diff_find_similar( git_diff *diff, const git_diff_find_options *options); -/** - * Initialize diff options structure - * - * In most cases, you can probably just use `GIT_DIFF_OPTIONS_INIT` to - * initialize the diff options structure, but in some cases that is not - * going to work. You can call this function instead. Note that you - * must pass both a pointer to the structure to be initialized and the - * `GIT_DIFF_OPTIONS_VERSION` value from the header you compiled with. - * - * @param options Pointer to git_diff_options memory to be initialized - * @param version Should be `GIT_DIFF_OPTIONS_VERSION` - * @return 0 on success, negative on failure (such as unsupported version) - */ -GIT_EXTERN(int) git_diff_options_init( - git_diff_options *options, - unsigned int version); - /**@}*/ diff --git a/src/diff.c b/src/diff.c index 5a6b127a1..b34d15312 100644 --- a/src/diff.c +++ b/src/diff.c @@ -1282,20 +1282,6 @@ int git_diff_tree_to_workdir_with_index( return error; } -int git_diff_options_init(git_diff_options *options, unsigned int version) -{ - git_diff_options template = GIT_DIFF_OPTIONS_INIT; - - if (version != template.version) { - giterr_set(GITERR_INVALID, - "Invalid version %d for git_diff_options", (int)version); - return -1; - } - - memcpy(options, &template, sizeof(*options)); - return 0; -} - size_t git_diff_num_deltas(const git_diff *diff) { assert(diff); diff --git a/tests/diff/blob.c b/tests/diff/blob.c index d1fff9c5b..527007965 100644 --- a/tests/diff/blob.c +++ b/tests/diff/blob.c @@ -26,7 +26,7 @@ void test_diff_blob__initialize(void) g_repo = cl_git_sandbox_init("attr"); - cl_git_pass(git_diff_options_init(&opts, GIT_DIFF_OPTIONS_VERSION)); + cl_git_pass(git_diff_init_options(&opts, GIT_DIFF_OPTIONS_VERSION)); opts.context_lines = 1; memset(&expected, 0, sizeof(expected)); diff --git a/tests/diff/tree.c b/tests/diff/tree.c index 582174b8b..6ab49fdb0 100644 --- a/tests/diff/tree.c +++ b/tests/diff/tree.c @@ -9,7 +9,7 @@ static diff_expects expect; void test_diff_tree__initialize(void) { - cl_git_pass(git_diff_options_init(&opts, GIT_DIFF_OPTIONS_VERSION)); + cl_git_pass(git_diff_init_options(&opts, GIT_DIFF_OPTIONS_VERSION)); memset(&expect, 0, sizeof(expect)); From 9c8ed4999740e921ecc2966bbcd0dbcfc725f59a Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Tue, 29 Apr 2014 15:05:58 -0700 Subject: [PATCH 09/13] Remove trace / add git_diff_perfdata struct + api --- include/git2/diff.h | 30 ++++++++-------- include/git2/status.h | 9 +++-- include/git2/sys/diff.h | 28 +++++++++++++++ include/git2/trace.h | 43 +++++++--------------- src/diff.c | 76 ++++++++++++++++++++------------------- src/diff.h | 2 ++ src/iterator.c | 5 +-- src/iterator.h | 1 + src/status.c | 36 +++++++++++++++---- src/trace.c | 15 ++++---- src/trace.h | 19 +++++----- tests/diff/diff_helpers.c | 19 ---------- tests/diff/diff_helpers.h | 14 -------- tests/diff/workdir.c | 60 +++++++++++-------------------- tests/status/worktree.c | 75 +++++++++++++++++++------------------- tests/trace/trace.c | 39 +++++++++----------- 16 files changed, 220 insertions(+), 251 deletions(-) diff --git a/include/git2/diff.h b/include/git2/diff.h index 18880889c..b849e3bb5 100644 --- a/include/git2/diff.h +++ b/include/git2/diff.h @@ -391,14 +391,13 @@ typedef struct { * Initializes a `git_diff_options` with default values. Equivalent to * creating an instance with GIT_DIFF_OPTIONS_INIT. * -* @param opts the `git_diff_options` instance to initialize. -* @param version the version of the struct; you should pass -* `GIT_DIFF_OPTIONS_VERSION` here. +* @param opts The `git_diff_options` instance to initialize. +* @param version Version of struct; pass `GIT_DIFF_OPTIONS_VERSION` * @return Zero on success; -1 on failure. */ GIT_EXTERN(int) git_diff_init_options( - git_diff_options* opts, - int version); + git_diff_options *opts, + unsigned int version); /** * When iterating over a diff, callback that will be made per file. @@ -632,14 +631,13 @@ typedef struct { * Initializes a `git_diff_find_options` with default values. Equivalent to * creating an instance with GIT_DIFF_FIND_OPTIONS_INIT. * -* @param opts the `git_diff_find_options` instance to initialize. -* @param version the version of the struct; you should pass -* `GIT_DIFF_FIND_OPTIONS_VERSION` here. +* @param opts The `git_diff_find_options` instance to initialize. +* @param version Version of struct; pass `GIT_DIFF_FIND_OPTIONS_VERSION` * @return Zero on success; -1 on failure. */ GIT_EXTERN(int) git_diff_find_init_options( - git_diff_find_options* opts, - int version); + git_diff_find_options *opts, + unsigned int version); /** @name Diff Generator Functions * @@ -1223,17 +1221,17 @@ GIT_EXTERN(int) git_diff_commit_as_email( const git_diff_options *diff_opts); /** -* Initializes a `git_diff_format_email_options` with default values. Equivalent to -* creating an instance with GIT_DIFF_FORMAT_EMAIL_OPTIONS_INIT. +* Initializes a `git_diff_format_email_options` with default values. * -* @param opts the `git_diff_format_email_options` instance to initialize. -* @param version the version of the struct; you should pass -* `GIT_DIFF_FORMAT_EMAIL_OPTIONS_VERSION` here. +* Equivalent to creating an instance with GIT_DIFF_FORMAT_EMAIL_OPTIONS_INIT. +* +* @param opts Uhe `git_diff_format_email_options` instance to initialize. +* @param version Version of struct; pass `GIT_DIFF_FORMAT_EMAIL_OPTIONS_VERSION` * @return Zero on success; -1 on failure. */ GIT_EXTERN(int) git_diff_format_email_init_options( git_diff_format_email_options *opts, - int version); + unsigned int version); GIT_END_DECL diff --git a/include/git2/status.h b/include/git2/status.h index ee2f33287..effe5e1ea 100644 --- a/include/git2/status.h +++ b/include/git2/status.h @@ -184,14 +184,13 @@ typedef struct { * Initializes a `git_status_options` with default values. Equivalent to * creating an instance with GIT_STATUS_OPTIONS_INIT. * - * @param opts the `git_status_options` instance to initialize. - * @param version the version of the struct; you should pass - * `GIT_STATUS_OPTIONS_VERSION` here. + * @param opts The `git_status_options` instance to initialize. + * @param version Version of struct; pass `GIT_STATUS_OPTIONS_VERSION` * @return Zero on success; -1 on failure. */ GIT_EXTERN(int) git_status_init_options( - git_status_options* opts, - int version); + git_status_options *opts, + unsigned int version); /** * A status entry, providing the differences between the file as it exists diff --git a/include/git2/sys/diff.h b/include/git2/sys/diff.h index bc6cdf393..48d72f4f9 100644 --- a/include/git2/sys/diff.h +++ b/include/git2/sys/diff.h @@ -10,6 +10,8 @@ #include "git2/common.h" #include "git2/types.h" #include "git2/oid.h" +#include "git2/diff.h" +#include "git2/status.h" /** * @file git2/sys/diff.h @@ -58,6 +60,32 @@ GIT_EXTERN(int) git_diff_print_callback__to_file_handle( const git_diff_line *line, void *payload); /*< payload must be a `FILE *` */ + +typedef struct { + unsigned int version; + size_t stat_calls; + size_t oid_calculations; +} git_diff_perfdata; + +#define GIT_DIFF_PERFDATA_VERSION 1 +#define GIT_DIFF_PERFDATA_INIT {GIT_DIFF_PERFDATA_VERSION,0,0} + +/** + * Get performance data for a diff object. + * + * @param out Structure to be filled with diff performance data + * @param diff Diff to read performance data from + * @return 0 for success, <0 for error + */ +GIT_EXTERN(int) git_diff_get_perfdata( + git_diff_perfdata *out, const git_diff *diff); + +/** + * Get performance data for diffs from a git_status_list + */ +GIT_EXTERN(int) git_status_list_get_perfdata( + git_diff_perfdata *out, const git_status_list *status); + /** @} */ GIT_END_DECL #endif diff --git a/include/git2/trace.h b/include/git2/trace.h index 867b34612..f9b4d6ff6 100644 --- a/include/git2/trace.h +++ b/include/git2/trace.h @@ -20,64 +20,47 @@ GIT_BEGIN_DECL /** - * Available tracing messages. Each tracing level can be enabled - * independently or pass GIT_TRACE_ALL to enable all levels. + * Available tracing levels. When tracing is set to a particular level, + * callers will be provided tracing at the given level and all lower levels. */ typedef enum { /** No tracing will be performed. */ - GIT_TRACE_NONE = 0x0000u, - - /** All tracing messages will be sent. */ - GIT_TRACE_ALL = 0xFFFFu, + GIT_TRACE_NONE = 0, /** Severe errors that may impact the program's execution */ - GIT_TRACE_FATAL = 0x0001u, + GIT_TRACE_FATAL = 1, /** Errors that do not impact the program's execution */ - GIT_TRACE_ERROR = 0x0002u, - GIT_TRACE_ERROR_AND_BELOW = 0x0003u, + GIT_TRACE_ERROR = 2, /** Warnings that suggest abnormal data */ - GIT_TRACE_WARN = 0x0004u, - GIT_TRACE_WARN_AND_BELOW = 0x0007u, + GIT_TRACE_WARN = 3, /** Informational messages about program execution */ - GIT_TRACE_INFO = 0x0008u, - GIT_TRACE_INFO_AND_BELOW = 0x000Fu, + GIT_TRACE_INFO = 4, /** Detailed data that allows for debugging */ - GIT_TRACE_DEBUG = 0x0010u, + GIT_TRACE_DEBUG = 5, /** Exceptionally detailed debugging data */ - GIT_TRACE_TRACE = 0x0020u, - - /** Performance tracking related traces */ - GIT_TRACE_PERF = 0x0040u, + GIT_TRACE_TRACE = 6 } git_trace_level_t; /** * An instance for a tracing function */ -typedef void (*git_trace_callback)( - git_trace_level_t level, /* just one bit will be sent */ - void *cb_payload, - void *msg_payload, - const char *msg); +typedef void (*git_trace_callback)(git_trace_level_t level, const char *msg); /** * Sets the system tracing configuration to the specified level with the * specified callback. When system events occur at a level equal to, or * lower than, the given level they will be reported to the given callback. * - * @param level Bitmask of all enabled trace levels - * @param cb Function to call with trace messages - * @param cb_payload Payload to pass when callback is invoked + * @param level Level to set tracing to + * @param cb Function to call with trace data * @return 0 or an error code */ -GIT_EXTERN(int) git_trace_set( - git_trace_level_t level, - git_trace_callback cb, - void *cb_payload); +GIT_EXTERN(int) git_trace_set(git_trace_level_t level, git_trace_callback cb); /** @} */ GIT_END_DECL diff --git a/src/diff.c b/src/diff.c index b34d15312..26e671dce 100644 --- a/src/diff.c +++ b/src/diff.c @@ -14,7 +14,6 @@ #include "index.h" #include "odb.h" #include "submodule.h" -#include "trace.h" #define DIFF_FLAG_IS_SET(DIFF,FLAG) (((DIFF)->opts.flags & (FLAG)) != 0) #define DIFF_FLAG_ISNT_SET(DIFF,FLAG) (((DIFF)->opts.flags & (FLAG)) == 0) @@ -555,7 +554,8 @@ int git_diff__oid_for_entry( if (!entry.mode) { struct stat st; - git_trace(GIT_TRACE_PERF, NULL, "stat"); + diff->perf.stat_calls++; + if (p_stat(full_path.ptr, &st) < 0) { error = git_path_set_error(errno, entry.path, "stat"); git_buf_free(&full_path); @@ -570,7 +570,6 @@ int git_diff__oid_for_entry( if (S_ISGITLINK(entry.mode)) { git_submodule *sm; - git_trace(GIT_TRACE_PERF, NULL, "submodule_lookup"); if (!git_submodule_lookup(&sm, diff->repo, entry.path)) { const git_oid *sm_oid = git_submodule_wd_id(sm); if (sm_oid) @@ -583,8 +582,8 @@ int git_diff__oid_for_entry( giterr_clear(); } } else if (S_ISLNK(entry.mode)) { - git_trace(GIT_TRACE_PERF, NULL, "oid_calculation"); error = git_odb__hashlink(out, full_path.ptr); + diff->perf.oid_calculations++; } else if (!git__is_sizet(entry.file_size)) { giterr_set(GITERR_OS, "File size overflow (for 32-bits) on '%s'", entry.path); @@ -596,10 +595,10 @@ int git_diff__oid_for_entry( if (fd < 0) error = fd; else { - git_trace(GIT_TRACE_PERF, NULL, "oid_calculation"); error = git_odb__hashfd_filtered( out, fd, (size_t)entry.file_size, GIT_OBJ_BLOB, fl); p_close(fd); + diff->perf.oid_calculations++; } git_filter_list_free(fl); @@ -655,8 +654,6 @@ static int maybe_modified_submodule( ign == GIT_SUBMODULE_IGNORE_ALL) return 0; - git_trace(GIT_TRACE_PERF, NULL, "submodule_lookup"); - if ((error = git_submodule_lookup( &sub, diff->repo, info->nitem->path)) < 0) { @@ -965,8 +962,6 @@ static int handle_unmatched_new_item( delta_type = GIT_DELTA_ADDED; else if (nitem->mode == GIT_FILEMODE_COMMIT) { - git_trace(GIT_TRACE_PERF, NULL, "submodule_lookup"); - /* ignore things that are not actual submodules */ if (git_submodule_lookup(NULL, info->repo, nitem->path) != 0) { giterr_clear(); @@ -1119,6 +1114,8 @@ int git_diff__from_iterators( error = 0; } + diff->perf.stat_calls += old_iter->stat_calls + new_iter->stat_calls; + cleanup: if (!error) *diff_ptr = diff; @@ -1313,6 +1310,22 @@ int git_diff_is_sorted_icase(const git_diff *diff) return (diff->opts.flags & GIT_DIFF_IGNORE_CASE) != 0; } +static int diff_options_bad_version(int version, const char *thing) +{ + giterr_set(GITERR_INVALID, "Invalid version %d for %s", version, thing); + return -1; +} + +int git_diff_get_perfdata(git_diff_perfdata *out, const git_diff *diff) +{ + if (!out || out->version != GIT_DIFF_PERFDATA_VERSION) + return diff_options_bad_version( + out ? out->version : 0, "git_diff_perfdata"); + out->stat_calls = diff->perf.stat_calls; + out->oid_calculations = diff->perf.oid_calculations; + return 0; +} + int git_diff__paired_foreach( git_diff *head2idx, git_diff *idx2wd, @@ -1615,38 +1628,29 @@ int git_diff_commit_as_email( return error; } -int git_diff_init_options(git_diff_options* opts, int version) +int git_diff_init_options(git_diff_options* opts, unsigned int version) { - if (version != GIT_DIFF_OPTIONS_VERSION) { - giterr_set(GITERR_INVALID, "Invalid version %d for git_diff_options", version); - return -1; - } else { - git_diff_options o = GIT_DIFF_OPTIONS_INIT; - memcpy(opts, &o, sizeof(o)); - return 0; - } + git_diff_options o = GIT_DIFF_OPTIONS_INIT; + if (version != o.version) + return diff_options_bad_version(version, "git_diff_options"); + memcpy(opts, &o, sizeof(o)); + return 0; } -int git_diff_find_init_options(git_diff_find_options* opts, int version) +int git_diff_find_init_options(git_diff_find_options* opts, unsigned int version) { - if (version != GIT_DIFF_FIND_OPTIONS_VERSION) { - giterr_set(GITERR_INVALID, "Invalid version %d for git_diff_find_options", version); - return -1; - } else { - git_diff_find_options o = GIT_DIFF_FIND_OPTIONS_INIT; - memcpy(opts, &o, sizeof(o)); - return 0; - } + git_diff_find_options o = GIT_DIFF_FIND_OPTIONS_INIT; + if (version != o.version) + return diff_options_bad_version(version, "git_diff_find_options"); + memcpy(opts, &o, sizeof(o)); + return 0; } -int git_diff_format_email_init_options(git_diff_format_email_options* opts, int version) +int git_diff_format_email_init_options(git_diff_format_email_options* opts, unsigned int version) { - if (version != GIT_DIFF_FORMAT_EMAIL_OPTIONS_VERSION) { - giterr_set(GITERR_INVALID, "Invalid version %d for git_diff_format_email_options", version); - return -1; - } else { - git_diff_format_email_options o = GIT_DIFF_FORMAT_EMAIL_OPTIONS_INIT; - memcpy(opts, &o, sizeof(o)); - return 0; - } + git_diff_format_email_options o = GIT_DIFF_FORMAT_EMAIL_OPTIONS_INIT; + if (version != o.version) + return diff_options_bad_version(version, "git_diff_format_email_options"); + memcpy(opts, &o, sizeof(o)); + return 0; } diff --git a/src/diff.h b/src/diff.h index 2e7ce0b7d..3305238d0 100644 --- a/src/diff.h +++ b/src/diff.h @@ -8,6 +8,7 @@ #define INCLUDE_diff_h__ #include "git2/diff.h" +#include "git2/sys/diff.h" #include "git2/oid.h" #include @@ -62,6 +63,7 @@ struct git_diff { git_iterator_type_t old_src; git_iterator_type_t new_src; uint32_t diffcaps; + git_diff_perfdata perf; int (*strcomp)(const char *, const char *); int (*strncomp)(const char *, const char *, size_t); diff --git a/src/iterator.c b/src/iterator.c index 0d7e5918d..4f8087c8d 100644 --- a/src/iterator.c +++ b/src/iterator.c @@ -11,7 +11,6 @@ #include "ignore.h" #include "buffer.h" #include "submodule.h" -#include "trace.h" #include #define ITERATOR_SET_CB(P,NAME_LC) do { \ @@ -1017,8 +1016,7 @@ static int fs_iterator__expand_dir(fs_iterator *fi) fs_iterator__free_frame(ff); return GIT_ENOTFOUND; } - - git_trace(GIT_TRACE_PERF, &ff->entries.length, "stat"); + fi->base.stat_calls += ff->entries.length; fs_iterator__seek_frame_start(fi, ff); @@ -1310,7 +1308,6 @@ static int workdir_iterator__enter_dir(fs_iterator *fi) if (!S_ISDIR(entry->st.st_mode) || !strcmp(GIT_DIR, entry->path)) continue; - git_trace(GIT_TRACE_PERF, entry->path, "submodule_lookup"); if (git_submodule__is_submodule(fi->base.repo, entry->path)) { entry->st.st_mode = GIT_FILEMODE_COMMIT; entry->path_len--; diff --git a/src/iterator.h b/src/iterator.h index ba9c1e486..f67830212 100644 --- a/src/iterator.h +++ b/src/iterator.h @@ -52,6 +52,7 @@ struct git_iterator { char *start; char *end; int (*prefixcomp)(const char *str, const char *prefix); + size_t stat_calls; unsigned int flags; }; diff --git a/src/status.c b/src/status.c index e418cf7b6..aab838bcf 100644 --- a/src/status.c +++ b/src/status.c @@ -518,14 +518,38 @@ int git_status_should_ignore( return git_ignore_path_is_ignored(ignored, repo, path); } -int git_status_init_options(git_status_options* opts, int version) +int git_status_init_options(git_status_options* opts, unsigned int version) { - if (version != GIT_STATUS_OPTIONS_VERSION) { + git_status_options o = GIT_STATUS_OPTIONS_INIT; + if (version != o.version) { giterr_set(GITERR_INVALID, "Invalid version %d for git_status_options", version); return -1; - } else { - git_status_options o = GIT_STATUS_OPTIONS_INIT; - memcpy(opts, &o, sizeof(o)); - return 0; } + memcpy(opts, &o, sizeof(o)); + return 0; } + +int git_status_list_get_perfdata( + git_diff_perfdata *out, const git_status_list *status) +{ + if (!out || out->version != GIT_DIFF_PERFDATA_VERSION) { + giterr_set(GITERR_INVALID, "Invalid version %d for git_diff_perfdata", + out ? out->version : 0); + return -1; + } + + out->stat_calls = 0; + out->oid_calculations = 0; + + if (status->head2idx) { + out->stat_calls += status->head2idx->perf.stat_calls; + out->oid_calculations += status->head2idx->perf.oid_calculations; + } + if (status->idx2wd) { + out->stat_calls += status->idx2wd->perf.stat_calls; + out->oid_calculations += status->idx2wd->perf.oid_calculations; + } + + return 0; +} + diff --git a/src/trace.c b/src/trace.c index 6ee2cf2ce..ee5039f56 100644 --- a/src/trace.c +++ b/src/trace.c @@ -17,25 +17,22 @@ struct git_trace_data git_trace__data = {0}; #endif -int git_trace_set( - git_trace_level_t level, git_trace_callback cb, void *cb_payload) +int git_trace_set(git_trace_level_t level, git_trace_callback callback) { #ifdef GIT_TRACE - assert(level == 0 || cb != NULL); + assert(level == 0 || callback != NULL); git_trace__data.level = level; - git_trace__data.callback = cb; - git_trace__data.callback_payload = cb_payload; + git_trace__data.callback = callback; GIT_MEMORY_BARRIER; return 0; #else GIT_UNUSED(level); - GIT_UNUSED(cb); - GIT_UNUSED(cb_payload); + GIT_UNUSED(callback); - giterr_set( - GITERR_INVALID, "This version of libgit2 was not built with tracing."); + giterr_set(GITERR_INVALID, + "This version of libgit2 was not built with tracing."); return -1; #endif } diff --git a/src/trace.h b/src/trace.h index b35e3808f..4d4e3bf53 100644 --- a/src/trace.h +++ b/src/trace.h @@ -15,16 +15,13 @@ struct git_trace_data { git_trace_level_t level; git_trace_callback callback; - void *callback_payload; }; extern struct git_trace_data git_trace__data; GIT_INLINE(void) git_trace__write_fmt( git_trace_level_t level, - void *message_payload, - const char *fmt, - ...) + const char *fmt, ...) { git_trace_callback callback = git_trace__data.callback; git_buf message = GIT_BUF_INIT; @@ -34,18 +31,18 @@ GIT_INLINE(void) git_trace__write_fmt( git_buf_vprintf(&message, fmt, ap); va_end(ap); - callback( - level, git_trace__data.callback_payload, message_payload, - git_buf_cstr(&message)); + callback(level, git_buf_cstr(&message)); git_buf_free(&message); } #define git_trace_level() (git_trace__data.level) -#define git_trace(l, p, ...) do { \ - if ((git_trace__data.level & (l)) != 0 && git_trace__data.callback) { \ - git_trace__write_fmt((l), (p), __VA_ARGS__); \ - } } while (0) +#define git_trace(l, ...) { \ + if (git_trace__data.level >= l && \ + git_trace__data.callback != NULL) { \ + git_trace__write_fmt(l, __VA_ARGS__); \ + } \ + } #else diff --git a/tests/diff/diff_helpers.c b/tests/diff/diff_helpers.c index 5de9834ba..279cb20c5 100644 --- a/tests/diff/diff_helpers.c +++ b/tests/diff/diff_helpers.c @@ -229,22 +229,3 @@ void diff_print_raw(FILE *fp, git_diff *diff) git_diff_print(diff, GIT_DIFF_FORMAT_RAW, git_diff_print_callback__to_file_handle, fp ? fp : stderr)); } - -void diff_perf_track_stats( - git_trace_level_t level, - void *cb_payload, - void *msg_payload, - const char *msg) -{ - diff_perf *data = cb_payload; - - if (!(level & GIT_TRACE_PERF)) - return; - - if (!strcmp("stat", msg)) - data->stat_calls += msg_payload ? *((size_t *)msg_payload) : 1; - else if (!strcmp("submodule_lookup", msg)) - data->submodule_lookups++; - else if (!strcmp("oid_calculation", msg)) - data->oid_calcs++; -} diff --git a/tests/diff/diff_helpers.h b/tests/diff/diff_helpers.h index 3ed538702..bf21f4b1f 100644 --- a/tests/diff/diff_helpers.h +++ b/tests/diff/diff_helpers.h @@ -62,17 +62,3 @@ extern int diff_foreach_via_iterator( extern void diff_print(FILE *fp, git_diff *diff); extern void diff_print_raw(FILE *fp, git_diff *diff); - -#include "git2/trace.h" - -typedef struct { - size_t stat_calls; - size_t oid_calcs; - size_t submodule_lookups; -} diff_perf; - -extern void diff_perf_track_stats( - git_trace_level_t level, - void *cb_payload, - void *msg_payload, - const char *msg); diff --git a/tests/diff/workdir.c b/tests/diff/workdir.c index 952c9022b..a6d48abc6 100644 --- a/tests/diff/workdir.c +++ b/tests/diff/workdir.c @@ -1,28 +1,13 @@ #include "clar_libgit2.h" #include "diff_helpers.h" #include "repository.h" +#include "git2/sys/diff.h" static git_repository *g_repo = NULL; -#ifdef GIT_TRACE -static diff_perf g_diff_perf; -#endif - -void test_diff_workdir__initialize(void) -{ -#ifdef GIT_TRACE - memset(&g_diff_perf, 0, sizeof(g_diff_perf)); - cl_git_pass(git_trace_set( - GIT_TRACE_PERF, diff_perf_track_stats, &g_diff_perf)); -#endif -} - void test_diff_workdir__cleanup(void) { cl_git_sandbox_cleanup(); -#ifdef GIT_TRACE - cl_git_pass(git_trace_set(GIT_TRACE_NONE, NULL, NULL)); -#endif } void test_diff_workdir__to_index(void) @@ -70,13 +55,14 @@ void test_diff_workdir__to_index(void) cl_assert_equal_i(5, exp.line_ctxt); cl_assert_equal_i(4, exp.line_adds); cl_assert_equal_i(5, exp.line_dels); + } -#ifdef GIT_TRACE + { + git_diff_perfdata perf = GIT_DIFF_PERFDATA_INIT; + cl_git_pass(git_diff_get_perfdata(&perf, diff)); cl_assert_equal_sz( - 13 /* in root */ + 3 /* in subdir */, g_diff_perf.stat_calls); - cl_assert_equal_sz(5, g_diff_perf.oid_calcs); - cl_assert_equal_sz(1, g_diff_perf.submodule_lookups); -#endif + 13 /* in root */ + 3 /* in subdir */, perf.stat_calls); + cl_assert_equal_sz(5, perf.oid_calculations); } git_diff_free(diff); @@ -1532,10 +1518,6 @@ static void basic_diff_status(git_diff **out, const git_diff_options *opts) { diff_expects exp; -#ifdef GIT_TRACE - memset(&g_diff_perf, 0, sizeof(g_diff_perf)); -#endif - cl_git_pass(git_diff_index_to_workdir(out, g_repo, NULL, opts)); memset(&exp, 0, sizeof(exp)); @@ -1555,6 +1537,7 @@ void test_diff_workdir__can_update_index(void) { git_diff_options opts = GIT_DIFF_OPTIONS_INIT; git_diff *diff = NULL; + git_diff_perfdata perf = GIT_DIFF_PERFDATA_INIT; g_repo = cl_git_sandbox_init("status"); @@ -1569,11 +1552,10 @@ void test_diff_workdir__can_update_index(void) opts.flags |= GIT_DIFF_INCLUDE_IGNORED | GIT_DIFF_INCLUDE_UNTRACKED; basic_diff_status(&diff, &opts); -#ifdef GIT_TRACE - cl_assert_equal_sz(13 + 3, g_diff_perf.stat_calls); - cl_assert_equal_sz(5, g_diff_perf.oid_calcs); - cl_assert_equal_sz(1, g_diff_perf.submodule_lookups); -#endif + + cl_git_pass(git_diff_get_perfdata(&perf, diff)); + cl_assert_equal_sz(13 + 3, perf.stat_calls); + cl_assert_equal_sz(5, perf.oid_calculations); git_diff_free(diff); @@ -1581,22 +1563,20 @@ void test_diff_workdir__can_update_index(void) opts.flags |= GIT_DIFF_UPDATE_INDEX; basic_diff_status(&diff, &opts); -#ifdef GIT_TRACE - cl_assert_equal_sz(13 + 3, g_diff_perf.stat_calls); - cl_assert_equal_sz(5, g_diff_perf.oid_calcs); - cl_assert_equal_sz(1, g_diff_perf.submodule_lookups); -#endif + + cl_git_pass(git_diff_get_perfdata(&perf, diff)); + cl_assert_equal_sz(13 + 3, perf.stat_calls); + cl_assert_equal_sz(5, perf.oid_calculations); git_diff_free(diff); /* now if we do it again, we should see fewer OID calculations */ basic_diff_status(&diff, &opts); -#ifdef GIT_TRACE - cl_assert_equal_sz(13 + 3, g_diff_perf.stat_calls); - cl_assert_equal_sz(0, g_diff_perf.oid_calcs); /* Yay */ - cl_assert_equal_sz(1, g_diff_perf.submodule_lookups); -#endif + + cl_git_pass(git_diff_get_perfdata(&perf, diff)); + cl_assert_equal_sz(13 + 3, perf.stat_calls); + cl_assert_equal_sz(0, perf.oid_calculations); git_diff_free(diff); } diff --git a/tests/status/worktree.c b/tests/status/worktree.c index 06864ad59..ca9068aba 100644 --- a/tests/status/worktree.c +++ b/tests/status/worktree.c @@ -6,19 +6,7 @@ #include "util.h" #include "path.h" #include "../diff/diff_helpers.h" - -#ifdef GIT_TRACE -static diff_perf g_diff_perf; -#endif - -void test_status_worktree__initialize(void) -{ -#ifdef GIT_TRACE - memset(&g_diff_perf, 0, sizeof(g_diff_perf)); - cl_git_pass(git_trace_set( - GIT_TRACE_PERF, diff_perf_track_stats, &g_diff_perf)); -#endif -} +#include "git2/sys/diff.h" /** * Cleanup @@ -29,9 +17,6 @@ void test_status_worktree__initialize(void) void test_status_worktree__cleanup(void) { cl_git_sandbox_cleanup(); -#ifdef GIT_TRACE - cl_git_pass(git_trace_set(GIT_TRACE_NONE, NULL, NULL)); -#endif } /** @@ -903,38 +888,50 @@ void test_status_worktree__long_filenames(void) * while reducing the amount of work that needs to be done */ +static void check_status0(git_status_list *status) +{ + size_t i, max_i = git_status_list_entrycount(status); + cl_assert_equal_sz(entry_count0, max_i); + for (i = 0; i < max_i; ++i) { + const git_status_entry *entry = git_status_byindex(status, i); + cl_assert_equal_i(entry_statuses0[i], entry->status); + } +} + void test_status_worktree__update_stat_cache_0(void) { git_repository *repo = cl_git_sandbox_init("status"); + git_status_options opts = GIT_STATUS_OPTIONS_INIT; + git_status_list *status; + git_diff_perfdata perf = GIT_DIFF_PERFDATA_INIT; - assert_show(entry_count0, entry_paths0, entry_statuses0, - repo, GIT_STATUS_SHOW_INDEX_AND_WORKDIR, 0); + opts.flags = GIT_STATUS_OPT_DEFAULTS; -#ifdef GIT_TRACE - cl_assert_equal_sz(13 + 3, g_diff_perf.stat_calls); - cl_assert_equal_sz(5, g_diff_perf.oid_calcs); - cl_assert_equal_sz(1, g_diff_perf.submodule_lookups); + cl_git_pass(git_status_list_new(&status, repo, &opts)); + check_status0(status); + cl_git_pass(git_status_list_get_perfdata(&perf, status)); + cl_assert_equal_sz(13 + 3, perf.stat_calls); + cl_assert_equal_sz(5, perf.oid_calculations); - memset(&g_diff_perf, 0, sizeof(g_diff_perf)); -#endif + git_status_list_free(status); - assert_show(entry_count0, entry_paths0, entry_statuses0, - repo, GIT_STATUS_SHOW_INDEX_AND_WORKDIR, GIT_STATUS_OPT_UPDATE_INDEX); + opts.flags |= GIT_STATUS_OPT_UPDATE_INDEX; -#ifdef GIT_TRACE - cl_assert_equal_sz(13 + 3, g_diff_perf.stat_calls); - cl_assert_equal_sz(5, g_diff_perf.oid_calcs); - cl_assert_equal_sz(1, g_diff_perf.submodule_lookups); + cl_git_pass(git_status_list_new(&status, repo, &opts)); + check_status0(status); + cl_git_pass(git_status_list_get_perfdata(&perf, status)); + cl_assert_equal_sz(13 + 3, perf.stat_calls); + cl_assert_equal_sz(5, perf.oid_calculations); - memset(&g_diff_perf, 0, sizeof(g_diff_perf)); -#endif + git_status_list_free(status); - assert_show(entry_count0, entry_paths0, entry_statuses0, - repo, GIT_STATUS_SHOW_INDEX_AND_WORKDIR, 0); + opts.flags &= ~GIT_STATUS_OPT_UPDATE_INDEX; -#ifdef GIT_TRACE - cl_assert_equal_sz(13 + 3, g_diff_perf.stat_calls); - cl_assert_equal_sz(0, g_diff_perf.oid_calcs); - cl_assert_equal_sz(1, g_diff_perf.submodule_lookups); -#endif + cl_git_pass(git_status_list_new(&status, repo, &opts)); + check_status0(status); + cl_git_pass(git_status_list_get_perfdata(&perf, status)); + cl_assert_equal_sz(13 + 3, perf.stat_calls); + cl_assert_equal_sz(0, perf.oid_calculations); + + git_status_list_free(status); } diff --git a/tests/trace/trace.c b/tests/trace/trace.c index 328539379..87b325378 100644 --- a/tests/trace/trace.c +++ b/tests/trace/trace.c @@ -3,49 +3,44 @@ static int written = 0; -static void trace_callback( - git_trace_level_t level, - void *cb_payload, - void *msg_payload, - const char *msg) +static void trace_callback(git_trace_level_t level, const char *message) { - GIT_UNUSED(level); GIT_UNUSED(msg_payload); + GIT_UNUSED(level); - cl_assert(strcmp(msg, "Hello world!") == 0); + cl_assert(strcmp(message, "Hello world!") == 0); - if (cb_payload) - *((int *)cb_payload) = 1; + written = 1; } void test_trace_trace__initialize(void) { - git_trace_set(GIT_TRACE_INFO_AND_BELOW, trace_callback, &written); + git_trace_set(GIT_TRACE_INFO, trace_callback); written = 0; } void test_trace_trace__cleanup(void) { - git_trace_set(GIT_TRACE_NONE, NULL, NULL); + git_trace_set(GIT_TRACE_NONE, NULL); } void test_trace_trace__sets(void) { #ifdef GIT_TRACE - cl_assert(git_trace_level() == GIT_TRACE_INFO_AND_BELOW); + cl_assert(git_trace_level() == GIT_TRACE_INFO); #endif } void test_trace_trace__can_reset(void) { #ifdef GIT_TRACE - cl_assert(git_trace_level() == GIT_TRACE_INFO_AND_BELOW); - cl_git_pass(git_trace_set(GIT_TRACE_ERROR, trace_callback, &written)); + cl_assert(git_trace_level() == GIT_TRACE_INFO); + cl_git_pass(git_trace_set(GIT_TRACE_ERROR, trace_callback)); cl_assert(written == 0); - git_trace(GIT_TRACE_INFO, NULL, "Hello %s!", "world"); + git_trace(GIT_TRACE_INFO, "Hello %s!", "world"); cl_assert(written == 0); - git_trace(GIT_TRACE_ERROR, NULL, "Hello %s!", "world"); + git_trace(GIT_TRACE_ERROR, "Hello %s!", "world"); cl_assert(written == 1); #endif } @@ -53,13 +48,13 @@ void test_trace_trace__can_reset(void) void test_trace_trace__can_unset(void) { #ifdef GIT_TRACE - cl_assert(git_trace_level() == GIT_TRACE_INFO_AND_BELOW); - cl_git_pass(git_trace_set(GIT_TRACE_NONE, NULL, NULL)); + cl_assert(git_trace_level() == GIT_TRACE_INFO); + cl_git_pass(git_trace_set(GIT_TRACE_NONE, NULL)); cl_assert(git_trace_level() == GIT_TRACE_NONE); cl_assert(written == 0); - git_trace(GIT_TRACE_FATAL, NULL, "Hello %s!", "world"); + git_trace(GIT_TRACE_FATAL, "Hello %s!", "world"); cl_assert(written == 0); #endif } @@ -68,7 +63,7 @@ void test_trace_trace__skips_higher_level(void) { #ifdef GIT_TRACE cl_assert(written == 0); - git_trace(GIT_TRACE_DEBUG, NULL, "Hello %s!", "world"); + git_trace(GIT_TRACE_DEBUG, "Hello %s!", "world"); cl_assert(written == 0); #endif } @@ -77,7 +72,7 @@ void test_trace_trace__writes(void) { #ifdef GIT_TRACE cl_assert(written == 0); - git_trace(GIT_TRACE_INFO, NULL, "Hello %s!", "world"); + git_trace(GIT_TRACE_INFO, "Hello %s!", "world"); cl_assert(written == 1); #endif } @@ -86,7 +81,7 @@ void test_trace_trace__writes_lower_level(void) { #ifdef GIT_TRACE cl_assert(written == 0); - git_trace(GIT_TRACE_ERROR, NULL, "Hello %s!", "world"); + git_trace(GIT_TRACE_ERROR, "Hello %s!", "world"); cl_assert(written == 1); #endif } From 702efc891f2a620f10998062ba0c00b34100f632 Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Wed, 30 Apr 2014 10:57:42 -0700 Subject: [PATCH 10/13] Make init_options fns use unsigned ints and macro Use an unsigned int for the version and add a helper macro so the code is simplified (and so the error message is a common string). --- include/git2/blame.h | 19 ++++++++--------- include/git2/clone.h | 19 ++++++++--------- include/git2/diff.h | 44 +++++++++++++++++++-------------------- include/git2/merge.h | 6 +++--- include/git2/push.h | 4 ++-- include/git2/repository.h | 9 ++++---- src/blame.c | 13 ++++-------- src/clone.c | 13 ++++-------- src/common.h | 5 +++++ src/diff.c | 27 +++++++++++------------- src/merge.c | 40 ++++++++++++----------------------- src/push.c | 13 ++++-------- src/repository.c | 15 ++++++------- src/status.c | 10 +++------ 14 files changed, 100 insertions(+), 137 deletions(-) diff --git a/include/git2/blame.h b/include/git2/blame.h index b7fa9aeda..7f0de1731 100644 --- a/include/git2/blame.h +++ b/include/git2/blame.h @@ -83,17 +83,16 @@ typedef struct git_blame_options { #define GIT_BLAME_OPTIONS_INIT {GIT_BLAME_OPTIONS_VERSION} /** -* Initializes a `git_blame_options` with default values. Equivalent to -* creating an instance with GIT_BLAME_OPTIONS_INIT. -* -* @param opts the `git_blame_options` instance to initialize. -* @param version the version of the struct; you should pass -* `GIT_BLAME_OPTIONS_VERSION` here. -* @return Zero on success; -1 on failure. -*/ + * Initializes a `git_blame_options` with default values. Equivalent to + * creating an instance with GIT_BLAME_OPTIONS_INIT. + * + * @param opts The `git_blame_options` struct to initialize + * @param version Version of struct; pass `GIT_BLAME_OPTIONS_VERSION` + * @return Zero on success; -1 on failure. + */ GIT_EXTERN(int) git_blame_init_options( - git_blame_options* opts, - int version); + git_blame_options *opts, + unsigned int version); /** * Structure that represents a blame hunk. diff --git a/include/git2/clone.h b/include/git2/clone.h index 20be1a105..985c04bf6 100644 --- a/include/git2/clone.h +++ b/include/git2/clone.h @@ -66,17 +66,16 @@ typedef struct git_clone_options { #define GIT_CLONE_OPTIONS_INIT {GIT_CLONE_OPTIONS_VERSION, {GIT_CHECKOUT_OPTIONS_VERSION, GIT_CHECKOUT_SAFE_CREATE}, GIT_REMOTE_CALLBACKS_INIT} /** -* Initializes a `git_clone_options` with default values. Equivalent to -* creating an instance with GIT_CLONE_OPTIONS_INIT. -* -* @param opts the `git_clone_options` instance to initialize. -* @param version the version of the struct; you should pass -* `GIT_CLONE_OPTIONS_VERSION` here. -* @return Zero on success; -1 on failure. -*/ + * Initializes a `git_clone_options` with default values. Equivalent to + * creating an instance with GIT_CLONE_OPTIONS_INIT. + * + * @param opts The `git_clone_options` struct to initialize + * @param version Version of struct; pass `GIT_CLONE_OPTIONS_VERSION` + * @return Zero on success; -1 on failure. + */ GIT_EXTERN(int) git_clone_init_options( - git_clone_options* opts, - int version); + git_clone_options *opts, + unsigned int version); /** * Clone a remote repository. diff --git a/include/git2/diff.h b/include/git2/diff.h index b849e3bb5..b40cc6135 100644 --- a/include/git2/diff.h +++ b/include/git2/diff.h @@ -388,13 +388,13 @@ typedef struct { {GIT_DIFF_OPTIONS_VERSION, 0, GIT_SUBMODULE_IGNORE_DEFAULT, {NULL,0}, NULL, NULL, 3} /** -* Initializes a `git_diff_options` with default values. Equivalent to -* creating an instance with GIT_DIFF_OPTIONS_INIT. -* -* @param opts The `git_diff_options` instance to initialize. -* @param version Version of struct; pass `GIT_DIFF_OPTIONS_VERSION` -* @return Zero on success; -1 on failure. -*/ + * Initializes a `git_diff_options` with default values. Equivalent to + * creating an instance with GIT_DIFF_OPTIONS_INIT. + * + * @param opts The `git_diff_options` struct to initialize + * @param version Version of struct; pass `GIT_DIFF_OPTIONS_VERSION` + * @return Zero on success; -1 on failure. + */ GIT_EXTERN(int) git_diff_init_options( git_diff_options *opts, unsigned int version); @@ -628,13 +628,13 @@ typedef struct { #define GIT_DIFF_FIND_OPTIONS_INIT {GIT_DIFF_FIND_OPTIONS_VERSION} /** -* Initializes a `git_diff_find_options` with default values. Equivalent to -* creating an instance with GIT_DIFF_FIND_OPTIONS_INIT. -* -* @param opts The `git_diff_find_options` instance to initialize. -* @param version Version of struct; pass `GIT_DIFF_FIND_OPTIONS_VERSION` -* @return Zero on success; -1 on failure. -*/ + * Initializes a `git_diff_find_options` with default values. Equivalent to + * creating an instance with GIT_DIFF_FIND_OPTIONS_INIT. + * + * @param opts The `git_diff_find_options` struct to initialize + * @param version Version of struct; pass `GIT_DIFF_FIND_OPTIONS_VERSION` + * @return Zero on success; -1 on failure. + */ GIT_EXTERN(int) git_diff_find_init_options( git_diff_find_options *opts, unsigned int version); @@ -1221,14 +1221,14 @@ GIT_EXTERN(int) git_diff_commit_as_email( const git_diff_options *diff_opts); /** -* Initializes a `git_diff_format_email_options` with default values. -* -* Equivalent to creating an instance with GIT_DIFF_FORMAT_EMAIL_OPTIONS_INIT. -* -* @param opts Uhe `git_diff_format_email_options` instance to initialize. -* @param version Version of struct; pass `GIT_DIFF_FORMAT_EMAIL_OPTIONS_VERSION` -* @return Zero on success; -1 on failure. -*/ + * Initializes a `git_diff_format_email_options` with default values. + * + * Equivalent to creating an instance with GIT_DIFF_FORMAT_EMAIL_OPTIONS_INIT. + * + * @param opts The `git_diff_format_email_options` struct to initialize + * @param version Version of struct; pass `GIT_DIFF_FORMAT_EMAIL_OPTIONS_VERSION` + * @return Zero on success; -1 on failure. + */ GIT_EXTERN(int) git_diff_format_email_init_options( git_diff_format_email_options *opts, unsigned int version); diff --git a/include/git2/merge.h b/include/git2/merge.h index 6d97e81e6..ef2dc3804 100644 --- a/include/git2/merge.h +++ b/include/git2/merge.h @@ -57,7 +57,7 @@ typedef struct { */ GIT_EXTERN(int) git_merge_file_init_input( git_merge_file_input *opts, - int version); + unsigned int version); /** * Flags for `git_merge_tree` options. A combination of these flags can be @@ -164,7 +164,7 @@ typedef struct { */ GIT_EXTERN(int) git_merge_file_init_options( git_merge_file_options *opts, - int version); + unsigned int version); typedef struct { /** @@ -232,7 +232,7 @@ typedef struct { */ GIT_EXTERN(int) git_merge_init_options( git_merge_options *opts, - int version); + unsigned int version); /** * The results of `git_merge_analysis` indicate the merge opportunities. diff --git a/include/git2/push.h b/include/git2/push.h index 7a8bec12c..cbf115661 100644 --- a/include/git2/push.h +++ b/include/git2/push.h @@ -49,8 +49,8 @@ typedef struct { * @return Zero on success; -1 on failure. */ GIT_EXTERN(int) git_push_init_options( - git_push_options* opts, - int version); + git_push_options *opts, + unsigned int version); /** Push network progress notification function */ typedef int (*git_push_transfer_progress)( diff --git a/include/git2/repository.h b/include/git2/repository.h index 4433e71a2..e3f687a29 100644 --- a/include/git2/repository.h +++ b/include/git2/repository.h @@ -271,14 +271,13 @@ typedef struct { * Initializes a `git_repository_init_options` with default values. Equivalent * to creating an instance with GIT_REPOSITORY_INIT_OPTIONS_INIT. * - * @param opts the `git_repository_init_options` instance to initialize. - * @param version the version of the struct; you should pass - * `GIT_REPOSITORY_INIT_OPTIONS_VERSION` here. + * @param opts the `git_repository_init_options` struct to initialize + * @param version Version of struct; pass `GIT_REPOSITORY_INIT_OPTIONS_VERSION` * @return Zero on success; -1 on failure. */ GIT_EXTERN(int) git_repository_init_init_options( - git_repository_init_options* opts, - int version); + git_repository_init_options *opts, + unsigned int version); /** * Create a new Git repository in the given folder with extended controls. diff --git a/src/blame.c b/src/blame.c index e45c0ee1c..eb977c287 100644 --- a/src/blame.c +++ b/src/blame.c @@ -480,14 +480,9 @@ int git_blame_buffer( return 0; } -int git_blame_init_options(git_blame_options* opts, int version) +int git_blame_init_options(git_blame_options *opts, unsigned int version) { - if (version != GIT_BLAME_OPTIONS_VERSION) { - giterr_set(GITERR_INVALID, "Invalid version %d for git_blame_options", version); - return -1; - } else { - git_blame_options o = GIT_BLAME_OPTIONS_INIT; - memcpy(opts, &o, sizeof(o)); - return 0; - } + GIT_INIT_STRUCTURE_FROM_TEMPLATE( + opts, version, git_blame_options, GIT_BLAME_OPTIONS_INIT); + return 0; } diff --git a/src/clone.c b/src/clone.c index 62f103561..c6be00f0e 100644 --- a/src/clone.c +++ b/src/clone.c @@ -445,14 +445,9 @@ int git_clone( return error; } -int git_clone_init_options(git_clone_options* opts, int version) +int git_clone_init_options(git_clone_options *opts, unsigned int version) { - if (version != GIT_CLONE_OPTIONS_VERSION) { - giterr_set(GITERR_INVALID, "Invalid version %d for git_clone_options", version); - return -1; - } else { - git_clone_options o = GIT_CLONE_OPTIONS_INIT; - memcpy(opts, &o, sizeof(o)); - return 0; - } + GIT_INIT_STRUCTURE_FROM_TEMPLATE( + opts, version, git_clone_options, GIT_CLONE_OPTIONS_INIT); + return 0; } diff --git a/src/common.h b/src/common.h index dd97a3099..807e5fa39 100644 --- a/src/common.h +++ b/src/common.h @@ -170,6 +170,11 @@ GIT_INLINE(void) git__init_structure(void *structure, size_t len, unsigned int v } #define GIT_INIT_STRUCTURE(S,V) git__init_structure(S, sizeof(*S), V) +#define GIT_INIT_STRUCTURE_FROM_TEMPLATE(PTR,VERSION,TYPE,TPL) do { \ + TYPE _tmpl = TPL; \ + GITERR_CHECK_VERSION(&(VERSION), _tmpl.version, #TYPE); \ + memcpy((PTR), &_tmpl, sizeof(_tmpl)); } while (0) + /* NOTE: other giterr functions are in the public errors.h header file */ #include "util.h" diff --git a/src/diff.c b/src/diff.c index 26e671dce..56f333f76 100644 --- a/src/diff.c +++ b/src/diff.c @@ -1628,29 +1628,26 @@ int git_diff_commit_as_email( return error; } -int git_diff_init_options(git_diff_options* opts, unsigned int version) +int git_diff_init_options(git_diff_options *opts, unsigned int version) { - git_diff_options o = GIT_DIFF_OPTIONS_INIT; - if (version != o.version) - return diff_options_bad_version(version, "git_diff_options"); - memcpy(opts, &o, sizeof(o)); + GIT_INIT_STRUCTURE_FROM_TEMPLATE( + opts, version, git_diff_options, GIT_DIFF_OPTIONS_INIT); return 0; } -int git_diff_find_init_options(git_diff_find_options* opts, unsigned int version) +int git_diff_find_init_options( + git_diff_find_options *opts, unsigned int version) { - git_diff_find_options o = GIT_DIFF_FIND_OPTIONS_INIT; - if (version != o.version) - return diff_options_bad_version(version, "git_diff_find_options"); - memcpy(opts, &o, sizeof(o)); + GIT_INIT_STRUCTURE_FROM_TEMPLATE( + opts, version, git_diff_find_options, GIT_DIFF_FIND_OPTIONS_INIT); return 0; } -int git_diff_format_email_init_options(git_diff_format_email_options* opts, unsigned int version) +int git_diff_format_email_init_options( + git_diff_format_email_options *opts, unsigned int version) { - git_diff_format_email_options o = GIT_DIFF_FORMAT_EMAIL_OPTIONS_INIT; - if (version != o.version) - return diff_options_bad_version(version, "git_diff_format_email_options"); - memcpy(opts, &o, sizeof(o)); + GIT_INIT_STRUCTURE_FROM_TEMPLATE( + opts, version, git_diff_format_email_options, + GIT_DIFF_FORMAT_EMAIL_OPTIONS_INIT); return 0; } diff --git a/src/merge.c b/src/merge.c index 69c42bc0c..6a8e5874f 100644 --- a/src/merge.c +++ b/src/merge.c @@ -2803,38 +2803,24 @@ void git_merge_head_free(git_merge_head *head) git__free(head); } -int git_merge_init_options(git_merge_options *opts, int version) +int git_merge_init_options(git_merge_options *opts, unsigned int version) { - if (version != GIT_MERGE_OPTIONS_VERSION) { - giterr_set(GITERR_INVALID, "Invalid version %d for git_merge_options", version); - return -1; - } else { - git_merge_options default_opts = GIT_MERGE_OPTIONS_INIT; - memcpy(opts, &default_opts, sizeof(git_merge_options)); - return 0; - } + GIT_INIT_STRUCTURE_FROM_TEMPLATE( + opts, version, git_merge_options, GIT_MERGE_OPTIONS_INIT); + return 0; } -int git_merge_file_init_input(git_merge_file_input *input, int version) +int git_merge_file_init_input(git_merge_file_input *input, unsigned int version) { - if (version != GIT_MERGE_FILE_INPUT_VERSION) { - giterr_set(GITERR_INVALID, "Invalid version %d for git_merge_file_input", version); - return -1; - } else { - git_merge_file_input i = GIT_MERGE_FILE_INPUT_INIT; - memcpy(input, &i, sizeof(i)); - return 0; - } + GIT_INIT_STRUCTURE_FROM_TEMPLATE( + input, version, git_merge_file_input, GIT_MERGE_FILE_INPUT_INIT); + return 0; } -int git_merge_file_init_options(git_merge_file_options *opts, int version) +int git_merge_file_init_options( + git_merge_file_options *opts, unsigned int version) { - if (version != GIT_MERGE_FILE_OPTIONS_VERSION) { - giterr_set(GITERR_INVALID, "Invalid version %d for git_merge_file_options", version); - return -1; - } else { - git_merge_file_options o = GIT_MERGE_FILE_OPTIONS_INIT; - memcpy(opts, &o, sizeof(o)); - return 0; - } + GIT_INIT_STRUCTURE_FROM_TEMPLATE( + opts, version, git_merge_file_options, GIT_MERGE_FILE_OPTIONS_INIT); + return 0; } diff --git a/src/push.c b/src/push.c index 9943f215c..be5ec1c0e 100644 --- a/src/push.c +++ b/src/push.c @@ -716,14 +716,9 @@ void git_push_free(git_push *push) git__free(push); } -int git_push_init_options(git_push_options* opts, int version) +int git_push_init_options(git_push_options *opts, unsigned int version) { - if (version != GIT_PUSH_OPTIONS_VERSION) { - giterr_set(GITERR_INVALID, "Invalid version %d for git_push_options", version); - return -1; - } else { - git_push_options o = GIT_PUSH_OPTIONS_INIT; - memcpy(opts, &o, sizeof(o)); - return 0; - } + GIT_INIT_STRUCTURE_FROM_TEMPLATE( + opts, version, git_push_options, GIT_PUSH_OPTIONS_INIT); + return 0; } diff --git a/src/repository.c b/src/repository.c index 8daa04d5d..ac7af7692 100644 --- a/src/repository.c +++ b/src/repository.c @@ -2026,14 +2026,11 @@ int git_repository_is_shallow(git_repository *repo) return st.st_size == 0 ? 0 : 1; } -int git_repository_init_init_options(git_repository_init_options* opts, int version) +int git_repository_init_init_options( + git_repository_init_options *opts, unsigned int version) { - if (version != GIT_REPOSITORY_INIT_OPTIONS_VERSION) { - giterr_set(GITERR_INVALID, "Invalid version %d for git_repository_init_options", version); - return -1; - } else { - git_repository_init_options o = GIT_REPOSITORY_INIT_OPTIONS_INIT; - memcpy(opts, &o, sizeof(o)); - return 0; - } + GIT_INIT_STRUCTURE_FROM_TEMPLATE( + opts, version, git_repository_init_options, + GIT_REPOSITORY_INIT_OPTIONS_INIT); + return 0; } diff --git a/src/status.c b/src/status.c index aab838bcf..bcc2692d2 100644 --- a/src/status.c +++ b/src/status.c @@ -518,14 +518,10 @@ int git_status_should_ignore( return git_ignore_path_is_ignored(ignored, repo, path); } -int git_status_init_options(git_status_options* opts, unsigned int version) +int git_status_init_options(git_status_options *opts, unsigned int version) { - git_status_options o = GIT_STATUS_OPTIONS_INIT; - if (version != o.version) { - giterr_set(GITERR_INVALID, "Invalid version %d for git_status_options", version); - return -1; - } - memcpy(opts, &o, sizeof(o)); + GIT_INIT_STRUCTURE_FROM_TEMPLATE( + opts, version, git_status_options, GIT_STATUS_OPTIONS_INIT); return 0; } From bc91347b5894c98964a12c6637d5ca91d9723b42 Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Wed, 30 Apr 2014 11:16:31 -0700 Subject: [PATCH 11/13] Fix remaining init_options inconsistencies There were a couple of "init_opts()" functions a few more cases of structure initialization that I somehow missed. --- include/git2/checkout.h | 11 +++++------ include/git2/cherrypick.h | 11 +++++------ include/git2/remote.h | 9 ++++----- include/git2/revert.h | 11 +++++------ include/git2/sys/config.h | 9 ++++----- include/git2/sys/odb_backend.h | 9 ++++----- include/git2/sys/refdb_backend.h | 9 ++++----- include/git2/transport.h | 9 ++++----- src/checkout.c | 13 ++++--------- src/cherrypick.c | 14 +++++--------- src/config.c | 13 ++++--------- src/diff.c | 11 ++--------- src/odb.c | 13 ++++--------- src/refdb.c | 13 ++++--------- src/remote.c | 13 ++++--------- src/revert.c | 13 ++++--------- src/status.c | 7 ++----- src/transport.c | 13 ++++--------- tests/structinit/structinit.c | 4 ++-- 19 files changed, 74 insertions(+), 131 deletions(-) diff --git a/include/git2/checkout.h b/include/git2/checkout.h index 69addb7d9..494f67456 100644 --- a/include/git2/checkout.h +++ b/include/git2/checkout.h @@ -270,14 +270,13 @@ typedef struct git_checkout_options { * Initializes a `git_checkout_options` with default values. Equivalent to * creating an instance with GIT_CHECKOUT_OPTIONS_INIT. * -* @param opts the `git_checkout_options` instance to initialize. -* @param version the version of the struct; you should pass -* `GIT_CHECKOUT_OPTIONS_VERSION` here. +* @param opts the `git_checkout_options` struct to initialize. +* @param version Version of struct; pass `GIT_CHECKOUT_OPTIONS_VERSION` * @return Zero on success; -1 on failure. */ -GIT_EXTERN(int) git_checkout_init_opts( - git_checkout_options* opts, - int version); +GIT_EXTERN(int) git_checkout_init_options( + git_checkout_options *opts, + unsigned int version); /** * Updates files in the index and the working tree to match the content of diff --git a/include/git2/cherrypick.h b/include/git2/cherrypick.h index 7c48e6659..e998d325f 100644 --- a/include/git2/cherrypick.h +++ b/include/git2/cherrypick.h @@ -37,14 +37,13 @@ typedef struct { * Initializes a `git_cherry_pick_options` with default values. Equivalent to * creating an instance with GIT_CHERRY_PICK_OPTIONS_INIT. * - * @param opts the `git_cherry_pick_options` instance to initialize. - * @param version the version of the struct; you should pass - * `GIT_CHERRY_PICK_OPTIONS_VERSION` here. + * @param opts the `git_cherry_pick_options` struct to initialize + * @param version Version of struct; pass `GIT_CHERRY_PICK_OPTIONS_VERSION` * @return Zero on success; -1 on failure. */ -GIT_EXTERN(int) git_cherry_pick_init_opts( - git_cherry_pick_options* opts, - int version); +GIT_EXTERN(int) git_cherry_pick_init_options( + git_cherry_pick_options *opts, + unsigned int version); /** * Cherry-picks the given commit against the given "our" commit, producing an diff --git a/include/git2/remote.h b/include/git2/remote.h index 11e1e26d0..3633501e2 100644 --- a/include/git2/remote.h +++ b/include/git2/remote.h @@ -501,14 +501,13 @@ struct git_remote_callbacks { * Initializes a `git_remote_callbacks` with default values. Equivalent to * creating an instance with GIT_REMOTE_CALLBACKS_INIT. * - * @param opts the `git_remote_callbacks` instance to initialize. - * @param version the version of the struct; you should pass - * `GIT_REMOTE_CALLBACKS_VERSION` here. + * @param opts the `git_remote_callbacks` struct to initialize + * @param version Version of struct; pass `GIT_REMOTE_CALLBACKS_VERSION` * @return Zero on success; -1 on failure. */ GIT_EXTERN(int) git_remote_init_callbacks( - git_remote_callbacks* opts, - int version); + git_remote_callbacks *opts, + unsigned int version); /** * Set the callbacks for a remote diff --git a/include/git2/revert.h b/include/git2/revert.h index 3a6beb6b8..da37fbe7b 100644 --- a/include/git2/revert.h +++ b/include/git2/revert.h @@ -37,14 +37,13 @@ typedef struct { * Initializes a `git_revert_options` with default values. Equivalent to * creating an instance with GIT_REVERT_OPTIONS_INIT. * - * @param opts the `git_revert_options` instance to initialize. - * @param version the version of the struct; you should pass - * `GIT_REVERT_OPTIONS_VERSION` here. + * @param opts the `git_revert_options` struct to initialize + * @param version Version of struct; pass `GIT_REVERT_OPTIONS_VERSION` * @return Zero on success; -1 on failure. */ -GIT_EXTERN(int) git_revert_init_opts( - git_revert_options* opts, - int version); +GIT_EXTERN(int) git_revert_init_options( + git_revert_options *opts, + unsigned int version); /** * Reverts the given commit against the given "our" commit, producing an diff --git a/include/git2/sys/config.h b/include/git2/sys/config.h index 3df2ba327..46bb65293 100644 --- a/include/git2/sys/config.h +++ b/include/git2/sys/config.h @@ -73,14 +73,13 @@ struct git_config_backend { * Initializes a `git_config_backend` with default values. Equivalent to * creating an instance with GIT_CONFIG_BACKEND_INIT. * - * @param opts the `git_config_backend` instance to initialize. - * @param version the version of the struct; you should pass - * `GIT_CONFIG_BACKEND_VERSION` here. + * @param opts the `git_config_backend` struct to initialize. + * @param version Version of struct; pass `GIT_CONFIG_BACKEND_VERSION` * @return Zero on success; -1 on failure. */ GIT_EXTERN(int) git_config_init_backend( - git_config_backend* backend, - int version); + git_config_backend *backend, + unsigned int version); /** * Add a generic config file instance to an existing config diff --git a/include/git2/sys/odb_backend.h b/include/git2/sys/odb_backend.h index 77fe0dd31..1fc3c3159 100644 --- a/include/git2/sys/odb_backend.h +++ b/include/git2/sys/odb_backend.h @@ -93,14 +93,13 @@ struct git_odb_backend { * Initializes a `git_odb_backend` with default values. Equivalent to * creating an instance with GIT_ODB_BACKEND_INIT. * - * @param opts the `git_odb_backend` instance to initialize. - * @param version the version of the struct; you should pass - * `GIT_ODB_BACKEND_VERSION` here. + * @param opts the `git_odb_backend` struct to initialize. + * @param version Version the struct; pass `GIT_ODB_BACKEND_VERSION` * @return Zero on success; -1 on failure. */ GIT_EXTERN(int) git_odb_init_backend( - git_odb_backend* backend, - int version); + git_odb_backend *backend, + unsigned int version); GIT_EXTERN(void *) git_odb_backend_malloc(git_odb_backend *backend, size_t len); diff --git a/include/git2/sys/refdb_backend.h b/include/git2/sys/refdb_backend.h index dce142c77..3b216a287 100644 --- a/include/git2/sys/refdb_backend.h +++ b/include/git2/sys/refdb_backend.h @@ -162,14 +162,13 @@ struct git_refdb_backend { * Initializes a `git_refdb_backend` with default values. Equivalent to * creating an instance with GIT_REFDB_BACKEND_INIT. * - * @param opts the `git_refdb_backend` instance to initialize. - * @param version the version of the struct; you should pass - * `GIT_REFDB_BACKEND_VERSION` here. + * @param opts the `git_refdb_backend` struct to initialize + * @param version Version of struct; pass `GIT_REFDB_BACKEND_VERSION` * @return Zero on success; -1 on failure. */ GIT_EXTERN(int) git_refdb_init_backend( - git_refdb_backend* backend, - int version); + git_refdb_backend *backend, + unsigned int version); /** * Constructors for default filesystem-based refdb backend diff --git a/include/git2/transport.h b/include/git2/transport.h index a33146ca8..af7812b5d 100644 --- a/include/git2/transport.h +++ b/include/git2/transport.h @@ -314,14 +314,13 @@ struct git_transport { * Initializes a `git_transport` with default values. Equivalent to * creating an instance with GIT_TRANSPORT_INIT. * - * @param opts the `git_transport` instance to initialize. - * @param version the version of the struct; you should pass - * `GIT_TRANSPORT_VERSION` here. + * @param opts the `git_transport` struct to initialize + * @param version Version of struct; pass `GIT_TRANSPORT_VERSION` * @return Zero on success; -1 on failure. */ GIT_EXTERN(int) git_transport_init( - git_transport* opts, - int version); + git_transport *opts, + unsigned int version); /** * Function to use to create a transport from a URL. The transport database diff --git a/src/checkout.c b/src/checkout.c index 727911694..b869efe2b 100644 --- a/src/checkout.c +++ b/src/checkout.c @@ -2240,14 +2240,9 @@ int git_checkout_head( return git_checkout_tree(repo, NULL, opts); } -int git_checkout_init_opts(git_checkout_options* opts, int version) +int git_checkout_init_options(git_checkout_options *opts, unsigned int version) { - if (version != GIT_CHECKOUT_OPTIONS_VERSION) { - giterr_set(GITERR_INVALID, "Invalid version %d for git_checkout_options", version); - return -1; - } else { - git_checkout_options o = GIT_CHECKOUT_OPTIONS_INIT; - memcpy(opts, &o, sizeof(o)); - return 0; - } + GIT_INIT_STRUCTURE_FROM_TEMPLATE( + opts, version, git_checkout_options, GIT_CHECKOUT_OPTIONS_INIT); + return 0; } diff --git a/src/cherrypick.c b/src/cherrypick.c index 6a5ca834c..e02348a03 100644 --- a/src/cherrypick.c +++ b/src/cherrypick.c @@ -217,14 +217,10 @@ done: return error; } -int git_cherry_pick_init_opts(git_cherry_pick_options* opts, int version) +int git_cherry_pick_init_options( + git_cherry_pick_options *opts, unsigned int version) { - if (version != GIT_CHERRY_PICK_OPTIONS_VERSION) { - giterr_set(GITERR_INVALID, "Invalid version %d for git_cherry_pick_options", version); - return -1; - } else { - git_cherry_pick_options o = GIT_CHERRY_PICK_OPTIONS_INIT; - memcpy(opts, &o, sizeof(o)); - return 0; - } + GIT_INIT_STRUCTURE_FROM_TEMPLATE( + opts, version, git_cherry_pick_options, GIT_CHERRY_PICK_OPTIONS_INIT); + return 0; } diff --git a/src/config.c b/src/config.c index 16854c0c8..ee92398fc 100644 --- a/src/config.c +++ b/src/config.c @@ -1276,14 +1276,9 @@ cleanup: return error; } -int git_config_init_backend(git_config_backend* backend, int version) +int git_config_init_backend(git_config_backend *backend, unsigned int version) { - if (version != GIT_CONFIG_BACKEND_VERSION) { - giterr_set(GITERR_INVALID, "Invalid version %d for git_config_backend", version); - return -1; - } else { - git_config_backend b = GIT_CONFIG_BACKEND_INIT; - memcpy(backend, &b, sizeof(b)); - return 0; - } + GIT_INIT_STRUCTURE_FROM_TEMPLATE( + backend, version, git_config_backend, GIT_CONFIG_BACKEND_INIT); + return 0; } diff --git a/src/diff.c b/src/diff.c index 56f333f76..781f23ec6 100644 --- a/src/diff.c +++ b/src/diff.c @@ -1310,17 +1310,10 @@ int git_diff_is_sorted_icase(const git_diff *diff) return (diff->opts.flags & GIT_DIFF_IGNORE_CASE) != 0; } -static int diff_options_bad_version(int version, const char *thing) -{ - giterr_set(GITERR_INVALID, "Invalid version %d for %s", version, thing); - return -1; -} - int git_diff_get_perfdata(git_diff_perfdata *out, const git_diff *diff) { - if (!out || out->version != GIT_DIFF_PERFDATA_VERSION) - return diff_options_bad_version( - out ? out->version : 0, "git_diff_perfdata"); + assert(out); + GITERR_CHECK_VERSION(out, GIT_DIFF_PERFDATA_VERSION, "git_diff_perfdata"); out->stat_calls = diff->perf.stat_calls; out->oid_calculations = diff->perf.oid_calculations; return 0; diff --git a/src/odb.c b/src/odb.c index 00740d2e2..20a3f6c6e 100644 --- a/src/odb.c +++ b/src/odb.c @@ -1123,14 +1123,9 @@ int git_odb__error_ambiguous(const char *message) return GIT_EAMBIGUOUS; } -int git_odb_init_backend(git_odb_backend* backend, int version) +int git_odb_init_backend(git_odb_backend *backend, unsigned int version) { - if (version != GIT_ODB_BACKEND_VERSION) { - giterr_set(GITERR_INVALID, "Invalid version %d for git_odb_backend", version); - return -1; - } else { - git_odb_backend b = GIT_ODB_BACKEND_INIT; - memcpy(backend, &b, sizeof(b)); - return 0; - } + GIT_INIT_STRUCTURE_FROM_TEMPLATE( + backend, version, git_odb_backend, GIT_ODB_BACKEND_INIT); + return 0; } diff --git a/src/refdb.c b/src/refdb.c index 3e7a592f8..69bf74734 100644 --- a/src/refdb.c +++ b/src/refdb.c @@ -236,14 +236,9 @@ int git_refdb_ensure_log(git_refdb *db, const char *refname) return db->backend->ensure_log(db->backend, refname); } -int git_refdb_init_backend(git_refdb_backend* backend, int version) +int git_refdb_init_backend(git_refdb_backend *backend, unsigned int version) { - if (version != GIT_REFDB_BACKEND_VERSION) { - giterr_set(GITERR_INVALID, "Invalid version %d for git_refdb_backend", version); - return -1; - } else { - git_refdb_backend b = GIT_REFDB_BACKEND_INIT; - memcpy(backend, &b, sizeof(b)); - return 0; - } + GIT_INIT_STRUCTURE_FROM_TEMPLATE( + backend, version, git_refdb_backend, GIT_REFDB_BACKEND_INIT); + return 0; } diff --git a/src/remote.c b/src/remote.c index be7198a98..8f302a51e 100644 --- a/src/remote.c +++ b/src/remote.c @@ -1736,14 +1736,9 @@ const git_refspec *git_remote_get_refspec(const git_remote *remote, size_t n) return git_vector_get(&remote->refspecs, n); } -int git_remote_init_callbacks(git_remote_callbacks* opts, int version) +int git_remote_init_callbacks(git_remote_callbacks *opts, unsigned int version) { - if (version != GIT_REMOTE_CALLBACKS_VERSION) { - giterr_set(GITERR_INVALID, "Invalid version %d for git_remote_callbacks", version); - return -1; - } else { - git_remote_callbacks o = GIT_REMOTE_CALLBACKS_INIT; - memcpy(opts, &o, sizeof(o)); - return 0; - } + GIT_INIT_STRUCTURE_FROM_TEMPLATE( + opts, version, git_remote_callbacks, GIT_REMOTE_CALLBACKS_INIT); + return 0; } diff --git a/src/revert.c b/src/revert.c index 29e124f6c..9c587724b 100644 --- a/src/revert.c +++ b/src/revert.c @@ -220,14 +220,9 @@ done: return error; } -int git_revert_init_opts(git_revert_options* opts, int version) +int git_revert_init_options(git_revert_options *opts, unsigned int version) { - if (version != GIT_REVERT_OPTIONS_VERSION) { - giterr_set(GITERR_INVALID, "Invalid version %d for git_revert_options", version); - return -1; - } else { - git_revert_options o = GIT_REVERT_OPTIONS_INIT; - memcpy(opts, &o, sizeof(o)); - return 0; - } + GIT_INIT_STRUCTURE_FROM_TEMPLATE( + opts, version, git_revert_options, GIT_REVERT_OPTIONS_INIT); + return 0; } diff --git a/src/status.c b/src/status.c index bcc2692d2..8d7612f72 100644 --- a/src/status.c +++ b/src/status.c @@ -528,11 +528,8 @@ int git_status_init_options(git_status_options *opts, unsigned int version) int git_status_list_get_perfdata( git_diff_perfdata *out, const git_status_list *status) { - if (!out || out->version != GIT_DIFF_PERFDATA_VERSION) { - giterr_set(GITERR_INVALID, "Invalid version %d for git_diff_perfdata", - out ? out->version : 0); - return -1; - } + assert(out); + GITERR_CHECK_VERSION(out, GIT_DIFF_PERFDATA_VERSION, "git_diff_perfdata"); out->stat_calls = 0; out->oid_calculations = 0; diff --git a/src/transport.c b/src/transport.c index dc074a503..2194b1864 100644 --- a/src/transport.c +++ b/src/transport.c @@ -218,14 +218,9 @@ int git_remote_supported_url(const char* url) return fn != &git_transport_dummy; } -int git_transport_init(git_transport* opts, int version) +int git_transport_init(git_transport *opts, unsigned int version) { - if (version != GIT_TRANSPORT_VERSION) { - giterr_set(GITERR_INVALID, "Invalid version %d for git_transport", version); - return -1; - } else { - git_transport o = GIT_TRANSPORT_INIT; - memcpy(opts, &o, sizeof(o)); - return 0; - } + GIT_INIT_STRUCTURE_FROM_TEMPLATE( + opts, version, git_transport, GIT_TRANSPORT_INIT); + return 0; } diff --git a/tests/structinit/structinit.c b/tests/structinit/structinit.c index 2942099dd..38bedada7 100644 --- a/tests/structinit/structinit.c +++ b/tests/structinit/structinit.c @@ -48,7 +48,7 @@ void test_structinit_structinit__compare(void) /* checkout */ CHECK_MACRO_FUNC_INIT_EQUAL( \ git_checkout_options, GIT_CHECKOUT_OPTIONS_VERSION, \ - GIT_CHECKOUT_OPTIONS_INIT, git_checkout_init_opts); + GIT_CHECKOUT_OPTIONS_INIT, git_checkout_init_options); /* clone */ CHECK_MACRO_FUNC_INIT_EQUAL( \ @@ -98,7 +98,7 @@ void test_structinit_structinit__compare(void) /* revert */ CHECK_MACRO_FUNC_INIT_EQUAL( \ git_revert_options, GIT_REVERT_OPTIONS_VERSION, \ - GIT_REVERT_OPTIONS_INIT, git_revert_init_opts); + GIT_REVERT_OPTIONS_INIT, git_revert_init_options); /* status */ CHECK_MACRO_FUNC_INIT_EQUAL( \ From 0f603132bc2397bf8308e72651c56cb7dbd3ad70 Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Thu, 1 May 2014 14:47:33 -0700 Subject: [PATCH 12/13] Improve handling of fake home directory There are a few tests that set up a fake home directory and a fake GLOBAL search path so that we can test things in global ignore or attribute or config files. This cleans up that code to work more robustly even if there is a test failure. This also fixes some valgrind warnings where scanning search paths for separators could end up doing a little bit of sketchy data access when coming to the end of search list. --- src/config.c | 18 ++++++++---------- src/sysdir.c | 16 +++++++++++----- tests/attr/ignore.c | 5 ++--- tests/clar_libgit2.c | 31 +++++++++++++++++++++---------- tests/clar_libgit2.h | 9 +++++++-- tests/config/global.c | 35 +++++++++++++++++++++++++++-------- tests/status/ignore.c | 5 +---- 7 files changed, 77 insertions(+), 42 deletions(-) diff --git a/src/config.c b/src/config.c index ee92398fc..f9d697197 100644 --- a/src/config.c +++ b/src/config.c @@ -984,24 +984,22 @@ int git_config__global_location(git_buf *buf) { const git_buf *paths; const char *sep, *start; - size_t len; if (git_sysdir_get(&paths, GIT_SYSDIR_GLOBAL) < 0) return -1; /* no paths, so give up */ - if (git_buf_len(paths) == 0) + if (!paths || !git_buf_len(paths)) return -1; - start = git_buf_cstr(paths); - sep = strchr(start, GIT_PATH_LIST_SEPARATOR); + /* find unescaped separator or end of string */ + for (sep = start = git_buf_cstr(paths); *sep; ++sep) { + if (*sep == GIT_PATH_LIST_SEPARATOR && + (sep <= start || sep[-1] != '\\')) + break; + } - if (sep) - len = sep - start; - else - len = paths->size; - - if (git_buf_set(buf, start, len) < 0) + if (git_buf_set(buf, start, (size_t)(sep - start)) < 0) return -1; return git_buf_joinpath(buf, buf->ptr, GIT_CONFIG_FILENAME_GLOBAL); diff --git a/src/sysdir.c b/src/sysdir.c index aebf23135..e0c24f3b3 100644 --- a/src/sysdir.c +++ b/src/sysdir.c @@ -194,14 +194,19 @@ static int git_sysdir_find_in_dirlist( const git_buf *syspath; GITERR_CHECK_ERROR(git_sysdir_get(&syspath, which)); + if (!syspath || !git_buf_len(syspath)) + goto done; for (scan = git_buf_cstr(syspath); scan; scan = next) { - for (next = strchr(scan, GIT_PATH_LIST_SEPARATOR); - next && next > scan && next[-1] == '\\'; - next = strchr(next + 1, GIT_PATH_LIST_SEPARATOR)) - /* find unescaped separator or end of string */; + /* find unescaped separator or end of string */ + for (next = scan; *next; ++next) { + if (*next == GIT_PATH_LIST_SEPARATOR && + (next <= scan || next[-1] != '\\')) + break; + } - len = next ? (size_t)(next++ - scan) : strlen(scan); + len = (size_t)(next - scan); + next = (*next ? next + 1 : NULL); if (!len) continue; @@ -213,6 +218,7 @@ static int git_sysdir_find_in_dirlist( return 0; } +done: git_buf_free(path); giterr_set(GITERR_OS, "The %s file '%s' doesn't exist", label, name); return GIT_ENOTFOUND; diff --git a/tests/attr/ignore.c b/tests/attr/ignore.c index 5eadc7b3e..b187db01c 100644 --- a/tests/attr/ignore.c +++ b/tests/attr/ignore.c @@ -148,12 +148,11 @@ void test_attr_ignore__skip_gitignore_directory(void) void test_attr_ignore__expand_tilde_to_homedir(void) { - git_buf cleanup = GIT_BUF_INIT; git_config *cfg; assert_is_ignored(false, "example.global_with_tilde"); - cl_fake_home(&cleanup); + cl_fake_home(); /* construct fake home with fake global excludes */ cl_git_mkfile("home/globalexclude", "# found me\n*.global_with_tilde\n"); @@ -168,7 +167,7 @@ void test_attr_ignore__expand_tilde_to_homedir(void) cl_git_pass(git_futils_rmdir_r("home", NULL, GIT_RMDIR_REMOVE_FILES)); - cl_fake_home_cleanup(&cleanup); + cl_fake_home_cleanup(NULL); git_attr_cache_flush(g_repo); /* must reset to pick up change */ diff --git a/tests/clar_libgit2.c b/tests/clar_libgit2.c index 6f6143dad..9ec9a6419 100644 --- a/tests/clar_libgit2.c +++ b/tests/clar_libgit2.c @@ -484,23 +484,34 @@ void clar__assert_equal_file( (size_t)expected_bytes, (size_t)total_bytes); } -void cl_fake_home(git_buf *restore) +static char *_cl_restore_home = NULL; + +void cl_fake_home_cleanup(void *payload) +{ + char *restore = _cl_restore_home; + _cl_restore_home = NULL; + + GIT_UNUSED(payload); + + cl_git_pass(git_libgit2_opts( + GIT_OPT_SET_SEARCH_PATH, GIT_CONFIG_LEVEL_GLOBAL, restore)); + git__free(restore); +} + +void cl_fake_home(void) { git_buf path = GIT_BUF_INIT; cl_git_pass(git_libgit2_opts( - GIT_OPT_GET_SEARCH_PATH, GIT_CONFIG_LEVEL_GLOBAL, restore)); + GIT_OPT_GET_SEARCH_PATH, GIT_CONFIG_LEVEL_GLOBAL, &path)); - cl_must_pass(p_mkdir("home", 0777)); + _cl_restore_home = git_buf_detach(&path); + cl_set_cleanup(cl_fake_home_cleanup, NULL); + + if (!git_path_exists("home")) + cl_must_pass(p_mkdir("home", 0777)); cl_git_pass(git_path_prettify(&path, "home", NULL)); cl_git_pass(git_libgit2_opts( GIT_OPT_SET_SEARCH_PATH, GIT_CONFIG_LEVEL_GLOBAL, path.ptr)); git_buf_free(&path); } - -void cl_fake_home_cleanup(git_buf *restore) -{ - cl_git_pass(git_libgit2_opts( - GIT_OPT_SET_SEARCH_PATH, GIT_CONFIG_LEVEL_GLOBAL, restore->ptr)); - git_buf_free(restore); -} diff --git a/tests/clar_libgit2.h b/tests/clar_libgit2.h index 082fa9f4a..f84d9e353 100644 --- a/tests/clar_libgit2.h +++ b/tests/clar_libgit2.h @@ -131,7 +131,12 @@ int cl_repo_get_bool(git_repository *repo, const char *cfg); void cl_repo_set_string(git_repository *repo, const char *cfg, const char *value); -void cl_fake_home(git_buf *restore); -void cl_fake_home_cleanup(git_buf *restore); +/* set up a fake "home" directory and set libgit2 GLOBAL search path. + * + * automatically configures cleanup function to restore the regular search + * path, although you can call it explicitly if you wish (with NULL). + */ +void cl_fake_home(void); +void cl_fake_home_cleanup(void *); #endif diff --git a/tests/config/global.c b/tests/config/global.c index d5f95f504..006b34628 100644 --- a/tests/config/global.c +++ b/tests/config/global.c @@ -2,22 +2,36 @@ #include "buffer.h" #include "fileops.h" +static git_config_level_t setting[3] = { + GIT_CONFIG_LEVEL_GLOBAL, + GIT_CONFIG_LEVEL_XDG, + GIT_CONFIG_LEVEL_SYSTEM +}; +static char *restore[3]; + void test_config_global__initialize(void) { + int i; git_buf path = GIT_BUF_INIT; - cl_assert_equal_i(0, p_mkdir("home", 0777)); + /* snapshot old settings to restore later */ + for (i = 0; i < 3; ++i) { + cl_git_pass( + git_libgit2_opts(GIT_OPT_GET_SEARCH_PATH, setting[i], &path)); + restore[i] = git_buf_detach(&path); + } + + cl_git_pass(git_futils_mkdir_r("home", NULL, 0777)); cl_git_pass(git_path_prettify(&path, "home", NULL)); cl_git_pass(git_libgit2_opts( GIT_OPT_SET_SEARCH_PATH, GIT_CONFIG_LEVEL_GLOBAL, path.ptr)); - cl_assert_equal_i(0, p_mkdir("xdg", 0777)); - cl_assert_equal_i(0, p_mkdir("xdg/git", 0777)); + cl_git_pass(git_futils_mkdir_r("xdg/git", NULL, 0777)); cl_git_pass(git_path_prettify(&path, "xdg/git", NULL)); cl_git_pass(git_libgit2_opts( GIT_OPT_SET_SEARCH_PATH, GIT_CONFIG_LEVEL_XDG, path.ptr)); - cl_assert_equal_i(0, p_mkdir("etc", 0777)); + cl_git_pass(git_futils_mkdir_r("etc", NULL, 0777)); cl_git_pass(git_path_prettify(&path, "etc", NULL)); cl_git_pass(git_libgit2_opts( GIT_OPT_SET_SEARCH_PATH, GIT_CONFIG_LEVEL_SYSTEM, path.ptr)); @@ -27,13 +41,18 @@ void test_config_global__initialize(void) void test_config_global__cleanup(void) { + int i; + + for (i = 0; i < 3; ++i) { + cl_git_pass( + git_libgit2_opts(GIT_OPT_SET_SEARCH_PATH, setting[i], restore[i])); + git__free(restore[i]); + restore[i] = NULL; + } + cl_git_pass(git_futils_rmdir_r("home", NULL, GIT_RMDIR_REMOVE_FILES)); cl_git_pass(git_futils_rmdir_r("xdg", NULL, GIT_RMDIR_REMOVE_FILES)); cl_git_pass(git_futils_rmdir_r("etc", NULL, GIT_RMDIR_REMOVE_FILES)); - - git_libgit2_opts(GIT_OPT_SET_SEARCH_PATH, GIT_CONFIG_LEVEL_SYSTEM, NULL); - git_libgit2_opts(GIT_OPT_SET_SEARCH_PATH, GIT_CONFIG_LEVEL_XDG, NULL); - git_libgit2_opts(GIT_OPT_SET_SEARCH_PATH, GIT_CONFIG_LEVEL_GLOBAL, NULL); } void test_config_global__open_global(void) diff --git a/tests/status/ignore.c b/tests/status/ignore.c index d88b2eb6b..caa1f1927 100644 --- a/tests/status/ignore.c +++ b/tests/status/ignore.c @@ -364,7 +364,6 @@ void test_status_ignore__leading_slash_ignores(void) { git_status_options opts = GIT_STATUS_OPTIONS_INIT; status_entry_counts counts; - git_buf home = GIT_BUF_INIT; static const char *paths_2[] = { "dir/.gitignore", "dir/a/ignore_me", @@ -385,7 +384,7 @@ void test_status_ignore__leading_slash_ignores(void) make_test_data(test_repo_1, test_files_1); - cl_fake_home(&home); + cl_fake_home(); cl_git_mkfile("home/.gitignore", "/ignore_me\n"); { git_config *cfg; @@ -412,8 +411,6 @@ void test_status_ignore__leading_slash_ignores(void) cl_assert_equal_i(counts.expected_entry_count, counts.entry_count); cl_assert_equal_i(0, counts.wrong_status_flags_count); cl_assert_equal_i(0, counts.wrong_sorted_path); - - cl_fake_home_cleanup(&home); } void test_status_ignore__contained_dir_with_matching_name(void) From 99dfa470398b9c4e06e5a5ee61868d3b9e21b26e Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Thu, 1 May 2014 15:12:12 -0700 Subject: [PATCH 13/13] Some further sandboxing cleanups to tests Trying to find other issues where tests may not clean up quite properly when they are through... --- tests/attr/repo.c | 5 ----- tests/clar_libgit2.c | 8 +++++--- tests/refs/branches/create.c | 9 +++------ tests/refs/branches/delete.c | 7 ++----- tests/refs/branches/ishead.c | 24 +++--------------------- 5 files changed, 13 insertions(+), 40 deletions(-) diff --git a/tests/attr/repo.c b/tests/attr/repo.c index 9aab7ed96..5e812a72b 100644 --- a/tests/attr/repo.c +++ b/tests/attr/repo.c @@ -9,11 +9,6 @@ static git_repository *g_repo = NULL; void test_attr_repo__initialize(void) { - /* Before each test, instantiate the attr repo from the fixtures and - * rename the .gitted to .git so it is a repo with a working dir. - * Also rename gitattributes to .gitattributes, because it contains - * macro definitions which are only allowed in the root. - */ g_repo = cl_git_sandbox_init("attr"); } diff --git a/tests/clar_libgit2.c b/tests/clar_libgit2.c index 9ec9a6419..f457adb33 100644 --- a/tests/clar_libgit2.c +++ b/tests/clar_libgit2.c @@ -493,9 +493,11 @@ void cl_fake_home_cleanup(void *payload) GIT_UNUSED(payload); - cl_git_pass(git_libgit2_opts( - GIT_OPT_SET_SEARCH_PATH, GIT_CONFIG_LEVEL_GLOBAL, restore)); - git__free(restore); + if (restore) { + cl_git_pass(git_libgit2_opts( + GIT_OPT_SET_SEARCH_PATH, GIT_CONFIG_LEVEL_GLOBAL, restore)); + git__free(restore); + } } void cl_fake_home(void) diff --git a/tests/refs/branches/create.c b/tests/refs/branches/create.c index b91eed6e8..38af2f681 100644 --- a/tests/refs/branches/create.c +++ b/tests/refs/branches/create.c @@ -7,10 +7,9 @@ static git_reference *branch; void test_refs_branches_create__initialize(void) { - cl_fixture_sandbox("testrepo.git"); - cl_git_pass(git_repository_open(&repo, "testrepo.git")); - + repo = cl_git_sandbox_init("testrepo.git"); branch = NULL; + target = NULL; } void test_refs_branches_create__cleanup(void) @@ -21,10 +20,8 @@ void test_refs_branches_create__cleanup(void) git_commit_free(target); target = NULL; - git_repository_free(repo); + cl_git_sandbox_cleanup(); repo = NULL; - - cl_fixture_cleanup("testrepo.git"); } static void retrieve_target_from_oid(git_commit **out, git_repository *repo, const char *sha) diff --git a/tests/refs/branches/delete.c b/tests/refs/branches/delete.c index ed5f1627b..e3199e230 100644 --- a/tests/refs/branches/delete.c +++ b/tests/refs/branches/delete.c @@ -10,8 +10,7 @@ void test_refs_branches_delete__initialize(void) { git_oid id; - cl_fixture_sandbox("testrepo.git"); - cl_git_pass(git_repository_open(&repo, "testrepo.git")); + repo = cl_git_sandbox_init("testrepo.git"); cl_git_pass(git_oid_fromstr(&id, "be3563ae3f795b2b4353bcce3a527ad0a4f7f644")); cl_git_pass(git_reference_create(&fake_remote, repo, "refs/remotes/nulltoken/master", &id, 0, NULL, NULL)); @@ -22,10 +21,8 @@ void test_refs_branches_delete__cleanup(void) git_reference_free(fake_remote); fake_remote = NULL; - git_repository_free(repo); + cl_git_sandbox_cleanup(); repo = NULL; - - cl_fixture_cleanup("testrepo.git"); } void test_refs_branches_delete__can_not_delete_a_branch_pointed_at_by_HEAD(void) diff --git a/tests/refs/branches/ishead.c b/tests/refs/branches/ishead.c index 12a8c4449..d16a79652 100644 --- a/tests/refs/branches/ishead.c +++ b/tests/refs/branches/ishead.c @@ -7,7 +7,8 @@ static git_reference *branch; void test_refs_branches_ishead__initialize(void) { - cl_git_pass(git_repository_open(&repo, cl_fixture("testrepo.git"))); + repo = cl_git_sandbox_init("testrepo.git"); + branch = NULL; } void test_refs_branches_ishead__cleanup(void) @@ -15,7 +16,7 @@ void test_refs_branches_ishead__cleanup(void) git_reference_free(branch); branch = NULL; - git_repository_free(repo); + cl_git_sandbox_cleanup(); repo = NULL; } @@ -28,34 +29,20 @@ void test_refs_branches_ishead__can_tell_if_a_branch_is_pointed_at_by_HEAD(void) void test_refs_branches_ishead__can_properly_handle_unborn_HEAD(void) { - git_repository_free(repo); - - repo = cl_git_sandbox_init("testrepo.git"); - make_head_unborn(repo, NON_EXISTING_HEAD); cl_git_pass(git_reference_lookup(&branch, repo, "refs/heads/master")); cl_assert_equal_i(false, git_branch_is_head(branch)); - - cl_git_sandbox_cleanup(); - repo = NULL; } void test_refs_branches_ishead__can_properly_handle_missing_HEAD(void) { - git_repository_free(repo); - - repo = cl_git_sandbox_init("testrepo.git"); - delete_head(repo); cl_git_pass(git_reference_lookup(&branch, repo, "refs/heads/master")); cl_assert_equal_i(false, git_branch_is_head(branch)); - - cl_git_sandbox_cleanup(); - repo = NULL; } void test_refs_branches_ishead__can_tell_if_a_branch_is_not_pointed_at_by_HEAD(void) @@ -95,9 +82,6 @@ void test_refs_branches_ishead__only_direct_references_are_considered(void) { git_reference *linked, *super, *head; - git_repository_free(repo); - repo = cl_git_sandbox_init("testrepo.git"); - cl_git_pass(git_reference_symbolic_create(&linked, repo, "refs/heads/linked", "refs/heads/master", 0, NULL, NULL)); cl_git_pass(git_reference_symbolic_create(&super, repo, "refs/heads/super", "refs/heads/linked", 0, NULL, NULL)); cl_git_pass(git_reference_symbolic_create(&head, repo, GIT_HEAD_FILE, "refs/heads/super", 1, NULL, NULL)); @@ -111,6 +95,4 @@ void test_refs_branches_ishead__only_direct_references_are_considered(void) git_reference_free(linked); git_reference_free(super); git_reference_free(head); - cl_git_sandbox_cleanup(); - repo = NULL; }