Re: [Nano-devel] [PATCH v2] pull in the futimens module from gnulib

[Benno Schulenberg]

On Mon, Apr 3, 2017, at 10:13, Kamil Dudka wrote:
>  /* Read from inn, write to out.  We assume inn is opened for reading,
>   * and out for writing.  We return 0 on success, -1 on read error, or -2
> - * on write error. */
> -int copy_file(FILE *inn, FILE *out)
> + * on write error.  inn is always closed by this function, out is closed
> + * only if close_out is true. */
> +int copy_file(FILE *inn, FILE *out, bool close_out)
>  {
>      int retval = 0;
>      char buf[BUFSIZ];
>      size_t charsread;
> +    int (*flush_out_fnc)(FILE *) = (close_out) ? fclose : fflush;
>  
>      assert(inn != NULL && out != NULL && inn != out);
>  
> @@ -1564,7 +1566,7 @@ int copy_file(FILE *inn, FILE *out)
>  
>      if (fclose(inn) == EOF)
>       retval = -1;
> -    if (fclose(out) == EOF)
> +    if (flush_out_fnc(out) == EOF)
>       retval = -2;

Why is the flush needed?  Could the timestamp get changed if
the actual writing happened a few seconds later?

> -     struct utimbuf filetime;
> +     static struct timespec filetime[2];

Why does it need to be static?

> -     filetime.actime = openfile->current_stat->st_atime;
> -     filetime.modtime = openfile->current_stat->st_mtime;
> +     filetime[/* atime */ 0].tv_sec = openfile->current_stat->st_atime;
> +     filetime[/* mtime */ 1].tv_sec = openfile->current_stat->st_mtime;

I don't think the comments are needed; it's obvious enough by
what gets assigned to them.

>       /* Copy the file. */
> -     copy_status = copy_file(f, backup_file);
> +     copy_status = copy_file(f, backup_file, /* close_out */ false);

Here the inline comment is confusing, because nano doesn't
use them anywhere else.  Boolean values are always uppercase.

Benno

-- 
http://www.fastmail.com - Or how I learned to stop worrying and
                          love email again