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

From: Tobias Klauser <tklauser@distanz.ch>
Date: Jan 15, 2016, 10:34 UTC
Message-ID: <20160115103402.GC21205@distanz.ch>
In-Reply-To: <xmqqio2vki0i.fsf@gitster.mtv.corp.google.com>

On 2016-01-14 at 21:45:01 +0100, Junio C Hamano <gitster@pobox.com> wrote:

Show 17 quoted lines

> Tobias Klauser <tklauser@distanz.ch> writes:
> 
> > 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.

Thanks a lot for your review!

Show 29 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".

Classical case of copy&paste and me not thinking enough what could be simplified ;)

Show 20 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 ...

Indeed, I forgot about this. I saw you already folded in the missing 'chmod +r message' in your tree. Thanks for that!

Show 7 quoted lines

> 
> > +	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?

The check for S_ISREG I added because it's also done in sed:

	$ mkdir /tmp/foobar
	$ sed -i 's/foo/baz/' /tmp/foobar
	sed: couldn't edit foobar: not a regular file

I quickly checked their source and they do indeed a check for S_ISREG in case of in-place editing (sed v4.2.2-98-g61c0a53ec997, sed/execute.c:600)

But the writable check is probably too strict, I agree.

Show 26 quoted lines

> 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.

True. AFAIK rename(2) only need write permissions on the containing directory, not the source/destination file itself. And rename_tempfile should barf about that. So the S_IWUSR is unnecessarily strict...

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

This should already be the case to to st.st_mode being passed to xmks_tempfile_m, no?

> 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.

I agree.

Show 12 quoted lines

> > +	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.

Thought about this for a while too, but I concluded that it would hurt readability more than it would help.