[PATCH 4/7] dir: hide untracked contents of untracked dirs
When we taught read_directory_recursive() to recurse into untracked directories in search of ignored files given DIR_SHOW_IGNORED_TOO, that had the side effect of teaching it to collect the untracked contents of untracked directories. It does not make sense to return these, so we teach read_directory() to strip dir->entries of any such untracked contents. --- dir.c | 44 1 file changed, 44 insertions(+) diff --git a/dir.c b/dir.c index 25cb9eadf..f0ddb4608 100644 --- a/dir.c +++ b/dir.c @@ -2075,6 +2075,50 @@ int read_directory(struct dir_struct *dir, const char *path, read_directory_recursive(dir, path, len, untracked, 0, pathspec); QSORT(dir->entries, dir->nr, cmp_name); QSORT(dir->ignored, dir->ignored_nr, cmp_name); + + // if collecting ignored files, never consider a directory containing + // ignored files to be untracked + if (dir->flags & DIR_SHOW_IGNORED_TOO) { + int i, j, nr_removed = 0; + + // remove from dir->entries untracked contents of untracked dirs + for (i = 0; i < dir->nr; i++) { + if (!dir->entries[i]) + continue; + + for (j = i + 1; j < dir->nr; j++) { + if (!dir->entries[j]) + continue; + if (check_contains(dir->entries[i], dir->entries[j])) { + nr_removed++; + free(dir->entries[j]); + dir->entries[j] = NULL; + } + else { + break; + } + } + } + + // strip dir->entries of NULLs + if (nr_removed) { + for (i = 0;;) { + while (i < dir->nr && dir->entries[i]) + i++; + if (i == dir->nr) + break; + j = i; + while (j < dir->nr && !dir->entries[j]) + j++; + if (j == dir->nr) + break; + dir->entries[i] = dir->entries[j]; + dir->entries[j] = NULL; + } + dir->nr -= nr_removed; + } + } + if (dir->untracked) { static struct trace_key trace_untracked_stats = TRACE_KEY_INIT(UNTRACKED_STATS); trace_printf_key(&trace_untracked_stats, -- 2.12.2
[PATCH 0/7] Keep git clean -d from inadvertently removing ignored files
This patch series fixes the bug where git clean -d culls directories containing only untracked and ignored files, by first teaching read_directory() and read_directory_recursive() to search "untracked" directories (read: directories *treated* as untracked because they only contain untracked and ignored files) for ignored contents, and then teaching cmd_clean() to skip untracked directories containing ignored files. This does, however, introduce a breaking change in the behavior of git status: when invoked with --ignored, git status will now return ignored files in an untracked directory, whereas previously it would not. First patches to the actual C code that I'm sending out! :D Looking forward to feedback - the changes I made in read_directory_recursive() and read_directory() feel a bit hacky, but I'm not sure how to get around that. Samuel Lijin (7): t7300: skip untracked dirs containing ignored files dir: recurse into untracked dirs for ignored files dir: add method to check if a dir_entry lexically contains another dir: hide untracked contents of untracked dirs dir: change linkage of cmp_name() and check_contains() builtin/clean: teach clean -d to skip dirs containing ignored files t7061: check for ignored file in untracked dir builtin/clean.c| 24 -- dir.c | 61 -- dir.h | 3 +++ t/t7061-wtstatus-ignore.sh | 1 + t/t7300-clean.sh | 10 5 files changed, 95 insertions(+), 4 deletions(-) -- 2.12.2
[PATCH 3/7] dir: add method to check if a dir_entry lexically contains another
Introduce a method that allows us to check if one dir_entry corresponds to a path which contains the path corresponding to another dir_entry. --- dir.c | 8 1 file changed, 8 insertions(+) diff --git a/dir.c b/dir.c index 6bd0350e9..25cb9eadf 100644 --- a/dir.c +++ b/dir.c @@ -1852,6 +1852,14 @@ static int cmp_name(const void *p1, const void *p2) return name_compare(e1->name, e1->len, e2->name, e2->len); } +// check if *out lexically contains *in +static int check_contains(const struct dir_entry *out, const struct dir_entry *in) +{ + return (out->len < in->len) && + (out->name[out->len - 1] == '/') && + !memcmp(out->name, in->name, out->len); +} + static int treat_leading_path(struct dir_struct *dir, const char *path, int len, const struct pathspec *pathspec) -- 2.12.2
[PATCH 6/7] builtin/clean: teach clean -d to skip dirs containing ignored files
There is an implicit assumption that a directory containing only untracked and ignored files should itself be considered untracked. This makes sense in use cases where we're asking if a directory should be added to the git database, but not when we're asking if a directory can be safely removed from the working tree; as a result, clean -d would assume that an "untracked" directory containing ignored files could be deleted. To get around this, we teach clean -d to collect ignored files and skip over so-called "untracked" directories if they contain any ignored files. --- builtin/clean.c | 24 ++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/builtin/clean.c b/builtin/clean.c index d861f836a..368e19427 100644 --- a/builtin/clean.c +++ b/builtin/clean.c @@ -859,7 +859,7 @@ static void interactive_main_loop(void) int cmd_clean(int argc, const char **argv, const char *prefix) { - int i, res; + int i, j, res; int dry_run = 0, remove_directories = 0, quiet = 0, ignored = 0; int ignored_only = 0, config_set = 0, errors = 0, gone = 1; int rm_flags = REMOVE_DIR_KEEP_NESTED_GIT; @@ -911,6 +911,9 @@ int cmd_clean(int argc, const char **argv, const char *prefix) " refusing to clean")); } + if (remove_directories) + dir.flags |= DIR_SHOW_IGNORED_TOO; + if (force > 1) rm_flags = 0; @@ -932,7 +935,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix) fill_directory(&dir, &pathspec); - for (i = 0; i < dir.nr; i++) { + for (j = i = 0; i < dir.nr; i++) { struct dir_entry *ent = dir.entries[i]; int matches = 0; struct stat st; @@ -954,10 +957,27 @@ int cmd_clean(int argc, const char **argv, const char *prefix) matches != MATCHED_EXACTLY) continue; + // skip any dir.entries which contains a dir.ignored + for (; j < dir.ignored_nr; j++) { + if (cmp_name(&dir.entries[i], + &dir.ignored[j]) < 0) + break; + } + if ((j < dir.ignored_nr) && + check_contains(dir.entries[i], dir.ignored[j])) { + continue; + } + rel = relative_path(ent->name, prefix, &buf); string_list_append(&del_list, rel); } + for (i = 0; i < dir.nr; i++) + free(dir.entries[i]); + + for (i = 0; i < dir.ignored_nr; i++) + free(dir.ignored[i]); + if (interactive && del_list.nr > 0) interactive_main_loop(); -- 2.12.2
[PATCH 7/7] t7061: check for ignored file in untracked dir
--- t/t7061-wtstatus-ignore.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/t/t7061-wtstatus-ignore.sh b/t/t7061-wtstatus-ignore.sh index cdc0747bf..fc6013ba3 100755 --- a/t/t7061-wtstatus-ignore.sh +++ b/t/t7061-wtstatus-ignore.sh @@ -9,6 +9,7 @@ cat >expected <<\EOF ?? actual ?? expected ?? untracked/ +!! untracked/ignored EOF test_expect_success 'status untracked directory with --ignored' ' -- 2.12.2
[PATCH 2/7] dir: recurse into untracked dirs for ignored files
We consider directories containing only untracked and ignored files to be themselves untracked, which in the usual case means we don't have to search these directories. This is problematic when we want to collect ignored files with DIR_SHOW_IGNORED_TOO, though, so we teach read_directory_recursive() to recurse into untracked directories to find the ignored files they contain when DIR_SHOW_IGNORED_TOO is set. This has the side effect of also collecting all untracked files in untracked directories as well. --- dir.c | 7 ++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/dir.c b/dir.c index f451bfa48..6bd0350e9 100644 --- a/dir.c +++ b/dir.c @@ -1784,7 +1784,12 @@ static enum path_treatment read_directory_recursive(struct dir_struct *dir, dir_state = state; /* recurse into subdir if instructed by treat_path */ - if (state == path_recurse) { + if ((state == path_recurse) || + ((get_dtype(cdir.de, path.buf, path.len) == DT_DIR) && +(state == path_untracked) && +(dir->flags & DIR_SHOW_IGNORED_TOO)) + ) + { struct untracked_cache_dir *ud; ud = lookup_untracked(dir->untracked, untracked, path.buf + baselen, -- 2.12.2
[PATCH 5/7] dir: change linkage of cmp_name() and check_contains()
--- dir.c | 4 ++-- dir.h | 3 +++ 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/dir.c b/dir.c index f0ddb4608..91103b561 100644 --- a/dir.c +++ b/dir.c @@ -1844,7 +1844,7 @@ static enum path_treatment read_directory_recursive(struct dir_struct *dir, return dir_state; } -static int cmp_name(const void *p1, const void *p2) +int cmp_name(const void *p1, const void *p2) { const struct dir_entry *e1 = *(const struct dir_entry **)p1; const struct dir_entry *e2 = *(const struct dir_entry **)p2; @@ -1853,7 +1853,7 @@ static int cmp_name(const void *p1, const void *p2) } // check if *out lexically contains *in -static int check_contains(const struct dir_entry *out, const struct dir_entry *in) +int check_contains(const struct dir_entry *out, const struct dir_entry *in) { return (out->len < in->len) && (out->name[out->len - 1] == '/') && diff --git a/dir.h b/dir.h index bf23a470a..1ddd8b611 100644 --- a/dir.h +++ b/dir.h @@ -326,6 +326,9 @@ static inline int dir_path_match(const struct dir_entry *ent, has_trailing_dir); } +int cmp_name(const void *p1, const void *p2); +int check_contains(const struct dir_entry *out, const struct dir_entry *in); + void untracked_cache_invalidate_path(struct index_state *, const char *); void untracked_cache_remove_from_index(struct index_state *, const char *); void untracked_cache_add_to_index(struct index_state *, const char *); -- 2.12.2
[PATCH 1/7] t7300: skip untracked dirs containing ignored files
If git sees a directory which contains only untracked and ignored files, clean -d should not remove that directory. --- t/t7300-clean.sh | 10 ++ 1 file changed, 10 insertions(+) diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh index b89fd2a6a..948a455e8 100755 --- a/t/t7300-clean.sh +++ b/t/t7300-clean.sh @@ -653,4 +653,14 @@ test_expect_success 'git clean -d respects pathspecs (pathspec is prefix of dir) test_path_is_dir foobar ' +test_expect_success 'git clean -d skips untracked dirs containing ignored files' ' + echo /foo/bar >.gitignore && + rm -rf foo && + mkdir -p foo && + touch foo/bar && + git clean -df && + test_path_is_file foo/bar && + test_path_is_dir foo +' + test_done -- 2.12.2
Su cuenta bancaria esta bloqueada
Su cuenta bancaria esta bloqueada. Para desbloquear tu cuenta, haz click aqui: http://baccredomaatic.com
Re: Git checkout issue - deleting file without apparent reason
On Tue, May 02, 2017 at 09:33:02PM -0400, Paul van Wichen wrote: > We are having a strange issue that we haven't been able to pin down. > Scenario: master branch and feature branch both have a specific file. > 1. Master checked out. > 2. git status show no changes / clean staging area. > 3. Checkout feature branch . > 4. git status show no changes / clean staging area. > 5. Checkout master again. > 6. git status shows that a file has been deleted (i.e. the file was > removed from the file system and the staging area shows it as > deleted). > > The file exists in both the feature branch and the master branch. As > best as we can tell, the file is identical on both commits. > The issue occurs on multiple platforms - tested on Windows and OS X. > It only occurs for 1 specific file. Just a guess, but might there be another file in the repository whose name differs only in case? On a case-insensitive filesystem that can cause gremlins like this, because the filesystem cannot represent all of the states in the git tree. -Peff
Re: Git checkout issue - deleting file without apparent reason
On Tue, May 2, 2017 at 6:33 PM, Paul van Wichen wrote: > Hi, > > We are having a strange issue that we haven't been able to pin down. > Scenario: master branch and feature branch both have a specific file. > 1. Master checked out. > 2. git status show no changes / clean staging area. > 3. Checkout feature branch . > 4. git status show no changes / clean staging area. > 5. Checkout master again. > 6. git status shows that a file has been deleted (i.e. the file was > removed from the file system and the staging area shows it as > deleted). > > The file exists in both the feature branch and the master branch. As > best as we can tell, the file is identical on both commits. > The issue occurs on multiple platforms - tested on Windows and OS X. > It only occurs for 1 specific file. > > Based on the activity of the file, nothing stands out as unusual. > > How we go about troubleshooting this and determining the cause? > > Thanks, > Paul van Wichen.
Git checkout issue - deleting file without apparent reason
Hi, We are having a strange issue that we haven't been able to pin down. Scenario: master branch and feature branch both have a specific file. 1. Master checked out. 2. git status show no changes / clean staging area. 3. Checkout feature branch . 4. git status show no changes / clean staging area. 5. Checkout master again. 6. git status shows that a file has been deleted (i.e. the file was removed from the file system and the staging area shows it as deleted). The file exists in both the feature branch and the master branch. As best as we can tell, the file is identical on both commits. The issue occurs on multiple platforms - tested on Windows and OS X. It only occurs for 1 specific file. Based on the activity of the file, nothing stands out as unusual. How we go about troubleshooting this and determining the cause? Thanks, Paul van Wichen.
Re: [PATCH v3 0/6] rebase -i: add config to abbreviate command-names
Hi Johannes, On Tue, 2017-05-02 at 17:48 +0200, Johannes Schindelin wrote: > Hi Liam, > > On Tue, 2 May 2017, Liam Beguin wrote: > > > Add the 'rebase.abbreviateCommands' configuration option to allow `git > > rebase -i` to default to the single-letter command-names in the todo > > list. > > > > Using single-letter command-names can present two benefits. First, it > > makes it easier to change the action since you only need to replace a > > single character (i.e.: in vim "r" instead of > > "ciw"). Second, using this with a large enough value of > > 'core.abbrev' enables the lines of the todo list to remain aligned > > making the files easier to read. > > > > Changes from v1 to v2: > > - Improve Documentation and commit message > > > > Changes from v2 to v3: > > - Transform a single patch into a series > > - change option name from 'rebase.abbrevCmd' to 'rebase.abbreviateCommands' > > - abbreviate all commands (not just pick) > > - teach `git rebase -i --autosquash` to recognise single-letter > > command-names > > - move rebase configuration documentation to > > Documentation/rebase-config.txt > > - update Documentation to use the preferred naming for the todo list > > - update Documentation and commit messages according to feedback > > Thank you for this pleasant read. It is an excellent contribution, and the > way you communicate what you changed and why is very welcome. > Thanks! and thank you for the support and help. > I offered a couple of comments, my biggest one probably being that this > patch series is crossing paths with my patch series that tries to move > more functionality out of the git-rebase--interactive.sh script into the > git-rebase--helper that is written in C, closely followed by my suggestion > to fold at least part of the functionality into the SHA-1 > collapsing/expanding. > I've seen a few messages about this migration, but I'm not yet sure I understand the difference between the shell and the C implementations. Is the C version going to replace 'git-rebase--interactive.sh'? > If your patch series "wins", I can easily forward-port your changes to the > rebase-i-extra branch, but it may actually make sense to build on top of > the rebase-i-extra branch to begin with. If you agree: I pushed the > proposed change to the `rebase-i-extra+abbrev` branch at > https://github.com/dscho/git. > If 'git-rebase--interactive.sh' is bound to be replaced, I could just shrink this to the Documentation cleanup (patches 4 and 5) and rework the rest on top of your new implementation. > I look forward to see this story unfold! > > Ciao, > Johannes Thanks, Liam
Re: [PATCH v3 3/6] rebase -i: add short command-name in --autosquash
Hi Johannes, On Tue, 2017-05-02 at 17:34 +0200, Johannes Schindelin wrote: > Hi Liam, > > On Tue, 2 May 2017, Liam Beguin wrote: > > > teach `git rebase -i` to recognise short command-names when using the > > '--autosquash' option. This allows commit with titles beginning with > > "s! ..." and "f! ..." to be treated the same way as "squash! ..." and > > "fixup! ..." respectively. > > As the recommended way to generate those commits is by using the > --fixup/--squash options of git-commit, and as there is *a much higher* > chance of false positives when using a very short tell-tale such as `f!` > (which could be an abbreviation for an expletive, likewise `s!`), I do not > think we will want this change. > > Let's keep handling just fixup!/squash! > > Ciao, > Johannes I was not quite sure about this change. My guess was that since --autosquash needs the whole commit title to find a match, the short version had little probability of generating a false positive. I thought it made sense to include the change in this series, but I understand why it's probably not a good idea to take it. I'll remove it in the next series. Thanks, Liam
[PATCH 18/24] cache.h: drop add_to_cache
Signed-off-by: Stefan Beller --- builtin/commit.c | 2 +- cache.h | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/builtin/commit.c b/builtin/commit.c index fa962c4f86..ac4fe97d36 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -287,7 +287,7 @@ static void add_remove_files(struct string_list *list) continue; if (!lstat(p->string, &st)) { - if (add_to_cache(p->string, &st, 0)) + if (add_to_index(&the_index, p->string, &st, 0)) die(_("updating files failed")); } else remove_file_from_index(&the_index, p->string); diff --git a/cache.h b/cache.h index 88c6f8a7df..bc49defc27 100644 --- a/cache.h +++ b/cache.h @@ -354,7 +354,6 @@ extern void free_name_hash(struct index_state *istate); #ifndef NO_THE_INDEX_COMPATIBILITY_MACROS -#define add_to_cache(path, st, flags) add_to_index(&the_index, (path), (st), (flags)) #define add_file_to_cache(path, flags) add_file_to_index(&the_index, (path), (flags)) #define chmod_cache_entry(ce, flip) chmod_index_entry(&the_index, (ce), (flip)) #define refresh_cache(flags) refresh_index(&the_index, (flags), NULL, NULL, NULL) -- 2.13.0.rc1.39.ga6db8bfa24
[PATCH 16/24] cache.h: drop rename_cache_entry_at
One define was unused, the other occurs just once. Easy patch. Signed-off-by: Stefan Beller --- builtin/mv.c | 2 +- cache.h | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/builtin/mv.c b/builtin/mv.c index 40e40bb098..1a3c61994c 100644 --- a/builtin/mv.c +++ b/builtin/mv.c @@ -287,7 +287,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix) pos = index_name_pos(&the_index, src, strlen(src)); assert(pos >= 0); if (!show_only) - rename_cache_entry_at(pos, dst); + rename_index_entry_at(&the_index, pos, dst); } if (gitmodules_modified) diff --git a/cache.h b/cache.h index 54c1bbf0d1..4c12ff265e 100644 --- a/cache.h +++ b/cache.h @@ -354,8 +354,6 @@ extern void free_name_hash(struct index_state *istate); #ifndef NO_THE_INDEX_COMPATIBILITY_MACROS -#define rename_cache_entry_at(pos, new_name) rename_index_entry_at(&the_index, (pos), (new_name)) -#define remove_cache_entry_at(pos) remove_index_entry_at(&the_index, (pos)) #define remove_file_from_cache(path) remove_file_from_index(&the_index, (path)) #define add_to_cache(path, st, flags) add_to_index(&the_index, (path), (st), (flags)) #define add_file_to_cache(path, flags) add_file_to_index(&the_index, (path), (flags)) -- 2.13.0.rc1.39.ga6db8bfa24
[PATCH 17/24] cache.h: drop remove_file_from_cache
coccinelle patch: @@ expression path; @@ -remove_file_from_cache(path) +remove_file_from_index(&the_index, path) Additionally drop the define from cache.h manually. Signed-off-by: Stefan Beller --- apply.c| 4 ++-- builtin/commit.c | 2 +- builtin/reset.c| 2 +- builtin/rm.c | 2 +- builtin/update-index.c | 10 +- cache.h| 1 - merge-recursive.c | 16 7 files changed, 18 insertions(+), 19 deletions(-) diff --git a/apply.c b/apply.c index 66d4969e72..8a61f19d03 100644 --- a/apply.c +++ b/apply.c @@ -4230,7 +4230,7 @@ static int build_fake_ancestor(struct apply_state *state, struct patch *list) static int remove_file(struct apply_state *state, struct patch *patch, int rmdir_empty) { if (state->update_index) { -if (remove_file_from_cache(patch->old_name) < 0) +if (remove_file_from_index(&the_index, patch->old_name) < 0) return error(_("unable to remove %s from index"), patch->old_name); } if (!state->cached) { @@ -4418,7 +4418,7 @@ static int add_conflicted_stages_file(struct apply_state *state, ce_size = cache_entry_size(namelen); mode = patch->new_mode ? patch->new_mode : (S_IFREG | 0644); - remove_file_from_cache(patch->new_name); + remove_file_from_index(&the_index, patch->new_name); for (stage = 1; stage < 4; stage++) { if (is_null_oid(&patch->threeway_stage[stage - 1])) continue; diff --git a/builtin/commit.c b/builtin/commit.c index ef12ea3991..fa962c4f86 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -290,7 +290,7 @@ static void add_remove_files(struct string_list *list) if (add_to_cache(p->string, &st, 0)) die(_("updating files failed")); } else - remove_file_from_cache(p->string); + remove_file_from_index(&the_index, p->string); } } diff --git a/builtin/reset.c b/builtin/reset.c index 0e19d6e8d5..90c56b46f2 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -125,7 +125,7 @@ static void update_index_from_diff(struct diff_queue_struct *q, struct cache_entry *ce; if (is_missing && !intent_to_add) { - remove_file_from_cache(one->path); + remove_file_from_index(&the_index, one->path); continue; } diff --git a/builtin/rm.c b/builtin/rm.c index c77c941ef0..f479100298 100644 --- a/builtin/rm.c +++ b/builtin/rm.c @@ -343,7 +343,7 @@ int cmd_rm(int argc, const char **argv, const char *prefix) if (!quiet) printf("rm '%s'\n", path); - if (remove_file_from_cache(path)) + if (remove_file_from_index(&the_index, path)) die(_("git rm: unable to remove %s"), path); } diff --git a/builtin/update-index.c b/builtin/update-index.c index e0738f74bf..8c4911e920 100644 --- a/builtin/update-index.c +++ b/builtin/update-index.c @@ -243,7 +243,7 @@ static int remove_one_path(const char *path) { if (!allow_remove) return error("%s: does not exist and --remove not passed", path); - if (remove_file_from_cache(path)) + if (remove_file_from_index(&the_index, path)) return error("%s: cannot remove from the index", path); return 0; } @@ -376,7 +376,7 @@ static int process_path(const char *path) * so updating it does not make sense. * On the other hand, removing it from index should work */ - if (allow_remove && remove_file_from_cache(path)) + if (allow_remove && remove_file_from_index(&the_index, path)) return error("%s: cannot remove from the index", path); return 0; } @@ -459,7 +459,7 @@ static void update_one(const char *path) } if (force_remove) { - if (remove_file_from_cache(path)) + if (remove_file_from_index(&the_index, path)) die("git update-index: unable to remove %s", path); report("remove '%s'", path); return; @@ -541,7 +541,7 @@ static void read_index_info(int nul_term_line) if (!mode) { /* mode == 0 means there is no such path -- remove */ - if (remove_file_from_cache(path_name)) + if (remove_file_from_index(&the_index, path_name)) die("git update-index: unable to remove %s", ptr); } @@ -658,7 +658,7 @@ static int unresolve_one(const char *path) goto free_return; } - remove_file_from_cache(path); + remove_file_fr
[PATCH 14/24] cache.h: drop cache_name_pos
coccinelle patch: @@ expression name, namelen; @@ -cache_name_pos(name, namelen) +index_name_pos(&the_index, name, namelen) Additionally manual editing: * drop the define from cache.h. Signed-off-by: Stefan Beller --- apply.c | 9 + builtin/blame.c | 4 ++-- builtin/checkout-index.c | 2 +- builtin/checkout.c | 2 +- builtin/ls-files.c | 7 --- builtin/merge-index.c| 2 +- builtin/mv.c | 8 builtin/rm.c | 4 ++-- builtin/update-index.c | 10 +- cache.h | 1 - diff.c | 2 +- dir.c| 8 merge-recursive.c| 4 ++-- pathspec.c | 2 +- rerere.c | 2 +- sha1_name.c | 6 +++--- submodule.c | 2 +- wt-status.c | 4 ++-- 18 files changed, 40 insertions(+), 39 deletions(-) diff --git a/apply.c b/apply.c index eb8eaeabec..bb1cd77c66 100644 --- a/apply.c +++ b/apply.c @@ -3496,7 +3496,7 @@ static int load_current(struct apply_state *state, if (!patch->is_new) die("BUG: patch to %s is not a creation", patch->old_name); - pos = cache_name_pos(name, strlen(name)); + pos = index_name_pos(&the_index, name, strlen(name)); if (pos < 0) return error(_("%s: does not exist in index"), name); ce = the_index.cache[pos]; @@ -3665,7 +3665,8 @@ static int check_preimage(struct apply_state *state, } if (state->check_index && !previous) { - int pos = cache_name_pos(old_name, strlen(old_name)); + int pos = index_name_pos(&the_index, old_name, +strlen(old_name)); if (pos < 0) { if (patch->is_new < 0) goto is_new; @@ -3721,7 +3722,7 @@ static int check_to_create(struct apply_state *state, struct stat nst; if (state->check_index && - cache_name_pos(new_name, strlen(new_name)) >= 0 && + index_name_pos(&the_index, new_name, strlen(new_name)) >= 0 && !ok_if_exists) return EXISTS_IN_INDEX; if (state->cached) @@ -3998,7 +3999,7 @@ static int get_current_oid(struct apply_state *state, const char *path, if (read_apply_cache(state) < 0) return -1; - pos = cache_name_pos(path, strlen(path)); + pos = index_name_pos(&the_index, path, strlen(path)); if (pos < 0) return -1; oidcpy(oid, &the_index.cache[pos]->oid); diff --git a/builtin/blame.c b/builtin/blame.c index b47aae25d4..c71d9a3340 100644 --- a/builtin/blame.c +++ b/builtin/blame.c @@ -2239,7 +2239,7 @@ static void verify_working_tree_path(struct commit *work_tree, const char *path) return; } - pos = cache_name_pos(path, strlen(path)); + pos = index_name_pos(&the_index, path, strlen(path)); if (pos >= 0) ; /* path is in the index */ else if (-1 - pos < the_index.cache_nr && @@ -2399,7 +2399,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt, len = strlen(path); if (!mode) { - int pos = cache_name_pos(path, len); + int pos = index_name_pos(&the_index, path, len); if (0 <= pos) mode = the_index.cache[pos]->ce_mode; else diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c index 1c3dcc1a8b..e8fc24b2ce 100644 --- a/builtin/checkout-index.c +++ b/builtin/checkout-index.c @@ -46,7 +46,7 @@ static void write_tempfile_record(const char *name, const char *prefix) static int checkout_file(const char *name, const char *prefix) { int namelen = strlen(name); - int pos = cache_name_pos(name, namelen); + int pos = index_name_pos(&the_index, name, namelen); int has_same_name = 0; int did_checkout = 0; int errs = 0; diff --git a/builtin/checkout.c b/builtin/checkout.c index a6cd8c0f37..039d3d296b 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -107,7 +107,7 @@ static int update_some(const unsigned char *sha1, struct strbuf *base, * entry in place. Whether it is UPTODATE or not, checkout_entry will * do the right thing. */ - pos = cache_name_pos(ce->name, ce->ce_namelen); + pos = index_name_pos(&the_index, ce->name, ce->ce_namelen); if (pos >= 0) { struct cache_entry *old = the_index.cache[pos]; if (ce->ce_mode == old->ce_mode && diff --git a/builtin/ls-files.c b/builtin/ls-files.c index 6f7ecec1b0..3507490d3e 100644 --- a/builtin/ls-files.c +++ b/builtin/ls-files.c @@ -134,7 +134,8 @@ static void show_killed_files(struct dir_struct *dir) /* If ent->name is prefix of an entry in the
[PATCH 19/24] cache.h: drop add_file_to_cache
Signed-off-by: Stefan Beller --- cache.h | 1 - rerere.c| 2 +- submodule.c | 2 +- 3 files changed, 2 insertions(+), 3 deletions(-) diff --git a/cache.h b/cache.h index bc49defc27..a9b059913e 100644 --- a/cache.h +++ b/cache.h @@ -354,7 +354,6 @@ extern void free_name_hash(struct index_state *istate); #ifndef NO_THE_INDEX_COMPATIBILITY_MACROS -#define add_file_to_cache(path, flags) add_file_to_index(&the_index, (path), (flags)) #define chmod_cache_entry(ce, flip) chmod_index_entry(&the_index, (ce), (flip)) #define refresh_cache(flags) refresh_index(&the_index, (flags), NULL, NULL, NULL) #define ce_match_stat(ce, st, options) ie_match_stat(&the_index, (ce), (st), (options)) diff --git a/rerere.c b/rerere.c index b6d84b8461..0ada0ef247 100644 --- a/rerere.c +++ b/rerere.c @@ -712,7 +712,7 @@ static void update_paths(struct string_list *update) for (i = 0; i < update->nr; i++) { struct string_list_item *item = &update->items[i]; - if (add_file_to_cache(item->string, 0)) + if (add_file_to_index(&the_index, item->string, 0)) exit(128); fprintf(stderr, "Staged '%s' using previous resolution.\n", item->string); diff --git a/submodule.c b/submodule.c index 148194831d..6587bc0d84 100644 --- a/submodule.c +++ b/submodule.c @@ -119,7 +119,7 @@ int remove_path_from_gitmodules(const char *path) void stage_updated_gitmodules(void) { - if (add_file_to_cache(".gitmodules", 0)) + if (add_file_to_index(&the_index, ".gitmodules", 0)) die(_("staging updated .gitmodules failed")); } -- 2.13.0.rc1.39.ga6db8bfa24
[PATCH 21/24] cache.h: drop refresh_cache
coccinelle patch: @@ expression flags; @@ -refresh_cache(flags) +refresh_index(&the_index, flags, NULL, NULL, NULL) Additionally drop the define from cache.h manually. This is a commit where I think the macro expansion is maybe silly, but for reasons outlined in the first patch of the series, we still follow through. Signed-off-by: Stefan Beller --- builtin/am.c | 4 ++-- builtin/checkout.c | 2 +- builtin/commit.c | 6 +++--- builtin/diff.c | 3 ++- builtin/merge.c| 8 builtin/update-index.c | 3 ++- cache.h| 1 - merge.c| 2 +- wt-status.c| 2 +- 9 files changed, 16 insertions(+), 15 deletions(-) diff --git a/builtin/am.c b/builtin/am.c index 38fc4566f7..a7388ec4e0 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -1122,7 +1122,7 @@ static void refresh_and_write_cache(void) struct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file)); hold_locked_index(lock_file, LOCK_DIE_ON_ERROR); - refresh_cache(REFRESH_QUIET); + refresh_index(&the_index, REFRESH_QUIET, NULL, NULL, NULL); if (write_locked_index(&the_index, lock_file, COMMIT_LOCK)) die(_("unable to write index file")); } @@ -1977,7 +1977,7 @@ static int fast_forward_to(struct tree *head, struct tree *remote, int reset) lock_file = xcalloc(1, sizeof(struct lock_file)); hold_locked_index(lock_file, LOCK_DIE_ON_ERROR); - refresh_cache(REFRESH_QUIET); + refresh_index(&the_index, REFRESH_QUIET, NULL, NULL, NULL); memset(&opts, 0, sizeof(opts)); opts.head_idx = 1; diff --git a/builtin/checkout.c b/builtin/checkout.c index 34ec9f7e32..49a8b3a7ff 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -509,7 +509,7 @@ static int merge_working_tree(const struct checkout_opts *opts, setup_unpack_trees_porcelain(&topts, "checkout"); - refresh_cache(REFRESH_QUIET); + refresh_index(&the_index, REFRESH_QUIET, NULL, NULL, NULL); if (unmerged_index(&the_index)) { error(_("you need to resolve your current index first")); diff --git a/builtin/commit.c b/builtin/commit.c index ac4fe97d36..4b95ff181b 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -328,7 +328,7 @@ static void refresh_cache_or_die(int refresh_flags) * refresh_flags contains REFRESH_QUIET, so the only errors * are for unmerged entries. */ - if (refresh_cache(refresh_flags | REFRESH_IN_PORCELAIN)) + if (refresh_index(&the_index, refresh_flags | REFRESH_IN_PORCELAIN, NULL, NULL, NULL)) die_resolve_conflict("commit"); } @@ -470,7 +470,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix hold_locked_index(&index_lock, LOCK_DIE_ON_ERROR); add_remove_files(&partial); - refresh_cache(REFRESH_QUIET); + refresh_index(&the_index, REFRESH_QUIET, NULL, NULL, NULL); update_main_cache_tree(WRITE_TREE_SILENT); if (write_locked_index(&the_index, &index_lock, CLOSE_LOCK)) die(_("unable to write new_index file")); @@ -482,7 +482,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix create_base_index(current_head); add_remove_files(&partial); - refresh_cache(REFRESH_QUIET); + refresh_index(&the_index, REFRESH_QUIET, NULL, NULL, NULL); if (write_locked_index(&the_index, &false_lock, CLOSE_LOCK)) die(_("unable to write temporary index file")); diff --git a/builtin/diff.c b/builtin/diff.c index 1efd0d6b61..b11b94b214 100644 --- a/builtin/diff.c +++ b/builtin/diff.c @@ -211,7 +211,8 @@ static void refresh_index_quietly(void) return; discard_index(&the_index); read_index(&the_index); - refresh_cache(REFRESH_QUIET|REFRESH_UNMERGED); + refresh_index(&the_index, REFRESH_QUIET | REFRESH_UNMERGED, NULL, + NULL, NULL); update_index_if_able(&the_index, lock_file); } diff --git a/builtin/merge.c b/builtin/merge.c index b023107d26..4e219cb9e8 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -324,7 +324,7 @@ static void restore_state(const struct object_id *head, run_command_v_opt(args, RUN_GIT_CMD); strbuf_release(&sb); - refresh_cache(REFRESH_QUIET); + refresh_index(&the_index, REFRESH_QUIET, NULL, NULL, NULL); } /* This is called when no merge was necessary. */ @@ -639,7 +639,7 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common, const char *head_arg = "HEAD"; hold_locked_index(&lock, LOCK_DIE_ON_ERROR); - refresh_cache(REFRESH_QUIET); + refresh_index(&the_index, REFRESH_QUIET, NULL, NULL, NULL); if (the_index.cache_changed && write_locked_index(&the_index, &lock, COMMIT_LOCK)) return error
[PATCH 15/24] cache.h: drop add_cache_entry
coccinelle patch -add_cache_entry(ce, option) +add_index_entry(&the_index, ce, option) Additionally drop the define from cache.h manually. Signed-off-by: Stefan Beller --- apply.c| 4 ++-- builtin/blame.c| 3 ++- builtin/checkout.c | 3 ++- builtin/reset.c| 3 ++- builtin/update-index.c | 8 cache.h| 1 - merge-recursive.c | 4 ++-- tree.c | 2 +- 8 files changed, 15 insertions(+), 13 deletions(-) diff --git a/apply.c b/apply.c index bb1cd77c66..66d4969e72 100644 --- a/apply.c +++ b/apply.c @@ -4284,7 +4284,7 @@ static int build_fake_ancestor(struct apply_state *state, struct patch *list) "for newly created file %s"), path); } } - if (add_cache_entry(ce, ADD_CACHE_OK_TO_ADD) < 0) { + if (add_index_entry(&the_index, ce, ADD_CACHE_OK_TO_ADD) < 0) { free(ce); return error(_("unable to add cache entry for %s"), path); } @@ -4428,7 +4428,7 @@ static int add_conflicted_stages_file(struct apply_state *state, ce->ce_flags = create_ce_flags(stage); ce->ce_namelen = namelen; oidcpy(&ce->oid, &patch->threeway_stage[stage - 1]); - if (add_cache_entry(ce, ADD_CACHE_OK_TO_ADD) < 0) { + if (add_index_entry(&the_index, ce, ADD_CACHE_OK_TO_ADD) < 0) { free(ce); return error(_("unable to add cache entry for %s"), patch->new_name); diff --git a/builtin/blame.c b/builtin/blame.c index c71d9a3340..5140015cc0 100644 --- a/builtin/blame.c +++ b/builtin/blame.c @@ -2413,7 +2413,8 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt, ce->ce_flags = create_ce_flags(0); ce->ce_namelen = len; ce->ce_mode = create_ce_mode(mode); - add_cache_entry(ce, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE); + add_index_entry(&the_index, ce, + ADD_CACHE_OK_TO_ADD | ADD_CACHE_OK_TO_REPLACE); cache_tree_invalidate_path(&the_index, path); diff --git a/builtin/checkout.c b/builtin/checkout.c index 039d3d296b..34ec9f7e32 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -118,7 +118,8 @@ static int update_some(const unsigned char *sha1, struct strbuf *base, } } - add_cache_entry(ce, ADD_CACHE_OK_TO_ADD | ADD_CACHE_OK_TO_REPLACE); + add_index_entry(&the_index, ce, + ADD_CACHE_OK_TO_ADD | ADD_CACHE_OK_TO_REPLACE); return 0; } diff --git a/builtin/reset.c b/builtin/reset.c index f8073b1caa..0e19d6e8d5 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -138,7 +138,8 @@ static void update_index_from_diff(struct diff_queue_struct *q, ce->ce_flags |= CE_INTENT_TO_ADD; set_object_name_for_intent_to_add_entry(ce); } - add_cache_entry(ce, ADD_CACHE_OK_TO_ADD | ADD_CACHE_OK_TO_REPLACE); + add_index_entry(&the_index, ce, + ADD_CACHE_OK_TO_ADD | ADD_CACHE_OK_TO_REPLACE); } } diff --git a/builtin/update-index.c b/builtin/update-index.c index d7a117c674..e0738f74bf 100644 --- a/builtin/update-index.c +++ b/builtin/update-index.c @@ -286,7 +286,7 @@ static int add_one_path(const struct cache_entry *old, const char *path, int len } option = allow_add ? ADD_CACHE_OK_TO_ADD : 0; option |= allow_replace ? ADD_CACHE_OK_TO_REPLACE : 0; - if (add_cache_entry(ce, option)) + if (add_index_entry(&the_index, ce, option)) return error("%s: cannot add to the index - missing --add option?", path); return 0; } @@ -416,7 +416,7 @@ static int add_cacheinfo(unsigned int mode, const struct object_id *oid, ce->ce_flags |= CE_VALID; option = allow_add ? ADD_CACHE_OK_TO_ADD : 0; option |= allow_replace ? ADD_CACHE_OK_TO_REPLACE : 0; - if (add_cache_entry(ce, option)) + if (add_index_entry(&the_index, ce, option)) return error("%s: cannot add to the index - missing --add option?", path); report("add '%s'", path); @@ -659,12 +659,12 @@ static int unresolve_one(const char *path) } remove_file_from_cache(path); - if (add_cache_entry(ce_2, ADD_CACHE_OK_TO_ADD)) { + if (add_index_entry(&the_index, ce_2, ADD_CACHE_OK_TO_ADD)) { error("%s: cannot add our version to the index.", path); ret = -1; goto free_return; } - if (!add_cache_entry(ce_3, ADD_CACHE_OK_TO_ADD)) + if (!add_index_entry(&the_index, ce_3, ADD_CACHE_OK_TO_ADD)) return 0; error("%s: cannot add their version to the index.", path); ret = -1; diff --git a/cache.h b/cache.h index a18ebf2
[PATCH 23/24] cache.h: drop ce_match_stat
coccinelle patch: @@ expression ce, st, options; @@ -ce_match_stat(ce, st, options) +ie_match_stat(&the_index, ce, st, options) Additionally drop the define from cache.h manually. Note that there is an empty define section in cache.h now. The cleanup of that is done in a later patch. Signed-off-by: Stefan Beller --- apply.c| 3 ++- builtin/rm.c | 2 +- builtin/update-index.c | 2 +- cache.h| 1 - check-racy.c | 4 ++-- diff-lib.c | 2 +- diff.c | 2 +- entry.c| 3 ++- submodule.c| 2 +- 9 files changed, 11 insertions(+), 10 deletions(-) diff --git a/apply.c b/apply.c index 8a61f19d03..46bc5a20b9 100644 --- a/apply.c +++ b/apply.c @@ -3364,7 +3364,8 @@ static int verify_index_match(const struct cache_entry *ce, struct stat *st) return -1; return 0; } - return ce_match_stat(ce, st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE); + return ie_match_stat(&the_index, ce, st, +CE_MATCH_IGNORE_VALID | CE_MATCH_IGNORE_SKIP_WORKTREE); } #define SUBMODULE_PATCH_WITHOUT_INDEX 1 diff --git a/builtin/rm.c b/builtin/rm.c index f479100298..51b64f2bae 100644 --- a/builtin/rm.c +++ b/builtin/rm.c @@ -163,7 +163,7 @@ static int check_local_mod(struct object_id *head, int index_only) * Is the index different from the file in the work tree? * If it's a submodule, is its work tree modified? */ - if (ce_match_stat(ce, &st, 0) || + if (ie_match_stat(&the_index, ce, &st, 0) || (S_ISGITLINK(ce->ce_mode) && bad_to_remove_submodule(ce->name, SUBMODULE_REMOVAL_DIE_ON_ERROR | diff --git a/builtin/update-index.c b/builtin/update-index.c index 9cbd346f95..042f4c94cf 100644 --- a/builtin/update-index.c +++ b/builtin/update-index.c @@ -268,7 +268,7 @@ static int add_one_path(const struct cache_entry *old, const char *path, int len struct cache_entry *ce; /* Was the old index entry already up-to-date? */ - if (old && !ce_stage(old) && !ce_match_stat(old, st, 0)) + if (old && !ce_stage(old) && !ie_match_stat(&the_index, old, st, 0)) return 0; size = cache_entry_size(len); diff --git a/cache.h b/cache.h index c34fc4fd40..f2a45eda9a 100644 --- a/cache.h +++ b/cache.h @@ -354,7 +354,6 @@ extern void free_name_hash(struct index_state *istate); #ifndef NO_THE_INDEX_COMPATIBILITY_MACROS -#define ce_match_stat(ce, st, options) ie_match_stat(&the_index, (ce), (st), (options)) #endif enum object_type { diff --git a/check-racy.c b/check-racy.c index 6599ae84cf..485d7ab8f8 100644 --- a/check-racy.c +++ b/check-racy.c @@ -16,9 +16,9 @@ int main(int ac, char **av) continue; } - if (ce_match_stat(ce, &st, 0)) + if (ie_match_stat(&the_index, ce, &st, 0)) dirty++; - else if (ce_match_stat(ce, &st, CE_MATCH_RACY_IS_DIRTY)) + else if (ie_match_stat(&the_index, ce, &st, CE_MATCH_RACY_IS_DIRTY)) racy++; else clean++; diff --git a/diff-lib.c b/diff-lib.c index de59ec0459..4ca3ce9c90 100644 --- a/diff-lib.c +++ b/diff-lib.c @@ -69,7 +69,7 @@ static int match_stat_with_submodule(struct diff_options *diffopt, struct stat *st, unsigned ce_option, unsigned *dirty_submodule) { - int changed = ce_match_stat(ce, st, ce_option); + int changed = ie_match_stat(&the_index, ce, st, ce_option); if (S_ISGITLINK(ce->ce_mode)) { unsigned orig_flags = diffopt->flags; if (!DIFF_OPT_TST(diffopt, OVERRIDE_SUBMODULE_CONFIG)) diff --git a/diff.c b/diff.c index f2ee40fe21..bd1478f6c9 100644 --- a/diff.c +++ b/diff.c @@ -2782,7 +2782,7 @@ static int reuse_worktree_file(const char *name, const unsigned char *sha1, int * If ce matches the file in the work tree, we can reuse it. */ if (ce_uptodate(ce) || - (!lstat(name, &st) && !ce_match_stat(ce, &st, 0))) + (!lstat(name, &st) && !ie_match_stat(&the_index, ce, &st, 0))) return 1; return 0; diff --git a/entry.c b/entry.c index d2b512da90..d3a34c9cc4 100644 --- a/entry.c +++ b/entry.c @@ -266,7 +266,8 @@ int checkout_entry(struct cache_entry *ce, if (!check_path(path.buf, path.len, &st, state->base_dir_len)) { const struct submodule *sub; - unsigned changed = ce_match_stat(ce, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE); + unsigned changed = ie_match_stat(&the_index, ce, &st, +CE_MATCH_IGNORE_VALID | CE_MATCH_IGNORE_SK
[PATCH 24/24] cache.h: retire NO_THE_INDEX_COMPATIBILITY_MACROS
The NO_THE_INDEX_COMPATIBILITY_MACROS pre processor setting was introduced in 4aab5b46f4 (Make read-cache.c "the_index" free., 2007-04-01), stating: This makes all low-level functions defined in read-cache.c to take an explicit index_state structure as their first parameter, to specify which index to work on. The reasoning is very vague, maybe indicating that having the index specified to work on is easier to for the new reader to understand what is going on. All preceding patches worked on retiring functions that do not take an explicit index as to where to perform its work. Spelling out the reasons why we want to specify the index at each call: 1) Ease of understanding: The recent patches dropped a lot of macros that may confuse new people diving into the code base. 2a) Spelling out global state explicitly: Anything you do in submodule land today needs to spawn new processes in the submodule. This is cumbersome and not performant. So we may want to have an abstraction of a repo, i.e. all repository state in one struct. That way we can open a submodule in-process and perform the required actions without spawning a process. The road to this future is a long road, and we have to start somewhere. Exposing the global state seems like a good starter point. 2b) Spelling out global state explicitly (II): (Jeff Hostetler wrote) In addition to (eventually) allowing multiple repos be open at the same time for submodules, it would also help with various multi-threading efforts. For example, we have loops that do a "for (k = 0, k < active_nr; k++) {...}" There is no visual clue in that code that it references "the_index" and therefore should be subject to the same locking. Granted, this is a trivial example, but goes to the argument that the code has lots of subtle global variables and macros that make it difficult to reason about the code. This step would help un-hide this. Signed-off-by: Stefan Beller --- attr.c | 1 - cache.h| 4 name-hash.c| 1 - read-cache.c | 1 - unpack-trees.c | 1 - 5 files changed, 8 deletions(-) diff --git a/attr.c b/attr.c index 7e2134471c..3decf432b8 100644 --- a/attr.c +++ b/attr.c @@ -7,7 +7,6 @@ * an insanely large number of attributes. */ -#define NO_THE_INDEX_COMPATIBILITY_MACROS #include "cache.h" #include "exec_cmd.h" #include "attr.h" diff --git a/cache.h b/cache.h index f2a45eda9a..f4b7b8621c 100644 --- a/cache.h +++ b/cache.h @@ -352,10 +352,6 @@ extern void add_name_hash(struct index_state *istate, struct cache_entry *ce); extern void remove_name_hash(struct index_state *istate, struct cache_entry *ce); extern void free_name_hash(struct index_state *istate); - -#ifndef NO_THE_INDEX_COMPATIBILITY_MACROS -#endif - enum object_type { OBJ_BAD = -1, OBJ_NONE = 0, diff --git a/name-hash.c b/name-hash.c index 39309efb7f..02870c1d19 100644 --- a/name-hash.c +++ b/name-hash.c @@ -5,7 +5,6 @@ * * Copyright (C) 2008 Linus Torvalds */ -#define NO_THE_INDEX_COMPATIBILITY_MACROS #include "cache.h" struct dir_entry { diff --git a/read-cache.c b/read-cache.c index 6681376f5b..e0c6f3ca11 100644 --- a/read-cache.c +++ b/read-cache.c @@ -3,7 +3,6 @@ * * Copyright (C) Linus Torvalds, 2005 */ -#define NO_THE_INDEX_COMPATIBILITY_MACROS #include "cache.h" #include "tempfile.h" #include "lockfile.h" diff --git a/unpack-trees.c b/unpack-trees.c index 3dd8f60fc1..41956c4b37 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -1,4 +1,3 @@ -#define NO_THE_INDEX_COMPATIBILITY_MACROS #include "cache.h" #include "dir.h" #include "tree.h" -- 2.13.0.rc1.39.ga6db8bfa24
[PATCH 09/24] cache.h: drop resolve_undo_clear
Signed-off-by: Stefan Beller --- builtin/checkout.c | 2 +- builtin/merge.c| 2 +- builtin/read-tree.c| 2 +- builtin/update-index.c | 2 +- cache.h| 1 - merge.c| 2 +- 6 files changed, 5 insertions(+), 6 deletions(-) diff --git a/builtin/checkout.c b/builtin/checkout.c index 2328a475ea..abcc45a74f 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -491,7 +491,7 @@ static int merge_working_tree(const struct checkout_opts *opts, if (read_index_preload(&the_index, NULL) < 0) return error(_("index file corrupt")); - resolve_undo_clear(); + resolve_undo_clear_index(&the_index); if (opts->force) { ret = reset_tree(new->commit->tree, opts, 1, writeout_error); if (ret) diff --git a/builtin/merge.c b/builtin/merge.c index c27c806ac1..b023107d26 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -1191,7 +1191,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix) else die(_("You have not concluded your cherry-pick (CHERRY_PICK_HEAD exists).")); } - resolve_undo_clear(); + resolve_undo_clear_index(&the_index); if (verbosity < 0) show_diffstat = 0; diff --git a/builtin/read-tree.c b/builtin/read-tree.c index 0bcf021ead..61f5f6f028 100644 --- a/builtin/read-tree.c +++ b/builtin/read-tree.c @@ -199,7 +199,7 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix) die("You need to resolve your current index first"); stage = opts.merge = 1; } - resolve_undo_clear(); + resolve_undo_clear_index(&the_index); for (i = 0; i < argc; i++) { const char *arg = argv[i]; diff --git a/builtin/update-index.c b/builtin/update-index.c index b8458016f0..c9f06169c0 100644 --- a/builtin/update-index.c +++ b/builtin/update-index.c @@ -796,7 +796,7 @@ static int chmod_callback(const struct option *opt, static int resolve_undo_clear_callback(const struct option *opt, const char *arg, int unset) { - resolve_undo_clear(); + resolve_undo_clear_index(&the_index); return 0; } diff --git a/cache.h b/cache.h index 8a2dc393dc..abf1474034 100644 --- a/cache.h +++ b/cache.h @@ -371,7 +371,6 @@ extern void free_name_hash(struct index_state *istate); #define cache_dir_exists(name, namelen) index_dir_exists(&the_index, (name), (namelen)) #define cache_file_exists(name, namelen, igncase) index_file_exists(&the_index, (name), (namelen), (igncase)) #define cache_name_is_other(name, namelen) index_name_is_other(&the_index, (name), (namelen)) -#define resolve_undo_clear() resolve_undo_clear_index(&the_index) #endif enum object_type { diff --git a/merge.c b/merge.c index 748305031e..06509a6df2 100644 --- a/merge.c +++ b/merge.c @@ -39,7 +39,7 @@ int try_merge_command(const char *strategy, size_t xopts_nr, discard_cache(); if (read_index(&the_index) < 0) die(_("failed to read the cache")); - resolve_undo_clear(); + resolve_undo_clear_index(&the_index); return ret; } -- 2.13.0.rc1.39.ga6db8bfa24
[PATCH 08/24] cache.h: drop unmerge_cache[_entry_at]
Signed-off-by: Stefan Beller --- builtin/update-index.c | 2 +- cache.h| 2 -- rerere.c | 2 +- 3 files changed, 2 insertions(+), 4 deletions(-) diff --git a/builtin/update-index.c b/builtin/update-index.c index 8667c48446..b8458016f0 100644 --- a/builtin/update-index.c +++ b/builtin/update-index.c @@ -613,7 +613,7 @@ static int unresolve_one(const char *path) pos = cache_name_pos(path, namelen); if (0 <= pos) { /* already merged */ - pos = unmerge_cache_entry_at(pos); + pos = unmerge_index_entry_at(&the_index, pos); if (pos < the_index.cache_nr) { const struct cache_entry *ce = the_index.cache[pos]; if (ce_stage(ce) && diff --git a/cache.h b/cache.h index d078e88c3f..8a2dc393dc 100644 --- a/cache.h +++ b/cache.h @@ -372,8 +372,6 @@ extern void free_name_hash(struct index_state *istate); #define cache_file_exists(name, namelen, igncase) index_file_exists(&the_index, (name), (namelen), (igncase)) #define cache_name_is_other(name, namelen) index_name_is_other(&the_index, (name), (namelen)) #define resolve_undo_clear() resolve_undo_clear_index(&the_index) -#define unmerge_cache_entry_at(at) unmerge_index_entry_at(&the_index, at) -#define unmerge_cache(pathspec) unmerge_index(&the_index, pathspec) #endif enum object_type { diff --git a/rerere.c b/rerere.c index b9b39a959e..03218166ab 100644 --- a/rerere.c +++ b/rerere.c @@ -1116,7 +1116,7 @@ int rerere_forget(struct pathspec *pathspec) * recover the original conflicted state and then * find the conflicted paths. */ - unmerge_cache(pathspec); + unmerge_index(&the_index, pathspec); find_conflict(&conflict); for (i = 0; i < conflict.nr; i++) { struct string_list_item *it = &conflict.items[i]; -- 2.13.0.rc1.39.ga6db8bfa24
[PATCH 20/24] cache.h: drop chmod_cache_entry
Signed-off-by: Stefan Beller --- builtin/add.c | 2 +- builtin/update-index.c | 2 +- cache.h| 1 - 3 files changed, 2 insertions(+), 3 deletions(-) diff --git a/builtin/add.c b/builtin/add.c index f6d71b10d0..288b1f5bb3 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -40,7 +40,7 @@ static void chmod_pathspec(struct pathspec *pathspec, int force_mode) if (pathspec && !ce_path_match(ce, pathspec, NULL)) continue; - if (chmod_cache_entry(ce, force_mode) < 0) + if (chmod_index_entry(&the_index, ce, force_mode) < 0) fprintf(stderr, "cannot chmod '%s'", ce->name); } } diff --git a/builtin/update-index.c b/builtin/update-index.c index 8c4911e920..f1c52a5531 100644 --- a/builtin/update-index.c +++ b/builtin/update-index.c @@ -432,7 +432,7 @@ static void chmod_path(char flip, const char *path) if (pos < 0) goto fail; ce = the_index.cache[pos]; - if (chmod_cache_entry(ce, flip) < 0) + if (chmod_index_entry(&the_index, ce, flip) < 0) goto fail; report("chmod %cx '%s'", flip, path); diff --git a/cache.h b/cache.h index a9b059913e..07ad23d912 100644 --- a/cache.h +++ b/cache.h @@ -354,7 +354,6 @@ extern void free_name_hash(struct index_state *istate); #ifndef NO_THE_INDEX_COMPATIBILITY_MACROS -#define chmod_cache_entry(ce, flip) chmod_index_entry(&the_index, (ce), (flip)) #define refresh_cache(flags) refresh_index(&the_index, (flags), NULL, NULL, NULL) #define ce_match_stat(ce, st, options) ie_match_stat(&the_index, (ce), (st), (options)) #define ce_modified(ce, st, options) ie_modified(&the_index, (ce), (st), (options)) -- 2.13.0.rc1.39.ga6db8bfa24
[PATCH 22/24] cache.h: drop ce_modified
Signed-off-by: Stefan Beller --- builtin/ls-files.c | 2 +- cache.h| 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/builtin/ls-files.c b/builtin/ls-files.c index 3507490d3e..89fac7ddf5 100644 --- a/builtin/ls-files.c +++ b/builtin/ls-files.c @@ -369,7 +369,7 @@ static void show_files(struct dir_struct *dir) err = lstat(ce->name, &st); if (show_deleted && err) show_ce_entry(tag_removed, ce); - if (show_modified && ce_modified(ce, &st, 0)) + if (show_modified && ie_modified(&the_index, ce, &st, 0)) show_ce_entry(tag_modified, ce); } } diff --git a/cache.h b/cache.h index 23c9c1697c..c34fc4fd40 100644 --- a/cache.h +++ b/cache.h @@ -355,7 +355,6 @@ extern void free_name_hash(struct index_state *istate); #ifndef NO_THE_INDEX_COMPATIBILITY_MACROS #define ce_match_stat(ce, st, options) ie_match_stat(&the_index, (ce), (st), (options)) -#define ce_modified(ce, st, options) ie_modified(&the_index, (ce), (st), (options)) #endif enum object_type { -- 2.13.0.rc1.39.ga6db8bfa24
[PATCH 13/24] cache.h: drop is_cache_unborn(), discard_cache(), unmerged_cache()
cocci semantic patch: @@ @@ -is_cache_unborn() +is_index_unborn(&the_index) @@ @@ -discard_cache() +discard_index(&the_index) @@ @@ -unmerged_cache() +unmerged_index(&the_index) Additionally the defines in cache.h were removed manually. Signed-off-by: Stefan Beller --- builtin/am.c| 8 builtin/blame.c | 2 +- builtin/checkout.c | 4 ++-- builtin/commit.c| 10 +- builtin/diff.c | 2 +- builtin/pull.c | 2 +- builtin/read-tree.c | 2 +- builtin/reset.c | 2 +- cache.h | 3 --- merge-recursive.c | 8 merge.c | 2 +- sequencer.c | 4 ++-- t/helper/test-lazy-init-name-hash.c | 10 +- t/helper/test-read-cache.c | 2 +- wt-status.c | 2 +- 15 files changed, 30 insertions(+), 33 deletions(-) diff --git a/builtin/am.c b/builtin/am.c index bb0927fbcc..38fc4566f7 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -1556,7 +1556,7 @@ static int run_apply(const struct am_state *state, const char *index_file) if (index_file) { /* Reload index as apply_all_patches() will have modified it. */ - discard_cache(); + discard_index(&the_index); read_index_from(&the_index, index_file); } @@ -1599,7 +1599,7 @@ static int fall_back_threeway(const struct am_state *state, const char *index_pa if (build_fake_ancestor(state, index_path)) return error("could not build fake ancestor"); - discard_cache(); + discard_index(&the_index); read_index_from(&the_index, index_path); if (write_index_as_tree(orig_tree.hash, &the_index, index_path, 0, NULL)) @@ -1632,7 +1632,7 @@ static int fall_back_threeway(const struct am_state *state, const char *index_pa say(state, stdout, _("Falling back to patching base and 3-way merge...")); - discard_cache(); + discard_index(&the_index); read_index(&the_index); /* @@ -1938,7 +1938,7 @@ static void am_resolve(struct am_state *state) die_user_resolve(state); } - if (unmerged_cache()) { + if (unmerged_index(&the_index)) { printf_ln(_("You still have unmerged paths in your index.\n" "Did you forget to use 'git add'?")); die_user_resolve(state); diff --git a/builtin/blame.c b/builtin/blame.c index cbb7c1fd9d..b47aae25d4 100644 --- a/builtin/blame.c +++ b/builtin/blame.c @@ -2394,7 +2394,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt, * bits; we are not going to write this index out -- we just * want to run "diff-index --cached". */ - discard_cache(); + discard_index(&the_index); read_index(&the_index); len = strlen(path); diff --git a/builtin/checkout.c b/builtin/checkout.c index abcc45a74f..a6cd8c0f37 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -510,13 +510,13 @@ static int merge_working_tree(const struct checkout_opts *opts, refresh_cache(REFRESH_QUIET); - if (unmerged_cache()) { + if (unmerged_index(&the_index)) { error(_("you need to resolve your current index first")); return 1; } /* 2-way merge to the new branch */ - topts.initial_checkout = is_cache_unborn(); + topts.initial_checkout = is_index_unborn(&the_index); topts.update = 1; topts.merge = 1; topts.gently = opts->merge && old->commit; diff --git a/builtin/commit.c b/builtin/commit.c index 687e7c8a3a..ef12ea3991 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -301,7 +301,7 @@ static void create_base_index(const struct commit *current_head) struct tree_desc t; if (!current_head) { - discard_cache(); + discard_index(&the_index); return; } @@ -369,7 +369,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix else unsetenv(INDEX_ENVIRONMENT); - discard_cache(); + discard_index(&the_index); read_index_from(&the_index, get_lock_file_path(&index_lock)); if (update_main_cache_tree(WRITE_TREE_SILENT) == 0) { if (reopen_lock_file(&index_lock) < 0) @@ -464,7 +464,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix if (list_paths(&partial, !current_head ? NULL : "HEAD", prefix, &pathspec)) exit(1); - discard_cache(); + discard_index(&the_index);
[PATCH 06/24] unpack-trees.c: rename parameter 'the_index'
As "the_index" is already a global variable, we do not want to confuse the local variable with the global variable. Signed-off-by: Stefan Beller --- unpack-trees.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/unpack-trees.c b/unpack-trees.c index aa15111fef..3dd8f60fc1 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -1199,7 +1199,7 @@ static int clear_ce_flags(struct cache_entry **cache, int nr, * Set/Clear CE_NEW_SKIP_WORKTREE according to $GIT_DIR/info/sparse-checkout */ static void mark_new_skip_worktree(struct exclude_list *el, - struct index_state *the_index, + struct index_state *index, int select_flag, int skip_wt_flag) { int i; @@ -1208,8 +1208,8 @@ static void mark_new_skip_worktree(struct exclude_list *el, * 1. Pretend the narrowest worktree: only unmerged entries * are checked out */ - for (i = 0; i < the_index->cache_nr; i++) { - struct cache_entry *ce = the_index->cache[i]; + for (i = 0; i < index->cache_nr; i++) { + struct cache_entry *ce = index->cache[i]; if (select_flag && !(ce->ce_flags & select_flag)) continue; @@ -1224,7 +1224,7 @@ static void mark_new_skip_worktree(struct exclude_list *el, * 2. Widen worktree according to sparse-checkout file. * Matched entries will have skip_wt_flag cleared (i.e. "in") */ - clear_ce_flags(the_index->cache, the_index->cache_nr, + clear_ce_flags(index->cache, index->cache_nr, select_flag, skip_wt_flag, el); } -- 2.13.0.rc1.39.ga6db8bfa24
[PATCH 12/24] cache.h: drop cache_dir_exists
Signed-off-by: Stefan Beller --- cache.h | 1 - dir.c | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/cache.h b/cache.h index 85a85f8b96..4b8e8c31fe 100644 --- a/cache.h +++ b/cache.h @@ -368,7 +368,6 @@ extern void free_name_hash(struct index_state *istate); #define refresh_cache(flags) refresh_index(&the_index, (flags), NULL, NULL, NULL) #define ce_match_stat(ce, st, options) ie_match_stat(&the_index, (ce), (st), (options)) #define ce_modified(ce, st, options) ie_modified(&the_index, (ce), (st), (options)) -#define cache_dir_exists(name, namelen) index_dir_exists(&the_index, (name), (namelen)) #endif enum object_type { diff --git a/dir.c b/dir.c index 0327832e53..63edaec2ef 100644 --- a/dir.c +++ b/dir.c @@ -1266,7 +1266,7 @@ static enum exist_status directory_exists_in_index_icase(const char *dirname, in { struct cache_entry *ce; - if (cache_dir_exists(dirname, len)) + if (index_dir_exists(&the_index, dirname, len)) return index_directory; ce = index_file_exists(&the_index, dirname, len, ignore_case); -- 2.13.0.rc1.39.ga6db8bfa24
[PATCH 11/24] cache.h: drop cache_file_exists
Signed-off-by: Stefan Beller --- apply.c | 2 +- cache.h | 1 - dir.c | 9 + merge-recursive.c | 3 ++- 4 files changed, 8 insertions(+), 7 deletions(-) diff --git a/apply.c b/apply.c index 159e039a18..eb8eaeabec 100644 --- a/apply.c +++ b/apply.c @@ -3810,7 +3810,7 @@ static int path_is_beyond_symlink_1(struct apply_state *state, struct strbuf *na if (state->check_index) { struct cache_entry *ce; - ce = cache_file_exists(name->buf, name->len, ignore_case); + ce = index_file_exists(&the_index, name->buf, name->len, ignore_case); if (ce && S_ISLNK(ce->ce_mode)) return 1; } else { diff --git a/cache.h b/cache.h index 5de8ab4e69..85a85f8b96 100644 --- a/cache.h +++ b/cache.h @@ -369,7 +369,6 @@ extern void free_name_hash(struct index_state *istate); #define ce_match_stat(ce, st, options) ie_match_stat(&the_index, (ce), (st), (options)) #define ce_modified(ce, st, options) ie_modified(&the_index, (ce), (st), (options)) #define cache_dir_exists(name, namelen) index_dir_exists(&the_index, (name), (namelen)) -#define cache_file_exists(name, namelen, igncase) index_file_exists(&the_index, (name), (namelen), (igncase)) #endif enum object_type { diff --git a/dir.c b/dir.c index d5e1c462bb..0327832e53 100644 --- a/dir.c +++ b/dir.c @@ -1235,7 +1235,7 @@ static struct dir_entry *dir_entry_new(const char *pathname, int len) static struct dir_entry *dir_add_name(struct dir_struct *dir, const char *pathname, int len) { - if (cache_file_exists(pathname, len, ignore_case)) + if (index_file_exists(&the_index, pathname, len, ignore_case)) return NULL; ALLOC_GROW(dir->entries, dir->nr+1, dir->alloc); @@ -1269,7 +1269,7 @@ static enum exist_status directory_exists_in_index_icase(const char *dirname, in if (cache_dir_exists(dirname, len)) return index_directory; - ce = cache_file_exists(dirname, len, ignore_case); + ce = index_file_exists(&the_index, dirname, len, ignore_case); if (ce && S_ISGITLINK(ce->ce_mode)) return index_gitdir; @@ -1460,7 +1460,7 @@ static int get_index_dtype(const char *path, int len) int pos; const struct cache_entry *ce; - ce = cache_file_exists(path, len, 0); + ce = index_file_exists(&the_index, path, len, 0); if (ce) { if (!ce_uptodate(ce)) return DT_UNKNOWN; @@ -1522,7 +1522,8 @@ static enum path_treatment treat_one_path(struct dir_struct *dir, int dtype, struct dirent *de) { int exclude; - int has_path_in_index = !!cache_file_exists(path->buf, path->len, ignore_case); + int has_path_in_index = !!index_file_exists(&the_index, path->buf, + path->len, ignore_case); if (dtype == DT_UNKNOWN) dtype = get_dtype(de, path->buf, path->len); diff --git a/merge-recursive.c b/merge-recursive.c index 57ca250c88..b8b3a153f1 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -623,7 +623,8 @@ static int remove_file(struct merge_options *o, int clean, if (update_working_directory) { if (ignore_case) { struct cache_entry *ce; - ce = cache_file_exists(path, strlen(path), ignore_case); + ce = index_file_exists(&the_index, path, + strlen(path), ignore_case); if (ce && ce_stage(ce) == 0) return 0; } -- 2.13.0.rc1.39.ga6db8bfa24
[PATCH 10/24] cache.h: drop cache_name_is_other
Signed-off-by: Stefan Beller --- builtin/clean.c| 2 +- builtin/ls-files.c | 2 +- cache.h| 1 - dir.c | 2 +- wt-status.c| 4 ++-- 5 files changed, 5 insertions(+), 6 deletions(-) diff --git a/builtin/clean.c b/builtin/clean.c index 9bdefca6dc..c6aacbb0f0 100644 --- a/builtin/clean.c +++ b/builtin/clean.c @@ -938,7 +938,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix) struct stat st; const char *rel; - if (!cache_name_is_other(ent->name, ent->len)) + if (!index_name_is_other(&the_index, ent->name, ent->len)) continue; if (pathspec.nr) diff --git a/builtin/ls-files.c b/builtin/ls-files.c index edcad6e8e1..6f7ecec1b0 100644 --- a/builtin/ls-files.c +++ b/builtin/ls-files.c @@ -114,7 +114,7 @@ static void show_other_files(struct dir_struct *dir) for (i = 0; i < dir->nr; i++) { struct dir_entry *ent = dir->entries[i]; - if (!cache_name_is_other(ent->name, ent->len)) + if (!index_name_is_other(&the_index, ent->name, ent->len)) continue; show_dir_entry(tag_other, ent); } diff --git a/cache.h b/cache.h index abf1474034..5de8ab4e69 100644 --- a/cache.h +++ b/cache.h @@ -370,7 +370,6 @@ extern void free_name_hash(struct index_state *istate); #define ce_modified(ce, st, options) ie_modified(&the_index, (ce), (st), (options)) #define cache_dir_exists(name, namelen) index_dir_exists(&the_index, (name), (namelen)) #define cache_file_exists(name, namelen, igncase) index_file_exists(&the_index, (name), (namelen), (igncase)) -#define cache_name_is_other(name, namelen) index_name_is_other(&the_index, (name), (namelen)) #endif enum object_type { diff --git a/dir.c b/dir.c index 8abad1b969..d5e1c462bb 100644 --- a/dir.c +++ b/dir.c @@ -1244,7 +1244,7 @@ static struct dir_entry *dir_add_name(struct dir_struct *dir, const char *pathna struct dir_entry *dir_add_ignored(struct dir_struct *dir, const char *pathname, int len) { - if (!cache_name_is_other(pathname, len)) + if (!index_name_is_other(&the_index, pathname, len)) return NULL; ALLOC_GROW(dir->ignored, dir->ignored_nr+1, dir->ignored_alloc); diff --git a/wt-status.c b/wt-status.c index 750ed28b49..ff0e70a25a 100644 --- a/wt-status.c +++ b/wt-status.c @@ -669,7 +669,7 @@ static void wt_status_collect_untracked(struct wt_status *s) for (i = 0; i < dir.nr; i++) { struct dir_entry *ent = dir.entries[i]; - if (cache_name_is_other(ent->name, ent->len) && + if (index_name_is_other(&the_index, ent->name, ent->len) && dir_path_match(ent, &s->pathspec, 0, NULL)) string_list_insert(&s->untracked, ent->name); free(ent); @@ -677,7 +677,7 @@ static void wt_status_collect_untracked(struct wt_status *s) for (i = 0; i < dir.ignored_nr; i++) { struct dir_entry *ent = dir.ignored[i]; - if (cache_name_is_other(ent->name, ent->len) && + if (index_name_is_other(&the_index, ent->name, ent->len) && dir_path_match(ent, &s->pathspec, 0, NULL)) string_list_insert(&s->ignored, ent->name); free(ent); -- 2.13.0.rc1.39.ga6db8bfa24
[PATCH 05/24] cache.h: drop read_cache_unmerged()
@@ @@ -read_cache_unmerged() +read_index_unmerged(&the_index) Additionally drop the define from cache.h manually. Signed-off-by: Stefan Beller --- builtin/am.c| 2 +- builtin/merge.c | 2 +- builtin/pull.c | 2 +- builtin/read-tree.c | 2 +- builtin/reset.c | 2 +- cache.h | 1 - sequencer.c | 2 +- 7 files changed, 6 insertions(+), 7 deletions(-) diff --git a/builtin/am.c b/builtin/am.c index cb3e4dff63..bb0927fbcc 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -2053,7 +2053,7 @@ static int clean_index(const struct object_id *head, const struct object_id *rem if (!remote_tree) return error(_("Could not parse object '%s'."), oid_to_hex(remote)); - read_cache_unmerged(); + read_index_unmerged(&the_index); if (fast_forward_to(head_tree, head_tree, 1)) return -1; diff --git a/builtin/merge.c b/builtin/merge.c index 4d4c56050c..c27c806ac1 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -1170,7 +1170,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix) goto done; } - if (read_cache_unmerged()) + if (read_index_unmerged(&the_index)) die_resolve_conflict("merge"); if (file_exists(git_path_merge_head())) { diff --git a/builtin/pull.c b/builtin/pull.c index dd1a4a94e4..42578cee05 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -788,7 +788,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix) git_config(git_pull_config, NULL); - if (read_cache_unmerged()) + if (read_index_unmerged(&the_index)) die_resolve_conflict("pull"); if (file_exists(git_path_merge_head())) diff --git a/builtin/read-tree.c b/builtin/read-tree.c index f997814933..0bcf021ead 100644 --- a/builtin/read-tree.c +++ b/builtin/read-tree.c @@ -195,7 +195,7 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix) */ if (opts.reset || opts.merge || opts.prefix) { - if (read_cache_unmerged() && (opts.prefix || opts.merge)) + if (read_index_unmerged(&the_index) && (opts.prefix || opts.merge)) die("You need to resolve your current index first"); stage = opts.merge = 1; } diff --git a/builtin/reset.c b/builtin/reset.c index 03c5498d6e..4a4eb723dd 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -66,7 +66,7 @@ static int reset_index(const struct object_id *oid, int reset_type, int quiet) opts.reset = 1; } - read_cache_unmerged(); + read_index_unmerged(&the_index); if (reset_type == KEEP) { struct object_id head_oid; diff --git a/cache.h b/cache.h index a66ae97fb7..9b94339573 100644 --- a/cache.h +++ b/cache.h @@ -355,7 +355,6 @@ extern void free_name_hash(struct index_state *istate); #ifndef NO_THE_INDEX_COMPATIBILITY_MACROS #define is_cache_unborn() is_index_unborn(&the_index) -#define read_cache_unmerged() read_index_unmerged(&the_index) #define discard_cache() discard_index(&the_index) #define unmerged_cache() unmerged_index(&the_index) #define cache_name_pos(name, namelen) index_name_pos(&the_index,(name),(namelen)) diff --git a/sequencer.c b/sequencer.c index 9409b65aaa..f20e05fe60 100644 --- a/sequencer.c +++ b/sequencer.c @@ -349,7 +349,7 @@ static struct tree *empty_tree(void) static int error_dirty_index(struct replay_opts *opts) { - if (read_cache_unmerged()) + if (read_index_unmerged(&the_index)) return error_resolve_conflict(_(action_name(opts))); error(_("your local changes would be overwritten by %s."), -- 2.13.0.rc1.39.ga6db8bfa24
[PATCH 07/24] cache.h: drop read_blob_data_from_cache
This was done without cocinelle, as we only have 2 occurrences. Signed-off-by: Stefan Beller --- cache.h | 1 - convert.c | 4 ++-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/cache.h b/cache.h index 9b94339573..d078e88c3f 100644 --- a/cache.h +++ b/cache.h @@ -374,7 +374,6 @@ extern void free_name_hash(struct index_state *istate); #define resolve_undo_clear() resolve_undo_clear_index(&the_index) #define unmerge_cache_entry_at(at) unmerge_index_entry_at(&the_index, at) #define unmerge_cache(pathspec) unmerge_index(&the_index, pathspec) -#define read_blob_data_from_cache(path, sz) read_blob_data_from_index(&the_index, (path), (sz)) #endif enum object_type { diff --git a/convert.c b/convert.c index 8d652bf27c..cf29280ae1 100644 --- a/convert.c +++ b/convert.c @@ -137,7 +137,7 @@ const char *get_cached_convert_stats_ascii(const char *path) { const char *ret; unsigned long sz; - void *data = read_blob_data_from_cache(path, &sz); + void *data = read_blob_data_from_index(&the_index, path, &sz); ret = gather_convert_stats_ascii(data, sz); free(data); return ret; @@ -222,7 +222,7 @@ static int has_cr_in_index(const char *path) void *data; int has_cr; - data = read_blob_data_from_cache(path, &sz); + data = read_blob_data_from_index(&the_index, path, &sz); if (!data) return 0; has_cr = memchr(data, '\r', sz) != NULL; -- 2.13.0.rc1.39.ga6db8bfa24
[PATCH 02/24] cache.h: drop active_* macros
Based on the coccinelle patch: @@ @@ -active_cache +the_index.cache @@ @@ -active_nr +the_index.cache_nr @@ @@ -active_alloc +the_index.cache_alloc @@ @@ -active_cache_changed +the_index.cache_changed @@ @@ -active_cache_tree +the_index.cache_tree Additional manual editing: * drop the macros from cache.h * fix the whitespace issue that apply complained about in builtin/checkout.c * builtin/commit.c had an occurrence of active_cache_tree->sha1, which was not picked up with the coccinelle patch. * diff.c and t/t2107-update-index-basic.sh referenced active_cache[_changed] in comments, fix them up. * ative_nr was referenced in comments in read-cache.c and builtin/update-index.c, fix them. Signed-off-by: Stefan Beller --- apply.c | 6 ++--- builtin/add.c| 6 ++--- builtin/am.c | 6 ++--- builtin/blame.c | 6 ++--- builtin/checkout-index.c | 8 +++ builtin/checkout.c | 49 builtin/commit.c | 20 builtin/fsck.c | 12 +- builtin/grep.c | 8 +++ builtin/ls-files.c | 36 ++--- builtin/merge-index.c| 10 builtin/merge.c | 12 +- builtin/mv.c | 10 builtin/read-tree.c | 2 +- builtin/rm.c | 16 ++--- builtin/submodule--helper.c | 8 +++ builtin/update-index.c | 48 --- cache.h | 6 - check-racy.c | 4 ++-- diff-lib.c | 6 ++--- diff.c | 8 +++ dir.c| 20 merge-recursive.c| 28 +++ pathspec.c | 14 ++-- read-cache.c | 2 +- rerere.c | 26 ++--- revision.c | 18 +++ sequencer.c | 19 sha1_name.c | 14 ++-- submodule.c | 12 +- t/helper/test-dump-cache-tree.c | 2 +- t/helper/test-scrap-cache-tree.c | 2 +- t/t2107-update-index-basic.sh| 2 +- tree.c | 8 +++ wt-status.c | 12 +- 35 files changed, 232 insertions(+), 234 deletions(-) diff --git a/apply.c b/apply.c index 82701d6580..ae1b659b68 100644 --- a/apply.c +++ b/apply.c @@ -3499,7 +3499,7 @@ static int load_current(struct apply_state *state, pos = cache_name_pos(name, strlen(name)); if (pos < 0) return error(_("%s: does not exist in index"), name); - ce = active_cache[pos]; + ce = the_index.cache[pos]; if (lstat(name, &st)) { if (errno != ENOENT) return error_errno("%s", name); @@ -3671,7 +3671,7 @@ static int check_preimage(struct apply_state *state, goto is_new; return error(_("%s: does not exist in index"), old_name); } - *ce = active_cache[pos]; + *ce = the_index.cache[pos]; if (stat_ret < 0) { if (checkout_target(&the_index, *ce, st)) return -1; @@ -4001,7 +4001,7 @@ static int get_current_oid(struct apply_state *state, const char *path, pos = cache_name_pos(path, strlen(path)); if (pos < 0) return -1; - oidcpy(oid, &active_cache[pos]->oid); + oidcpy(oid, &the_index.cache[pos]->oid); return 0; } diff --git a/builtin/add.c b/builtin/add.c index ab6d04e8db..f6d71b10d0 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -34,8 +34,8 @@ static void chmod_pathspec(struct pathspec *pathspec, int force_mode) { int i; - for (i = 0; i < active_nr; i++) { - struct cache_entry *ce = active_cache[i]; + for (i = 0; i < the_index.cache_nr; i++) { + struct cache_entry *ce = the_index.cache[i]; if (pathspec && !ce_path_match(ce, pathspec, NULL)) continue; @@ -458,7 +458,7 @@ int cmd_add(int argc, const char **argv, const char *prefix) unplug_bulk_checkin(); finish: - if (active_cache_changed) { + if (the_index.cache_changed) { if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK)) die(_("Unable to write new index file")); } diff --git a/builtin/am.c b/builtin/am.c index 4405d7307c..c6a679d8e1 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -1155,12 +1155,12 @@ static int index_has_changes(struct strbuf *sb) diff_flush(&opt); return DIFF_OPT_TST(&o
[PATCH 03/24] cache.h: drop read_cache_from
coccinelle patch: @@ expression E; @@ -read_cache_from(E) +read_index_from(&the_index, E) additionally drop the define from cache.h manually Signed-off-by: Stefan Beller --- apply.c | 2 +- builtin/am.c | 4 ++-- builtin/commit.c | 6 +++--- cache.h | 1 - 4 files changed, 6 insertions(+), 7 deletions(-) diff --git a/apply.c b/apply.c index ae1b659b68..159e039a18 100644 --- a/apply.c +++ b/apply.c @@ -3985,7 +3985,7 @@ static int check_patch_list(struct apply_state *state, struct patch *patch) static int read_apply_cache(struct apply_state *state) { if (state->index_file) - return read_cache_from(state->index_file); + return read_index_from(&the_index, state->index_file); else return read_index(&the_index); } diff --git a/builtin/am.c b/builtin/am.c index c6a679d8e1..cb3e4dff63 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -1557,7 +1557,7 @@ static int run_apply(const struct am_state *state, const char *index_file) if (index_file) { /* Reload index as apply_all_patches() will have modified it. */ discard_cache(); - read_cache_from(index_file); + read_index_from(&the_index, index_file); } return 0; @@ -1600,7 +1600,7 @@ static int fall_back_threeway(const struct am_state *state, const char *index_pa return error("could not build fake ancestor"); discard_cache(); - read_cache_from(index_path); + read_index_from(&the_index, index_path); if (write_index_as_tree(orig_tree.hash, &the_index, index_path, 0, NULL)) return error(_("Repository lacks necessary blobs to fall back on 3-way merge.")); diff --git a/builtin/commit.c b/builtin/commit.c index 01d298c836..65a04ac199 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -370,7 +370,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix unsetenv(INDEX_ENVIRONMENT); discard_cache(); - read_cache_from(get_lock_file_path(&index_lock)); + read_index_from(&the_index, get_lock_file_path(&index_lock)); if (update_main_cache_tree(WRITE_TREE_SILENT) == 0) { if (reopen_lock_file(&index_lock) < 0) die(_("unable to write index file")); @@ -489,7 +489,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix discard_cache(); ret = get_lock_file_path(&false_lock); - read_cache_from(ret); + read_index_from(&the_index, ret); return ret; } @@ -949,7 +949,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix, * the editor and after we invoke run_status above. */ discard_cache(); - read_cache_from(index_file); + read_index_from(&the_index, index_file); if (update_main_cache_tree(0)) { error(_("Error building trees")); return 0; diff --git a/cache.h b/cache.h index 4e913d1346..6abf48dcc3 100644 --- a/cache.h +++ b/cache.h @@ -354,7 +354,6 @@ extern void free_name_hash(struct index_state *istate); #ifndef NO_THE_INDEX_COMPATIBILITY_MACROS -#define read_cache_from(path) read_index_from(&the_index, (path)) #define read_cache_preload(pathspec) read_index_preload(&the_index, (pathspec)) #define is_cache_unborn() is_index_unborn(&the_index) #define read_cache_unmerged() read_index_unmerged(&the_index) -- 2.13.0.rc1.39.ga6db8bfa24
[PATCH 04/24] cache.h: drop read_cache_preload(pathspec)
coccinelle patch: @@ expression E; @@ -read_cache_preload(E) +read_index_preload(&the_index, E) Additionally manual editing: * drop the define from cache.h. * builtin/{commit,describe}.c were not picked up as we have NULL and the address of an expression. Converted them manually. * builtin/diff{-files,-index}.c error messages converted as well. Signed-off-by: Stefan Beller --- builtin/checkout.c | 4 ++-- builtin/commit.c | 4 ++-- builtin/describe.c | 2 +- builtin/diff-files.c | 4 ++-- builtin/diff-index.c | 4 ++-- builtin/diff.c | 8 builtin/update-index.c | 2 +- cache.h| 1 - 8 files changed, 14 insertions(+), 15 deletions(-) diff --git a/builtin/checkout.c b/builtin/checkout.c index 0aac616ad6..2328a475ea 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -295,7 +295,7 @@ static int checkout_paths(const struct checkout_opts *opts, lock_file = xcalloc(1, sizeof(struct lock_file)); hold_locked_index(lock_file, LOCK_DIE_ON_ERROR); - if (read_cache_preload(&opts->pathspec) < 0) + if (read_index_preload(&the_index, &opts->pathspec) < 0) return error(_("index file corrupt")); if (opts->source_tree) @@ -488,7 +488,7 @@ static int merge_working_tree(const struct checkout_opts *opts, struct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file)); hold_locked_index(lock_file, LOCK_DIE_ON_ERROR); - if (read_cache_preload(NULL) < 0) + if (read_index_preload(&the_index, NULL) < 0) return error(_("index file corrupt")); resolve_undo_clear(); diff --git a/builtin/commit.c b/builtin/commit.c index 65a04ac199..687e7c8a3a 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -346,7 +346,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix PATHSPEC_PREFER_FULL, prefix, argv); - if (read_cache_preload(&pathspec) < 0) + if (read_index_preload(&the_index, &pathspec) < 0) die(_("index file corrupt")); if (interactive) { @@ -1377,7 +1377,7 @@ int cmd_status(int argc, const char **argv, const char *prefix) PATHSPEC_PREFER_FULL, prefix, argv); - read_cache_preload(&s.pathspec); + read_index_preload(&the_index, &s.pathspec); refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, &s.pathspec, NULL, NULL); fd = hold_locked_index(&index_lock, 0); diff --git a/builtin/describe.c b/builtin/describe.c index a5cd8c513f..0229458ac6 100644 --- a/builtin/describe.c +++ b/builtin/describe.c @@ -531,7 +531,7 @@ int cmd_describe(int argc, const char **argv, const char *prefix) static struct lock_file index_lock; int fd; - read_cache_preload(NULL); + read_index_preload(&the_index, NULL); refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL); fd = hold_locked_index(&index_lock, 0); diff --git a/builtin/diff-files.c b/builtin/diff-files.c index 15c61fd8d1..d400d8c1fc 100644 --- a/builtin/diff-files.c +++ b/builtin/diff-files.c @@ -62,8 +62,8 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix) (rev.diffopt.output_format & DIFF_FORMAT_PATCH)) rev.combine_merges = rev.dense_combined_merges = 1; - if (read_cache_preload(&rev.diffopt.pathspec) < 0) { - perror("read_cache_preload"); + if (read_index_preload(&the_index, &rev.diffopt.pathspec) < 0) { + perror("read_index_preload"); return -1; } result = run_diff_files(&rev, options); diff --git a/builtin/diff-index.c b/builtin/diff-index.c index 49fd64d4ce..3fbe33a90a 100644 --- a/builtin/diff-index.c +++ b/builtin/diff-index.c @@ -44,8 +44,8 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix) usage(diff_cache_usage); if (!cached) { setup_work_tree(); - if (read_cache_preload(&rev.diffopt.pathspec) < 0) { - perror("read_cache_preload"); + if (read_index_preload(&the_index, &rev.diffopt.pathspec) < 0) { + perror("read_index_preload"); return -1; } } else if (read_index(&the_index) < 0) { diff --git a/builtin/diff.c b/builtin/diff.c index ed9edb2d0c..0ae33bce2b 100644 --- a/builtin/diff.c +++ b/builtin/diff.c @@ -144,8 +144,8 @@ static int builtin_diff_index(struct rev_info *revs, usage(builtin_diff_usage); if (!cached) { setup_work_tree(); - if (read_cache_preload(&revs->diffopt.pathspec) < 0) { - perror("read_cache_preload"); + if (
[PATCH 01/24] cache.h: drop read_cache()
This patch is produced via coccinelle using this semantic patch: @@ @@ -read_cache() +read_index(&the_index) Additional manual editing: * drop define in cache.h * a comment in builtin/check-ignore.c and read-cache.c were converted * builtin/diff.c: fix error message referencing the function. Signed-off-by: Stefan Beller --- apply.c | 2 +- builtin/add.c| 4 ++-- builtin/am.c | 2 +- builtin/blame.c | 4 ++-- builtin/check-attr.c | 2 +- builtin/check-ignore.c | 4 ++-- builtin/checkout-index.c | 2 +- builtin/clean.c | 2 +- builtin/commit.c | 4 ++-- builtin/diff-index.c | 2 +- builtin/diff.c | 6 +++--- builtin/fsck.c | 2 +- builtin/grep.c | 2 +- builtin/ls-files.c | 2 +- builtin/merge-index.c| 2 +- builtin/mv.c | 2 +- builtin/reset.c | 2 +- builtin/rev-parse.c | 2 +- builtin/rm.c | 2 +- builtin/submodule--helper.c | 2 +- builtin/update-index.c | 2 +- cache.h | 1 - check-racy.c | 2 +- diff.c | 2 +- merge-recursive.c| 2 +- merge.c | 2 +- read-cache.c | 2 +- rerere.c | 6 +++--- revision.c | 4 ++-- sequencer.c | 6 +++--- sha1_name.c | 2 +- submodule.c | 4 ++-- t/helper/test-dump-cache-tree.c | 2 +- t/helper/test-dump-untracked-cache.c | 2 +- t/helper/test-lazy-init-name-hash.c | 10 +- t/helper/test-read-cache.c | 2 +- t/helper/test-scrap-cache-tree.c | 2 +- 37 files changed, 52 insertions(+), 53 deletions(-) diff --git a/apply.c b/apply.c index e6dbab26ad..82701d6580 100644 --- a/apply.c +++ b/apply.c @@ -3987,7 +3987,7 @@ static int read_apply_cache(struct apply_state *state) if (state->index_file) return read_cache_from(state->index_file); else - return read_cache(); + return read_index(&the_index); } /* This function tries to read the object name from the current index */ diff --git a/builtin/add.c b/builtin/add.c index 9f53f020d0..ab6d04e8db 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -205,7 +205,7 @@ static int edit_patch(int argc, const char **argv, const char *prefix) git_config(git_diff_basic_config, NULL); /* no "diff" UI options */ - if (read_cache() < 0) + if (read_index(&the_index) < 0) die(_("Could not read the index")); init_revisions(&rev, prefix); @@ -376,7 +376,7 @@ int cmd_add(int argc, const char **argv, const char *prefix) return 0; } - if (read_cache() < 0) + if (read_index(&the_index) < 0) die(_("index file corrupt")); /* diff --git a/builtin/am.c b/builtin/am.c index a95dd8b4e6..4405d7307c 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -1633,7 +1633,7 @@ static int fall_back_threeway(const struct am_state *state, const char *index_pa say(state, stdout, _("Falling back to patching base and 3-way merge...")); discard_cache(); - read_cache(); + read_index(&the_index); /* * This is not so wrong. Depending on which base we picked, orig_tree diff --git a/builtin/blame.c b/builtin/blame.c index 07506a3e45..59955208fd 100644 --- a/builtin/blame.c +++ b/builtin/blame.c @@ -2313,7 +2313,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt, unsigned mode; struct strbuf msg = STRBUF_INIT; - read_cache(); + read_index(&the_index); time(&now); commit = alloc_commit_node(); commit->object.parsed = 1; @@ -2395,7 +2395,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt, * want to run "diff-index --cached". */ discard_cache(); - read_cache(); + read_index(&the_index); len = strlen(path); if (!mode) { diff --git a/builtin/check-attr.c b/builtin/check-attr.c index 4d01ca0c8b..9cc3675d62 100644 --- a/builtin/check-attr.c +++ b/builtin/check-attr.c @@ -114,7 +114,7 @@ int cmd_check_attr(int argc, const char **argv, const char *prefix) argc = parse_options(argc, argv, prefix, check_attr_options, check_attr_usage, PARSE_OPT_KEEP_DASHDASH); - if (read_cache() < 0) { + if (read_index(&the_index) < 0) { die("invalid cache"); } diff --git a/builtin/check-ignore.c b/buil
[PATCH 00/24] Retire NO_THE_INDEX_COMPATIBILITY_MACROS
This is the follow up to [1], but actually completed now. The reasoning why this series is a good idea is in the commit message of the last patch, quoted here: The NO_THE_INDEX_COMPATIBILITY_MACROS pre processor setting was introduced in 4aab5b46f4 (Make read-cache.c "the_index" free., 2007-04-01), stating: This makes all low-level functions defined in read-cache.c to take an explicit index_state structure as their first parameter, to specify which index to work on. The reasoning is very vague, maybe indicating that having the index specified to work on is easier to for the new reader to understand what is going on. All preceding patches worked on retiring functions that do not take an explicit index as to where to perform its work. Spelling out the reasons why we want to specify the index at each call: 1) Ease of understanding: The recent patches dropped a lot of macros that may confuse new people diving into the code base. 2a) Spelling out global state explicitly: Anything you do in submodule land today needs to spawn new processes in the submodule. This is cumbersome and not performant. So we may want to have an abstraction of a repo, i.e. all repository state in one struct. That way we can open a submodule in-process and perform the required actions without spawning a process. The road to this future is a long road, and we have to start somewhere. Exposing the global state seems like a good starter point. 2b) Spelling out global state explicitly (II): (Jeff Hostetler wrote) In addition to (eventually) allowing multiple repos be open at the same time for submodules, it would also help with various multi-threading efforts. For example, we have loops that do a "for (k = 0, k < active_nr; k++) {...}" There is no visual clue in that code that it references "the_index" and therefore should be subject to the same locking. Granted, this is a trivial example, but goes to the argument that the code has lots of subtle global variables and macros that make it difficult to reason about the code. This step would help un-hide this. Review as well as critique if this is the right approach to an endgame with less globals splattered throughout the codebase is welcome. Thanks, Stefan [1] https://public-inbox.org/git/20170501190719.10669-1-sbel...@google.com/ Stefan Beller (24): cache.h: drop read_cache() cache.h: drop active_* macros cache.h: drop read_cache_from cache.h: drop read_cache_preload(pathspec) cache.h: drop read_cache_unmerged() unpack-trees.c: rename parameter 'the_index' cache.h: drop read_blob_data_from_cache cache.h: drop unmerge_cache[_entry_at] cache.h: drop resolve_undo_clear cache.h: drop cache_name_is_other cache.h: drop cache_file_exists cache.h: drop cache_dir_exists cache.h: drop is_cache_unborn(), discard_cache(), unmerged_cache() cache.h: drop cache_name_pos cache.h: drop add_cache_entry cache.h: drop rename_cache_entry_at cache.h: drop remove_file_from_cache cache.h: drop add_to_cache cache.h: drop add_file_to_cache cache.h: drop chmod_cache_entry cache.h: drop refresh_cache cache.h: drop ce_modified cache.h: drop ce_match_stat cache.h: retire NO_THE_INDEX_COMPATIBILITY_MACROS apply.c | 32 +++-- attr.c | 1 - builtin/add.c| 12 ++--- builtin/am.c | 26 +-- builtin/blame.c | 19 builtin/check-attr.c | 2 +- builtin/check-ignore.c | 4 +- builtin/checkout-index.c | 12 ++--- builtin/checkout.c | 66 +- builtin/clean.c | 4 +- builtin/commit.c | 52 ++--- builtin/describe.c | 2 +- builtin/diff-files.c | 4 +- builtin/diff-index.c | 6 +-- builtin/diff.c | 19 builtin/fsck.c | 14 +++--- builtin/grep.c | 10 ++-- builtin/ls-files.c | 49 +-- builtin/merge-index.c| 14 +++--- builtin/merge.c | 24 +- builtin/mv.c | 22 - builtin/pull.c | 4 +- builtin/read-tree.c | 8 ++-- builtin/reset.c | 11 +++-- builtin/rev-parse.c | 2 +- builtin/rm.c | 26 +-- builtin/submodule--helper.c | 10 ++-- builtin/update-index.c | 91 +++- cache.h | 35 -- check-racy.c | 10 ++-- convert.c| 4 +- diff-lib.c | 8 ++-- diff.c | 14 +++--- dir.c
Re: [PATCH 0/5] Abide by our own rules regarding line endings
Hi, Johannes Schindelin wrote: > Over the past decade, there have been a couple of attempts to remedy the [...] I'm intentionally skimming this cover letter, since anything important it says needs to also be in the commit messages. [...] > Without these fixes, Git will fail to build and pass the test suite, as > can be verified even on Linux using this cadence: > > git config core.autocrlf true > rm .git/index && git stash > make DEVELOPER=1 -j15 test This should go in a commit message (or perhaps in all of them). [...] > Johannes Schindelin (5): > Fix build with core.autocrlf=true > git-new-workdir: mark script as LF-only > completion: mark bash script as LF-only > Fix the remaining tests that failed with core.autocrlf=true > t4051: mark supporting files as requiring LF-only line endings With or without that change, Reviewed-by: Jonathan Nieder The t/README bit in patch 4/5 is sad (I want to be able to open t/README using an old version of Notepad) but changing that test to use another file seems out of scope for what this series does. Thanks, Jonathan
Re: Proposal for missing blob support in Git repos
On 05/02/2017 11:32 AM, Ævar Arnfjörð Bjarmason wrote: On Tue, May 2, 2017 at 7:21 PM, Jonathan Tan wrote: On Mon, May 1, 2017 at 6:41 PM, Junio C Hamano wrote: Jonathan Tan writes: On 05/01/2017 04:29 PM, Junio C Hamano wrote: Jonathan Tan writes: Thanks for your comments. If you're referring to the codepath involving write_sha1_file() (for example, builtin/hash-object -> index_fd or builtin/unpack-objects), that is fine because write_sha1_file() invokes freshen_packed_object() and freshen_loose_object() directly to check if the object already exists (and thus does not invoke the new mechanism in this patch). Is that a good thing, though? It means that you an attacker can feed one version to the remote object store your "grab blob" hook gets the blobs from, and have you add a colliding object locally, and the usual "are we recording the same object as existing one?" check is bypassed. If I understand this correctly, what you mean is the situation where the hook adds an object to the local repo, overriding another object of the same name? No. write_sha1_file() pays attention to objects already in the local object store to avoid hash collisions that can be used to replace a known-to-be-good object and that is done as a security measure. What I am reading in your response was that this new mechanism bypasses that, and I was wondering if that is a good thing. Oh, what I meant was that write_sha1_file() bypasses the new mechanism, not that the new mechanism bypasses the checks in write_sha1_file(). To be clear, here's what happens when write_sha1_file() is invoked (before and after this patch - this patch does not affect write_sha1_file at all): 1. (some details omitted) 2. call freshen_packed_object 3, call freshen_loose_object if necessary 4. write object (if freshen_packed_object and freshen_loose_object do not both return 0) Nothing changes in this patch (whether a hook is defined or not). But don't the semantics change in the sense that before core.missingBlobCommand you couldn't write a new blob SHA1 that was already part of your history, Strictly speaking, you can already do this if you don't have the blob in your local repo (for example, with shallow clones - you likely wouldn't have blobs pointed to by historical commits outside whatever depth is set). > whereas with this change write_sha1_file() might write what it considers to be a new blob, but it's actually colliding with an existing blob, but write_sha1_file() doesn't know that because knowing would involve asking the hook to fetch the blob? Yes, this might happen. I see the semantics as "don't write what you already have", where "have" means what you have in local storage, but if you extend "have" to what upstream has, then yes, you're right that this changes (ignoring shallow clones). This does remove a resistance that we have against hash collision (in that normally we would have the correct object for a given hash and can resist other servers trying to introduce a wrong object, but now that is no longer the case), but I think it's better than consulting the hook whenever you want to write anything (which is also a change in semantics in that you're consulting an external source whenever you're writing an object, besides the performance implications). And here's what happens when has_sha1_file (or another function listed in the commit message) is invoked: 1. check for existence of packed object of the requested name 2. check for existence of loose object of the requested name 3. check again for existence of packed object of the requested name 4. if a hook is defined, invoke the hook and repeat 1-3 Here, in step 4, the hook could do whatever it wants to the repository. This might be a bit of early bikeshedding, but then again the lack of early bikeshedding tends to turn into standards. Wouldn't it be better to name this core.missingObjectCommand & have the hook take a list on stdin like: [] And have the hook respond: [] I.e. what you'd do now is send this to the hook: 1 blobmissing And the hook would respond: 1 ok But this leaves open the door addressing this potential edge case with writing new blobs in the future, i.e. write_sha1_file() could call it as: 1 blobnew And the hook could either respond immediately as: 1 ok If it's in some #YOLO mode where it's not going to check for colliding blobs over the network, or alternatively & ask the parent repo if it has those blobs, and if so print: 1 collision Or something like that. This also enables future lazy loading of trees/commits from the same API, and for the hook to respond out-of-order to the input it gets as it can, since each request is prefixed with an incrementing request id. My initial thought is that it would be better to extend hook support by adding configuration options for separate hooks instead of extending an existing protocol. For example, with t
Re: PCRE v2 compile error, was Re: What's cooking in git.git (May 2017, #01; Mon, 1)
On Tue, May 2, 2017 at 10:51 PM, Kevin Daudt wrote: > On Tue, May 02, 2017 at 08:52:21PM +0200, Ęvar Arnfjörš Bjarmason wrote: >> >> * Due to the bizarro existing semantics of the configure script noted >> upthread if you have a git build script that does --with-libpcre & you >> have libpcre1 installed, it'll link to it, but now since >> --with-libpcre defaults to libpcre2 it'll silently skip linking to it >> if you don't have it installed. >> > > Case in point: The Archlinux git-git aur package[0] (community maintained, > latest git version) does run ./configure without --with-libpcre, but > requests it from make with USE_LIBPCRE=1. > > I noticed when trying git grep -P which then failed. > > [0]: https://aur.archlinux.org/cgit/aur.git/tree/PKGBUILD?h=git-git Whatever's going on there is unrelated to the issue I'm talking about, but if that's producing a package where -P doesn't work that looks like a bug in the Makefile. The way --with-libpcre works now is that it second guesses you, i.e. it'll put USE_LIBPCRE=YesPlease into config.mak.autogen (sourced by the Makefile) only if you supply --with-libpcre *and* it can find a working libpcre on the system, otherwise it silently ignores you. Whatever you supply to the configure script it'll be overridden by Makefile arguments if present, but even if there was another bug where ./configure arguments took precedence over Makefile arguments I don't see how it would apply here, in this case nothing libpcre related will be written to config.mak.autogen. So I must be missing something. I don't see how that package build doesn't either fail at compile time because it doesn't have pcre, or work, in which case the -P option will work.
Re: PCRE v2 compile error, was Re: What's cooking in git.git (May 2017, #01; Mon, 1)
On Tue, May 02, 2017 at 08:52:21PM +0200, Ævar Arnfjörð Bjarmason wrote: > > * Due to the bizarro existing semantics of the configure script noted > upthread if you have a git build script that does --with-libpcre & you > have libpcre1 installed, it'll link to it, but now since > --with-libpcre defaults to libpcre2 it'll silently skip linking to it > if you don't have it installed. > Case in point: The Archlinux git-git aur package[0] (community maintained, latest git version) does run ./configure without --with-libpcre, but requests it from make with USE_LIBPCRE=1. I noticed when trying git grep -P which then failed. [0]: https://aur.archlinux.org/cgit/aur.git/tree/PKGBUILD?h=git-git
Re: Reference help
On Tue, May 2, 2017 at 9:33 PM, Desjardin, Donald wrote: > Sorry if this is not the place for this. > > I'm looking for any reference to potential problems when updating a git > client (say from 1.7.N to 1.8.N) with old workspaces. > > The scenario is this: > Lots of developers use a single machine for work > They have lots of workspaces created using the old client > We want to upgrade to the new client > > Are there any potential problems just upgrading the client and NOT re-cloning > their workspaces (or stashing or committing or pushing)? > > Subversion had/has some feature that could tell that the workspace was > created using an older client and you could magically run something to update > the workspace. > > I'm not above telling all my developers to commit/push to a feature branch, > remove the workspaces and re-clone as needed on a flag-day, but I'd like to > know if I have to. > > If there is any documentation that talks about this (I know 1.7 is old). Git, unlike Subversion has used the same stable underlying storage format for all of its history. So you certainly don't need to migrate these checkouts when you upgrade git, they'll just work. There are several things that changed in the UI, some in incompatible ways, most prominently the behavior of "git push", but it's unlikely anyone will be affected by that beyond some minor annoyance. You can see announcements of major changes in git's release notes, which you can browse here: https://github.com/git/git/tree/master/Documentation/RelNotes E.g. 1.8.0.txt in that directory will have notes for upgrading from 1.7.x to 1.8.0. Note that both 1.7.0 and 1.8.0 are really old, from 2010 & 2012 respectively. Although it'll be a bigger jump you might want to consider updating to the most recent release directly, currently 2.12.2, this'll be more changes at once, but you won't have to go from 1.7 to 1.8, 1.8 to 1.9 etc.
Reference help
Sorry if this is not the place for this. I'm looking for any reference to potential problems when updating a git client (say from 1.7.N to 1.8.N) with old workspaces. The scenario is this: Lots of developers use a single machine for work They have lots of workspaces created using the old client We want to upgrade to the new client Are there any potential problems just upgrading the client and NOT re-cloning their workspaces (or stashing or committing or pushing)? Subversion had/has some feature that could tell that the workspace was created using an older client and you could magically run something to update the workspace. I'm not above telling all my developers to commit/push to a feature branch, remove the workspaces and re-clone as needed on a flag-day, but I'd like to know if I have to. If there is any documentation that talks about this (I know 1.7 is old). Thanks, Don Desjardin
Re: [PATCHv2 0/3] Some submodule bugfixes
On 05/02, Stefan Beller wrote: > v2: > * I dropped the RFC patches for reattaching HEAD as that is not the main >concern of this series. > * patch1 is just about dropping cp1 to reusing cp, instead of additionally >"fixing" mem leaks as finish_command does that for us > * reordered the patches, former patch 3 now as patch 2 (avoid auto-discovery >in new working tree manipulator code) > * Regarding former patch2, I wrote: > > Junio wrote: > >> Sounds good (if only because this makes it similar to other > >> codepaths). > >> > >> Is this something whose breakage before the patch is easily > >> demonstrated with a test? > > > I'll try to come up with a test in a reroll. > > done > > Thanks, > Stefan Changes look good. > > > Stefan Beller (3): > submodule_move_head: reuse child_process structure for futher commands > submodule: avoid auto-discovery in new working tree manipulator code > submodule: properly recurse for read-tree and checkout > > submodule.c| 21 +++-- > t/lib-submodule-update.sh | 7 +-- > t/t1013-read-tree-submodule.sh | 1 - > t/t2013-checkout-submodule.sh | 1 - > 4 files changed, 12 insertions(+), 18 deletions(-) > > -- > 2.13.0.rc1.19.g083305f9b1 > -- Brandon Williams
[PATCHv2 2/3] submodule: avoid auto-discovery in new working tree manipulator code
All commands that are run in a submodule, are run in a correct setup, there is no need to prepare the environment without setting the GIT_DIR variable. By setting the GIT_DIR variable we fix issues as discussed in 10f5c52656 (submodule: avoid auto-discovery in prepare_submodule_repo_env(), 2016-09-01) Signed-off-by: Stefan Beller --- submodule.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/submodule.c b/submodule.c index a2377ce019..b0141a66dd 100644 --- a/submodule.c +++ b/submodule.c @@ -1363,7 +1363,7 @@ static int submodule_has_dirty_index(const struct submodule *sub) { struct child_process cp = CHILD_PROCESS_INIT; - prepare_submodule_repo_env_no_git_dir(&cp.env_array); + prepare_submodule_repo_env(&cp.env_array); cp.git_cmd = 1; argv_array_pushl(&cp.args, "diff-index", "--quiet", @@ -1380,7 +1380,7 @@ static int submodule_has_dirty_index(const struct submodule *sub) static void submodule_reset_index(const char *path) { struct child_process cp = CHILD_PROCESS_INIT; - prepare_submodule_repo_env_no_git_dir(&cp.env_array); + prepare_submodule_repo_env(&cp.env_array); cp.git_cmd = 1; cp.no_stdin = 1; @@ -1438,7 +1438,7 @@ int submodule_move_head(const char *path, } } - prepare_submodule_repo_env_no_git_dir(&cp.env_array); + prepare_submodule_repo_env(&cp.env_array); cp.git_cmd = 1; cp.no_stdin = 1; -- 2.13.0.rc1.19.g083305f9b1
[PATCHv2 3/3] submodule: properly recurse for read-tree and checkout
We forgot to prepare the submodule env, which is only a problem for nested submodules. See 2e5d6503bd (ls-files: fix recurse-submodules with nested submodules, 2017-04-13) for further explanation. To come up with a proper test for this, we'd need to look at nested submodules just as in that given commit. It turns out we're lucky and these tests already exist, but are marked as failing. We need to pass `--recurse-submodules` to read-tree additionally to make these tests pass. Passing that flag alone would not make the tests pass, such that this covers testing for the bug fix of the submodule env as well. Signed-off-by: Stefan Beller --- submodule.c| 3 ++- t/lib-submodule-update.sh | 7 +-- t/t1013-read-tree-submodule.sh | 1 - t/t2013-checkout-submodule.sh | 1 - 4 files changed, 3 insertions(+), 9 deletions(-) diff --git a/submodule.c b/submodule.c index b0141a66dd..b3ae642f29 100644 --- a/submodule.c +++ b/submodule.c @@ -1446,7 +1446,7 @@ int submodule_move_head(const char *path, argv_array_pushf(&cp.args, "--super-prefix=%s%s/", get_super_prefix_or_empty(), path); - argv_array_pushl(&cp.args, "read-tree", NULL); + argv_array_pushl(&cp.args, "read-tree", "--recurse-submodules", NULL); if (flags & SUBMODULE_MOVE_HEAD_DRY_RUN) argv_array_push(&cp.args, "-n"); @@ -1474,6 +1474,7 @@ int submodule_move_head(const char *path, cp.no_stdin = 1; cp.dir = path; + prepare_submodule_repo_env(&cp.env_array); argv_array_pushl(&cp.args, "update-ref", "HEAD", new, NULL); if (run_command(&cp)) { diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh index fb4f7b014e..2c17826e95 100755 --- a/t/lib-submodule-update.sh +++ b/t/lib-submodule-update.sh @@ -787,11 +787,6 @@ test_submodule_switch_recursing () { then RESULTDS=failure fi - RESULTR=success - if test "$KNOWN_FAILURE_SUBMODULE_RECURSIVE_NESTED" = 1 - then - RESULTR=failure - fi RESULTOI=success if test "$KNOWN_FAILURE_SUBMODULE_OVERWRITE_IGNORED_UNTRACKED" = 1 then @@ -1003,7 +998,7 @@ test_submodule_switch_recursing () { ' # recursing deeper than one level doesn't work yet. - test_expect_$RESULTR "$command: modified submodule updates submodule recursively" ' + test_expect_success "$command: modified submodule updates submodule recursively" ' prolog && reset_work_tree_to_interested add_nested_sub && ( diff --git a/t/t1013-read-tree-submodule.sh b/t/t1013-read-tree-submodule.sh index de1ba02dc5..7019d0a04f 100755 --- a/t/t1013-read-tree-submodule.sh +++ b/t/t1013-read-tree-submodule.sh @@ -5,7 +5,6 @@ test_description='read-tree can handle submodules' . ./test-lib.sh . "$TEST_DIRECTORY"/lib-submodule-update.sh -KNOWN_FAILURE_SUBMODULE_RECURSIVE_NESTED=1 KNOWN_FAILURE_DIRECTORY_SUBMODULE_CONFLICTS=1 KNOWN_FAILURE_SUBMODULE_OVERWRITE_IGNORED_UNTRACKED=1 diff --git a/t/t2013-checkout-submodule.sh b/t/t2013-checkout-submodule.sh index e8f70b806f..aa35223369 100755 --- a/t/t2013-checkout-submodule.sh +++ b/t/t2013-checkout-submodule.sh @@ -64,7 +64,6 @@ test_expect_success '"checkout " honors submodule.*.ignore from .git/ ' KNOWN_FAILURE_DIRECTORY_SUBMODULE_CONFLICTS=1 -KNOWN_FAILURE_SUBMODULE_RECURSIVE_NESTED=1 test_submodule_switch_recursing "git checkout --recurse-submodules" test_submodule_forced_switch_recursing "git checkout -f --recurse-submodules" -- 2.13.0.rc1.19.g083305f9b1
[PATCHv2 0/3] Some submodule bugfixes
v2: * I dropped the RFC patches for reattaching HEAD as that is not the main concern of this series. * patch1 is just about dropping cp1 to reusing cp, instead of additionally "fixing" mem leaks as finish_command does that for us * reordered the patches, former patch 3 now as patch 2 (avoid auto-discovery in new working tree manipulator code) * Regarding former patch2, I wrote: > Junio wrote: >> Sounds good (if only because this makes it similar to other >> codepaths). >> >> Is this something whose breakage before the patch is easily >> demonstrated with a test? > I'll try to come up with a test in a reroll. done Thanks, Stefan Stefan Beller (3): submodule_move_head: reuse child_process structure for futher commands submodule: avoid auto-discovery in new working tree manipulator code submodule: properly recurse for read-tree and checkout submodule.c| 21 +++-- t/lib-submodule-update.sh | 7 +-- t/t1013-read-tree-submodule.sh | 1 - t/t2013-checkout-submodule.sh | 1 - 4 files changed, 12 insertions(+), 18 deletions(-) -- 2.13.0.rc1.19.g083305f9b1
[PATCHv2 1/3] submodule_move_head: reuse child_process structure for futher commands
We do not need to declare another struct child_process, but we can just reuse the existing `cp` struct. Signed-off-by: Stefan Beller --- submodule.c | 12 ++-- 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/submodule.c b/submodule.c index d3299e29c0..a2377ce019 100644 --- a/submodule.c +++ b/submodule.c @@ -1468,15 +1468,15 @@ int submodule_move_head(const char *path, if (!(flags & SUBMODULE_MOVE_HEAD_DRY_RUN)) { if (new) { - struct child_process cp1 = CHILD_PROCESS_INIT; + child_process_init(&cp); /* also set the HEAD accordingly */ - cp1.git_cmd = 1; - cp1.no_stdin = 1; - cp1.dir = path; + cp.git_cmd = 1; + cp.no_stdin = 1; + cp.dir = path; - argv_array_pushl(&cp1.args, "update-ref", "HEAD", new, NULL); + argv_array_pushl(&cp.args, "update-ref", "HEAD", new, NULL); - if (run_command(&cp1)) { + if (run_command(&cp)) { ret = -1; goto out; } -- 2.13.0.rc1.19.g083305f9b1
Re: [PATCH v2 5/6] submodule: improve submodule_has_commits
On 05/02, Brandon Williams wrote: > On 05/02, Stefan Beller wrote: > > On Tue, May 2, 2017 at 10:25 AM, Brandon Williams wrote: > > > On 05/01, Stefan Beller wrote: > > >> On Mon, May 1, 2017 at 6:02 PM, Brandon Williams > > >> wrote: > > >> > + > > >> > + if (capture_command(&cp, &out, GIT_MAX_HEXSZ + 1) || > > >> > out.len) > > >> > > >> eh, I gave too much and self-contradicting feedback here earlier, > > >> ideally I'd like to review this to be similar as: > > >> > > >> if (capture_command(&cp, &out, GIT_MAX_HEXSZ + 1) > > >> die("cannot capture git-rev-list in submodule '%s', sub->path); > > > > > > This wouldn't really work because if you provide a SHA1 to rev-list > > > which it isn't able to find then it returns a non-zero exit code which > > > would cause this to die, which isn't the desired behavior. > > > > Oh. In that case, why do we even check for its stdout? > > (from rev-lists man page) > >--quiet > >Don’t print anything to standard output. This form is primarily > >meant to allow the caller to test the exit status to see if a > > range > >of objects is fully connected (or not). It is faster than > >redirecting stdout to /dev/null as the output does not have to be > >formatted. > > > > Sounds good, Let me take a look at using --quiet >From our offline discussion --quiet doesn't do what we want here. We are checking (1) if the commit exists and (2) if it is reachable. Using --quiet would only satisfy (1) > > > > > > > I feel like you're making this a little too complicated, as all I'm > > > doing is shuffling around already existing logic. I understand the want > > > to make things more robust but this seems unnecessarily complex. > > > > ok. I was just giving my thoughts on how I would approach it. > > > > Thanks, > > Stefan > > -- > Brandon Williams -- Brandon Williams
Re: [PATCH v2 5/6] submodule: improve submodule_has_commits
On 05/02, Stefan Beller wrote: > On Tue, May 2, 2017 at 10:25 AM, Brandon Williams wrote: > > On 05/01, Stefan Beller wrote: > >> On Mon, May 1, 2017 at 6:02 PM, Brandon Williams wrote: > >> > + > >> > + if (capture_command(&cp, &out, GIT_MAX_HEXSZ + 1) || > >> > out.len) > >> > >> eh, I gave too much and self-contradicting feedback here earlier, > >> ideally I'd like to review this to be similar as: > >> > >> if (capture_command(&cp, &out, GIT_MAX_HEXSZ + 1) > >> die("cannot capture git-rev-list in submodule '%s', sub->path); > > > > This wouldn't really work because if you provide a SHA1 to rev-list > > which it isn't able to find then it returns a non-zero exit code which > > would cause this to die, which isn't the desired behavior. > > Oh. In that case, why do we even check for its stdout? > (from rev-lists man page) >--quiet >Don’t print anything to standard output. This form is primarily >meant to allow the caller to test the exit status to see if a range >of objects is fully connected (or not). It is faster than >redirecting stdout to /dev/null as the output does not have to be >formatted. > Sounds good, Let me take a look at using --quiet > > > > I feel like you're making this a little too complicated, as all I'm > > doing is shuffling around already existing logic. I understand the want > > to make things more robust but this seems unnecessarily complex. > > ok. I was just giving my thoughts on how I would approach it. > > Thanks, > Stefan -- Brandon Williams
Re: [PATCH v2 00/53] object_id part 8
On 05/01, brian m. carlson wrote: > This is the eighth series of patches to convert unsigned char [20] to > struct object_id. This series converts lookup_commit, lookup_blob, > lookup_tree, lookup_tag, and finally parse_object to struct object_id. > > A small number of functions have temporaries inserted during the > conversion in order to allow conversion of functions that still need to > take unsigned char *; they are removed either later in the series or > will be in a future series. > > This series can be fetched from the object-id-part8 branch from either > of the follwing: > > https://github.com/bk2204/git > https://git.crustytoothpaste.net/git/bmc/git.git I've looked through the series and overall everything looks good. -- Brandon Williams
Re: PCRE v2 compile error, was Re: What's cooking in git.git (May 2017, #01; Mon, 1)
On Tue, May 2, 2017 at 6:05 PM, Johannes Schindelin wrote: > Hi Ævar, > > On Tue, 2 May 2017, Ævar Arnfjörð Bjarmason wrote: > >> On Tue, May 2, 2017 at 2:09 PM, Johannes Schindelin >> wrote: >> > >> > On Sun, 30 Apr 2017, Junio C Hamano wrote: >> > >> >> * ab/grep-pcre-v2 (2017-04-25) 20 commits >> >> - SQUASH??? >> >> - Makefile & configure: make PCRE v2 the default PCRE implementation >> >> - grep: remove support for concurrent use of both PCRE v1 & v2 >> >> - grep: add support for PCRE v2 >> >> - grep: add support for the PCRE v1 JIT API >> >> - perf: add a performance comparison test of grep -E and -P >> >> - grep: change the internal PCRE code & header names to be PCRE1 >> >> - grep: change the internal PCRE macro names to be PCRE1 >> >> - test-lib: rename the LIBPCRE prerequisite to PCRE >> >> - grep: make grep.patternType=[pcre|pcre1] a synonym for "perl" >> >> - grep & rev-list doc: stop promising libpcre for --perl-regexp >> >> - log: add -P as a synonym for --perl-regexp >> >> - log: add exhaustive tests for pattern style options & config >> >> - grep: add a test for backreferences in PCRE patterns >> >> - Makefile & configure: reword outdated comment about PCRE >> >> - grep: remove redundant `regflags &= ~REG_EXTENDED` assignments >> >> - grep: remove redundant regflags assignment under PCRE >> >> - grep: submodule-related case statements should die if new fields are >> >> added >> >> - grep: add tests for grep pattern types being passed to submodules >> >> - grep: amend submodule recursion test in preparation for rx engine >> >> testing >> >> >> >> PCRE2, which has an API different from and incompatible with PCRE, >> >> can now be chosen to support "grep -P -e ''" and friends. >> > >> > FWIW for quite a couple of recent builds, `pu` fails on Windows with a >> > variation of this error: >> > >> > CC blob.o >> > In file included from revision.h:5:0, >> > from bisect.c:4: >> > grep.h:16:19: fatal error: pcre2.h: No such file or directory >> > #include >> >^ >> > compilation terminated. >> > >> > Maybe this can be fixed before hitting `next`? >> >> This will be due to a combination of the build machine not having pcre >> v2 (but having v1) & my "Makefile & configure: make PCRE v2 the >> default PCRE implementation" patch, which makes v2 the default for >> USE_LIBPCRE=YesPlease. >> >> Is it easy to install v2 on these build machines? Alternatively that >> patch could be ejected out of pu, or you could USE_LIBPCRE1=YesPlease >> to use v1, but as explained in that commit I think it makes sense to >> make v2 the default. > > Let me put it this way: Installing PCRE v1 in MSYS2 is as easy as > > pacman -Sy mingw-w64-x86_64-pcre > > To install PCRE v2, you would have to copy-edit > https://github.com/Alexpux/MINGW-packages/blob/master/mingw-w64-pcre/PKGBUILD, > make sure that it builds by running it through > > makepkg-mingw -s > > possibly initializing a Git repository in the > mingw-w64-pcre/src/${_realname}-${pkgver}/ directory, patching the source > until it builds, committing the changes, adding them as patch files to the > same directory as the new PKGBUILD, adjusting the PKGBUILD file to list > the patch files with their checksums and to add the commands to apply > them. > > Then (and this is a one time cost, fortunately) initializing two packages > on BinTray (which we use to serve the Pacman packages of Git for Windows), > then build and upload the packages. > > In short, PCRE v2 would be slightly (ahem, ahem) more involved than PCRE > v1. > > I cannot imagine that MSYS2 is the only environment with that issue. I think it's worth it to copy/paste the commit message where I made this change, since we're having this discussion in this thread: Makefile & configure: make PCRE v2 the default PCRE implementation Change the USE_LIBPCRE=YesPlease & --with-libpcre flags to the Makefile & configure script, respectively, to mean use PCRE v2, not PCRE v1. The legacy library previously available via those options is still available on request via USE_LIBPCRE1=YesPlease or --with-libpcre1. The existing USE_LIBPCRE2=YesPlease & --with-libpcre2 still explicitly ask for v2. The v2 PCRE is stable & end-user compatible, all this change does is change the default. Someone building a new git is likely to also have packaged PCRE v2 sometime in the last 2 years since it was released. If not they can choose to use the legacy v2 library by making the trivial s/USE_LIBPCRE/USE_LIBPCRE1/ change, or package up PCRE v2. New releases of PCRE v2 are already faster than PCRE v1, and not only is all significant development is happening on v2, but bugs in reported against v1 have started getting WONTFIX'd asking users to just upgrade to v2. So it makes sense to give our downstream distributors a nudge to swit
Re: CYGWIN git cannot install python packages with pip
Note that MSYS2 Git-12.2.1 also has no such problem. On 2 May 2017 at 20:08, ankostis wrote: > On Windows, with Cygwin-git 02.12.2-1 the python command: > > pip install git+https://github.com/...` > > fails to work with the following error: > > ... > > Git-2.8.3 had no such problem. > > Any ideas?
Re: Proposal for missing blob support in Git repos
On Tue, May 2, 2017 at 7:21 PM, Jonathan Tan wrote: > On Mon, May 1, 2017 at 6:41 PM, Junio C Hamano wrote: >> Jonathan Tan writes: >> >>> On 05/01/2017 04:29 PM, Junio C Hamano wrote: Jonathan Tan writes: > Thanks for your comments. If you're referring to the codepath > involving write_sha1_file() (for example, builtin/hash-object -> > index_fd or builtin/unpack-objects), that is fine because > write_sha1_file() invokes freshen_packed_object() and > freshen_loose_object() directly to check if the object already exists > (and thus does not invoke the new mechanism in this patch). Is that a good thing, though? It means that you an attacker can feed one version to the remote object store your "grab blob" hook gets the blobs from, and have you add a colliding object locally, and the usual "are we recording the same object as existing one?" check is bypassed. >>> >>> If I understand this correctly, what you mean is the situation where >>> the hook adds an object to the local repo, overriding another object >>> of the same name? >> >> No. >> >> write_sha1_file() pays attention to objects already in the local >> object store to avoid hash collisions that can be used to replace a >> known-to-be-good object and that is done as a security measure. >> What I am reading in your response was that this new mechanism >> bypasses that, and I was wondering if that is a good thing. > > Oh, what I meant was that write_sha1_file() bypasses the new > mechanism, not that the new mechanism bypasses the checks in > write_sha1_file(). > > To be clear, here's what happens when write_sha1_file() is invoked > (before and after this patch - this patch does not affect > write_sha1_file at all): > 1. (some details omitted) > 2. call freshen_packed_object > 3, call freshen_loose_object if necessary > 4. write object (if freshen_packed_object and freshen_loose_object do > not both return 0) > > Nothing changes in this patch (whether a hook is defined or not). But don't the semantics change in the sense that before core.missingBlobCommand you couldn't write a new blob SHA1 that was already part of your history, whereas with this change write_sha1_file() might write what it considers to be a new blob, but it's actually colliding with an existing blob, but write_sha1_file() doesn't know that because knowing would involve asking the hook to fetch the blob? > And here's what happens when has_sha1_file (or another function listed > in the commit message) is invoked: > 1. check for existence of packed object of the requested name > 2. check for existence of loose object of the requested name > 3. check again for existence of packed object of the requested name > 4. if a hook is defined, invoke the hook and repeat 1-3 > > Here, in step 4, the hook could do whatever it wants to the repository. This might be a bit of early bikeshedding, but then again the lack of early bikeshedding tends to turn into standards. Wouldn't it be better to name this core.missingObjectCommand & have the hook take a list on stdin like: [] And have the hook respond: [] I.e. what you'd do now is send this to the hook: 1 blobmissing And the hook would respond: 1 ok But this leaves open the door addressing this potential edge case with writing new blobs in the future, i.e. write_sha1_file() could call it as: 1 blobnew And the hook could either respond immediately as: 1 ok If it's in some #YOLO mode where it's not going to check for colliding blobs over the network, or alternatively & ask the parent repo if it has those blobs, and if so print: 1 collision Or something like that. This also enables future lazy loading of trees/commits from the same API, and for the hook to respond out-of-order to the input it gets as it can, since each request is prefixed with an incrementing request id.
Re: [PATCH v3 14/25] setup_discovered_git_dir(): plug memory leak
On Tue, May 02, 2017 at 10:20:29AM -0700, Stefan Beller wrote: > > - gitdir = real_pathdup(gitdir, 1); > > + gitdir = to_free = real_pathdup(gitdir, 1); > > if (chdir(cwd->buf)) > > die_errno("Could not come back to cwd"); > > As the original motivation was to shut up Coverity, this may not > accomplish that goal, as in the path of taking the die_errno, we do not > free `to_free`. But that is ok as the actual goal is to hav no memleaks > in the good case. A memleak just before a die is no big deal. I think Coverity understands our NORETURN attributes, so this should be fine (and if it doesn't, then we should fix that in the model file; but from the general results I've seen, it does). -Peff
Re: [PATCH v2 03/53] Convert struct cache_tree to use struct object_id
On 05/01, brian m. carlson wrote: > Convert the sha1 member of struct cache_tree to struct object_id by > changing the definition and applying the following semantic patch, plus > the standard object_id transforms: > > @@ > struct cache_tree E1; > @@ > - E1.sha1 > + E1.oid.hash > > @@ > struct cache_tree *E1; > @@ > - E1->sha1 > + E1->oid.hash > > Fix up one reference to active_cache_tree which was not automatically > caught by Coccinelle. These changes are prerequisites for converting > parse_object. > > Signed-off-by: brian m. carlson > --- > builtin/commit.c| 2 +- > builtin/fsck.c | 4 ++-- > cache-tree.c| 31 --- > cache-tree.h| 3 ++- > merge-recursive.c | 2 +- > revision.c | 2 +- > sequencer.c | 3 ++- > t/helper/test-dump-cache-tree.c | 4 ++-- > 8 files changed, 27 insertions(+), 24 deletions(-) > > diff --git a/builtin/commit.c b/builtin/commit.c > index 1d805f5da..8685c888f 100644 > --- a/builtin/commit.c > +++ b/builtin/commit.c > @@ -1758,7 +1758,7 @@ int cmd_commit(int argc, const char **argv, const char > *prefix) > append_merge_tag_headers(parents, &tail); > } > > - if (commit_tree_extended(sb.buf, sb.len, active_cache_tree->sha1, > + if (commit_tree_extended(sb.buf, sb.len, active_cache_tree->oid.hash, >parents, oid.hash, author_ident.buf, sign_commit, > extra)) { > rollback_index_files(); > die(_("failed to write commit object")); > diff --git a/builtin/fsck.c b/builtin/fsck.c > index b5e13a455..c40e14de6 100644 > --- a/builtin/fsck.c > +++ b/builtin/fsck.c > @@ -599,10 +599,10 @@ static int fsck_cache_tree(struct cache_tree *it) > fprintf(stderr, "Checking cache tree\n"); > > if (0 <= it->entry_count) { > - struct object *obj = parse_object(it->sha1); > + struct object *obj = parse_object(it->oid.hash); > if (!obj) { > error("%s: invalid sha1 pointer in cache-tree", > - sha1_to_hex(it->sha1)); > + oid_to_hex(&it->oid)); > errors_found |= ERROR_REFS; > return 1; > } > diff --git a/cache-tree.c b/cache-tree.c > index 345ea3596..35d507ed7 100644 > --- a/cache-tree.c > +++ b/cache-tree.c > @@ -225,7 +225,7 @@ int cache_tree_fully_valid(struct cache_tree *it) > int i; > if (!it) > return 0; > - if (it->entry_count < 0 || !has_sha1_file(it->sha1)) > + if (it->entry_count < 0 || !has_sha1_file(it->oid.hash)) > return 0; > for (i = 0; i < it->subtree_nr; i++) { > if (!cache_tree_fully_valid(it->down[i]->cache_tree)) > @@ -253,7 +253,7 @@ static int update_one(struct cache_tree *it, > > *skip_count = 0; > > - if (0 <= it->entry_count && has_sha1_file(it->sha1)) > + if (0 <= it->entry_count && has_sha1_file(it->oid.hash)) > return it->entry_count; > > /* > @@ -340,7 +340,7 @@ static int update_one(struct cache_tree *it, > die("cache-tree.c: '%.*s' in '%s' not found", > entlen, path + baselen, path); > i += sub->count; > - sha1 = sub->cache_tree->sha1; > + sha1 = sub->cache_tree->oid.hash; > mode = S_IFDIR; > contains_ita = sub->cache_tree->entry_count < 0; > if (contains_ita) { > @@ -402,12 +402,13 @@ static int update_one(struct cache_tree *it, > unsigned char sha1[20]; > hash_sha1_file(buffer.buf, buffer.len, tree_type, sha1); > if (has_sha1_file(sha1)) > - hashcpy(it->sha1, sha1); > + hashcpy(it->oid.hash, sha1); > else > to_invalidate = 1; > } else if (dryrun) > - hash_sha1_file(buffer.buf, buffer.len, tree_type, it->sha1); > - else if (write_sha1_file(buffer.buf, buffer.len, tree_type, it->sha1)) { > + hash_sha1_file(buffer.buf, buffer.len, tree_type, > +it->oid.hash); > + else if (write_sha1_file(buffer.buf, buffer.len, tree_type, > it->oid.hash)) { > strbuf_release(&buffer); > return -1; > } > @@ -417,7 +418,7 @@ static int update_one(struct cache_tree *it, > #if DEBUG > fprintf(stderr, "cache-tree update-one (%d ent, %d subtree) %s\n", > it->entry_count, it->subtree_nr, > - sha1_to_hex(it->sha1)); > + oid_to_hex(&it->oid)); > #endif > return i; > } > @@ -457,14 +458,14 @@ static void write_one(struct strbuf *buffer, struct > cache_tree *it, > if (0 <= it->entry_count) >
CYGWIN git cannot install python packages with pip
On Windows, with Cygwin-git 02.12.2-1 the python command: pip install git+https://github.com/...` fails to work with the following error: $ pip install git+https://github.com/ipython/traitlets/ Collecting git+https://github.com/ipython/traitlets/ Cloning https://github.com/ipython/traitlets/ to c:\users\username\appdata \local\temp\pip-kjwxq_oy-build fatal: Invalid path '/cygdrive/d/Work/ALLINONES2/co2mpas_AIO-v1.6.0/CO2MPA S/C:\Users\username\AppData\Local\Temp\pip-kjwxq_oy-build': No such file or di rectory git clone -q https://github.com/ipython/traitlets/ C:\Users\user\AppData\L ocal\Temp\pip-kjwxq_oy-build" failed with error code 128 in None Git-2.8.3 had no such problem. Any ideas?
Re: [PATCH v2 02/53] Clean up outstanding object_id transforms.
On 05/01, brian m. carlson wrote: > The semantic patch for standard object_id transforms found two > outstanding places where we could make a transformation automatically. > Apply these changes. > > Signed-off-by: brian m. carlson > --- > builtin/diff.c | 2 +- > reflog-walk.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/builtin/diff.c b/builtin/diff.c > index d184aafab..a25b4e4ae 100644 > --- a/builtin/diff.c > +++ b/builtin/diff.c > @@ -408,7 +408,7 @@ int cmd_diff(int argc, const char **argv, const char > *prefix) > } else if (obj->type == OBJ_BLOB) { > if (2 <= blobs) > die(_("more than two blobs given: '%s'"), name); > - hashcpy(blob[blobs].oid.hash, obj->oid.hash); > + oidcpy(&blob[blobs].oid, &obj->oid); > blob[blobs].name = name; > blob[blobs].mode = entry->mode; > blobs++; > diff --git a/reflog-walk.c b/reflog-walk.c > index 99679f582..c8fdf051d 100644 > --- a/reflog-walk.c > +++ b/reflog-walk.c > @@ -241,7 +241,7 @@ void fake_reflog_parent(struct reflog_walk_info *info, > struct commit *commit) > logobj = parse_object(reflog->ooid.hash); > } while (commit_reflog->recno && (logobj && logobj->type != > OBJ_COMMIT)); > > - if (!logobj && commit_reflog->recno >= 0 && > is_null_sha1(reflog->ooid.hash)) { > + if (!logobj && commit_reflog->recno >= 0 && is_null_oid(&reflog->ooid)) > { Not relevant to this series but I was confused for a second seeing 'ooid' as I have no clue what that means :) > /* a root commit, but there are still more entries to show */ > reflog = &commit_reflog->reflogs->items[commit_reflog->recno]; > logobj = parse_object(reflog->noid.hash); -- Brandon Williams
Re: [PATCH v2 5/6] submodule: improve submodule_has_commits
On Tue, May 2, 2017 at 10:25 AM, Brandon Williams wrote: > On 05/01, Stefan Beller wrote: >> On Mon, May 1, 2017 at 6:02 PM, Brandon Williams wrote: >> > + >> > + if (capture_command(&cp, &out, GIT_MAX_HEXSZ + 1) || >> > out.len) >> >> eh, I gave too much and self-contradicting feedback here earlier, >> ideally I'd like to review this to be similar as: >> >> if (capture_command(&cp, &out, GIT_MAX_HEXSZ + 1) >> die("cannot capture git-rev-list in submodule '%s', sub->path); > > This wouldn't really work because if you provide a SHA1 to rev-list > which it isn't able to find then it returns a non-zero exit code which > would cause this to die, which isn't the desired behavior. Oh. In that case, why do we even check for its stdout? (from rev-lists man page) --quiet Don’t print anything to standard output. This form is primarily meant to allow the caller to test the exit status to see if a range of objects is fully connected (or not). It is faster than redirecting stdout to /dev/null as the output does not have to be formatted. > > I feel like you're making this a little too complicated, as all I'm > doing is shuffling around already existing logic. I understand the want > to make things more robust but this seems unnecessarily complex. ok. I was just giving my thoughts on how I would approach it. Thanks, Stefan
Re: PCRE v2 compile error, was Re: What's cooking in git.git (May 2017, #01; Mon, 1)
On Tue, May 2, 2017 at 7:43 PM, Brandon Williams wrote: > On 05/02, Ævar Arnfjörð Bjarmason wrote: >> On Tue, May 2, 2017 at 2:09 PM, Johannes Schindelin >> wrote: >> > Hi Ævar, >> > >> > On Sun, 30 Apr 2017, Junio C Hamano wrote: >> > >> >> * ab/grep-pcre-v2 (2017-04-25) 20 commits >> >> - SQUASH??? >> >> - Makefile & configure: make PCRE v2 the default PCRE implementation >> >> - grep: remove support for concurrent use of both PCRE v1 & v2 >> >> - grep: add support for PCRE v2 >> >> - grep: add support for the PCRE v1 JIT API >> >> - perf: add a performance comparison test of grep -E and -P >> >> - grep: change the internal PCRE code & header names to be PCRE1 >> >> - grep: change the internal PCRE macro names to be PCRE1 >> >> - test-lib: rename the LIBPCRE prerequisite to PCRE >> >> - grep: make grep.patternType=[pcre|pcre1] a synonym for "perl" >> >> - grep & rev-list doc: stop promising libpcre for --perl-regexp >> >> - log: add -P as a synonym for --perl-regexp >> >> - log: add exhaustive tests for pattern style options & config >> >> - grep: add a test for backreferences in PCRE patterns >> >> - Makefile & configure: reword outdated comment about PCRE >> >> - grep: remove redundant `regflags &= ~REG_EXTENDED` assignments >> >> - grep: remove redundant regflags assignment under PCRE >> >> - grep: submodule-related case statements should die if new fields are >> >> added >> >> - grep: add tests for grep pattern types being passed to submodules >> >> - grep: amend submodule recursion test in preparation for rx engine >> >> testing >> >> >> >> PCRE2, which has an API different from and incompatible with PCRE, >> >> can now be chosen to support "grep -P -e ''" and friends. >> > >> > FWIW for quite a couple of recent builds, `pu` fails on Windows with a >> > variation of this error: >> > >> > CC blob.o >> > In file included from revision.h:5:0, >> > from bisect.c:4: >> > grep.h:16:19: fatal error: pcre2.h: No such file or directory >> > #include >> >^ >> > compilation terminated. >> > >> > Maybe this can be fixed before hitting `next`? >> >> This will be due to a combination of the build machine not having pcre >> v2 (but having v1) & my "Makefile & configure: make PCRE v2 the >> default PCRE implementation" patch, which makes v2 the default for >> USE_LIBPCRE=YesPlease. >> >> Is it easy to install v2 on these build machines? Alternatively that >> patch could be ejected out of pu, or you could USE_LIBPCRE1=YesPlease >> to use v1, but as explained in that commit I think it makes sense to >> make v2 the default. > > Shouldn't the Makefile check for the existence of PCREv2 before making > it the default? We can't check for the existence of anything in the Makefile, that's what the configure script is for. The configure script does the "right" thing with pcre v2. The "right" thing being the bizarro pre-existing behavior of "oh you asked for PCRE? Well let me check if it's on the system, if it's not I'll just silently undo what you asked for". Which is crazy, but was there before my patches, so I didn't change it. I.e. instead of the more sane behavior of opting the user into an optional library if it's found on the system, and producing an error if you supply --with-that-library but don't have thatlibrary.so.
Re: PCRE v2 compile error, was Re: What's cooking in git.git (May 2017, #01; Mon, 1)
On 05/02, Ævar Arnfjörð Bjarmason wrote: > On Tue, May 2, 2017 at 2:09 PM, Johannes Schindelin > wrote: > > Hi Ævar, > > > > On Sun, 30 Apr 2017, Junio C Hamano wrote: > > > >> * ab/grep-pcre-v2 (2017-04-25) 20 commits > >> - SQUASH??? > >> - Makefile & configure: make PCRE v2 the default PCRE implementation > >> - grep: remove support for concurrent use of both PCRE v1 & v2 > >> - grep: add support for PCRE v2 > >> - grep: add support for the PCRE v1 JIT API > >> - perf: add a performance comparison test of grep -E and -P > >> - grep: change the internal PCRE code & header names to be PCRE1 > >> - grep: change the internal PCRE macro names to be PCRE1 > >> - test-lib: rename the LIBPCRE prerequisite to PCRE > >> - grep: make grep.patternType=[pcre|pcre1] a synonym for "perl" > >> - grep & rev-list doc: stop promising libpcre for --perl-regexp > >> - log: add -P as a synonym for --perl-regexp > >> - log: add exhaustive tests for pattern style options & config > >> - grep: add a test for backreferences in PCRE patterns > >> - Makefile & configure: reword outdated comment about PCRE > >> - grep: remove redundant `regflags &= ~REG_EXTENDED` assignments > >> - grep: remove redundant regflags assignment under PCRE > >> - grep: submodule-related case statements should die if new fields are > >> added > >> - grep: add tests for grep pattern types being passed to submodules > >> - grep: amend submodule recursion test in preparation for rx engine > >> testing > >> > >> PCRE2, which has an API different from and incompatible with PCRE, > >> can now be chosen to support "grep -P -e ''" and friends. > > > > FWIW for quite a couple of recent builds, `pu` fails on Windows with a > > variation of this error: > > > > CC blob.o > > In file included from revision.h:5:0, > > from bisect.c:4: > > grep.h:16:19: fatal error: pcre2.h: No such file or directory > > #include > >^ > > compilation terminated. > > > > Maybe this can be fixed before hitting `next`? > > This will be due to a combination of the build machine not having pcre > v2 (but having v1) & my "Makefile & configure: make PCRE v2 the > default PCRE implementation" patch, which makes v2 the default for > USE_LIBPCRE=YesPlease. > > Is it easy to install v2 on these build machines? Alternatively that > patch could be ejected out of pu, or you could USE_LIBPCRE1=YesPlease > to use v1, but as explained in that commit I think it makes sense to > make v2 the default. Shouldn't the Makefile check for the existence of PCREv2 before making it the default? -- Brandon Williams
Re: [PATCH v2 5/6] submodule: improve submodule_has_commits
On 05/01, Stefan Beller wrote: > On Mon, May 1, 2017 at 6:02 PM, Brandon Williams wrote: > > + > > + if (capture_command(&cp, &out, GIT_MAX_HEXSZ + 1) || > > out.len) > > eh, I gave too much and self-contradicting feedback here earlier, > ideally I'd like to review this to be similar as: > > if (capture_command(&cp, &out, GIT_MAX_HEXSZ + 1) > die("cannot capture git-rev-list in submodule '%s', sub->path); This wouldn't really work because if you provide a SHA1 to rev-list which it isn't able to find then it returns a non-zero exit code which would cause this to die, which isn't the desired behavior. > > if (out.len) > has_commit = 0; > > instead as that does not have a silent error. (though it errs > on the safe side, so maybe it is not to bad.) > > I could understand if the callers do not want to have > `submodule_has_commits` die()-ing on them, so maybe > > if (capture_command(&cp, &out, GIT_MAX_HEXSZ + 1) { > warning("cannot capture git-rev-list in submodule '%s', sub->path); > has_commit = -1; > /* this would require auditing all callers and handling -1 though */ > } > > if (out.len) > has_commit = 0; > > As the comment eludes, we'd then have > 0 -> has no commits > 1 -> has commits > -1 -> error > > So to group (error || has_no_commits), we could write > > if (submodule_has_commits(..) <= 0) > > which is awkward. So maybe we can rename the function > to misses_submodule_commits instead, as then we could > flip the return value as well and have > > 0 -> has commits > 1 -> has no commits > -1 -> error > > and the lazy invoker could just go with > > if (!misses_submodule_commits(..)) > proceed(); > else > die("missing submodule commits or errors; I don't care"); > > whereas the careful invoker could go with > > switch (misses_submodule_commits(..)) { > case 0: > proceed(); break; > case 1: > pull_magic_trick(); break; > case -1: > make_errors_go_away_and_retry(); break; > } I feel like you're making this a little too complicated, as all I'm doing is shuffling around already existing logic. I understand the want to make things more robust but this seems unnecessarily complex. > --- > On the longer term plan: > As you wrote about costs. Maybe instead of invoking rev-list, > we could try to have this in-core as a first try-out for > "classified-repos", looking at refs.h there is e.g. > > int for_each_ref_submodule(const char *submodule_path, > each_ref_fn fn, void *cb_data); > > which we could use to obtain all submodule refs and then > use the revision walking machinery to find out ourselves if > we have or do not have the commits. (As we loaded the > odb of the submodule, this would *just work*, building one > kludgy hack upon the next.) > > Thanks, > Stefan -- Brandon Williams
Re: Proposal for missing blob support in Git repos
On Mon, May 1, 2017 at 6:41 PM, Junio C Hamano wrote: > Jonathan Tan writes: > >> On 05/01/2017 04:29 PM, Junio C Hamano wrote: >>> Jonathan Tan writes: >>> Thanks for your comments. If you're referring to the codepath involving write_sha1_file() (for example, builtin/hash-object -> index_fd or builtin/unpack-objects), that is fine because write_sha1_file() invokes freshen_packed_object() and freshen_loose_object() directly to check if the object already exists (and thus does not invoke the new mechanism in this patch). >>> >>> Is that a good thing, though? It means that you an attacker can >>> feed one version to the remote object store your "grab blob" hook >>> gets the blobs from, and have you add a colliding object locally, >>> and the usual "are we recording the same object as existing one?" >>> check is bypassed. >> >> If I understand this correctly, what you mean is the situation where >> the hook adds an object to the local repo, overriding another object >> of the same name? > > No. > > write_sha1_file() pays attention to objects already in the local > object store to avoid hash collisions that can be used to replace a > known-to-be-good object and that is done as a security measure. > What I am reading in your response was that this new mechanism > bypasses that, and I was wondering if that is a good thing. Oh, what I meant was that write_sha1_file() bypasses the new mechanism, not that the new mechanism bypasses the checks in write_sha1_file(). To be clear, here's what happens when write_sha1_file() is invoked (before and after this patch - this patch does not affect write_sha1_file at all): 1. (some details omitted) 2. call freshen_packed_object 3, call freshen_loose_object if necessary 4. write object (if freshen_packed_object and freshen_loose_object do not both return 0) Nothing changes in this patch (whether a hook is defined or not). And here's what happens when has_sha1_file (or another function listed in the commit message) is invoked: 1. check for existence of packed object of the requested name 2. check for existence of loose object of the requested name 3. check again for existence of packed object of the requested name 4. if a hook is defined, invoke the hook and repeat 1-3 Here, in step 4, the hook could do whatever it wants to the repository.
Re: [PATCH v3 14/25] setup_discovered_git_dir(): plug memory leak
On Tue, May 2, 2017 at 9:02 AM, Johannes Schindelin wrote: > The setup_explicit_git_dir() function does not take custody of the string > passed as first parameter; we have to release it if we turned the value of > git_dir into an absolute path. > > Signed-off-by: Johannes Schindelin > --- > setup.c | 9 +++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/setup.c b/setup.c > index 0320a9ad14c..e3f7699a902 100644 > --- a/setup.c > +++ b/setup.c > @@ -703,11 +703,16 @@ static const char *setup_discovered_git_dir(const char > *gitdir, > > /* --work-tree is set without --git-dir; use discovered one */ > if (getenv(GIT_WORK_TREE_ENVIRONMENT) || git_work_tree_cfg) { > + char *to_free = NULL; > + const char *ret; > + > if (offset != cwd->len && !is_absolute_path(gitdir)) > - gitdir = real_pathdup(gitdir, 1); > + gitdir = to_free = real_pathdup(gitdir, 1); > if (chdir(cwd->buf)) > die_errno("Could not come back to cwd"); As the original motivation was to shut up Coverity, this may not accomplish that goal, as in the path of taking the die_errno, we do not free `to_free`. But that is ok as the actual goal is to hav no memleaks in the good case. A memleak just before a die is no big deal. > - return setup_explicit_git_dir(gitdir, cwd, nongit_ok); > + ret = setup_explicit_git_dir(gitdir, cwd, nongit_ok); > + free(to_free); > + return ret; And we free our pathdup from above, makes sense. Thanks, Stefan
Re: [PATCH 0/5] Start of a journey: drop NO_THE_INDEX_COMPATIBILITY_MACROS
On Tue, May 2, 2017 at 8:35 AM, Jeff Hostetler wrote: > You may also want to look at unpack-trees.c : mark_new_skip_worktree(). > It has a local variable named "the_index" in the argument list. > You may want to rename this to avoid confusion. Thanks for bringing this up. I just made a local commit, to be sent out with v2. > > Thank you for bringing this up and making this proposal. > I started a similar effort internally last fall, but > stopped because of the footprint size. > Yeah, I also have a bad feeling about the foot print, which is why I asked if now is a good time to go with such a series. > In addition to (eventually) allowing multiple repos be open at > the same time for submodules, it would also help with various > multi-threading efforts. For example, we have loops that do a > "for (k = 0, k < active_nr; k++) {...}" There is no visual clue > in that code that it references "the_index" and therefore should > be subject to the same locking. Granted, this is a trivial example, > but goes to the argument that the code has lots of subtle global > variables and macros that make it difficult to reason about the > code. > > This step would help un-hide this. Thanks for pointing out the actual underlying reason, that I was trying to formulate. I'll borrow these lines for future cover letters. Thanks, Stefan
[PATCH v3] l10n: de.po: update German translation
Translate 96 new messages came from git.pot update in dfc182b (l10n: git.pot: v2.13.0 round 1 (96 new, 37 removed)). Signed-off-by: Ralf Thielow --- po/de.po | 323 --- 1 file changed, 142 insertions(+), 181 deletions(-) diff --git a/po/de.po b/po/de.po index b92c4fe11..8272b06b5 100644 --- a/po/de.po +++ b/po/de.po @@ -866,9 +866,9 @@ msgid "Argument not supported for format '%s': -%d" msgstr "Argument für Format '%s' nicht unterstützt: -%d" #: attr.c:212 -#, fuzzy, c-format +#, c-format msgid "%.*s is not a valid attribute name" -msgstr "'%s' ist kein gültiger Name für ein Remote-Repository" +msgstr "%.*s ist kein gültiger Attributname" #: attr.c:408 msgid "" @@ -1260,6 +1260,8 @@ msgstr "Speicher verbraucht" #: config.c:191 msgid "relative config include conditionals must come from files" msgstr "" +"Bedingungen für das Einbinden von Konfigurationen aus relativen Pfaden muss\n" +"aus Dateien kommen." #: config.c:701 #, c-format @@ -1379,12 +1381,12 @@ msgstr "Ungültiger %s: '%s'" #: config.c:1952 #, c-format msgid "unknown core.untrackedCache value '%s'; using 'keep' default value" -msgstr "" +msgstr "Unbekannter Wert '%s' in core.untrackedCache; benutze Standardwert 'keep'" #: config.c:1978 #, c-format msgid "splitIndex.maxPercentChange value '%d' should be between 0 and 100" -msgstr "" +msgstr "Der Wert '%d' von splitIndex.maxPercentChange sollte zwischen 0 und 100 liegen." #: config.c:1989 #, c-format @@ -1645,9 +1647,9 @@ msgstr "" "für dieses Verzeichnis deaktiviert." #: dir.c:2776 dir.c:2781 -#, fuzzy, c-format +#, c-format msgid "could not create directories for %s" -msgstr "Konnte Verzeichnis '%s' nicht erstellen." +msgstr "Konnte Verzeichnisse für '%s' nicht erstellen." #: dir.c:2806 #, c-format @@ -1655,9 +1657,9 @@ msgid "could not migrate git directory from '%s' to '%s'" msgstr "Konnte Git-Verzeichnis nicht von '%s' nach '%s' migrieren." #: entry.c:280 -#, fuzzy, c-format +#, c-format msgid "could not stat file '%s'" -msgstr "konnte Datei '%s' nicht erstellen" +msgstr "konnte Datei '%s' nicht lesen" #: fetch-pack.c:249 msgid "git fetch-pack: expected shallow list" @@ -1827,14 +1829,14 @@ msgid "no matching remote head" msgstr "kein übereinstimmender Remote-Branch" #: fetch-pack.c:1147 -#, fuzzy, c-format +#, c-format msgid "no such remote ref %s" -msgstr "Kein solches Remote-Repository: %s" +msgstr "keine solche Remote-Referenz %s" #: fetch-pack.c:1150 #, c-format msgid "Server does not allow request for unadvertised object %s" -msgstr "" +msgstr "Der Server erlaubt keine Anfrage für nicht angebotenes Objekt %s." #: gpg-interface.c:185 msgid "gpg failed to sign the data" @@ -1961,31 +1963,31 @@ msgstr "" #: ident.c:367 msgid "no email was given and auto-detection is disabled" -msgstr "" +msgstr "keine E-Mail angegeben und automatische Erkennung ist deaktiviert" #: ident.c:372 -#, fuzzy, c-format +#, c-format msgid "unable to auto-detect email address (got '%s')" -msgstr "Fehler: konnte keine gültige Adresse aus %s extrahieren\n" +msgstr "Konnte die E-Mail-Adresse nicht automatisch erkennen ('%s' erhalten)" #: ident.c:382 msgid "no name was given and auto-detection is disabled" -msgstr "" +msgstr "kein Name angegeben und automatische Erkennung ist deaktiviert" #: ident.c:388 -#, fuzzy, c-format +#, c-format msgid "unable to auto-detect name (got '%s')" -msgstr "konnte \"Tree\"-Objekt (%s) nicht lesen" +msgstr "konnte Namen nicht automatisch erkennen ('%s' erhalten)" #: ident.c:396 #, c-format msgid "empty ident name (for <%s>) not allowed" -msgstr "" +msgstr "Leerer Name in Identifikation (für <%s>) nicht erlaubt." #: ident.c:402 #, c-format msgid "name consists only of disallowed characters: %s" -msgstr "" +msgstr "Name besteht nur aus nicht erlaubten Zeichen: %s" #: ident.c:417 builtin/commit.c:611 #, c-format @@ -2102,13 +2104,11 @@ msgstr "" "im Arbeitsbereich gelassen." #: merge-recursive.c:1097 -#, fuzzy, c-format +#, c-format msgid "" "CONFLICT (%s/delete): %s deleted in %s and %s to %s in %s. Version %s of %s " "left in tree." -msgstr "" -"KONFLIKT (%s/löschen): %s gelöscht in %s und %s in %s. Stand %s von %s wurde " -"im Arbeitsbereich gelassen." +msgstr "KONFLIKT (%s/löschen): %s gelöscht in %s und %s nach %s in %s. Stand %s von %s wurde im Arbeitsbereich gelassen." #: merge-recursive.c:1104 #, c-format @@ -2120,13 +2120,11 @@ msgstr "" "im Arbeitsbereich bei %s gelassen." #: merge-recursive.c:1109 -#, fuzzy, c-format +#, c-format msgid "" "CONFLICT (%s/delete): %s deleted in %s and %s to %s in %s. Version %s of %s " "left in tree at %s." -msgstr "" -"KONFLIKT (%s/löschen): %s gelöscht in %s und %s in %s. Stand %s von %s wurde " -"im Arbeitsbereich bei %s gelassen." +msgstr "KONFLIKT (%s/löschen): %s gelöscht in %s und %s nach %s in %s. Stand %s von %s wurde im Arbeitsbereich bei %s gelassen." #: merge-recu
Re: [PATCH v2] l10n: de.po: update German translation
2017-05-01 14:19 GMT+02:00 Simon Ruderich : > On Sun, Apr 30, 2017 at 09:11:49PM +0200, Ralf Thielow wrote: >> #: config.c:1952 >> #, c-format >> msgid "unknown core.untrackedCache value '%s'; using 'keep' default value" >> -msgstr "" >> +msgstr "Unbekannter Wert '%s' in core.untrackedCache; benutze Stardardwert >> 'keep'" > > Standardwert > >> #: entry.c:280 >> -#, fuzzy, c-format >> +#, c-format >> msgid "could not stat file '%s'" >> -msgstr "konnte Datei '%s' nicht erstellen" >> +msgstr "konnte Datei '%s' nicht lesen" > > Read is not quite stat (there are situations where you can stat > but not read a file), but I can't think of a better translation. > >> #: builtin/describe.c:551 >> -#, fuzzy >> msgid "--broken is incompatible with commit-ishes" >> -msgstr "Die Option --dirty kann nicht mit Commits verwendet werden." >> +msgstr "Die Option --broken kann nicht nit Commits verwendet werden." > ^^^ > mit > >> #: builtin/tag.c:312 >> msgid "tag: tagging " >> -msgstr "" >> +msgstr "Tag: tagge " > > Lowercase Tag because it's used as command prefix? In other > places in this patch the lowercase version was used for commands. > > Regards > Simon > -- Thanks! > + privacy is necessary > + using gnupg http://gnupg.org > + public key id: 0x92FEFDB7E44C32F9
Re: [PATCH v2 1/1] t0027: tests are not expensive; remove t0025
Hi Bögi, On Tue, 2 May 2017, tbo...@web.de wrote: > From: Torsten Bögershausen > > The purpose of t0027 is to test all CRLF related conversions at > "git checkout" and "git add". > > Running t0027 under Git for Windows takes 3-4 minutes, so the whole script > had been marked as "EXPENSIVE". > > The source code for "Git for Windows" overrides this since 2014: > "t0027 is marked expensive, but really, for MinGW we want to run these > tests always." Indeed. > Recent "stress" tests show that t0025 if flaky, reported by Lars Schneider, > larsxschnei...@gmail.com > > All tests from t0025 are covered in t0027 already, so that t0025 can be > retiered: s/retiered/retired/ > The execution time for t0027 is 14 seconds under Linux, > and 63 seconds under Mac Os X. > And in case you ask, things are not going significantly faster using a SSD > instead of a spinning disk. > > Signed-off-by: Torsten Bögershausen Thank you for this patch. Apart from the tyop, would it be possible to fix the formatting to look less strange? (Unless you use this to transport a super-secret message steganographically to an alien planet or some such, of course.) Ciao, Dscho
Re: PCRE v2 compile error, was Re: What's cooking in git.git (May 2017, #01; Mon, 1)
Hi Ævar, On Tue, 2 May 2017, Ævar Arnfjörð Bjarmason wrote: > On Tue, May 2, 2017 at 2:09 PM, Johannes Schindelin > wrote: > > > > On Sun, 30 Apr 2017, Junio C Hamano wrote: > > > >> * ab/grep-pcre-v2 (2017-04-25) 20 commits > >> - SQUASH??? > >> - Makefile & configure: make PCRE v2 the default PCRE implementation > >> - grep: remove support for concurrent use of both PCRE v1 & v2 > >> - grep: add support for PCRE v2 > >> - grep: add support for the PCRE v1 JIT API > >> - perf: add a performance comparison test of grep -E and -P > >> - grep: change the internal PCRE code & header names to be PCRE1 > >> - grep: change the internal PCRE macro names to be PCRE1 > >> - test-lib: rename the LIBPCRE prerequisite to PCRE > >> - grep: make grep.patternType=[pcre|pcre1] a synonym for "perl" > >> - grep & rev-list doc: stop promising libpcre for --perl-regexp > >> - log: add -P as a synonym for --perl-regexp > >> - log: add exhaustive tests for pattern style options & config > >> - grep: add a test for backreferences in PCRE patterns > >> - Makefile & configure: reword outdated comment about PCRE > >> - grep: remove redundant `regflags &= ~REG_EXTENDED` assignments > >> - grep: remove redundant regflags assignment under PCRE > >> - grep: submodule-related case statements should die if new fields are > >> added > >> - grep: add tests for grep pattern types being passed to submodules > >> - grep: amend submodule recursion test in preparation for rx engine > >> testing > >> > >> PCRE2, which has an API different from and incompatible with PCRE, > >> can now be chosen to support "grep -P -e ''" and friends. > > > > FWIW for quite a couple of recent builds, `pu` fails on Windows with a > > variation of this error: > > > > CC blob.o > > In file included from revision.h:5:0, > > from bisect.c:4: > > grep.h:16:19: fatal error: pcre2.h: No such file or directory > > #include > >^ > > compilation terminated. > > > > Maybe this can be fixed before hitting `next`? > > This will be due to a combination of the build machine not having pcre > v2 (but having v1) & my "Makefile & configure: make PCRE v2 the > default PCRE implementation" patch, which makes v2 the default for > USE_LIBPCRE=YesPlease. > > Is it easy to install v2 on these build machines? Alternatively that > patch could be ejected out of pu, or you could USE_LIBPCRE1=YesPlease > to use v1, but as explained in that commit I think it makes sense to > make v2 the default. Let me put it this way: Installing PCRE v1 in MSYS2 is as easy as pacman -Sy mingw-w64-x86_64-pcre To install PCRE v2, you would have to copy-edit https://github.com/Alexpux/MINGW-packages/blob/master/mingw-w64-pcre/PKGBUILD, make sure that it builds by running it through makepkg-mingw -s possibly initializing a Git repository in the mingw-w64-pcre/src/${_realname}-${pkgver}/ directory, patching the source until it builds, committing the changes, adding them as patch files to the same directory as the new PKGBUILD, adjusting the PKGBUILD file to list the patch files with their checksums and to add the commands to apply them. Then (and this is a one time cost, fortunately) initializing two packages on BinTray (which we use to serve the Pacman packages of Git for Windows), then build and upload the packages. In short, PCRE v2 would be slightly (ahem, ahem) more involved than PCRE v1. I cannot imagine that MSYS2 is the only environment with that issue. Ciao, Dscho
[PATCH v3 23/25] name-rev: avoid leaking memory in the `deref` case
When the `name_rev()` function is asked to dereference the tip name, it allocates memory. But when it turns out that another tip already described the commit better than the current one, we forgot to release the memory. Pointed out by Coverity. Signed-off-by: Johannes Schindelin --- builtin/name-rev.c | 7 +-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/builtin/name-rev.c b/builtin/name-rev.c index 92a5d8a5d26..e7a3fe7ee70 100644 --- a/builtin/name-rev.c +++ b/builtin/name-rev.c @@ -28,6 +28,7 @@ static void name_rev(struct commit *commit, struct rev_name *name = (struct rev_name *)commit->util; struct commit_list *parents; int parent_number = 1; + char *to_free = NULL; parse_commit(commit); @@ -35,7 +36,7 @@ static void name_rev(struct commit *commit, return; if (deref) { - tip_name = xstrfmt("%s^0", tip_name); + tip_name = to_free = xstrfmt("%s^0", tip_name); if (generation) die("generation: %d, but deref?", generation); @@ -53,8 +54,10 @@ static void name_rev(struct commit *commit, name->taggerdate = taggerdate; name->generation = generation; name->distance = distance; - } else + } else { + free(to_free); return; + } for (parents = commit->parents; parents; -- 2.12.2.windows.2.800.gede8f145e06
[PATCH v3 25/25] submodule_uses_worktrees(): plug memory leak
There is really no reason why we would need to hold onto the allocated string longer than necessary. Reported by Coverity. Signed-off-by: Johannes Schindelin --- worktree.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/worktree.c b/worktree.c index bae787cf8d7..89a81b13de3 100644 --- a/worktree.c +++ b/worktree.c @@ -399,6 +399,7 @@ int submodule_uses_worktrees(const char *path) /* The env would be set for the superproject. */ get_common_dir_noenv(&sb, submodule_gitdir); + free(submodule_gitdir); /* * The check below is only known to be good for repository format @@ -418,7 +419,6 @@ int submodule_uses_worktrees(const char *path) /* See if there is any file inside the worktrees directory. */ dir = opendir(sb.buf); strbuf_release(&sb); - free(submodule_gitdir); if (!dir) return 0; -- 2.12.2.windows.2.800.gede8f145e06
[PATCH v3 19/25] line-log: avoid memory leak
Discovered by Coverity. Signed-off-by: Johannes Schindelin --- line-log.c | 1 + 1 file changed, 1 insertion(+) diff --git a/line-log.c b/line-log.c index a23b910471b..b9087814b8c 100644 --- a/line-log.c +++ b/line-log.c @@ -1125,6 +1125,7 @@ static int process_ranges_ordinary_commit(struct rev_info *rev, struct commit *c changed = process_all_files(&parent_range, rev, &queue, range); if (parent) add_line_range(rev, parent, parent_range); + free_line_log_data(parent_range); return changed; } -- 2.12.2.windows.2.800.gede8f145e06
[PATCH v3 24/25] show_worktree(): plug memory leak
The buffer allocated by shorten_unambiguous_ref() needs to be released. Discovered by Coverity. Signed-off-by: Johannes Schindelin --- builtin/worktree.c | 8 +--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/builtin/worktree.c b/builtin/worktree.c index 1722a9bdc2a..ff5dfd2b102 100644 --- a/builtin/worktree.c +++ b/builtin/worktree.c @@ -414,9 +414,11 @@ static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len) find_unique_abbrev(wt->head_sha1, DEFAULT_ABBREV)); if (wt->is_detached) strbuf_addstr(&sb, "(detached HEAD)"); - else if (wt->head_ref) - strbuf_addf(&sb, "[%s]", shorten_unambiguous_ref(wt->head_ref, 0)); - else + else if (wt->head_ref) { + char *ref = shorten_unambiguous_ref(wt->head_ref, 0); + strbuf_addf(&sb, "[%s]", ref); + free(ref); + } else strbuf_addstr(&sb, "(error)"); } printf("%s\n", sb.buf); -- 2.12.2.windows.2.800.gede8f145e06
[PATCH v3 20/25] shallow: avoid memory leak
Reported by Coverity. Signed-off-by: Johannes Schindelin --- shallow.c | 8 ++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/shallow.c b/shallow.c index 25b6db989bf..f9370961f99 100644 --- a/shallow.c +++ b/shallow.c @@ -473,11 +473,15 @@ static void paint_down(struct paint_info *info, const unsigned char *sha1, struct commit_list *head = NULL; int bitmap_nr = (info->nr_bits + 31) / 32; size_t bitmap_size = st_mult(sizeof(uint32_t), bitmap_nr); - uint32_t *tmp = xmalloc(bitmap_size); /* to be freed before return */ - uint32_t *bitmap = paint_alloc(info); struct commit *c = lookup_commit_reference_gently(sha1, 1); + uint32_t *tmp; /* to be freed before return */ + uint32_t *bitmap; + if (!c) return; + + tmp = xmalloc(bitmap_size); + bitmap = paint_alloc(info); memset(bitmap, 0, bitmap_size); bitmap[id / 32] |= (1U << (id % 32)); commit_list_insert(c, &head); -- 2.12.2.windows.2.800.gede8f145e06
[PATCH v3 21/25] add_reflog_for_walk: avoid memory leak
We free()d the `log` buffer when dwim_log() returned 1, but not when it returned a larger value (which meant that it still allocated the buffer but we simply ignored it). While in the vicinity, make sure that the `reflogs` structure as well as the `branch` variable are released properly, too. Identified by Coverity. Signed-off-by: Johannes Schindelin --- reflog-walk.c | 20 +--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/reflog-walk.c b/reflog-walk.c index 99679f58255..c63eb1a3fd7 100644 --- a/reflog-walk.c +++ b/reflog-walk.c @@ -183,7 +183,11 @@ int add_reflog_for_walk(struct reflog_walk_info *info, if (!reflogs || reflogs->nr == 0) { struct object_id oid; char *b; - if (dwim_log(branch, strlen(branch), oid.hash, &b) == 1) { + int ret = dwim_log(branch, strlen(branch), + oid.hash, &b); + if (ret > 1) + free(b); + else if (ret == 1) { if (reflogs) { free(reflogs->ref); free(reflogs); @@ -193,17 +197,27 @@ int add_reflog_for_walk(struct reflog_walk_info *info, reflogs = read_complete_reflog(branch); } } - if (!reflogs || reflogs->nr == 0) + if (!reflogs || reflogs->nr == 0) { + if (reflogs) { + free(reflogs->ref); + free(reflogs); + } + free(branch); return -1; + } string_list_insert(&info->complete_reflogs, branch)->util = reflogs; } + free(branch); commit_reflog = xcalloc(1, sizeof(struct commit_reflog)); if (recno < 0) { commit_reflog->recno = get_reflog_recno_by_time(reflogs, timestamp); if (commit_reflog->recno < 0) { - free(branch); + if (reflogs) { + free(reflogs->ref); + free(reflogs); + } free(commit_reflog); return -1; } -- 2.12.2.windows.2.800.gede8f145e06
[PATCH v3 22/25] remote: plug memory leak in match_explicit()
The `guess_ref()` returns an allocated buffer of which `make_linked_ref()` does not take custody (`alloc_ref()` makes a copy), therefore we need to release the buffer afterwards. Noticed via Coverity. Signed-off-by: Johannes Schindelin --- remote.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/remote.c b/remote.c index 801137c72eb..72b4591b983 100644 --- a/remote.c +++ b/remote.c @@ -1191,9 +1191,10 @@ static int match_explicit(struct ref *src, struct ref *dst, else if (is_null_oid(&matched_src->new_oid)) error("unable to delete '%s': remote ref does not exist", dst_value); - else if ((dst_guess = guess_ref(dst_value, matched_src))) + else if ((dst_guess = guess_ref(dst_value, matched_src))) { matched_dst = make_linked_ref(dst_guess, dst_tail); - else + free(dst_guess); + } else error("unable to push to unqualified destination: %s\n" "The destination refspec neither matches an " "existing ref on the remote nor\n" -- 2.12.2.windows.2.800.gede8f145e06
[PATCH v3 18/25] receive-pack: plug memory leak in update()
Reported via Coverity. Signed-off-by: Johannes Schindelin --- builtin/receive-pack.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index f96834f42c9..48c07ddb659 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -986,7 +986,8 @@ static const char *update(struct command *cmd, struct shallow_info *si) { const char *name = cmd->ref_name; struct strbuf namespaced_name_buf = STRBUF_INIT; - const char *namespaced_name, *ret; + static char *namespaced_name; + const char *ret; struct object_id *old_oid = &cmd->old_oid; struct object_id *new_oid = &cmd->new_oid; @@ -997,6 +998,7 @@ static const char *update(struct command *cmd, struct shallow_info *si) } strbuf_addf(&namespaced_name_buf, "%s%s", get_git_namespace(), name); + free(namespaced_name); namespaced_name = strbuf_detach(&namespaced_name_buf, NULL); if (is_ref_checked_out(namespaced_name)) { -- 2.12.2.windows.2.800.gede8f145e06
[PATCH v3 10/25] cat-file: fix memory leak
Discovered by Coverity. Signed-off-by: Johannes Schindelin --- builtin/cat-file.c | 1 + 1 file changed, 1 insertion(+) diff --git a/builtin/cat-file.c b/builtin/cat-file.c index 1890d7a6390..9af863e7915 100644 --- a/builtin/cat-file.c +++ b/builtin/cat-file.c @@ -165,6 +165,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name, die("git cat-file %s: bad file", obj_name); write_or_die(1, buf, size); + free(buf); return 0; } -- 2.12.2.windows.2.800.gede8f145e06
[PATCH v3 12/25] split_commit_in_progress(): fix memory leak
Reported via Coverity. Signed-off-by: Johannes Schindelin --- wt-status.c | 7 ++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/wt-status.c b/wt-status.c index 0a6e16dbe0f..1f3f6bcb980 100644 --- a/wt-status.c +++ b/wt-status.c @@ -1088,8 +1088,13 @@ static int split_commit_in_progress(struct wt_status *s) char *rebase_orig_head = read_line_from_git_path("rebase-merge/orig-head"); if (!head || !orig_head || !rebase_amend || !rebase_orig_head || - !s->branch || strcmp(s->branch, "HEAD")) + !s->branch || strcmp(s->branch, "HEAD")) { + free(head); + free(orig_head); + free(rebase_amend); + free(rebase_orig_head); return split_in_progress; + } if (!strcmp(rebase_amend, rebase_orig_head)) { if (strcmp(head, rebase_amend)) -- 2.12.2.windows.2.800.gede8f145e06
[PATCH v3 14/25] setup_discovered_git_dir(): plug memory leak
The setup_explicit_git_dir() function does not take custody of the string passed as first parameter; we have to release it if we turned the value of git_dir into an absolute path. Signed-off-by: Johannes Schindelin --- setup.c | 9 +++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/setup.c b/setup.c index 0320a9ad14c..e3f7699a902 100644 --- a/setup.c +++ b/setup.c @@ -703,11 +703,16 @@ static const char *setup_discovered_git_dir(const char *gitdir, /* --work-tree is set without --git-dir; use discovered one */ if (getenv(GIT_WORK_TREE_ENVIRONMENT) || git_work_tree_cfg) { + char *to_free = NULL; + const char *ret; + if (offset != cwd->len && !is_absolute_path(gitdir)) - gitdir = real_pathdup(gitdir, 1); + gitdir = to_free = real_pathdup(gitdir, 1); if (chdir(cwd->buf)) die_errno("Could not come back to cwd"); - return setup_explicit_git_dir(gitdir, cwd, nongit_ok); + ret = setup_explicit_git_dir(gitdir, cwd, nongit_ok); + free(to_free); + return ret; } /* #16.2, #17.2, #20.2, #21.2, #24, #25, #28, #29 (see t1510) */ -- 2.12.2.windows.2.800.gede8f145e06
[PATCH v3 15/25] pack-redundant: plug memory leak
Identified via Coverity. Signed-off-by: Johannes Schindelin --- builtin/pack-redundant.c | 1 + 1 file changed, 1 insertion(+) diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c index 72c815844dd..cb1df1c7614 100644 --- a/builtin/pack-redundant.c +++ b/builtin/pack-redundant.c @@ -442,6 +442,7 @@ static void minimize(struct pack_list **min) /* return if there are no objects missing from the unique set */ if (missing->size == 0) { *min = unique; + free(missing); return; } -- 2.12.2.windows.2.800.gede8f145e06
[PATCH v3 17/25] fast-export: avoid leaking memory in handle_tag()
Reported by, you guessed it, Coverity. Signed-off-by: Johannes Schindelin --- builtin/fast-export.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/builtin/fast-export.c b/builtin/fast-export.c index e0220630d00..64617ad8e36 100644 --- a/builtin/fast-export.c +++ b/builtin/fast-export.c @@ -734,6 +734,7 @@ static void handle_tag(const char *name, struct tag *tag) oid_to_hex(&tag->object.oid)); case DROP: /* Ignore this tag altogether */ + free(buf); return; case REWRITE: if (tagged->type != OBJ_COMMIT) { @@ -765,6 +766,7 @@ static void handle_tag(const char *name, struct tag *tag) (int)(tagger_end - tagger), tagger, tagger == tagger_end ? "" : "\n", (int)message_size, (int)message_size, message ? message : ""); + free(buf); } static struct commit *get_commit(struct rev_cmdline_entry *e, char *full_name) -- 2.12.2.windows.2.800.gede8f145e06
[PATCH v3 11/25] checkout: fix memory leak
This change addresses part of the NEEDSWORK comment above the code, therefore the comment needs to be adjusted, too. Discovered via Coverity. Signed-off-by: Johannes Schindelin --- builtin/checkout.c | 17 + 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/builtin/checkout.c b/builtin/checkout.c index bfa5419f335..5faea3a05fa 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -235,14 +235,14 @@ static int checkout_merged(int pos, const struct checkout *state) /* * NEEDSWORK: * There is absolutely no reason to write this as a blob object -* and create a phony cache entry just to leak. This hack is -* primarily to get to the write_entry() machinery that massages -* the contents to work-tree format and writes out which only -* allows it for a cache entry. The code in write_entry() needs -* to be refactored to allow us to feed a -* instead of a cache entry. Such a refactoring would help -* merge_recursive as well (it also writes the merge result to the -* object database even when it may contain conflicts). +* and create a phony cache entry. This hack is primarily to get +* to the write_entry() machinery that massages the contents to +* work-tree format and writes out which only allows it for a +* cache entry. The code in write_entry() needs to be refactored +* to allow us to feed a instead of a cache +* entry. Such a refactoring would help merge_recursive as well +* (it also writes the merge result to the object database even +* when it may contain conflicts). */ if (write_sha1_file(result_buf.ptr, result_buf.size, blob_type, oid.hash)) @@ -251,6 +251,7 @@ static int checkout_merged(int pos, const struct checkout *state) if (!ce) die(_("make_cache_entry failed for path '%s'"), path); status = checkout_entry(ce, state, NULL); + free(ce); return status; } -- 2.12.2.windows.2.800.gede8f145e06
[PATCH v3 16/25] mktree: plug memory leaks reported by Coverity
Signed-off-by: Johannes Schindelin --- builtin/mktree.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/builtin/mktree.c b/builtin/mktree.c index de9b40fc63b..da0fd8cd706 100644 --- a/builtin/mktree.c +++ b/builtin/mktree.c @@ -72,7 +72,7 @@ static void mktree_line(char *buf, size_t len, int nul_term_line, int allow_miss unsigned mode; enum object_type mode_type; /* object type derived from mode */ enum object_type obj_type; /* object type derived from sha */ - char *path; + char *path, *to_free = NULL; unsigned char sha1[20]; ptr = buf; @@ -102,7 +102,7 @@ static void mktree_line(char *buf, size_t len, int nul_term_line, int allow_miss struct strbuf p_uq = STRBUF_INIT; if (unquote_c_style(&p_uq, path, NULL)) die("invalid quoting"); - path = strbuf_detach(&p_uq, NULL); + path = to_free = strbuf_detach(&p_uq, NULL); } /* @@ -136,6 +136,7 @@ static void mktree_line(char *buf, size_t len, int nul_term_line, int allow_miss } append_to_tree(mode, sha1, path); + free(to_free); } int cmd_mktree(int ac, const char **av, const char *prefix) -- 2.12.2.windows.2.800.gede8f145e06
[PATCH v3 13/25] setup_bare_git_dir(): help static analysis
Coverity reported a memory leak in this function. However, it can only be called once, as setup_git_directory() changes global state and hence is not reentrant. Mark the variable as static to indicate that this is a singleton. Signed-off-by: Johannes Schindelin --- setup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/setup.c b/setup.c index 0309c278218..0320a9ad14c 100644 --- a/setup.c +++ b/setup.c @@ -748,7 +748,7 @@ static const char *setup_bare_git_dir(struct strbuf *cwd, int offset, /* --work-tree is set without --git-dir; use discovered one */ if (getenv(GIT_WORK_TREE_ENVIRONMENT) || git_work_tree_cfg) { - const char *gitdir; + static const char *gitdir; gitdir = offset == cwd->len ? "." : xmemdupz(cwd->buf, offset); if (chdir(cwd->buf)) -- 2.12.2.windows.2.800.gede8f145e06
[PATCH v3 09/25] mailinfo & mailsplit: check for EOF while parsing
While POSIX states that it is okay to pass EOF to isspace() (and it seems to be implied that EOF should *not* be treated as whitespace), and also to pass EOF to ungetc() (which seems to be intended to fail without buffering the character), it is much better to handle these cases explicitly. Not only does it reduce head-scratching (and helps static analysis avoid reporting false positives), it also lets us handle files containing nothing but whitespace by erroring out. Reported via Coverity. Signed-off-by: Johannes Schindelin --- builtin/mailsplit.c | 10 ++ mailinfo.c | 9 - 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c index 30681681c13..664400b8169 100644 --- a/builtin/mailsplit.c +++ b/builtin/mailsplit.c @@ -232,6 +232,16 @@ static int split_mbox(const char *file, const char *dir, int allow_bare, do { peek = fgetc(f); + if (peek == EOF) { + if (f == stdin) + /* empty stdin is OK */ + ret = skip; + else { + fclose(f); + error(_("empty mbox: '%s'"), file); + } + goto out; + } } while (isspace(peek)); ungetc(peek, f); diff --git a/mailinfo.c b/mailinfo.c index 68037758f2f..f92cb9f729c 100644 --- a/mailinfo.c +++ b/mailinfo.c @@ -882,7 +882,10 @@ static int read_one_header_line(struct strbuf *line, FILE *in) for (;;) { int peek; - peek = fgetc(in); ungetc(peek, in); + peek = fgetc(in); + if (peek == EOF) + break; + ungetc(peek, in); if (peek != ' ' && peek != '\t') break; if (strbuf_getline_lf(&continuation, in)) @@ -1099,6 +1102,10 @@ int mailinfo(struct mailinfo *mi, const char *msg, const char *patch) do { peek = fgetc(mi->input); + if (peek == EOF) { + fclose(cmitmsg); + return error("empty patch: '%s'", patch); + } } while (isspace(peek)); ungetc(peek, mi->input); -- 2.12.2.windows.2.800.gede8f145e06
[PATCH v3 05/25] git_config_rename_section_in_file(): avoid resource leak
In case of errors, we really want the file descriptor to be closed. Discovered by a Coverity scan. Signed-off-by: Johannes Schindelin --- config.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/config.c b/config.c index b4a3205da32..a30056ec7e9 100644 --- a/config.c +++ b/config.c @@ -2621,7 +2621,7 @@ int git_config_rename_section_in_file(const char *config_filename, struct lock_file *lock; int out_fd; char buf[1024]; - FILE *config_file; + FILE *config_file = NULL; struct stat st; if (new_name && !section_name_is_ok(new_name)) { @@ -2703,11 +2703,14 @@ int git_config_rename_section_in_file(const char *config_filename, } } fclose(config_file); + config_file = NULL; commit_and_out: if (commit_lock_file(lock) < 0) ret = error_errno("could not write config file %s", config_filename); out: + if (config_file) + fclose(config_file); rollback_lock_file(lock); out_no_rollback: free(filename_buf); -- 2.12.2.windows.2.800.gede8f145e06
[PATCH v3 08/25] status: close file descriptor after reading git-rebase-todo
Reported via Coverity. Signed-off-by: Johannes Schindelin --- wt-status.c | 1 + 1 file changed, 1 insertion(+) diff --git a/wt-status.c b/wt-status.c index 03754849626..0a6e16dbe0f 100644 --- a/wt-status.c +++ b/wt-status.c @@ -1168,6 +1168,7 @@ static int read_rebase_todolist(const char *fname, struct string_list *lines) abbrev_sha1_in_line(&line); string_list_append(lines, line.buf); } + fclose(f); return 0; } -- 2.12.2.windows.2.800.gede8f145e06
[PATCH v3 07/25] difftool: address a couple of resource/memory leaks
This change plugs a couple of memory leaks and makes sure that the file descriptor is closed in run_dir_diff(). Spotted by Coverity. Signed-off-by: Johannes Schindelin --- builtin/difftool.c | 33 +++-- 1 file changed, 23 insertions(+), 10 deletions(-) diff --git a/builtin/difftool.c b/builtin/difftool.c index 1354d0e4625..b9a892f2693 100644 --- a/builtin/difftool.c +++ b/builtin/difftool.c @@ -226,6 +226,7 @@ static void changed_files(struct hashmap *result, const char *index_path, hashmap_entry_init(entry, strhash(buf.buf)); hashmap_add(result, entry); } + fclose(fp); if (finish_command(&diff_files)) die("diff-files did not exit properly"); strbuf_release(&index_env); @@ -439,8 +440,10 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix, } if (lmode && status != 'C') { - if (checkout_path(lmode, &loid, src_path, &lstate)) - return error("could not write '%s'", src_path); + if (checkout_path(lmode, &loid, src_path, &lstate)) { + ret = error("could not write '%s'", src_path); + goto finish; + } } if (rmode && !S_ISLNK(rmode)) { @@ -456,9 +459,12 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix, hashmap_add(&working_tree_dups, entry); if (!use_wt_file(workdir, dst_path, &roid)) { - if (checkout_path(rmode, &roid, dst_path, &rstate)) - return error("could not write '%s'", -dst_path); + if (checkout_path(rmode, &roid, dst_path, + &rstate)) { + ret = error("could not write '%s'", + dst_path); + goto finish; + } } else if (!is_null_oid(&roid)) { /* * Changes in the working tree need special @@ -473,10 +479,12 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix, ADD_CACHE_JUST_APPEND); add_path(&rdir, rdir_len, dst_path); - if (ensure_leading_directories(rdir.buf)) - return error("could not create " -"directory for '%s'", -dst_path); + if (ensure_leading_directories(rdir.buf)) { + ret = error("could not create " + "directory for '%s'", + dst_path); + goto finish; + } add_path(&wtdir, wtdir_len, dst_path); if (symlinks) { if (symlink(wtdir.buf, rdir.buf)) { @@ -497,13 +505,15 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix, } } + fclose(fp); + fp = NULL; if (finish_command(&child)) { ret = error("error occurred running diff --raw"); goto finish; } if (!i) - return 0; + goto finish; /* * Changes to submodules require special treatment.This loop writes a @@ -626,6 +636,9 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix, exit_cleanup(tmpdir, rc); finish: + if (fp) + fclose(fp); + free(lbase_dir); free(rbase_dir); strbuf_release(&ldir); -- 2.12.2.windows.2.800.gede8f145e06
[PATCH v3 06/25] get_mail_commit_oid(): avoid resource leak
When we fail to read, or parse, the file, we still want to close the file descriptor and release the strbuf. Reported via Coverity. Signed-off-by: Johannes Schindelin --- builtin/am.c | 15 ++- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/builtin/am.c b/builtin/am.c index a95dd8b4e6c..9c5c2c778e2 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -1351,19 +1351,16 @@ static int get_mail_commit_oid(struct object_id *commit_id, const char *mail) struct strbuf sb = STRBUF_INIT; FILE *fp = xfopen(mail, "r"); const char *x; + int ret = 0; - if (strbuf_getline_lf(&sb, fp)) - return -1; - - if (!skip_prefix(sb.buf, "From ", &x)) - return -1; - - if (get_oid_hex(x, commit_id) < 0) - return -1; + if (strbuf_getline_lf(&sb, fp) || + !skip_prefix(sb.buf, "From ", &x) || + get_oid_hex(x, commit_id) < 0) + ret = -1; strbuf_release(&sb); fclose(fp); - return 0; + return ret; } /** -- 2.12.2.windows.2.800.gede8f145e06
[PATCH v3 04/25] add_commit_patch_id(): avoid allocating memory unnecessarily
It would appear that we allocate (and forget to release) memory if the patch ID is not even defined. Reported by the Coverity tool. Signed-off-by: Johannes Schindelin --- patch-ids.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/patch-ids.c b/patch-ids.c index fa8f11de826..92eba7a059e 100644 --- a/patch-ids.c +++ b/patch-ids.c @@ -99,11 +99,12 @@ struct patch_id *has_commit_patch_id(struct commit *commit, struct patch_id *add_commit_patch_id(struct commit *commit, struct patch_ids *ids) { - struct patch_id *key = xcalloc(1, sizeof(*key)); + struct patch_id *key; if (!patch_id_defined(commit)) return NULL; + key = xcalloc(1, sizeof(*key)); if (init_patch_id_entry(key, commit, ids)) { free(key); return NULL; -- 2.12.2.windows.2.800.gede8f145e06
[PATCH v3 03/25] winansi: avoid buffer overrun
When we could not convert the UTF-8 sequence into Unicode for writing to the Console, we should not try to write an insanely-long sequence of invalid wide characters (mistaking the negative return value for an unsigned length). Reported by Coverity. Signed-off-by: Johannes Schindelin --- compat/winansi.c | 5 + 1 file changed, 5 insertions(+) diff --git a/compat/winansi.c b/compat/winansi.c index fd6910746c8..861b79d8c31 100644 --- a/compat/winansi.c +++ b/compat/winansi.c @@ -135,6 +135,11 @@ static void write_console(unsigned char *str, size_t len) /* convert utf-8 to utf-16 */ int wlen = xutftowcsn(wbuf, (char*) str, ARRAY_SIZE(wbuf), len); + if (wlen < 0) { + wchar_t *err = L"[invalid]"; + WriteConsoleW(console, err, wcslen(err), &dummy, NULL); + return; + } /* write directly to console */ WriteConsoleW(console, wbuf, wlen, &dummy, NULL); -- 2.12.2.windows.2.800.gede8f145e06
[PATCH v3 02/25] winansi: avoid use of uninitialized value
When stdout is not connected to a Win32 console, we incorrectly used an uninitialized value for the "plain" character attributes. Detected by Coverity. Signed-off-by: Johannes Schindelin --- compat/winansi.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/compat/winansi.c b/compat/winansi.c index 793420f9d0d..fd6910746c8 100644 --- a/compat/winansi.c +++ b/compat/winansi.c @@ -105,6 +105,8 @@ static int is_console(int fd) if (!fd) { if (!GetConsoleMode(hcon, &mode)) return 0; + sbi.wAttributes = FOREGROUND_BLUE | FOREGROUND_GREEN | + FOREGROUND_RED; } else if (!GetConsoleScreenBufferInfo(hcon, &sbi)) return 0; -- 2.12.2.windows.2.800.gede8f145e06
[PATCH v3 00/25] Address a couple of issues identified by Coverity
I recently registered the git-for-windows fork with Coverity to ensure that even the Windows-specific patches get some static analysis love. While at it, I squashed a couple of obvious issues in the part that is not Windows-specific. Note: while this patch series squashes some of those issues, the remaining issues are not necessarily all false positives (e.g. Coverity getting fooled by our FLEX_ARRAY trick into believing that the last member of the struct is indeed a 0 or 1 size array) or intentional (e.g. builtins not releasing memory because exit() is called right after returning from the function that leaks memory). Notable examples of the remaining issues are: - a couple of callers of shorten_unambiguous_ref() assume that they do not have to release the memory returned from that function, often assigning the pointer to a `const` variable that typically does not hold a pointer that needs to be free()d. My hunch is that we will want to convert that function to have a fixed number of static buffers and use those in a round robin fashion à la sha1_to_hex(). - pack-redundant.c seems to have hard-to-reason-about code paths that may eventually leak memory. Essentially, the custody of the allocated memory is not clear at all. - fast-import.c calls strbuf_detach() on the command_buf without any obvious rationale. Turns out that *some* code paths assign command_buf.buf to a `recent_command` which releases the buffer later. However, from a cursory look it appears to me as if there are some other code paths that skip that assignment, effectively causing a memory leak once strbuf_detach() is called. Sadly, I lack the time to pursue those remaining issues further. Changes since v2: - renamed the `p` variables introduced by the patch series to the more explanatory `to_free`. - reworded incorrect commit message claiming that setup_discovered_git_dir() was using a static variable that is actually a singleton - reverted a code move that would have resulted in accessing uninitialized data of callers of mailinfo() that do not die() right away but clean up faithfully Johannes Schindelin (25): mingw: avoid memory leak when splitting PATH winansi: avoid use of uninitialized value winansi: avoid buffer overrun add_commit_patch_id(): avoid allocating memory unnecessarily git_config_rename_section_in_file(): avoid resource leak get_mail_commit_oid(): avoid resource leak difftool: address a couple of resource/memory leaks status: close file descriptor after reading git-rebase-todo mailinfo & mailsplit: check for EOF while parsing cat-file: fix memory leak checkout: fix memory leak split_commit_in_progress(): fix memory leak setup_bare_git_dir(): help static analysis setup_discovered_git_dir(): plug memory leak pack-redundant: plug memory leak mktree: plug memory leaks reported by Coverity fast-export: avoid leaking memory in handle_tag() receive-pack: plug memory leak in update() line-log: avoid memory leak shallow: avoid memory leak add_reflog_for_walk: avoid memory leak remote: plug memory leak in match_explicit() name-rev: avoid leaking memory in the `deref` case show_worktree(): plug memory leak submodule_uses_worktrees(): plug memory leak builtin/am.c | 15 ++- builtin/cat-file.c | 1 + builtin/checkout.c | 17 + builtin/difftool.c | 33 +++-- builtin/fast-export.c| 2 ++ builtin/mailsplit.c | 10 ++ builtin/mktree.c | 5 +++-- builtin/name-rev.c | 7 +-- builtin/pack-redundant.c | 1 + builtin/receive-pack.c | 4 +++- builtin/worktree.c | 8 +--- compat/mingw.c | 4 +++- compat/winansi.c | 7 +++ config.c | 5 - line-log.c | 1 + mailinfo.c | 9 - patch-ids.c | 3 ++- reflog-walk.c| 20 +--- remote.c | 5 +++-- setup.c | 11 --- shallow.c| 8 ++-- worktree.c | 2 +- wt-status.c | 8 +++- 23 files changed, 135 insertions(+), 51 deletions(-) base-commit: d2bbb7c2bcf6e77ebfcabf4e12110fe6d5c91de6 Published-As: https://github.com/dscho/git/releases/tag/coverity-v3 Fetch-It-Via: git fetch https://github.com/dscho/git coverity-v3 Interdiff vs v2: diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c index 9b3efc8e987..664400b8169 100644 --- a/builtin/mailsplit.c +++ b/builtin/mailsplit.c @@ -232,9 +232,18 @@ static int split_mbox(const char *file, const char *dir, int allow_bare, do { peek = fgetc(f); + if (peek == EOF) { + if (f == stdin) + /* empty stdin is OK */ + ret = skip; + else { + fclose
[PATCH v3 01/25] mingw: avoid memory leak when splitting PATH
In the (admittedly, concocted) case that PATH consists only of colons, we would leak the duplicated string. Signed-off-by: Johannes Schindelin --- compat/mingw.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/compat/mingw.c b/compat/mingw.c index 3fbfda5978b..fe0e3ccd248 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -961,8 +961,10 @@ static char **get_path_split(void) ++n; } } - if (!n) + if (!n) { + free(envpath); return NULL; + } ALLOC_ARRAY(path, n + 1); p = envpath; -- 2.12.2.windows.2.800.gede8f145e06
[PATCH v2 1/1] t0027: tests are not expensive; remove t0025
From: Torsten Bögershausen The purpose of t0027 is to test all CRLF related conversions at "git checkout" and "git add". Running t0027 under Git for Windows takes 3-4 minutes, so the whole script had been marked as "EXPENSIVE". The source code for "Git for Windows" overrides this since 2014: "t0027 is marked expensive, but really, for MinGW we want to run these tests always." Recent "stress" tests show that t0025 if flaky, reported by Lars Schneider, larsxschnei...@gmail.com All tests from t0025 are covered in t0027 already, so that t0025 can be retiered: The execution time for t0027 is 14 seconds under Linux, and 63 seconds under Mac Os X. And in case you ask, things are not going significantly faster using a SSD instead of a spinning disk. Signed-off-by: Torsten Bögershausen --- t/t0025-crlf-auto.sh | 181 --- t/t0027-auto-crlf.sh | 6 -- 2 files changed, 187 deletions(-) delete mode 100755 t/t0025-crlf-auto.sh diff --git a/t/t0025-crlf-auto.sh b/t/t0025-crlf-auto.sh deleted file mode 100755 index 89826c5..000 --- a/t/t0025-crlf-auto.sh +++ /dev/null @@ -1,181 +0,0 @@ -#!/bin/sh - -test_description='CRLF conversion' - -. ./test-lib.sh - -has_cr() { - tr '\015' Q <"$1" | grep Q >/dev/null -} - -test_expect_success setup ' - - git config core.autocrlf false && - - for w in Hello world how are you; do echo $w; done >LFonly && - for w in I am very very fine thank you; do echo ${w}Q; done | q_to_cr >CRLFonly && - for w in Oh here is a QNUL byte how alarming; do echo ${w}; done | q_to_nul >LFwithNUL && - git add . && - - git commit -m initial && - - LFonly=$(git rev-parse HEAD:LFonly) && - CRLFonly=$(git rev-parse HEAD:CRLFonly) && - LFwithNUL=$(git rev-parse HEAD:LFwithNUL) && - - echo happy. -' - -test_expect_success 'default settings cause no changes' ' - - rm -f .gitattributes tmp LFonly CRLFonly LFwithNUL && - git read-tree --reset -u HEAD && - - ! has_cr LFonly && - has_cr CRLFonly && - LFonlydiff=$(git diff LFonly) && - CRLFonlydiff=$(git diff CRLFonly) && - LFwithNULdiff=$(git diff LFwithNUL) && - test -z "$LFonlydiff" -a -z "$CRLFonlydiff" -a -z "$LFwithNULdiff" -' - -test_expect_success 'crlf=true causes a CRLF file to be normalized' ' - - # Backwards compatibility check - rm -f .gitattributes tmp LFonly CRLFonly LFwithNUL && - echo "CRLFonly crlf" > .gitattributes && - git read-tree --reset -u HEAD && - - # Note, "normalized" means that git will normalize it if added - has_cr CRLFonly && - CRLFonlydiff=$(git diff CRLFonly) && - test -n "$CRLFonlydiff" -' - -test_expect_success 'text=true causes a CRLF file to be normalized' ' - - rm -f .gitattributes tmp LFonly CRLFonly LFwithNUL && - echo "CRLFonly text" > .gitattributes && - git read-tree --reset -u HEAD && - - # Note, "normalized" means that git will normalize it if added - has_cr CRLFonly && - CRLFonlydiff=$(git diff CRLFonly) && - test -n "$CRLFonlydiff" -' - -test_expect_success 'eol=crlf gives a normalized file CRLFs with autocrlf=false' ' - - rm -f .gitattributes tmp LFonly CRLFonly LFwithNUL && - git config core.autocrlf false && - echo "LFonly eol=crlf" > .gitattributes && - git read-tree --reset -u HEAD && - - has_cr LFonly && - LFonlydiff=$(git diff LFonly) && - test -z "$LFonlydiff" -' - -test_expect_success 'eol=crlf gives a normalized file CRLFs with autocrlf=input' ' - - rm -f .gitattributes tmp LFonly CRLFonly LFwithNUL && - git config core.autocrlf input && - echo "LFonly eol=crlf" > .gitattributes && - git read-tree --reset -u HEAD && - - has_cr LFonly && - LFonlydiff=$(git diff LFonly) && - test -z "$LFonlydiff" -' - -test_expect_success 'eol=lf gives a normalized file LFs with autocrlf=true' ' - - rm -f .gitattributes tmp LFonly CRLFonly LFwithNUL && - git config core.autocrlf true && - echo "LFonly eol=lf" > .gitattributes && - git read-tree --reset -u HEAD && - - ! has_cr LFonly && - LFonlydiff=$(git diff LFonly) && - test -z "$LFonlydiff" -' - -test_expect_success 'autocrlf=true does not normalize CRLF files' ' - - rm -f .gitattributes tmp LFonly CRLFonly LFwithNUL && - git config core.autocrlf true && - git read-tree --reset -u HEAD && - - has_cr LFonly && - has_cr CRLFonly && - LFonlydiff=$(git diff LFonly) && - CRLFonlydiff=$(git diff CRLFonly) && - LFwithNULdiff=$(git diff LFwithNUL) && - test -z "$LFonlydiff" -a -z "$CRLFonlydiff" -a -z "$LFwithNULdiff" -' - -test_expect_success 'text=auto, autocrlf=true does not normalize CRLF files' ' - - rm -f .gitattributes tmp LFonly CRLFonly LFwithNUL && - git config core.autocrlf true && - echo "* text=auto" > .gitattr
Re: Could GIT manage revision headers embedded in code ?
Hi Eric, On Tue, 2 May 2017, Delanoe Eric wrote: > Could this kind of "keyword expansion" feature be added into GIT ? Back in the days, it was decided that this should be part of the "export as zip" functionality, see the `export-subst` feature in git-archive's and gitattributes' documentation. In reality, what most projects do is to generate a header or some such as part of their build process. Git itself may serve as a case in point: https://github.com/git/git/blob/master/GIT-VERSION-GEN used in https://github.com/git/git/blob/v2.12.2/Makefile#L390-L392 Ciao, Johannes
Re: [PATCH v3 0/6] rebase -i: add config to abbreviate command-names
Hi Liam, On Tue, 2 May 2017, Liam Beguin wrote: > Add the 'rebase.abbreviateCommands' configuration option to allow `git > rebase -i` to default to the single-letter command-names in the todo > list. > > Using single-letter command-names can present two benefits. First, it > makes it easier to change the action since you only need to replace a > single character (i.e.: in vim "r" instead of > "ciw"). Second, using this with a large enough value of > 'core.abbrev' enables the lines of the todo list to remain aligned > making the files easier to read. > > Changes from v1 to v2: > - Improve Documentation and commit message > > Changes from v2 to v3: > - Transform a single patch into a series > - change option name from 'rebase.abbrevCmd' to 'rebase.abbreviateCommands' > - abbreviate all commands (not just pick) > - teach `git rebase -i --autosquash` to recognise single-letter command-names > - move rebase configuration documentation to Documentation/rebase-config.txt > - update Documentation to use the preferred naming for the todo list > - update Documentation and commit messages according to feedback Thank you for this pleasant read. It is an excellent contribution, and the way you communicate what you changed and why is very welcome. I offered a couple of comments, my biggest one probably being that this patch series is crossing paths with my patch series that tries to move more functionality out of the git-rebase--interactive.sh script into the git-rebase--helper that is written in C, closely followed by my suggestion to fold at least part of the functionality into the SHA-1 collapsing/expanding. If your patch series "wins", I can easily forward-port your changes to the rebase-i-extra branch, but it may actually make sense to build on top of the rebase-i-extra branch to begin with. If you agree: I pushed the proposed change to the `rebase-i-extra+abbrev` branch at https://github.com/dscho/git. I look forward to see this story unfold! Ciao, Johannes