git/cache-tree.c, branch v2.16.2

Merge branch 'ma/lockfile-fixes'

2017-11-06T04:11:21Z

An earlier update made it possible to use an on-stack in-core lockfile structure (as opposed to having to deliberately leak an on-heap one). Many codepaths have been updated to take advantage of this new facility. * ma/lockfile-fixes: read_cache: roll back lock in `update_index_if_able()` read-cache: leave lock in right state in `write_locked_index()` read-cache: drop explicit `CLOSE_LOCK`-flag cache.h: document `write_locked_index()` apply: remove `newfd` from `struct apply_state` apply: move lockfile into `apply_state` cache-tree: simplify locking logic checkout-index: simplify locking logic tempfile: fix documentation on `delete_tempfile()` lockfile: fix documentation on `close_lock_file_gently()` treewide: prefer lockfiles on the stack sha1_file: do not leak `lock_file`

cleanup: fix possible overflow errors in binary search

2017-10-09T23:57:24Z

A common mistake when writing binary search is to allow possible integer overflow by using the simple average: mid = (min + max) / 2; Instead, use the overflow-safe version: mid = min + (max - min) / 2; This translation is safe since the operation occurs inside a loop conditioned on "min < max". The included changes were found using the following git grep: git grep '/ *2;' '*.c' Making this cleanup will prevent future review friction when a new binary search is contructed based on existing code. Signed-off-by: Derrick Stolee Reviewed-by: Jeff King Signed-off-by: Junio C Hamano

cache-tree: simplify locking logic

2017-10-06T01:07:18Z

After we have taken the lock using `LOCK_DIE_ON_ERROR`, we know that `newfd` is non-negative. So when we check for exactly that property before calling `write_locked_index()`, the outcome is guaranteed. If we write and commit successfully, we set `newfd = -1`, so that we can later avoid calling `rollback_lock_file` on an already-committed lock. But we might just as well unconditionally call `rollback_lock_file()` -- it will be a no-op if we have already committed. All in all, we use `newfd` as a bool and the only benefit we get from it is that we can avoid calling a no-op. Remove `newfd` so that we have one variable less to reason about. Signed-off-by: Martin Ågren Signed-off-by: Junio C Hamano

stop leaking lock structs in some simple cases

2017-09-06T08:19:54Z

Now that it's safe to declare a "struct lock_file" on the stack, we can do so (and avoid an intentional leak). These leaks were found by running t0000 and t0001 under valgrind (though certainly other similar leaks exist and just don't happen to be exercised by those tests). Initializing the lock_file's inner tempfile with NULL is not strictly necessary in these cases, but it's a good practice to model. It means that if we were to call a function like rollback_lock_file() on a lock that was never taken in the first place, it becomes a quiet noop (rather than undefined behavior). Likewise, it's always safe to rollback_lock_file() on a file that has already been committed or deleted, since that operation is a noop on an inactive lockfile (and that's why the case in config.c can drop the "if (lock)" check as we move away from using a pointer). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano

write_index_as_tree: cleanup tempfile on error

2017-09-06T08:19:52Z

If we failed to write our new index file, we rollback our lockfile to remove the temporary index. But if we fail before we even get to the write step (because reading the old index failed), we leave the lockfile in place, which makes no sense. In practice this hasn't been a big deal because failing at write_index_as_tree() typically results in the whole program exiting (and thus the tempfile handler kicking in and cleaning up the files). But this function should consistently take responsibility for the resources it allocates. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano

use MOVE_ARRAY

2017-07-17T21:54:56Z

Simplify the code for moving members inside of an array and make it more robust by using the helper macro MOVE_ARRAY. It calculates the size based on the specified number of elements for us and supports NULL pointers when that number is zero. Raw memmove(3) calls with NULL can cause the compiler to (over-eagerly) optimize out later NULL checks. This patch was generated with contrib/coccinelle/array.cocci and spatch (Coccinelle). Signed-off-by: Rene Scharfe Signed-off-by: Junio C Hamano

Merge branch 'bc/object-id'

2017-05-29T03:34:43Z

Conversion from uchar[20] to struct object_id continues. * bc/object-id: (53 commits) object: convert parse_object* to take struct object_id tree: convert parse_tree_indirect to struct object_id sequencer: convert do_recursive_merge to struct object_id diff-lib: convert do_diff_cache to struct object_id builtin/ls-tree: convert to struct object_id merge: convert checkout_fast_forward to struct object_id sequencer: convert fast_forward_to to struct object_id builtin/ls-files: convert overlay_tree_on_cache to object_id builtin/read-tree: convert to struct object_id sha1_name: convert internals of peel_onion to object_id upload-pack: convert remaining parse_object callers to object_id revision: convert remaining parse_object callers to object_id revision: rename add_pending_sha1 to add_pending_oid http-push: convert process_ls_object and descendants to object_id refs/files-backend: convert many internals to struct object_id refs: convert struct ref_update to use struct object_id ref-filter: convert some static functions to struct object_id Convert struct ref_array_item to struct object_id Convert the verify_pack callback to struct object_id Convert lookup_tag to struct object_id ...

Convert lookup_tree to struct object_id

2017-05-08T06:12:57Z

Convert the lookup_tree function to take a pointer to struct object_id. The commit was created with manual changes to tree.c, tree.h, and object.c, plus the following semantic patch: @@ @@ - lookup_tree(EMPTY_TREE_SHA1_BIN) + lookup_tree(&empty_tree_oid) @@ expression E1; @@ - lookup_tree(E1.hash) + lookup_tree(&E1) @@ expression E1; @@ - lookup_tree(E1->hash) + lookup_tree(E1) Signed-off-by: brian m. carlson Signed-off-by: Junio C Hamano

Convert struct cache_tree to use struct object_id

2017-05-02T01:46:41Z

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 Signed-off-by: Junio C Hamano

cache-tree: reject entries with null sha1

2017-04-24T01:21:59Z

We generally disallow null sha1s from entering the index, due to 4337b5856 (do not write null sha1s to on-disk index, 2012-07-28). However, we loosened that in 83bd7437c (write_index: optionally allow broken null sha1s, 2013-08-27) so that tools like filter-branch could be used to repair broken history. However, we should make sure that these broken entries do not get propagated into new trees. For most entries, we'd catch them with the missing-object check (since presumably the null sha1 does not exist in our object database). But gitlink entries do not need reachability, so we may blindly copy the entry into a bogus tree. This patch rejects all null sha1s (with the same "invalid entry" message that missing objects get) when building trees from the index. It does so even for non-gitlinks, and even when "write-tree" is given the --missing-ok flag. The null sha1 is a special sentinel value that is already rejected in trees by fsck; whether the object exists or not, it is an error to put it in a tree. Note that for this to work, we must also avoid reusing an existing cache-tree that contains the null sha1. This patch does so by just refusing to write out any cache tree when the index contains a null sha1. This is blunter than we need to be; we could just reject the subtree that contains the offending entry. But it's not worth the complexity. The behavior is unchanged unless you have a broken index entry, and even then we'd refuse the whole index write unless the emergency GIT_ALLOW_NULL_SHA1 is in use. And even then the end result is only a performance drop (any write-tree will have to generate the whole cache-tree from scratch). The tests bear some explanation. The existing test in t7009 doesn't catch this problem, because our index-filter runs "git rm --cached", which will try to rewrite the updated index and barf on the bogus entry. So we never even make it to write-tree. The new test there adds a noop index-filter, which does show the problem. The new tests in t1601 are slightly redundant with what filter-branch is doing under the hood in t7009. But as they're much more direct, they're easier to reason about. And should filter-branch ever change or go away, we'd want to make sure that these plumbing commands behave sanely. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano