search

pharo-vcs / iceberg

Iceberg is the main toolset for handling VCS in Pharo.
MIT License
134 stars 85 forks source link

LGitReadStream does not understand #position #1376

Closed jvalteren closed 3 years ago

jvalteren commented 4 years ago

Applicable to Iceberg in both Pharo 7.0 and 8.0. Don't know about 9.0.

Scenario I fetch a repository and it shows incoming changes to be pulled (e.g. the Pull toolbar icon showing 2 changes in a red badge). I click on the Pull toolbar icon, which opens the 'pull from' dialog showing details. I want to see changes made in one specific commit, so I select it and click on the second tab, for example with the title 'a1fdbd3 to 81732a9'. This opens a debugger with the message 'Instance of LGitReadStream did not understand #position'.

Description The LGitReadStream class has a position instance variable, but doesn't have an accessor get method for it, which is expected by the ZnBufferedReadStream object it is wrapped in.

Solution Simply adding the accessor get method #position in the LGitReadStream class solves the issue, after which the commit diff shows nicely.

jvalteren commented 4 years ago

I'd love to create a PR for this supersimple change, but loading the latest dev branch seems to screw up my Pharo image 😕 I'll try to uninstall and reload, see if that works.

tinchodias commented 3 years ago

Thanks for the detailed report.

This should be fixed in Pharo 9, but not in 8.0. The missing method was added by https://github.com/pharo-vcs/libgit2-pharo-bindings/pull/34 in the libgit bindings project, and released under https://github.com/pharo-vcs/libgit2-pharo-bindings/tree/v2.1.0 .

Do we want to fix this in Pharo 8 as a hotfix? opinions?

For contributions, it's more complex than for other projects due to Iceberg updating Iceberg. But there was this wiki entry (that I updated 2 weeks ago and put a more clear link from README, I think): https://github.com/pharo-vcs/iceberg/wiki/Contributing-to-Iceberg

tinchodias commented 3 years ago

This is fixed in dev-2.0 branch and then in current Pharo 9. So, I'll close the issue, but please re-open it if you consider important to hotfix it in Pharo 8.

@tesonep ?

StephanEggermont commented 3 years ago

Yes, this needs a hot fix in Pharo 8

macta commented 3 years ago

I've had this several times, but needed to share something with someone quickly and this was a real bummer... fortunately the workaround was searchable on Discord - but its not a great experience. If Pharo 9 is 6-12 months away - would be useful to get this in point release.

Ducasse commented 3 years ago

We should check because there are more implication than just a patch. And P9 should be released in april/may. S

timm-mwp commented 3 years ago

If P9 is just months away - and its fixed in P9, then I can understand holding out...

StephanEggermont commented 3 years ago

The release is not relevant for this. It is needed to allow porting forwards.