Re: [PATCH 02/14] tempfile: a new module for handling temporary files

[Junio C Hamano <gitster@pobox.com>]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> 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.

> @@ -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.