Re: [PATCH] Improved error messages when temporary file creation fails

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Dec 18, 2010, 20:05 UTC
Message-ID: <20101218200516.GA10031@burratino>
In-Reply-To: <20101218165514.GT25767@bzzt.net>

Hi again,

Arnout Engelen wrote:

> Updated version of the patch

Almost there.  Some remaining nits from my pov:

> , taking into account the feedback from this 
> thread.

[...]

This text above the "---" becomes part of the log message when a patch is committed to git.git, so it is best to make it self-contained. The usual advice is "describe the current behavior, why the proposed behavior is better, and then how the proposed behavior is achieved."

Show 8 quoted lines

> --- /dev/null
> +++ b/test-mktemp.c
> @@ -0,0 +1,15 @@
> +/*
> + * test-mktemp.c: code to exercise the creation of temporary files
> + */
> +#include <string.h>
> +#include "git-compat-util.h"

git-compat-util.h takes care of portably including system headers in the right order. (For example, #include-ing <string.h> before setting _POSIX_SOURCE will cause some symbols not to be defined in _other_ headers on some operating systems, iirc.) I'd suggest removing the redundant #include <string.h>.

See Documentation/CodingGuidelines.

Show 6 quoted lines

> +int main(int argc, char *argv[])
> +{
> +	if (argc != 2)
> +		usage("Expected 1 parameter defining the temporary file template");
> +
> +	xmkstemp(strdup(argv[1]));

Why not xstrdup(), which diagnoses allocation failures?

Show 15 quoted lines

> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -196,10 +196,22 @@ FILE *xfdopen(int fd, const char *mode)
>  int xmkstemp(char *template)
>  {
>  	int fd;
> +	char origtemplate[PATH_MAX];
> +	strlcpy(origtemplate, template, sizeof(origtemplate));
>  
>  	fd = mkstemp(template);
> -	if (fd < 0)
> -		die_errno("Unable to create temporary file");
> +	if (fd < 0) {
> +		int saved_errno = errno;
> +		const char * nonrelative_template;

It would be more usual not to include a space between the '*' and 'nonrelative_template'.

Thanks for your perseverance.