Clean working directory skip paths #11102

scottsumrall · 2023-07-13T20:16:55Z

Fixes #11099

Proposed changes

Allow users to exclude certain file paths when cleaning repository

Screenshots

Before

After

Test methodology

Manually tested in local repository
Added and ran relevant unit tests for new functionality

Test environment(s)

GIT 2.41.0 windows.2
Windows 10

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.

GitExtensions.sln

GitUI/CommandsDialogs/FormCleanupRepository.Designer.cs

GitUI/CommandsDialogs/FormCleanupRepository.cs

GitUI/CommandsDialogs/FormCleanupRepository.resx

GitUI/GitExtensionsForm.resx

UnitTests/GitCommands.Tests/Git/Commands/GitCommandHelpersTest.cs

gerhardol

looks OK, no deep review, not run

GitUI/CommandsDialogs/FormCleanupRepository.Designer.cs

pmiossec

Please use the more meaningful --exclude option string

pmiossec · 2023-07-30T10:28:56Z

GitUI/CommandsDialogs/FormCleanupRepository.cs

+            // 3. Replace whitespace with '?' and convert to POSIX path
+            // 3. join together with space as separator
+
+            return string.Join(" ", textBoxExcludePaths.Lines.Where(a => !string.IsNullOrEmpty(a)).Select(a => string.Format("-e {0}", a.Replace(" ", "?")).ToPosixPath()));


Please use the more meaningful --exclude (short option format should be used only by humans when typing the command from command line)

I also would have preferred a better letter than 'a'. 'p' for 'path'?

And maybe use string interpolation instead of string.Format()?

pmiossec · 2023-07-30T10:35:09Z

GitUI/CommandsDialogs/FormCleanupRepository.cs

            {
                return null;
            }

            // 1. get all lines from text box which are not empty
            // 2. wrap lines with ""
            // 3. join together with space as separator
-            return string.Join(" ", textBoxPaths.Lines.Where(a => !string.IsNullOrEmpty(a)).Select(a => string.Format("\"{0}\"", a)));
+            return string.Join(" ", textBoxIncludePaths.Lines.Where(a => !string.IsNullOrEmpty(a)).Select(a => string.Format("\"{0}\"", a)));


I would have preferred a better letter than 'a'. 'p' for 'path'?

And maybe use string interpolation instead of string.Format()?

Should we convert to posix path here also?

mstv · 2023-08-02T21:05:36Z

Could you rebase and resolve the conflicts, please?

Conflicting files

GitUI/CommandsDialogs/FormCleanupRepository.Designer.cs

GitUI/CommandsDialogs/FormCleanupRepository.cs

contributors.txt

gerhardol · 2023-08-04T19:50:21Z

GitUI/CommandsDialogs/FormCleanupRepository.cs

+            }
+
+            // 1. get all lines from text box which are not empty
+            // 2. Prepend lines with '-e '


Suggested change

// 2. Prepend lines with '-e '

// 2. Prepend lines with '--exclude '

Same for tests -e there too.

I also would prefer to move this to GitCommandHelpers.cs, just providing the string or an array (if checkbox set).
(This would affect tests too.)
But I leave this to another maintainer to request, I can live with this.

RussKie · 2023-08-06T08:10:45Z

Judging by the number of commits it doesn't look like you managed to cleanly rebase your work. E.g., there are unrelated commits:

Please perform a clean rebase of your work on top of the latest master. https://github.com/gitextensions/gitextensions/wiki/How-To%3A-Squash-and-Rebase-your-changes#rebase may be of some help.

RussKie

Please rebase

scottsumrall · 2023-08-07T19:52:58Z

Judging by the number of commits it doesn't look like you managed to cleanly rebase your work. E.g., there are unrelated commits:

Please perform a clean rebase of your work on top of the latest master. https://github.com/gitextensions/gitextensions/wiki/How-To%3A-Squash-and-Rebase-your-changes#rebase may be of some help.

Apologies for the mix up, still figuring out the git workflow. Please let me know if this is looking correct.

Additionally, are you saying I should squash these commits together?

gerhardol · 2023-08-07T21:43:25Z

It is not fixed. Rebase preserving commits is not straight forward as there are intermediate conflicts.

In this case, it is easiest to squash on the BASE (soft reset), commit with proper message and then rebase on master. There are quite some commits here, I do not think they add value in a further review (review has basically to be restarted, in this case it is OK).
There should only be 6 changed files in the commit, other files have not been changed in master.

scottsumrall · 2023-08-11T17:58:38Z

It is not fixed. Rebase preserving commits is not straight forward as there are intermediate conflicts.

In this case, it is easiest to squash on the BASE (soft reset), commit with proper message and then rebase on master. There are quite some commits here, I do not think they add value in a further review (review has basically to be restarted, in this case it is OK). There should only be 6 changed files in the commit, other files have not been changed in master.

Would you mind taking a look at this temp branch I made? I'd like to make sure I'm doing this right before I start overwriting things here.
master...scottsumrall:gitextensions:skip-paths-test

To get this PR's branch to the same state I should just need to force push then do a simple rebase for the latest changes.

gerhardol · 2023-08-11T21:52:31Z

Would you mind taking a look at this temp branch I made? I'd like to make sure I'm doing this right before I start overwriting things here. master...scottsumrall:gitextensions:skip-paths-test

Fine, except that GitCommandHelpersTest.cs still has -e instead of --exclude
Hope you learn something!

scottsumrall · 2023-08-12T02:56:08Z

Appreciate the assistance!

RussKie · 2023-08-12T07:47:02Z

@scottsumrall thank you.

@gerhardol please merge, if all your feedback is addressed.

mstv

LGTM, have not run

mstv · 2023-08-12T13:51:20Z

GitUI/CommandsDialogs/FormCleanupRepository.cs

            string cmdOutput = FormProcess.ReadDialog(this, arguments: cleanUpCmd, Module.WorkingDir, input: null, useDialogSettings: true);
            PreviewOutput.Text = EnvUtils.ReplaceLinuxNewLinesDependingOnPlatform(cmdOutput);

            if (CleanSubmodules.Checked)
            {
-                var cleanSubmodulesCmd = GitCommandHelpers.CleanSubmodules(mode, dryRun: true, directories: RemoveDirectories.Checked, paths: pathArgument);
+                var cleanSubmodulesCmd = GitCommandHelpers.CleanSubmodules(mode, dryRun: true, directories: RemoveDirectories.Checked, paths: includePathArgument);


❔ Should we apply excludePathArguments for submodules, too?

I see two viewpoints:

Paths are relative the superproject and only should be relative the master. Hard to implement though, if wildcards are to be allowed.

apply the same as for the superproject, similar to include paths. Easier to implement, maybe not expected?

Probably second option.
@scottsumrall thoughts?

I'm not sure I completely follow. As of right now, the excludes for the general clean are only for excluding individual files and do not support wildcards. I assume it would be the same for submodules? If we want this feature included I would lean towards the first solution but the second option seems fine for its current state.

The first option would affect the include files too. But that wasrecently added without consideration.
However, I do not find it reasonable.
Use second alternative, in this PR before merge.

Add exclude paths to CleanSubmodules()
(Make sure there is a test as well, also for include not tested at all now)

GitUI/CommandsDialogs/FormCleanupRepository.cs

gerhardol · 2023-08-12T21:51:44Z

I pushed some commits, the merge was bad and preview removed files.
Part of that was that use of common code I suggsested in #11101 was not done, so I added it again.
Also using string interpolation instead of string.format - pushed a change instead of suggesting (also for existing code)

gerhardol · 2023-08-13T18:51:33Z

@scottsumrall Please acknowledge the changes and I will merge

scottsumrall · 2023-08-13T19:58:21Z

Looks good to me

scottsumrall · 2023-08-14T19:31:24Z

@gerhardol Should I just open a new PR with the changes we discussed or should I create a new issue?

gerhardol · 2023-08-14T20:07:25Z

@gerhardol Should I just open a new PR with the changes we discussed or should I create a new issue?

A new PR only is enough. PR not needed regardless - not big enough to track in my opinion, still reasonable to make this change 'complete'.

ghost assigned scottsumrall Jul 13, 2023

RussKie requested changes Jul 14, 2023

View reviewed changes

ghost added 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity and removed 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity labels Jul 14, 2023

scottsumrall marked this pull request as draft July 14, 2023 19:36

scottsumrall requested a review from RussKie July 15, 2023 19:23

RussKie mentioned this pull request Jul 18, 2023

Feature/i11054 clean submodules #11101

Merged

scottsumrall marked this pull request as ready for review July 18, 2023 19:17

gerhardol reviewed Jul 18, 2023

View reviewed changes

GitUI/CommandsDialogs/FormCleanupRepository.Designer.cs Outdated Show resolved Hide resolved

RussKie approved these changes Jul 30, 2023

View reviewed changes

RussKie added 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity and removed 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity labels Jul 30, 2023

pmiossec requested changes Jul 30, 2023

View reviewed changes

ghost added 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity and removed 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity labels Jul 30, 2023

pmiossec approved these changes Aug 2, 2023

View reviewed changes

scottsumrall closed this Aug 3, 2023

scottsumrall reopened this Aug 3, 2023

gerhardol reviewed Aug 4, 2023

View reviewed changes

RussKie requested changes Aug 6, 2023

View reviewed changes

ghost added the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Aug 6, 2023

ghost removed the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Aug 7, 2023

scottsumrall force-pushed the clean-working-directory-skip-paths branch from 3a9c2a8 to c18c621 Compare August 12, 2023 02:08

Feature/i11099-skip-file-paths

172e5fd

scottsumrall force-pushed the clean-working-directory-skip-paths branch from c18c621 to 172e5fd Compare August 12, 2023 02:09

RussKie approved these changes Aug 12, 2023

View reviewed changes

mstv reviewed Aug 12, 2023

View reviewed changes

gerhardol added 4 commits August 12, 2023 23:36

fixup! mstv review comment

b475a3f

fixup! bad merge

5379a4e

fixup! review comment to to use string interpolating

5bfe577

fixup! Use common code for cleanup

faa935e

RussKie added the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Aug 13, 2023

ghost removed the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Aug 13, 2023

RussKie merged commit b94bd9c into gitextensions:master Aug 14, 2023
3 of 4 checks passed

ghost added this to the vNext milestone Aug 14, 2023

scottsumrall mentioned this pull request Aug 17, 2023

Allow skipping file paths in recursive submodule directory clean #11163

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean working directory skip paths #11102

Clean working directory skip paths #11102

scottsumrall commented Jul 13, 2023

gerhardol left a comment

pmiossec left a comment

pmiossec Jul 30, 2023

pmiossec Jul 30, 2023

mstv commented Aug 2, 2023

gerhardol Aug 4, 2023

RussKie commented Aug 6, 2023

RussKie left a comment

scottsumrall commented Aug 7, 2023

gerhardol commented Aug 7, 2023

scottsumrall commented Aug 11, 2023

gerhardol commented Aug 11, 2023

scottsumrall commented Aug 12, 2023

RussKie commented Aug 12, 2023

mstv left a comment

mstv Aug 12, 2023

gerhardol Aug 12, 2023

scottsumrall Aug 13, 2023

gerhardol Aug 13, 2023

gerhardol commented Aug 12, 2023

gerhardol commented Aug 13, 2023

scottsumrall commented Aug 13, 2023

scottsumrall commented Aug 14, 2023

gerhardol commented Aug 14, 2023

	// 2. Prepend lines with '-e '
	// 2. Prepend lines with '--exclude '

Clean working directory skip paths #11102

Clean working directory skip paths #11102

Conversation

scottsumrall commented Jul 13, 2023

Proposed changes

Screenshots

Before

After

Test methodology

Test environment(s)

Merge strategy

gerhardol left a comment

Choose a reason for hiding this comment

pmiossec left a comment

Choose a reason for hiding this comment

pmiossec Jul 30, 2023

Choose a reason for hiding this comment

pmiossec Jul 30, 2023

Choose a reason for hiding this comment

mstv commented Aug 2, 2023

gerhardol Aug 4, 2023

Choose a reason for hiding this comment

RussKie commented Aug 6, 2023

RussKie left a comment

Choose a reason for hiding this comment

scottsumrall commented Aug 7, 2023

gerhardol commented Aug 7, 2023

scottsumrall commented Aug 11, 2023

gerhardol commented Aug 11, 2023

scottsumrall commented Aug 12, 2023

RussKie commented Aug 12, 2023

mstv left a comment

Choose a reason for hiding this comment

mstv Aug 12, 2023

Choose a reason for hiding this comment

gerhardol Aug 12, 2023

Choose a reason for hiding this comment

scottsumrall Aug 13, 2023

Choose a reason for hiding this comment

gerhardol Aug 13, 2023

Choose a reason for hiding this comment

gerhardol commented Aug 12, 2023

gerhardol commented Aug 13, 2023

scottsumrall commented Aug 13, 2023

scottsumrall commented Aug 14, 2023

gerhardol commented Aug 14, 2023