Junio C Hamano wrote:
> Brandon Casey <casey@xxxxxxxxxxxxxxx> writes:
>
>> My patch does this, though I understand it may take some time to review.
>
> This is what I have right now, squashed your change into [2/2]
> I sent earlier, along with a couple of further fixups.
>
Mainly, I prefer to not modify the data structures when a failure occurs.
Generally when commit_lock_file fails, the caller die()'s and then
remove_lock_file will delete the temporary file. If the caller were to
not call die, I think it should call rollback_lock_file() similarly to
how unlock_ref() is called everywhere in refs.c.
So I think the semantics should be:
lock_fd = hold_lock_file(&lock);
<do_something>
if (close_lock_file(&lock)) {
rollback_lock_file(&lock);
return error;
}
if (commit_lock_file(&lock)) {
rollback_lock_file(&lock);
return error;
}
> diff --git a/lockfile.c b/lockfile.c
> index f45d3ed..fcf9285 100644
> --- a/lockfile.c
> +++ b/lockfile.c
> @@ -13,7 +13,8 @@ static void remove_lock_file(void)
> while (lock_file_list) {
> if (lock_file_list->owner == me &&
> lock_file_list->filename[0]) {
> - close(lock_file_list->fd);
> + if (lock_file_list->fd >= 0)
> + close(lock_file_list->fd);
> unlink(lock_file_list->filename);
> }
> lock_file_list = lock_file_list->next;
> @@ -159,11 +160,23 @@ int hold_lock_file_for_update(struct lock_file *lk, const char *path, int die_on
> return fd;
> }
>
> +int close_lock_file(struct lock_file *lk)
> +{
> + int fd = lk->fd;
> + lk->fd = -1;
> + return close(fd);
> +}
minor nit on this that I mentioned in another email.
> +
> int commit_lock_file(struct lock_file *lk)
> {
> char result_file[PATH_MAX];
> int i;
> - close(lk->fd);
> +
> + if (lk->fd >= 0 && close_lock_file(lk)) {
> + unlink(lk->filename);
> + lk->filename[0] = 0;
> + return -1;
> + }
I would rather have the caller call rollback_lock_file, or
fall back to remove_lock_file rather than do this unlinking
and modifying lk->filename here in commit_lock_file.
> strcpy(result_file, lk->filename);
> i = strlen(result_file) - 5; /* .lock */
> result_file[i] = 0;
> @@ -185,9 +198,15 @@ void set_alternate_index_output(const char *name)
> int commit_locked_index(struct lock_file *lk)
> {
> if (alternate_index_output) {
> - int result = rename(lk->filename, alternate_index_output);
> - lk->filename[0] = 0;
> - return result;
> + const char *newname = alternate_index_output;
> + alternate_index_output = NULL;
> +
> + if (lk->fd >= 0 && close_lock_file(lk)) {
> + unlink(lk->filename);
> + lk->filename[0] = 0;
> + return -1;
> + }
ditto here.
> + return rename(lk->filename, newname);
If rename succeeds, we'll try to do an unnecessary unlink atexit.
Having said those things, I will defer to your more experienced judgment.
-brandon
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
