Check for executable bits that disagree with shebangs

[EliahKagan]

As discussed in comments in #1589, this adds a shell script to report .sh files that have #! but are not executable, or that don't have #! but are executable. It also adds a justfile recipe to call it, and a CI job, though I suspect it might be slightly preferable to include it in or combine it with another CI job. Both for mode metadata and for file (blob) contents, this uses git to get information from the repository reflecting what is committed and staged, but not any unstaged files or changes.

This found five fixture scripts that should have been marked executable but weren't. The opposite situation was not found, so I temporarily created some files to test it (see commit messages for details) and then removed them. The five fixture scripts that lacked +x now have it, so the new script reports success when run locally as well as when run on CI. The new script does not modify the repository; I ran chmod to add +x to the files it found.

Not all .sh files in the repository have, or should have, executable permissions, because some are "libraries" rather than "programs": in particular, the journey tests have files they source, that only make sense to source and should not be run, and that do not have shebangs. The new script does not misdetect these as wrong, because their shebang status and executable status agree: they have no shebangs, and are not marked executable.

---

Ideally we would scan not just .sh files but all files that Git considers to be text: those that are marked as such with .gitattributes, as well as all files where this status is not specified by .gitattributes but that Git auto-detects as text. After all, if a .rs file were given +x permissions, then that would be a mistake, which would be convenient to catch. We may also have other scripts besides shell scripts in the project at some point. Furthermore, justfile is not currently scanned.

Since all the mistakes of the kind this catches have happened in .sh files, I think this change as it stands may be worthwhile. I am interested in extending this to operate on all files Git considers to be text in its own operations. But my thinking is that it makes sense to put that off until I've decided whether I want to rewrite this in Rust. Maybe it will also have bearing on whether this should be rewritten in Rust.

[Byron]

Thanks a lot for your help, it's much appreciated!

Since all the mistakes of the kind this catches have happened in .sh files, I think this change as it stands may be worthwhile. I am interested in extending this to operate on all files Git considers to be text in its own operations. But my thinking is that it makes sense to put that off until I've decided whether I want to rewrite this in Rust. Maybe it will also have bearing on whether this should be rewritten in Rust.

I agree, and am happy to start with *.sh only as this is the most common filetype where executable bits are (mis)placed. Going beyond that seems more niche, but I wouldn't oppose it. Rewriting the script in Rust can always be done even with the *.sh filter, it will be interesting to see if it can be as sleek as the shell script. Even if not as sleek, I can imagine it to be more readable as parts of the script seem like black magic to me. od anyone ;)?

[EliahKagan]

od anyone ;)?

I put the actual and expected bytes in symbolic form for clearer debug output when running the script with bash -x, so I used od. But that isn't essential, and rather than an od -N2 command, it would be okay to read the bytes that shall be compared to #! using a dd bs=2 count=1 command instead. 😁

Various approaches with sed, awk, or even read are possible, though I think it would be less clear that they are correct, compared to od or dd.

So, yes, this is an area where it might become more intuitive when implemented in Rust. But really the part that I don't like is:

gitoxide/etc/check-mode.sh

Lines 6 to 7 in 34efe03

	root_padded="$(git rev-parse --show-toplevel && echo -n .)"
	root="${root_padded%$'\n.'}"

There's a lot of code in the world that uses $(git rev-parse --show-toplevel), and most of it is probably fine. Nonetheless, tools ought to tolerate names that end in a newline character, or at least avoid operating on a different location instead. The command-substitution behavior of removing multiple trailing newlines (if present) makes it hard to do this elegantly and correctly in a shell script. It may be that git rev-parse has something for this, but if so, I am not aware of it. In particular, --sq and --sq-quote do not help with this.

Process substitution does not remove any newlines. It can be used, followed by the removal of exactly one newline character, but this isn't really less complicated or less confusing than that code. I think this is more natural to express in most languages, including Rust.

[Byron]

Ignorance is a bliss. I am not aware of this, so I wouldn't have cared for it, and it should still have worked fine as long as gitoxide isn't renamed to gitoxide\n\n?
Edit: Local clones could of course be located anywhere, but there I also wonder how newlines would make it into the name of the worktree checkout.

[EliahKagan]

In the rest of this comment, I always use \n to mean a newline character and \0 to mean a null character, rather than those literal sequences. The only exception is in code blocks rendered by GitHub from hyperlinks, where of course whatever appears literally is what is in the code the link points to.

In a Git repository foo, a successful run of git rev-parse --show-toplevel will output something like /path/to/foo\n. What makes command substitution usually work properly for this, and many, commands, is that it removes the trailing newline. That is, "$(git rev-parse --show-toplevel)" expands to /path/to/foo, not to /path/too/foo\n. This is usually wanted.

The problem is that command substitution removes any number of trailing newlines. So in a repository foo\n, a successful run of git rev-parse --show-toplevel outputs something like /path/to/foo\n\n, but "$(git rev-parse --show-toplevel)" still expands to /path/to/foo. But we would want it to expand to /path/to/foo\n in that scenario: we would want it to remove only one newline.

This is only a problem for trailing newlines. Newlines that have any non-newline character anywhere after them, including consecutive newlines that have a non-newline character anywhere after them, are not a problem. It is rare for a filename to end in \n, but it could happen.

gitoxide\n would be sufficient to trigger the problem, if command substitution is used in a way that does not avoid it. But even a single trailing newline is unusual. For example, in a local clone, the only way a newline would make it into the name is if the name of the repository being cloned had a newline in it, or if the user specified a differently named directory that contained a newline. (Even then, this command substitution issue would only be a problem when a name is used that ends in a newline.)

This problem is usually minor, because it is rarely triggered and the impact of it being triggered is usually limited, and the case can sometimes be made for not bothering to avoid it. I especially try to avoid it in scripts that make changes. Having already addressed it in copy-packetline.sh in #1340, I decided to address it here in check-mode.sh too and to do it the same way. Because of the lower complexity and lower stakes of this script, I didn't make a specialized function or write a custom error message here. So the code there is perhaps clearer:

gitoxide/etc/copy-packetline.sh

Lines 14 to 23 in 34efe03

	function chdir_toplevel () {
	local root_padded root
	# Find the working tree's root. (Padding covers the trailing-newline case.)
	root_padded="$(git rev-parse --show-toplevel && echo -n .)" ||
	fail 'git-rev-parse failed to find top-level dir'
	root="${root_padded%$'\n.'}"
	cd -- "$root"
	}

In both scripts, the problem is avoided. In a repository foo\n or bar\n\n, a successful run of git rev-parse --show-toplevel && echo -n . outputs something like /path/to/foo\n. or /path/to/bar\n\n.. In command substitution, all trailing newlines are removed, but there are no trailing newlines. All newlines from git, however few or many, are preserved, because none are trailing, because a . appears after all of them in the output of the command run via command substitution.

(This would also work without passing -n to echo--the trailing newline added by echo would be removed, but the newline that is not trailing because of the . added after it still would not be--but it might be slightly more confusing to think about.)

Then, of course, it is necessary to remove \n. from the end, so that we preserve exactly the trailing newlines, if any, that are really from the path. Parameter expansion of root_padded with % followed by text that itself expands to \n. achieves this.

This is not the only way to deal with the problem. For example, process substitution can be used, which preserves all newlines. Then one can make sure to remove a single newline. But because one must still remove the newline, using process substitution does not necessarily entail simpler code.

I do not recommend being especially worried about this problem. However, what makes it even easier not to be worried is if one avoids it. So one of my goals is to make it easier to be confident about the code's correctness. Or, more importantly, to be able to place one's attention on other areas of the code, if any, that are suspected to be wrong.

The treatment of trailing newlines is not the only modification performed in command substitution on output. bash drops \0 automatically, and warns about it:

$ printf 'ab\0cd\n' | od -An -ta
   a   b nul   c   d  nl
$ echo -n "$(printf 'ab\0cd\n')" | od -An -ta
bash: warning: command substitution: ignored null byte in input
   a   b   c   d

But the behavior of dropping \0 is usually not a problem, since often it is known that \0 is absent, or the use case does not depend on it being preserved. Maybe more significantly, unlike the treatment of trailing newlines, the treatment of \0 isn't really something that makes command substitution trickier than other constructs: there are a lot of situations where \0 is not preserved, though the more common treatment is to also drop everything appearing after it.

[Byron]

Thanks a lot for elaborating!
I completely forgot about the documented functional version in copy-packetline.sh.

In any case, I think the explainer really would be great in its own "What you probably didn't know about bash" series :D.

I do not recommend being especially worried about this problem. However, what makes it even easier not to be worried is if one avoids it. So one of my goals is to make it easier to be confident about the code's correctness. Or, more importantly, to be able to place one's attention on other areas of the code, if any, that are suspected to be wrong.

In this moment I think I'd definitely write it in Rust, just to not have to worry about that particular issue (and probably many more) anymore.

[EliahKagan]

(and probably many more)

Yes, I have opened #1709 to fix a mistake I made in the script. 🤦‍♂️