summaryrefslogtreecommitdiff
path: root/lockfile.c
AgeCommit message (Collapse)AuthorFilesLines
2018-05-06Replace all die("BUG: ...") calls by BUG() onesLibravatar Johannes Schindelin1-1/+1
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-06tempfile: auto-allocate tempfiles on heapLibravatar Jeff King1-4/+3
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>
2016-12-07lockfile: LOCK_REPORT_ON_ERRORLibravatar Junio C Hamano1-2/+10
The "libify sequencer" topic stopped passing the die_on_error option to hold_locked_index(), and this lost an error message from "git merge --ff-only $commit" when there are competing updates in progress. The command still exits with a non-zero status, but that is not of much help for an interactive user. The last thing the command says is "Updating $from..$to". We used to follow it with a big error message that makes it clear that "merge --ff-only" did not succeed. What is sad is that we should have noticed this regression while reviewing the change. It was clear that the update to the checkout_fast_forward() function made a failing hold_locked_index() silent, but the only caller of the checkout_fast_forward() function had this comment: if (checkout_fast_forward(from, to, 1)) - exit(128); /* the callee should have complained already */ + return -1; /* the callee should have complained already */ which clearly contradicted the assumption X-<. Add a new option LOCK_REPORT_ON_ERROR that can be passed instead of LOCK_DIE_ON_ERROR to the hold_lock*() family of functions and teach checkout_fast_forward() to use it to fix this regression. After going thourgh all calls to hold_lock*() family of functions that used to pass LOCK_DIE_ON_ERROR but were modified to pass 0 in the "libify sequencer" topic "git show --first-parent 2a4062a4a8", it appears that this is the only one that has become silent. Many others used to give detailed report that talked about "there may be competing Git process running" but with the series merged they now only give a single liner "Unable to lock ...", some of which may have to be tweaked further, but at least they say something, unlike the one this patch fixes. Reported-by: Robbie Iannucci <iannucci@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2016-03-01lockfile: improve error message when lockfile existsLibravatar Matthieu Moy1-3/+5
A common mistake leading a user to see this message is to launch "git commit", let the editor open (and forget about it), and try again to commit. The previous message was going too quickly to "a git process crashed" and to the advice "remove the file manually". This patch modifies the message in two ways: first, it considers that "another process is running" is the norm, not the exception, and it explicitly hints the user to look at text editors. The message is 2 lines longer, but this is not a problem since experienced users do not see the message often. Helped-by: Moritz Neeb <lists@moritzneeb.de> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2016-03-01lockfile: mark strings for translationLibravatar Matthieu Moy1-3/+3
Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-08-28lockfile: remove function "hold_lock_file_for_append"Libravatar Ralf Thielow1-38/+0
With 77b9b1d (add_to_alternates_file: don't add duplicate entries, 2015-08-10) the last caller of function "hold_lock_file_for_append" has been removed, so we can remove the function as well. Signed-off-by: Ralf Thielow <ralf.thielow@gmail.com> Acked-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-08-25Merge branch 'mh/tempfile'Libravatar Junio C Hamano1-182/+23
The "lockfile" API has been rebuilt on top of a new "tempfile" API. * mh/tempfile: credential-cache--daemon: use tempfile module credential-cache--daemon: delete socket from main() gc: use tempfile module to handle gc.pid file lock_repo_for_gc(): compute the path to "gc.pid" only once diff: use tempfile module setup_temporary_shallow(): use tempfile module write_shared_index(): use tempfile module register_tempfile(): new function to handle an existing temporary file tempfile: add several functions for creating temporary files prepare_tempfile_object(): new function, extracted from create_tempfile() tempfile: a new module for handling temporary files commit_lock_file(): use get_locked_file_path() lockfile: add accessor get_lock_file_path() lockfile: add accessors get_lock_file_fd() and get_lock_file_fp() create_bundle(): duplicate file descriptor to avoid closing it twice lockfile: move documentation to lockfile.h and lockfile.c
2015-08-10tempfile: a new module for handling temporary filesLibravatar Michael Haggerty1-245/+16
A lot of work went into defining the state diagram for lockfiles and ensuring correct, race-resistant cleanup in all circumstances. Most of that infrastructure can be applied directly to *any* temporary file. So extract a new "tempfile" module from the "lockfile" module. Reimplement lockfile on top of tempfile. Subsequent commits will add more users of the new module. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-08-10commit_lock_file(): use get_locked_file_path()Libravatar Michael Haggerty1-16/+12
First beef up the sanity checking in get_locked_file_path() to match that in commit_lock_file(). Then rewrite commit_lock_file() to use get_locked_file_path() for its pathname computation. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-08-10lockfile: add accessor get_lock_file_path()Libravatar Michael Haggerty1-0/+7
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-08-10lockfile: add accessors get_lock_file_fd() and get_lock_file_fp()Libravatar Michael Haggerty1-0/+14
We are about to move those members, so change client code to read them through accessor functions. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-08-10lockfile: move documentation to lockfile.h and lockfile.cLibravatar Michael Haggerty1-0/+53
Rearrange/rewrite it somewhat to fit its new environment. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-06-24Merge branch 'js/sleep-without-select'Libravatar Junio C Hamano1-22/+9
Portability fix. * js/sleep-without-select: lockfile: wait using sleep_millisec() instead of select() lockfile: convert retry timeout computations to millisecond help.c: wrap wait-only poll() invocation in sleep_millisec() lockfile: replace random() by rand()
2015-06-05lockfile: wait using sleep_millisec() instead of select()Libravatar Johannes Sixt1-9/+1
Use the new function sleep_millisec() to delay execution for a short time. This avoids the invocation of select() with just a timeout, but no file descriptors. Such a use of select() is quit with EINVAL on Windows, leading to no delay at all. Signed-off-by: Johannes Sixt <j6t@kdbg.org> Reviewed-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-06-05lockfile: convert retry timeout computations to millisecondLibravatar Johannes Sixt1-13/+8
When the goal is to wait for some random amount of time up to one second, it is not necessary to compute with microsecond precision. This is a preparation to re-use sleep_millisec(). Signed-off-by: Johannes Sixt <j6t@kdbg.org> Reviewed-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-06-05lockfile: replace random() by rand()Libravatar Johannes Sixt1-2/+2
On Windows, we do not have functions srandom() and random(). Use srand() and rand(). These functions produce random numbers of lesser quality, but for the purpose (a retry time-out) they are still good enough. Signed-off-by: Johannes Sixt <j6t@kdbg.org> Reviewed-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-05-22Merge branch 'jc/ignore-epipe-in-filter'Libravatar Junio C Hamano1-1/+1
Filter scripts were run with SIGPIPE disabled on the Git side, expecting that they may not read what Git feeds them to filter. We however treated a filter that does not read its input fully before exiting as an error. This changes semantics, but arguably in a good way. If a filter can produce its output without consuming its input using whatever magic, we now let it do so, instead of diagnosing it as a programming error. * jc/ignore-epipe-in-filter: filter_buffer_or_fd(): ignore EPIPE copy.c: make copy_fd() report its status silently
2015-05-19copy.c: make copy_fd() report its status silentlyLibravatar Junio C Hamano1-1/+1
When copy_fd() function encounters errors, it emits error messages itself, which makes it impossible for callers to take responsibility for reporting errors, especially when they want to ignore certain errors. Move the error reporting to its callers in preparation. - copy_file() and copy_file_with_time() by indirection get their own calls to error(). - hold_lock_file_for_append(), when told to die on error, used to exit(128) relying on the error message from copy_fd(), but now it does its own die() instead. Note that the callers that do not pass LOCK_DIE_ON_ERROR need to be adjusted for this change, but fortunately there is none ;-) - filter_buffer_or_fd() has its own error() already, in addition to the message from copy_fd(), so this will change the output but arguably in a better way. Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-05-14lockfile: allow file locking to be retried with a timeoutLibravatar Michael Haggerty1-2/+77
Currently, there is only one attempt to lock a file. If it fails, the whole operation fails. But it might sometimes be advantageous to try acquiring a file lock a few times before giving up. So add a new function, hold_lock_file_for_update_timeout(), that allows a timeout to be specified. Make hold_lock_file_for_update() a thin wrapper around the new function. If timeout_ms is positive, then retry for at least that many milliseconds to acquire the lock. On each failed attempt, use select() to wait for a backoff time that increases quadratically (capped at 1 second) and has a random component to prevent two processes from getting synchronized. If timeout_ms is negative, retry indefinitely. In a moment we will switch to using the new function when locking packed-refs. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2014-11-03lockfile.c: store absolute pathLibravatar Nguyễn Thái Ngọc Duy1-3/+11
Locked paths can be saved in a linked list so that if something wrong happens, *.lock are removed. For relative paths, this works fine if we keep cwd the same, which is true 99% of time except: - update-index and read-tree hold the lock on $GIT_DIR/index really early, then later on may call setup_work_tree() to move cwd. - Suppose a lock is being held (e.g. by "git add") then somewhere down the line, somebody calls real_path (e.g. "link_alt_odb_entry"), which temporarily moves cwd away and back. During that time when cwd is moved (either permanently or temporarily) and we decide to die(), attempts to remove relative *.lock will fail, and the next operation will complain that some files are still locked. Avoid this case by turning relative paths to absolute before storing the path in "filename" field. Reported-by: Yue Lin Ho <yuelinho777@gmail.com> Helped-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk> Helped-by: Johannes Sixt <j6t@kdbg.org> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> Adapted-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2014-10-15lockfile: remove unable_to_lock_errorLibravatar Jonathan Nieder1-10/+0
The former caller uses unable_to_lock_message now. Signed-off-by: Jonathan Nieder <jrnieder@gmail.com> Reviewed-by: Ronnie Sahlberg <sahlberg@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2014-10-14Merge branch 'mh/lockfile-stdio'Libravatar Junio C Hamano1-5/+41
* mh/lockfile-stdio: commit_packed_refs(): reimplement using fdopen_lock_file() dump_marks(): reimplement using fdopen_lock_file() fdopen_lock_file(): access a lockfile using stdio
2014-10-14Merge branch 'mh/lockfile'Libravatar Junio C Hamano1-131/+153
The lockfile API and its users have been cleaned up. * mh/lockfile: (38 commits) lockfile.h: extract new header file for the functions in lockfile.c hold_locked_index(): move from lockfile.c to read-cache.c hold_lock_file_for_append(): restore errno before returning get_locked_file_path(): new function lockfile.c: rename static functions lockfile: rename LOCK_NODEREF to LOCK_NO_DEREF commit_lock_file_to(): refactor a helper out of commit_lock_file() trim_last_path_component(): replace last_path_elm() resolve_symlink(): take a strbuf parameter resolve_symlink(): use a strbuf for internal scratch space lockfile: change lock_file::filename into a strbuf commit_lock_file(): use a strbuf to manage temporary space try_merge_strategy(): use a statically-allocated lock_file object try_merge_strategy(): remove redundant lock_file allocation struct lock_file: declare some fields volatile lockfile: avoid transitory invalid states git_config_set_multivar_in_file(): avoid call to rollback_lock_file() dump_marks(): remove a redundant call to rollback_lock_file() api-lockfile: document edge cases commit_lock_file(): rollback lock file on failure to rename ...
2014-10-08Merge branch 'sp/stream-clean-filter'Libravatar Junio C Hamano1-0/+3
When running a required clean filter, we do not have to mmap the original before feeding the filter. Instead, stream the file contents directly to the filter and process its output. * sp/stream-clean-filter: sha1_file: don't convert off_t to size_t too early to avoid potential die() convert: stream from fd to required clean filter to reduce used address space copy_fd(): do not close the input file descriptor mmap_limit: introduce GIT_MMAP_LIMIT to allow testing expected mmap size memory_limit: use git_env_ulong() to parse GIT_ALLOC_LIMIT config.c: add git_env_ulong() to parse environment variable convert: drop arguments other than 'path' from would_convert_to_git()
2014-10-01fdopen_lock_file(): access a lockfile using stdioLibravatar Michael Haggerty1-5/+41
Add a new function, fdopen_lock_file(), which returns a FILE pointer open to the lockfile. If a stream is open on a lock_file object, it is closed using fclose() on commit, rollback, or close_lock_file(). This change will allow callers to use stdio to write to a lockfile without having to muck around in the internal representation of the lock_file object (callers will be rewritten in upcoming commits). Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2014-10-01lockfile.h: extract new header file for the functions in lockfile.cLibravatar Michael Haggerty1-51/+1
Move the interface declaration for the functions in lockfile.c from cache.h to a new file, lockfile.h. Add #includes where necessary (and remove some redundant includes of cache.h by files that already include builtin.h). Move the documentation of the lock_file state diagram from lockfile.c to the new header file. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2014-10-01hold_locked_index(): move from lockfile.c to read-cache.cLibravatar Michael Haggerty1-8/+0
lockfile.c contains the general API for locking any file. Code specifically about the index file doesn't belong here. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2014-10-01hold_lock_file_for_append(): restore errno before returningLibravatar Michael Haggerty1-1/+8
Callers who don't pass LOCK_DIE_ON_ERROR might want to examine errno to see what went wrong, so restore errno before returning. In fact this function only has one caller, add_to_alternates_file(), and it *does* use LOCK_DIE_ON_ERROR, but, you know, think of future generations. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2014-10-01get_locked_file_path(): new functionLibravatar Michael Haggerty1-0/+9
Add a function to return the path of the file that is locked by a lock_file object. This reduces the knowledge that callers have to have about the lock_file layout. Suggested-by: Ronnie Sahlberg <sahlberg@google.com> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2014-10-01lockfile.c: rename static functionsLibravatar Michael Haggerty1-5/+5
* remove_lock_file() -> remove_lock_files() * remove_lock_file_on_signal() -> remove_lock_files_on_signal() Suggested-by: Torsten Bögershausen <tboegi@web.de> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2014-10-01lockfile: rename LOCK_NODEREF to LOCK_NO_DEREFLibravatar Michael Haggerty1-1/+1
This makes it harder to misread the name as LOCK_NODE_REF. Suggested-by: Torsten Bögershausen <tboegi@web.de> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2014-10-01commit_lock_file_to(): refactor a helper out of commit_lock_file()Libravatar Michael Haggerty1-14/+26
commit_locked_index(), when writing to an alternate index file, duplicates (poorly) the code in commit_lock_file(). And anyway, it shouldn't have to know so much about the internal workings of lockfile objects. So extract a new function commit_lock_file_to() that does the work common to the two functions, and call it from both commit_lock_file() and commit_locked_index(). Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2014-10-01trim_last_path_component(): replace last_path_elm()Libravatar Michael Haggerty1-22/+16
Rewrite last_path_elm() to take a strbuf parameter and to trim off the last path name element in place rather than returning a pointer to the beginning of the last path name element. This simplifies the function a bit and makes it integrate better with its caller, which is now also strbuf-based. Rename the function accordingly and a bit less tersely. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2014-10-01resolve_symlink(): take a strbuf parameterLibravatar Michael Haggerty1-35/+22
Change resolve_symlink() to take a strbuf rather than a string as parameter. This simplifies the code and removes an arbitrary pathname length restriction. It also means that lock_file's filename field no longer needs to be initialized to a large size. Helped-by: Torsten Bögershausen <tboegi@web.de> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2014-10-01resolve_symlink(): use a strbuf for internal scratch spaceLibravatar Michael Haggerty1-21/+12
Aside from shortening and simplifying the code, this removes another place where the path name length is arbitrarily limited. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2014-10-01lockfile: change lock_file::filename into a strbufLibravatar Michael Haggerty1-29/+24
For now, we still make sure to allocate at least PATH_MAX characters for the strbuf because resolve_symlink() doesn't know how to expand the space for its return value. (That will be fixed in a moment.) Another alternative would be to just use a strbuf as scratch space in lock_file() but then store a pointer to the naked string in struct lock_file. But lock_file objects are often reused. By reusing the same strbuf, we can avoid having to reallocate the string most times when a lock_file object is reused. Helped-by: Torsten Bögershausen <tboegi@web.de> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2014-10-01commit_lock_file(): use a strbuf to manage temporary spaceLibravatar Michael Haggerty1-5/+7
Avoid relying on the filename length restrictions that are currently checked by lock_file(). Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2014-10-01struct lock_file: declare some fields volatileLibravatar Michael Haggerty1-1/+1
The function remove_lock_file_on_signal() is used as a signal handler. It is not realistic to make the signal handler conform strictly to the C standard, which is very restrictive about what a signal handler is allowed to do. But let's increase the likelihood that it will work: The lock_file_list global variable and several fields from struct lock_file are used by the signal handler. Declare those values "volatile" to (1) force the main process to write the values to RAM promptly, and (2) prevent updates to these fields from being reordered in a way that leaves an opportunity for a jump to the signal handler while the object is in an inconsistent state. We don't mark the filename field volatile because that would prevent the use of strcpy(), and it is anyway unlikely that a compiler re-orders a strcpy() call across other expressions. So in practice it should be possible to get away without "volatile" in the "filename" case. Suggested-by: Johannes Sixt <j6t@kdbg.org> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2014-10-01lockfile: avoid transitory invalid statesLibravatar Michael Haggerty1-11/+26
Because remove_lock_file() can be called any time by the signal handler, it is important that any lock_file objects that are in the lock_file_list are always in a valid state. And since lock_file objects are often reused (but are never removed from lock_file_list), that means we have to be careful whenever mutating a lock_file object to always keep it in a well-defined state. This was formerly not the case, because part of the state was encoded by setting lk->filename to the empty string vs. a valid filename. It is wrong to assume that this string can be updated atomically; for example, even strcpy(lk->filename, value) is unsafe. But the old code was even more reckless; for example, strcpy(lk->filename, path); if (!(flags & LOCK_NODEREF)) resolve_symlink(lk->filename, max_path_len); strcat(lk->filename, ".lock"); During the call to resolve_symlink(), lk->filename contained the name of the file that was being locked, not the name of the lockfile. If a signal were raised during that interval, then the signal handler would have deleted the valuable file! We could probably continue to use the filename field to encode the state by being careful to write characters 1..N-1 of the filename first, and then overwrite the NUL at filename[0] with the first character of the filename, but that would be awkward and error-prone. So, instead of using the filename field to determine whether the lock_file object is active, add a new field "lock_file::active" for this purpose. Be careful to set this field only when filename really contains the name of a file that should be deleted on cleanup. Helped-by: Johannes Sixt <j6t@kdbg.org> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2014-10-01commit_lock_file(): rollback lock file on failure to renameLibravatar Michael Haggerty1-1/+6
If rename() fails, call rollback_lock_file() to delete the lock file (in case it is still present) and reset the filename field to the empty string so that the lockfile object is left in a valid state. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2014-10-01close_lock_file(): if close fails, roll backLibravatar Michael Haggerty1-10/+18
If closing an open lockfile fails, then we cannot be sure of the contents of the lockfile, so there is nothing sensible to do but delete it. This change also insures that the lock_file object is left in a defined state in this error path (namely, unlocked). The only caller that is ultimately affected by this change is try_merge_strategy() -> write_locked_index(), which can call close_lock_file() via various execution paths. This caller uses a static lock_file object which previously could have been reused after a failed close_lock_file() even though it was still in locked state. This change causes the lock_file object to be unlocked on failure, thus fixing this error-handling path. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2014-10-01commit_lock_file(): die() if called for unlocked lockfile objectLibravatar Michael Haggerty1-0/+3
It was previously a bug to call commit_lock_file() with a lock_file object that was not active (an illegal access would happen within the function). It was presumably never done, but this would be an easy programming error to overlook. So before continuing, do a consistency check that the lock_file object really is locked. Helped-by: Johannes Sixt <j6t@kdbg.org> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2014-10-01commit_lock_file(): inline temporary variableLibravatar Michael Haggerty1-3/+5
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2014-10-01remove_lock_file(): call rollback_lock_file()Libravatar Michael Haggerty1-6/+2
It does just what we need. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2014-10-01lock_file(): exit early if lockfile cannot be openedLibravatar Michael Haggerty1-12/+11
This is a bit easier to read than the old version, which nested part of the non-error code in an "if" block. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Reviewed-by: Ronnie Sahlberg <sahlberg@google.com> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2014-10-01cache.h: define constants LOCK_SUFFIX and LOCK_SUFFIX_LENLibravatar Michael Haggerty1-5/+6
There are a few places that use these values, so define constants for them. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2014-10-01lockfile.c: document the various states of lock_file objectsLibravatar Michael Haggerty1-0/+42
Document the valid states of lock_file objects, how they get into each state, and how the state is encoded in the object's fields. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Reviewed-by: Ronnie Sahlberg <sahlberg@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2014-10-01lock_file(): always initialize and register lock_file objectLibravatar Michael Haggerty1-9/+16
The purpose of this change is to make the state diagram for lock_file objects simpler and deterministic. If locking fails, lock_file() sometimes leaves the lock_file object partly initialized, but sometimes not. It sometimes registers the object in lock_file_list, but sometimes not. This makes the state diagram for lock_file objects effectively indeterministic and hard to reason about. A future patch will also change the filename field into a strbuf, which needs more involved initialization, so it will become even more important that the state of a lock_file object is well-defined after a failed attempt to lock. The ambiguity doesn't currently have any ill effects, because lock_file objects cannot be removed from the lock_file_list anyway. But to make it easier to document and reason about the code, make this behavior consistent: *always* initialize the lock_file object and *always* register it in lock_file_list the first time it is used, regardless of whether an error occurs. While we're at it, make sure that all of the lock_file fields are initialized to values appropriate for an unlocked object; the caller is only responsible for making sure that on_list is set to zero before the first time it is used. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2014-10-01hold_lock_file_for_append(): release lock on errorsLibravatar Michael Haggerty1-2/+2
If there is an error copying the old contents to the lockfile, roll back the lockfile before exiting so that the lockfile is not held until process cleanup. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2014-10-01lockfile: unlock file if lockfile permissions cannot be adjustedLibravatar Michael Haggerty1-0/+1
If the call to adjust_shared_perm() fails, lock_file returns -1, which to the caller looks like any other failure to lock the file. So in this case, roll back the lockfile before returning so that the lock file is deleted immediately and the lockfile object is left in a predictable state (namely, unlocked). Previously, the lockfile was retained until process cleanup in this situation. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>