Add EnlistmentId to RepairVerb

[jeschu1]

If we can read the enlismentId from the config file we will add it to telemetry.

resolves #498

[derrickstolee]

This is good to do, but we should stop repeating ourselves! I can see this becoming an important thing to do in more places.

I would add an EnlistmentId property to GVFSEnlistment that initializes on first call using this logic. Then replace the logic in InProcessMount.CreateTracer() with enlistment.EnlistmentId.

        private bool initializedIds;
        private string enlistmentId;
        private string mountId;

...

        public string EnlistmentId
        {
            get
            {
                this.InitializeIds();
                return this.enlistmentId;
            }
        }

        public string MountId
        {
            get
            {
                this.InitializeIds();
                return this.mountId;
            }
        }

...

        private void InitializeIds()
        {
            if (this.initializedIds)
            {
                return;
            }

            GitProcess git = new GitProcess(this);


            GitProcess.Result configResult = git.GetFromLocalConfig(
                                                GVFSConstants.GitConfig.EnlistmentId);
            if (!configResult.HasErrors)
            {
                this.enlistmentId = configResult.Output.Trim();
            }

            configResult = git.GetFromLocalConfig(GVFSConstants.GitConfig.MountId);
            if (!configResult.HasErrors)
            {
                this.mountId = configResult.Output.Trim();
            }

            this.initializedIds = true;
        }

[kewillford]

Yeah I agree we need this logic in one place. We also should also use the RepoMetadata to get the EnlistmentId since this is where it is stored to begin with. We could even try both places. We should probably also detect when there is not an entry and create a new id if there is not one. Not sure if this would only be if the call succeeded but there wasn't a value or for an error condition as well. If there was an error and we change the EnlistmentId reporting on the enlistment would not be right but we at least have an EnlistmentId. There is issue #398 that is for this work.

[jeschu1]

@derrickstolee @kewillford

Updated to put the read/config logic in one place.

I think there is an eventual bigger ask to put enlistment/mount on more verbs and this makes it easy if they already have the enlistment. For simplicity I'm leaving it as the config lookup which matches what we do today on InProcessMountVerb.

[derrickstolee]

LGTM. Let's worry about adding more consumers of EnlistmentId/MountId to future PRs.

[sanoursa]

I don't think it's a good idea to have a property do things as expensive as shelling out to a process.

Is there a good reason to lazy-initialize these values? Can we just initialize them up front and get rid of this extra complexity?

[jeschu1]

@sanoursa GVFSEnlistment doesn't necessarily need to know the values of EnlistmentId/MountId.

If we're okay with doing the config lookup everytime GVFSEnlistment is created, I'm certainly fine to move the lookup to the constructor and remove the lazy-init. That said, I think it would be a great goal to start logging with EnlistmentId/MountIds when available.

[sanoursa]

Do we have code paths where we create an enlistment and don't log its id?

[jeschu1]

@kewillford please do correct me if I'm wrong

GVFSVerb.ForExistingEnlistment will create a GVFSEnlistment, but I"m not seeing any of the verbs that extend it use enlistmentId. Ex Dehydrate/Diagnose/Status/Prefetch. I do think it would be great if we have the value there though!

MountId gets a little bit more tricky. We don't need it for Repair for instance, but probably not a killer if we look up another config setting. However, MountId can change from it's previous config value while we run the mount process. I think making this one into a function instead of a property and looking it up on-demand is probably safest. It should at most be called once and that would match the current functionality.

[derrickstolee]

The laziness was my idea. Basically, don't add work where it's not needed! If we make this config check on each constructor, then we are calling two processes at every spot we are creating an enlistment object. This could have a noticeable impact on commands like gvfs status.

A property is just syntactic sugar over a method, but if that's your issue we can replace it with GetEnlistmentId() and GetMountId().

[kewillford]

In most cases we only have one Enlistment object that gets created and what if at some point we want gvfs status to output the EnlistmentId and MountId so we can ask a user for them and run queries against them?

[jeschu1]

@kewillford that's for the clarification here! I've updated it to be a lookup on demand which is hopefully pretty straightforward. Since it's on Enlistment it's a pretty central place to get at EnlistmentId/MountId.

My understanding (and @derrickstolee feel free to correct me) is that we want status to be as fast as possible so a config lookup wouldn't be in the cards.

[derrickstolee]

I think gvfs status is ok to slow down a bit if we actually show the information. Kevin knows this area the best, so I defer to his opinions.

[kewillford]

I think we can defer initializing in the constructor until we need them initialized in more cases.

[jeschu1]

I really do appreciate the conversation @kewillford / @derrickstolee ! At a minimum now we do have an easy way to grab enlistment/mountIds.

I will look at adding them to more verbs when we have the need.

[kewillford]

These methods should probably start with Get

[kewillford]

This opens an interesting question about how we are using the GitProcess in other places since in most of those places we pass the Enlistment it might make sense to just grab this one that is in the enlistment object. But that would be for a future PR.