git-prompt.sh: remove duplicate space when $MSYSTEM not set

[DRSDavidSoft]

Signed-off-by: David Refoua [email protected]

This PR removes a duplicated (double space) in the first line of the PS1 prompt when the $MSYSTEM variable is not set.

This is mainly the result of the bash login profile not being called when bash is executed, which results in the variable NOT being set. Here's an example:

As you see, there are two spaces instead of one. This PR replaces the older (now closed) #472.

Alternatively, something like this one liner can also be used, as suggested before:

PS1="$PS1${MSYSTEM:+\[\033[35m\]$MSYSTEM }"  # show MSYSTEM in purple (if set)

Please consider adding this PR as we use GfW in the Cmder project, and the invocation used may not always load the profile, hence the missing MSYSTEM variable.

[rimrul]

To quote the old PR:

While I suspect that the use case is broken (because MSYSTEM should always be set), I am open to getting convinced that it makes sense.

But a terse "remove annoying space when MSYSTEM not set" does not even begin to convince anyone.

In other words, this change needs a substantially improved commit message, one in accordance to https://github.blog/2022-06-30-write-better-commits-build-better-projects/.

None of this has been addressed.

Alternatively, something like this one liner can also be used:

PS1="$PS1${MSYSTEM:+\[\033[35m\]$MSYSTEM }"  # show MSYSTEM in purple

Yes, that's what Johannes also suggested, in the same review comment on the old PR.

[dscho]

MSYSTEM should always be set

This is as true as it has been then. Why is it not set in your setup?

Are you maybe using an unsupported scenario (but even there, MSYSTEM should always be set, or you're holding it wrong)?

[DRSDavidSoft]

@rimrul and @dscho

Thanks for taking time reviewing this case.

1. Regarding commit message

In other words, this change needs a substantially improved commit message, one in accordance to github.blog/2022-06-30-write-better-commits-build-better-projects.

I'll change the commit message to attempt to comply with the instructed link above. Hopefully, the new commit message is acceptable.

2. Regarding scripting syntax

PS1="$PS1${MSYSTEM:+\[\033[35m\]$MSYSTEM }"  # show MSYSTEM in purple (if set)

Yes, that's what Johannes also suggested, in the same review comment on the old PR.

Sure, if using a one liner is prefered, I'll change the commit to reflect this as well. Which one should I keep?

3. Regarding the actual need for this change

I suspect that the use case is broken (because MSYSTEM should always be set) [...] Why is it not set in your setup?

As I've explained in the old PR, the root cause is as follows:

[...] that executing bash.exe --login will result in scripts in the /etc/profile.d being sourced -- thus MSYSTEM being set, while executing bash.exe is not sourcing the scripts which will result on MSYSTEM not being set.

As for your other important observation:

Are you maybe using an unsupported scenario

This problem has existed for as long as I can remember (assuming it's reproduced using the example I've provided above). When I opened the previous PR, I was using the official installation, without any modifications.

Now, I'm in fact using the unsupported scenario as it's described, but I don't believe it's relevant to this issue. To make sure, I'll install a fresh version to ensure that this is true.

---

Once done, I'll explain the problem in more detail and hopefully you'll be able to repro it. Additionally, please decide whether to keep the current implementation, or change it to the one-liner one. I'll also update the commit message to comply with the instructed link above.

Once again, thank you for taking time to consider this issue. I'll post an update with the fresh installation and repro steps soon.

[dscho]

I suspect that the use case is broken (because MSYSTEM should always be set) [...] Why is it not set in your setup?

As I've explained in the old PR, the root cause is as follows:

[...] that executing bash.exe --login will result in scripts in the /etc/profile.d being sourced -- thus MSYSTEM being set, while executing bash.exe is not sourcing the scripts which will result on MSYSTEM not being set.

To reiterate, whatever your entry point into Git for Windows, whether that be C:\Program Files\Git\cmd\git.exe or C:\Program Files\Git\bin\git.exe or even C:\Program Files\Git\bin\bash.exe, MSYSTEM will be set. It is a pretty fundamental thing.

If you try to use Git for Windows as a kind of distribution of Bash, that's not really what this project is all about.
The purpose of Git for Windows is really to bring Git to Windows.

Having said that, what you really are looking for is MSYS2. Git for Windows leverages MSYS2 and ships with a subset of its files. MSYS2 even sports a package management system called "pacman" to install more tools (including Git...), so why don't you give it a whirl?

[DRSDavidSoft]

Repro steps

1. Installed Git-2.49.0-64-bit (the latest installer) using default options during install
2. Lookup bash.exe as follows:

Here are the results for each:

\git-bash.exe

✅ No double space issue.

Reason: The git-bash.exe is used to execute a full bash session. It bypasses the current terminal and launches a new MinTTY window, with the bash and the associated profile, meaning the variable is set:

\bin\bash.exe

✅ No double space issue.

Reason: The \bin\bash.exe is NOT the actual bash shell, it's just a simple wrapper that is used to execute the actual bash shell, while also loading the profiles needed that eventually also set the required variable.

Here's the source: https://github.com/git-for-windows/MINGW-packages/blob/main/mingw-w64-git/git-wrapper.c

This is probably the intended, supported use case to launch bash from Git for Windows.

\usr\bin\bash.exe

❌ The double space issue!

Reason: This is the actual bash shell executable that is provided by the Git for Windows package. When it is directly launched (without any arguments), the profile scripts are not being sourced, hence the missing variable.

For reference, here's where the variable is set:

https://github.com/msys2/MSYS2-packages/blob/master/filesystem/msystem

This is the profile file that sources it:

https://github.com/msys2/MSYS2-packages/blob/9b8ed03b1e116257d17736c27de59a125707559d/filesystem/profile#L48

Now let's try passing the --login argument and see the variable being loaded:

Now, the /etc/profile (provided by MSYS2 is being loaded. It sources the msystem file, which in turn sets the variable).

Please note that:

1. Executing bash, without necessarily loading the profile scripts is a completely valid case.
2. Even though Git for Windows is providing the "wrapper" binary, which loads the profile, we're actually looking for the actual bash executable.

I believe the problem is that git-prompt.sh, that is modifying the PS1 prompt, should not always assume that the profile script is loaded. If you look at the profile script, it isn't always necessary to load it.

I hope this repro will satisfy/convince you as originally requested.

In my next update, I'll look into how the original MSYS2 bash prompt works. I assume this prompt issue doesn't exist in the un-modified MSYS2 package, and instead is something in the Git for Windows that needs to be corrected.

[dscho]

\usr\bin\bash.exe

❌ The double space issue!

Do note that this was not in the list of supported entry points I provided. You are asking for an undue maintenance burden here by insisting to not run the Bash profile. The Git for Windows project does not enjoy a large number of people helping with maintenance, therefore I cannot, and do not want to, fulfill your wish.

[dscho]

I will also have to note that what you illustrated is a use case where you want an interactive Bash on Windows. This here project isn't called "Bash on Windows", though. It's called "Git for Windows", for a good reason, as I pointed out earlier.

[DRSDavidSoft]

Having said that, what you really are looking for is MSYS2. Git for Windows leverages MSYS2 and ships with a subset of its files. MSYS2 even sports a package management system called "pacman" to install more tools (including Git...), so why don't you give it a whirl?

Well, that's how the Cmder project has always been structured, as the MSYS2 package is larger in size, and I believe it lacks Git support.

https://github.com/cmderdev/cmder

I believe dropping GfW to replace it with unmodified MSYS2 to solve minor issues such as this may not be the best approach to do.

To reiterate, whatever your entry point into Git for Windows, whether that be C:\Program Files\Git\cmd\git.exe or C:\Program Files\Git\bin\git.exe or even C:\Program Files\Git\bin\bash.exe, MSYSTEM will be set. It is a pretty fundamental thing.

Hopefully #625 (comment) will show how this is not true. Please note that I used Windows sandbox (a clean VM) to try this out; I believe you'll also see the same results if you install and launch the actual bash binary (located at /usr/bin) on a new, clean installation.

If you try to use Git for Windows as a kind of distribution of Bash, that's not really what this project is all about.
The purpose of Git for Windows is really to bring Git to Windows.

I understand, and I agree. You are completely correct, however, I'm hoping that GfW could also be open to be useful for projects and use cases like this.

Git bash is a useful and acceptable utility to be used for these projects. Additionally, it seems acceptable to use it in the way that I've outlined above.

I'm not asking to support this entry, I'm just asking to consider minor issues that can be benefited from PRs such as this one.

[DRSDavidSoft]

You are asking for an undue maintenance burden here by insisting to not run the Bash profile.

I hope that this isn't too much of a burdon; for reference we've been using the bash.exe that is provided by GfW for a long time now, and so far no body has had any major/minor problems with the way it invokes.

The git-prompt.sh file is just a simple prompt script that assumes that the profile is loaded, however if you actually look into the profile script, the git prompt will just work fine. If the MSYSTEM variable is not set, the only side effect is that the prompt will not display the msystem in purple, and two spaces are shown instead of one. This PR is intended to just address this simple issue.

The Git for Windows project does not enjoy a large number of people helping with maintenance

Hopefully I can be more useful and help with the maintenance in those regards. We've been enjoying the GfW package in the Cmder project, and as a long time maintainer, it's only fair to contribute back where possible.

I cannot, and do not want to, fulfill your wish

In any case, thanks for the time to consider this. Please note, I'm against asking to support a new un-supported entry for bash, or to expect a "Bash for Windows" feature being provided by GfW. I understand that you guys are already time and effort constrained, and this isn't the point of the PR to begin with. I would just like to ask for a minor change in the git-prompt.sh, and hopefully, I assume that a change such as this wouldn't involve future maintenance burdons (of course, if I'm mistaken, I would appreciate to learn more).

I started this PR because I assume the "Git bash" feature that GfW provides is a valid use case, and I would prefer to address issues such as this one at the upstream project, instead of modifying it downstream.

Once again, sorry for any distresses caused, and thank you for considering this case.

[DRSDavidSoft]

I'm hoping that by applying patches such as this, we would be able to stop relying on downstream patches that are intended to address minor cosmetic issues such as this one.

https://github.com/cmderdev/cmder/blame/93785426a6a94bb6ef1fbda25862104a08026f95/vendor/git-prompt.sh#L45-L46

Update: Since this is now closed, the accepted alternative is to patch the git-prompt.sh in the downstream project.