{"thread":{"id":"54721","subject":"[PATCH] init-db: init the_repository->hash_algo early from GIT_DEFAULT_HASH","startedAt":"2020-11-26T19:42:53Z","lastAt":"2020-12-07T19:32:51Z","messageCount":6,"participants":["Han-Wen Nienhuys via GitGitGadget","Junio C Hamano","Han-Wen Nienhuys"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"410882","messageId":"pull.924.git.git.1606419582190.gitgitgadget@gmail.com","threadId":"54721","inReplyTo":null,"subject":"[PATCH] init-db: init the_repository->hash_algo early from GIT_DEFAULT_HASH","fromName":"Han-Wen Nienhuys via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-26T19:39:41Z","receivedAt":"2020-11-26T19:42:53Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"From: Han-Wen Nienhuys <hanwen@google.com>\n\nThe reftable backend needs to know the hash algorithm for writing the\ninitialization hash table.\n\nThe initial reftable contains a symref HEAD => \"main\" (or \"master\"), which is\nagnostic to the size of hash value, but this is an exceptional circumstance, and\nthe reftable library does not cater for this exception. It insists that all\ntables in the stack have a consistent format ID for the hash algorithm.\n\nCall set_repo_hash_algo directly after reading out GIT_DEFAULT_HASH.\n\nSigned-off-by: Han-Wen Nienhuys <hanwen@google.com>\n---\n    init-db: init the_repository->hash_algo early from GIT_DEFAULT_HASH\n    \n    The reftable backend needs to know the hash algorithm for writing the\n    initialization hash table.\n    \n    The initial reftable contains a symref HEAD => \"main\" (or \"master\"),\n    which is agnostic to the size of hash value, but this is an exceptional\n    circumstance, and the reftable library does not cater for this\n    exception. It insists that all tables in the stack have a consistent\n    format ID for the hash algorithm.\n    \n    Call set_repo_hash_algo directly after reading out GIT_DEFAULT_HASH.\n    \n    Signed-off-by: Han-Wen Nienhuys hanwen@google.com [hanwen@google.com]\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-924%2Fhanwen%2Fset-hash-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-924/hanwen/set-hash-v1\nPull-Request: https://github.com/git/git/pull/924\n\n builtin/init-db.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/init-db.c b/builtin/init-db.c\nindex 01bc648d41..5c8c67fec6 100644\n--- a/builtin/init-db.c\n+++ b/builtin/init-db.c\n@@ -391,6 +391,7 @@ static void validate_hash_algorithm(struct repository_format *repo_fmt, int hash\n \t\t\tdie(_(\"unknown hash algorithm '%s'\"), env);\n \t\trepo_fmt->hash_algo = env_algo;\n \t}\n+\trepo_set_hash_algo(the_repository, repo_fmt->hash_algo);\n }\n \n int init_db(const char *git_dir, const char *real_git_dir,\n\nbase-commit: faefdd61ec7c7f6f3c8c9907891465ac9a2a1475\n-- \ngitgitgadget\n"},{"id":"411037","messageId":"xmqq5z5mruia.fsf@gitster.c.googlers.com","threadId":"54721","inReplyTo":"pull.924.git.git.1606419582190.gitgitgadget@gmail.com","subject":"Re: [PATCH] init-db: init the_repository->hash_algo early from GIT_DEFAULT_HASH","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-01T04:59:57Z","receivedAt":"2020-12-01T05:01:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Han-Wen Nienhuys via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Han-Wen Nienhuys <hanwen@google.com>\n>\n> The reftable backend needs to know the hash algorithm for writing the\n> initialization hash table.\n>\n> The initial reftable contains a symref HEAD => \"main\" (or \"master\"), which is\n> agnostic to the size of hash value, but this is an exceptional circumstance, and\n> the reftable library does not cater for this exception. It insists that all\n> tables in the stack have a consistent format ID for the hash algorithm.\n>\n> Call set_repo_hash_algo directly after reading out GIT_DEFAULT_HASH.\n\nSeeing that there is no mention of the_repository in the entire\ninit-db.c file, that the same information is in repo_fmt which is\npassed to the init-db.c::create_default_files() function, and that\ncreate_default_files() is where the initial set of refs is prepared\nin the current system, it is not clear why this patch is needed\n(i.e. why we consider the current code that has no mention of\nthe_repository is wrong).  Isn't it the matter of passing the hash\ntaken from repo_fmt to the refs API to initialize that part of the\nrepository, instead of relying on half-initialized state in\nthe_repository?  It's not like only the hash_algo member is yet to\nbe prepared in the_repository instance at that point in the code.\nMost of the members are, except for a very few fields initialized by\ninitialize_the_repository(), not filled in the codepath, no?\n\nSo, this might have been the most convenient way to pass hash_algo\ndown but the patch does not convince me that it is the best way.  It\nmay be, but it does not answer questions like \"what makes us sure\nthat hash-algo will stay to be the only thing we need to fill early\nin this codepath?\"\n\nIf the patch _were_ to fill many other members of the_repository to\nmake us pretend as if we did the setup_git_dir() in the repository\nwe just created, and the hash_algo is filled merely as part of it,\nthen it would have been much more convincing that we are moving in\nthe right direction.  It would for example mean gitdir/commondir\nwould be filled among other members, and the existing \"mkdir()\" to\nmanually prepare \"refs/\" for files backend in init-db.c can probably\nbe removed and instead would be done by calling a new entry-point\nthat initializes the initial set of refs in the refs API.  And that\nentry-point would either rely on the_repository or take a repository\ninstance, and use hash_algo member of that structure to do its thing.\n\nSo, I dunno.\n\n> diff --git a/builtin/init-db.c b/builtin/init-db.c\n> index 01bc648d41..5c8c67fec6 100644\n> --- a/builtin/init-db.c\n> +++ b/builtin/init-db.c\n> @@ -391,6 +391,7 @@ static void validate_hash_algorithm(struct repository_format *repo_fmt, int hash\n>  \t\t\tdie(_(\"unknown hash algorithm '%s'\"), env);\n>  \t\trepo_fmt->hash_algo = env_algo;\n>  \t}\n> +\trepo_set_hash_algo(the_repository, repo_fmt->hash_algo);\n>  }\n>  \n>  int init_db(const char *git_dir, const char *real_git_dir,\n>\n> base-commit: faefdd61ec7c7f6f3c8c9907891465ac9a2a1475\n"},{"id":"411279","messageId":"CAFQ2z_M3OO_nR6dhw6zzE0orYxcawP1DaJ_EOL5=+RUiZgCo8w@mail.gmail.com","threadId":"54721","inReplyTo":"xmqq5z5mruia.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] init-db: init the_repository->hash_algo early from GIT_DEFAULT_HASH","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2020-12-03T20:23:36Z","receivedAt":"2020-12-03T20:24:34Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Tue, Dec 1, 2020 at 6:00 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> \"Han-Wen Nienhuys via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>\n> > From: Han-Wen Nienhuys <hanwen@google.com>\n> >\n> > The reftable backend needs to know the hash algorithm for writing the\n> > initialization hash table.\n> >\n> > The initial reftable contains a symref HEAD => \"main\" (or \"master\"), which is\n> > agnostic to the size of hash value, but this is an exceptional circumstance, and\n> > the reftable library does not cater for this exception. It insists that all\n> > tables in the stack have a consistent format ID for the hash algorithm.\n> >\n> > Call set_repo_hash_algo directly after reading out GIT_DEFAULT_HASH.\n>\n> Seeing that there is no mention of the_repository in the entire\n> init-db.c file,\n\nit's not because it's hidden in the call to refs_init_db().\nrefs_init_db accesses the_repository global variable, because\nget_main_ref_store() takes a repository (and not repository_format)\nargument.\n\n> Isn't it the matter of passing the hash\n> taken from repo_fmt to the refs API to initialize that part of the\n> repository, instead of relying on half-initialized state in\n> the_repository?\n\nThe refstore is created in get_main_ref_store(). I can't add a\nrepository_format argument there, because it has a lot of call sites\nwhere the repository_format isn't available.\n\nIf the_repository is only half-initialized at this point in init_db(),\nthen why are we passing it in refs_init_db() just a couple of lines\nfurther? At what point the_repository considered initialized?\n\n> It's not like only the hash_algo member is yet to\n> be prepared in the_repository instance at that point in the code.\n> Most of the members are, except for a very few fields initialized by\n> initialize_the_repository(), not filled in the codepath, no?\n>\n> So, this might have been the most convenient way to pass hash_algo\n> down but the patch does not convince me that it is the best way.\n\nI'm a bit at a loss here; I never learned how to cleanly work with so\nmany global variables, so I'm happy to take your suggestion.\n\n-- \nHan-Wen Nienhuys - Google Munich\nI work 80%. Don't expect answers from me on Fridays.\n--\n\nGoogle Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich\n\nRegistergericht und -nummer: Hamburg, HRB 86891\n\nSitz der Gesellschaft: Hamburg\n\nGeschäftsführer: Paul Manicle, Halimah DeLaine Prado\n"},{"id":"411288","messageId":"xmqqtut2fp5z.fsf@gitster.c.googlers.com","threadId":"54721","inReplyTo":"CAFQ2z_M3OO_nR6dhw6zzE0orYxcawP1DaJ_EOL5=+RUiZgCo8w@mail.gmail.com","subject":"Re: [PATCH] init-db: init the_repository->hash_algo early from GIT_DEFAULT_HASH","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-03T23:25:12Z","receivedAt":"2020-12-03T23:26:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Han-Wen Nienhuys <hanwen@google.com> writes:\n\n> If the_repository is only half-initialized at this point in init_db(),\n> then why are we passing it in refs_init_db() just a couple of lines\n> further? At what point the_repository considered initialized?\n\nI would have to say it probably depends on what callees expect.  The\ncurrent implementation of refs_init_db() for files backend may not\nneed anything other than the hash algorithm enum, but many other\nfields are missing, and they should ideally be populated, no?  \n\nFor example, I see files_ref_store_create() cheats by calling\nget_common_dir_noenv() to find out where the commondir is, instead\nof ever asking the repository the ref store belongs to. At least,\nget_main_ref_store() is told to get the ref store that belongs to\nthe_repository, and it would be the right place to learn relevant\npieces of information (for that matter, I am not sure why struct\nref_store does not have a pointer to a repository structure; perhaps\nwe are seeing the result of piecemeal evolution, not a designed\nstructure?).\n\n> I'm a bit at a loss here; I never learned how to cleanly work with so\n> many global variables, so I'm happy to take your suggestion.\n\nI am only interested in giving a clear direction to future\ndevelopers where to populate the_repository's members (and nothing\nelse) if their enhancement needs members other than the hash\nalgorithm to be populated, as if it were the_repository initialized\nin an already working repository (I am not talking about many global\nvariables, whichever you are referring to).\n\nOne way to do so would probably be to do something like the\nattached.\n\nThe patch that started this thread (or the equivalent one in the\nupdated reftable series) may want to initialize a bit more members\nwhile at it (looking at how commondir from the_repository is not\nused by the refs/files-backend.c::files_init_db() to decide where\nthe function creates the refs/heads and refs/tags directories, we\nprobably would need to populate the_repository->commondir before\nthat codepath can be fixed to look at the member, for example).\n\n\n builtin/init-db.c | 21 +++++++++++++++++++++\n 1 file changed, 21 insertions(+)\n\ndiff --git c/builtin/init-db.c w/builtin/init-db.c\nindex dcc45bef51..2b5c94596f 100644\n--- c/builtin/init-db.c\n+++ w/builtin/init-db.c\n@@ -438,6 +438,27 @@ int init_db(const char *git_dir, const char *real_git_dir,\n \n \tvalidate_hash_algorithm(&repo_fmt, hash);\n \n+\t/*\n+\t * At this point, the_repository we have in-core does not look\n+\t * anything like one that we would see initialized in an already\n+\t * working repository after calling setup_git_directory().\n+\t *\n+\t * Calling repository.c::initialize_the_repository() may have\n+\t * prepared the .index .objects and .parsed_objects members, but\n+\t * other members like .gitdir, .commondir, etc. have not been\n+\t * initialized.\n+\t *\n+\t * Many API functions assume they are working with the_repository\n+\t * that has sensibly been initialized, but because we haven't\n+\t * really read from an existing repository, we need to hand-craft\n+\t * the necessary members of the structure to get out of this\n+\t * chicken-and-egg situation.\n+\t *\n+\t * For now, we update the hash algorithm member to what the\n+\t * validate_hash_algorithm() call decided for us.\n+\t */\n+\trepo_set_hash_algo(the_repository, repo_fmt->hash_algo);\n+\n \treinit = create_default_files(template_dir, original_git_dir,\n \t\t\t\t      initial_branch, &repo_fmt,\n \t\t\t\t      flags & INIT_DB_QUIET);\n\n\n"},{"id":"411599","messageId":"CAFQ2z_OsyseuPWxpj9S4R_Pp_Koa0JuJeO2gte+TLxDh+oHPNg@mail.gmail.com","threadId":"54721","inReplyTo":"xmqqtut2fp5z.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] init-db: init the_repository->hash_algo early from GIT_DEFAULT_HASH","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2020-12-07T11:52:11Z","receivedAt":"2020-12-07T11:53:23Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Fri, Dec 4, 2020 at 12:25 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Han-Wen Nienhuys <hanwen@google.com> writes:\n>\n> > If the_repository is only half-initialized at this point in init_db(),\n> > then why are we passing it in refs_init_db() just a couple of lines\n> > further? At what point the_repository considered initialized?\n>\n> I would have to say it probably depends on what callees expect.  The\n> current implementation of refs_init_db() for files backend may not\n> need anything other than the hash algorithm enum, but many other\n> fields are missing, and they should ideally be populated, no?\n\nI think so too, but it looks like a major refactoring by itself, which\nis only very tangentially related to the reftable effort, so I'd like\nto punt on it.\n\n> For example, I see files_ref_store_create() cheats by calling\n> get_common_dir_noenv() to find out where the commondir is, instead\n> of ever asking the repository the ref store belongs to. At least,\n> get_main_ref_store() is told to get the ref store that belongs to\n> the_repository, and it would be the right place to learn relevant\n> pieces of information (for that matter, I am not sure why struct\n> ref_store does not have a pointer to a repository structure; perhaps\n> we are seeing the result of piecemeal evolution, not a designed\n> structure?).\n>\n> > I'm a bit at a loss here; I never learned how to cleanly work with so\n> > many global variables, so I'm happy to take your suggestion.\n>\n> I am only interested in giving a clear direction to future\n> developers where to populate the_repository's members (and nothing\n> else) if their enhancement needs members other than the hash\n> algorithm to be populated, as if it were the_repository initialized\n> in an already working repository (I am not talking about many global\n> variables, whichever you are referring to).\n\nYou said earlier that maybe ref_store should hold a link to 'struct\nrepository'. However, \"struct repository\" doesn't have a documented\npurpose (there is literally no comment documenting its purpose), so\nit's hard to tell upfront how to correctly configure the relation\nbetween \"struct ref_store\" and \"struct repository\". Currently, \"struct\nrepository\" has a pointer to 'struct ref_store\", which makes it\nsuspect for there to be a pointer in the other direction as well.\n\nThe reason I bring up global variables is that without them, the code\nwould enforce a much more clear relation between different entities.\n\nLooking at the definition of repository, there seems to be some\noverlap in functionality between struct repository, struct\nrepository_format, struct repo_settings, and config_set, and none of\nthese structs have a clearly documented role.   Before we change code,\nit's probably better to spell out what these structs are thought to\nbe.\n\n> One way to do so would probably be to do something like the\n> attached.\n\nThanks, I've amended my patch.\n\n-- \nHan-Wen Nienhuys - Google Munich\nI work 80%. Don't expect answers from me on Fridays.\n--\n\nGoogle Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich\n\nRegistergericht und -nummer: Hamburg, HRB 86891\n\nSitz der Gesellschaft: Hamburg\n\nGeschäftsführer: Paul Manicle, Halimah DeLaine Prado\n"},{"id":"411638","messageId":"xmqqy2i99zvc.fsf@gitster.c.googlers.com","threadId":"54721","inReplyTo":"CAFQ2z_OsyseuPWxpj9S4R_Pp_Koa0JuJeO2gte+TLxDh+oHPNg@mail.gmail.com","subject":"Re: [PATCH] init-db: init the_repository->hash_algo early from GIT_DEFAULT_HASH","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-07T19:31:51Z","receivedAt":"2020-12-07T19:32:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Han-Wen Nienhuys <hanwen@google.com> writes:\n\n> You said earlier that maybe ref_store should hold a link to 'struct\n> repository'. However, \"struct repository\" doesn't have a documented\n> purpose (there is literally no comment documenting its purpose), so\n> it's hard to tell upfront how to correctly configure the relation\n> between \"struct ref_store\" and \"struct repository\".\n\nFolks, who have been pushing to recurse into submodules in-process,\ndoes any of you care to help out?\n\nIn order to deal with separate repositories inside a process,\ninstead of allowing many API functions to assume that they only have\nto work in the repository \"git\" started in, you need to be able to\nmake them to work on a different repository, which would have\ndifferent configuration files, with working tree locations that are\ndifferent from that of the initial repository, and certainly would\nhave different set of refs, etc., and the way to do so is by passing\nan instance of the repository structure.  To the API functions that\nhave been converted to take such an instance, working in the\nrepository \"git\" started in is done by working with the_repository\ninstance.\n\n\n"}]}