Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork938
Fix Fetch progress bar#1971
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Fix Fetch progress bar#1971
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Seegitpython-developers#1969stderr parser call RemoteProgress update on each line received.With universal_newlines set to False, there is a mixup betweenline feed and carriage return.In the `handle_process_output` thread, this is thus seen as a singleline for the whole output on each steps.
Thanks a lot for contributing this fix! It appears thatCI is legitimately failing though. Could you investigate and see what changed between the last known working version, and the first broken one? Was it really this that changed, or maybe something else? I am wondering if there could be a different fix for this, that would then also pass CI naturally. |
Hi, By the way, python 3.7 is supported by EOL and not package on ubuntu latest (i.e. ubuntu 24.04 ==>https://github.com/gitpython-developers/GitPython/actions/runs/11334079345/job/31534626048?pr=1971). |
That's interesting, as it sounds like a python security update on Windows broke GitPython? Or only universal newlines?
That's true - thanks for pointing it out. Maybe you can remove testing it on CI while at it. |
fvalette-ledger commentedOct 15, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I didn't bisect yet, but the regression is introduced between Concerning Line 152 in49ca909
yields only once on <LF> with all progress lines (sep w/<CR> ) concatenated and thus broke progress bar. |
As python 3.7 is officially supported by the package, IMHO, it must be covered by unit testing. |
I am definitely curious what it turns out to be in the end. As it seems, the issue is definitely within GitPython, and I'd hope the security-related change can also be made so that it doesn't break anything.
That's a great point! I also think that using a specific version label should be the way to go, if it's still available. |
It was an encoding issue, while using universal_newlines, one may specify the right charset. |
FYI,encoding argument is missing for Popen. It should be defined if text outputis required (i.e. w/ universal_newlines or text set to True).The last commit on the PR fixes Windows unit test.As windows does not use utf-8 as encoding (unless linux/macos), it'srequired.Security fix, using safe_popen, is not impacted by the fix, maybe the author of this fix could review the pull request to be sure thatnothing else is broken under the hood.Regards, …On Tue, Oct 15, 2024 at 2:02 PM Sebastian Thiel ***@***.***> wrote: I didn't bisect yet, but the regression is introduced between 3.1.40 and 3.1.41. I am definitely curious what it turns out to be in the end. As it seems, the issue is definitely within GitPython, and I'd hope the security-related change can also be made so that it doesn't break anything. As python 3.7 is officially supported by the package, IMHO, it must be covered by unit testing. Maybe use matrix configuration to select suitable ubuntu version ( ubuntu-22.04 is a valid label, seehttps://github.com/actions/runner-images?tab=readme-ov-file#available-images ). That's a great point! I also think that using a specific version label should be the way to go, if it's still available. — Reply to this email directly, view it on GitHub <#1971 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/A2KVVAXMJTLB2WKBNEGMFN3Z3T77DAVCNFSM6AAAAABP5U6ITOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMJTG4ZDGNZSG4> . You are receiving this because you authored the thread.Message ID: ***@***.***> -- [image: Ledger Logo]www.ledger.com1 rue du Mail75002 ParisFranceFlorent VALETTESenior Firmware Engineer***@***.***[image: Tw] <https://twitter.com/Ledger/>[image: Ln]<https://www.linkedin.com/company/ledgerhq/>[image: Fb]<https://github.com/LedgerHQ/>[image: Fb]<https://www.facebook.com/Ledger/>[image: Ig]<https://www.instagram.com/ledger/> -- Les informations contenues dans ce message électronique ainsi que cellescontenues dans les documents attachés sont strictement confidentielles etsont destinées à l'usage exclusif du (des) destinataire(s) nommé(s).Toutedivulgation, distribution ou reproduction, même partielle, en eststrictement interdite sauf autorisation écrite et expresse de l’émetteur.Si vous recevez ce message par erreur, veuillez le notifier immédiatement àson émetteur par retour, et le détruire ainsi que tous les documents qui ysont attachés.The information contained in this email and in anydocument enclosed is strictly confidential and is intended solely for theuse of the individual or entity to which it is addressed.Partial or totaldisclosure, distribution or reproduction of its contents is strictlyprohibited unless expressly approved in writing by the sender.If you havereceived this communication in error, please notify us immediately byresponding to this email, and then delete the message and its attachedfiles from your system. |
This should be undone once python 3.7 is EOL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks a lot for helping with this!
I think this will always be prone to breaking as the interactivity of remote progress isn't tested, but let's hope it will work for a while.
a527224
intogitpython-developers:mainUh oh!
There was an error while loading.Please reload this page.
> Fix Fetch progress bar by@fvalette-ledger ingitpython-developers/GitPython#1971GitPython 3.1.44 got my fix merged, and thus, we can reintroduceprogress bar on git pull (barbican update)
See#1969
stderr parser call RemoteProgress update on each line received. With universal_newlines set to False, there is a mixup between line feed and carriage return.
In the
handle_process_output
thread, this is thus seen as a single line for the whole output on each steps.