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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 14, 2016, 20:45 UTC
Message-ID: <xmqqio2vki0i.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1452790676-11937-3-git-send-email-tklauser@distanz.ch>

Tobias Klauser <tklauser@distanz.ch> writes:

Show 11 quoted lines

> Add a command line option --in-place to support in-place editing akin to
> sed -i.  This allows to write commands like the following:
>
>   git interpret-trailers --trailer "X: Y" a.txt > b.txt && mv b.txt a.txt
>
> in a more concise way:
>
>   git interpret-trailers --trailer "X: Y" --in-place a.txt
>
> Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
> ---

Thanks, will replace. I found some micronits, none of which I think is big enough to require another reroll, but since I found them already, I'll just point them out.

Show 26 quoted lines

> diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
> index 322c436a494c..aee785cffa8d 100755
> --- a/t/t7513-interpret-trailers.sh
> +++ b/t/t7513-interpret-trailers.sh
> @@ -326,6 +326,46 @@ test_expect_success 'with complex patch, args and --trim-empty' '
>  	test_cmp expected actual
>  '
>  
> +test_expect_success 'in-place editing with basic patch' '
> +	cat basic_message >message &&
> +	cat basic_patch >>message &&
> +	cat basic_message >expected &&
> +	echo >>expected &&
> +	cat basic_patch >>expected &&
> +	git interpret-trailers --in-place message &&
> +	test_cmp expected message
> +'
> +
> +test_expect_success 'in-place editing with additional trailer' '
> +	cat basic_message >message &&
> +	cat basic_patch >>message &&
> +	cat basic_message >expected &&
> +	echo >>expected &&
> +	cat >>expected <<-\EOF &&
> +		Reviewed-by: Alice
> +	EOF

The "echo" is not needed, if you just include a leading blank line in the here-document you use with this "cat".

Show 7 quoted lines

> +test_expect_success POSIXPERM,SANITY "in-place editing doesn't clobber original file on error" '
> +	cat basic_message >message &&
> +	chmod -r message &&
> +	test_must_fail git interpret-trailers --trailer "Reviewed-by: Alice" --in-place message &&
> +	chmod +r message &&
> +	test_cmp message basic_message
> +'

If for some reason interpret-trailers fails to fail, this would leave an unreadable 'message' in the trash directory. Maybe no other tests that come after this one want to be able to read the contents of the file right now, but this is an accident waiting to happen:

	cat basic_message >message &&
+       test_when_finished "chmod +r message" &&
        chmod -r message &&
        test_must_fail ... &&
	chmod +r message &&
        test_cmp ...

> +	if (!S_ISREG(st.st_mode))
> +		die(_("file %s is not a regular file"), file);
> +	if (!(st.st_mode & S_IWUSR))
> +		die(_("file %s is not writable by user"), file);

Hmph, are these two necessary, and do they make sense?

When doing an in-place thing, the primary thing you care about is that you can read from the file and you can deposit the result of the rewrite under the original name. If for some reason a system allowed you to read from a non-regular file and interpret-trailers can do a sensible thing to the contents you read from there, do you have to insist that original must be S_ISREG()? Also, a funny file (e.g. "interpret-trailers -i .") is likely to fail on the input side.

For the latter,

    $ chmod a-w COPYING
    $ sed -i -e 's/a/b/' COPYING

seems to succeed _and_ leave the permission bits intact, i.e. I get this before and after "sed -i"

    $ ls -l COPYING
    -r--r----- 1 jch eng 18765 Jan 14 12:34 COPYING

which hints at two points:

 - The users (of "sed -i") may have demanded that in-place update of
   read-only file must be allowed, and there may have been a good
   reason for wanting to do so.  That reason may apply equally to us
   here.

 - If we were to follow suit, then we should not forget to restore
   the permission bits on the new file.

In any case, these are something we could loosen after people gain experience with the feature, so I think it is OK as-is, at least for now.

> +	if (in_place)
> +		if (rename_tempfile(&trailers_tempfile, file))
> +			die_errno(_("could not rename temporary file to %s"), file);
> +

I briefly wondered if this should be

	if (in_place && rename_tempfile(...))
		die_errno(...);

to save one indentation level, but I think it is a bad idea, i.e. the above code should stay as-is.