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

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Jun 19, 2016, 07:59 UTC
Message-ID: <576650E7.70107@alum.mit.edu>
In-Reply-To: <1634E84E-5260-4F7B-A74F-AF5D3A7C0181@gmail.com>

On 06/18/2016 12:05 AM, Lars Schneider wrote:

Show 57 quoted lines

> 
>> On 17 Jun 2016, at 05:20, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> ...
>>
>> * mh/split-under-lock (2016-05-13) 33 commits
>>  (merged to 'next' on 2016-06-03 at 2e71330)
>> + lock_ref_sha1_basic(): only handle REF_NODEREF mode
>> + commit_ref_update(): remove the flags parameter
>> + lock_ref_for_update(): don't resolve symrefs
>> + lock_ref_for_update(): don't re-read non-symbolic references
>> + refs: resolve symbolic refs first
>> + ref_transaction_update(): check refname_is_safe() at a minimum
>> + unlock_ref(): move definition higher in the file
>> + lock_ref_for_update(): new function
>> + add_update(): initialize the whole ref_update
>> + verify_refname_available(): adjust constness in declaration
>> + refs: don't dereference on rename
>> + refs: allow log-only updates
>> + delete_branches(): use resolve_refdup()
>> + ref_transaction_commit(): correctly report close_ref() failure
>> + ref_transaction_create(): disallow recursive pruning
>> + refs: make error messages more consistent
>> + lock_ref_sha1_basic(): remove unneeded local variable
>> + read_raw_ref(): move docstring to header file
>> + read_raw_ref(): improve docstring
>> + read_raw_ref(): rename symref argument to referent
>> + read_raw_ref(): clear *type at start of function
>> + read_raw_ref(): rename flags argument to type
>> + ref_transaction_commit(): remove local variable n
>> + rename_ref(): remove unneeded local variable
>> + commit_ref_update(): write error message to *err, not stderr
>> + refname_is_safe(): insist that the refname already be normalized
>> + refname_is_safe(): don't allow the empty string
>> + refname_is_safe(): use skip_prefix()
>> + remove_dir_recursively(): add docstring
>> + safe_create_leading_directories(): improve docstring
>> + read_raw_ref(): don't get confused by an empty directory
>> + commit_ref(): if there is an empty dir in the way, delete it
>> + t1404: demonstrate a bug resolving references
>> (this branch is used by mh/ref-iterators, mh/ref-store and mh/update-ref-errors.)
>>
>> 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.

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.

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.

Michael

[1] https://travis-ci.org/git/git/jobs/137333785#L2025-L2026 [2] https://github.com/mhagger/git/blob/7a418f3a17b95746eb94cfd55f4fe0385d058777/refs.c#L121-L151