summaryrefslogtreecommitdiff
path: root/shallow.c
AgeCommit message (Collapse)AuthorFilesLines
2019-01-10fetch-pack: do not take shallow lock unnecessarilyLibravatar Jonathan Tan1-0/+7
When fetching using protocol v2, the remote may send a "shallow-info" section if the client is shallow. If so, Git as the client currently takes the shallow file lock, even if the "shallow-info" section is empty. This is not a problem except that Git does not support taking the shallow file lock after modifying the shallow file, because is_repository_shallow() stores information that is never cleared. And this take-after-modify occurs when Git does a tag-following fetch from a shallow repository on a transport that does not support tag following (since in this case, 2 fetches are performed). To solve this issue, take the shallow file lock (and perform all other shallow processing) only if the "shallow-info" section is non-empty; otherwise, behave as if it were empty. A full solution (probably, ensuring that any action of committing shallow file locks also includes clearing the information stored by is_repository_shallow()) would solve the issue without need for this patch, but this patch is independently useful (as an optimization to prevent writing a file in an unnecessary case), hence why I wrote it. I have included a NEEDSWORK outlining the full solution. Signed-off-by: Jonathan Tan <jonathantanmy@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-11-06Merge branch 'js/shallow-and-fetch-prune'Libravatar Junio C Hamano1-6/+17
"git repack" in a shallow clone did not correctly update the shallow points in the repository, leading to a repository that does not pass fsck. * js/shallow-and-fetch-prune: repack -ad: prune the list of shallow commits shallow: offer to prune only non-existing entries repack: point out a bug handling stale shallow info
2018-10-25shallow: offer to prune only non-existing entriesLibravatar Johannes Schindelin1-6/+17
The `prune_shallow()` function wants a full reachability check to be completed before it goes to work, to ensure that all unreachable entries are removed from the shallow file. However, in the upcoming patch we do not even want to go that far. We really only need to remove entries corresponding to pruned commits, i.e. to commits that no longer exist. Let's support that use case. Rather than extending the signature of `prune_shallow()` to accept another Boolean, let's turn it into a bit field and declare constants, for readability. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-10-19Merge branch 'nd/the-index'Libravatar Junio C Hamano1-1/+1
Various codepaths in the core-ish part learn to work on an arbitrary in-core index structure, not necessarily the default instance "the_index". * nd/the-index: (23 commits) revision.c: reduce implicit dependency the_repository revision.c: remove implicit dependency on the_index ws.c: remove implicit dependency on the_index tree-diff.c: remove implicit dependency on the_index submodule.c: remove implicit dependency on the_index line-range.c: remove implicit dependency on the_index userdiff.c: remove implicit dependency on the_index rerere.c: remove implicit dependency on the_index sha1-file.c: remove implicit dependency on the_index patch-ids.c: remove implicit dependency on the_index merge.c: remove implicit dependency on the_index merge-blobs.c: remove implicit dependency on the_index ll-merge.c: remove implicit dependency on the_index diff-lib.c: remove implicit dependency on the_index read-cache.c: remove implicit dependency on the_index diff.c: remove implicit dependency on the_index grep.c: remove implicit dependency on the_index diff.c: remove the_index dependency in textconv() functions blame.c: rename "repo" argument to "r" combine-diff.c: remove implicit dependency on the_index ...
2018-09-21revision.c: remove implicit dependency on the_indexLibravatar Nguyễn Thái Ngọc Duy1-1/+1
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-07-20commit.h: remove method declarationsLibravatar Derrick Stolee1-0/+1
These methods are now declared in commit-reach.h. Remove them from commit.h and add new include statements in all files that require these declarations. Signed-off-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-06-29tag: add repository argument to deref_tagLibravatar Stefan Beller1-1/+3
Add a repository argument to allow the callers of deref_tag to be more specific about which repository to act on. This is a small mechanical change; it doesn't change the implementation to handle repositories other than the_repository yet. As with the previous commits, use a macro to catch callers passing a repository other than the_repository at compile time. Signed-off-by: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Stefan Beller <sbeller@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-06-29commit: add repository argument to lookup_commitLibravatar Stefan Beller1-7/+10
Add a repository argument to allow callers of lookup_commit to be more specific about which repository to handle. This is a small mechanical change; it doesn't change the implementation to handle repositories other than the_repository yet. As with the previous commits, use a macro to catch callers passing a repository other than the_repository at compile time. Signed-off-by: Stefan Beller <sbeller@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-06-29commit: add repository argument to lookup_commit_reference_gentlyLibravatar Stefan Beller1-3/+6
Add a repository argument to allow callers of lookup_commit_reference_gently to be more specific about which repository to handle. This is a small mechanical change; it doesn't change the implementation to handle repositories other than the_repository yet. As with the previous commits, use a macro to catch callers passing a repository other than the_repository at compile time. Signed-off-by: Stefan Beller <sbeller@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-06-29Merge branch 'sb/object-store-grafts' into sb/object-store-lookupLibravatar Junio C Hamano1-36/+38
* sb/object-store-grafts: commit: allow lookup_commit_graft to handle arbitrary repositories commit: allow prepare_commit_graft to handle arbitrary repositories shallow: migrate shallow information into the object parser path.c: migrate global git_path_* to take a repository argument cache: convert get_graft_file to handle arbitrary repositories commit: convert read_graft_file to handle arbitrary repositories commit: convert register_commit_graft to handle arbitrary repositories commit: convert commit_graft_pos() to handle arbitrary repositories shallow: add repository argument to is_repository_shallow shallow: add repository argument to check_shallow_file_for_update shallow: add repository argument to register_shallow shallow: add repository argument to set_alternate_shallow_file commit: add repository argument to lookup_commit_graft commit: add repository argument to prepare_commit_graft commit: add repository argument to read_graft_file commit: add repository argument to register_commit_graft commit: add repository argument to commit_graft_pos object: move grafts to object parser object-store: move object access functions to object-store.h
2018-06-25Merge branch 'nd/reject-empty-shallow-request'Libravatar Junio C Hamano1-0/+3
"git fetch --shallow-since=<cutoff>" that specifies the cut-off point that is newer than the existing history used to end up grabbing the entire history. Such a request now errors out. * nd/reject-empty-shallow-request: upload-pack: reject shallow requests that would return nothing
2018-06-25Merge branch 'nd/commit-util-to-slab'Libravatar Junio C Hamano1-12/+28
The in-core "commit" object had an all-purpose "void *util" field, which was tricky to use especially in library-ish part of the code. All of the existing uses of the field has been migrated to a more dedicated "commit-slab" mechanism and the field is eliminated. * nd/commit-util-to-slab: commit.h: delete 'util' field in struct commit merge: use commit-slab in merge remote desc instead of commit->util log: use commit-slab in prepare_bases() instead of commit->util show-branch: note about its object flags usage show-branch: use commit-slab for commit-name instead of commit->util name-rev: use commit-slab for rev-name instead of commit->util bisect.c: use commit-slab for commit weight instead of commit->util revision.c: use commit-slab for show_source sequencer.c: use commit-slab to associate todo items to commits sequencer.c: use commit-slab to mark seen commits shallow.c: use commit-slab for commit depth instead of commit->util describe: use commit-slab for commit names instead of commit->util blame: use commit-slab for blame suspects instead of commit->util commit-slab: support shared commit-slab commit-slab.h: code split
2018-06-04upload-pack: reject shallow requests that would return nothingLibravatar Nguyễn Thái Ngọc Duy1-0/+3
Shallow clones with --shallow-since or --shalow-exclude work by running rev-list to get all reachable commits, then draw a boundary between reachable and unreachable and send "shallow" requests based on that. The code does miss one corner case: if rev-list returns nothing, we'll have no border and we'll send no shallow requests back to the client (i.e. no history cuts). This essentially means a full clone (or a full branch if the client requests just one branch). One example is the oldest commit is older than what is specified by --shallow-since. To avoid this, if rev-list returns nothing, we abort the clone/fetch. The user could adjust their request (e.g. --shallow-since further back in the past) and retry. Another possible option for this case is to fall back to a default depth (like depth 1). But I don't like too much magic that way because we may return something unexpected to the user. If they request "history since 2008" and we return a single depth at 2000, that might break stuff for them. It is better to tell them that something is wrong and let them take the best course of action. Note that we need to die() in get_shallow_commits_by_rev_list() instead of just checking for empty result from its caller deepen_by_rev_list() and handling the error there. The reason is, empty result could be a valid case: if you have commits in year 2013 and you request --shallow-since=year.2000 then you should get a full clone (i.e. empty result). Reported-by: Andreas Krey <a.krey@gmx.de> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-05-30Merge branch 'js/use-bug-macro'Libravatar Junio C Hamano1-3/+3
Developer support update, by using BUG() macro instead of die() to mark codepaths that should not happen more clearly. * js/use-bug-macro: BUG_exit_code: fix sparse "symbol not declared" warning Convert remaining die*(BUG) messages Replace all die("BUG: ...") calls by BUG() ones run-command: use BUG() to report bugs, not die() test-tool: help verifying BUG() code paths
2018-05-21shallow.c: use commit-slab for commit depth instead of commit->utilLibravatar Nguyễn Thái Ngọc Duy1-12/+28
It's done so that commit->util can be removed. See more explanation in the commit that removes commit->util. While at there, plug a leak for keeping track of depth in this code. Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-05-18shallow: migrate shallow information into the object parserLibravatar Stefan Beller1-27/+23
We need to convert the shallow functions all at the same time as we move the data structures they operate on into the repository. Signed-off-by: Stefan Beller <sbeller@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-05-18path.c: migrate global git_path_* to take a repository argumentLibravatar Stefan Beller1-5/+7
Migrate all git_path_* functions that are defined in path.c to take a repository argument. Unlike other patches in this series, do not use the #define trick, as we rewrite the whole function, which is rather small. This doesn't migrate all the functions, as other builtins have their own local path functions defined using GIT_PATH_FUNC. So keep that macro around to serve the other locations. Signed-off-by: Stefan Beller <sbeller@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-05-18shallow: add repository argument to is_repository_shallowLibravatar Stefan Beller1-4/+4
Add a repository argument to allow callers of is_repository_shallow to be more specific about which repository to handle. This is a small mechanical change; it doesn't change the implementation to handle repositories other than the_repository yet. As with the previous commits, use a macro to catch callers passing a repository other than the_repository at compile time. Signed-off-by: Stefan Beller <sbeller@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-05-18shallow: add repository argument to check_shallow_file_for_updateLibravatar Stefan Beller1-3/+4
Add a repository argument to allow callers of check_shallow_file_for_update to be more specific about which repository to handle. This is a small mechanical change; it doesn't change the implementation to handle repositories other than the_repository yet. As with the previous commits, use a macro to catch callers passing a repository other than the_repository at compile time. Signed-off-by: Stefan Beller <sbeller@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-05-18shallow: add repository argument to register_shallowLibravatar Stefan Beller1-2/+2
Add a repository argument to allow callers of register_shallow to be more specific about which repository to handle. This is a small mechanical change; it doesn't change the implementation to handle repositories other than the_repository yet. As with the previous commits, use a macro to catch callers passing a repository other than the_repository at compile time. Signed-off-by: Stefan Beller <sbeller@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-05-18shallow: add repository argument to set_alternate_shallow_fileLibravatar Stefan Beller1-1/+1
Add a repository argument to allow callers of set_alternate_shallow_file to be more specific about which repository to handle. This is a small mechanical change; it doesn't change the implementation to handle repositories other than the_repository yet. As with the previous commits, use a macro to catch callers passing a repository other than the_repository at compile time. Signed-off-by: Stefan Beller <sbeller@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-05-18commit: add repository argument to lookup_commit_graftLibravatar Jonathan Nieder1-2/+3
Add a repository argument to allow callers of lookup_commit_graft to be more specific about which repository to handle. This is a small mechanical change; it doesn't change the implementation to handle repositories other than the_repository yet. As with the previous commits, use a macro to catch callers passing a repository other than the_repository at compile time. Signed-off-by: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Stefan Beller <sbeller@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-05-16commit: add repository argument to register_commit_graftLibravatar Jonathan Nieder1-1/+2
Add a repository argument to allow callers of register_commit_graft to be more specific about which repository to handle. This is a small mechanical change; it doesn't change the implementation to handle repositories other than the_repository yet. As with the previous commits, use a macro to catch callers passing a repository other than the_repository at compile time. Signed-off-by: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Stefan Beller <sbeller@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-05-16object-store: move object access functions to object-store.hLibravatar Stefan Beller1-0/+1
This should make these functions easier to find and cache.h less overwhelming to read. In particular, this moves: - read_object_file - oid_object_info - write_object_file As a result, most of the codebase needs to #include object-store.h. In this patch the #include is only added to files that would fail to compile otherwise. It would be better to #include wherever identifiers from the header are used. That can happen later when we have better tooling for it. Signed-off-by: Stefan Beller <sbeller@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-05-10lock_file: make function-local locks non-staticLibravatar Martin Ågren1-1/+1
Placing `struct lock_file`s on the stack used to be a bad idea, because the temp- and lockfile-machinery would keep a pointer into the struct. But after 076aa2cbd (tempfile: auto-allocate tempfiles on heap, 2017-09-05), we can safely have lockfiles on the stack. (This applies even if a user returns early, leaving a locked lock behind.) These `struct lock_file`s are local to their respective functions and we can drop their staticness. For good measure, I have inspected these sites and come to believe that they always release the lock, with the possible exception of bailing out using `die()` or `exit()` or by returning from a `cmd_foo()`. As pointed out by Jeff King, it would be bad if someone held on to a `struct lock_file *` for some reason. After some grepping, I agree with his findings: no-one appears to be doing that. Signed-off-by: Martin Ågren <martin.agren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-05-06Replace all die("BUG: ...") calls by BUG() onesLibravatar Johannes Schindelin1-3/+3
In d8193743e08 (usage.c: add BUG() function, 2017-05-12), a new macro was introduced to use for reporting bugs instead of die(). It was then subsequently used to convert one single caller in 588a538ae55 (setup_git_env: convert die("BUG") to BUG(), 2017-05-12). The cover letter of the patch series containing this patch (cf 20170513032414.mfrwabt4hovujde2@sigill.intra.peff.net) is not terribly clear why only one call site was converted, or what the plan is for other, similar calls to die() to report bugs. Let's just convert all remaining ones in one fell swoop. This trick was performed by this invocation: sed -i 's/die("BUG: /BUG("/g' $(git grep -l 'die("BUG' \*.c) Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-09-29Merge branch 'ma/leakplugs'Libravatar Junio C Hamano1-1/+1
Memory leaks in various codepaths have been plugged. * ma/leakplugs: pack-bitmap[-write]: use `object_array_clear()`, don't leak object_array: add and use `object_array_pop()` object_array: use `object_array_clear()`, not `free()` leak_pending: use `object_array_clear()`, not `free()` commit: fix memory leak in `reduce_heads()` builtin/commit: fix memory leak in `prepare_index()`
2017-09-25Merge branch 'jk/write-in-full-fix'Libravatar Junio C Hamano1-3/+3
Many codepaths did not diagnose write failures correctly when disks go full, due to their misuse of write_in_full() helper function, which have been corrected. * jk/write-in-full-fix: read_pack_header: handle signed/unsigned comparison in read result config: flip return value of store_write_*() notes-merge: use ssize_t for write_in_full() return value pkt-line: check write_in_full() errors against "< 0" convert less-trivial versions of "write_in_full() != len" avoid "write_in_full(fd, buf, len) != len" pattern get-tar-commit-id: check write_in_full() return against 0 config: avoid "write_in_full(fd, buf, len) < len" pattern
2017-09-24object_array: add and use `object_array_pop()`Libravatar Martin Ågren1-1/+1
In a couple of places, we pop objects off an object array `foo` by decreasing `foo.nr`. We access `foo.nr` in many places, but most if not all other times we do so read-only, e.g., as we iterate over the array. But when we change `foo.nr` behind the array's back, it feels a bit nasty and looks like it might leak memory. Leaks happen if the popped element has an allocated `name` or `path`. At the moment, that is not the case. Still, 1) the object array might gain more fields that want to be freed, 2) a code path where we pop might start using names or paths, 3) one of these code paths might be copied to somewhere where we do, and 4) using a dedicated function for popping is conceptually cleaner. Introduce and use `object_array_pop()` instead. Release memory in the new function. Document that popping an object leaves the associated elements in limbo. The converted places were identified by grepping for "\.nr\>" and looking for "--". Make the new function return NULL on an empty array. This is consistent with `pop_commit()` and allows the following: while ((o = object_array_pop(&foo)) != NULL) { // do something } But as noted above, we don't need to go out of our way to avoid reading `foo.nr`. This is probably more readable: while (foo.nr) { ... o = object_array_pop(&foo); // do something } The name of `object_array_pop()` does not quite align with `add_object_array()`. That is unfortunate. On the other hand, it matches `object_array_clear()`. Arguably it's `add_...` that is the odd one out, since it reads like it's used to "add" an "object array". For that reason, side with `object_array_clear()`. Signed-off-by: Martin Ågren <martin.agren@gmail.com> Reviewed-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-09-14avoid "write_in_full(fd, buf, len) != len" patternLibravatar Jeff King1-3/+3
The return value of write_in_full() is either "-1", or the requested number of bytes[1]. If we make a partial write before seeing an error, we still return -1, not a partial value. This goes back to f6aa66cb95 (write_in_full: really write in full or return error on disk full., 2007-01-11). So checking anything except "was the return value negative" is pointless. And there are a couple of reasons not to do so: 1. It can do a funny signed/unsigned comparison. If your "len" is signed (e.g., a size_t) then the compiler will promote the "-1" to its unsigned variant. This works out for "!= len" (unless you really were trying to write the maximum size_t bytes), but is a bug if you check "< len" (an example of which was fixed recently in config.c). We should avoid promoting the mental model that you need to check the length at all, so that new sites are not tempted to copy us. 2. Checking for a negative value is shorter to type, especially when the length is an expression. 3. Linus says so. In d34cf19b89 (Clean up write_in_full() users, 2007-01-11), right after the write_in_full() semantics were changed, he wrote: I really wish every "write_in_full()" user would just check against "<0" now, but this fixes the nasty and stupid ones. Appeals to authority aside, this makes it clear that writing it this way does not have an intentional benefit. It's a historical curiosity that we never bothered to clean up (and which was undoubtedly cargo-culted into new sites). So let's convert these obviously-correct cases (this includes write_str_in_full(), which is just a wrapper for write_in_full()). [1] A careful reader may notice there is one way that write_in_full() can return a different value. If we ask write() to write N bytes and get a return value that is _larger_ than N, we could return a larger total. But besides the fact that this would imply a totally broken version of write(), it would already invoke undefined behavior. Our internal remaining counter is an unsigned size_t, which means that subtracting too many byte will wrap it around to a very large number. So we'll instantly begin reading off the end of the buffer, trying to write gigabytes (or petabytes) of data. Signed-off-by: Jeff King <peff@peff.net> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-09-06tempfile: auto-allocate tempfiles on heapLibravatar Jeff King1-7/+6
The previous commit taught the tempfile code to give up ownership over tempfiles that have been renamed or deleted. That makes it possible to use a stack variable like this: struct tempfile t; create_tempfile(&t, ...); ... if (!err) rename_tempfile(&t, ...); else delete_tempfile(&t); But doing it this way has a high potential for creating memory errors. The tempfile we pass to create_tempfile() ends up on a global linked list, and it's not safe for it to go out of scope until we've called one of those two deactivation functions. Imagine that we add an early return from the function that forgets to call delete_tempfile(). With a static or heap tempfile variable, the worst case is that the tempfile hangs around until the program exits (and some functions like setup_shallow_temporary rely on this intentionally, creating a tempfile and then leaving it for later cleanup). But with a stack variable as above, this is a serious memory error: the variable goes out of scope and may be filled with garbage by the time the tempfile code looks at it. Let's see if we can make it harder to get this wrong. Since many callers need to allocate arbitrary numbers of tempfiles, we can't rely on static storage as a general solution. So we need to turn to the heap. We could just ask all callers to pass us a heap variable, but that puts the burden on them to call free() at the right time. Instead, let's have the tempfile code handle the heap allocation _and_ the deallocation (when the tempfile is deactivated and removed from the list). This changes the return value of all of the creation functions. For the cleanup functions (delete and rename), we'll add one extra bit of safety: instead of taking a tempfile pointer, we'll take a pointer-to-pointer and set it to NULL after freeing the object. This makes it safe to double-call functions like delete_tempfile(), as the second call treats the NULL input as a noop. Several callsites follow this pattern. The resulting patch does have a fair bit of noise, as each caller needs to be converted to handle: 1. Storing a pointer instead of the struct itself. 2. Passing the pointer instead of taking the struct address. 3. Handling a "struct tempfile *" return instead of a file descriptor. We could play games to make this less noisy. For example, by defining the tempfile like this: struct tempfile { struct heap_allocated_part_of_tempfile { int fd; ...etc } *actual_data; } Callers would continue to have a "struct tempfile", and it would be "active" only when the inner pointer was non-NULL. But that just makes things more awkward in the long run. There aren't that many callers, so we can simply bite the bullet and adjust all of them. And the compiler makes it easy for us to find them all. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-09-06tempfile: do not delete tempfile on failed closeLibravatar Jeff King1-1/+1
When close_tempfile() fails, we delete the tempfile and reset the fields of the tempfile struct. This makes it easier for callers to return without cleaning up, but it also makes this common pattern: if (close_tempfile(tempfile)) return error_errno("error closing %s", tempfile->filename.buf); wrong, because the "filename" field has been reset after the failed close. And it's not easy to fix, as in many cases we don't have another copy of the filename (e.g., if it was created via one of the mks_tempfile functions, and we just have the original template string). Let's drop the feature that a failed close automatically deletes the file. This puts the burden on the caller to do the deletion themselves, but this isn't that big a deal. Callers which do: if (write(...) || close_tempfile(...)) { delete_tempfile(...); return -1; } already had to call delete when the write() failed, and so aren't affected. Likewise, any caller which just calls die() in the error path is OK; we'll delete the tempfile during the atexit handler. Because this patch changes the semantics of close_tempfile() without changing its signature, all callers need to be manually checked and converted to the new scheme. This patch covers all in-tree callers, but there may be others for not-yet-merged topics. To catch these, we rename the function to close_tempfile_gently(), which will attract compile-time attention to new callers. (Technically the original could be considered "gentle" already in that it didn't die() on errors, but this one is even more so). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-09-06always check return value of close_tempfileLibravatar Jeff King1-2/+2
If close_tempfile() encounters an error, then it deletes the tempfile and resets the "struct tempfile". But many code paths ignore the return value and continue to use the tempfile. Instead, we should generally treat this the same as a write() error. Note that in the postimage of some of these cases our error message will be bogus after a failed close because we look at tempfile->filename (either directly or via get_tempfile_path). But after the failed close resets the tempfile object, this is guaranteed to be the empty string. That will be addressed in a future patch (because there are many more cases of the same problem than just these instances). Note also in the hunk in gpg-interface.c that it's fine to call delete_tempfile() in the error path, even if close_tempfile() failed and already deleted the file. The tempfile code is smart enough to know the second deletion is a noop. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-09-06setup_temporary_shallow: move tempfile struct into functionLibravatar Jeff King1-6/+5
The setup_temporary_shallow() function creates a temporary file, but we never access the tempfile struct outside of the function. This is OK, since it means we'll just clean up the tempfile on exit. But we can simplify the code a bit by moving the global tempfile struct to the only function in which it's used. Note that it must remain "static" due to tempfile.c's requirement that tempfile storage never goes away until program exit. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-09-06setup_temporary_shallow: avoid using inactive tempfileLibravatar Jeff King1-1/+1
When there are no shallow entries to write, we skip creating the tempfile entirely and try to return the empty string. But we do so by calling get_tempfile_path() on the inactive tempfile object. This will trigger an assertion that kills the program. The bug was introduced by 6e122b449b (setup_temporary_shallow(): use tempfile module, 2015-08-10). But nobody seems to have noticed since then because we do not end up calling this function at all when there are no shallow items. In other words, this code path is completely unexercised. Since the tempfile object is a static global, it _is_ possible that we call the function twice, writing out shallow info the first time and then "reusing" our tempfile object the second time. But: 1. It seems unlikely that this was the intent, as hitting this code path would imply somebody clearing the shallow_info list between calls. And if somebody _did_ call the function multiple times without clearing the shallow_info list, we'd hit a different BUG for trying to reuse an already-active tempfile. 2. I verified by code inspection that the function is only called once per program. And also replacing this code with a BUG() and running the test suite demonstrates that it is not triggered there. So we could probably just replace this with an assertion and confirm that it's never called. However, the original intent does seem to be that you _could_ call it when the shallow_info is empty. And that's easy enough to do; since the return value doesn't need to point to a writable buffer, we can just return a string literal. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-07-13commit: convert lookup_commit_graft to struct object_idLibravatar Stefan Beller1-2/+2
With this patch, commit.h doesn't contain the string 'sha1' any more. Signed-off-by: Stefan Beller <sbeller@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-07-12Merge branch 'rs/use-div-round-up'Libravatar Junio C Hamano1-4/+4
Code cleanup. * rs/use-div-round-up: use DIV_ROUND_UP
2017-07-10use DIV_ROUND_UPLibravatar René Scharfe1-4/+4
Convert code that divides and rounds up to use DIV_ROUND_UP to make the intent clearer and reduce the number of magic constants. Signed-off-by: Rene Scharfe <l.s.r@web.de> Reviewed-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-05-29Merge branch 'js/plug-leaks'Libravatar Junio C Hamano1-2/+6
Fix memory leaks pointed out by Coverity (and people). * js/plug-leaks: (26 commits) checkout: fix memory leak submodule_uses_worktrees(): plug memory leak show_worktree(): plug memory leak name-rev: avoid leaking memory in the `deref` case remote: plug memory leak in match_explicit() add_reflog_for_walk: avoid memory leak shallow: avoid memory leak line-log: avoid memory leak receive-pack: plug memory leak in update() fast-export: avoid leaking memory in handle_tag() mktree: plug memory leaks reported by Coverity pack-redundant: plug memory leak setup_discovered_git_dir(): plug memory leak setup_bare_git_dir(): help static analysis split_commit_in_progress(): simplify & fix memory leak checkout: fix memory leak cat-file: fix memory leak mailinfo & mailsplit: check for EOF while parsing status: close file descriptor after reading git-rebase-todo difftool: address a couple of resource/memory leaks ...
2017-05-08Convert lookup_commit* to struct object_idLibravatar brian m. carlson1-10/+10
Convert lookup_commit, lookup_commit_or_die, lookup_commit_reference, and lookup_commit_reference_gently to take struct object_id arguments. Introduce a temporary in parse_object buffer in order to convert this function. This is required since in order to convert parse_object and parse_object_buffer, lookup_commit_reference_gently and lookup_commit_or_die would need to be converted. Not introducing a temporary would therefore require that lookup_commit_or_die take a struct object_id *, but lookup_commit would take unsigned char *, leaving a confusing and hard-to-use interface. parse_object_buffer will lose this temporary in a later patch. This commit was created with manual changes to commit.c, commit.h, and object.c, plus the following semantic patch: @@ expression E1, E2; @@ - lookup_commit_reference_gently(E1.hash, E2) + lookup_commit_reference_gently(&E1, E2) @@ expression E1, E2; @@ - lookup_commit_reference_gently(E1->hash, E2) + lookup_commit_reference_gently(E1, E2) @@ expression E1; @@ - lookup_commit_reference(E1.hash) + lookup_commit_reference(&E1) @@ expression E1; @@ - lookup_commit_reference(E1->hash) + lookup_commit_reference(E1) @@ expression E1; @@ - lookup_commit(E1.hash) + lookup_commit(&E1) @@ expression E1; @@ - lookup_commit(E1->hash) + lookup_commit(E1) @@ expression E1, E2; @@ - lookup_commit_or_die(E1.hash, E2) + lookup_commit_or_die(&E1, E2) @@ expression E1, E2; @@ - lookup_commit_or_die(E1->hash, E2) + lookup_commit_or_die(E1, E2) Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-05-08Convert remaining callers of lookup_commit_reference* to object_idLibravatar brian m. carlson1-3/+3
There are a small number of remaining callers of lookup_commit_reference and lookup_commit_reference_gently that still need to be converted to struct object_id. Convert these. Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-05-08shallow: convert shallow registration functions to object_idLibravatar brian m. carlson1-6/+6
Convert register_shallow and unregister_shallow to take struct object_id. register_shallow is a caller of lookup_commit, which we will convert later. It doesn't make sense for the registration and unregistration functions to have incompatible interfaces, so convert them both. Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-05-08shallow: avoid memory leakLibravatar Johannes Schindelin1-2/+6
Reported by Coverity. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-03-31Rename sha1_array to oid_arrayLibravatar brian m. carlson1-6/+6
Since this structure handles an array of object IDs, rename it to struct oid_array. Also rename the accessor functions and the initialization constant. This commit was produced mechanically by providing non-Documentation files to the following Perl one-liners: perl -pi -E 's/struct sha1_array/struct oid_array/g' perl -pi -E 's/\bsha1_array_/oid_array_/g' perl -pi -E 's/SHA1_ARRAY_INIT/OID_ARRAY_INIT/g' Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-03-28sha1-array: convert internal storage for struct sha1_array to object_idLibravatar brian m. carlson1-13/+13
Make the internal storage for struct sha1_array use an array of struct object_id internally. Update the users of this struct which inspect its internals. Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2016-12-21Merge branch 'nd/shallow-fixup'Libravatar Junio C Hamano1-19/+20
Code cleanup in shallow boundary computation. * nd/shallow-fixup: shallow.c: remove useless code shallow.c: bit manipulation tweaks shallow.c: avoid theoretical pointer wrap-around shallow.c: make paint_alloc slightly more robust shallow.c: stop abusing COMMIT_SLAB_SIZE for paint_info's memory pools shallow.c: rename fields in paint_info to better express their purposes
2016-12-07shallow.c: remove useless codeLibravatar Nguyễn Thái Ngọc Duy1-4/+0
Some context before we talk about the removed code. This paint_down() is part of step 6 of 58babff (shallow.c: the 8 steps to select new commits for .git/shallow - 2013-12-05). When we fetch from a shallow repository, we need to know if one of the new/updated refs needs new "shallow commits" in .git/shallow (because we don't have enough history of those refs) and which one. The question at step 6 is, what (new) shallow commits are required in other to maintain reachability throughout the repository _without_ cutting our history short? To answer, we mark all commits reachable from existing refs with UNINTERESTING ("rev-list --not --all"), mark shallow commits with BOTTOM, then for each new/updated refs, walk through the commit graph until we either hit UNINTERESTING or BOTTOM, marking the ref on the commit as we walk. After all the walking is done, we check the new shallow commits. If we have not seen any new ref marked on a new shallow commit, we know all new/updated refs are reachable using just our history and .git/shallow. The shallow commit in question is not needed and can be thrown away. So, the code. The loop here (to walk through commits) is basically 1. get one commit from the queue 2. ignore if it's SEEN or UNINTERESTING 3. mark it 4. go through all the parents and.. 5a. mark it if it's never marked before 5b. put it back in the queue What we do in this patch is drop step 5a because it is not necessary. The commit being marked at 5a is put back on the queue, and will be marked at step 3 at the next iteration. The only case it will not be marked is when the commit is already marked UNINTERESTING (5a does not check this), which will be ignored at step 2. But we don't care about refs marking on UNINTERESTING. We care about the marking on _shallow commits_ that are not reachable from our current history (and having UNINTERESTING on it means it's reachable). So it's ok for an UNINTERESTING not to be ref-marked. Reported-by: Rasmus Villemoes <rv@rasmusvillemoes.dk> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2016-12-07shallow.c: bit manipulation tweaksLibravatar Rasmus Villemoes1-4/+4
First of all, 1 << 31 is technically undefined behaviour, so let's just use an unsigned literal. If i is 'signed int' and gcc doesn't know that i is positive, gcc generates code to compute the C99-mandated values of "i / 32" and "i % 32", which is a lot more complicated than simple a simple shifts/mask. The only caller of paint_down actually passes an "unsigned int" value, but the prototype of paint_down causes (completely well-defined) conversion to signed int, and gcc has no way of knowing that the converted value is non-negative. Just make the id parameter unsigned. In update_refstatus, the change in generated code is much smaller, presumably because gcc is smart enough to see that i starts as 0 and is only incremented, so it is allowed (per the UD of signed overflow) to assume that i is always non-negative. But let's just help less smart compilers generate good code anyway. Signed-off-by: Rasmus Villemoes <rv@rasmusvillemoes.dk> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> Reviewed-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2016-12-07shallow.c: avoid theoretical pointer wrap-aroundLibravatar Rasmus Villemoes1-1/+1
The expression info->free+size is technically undefined behaviour in exactly the case we want to test for. Moreover, the compiler is likely to translate the expression to (unsigned long)info->free + size > (unsigned long)info->end where there's at least a theoretical chance that the LHS could wrap around 0, giving a false negative. This might as well be written using pointer subtraction avoiding these issues. Signed-off-by: Rasmus Villemoes <rv@rasmusvillemoes.dk> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> Reviewed-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2016-12-07shallow.c: make paint_alloc slightly more robustLibravatar Nguyễn Thái Ngọc Duy1-0/+3
paint_alloc() allocates a big block of memory and splits it into smaller, fixed size, chunks of memory whenever it's called. Each chunk contains enough bits to present all "new refs" [1] in a fetch from a shallow repository. We do not check if the new "big block" is smaller than the requested memory chunk though. If it happens, we'll happily pass back a memory region smaller than expected. Which will lead to problems eventually. A normal fetch may add/update a dozen new refs. Let's stay on the "reasonably extreme" side and say we need 16k refs (or bits from paint_alloc's perspective). Each chunk of memory would be 2k, much smaller than the memory pool (512k). So, normally, the under-allocation situation should never happen. A bad guy, however, could make a fetch that adds more than 4m new/updated refs to this code which results in a memory chunk larger than pool size. Check this case and abort. Noticed-by: Rasmus Villemoes <rv@rasmusvillemoes.dk> Reviewed-by: Jeff King <peff@peff.net> [1] Details are in commit message of 58babff (shallow.c: the 8 steps to select new commits for .git/shallow - 2013-12-05), step 6. Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> Reviewed-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>