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

Ignore nonexisting submodule paths #9819

Merged

Conversation

gerhardol
Copy link
Member

Fixes #8366

Proposed changes

Show MessageBox with submodule information

Replaces #9612, also some other situations handling #9056
This could be done in a nicer way, but this is important to point users in a better direction just not ignoring (as in 3.5) or showing a popup recommending to report an issue on GE (as in master).
For users, the behavior in master seem like a regression.

  1. See Submodules: ignore bad submodules instead of crashing #9612 - due to null types in master
    Note that the popup is shown two times when opening a repo (structure and status) then periodically when status is updated. (So did the Validate error.)
    The popup could be limited to be shown for just the first popup when the structure has been updated, but this is an error in the repo setup.

See this (auto closed) PR for how to recreate the issue.

  1. Open when submodule path (not yet) exists due to Throw on non-zero exit code and on stderror output #9056
  • Submodule Open in Sidepanel
  • Submodule open in RevDiff/RevFileTree
  • Submodule open in Browse submodule toolbar

This is easiest done by deleting or renaming a submodule.
This also occurred when doubleclicking a submodule no longer existing, for instance the GitExtensionsDoc for commits two years ago.
Some occurrences like doubleclicking a deleted submodule in RevDiff was previously just ignored.

Screenshots

Before

See also #9612

image

After

image

image

image

Test methodology

Manual

Merge strategy

I agree that the maintainer squash merge this PR (if the commit message is clear).


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

@ghost ghost assigned gerhardol Jan 15, 2022
@RussKie
Copy link
Member

RussKie commented Jan 15, 2022 via email

@gerhardol
Copy link
Member Author

Should we provide a button/link to open submodules management dialog?

I rather keep this simple, there are other PRs that I want to have merged...

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.

Use MessageBoxes, please.

@RussKie
Copy link
Member

RussKie commented Jan 18, 2022

I'm worried that an average Joe user may not know what to do when these messages pop up. A link to "Manage submodules" will provide a significantly more positive experience.

@gerhardol gerhardol force-pushed the feature/i9612-no-directory-browse branch from 74d3b95 to bfee15f Compare January 19, 2022 20:50
@gerhardol
Copy link
Member Author

Use MessageBoxes, please.

Done

I'm worried that an average Joe user may not know what to do when these messages pop up. A link to "Manage submodules" will provide a significantly more positive experience.

I just want something good enough. This is better than the igore or error popup in 3.5 or the popup after #9056 in master.
This can be improved sure, there are other issues to work on too.

@@ -93,6 +93,12 @@ protected override string NodeName()

public void Open()
{
if (!Directory.Exists(Info.Path))
{
MessageBoxes.SubmoduleDirectoryDoesNotExist(null, Info.Path, Info.Text);
Copy link
Member

Choose a reason for hiding this comment

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

Give the null parameter name. Why is it null? The error will be unparented, and can get lost behind other windows.

Copy link
Member Author

Choose a reason for hiding this comment

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

changed to this where possible (not in side panel or the generic Launch).

@gerhardol gerhardol force-pushed the feature/i9612-no-directory-browse branch from bfee15f to 0a3e671 Compare January 19, 2022 21:13
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.

LGTM

GitUI/CommandsDialogs/FormBrowse.cs Outdated Show resolved Hide resolved
Show MessageBox with submodule information
@gerhardol gerhardol force-pushed the feature/i9612-no-directory-browse branch from b95f513 to 7884381 Compare January 21, 2022 21:24
@gerhardol
Copy link
Member Author

Plan to squash merge tomorrow.
Possible enhancements can be added later.

@gerhardol gerhardol merged commit cedee80 into gitextensions:master Jan 25, 2022
@gerhardol gerhardol deleted the feature/i9612-no-directory-browse branch January 25, 2022 23:39
@ghost ghost added this to the vNext milestone Jan 25, 2022
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.

[NBug] Crashing on startup when no submodule present
3 participants