tattersoftware / codeigniter4-patches

Automated project updates for CodeIgniter 4
MIT License
30 stars 8 forks source link

Required git version #28

Closed whoisninjaturtle closed 1 year ago

whoisninjaturtle commented 2 years ago

It might be helpful to others to mention that this requires git <= 2.23 to satisfy the git-switch command. This leaves some downstream users in the RedHat river either out of luck with an "older" version of git or installing a newer version via side channels.

At a cursory glance I did not see why those two git-switch commands couldn't be git-checkout instead, so I will be trying that as a potential workaround but I welcome any input on the matter.

MGatner commented 2 years ago

Good point! If your testing is successful please send a PR to change to checkout. I'm a fan of switch and have gone so far as to compile my own updated version of Git to have access to it 😅

whoisninjaturtle commented 1 year ago

I think it works correctly with the appropriate substitutions; I'm testing my way through a fairly large merge conflict right now so I'll update on that when I'm done.

One other silent requirement I've found is for Composer v2, which CI4 itself does not require at this time. This is due to the use of the --with-all-dependencies options passed to composer require and composer update. This option, added in version 2, is simply an alias for the --update-with-all-dependencies option, which exists in both versions 1 and 2. When using Composer version 1, it seems that the script fails for only that reason.

MGatner commented 1 year ago

Thanks for the update! I don't feel the need to support Composer v1, but if that is really an alias (and the original is not deprecated) then I would be glad for that change so both can work.

whoisninjaturtle commented 1 year ago

I've created a PR to the develop branch with the git-related changes discussed.

if that is really an alias (and the original is not deprecated)

The issue is Composer's 'inconsistency' in flag naming between the options. In both versions 1 and 2, the update command uses the --with-all-dependencies flag while the require command uses the --update-with-all-dependencies flag. At this point my understanding is those are the 'expected' flags for their respective commands and are not deprecated.

In version 2, an alias flag named --with-all-dependencies was added to the require command, which aliases --update-with-all-dependencies. So in order to support both Composer 1 and 2, it would be necessary to change --with-all-dependencies to --update-with-all-dependencies in the two instances where composer require is executed; in other words, use the primary flag and not its alias.

MGatner commented 1 year ago

Yes, I'm fine with that. Please make it a separate PR from the Git one but it all sounds good to me. I had to do a deep dive on these commands recently and I have my head around them.

whoisninjaturtle commented 1 year ago

Sorry, I shouldn't have cluttered this one issue with multiple topics. I'm focused on the git changes right now and will revisit Composer in a separate issue/PR, if and when I do. Regarding those git changes, I think I've found an issue with older versions of git caused by the behavior of git-clean (per our conversation in the PR, the versions I'm using are 1.8.3.1 and 2.31.1).

I didn't notice this before (sorry again), but my writable and tests directories were getting wiped out completely by git-clean -fd, despite having all their contents properly listed within .gitignore: it seems that unless those upper-most directories are listed within .gitignore as well, older versions of git-clean treat them as untracked, despite the ignored contents, and delete the entire directory. The newer version of git-clean does not appear to have this behavior, so at some point between 1.8.3.1 and 2.31.1 I'm guessing the behavior changed; I'm unclear at this point exactly when.

Since there's nothing critical to it in those directories, the patch process never failed, but for obvious reasons this causes a lot of issues for the application itself, after the fact. At this time it appears the 'solution' would be a new requirement: if you're using an older version of git, to make sure that all top-level directories within the project root are either a) fully tracked or b) explicitly listed in .gitignore, even if all sub-directories/files within are already listed.

I leave this up to you; expanding the backwards compatibility is good but there can and will be knock-on side effects like this, so if you want to back off this merge I totally understand. I'm continuing to test this out so I will update with any more information.

MGatner commented 1 year ago

That sounds like a fine "warning note" for the README. It hopefully won't be relevant to most users, but good to mention for troubleshooting and mistake prevention.

At the end of the day this is a tool I use heavily myself, which I will always be on a recent version of Git. Anything else is work put in by the community (like you); I am happy to discuss and merge but I will trust you to figure it out.

whoisninjaturtle commented 1 year ago

Sorry, it took me forever to get a successful merge using this tool but the issues were all mine in the end; that terribly stupid git-clean behavior, then it took me a while to figure out that my merge strategy was flawed for the CI4 upgrade I'm testing against (4.1.3 -> 4.1.4) and was very much breaking autoloading. But in the end I managed to make it work the way that I'd expect it should be. I'm going to close this issue unless you have anything else. Thanks again for this tool, I look forward to contributing further.

MGatner commented 1 year ago

Thank you! I appreciate the thorough testing and feedback. Did you decide against the Composer changes? If so I will go ahead and release what you've submitted for Git compatibility.

whoisninjaturtle commented 1 year ago

Sorry, I've been somewhat distracted trying to make this work for me personally. In my development environments I typically have files within app/ that are .gitignored because they're necessary in development but I don't want them moving downstream; database seeds for unit tests, unfinished Library classes, the like. So I'm trying to work out a way to keep those around.

At the moment my workaround is to un-ignore, track and commit all the aforementioned files. Then go through the patch process; after merging the 'tatter/patches' branch, 'untrack' them (git -rm --cached <file>) and re-ignore them. Ugly and not really extensible, but it works in the meantime.

The Composer changes are probably not worth it; I myself won't even need that backwards compatibility for much longer. If for some reason you do want them, they still stand as described above. As for the git changes, I guess I'm indifferent at this point. No one else has brought it up before, it won't have any direct benefit to you (the primary user in truth) and I'm more than capable of maintaining my own fork for my own compatibility needs. So yeah your call.

MGatner commented 1 year ago

No problem! I will get an update released this weekend. I'll also put a little thought into your untracked files issue, but it sounds like you've done plenty of that so far 😳 Maybe removing .gitignore on the intermediate branch and stash anything affected? I'll have to tinker with it.