patchFix handle leak in write_tree

This is a quick and dirty fix for the broken "git cherry-pick -n" on some broken OS, which does not remove the directory entry after unlink succeeded(!) if the file is still open somewher. The entry is left but "protected": no open, no unlink, no stat. Very annoying.

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---

That should be enough to get going, but I have to say that the interface to lockfiles is really troublesome. Why has the caller close a handle it didn't open? Especially if there are perfect matches for the opening function (hold_locked_index) in form of commit and rollback?

How about something like this (just interface):

struct lock_file
{
	struct lock_file *next;
	pid_t owner;
	int fd;
	char on_list;
	char filename[PATH_MAX];
};

struct lock_file *open_locked(const char *path, int die_on_error); struct lock_file *open_index_locked(int die_on_error); void commit_lock_file(struct lock_file *); /* always assuming .lock */ void rollback_lock_file(struct lock_file *);

 builtin-write-tree.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

Alex Riesen <raa.lkml@gmail.com> writes:

I agree that making commit and rollback close the file descriptor and lock holders to use lock->fd for write() makes more sense, although it is a bit unclear from the above set of function signatures what your plan on the lifetime rule for "struct lock_file" is. If it will be linked to the list given to the atexit() handler and the caller of open_locked() never frees it, I think I am fine with the interface.

On 4/24/07, Junio C Hamano <junkio@cox.net> wrote:

I actually expected the caller to define the lifetime. atexit is not exactly libification-effort friendly. I could imagine open*locked with an additional argument for atexit registration, I just don't like the idea (dislike die_on_error too). Isn't such kind of resource control _generally_ nicer to implement in the top levels of a program?

"Alex Riesen" <raa.lkml@gmail.com> writes:

In theory perhaps, but my understanding of our use of atexit() is to clean them up in situations beyond the control of the top levels of a program, most notably upon exit on signal.

Junio C Hamano, Tue, Apr 24, 2007 11:33:48 +0200:

struct git_context;
/* or whatever is the latest name for the repo bookeeping is.
   It have to be passed to every git-related function anyway. */

struct lock_file *open_index_locked(struct git_context *, int die_on_error); int commit_index_locked(struct git_context *, struct lock_file *); ... and so on.

Then let the top level call something like

    git_cleanup(struct git_context *);

in its _own_ signal or atexit handlers. If it didn't setup the handlers, than perhaps it did want it so (leaving tempfiles behind is sometimes done on purpose). Or it _is_ a bug, but then it is clear: you have to cleanup, and you do git's part of cleanup with git_cleanup.

Alex Riesen <raa.lkml@gmail.com> writes:

I would agree that libification would need to go something along that line.

In the meantime for 1.5.2 stabilization, let's do the minimum fix of applying your earlier patch.

This is a quick and dirty fix for the broken "git cherry-pick -n" on some broken OS, which does not remove the directory entry after unlink succeeded(!) if the file is still open somewher. The entry is left but "protected": no open, no unlink, no stat. Very annoying.

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---
Alex Riesen, Mon, Apr 23, 2007 21:49:25 +0200:

BTW, can someone explain this hunk? And how does it manage to pass the tests?! And HOW did it manage to pass me?!!!

 builtin-write-tree.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)