10000 Draft: add blame progress callback by Murmele · Pull Request #6136 · libgit2/libgit2 · GitHub
[go: up one dir, main page]
Skip to content
8000

Draft: add blame progress callback #6136

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

Draft: add blame progress callback #6136

wants to merge 2 commits into from

Conversation

Murmele
Copy link
@Murmele Murmele commented Dec 8, 2021 •
edited
Loading

Add blame progress callback.

Originally created by the GitAhead creator. I would like to get it mainlined so upgrading to newer versions get easier

@Murmele Murmele changed the title add blame progress callback Draft: add blame progress callback Dec 8, 2021
@boretrk
Copy link
Contributor
boretrk commented Jan 6, 2022

Duplicate of #5728

@extrawurst
Copy link
extrawurst commented Jan 6, 2022 •
edited
Loading

it is sad to see these contributions go unnoticed for so long that duplicates of PRs even pop up :(
is there any way to support maintainers in getting these in?

@boretrk
Copy link
Contributor
boretrk commented Jan 6, 2022

In #5728 the author claims that the PRs that have been duplicated here aren't ready to be upstreamed.
GPL allows for it to be merged upstream against the authors wishes but wouldn't it be a good idea to figured out why they weren't ready back then and if anything has changed in that regard first?

@extrawurst
Copy link

Let’s ping @hackhaslam then :)

@hackhaslam
Copy link
Contributor

My comment about not being ready to be upstreamed was a general statement about all of the remaining patches in the GitAhead fork of libgit2. I felt like they needed more work and I wasn't ready to defend them.

This specific change is probably fine as is for what it does. It could be fleshed out a bit to provide actual progress. The use case is to abort long running blame operations. For example, if you start a blame on a file, then navigate away to some other file, it's useful to abort the current blame operation first.

@boretrk
Copy link
Contributor
boretrk commented Jan 7, 2022

Well, that resolves that part at least.

As for why it hasn't been merged I'm not a maintainer so I can only speculate.
Looking at the number of issues and pull requests I suspect most of the problem is that not enough people help with reviewing others pull requests.

Another could be that this PR and the previous duplicates were part of a bunch of requests that were originally added without description, but that is fixed now.

What haven't been addressed is the concern about the naming being misleading in previous PR.
Can the callback reasonably be used for progress report and if not, can the callback be extended to make that possible? Otherwise a more accurate name might be better.

It could also be worth checking out https://github.com/libgit2/libgit2/blob/main/docs/contributing.md#pull-requests
Tests and API documentation seems to be missing.

Again, I'm not a maintainer so there might be other reasons for not merging this PR.

@Murmele Murmele closed this Apr 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Murmele @boretrk @extrawurst @hackhaslam
0