| Message ID | 0e732d30b516855a7e91240a9055712b26905b2e.1589226388.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Reftable support git-core | expand |
"Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes: > diff --git a/builtin/init-db.c b/builtin/init-db.c > index b7053b9e370..da5b4670c84 100644 > --- a/builtin/init-db.c > +++ b/builtin/init-db.c > @@ -545,7 +545,7 @@ static const char *const init_db_usage[] = { > int cmd_init_db(int argc, const char **argv, const char *prefix) > { > const char *git_dir; > - const char *ref_storage_format = DEFAULT_REF_STORAGE; > + const char *ref_storage_format = default_ref_storage(); > const char *real_git_dir = NULL; > const char *work_tree; > const char *template_dir = NULL; > diff --git a/refs.c b/refs.c > index e8751415a9e..70c11b05391 100644 > --- a/refs.c > +++ b/refs.c > @@ -1742,7 +1742,7 @@ struct ref_store *get_main_ref_store(struct repository *r) > r->refs_private = ref_store_init(r->gitdir, > r->ref_storage_format ? > r->ref_storage_format : > - DEFAULT_REF_STORAGE, > + default_ref_storage(), > REF_STORE_ALL_CAPS); > return r->refs_private; > } Would it make sense to let NULL stand for "we use the default, whatever it is" so that anybody outside the implementation of the refs API does not have to care what the default is, and make ref_store_init() function the only thing that knows what it is?
On Wed, May 13, 2020 at 9:57 PM Junio C Hamano <gitster@pobox.com> wrote: > > "Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > diff --git a/builtin/init-db.c b/builtin/init-db.c > > index b7053b9e370..da5b4670c84 100644 > > --- a/builtin/init-db.c > > +++ b/builtin/init-db.c > > @@ -545,7 +545,7 @@ static const char *const init_db_usage[] = { > > int cmd_init_db(int argc, const char **argv, const char *prefix) > > { > > const char *git_dir; > > - const char *ref_storage_format = DEFAULT_REF_STORAGE; > > + const char *ref_storage_format = default_ref_storage(); > > const char *real_git_dir = NULL; > > const char *work_tree; > > const char *template_dir = NULL; > > diff --git a/refs.c b/refs.c > > index e8751415a9e..70c11b05391 100644 > > --- a/refs.c > > +++ b/refs.c > > @@ -1742,7 +1742,7 @@ struct ref_store *get_main_ref_store(struct repository *r) > > r->refs_private = ref_store_init(r->gitdir, > > r->ref_storage_format ? > > r->ref_storage_format : > > - DEFAULT_REF_STORAGE, > > + default_ref_storage(), > > REF_STORE_ALL_CAPS); > > return r->refs_private; > > } > > Would it make sense to let NULL stand for "we use the default, > whatever it is" so that anybody outside the implementation of the > refs API does not have to care what the default is, and make > ref_store_init() function the only thing that knows what it is? Is this your strong recommendation in the form of a question? :) If it is just a question: maybe. I don't know enough of Git to say for sure. How do submodules decide on other settings, such as the hash ID?
Han-Wen Nienhuys <hanwen@google.com> writes: > On Wed, May 13, 2020 at 9:57 PM Junio C Hamano <gitster@pobox.com> wrote: >> >> > --- a/refs.c >> > +++ b/refs.c >> > @@ -1742,7 +1742,7 @@ struct ref_store *get_main_ref_store(struct repository *r) >> > r->refs_private = ref_store_init(r->gitdir, >> > r->ref_storage_format ? >> > r->ref_storage_format : >> > - DEFAULT_REF_STORAGE, >> > + default_ref_storage(), >> > REF_STORE_ALL_CAPS); >> > return r->refs_private; >> > } >> >> Would it make sense to let NULL stand for "we use the default, >> whatever it is" so that anybody outside the implementation of the >> refs API does not have to care what the default is, and make >> ref_store_init() function the only thing that knows what it is? > > Is this your strong recommendation in the form of a question? :) > > If it is just a question: maybe. I don't know enough of Git to say for sure. I try not to ask a rhetorical question without marking it explicitly as such. The question "instead of having to sprinkle ?: all over the place on the callers' side, would it make it better or worse to give a 'not decided' default to the ref_storage_format field?" came to mind while reading the series. > How do submodules decide on other settings, such as the hash ID? I am not sure the issue of "what default to use" is limited to the submodule case (e.g. "git init" and "git clone" starts from having no "current repository" and they need to make the decision among available choices). As I've seen this series and SHA-256 series conflict in the "init" area, it probably is time for me to introduce you to Brian (cc'ed) to pick his brain ;-). Thanks.
diff --git a/builtin/clone.c b/builtin/clone.c index 4d0cf065e4a..780c5807415 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -1109,7 +1109,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix) } init_db(git_dir, real_git_dir, option_template, GIT_HASH_UNKNOWN, - DEFAULT_REF_STORAGE, INIT_DB_QUIET); + default_ref_storage(), INIT_DB_QUIET); if (real_git_dir) git_dir = real_git_dir; diff --git a/builtin/init-db.c b/builtin/init-db.c index b7053b9e370..da5b4670c84 100644 --- a/builtin/init-db.c +++ b/builtin/init-db.c @@ -545,7 +545,7 @@ static const char *const init_db_usage[] = { int cmd_init_db(int argc, const char **argv, const char *prefix) { const char *git_dir; - const char *ref_storage_format = DEFAULT_REF_STORAGE; + const char *ref_storage_format = default_ref_storage(); const char *real_git_dir = NULL; const char *work_tree; const char *template_dir = NULL; diff --git a/refs.c b/refs.c index e8751415a9e..70c11b05391 100644 --- a/refs.c +++ b/refs.c @@ -1742,7 +1742,7 @@ struct ref_store *get_main_ref_store(struct repository *r) r->refs_private = ref_store_init(r->gitdir, r->ref_storage_format ? r->ref_storage_format : - DEFAULT_REF_STORAGE, + default_ref_storage(), REF_STORE_ALL_CAPS); return r->refs_private; } @@ -1798,7 +1798,7 @@ struct ref_store *get_submodule_ref_store(const char *submodule) goto done; /* assume that add_submodule_odb() has been called */ - refs = ref_store_init(submodule_sb.buf, DEFAULT_REF_STORAGE, /* XXX */ + refs = ref_store_init(submodule_sb.buf, default_ref_storage(), REF_STORE_READ | REF_STORE_ODB); register_ref_store_map(&submodule_ref_stores, "submodule", refs, submodule); @@ -1812,7 +1812,7 @@ struct ref_store *get_submodule_ref_store(const char *submodule) struct ref_store *get_worktree_ref_store(const struct worktree *wt) { - const char *format = DEFAULT_REF_STORAGE; /* XXX */ + const char *format = default_ref_storage(); struct ref_store *refs; const char *id; diff --git a/t/t1409-avoid-packing-refs.sh b/t/t1409-avoid-packing-refs.sh index be12fb63506..c6f78325563 100755 --- a/t/t1409-avoid-packing-refs.sh +++ b/t/t1409-avoid-packing-refs.sh @@ -4,6 +4,12 @@ test_description='avoid rewriting packed-refs unnecessarily' . ./test-lib.sh +if test_have_prereq REFTABLE +then + skip_all='skipping pack-refs tests; incompatible with reftable' + test_done +fi + # Add an identifying mark to the packed-refs file header line. This # shouldn't upset readers, and it should be omitted if the file is # ever rewritten. diff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh index f41b2afb996..edaef2c175a 100755 --- a/t/t3210-pack-refs.sh +++ b/t/t3210-pack-refs.sh @@ -11,6 +11,12 @@ semantic is still the same. ' . ./test-lib.sh +if test_have_prereq REFTABLE +then + skip_all='skipping pack-refs tests; incompatible with reftable' + test_done +fi + test_expect_success 'enable reflogs' ' git config core.logallrefupdates true ' diff --git a/t/test-lib.sh b/t/test-lib.sh index baf94546da1..21740e2c13d 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -1516,6 +1516,11 @@ FreeBSD) ;; esac +if test -n "$GIT_TEST_REFTABLE" +then + test_set_prereq REFTABLE +fi + ( COLUMNS=1 && test $COLUMNS = 1 ) && test_set_prereq COLUMNS_CAN_BE_1 test -z "$NO_PERL" && test_set_prereq PERL test -z "$NO_PTHREADS" && test_set_prereq PTHREADS