Add --force flag to fast fetch #159

kyle-rader · 2018-08-13T21:51:56Z

Adding `--force` to FastFetch

This adds a --force option when using FastFetch. This will cause FastFetch to treat the repo as if it had just been initialized meaning new folders can be fetched after the initial FastFetch is run. You can effectively grow the number of folders a FastFetched repo has.

This is required for boot-strapping scenarios where the set of desired folders to fetch must be dynamically computed after an initial fast fetch.

sanoursa · 2018-08-13T22:20:51Z

GVFS/FastFetch/CheckoutPrefetcher.cs

@@ -66,7 +66,7 @@ public override void Prefetch(string branchOrCommit, bool isBranch)
            // Configure pipeline
            // Checkout uses DiffHelper when running checkout.Start(), which we use instead of LsTreeHelper
            // Checkout diff output => FindMissingBlobs => BatchDownload => IndexPack => Checkout available blobs
-            CheckoutJob checkout = new CheckoutJob(this.checkoutThreadCount, this.FolderList, commitToFetch, this.Tracer, this.Enlistment);
+            CheckoutJob checkout = new CheckoutJob(this.checkoutThreadCount, this.FolderList, commitToFetch, this.Tracer, this.Enlistment, force: force);


Nit: there's no need for a named parameter here

sanoursa · 2018-08-13T22:22:13Z

GVFS/FastFetch/FastFetchVerb.cs

+            "force",
+            Required = false,
+            Default = false,
+            HelpText = "Force FastFetch to download content as if the current repository was just initialized.")]


It's not really "downloading" that's the issue, it's checking out the files.This flag only does anything if you also specify --checkout right? I think that also suggests that the flag should be called --force-checkout

Hmm, currently it controls the diff for both the --checkout and non checkout cases. There are 2 blob fetchers for each case. This usage is really just for checkout though so I like the idea of calling it --force-checkout and then limiting it to the checkout path.

sanoursa · 2018-08-13T22:22:43Z

GVFS/FastFetch/FastFetchVerb.cs

@@ -237,7 +244,7 @@ private int ExecuteWithExitCode()
                            try
                            {
                                bool isBranch = this.Commit == null;
-                                prefetcher.Prefetch(commitish, isBranch);
+                                prefetcher.Prefetch(commitish, isBranch, force: this.Force);


In general, you only need to use named parameters when passing in a literal. No need when the param name and variable name are equally descriptive.

sanoursa · 2018-08-13T22:28:04Z

GVFS/GVFS.Common/Prefetch/BlobPrefetcher.cs

@@ -162,13 +162,13 @@ public static void AppendToNewlineSeparatedFile(PhysicalFileSystem fileSystem, s
        }

        /// <param name="branchOrCommit">A specific branch to filter for, or null for all branches returned from info/refs</param>
-        public virtual void Prefetch(string branchOrCommit, bool isBranch)
+        public virtual void Prefetch(string branchOrCommit, bool isBranch, bool force = false)


There's no need for a default value for this parameter. There's only one caller of this method, and it passes in a value.

sanoursa · 2018-08-13T22:34:29Z

GVFS/GVFS.Common/Prefetch/BlobPrefetcher.cs

@@ -177,7 +177,8 @@ public virtual void Prefetch(string branchOrCommit, bool isBranch)
            bool readFilesAfterDownload,
            out int matchedBlobCount,
            out int downloadedBlobCount,
-            out int readFileCount)
+            out int readFileCount,
+            bool force = false)


Seeing the force flag passed around everywhere makes me think that a slight redesign is in order here.

It's actually kind of meaningless to pass in force=true when dealing with the BlobPrefetcher directly. That flag only makes sense when doing a checkout, via fastfetch, so it seems like it should only be a parameter to the constructor of CheckoutPrefetcher. That constructor can then pass a flag to the base constructor telling it to ignore the shallow file, but no caller of BlobPrefetcher should have access to that.

sanoursa · 2018-08-13T22:38:51Z

GVFS/GVFS.Common/Prefetch/BlobPrefetcher.cs

-            // Use the shallow file to find a recent commit to diff against to try and reduce the number of SHAs to check
-            DiffHelper blobEnumerator = new DiffHelper(this.Tracer, this.Enlistment, this.FileList, this.FolderList);
-            if (File.Exists(shallowFile))
+            // Use the shallow file to find a recent commit to diff against to try and reduce the number of SHAs to check.


I think all of this logic of looking for the shallow file only applies to fastfetch, meaning you could move this into a method of CheckoutPrefetcher that overrides an empty virtual method in this class. With that approach, you can keep the force logic completely contained within CheckoutPrefetcher and not even pass that flag to its base constructor.

sanoursa · 2018-08-13T22:42:24Z

GVFS/GVFS.Common/Prefetch/Jobs/CheckoutJob.cs

@@ -31,13 +32,14 @@ public class CheckoutJob : Job
        // Checkout requires synchronization between the delete/directory/add stages, so control the parallelization
        private int maxParallel;

-        public CheckoutJob(int maxParallel, IEnumerable<string> folderList, string targetCommitSha, ITracer tracer, Enlistment enlistment)
+        public CheckoutJob(int maxParallel, IEnumerable<string> folderList, string targetCommitSha, ITracer tracer, Enlistment enlistment, bool force = false)


I think this class should never have been moved to GVFS.Common. It is only used by CheckoutPrefetcher, so it can be moved back into the FastFetch project. This wasn't your change, but would you mind cleaning that up? (Again, the goal being to contain the force logic rather than spread it all over)

Responding to last 3 comments - all sounds good. Thanks for the quick review!

sanoursa · 2018-08-13T22:43:22Z

GVFS/FastFetch/FastFetchVerb.cs

+            Required = false,
+            Default = false,
+            HelpText = "Force FastFetch to download content as if the current repository was just initialized.")]
+        public bool Force { get; set; }


We should have logic down below that enforces that --force (or --force-checkout) must be paired with --checkout

kyle-rader · 2018-08-14T17:29:49Z

@sanoursa I've factored the force-checkout logic exclusively into the FastFetch project.

sanoursa

The latest changes look good to me, thanks for the cleanup

sanoursa · 2018-08-14T17:58:56Z

GVFS/FastFetch/FastFetchVerb.cs

+            Default = false,
+            HelpText = "Force FastFetch to fetch and checkout content as if the current repo had just been initialized." +
+                       "This allows you to include more folders from the repo that were not originally checked out." +
+                       "Can only be used with --checkout")]


Nit: Add line breaks between the lines of the message

@sanoursa Adding actual line breaks in the help messages causes it to render oddly on the CLI, it wraps at the correct indentation level though.

With actual line breaks in the message:

Got it, yea if the CLI is already handling multiline help messages, then leave it alone :-). I think then the only issue with your original string was that there was no space after the .'s

sanoursa · 2018-08-14T18:00:11Z

GVFS/FastFetch/FastFetchVerb.cs

+            "force-checkout",
+            Required = false,
+            Default = false,
+            HelpText = "Force FastFetch to fetch and checkout content as if the current repo had just been initialized." +


I don't think this flag actually affects what we "fetch", at least not directly. It does affect what files we want to checkout, but if we already have the blobs, we're not going to re-download them.

sanoursa · 2018-08-14T18:00:52Z

GVFS/GVFS.Common/Prefetch/BlobPrefetcher.cs

-            // Use the shallow file to find a recent commit to diff against to try and reduce the number of SHAs to check
-            DiffHelper blobEnumerator = new DiffHelper(this.Tracer, this.Enlistment, this.FileList, this.FolderList);
+            // Use the shallow file to find a recent commit to diff against to try and reduce the number of SHAs to check.
+            // Unless force flag has been given, in which case treat as if it's a fresh repo.


We don't know anything about the force flag here

sanoursa · 2018-08-14T19:55:02Z

Please make sure to run the full functional tests as well, since the required set of functional tests don't actually execute the fastfetch test cases

kyle-rader · 2018-08-15T14:42:29Z

Full Functional Test Suite has passed.

Includes changes from microsoft#158 and microsoft#159. Signed-off-by: Derrick Stolee <dstolee@microsoft.com>

sanoursa suggested changes Aug 13, 2018

View reviewed changes

sanoursa approved these changes Aug 14, 2018

View reviewed changes

wilbaker approved these changes Aug 15, 2018

View reviewed changes

Kyle Rader added 4 commits August 16, 2018 16:32

Add --force flag to fast fetch (apply patch from VSTS work)

ccbd974

Simplify help text for force flag

2a6b4da

Rename force to force-checkout and pull into FastFetch CheckoutJob

22e41b5

Update comments and help text

f41452d

kyle-rader force-pushed the feature/fast-fetch-force-flag branch from cfb1c05 to f41452d Compare August 16, 2018 23:32

nickgra merged commit 4e195a4 into microsoft:master Aug 17, 2018

sanoursa mentioned this pull request Aug 17, 2018

ForceCheckoutRequiresCheckout test fails #185

Closed

kyle-rader deleted the feature/fast-fetch-force-flag branch August 20, 2018 17:51

jrbriggs mentioned this pull request Oct 12, 2018

Merge milestones/M142 to releases/shipped #381

Merged

derrickstolee added a commit to derrickstolee/VFSForGit that referenced this pull request Jul 22, 2019

Update Git to include tracing updates

0dcacf7

Includes changes from microsoft#158 and microsoft#159. Signed-off-by: Derrick Stolee <dstolee@microsoft.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --force flag to fast fetch #159

Add --force flag to fast fetch #159

kyle-rader commented Aug 13, 2018

sanoursa Aug 13, 2018

sanoursa Aug 13, 2018

wilbaker Aug 13, 2018

kyle-rader Aug 14, 2018

sanoursa Aug 13, 2018

sanoursa Aug 13, 2018

sanoursa Aug 13, 2018

sanoursa Aug 13, 2018

sanoursa Aug 13, 2018 •

edited

Loading

kyle-rader Aug 14, 2018

sanoursa Aug 13, 2018

kyle-rader commented Aug 14, 2018

sanoursa left a comment

sanoursa Aug 14, 2018

kyle-rader Aug 14, 2018

kyle-rader Aug 14, 2018 •

edited

Loading

sanoursa Aug 14, 2018

sanoursa Aug 14, 2018

sanoursa Aug 14, 2018

sanoursa commented Aug 14, 2018

kyle-rader commented Aug 15, 2018

Add --force flag to fast fetch #159

Add --force flag to fast fetch #159

Conversation

kyle-rader commented Aug 13, 2018

Adding --force to FastFetch

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanoursa Aug 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyle-rader commented Aug 14, 2018

sanoursa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyle-rader Aug 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanoursa commented Aug 14, 2018

kyle-rader commented Aug 15, 2018

Adding `--force` to FastFetch

sanoursa Aug 13, 2018 •

edited

Loading

kyle-rader Aug 14, 2018 •

edited

Loading