From: Junio C Hamano <hidden> Date: 2019-11-14 02:36:40
"Ben Keene via GitGitGadget" [off-list ref] writes:
Issue: Using git-p4.py on Windows does not resolve properly to the p4.exe
binary in all instances.
Two new code features are added to resolve the p4 executable location:
1. A new variable, git-p4.binary, has been added that takes precedence over
the default p4 executable name. If this git option is set and the
path.exists() passes for this file it will be used as executable for the
system.popen calls.
2. If the new variable git-p4.binary is not set, the program checks if the
operating system is Windows. If it is, the executable is changed to
'p4.exe'. All other operating systems
(those that do not report 'Windows' in the platform.system() call)
continue to use the current executable of 'p4'.
I do not use Windows nor git-p4, but the above two changes make
sense to me at the design level. One thing that needs to be updated
is the configuration variable, though. It is more in line with the
other parts of the system to name this "git-p4.program", because
binary-ness does not matter much to you, as long as the named thing
works correctly as "p4" program replacement.
Seeing "gpg.program" is used in a similar way, it also is tempting
to call it "p4.program", but no other parts of Git other than
"git-p4" uses P4, and the "git-p4." prefix is to collect variables
related to "git-p4" together, so calling it "p4.program" may not be
a good idea---it would hurt discoverability. "git-p4.p4program"
may be OK, if we anticipate that git-p4 may need to use (and its
users need to specify paths to) external programs other than "p4",
but it probably is overkill.
Thanks.
From: Luke Diamand <hidden> Date: 2019-11-14 09:53:02
On Thu, 14 Nov 2019 at 02:36, Junio C Hamano [off-list ref] wrote:
"Ben Keene via GitGitGadget" [off-list ref] writes:
quoted
Issue: Using git-p4.py on Windows does not resolve properly to the p4.exe
binary in all instances.
Two new code features are added to resolve the p4 executable location:
1. A new variable, git-p4.binary, has been added that takes precedence over
the default p4 executable name. If this git option is set and the
path.exists() passes for this file it will be used as executable for the
system.popen calls.
2. If the new variable git-p4.binary is not set, the program checks if the
operating system is Windows. If it is, the executable is changed to
'p4.exe'. All other operating systems
(those that do not report 'Windows' in the platform.system() call)
continue to use the current executable of 'p4'.
I do not use Windows nor git-p4, but the above two changes make
sense to me at the design level. One thing that needs to be updated
is the configuration variable, though. It is more in line with the
other parts of the system to name this "git-p4.program", because
binary-ness does not matter much to you, as long as the named thing
works correctly as "p4" program replacement.
Seeing "gpg.program" is used in a similar way, it also is tempting
to call it "p4.program", but no other parts of Git other than
"git-p4" uses P4, and the "git-p4." prefix is to collect variables
related to "git-p4" together, so calling it "p4.program" may not be
a good idea---it would hurt discoverability. "git-p4.p4program"
may be OK, if we anticipate that git-p4 may need to use (and its
users need to specify paths to) external programs other than "p4",
but it probably is overkill.
Thanks.
There's some trailing whitespace in 98bae, can we get that tidied up?
Otherwise, modulo Junio's comments, it looks good.
I agree that "git-p4.binary" might be harder to discover ("Oh, I
assumed that enabled binary mode!"). p4program should be fine.
From: Ben Keene <hidden> Date: 2019-11-14 20:17:03
On Thu, Nov 14, 2019 at 4:52 AM Luke Diamand [off-list ref] wrote:
On Thu, 14 Nov 2019 at 02:36, Junio C Hamano [off-list ref] wrote:
quoted
"Ben Keene via GitGitGadget" [off-list ref] writes:
quoted
Issue: Using git-p4.py on Windows does not resolve properly to the p4.exe
binary in all instances.
Two new code features are added to resolve the p4 executable location:
1. A new variable, git-p4.binary, has been added that takes precedence over
the default p4 executable name. If this git option is set and the
path.exists() passes for this file it will be used as executable for the
system.popen calls.
2. If the new variable git-p4.binary is not set, the program checks if the
operating system is Windows. If it is, the executable is changed to
'p4.exe'. All other operating systems
(those that do not report 'Windows' in the platform.system() call)
continue to use the current executable of 'p4'.
I do not use Windows nor git-p4, but the above two changes make
sense to me at the design level. One thing that needs to be updated
is the configuration variable, though. It is more in line with the
other parts of the system to name this "git-p4.program", because
binary-ness does not matter much to you, as long as the named thing
works correctly as "p4" program replacement.
Seeing "gpg.program" is used in a similar way, it also is tempting
to call it "p4.program", but no other parts of Git other than
"git-p4" uses P4, and the "git-p4." prefix is to collect variables
related to "git-p4" together, so calling it "p4.program" may not be
a good idea---it would hurt discoverability. "git-p4.p4program"
may be OK, if we anticipate that git-p4 may need to use (and its
users need to specify paths to) external programs other than "p4",
but it probably is overkill.
Thanks.
There's some trailing whitespace in 98bae, can we get that tidied up?
Otherwise, modulo Junio's comments, it looks good.
I agree that "git-p4.binary" might be harder to discover ("Oh, I
assumed that enabled binary mode!"). p4program should be fine.
I have updated the Pull Request to change the variable name from
'binary' to 'p4program'. I have also updated the PR description.
Here's the new commit message:
Issue: Using git-p4.py on Windows does not resolve properly to the
p4.exe binary in all instances.
Changes since v1:
Commit: (dc6817e) 2019-11-14
Renamed the variable "git-p4.binary" to "git-p4.p4program"
v1:
Two new code features are added to resolve the p4 executable location:
A new variable, git-p4.binary, has been added that takes precedence
over the default
p4 executable name. If this git option is set and the path.exists()
passes for this file
it will be used as executable for the system.popen calls.
If the new variable git-p4.binary is not set, the program checks if
the operating system
is Windows. If it is, the executable is changed to 'p4.exe'. All other
operating systems
(those that do not report 'Windows' in the platform.system() call)
continue to use the
current executable of 'p4'.
From: Luke Diamand <hidden> Date: 2019-11-15 09:28:12
On Thu, 14 Nov 2019 at 20:17, Ben Keene [off-list ref] wrote:
On Thu, Nov 14, 2019 at 4:52 AM Luke Diamand [off-list ref] wrote:
quoted
On Thu, 14 Nov 2019 at 02:36, Junio C Hamano [off-list ref] wrote:
quoted
"Ben Keene via GitGitGadget" [off-list ref] writes:
quoted
Issue: Using git-p4.py on Windows does not resolve properly to the p4.exe
binary in all instances.
Two new code features are added to resolve the p4 executable location:
1. A new variable, git-p4.binary, has been added that takes precedence over
the default p4 executable name. If this git option is set and the
path.exists() passes for this file it will be used as executable for the
system.popen calls.
2. If the new variable git-p4.binary is not set, the program checks if the
operating system is Windows. If it is, the executable is changed to
'p4.exe'. All other operating systems
(those that do not report 'Windows' in the platform.system() call)
continue to use the current executable of 'p4'.
I do not use Windows nor git-p4, but the above two changes make
sense to me at the design level. One thing that needs to be updated
is the configuration variable, though. It is more in line with the
other parts of the system to name this "git-p4.program", because
binary-ness does not matter much to you, as long as the named thing
works correctly as "p4" program replacement.
Seeing "gpg.program" is used in a similar way, it also is tempting
to call it "p4.program", but no other parts of Git other than
"git-p4" uses P4, and the "git-p4." prefix is to collect variables
related to "git-p4" together, so calling it "p4.program" may not be
a good idea---it would hurt discoverability. "git-p4.p4program"
may be OK, if we anticipate that git-p4 may need to use (and its
users need to specify paths to) external programs other than "p4",
but it probably is overkill.
Thanks.
There's some trailing whitespace in 98bae, can we get that tidied up?
Otherwise, modulo Junio's comments, it looks good.
I agree that "git-p4.binary" might be harder to discover ("Oh, I
assumed that enabled binary mode!"). p4program should be fine.
I have updated the Pull Request to change the variable name from
'binary' to 'p4program'. I have also updated the PR description.
I couldn't figure out how to download this. I tried p4-binary-1, but
it gave me the old one. Maybe I was just using it wrong?
Could you update the commit message to say why we needed to do this;
if we have to dig through history in the future to figure out what's
going on, it will be a lot easier if the explanation is in the commit
message, rather than in an email thread. The branch description on
gitgitgadget won't be around either.
Here's the new commit message:
Issue: Using git-p4.py on Windows does not resolve properly to the
p4.exe binary in all instances.
Changes since v1:
Commit: (dc6817e) 2019-11-14
Renamed the variable "git-p4.binary" to "git-p4.p4program"
v1:
Two new code features are added to resolve the p4 executable location:
A new variable, git-p4.binary, has been added that takes precedence
over the default
p4 executable name. If this git option is set and the path.exists()
passes for this file
it will be used as executable for the system.popen calls.
If the new variable git-p4.binary is not set, the program checks if
the operating system
is Windows. If it is, the executable is changed to 'p4.exe'. All other
operating systems
(those that do not report 'Windows' in the platform.system() call)
continue to use the
current executable of 'p4'.