summaryrefslogtreecommitdiff
path: root/refs.c
Commit message (Collapse)AuthorAgeFilesLines
* Merge branch 'mh/ref-races-optim-invalidate-cached'Junio C Hamano2013-07-241-3/+6
|\ | | | | | | | | * mh/ref-races-optim-invalidate-cached: refs: do not invalidate the packed-refs cache unnecessarily
| * refs: do not invalidate the packed-refs cache unnecessarilyMichael Haggerty2013-06-201-3/+6
| | | | | | | | | | | | | | | | | | | | | | | | Now that we keep track of the packed-refs file metadata, we can detect when the packed-refs file has been modified since we last read it, and we do so automatically every time that get_packed_ref_cache() is called. So there is no need to invalidate the cache automatically when lock_packed_refs() is called; usually the old copy will still be valid. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* | Merge branch 'mh/ref-races'Junio C Hamano2013-06-301-51/+265
|\ \ | |/ | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | "git pack-refs" that races with new ref creation or deletion have been susceptible to lossage of refs under right conditions, which has been tightened up. * mh/ref-races: for_each_ref: load all loose refs before packed refs get_packed_ref_cache: reload packed-refs file when it changes add a stat_validity struct Extract a struct stat_data from cache_entry packed_ref_cache: increment refcount when locked do_for_each_entry(): increment the packed refs cache refcount refs: manage lifetime of packed refs cache via reference counting refs: implement simple transactions for the packed-refs file refs: wrap the packed refs cache in a level of indirection pack_refs(): split creation of packed refs and entry writing repack_without_ref(): split list curation and entry writing
| * for_each_ref: load all loose refs before packed refsJeff King2013-06-201-4/+35
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | If we are iterating through the refs using for_each_ref (or any of its sister functions), we can get into a race condition with a simultaneous "pack-refs --prune" that looks like this: 0. We have a large number of loose refs, and a few packed refs. refs/heads/z/foo is loose, with no matching entry in the packed-refs file. 1. Process A starts iterating through the refs. It loads the packed-refs file from disk, then starts lazily traversing through the loose ref directories. 2. Process B, running "pack-refs --prune", writes out the new packed-refs file. It then deletes the newly packed refs, including refs/heads/z/foo. 3. Meanwhile, process A has finally gotten to refs/heads/z (it traverses alphabetically). It descends, but finds nothing there. It checks its cached view of the packed-refs file, but it does not mention anything in "refs/heads/z/" at all (it predates the new file written by B in step 2). The traversal completes successfully without mentioning refs/heads/z/foo at all (the name, of course, isn't important; but the more refs you have and the farther down the alphabetical list a ref is, the more likely it is to hit the race). If refs/heads/z/foo did exist in the packed refs file at state 0, we would see an entry for it, but it would show whatever sha1 the ref had the last time it was packed (which could be an arbitrarily long time ago). This can be especially dangerous when process A is "git prune", as it means our set of reachable tips will be incomplete, and we may erroneously prune objects reachable from that tip (the same thing can happen if "repack -ad" is used, as it simply drops unreachable objects that are packed). This patch solves it by loading all of the loose refs for our traversal into our in-memory cache, and then refreshing the packed-refs cache. Because a pack-refs writer will always put the new packed-refs file into place before starting the prune, we know that any loose refs we fail to see will either truly be missing, or will have already been put in the packed-refs file by the time we refresh. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * get_packed_ref_cache: reload packed-refs file when it changesJeff King2013-06-201-5/+16
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Once we read the packed-refs file into memory, we cache it to save work on future ref lookups. However, our cache may be out of date with respect to what is on disk if another process is simultaneously packing the refs. Normally it is acceptable for us to be a little out of date, since there is no guarantee whether we read the file before or after the simultaneous update. However, there is an important special case: our packed-refs file must be up to date with respect to any loose refs we read. Otherwise, we risk the following race condition: 0. There exists a loose ref refs/heads/master. 1. Process A starts and looks up the ref "master". It first checks $GIT_DIR/master, which does not exist. It then loads (and caches) the packed-refs file to see if "master" exists in it, which it does not. 2. Meanwhile, process B runs "pack-refs --all --prune". It creates a new packed-refs file which contains refs/heads/master, and removes the loose copy at $GIT_DIR/refs/heads/master. 3. Process A continues its lookup, and eventually tries $GIT_DIR/refs/heads/master. It sees that the loose ref is missing, and falls back to the packed-refs file. But it examines its cached version, which does not have refs/heads/master. After trying a few other prefixes, it reports master as a non-existent ref. There are many variants (e.g., step 1 may involve process A looking up another ref entirely, so even a fully qualified refname can fail). One of the most interesting ones is if "refs/heads/master" is already packed. In that case process A will not see it as missing, but rather will report whatever value happened to be in the packed-refs file before process B repacked (which might be an arbitrarily old value). We can fix this by making sure we reload the packed-refs file from disk after looking at any loose refs. That's unacceptably slow, so we can check its stat()-validity as a proxy, and read it only when it appears to have changed. Reading the packed-refs file after performing any loose-ref system calls is sufficient because we know the ordering of the pack-refs process: it always makes sure the newly written packed-refs file is installed into place before pruning any loose refs. As long as those operations by B appear in their executed order to process A, by the time A sees the missing loose ref, the new packed-refs file must be in place. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * packed_ref_cache: increment refcount when lockedMichael Haggerty2013-06-201-1/+7
| | | | | | | | | | | | | | | | Increment the packed_ref_cache reference count while it is locked to prevent its being freed. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * do_for_each_entry(): increment the packed refs cache refcountMichael Haggerty2013-06-201-1/+4
| | | | | | | | | | | | | | | | | | | | This function calls a user-supplied callback function which could do something that causes the packed refs cache to be invalidated. So acquire a reference count on the data structure to prevent our copy from being freed while we are iterating over it. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * refs: manage lifetime of packed refs cache via reference countingMichael Haggerty2013-06-201-3/+36
| | | | | | | | | | | | | | | | | | | | | | | | | | | | In struct packed_ref_cache, keep a count of the number of users of the data structure. Only free the packed ref cache when the reference count goes to zero rather than when the packed ref cache is cleared. This mechanism will be used to prevent the cache data structure from being freed while it is being iterated over. So far, only the reference in struct ref_cache::packed is counted; other users will be adjusted in separate commits. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * refs: implement simple transactions for the packed-refs fileMichael Haggerty2013-06-201-18/+70
| | | | | | | | | | | | | | | | | | | | | | | | | | | | Handle simple transactions for the packed-refs file at the packed_ref_cache level via new functions lock_packed_refs(), commit_packed_refs(), and rollback_packed_refs(). Only allow the packed ref cache to be modified (via add_packed_ref()) while the packed refs file is locked. Change clone to add the new references within a transaction. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * refs: wrap the packed refs cache in a level of indirectionMichael Haggerty2013-06-201-6/+26
| | | | | | | | | | | | | | | | | | | | As we know, we can solve any problem in this manner. In this case, the problem is to avoid freeing a packed refs cache while somebody is using it. So add a level of indirection as a prelude to reference-counting the packed refs cache. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * pack_refs(): split creation of packed refs and entry writingMichael Haggerty2013-06-201-14/+34
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Split pack_refs() into multiple passes: * Iterate over loose refs. For each one that can be turned into a packed ref, create a corresponding entry in the packed refs cache. * Write the packed refs to the packed-refs file. This change isolates the mutation of the packed-refs file to a single place. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * repack_without_ref(): split list curation and entry writingMichael Haggerty2013-06-201-12/+50
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | The repack_without_ref() function first removes the deleted ref from the internal packed-refs list, then writes the packed-refs list to disk, omitting any broken or stale entries. This patch splits that second step into multiple passes: * collect the list of refnames that should be deleted from packed_refs * delete those refnames from the cache * write the remainder to the packed-refs file The purpose of this change is to make the "write the remainder" part reusable. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* | resolve_ref_unsafe(): close race condition reading loose refsMichael Haggerty2013-06-191-4/+24
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | We read loose references in two steps. The code is roughly: lstat() if error ENOENT: loose ref is missing; look for corresponding packed ref else if S_ISLNK: readlink() if error: report failure else if S_ISDIR: report failure else open() if error: report failure read() The problem is that the first filesystem call, to lstat(), is not atomic with the second filesystem call, to readlink() or open(). Therefore it is possible for another process to change the file between our two calls, for example: * If the other process deletes the file, our second call will fail with ENOENT, which we *should* interpret as "loose ref is missing; look for corresponding packed ref". This can arise if the other process is pack-refs; it might have just written a new packed-refs file containing the old contents of the reference then deleted the loose ref. * If the other process changes a symlink into a plain file, our call to readlink() will fail with EINVAL, which we *should* respond to by trying to open() and read() the file. The old code treats the reference as missing in both of these cases, which is incorrect. So instead, handle errors more selectively: if the result of readline()/open() is a failure that is inconsistent with the result of the previous lstat(), then something is fishy. In this case jump back and start over again with a fresh call to lstat(). One race is still possible and undetected: another process could change the file from a regular file into a symlink between the call to lstat and the call to open(). The open() call would silently follow the symlink and not know that something is wrong. This situation could be detected in two ways: * On systems that support O_NOFOLLOW, pass that option to the open(). * On other systems, call fstat() on the fd returned by open() and make sure that it agrees with the stat info from the original lstat(). However, we don't use symlinks anymore, so this situation is unlikely. Moreover, it doesn't appear that treating a symlink as a regular file would have grave consequences; after all, this is exactly how the code handles non-relative symlinks. So this commit leaves that race unaddressed. Note that this solves only the part of the race within resolve_ref_unsafe. In the situation described above, we may still be depending on a cached view of the packed-refs file; that race will be dealt with in a future patch. This problem was reported and diagnosed by Jeff King <peff@peff.net>, and this solution is derived from his patch. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Reviewed-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* | resolve_ref_unsafe(): handle the case of an SHA-1 within loopMichael Haggerty2013-06-191-9/+13
| | | | | | | | | | | | | | | | | | | | | | | | There is only one "break" statement within the loop, which jumps to the code after the loop that handles the case of a file that holds a SHA-1. So move that code from below the loop into the if statement where the break was previously located. This makes the logic flow more local. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Reviewed-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* | resolve_ref_unsafe(): extract function handle_missing_loose_ref()Michael Haggerty2013-06-191-21/+35
|/ | | | | | | | | The nesting was getting a bit out of hand, and it's about to get worse. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Reviewed-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* Merge branch 'fc/at-head'Junio C Hamano2013-06-111-0/+4
|\ | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Instead of typing four capital letters "HEAD", you can say "@" instead. * fc/at-head: sha1_name: compare variable with constant, not constant with variable Add new @ shortcut for HEAD sha1_name: refactor reinterpret() sha1_name: check @{-N} errors sooner sha1_name: reorganize get_sha1_basic() sha1_name: don't waste cycles in the @-parsing loop sha1_name: remove unnecessary braces sha1_name: remove no-op tests: at-combinations: @{N} versus HEAD@{N} tests: at-combinations: increase coverage tests: at-combinations: improve nonsense() tests: at-combinations: check ref names directly tests: at-combinations: simplify setup
| * Add new @ shortcut for HEADFelipe Contreras2013-05-081-0/+4
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Typing 'HEAD' is tedious, especially when we can use '@' instead. The reason for choosing '@' is that it follows naturally from the ref@op syntax (e.g. HEAD@{u}), except we have no ref, and no operation, and when we don't have those, it makes sens to assume 'HEAD'. So now we can use 'git show @~1', and all that goody goodness. Until now '@' was a valid name, but it conflicts with this idea, so let's make it invalid. Probably very few people, if any, used this name. Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* | Merge branch 'mh/packed-refs-various'Junio C Hamano2013-05-291-171/+562
|\ \ | |/ |/| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Update reading and updating packed-refs file, correcting corner case bugs. * mh/packed-refs-various: (33 commits) refs: handle the main ref_cache specially refs: change do_for_each_*() functions to take ref_cache arguments pack_one_ref(): do some cheap tests before a more expensive one pack_one_ref(): use write_packed_entry() to do the writing pack_one_ref(): use function peel_entry() refs: inline function do_not_prune() pack_refs(): change to use do_for_each_entry() refs: use same lock_file object for both ref-packing functions pack_one_ref(): rename "path" parameter to "refname" pack-refs: merge code from pack-refs.{c,h} into refs.{c,h} pack-refs: rename handle_one_ref() to pack_one_ref() refs: extract a function write_packed_entry() repack_without_ref(): write peeled refs in the rewritten file t3211: demonstrate loss of peeled refs if a packed ref is deleted refs: change how packed refs are deleted search_ref_dir(): return an index rather than a pointer repack_without_ref(): silence errors for dangling packed refs t3210: test for spurious error messages for dangling packed refs refs: change the internal reference-iteration API refs: extract a function peel_entry() ...
| * refs: handle the main ref_cache speciallyMichael Haggerty2013-05-011-29/+31
| | | | | | | | | | | | | | | | | | | | | | Hold the ref_cache instance for the main repository in a dedicated, statically-allocated instance to avoid the need for a function call and a linked-list traversal when it is needed. Suggested by: Heiko Voigt <hvoigt@hvoigt.net> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * refs: change do_for_each_*() functions to take ref_cache argumentsMichael Haggerty2013-05-011-15/+14
| | | | | | | | | | | | | | Change the callers convert submodule names into ref_cache pointers. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * pack_one_ref(): do some cheap tests before a more expensive oneMichael Haggerty2013-05-011-6/+5
| | | | | | | | | | Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * pack_one_ref(): use write_packed_entry() to do the writingMichael Haggerty2013-05-011-25/+8
| | | | | | | | | | | | | | | | | | Change pack_refs() to work with a file descriptor instead of a FILE* (making the file-locking code less awkward) and use write_packed_entry() to do the writing. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * pack_one_ref(): use function peel_entry()Michael Haggerty2013-05-011-16/+23
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Change pack_one_ref() to call peel_entry() rather than using its own code for peeling references. Aside from sharing code, this lets it take advantage of the optimization introduced by 6c4a060d7d. Please note that we *could* use any peeled values that happen to already be stored in the ref_entries, which would avoid some object lookups for references that were already packed. But doing so would also propagate any peeling errors across runs of "git pack-refs" and give no way to recover from such errors. And "git pack-refs" isn't run often enough that the performance cost is a problem. So instead, add a new option to peel_entry() to force the entry to be re-peeled, and call it with that option set. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * refs: inline function do_not_prune()Michael Haggerty2013-05-011-9/+2
| | | | | | | | | | | | | | | | | | Function do_not_prune() was redundantly checking REF_ISSYMREF, which was already tested at the top of pack_one_ref(), so remove that check. And the rest was trivial, so inline the function. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * pack_refs(): change to use do_for_each_entry()Michael Haggerty2013-05-011-14/+15
| | | | | | | | | | | | | | | | | | | | pack_refs() was not using any of the extra features of for_each_ref(), so change it to use do_for_each_entry(). This also gives it access to the ref_entry and in particular its peeled field, which will be taken advantage of in the next commit. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * refs: use same lock_file object for both ref-packing functionsMichael Haggerty2013-05-011-6/+4
| | | | | | | | | | | | | | | | Use a single struct lock_file for both pack_refs() and repack_without_ref(). Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * pack_one_ref(): rename "path" parameter to "refname"Michael Haggerty2013-05-011-7/+7
| | | | | | | | | | | | | | | | Make this function conform to the naming convention established in 65385ef7d4 for the rest of the refs.c file. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * pack-refs: merge code from pack-refs.{c,h} into refs.{c,h}Michael Haggerty2013-05-011-0/+144
| | | | | | | | | | | | | | | | | | | | | | | | pack-refs.c doesn't contain much code, and the code it does contain is closely related to reference handling. Moreover, there is some duplication between pack_refs() and repack_without_ref(). Therefore, merge pack-refs.c into refs.c and pack-refs.h into refs.h. The code duplication will be addressed in future commits. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * refs: extract a function write_packed_entry()Michael Haggerty2013-05-011-16/+30
| | | | | | | | | | | | | | | | Extract the I/O code from the "business logic" in repack_ref_fn(). Later there will be another caller for this function. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * repack_without_ref(): write peeled refs in the rewritten fileMichael Haggerty2013-05-011-0/+23
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | When a reference that existed in the packed-refs file is deleted, the packed-refs file must be rewritten. Previously, the file was rewritten without any peeled refs, even if the file contained peeled refs when it was read. This was not a bug, because the packed-refs file header didn't claim that the file contained peeled values. But it had a performance cost, because the repository would lose the benefit of having precomputed peeled references until pack-refs was run again. Teach repack_without_ref() to write peeled refs to the packed-refs file (regardless of whether they were present in the old version of the file). This means that if the old version of the packed-refs file was not fully peeled, then repack_without_ref() will have to peel references. To avoid the expense of reading lots of loose references, we take two shortcuts relative to pack-refs: * If the peeled value of a reference is already known (i.e., because it was read from the old version of the packed-refs file), then output that peeled value again without any checks. This is the usual code path and should avoid any noticeable overhead. (This is different than pack-refs, which always re-peels references.) * We don't verify that the packed ref is still current. It could be that a packed references is overridden by a loose reference, in which case the packed ref is no longer needed and might even refer to an object that has been garbage collected. But we don't check; instead, we just try to peel all references. If peeling is successful, the peeled value is written out (even though it might not be needed any more); if not, then the reference is silently omitted from the output. The extra overhead of peeling references in repack_without_ref() should only be incurred the first time the packed-refs file is written by a version of Git that knows about the "fully-peeled" attribute. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * refs: change how packed refs are deletedMichael Haggerty2013-05-011-16/+68
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Add a function remove_ref(), which removes a single entry from a reference cache. Use this function to reimplement repack_without_ref(). The old version iterated over all refs, packing all of them except for the one to be deleted, then discarded the entire packed reference cache. The new version deletes the doomed reference from the cache *before* iterating. This has two advantages: * the code for writing packed-refs becomes simpler, because it doesn't have to exclude one of the references. * it is no longer necessary to discard the packed refs cache after deleting a reference: symbolic refs cannot be packed, so packed references cannot depend on each other, so the rest of the packed refs cache remains valid after a reference is deleted. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * search_ref_dir(): return an index rather than a pointerMichael Haggerty2013-05-011-12/+18
| | | | | | | | | | | | | | | | | | Change search_ref_dir() to return the index of the sought entry (or -1 on error) rather than a pointer to the entry. This will make it more natural to use the function for removing an entry from the list. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * repack_without_ref(): silence errors for dangling packed refsMichael Haggerty2013-05-011-2/+35
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Stop emitting an error message when deleting a packed reference if we find another dangling packed reference that is overridden by a loose reference. See the previous commit for a longer explanation of the issue. We have to be careful to make sure that the invalid packed reference really *is* overridden by a loose reference; otherwise what we have found is repository corruption, which we *should* report. Please note that this approach is vulnerable to a race condition similar to the race conditions already known to affect packed references [1]: * Process 1 tries to peel packed reference X as part of deleting another packed reference. It discovers that X does not refer to a valid object (because the object that it referred to has been garbage collected). * Process 2 tries to delete reference X. It starts by deleting the loose reference X. * Process 1 checks whether there is a loose reference X. There is not (it has just been deleted by process 2), so process 1 reports a spurious error "X does not point to a valid object!" The worst case seems relatively harmless, and the fix is identical to the fix that will be needed for the other race conditions (namely holding a lock on the packed-refs file during *all* reference deletions), so we leave the cleaning up of all of them as a future project. [1] http://thread.gmane.org/gmane.comp.version-control.git/211956 Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * refs: change the internal reference-iteration APIMichael Haggerty2013-05-011-61/+83
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Establish an internal API for iterating over references, which gives the callback functions direct access to the ref_entry structure describing the reference. (Do not change the iteration API that is exposed outside of the module.) Define a new internal callback signature int each_ref_entry_fn(struct ref_entry *entry, void *cb_data) Change do_for_each_ref_in_dir() and do_for_each_ref_in_dirs() to accept each_ref_entry_fn callbacks, and rename them to do_for_each_entry_in_dir() and do_for_each_entry_in_dirs(), respectively. Adapt their callers accordingly. Add a new function do_for_each_entry() analogous to do_for_each_ref() but using the new callback style. Change do_one_ref() into an each_ref_entry_fn that does some bookkeeping and then calls a wrapped each_ref_fn. Reimplement do_for_each_ref() in terms of do_for_each_entry(), using do_one_ref() as an adapter. Please note that the responsibility for setting current_ref remains in do_one_ref(), which means that current_ref is *not* set when iterating over references via the new internal API. This is not a disadvantage, because current_ref is not needed by callers of the internal API (they receive a pointer to the current ref_entry anyway). But more importantly, this change prevents peel_ref() from returning invalid results in the following scenario: When iterating via the external API, the iteration always includes both packed and loose references, and in particular never presents a packed ref if there is a loose ref with the same name. The internal API, on the other hand, gives the option to iterate over only the packed references. During such an iteration, there is no check whether the packed ref might be hidden by a loose ref of the same name. But until now the packed ref was recorded in current_ref during the iteration. So if peel_ref() were called with the reference name corresponding to current ref, it would return the peeled version of the packed ref even though there might be a loose ref that peels to a different value. This scenario doesn't currently occur in the code, but fix it to prevent things from breaking in a very confusing way in the future. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * refs: extract a function peel_entry()Michael Haggerty2013-05-011-14/+49
| | | | | | | | | | | | | | | | | | | | | | | | Peel the entry, and as a side effect store the peeled value in the entry. Use this function from two places in peel_ref(); a third caller will be added soon. Please note that this change can lead to ref_entries for unpacked refs being peeled. This has no practical benefit but is harmless. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * peel_ref(): fix return value for non-peelable, not-current referenceMichael Haggerty2013-05-011-1/+4
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | The old version was inconsistent: when a reference was REF_KNOWS_PEELED but with a null peeled value, it returned non-zero for the current reference but zero for other references. Change the behavior for non-current references to match that of current_ref, which is what callers expect. Document the behavior. Current callers only call peel_ref() from within a for_each_ref-style iteration and only for the current ref; therefore, the buggy code path was never reached. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * peel_object(): give more specific information in return valueMichael Haggerty2013-05-011-8/+24
| | | | | | | | | | | | | | | | | | Instead of just returning a success/failure bit, return an enumeration value that explains the reason for any failure. This will come in handy shortly. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * refs: extract function peel_object()Michael Haggerty2013-05-011-20/+30
| | | | | | | | | | | | | | | | | | | | | | It is a nice, logical unit of work, and putting it in a function removes the need to use a goto in peel_ref(). Soon it will also have other uses. The algorithm is unchanged. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * refs: extract a function ref_resolves_to_object()Michael Haggerty2013-05-011-8/+20
| | | | | | | | | | | | | | | | It is a nice unit of work and soon will be needed from multiple locations. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * repack_without_ref(): use function get_packed_ref()Michael Haggerty2013-05-011-3/+5
| | | | | | | | | | Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * peel_ref(): use function get_packed_ref()Michael Haggerty2013-05-011-3/+2
| | | | | | | | | | Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * get_packed_ref(): return a ref_entryMichael Haggerty2013-05-011-11/+9
| | | | | | | | | | | | | | | | | | | | Instead of copying the reference's SHA1 into a caller-supplied variable, just return the ref_entry itself (or NULL if there is no such entry). This change will allow the function to be used from elsewhere. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * do_for_each_ref_in_dirs(): remove dead codeMichael Haggerty2013-05-011-7/+0
| | | | | | | | | | | | | | | | There is no way to drop out of the while loop. This code has been dead since 432ad41e. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * refs: define constant PEELED_LINE_LENGTHMichael Haggerty2013-05-011-2/+5
| | | | | | | | | | Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * refs: document how current_ref is usedMichael Haggerty2013-05-011-0/+9
| | | | | | | | | | Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * refs: document do_for_each_ref() and do_one_ref()Michael Haggerty2013-05-011-1/+14
| | | | | | | | | | Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * refs: document the fields of struct ref_valueMichael Haggerty2013-05-011-0/+12
| | | | | | | | | | Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * refs: document flags constants REF_*Michael Haggerty2013-05-011-1/+11
| | | | | | | | | | | | | | | | Document the bits that can appear in the "flags" parameter passed to an each_ref_function and/or in the ref_entry::flag field. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * Merge branch 'jk/fully-peeled-packed-ref' into maint-1.8.1Junio C Hamano2013-04-031-5/+44
| |\ | | | | | | | | | | | | | | | | | | | | | * jk/fully-peeled-packed-ref: pack-refs: add fully-peeled trait pack-refs: write peeled entry for non-tags use parse_object_or_die instead of die("bad object") avoid segfaults on parse_object failure
| * \ Merge branch 'jk/repack-ref-racefix' into maintJunio C Hamano2013-01-101-1/+4
| |\ \ | | | | | | | | | | | | | | | | * jk/repack-ref-racefix: refs: do not use cached refs in repack_without_ref