Re: [PATCH/RFC] Revoke write access to refs and odb after importing another repo's odb
On Wed, Jan 23, 2013 at 8:34 PM, Nguyễn Thái Ngọc Duy pclo...@gmail.com wrote: add_submodule_odb() can be used to import objects from another repository temporarily. After this point we don't know which objects are ours, which are external. If we create an object that refers to an external object, next time git runs, it may find a hole in the object graph because the external repository may not be imported. The same goes for pointing a ref to an external SHA-1. To protect ourselves, once add_submodule_odb() is used: - trees, tags and commits cannot be created - refs cannot be updated .. and soon after I pressed send, I realized I missed at least two other places write should not be allowed: - index - reflog But the general idea is probably more important than implementation details at this stage. -- Duy -- 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: [PATCH/RFC] Revoke write access to refs and odb after importing another repo's odb
Nguyễn Thái Ngọc Duy pclo...@gmail.com writes: add_submodule_odb() can be used to import objects from another repository temporarily. After this point we don't know which objects are ours, which are external. If we create an object that refers to an external object, next time git runs, it may find a hole in the object graph because the external repository may not be imported. The same goes for pointing a ref to an external SHA-1. To protect ourselves, once add_submodule_odb() is used: - trees, tags and commits cannot be created - refs cannot be updated In certain cases that submodule code knows that it's safe to write, it can turn the readonly flag off. Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com --- I think this is a good safety check. Two step implementation to bring read-only support into a testable shape and then flip that bit in add_submdule_odb() would be a sensible approach. I however have this suspicion that this will become a losing battle and we would be better off getting rid of add_submodule_odb(); instead operations that work across repositories will be done as a subprocess, which will get us back closer to one of the original design goals of submodule support to have a clear separation between the superproject and its submodules. -- 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: [PATCH/RFC] Revoke write access to refs and odb after importing another repo's odb
Am 23.01.2013 18:01, schrieb Junio C Hamano: Nguyễn Thái Ngọc Duy pclo...@gmail.com writes: add_submodule_odb() can be used to import objects from another repository temporarily. After this point we don't know which objects are ours, which are external. If we create an object that refers to an external object, next time git runs, it may find a hole in the object graph because the external repository may not be imported. The same goes for pointing a ref to an external SHA-1. To protect ourselves, once add_submodule_odb() is used: - trees, tags and commits cannot be created - refs cannot be updated In certain cases that submodule code knows that it's safe to write, it can turn the readonly flag off. Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com --- I think this is a good safety check. Two step implementation to bring read-only support into a testable shape and then flip that bit in add_submdule_odb() would be a sensible approach. I agree this is a worthwhile change so nobody accidentally screws things up. It catches at least a case in t7405.3. I did not investigate further though. This is a false positive. The merge algorithm picked a fast-forward in a submodule as a proper merge result and records that in a gitlink. But as Duy pointed out this could be easily fixed by turning the readonly flag off in that case. I however have this suspicion that this will become a losing battle and we would be better off getting rid of add_submodule_odb(); instead operations that work across repositories will be done as a subprocess, which will get us back closer to one of the original design goals of submodule support to have a clear separation between the superproject and its submodules. Please don't. While I agree with your goal, I'd be unhappy to do that because of the performance drop (especially on fork-challenged operating systems). -- 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: [PATCH/RFC] Revoke write access to refs and odb after importing another repo's odb
Jens Lehmann jens.lehm...@web.de writes: This is a false positive. The merge algorithm picked a fast-forward in a submodule as a proper merge result and records that in a gitlink. But as Duy pointed out this could be easily fixed by turning the readonly flag off in that case. I see that as easily circumvented and not an effective protection, though. In theory, adding a gitlink to the index, removing a gitlink to the index and modifying an existing gitlink in the index to another gitlink in the index and writing the resulting in-core index out to the on-disk index should be allowed, even after objects from the submodule object database have contaminated our in-core object pool, as long as you do not run cache_tree_update(). I am not sure if that single loophole would be sufficient, though. -- 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: [PATCH/RFC] Revoke write access to refs and odb after importing another repo's odb
On Thu, Jan 24, 2013 at 4:06 AM, Junio C Hamano gits...@pobox.com wrote: Jens Lehmann jens.lehm...@web.de writes: This is a false positive. The merge algorithm picked a fast-forward in a submodule as a proper merge result and records that in a gitlink. But as Duy pointed out this could be easily fixed by turning the readonly flag off in that case. I see that as easily circumvented and not an effective protection, though. In theory, adding a gitlink to the index, removing a gitlink to the index and modifying an existing gitlink in the index to another gitlink in the index and writing the resulting in-core index out to the on-disk index should be allowed, even after objects from the submodule object database have contaminated our in-core object pool, as long as you do not run cache_tree_update(). I am not sure if that single loophole would be sufficient, though. The problem is we don't know which entries are updated in index. We don't keep track of them. And I think in the unpack-trees case, we scape the whole index then copy over, making it look like the whole index is updated (even with the same content). One way to check this is verify the source of all non-gitlink entries in index before writing to disk (only when readonly flag is on, of course). sha1_object_info_extended() should help (or be extended to do the job). Hmm.. if we do this, we could also verify if new sha-1 objects do not refer to an external source, if so allow them to be created. -- Duy -- 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
