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

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 11, 2019, 17:11 UTC
Message-ID: <xmqqsglqpz2z.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <bc7009541b3f03c3065a7be2b569cd4bf91f7c05.1575740863.git.gitgitgadget@gmail.com>

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

Show 26 quoted lines

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