Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)
Torsten Bögershausen writes: >> There's less redundant work going on than at first seems, because >> .gitattribute files are only read once and cached. Verified by strace. >> > OK, I think I missed that work (not enough time for Git at the moment) > Junio, please help me out, do we have a cache here now? The call convert_attrs() makes to the attribute subsystem is: git_check_attr(path, NUM_CONV_ATTRS, ccheck) Conceptually, this call has to do the following, and for the very first call that is actually what it does: 1. Read all the relevant attrubutes files. If you are asking for "a/x/1", we need to read $GIT_DIR/info/attributes, ".gitattributes", "a/.gitattributes" and "a/x/.gitattributes". 2. Find matching patterns that cover "a/x/1", and pick up the attribute definition from the above. If you have asked for "a/x/1", it is very likely that you would next ask for "a/x/2" (think: "git checkout a/x"), and we can realize that exactly the same set of attributes files apply to that path. So an obvious optimization is to cache the result of the first step. In addition, it is also likely that you would later ask for "a/y/3" before asking for "b/z/4" (think: "git add ."). A part of the step 1. that was done when you asked for "a/x/1" and then was reused when you asked for "a/x/2" can further be reused for this request, by discarding only what was read from "a/x/.gitattributes" and reading only from "a/y/.gitattributes". The above two optimizations are done in prepare_attr_stack(). Unfortunately this is one of the three reasons why the attribute subsystem is not thread-ready. I.e. there is only one global cache, so if you spawn two threads to speed up "git add ." by handing paths [a-m]* to one and [n-z]* to the other, they would thrash the cache and making it ineffective (even if we protect the cache with mutex, which obviously has not been done). I earlier started looking into this, but the effort stalled. But for a single-thread use, the attributes read from the filesystem are cached, and the cache is designed to perform well as long as you make requests in-order. To make the attribute look-up thread-ready, the attribute cache needs to become per-thread. Orthogonal of the threading issue, there is another posssible optimization that is not there yet. The cache can be tied to what is in ccheck[] to further reduce the size of the cache, making step 2. a lot cheaper. Currently in step 1. we read and keep everything, but if we tie the cache to the contents of ccheck[], we can read and ignore entries we read in step 1. that does not talk about the attributes ccheck[] is interested in. My plan is to either (1) make the cache per-thread, limit the reading done in 1. to ccheck[], but invalidate the cache when a different set of attributes are asked; or (2) make the cache per . -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)
On 07/13/2016 12:20 AM, Joey Hess wrote: Torsten Bögershausen wrote re jh/clean-smudge-annex: The thing is that we need to check the file system to find .gitatttibutes, even if we just did it 1 nanosecond ago. So the .gitattributes is done 3 times: -1 would_convert_to_git_filter_fd( -2 assert(would_convert_to_git_filter_fd(path)); -3 convert.c/convert_to_git_filter_fd() The only situation where this could be useful is when the .gitattributes change between -1 and -2, but then they would have changed between -1 and -3, and convert.c will die(). Does it make sense to remove -2 ? There's less redundant work going on than at first seems, because .gitattribute files are only read once and cached. Verified by strace. OK, I think I missed that work (not enough time for Git at the moment) Junio, please help me out, do we have a cache here now? I tried to figure out that following your attr branch, but failed. So, the redundant work is only in the processing that convert_attrs() does of the cached .gitattributes. Notice that there was a similar redundant triple call to convert_attrs() before my patch set: 1. index_fd checks would_convert_to_git_filter_fd 2. index_stream_convert_blob does assert(would_convert_to_git_filter_fd(path)) (Again redundantly since 1. is its only caller and has already checked.) 3. in convert_to_git_filter_fd If convert_attrs() is somehow expensive, it might be worth passing a struct conv_attrs * into the functions that currently call convert_attrs(). But it does not look expensive to me. I have that on the list, but seems to be uneccesary now. I think it would be safe enough to remove both asserts, at least as the code is structured now. OK. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)
Torsten Bögershausen wrote re jh/clean-smudge-annex: > The thing is that we need to check the file system to find .gitatttibutes, > even if we just did it 1 nanosecond ago. > > So the .gitattributes is done 3 times: > -1 would_convert_to_git_filter_fd( > -2 assert(would_convert_to_git_filter_fd(path)); > -3 convert.c/convert_to_git_filter_fd() > > The only situation where this could be useful is when the .gitattributes > change between -1 and -2, > but then they would have changed between -1 and -3, and convert.c > will die(). > > Does it make sense to remove -2 ? There's less redundant work going on than at first seems, because .gitattribute files are only read once and cached. Verified by strace. So, the redundant work is only in the processing that convert_attrs() does of the cached .gitattributes. Notice that there was a similar redundant triple call to convert_attrs() before my patch set: 1. index_fd checks would_convert_to_git_filter_fd 2. index_stream_convert_blob does assert(would_convert_to_git_filter_fd(path)) (Again redundantly since 1. is its only caller and has already checked.) 3. in convert_to_git_filter_fd If convert_attrs() is somehow expensive, it might be worth passing a struct conv_attrs * into the functions that currently call convert_attrs(). But it does not look expensive to me. I think it would be safe enough to remove both asserts, at least as the code is structured now. -- see shy jo signature.asc Description: PGP signature
Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)
Michael Haggerty writes: > (I suppose that `REFNAME_ALLOW_ONELEVEL` was > meant for checking partial reference names, for example to vet "foo" > that the caller wants to pass to `git branch`, which automatically turns > it into `refs/heads/foo`.) Correct. > In summary, `check_refname_format()` is in some ways less strict than > `refname_is_safe()`. For example, it allows reference names like > `git-p4-tmp/6` or even `git-p4-tmp`. Again, correct. > I don't know what I was thinking. Long ago, when I refactored and > documented check_refname_format(), I realized that the checks are > surprisingly lax and that the `REFNAME_ALLOW_ONELEVEL` option is > misleading. But I was new to the project and wasn't brave or energetic > enough to do something about it. Meanwhile I guess I forgot that it > never got fixed. Commit > > d0f810f0 2014-09-03 refs.c: allow listing and deleting badly named refs > > , which introduced `refname_is_safe()`, perhaps muddled the situation by > adding a "fallback" check that is not strictly laxer than the main check. > > Where does that leave us? > > * It is currently possible to create and delete references with > names that are neither within `refs/` nor ALL_CAPS (though not > remotely). > > * Such references do not participate in reachability checks, so > they have to be considered semi-broken. > > * Users could create chaos (though not remotely) by creating a > loose "reference" whose name coincides with that of another > file under `$GIT_DIR`. > (`git update-ref objects/info/alternates HEAD` anyone?) All correct. > * `git-p4` is apparently the only code within the Git project that > was making use of this questionable freedom. True. > * Presumably there is user code in the wild that creates and uses > such references. I doubt it, if they value their data. .git/p4-tmp or .git/p4-tmp/6 are not just "partly broken"--they are just as transient as things like MERGE_HEAD in that the objects pointed by them are subject to be pruned. > I think we need to get this under control, but given that we've allowed > such references (albeit partly broken) for a long time, we probably need > to provide an escape hatch to aid the transition. I'm skeptical that the > mh/split-under-lock patch series is the best place to start such a > campaign. On the other hand, I don't want that patch series to open up > any new avenues to creating references that escape `$GIT_DIR`. > > Let me think for a bit about the next step. Input is welcome. > > In any case, I think that Lars's patch to `git-p4` is a good thing. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)
On 06/23/2016 09:32 AM, Michael Haggerty wrote: > [...] > I have to say, I'm still confused about how the old code could have > worked at all. For quite some time, Git hasn't allowed references with > weird names like `git-p4-tmp/6` to be created, so how could there be any > references there that need to be deleted? It seems to me that one of the > following must be true: > [...] > * I'm wrong about Git not allowing references like `git-p4-tmp/6` to > be created, in which case I'd like to know what code path allows it > so that I can fix it. I was indeed wrong. Reference names are vetted by `check_refname_format()` on creation, but that function knows and enforces nothing about the `refs/` namespace or the ALL_CAPS convention for top-level references. Moreover, the relevant caller passes the `REFNAME_ALLOW_ONELEVEL` option, which has semantics that are fairly useless as far as I can tell. A casual reader might assume that `REFNAME_ALLOW_ONELEVEL` allows one-level reference only if they satisfy the special `ALL_CAPS` convention, but in fact it doesn't enforce the convention. (I suppose that `REFNAME_ALLOW_ONELEVEL` was meant for checking partial reference names, for example to vet "foo" that the caller wants to pass to `git branch`, which automatically turns it into `refs/heads/foo`.) In summary, `check_refname_format()` is in some ways less strict than `refname_is_safe()`. For example, it allows reference names like `git-p4-tmp/6` or even `git-p4-tmp`. With current master: $ git update-ref git-p4-tmp HEAD $ echo $? 0 $ git rev-parse git-p4-tmp cf4c2cfe52be5bd973a4838f73a35d3959ce2f43 $ git update-ref -d git-p4-tmp $ echo $? 0 I don't know what I was thinking. Long ago, when I refactored and documented check_refname_format(), I realized that the checks are surprisingly lax and that the `REFNAME_ALLOW_ONELEVEL` option is misleading. But I was new to the project and wasn't brave or energetic enough to do something about it. Meanwhile I guess I forgot that it never got fixed. Commit d0f810f0 2014-09-03 refs.c: allow listing and deleting badly named refs , which introduced `refname_is_safe()`, perhaps muddled the situation by adding a "fallback" check that is not strictly laxer than the main check. Where does that leave us? * It is currently possible to create and delete references with names that are neither within `refs/` nor ALL_CAPS (though not remotely). * Such references do not participate in reachability checks, so they have to be considered semi-broken. * Users could create chaos (though not remotely) by creating a loose "reference" whose name coincides with that of another file under `$GIT_DIR`. (`git update-ref objects/info/alternates HEAD` anyone?) * `git-p4` is apparently the only code within the Git project that was making use of this questionable freedom. * Presumably there is user code in the wild that creates and uses such references. I think we need to get this under control, but given that we've allowed such references (albeit partly broken) for a long time, we probably need to provide an escape hatch to aid the transition. I'm skeptical that the mh/split-under-lock patch series is the best place to start such a campaign. On the other hand, I don't want that patch series to open up any new avenues to creating references that escape `$GIT_DIR`. Let me think for a bit about the next step. Input is welcome. In any case, I think that Lars's patch to `git-p4` is a good thing. Michael -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)
Michael Haggerty writes: > I'd like to request that the change for the p4 temprefs be made in two > steps: > > 1. Write references to `refs/git-p4-tmp` or whatever, without >worrying about making them per-worktree. > > 2. Carve out a per-worktree namespace, bikeshed about its name and >semantics, make sure it works correctly, then finally move the >p4 temporary references and eventually the bisect references there. Hasn't 2. already happened long time ago? I thought that the direction was that bisect will have to be grandfathered to stay there, and new things would hang beneath a single per-worktree hierarchy. I would have expected that your 1. would be a request to use refs/worktrees/git-p4-tmp. ... goes and looks ... Ahh, I did misremember the discussions. http://thread.gmane.org/gmane.comp.version-control.git/276628 http://thread.gmane.org/gmane.comp.version-control.git/276960/focus=276963 It appears that in the latter, a more generic refs/worktrees/ was somehow postponed, and only bisect was made per-worktree to fix the immediate breakage. > The reason is that step 1 is low-risk, could be made quickly, and is > enough to unblock mh/split-under-lock and the other patch series that > depend on it. Step 2, on the other hand, could take quite a while, and > its implementation might benefit from changes to reference handling that > are in the pipeline (e.g., perhaps the horrible hack can be dispensed > with). Meanwhile, as far as I understand these references are transient, > so there are no backwards-compatibility considerations that make > renaming them twice more cumbersome than renaming them once. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)
> On 23 Jun 2016, at 09:32, Michael Haggerty wrote: > > On 06/20/2016 09:57 AM, Lars Schneider wrote: >> >>> On 20 Jun 2016, at 01:51, Junio C Hamano wrote: >>> >>> Junio C Hamano writes: >>> Michael Haggerty writes: > 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. Ah, OK, that would try mucking with .git/git-p4-tmp/6 but that is not a place to have a ref. It will not participate in reachability analysis and will end up losing the referents. Perhaps placing it under refs/git-p4-tmp would fix it (both in git-p4 and in tests)? > > I have to say, I'm still confused about how the old code could have > worked at all. For quite some time, Git hasn't allowed references with > weird names like `git-p4-tmp/6` to be created, so how could there be any > references there that need to be deleted? It seems to me that one of the > following must be true: > > * This feature (i.e., whatever needs to create the temporary > references) has been broken for a long time, which would imply that > there are no tests for it. > > * The references are created in some bogus way, like writing files > directly to the filesystem, rather than using `git update-ref`. > This is broken and will be even more broken soon when we add > support for different reference storage backends. > > * Such references are never actually created and never actually > needed. This would imply that the cleanup code is unnecessary. > > * The temporary references are created using `git branch`, and are > thus actually named like `refs/heads/git-p4-tmp/6`. In this case > the deletion code wasn't cleaning them up right, but the feature > might have otherwise worked correctly. > > * I'm wrong about Git not allowing references like `git-p4-tmp/6` to > be created, in which case I'd like to know what code path allows it > so that I can fix it. > > Any of these possibilities would mean that your proposed fix is wrong or > incomplete. Although I know git-p4 quite well, I am certainly no expert for the git-p4 branch import. I also pointed out in my previous reply ($gmane/297703) that the git-p4 branch import never worked successfully for me. I always thought the reason is that my P4 branch history is a bit messy. The clean git-p4 branch test cases work fine, though. >>> Oh, another thing. If these refs are meant to be transient, they >>> are likely to be per worktree, if "git worktree" managed multiple >>> worktrees that share the same set of branches and tags are in use. >>> >>> I recall we carved out one hierarchy under refs/ as "not shared >>> across worktrees" (was that refs/worktree/ hierarchy? I didn't >>> check but please do so when the patch actually is written), and >>> that hierarchy is the appropriate thing to use for this, I think. >> >> Thanks for the hint. It looks like as if the "per worktree" decision >> is made in "ref.c:466" "is_per_worktree_ref": >> https://github.com/git/git/blob/3dc84b0c19932ec9947ca4936b6bfd6421ccb1b4/refs.c#L466-L470 >> >> In ce414b3 "refs/bisect" was added to a list of prefixes that are >> per worktree. I could easily add "refs/git-p4-tmp" to this list but >> I don't think that would be a good solution. I would prefer to go with >> your suggestion and add "refs/worktree" to the prefix list and then use >> "refs/worktree/git-p4-tmp". >> >> Later on we could move "refs/bisect" to "refs/worktree/bisect" and >> simplify the "is_per_worktree_ref" code, too. > > The other part of making `refs/bisect` per-worktree is this horrible > hack here [1]. Thanks for the pointer! > I'd like to request that the change for the p4 temprefs be made in two > steps: > > 1. Write references to `refs/git-p4-tmp` or whatever, without > worrying about making them per-worktree. > > 2. Carve out a per-worktree namespace, bikeshed about its name and > semantics, make sure it works correctly, then finally move the > p4 temporary references and eventually the bisect references there. > > The reason is that step 1 is low-risk, could be made quickly, and is > enough to unblock mh/split-under-lock and the other patch series that > depend on it. Step 2, on the other hand, could take quite a while, and > its implementation might benefit from changes to reference handling that > are in the pipeline (e.g., perhaps the horrible hack can be dispensed > with). Meanwhile, as far as I understand these references are transient, > so there are no backwards-compatibility considerations that make > renaming them twice more cumbersome than renaming them once. Agreed! I will post the patch in a few seconds. Cheers, Lars > > Thanks, > Michael > > [1] > https://github.com/git/git/blob/ab7797dbe95fff38d9265869ea367020046db118/refs/files-backend.c#L176-L192 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to major
Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)
On 22/06/16 23:09, Joey Hess wrote: Torsten Bögershausen wrote: There is a conflict in pu: "jh/clean-smudge-annex" does not work together with "tb/convert-peek-in-index" (And currently pu didn't compile) I'm sending a v4 of jh/clean-smudge-annex that is rebased on top of tb/convert-peek-in-index to fix this. (I will hopefully be able to do a separate review of the smudge/clean patch) Would be appreciated. It'll be 2 weeks until I can work on this any more. (And jo...@joeyh.name is not reachable from web.de) I'd like to fix whatever's broken; you could send details out of band to joeyh...@gmail.com Currently there is one comment: The (new) usage of assert() in sha1_file.c: assert(would_convert_to_git_filter_fd(path)); The thing is that we need to check the file system to find .gitatttibutes, even if we just did it 1 nanosecond ago. So the .gitattributes is done 3 times: -1 would_convert_to_git_filter_fd( -2 assert(would_convert_to_git_filter_fd(path)); -3 convert.c/convert_to_git_filter_fd() The only situation where this could be useful is when the .gitattributes change between -1 and -2, but then they would have changed between -1 and -3, and convert.c will die(). Does it make sense to remove -2 ? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)
On 06/20/2016 09:57 AM, Lars Schneider wrote: > >> On 20 Jun 2016, at 01:51, Junio C Hamano wrote: >> >> Junio C Hamano writes: >> >>> Michael Haggerty writes: >>> 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. >>> >>> Ah, OK, that would try mucking with .git/git-p4-tmp/6 but that is >>> not a place to have a ref. It will not participate in reachability >>> analysis and will end up losing the referents. >>> >>> Perhaps placing it under refs/git-p4-tmp would fix it (both in >>> git-p4 and in tests)? I have to say, I'm still confused about how the old code could have worked at all. For quite some time, Git hasn't allowed references with weird names like `git-p4-tmp/6` to be created, so how could there be any references there that need to be deleted? It seems to me that one of the following must be true: * This feature (i.e., whatever needs to create the temporary references) has been broken for a long time, which would imply that there are no tests for it. * The references are created in some bogus way, like writing files directly to the filesystem, rather than using `git update-ref`. This is broken and will be even more broken soon when we add support for different reference storage backends. * Such references are never actually created and never actually needed. This would imply that the cleanup code is unnecessary. * The temporary references are created using `git branch`, and are thus actually named like `refs/heads/git-p4-tmp/6`. In this case the deletion code wasn't cleaning them up right, but the feature might have otherwise worked correctly. * I'm wrong about Git not allowing references like `git-p4-tmp/6` to be created, in which case I'd like to know what code path allows it so that I can fix it. Any of these possibilities would mean that your proposed fix is wrong or incomplete. >> Oh, another thing. If these refs are meant to be transient, they >> are likely to be per worktree, if "git worktree" managed multiple >> worktrees that share the same set of branches and tags are in use. >> >> I recall we carved out one hierarchy under refs/ as "not shared >> across worktrees" (was that refs/worktree/ hierarchy? I didn't >> check but please do so when the patch actually is written), and >> that hierarchy is the appropriate thing to use for this, I think. > > Thanks for the hint. It looks like as if the "per worktree" decision > is made in "ref.c:466" "is_per_worktree_ref": > https://github.com/git/git/blob/3dc84b0c19932ec9947ca4936b6bfd6421ccb1b4/refs.c#L466-L470 > > In ce414b3 "refs/bisect" was added to a list of prefixes that are > per worktree. I could easily add "refs/git-p4-tmp" to this list but > I don't think that would be a good solution. I would prefer to go with > your suggestion and add "refs/worktree" to the prefix list and then use > "refs/worktree/git-p4-tmp". > > Later on we could move "refs/bisect" to "refs/worktree/bisect" and > simplify the "is_per_worktree_ref" code, too. The other part of making `refs/bisect` per-worktree is this horrible hack here [1]. I'd like to request that the change for the p4 temprefs be made in two steps: 1. Write references to `refs/git-p4-tmp` or whatever, without worrying about making them per-worktree. 2. Carve out a per-worktree namespace, bikeshed about its name and semantics, make sure it works correctly, then finally move the p4 temporary references and eventually the bisect references there. The reason is that step 1 is low-risk, could be made quickly, and is enough to unblock mh/split-under-lock and the other patch series that depend on it. Step 2, on the other hand, could take quite a while, and its implementation might benefit from changes to reference handling that are in the pipeline (e.g., perhaps the horrible hack can be dispensed with). Meanwhile, as far as I understand these references are transient, so there are no backwards-compatibility considerations that make renaming them twice more cumbersome than renaming them once. Thanks, Michael [1] https://github.com/git/git/blob/ab7797dbe95fff38d9265869ea367020046db118/refs/files-backend.c#L176-L192 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)
Torsten Bögershausen wrote: > There is a conflict in pu: > "jh/clean-smudge-annex" does not work together with "tb/convert-peek-in-index" > > (And currently pu didn't compile) I'm sending a v4 of jh/clean-smudge-annex that is rebased on top of tb/convert-peek-in-index to fix this. > (I will hopefully be able to do a separate review of the smudge/clean patch) Would be appreciated. It'll be 2 weeks until I can work on this any more. > (And jo...@joeyh.name is not reachable from web.de) I'd like to fix whatever's broken; you could send details out of band to joeyh...@gmail.com -- see shy jo signature.asc Description: PGP signature
Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)
Torsten Bögershausen writes: > There is a conflict in pu: > "jh/clean-smudge-annex" does not work together with "tb/convert-peek-in-index" > > (And currently pu didn't compile) Thanks for a report, but didn't I mention this breakage in the What's cooking report already? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)
> On 20 Jun 2016, at 01:51, Junio C Hamano wrote: > > Junio C Hamano writes: > >> Michael Haggerty writes: >> >>> 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. >> >> Ah, OK, that would try mucking with .git/git-p4-tmp/6 but that is >> not a place to have a ref. It will not participate in reachability >> analysis and will end up losing the referents. >> >> Perhaps placing it under refs/git-p4-tmp would fix it (both in >> git-p4 and in tests)? > > Oh, another thing. If these refs are meant to be transient, they > are likely to be per worktree, if "git worktree" managed multiple > worktrees that share the same set of branches and tags are in use. > > I recall we carved out one hierarchy under refs/ as "not shared > across worktrees" (was that refs/worktree/ hierarchy? I didn't > check but please do so when the patch actually is written), and > that hierarchy is the appropriate thing to use for this, I think. Thanks for the hint. It looks like as if the "per worktree" decision is made in "ref.c:466" "is_per_worktree_ref": https://github.com/git/git/blob/3dc84b0c19932ec9947ca4936b6bfd6421ccb1b4/refs.c#L466-L470 In ce414b3 "refs/bisect" was added to a list of prefixes that are per worktree. I could easily add "refs/git-p4-tmp" to this list but I don't think that would be a good solution. I would prefer to go with your suggestion and add "refs/worktree" to the prefix list and then use "refs/worktree/git-p4-tmp". Later on we could move "refs/bisect" to "refs/worktree/bisect" and simplify the "is_per_worktree_ref" code, too. Thoughts? Thanks, Lars-- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)
There is a conflict in pu: "jh/clean-smudge-annex" does not work together with "tb/convert-peek-in-index" (And currently pu didn't compile) (I will hopefully be able to do a separate review of the smudge/clean patch) (And jo...@joeyh.name is not reachable from web.de) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)
Junio C Hamano writes: > Michael Haggerty writes: > >> 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. > > Ah, OK, that would try mucking with .git/git-p4-tmp/6 but that is > not a place to have a ref. It will not participate in reachability > analysis and will end up losing the referents. > > Perhaps placing it under refs/git-p4-tmp would fix it (both in > git-p4 and in tests)? Oh, another thing. If these refs are meant to be transient, they are likely to be per worktree, if "git worktree" managed multiple worktrees that share the same set of branches and tags are in use. I recall we carved out one hierarchy under refs/ as "not shared across worktrees" (was that refs/worktree/ hierarchy? I didn't check but please do so when the patch actually is written), and that hierarchy is the appropriate thing to use for this, I think. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)
> On 19 Jun 2016, at 20:49, Lars Schneider wrote:
>
>
>> On 19 Jun 2016, at 20:13, Junio C Hamano wrote:
>>
>> Lars Schneider writes:
>>
>>> 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'):
>>
>> Anywhere in refs/ would be OK, but don't you need to adjust the
>> test, too?
>>
>> Even though I do not use git-p4, I'd imagine that I would be upset
>> if temporary refs that are used only during sync contaminated the
>> set of local branches I have, if I were a user of git-p4. Would it
>> make sense to use "refs/git-p4-tmp" or something instead?
> Yes, "refs/git-p4-tmp" would work equally well.
Plus, you are right. A minor test adjustment is necessary (although
the tests pass without adjustment). I will post a full patch.
- Lars
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)
> On 19 Jun 2016, at 20:13, Junio C Hamano wrote:
>
> Lars Schneider writes:
>
>> 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'):
>
> Anywhere in refs/ would be OK, but don't you need to adjust the
> test, too?
>
> Even though I do not use git-p4, I'd imagine that I would be upset
> if temporary refs that are used only during sync contaminated the
> set of local branches I have, if I were a user of git-p4. Would it
> make sense to use "refs/git-p4-tmp" or something instead?
Yes, "refs/git-p4-tmp" would work equally well.
- Lars
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)
Lars Schneider writes:
> 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'):
Anywhere in refs/ would be OK, but don't you need to adjust the
test, too?
Even though I do not use git-p4, I'd imagine that I would be upset
if temporary refs that are used only during sync contaminated the
set of local branches I have, if I were a user of git-p4. Would it
make sense to use "refs/git-p4-tmp" or something instead?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)
Michael Haggerty writes: > 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. Ah, OK, that would try mucking with .git/git-p4-tmp/6 but that is not a place to have a ref. It will not participate in reachability analysis and will end up losing the referents. Perhaps placing it under refs/git-p4-tmp would fix it (both in git-p4 and in tests)? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)
Michael Haggerty writes: >> Good timing. I was planning to kick split-under-lock and any of its >> dependents temporarily out of 'next', so that fixes can choose not >> to be incremental, and dependent topics can be rebased on top of the >> fixed fondation. Even if we do incremental, [4] is not sufficient >> material for me to write a log message for. >> >> So people who reviewed what has been in 'next' can revisit [4] and >> give review comments, while I could just pick up the history >> mentioned there, i.e. >> >> git checkout pu >> git pull git://github.com/mhagger/git >> +split-under-lock:mh/split-under-lock >> >> and we can start from there? > > Sure. The branches in my GitHub fork already include all of the > improvements and fixes that I know of, and the only outstanding issue is > the one that Lars mentioned in this thread (which I believe to be a > problem in git-p4). > > BTW, there are still no conflicts between these branches > (split-under-lock, update-ref-errors, ref-iterators, and ref-store) and > current master. Therefore, I don't see a need to rebase them onto > master. But if you would prefer that I do so, just let me know. If the updated split-under-lock itself can build on the same upstream commit, then there is no reason to rebase it on top of v2.9 or anything newer. Your other topics queued in my tree build on the version of split-under-lock I have (e.g. mh/ref-store uses ref-iterators and split-under-lock). When split-under-lock gets updated, they need to be rebuilt on top of the updated version, and that is what I meant by "dependent topics can be rebased on top". -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)
> On 19 Jun 2016, at 17:04, Lars Schneider wrote:
>
>
>> On 19 Jun 2016, at 09:59, Michael Haggerty wrote:
>>
>> On 06/18/2016 12:05 AM, Lars Schneider wrote:
>>>
On 17 Jun 2016, at 05:20, Junio C Hamano 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)
> On 19 Jun 2016, at 09:59, Michael Haggerty wrote: > > On 06/18/2016 12:05 AM, Lars Schneider wrote: >> >>> On 17 Jun 2016, at 05:20, Junio C Hamano 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. > > 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-- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)
On 06/18/2016 08:20 PM, Junio C Hamano wrote: > Michael Haggerty writes: > >> On 06/17/2016 05:20 AM, Junio C Hamano wrote: >>> [...] >>> * mh/ref-iterators (2016-06-03) 13 commits >>> (merged to 'next' on 2016-06-06 at c8e79dc) >>> + ... >>> (this branch is used by mh/ref-store; uses mh/split-under-lock; is tangled >>> with mh/update-ref-errors.) >>> >>> The API to iterate over all the refs (i.e. for_each_ref(), etc.) >>> has been revamped. >>> >>> Will merge to 'master'. >> >> It would be preferable (though not critical) to use the promised v3, >> which I just sent [1]. This includes some minor improvements, described >> here [2]. This is also available from my GitHub fork [3] as branch >> "ref-iterators". >> >>> * 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 >>> + ... >>> Will merge to 'master'. >> >> Please make sure to pick up the important bugfix discussed here [4], >> which is integrated into branch "split-under-lock" on my GitHub fork [3]. > > Good timing. I was planning to kick split-under-lock and any of its > dependents temporarily out of 'next', so that fixes can choose not > to be incremental, and dependent topics can be rebased on top of the > fixed fondation. Even if we do incremental, [4] is not sufficient > material for me to write a log message for. > > So people who reviewed what has been in 'next' can revisit [4] and > give review comments, while I could just pick up the history > mentioned there, i.e. > > git checkout pu > git pull git://github.com/mhagger/git > +split-under-lock:mh/split-under-lock > > and we can start from there? Sure. The branches in my GitHub fork already include all of the improvements and fixes that I know of, and the only outstanding issue is the one that Lars mentioned in this thread (which I believe to be a problem in git-p4). BTW, there are still no conflicts between these branches (split-under-lock, update-ref-errors, ref-iterators, and ref-store) and current master. Therefore, I don't see a need to rebase them onto master. But if you would prefer that I do so, just let me know. Michael -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)
On 06/18/2016 12:05 AM, Lars Schneider wrote: > >> On 17 Jun 2016, at 05:20, Junio C Hamano 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)
Michael Haggerty writes: > On 06/17/2016 05:20 AM, Junio C Hamano wrote: >> [...] >> * mh/ref-iterators (2016-06-03) 13 commits >> (merged to 'next' on 2016-06-06 at c8e79dc) >> + ... >> (this branch is used by mh/ref-store; uses mh/split-under-lock; is tangled >> with mh/update-ref-errors.) >> >> The API to iterate over all the refs (i.e. for_each_ref(), etc.) >> has been revamped. >> >> Will merge to 'master'. > > It would be preferable (though not critical) to use the promised v3, > which I just sent [1]. This includes some minor improvements, described > here [2]. This is also available from my GitHub fork [3] as branch > "ref-iterators". > >> * 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 >> + ... >> Will merge to 'master'. > > Please make sure to pick up the important bugfix discussed here [4], > which is integrated into branch "split-under-lock" on my GitHub fork [3]. Good timing. I was planning to kick split-under-lock and any of its dependents temporarily out of 'next', so that fixes can choose not to be incremental, and dependent topics can be rebased on top of the fixed fondation. Even if we do incremental, [4] is not sufficient material for me to write a log message for. So people who reviewed what has been in 'next' can revisit [4] and give review comments, while I could just pick up the history mentioned there, i.e. git checkout pu git pull git://github.com/mhagger/git +split-under-lock:mh/split-under-lock and we can start from there? Thanks. > > Michael > > [1] http://thread.gmane.org/gmane.comp.version-control.git/297625 > [2] > http://thread.gmane.org/gmane.comp.version-control.git/296322/focus=296883 > [3] https://github.com/mhagger/git > [4] http://article.gmane.org/gmane.comp.version-control.git/297174 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)
On 06/18/2016 12:05 AM, Lars Schneider wrote: > >> On 17 Jun 2016, at 05:20, Junio C Hamano wrote: >> >> ... >> >> * mh/split-under-lock (2016-05-13) 33 commits >> (merged to 'next' on 2016-06-03 at 2e71330) >> [...] > > 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) Thanks for the bug report. I'll look into this as soon as I have the chance. Do you happen to know if there is a way to get a copy of p4 without paying for it so that I can run the tests locally? Given the commit that you bisected to, one likely possibility is that the test is trying to create a reference with an unsafe name, in the sense of `refname_is_safe()`. Is that possible? Do you happen to know what Git reference names that test case wants to create? Michael -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)
On 06/17/2016 05:20 AM, Junio C Hamano wrote: > [...] > * mh/ref-iterators (2016-06-03) 13 commits > (merged to 'next' on 2016-06-06 at c8e79dc) > + for_each_reflog(): reimplement using iterators > + dir_iterator: new API for iterating over a directory tree > + for_each_reflog(): don't abort for bad references > + do_for_each_ref(): reimplement using reference iteration > + refs: introduce an iterator interface > + ref_resolves_to_object(): new function > + entry_resolves_to_object(): rename function from ref_resolves_to_object() > + get_ref_cache(): only create an instance if there is a submodule > + remote rm: handle symbolic refs correctly > + delete_refs(): add a flags argument > + refs: use name "prefix" consistently > + do_for_each_ref(): move docstring to the header file > + refs: remove unnecessary "extern" keywords > (this branch is used by mh/ref-store; uses mh/split-under-lock; is tangled > with mh/update-ref-errors.) > > The API to iterate over all the refs (i.e. for_each_ref(), etc.) > has been revamped. > > Will merge to 'master'. It would be preferable (though not critical) to use the promised v3, which I just sent [1]. This includes some minor improvements, described here [2]. This is also available from my GitHub fork [3] as branch "ref-iterators". > * 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'. Please make sure to pick up the important bugfix discussed here [4], which is integrated into branch "split-under-lock" on my GitHub fork [3]. Michael [1] http://thread.gmane.org/gmane.comp.version-control.git/297625 [2] http://thread.gmane.org/gmane.comp.version-control.git/296322/focus=296883 [3] https://github.com/mhagger/git [4] http://article.gmane.org/gmane.comp.version-control.git/297174 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)
Lars Schneider writes: >> On 17 Jun 2016, at 05:20, Junio C Hamano 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 >> + ... >> + 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) Thanks. Let's hold it and all of its dependents to see what is going on. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)
> On 17 Jun 2016, at 05:20, Junio C Hamano 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-- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)
Junio C Hamano pobox.com> writes: > * va/i18n-even-more (2016-06-07) 38 commits > - i18n: branch: mark comment when editing branch description for translation > - i18n: unmark die messages for translation > - i18n: submodule: escape shell variables inside eval_gettext > - i18n: submodule: join strings marked for translation > - i18n: init-db: join message pieces [snip] > - i18n: builtin/remote.c: fix mark for translation > - Merge branch 'jc/t2300-setup' into HEAD Patch "i18n: init-db: join message pieces" breaks test t0204-gettext-reencode-sanity.sh because it changes message id used in Icelandic translation used on that test. Please, don't merge this series, I'm going to send a re-roll (v5). Discussion: http://mid.gmane.org/1562644.BnkjqA6nsN@linux-omuo -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)
Hey Junio, On Fri, Jun 17, 2016 at 8:50 AM, Junio C Hamano wrote: > * pb/bisect (2016-05-24) 3 commits > - bisect--helper: `write_terms` shell function in C > - bisect: rewrite `check_term_format` shell function in C > - bisect--helper: use OPT_CMDMODE instead of OPT_BOOL > > Beginning of GSoC "git bisect" project. > > I know another topic is getting rerolled many times on top of this; > are people happy with these three patches? If so, will merge to > 'next'. This matches the last re-roll[1] I did which looks good to me. Thanks! Though I was under the impression that the whole topic would be merged to next after the completion of the GSoC project because the test suite seems lacking at a few places (pointed out by Eric). So I had intended to do the coverage tests after the completion so that I could use the gcov rather than setting up a coverage tool for shell scripts. [1]: http://thread.gmane.org/gmane.comp.version-control.git/295518 Regards, Pranit Bauva -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
What's cooking in git.git (Jun 2016, #05; Thu, 16)
Here are the topics that have been cooking. Commits prefixed with '-' are only in 'pu' (proposed updates) while commits prefixed with '+' are in 'next'. The ones marked with '.' do not appear in any of the integration branches, but I am still holding onto them. Let's start a new cycle by rewinding the tip of 'next' soonish. I expect I'd eject a few premature topics out of 'next' while doing so. There are many topics that need input from the list (look for '?' in this document) to decide either to drop them or to move them forward. You can find the changes described here in the integration branches of the repositories listed at http://git-blame.blogspot.com/p/git-public-repositories.html -- [New Topics] * ap/git-svn-propset-doc (2016-06-15) 1 commit - git-svn: document the 'git svn propset' command "git svn propset" subcommand that was added in 2.3 days is documented now. Will merge to 'next'. * jk/add-i-diff-compact-heuristics (2016-06-16) 1 commit - add--interactive: respect diff.compactionHeuristic "git add -i/-p" learned to honor diff.compactionHeuristic experimental knob, so that the user can work on the same hunk split as "git diff" output. Will merge to 'next'. * jk/big-and-old-archive-tar (2016-06-16) 2 commits - archive-tar: write extended headers for far-future mtime - archive-tar: write extended headers for file sizes >= 8GB "git archive" learned to handle files that are larger than 8GB and commits far in the future than expressible by the traditional US-TAR format. Will merge to 'next'. * jk/gpg-interface-cleanup (2016-06-16) 7 commits - gpg-interface: check gpg signature creation status - sign_buffer: use pipe_command - verify_signed_buffer: use pipe_command - run-command: add pipe_command helper - verify_signed_buffer: use tempfile object - verify_signed_buffer: drop pbuf variable - gpg-interface: use child_process.args A new run-command API function pipe_command() is introduced to sanely feed data to the standard input while capturing data from the standard output and the standard error of an external process, which is cumbersome to hand-roll correctly without deadlocking. The codepath to sign data in a prepared buffer with GPG has been updated to use this API to read from the status-fd to check for errors (instead of relying on GPG's exit status). Will merge to 'next'. * lf/sideband-returns-void (2016-06-16) 2 commits - upload-pack.c: make send_client_data() return void - sideband.c: make send_sideband() return void A small internal API cleanup. Will merge to 'next'. * nd/graph-width-padded (2016-06-16) 2 commits - pretty.c: support |() forms - pretty: pass graph width to pretty formatting for use in '%>|(N)' "log --graph --format=" learned that "%>|(N)" specifies the width relative to the terminal's left edge, not relative to the area to draw text that is to the right of the ancestry-graph section. It also now accepts negative N that means the column limit is relative to the right border. Will merge to 'next'. * dn/gpg-doc (2016-06-16) 1 commit - Documentation: GPG capitalization The documentation tries to consistently spell "GPG"; when referring to the specific program name, "gpg" is used. Will merge to 'next'. * jk/bisect-show-tree (2016-06-16) 1 commit - bisect: always call setup_revisions after init_revisions "git bisect" makes an internal call to "git diff-tree" when bisection finds the culprit, but this call did not initialize the data structure to pass to the diff-tree API correctly. Will merge to 'next'. -- [Stalled] * mj/log-show-signature-conf (2016-06-06) 2 commits - log: "--no-show-signature" commmand-line option - log: add "log.showsignature" configuration variable "git log" learns log.showSignature configuration variable, and a command line option "--no-show-signature" to countermand it. The order of the commits in the topic need to be reversed. Expecting a reroll. * sb/bisect (2016-04-15) 22 commits - SQUASH??? - bisect: get back halfway shortcut - bisect: compute best bisection in compute_relevant_weights() - bisect: use a bottom-up traversal to find relevant weights - bisect: prepare for different algorithms based on find_all - bisect: rename count_distance() to compute_weight() - bisect: make total number of commits global - bisect: introduce distance_direction() - bisect: extract get_distance() function from code duplication - bisect: use commit instead of commit list as arguments when appropriate - bisect: replace clear_distance() by unique markers - bisect: use struct node_data array instead of int array - bisect: get rid of recursion in count_distance() - bisect: make algorithm behavior independent of DEBUG_BISECT - bisect: make bisect compile if DEBUG_BISECT is set - bisect: plug the biggest memory leak - bisect: add test for the bisect algorithm - t
