-
Notifications
You must be signed in to change notification settings - Fork 68
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: fix ENAMETOOLONG for too big commit messages #263
base: master
Are you sure you want to change the base?
fix: fix ENAMETOOLONG for too big commit messages #263
Conversation
@pvdlg Ping, is there anything not clear? This is breaking our build currently and while using the fix directly is really easy (because all plugins are old-school w/o any bundling yay!), our team lead is somewhat hesitant using private fixes from GitHub. :) |
@travi BUMP. This is still a problem... Luckily the one year old fix, still happily merges. :) |
i'm sorry that i've been silent on this thread for so long. it can be tough to keep up with all of our priorities and this one has, unfortunately, fallen through the cracks so far. i have been aware of this request and have had it on my radar for quite a while, but have not been able to give it priority. i recently had a conversation about this situation, so i want to capture and share some details that i think are important to consider for this case:
hopefully some of that context is helpful for some folks that land here to understand some of the contributors to the situation and potentially find some adjustments that could be made to avoid this situation. all of that said, part of what has delayed my acceptance of this proposed solution is that it feels like an overly complex solution to a problem that should be very rare under normal circumstances. the "very rare under normal circumstances" makes it hard to prioritize in the first place, but to add the need to write a temp file, which potentially needs additional system access, only to read it back in for this rare case makes this hard to accept and maintain over the long term. i'm also very hesitant to do anything along the lines of truncating the content because i think that presents the worst of both worlds. we would only partially be enabling the behavior of including the release notes in the commit. i admit, i'm unsure about why we include the release notes in the commit body in the first place. i can understand some folks might value that information being directly in the commit history, but i wonder if that is something many folks give much attention to. that behavior existed before i became a maintainer. i honestly wonder if the simpler solution would be to simply not include that information in the commit. we already support various ways to capture release notes in more visible locations, so maybe it doesnt need to be in the commit. that would be a breaking change, but maybe a justified one. as highlighted in #91 (comment), it is already possible to opt-out of this default behavior. maybe the best course of action would be to flip this to an opt-in instead? i'm curious if folks have thoughts about this option |
This PR allows running
semantic-release
to reliably commit arbitrary big commit messages. The old way of adding a commit message was by passing it simply along as a command line argument togit commit -m ${message}
this will easily break for huge commit messages, depending on the platform:execa()
call to roughly 8 KBIn practice one can easily hit those limits by having a moderate number of changes during releases, because upstream in
semantic-release
for a default setup ofsemantic-release/changelog
,semantic-release/git
the generated changelog is added as a commit message.The fix is, that the message is not passed as an argument, but a temporary file containing the message will be created and the file will be passed instead.
There are now some additional implications; whenever something goes wrong, there is the possibility that temporary files are not deleted, however as
os.tmpdir()
is used, this is something that the OS should deal with. Furthermore this now will break, ifos.tmpdir()
is not writeable bysemantic-release
. However I think it is a fair assumption, thatos.tmpdir()
is writeable - although an additional check could be added.Whether this is a breaking change, this is left as
an exercise for the astute readersomething to be answerer by the maintainers. I'd say it is not, because it is behaving as intended. However this might break some pipelines, that have notos.tmpdir()
writeable. I tested it on an Azure DevOps Windows hosted agent and I am pretty sure™ that in a sane default™ setup of build agents this should not pose any problems.This is somehow related to: #91 (although I think the actual course for the problem there was the amout of changed files); at least it is giving the same error message.
There are further places in the code, where seemingly unrestricted data is passed via
execa()
that is suffering from the same problems (adding a lot of files via add), however fixing this is not so easy, becausegit add
does not support adding files from a "list file". Possible solutions would be a chunked adding (splitting the files by "length" and thus multiplegit add
calls) - however this has further implications. I'd rather fix those problems in another PR if the maintainers are fine with this approach (or suggesting sth. better).