Skip to content
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

Start blame on the commit passed to a "tig show" commandline #1135

Merged
merged 1 commit into from
Oct 4, 2021

Conversation

krobelus
Copy link
Contributor

Fixes the first issue described in #1127 (comment)

@koutcher
Copy link
Collaborator

koutcher commented Sep 9, 2021

I found a better approach: get the commid id from the commit line in the diff. It also works when there are several commits in the diff view (e.g. tig show --all). See koutcher@6c77c61.

@krobelus
Copy link
Contributor Author

Yeah that seems better. I used %(commit) which works the same way except it also works for stash objects.
Unfortunately there's another regression with blame on stashes, see the stash test case.
Maybe we should just include the object-ID of a stashes in their diffs?

@koutcher
Copy link
Collaborator

koutcher commented Sep 15, 2021

I don't think there is an option in git stash show to do that. We could use git show but I'm not sure we want to do that. Based on my initial proposal, we could fix the problem by using view->vid when there is no commit line (see koutcher@blame-initial-diff-view). I liked the way you used %(commit) but I don't see any easy way to fix the stash issue with it.

I thought I did but actually it is not so good and neither is the use of view->env->commit because it doesn't work if you go backward in the diff view.

@krobelus
Copy link
Contributor Author

Ok falling back to view-vid if there is no commit line seems to work.
I think this is a workaround because view->env->commit should also be set if we go backwards.

@koutcher
Copy link
Collaborator

Yes and no. When we fix view->env->commit (let's put it on the todo list, we can reuse bits from the log view), we will still need to look for a commit line to know when we have to use view->vid as view->env->commit can be overwritten by other views.

I noticed test/blame/initial-diff-test fails on my Mac because of sed -i. sed -i.old -n /asInstanceOf/p *.screen fixes it.

When running "tig show 529182c -- README.adoc", blames were started
on HEAD instead of 529182c.  Fix this by reading the commit line of
the displayed diff.  This also works when multiple commits are shown
("tig show --all").

Using %(commit) instead of parsing the commit line did unfortunately
not work because, because %(commit) can contain the wrong SHA for when
returning from a blame view to a stash view, see test/blame/stash-test.

Sadly, stashes don't contain commit lines, so we need to find out
their SHAs in another way. Once we fix %(commit), we can use it,
but for now this commit adds a workaround to use the old
"view->vid", which seems to be set in the cases where display
the output from "git stash show".

Also, when blame is requested on a context line, use the current
commit rather than its parent.  This does not change behavior since
a context line can never be blamed on the current commit, but this
is consistent with the stage view, which only uses HEAD for deletions
and the index for others.

[ja: add test and commit message]
@krobelus
Copy link
Contributor Author

as view->env->commit can be overwritten by other views.

What is an example of that overwriting? Shouldn't view->env->commit (aka %(commit)) always be the commit of the thing at the cursor?
Since :view-blame blames at the cursor, view->env->commit should then be the correct one.
Otherwise my commit message is slightly off.

@koutcher
Copy link
Collaborator

This was typically what happened in your scenario with a stash. When the diff view is opened on the stash, view->env->commit is initialized from the stash view, but it is later overwritten in the blame view and when coming back to the diff view, there is nothing to set view->env->commit back as there is no commit line.

@krobelus
Copy link
Contributor Author

Ah right, so we do just need to fix view->env->commit but that probably requires a commit line.

@koutcher koutcher merged commit 874ee11 into jonas:master Oct 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants