Fix cleanup of temporary files #1227
Conversation
| fi; \ | ||
| if [ "$$should_install" = 'yes' ]; then \ | ||
| echo "... installing $(COMMAND)"; \ | ||
| head -1 bin/$(COMMAND) > $(TEMPFILE); \ |
There was a problem hiding this comment.
What about building the script content in a subshell and redirecting it directly to the destination? This avoids mktemp, chmod on temp files from the beginning.
There was a problem hiding this comment.
Maybe something like this?
- source the
helper/scripts rather than embedding them- add the source command to each script in
bin/
- add the source command to each script in
- install the
helper/scripts in/usr/share/git-extras/helperor similar - separate out a
buildtarget from theinstalltarget - build each of the to-be-installed scripts in a pattern rule that would just add the install path for the
helper/scripts.
There was a problem hiding this comment.
Is this plan a little complicated? We can just use the subshell's stdout as a temp file, so no need to use a real file.
There was a problem hiding this comment.
@spacewander Could we keep using the temporary file? Things kind of look complicated enough with this being embedded in a Makefile and all,.
hyperupcall
left a comment
hyperupcall
left a comment
There was a problem hiding this comment.
Looks great so far! Was a little bothersome to learn that git-extras didn't cleanup properly after execution.
Just had a few minor comments; once addressed, I'll approve!
| fi; \ | ||
| tail -n +2 bin/$(COMMAND) >> $(TEMPFILE); \ | ||
| cp -f $(TEMPFILE) $(DESTDIR)$(BINPREFIX)/$(COMMAND); \ | ||
| rm -f $(TEMPFILE); \ |
There was a problem hiding this comment.
If we're adding this, could this go after the @foreach and before the @if [ -z "$(wildcard man/git-*.1)" ]; then \? The temporary file is created outside of the loop, not inside,.
| fi; \ | ||
| if [ "$$should_install" = 'yes' ]; then \ | ||
| echo "... installing $(COMMAND)"; \ | ||
| head -1 bin/$(COMMAND) > $(TEMPFILE); \ |
There was a problem hiding this comment.
@spacewander Could we keep using the temporary file? Things kind of look complicated enough with this being embedded in a Makefile and all,.
|
|
||
| set -euo pipefail | ||
|
|
||
| proceed=false |
There was a problem hiding this comment.
Could this be moved out to a separate PR? For matching the argument, would prefer using $1 == @(--proceed|-p) (enabling extglob) or keeping the existing structure and using group commands ({ true && true; }) instead of subshells.
|
|
||
| if [[ -n "$DEBUG" ]] ; then | ||
| sort -nr "$DEBUG" && | ||
| rm -f "$DEBUG" |
There was a problem hiding this comment.
Shouldn't DEBUG not be removed, so that the user could see the debug output after git guilt runs?
hyperupcall
changed the title
4 participants
No description provided.