search

gitpython-developers / GitPython

GitPython is a python library used to interact with Git repositories.
http://gitpython.readthedocs.org
BSD 3-Clause "New" or "Revised" License
4.65k stars 906 forks source link

Fix Fetch progress bar #1971

Closed fvalette-ledger closed 1 month ago

fvalette-ledger commented 1 month ago

See https://github.com/gitpython-developers/GitPython/issues/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.

Byron commented 1 month ago

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.

fvalette-ledger commented 1 month ago

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, Indeed, it seems that a security fix for windows broke the progress bar. At first sight, it seems that universal_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).

Byron commented 1 month ago

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 commented 1 month ago

I didn't bisect yet, but the regression is introduced between 3.1.40 and 3.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/ GitPython 3.1.40

Concerning git/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 in handle_process_output. But there is major security fix for windows that, somehow, changes the behavior in handle_process_output.pump_stream where https://github.com/gitpython-developers/GitPython/blob/49ca9099dc75d0d686ec6737da36637cbee1c000/git/cmd.py#L152 yields only once on <LF> with all progress lines (sep w/ <CR>) concatenated and thus broke progress bar.

fvalette-ledger commented 1 month ago

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, see https://github.com/actions/runner-images?tab=readme-ov-file#available-images).

Byron commented 1 month ago

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.

fvalette-ledger commented 1 month ago

It appears that CI is legitimately failing though.

It was an encoding issue, while using universal_newlines, one may specify the right charset. This is fixed by https://github.com/gitpython-developers/GitPython/pull/1971/commits/52cceaf2663422a79a0f1d21f905eb132e46b556

fvalette-ledger commented 1 month ago

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 https://github.com/gitpython-developers/GitPython/pull/1971#issuecomment-2413723727, 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.