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

Git Notes don't show in beta 3.00.00.01 #5653

Closed
jdunne525 opened this issue Oct 30, 2018 · 10 comments
Closed

Git Notes don't show in beta 3.00.00.01 #5653

jdunne525 opened this issue Oct 30, 2018 · 10 comments
Assignees
Labels
type: bug 🐛 type: regression regression, normally to latest official release
Milestone

Comments

@jdunne525
Copy link

Do you want to request a feature or report a bug?
Bug

What is the current behavior?
Git Notes show just fine in v2.51.05, but don't show at all in the latest beta.

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem.
Create a git note.

What is the expected behavior?
The note should show as it does in the previous stable version.

Environment you encounter the issue:
Windows

The "Help-About Copy" or "Report an Issue" button doesn't seem to put anything into my clipboard either.

  • GitExtensions version:
  • GIT version: git version 2.18.0.windows.1
  • OS version: Windows 10
  • .NET version:

Did this work in previous version of GitExtensions (which)?

@vbjay
Copy link
Contributor

vbjay commented Oct 30, 2018

The "Help-About Copy" or "Report an Issue" button doesn't seem to put anything into my clipboard either.

That's because it was JUST added to master
image

master is at tag: v3.00.00-beta1 + 107 commits this moment. We are preparing for a RC soon.

@RussKie RussKie added type: bug 🐛 type: regression regression, normally to latest official release labels Oct 31, 2018
@RussKie RussKie added this to the 3.00-beta2 milestone Oct 31, 2018
@RussKie
Copy link
Member

RussKie commented Oct 31, 2018

From what I could infer, the problem is that we initialise the body for each revision (L419):

revision = new GitRevision(objectId)
{
ParentIds = parentIds,
TreeGuid = treeId,
Author = author,
AuthorEmail = authorEmail,
AuthorDate = authorDate,
Committer = committer,
CommitterEmail = committerEmail,
CommitDate = commitDate,
MessageEncoding = encodingName,
Subject = subject,
Body = body,
HasMultiLineMessage = !ReferenceEquals(subject, body)
};

And when we display the revision we no longer attempt to load auxiliary information, such as notes:

var data = _commitDataManager.CreateFromRevision(_revision, _children);
if (_revision != null && _revision.Body == null)
{
_commitDataManager.UpdateBody(data, out _);
_revision.Body = data.Body;
}

One way of fixing the issue is not to set the Body in the RevisionReader.
I've run it locally and haven't observed any issues. However it kind of feels dirty to do this....

I've skimped through 2.5x codebase, and it looks like we didn't always read the body in git log. Hence we needed to hydrate this property on demand.

@spdr870 @drewnoakes @gerhardol: What's the best course of action?
@mstv: you rely on revision.Body property for #5643 to work, don't you?

@vbjay
Copy link
Contributor

vbjay commented Oct 31, 2018

Long Term:
With the pub sub work on the submodule in left panel feature and a two part load process(load grid with a log call that fills in basic info, then an async load step that loads whatever else we desire we could fill in the details.)

Notes do show up when you view it by double clicking on commit.

@mstv
Copy link
Member

mstv commented Oct 31, 2018

You rely on revision.Body property for #5643 to work?

No, I don't. revision.Subject is sufficient.
The revision.Body is not evaluated. It is just displayed in the tooltip, of the columns Graph and Message, if the Body is available. Else a -- meanwhile -- factored out, translatable hint is given.

@gerhardol
Copy link
Member

Body is just read (or saved?) for commits not older than six months.
Drew made some investigations about it.
In GitModule.cs GetRevision() it seems like the intention is to fetch git notes if requested (and it should work in CommitInfo).
So it may be a matter of requesting/keeping notes.

I suggest this is listed as a known error in beta2/rc1.

@RussKie
Copy link
Member

RussKie commented Nov 1, 2018

Thanks, I'll downgrade it. Though we need to get it fixed for the official release.

@RussKie RussKie modified the milestones: 3.00-RC1, 3.00 Nov 4, 2018
@RussKie
Copy link
Member

RussKie commented Nov 7, 2018

In GitModule.cs GetRevision() it seems like the intention is to fetch git notes if requested (and it should work in CommitInfo).

I don't think this approach will work in the current use case.

  • we bulk load revisions into the revision grid, this is done without loading notes
  • whenever a user clicks on a revision in a graph, the revision is passed on to the commit info control
  • the commit info control loads notes only if the commit's body property is null

IIUC, we started loading the body of commits to show [...] in the grid.

Thinking out load, we can introduce Notes property, that can be loaded on demand, as oppose to tucking available notes to the body of a commit.
Thoughts?

@gerhardol
Copy link
Member

Thoughts?

Seem OK

@gerhardol
Copy link
Member

This is worse than I saw in the first analysis. Notes can only be seen for commits older than 6 months that has multiline messages.

#6026 implements the handling similar to how it was done in 2.x, requesting the information if needed in CommitInfo. I.e. GE need to track if notes is included in the Body or if it should be loaded with a separate property. This limit processing and storage but adds some tenths of ms extra to open CommitInfo (not an issue).
However, the "async loading" is required also in other situations, where the extra time may be too much:

  • The tooltip info includes the notes only if available, i.e. the CommitInfo has been shown (the info could be read async and tooltip being updated)
  • Copy similar to tooltip (the info should load here though), you need to click to copy

Another way could be to always load the notes or to separate notes and body in separate attributes.

@RussKie
Copy link
Member

RussKie commented Jan 2, 2019

Let's patch it the way you've done it. We can rework it in the following releases.

@gerhardol gerhardol modified the milestones: 3.1.0, 3.0.1 Jan 3, 2019
@ghost ghost removed the status: ready label Jan 7, 2019
@RussKie RussKie modified the milestones: 3.0.1, 3.1.0 Jan 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug 🐛 type: regression regression, normally to latest official release
Projects
None yet
Development

No branches or pull requests

5 participants