Re: [PATCH 02/14] tempfile: a new module for handling temporary files
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 10, 2015, 17:36 UTC
- Message-ID
- <xmqqy4jrlb32.fsf@gitster.dls.corp.google.com>
- In-Reply-To
- <f7681cfc8ee4f1f5eba50f315f8e0ed0fed883a8.1433751986.git.mhagger@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
Show 12 quoted lines
> diff --git a/builtin/add.c b/builtin/add.c > index df5135b..aaa9ce4 100644 > --- a/builtin/add.c > +++ b/builtin/add.c > @@ -5,6 +5,7 @@ > */ > #include "cache.h" > #include "builtin.h" > +#include "tempfile.h" > #include "lockfile.h" > #include "dir.h" > #include "pathspec.h"
It is a bit sad that all users of lockfile.h has to include tempfile.h; even when trying to find out something as basic as the name of the file on which the lock is held, they need to look at lk->tempfile.filename and that requires inclusion of tempfile.h
It is a good idea to have tempfile as a separate module, as it allows new callers to use the same "clean-on-exit" infrastructure on things that are not locks, i.e. they can include tempfile.h without having to include lockfile.h, but I have to wonder if it is better to include tempfile.h from inside lockfile.h (which is alrady done) and allow users of lockfile API to assume that inclusion will always stay there. After all, if they are taking locks, they already know lk->tempfile is the mechanism through which they need to learn about various aspects of the underlying files.
Show 28 quoted lines
> @@ -101,60 +72,17 @@ static void resolve_symlink(struct strbuf *path)
> /* Make sure errno contains a meaningful value on error */
> static int lock_file(struct lock_file *lk, const char *path, int flags)
> {
> ...
> + int fd;
> + struct strbuf filename = STRBUF_INIT;
>
> - if (flags & LOCK_NO_DEREF) {
> - strbuf_add_absolute_path(&lk->filename, path);
> - } else {
> - struct strbuf resolved_path = STRBUF_INIT;
> + strbuf_addstr(&filename, path);
> + if (!(flags & LOCK_NO_DEREF))
> + resolve_symlink(&filename);
>
> - strbuf_add(&resolved_path, path, pathlen);
> - resolve_symlink(&resolved_path);
> - strbuf_add_absolute_path(&lk->filename, resolved_path.buf);
> - strbuf_release(&resolved_path);
> - }
> ...
> - return lk->fd;
> + strbuf_addstr(&filename, LOCK_SUFFIX);
> + fd = create_tempfile(&lk->tempfile, filename.buf);
> + strbuf_release(&filename);
> + return fd;
> }This was the only part of this entire patch that needed more than cursory reading ;-) and it looks correct.
Thanks.