Add missing +x bit on scripts that are run and not sourced #1345
Conversation
This adds executable permissions to shell scripts that begin with a `#!` and are meant to be run rather than sourced into another script. Most of these already had those set, so the inconsistency of having a handful of them unset does not appear intended. The purpose of this change is to make clearer which .sh files are meant to be run directly in a new shell process (which is most, but not all, of them), and which are meant to be sourced into another script rather than run (i.e., those that are script "libraries"). Executable permissions are intentionally not added to .sh files without shebangs that are meant for sourcing rather than running. This was not causing test failures because the way the scripts were invoked did not rely on being able to directly execute them. Nonetheless, it is clearest for these metadata to match the scripts' shebangs. (Scripts that shouldn't have the executable bit set should likewise not have shebangs, but there didn't appear to be cases where scripts wrongly had shebangs or wrongly had +x set.) A further benefit is that executable scripts are traditionally named with no special suffix while files meant for sourcing into a shell are traditionally named with a .sh suffix. Since that is not the convention followed here, setting the executable bits may be especially useful to clarify intent. This commit also removes doubled end-of-file newlines (where a file ended in two newline characters instead of just one) where present in a few files whose permissions were being changed. The rationale for including that here, even though no other script cleanup is done here, is that sometimes this happens as an overcorrection when editing scripts on Windows, where many editors don't automatically add even a single end-of-file newline. Windows is also where intended executable bits are sometimes omitted. That, in turn, is due to the absence of a Unix-style chmod, and how Cygwin-like environments infer likely executable permissions from shebangs rather than file metadata. (git still tracks them, but Windows's own permissions system differs). Files whose metadata were not being changed were not checked for extra newlines, nor otherwise edited, here.
There was a problem hiding this comment.
Thanks a lot for cleaning this up, it's much appreciated! I also enjoyed reading the rationale, I learned something.
The reason I have not included that change is that there is already specific and intentional handling of line endings for shell scripts in
.gitattributesfiles in this project, which I think I may not fully understand. I think the simpler rule*.sh text=auto eol=lfmay be sufficient to achieve those goals, but I am not sure.If such a change is wanted then I could add another commit here for it, or do it in a separate PR.
I'd very much appreciated cleaning up the attributes on shell-scripts! The reason they are like they are is me not being fully aware of what's truly needed, and me not working on Windows so I wouldn't really find out.
It's notable that the way the fixture-system is designed, it won't have to actually execute most scripts on Windows anyway as it instead decompresses an archive that has been created on a unix system. This is usually done in the name of performance, but also when it's considered to have no effect.
However, since you are working on Windows and know everything about its differences and subtleties, it might be better for validating system compatibility in tests if Windows would not rely on Unix archives as much. Again, these are created for performance as scripts take some time to run, particularly on Windows, but having done so whenever possible might have introduced a smoother cross-platform testing experience than would otherwise be the case.
For me, testing and making changes for Windows is rather painful, so I was OK with taking that shortcut. But I am also very open to seeing this changed.
A way to find out about differences between the archive and actual script execution would be to set GIX_TEST_IGNORE_ARCHIVES=1 and try to run the test suite. It might fail because there are differences in expected outcomes, or because it can't actually execute the fixture scripts at all. Ideally, the archives are just for performance optimization, and all scripts should work.
Lastly, should you decide to take on such investigation at some point, gix-testtools (source) already uses .gitignore to indicate that archives shouldn't be created (or used, for that matter), and I could totally imagine that additional differences and tuning could be performed by means of .gitattributes.
Please let me know if there are any other questions about this - here I can answer them all in detail :).
I had thought I'd tried this before, but actually I had not. 16 tests fail when this is done. I can try to open an issue about this sometime soon. I don't know how much detail would be desired. This should not be confused with failures that occur when the tests are run outside a Git Bash environment, which happens even without setting that environment variable and is due to having paths as the current working directory, and possibly in some other environment variables, with Windows-style |
Thanks for trying it out! 16 tests is only a small fraction, fortunately, even though trying to address these can be a time-sink. One test failure that stuck out to me might be a curious case where Please also note that one day, once tests work on Windows without archives, one could consider running the actual tests (without archives) on Windows as well. It's probably going to take twice as long as on Linux which means that it would probably have to run so that failures are just warnings so one doesn't necessarily have to wait for it to finish.
I agree, this sounds like a separate issue which requires a separate kind of fix. Maybe it happens in 2 stages as well. Once with archives, and once without once the 16 test failures are resolved. Please do, however, proceed in any direction you want, with the issues nothing will be forgotten. |
|
For that one, is the idea that the pattern If so, then I wonder if this is a difference between upstream Git, and Git for Windows. On Windows, Should I can do some experiments later if the answer is not apparent. |
I am not sure what it is supposed to do or why there is a difference on Windows, but it seems that
I also couldn't find anything that would indicate However, it does look like maybe that's the reason they differ, and
Yes, it would be nice to learn what is what here. In theory, if just |
I've returned to this (with the intention of opening an issue for the 16 failures on Windows with It looks to me like the assertion that is failing is not directly testing gitoxide, but is instead checking that Git itself behaves in the expected way, so subsequent checks can reasonably compare against it. On the assertion that is failing, the expected results for a case seems to expressed as a combination of which file In a loop though the entries, we have the assertion that is failing: This keeps it from getting to the " So to me it looks like the "expected" value in the failing assertion is the value I think there is a substantial amount of relevant code that I have not examined here. So I would not be surprised to learn that my impression so far is mistaken. However, I figured I'd post my thinking on this as it stands, in case there's a easy explanation or correction related to my thinking. (My short-term goal in opening this issue is not to work on the bug immediately, but instead to open issues for test failures and other bugs that I am aware of but not working on at the moment. As you have said, with the issues, nothing will be forgotten. However, there is no rush here, since GitHub will notify me whenever you reply. You can also of course ask me to open a brief issue before any of this is clarified, in which case I will do so.) |
|
Thanks for sharing all these details which help me to get back into it. The baseline-test was definitely written with unix-only assumptions, and it never inconvenienced itself with dealing with Windows-specific differences that might arise without an archive. Git for Windows not being a match here might mean there is some special handling of backslashes. Ultimately, what would have to be done is to examine what Git for Windows does, and then see if the test can be made to match its outcome, if especially if it seems more sensible. Then again, I was never looking into Git for Windows and hence didn't see anything specific it might have added to the wildmatch.c code, or maybe in other places. Ultimately, once the test passes, one would have to |
This is my main area of confusion: it seems to me that Git for Windows is a match here, and that Git on other systems is not, and that the purpose of the failing assertion is to verify that it is not a match. But I've interpreted some of the things you've said, including that quoted text above, to mean that this understanding of mine is not correct. When I create a repository with the */\'and run the command git check-ignore -vn "XXX/\'"I get this output on Ubuntu: And when I run it on Windows (the quoting rules for To double-check this, and also to make it more demonstrable, I performed a variation on this experiment on CI, where Ubuntu, macOS, and Windows were all tested, and where That seems to accord with my understanding of the test failure as indicating that this is a match only on Windows, that the assertion is asserting that it is never a match, and that the assertion is testing only expected behavior of Git as opposed to the subsequent test logic that also involves behavior of gitoxide. Yet, I worry that I may be mistaken, since it seems like we are each expressing opposite interpretations of what is being asserted--though maybe we are saying the same thing, somehow, yet talking past one another. (But I reiterate that there is no hurry here. It looks like I may end up opening an issue for this even before I am totally clear on what you are saying about what this shows, since it looks like at least one other issue that I wish to open would benefit from being able to refer to that issue for clarification. If I open an issue before I full understand this, then I can and will expand or otherwise revise it when more information is available.) |
|
As suggested above, I've gone ahead and opened issue #1358 for this even though I'm not confident that I adequately understand the specifics of the particular failure we were discussing here, so that I can reference the issue in other issues. Further discussion of |
|
Thanks for elaborating - I am pretty sure that I flipped the meaning of match and not-match as you are referring to the |
This adds executable permissions to shell scripts that begin with a
#!and are meant to be run rather than sourced into another script. Most of these already had those set, so the inconsistency of having a handful of them unset does not appear intended.The purpose of this change is to make clearer which .sh files are meant to be run directly in a new shell process (which is most, but not all, of them), and which are meant to be sourced into another script rather than run (i.e., those that are script "libraries"). Executable permissions are intentionally not added to .sh files without shebangs that are meant for sourcing rather than running.
This was not causing test failures because the way the scripts were invoked did not rely on being able to directly execute them. Nonetheless, it is clearest for these metadata to match the scripts' shebangs. (Scripts that shouldn't have the executable bit set should likewise not have shebangs, but there didn't appear to be cases where scripts wrongly had shebangs or wrongly had +x set.)
A further benefit is that executable scripts are traditionally named with no special suffix while files meant for sourcing into a shell are traditionally named with a .sh suffix. Since that is not the convention followed here, setting the executable bits may be especially useful to clarify intent.
This pull request also removes doubled end-of-file newlines (where a file ended in two newline characters instead of just one) where present in a few files whose permissions were being changed.
The rationale for including that here, even though no other script cleanup is done here, is that sometimes this happens as an overcorrection when editing scripts on Windows, where many editors don't automatically add even a single end-of-file newline. Windows is also where intended executable bits are sometimes omitted. That, in turn, is due to the absence of a Unix-style chmod, and how Cygwin-like environments infer likely executable permissions from shebangs rather than file metadata. (Git still tracks them, but Windows's own permissions system differs).
Files whose metadata were not being changed were not checked for extra newlines, nor otherwise edited, here.
I had originally intended also to include a change to one or more
.gitattributesfiles here, to make it so that files meant to be run or sourced as shell scripts (i.e., in this project, all.shfiles) are checked out with their actual LF line endings on all platforms, including Windows where Git is usually configured to replace LF line endings with CR LF line endings.The reason is that, unlike code in most languages, shell scripts technically require LF line endings. I say "technically" because Bash in Cygwin-like environments--but not WSL--will automatically pretend CR LF endings are LF in order to work around these kinds of issues. One disadvantage of such line endings this is that it is hard to run
shellcheckon them on Windows (without runningdos2unixfirst) because of all the warnings it gives about the line endings being wrong.The reason I have not included that change is that there is already specific and intentional handling of line endings for shell scripts in
.gitattributesfiles in this project, which I think I may not fully understand. I think the simpler rule*.sh text=auto eol=lfmay be sufficient to achieve those goals, but I am not sure.If such a change is wanted then I could add another commit here for it, or do it in a separate PR.