On Thu, 14 Nov 2019 at 02:36, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> "Ben Keene via GitGitGadget" <gitgitgadget@xxxxxxxxx> 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.
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.
