hook: fix newlines in command missing message #5886
Conversation
xdavidwu
force-pushed
the
hook-command-missing-message
branch
from
20f3166 to
1387903
Compare
|
Hey, thanks for the contribution and welcome to the Git LFS project! On a first look this appears to be a valuable and comprehensive set of changes, which is really great. I'd like to take a few days to reflect on whether there are any other changes we should include the hooks at the same time, or anything I didn't see on a first review, but either way, I expect we'll be able to merge this shortly. In the meantime I'll get the CI suite started on this PR. Thanks again very much! |
|
Just a note re the CI builds—it appears GitHub Actions have upgraded their default runners to Ubuntu 24.04, and we'll have to adjust one of our build scripts for that change, so for now, just ignore those CI failures. I'll get a patch up to deal with the CI breakage shortly. |
xdavidwu
force-pushed
the
hook-command-missing-message
branch
from
1387903 to
323ba2e
Compare
|
Thanks for your patience! I haven't had a chance to circle back past this PR yet, and while I've resolved our CI problems with Ubuntu 24.04, unfortunately our CI suite has broken once again, this time due to a small upstream change in Git's |
chrisd8088
left a comment
chrisd8088
left a comment
There was a problem hiding this comment.
Thank you for this improvement! The hook script changes look good to me.
I did notice that we have an extra check in our existing t/t-update.sh test script at the end of the replace old hook 6 section in the update test, and that we're missing the same check at the end the replace old hook 5 section in the test. This PR then copies the duplicate check another time into its new replace old hook 7 section.
Would you mind either adding a commit to this PR to address those issues, with a bit of a description in the commit message to explain the history, or granting me permission to push a commit to your branch to do the same?
Aside from that, this all seems ready to merge. Thank you again for the contribution!
Under most shells `echo "\n"` produces \n, not a newline. This fixes it via moving to printf, which should be more portable and is encouraged by POSIX.
xdavidwu
force-pushed
the
hook-command-missing-message
branch
from
7f4e8d2 to
a18876b
Compare
chrisd8088
left a comment
chrisd8088
left a comment
There was a problem hiding this comment.
Thank you for the extra commit; this looks great!
2 participants
Under most shells
echo "\n"produces \n, not a newline. This fixes itvia moving to printf, which should be more portable and is encouraged by
POSIX.