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.

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

Allow skipping file paths in recursive submodule directory clean #11163

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Allow skipping file paths in recursive submodule directory clean #11163

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b0ae93c
Skip paths in clean submodules
scottsumrall Aug 15, 2023
78a8005
Make path relative to module for recursive clean
scottsumrall Aug 17, 2023
638c39a
Minor style changes
scottsumrall Aug 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 3 additions & 2 deletions GitCommands/Git/Commands/GitCommandHelpers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -551,15 +551,16 @@ public static ArgumentString CleanCmd(CleanMode mode, bool dryRun, bool director
/// <param name="dryRun">Only show what would be deleted.</param>
/// <param name="directories">Delete untracked directories too.</param>
/// <param name="paths">Limit to specific paths.</param>
public static ArgumentString CleanSubmodules(CleanMode mode, bool dryRun, bool directories, string? paths = null)
public static ArgumentString CleanSubmodules(CleanMode mode, bool dryRun, bool directories, string? paths = null, string? excludes = null)
{
return new GitArgumentBuilder("submodule")
{
"foreach --recursive git clean",
mode,
{ directories, "-d" },
{ dryRun, "--dry-run", "-f" },
paths
paths,
{ !string.IsNullOrEmpty(excludes), excludes }
};
}

Expand Down
25 changes: 22 additions & 3 deletions GitUI/CommandsDialogs/FormCleanupRepository.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using GitCommands;
using System.Collections.Generic;
using GitCommands;
using GitCommands.Git.Commands;
using GitCommands.Utils;
using GitExtUtils;
Expand Down Expand Up @@ -63,7 +64,7 @@ private void CleanUp(bool dryRun)

if (CleanSubmodules.Checked)
{
ArgumentString cleanSubmodulesCmd = GitCommandHelpers.CleanSubmodules(mode, dryRun, directories: RemoveDirectories.Checked, paths: includePathArgument);
ArgumentString cleanSubmodulesCmd = GitCommandHelpers.CleanSubmodules(mode, dryRun, directories: RemoveDirectories.Checked, paths: includePathArgument, excludes: excludePathArguments);
cmdOutput = FormProcess.ReadDialog(this, arguments: cleanSubmodulesCmd, Module.WorkingDir, input: null, useDialogSettings: true);
PreviewOutput.Text += EnvUtils.ReplaceLinuxNewLinesDependingOnPlatform(cmdOutput);
}
Expand Down Expand Up @@ -164,11 +165,29 @@ private void AddExcludePath_Click(object sender, EventArgs e)

if (path is not null)
{
path = path.Replace(Module.WorkingDir, "");
path = MakePathRelativeToModule(path);
textBoxExcludePaths.Text += path;
}
}

private string MakePathRelativeToModule(string targetPath)
{
targetPath = targetPath.Replace(Module.WorkingDir, "").ToPosixPath();
IReadOnlyList<string> submodulePaths = Module.GetSubmodulesLocalPaths();
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
targetPath = targetPath.Replace(Module.WorkingDir, "").ToPosixPath();
targetPath = targetPath.ToPosixPath().Replace(Module.WorkingDir, "");

This should be tested, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tested, this doesn't work since WorkingDir is not stored as Posix path

Copy link
Member

Choose a reason for hiding this comment

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

Does the user need to input paths in the native form in order to match WorkingDir?
I would not make assumptions on the type of path separators in WorkingDir either.
I.e. convert both?

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
targetPath = targetPath.Replace(Module.WorkingDir, "").ToPosixPath();
targetPath = targetPath.ToPosixPath().Replace(Module.WorkingDir.ToPosixPath(), "");

Copy link
Member

Choose a reason for hiding this comment

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

Sorry if I was not clear, but it may be clearer to not resolve a relative path here. Regex cannot be resolved anyway.
#11102 (comment)

If replaced, the exclude (and include) path should be changed for each submodule individually. The "cleaned" path could be have an unexpected effect for the other superprojects and submodules. There may be layers of submodules too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries, I uploaded this as draft to get clarification but forgot to comment. From my understanding, I don't see a clear way to avoid weird edge case conflicts without changes to #11101. The exclude parameter requires that the path be relative to the submodule.

For example if the user wants to exclude:
repo/submodule1/somefile.txt
the exclude has to be written as --exclude=somefile.txt When we iterate, this would also exclude a theoretical file:
repo/submodule2/somefile.txt

Since I don't think the include has a similar requirement, I don't necessarily think it needs to be changed for each submodule individually? I believe this is due to the exclude parameter following gitignore rules while the pathspec parameter (includes) just uses general file paths.

Copy link
Member

Choose a reason for hiding this comment

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

I believe no manipulation is easier to explain, so that is my preference.
somefile.txt will be ignored in the other repos anyway with this implementation.
include/exclude should be handled the same

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Either I didn't explain myself well or I am misunderstanding, apologies.

By no manipulation do you mean using the full path? I am saying that, from my understanding, the --exclude parameter fundamentally requires that the path be written relative to module, as in my previous example. This is because we aren't passing in a path, we are passing in a Glob pattern since the --exclude parameter follows gitignore rules. As we iterate through each submodule, the exclude pattern gets applied relative to the submodule the clean command is being run within.

Copy link
Member

Choose a reason for hiding this comment

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

By no manipulation do you mean using the full path?

(missed this, sorry)
Yes

The exclude can be a regex, then the path cannot be resolved. The paths that applies to the superproject will apply to the submodules anyway. There is also a question on how sub-submodules should be handled.

It is just easier to explain if exactly the same set is applied to all clean commands.

Regardless, the handling should be the same for include/exclude.

Copy link
Member

Choose a reason for hiding this comment

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

Regardless, the handling should be the same for include/exclude.

In git command line, the behavior is not exactly the same as @scottsumrall said.
Include accept relative or absolute paths whereas exclude follow the same regex pattern as .gitignore and so accept only relative paths.
So, if we want the same behavior, the only possibility is to transform path to relative ones for include.
I think that's something that should be done because I find it easier to read when double checking the path selected.
And that's also the formatting of output path when doing a dry run/preview.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This gets a little messy since the code was designed using the GUI to store the file paths. I think it might be better to separate the code from the UI. Then I could just put whatever path we want displayed in the GUI and store the proper, transformed path in code. This would also get rid of some unnecessary parsing steps. It's a bit out of scope for this PR but I think it would be beneficial.

In terms of what we want to display, this would give a lot of flexibility. Since the excludes need to be relative to the module just appending the parent module might be clear enough. This would be closest to matching the command line output while still having some level of clarity for the user.

E.g.

file1.txt
folder1/folder2/file2.txt

could look like

SuperProject/file1.txt
SubmoduleX/folder1/folder2/file2.txt



Alternatively, we could display the full path from the super project:

file1.txt
folder1/folder2/file2.txt

could look like

SuperProject/file1.txt
SuperProject/SubmoduleY/SubmoduleX/folder1/folder2/file2.txt


Whatever we decide, it should be relatively easy to make the includes that are shown to the user follow the same pattern.

Its important to note though that this is only a surface level change and the edge cases of matching files of the same relative path in different submodules will still be an issue. I am unsure how to address this other than modifying #11101 to iterate through submodules in code rather than via command line. This would allow me to only apply the relevant exclude patterns for each module.


string? nearestSubmodule = submodulePaths
.Where(submodulePath => targetPath.StartsWith(submodulePath))
.OrderByDescending(submodulePath => submodulePath.Length)
.FirstOrDefault();

if (nearestSubmodule is null)
{
return targetPath;
}

return targetPath[(nearestSubmodule.Length + 1)..];
}

private string? RequestUserFolderPath()
{
using FolderBrowserDialog dialog = new()
Expand Down
21 changes: 13 additions & 8 deletions UnitTests/GitCommands.Tests/Git/Commands/GitCommandHelpersTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -515,16 +515,21 @@ public void CleanCmd(CleanMode mode, bool dryRun, bool directories, string paths
Assert.AreEqual(expected, GitCommandHelpers.CleanCmd(mode, dryRun, directories, paths, excludes).Arguments);
}

[TestCase(CleanMode.OnlyNonIgnored, true, false, null, "clean --dry-run")]
[TestCase(CleanMode.OnlyNonIgnored, false, false, null, "clean -f")]
[TestCase(CleanMode.OnlyNonIgnored, false, true, null, "clean -d -f")]
[TestCase(CleanMode.OnlyNonIgnored, false, false, "paths", "clean -f paths")]
[TestCase(CleanMode.OnlyIgnored, false, false, null, "clean -X -f")]
[TestCase(CleanMode.All, false, false, null, "clean -x -f")]
public void CleanupSubmoduleCommand(CleanMode mode, bool dryRun, bool directories, string paths, string expected)
[TestCase(CleanMode.OnlyNonIgnored, true, false, null, null, "clean --dry-run")]
[TestCase(CleanMode.OnlyNonIgnored, false, false, null, null, "clean -f")]
[TestCase(CleanMode.OnlyNonIgnored, false, true, null, null, "clean -d -f")]
[TestCase(CleanMode.OnlyNonIgnored, false, false, "\"path1\"", null, "clean -f \"path1\"")]
[TestCase(CleanMode.OnlyNonIgnored, false, false, "\"path1\"", "--exclude=excludes", "clean -f \"path1\" --exclude=excludes")]
[TestCase(CleanMode.OnlyNonIgnored, false, false, null, "excludes", "clean -f excludes")]
[TestCase(CleanMode.OnlyNonIgnored, false, false, "\"path1\" \"path2\"", null, "clean -f \"path1\" \"path2\"")]
[TestCase(CleanMode.OnlyNonIgnored, false, false, "\"path1\" \"path2\"", "--exclude=exclude1 --exclude=exclude2", "clean -f \"path1\" \"path2\" --exclude=exclude1 --exclude=exclude2")]
[TestCase(CleanMode.OnlyNonIgnored, false, false, null, "--exclude=exclude1 --exclude=exclude2", "clean -f --exclude=exclude1 --exclude=exclude2")]
[TestCase(CleanMode.OnlyIgnored, false, false, null, null, "clean -X -f")]
[TestCase(CleanMode.All, false, false, null, null, "clean -x -f")]
public void CleanupSubmoduleCommand(CleanMode mode, bool dryRun, bool directories, string paths, string excludes, string expected)
{
string subExpected = "submodule foreach --recursive git " + expected;
Assert.AreEqual(subExpected, GitCommandHelpers.CleanSubmodules(mode, dryRun, directories, paths).Arguments);
Assert.AreEqual(subExpected, GitCommandHelpers.CleanSubmodules(mode, dryRun, directories, paths, excludes).Arguments);
}

[TestCase(null)]
Expand Down