- Sponsor
-
Notifications
You must be signed in to change notification settings - Fork 908
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 Fetch progress bar #1971
Fix Fetch progress bar #1971
Conversation
See gitpython-developers#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.
Thanks a lot for contributing this fix! It appears that CI 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. |
I didn't bisect yet, but the regression is introduced between Concerning Line 152 in 49ca909
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 output
is 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's
required.
Security fix, using safe_popen, is not impacted by the fix, may
be the author of this fix could review the pull request to be sure that
nothing 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, see
https://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.com
1 rue du Mail
75002 Paris
France
Florent VALETTE
Senior 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 celles
contenues dans les documents attachés sont strictement confidentielles et
sont destinées à l'usage exclusif du (des) destinataire(s) nommé(s).
Toute
divulgation, distribution ou reproduction, même partielle, en est
strictement 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 y
sont attachés.
The information contained in this email and in any
document enclosed is strictly confidential and is intended solely for the
use of the individual or entity to which it is addressed.
Partial or total
disclosure, distribution or reproduction of its contents is strictly
prohibited unless expressly approved in writing by the sender.
If you have
received this communication in error, please notify us immediately by
responding to this email, and then delete the message and its attached
files 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.
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.