On 06/18/2016 12:05 AM, Lars Schneider wrote: > >> On 17 Jun 2016, at 05:20, Junio C Hamano <gitster@xxxxxxxxx> 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html