Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)

From: Lars Schneider <larsxschneider@gmail.com>
Date: Jun 19, 2016, 16:11 UTC
Message-ID: <BC5163B8-7CF2-4106-9FBE-8CCD096252A2@gmail.com>
In-Reply-To: <3E132EEE-CEBA-4C0D-A5C4-62F587A02225@gmail.com>

Show 84 quoted lines

> On 19 Jun 2016, at 17:04, Lars Schneider <larsxschneider@gmail.com> wrote:
> 
> 
>> On 19 Jun 2016, at 09:59, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> 
>> On 06/18/2016 12:05 AM, Lars Schneider wrote:
>>> 
>>>> On 17 Jun 2016, at 05:20, Junio C Hamano <gitster@pobox.com> wrote:
>>>> 
>>>> ...
>>>> 
>>>> * mh/split-under-lock (2016-05-13) 33 commits
>>>> ...
>>>> 
>>>> Further preparatory work on the refs API before the pluggable
>>>> backend series can land.
>>>> 
>>>> Will merge to 'master'.
>>> 
>>> This topic seems break two git-p4 tests (t9801 and t9803) on next:
>>> https://travis-ci.org/git/git/jobs/137333785
>>> 
>>> According to git bisect the commit "ref_transaction_update(): 
>>> check refname_is_safe() at a minimum" (3da1f3) introduces the problem: 
>>> https://s3.amazonaws.com/archive.travis-ci.org/jobs/138457628/log.txt
>>> (scroll all the way down to see the bisecting)
>>> 
>>> - Lars
>>> 
>> 
>> Lars,
>> 
>> According to [1], something in that test seems to have been trying to run
>> 
>>   git update-ref -d git-p4-tmp/6
>> 
>> Similarly in the other failed test.
>> 
>> Because `update-ref` doesn't do DWIM for reference names, this is *not*
>> expanded to `refs/heads/git-p4-tmp/6` or something. Previously this
>> command would have quietly failed to do anything. But after
>> "ref_transaction_update(): check refname_is_safe() at a minimum", `git
>> update-ref` notices that `git/p4/tmp/6` is not a safe refname (according
>> to `refname_is_safe()` [2]), and correctly fails with an error message.
> 
> All errors seem to be related to the Git-P4 branch import. I am no expert
> in that area because the branch import never worked for me (and I am puzzled
> to some extend how it is supposed to work given the differences how branches
> work in Git and P4).
> 
> This is the offending call:
> https://github.com/git/git/blob/05219a1276341e72d8082d76b7f5ed394b7437a4/git-p4.py#L3464
> 
> This is only a cleanup call and we could make all tests work if we remove the
> cleanup and also the "cleanup successful check":
> https://github.com/git/git/blob/05219a1276341e72d8082d76b7f5ed394b7437a4/t/t9801-git-p4-branch.sh#L303
> https://github.com/git/git/blob/05219a1276341e72d8082d76b7f5ed394b7437a4/t/t9801-git-p4-branch.sh#L355
> 
> I am a bit surprised that we do not see other errors given the fact 
> that the branch name is clearly invalid:
> https://github.com/git/git/blob/05219a1276341e72d8082d76b7f5ed394b7437a4/t/t9803-git-p4-shell-metachars.sh#L102
> 
> I see two ways to proceed:
> 
> (1) We remove the cleanup.
> 
> (2) We sanitize the branch names (e.g. by removing invalid characters).
> @Michael: Is there a function to "sanitize" a given branch name already?
> 
> Option 1 is trivial and option 2 (my preference) shouldn't be too hard. 
> But maybe Luke has some insights since he added the "branch with shell char" 
> test in 52a4880.
> 
> 
>> Even before this change, Git didn't allow such references to be created
>> or updated. So I think this test failure is revealing an error in `git
>> p4 clone` that went undetected before this change.
>> 
>> Please let me know whether you agree. If so, it is realistic to fix
>> `git-p4` promptly? This failure is currently blocking
>> mh/split-under-lock, so if `git-p4` can't be fixed, then I'd have to
>> either disable t9801 and t9803 in this patch series, or omit the
>> `refname_is_safe()` check.
> I am looking into option 2.

After looking more into it I realized that the character "\$" in the branch name is not even the problem. The git-p4 temp refs are just not located under refs/heads.

This seems to fix the issue:

--- a/git-p4.py
+++ b/git-p4.py
@@ -2274,7 +2274,7 @@ class P4Sync(Command, P4UserMap):
         self.useClientSpec_from_options = False
         self.clientSpecDirs = None
         self.tempBranches = []
-        self.tempBranchLocation = "git-p4-tmp"
+        self.tempBranchLocation = "refs/heads/git-p4-tmp"
         self.largeFileSystem = None
 
         if gitConfig('git-p4.largeFileSystem'):

-- 
@Luke: Would that be an acceptable solution?

Thanks,
Lars


> 
>> 
>> In the interest of backwards compatibility, I considered making `git
>> update-ref -d` continue to fail silently for NOOP operations with unsafe
>> refnames (one of the requirements being that no old_oid is specified).
>> But I think that would be giving the wrong signal to scripts that are
>> doing something that is invalid but pausible, like trying to delete the
>> reference `../$(basename $PWD)/refs/heads/foo`. Such scripts would be
>> misled into thinking the deletion was successful. And yet treating
>> plausibly-sensible requests differently than obviously bogus requests
>> seems like a path to madness.
> Agreed!
> 
> Cheers,
> Lars