Fix problem with incorrect building command line arguments

[azarkevich]

Situation:

Branch name contains word push (for example pr/fix-cmdline-in-retry-push-with-force)
and remote reject push due to non-fast-forward
and user select retry with force

then word push in branch name was replaced with 'push --force-with-lease' by .Replace("push", "push --force-with-lease")

[amaiorano]

@jbialobr is encouraging us to review each other's changes so if you don't mind, I'll review yours. Perhaps you can take a look at my change as well.

[amaiorano]

I don't understand why you're checking if the args ends with -f here. Even if it were necessary, wouldn't you want to also check if it's --force?

[azarkevich]

It is not actual anymore, but answer is: I add -f to end of arguments, so check .Contains(" -f ") will not work because of trailing space.

So this condition should cover all possible cases:

push -f origin
push --force origin
push --force-with-lease origin

push origin ... -f
push origin ... --force
push origin ... --force-with-lease

[amaiorano]

I don't think appending to the end of the args is the right thing to do. At this point, args will look something like:

push --recurse-submodules=check --progress "origin" refs/heads/test-force-push:refs/heads/test-force-push

We usually want to avoid adding args after we specify the remote and name of branch. I'm surprised this works.

I tried something locally, and this seems to work:

                if (forcePush)
                {
                    if (!form.ProcessArguments.Contains(" -f ") && !form.ProcessArguments.Contains(" --force"))
                    {
                        string forceArg = GitCommandHelpers.VersionInUse.SupportPushForceWithLease? " --force-with-lease" : " -f";
                        form.ProcessArguments = form.ProcessArguments.Insert(form.ProcessArguments.IndexOf("push") + "push".Length, forceArg);
                    }

Or if we can assume "force" is always the first value in the args list, it can be simplified to:

                if (forcePush)
                {
                    if (!form.ProcessArguments.Contains(" -f ") && !form.ProcessArguments.Contains(" --force"))
                    {
                        string forceArg = GitCommandHelpers.VersionInUse.SupportPushForceWithLease? " --force-with-lease" : " -f";
                        form.ProcessArguments = form.ProcessArguments.Insert("push".Length, forceArg);
                    }

[azarkevich]

@amaiorano, I update fix to your variant. I made it slightly more strict.

[amaiorano]

This is better, but a few things:

• Missing a space between if and the open parenthesis.
• No need to check with ignore case for "push", Git is case sensitive so if it were anything but "push" it wouldn't work.
• In the few places in this code base where Debug.Assert is used, System.Diagnostics is imported at the top.
• I'm not a fan of Debug.Assert(false, ...) as we don't see the condition for the failure. In this case, it is extremely unlikely that "push" will be missing, so I would rather we just assert that it starts with "push", then just assume it, so like:

if (!form.ProcessArguments.Contains(" -f ") && !form.ProcessArguments.Contains(" --force"))
{
    Debug.Assert(form.ProcessArguments.StartsWith("push"), "Arguments should start with 'push' command");
    string forceArg = GitCommandHelpers.VersionInUse.SupportPushForceWithLease ? " --force-with-lease" : " -f";
    form.ProcessArguments = form.ProcessArguments.Insert("push".Length, forceArg);
}

I personally prefer this over having the "safe code" with the else asserting false, but perhaps we should ask @jbialobr to weigh in on this.

[jbialobr]

I personally prefer this over having the "safe code" with the else asserting false, but perhaps we should ask @jbialobr to weigh in on this.

First of all, I want to thank you for reviewing PRs. This is a community project and I don't want to force my coding preferences. Let's wait for @azarkevich to see his opinion (pros and cons). I will have to decide only If you won't find an agreement.

[azarkevich]

Agree with your proposals and remarks. Pushed new version.
Though I use Trace.Assert instead of Debug.Assert, so this condition will be verified even in Release.

[amaiorano]

This looks good to me!