git update-git-for-windows: escape installer arguments

[ge0rdi]

When installer is being executed MSYS2 subsystem tries to convert unix paths to Windows paths.

It seems it applies to installer arguments as well (because they start with slash).
Thus /SILENT is converted to C:/Program Files/Git/SILENT (or something alike).

This effectively bypasses installer arguments.

The solution is to escape slashes in installer arguments.

Signed-off-by: ge0rdi [email protected]

---

With this change git update works silently without any user interaction and any progress.
That second part (no progress displayed) is caused by /VERYSILENT option and imo it is not good. Because user doesn't even know if the installation started/ended.

Personally I think /VERYSILENT should be removed. Without that option it seems to work well for me. There is still no user interaction (tried update from 2.28 to latest one) and there is at least installer window with progress shown.

[dscho]

With this change git update works silently without any user interaction and any progress.

Taking a step back, is this even a good user experience? Maybe we should drop the /SILENT option, too?

[dscho]

Oh, and please note that the "Signed-off-by:" line does not make sense unless you use your real name and a working email address: the idea is that you can be reached in case of questions (originally only legal ones, but I find it useful to be able to ping an author about a commit I find in the commit history that I have questions about).

[ge0rdi]

Taking a step back, is this even a good user experience? Maybe we should drop the /SILENT option, too?

Hard to tell.
For me it makes sense to do upgrade that doesn't require user interaction.
Because when I'm running git update-git-for-windows I just want to update git without going through the whole installer.

Though maybe you can add this as option for update-git-for-windows?
So user will decide whether he wants to tweak installation during update or not.

Oh, and please note that the "Signed-off-by:" line does not make sense unless you use your real name and a working email address

Ah, sorry. I don't use my real name on github (kind of old-school ;) ).

Will providing working e-mail address be enough?

[ge0rdi]

There is one thing I forgot to mention.
I've noticed this weird installer arguments (actually Windows paths) when I was trying to investigate git update failures when running as non-admin user.

I wrote my findings to similar existing issue:
git-for-windows/git#2295 (comment)

Maybe you could have a look at that too?
Or should I rather create separate issue for it?

[dscho]

when I'm running git update-git-for-windows I just want to update git without going through the whole installer.

Right...

What about the thing when you click on the notification (which should show up when you check the "check daily for updates" option)? it probably should just show the progress, too.

please note that the "Signed-off-by:" line does not make sense unless you use your real name and a working email address

Ah, sorry. I don't use my real name on github (kind of old-school ;) ).

Why not simply dropping that line, then? We do not really require it in the build-extra repository, it is more a habit from the git repository (because we want to upstream those patches eventually, and the upstream Git project does require a DCO).

I've noticed this weird installer arguments (actually Windows paths

Could you elaborate?

I wrote my findings to similar existing issue:
git-for-windows/git#2295 (comment)

Maybe you could have a look at that too?
Or should I rather create separate issue for it?

If it is a lot easier, you could add it to the current PR. I am not certain about UsePreviousPrivileges, to be honest, therefore I would appreciate a good discussion and rationale in the commit message.

[ge0rdi]

What about the thing when you click on the notification (which should show up when you check the "check daily for updates" option)? it probably should just show the progress, too.

I'd say it should display the progress too.
Actually it should also show some progress when installer is being downloaded. Because it is quite weird that one clicks to Yes button in toast notification and then nothing seems to happen. After some time git installer pops up.

Why not simply dropping that line, then? We do not really require it in the build-extra repository, it is more a habit from the git repository (because we want to upstream those patches eventually, and the upstream Git project does require a DCO).

Done.

Could you elaborate?

Those weird paths in installer command line are result of not escaped installer arguments (thing that this PR is fixing).

If it is a lot easier, you could add it to the current PR. I am not certain about UsePreviousPrivileges, to be honest, therefore I would appreciate a good discussion and rationale in the commit message.

Actually I'm not even sure the option will help. I don't have whole installer environment set up (at the moment) so I cannot test it for myself.
I just though this hint may help.

[rimrul]

Actually it should also show some progress when installer is being downloaded. Because it is quite weird that one clicks to Yes button in toast notification and then nothing seems to happen. After some time git installer pops up.

I am working on that. See also git-for-windows/git#2395.

[dscho]

See also git-for-windows/git#2395.

That ticket has stalled, it seems, the reporter is unresponsive for over a year.

Maybe we can use this here PR as an excuse to revive the effort?

@rimrul what do you think, would it be okay for @ge0rdi to include a commit dropping /VERYSILENT, or would that make your work harder?

Also, I think that it might be best to leave the UsePreviousPrivileges to a future PR. That way, we could finalize this PR quicker, I think.

Thoughts?

[ge0rdi]

include a commit dropping /VERYSILENT

I have no problem to do it once you guys will agree on that.
Just note that this switch was added for some reason.
This is from commit message that added the switch:

The command successfully identifies and downloads the update however when it
triggers the update it causes a license screen to display which requires
user interaction. Therefore the update can't be fully automated.

When I was testing it (removed /VERYSILENT) there was no such license screen displayed and there was no user interaction required.
Though maybe there are some cases where license screen is always displayed? Not sure how this works in Inno.

Also, I think that it might be best to leave the UsePreviousPrivileges to a future PR. That way, we could finalize this PR quicker, I think.

Yup, definitnely.

I've just mentioned it to make you aware of the issue with upgrade done by non-admin user. But it is definitely not related to current PR.
Sorry, had to be more clear about it.

[dscho]

Thanks for the clarification, @ge0rdi !

Your reasoning about the license page makes a lot of sense. Could you include it in the commit removing /VERYSILENT?

[rimrul]

@rimrul what do you think, would it be okay for @ge0rdi to include a commit dropping /VERYSILENT, or would that make your work harder?

Fine by me.

[ge0rdi]

Could you include it in the commit removing /VERYSILENT?

Done.
Please, have a look if it makes sense.