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