Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Conversation

fvalette-ledger
Copy link
Contributor

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 thehandle_process_output thread, this is thus seen as a single line for the whole output on each steps.

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.
@Byron
Copy link
Member

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.

@fvalette-ledger
Copy link
ContributorAuthor

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,
Indeed, it seems that a security fix for windows broke the progress bar.
At first sight, it seems thatuniversal_newlines arguments does more than it said, clearly the fix will be more complex than this first try.
i'll have a look, until CI is green, i convert this PR to draft.

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).

@fvalette-ledgerfvalette-ledger marked this pull request as draftOctober 15, 2024 06:01
@Byron
Copy link
Member

That's interesting, as it sounds like a python security update on Windows broke GitPython? Or only universal newlines?
In any case, I am looking forward to you tackling it, thanks again for your help.

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 true - thanks for pointing it out. Maybe you can remove testing it on CI while at it.

@fvalette-ledger
Copy link
ContributorAuthor

fvalette-ledger commentedOct 15, 2024
edited
Loading

I didn't bisect yet, but the regression is introduced between3.1.40 and3.1.41.
As said in the related issue, i'm using python 3.10 and ubuntu 22.04.1 and i tried different git version with the same result (from 2.30, 2.34.1 to last release with the result). Whatever the git version, the last working progress bar for fetch (and likely pull) is w/ GitPython3.1.40

Concerninggit/remote.py file, the diff only concern documentation fix (rewording, style, typo fixes). So, in the last working release use stderr as bytes and not text, convert inhandle_process_output.
But there is major security fix for windows that, somehow, changes the behavior inhandle_process_output.pump_stream where

forlineinstream:

yields only once on<LF> with all progress lines (sep w/<CR>) concatenated and thus broke progress bar.

Byron reacted with thumbs up emoji

@fvalette-ledger
Copy link
ContributorAuthor

That's true - thanks for pointing it out. Maybe you can remove testing it on CI while at it.

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).

@Byron
Copy link
Member

I didn't bisect yet, but the regression is introduced between3.1.40 and3.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.

@fvalette-ledgerfvalette-ledger marked this pull request as ready for reviewOctober 15, 2024 16:12
@fvalette-ledger
Copy link
ContributorAuthor

It appears thatCI is legitimately failing though.

It was an encoding issue, while using universal_newlines, one may specify the right charset.
This is fixed by52cceaf

@fvalette-ledger
Copy link
ContributorAuthor

fvalette-ledger commentedOct 15, 2024 via email

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.
Copy link
Member

@ByronByron left a 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.

@ByronByron merged commita527224 intogitpython-developers:mainOct 15, 2024
22 checks passed
fvalette-ledger added a commit to outpost-os/barbican that referenced this pull requestJan 14, 2025
> 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)
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@ByronByronByron approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

2 participants
@fvalette-ledger@Byron

[8]ページ先頭

©2009-2025 Movatter.jp