Re: [PATCH v4 14/15] files-backend: remove submodule_allowed from files_downcast()
On Mon, Feb 20, 2017 at 7:11 PM, Michael Haggerty wrote: > On 02/18/2017 02:33 PM, Nguyễn Thái Ngọc Duy wrote: >> Since submodule or not is irrelevant to files-backend after the last >> patch, remove the parameter from files_downcast() and kill >> files_assert_main_repository(). >> >> PS. This implies that all ref operations are allowed for submodules. But >> we may need to look more closely to see if that's really true... > > I think you are jumping the gun here. > > Even after your changes, there is still a significant difference between > the main repository and submodules: we have access to the object > database for the main repository, but not for submodules. I did jump the gun for another reason: files-backend makes function calls to the frontend, which unconditionally target the main ref store (e.g. resolve_ref_unsafe, delete_ref...). Of course, because store-aware api does not exist. My decision (off-list) to add test-ref-store was the right call. I would not have seen these because I was not (and still am not) familiar with files-backend.c enough to see its dark corners. files-backend.c is not all unicorn and rainbow :( -- Duy
Re: [PATCH v4 14/15] files-backend: remove submodule_allowed from files_downcast()
On 02/20/2017 01:33 PM, Duy Nguyen wrote: > On Mon, Feb 20, 2017 at 7:30 PM, Michael Haggerty > wrote: >> On 02/20/2017 01:21 PM, Duy Nguyen wrote: >>> On Mon, Feb 20, 2017 at 7:11 PM, Michael Haggerty >>> wrote: [...] These limitations, I think, imply that submodule references have to be treated as read-only. >>> >>> Behind the scene submodule does add_submodule_odb(), which basically >>> makes the submodule's odb an alternate of in-core odb. So odb access >>> works. I was puzzled how submodules could by pass odb access at the >>> beginning only to find that out. technical/api-ref-iteration.txt also >>> mentions that you need to add_submodule_odb(), so I think it's >>> deliberate (albeit hacky) design. >> >> That's interesting. I didn't know it before. >> >> But I still don't think that means that reference writing can work >> correctly. If I try to set a submodule branch to an SHA-1 and I verify >> that the SHA-1 points to a valid commit, how do I know that the commit >> is in the same submodule that holds the reference? > > Good point. So will the new flag be "read_only" (no reference to > submodule), or "submodule"? This flag will be passed in at ref-store > init time and kept in files_ref_store. I haven't checked carefully whether there are other operations that don't work and/or don't make sense for submodules. If not, then `read_only` would also be a fine name for the flag. Michael
Re: [PATCH v4 14/15] files-backend: remove submodule_allowed from files_downcast()
On Mon, Feb 20, 2017 at 7:30 PM, Michael Haggerty wrote: > On 02/20/2017 01:21 PM, Duy Nguyen wrote: >> On Mon, Feb 20, 2017 at 7:11 PM, Michael Haggerty >> wrote: >>> On 02/18/2017 02:33 PM, Nguyễn Thái Ngọc Duy wrote: Since submodule or not is irrelevant to files-backend after the last patch, remove the parameter from files_downcast() and kill files_assert_main_repository(). PS. This implies that all ref operations are allowed for submodules. But we may need to look more closely to see if that's really true... >>> >>> I think you are jumping the gun here. >>> >>> Even after your changes, there is still a significant difference between >>> the main repository and submodules: we have access to the object >>> database for the main repository, but not for submodules. >>> >>> So, for example, the following don't work for submodules: >>> >>> * `parse_object()` is used to ensure that references are only pointed at >>> valid objects, and that branches are only pointed at commit objects. >>> >>> * `peel_object()` is used to write the peeled version of references in >>> `packed-refs`, and to peel references while they are being iterated >>> over. Since this doesn't work, I think you can't write `packed-refs` in >>> a submodule. >>> >>> These limitations, I think, imply that submodule references have to be >>> treated as read-only. >> >> Behind the scene submodule does add_submodule_odb(), which basically >> makes the submodule's odb an alternate of in-core odb. So odb access >> works. I was puzzled how submodules could by pass odb access at the >> beginning only to find that out. technical/api-ref-iteration.txt also >> mentions that you need to add_submodule_odb(), so I think it's >> deliberate (albeit hacky) design. > > That's interesting. I didn't know it before. > > But I still don't think that means that reference writing can work > correctly. If I try to set a submodule branch to an SHA-1 and I verify > that the SHA-1 points to a valid commit, how do I know that the commit > is in the same submodule that holds the reference? Good point. So will the new flag be "read_only" (no reference to submodule), or "submodule"? This flag will be passed in at ref-store init time and kept in files_ref_store. -- Duy
Re: [PATCH v4 14/15] files-backend: remove submodule_allowed from files_downcast()
On 02/20/2017 01:21 PM, Duy Nguyen wrote: > On Mon, Feb 20, 2017 at 7:11 PM, Michael Haggerty > wrote: >> On 02/18/2017 02:33 PM, Nguyễn Thái Ngọc Duy wrote: >>> Since submodule or not is irrelevant to files-backend after the last >>> patch, remove the parameter from files_downcast() and kill >>> files_assert_main_repository(). >>> >>> PS. This implies that all ref operations are allowed for submodules. But >>> we may need to look more closely to see if that's really true... >> >> I think you are jumping the gun here. >> >> Even after your changes, there is still a significant difference between >> the main repository and submodules: we have access to the object >> database for the main repository, but not for submodules. >> >> So, for example, the following don't work for submodules: >> >> * `parse_object()` is used to ensure that references are only pointed at >> valid objects, and that branches are only pointed at commit objects. >> >> * `peel_object()` is used to write the peeled version of references in >> `packed-refs`, and to peel references while they are being iterated >> over. Since this doesn't work, I think you can't write `packed-refs` in >> a submodule. >> >> These limitations, I think, imply that submodule references have to be >> treated as read-only. > > Behind the scene submodule does add_submodule_odb(), which basically > makes the submodule's odb an alternate of in-core odb. So odb access > works. I was puzzled how submodules could by pass odb access at the > beginning only to find that out. technical/api-ref-iteration.txt also > mentions that you need to add_submodule_odb(), so I think it's > deliberate (albeit hacky) design. That's interesting. I didn't know it before. But I still don't think that means that reference writing can work correctly. If I try to set a submodule branch to an SHA-1 and I verify that the SHA-1 points to a valid commit, how do I know that the commit is in the same submodule that holds the reference? > [...] Michael
Re: [PATCH v4 14/15] files-backend: remove submodule_allowed from files_downcast()
On Mon, Feb 20, 2017 at 7:11 PM, Michael Haggerty wrote: > On 02/18/2017 02:33 PM, Nguyễn Thái Ngọc Duy wrote: >> Since submodule or not is irrelevant to files-backend after the last >> patch, remove the parameter from files_downcast() and kill >> files_assert_main_repository(). >> >> PS. This implies that all ref operations are allowed for submodules. But >> we may need to look more closely to see if that's really true... > > I think you are jumping the gun here. > > Even after your changes, there is still a significant difference between > the main repository and submodules: we have access to the object > database for the main repository, but not for submodules. > > So, for example, the following don't work for submodules: > > * `parse_object()` is used to ensure that references are only pointed at > valid objects, and that branches are only pointed at commit objects. > > * `peel_object()` is used to write the peeled version of references in > `packed-refs`, and to peel references while they are being iterated > over. Since this doesn't work, I think you can't write `packed-refs` in > a submodule. > > These limitations, I think, imply that submodule references have to be > treated as read-only. Behind the scene submodule does add_submodule_odb(), which basically makes the submodule's odb an alternate of in-core odb. So odb access works. I was puzzled how submodules could by pass odb access at the beginning only to find that out. technical/api-ref-iteration.txt also mentions that you need to add_submodule_odb(), so I think it's deliberate (albeit hacky) design. > When I was working on a patch series similar to yours, I introduced a > boolean "main_repository" flag in `struct ref_store`, and use that > member to implement `files_assert_main_repository()`. That way > `files_ref_store::submodule` can still be removed, which is the more > important goal from a design standpoint. I could keep the submodule check back (and replace the submodule string in files_ref_store with just a flag). But I really think all backend functions work with submodule. Perhaps add some tests to exercise/verify that files-backend-on-submodule works? -- Duy
Re: [PATCH v4 14/15] files-backend: remove submodule_allowed from files_downcast()
On 02/18/2017 02:33 PM, Nguyễn Thái Ngọc Duy wrote: > Since submodule or not is irrelevant to files-backend after the last > patch, remove the parameter from files_downcast() and kill > files_assert_main_repository(). > > PS. This implies that all ref operations are allowed for submodules. But > we may need to look more closely to see if that's really true... I think you are jumping the gun here. Even after your changes, there is still a significant difference between the main repository and submodules: we have access to the object database for the main repository, but not for submodules. So, for example, the following don't work for submodules: * `parse_object()` is used to ensure that references are only pointed at valid objects, and that branches are only pointed at commit objects. * `peel_object()` is used to write the peeled version of references in `packed-refs`, and to peel references while they are being iterated over. Since this doesn't work, I think you can't write `packed-refs` in a submodule. These limitations, I think, imply that submodule references have to be treated as read-only. When I was working on a patch series similar to yours, I introduced a boolean "main_repository" flag in `struct ref_store`, and use that member to implement `files_assert_main_repository()`. That way `files_ref_store::submodule` can still be removed, which is the more important goal from a design standpoint. Michael
[PATCH v4 14/15] files-backend: remove submodule_allowed from files_downcast()
Since submodule or not is irrelevant to files-backend after the last
patch, remove the parameter from files_downcast() and kill
files_assert_main_repository().
PS. This implies that all ref operations are allowed for submodules. But
we may need to look more closely to see if that's really true...
Signed-off-by: Nguyễn Thái Ngọc Duy
---
refs/files-backend.c | 70
1 file changed, 21 insertions(+), 49 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 529c80af1..b8ccf4e47 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1030,24 +1030,13 @@ static struct ref_store *files_ref_store_create(const
char *gitdir)
}
/*
- * Die if refs is for a submodule (i.e., not for the main repository).
- * caller is used in any necessary error messages.
- */
-static void files_assert_main_repository(struct files_ref_store *refs,
-const char *caller)
-{
- /* This function is to be deleted in the next patch */
-}
-
-/*
* Downcast ref_store to files_ref_store. Die if ref_store is not a
* files_ref_store. If submodule_allowed is not true, then also die if
* files_ref_store is for a submodule (i.e., not for the main
* repository). caller is used in any necessary error messages.
*/
-static struct files_ref_store *files_downcast(
- struct ref_store *ref_store, int submodule_allowed,
- const char *caller)
+static struct files_ref_store *files_downcast(struct ref_store *ref_store,
+ const char *caller)
{
struct files_ref_store *refs;
@@ -1057,9 +1046,6 @@ static struct files_ref_store *files_downcast(
refs = (struct files_ref_store *)ref_store;
- if (!submodule_allowed)
- files_assert_main_repository(refs, caller);
-
return refs;
}
@@ -1395,7 +1381,7 @@ static int files_read_raw_ref(struct ref_store *ref_store,
struct strbuf *referent, unsigned int *type)
{
struct files_ref_store *refs =
- files_downcast(ref_store, 1, "read_raw_ref");
+ files_downcast(ref_store, "read_raw_ref");
struct strbuf sb_contents = STRBUF_INIT;
struct strbuf sb_path = STRBUF_INIT;
const char *path;
@@ -1588,7 +1574,6 @@ static int lock_raw_ref(struct files_ref_store *refs,
int ret = TRANSACTION_GENERIC_ERROR;
assert(err);
- files_assert_main_repository(refs, "lock_raw_ref");
*type = 0;
@@ -1812,7 +1797,7 @@ static enum peel_status peel_entry(struct ref_entry
*entry, int repeel)
static int files_peel_ref(struct ref_store *ref_store,
const char *refname, unsigned char *sha1)
{
- struct files_ref_store *refs = files_downcast(ref_store, 0, "peel_ref");
+ struct files_ref_store *refs = files_downcast(ref_store, "peel_ref");
int flag;
unsigned char base[20];
@@ -1921,7 +1906,7 @@ static struct ref_iterator *files_ref_iterator_begin(
const char *prefix, unsigned int flags)
{
struct files_ref_store *refs =
- files_downcast(ref_store, 1, "ref_iterator_begin");
+ files_downcast(ref_store, "ref_iterator_begin");
struct ref_dir *loose_dir, *packed_dir;
struct ref_iterator *loose_iter, *packed_iter;
struct files_ref_iterator *iter;
@@ -2059,7 +2044,6 @@ static struct ref_lock *lock_ref_sha1_basic(struct
files_ref_store *refs,
int resolve_flags = RESOLVE_REF_NO_RECURSE;
int resolved;
- files_assert_main_repository(refs, "lock_ref_sha1_basic");
assert(err);
lock = xcalloc(1, sizeof(struct ref_lock));
@@ -2184,8 +2168,6 @@ static int lock_packed_refs(struct files_ref_store *refs,
int flags)
struct strbuf sb = STRBUF_INIT;
int ret;
- files_assert_main_repository(refs, "lock_packed_refs");
-
if (!timeout_configured) {
git_config_get_int("core.packedrefstimeout", &timeout_value);
timeout_configured = 1;
@@ -2225,8 +2207,6 @@ static int commit_packed_refs(struct files_ref_store
*refs)
int save_errno = 0;
FILE *out;
- files_assert_main_repository(refs, "commit_packed_refs");
-
if (!packed_ref_cache->lock)
die("internal error: packed-refs not locked");
@@ -2258,8 +2238,6 @@ static void rollback_packed_refs(struct files_ref_store
*refs)
struct packed_ref_cache *packed_ref_cache =
get_packed_ref_cache(refs);
- files_assert_main_repository(refs, "rollback_packed_refs");
-
if (!packed_ref_cache->lock)
die("internal error: packed-refs not locked");
rollback_lock_file(packed_ref_cache->lock);
@@ -2420,7 +2398,7 @@ static void prune_refs(struct files_ref_store *refs,
struct ref_to_prune *r)
static int files_pack_refs(struct ref_stor
