Re: [PATCH v5 07/15] git-p4: add new support function gitConfigSet()

[Junio C Hamano <gitster@pobox.com>]

"Ben Keene via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Ben Keene <seraphire@gmail.com>
>
> Add a new method gitConfigSet(). This method will set a value in the git
> configuration cache list.
>
> Signed-off-by: Ben Keene <seraphire@gmail.com>
> ---
>  git-p4.py | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/git-p4.py b/git-p4.py
> index e7c24817ad..e020958083 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -860,6 +860,11 @@ def gitConfigList(key):
>              _gitConfig[key] = []
>      return _gitConfig[key]
>  
> +def gitConfigSet(key, value):
> +    """ Set the git configuration key 'key' to 'value' for this session
> +    """
> +    _gitConfig[key] = value
> +
>  def p4BranchesInGit(branchesAreInRemotes=True):
>      """Find all the branches whose names start with "p4/", looking
>         in remotes or heads as specified by the argument.  Return

I am not sure if we want to do this.  The function makes it look as
if we are not just updating the cached version but also is updating
the underlying configuration file, effective even for future use,
but that is not what is happening (and you do not want to touch the
configuration file with this helper anyway).  It is misleading.

This seems to be used only in one place in a later patch (14/15)
 
         depotPaths = args
 
+        # If we have an encoding provided, ignore what may already exist
+        # in the registry. This will ensure we show the displayed values
+        # using the correct encoding.
+        if self.setPathEncoding:
+            gitConfigSet("git-p4.pathEncoding", self.setPathEncoding)
+
+        # If more than 1 path element is supplied, the last element
+        # is the clone destination.
         if not self.cloneDestination and len(depotPaths) > 1:
             self.cloneDestination = depotPaths[-1]
             depotPaths = depotPaths[:-1]

and the reason why it is needed, I am guessing, is because pieces of
code that gets the control later in the flow will use "git-p4.pathEncoding"
configuration variable to determine how the path need to be encoded.

I think the right fix for that kind of problem is to make sure that
we clearly separate (1) what the configured value is, (2) what the
value used to override the configured value for this single shot
invocation is, and (3) which value is used.  Perhaps the existing
code is fuzzy about the distinction and without allowing the caller
to override, always uses the configured value, in which  case that
is what needs to be fixed, perhaps?

I see encodeWithUTF8(self, path) method of P4Sync class (I am
working this review on the version in 'master', not with any of the
previous steps in this series) unconditionally uses gitConfig() to
grab the configured value.  Probably the logical thing to do there
(i.e. before your step 05/15) would have been to store in the P4Sync
instance (i.e. 'self') to hold its preferred path encoding in a new
field (say 'self.pathEncoding'), and use that if exists before
consulting the config and finally fall back to utf-8.  Or perhaps
when the class is instantiated, populate that configured value to
the 'self.pathEncoding' field, and then override it by whatever
codepath you call gitConfigSet() in this series to instead override
that field.  That way, encodeWithUTF8(self, path) method (by the
way, that is a horrible name, as the function can use arbitrary
encoding and not just UTF-8) can always encode in self.pathEncoding
which would make the logic flow simpler, I would imagine.