On 2016-01-14 at 21:45:01 +0100, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Tobias Klauser <tklauser@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
>
> 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!
> > 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 ;)
> > +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!
>
> > + 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.
> 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.
> > + 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html