Skip to content

FormCommit: Read committer info using git #10843

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
Merged
merged 2 commits into from
Apr 1, 2023
Merged

FormCommit: Read committer info using git #10843

merged 2 commits into from
Apr 1, 2023

Conversation

mstv
Copy link
Member

@mstv mstv commented Mar 29, 2023 •
edited
Loading

Fixes #5492 (comment)

Proposed changes

  • FormCommit: Read committer info using git,
    use RunAsync and
    do not cache the git output in order to update on FormActivate (existing testcase)

Screenshots

unchanged

Test methodology

  • manual
  • existing tests

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.

@mstv mstv self-assigned this Mar 29, 2023
gerhardol
gerhardol reviewed Mar 29, 2023
View reviewed changes
Copy link
Member

@gerhardol gerhardol left a comment

Choose a reason for hiding this comment

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

+1
This increases the startup time for the first start of FormCommit with about 150 ms.
Annoying...
The cache should probably be cleared when exiting settings form.

GitCommands/Git/GitModule.cs Outdated
@@ -2313,8 +2313,14 @@ public string GetSetting(string setting)
return LocalConfigFile.GetValue(setting);
}

public string GetEffectiveSetting(string setting)
public string GetEffectiveSetting(string setting, bool useGit = false, bool useCache = false)
Copy link
Member

Choose a reason for hiding this comment

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

Use a separate function instead?

Copy link
Member

Choose a reason for hiding this comment

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

Yes please. Let's call it GetEffectiveGitSetting(). Cache by default until we have a real use case that requires no caching.

RussKie
RussKie reviewed Mar 30, 2023
View reviewed changes
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 simulating the reported scenario would be great to have.

@RussKie RussKie added 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity and removed 👓 status: needs review labels Mar 30, 2023
@RussKie
Copy link
Member

RussKie commented Mar 30, 2023

Should the setting be reloaded when a repo is switched?

@vbjay
Copy link
Contributor

vbjay commented Mar 30, 2023

Should the setting be reloaded when a repo is switched?

Yes. Config in other repo can be different.

@mstv
Copy link
Member Author

mstv commented Mar 30, 2023

This increases the startup time for the first start of FormCommit with about 150 ms.

I am going to do it async.

The cache should probably be cleared when exiting settings form.

good point

Should the setting be reloaded when a repo is switched?

Yes. Config in other repo can be different.

I thought we do so already - and was wrong again as in a discussion longer ago. I am adding a non-static cache.

Tests simulating the reported scenario would be great to have.

I am going to adapt the existing tests as far as necessary.
My interest in this feature is very limited though. I just see the necessity to correctly display the info. Let's leave it up to git.
I hope @Gutza will verify it. Could you setup an integration testcase?

@ghost ghost removed the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Mar 30, 2023
@mstv
Copy link
Member Author

mstv commented Mar 30, 2023

I am adding a non-static cache.

All current usages of the GitModule.GitCommandCache seem to be repo-specific as far as I can see.
So, having only a static instance is not correct for these.
Are there objections turning it into a member instance (other than the binding in FormGitCommandLog)?

BTW: Is a cache entry really such expensive that we limit the MRU cache to 50 entries?

@mstv mstv force-pushed the fix/i5492_committer branch 3 times, most recently from b089a68 to 97f7a26 Compare March 30, 2023 20:27
@mstv mstv force-pushed the fix/i5492_committer branch from 97f7a26 to 0b0d26a Compare March 30, 2023 20:56
gerhardol
gerhardol approved these changes Mar 30, 2023
View reviewed changes
Copy link
Member

@gerhardol gerhardol left a comment

Choose a reason for hiding this comment

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

Have not run, but it looks fine.

GitUI/CommandsDialogs/FormCommit.cs Outdated
? committer
: $"{committer} {_commitAuthorInfo.Text} {toolAuthor.Text}";
await this.SwitchToMainThreadAsync();
commitAuthorStatus.Text = string.IsNullOrEmpty(toolAuthor.Text?.Trim())
Copy link
Member

Choose a reason for hiding this comment

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

?

Suggested change
commitAuthorStatus.Text = string.IsNullOrEmpty(toolAuthor.Text?.Trim())
commitAuthorStatus.Text = string.IsNullOrWhitespace(toolAuthor.Text))

@mstv
Copy link
Member Author

mstv commented Mar 30, 2023

Let's call it GetEffectiveGitSetting().

Thank you for directly proposing a name, Igor.

Cache by default until we have a real use case that requires no caching.

I agree. Caching should be the default for settings.
And we already have a use case: The committer info shall be updated on FormActivate as per existing testcase.

So, since this PR does not depend on caching (RunAsync is the key), this PR would be ready if AppVeyor would be able to run Form tests without headstands.
Any idea how to make Should_update_committer_info_on_form_activated pass?

@mstv mstv force-pushed the fix/i5492_committer branch from b412669 to f9631e2 Compare March 30, 2023 21:36
@mstv
Copy link
Member Author

mstv commented Mar 30, 2023

Sorry AppVeyor, I have found where to place the missing wait.

@mstv
Copy link
Member Author

mstv commented Mar 31, 2023

Does https://ci.appveyor.com/api/buildjobs/whg8axsj2xdhj86v/artifacts/artifacts%2FRelease%2Fpublish%2FGitExtensions-Portable-4.1.0.16396-f9631e23c.zip work for you, @Gutza?

@mstv mstv merged commit 5e79d32 into gitextensions:master Apr 1, 2023
@mstv mstv deleted the fix/i5492_committer branch April 1, 2023 07:46
@ghost ghost added this to the vNext milestone Apr 1, 2023
@Gutza
Copy link

Gutza commented Apr 1, 2023

Does https://ci.appveyor.com/api/buildjobs/whg8axsj2xdhj86v/artifacts/artifacts%2FRelease%2Fpublish%2FGitExtensions-Portable-4.1.0.16396-f9631e23c.zip work for you, @Gutza?

Yes, it works wonderfully, thank you!

@gerhardol gerhardol mentioned this pull request Aug 16, 2023
Open
@gerhardol gerhardol mentioned this pull request Nov 18, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RussKie RussKie RussKie approved these changes

@gerhardol gerhardol gerhardol approved these changes

Assignees

@mstv mstv

Labels
None yet
Projects
None yet
Milestone
4.1
Development

Successfully merging this pull request may close these issues.

Support for .gitconfig conditional includes
5 participants
@mstv @RussKie @vbjay @Gutza @gerhardol