Re: [PATCH 2/2] interpret-trailers: add option for in-place editing

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Jan 11, 2016, 16:33 UTC
Message-ID: <vpqziwc3wjv.fsf@anie.imag.fr>
In-Reply-To: <1452519213-1819-3-git-send-email-tklauser@distanz.ch>

Tobias Klauser <tklauser@distanz.ch> writes:

Show 6 quoted lines

> @@ -843,7 +844,9 @@ static void free_all(struct trailer_item **first)
>  	}
>  }
>  
> -void process_trailers(const char *file, int trim_empty, struct string_list *trailers)
> +static struct tempfile trailers_tempfile;

Does this need to be a static global? I'd rather have this be a local variable of process_trailers.

> +			die_errno(_("could not fdopen tempfile"));

I think you should spell it "could not open temporary file" to be more user-friendly.

Show 8 quoted lines

> @@ -872,5 +900,10 @@ void process_trailers(const char *file, int trim_empty, struct string_list *trai
>  	/* Print the lines after the trailers as is */
>  	print_lines(outfile, lines, trailer_end, INT_MAX);
>  
> +	if (in_place) {
> +		if (rename_tempfile(&trailers_tempfile, file))
> +			die_errno(_("could not rename tempfile"));
> +	}

When this happens, I think you also want to try removing the temporary file. Not sure, though: it may be nice to leave the tempfile for the user to debug. What do we do in other places of the code?

It may help the user to get "could not rename temporary file %s to %s" in case this happens.

On overall, the split makes the series much more pleasant to review, and other than these details, this sounds good to me. Thanks!

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/