Re: [PATCH v2] gc: use temporary file for editing crontab

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

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> While cron is specified by POSIX, there are a wide variety of
> implementations in use.

I would innsert: 

    "git maintenance" assumes that the "crontab" command can be fed
    from its standard input the new contents and the syntax to do so
    is not to have any filename argument, as POSIX describes.
    However,

here and downcase "O" in "On FreeBSD".

> On FreeBSD, the cron implementation
> requires a file name argument: if the user wants to edit standard
> input, they must specify "-".  

> However, this notation is not
> specified by POSIX, allowing the possibility that making such a
> change may break other, less common implementations.

And to avoid two However's in a row, perhaps

    Unfortunately, POSIX systems do not have to interpret "-" on the
    command line of crontab as a request to read from the standard
    input.  Blindly adding "-" on the command line would not work as
    a general solution.

> Since POSIX tells us that cron must accept a file name argument, let's
> solve this problem by specifying a temporary file instead.  This will
> ensure that we work with the vast majority of implementations.
>
> Note that because delete_tempfile closes the file for us, we should not
> call fclose here on the handle, since doing so will introduce a double
> free.
>
> Reported-by: Renato Botelho <garga@FreeBSD.org>
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
> Changes from v1:
>
> * Use `goto out;` in additional places.
> * Fix broken test.
> * Use `delete_tempfile`.
> * Improve commit message to mention `fclose` rationale.

Yup.  All nicely done.

>  builtin/gc.c            | 39 +++++++++++++++++++++++----------------
>  t/helper/test-crontab.c |  4 ++--
>  2 files changed, 25 insertions(+), 18 deletions(-)

Will queue.  Thanks.