Re: [PATCH 0/2] Feature: New Variable git-p4.binary

[Ben Keene <seraphire@gmail.com>]

On Thu, Nov 14, 2019 at 4:52 AM Luke Diamand <luke@diamand.org> wrote:
>
> On Thu, 14 Nov 2019 at 02:36, Junio C Hamano <gitster@pobox.com> wrote:
> >
> > "Ben Keene via GitGitGadget" <gitgitgadget@gmail.com> 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.

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'.