Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)
- From
Lars Schneider <larsxschneider@gmail.com>
- Date
- Jun 19, 2016, 15:04 UTC
- Message-ID
- <3E132EEE-CEBA-4C0D-A5C4-62F587A02225@gmail.com>
- In-Reply-To
- <576650E7.70107@alum.mit.edu>
Show 41 quoted lines
> 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.
Show 9 quoted lines
> 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.
Show 10 quoted lines
> > 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