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

RevDiff: FirstRevision is null for the initial commit #8086

Merged
merged 4 commits into from May 17, 2020

Conversation

gerhardol
Copy link
Member

@gerhardol gerhardol commented May 9, 2020

Fixes #8085

Proposed changes

  • Refactor: Align FirstParentId name
    Guid suffix is quite consistently used for the string representation of the hash, while Id suffix is used for ObjectId, this was therefore switched
    Some parentGuids changed too, not changing treeGuid

  • Use "--root" for first revision if null
    Handle GitRevision == null as the root commit, before the initial commit, i.e. compare to /dev/null
    SemanticMerge and KDiff3 shows such a diff, Tortoise does not for some reason
    (The alternative would be to not add null to the revisions to diff which will result in a argument popup which is OK to me.)

  • RevDiff: FirstRevision is null for the initial commit
    Handle that FirstRevision is not set for the initial commit
    All situations changed are not causing problems, but it is easier to handle FirstRevision consistently this way.

Should be merged with these three merges, separate PR for 3.4 when approved

Test methodology

Review if FirstRevision usage and Manual
The (meaningless) IRevisionDiffController do not cover arguments in RevisionDiffControl


✒️ I contribute this code under The Developer Certificate of Origin.

@ghost ghost assigned gerhardol May 9, 2020
@gerhardol gerhardol requested a review from mstv May 9, 2020 10:01
@gerhardol gerhardol changed the title Bugfix/i8085 first null RevDiff: FirstRevision is null for the initial commit May 9, 2020
@codecov
Copy link

codecov bot commented May 9, 2020

Codecov Report

Merging #8086 into master will increase coverage by 0.02%.
The diff coverage is 58.82%.

@@            Coverage Diff             @@
##           master    #8086      +/-   ##
==========================================
+ Coverage   49.54%   49.57%   +0.02%     
==========================================
  Files         847      848       +1     
  Lines       60916    60957      +41     
  Branches    10924    10927       +3     
==========================================
+ Hits        30182    30217      +35     
- Misses      28497    28504       +7     
+ Partials     2237     2236       -1     
Flag Coverage Δ
#production 37.37% <31.42%> (+0.01%) ⬆️
#tests 94.86% <87.87%> (+<0.01%) ⬆️

@gerhardol gerhardol force-pushed the bugfix/i8085-first-null branch 2 times, most recently from 7cfe454 to 9d7acd0 Compare May 9, 2020 11:37
@fsfod
Copy link

fsfod commented May 9, 2020

I guess what happens for "Open with Diff tool" menu entries if the selected file's FirstRevision is null are going to be weird or broken. The only one enabled is "first -> selected" but p4 merge doesn't like "nul" as the second file name idk if thats bug or not. Trying "cherry picked changes" for the same file does nothing idk if it should or not.

@gerhardol
Copy link
Member Author

I guess what happens for "Open with Diff tool" menu entries if the selected file's FirstRevision is null are going to be weird or broken.

This PR handles firstRev==null as "--root", so in supported difftools is works OK
Diff should be handled as for new files

The only one enabled is selected -> first but p4 merge doesn't like "nul" as the second file name idk if thats bug or not. Trying "cherry picked changes" for the same file does nothing idk if it should or not.

A specific problem in p4merge, similar to tortoisemerge but you get a popup
semanticmerge and kdiff3 displays the "diff". Blocking diff for new files would remove this functionality for these tools, just to not show the popup for p4merge

@fsfod
Copy link

fsfod commented May 9, 2020

Yes the original issue is fixed for me.

Copy link
Member

@RussKie RussKie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests for RevisionDiffInfoProvider changes?

Comment on lines 288 to 291
var parentIds = DiffFiles.SelectedItems
.Where(i => i.FirstRevision != null)
.Select(i => i.FirstRevision.ObjectId)
.Distinct()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're executing this selector multiple times.
Have you considered creating a property on FileStatusList (similar to SelectedGitItems) to simply the call sites?

E.g. something like:

    public sealed partial class FileStatusList : GitModuleControl
    {
        [DesignerSerializationVisibility(DesignerSerializationVisibility.Hidden)]
        [Browsable(false)]
        public IEnumerable<FileStatusItem> SelectedGitObjects
        {
            get => SelectedItems
                .Where(i => i.FirstRevision != null)
                .Select(i => i.FirstRevision.ObjectId)
                .Distinct();
        }

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was a little like this before, with many access functions. These properties applies to Enumerate for the selected items. Similar could apply to all items etc,
But I do not know how to do this in the best way, I assumed that a static helper on FileStatusItem would not be accepted, so I let that be.
Similar for First, Second, GitStatus.
(I was not ready thinking about this very late with #7899 and did not want to do any more changes...)

SelectedGitItems should be removed too, but it appeared in a little too many places.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added extension methods, OK now?

@RussKie RussKie added this to the 3.4 milestone May 10, 2020
@gerhardol
Copy link
Member Author

Tests for RevisionDiffInfoProvider changes?

Added tests as well as reordered arguments

Copy link
Member

@mstv mstv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@gerhardol gerhardol force-pushed the bugfix/i8085-first-null branch 2 times, most recently from b539c7f to 6d3a2ae Compare May 13, 2020 22:14
@RussKie RussKie merged commit 5d5c4ee into gitextensions:master May 17, 2020
@ghost ghost modified the milestones: 3.4, 4.0 May 17, 2020
@RussKie
Copy link
Member

RussKie commented May 17, 2020

Taking into 3.4

@gerhardol gerhardol deleted the bugfix/i8085-first-null branch May 17, 2020 12:14
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.

Right clicking an item in RevisionDiffControl results in null exception for the initial commit
4 participants