{"thread":{"id":"49400","subject":"[PATCHv3 0/8] fetch: make sure submodule oids are fetched","startedAt":"2018-09-21T22:36:11Z","lastAt":"2018-09-29T07:46:07Z","messageCount":51,"participants":["Stefan Beller","Ævar Arnfjörð Bjarmason","Jeff King","Junio C Hamano","Jonathan Nieder","Martin Ågren"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"358672","messageId":"20180921223558.65055-1-sbeller@google.com","threadId":"49400","inReplyTo":null,"subject":"[PATCHv3 0/8] fetch: make sure submodule oids are fetched","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-21T22:35:50Z","receivedAt":"2018-09-21T22:36:11Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"v3:\n* I discovered some issues with v2 after sending,\n which is why I rewrote the later patches completely\n and now we pass around a \"task\" struct that contains everything to know\n about the things to work on and what needs free()ing afterwards.\n* as it is no longer string list based, this drops adding string_list_{pop, last}\n\nv2:\n* extended commit messages,\n* plugged a memory leak\n* rewrote the patch \"sha1-array: provide oid_array_filter\" to be much more like \n object_array_fiter\n* fixed a typo pointed out by Ramsay.\n\nThe range diff is below.\n \nThanks,\nStefan\n\nv1:\nCurrently when git-fetch is asked to recurse into submodules, it dispatches\na plain \"git-fetch -C \" (and some submodule related options\nsuch as prefix and recusing strategy, but) without any information of the\nremote or the tip that should be fetched.\n\nThis works surprisingly well in some workflows, not so well in others,\nwhich this series aims to fix.\n\nThe first patches provide new basic functionality and do some refactoring;\nthe interesting part is in the two last patches.\n\nThis was discussed in\nhttps://public-inbox.org/git/20180808221752.195419-1-sbeller@google.com/\nand I think I addressed all feedback so far.\n\nStefan Beller (8):\n sha1-array: provide oid_array_filter\n submodule.c: fix indentation\n submodule.c: sort changed_submodule_names before searching it\n submodule: move global changed_submodule_names into fetch submodule\n struct\n submodule.c: do not copy around submodule list\n submodule: fetch in submodules git directory instead of in worktree\n fetch: retry fetching submodules if needed objects were not fetched\n builtin/fetch: check for submodule updates for non branch fetches\n\n builtin/fetch.c | 14 +-\n sha1-array.c | 17 +++\n sha1-array.h | 9 ++\n submodule.c | 268 ++++++++++++++++++++++++++++--------\n t/t5526-fetch-submodules.sh | 23 +++-\n 5 files changed, 268 insertions(+), 63 deletions(-)\n\n-- \n2.19.0.444.g18242da7ef-goog\n\n"},{"id":"358673","messageId":"20180921223558.65055-2-sbeller@google.com","threadId":"49400","inReplyTo":"20180921223558.65055-1-sbeller@google.com","subject":"[PATCH 1/8] sha1-array: provide oid_array_filter","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-21T22:35:51Z","receivedAt":"2018-09-21T22:36:19Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Helped-by: Junio C Hamano \nSigned-off-by: Stefan Beller \n---\n sha1-array.c | 17 +++++++++++++++++\n sha1-array.h | 9 +++++++++\n 2 files changed, 26 insertions(+)\n\ndiff --git a/sha1-array.c b/sha1-array.c\nindex b94e0ec0f5e..d922e94e3fc 100644\n--- a/sha1-array.c\n+++ b/sha1-array.c\n@@ -77,3 +77,20 @@ int oid_array_for_each_unique(struct oid_array *array,\n \t}\n \treturn 0;\n }\n+\n+void oid_array_filter(struct oid_array *array,\n+\t\t for_each_oid_fn want,\n+\t\t void *cb_data)\n+{\n+\tunsigned nr = array->nr, src, dst;\n+\tstruct object_id *oids = array->oid;\n+\n+\tfor (src = dst = 0; src < nr; src++) {\n+\t\tif (want(&oids[src], cb_data)) {\n+\t\t\tif (src != dst)\n+\t\t\t\toidcpy(&oids[dst], &oids[src]);\n+\t\t\tdst++;\n+\t\t}\n+\t}\n+\tarray->nr = dst;\n+}\ndiff --git a/sha1-array.h b/sha1-array.h\nindex 232bf950172..275e5b02f5e 100644\n--- a/sha1-array.h\n+++ b/sha1-array.h\n@@ -23,4 +23,13 @@ int oid_array_for_each_unique(struct oid_array *array,\n \t\t\t for_each_oid_fn fn,\n \t\t\t void *data);\n \n+/*\n+ * Apply want to each entry in array, retaining only the entries for\n+ * which the function returns true. Preserve the order of the entries\n+ * that are retained.\n+ */\n+void oid_array_filter(struct oid_array *array,\n+\t\t for_each_oid_fn want,\n+\t\t void *cbdata);\n+\n #endif /* SHA1_ARRAY_H */\n-- \n2.19.0.444.g18242da7ef-goog\n\n"},{"id":"358674","messageId":"20180921223558.65055-3-sbeller@google.com","threadId":"49400","inReplyTo":"20180921223558.65055-1-sbeller@google.com","subject":"[PATCH 2/8] submodule.c: fix indentation","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-21T22:35:52Z","receivedAt":"2018-09-21T22:36:27Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The submodule subsystem is really bad at staying within 80 characters.\nFix it while we are here.\n\nSigned-off-by: Stefan Beller \n---\n submodule.c | 9 ++++++---\n 1 file changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex ed05339b588..67469a8f513 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1245,7 +1245,8 @@ static int get_next_submodule(struct child_process *cp,\n \t\tif (!submodule) {\n \t\t\tconst char *name = default_name_or_path(ce->name);\n \t\t\tif (name) {\n-\t\t\t\tdefault_submodule.path = default_submodule.name = name;\n+\t\t\t\tdefault_submodule.path = name;\n+\t\t\t\tdefault_submodule.name = name;\n \t\t\t\tsubmodule = &default_submodule;\n \t\t\t}\n \t\t}\n@@ -1255,8 +1256,10 @@ static int get_next_submodule(struct child_process *cp,\n \t\tdefault:\n \t\tcase RECURSE_SUBMODULES_DEFAULT:\n \t\tcase RECURSE_SUBMODULES_ON_DEMAND:\n-\t\t\tif (!submodule || !unsorted_string_list_lookup(&changed_submodule_names,\n-\t\t\t\t\t\t\t submodule->name))\n+\t\t\tif (!submodule ||\n+\t\t\t !unsorted_string_list_lookup(\n+\t\t\t\t\t&changed_submodule_names,\n+\t\t\t\t\tsubmodule->name))\n \t\t\t\tcontinue;\n \t\t\tdefault_argv = \"on-demand\";\n \t\t\tbreak;\n-- \n2.19.0.444.g18242da7ef-goog\n\n"},{"id":"358675","messageId":"20180921223558.65055-4-sbeller@google.com","threadId":"49400","inReplyTo":"20180921223558.65055-1-sbeller@google.com","subject":"[PATCH 3/8] submodule.c: sort changed_submodule_names before searching it","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-21T22:35:53Z","receivedAt":"2018-09-21T22:36:35Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"We can string_list_insert() to maintain sorted-ness of the\nlist as we find new items, or we can string_list_append() to\nbuild an unsorted list and sort it at the end just once.\n\nTo pick which one is more appropriate, we notice the fact\nthat we discover new items more or less in the already\nsorted order. That makes \"append then sort\" more\nappropriate.\n\nSigned-off-by: Stefan Beller \n---\n submodule.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 67469a8f513..2f5c19d0aac 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1257,7 +1257,7 @@ static int get_next_submodule(struct child_process *cp,\n \t\tcase RECURSE_SUBMODULES_DEFAULT:\n \t\tcase RECURSE_SUBMODULES_ON_DEMAND:\n \t\t\tif (!submodule ||\n-\t\t\t !unsorted_string_list_lookup(\n+\t\t\t !string_list_lookup(\n \t\t\t\t\t&changed_submodule_names,\n \t\t\t\t\tsubmodule->name))\n \t\t\t\tcontinue;\n@@ -1351,6 +1351,7 @@ int fetch_populated_submodules(struct repository *r,\n \t/* default value, \"--submodule-prefix\" and its value are added later */\n \n \tcalculate_changed_submodule_paths();\n+\tstring_list_sort(&changed_submodule_names);\n \trun_processes_parallel(max_parallel_jobs,\n \t\t\t get_next_submodule,\n \t\t\t fetch_start_failure,\n-- \n2.19.0.444.g18242da7ef-goog\n\n"},{"id":"358676","messageId":"20180921223558.65055-5-sbeller@google.com","threadId":"49400","inReplyTo":"20180921223558.65055-1-sbeller@google.com","subject":"[PATCH 4/8] submodule: move global changed_submodule_names into fetch submodule struct","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-21T22:35:54Z","receivedAt":"2018-09-21T22:36:44Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The `changed_submodule_names` are only used for fetching, so let's make it\npart of the struct that is passed around for fetching submodules.\n\nSigned-off-by: Stefan Beller \n---\n submodule.c | 42 +++++++++++++++++++++++-------------------\n 1 file changed, 23 insertions(+), 19 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 2f5c19d0aac..7b4d136849b 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -25,7 +25,7 @@\n #include \"commit-reach.h\"\n \n static int config_update_recurse_submodules = RECURSE_SUBMODULES_OFF;\n-static struct string_list changed_submodule_names = STRING_LIST_INIT_DUP;\n+\n static int initialized_fetch_ref_tips;\n static struct oid_array ref_tips_before_fetch;\n static struct oid_array ref_tips_after_fetch;\n@@ -1111,7 +1111,22 @@ void check_for_new_submodule_commits(struct object_id *oid)\n \toid_array_append(&ref_tips_after_fetch, oid);\n }\n \n-static void calculate_changed_submodule_paths(void)\n+struct submodule_parallel_fetch {\n+\tint count;\n+\tstruct argv_array args;\n+\tstruct repository *r;\n+\tconst char *prefix;\n+\tint command_line_option;\n+\tint default_option;\n+\tint quiet;\n+\tint result;\n+\n+\tstruct string_list changed_submodule_names;\n+};\n+#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0, STRING_LIST_INIT_DUP }\n+\n+static void calculate_changed_submodule_paths(\n+\tstruct submodule_parallel_fetch *spf)\n {\n \tstruct argv_array argv = ARGV_ARRAY_INIT;\n \tstruct string_list changed_submodules = STRING_LIST_INIT_DUP;\n@@ -1149,7 +1164,8 @@ static void calculate_changed_submodule_paths(void)\n \t\t\tcontinue;\n \n \t\tif (!submodule_has_commits(path, commits))\n-\t\t\tstring_list_append(&changed_submodule_names, name->string);\n+\t\t\tstring_list_append(&spf->changed_submodule_names,\n+\t\t\t\t\t name->string);\n \t}\n \n \tfree_submodules_oids(&changed_submodules);\n@@ -1186,18 +1202,6 @@ int submodule_touches_in_range(struct object_id *excl_oid,\n \treturn ret;\n }\n \n-struct submodule_parallel_fetch {\n-\tint count;\n-\tstruct argv_array args;\n-\tstruct repository *r;\n-\tconst char *prefix;\n-\tint command_line_option;\n-\tint default_option;\n-\tint quiet;\n-\tint result;\n-};\n-#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0}\n-\n static int get_fetch_recurse_config(const struct submodule *submodule,\n \t\t\t\t struct submodule_parallel_fetch *spf)\n {\n@@ -1258,7 +1262,7 @@ static int get_next_submodule(struct child_process *cp,\n \t\tcase RECURSE_SUBMODULES_ON_DEMAND:\n \t\t\tif (!submodule ||\n \t\t\t !string_list_lookup(\n-\t\t\t\t\t&changed_submodule_names,\n+\t\t\t\t\t&spf->changed_submodule_names,\n \t\t\t\t\tsubmodule->name))\n \t\t\t\tcontinue;\n \t\t\tdefault_argv = \"on-demand\";\n@@ -1350,8 +1354,8 @@ int fetch_populated_submodules(struct repository *r,\n \targv_array_push(&spf.args, \"--recurse-submodules-default\");\n \t/* default value, \"--submodule-prefix\" and its value are added later */\n \n-\tcalculate_changed_submodule_paths();\n-\tstring_list_sort(&changed_submodule_names);\n+\tcalculate_changed_submodule_paths(&spf);\n+\tstring_list_sort(&spf.changed_submodule_names);\n \trun_processes_parallel(max_parallel_jobs,\n \t\t\t get_next_submodule,\n \t\t\t fetch_start_failure,\n@@ -1360,7 +1364,7 @@ int fetch_populated_submodules(struct repository *r,\n \n \targv_array_clear(&spf.args);\n out:\n-\tstring_list_clear(&changed_submodule_names, 1);\n+\tstring_list_clear(&spf.changed_submodule_names, 1);\n \treturn spf.result;\n }\n \n-- \n2.19.0.444.g18242da7ef-goog\n\n"},{"id":"358677","messageId":"20180921223558.65055-6-sbeller@google.com","threadId":"49400","inReplyTo":"20180921223558.65055-1-sbeller@google.com","subject":"[PATCH 5/8] submodule.c: do not copy around submodule list","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-21T22:35:55Z","receivedAt":"2018-09-21T22:36:49Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"'calculate_changed_submodule_paths' uses a local list to compute the\nchanged submodules, and then produces the result by copying appropriate\nitems into the result list.\n\nInstead use the result list directly and prune items afterwards\nusing string_list_remove_empty_items.\n\nBy doing so we'll have access to the util pointer for longer that\ncontains the commits that we need to fetch, which will be\nuseful in a later patch.\n\nSigned-off-by: Stefan Beller \n---\n submodule.c | 19 ++++++++++---------\n 1 file changed, 10 insertions(+), 9 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 7b4d136849b..209680ec076 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1129,8 +1129,7 @@ static void calculate_changed_submodule_paths(\n \tstruct submodule_parallel_fetch *spf)\n {\n \tstruct argv_array argv = ARGV_ARRAY_INIT;\n-\tstruct string_list changed_submodules = STRING_LIST_INIT_DUP;\n-\tconst struct string_list_item *name;\n+\tstruct string_list_item *name;\n \n \t/* No need to check if there are no submodules configured */\n \tif (!submodule_from_path(the_repository, NULL, NULL))\n@@ -1147,9 +1146,9 @@ static void calculate_changed_submodule_paths(\n \t * Collect all submodules (whether checked out or not) for which new\n \t * commits have been recorded upstream in \"changed_submodule_names\".\n \t */\n-\tcollect_changed_submodules(&changed_submodules, &argv);\n+\tcollect_changed_submodules(&spf->changed_submodule_names, &argv);\n \n-\tfor_each_string_list_item(name, &changed_submodules) {\n+\tfor_each_string_list_item(name, &spf->changed_submodule_names) {\n \t\tstruct oid_array *commits = name->util;\n \t\tconst struct submodule *submodule;\n \t\tconst char *path = NULL;\n@@ -1163,12 +1162,14 @@ static void calculate_changed_submodule_paths(\n \t\tif (!path)\n \t\t\tcontinue;\n \n-\t\tif (!submodule_has_commits(path, commits))\n-\t\t\tstring_list_append(&spf->changed_submodule_names,\n-\t\t\t\t\t name->string);\n+\t\tif (submodule_has_commits(path, commits)) {\n+\t\t\toid_array_clear(commits);\n+\t\t\t*name->string = '\\0';\n+\t\t}\n \t}\n \n-\tfree_submodules_oids(&changed_submodules);\n+\tstring_list_remove_empty_items(&spf->changed_submodule_names, 1);\n+\n \targv_array_clear(&argv);\n \toid_array_clear(&ref_tips_before_fetch);\n \toid_array_clear(&ref_tips_after_fetch);\n@@ -1364,7 +1365,7 @@ int fetch_populated_submodules(struct repository *r,\n \n \targv_array_clear(&spf.args);\n out:\n-\tstring_list_clear(&spf.changed_submodule_names, 1);\n+\tfree_submodules_oids(&spf.changed_submodule_names);\n \treturn spf.result;\n }\n \n-- \n2.19.0.444.g18242da7ef-goog\n\n"},{"id":"358678","messageId":"20180921223558.65055-7-sbeller@google.com","threadId":"49400","inReplyTo":"20180921223558.65055-1-sbeller@google.com","subject":"[PATCH 6/8] submodule: fetch in submodules git directory instead of in worktree","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-21T22:35:56Z","receivedAt":"2018-09-21T22:36:52Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This patch started as a refactoring to make 'get_next_submodule' more\nreadable, but upon doing so, I realized that \"git fetch\" of the submodule\nactually doesn't need to be run in the submodules worktree. So let's run\nit in its git dir instead.\n\nThat should pave the way towards fetching submodules that are currently\nnot checked out.\n\nThis patch leaks the cp->dir in get_next_submodule, as any further\ncallback in run_processes_parallel doesn't have access to the child\nprocess any more. In an early iteration of this patch, the function\nget_submodule_repo_for directly returned the string containing the\ngit directory, which would be a better design choice for this patch.\n\nHowever the next patch both fixes the memory leak of cp->dir and also has\na use case for using the full repository handle of the submodule, so\nit makes sense to introduce the get_submodule_repo_for here already.\n\nSigned-off-by: Stefan Beller \n---\n submodule.c | 47 +++++++++++++++++++++++++++----------\n t/t5526-fetch-submodules.sh | 7 +++++-\n 2 files changed, 40 insertions(+), 14 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 209680ec076..8829e8d4eff 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -482,6 +482,12 @@ void prepare_submodule_repo_env(struct argv_array *out)\n \t\t\t DEFAULT_GIT_DIR_ENVIRONMENT);\n }\n \n+static void prepare_submodule_repo_env_in_gitdir(struct argv_array *out)\n+{\n+\tprepare_submodule_repo_env_no_git_dir(out);\n+\targv_array_pushf(out, \"%s=.\", GIT_DIR_ENVIRONMENT);\n+}\n+\n /* Helper function to display the submodule header line prior to the full\n * summary output. If it can locate the submodule objects directory it will\n * attempt to lookup both the left and right commits and put them into the\n@@ -1228,6 +1234,25 @@ static int get_fetch_recurse_config(const struct submodule *submodule,\n \treturn spf->default_option;\n }\n \n+static struct repository *get_submodule_repo_for(struct repository *r,\n+\t\t\t\t\t\t const char *path)\n+{\n+\tstruct repository *ret = xmalloc(sizeof(*ret));\n+\n+\tif (repo_submodule_init(ret, r, path)) {\n+\t\t/* no entry in .gitmodules? */\n+\t\tstruct strbuf gitdir = STRBUF_INIT;\n+\t\tstrbuf_repo_worktree_path(&gitdir, r, \"%s/.git\", path);\n+\t\tif (repo_init(ret, gitdir.buf, NULL)) {\n+\t\t\tstrbuf_release(&gitdir);\n+\t\t\treturn NULL;\n+\t\t}\n+\t\tstrbuf_release(&gitdir);\n+\t}\n+\n+\treturn ret;\n+}\n+\n static int get_next_submodule(struct child_process *cp,\n \t\t\t struct strbuf *err, void *data, void **task_cb)\n {\n@@ -1235,12 +1260,11 @@ static int get_next_submodule(struct child_process *cp,\n \tstruct submodule_parallel_fetch *spf = data;\n \n \tfor (; spf->count < spf->r->index->cache_nr; spf->count++) {\n-\t\tstruct strbuf submodule_path = STRBUF_INIT;\n-\t\tstruct strbuf submodule_git_dir = STRBUF_INIT;\n \t\tstruct strbuf submodule_prefix = STRBUF_INIT;\n \t\tconst struct cache_entry *ce = spf->r->index->cache[spf->count];\n-\t\tconst char *git_dir, *default_argv;\n+\t\tconst char *default_argv;\n \t\tconst struct submodule *submodule;\n+\t\tstruct repository *repo;\n \t\tstruct submodule default_submodule = SUBMODULE_INIT;\n \n \t\tif (!S_ISGITLINK(ce->ce_mode))\n@@ -1275,16 +1299,12 @@ static int get_next_submodule(struct child_process *cp,\n \t\t\tcontinue;\n \t\t}\n \n-\t\tstrbuf_repo_worktree_path(&submodule_path, spf->r, \"%s\", ce->name);\n-\t\tstrbuf_addf(&submodule_git_dir, \"%s/.git\", submodule_path.buf);\n \t\tstrbuf_addf(&submodule_prefix, \"%s%s/\", spf->prefix, ce->name);\n-\t\tgit_dir = read_gitfile(submodule_git_dir.buf);\n-\t\tif (!git_dir)\n-\t\t\tgit_dir = submodule_git_dir.buf;\n-\t\tif (is_directory(git_dir)) {\n+\t\trepo = get_submodule_repo_for(spf->r, ce->name);\n+\t\tif (repo) {\n \t\t\tchild_process_init(cp);\n-\t\t\tcp->dir = strbuf_detach(&submodule_path, NULL);\n-\t\t\tprepare_submodule_repo_env(&cp->env_array);\n+\t\t\tprepare_submodule_repo_env_in_gitdir(&cp->env_array);\n+\t\t\tcp->dir = xstrdup(repo->gitdir);\n \t\t\tcp->git_cmd = 1;\n \t\t\tif (!spf->quiet)\n \t\t\t\tstrbuf_addf(err, \"Fetching submodule %s%s\\n\",\n@@ -1294,10 +1314,11 @@ static int get_next_submodule(struct child_process *cp,\n \t\t\targv_array_push(&cp->args, default_argv);\n \t\t\targv_array_push(&cp->args, \"--submodule-prefix\");\n \t\t\targv_array_push(&cp->args, submodule_prefix.buf);\n+\n+\t\t\trepo_clear(repo);\n+\t\t\tfree(repo);\n \t\t\tret = 1;\n \t\t}\n-\t\tstrbuf_release(&submodule_path);\n-\t\tstrbuf_release(&submodule_git_dir);\n \t\tstrbuf_release(&submodule_prefix);\n \t\tif (ret) {\n \t\t\tspf->count++;\ndiff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\nindex 6c2f9b2ba26..42692219a1a 100755\n--- a/t/t5526-fetch-submodules.sh\n+++ b/t/t5526-fetch-submodules.sh\n@@ -566,7 +566,12 @@ test_expect_success 'fetching submodule into a broken repository' '\n \n \ttest_must_fail git -C dst status &&\n \ttest_must_fail git -C dst diff &&\n-\ttest_must_fail git -C dst fetch --recurse-submodules\n+\n+\t# git-fetch cannot find the git directory of the submodule,\n+\t# so it will do nothing, successfully, as it cannot distinguish between\n+\t# this broken submodule and a submodule that was just set active but\n+\t# not cloned yet\n+\tgit -C dst fetch --recurse-submodules\n '\n \n test_expect_success \"fetch new commits when submodule got renamed\" '\n-- \n2.19.0.444.g18242da7ef-goog\n\n"},{"id":"358679","messageId":"20180921223558.65055-8-sbeller@google.com","threadId":"49400","inReplyTo":"20180921223558.65055-1-sbeller@google.com","subject":"[PATCH 7/8] fetch: retry fetching submodules if needed objects were not fetched","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-21T22:35:57Z","receivedAt":"2018-09-21T22:36:55Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Currently when git-fetch is asked to recurse into submodules, it dispatches\na plain \"git-fetch -C \" (with some submodule related options\nsuch as prefix and recusing strategy, but) without any information of the\nremote or the tip that should be fetched.\n\nThis works surprisingly well in some workflows (such as using submodules\nas a third party library), while not so well in other scenarios, such\nas in a Gerrit topic-based workflow, that can tie together changes\n(potentially across repositories) on the server side. One of the parts\nof such a Gerrit workflow is to download a change when wanting to examine\nit, and you'd want to have its submodule changes that are in the same\ntopic downloaded as well. However these submodule changes reside in their\nown repository in their own ref (refs/changes/).\n\nRetry fetching a submodule by object name if the object id that the\nsuperproject points to, cannot be found.\n\nThis retrying does not happen when the \"git fetch\" done at the\nsuperproject is not storing the fetched results in remote\ntracking branches (i.e. instead just recording them to\nFETCH_HEAD) in this step. A later patch will fix this.\n\nbuiltin/fetch used to only inspect submodules when they were fetched\n\"on-demand\", as in either on/off case it was clear whether the submodule\nneeds to be fetched. However to know whether we need to try fetching the\nobject ids, we need to identify the object names, which is done in this\nfunction check_for_new_submodule_commits(), so we'll also run that code\nin case the submodule recursion is set to \"on\".\n\nSigned-off-by: Stefan Beller \n---\n builtin/fetch.c | 9 +-\n submodule.c | 182 ++++++++++++++++++++++++++++++------\n t/t5526-fetch-submodules.sh | 16 ++++\n 3 files changed, 174 insertions(+), 33 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 0696abfc2a1..e3b03ad3bd3 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -707,8 +707,7 @@ static int update_local_ref(struct ref *ref,\n \t\t\twhat = _(\"[new ref]\");\n \t\t}\n \n-\t\tif ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&\n-\t\t (recurse_submodules != RECURSE_SUBMODULES_ON))\n+\t\tif (recurse_submodules != RECURSE_SUBMODULES_OFF)\n \t\t\tcheck_for_new_submodule_commits(&ref->new_oid);\n \t\tr = s_update_ref(msg, ref, 0);\n \t\tformat_display(display, r ? '!' : '*', what,\n@@ -723,8 +722,7 @@ static int update_local_ref(struct ref *ref,\n \t\tstrbuf_add_unique_abbrev(&quickref, ¤t->object.oid, DEFAULT_ABBREV);\n \t\tstrbuf_addstr(&quickref, \"..\");\n \t\tstrbuf_add_unique_abbrev(&quickref, &ref->new_oid, DEFAULT_ABBREV);\n-\t\tif ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&\n-\t\t (recurse_submodules != RECURSE_SUBMODULES_ON))\n+\t\tif (recurse_submodules != RECURSE_SUBMODULES_OFF)\n \t\t\tcheck_for_new_submodule_commits(&ref->new_oid);\n \t\tr = s_update_ref(\"fast-forward\", ref, 1);\n \t\tformat_display(display, r ? '!' : ' ', quickref.buf,\n@@ -738,8 +736,7 @@ static int update_local_ref(struct ref *ref,\n \t\tstrbuf_add_unique_abbrev(&quickref, ¤t->object.oid, DEFAULT_ABBREV);\n \t\tstrbuf_addstr(&quickref, \"...\");\n \t\tstrbuf_add_unique_abbrev(&quickref, &ref->new_oid, DEFAULT_ABBREV);\n-\t\tif ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&\n-\t\t (recurse_submodules != RECURSE_SUBMODULES_ON))\n+\t\tif (recurse_submodules != RECURSE_SUBMODULES_OFF)\n \t\t\tcheck_for_new_submodule_commits(&ref->new_oid);\n \t\tr = s_update_ref(\"forced-update\", ref, 1);\n \t\tformat_display(display, r ? '!' : '+', quickref.buf,\ndiff --git a/submodule.c b/submodule.c\nindex 8829e8d4eff..ba98a94d4db 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1128,8 +1128,12 @@ struct submodule_parallel_fetch {\n \tint result;\n \n \tstruct string_list changed_submodule_names;\n+\tstruct get_next_submodule_task **retry;\n+\tint retry_nr, retry_alloc;\n };\n-#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0, STRING_LIST_INIT_DUP }\n+#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0, \\\n+\t\t STRING_LIST_INIT_DUP, \\\n+\t\t NULL, 0, 0}\n \n static void calculate_changed_submodule_paths(\n \tstruct submodule_parallel_fetch *spf)\n@@ -1234,6 +1238,56 @@ static int get_fetch_recurse_config(const struct submodule *submodule,\n \treturn spf->default_option;\n }\n \n+struct get_next_submodule_task {\n+\tstruct repository *repo;\n+\tconst struct submodule *sub;\n+\tunsigned free_sub : 1; /* Do we need to free the submodule? */\n+\tstruct oid_array *commits;\n+};\n+\n+static const struct submodule *get_default_submodule(const char *path)\n+{\n+\tstruct submodule *ret = NULL;\n+\tconst char *name = default_name_or_path(path);\n+\n+\tif (!name)\n+\t\treturn NULL;\n+\n+\tret = xmalloc(sizeof(*ret));\n+\tmemset(ret, 0, sizeof(*ret));\n+\tret->path = name;\n+\tret->name = name;\n+\n+\treturn (const struct submodule *) ret;\n+}\n+\n+static struct get_next_submodule_task *get_next_submodule_task_create(\n+\tstruct repository *r, const char *path)\n+{\n+\tstruct get_next_submodule_task *task = xmalloc(sizeof(*task));\n+\tmemset(task, 0, sizeof(*task));\n+\n+\ttask->sub = submodule_from_path(r, &null_oid, path);\n+\tif (!task->sub) {\n+\t\ttask->sub = get_default_submodule(path);\n+\t\ttask->free_sub = 1;\n+\t}\n+\n+\treturn task;\n+}\n+\n+static void get_next_submodule_task_release(struct get_next_submodule_task *p)\n+{\n+\tif (p->free_sub)\n+\t\tfree((void*)p->sub);\n+\tp->free_sub = 0;\n+\tp->sub = NULL;\n+\n+\tif (p->repo)\n+\t\trepo_clear(p->repo);\n+\tFREE_AND_NULL(p->repo);\n+}\n+\n static struct repository *get_submodule_repo_for(struct repository *r,\n \t\t\t\t\t\t const char *path)\n {\n@@ -1256,39 +1310,35 @@ static struct repository *get_submodule_repo_for(struct repository *r,\n static int get_next_submodule(struct child_process *cp,\n \t\t\t struct strbuf *err, void *data, void **task_cb)\n {\n-\tint ret = 0;\n \tstruct submodule_parallel_fetch *spf = data;\n \n \tfor (; spf->count < spf->r->index->cache_nr; spf->count++) {\n-\t\tstruct strbuf submodule_prefix = STRBUF_INIT;\n+\t\tint recurse_config;\n \t\tconst struct cache_entry *ce = spf->r->index->cache[spf->count];\n \t\tconst char *default_argv;\n-\t\tconst struct submodule *submodule;\n-\t\tstruct repository *repo;\n-\t\tstruct submodule default_submodule = SUBMODULE_INIT;\n+\t\tstruct get_next_submodule_task *task;\n \n \t\tif (!S_ISGITLINK(ce->ce_mode))\n \t\t\tcontinue;\n \n-\t\tsubmodule = submodule_from_path(spf->r, &null_oid, ce->name);\n-\t\tif (!submodule) {\n-\t\t\tconst char *name = default_name_or_path(ce->name);\n-\t\t\tif (name) {\n-\t\t\t\tdefault_submodule.path = name;\n-\t\t\t\tdefault_submodule.name = name;\n-\t\t\t\tsubmodule = &default_submodule;\n-\t\t\t}\n+\t\ttask = get_next_submodule_task_create(spf->r, ce->name);\n+\n+\t\tif (!task->sub) {\n+\t\t\tfree(task);\n+\t\t\tcontinue;\n \t\t}\n \n-\t\tswitch (get_fetch_recurse_config(submodule, spf))\n+\t\trecurse_config = get_fetch_recurse_config(task->sub, spf);\n+\n+\t\tswitch (recurse_config)\n \t\t{\n \t\tdefault:\n \t\tcase RECURSE_SUBMODULES_DEFAULT:\n \t\tcase RECURSE_SUBMODULES_ON_DEMAND:\n-\t\t\tif (!submodule ||\n+\t\t\tif (!task->sub ||\n \t\t\t !string_list_lookup(\n \t\t\t\t\t&spf->changed_submodule_names,\n-\t\t\t\t\tsubmodule->name))\n+\t\t\t\t\ttask->sub->name))\n \t\t\t\tcontinue;\n \t\t\tdefault_argv = \"on-demand\";\n \t\t\tbreak;\n@@ -1299,12 +1349,13 @@ static int get_next_submodule(struct child_process *cp,\n \t\t\tcontinue;\n \t\t}\n \n-\t\tstrbuf_addf(&submodule_prefix, \"%s%s/\", spf->prefix, ce->name);\n-\t\trepo = get_submodule_repo_for(spf->r, ce->name);\n-\t\tif (repo) {\n+\t\ttask->repo = get_submodule_repo_for(spf->r,\n+\t\t\t\t\t\t task->sub->path);\n+\t\tif (task->repo) {\n+\t\t\tstruct strbuf submodule_prefix = STRBUF_INIT;\n \t\t\tchild_process_init(cp);\n \t\t\tprepare_submodule_repo_env_in_gitdir(&cp->env_array);\n-\t\t\tcp->dir = xstrdup(repo->gitdir);\n+\t\t\tcp->dir = task->repo->gitdir;\n \t\t\tcp->git_cmd = 1;\n \t\t\tif (!spf->quiet)\n \t\t\t\tstrbuf_addf(err, \"Fetching submodule %s%s\\n\",\n@@ -1313,18 +1364,51 @@ static int get_next_submodule(struct child_process *cp,\n \t\t\targv_array_pushv(&cp->args, spf->args.argv);\n \t\t\targv_array_push(&cp->args, default_argv);\n \t\t\targv_array_push(&cp->args, \"--submodule-prefix\");\n+\n+\t\t\tstrbuf_addf(&submodule_prefix, \"%s%s/\",\n+\t\t\t\t\t\t spf->prefix,\n+\t\t\t\t\t\t task->sub->path);\n \t\t\targv_array_push(&cp->args, submodule_prefix.buf);\n \n-\t\t\trepo_clear(repo);\n-\t\t\tfree(repo);\n-\t\t\tret = 1;\n-\t\t}\n-\t\tstrbuf_release(&submodule_prefix);\n-\t\tif (ret) {\n \t\t\tspf->count++;\n+\t\t\t*task_cb = task;\n+\n+\t\t\tstrbuf_release(&submodule_prefix);\n \t\t\treturn 1;\n+\t\t} else {\n+\t\t\tget_next_submodule_task_release(task);\n+\t\t\tfree(task);\n \t\t}\n \t}\n+\n+\tif (spf->retry_nr) {\n+\t\tstruct get_next_submodule_task *task = spf->retry[spf->retry_nr - 1];\n+\t\tstruct strbuf submodule_prefix = STRBUF_INIT;\n+\t\tspf->retry_nr--;\n+\n+\t\tstrbuf_addf(&submodule_prefix, \"%s%s/\", spf->prefix, task->sub->path);\n+\n+\t\tchild_process_init(cp);\n+\t\tprepare_submodule_repo_env_in_gitdir(&cp->env_array);\n+\t\tcp->git_cmd = 1;\n+\t\tcp->dir = task->repo->gitdir;\n+\n+\t\targv_array_init(&cp->args);\n+\t\targv_array_pushv(&cp->args, spf->args.argv);\n+\t\targv_array_push(&cp->args, \"on-demand\");\n+\t\targv_array_push(&cp->args, \"--submodule-prefix\");\n+\t\targv_array_push(&cp->args, submodule_prefix.buf);\n+\n+\t\t/* NEEDSWORK: have get_default_remote from s--h */\n+\t\targv_array_push(&cp->args, \"origin\");\n+\t\toid_array_for_each_unique(task->commits,\n+\t\t\t\t\t append_oid_to_argv, &cp->args);\n+\n+\t\t*task_cb = task;\n+\t\tstrbuf_release(&submodule_prefix);\n+\t\treturn 1;\n+\t}\n+\n \treturn 0;\n }\n \n@@ -1332,20 +1416,64 @@ static int fetch_start_failure(struct strbuf *err,\n \t\t\t void *cb, void *task_cb)\n {\n \tstruct submodule_parallel_fetch *spf = cb;\n+\tstruct get_next_submodule_task *task = task_cb;\n \n \tspf->result = 1;\n \n+\tget_next_submodule_task_release(task);\n \treturn 0;\n }\n \n+static int commit_exists_in_sub(const struct object_id *oid, void *data)\n+{\n+\tstruct repository *subrepo = data;\n+\n+\tenum object_type type = oid_object_info(subrepo, oid, NULL);\n+\n+\treturn type != OBJ_COMMIT;\n+}\n+\n static int fetch_finish(int retvalue, struct strbuf *err,\n \t\t\tvoid *cb, void *task_cb)\n {\n \tstruct submodule_parallel_fetch *spf = cb;\n+\tstruct get_next_submodule_task *task = task_cb;\n+\tconst struct submodule *sub;\n+\n+\tstruct string_list_item *it;\n+\tstruct oid_array *commits;\n \n \tif (retvalue)\n \t\tspf->result = 1;\n \n+\tif (!task)\n+\t\treturn 0;\n+\n+\tsub = task->sub;\n+\tif (!sub)\n+\t\tgoto out;\n+\n+\tit = string_list_lookup(&spf->changed_submodule_names, sub->name);\n+\tif (!it)\n+\t\tgoto out;\n+\n+\tcommits = it->util;\n+\toid_array_filter(commits,\n+\t\t\t commit_exists_in_sub,\n+\t\t\t task->repo);\n+\n+\ttrace_printf(\"checking for submodule: needs %d more commits\", commits->nr);\n+\tif (commits->nr) {\n+\t\ttask->commits = commits;\n+\t\tALLOC_GROW(spf->retry, spf->retry_nr + 1, spf->retry_alloc);\n+\t\tspf->retry[spf->retry_nr] = task;\n+\t\tspf->retry_nr++;\n+\t\treturn 0;\n+\t}\n+\n+out:\n+\tget_next_submodule_task_release(task);\n+\n \treturn 0;\n }\n \ndiff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\nindex 42692219a1a..af12c50e7dd 100755\n--- a/t/t5526-fetch-submodules.sh\n+++ b/t/t5526-fetch-submodules.sh\n@@ -605,4 +605,20 @@ test_expect_success \"fetch new commits when submodule got renamed\" '\n \ttest_cmp expect actual\n '\n \n+test_expect_success \"fetch new commits on-demand when they are not reachable\" '\n+\tgit checkout --detach &&\n+\tC=$(git -C submodule commit-tree -m \"new change outside refs/heads\" HEAD^{tree}) &&\n+\tgit -C submodule update-ref refs/changes/1 $C &&\n+\tgit update-index --cacheinfo 160000 $C submodule &&\n+\tgit commit -m \"updated submodule outside of refs/heads\" &&\n+\tD=$(git rev-parse HEAD) &&\n+\tgit update-ref refs/changes/2 $D &&\n+\t(\n+\t\tcd downstream &&\n+\t\tgit fetch --recurse-submodules --recurse-submodules-default on-demand origin refs/changes/2:refs/heads/my_branch &&\n+\t\tgit -C submodule cat-file -t $C &&\n+\t\tgit checkout --recurse-submodules FETCH_HEAD\n+\t)\n+'\n+\n test_done\n-- \n2.19.0.444.g18242da7ef-goog\n\n"},{"id":"358680","messageId":"20180921223558.65055-9-sbeller@google.com","threadId":"49400","inReplyTo":"20180921223558.65055-1-sbeller@google.com","subject":"[PATCH 8/8] builtin/fetch: check for submodule updates for non branch fetches","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-21T22:35:58Z","receivedAt":"2018-09-21T22:36:57Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Gerrit, the code review tool, has a different workflow than our mailing\nlist based approach. Usually users upload changes to a Gerrit server and\ncontinuous integration and testing happens by bots. Sometimes however a\nuser wants to checkout a change locally and look at it locally. For this\nuse case, Gerrit offers a command line snippet to copy and paste to your\nterminal, which looks like\n\n git fetch https:///gerrit refs/changes/ &&\n git checkout FETCH_HEAD\n\nFor Gerrit changes that contain changing submodule gitlinks, it would be\neasy to extend both the fetch and checkout with the '--recurse-submodules'\nflag, such that this command line snippet would produce the state of a\nchange locally.\n\nHowever the functionality added in the previous patch, which would\nensure that we fetch the objects in the submodule that the gitlink pointed\nat, only works for remote tracking branches so far, not for FETCH_HEAD.\n\nMake sure that fetching a superproject to its FETCH_HEAD, also respects\nthe existence checks for objects in the submodule recursion.\n\nSigned-off-by: Stefan Beller \n---\n builtin/fetch.c | 5 ++++-\n t/t5526-fetch-submodules.sh | 2 +-\n 2 files changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex e3b03ad3bd3..f2d9e548bf0 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -894,11 +894,14 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\t\t\trc |= update_local_ref(ref, what, rm, ¬e,\n \t\t\t\t\t\t summary_width);\n \t\t\t\tfree(ref);\n-\t\t\t} else\n+\t\t\t} else {\n+\t\t\t\tcheck_for_new_submodule_commits(&rm->old_oid);\n \t\t\t\tformat_display(¬e, '*',\n \t\t\t\t\t *kind ? kind : \"branch\", NULL,\n \t\t\t\t\t *what ? what : \"HEAD\",\n \t\t\t\t\t \"FETCH_HEAD\", summary_width);\n+\t\t\t}\n+\n \t\t\tif (note.len) {\n \t\t\t\tif (verbosity >= 0 && !shown_url) {\n \t\t\t\t\tfprintf(stderr, _(\"From %.*s\\n\"),\ndiff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\nindex af12c50e7dd..a509eabb044 100755\n--- a/t/t5526-fetch-submodules.sh\n+++ b/t/t5526-fetch-submodules.sh\n@@ -615,7 +615,7 @@ test_expect_success \"fetch new commits on-demand when they are not reachable\" '\n \tgit update-ref refs/changes/2 $D &&\n \t(\n \t\tcd downstream &&\n-\t\tgit fetch --recurse-submodules --recurse-submodules-default on-demand origin refs/changes/2:refs/heads/my_branch &&\n+\t\tgit fetch --recurse-submodules origin refs/changes/2 &&\n \t\tgit -C submodule cat-file -t $C &&\n \t\tgit checkout --recurse-submodules FETCH_HEAD\n \t)\n-- \n2.19.0.444.g18242da7ef-goog\n\n"},{"id":"358697","messageId":"87h8ihk7sl.fsf@evledraar.gmail.com","threadId":"49400","inReplyTo":"20180921223558.65055-2-sbeller@google.com","subject":"Re: [PATCH 1/8] sha1-array: provide oid_array_filter","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-09-22T12:58:50Z","receivedAt":"2018-09-22T12:58:55Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Sep 21 2018, Stefan Beller wrote:\n\n> +/*\n> + * Apply want to each entry in array, retaining only the entries for\n> + * which the function returns true. Preserve the order of the entries\n> + * that are retained.\n> + */\n> +void oid_array_filter(struct oid_array *array,\n> +\t\t for_each_oid_fn want,\n> +\t\t void *cbdata);\n> +\n> #endif /* SHA1_ARRAY_H */\n\nThe code LGTM, but this comment should instead be an update to the API\ndocs, see my recent 5cc044e025 (\"get_short_oid: sort ambiguous objects\nby type, then SHA-1\", 2018-05-10) for an addition of a new function to\nthis API where I added some new docs.\n"},{"id":"358856","messageId":"CAGZ79kZCkccV=4a1cAE0DpV7hZGxuCyJuceyAEow_u0LspwYAw@mail.gmail.com","threadId":"49400","inReplyTo":"87h8ihk7sl.fsf@evledraar.gmail.com","subject":"Re: [PATCH 1/8] sha1-array: provide oid_array_filter","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-25T19:26:44Z","receivedAt":"2018-09-25T19:26:57Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Sat, Sep 22, 2018 at 5:58 AM Ævar Arnfjörð Bjarmason\n wrote:\n>\n>\n> On Fri, Sep 21 2018, Stefan Beller wrote:\n>\n> > +/*\n> > + * Apply want to each entry in array, retaining only the entries for\n> > + * which the function returns true. Preserve the order of the entries\n> > + * that are retained.\n> > + */\n> > +void oid_array_filter(struct oid_array *array,\n> > + for_each_oid_fn want,\n> > + void *cbdata);\n> > +\n> > #endif /* SHA1_ARRAY_H */\n>\n> The code LGTM, but this comment should instead be an update to the API\n> docs, see my recent 5cc044e025 (\"get_short_oid: sort ambiguous objects\n> by type, then SHA-1\", 2018-05-10) for an addition of a new function to\n> this API where I added some new docs.\n\nok will fix for consistency (this whole API is there).\n\nLonger term (I thought) we were trying to migrate API docs\nto headers instead?\n\nStefan\n"},{"id":"358926","messageId":"20180926041517.GA2140@sigill.intra.peff.net","threadId":"49400","inReplyTo":"CAGZ79kZCkccV=4a1cAE0DpV7hZGxuCyJuceyAEow_u0LspwYAw@mail.gmail.com","subject":"Re: [PATCH 1/8] sha1-array: provide oid_array_filter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-09-26T04:15:17Z","receivedAt":"2018-09-26T04:15:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 25, 2018 at 12:26:44PM -0700, Stefan Beller wrote:\n\n> On Sat, Sep 22, 2018 at 5:58 AM Ævar Arnfjörð Bjarmason\n> wrote:\n> >\n> >\n> > On Fri, Sep 21 2018, Stefan Beller wrote:\n> >\n> > > +/*\n> > > + * Apply want to each entry in array, retaining only the entries for\n> > > + * which the function returns true. Preserve the order of the entries\n> > > + * that are retained.\n> > > + */\n> > > +void oid_array_filter(struct oid_array *array,\n> > > + for_each_oid_fn want,\n> > > + void *cbdata);\n> > > +\n> > > #endif /* SHA1_ARRAY_H */\n> >\n> > The code LGTM, but this comment should instead be an update to the API\n> > docs, see my recent 5cc044e025 (\"get_short_oid: sort ambiguous objects\n> > by type, then SHA-1\", 2018-05-10) for an addition of a new function to\n> > this API where I added some new docs.\n> \n> ok will fix for consistency (this whole API is there).\n> \n> Longer term (I thought) we were trying to migrate API docs\n> to headers instead?\n\nYes, please. I think it prevents exactly this sort of confusion. :)\n\n-Peff\n"},{"id":"358958","messageId":"xmqqlg7ombgt.fsf@gitster-ct.c.googlers.com","threadId":"49400","inReplyTo":"20180926041517.GA2140@sigill.intra.peff.net","subject":"Re: [PATCH 1/8] sha1-array: provide oid_array_filter","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-26T17:10:10Z","receivedAt":"2018-09-26T17:15:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Sep 25, 2018 at 12:26:44PM -0700, Stefan Beller wrote:\n>\n>> On Sat, Sep 22, 2018 at 5:58 AM Ævar Arnfjörð Bjarmason\n>> wrote:\n>> >\n>> >\n>> > On Fri, Sep 21 2018, Stefan Beller wrote:\n>> >\n>> > > +/*\n>> > > + * Apply want to each entry in array, retaining only the entries for\n>> > > + * which the function returns true. Preserve the order of the entries\n>> > > + * that are retained.\n>> > > + */\n>> > > +void oid_array_filter(struct oid_array *array,\n>> > > + for_each_oid_fn want,\n>> > > + void *cbdata);\n>> > > +\n>> > > #endif /* SHA1_ARRAY_H */\n>> >\n>> > The code LGTM, but this comment should instead be an update to the API\n>> > docs, see my recent 5cc044e025 (\"get_short_oid: sort ambiguous objects\n>> > by type, then SHA-1\", 2018-05-10) for an addition of a new function to\n>> > this API where I added some new docs.\n>> \n>> ok will fix for consistency (this whole API is there).\n>> \n>> Longer term (I thought) we were trying to migrate API docs\n>> to headers instead?\n>\n> Yes, please. I think it prevents exactly this sort of confusion. :)\n\nCodingGuidelines or SubmittingPatches update, perhaps?\n\n Documentation/CodingGuidelines | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\nindex 48aa4edfbd..b54684e807 100644\n--- a/Documentation/CodingGuidelines\n+++ b/Documentation/CodingGuidelines\n@@ -358,7 +358,11 @@ For C programs:\n string_list for sorted string lists, a hash map (mapping struct\n objects) named \"struct decorate\", amongst other things.\n \n- - When you come up with an API, document it.\n+ - When you come up with an API, document it. It used to be\n+ encouraged to do so in Documentation/technical/, and the birds-eye\n+ level overview may still be more suitable there, but detailed\n+ function-by-function level of documentation is done by comments in\n+ corresponding .h files these days.\n \n - The first #include in C files, except in platform specific compat/\n implementations, must be either \"git-compat-util.h\", \"cache.h\" or\n"},{"id":"358962","messageId":"87d0t0jghm.fsf@evledraar.gmail.com","threadId":"49400","inReplyTo":"xmqqlg7ombgt.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/8] sha1-array: provide oid_array_filter","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-09-26T17:49:57Z","receivedAt":"2018-09-26T17:50:03Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Sep 26 2018, Junio C Hamano wrote:\n\n> Jeff King writes:\n>\n>> On Tue, Sep 25, 2018 at 12:26:44PM -0700, Stefan Beller wrote:\n>>\n>>> On Sat, Sep 22, 2018 at 5:58 AM Ævar Arnfjörð Bjarmason\n>>> wrote:\n>>> >\n>>> >\n>>> > On Fri, Sep 21 2018, Stefan Beller wrote:\n>>> >\n>>> > > +/*\n>>> > > + * Apply want to each entry in array, retaining only the entries for\n>>> > > + * which the function returns true. Preserve the order of the entries\n>>> > > + * that are retained.\n>>> > > + */\n>>> > > +void oid_array_filter(struct oid_array *array,\n>>> > > + for_each_oid_fn want,\n>>> > > + void *cbdata);\n>>> > > +\n>>> > > #endif /* SHA1_ARRAY_H */\n>>> >\n>>> > The code LGTM, but this comment should instead be an update to the API\n>>> > docs, see my recent 5cc044e025 (\"get_short_oid: sort ambiguous objects\n>>> > by type, then SHA-1\", 2018-05-10) for an addition of a new function to\n>>> > this API where I added some new docs.\n>>>\n>>> ok will fix for consistency (this whole API is there).\n>>>\n>>> Longer term (I thought) we were trying to migrate API docs\n>>> to headers instead?\n>>\n>> Yes, please. I think it prevents exactly this sort of confusion. :)\n>\n> CodingGuidelines or SubmittingPatches update, perhaps?\n>\n> Documentation/CodingGuidelines | 6 +++++-\n> 1 file changed, 5 insertions(+), 1 deletion(-)\n>\n> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\n> index 48aa4edfbd..b54684e807 100644\n> --- a/Documentation/CodingGuidelines\n> +++ b/Documentation/CodingGuidelines\n> @@ -358,7 +358,11 @@ For C programs:\n> string_list for sorted string lists, a hash map (mapping struct\n> objects) named \"struct decorate\", amongst other things.\n>\n> - - When you come up with an API, document it.\n> + - When you come up with an API, document it. It used to be\n> + encouraged to do so in Documentation/technical/, and the birds-eye\n> + level overview may still be more suitable there, but detailed\n> + function-by-function level of documentation is done by comments in\n> + corresponding .h files these days.\n>\n> - The first #include in C files, except in platform specific compat/\n> implementations, must be either \"git-compat-util.h\", \"cache.h\" or\n\nThanks. I had not looked at this closely and was under the false\nimpression that it was going in the other direction. Good to have it\nclarified.\n"},{"id":"358971","messageId":"xmqqd0t0ktau.fsf@gitster-ct.c.googlers.com","threadId":"49400","inReplyTo":"87d0t0jghm.fsf@evledraar.gmail.com","subject":"Re: [PATCH 1/8] sha1-array: provide oid_array_filter","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-26T18:27:53Z","receivedAt":"2018-09-26T18:27:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> On Wed, Sep 26 2018, Junio C Hamano wrote:\n>\n>> Jeff King writes:\n>>>\n>>> Yes, please. I think it prevents exactly this sort of confusion. :)\n>>\n>> CodingGuidelines or SubmittingPatches update, perhaps?\n>>\n>> Documentation/CodingGuidelines | 6 +++++-\n>> 1 file changed, 5 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\n>> index 48aa4edfbd..b54684e807 100644\n>> --- a/Documentation/CodingGuidelines\n>> +++ b/Documentation/CodingGuidelines\n>> @@ -358,7 +358,11 @@ For C programs:\n>> string_list for sorted string lists, a hash map (mapping struct\n>> objects) named \"struct decorate\", amongst other things.\n>>\n>> - - When you come up with an API, document it.\n>> + - When you come up with an API, document it. It used to be\n>> + encouraged to do so in Documentation/technical/, and the birds-eye\n>> + level overview may still be more suitable there, but detailed\n>> + function-by-function level of documentation is done by comments in\n>> + corresponding .h files these days.\n>>\n>> - The first #include in C files, except in platform specific compat/\n>> implementations, must be either \"git-compat-util.h\", \"cache.h\" or\n>\n> Thanks. I had not looked at this closely and was under the false\n> impression that it was going in the other direction. Good to have it\n> clarified.\n\nHeh, I knew people were in favor of one over the other but until\nPeff chimed in to this thread, I didn't recall which one was\npreferred, partly because I personally do not see a huge advantage\nin using in-code comments as docs for programmers, and do not like\nhaving to read them as in-code comments.\n\nIf somebody wants to wordsmith the text and send in a patch with\ngood log message, please do so, as I myself am not sure if what I\nwrote is the consensus position. It could be that they want to have\neven birds-eye overview in the header files.\n\n"},{"id":"358972","messageId":"20180926183451.GA30680@sigill.intra.peff.net","threadId":"49400","inReplyTo":"xmqqd0t0ktau.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/8] sha1-array: provide oid_array_filter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-09-26T18:34:51Z","receivedAt":"2018-09-26T18:34:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 26, 2018 at 11:27:53AM -0700, Junio C Hamano wrote:\n\n> >> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\n> >> index 48aa4edfbd..b54684e807 100644\n> >> --- a/Documentation/CodingGuidelines\n> >> +++ b/Documentation/CodingGuidelines\n> >> @@ -358,7 +358,11 @@ For C programs:\n> >> string_list for sorted string lists, a hash map (mapping struct\n> >> objects) named \"struct decorate\", amongst other things.\n> >>\n> >> - - When you come up with an API, document it.\n> >> + - When you come up with an API, document it. It used to be\n> >> + encouraged to do so in Documentation/technical/, and the birds-eye\n> >> + level overview may still be more suitable there, but detailed\n> >> + function-by-function level of documentation is done by comments in\n> >> + corresponding .h files these days.\n> >>\n> >> - The first #include in C files, except in platform specific compat/\n> >> implementations, must be either \"git-compat-util.h\", \"cache.h\" or\n> >\n> > Thanks. I had not looked at this closely and was under the false\n> > impression that it was going in the other direction. Good to have it\n> > clarified.\n> \n> Heh, I knew people were in favor of one over the other but until\n> Peff chimed in to this thread, I didn't recall which one was\n> preferred, partly because I personally do not see a huge advantage\n> in using in-code comments as docs for programmers, and do not like\n> having to read them as in-code comments.\n> \n> If somebody wants to wordsmith the text and send in a patch with\n> good log message, please do so, as I myself am not sure if what I\n> wrote is the consensus position. It could be that they want to have\n> even birds-eye overview in the header files.\n\nYes, I would say that everything should go into the header files. For\nthe same reason that the function descriptions should go there: by being\nclose to the thing being changed, it is more likely that authors will\nremember to adjust the documentation. It's not exactly literate\nprogramming, but it's a step in that direction.\n\nThat's just my opinion, of course. I've been very happy so far with the\ndocumentation that we have transitioned. We talked a while ago about a\nscript to extract the comments into a \"just documentation\" and build it\nas asciidoc, but I doubt I would use such a thing myself.\n\nI do agree in general that mentioning this in CodingGuidelines is a good\nidea.\n\n-Peff\n"},{"id":"358976","messageId":"87a7o4je0t.fsf@evledraar.gmail.com","threadId":"49400","inReplyTo":"xmqqd0t0ktau.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/8] sha1-array: provide oid_array_filter","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-09-26T18:43:14Z","receivedAt":"2018-09-26T18:43:18Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Sep 26 2018, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> On Wed, Sep 26 2018, Junio C Hamano wrote:\n>>\n>>> Jeff King writes:\n>>>>\n>>>> Yes, please. I think it prevents exactly this sort of confusion. :)\n>>>\n>>> CodingGuidelines or SubmittingPatches update, perhaps?\n>>>\n>>> Documentation/CodingGuidelines | 6 +++++-\n>>> 1 file changed, 5 insertions(+), 1 deletion(-)\n>>>\n>>> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\n>>> index 48aa4edfbd..b54684e807 100644\n>>> --- a/Documentation/CodingGuidelines\n>>> +++ b/Documentation/CodingGuidelines\n>>> @@ -358,7 +358,11 @@ For C programs:\n>>> string_list for sorted string lists, a hash map (mapping struct\n>>> objects) named \"struct decorate\", amongst other things.\n>>>\n>>> - - When you come up with an API, document it.\n>>> + - When you come up with an API, document it. It used to be\n>>> + encouraged to do so in Documentation/technical/, and the birds-eye\n>>> + level overview may still be more suitable there, but detailed\n>>> + function-by-function level of documentation is done by comments in\n>>> + corresponding .h files these days.\n>>>\n>>> - The first #include in C files, except in platform specific compat/\n>>> implementations, must be either \"git-compat-util.h\", \"cache.h\" or\n>>\n>> Thanks. I had not looked at this closely and was under the false\n>> impression that it was going in the other direction. Good to have it\n>> clarified.\n>\n> Heh, I knew people were in favor of one over the other but until\n> Peff chimed in to this thread, I didn't recall which one was\n> preferred, partly because I personally do not see a huge advantage\n> in using in-code comments as docs for programmers, and do not like\n> having to read them as in-code comments.\n>\n> If somebody wants to wordsmith the text and send in a patch with\n> good log message, please do so, as I myself am not sure if what I\n> wrote is the consensus position. It could be that they want to have\n> even birds-eye overview in the header files.\n\nWhile we're on that subject, I'd much prefer if these technical docs and\nother asciidoc-ish stuff we would be manpages build as part of our\nnormal \"make doc\" target. So e.g. this one would be readable via\nsomething like \"man 3 gitapi-oid-array\".\n\nThey could still be maintained along with the code given a sufficiently\nsmart doc build system, e.g. perl does that:\nhttps://github.com/Perl/perl5/blob/v5.26.0/av.c#L294-L387\n\nAlthough doing that in some readable and sane way would mean getting rid\nof your beloved multi-line /* ... */ comment formatting, at least for\nthat case. It would be a pain to have everyone configure their editor to\nword-wrap lines with leading \"*\" without screwing up the asciidoc\nformat.\n"},{"id":"358980","messageId":"20180926185812.GD30680@sigill.intra.peff.net","threadId":"49400","inReplyTo":"87a7o4je0t.fsf@evledraar.gmail.com","subject":"Re: [PATCH 1/8] sha1-array: provide oid_array_filter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-09-26T18:58:13Z","receivedAt":"2018-09-26T18:58:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 26, 2018 at 08:43:14PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> While we're on that subject, I'd much prefer if these technical docs and\n> other asciidoc-ish stuff we would be manpages build as part of our\n> normal \"make doc\" target. So e.g. this one would be readable via\n> something like \"man 3 gitapi-oid-array\".\n\nI'm mildly negative on this, just because it introduces extra work on\npeople writing the documentation. Now it has to follow special\nformatting rules, and sometimes the source is uglier (e.g., the horrible\n+-continuation in lists). Are authors now responsible for formatting any\nchanges they make to make sure they look good in asciidoc? Or are people\nwho care about the formatted output going to come along afterwards and\nsubmit fixup patches? Either way it seems like make-work.\n\nNow I'll admit it seems like make-work to me because I would not plan to\never look at the formatted output myself. But I guess I don't understand\nthe audience for this formatted output. These are APIs internal to Git\nitself. We would not generally want to install gitapi-oid-array into\n/usr/share/man, because only people actually working on Git would be\nable to use it. So it sounds like a convenience for a handful of\ndevelopers (who like to look at this manpage versus the source). It\ndoesn't seem like the cost/benefit is there.\n\nAnd if we were going to generate something external, would it make more\nsense to write in a structured format like doxygen? I am not a big fan\nof it myself, but at least from there you can generate a more richly\ninterconnected set of documentation.\n\n-Peff\n"},{"id":"358987","messageId":"CAGZ79ka=5zs2vnFhoA7NT6PjCNHm3sRUiS98Zb+rpirE3mFKUQ@mail.gmail.com","threadId":"49400","inReplyTo":"20180926185812.GD30680@sigill.intra.peff.net","subject":"Re: [PATCH 1/8] sha1-array: provide oid_array_filter","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-26T19:39:45Z","receivedAt":"2018-09-26T19:39:59Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Sep 26, 2018 at 11:58 AM Jeff King wrote:\n\n> Now I'll admit it seems like make-work to me because I would not plan to\n> ever look at the formatted output myself. But I guess I don't understand\n> the audience for this formatted output.\n\nI have been watching from the side lines so far, as I have a view that is very\nmuch like Peffs, if I were to dictate my opinion onto the project, we'd:\n* put all internal API docs into headerfiles.\n* get rid of all Documentation/technical/ that describes things\n at a function level. So the remaining things are high level docs such\n as protocol, hash transition plan, partial clones...\n\nBut I'd want to understand why we are not doing this (obvious to me)\nbest thing, I have the impression other people use the docs differently.\n\nHow are these docs (for example api-oid-array or\napi-revision-walking) used?\n\nI usually tend to use git-grep a lot when looking for a function, just to\njump to the header file and read the comment there and go around the\nheader file looking if we have a better suited thing.\n(Also I tend to like to have fewer files open, such that I prefer\na header file of reasonable size over 2 files, one docs and one header)\n\nSo when I find a function documented in Docs/technical, I would first\nread there, then go to the header to see the signature and then make\nuse of it.\n\nIf I recall an earlier discussion on this topic, I recall Junio saying that\nthis *may* pollute the header files, as for example sha1-array.h\nis easy to grok in its entirety, and makes it easy to see what\nfunctions regarding oid_arrays are available. A counter example would\nbe hashmap.h, as that requires some scrolling and skimming.\n\n> These are APIs internal to Git\n> itself. We would not generally want to install gitapi-oid-array into\n> /usr/share/man, because only people actually working on Git would be\n> able to use it. So it sounds like a convenience for a handful of\n> developers (who like to look at this manpage versus the source). It\n> doesn't seem like the cost/benefit is there.\n\nIf this type of docs makes Ævar more productive, I am all for keeping it.\n\n> And if we were going to generate something external, would it make more\n> sense to write in a structured format like doxygen? I am not a big fan\n> of it myself, but at least from there you can generate a more richly\n> interconnected set of documentation.\n\nThat sounds sensible to me as the additional burden of yet-another-tool\nis only imposed to the developers, but doxygen would provide\na better output in my experience.\n\nStefan\n"},{"id":"358988","messageId":"xmqqtvmcjaxx.fsf@gitster-ct.c.googlers.com","threadId":"49400","inReplyTo":"20180926185812.GD30680@sigill.intra.peff.net","subject":"Re: [PATCH 1/8] sha1-array: provide oid_array_filter","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-26T19:49:46Z","receivedAt":"2018-09-26T19:49:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Now I'll admit it seems like make-work to me because I would not plan to\n> ever look at the formatted output myself. But I guess I don't understand\n> the audience for this formatted output. These are APIs internal to Git\n> itself. We would not generally want to install gitapi-oid-array into\n> /usr/share/man, because only people actually working on Git would be\n> able to use it. So it sounds like a convenience for a handful of\n> developers (who like to look at this manpage versus the source). It\n> doesn't seem like the cost/benefit is there.\n>\n> And if we were going to generate something external, would it make more\n> sense to write in a structured format like doxygen? I am not a big fan\n> of it myself, but at least from there you can generate a more richly\n> interconnected set of documentation.\n\nI agree on both counts. I just like to read these in plain text\nwhile I am coding for Git (or reviewing patches coded for Git). \n\nThe reason why I have mild preference to D/technical/ over in-header\ndoc is only because I find even these asterisks at the left-side-end\ndistracting; it is not that materials in D/technical could be passed\nthrough AsciiDoc.\n\n"},{"id":"358997","messageId":"CAGZ79kbeQvLuH1nGPkf8hHbs49SmuPeme_XBdz7ek5fU5AZ9Zg@mail.gmail.com","threadId":"49400","inReplyTo":"xmqqtvmcjaxx.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/8] sha1-array: provide oid_array_filter","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-26T19:59:30Z","receivedAt":"2018-09-26T19:59:44Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"> I agree on both counts. I just like to read these in plain text\n> while I am coding for Git (or reviewing patches coded for Git).\n>\n> The reason why I have mild preference to D/technical/ over in-header\n> doc is only because I find even these asterisks at the left-side-end\n> distracting; it is not that materials in D/technical could be passed\n> through AsciiDoc.\n\n /**\n Would we be open to consider another commenting style?\n AFAICT the asterisks are a cultural thing and not enforced by\n and old compiler not understanding these comments.\n\n Just like ending the comment in an asymetric fashion ;-) */\n\n(I am not seriously proposing unbalanced comments, but for\nlonger documenting comments we may want to consider,\nomitting the asterisk?)\n"},{"id":"358999","messageId":"xmqqpnx0j9kn.fsf@gitster-ct.c.googlers.com","threadId":"49400","inReplyTo":"CAGZ79kbeQvLuH1nGPkf8hHbs49SmuPeme_XBdz7ek5fU5AZ9Zg@mail.gmail.com","subject":"Re: [PATCH 1/8] sha1-array: provide oid_array_filter","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-26T20:19:20Z","receivedAt":"2018-09-26T20:19:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n>> I agree on both counts. I just like to read these in plain text\n>> while I am coding for Git (or reviewing patches coded for Git).\n>>\n>> The reason why I have mild preference to D/technical/ over in-header\n>> doc is only because I find even these asterisks at the left-side-end\n>> distracting; it is not that materials in D/technical could be passed\n>> through AsciiDoc.\n>\n> /**\n> Would we be open to consider another commenting style?\n\nNo. You still cannot write */ in that comment section, for example,\nso you cannot do \"plain text\" still---that is not a great trade off\nto deviate from the common comment style used for other stuff.\n\nWhen I say I want plain text, I mean it. Anything you write in .h\ncannot be plain text, unless you make all readers agree with some\nconvention, and \"there is always '*' at the left edge\" is such an\nacceptable convention can learn to live with, just like everybody\nelse does ; at least, that is consistent with other comments.\n\n> (I am not seriously proposing unbalanced comments, but for\n> longer documenting comments we may want to consider,\n> omitting the asterisk?)\n\nIf this is not a serious proposal, then I'll stop wasting\neverybody's time.\n\nNow it is clear that the only pieces of consensus we got out of this\ndiscussion so far is that we want to add to CodingGuidelines, that\nmy earlier \"something like this\" is not the text we want and that we\nwant everything in the header as comment.\n\nSo let's see if we can have usable alternative, apply that and move\non.\n"},{"id":"359001","messageId":"878t3oj8em.fsf@evledraar.gmail.com","threadId":"49400","inReplyTo":"20180926185812.GD30680@sigill.intra.peff.net","subject":"On shipping more of our technical docs as manpages","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-09-26T20:44:33Z","receivedAt":"2018-09-26T20:44:38Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\n[Changing subject because this stopped being about Stefan's patches a\nwhile ago]\n\nOn Wed, Sep 26 2018, Jeff King wrote:\n\n> On Wed, Sep 26, 2018 at 08:43:14PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n>> While we're on that subject, I'd much prefer if these technical docs and\n>> other asciidoc-ish stuff we would be manpages build as part of our\n>> normal \"make doc\" target. So e.g. this one would be readable via\n>> something like \"man 3 gitapi-oid-array\".\n\n[responding out of order]\n\n> Now I'll admit it seems like make-work to me because I would not plan to\n> ever look at the formatted output myself. But I guess I don't understand\n> the audience for this formatted output. These are APIs internal to Git\n> itself. We would not generally want to install gitapi-oid-array into\n> /usr/share/man, because only people actually working on Git would be\n> able to use it. So it sounds like a convenience for a handful of\n> developers (who like to look at this manpage versus the source). It\n> doesn't seem like the cost/benefit is there.\n>\n> And if we were going to generate something external, would it make more\n> sense to write in a structured format like doxygen? I am not a big fan\n> of it myself, but at least from there you can generate a more richly\n> interconnected set of documentation.\n\nIt's useful to have a single authoritative source for all documentation\nthat's easy to search through.\n\nI don't really care where that text actually lives, i.e. whether it's\nall in *.txt to begin with, or in *.c or *.h and pulled together by some\n\"make\" step, or much how it's formatted, beyond it being easy to work\nwith. I'm not a big fan of asciidoc, but it's what we've got if we want\nformatting, inter-linking etc.\n\nMy bias here is that I've also contributed actively to the perl project\nin the past, and with that project you can get an overview of *all* of\nthe docs by typing:\n\n man perl\n\nThat includes stuff like perl585delta(1) which we'd stick in\nDocumentation/RelNotes, and \"Internals and C Language Interface\". Most\nof what we'd put in Documentation/technical/api-* & headers is in\nperlapi(1).\n\nI spent an embarrassingly long time submitting patches to git before\ndiscovering that Documentation/technical/api-*.txt even existed, I think\nsomething like 1-2 years ago.\n\nWe have very well documented stuff like strbuf.h (mostly thanks to you),\nbut how is such documentation supposed to be discovered? Something like:\n\n git grep -A20 '^/\\*$' -- *.h\n \n\n?\n\nIn terms of getting an overview it's indistinguishable from\ncomments. I.e. there's nothing like an index of:\n\n man git-api-strbuf ==> working with strings\n man git-api-sha1-array ==> list, iterate and binary lookup SHA1s\n\netc.\n\nSometimes it's obvious where to look, but as someone who uses most of\nthese APIs very occasionally (because I contribute less) I keep\n(re-)discovering various internal APIs.\n\nI'm also not in the habit of opening the *.h file for something, I\nusually just start reading the *.c and only later discover there's some\nAPI docs in the relevant *.h (or not).\n\nSo in terms of implementation I'm a big fan of the perl.git approach of\nhaving these docs as comments before the function implementation in the\n*.c file.\n\nIt means you can't avoid seeing it or updating it when source\nspelunking, and it leaves header files short, which is useful when you'd\nlike to get a general API overview without all the docs. Our documented\nheaders are quite fat, e.g. strbuf.h is 60% of the size of strbuf.c, but\nless than 20% when you strip the docs.\n\n> I'm mildly negative on this, just because it introduces extra work on\n> people writing the documentation. Now it has to follow special\n> formatting rules, and sometimes the source is uglier (e.g., the horrible\n> +-continuation in lists). Are authors now responsible for formatting any\n> changes they make to make sure they look good in asciidoc? Or are people\n> who care about the formatted output going to come along afterwards and\n> submit fixup patches? Either way it seems like make-work.\n\nThis part I'm slightly confused by. If you're just saying \"let's\ndocument stuff in *.h files in free-flowing text\", fine. But if we're\ntalking about the difference between Documentation/technical/api-*.txt\nand having the same stuff in manpages, then the stuff in api-*.txt *is*\nalready asciidoc, and we have a target to format it all up and ship it\nwith git, and distros ship that.\n\nE.g. on Debian you can \"apt install git-doc\" and browse stuff like\nfile:///usr/share/doc/git-doc/technical/api-argv-array.html which is the\nHTML formatted version, same for all the other api-*.txt docs.\n\nWe just don't have some central index of those, and they're not a\ndefault target which means the likes of our own https://git-scm.com\ndon't build them, so they're harder to find.\n\nEvery perl installation also ships perlapi and a MB or so of obscure\ninternal docs to everyone, which makes it easier to find via Google et\nal, which I find useful. So I guess I'm more on the side fence of\ndropping a hypothetical gitapi-oid-array into /usr/share/man than you\nare.\n\nANYWAY\n\nThis E-mail is much longer than I intended, sorry about that. I didn't\njust mean to bitch about how we ship docs, but I was wondering if there\nwas a desire to move to something like what I've outlined above, or\nwhether the status quo was mostly by design and intended.\n\nI just thought I'd write this rather lengthy E-Mails because this is one\nof the rare areas where you can fully describe an idea in E-Mail without\nwriting any patches.\n\nIf the consensus is that something like the exhaustive index \"perldoc\nperl\" provides wouldn't be useful for git I can just drop this, but if\npeople are enthusiastic about having that it would be useful to work on\nit...\n"},{"id":"359002","messageId":"20180926205148.GA12340@sigill.intra.peff.net","threadId":"49400","inReplyTo":"xmqqpnx0j9kn.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/8] sha1-array: provide oid_array_filter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-09-26T20:51:48Z","receivedAt":"2018-09-26T20:51:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 26, 2018 at 01:19:20PM -0700, Junio C Hamano wrote:\n\n> > /**\n> > Would we be open to consider another commenting style?\n> \n> No. You still cannot write */ in that comment section, for example,\n> so you cannot do \"plain text\" still---that is not a great trade off\n> to deviate from the common comment style used for other stuff.\n\nWe can do:\n\n#if HEADER_DOC\n\nWrite anything here except #endif!\n\n#endif /* HEADER_DOC */\n\nBut I actually think that is more jarring than a big comment (when I am\nreading C code, I expect to see C-like things mostly.\n\nI do agree with you that a true plain text file is nicer in terms of\nfreedom to format, etc. But IMHO the tradeoff is easily worth it to keep\nthe related content (function declarations and their documentation)\nclose together.\n\n-Peff\n"},{"id":"359007","messageId":"CAGZ79kauPS58UpryN+cfAzoiDo+Ee98_RLOnHPJhTUXv5OQM0w@mail.gmail.com","threadId":"49400","inReplyTo":"xmqqpnx0j9kn.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/8] sha1-array: provide oid_array_filter","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-26T21:22:10Z","receivedAt":"2018-09-26T21:22:24Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"> > (I am not seriously proposing unbalanced comments, but for\n> > longer documenting comments we may want to consider,\n> > omitting the asterisk?)\n>\n> If this is not a serious proposal, then I'll stop wasting\n> everybody's time.\n\nI proposed two things, one of which I was not serious about.\n\nThe other has downsides as both you and Peff point out.\nSo living with comments may not be the worst after all.\n\nI'll read&reply to Aevars proposal now.\n"},{"id":"359010","messageId":"xmqqefdgj5tt.fsf@gitster-ct.c.googlers.com","threadId":"49400","inReplyTo":"878t3oj8em.fsf@evledraar.gmail.com","subject":"Re: On shipping more of our technical docs as manpages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-26T21:40:14Z","receivedAt":"2018-09-26T21:40:20Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> So in terms of implementation I'm a big fan of the perl.git approach of\n> having these docs as comments before the function implementation in the\n> *.c file.\n>\n> It means you can't avoid seeing it or updating it when source\n> spelunking, and it leaves header files short, which is useful when you'd\n> like to get a general API overview without all the docs. Our documented\n> headers are quite fat, e.g. strbuf.h is 60% of the size of strbuf.c, but\n> less than 20% when you strip the docs.\n\nJust like you I do not care too much where the authoritative source\nlives, and I do agree with Peff that separate text file is much more\nlikely to go stale wrt the code, and that it is an acceptable trade\noff to write docs in comment and live with slightly less freedom\nthan writing in plain text in order to keep the docs and source\ncloser together. I do not think between Peff and me, neither of us\nwere contrasting in-header vs in-code comments.\n\nAnd I tend to agree that it makes it even harder to let doc and code\ndrift apart to have the doc near the implementation (as opposed to\nin the header). It probably makes the result less useful, unless\nthe project invests in making the result accessible like \"man\nperlapi\" does, than having them in the header for people who view\nheaders as guide to users of API, though.\n\n> We just don't have some central index of those, and they're not a\n> default target which means the likes of our own https://git-scm.com\n> don't build them, so they're harder to find.\n>\n> Every perl installation also ships perlapi and a MB or so of obscure\n> internal docs to everyone, which makes it easier to find via Google et\n> al, which I find useful. So I guess I'm more on the side fence of\n> dropping a hypothetical gitapi-oid-array into /usr/share/man than you\n> are.\n\nRegardless of where we place docs (between .c/.h or .txt), making\nthem indexable and investing in make target to actually produce\nthese docs with ToC is different matter. I do not think people who\nactually spent time on our docs had enough inclination to do so\nthemselves, but that should not prevent you or other like-minded\npeople from leading by example.\n\n"},{"id":"359020","messageId":"CAGZ79kZOZL24Q=L-XkdrKRMjNg=dynQ+67qyD+iceg7GLru3yw@mail.gmail.com","threadId":"49400","inReplyTo":"878t3oj8em.fsf@evledraar.gmail.com","subject":"Re: On shipping more of our technical docs as manpages","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-26T23:21:28Z","receivedAt":"2018-09-26T23:21:41Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Sep 26, 2018 at 1:44 PM Ævar Arnfjörð Bjarmason\n wrote:\n\n> > And if we were going to generate something external, would it make more\n> > sense to write in a structured format like doxygen? I am not a big fan\n> > of it myself, but at least from there you can generate a more richly\n> > interconnected set of documentation.\n>\n> It's useful to have a single authoritative source for all documentation\n> that's easy to search through.\n\nIf that is the case I would propose to keep it all in header files and\norganize the headers.\n\n> That includes stuff like perl585delta(1) which we'd stick in\n> Documentation/RelNotes, and \"Internals and C Language Interface\". Most\n> of what we'd put in Documentation/technical/api-* & headers is in\n> perlapi(1).\n\nThis seems cool, but was also a recent introduction?\nperl400delta seems to yield nothing for me (which may be because\nI do not have an old version of perl installed?)\n\n>\n> Sometimes it's obvious where to look, but as someone who uses most of\n> these APIs very occasionally (because I contribute less) I keep\n> (re-)discovering various internal APIs.\n\nSometimes I have the same feeling. Maybe more structure in the\nsource files would help (e.g. datastructures/{strbuf, string-list}.h\nand objects/{packfile.h, sha1*} ?\n\n> Every perl installation also ships perlapi and a MB or so of obscure\n> internal docs to everyone, which makes it easier to find via Google et\n> al, which I find useful. So I guess I'm more on the side fence of\n> dropping a hypothetical gitapi-oid-array into /usr/share/man than you\n> are.\n>\n\nPersonally I would not want to ship our internal docs everywhere\nas it seems like overly wasteful. But then, only my early days\nof childhood were guided by Internet that is not available anywhere\nand everywhere. Today I would just take for granted that I can lookup\nthings in github/git/git when I am in not at my regular desk.\n\n> ANYWAY\n>\n> This E-mail is much longer than I intended, sorry about that. I didn't\n> just mean to bitch about how we ship docs, but I was wondering if there\n> was a desire to move to something like what I've outlined above, or\n> whether the status quo was mostly by design and intended.\n>\n> I just thought I'd write this rather lengthy E-Mails because this is one\n> of the rare areas where you can fully describe an idea in E-Mail without\n> writing any patches.\n>\n> If the consensus is that something like the exhaustive index \"perldoc\n> perl\" provides wouldn't be useful for git I can just drop this, but if\n> people are enthusiastic about having that it would be useful to work on\n> it...\n\nI agree with Junio, as that the discoverability is unrelated to where to store\nthe actual docs, Documentation/technical/* has the advantage that it\nonly has \"good\" stuff, i.e. some topic that someone cared enough to\nwrite about and it is easy to guess if it is relevant to your need.\nIn *.h, we have a lot of false positives (xdiff-interface.h or cache.h\njust pollute the searching space when looking for suitable storage\nclasses.)\n\nSo I wonder if we'd want to have a list (similar as in\ncommand-list.txt) that describes the header files and gives\nsome classification of them, for example one class could be the\ndata structures (strbuf, stringlist, hashmap), algorithms\n(diff, range-diff), OS specific stuff (run-command, trace, alloc)\nor Git specific things (apply, fetch, submodule)\n"},{"id":"359032","messageId":"20180927062011.GA1318@sigill.intra.peff.net","threadId":"49400","inReplyTo":"878t3oj8em.fsf@evledraar.gmail.com","subject":"Re: On shipping more of our technical docs as manpages","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-09-27T06:20:12Z","receivedAt":"2018-09-27T06:20:28Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 26, 2018 at 10:44:33PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> My bias here is that I've also contributed actively to the perl project\n> in the past, and with that project you can get an overview of *all* of\n> the docs by typing:\n> \n> man perl\n> \n> That includes stuff like perl585delta(1) which we'd stick in\n> Documentation/RelNotes, and \"Internals and C Language Interface\". Most\n> of what we'd put in Documentation/technical/api-* & headers is in\n> perlapi(1).\n\nI like the perl documentation, too. But I think most of what you're\ntalking about there is the public API, where you have many readers. But\nhere we're talking about internal functions (albeit ones we hope to see\nreused across the codebase). My concern is mostly: is the work in\nmaintaining this documentation-formatting system worth the number of\nreaders it will get?\n\nThere are two numbers there, and I'm guessing at both of them. ;)\n\nIf you're interested in pulling documentation out of the header files\nand generating asciidoc from it, I'm happy to at least try keeping it up\nto date. When we started putting this information into header files, we\nused \"/**\" to start the comment, as a special marker to indicate it was\nworth pulling out. I don't know how well we've maintained that\nconvention, but it's a starting point.\n\n> I spent an embarrassingly long time submitting patches to git before\n> discovering that Documentation/technical/api-*.txt even existed, I think\n> something like 1-2 years ago.\n\nThat's another thing that I think is improved by keeping the\ndocumentation and code together. If you find one, you find the other.\nYou just have to \"somehow\" find one, which is what you're getting at\nbelow.\n\n> We have very well documented stuff like strbuf.h (mostly thanks to you),\n> but how is such documentation supposed to be discovered? Something like:\n> \n> git grep -A20 '^/\\*$' -- *.h\n> \n> \n> ?\n> \n> In terms of getting an overview it's indistinguishable from\n> comments. I.e. there's nothing like an index of:\n> \n> man git-api-strbuf ==> working with strings\n> man git-api-sha1-array ==> list, iterate and binary lookup SHA1s\n\nI agree that is a problem, especially for contributors less familiar\nwith the code base. But I think generating an index is a separate (and\nmuch easier) problem than formatting all of the documentation.\n\nWe already have the /** convention I mentioned above. Could we have\nanother micro-format like:\n\n /** API:strbuf - working with strings */\n\nin each header file? That would make generating such an index pretty\ntrivial.\n\n> I'm also not in the habit of opening the *.h file for something, I\n> usually just start reading the *.c and only later discover there's some\n> API docs in the relevant *.h (or not).\n\nInteresting. I'm not totally opposed to putting the documentation\nalongside the actual code. It does feel a bit cluttered to me, but I\nthink you're right that it keeps everything as close together as\npossible.\n\n> It means you can't avoid seeing it or updating it when source\n> spelunking, and it leaves header files short, which is useful when you'd\n> like to get a general API overview without all the docs. Our documented\n> headers are quite fat, e.g. strbuf.h is 60% of the size of strbuf.c, but\n> less than 20% when you strip the docs.\n\nI don't use folds in my editor, and I guess nobody else in this thread\ndoes, either. But they may be a reasonable tool for \"wow, there are\ncomments, declarations, and definitions all together and I just want to\nview one of them\". In vim, try \"set foldmethod=syntax\" and then \"zc\" to\nclose the folds.\n\nI kind of hate it myself, but just another option to explore.\n\n> > I'm mildly negative on this, just because it introduces extra work on\n> > people writing the documentation. Now it has to follow special\n> > formatting rules, and sometimes the source is uglier (e.g., the horrible\n> > +-continuation in lists). Are authors now responsible for formatting any\n> > changes they make to make sure they look good in asciidoc? Or are people\n> > who care about the formatted output going to come along afterwards and\n> > submit fixup patches? Either way it seems like make-work.\n> \n> This part I'm slightly confused by. If you're just saying \"let's\n> document stuff in *.h files in free-flowing text\", fine. But if we're\n> talking about the difference between Documentation/technical/api-*.txt\n> and having the same stuff in manpages, then the stuff in api-*.txt *is*\n> already asciidoc, and we have a target to format it all up and ship it\n> with git, and distros ship that.\n\nYes, the \"free-flowing text\" thing is more or less what I'm saying. I\nsuspect what's in technical/* currently probably has formatting\nproblems, and people update it (if we're lucky!) without regard to what\nthe result looks like.\n\n> E.g. on Debian you can \"apt install git-doc\" and browse stuff like\n> file:///usr/share/doc/git-doc/technical/api-argv-array.html which is the\n> HTML formatted version, same for all the other api-*.txt docs.\n\nIMHO this is just silly. What am I as an end user going to do with\napi-argv-array.html?\n\n> This E-mail is much longer than I intended, sorry about that. I didn't\n> just mean to bitch about how we ship docs, but I was wondering if there\n> was a desire to move to something like what I've outlined above, or\n> whether the status quo was mostly by design and intended.\n\nI think we're actually half-way between status quos. The stuff in\ntechnical/api-* has existed for a long time. We've been slowly moving\nstuff over to header files, but there's still a bit of stuff there.\n\nBuilding those files as asciidoc is one of those things that just\ncropped up along the way. I have never thought it was useful myself, but\nsomebody bothered to write the patches, so OK. The person who did so is\nnot even somebody who has otherwise worked on the project, AFAICT.\n\n-Peff\n"},{"id":"359033","messageId":"20180927063404.GB220288@aiede.svl.corp.google.com","threadId":"49400","inReplyTo":"20180927062011.GA1318@sigill.intra.peff.net","subject":"Re: On shipping more of our technical docs as manpages","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-09-27T06:34:04Z","receivedAt":"2018-09-27T06:34:10Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJeff King wrote:\n> On Wed, Sep 26, 2018 at 10:44:33PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n>> In terms of getting an overview it's indistinguishable from\n>> comments. I.e. there's nothing like an index of:\n>>\n>> man git-api-strbuf ==> working with strings\n>> man git-api-sha1-array ==> list, iterate and binary lookup SHA1s\n>\n> I agree that is a problem, especially for contributors less familiar\n> with the code base. But I think generating an index is a separate (and\n> much easier) problem than formatting all of the documentation.\n>\n> We already have the /** convention I mentioned above. Could we have\n> another micro-format like:\n>\n> /** API:strbuf - working with strings */\n>\n> in each header file? That would make generating such an index pretty\n> trivial.\n\nCan you spell out the problem for me a little more? E.g. if we had a\nconvention that the first comment in strbuf.h should say\n\n\t/* strbuf - Git's standard string type */\n\nor even just\n\n\t/* Git's standard string type */\n\nwould that do the trick?\n\n>> I'm also not in the habit of opening the *.h file for something, I\n>> usually just start reading the *.c and only later discover there's some\n>> API docs in the relevant *.h (or not).\n>\n> Interesting. I'm not totally opposed to putting the documentation\n> alongside the actual code. It does feel a bit cluttered to me, but I\n> think you're right that it keeps everything as close together as\n> possible.\n\nI've experienced projects following both conventions. One thing I like\na lot about documentation in the header file is that it encourages\npeople to make the API documentation self-contained. That is, you\ndescribe the contract in a way that doesn't require reading the\nimplementation for details.\n\nIt took me a while to get used to this kind of convention but now I\nreally like it. So I really do prefer to keep putting API\ndocumentation in the header files in Git. (Implementation\ndocumentation in the source files is of course also very welcome.)\n\n>> It means you can't avoid seeing it or updating it when source\n>> spelunking, and it leaves header files short, which is useful when you'd\n>> like to get a general API overview without all the docs. Our documented\n>> headers are quite fat, e.g. strbuf.h is 60% of the size of strbuf.c, but\n>> less than 20% when you strip the docs.\n>\n> I don't use folds in my editor, and I guess nobody else in this thread\n> does, either. But they may be a reasonable tool for \"wow, there are\n> comments, declarations, and definitions all together and I just want to\n> view one of them\". In vim, try \"set foldmethod=syntax\" and then \"zc\" to\n> close the folds.\n\nI use kythe to get an outline view of the header files.\n\n[...]\n>> E.g. on Debian you can \"apt install git-doc\" and browse stuff like\n>> file:///usr/share/doc/git-doc/technical/api-argv-array.html which is the\n>> HTML formatted version, same for all the other api-*.txt docs.\n>\n> IMHO this is just silly. What am I as an end user going to do with\n> api-argv-array.html?\n\nIn Debian we just ship all the docs. For something like\ntechnical/pack-heuristics, it's quite nice. For the API docs, I think\nthey belong in the headers.\n\nThanks,\nJonathan\n"},{"id":"359035","messageId":"20180927064055.GA2865@sigill.intra.peff.net","threadId":"49400","inReplyTo":"20180927063404.GB220288@aiede.svl.corp.google.com","subject":"Re: On shipping more of our technical docs as manpages","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-09-27T06:40:55Z","receivedAt":"2018-09-27T06:40:59Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 26, 2018 at 11:34:04PM -0700, Jonathan Nieder wrote:\n\n> > We already have the /** convention I mentioned above. Could we have\n> > another micro-format like:\n> >\n> > /** API:strbuf - working with strings */\n> >\n> > in each header file? That would make generating such an index pretty\n> > trivial.\n> \n> Can you spell out the problem for me a little more? E.g. if we had a\n> convention that the first comment in strbuf.h should say\n> \n> \t/* strbuf - Git's standard string type */\n> \n> or even just\n> \n> \t/* Git's standard string type */\n> \n> would that do the trick?\n\nI assumed that not all header files would want to advertise themselves\nas a public API, so we'd want to some way to differentiate these from\nfirst comments in \"other\" header files (and I assumed we did not want a\nseparate list of \"these are the API files to go into the index\", at\nwhich point you might as well just write \"standard string type\" in that\nlist).\n\nBut I'm happy with any convention that lets Ævar generate the index he\nwants.\n\n> > Interesting. I'm not totally opposed to putting the documentation\n> > alongside the actual code. It does feel a bit cluttered to me, but I\n> > think you're right that it keeps everything as close together as\n> > possible.\n> \n> I've experienced projects following both conventions. One thing I like\n> a lot about documentation in the header file is that it encourages\n> people to make the API documentation self-contained. That is, you\n> describe the contract in a way that doesn't require reading the\n> implementation for details.\n> \n> It took me a while to get used to this kind of convention but now I\n> really like it. So I really do prefer to keep putting API\n> documentation in the header files in Git. (Implementation\n> documentation in the source files is of course also very welcome.)\n\nYeah, I'd agree with all of that.\n\n> I use kythe to get an outline view of the header files.\n\nLooks neat. Doesn't seem to be package for Debian, though. ;)\n\n> In Debian we just ship all the docs. For something like\n> technical/pack-heuristics, it's quite nice. For the API docs, I think\n> they belong in the headers.\n\nYes, I'd agree with that. About half of technical/* is design docs, etc,\nthat might be of use to random folks. But the internal code APIs are\nreally only useful if you have an actual clone of git.git.\n\n-Peff\n"},{"id":"359036","messageId":"20180927064836.GA13130@sigill.intra.peff.net","threadId":"49400","inReplyTo":"20180926185812.GD30680@sigill.intra.peff.net","subject":"Re: [PATCH 1/8] sha1-array: provide oid_array_filter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-09-27T06:48:37Z","receivedAt":"2018-09-27T06:48:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 26, 2018 at 02:58:12PM -0400, Jeff King wrote:\n\n> And if we were going to generate something external, would it make more\n> sense to write in a structured format like doxygen? I am not a big fan\n> of it myself, but at least from there you can generate a more richly\n> interconnected set of documentation.\n\nBy the way, I tried running Doxygen on the current code-base. I _still_\nhate the presentation of it (mostly because it's jarring to jump out of\nmy terminal to a web browser). But it actually does an OK job of showing\nstrbuf.h, with our overview attached to the struct and individual\nfunctions correctly documented. There are a couple of formatting\nhiccups, but it's actually pretty close.\n\n-Peff\n"},{"id":"359039","messageId":"874lebjp02.fsf@evledraar.gmail.com","threadId":"49400","inReplyTo":"CAGZ79kZOZL24Q=L-XkdrKRMjNg=dynQ+67qyD+iceg7GLru3yw@mail.gmail.com","subject":"Re: On shipping more of our technical docs as manpages","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-09-27T08:58:21Z","receivedAt":"2018-09-27T08:58:26Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Sep 26 2018, Stefan Beller wrote:\n\n> On Wed, Sep 26, 2018 at 1:44 PM Ævar Arnfjörð Bjarmason\n> wrote:\n>\n>> > And if we were going to generate something external, would it make more\n>> > sense to write in a structured format like doxygen? I am not a big fan\n>> > of it myself, but at least from there you can generate a more richly\n>> > interconnected set of documentation.\n>>\n>> It's useful to have a single authoritative source for all documentation\n>> that's easy to search through.\n>\n> If that is the case I would propose to keep it all in header files and\n> organize the headers.\n>\n>> That includes stuff like perl585delta(1) which we'd stick in\n>> Documentation/RelNotes, and \"Internals and C Language Interface\". Most\n>> of what we'd put in Documentation/technical/api-* & headers is in\n>> perlapi(1).\n>\n> This seems cool, but was also a recent introduction?\n> perl400delta seems to yield nothing for me (which may be because\n> I do not have an old version of perl installed?)\n\nDepends on what you think is \"recent\" I suppose. Perl 5.4 is the first\nversion where deltas in POD format started being maintained\nconsistently, that version was released in mid-1997. Perl 4 was released\nin 1991, see \"perldoc perlhist\". So ~everything consistently in POD has\nbeen the case for ~20 years.\n"},{"id":"359073","messageId":"xmqqwor6hmgk.fsf@gitster-ct.c.googlers.com","threadId":"49400","inReplyTo":"20180927062011.GA1318@sigill.intra.peff.net","subject":"Re: On shipping more of our technical docs as manpages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-27T17:36:11Z","receivedAt":"2018-09-27T17:36:18Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> If you're interested in pulling documentation out of the header files\n> and generating asciidoc from it, I'm happy to at least try keeping it up\n> to date. When we started putting this information into header files, we\n> used \"/**\" to start the comment, as a special marker to indicate it was\n> worth pulling out. I don't know how well we've maintained that\n> convention, but it's a starting point.\n\nI noticed some people add these extra asterisk at the beginning of\ncomment, but I do not recall that we declared it is a convention we\nadopt, so I'd rather be surprised if we've \"maintained\" it.\n\nPlease have it in CodingGuidelines or somewhere once this thread\nsettles and we decide to keep that convention (I have no problem\nwith the convention; it is just I personally didn't think it was\nworth doing myself at least until now that we might have found\nsomebody who wants to make use of the markings).\n"},{"id":"359083","messageId":"20180927182528.GB2468@sigill.intra.peff.net","threadId":"49400","inReplyTo":"xmqqwor6hmgk.fsf@gitster-ct.c.googlers.com","subject":"Re: On shipping more of our technical docs as manpages","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-09-27T18:25:29Z","receivedAt":"2018-09-27T18:25:32Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 27, 2018 at 10:36:11AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > If you're interested in pulling documentation out of the header files\n> > and generating asciidoc from it, I'm happy to at least try keeping it up\n> > to date. When we started putting this information into header files, we\n> > used \"/**\" to start the comment, as a special marker to indicate it was\n> > worth pulling out. I don't know how well we've maintained that\n> > convention, but it's a starting point.\n> \n> I noticed some people add these extra asterisk at the beginning of\n> comment, but I do not recall that we declared it is a convention we\n> adopt, so I'd rather be surprised if we've \"maintained\" it.\n\nTrue. _I_ declared it as a convention and used it when I migrated some\nof the initial api-* documents, but I don't know if anybody else\nfollowed that lead.\n\nFWIW, it is not my own convention, but one used by other tools like\ndoxygen (which in turn got it from javadoc, I think).\n\n> Please have it in CodingGuidelines or somewhere once this thread\n> settles and we decide to keep that convention (I have no problem\n> with the convention; it is just I personally didn't think it was\n> worth doing myself at least until now that we might have found\n> somebody who wants to make use of the markings).\n\nYeah, this is basically why I hadn't pushed harder for it. If nobody is\nactually extracting based on the convention, there is not much point. So\nI was waiting for somebody to show up with an interest in using an\nextraction tool.\n\n-Peff\n"},{"id":"359122","messageId":"20180927212727.128214-1-sbeller@google.com","threadId":"49400","inReplyTo":"xmqqwor6hmgk.fsf@gitster-ct.c.googlers.com","subject":"[PATCH] Documentation/CodingGuidelines: How to document new APIs","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-27T21:27:27Z","receivedAt":"2018-09-27T21:27:39Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"There are different opinions on how to document an API properly.\nDiscussion turns out nobody disagrees with documenting new APIs on the\nfunction level in the header file and high level concepts in\nDocumentation/technical, so let's state that in the guidelines.\n\nSigned-off-by: Stefan Beller \n---\n\n This is how I would approach the documentation patch.\n \n Thanks,\n Stefan\n \n Documentation/CodingGuidelines | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n \ndiff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\nindex 48aa4edfbd..15bfb8bbb8 100644\n--- a/Documentation/CodingGuidelines\n+++ b/Documentation/CodingGuidelines\n@@ -358,7 +358,10 @@ For C programs:\n string_list for sorted string lists, a hash map (mapping struct\n objects) named \"struct decorate\", amongst other things.\n \n- - When you come up with an API, document it.\n+ - When you come up with an API, document the functions in the header\n+ and highlevel concepts in Documentation/technical/. Note that this\n+ directory still contains function level documentation as historically\n+ everything was documented there.\n \n - The first #include in C files, except in platform specific compat/\n implementations, must be either \"git-compat-util.h\", \"cache.h\" or\n-- \n2.19.0\n\n"},{"id":"359125","messageId":"xmqqftxufwgp.fsf@gitster-ct.c.googlers.com","threadId":"49400","inReplyTo":"20180927212727.128214-1-sbeller@google.com","subject":"Re: [PATCH] Documentation/CodingGuidelines: How to document new APIs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-27T21:43:02Z","receivedAt":"2018-09-27T21:43:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> There are different opinions on how to document an API properly.\n> Discussion turns out nobody disagrees with documenting new APIs on the\n> function level in the header file and high level concepts in\n\nLooks conditionally good ;-) Thanks for keeping the ball rolling.\n\nI didn't get an impression that \"next to implementation\" vs \"in\nheader to force people write clearly\" was totally settled. I'd wait\nuntil Ævar says something on this ;-)\n\n\n> Documentation/technical, so let's state that in the guidelines.\n>\n> Signed-off-by: Stefan Beller \n> ---\n>\n> This is how I would approach the documentation patch.\n> \n> Thanks,\n> Stefan\n> \n> Documentation/CodingGuidelines | 5 ++++-\n> 1 file changed, 4 insertions(+), 1 deletion(-)\n> \n> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\n> index 48aa4edfbd..15bfb8bbb8 100644\n> --- a/Documentation/CodingGuidelines\n> +++ b/Documentation/CodingGuidelines\n> @@ -358,7 +358,10 @@ For C programs:\n> string_list for sorted string lists, a hash map (mapping struct\n> objects) named \"struct decorate\", amongst other things.\n> \n> - - When you come up with an API, document it.\n> + - When you come up with an API, document the functions in the header\n> + and highlevel concepts in Documentation/technical/. Note that this\n> + directory still contains function level documentation as historically\n> + everything was documented there.\n> \n> - The first #include in C files, except in platform specific compat/\n> implementations, must be either \"git-compat-util.h\", \"cache.h\" or\n"},{"id":"359152","messageId":"87mus2insr.fsf@evledraar.gmail.com","threadId":"49400","inReplyTo":"xmqqftxufwgp.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] Documentation/CodingGuidelines: How to document new APIs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-09-27T22:21:56Z","receivedAt":"2018-09-27T22:22:01Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Sep 27 2018, Junio C Hamano wrote:\n\n> Stefan Beller writes:\n>\n>> There are different opinions on how to document an API properly.\n>> Discussion turns out nobody disagrees with documenting new APIs on the\n>> function level in the header file and high level concepts in\n>\n> Looks conditionally good ;-) Thanks for keeping the ball rolling.\n>\n> I didn't get an impression that \"next to implementation\" vs \"in\n> header to force people write clearly\" was totally settled. I'd wait\n> until Ævar says something on this ;-)\n\nI think this patch is good and should go in. Having it be consistent is\na good thing, and we're drifting more in the *.h direction than *.txt.\n\nThe \"next to implementation\" suggestion was in the context of what the\nperl project does, to get good use out of that we'd a) need to move all\nthe *.h docs and *.txt to *.c b) have something to slurp up those docs\nso they're formatted. I'm not about to submit any patches for that.\n\n>> Documentation/technical, so let's state that in the guidelines.\n>>\n>> Signed-off-by: Stefan Beller \n>> ---\n>>\n>> This is how I would approach the documentation patch.\n>>\n>> Thanks,\n>> Stefan\n>>\n>> Documentation/CodingGuidelines | 5 ++++-\n>> 1 file changed, 4 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\n>> index 48aa4edfbd..15bfb8bbb8 100644\n>> --- a/Documentation/CodingGuidelines\n>> +++ b/Documentation/CodingGuidelines\n>> @@ -358,7 +358,10 @@ For C programs:\n>> string_list for sorted string lists, a hash map (mapping struct\n>> objects) named \"struct decorate\", amongst other things.\n>>\n>> - - When you come up with an API, document it.\n>> + - When you come up with an API, document the functions in the header\n>> + and highlevel concepts in Documentation/technical/. Note that this\n>> + directory still contains function level documentation as historically\n>> + everything was documented there.\n>>\n>> - The first #include in C files, except in platform specific compat/\n>> implementations, must be either \"git-compat-util.h\", \"cache.h\" or\n"},{"id":"359162","messageId":"20180927232732.GA216193@aiede.svl.corp.google.com","threadId":"49400","inReplyTo":"20180927212727.128214-1-sbeller@google.com","subject":"Re: [PATCH] Documentation/CodingGuidelines: How to document new APIs","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-09-27T23:27:32Z","receivedAt":"2018-09-27T23:27:38Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nStefan Beller wrote:\n\n> There are different opinions on how to document an API properly.\n> Discussion turns out nobody disagrees with documenting new APIs on the\n> function level in the header file and high level concepts in\n> Documentation/technical, so let's state that in the guidelines.\n\nI disagree with this. I think we should put all the API docs in the\nheader file, like we're already doing in e.g. strbuf.h.\n\nDo you have a link to where in the discussion this split-docs strategy\nwas decided?\n\nThanks,\nJonathan\n"},{"id":"359170","messageId":"20180928011129.GD11281@sigill.intra.peff.net","threadId":"49400","inReplyTo":"20180927232732.GA216193@aiede.svl.corp.google.com","subject":"Re: [PATCH] Documentation/CodingGuidelines: How to document new APIs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-09-28T01:11:29Z","receivedAt":"2018-09-28T01:11:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 27, 2018 at 04:27:32PM -0700, Jonathan Nieder wrote:\n\n> > There are different opinions on how to document an API properly.\n> > Discussion turns out nobody disagrees with documenting new APIs on the\n> > function level in the header file and high level concepts in\n> > Documentation/technical, so let's state that in the guidelines.\n> \n> I disagree with this. I think we should put all the API docs in the\n> header file, like we're already doing in e.g. strbuf.h.\n\nMe too.\n\nI think other high-level concepts that are _not_ APIs (e.g., file\nformats, protocol, etc) could go into technical/.\n\n(Though actually, those are the thing that I would not mind at all if\nthey get formatted into real manpages and shipped to end users. We do\nnot expect most users to dissect our file-formats, but they could at\nleast be useful to somebody poking around).\n\n> Do you have a link to where in the discussion this split-docs strategy\n> was decided?\n\nI think the purpose of this patch is to spur people towards a decision.\n:)\n\n-Peff\n"},{"id":"359218","messageId":"xmqq5zypefcp.fsf@gitster-ct.c.googlers.com","threadId":"49400","inReplyTo":"20180928011129.GD11281@sigill.intra.peff.net","subject":"Re: [PATCH] Documentation/CodingGuidelines: How to document new APIs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-28T16:50:14Z","receivedAt":"2018-09-28T16:50:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Sep 27, 2018 at 04:27:32PM -0700, Jonathan Nieder wrote:\n>\n>> > There are different opinions on how to document an API properly.\n>> > Discussion turns out nobody disagrees with documenting new APIs on the\n>> > function level in the header file and high level concepts in\n>> > Documentation/technical, so let's state that in the guidelines.\n>> \n>> I disagree with this. I think we should put all the API docs in the\n>> header file, like we're already doing in e.g. strbuf.h.\n>\n> Me too.\n>\n> I think other high-level concepts that are _not_ APIs (e.g., file\n> formats, protocol, etc) could go into technical/.\n>\n> (Though actually, those are the thing that I would not mind at all if\n> they get formatted into real manpages and shipped to end users. We do\n> not expect most users to dissect our file-formats, but they could at\n> least be useful to somebody poking around).\n>\n>> Do you have a link to where in the discussion this split-docs strategy\n>> was decided?\n>\n> I think the purpose of this patch is to spur people towards a decision.\n> :)\n\nOK.\n\n-- >8 --\nSubject: CodingGuidelines: document the API in *.h files\n\nIt makes it harder to let the API description and the reality drift\napart if the doc is kept close to the implementation or the header\nof the API. We have been slowly migrating API docs out of the\nDocumentation/technical/api-* to *.h files, and the development\ncommunity generally considers that how inline docs in strbuf.h is\ndone the best current practice.\n\nWe recommend documenting in the header over documenting near the\nimplementation to encourage people to write the docs that are\nreadable without peeking at the implemention.\n\nSigned-off-by: Junio C Hamano \n---\n Documentation/CodingGuidelines | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\nindex 6d265327c9..e87090c849 100644\n--- a/Documentation/CodingGuidelines\n+++ b/Documentation/CodingGuidelines\n@@ -385,7 +385,11 @@ For C programs:\n string_list for sorted string lists, a hash map (mapping struct\n objects) named \"struct decorate\", amongst other things.\n \n- - When you come up with an API, document it.\n+ - When you come up with an API, document it the functions and the\n+ structures in the header file that exposes the API to its callers.\n+ Use what is in \"strbuf.h\" as a model to decide the appropriate tone\n+ in which the description is given, and the level of details to put\n+ in the description.\n \n - The first #include in C files, except in platform specific compat/\n implementations, must be either \"git-compat-util.h\", \"cache.h\" or\n"},{"id":"359222","messageId":"CAGZ79kZzR+n+8ODs3SBhOEXFbOmccugOampzkpFKGoEXjggEwQ@mail.gmail.com","threadId":"49400","inReplyTo":"20180928011129.GD11281@sigill.intra.peff.net","subject":"Re: [PATCH] Documentation/CodingGuidelines: How to document new APIs","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-28T17:14:12Z","receivedAt":"2018-09-28T17:14:27Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Sep 27, 2018 at 6:11 PM Jeff King wrote:\n>\n> On Thu, Sep 27, 2018 at 04:27:32PM -0700, Jonathan Nieder wrote:\n>\n> > > There are different opinions on how to document an API properly.\n> > > Discussion turns out nobody disagrees with documenting new APIs on the\n> > > function level in the header file and high level concepts in\n> > > Documentation/technical, so let's state that in the guidelines.\n> >\n> > I disagree with this. I think we should put all the API docs in the\n> > header file, like we're already doing in e.g. strbuf.h.\n>\n> Me too.\n>\n> I think other high-level concepts that are _not_ APIs (e.g., file\n> formats, protocol, etc) could go into technical/.\n\nThat is what I meant with high level concepts. Anything that talks\nabout or implies the existence of a function is clearly header level.\n\n> (Though actually, those are the thing that I would not mind at all if\n> they get formatted into real manpages and shipped to end users. We do\n> not expect most users to dissect our file-formats, but they could at\n> least be useful to somebody poking around).\n\nFormats are sensible thing to present to the end user. I was also thinking\nabout partial-clone, which is a concept rather than a format.\n\n>\n> > Do you have a link to where in the discussion this split-docs strategy\n> > was decided?\n>\n> I think the purpose of this patch is to spur people towards a decision.\n> :)\n\nFor me it might end up in an exercise of writing clear and concise English.\nThough after reading Junios patch below, I would hope that may find\nconsensus.\n\nStefan\n"},{"id":"359224","messageId":"20180928173033.159192-1-sbeller@google.com","threadId":"49400","inReplyTo":"xmqq5zypefcp.fsf@gitster-ct.c.googlers.com","subject":"[PATCH] strbuf.h: format according to coding guidelines","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-28T17:30:33Z","receivedAt":"2018-09-28T17:30:40Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The previous patch suggested the strbuf header to be the leading example\nof how we would want our APIs to be documented. This may lead to some\nscrutiny of that code and the coding style (which is different from the\nAPI documentation style) and hence might be taken as an example on how\nto format code as well.\n\nSo let's format strbuf.h in a way that we'd like to see:\n* omit the extern keyword from function declarations\n* name all parameters (usually the parameters are obvious from its type,\n but consider exceptions like\n `int strbuf_getwholeline_fd(struct strbuf *, int, int);`\n* break overly long lines\n\nSigned-off-by: Stefan Beller \n---\n\nMaybe this on top of Junios guideline patch?\n\nThanks,\nStefan\n\n strbuf.h | 148 ++++++++++++++++++++++++++++++-------------------------\n 1 file changed, 81 insertions(+), 67 deletions(-)\n\ndiff --git a/strbuf.h b/strbuf.h\nindex 60a35aef16..bf18fddb5b 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -87,7 +87,7 @@ struct object_id;\n * Initialize the structure. The second parameter can be zero or a bigger\n * number to allocate memory, in case you want to prevent further reallocs.\n */\n-extern void strbuf_init(struct strbuf *, size_t);\n+void strbuf_init(struct strbuf *sb, size_t alloc);\n \n /**\n * Release a string buffer and the memory it used. After this call, the\n@@ -97,7 +97,7 @@ extern void strbuf_init(struct strbuf *, size_t);\n * To clear a strbuf in preparation for further use without the overhead\n * of free()ing and malloc()ing again, use strbuf_reset() instead.\n */\n-extern void strbuf_release(struct strbuf *);\n+void strbuf_release(struct strbuf *sb);\n \n /**\n * Detach the string from the strbuf and returns it; you now own the\n@@ -107,7 +107,7 @@ extern void strbuf_release(struct strbuf *);\n * The strbuf that previously held the string is reset to `STRBUF_INIT` so\n * it can be reused after calling this function.\n */\n-extern char *strbuf_detach(struct strbuf *, size_t *);\n+char *strbuf_detach(struct strbuf *sb, size_t *sz);\n \n /**\n * Attach a string to a buffer. You should specify the string to attach,\n@@ -117,7 +117,7 @@ extern char *strbuf_detach(struct strbuf *, size_t *);\n * malloc()ed, and after attaching, the pointer cannot be relied upon\n * anymore, and neither be free()d directly.\n */\n-extern void strbuf_attach(struct strbuf *, void *, size_t, size_t);\n+void strbuf_attach(struct strbuf *sb, void *str, size_t len, size_t mem);\n \n /**\n * Swap the contents of two string buffers.\n@@ -148,7 +148,7 @@ static inline size_t strbuf_avail(const struct strbuf *sb)\n * This is never a needed operation, but can be critical for performance in\n * some cases.\n */\n-extern void strbuf_grow(struct strbuf *, size_t);\n+void strbuf_grow(struct strbuf *sb, size_t amount);\n \n /**\n * Set the length of the buffer to a given value. This function does *not*\n@@ -183,30 +183,30 @@ static inline void strbuf_setlen(struct strbuf *sb, size_t len)\n * Strip whitespace from the beginning (`ltrim`), end (`rtrim`), or both side\n * (`trim`) of a string.\n */\n-extern void strbuf_trim(struct strbuf *);\n-extern void strbuf_rtrim(struct strbuf *);\n-extern void strbuf_ltrim(struct strbuf *);\n+void strbuf_trim(struct strbuf *sb);\n+void strbuf_rtrim(struct strbuf *sb);\n+void strbuf_ltrim(struct strbuf *sb);\n \n /* Strip trailing directory separators */\n-extern void strbuf_trim_trailing_dir_sep(struct strbuf *);\n+void strbuf_trim_trailing_dir_sep(struct strbuf *sb);\n \n /**\n * Replace the contents of the strbuf with a reencoded form. Returns -1\n * on error, 0 on success.\n */\n-extern int strbuf_reencode(struct strbuf *sb, const char *from, const char *to);\n+int strbuf_reencode(struct strbuf *sb, const char *from, const char *to);\n \n /**\n * Lowercase each character in the buffer using `tolower`.\n */\n-extern void strbuf_tolower(struct strbuf *sb);\n+void strbuf_tolower(struct strbuf *sb);\n \n /**\n * Compare two buffers. Returns an integer less than, equal to, or greater\n * than zero if the first buffer is found, respectively, to be less than,\n * to match, or be greater than the second buffer.\n */\n-extern int strbuf_cmp(const struct strbuf *, const struct strbuf *);\n+int strbuf_cmp(const struct strbuf *first, const struct strbuf *second);\n \n \n /**\n@@ -233,37 +233,38 @@ static inline void strbuf_addch(struct strbuf *sb, int c)\n /**\n * Add a character the specified number of times to the buffer.\n */\n-extern void strbuf_addchars(struct strbuf *sb, int c, size_t n);\n+void strbuf_addchars(struct strbuf *sb, int c, size_t n);\n \n /**\n * Insert data to the given position of the buffer. The remaining contents\n * will be shifted, not overwritten.\n */\n-extern void strbuf_insert(struct strbuf *, size_t pos, const void *, size_t);\n+void strbuf_insert(struct strbuf *sb, size_t pos, const void *, size_t);\n \n /**\n * Remove given amount of data from a given position of the buffer.\n */\n-extern void strbuf_remove(struct strbuf *, size_t pos, size_t len);\n+void strbuf_remove(struct strbuf *sb, size_t pos, size_t len);\n \n /**\n * Remove the bytes between `pos..pos+len` and replace it with the given\n * data.\n */\n-extern void strbuf_splice(struct strbuf *, size_t pos, size_t len,\n-\t\t\t const void *, size_t);\n+void strbuf_splice(struct strbuf *sb, size_t pos, size_t len,\n+\t\t const void *data, size_t data_len);\n \n /**\n * Add a NUL-terminated string to the buffer. Each line will be prepended\n * by a comment character and a blank.\n */\n-extern void strbuf_add_commented_lines(struct strbuf *out, const char *buf, size_t size);\n+void strbuf_add_commented_lines(struct strbuf *out,\n+\t\t\t\tconst char *buf, size_t size);\n \n \n /**\n * Add data of given length to the buffer.\n */\n-extern void strbuf_add(struct strbuf *, const void *, size_t);\n+void strbuf_add(struct strbuf *sb, const void *data, size_t len);\n \n /**\n * Add a NUL-terminated string to the buffer.\n@@ -282,7 +283,7 @@ static inline void strbuf_addstr(struct strbuf *sb, const char *s)\n /**\n * Copy the contents of another buffer at the end of the current one.\n */\n-extern void strbuf_addbuf(struct strbuf *sb, const struct strbuf *sb2);\n+void strbuf_addbuf(struct strbuf *sb, const struct strbuf *sb2);\n \n /**\n * This function can be used to expand a format string containing\n@@ -308,8 +309,13 @@ extern void strbuf_addbuf(struct strbuf *sb, const struct strbuf *sb2);\n * parameters to the callback, `strbuf_expand()` passes a context pointer,\n * which can be used by the programmer of the callback as she sees fit.\n */\n-typedef size_t (*expand_fn_t) (struct strbuf *sb, const char *placeholder, void *context);\n-extern void strbuf_expand(struct strbuf *sb, const char *format, expand_fn_t fn, void *context);\n+typedef size_t (*expand_fn_t) (struct strbuf *sb,\n+\t\t\t const char *placeholder,\n+\t\t\t void *context);\n+void strbuf_expand(struct strbuf *sb,\n+\t\t const char *format,\n+\t\t expand_fn_t fn,\n+\t\t void *context);\n \n /**\n * Used as callback for `strbuf_expand()`, expects an array of\n@@ -321,7 +327,9 @@ struct strbuf_expand_dict_entry {\n \tconst char *placeholder;\n \tconst char *value;\n };\n-extern size_t strbuf_expand_dict_cb(struct strbuf *sb, const char *placeholder, void *context);\n+size_t strbuf_expand_dict_cb(struct strbuf *sb,\n+\t\t\t const char *placeholder,\n+\t\t\t void *context);\n \n /**\n * Append the contents of one strbuf to another, quoting any\n@@ -329,29 +337,29 @@ extern size_t strbuf_expand_dict_cb(struct strbuf *sb, const char *placeholder,\n * destination. This is useful for literal data to be fed to either\n * strbuf_expand or to the *printf family of functions.\n */\n-extern void strbuf_addbuf_percentquote(struct strbuf *dst, const struct strbuf *src);\n+void strbuf_addbuf_percentquote(struct strbuf *dst, const struct strbuf *src);\n \n /**\n * Append the given byte size as a human-readable string (i.e. 12.23 KiB,\n * 3.50 MiB).\n */\n-extern void strbuf_humanise_bytes(struct strbuf *buf, off_t bytes);\n+void strbuf_humanise_bytes(struct strbuf *buf, off_t bytes);\n \n /**\n * Add a formatted string to the buffer.\n */\n __attribute__((format (printf,2,3)))\n-extern void strbuf_addf(struct strbuf *sb, const char *fmt, ...);\n+void strbuf_addf(struct strbuf *sb, const char *fmt, ...);\n \n /**\n * Add a formatted string prepended by a comment character and a\n * blank to the buffer.\n */\n __attribute__((format (printf, 2, 3)))\n-extern void strbuf_commented_addf(struct strbuf *sb, const char *fmt, ...);\n+void strbuf_commented_addf(struct strbuf *sb, const char *fmt, ...);\n \n __attribute__((format (printf,2,0)))\n-extern void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap);\n+void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap);\n \n /**\n * Add the time specified by `tm`, as formatted by `strftime`.\n@@ -361,9 +369,9 @@ extern void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap);\n * `suppress_tz_name`, when set, expands %Z internally to the empty\n * string rather than passing it to `strftime`.\n */\n-extern void strbuf_addftime(struct strbuf *sb, const char *fmt,\n-\t\t\t const struct tm *tm, int tz_offset,\n-\t\t\t int suppress_tz_name);\n+void strbuf_addftime(struct strbuf *sb, const char *fmt,\n+\t\t const struct tm *tm, int tz_offset,\n+\t\t int suppress_tz_name);\n \n /**\n * Read a given size of data from a FILE* pointer to the buffer.\n@@ -373,14 +381,14 @@ extern void strbuf_addftime(struct strbuf *sb, const char *fmt,\n * `strbuf_read()`, `strbuf_read_file()` and `strbuf_getline_*()`\n * family of functions have the same behaviour as well.\n */\n-extern size_t strbuf_fread(struct strbuf *, size_t, FILE *);\n+size_t strbuf_fread(struct strbuf *sb, size_t size, FILE *file);\n \n /**\n * Read the contents of a given file descriptor. The third argument can be\n * used to give a hint about the file size, to avoid reallocs. If read fails,\n * any partial read is undone.\n */\n-extern ssize_t strbuf_read(struct strbuf *, int fd, size_t hint);\n+ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint);\n \n /**\n * Read the contents of a given file descriptor partially by using only one\n@@ -388,7 +396,7 @@ extern ssize_t strbuf_read(struct strbuf *, int fd, size_t hint);\n * file size, to avoid reallocs. Returns the number of new bytes appended to\n * the sb.\n */\n-extern ssize_t strbuf_read_once(struct strbuf *, int fd, size_t hint);\n+ssize_t strbuf_read_once(struct strbuf *sb, int fd, size_t hint);\n \n /**\n * Read the contents of a file, specified by its path. The third argument\n@@ -396,19 +404,19 @@ extern ssize_t strbuf_read_once(struct strbuf *, int fd, size_t hint);\n * Return the number of bytes read or a negative value if some error\n * occurred while opening or reading the file.\n */\n-extern ssize_t strbuf_read_file(struct strbuf *sb, const char *path, size_t hint);\n+ssize_t strbuf_read_file(struct strbuf *sb, const char *path, size_t hint);\n \n /**\n * Read the target of a symbolic link, specified by its path. The third\n * argument can be used to give a hint about the size, to avoid reallocs.\n */\n-extern int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint);\n+int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint);\n \n /**\n * Write the whole content of the strbuf to the stream not stopping at\n * NUL bytes.\n */\n-extern ssize_t strbuf_write(struct strbuf *sb, FILE *stream);\n+ssize_t strbuf_write(struct strbuf *sb, FILE *stream);\n \n /**\n * Read a line from a FILE *, overwriting the existing contents of\n@@ -422,10 +430,10 @@ extern ssize_t strbuf_write(struct strbuf *sb, FILE *stream);\n typedef int (*strbuf_getline_fn)(struct strbuf *, FILE *);\n \n /* Uses LF as the line terminator */\n-extern int strbuf_getline_lf(struct strbuf *sb, FILE *fp);\n+int strbuf_getline_lf(struct strbuf *sb, FILE *fp);\n \n /* Uses NUL as the line terminator */\n-extern int strbuf_getline_nul(struct strbuf *sb, FILE *fp);\n+int strbuf_getline_nul(struct strbuf *sb, FILE *fp);\n \n /*\n * Similar to strbuf_getline_lf(), but additionally treats a CR that\n@@ -434,14 +442,14 @@ extern int strbuf_getline_nul(struct strbuf *sb, FILE *fp);\n * that can come from platforms whose native text format is CRLF\n * terminated.\n */\n-extern int strbuf_getline(struct strbuf *, FILE *);\n+int strbuf_getline(struct strbuf *sb, FILE *file);\n \n \n /**\n * Like `strbuf_getline`, but keeps the trailing terminator (if\n * any) in the buffer.\n */\n-extern int strbuf_getwholeline(struct strbuf *, FILE *, int);\n+int strbuf_getwholeline(struct strbuf *sb, FILE *file, int term);\n \n /**\n * Like `strbuf_getwholeline`, but operates on a file descriptor.\n@@ -449,19 +457,19 @@ extern int strbuf_getwholeline(struct strbuf *, FILE *, int);\n * use it unless you need the correct position in the file\n * descriptor.\n */\n-extern int strbuf_getwholeline_fd(struct strbuf *, int, int);\n+int strbuf_getwholeline_fd(struct strbuf *sb, int fd, int term);\n \n /**\n * Set the buffer to the path of the current working directory.\n */\n-extern int strbuf_getcwd(struct strbuf *sb);\n+int strbuf_getcwd(struct strbuf *sb);\n \n /**\n * Add a path to a buffer, converting a relative path to an\n * absolute one in the process. Symbolic links are not\n * resolved.\n */\n-extern void strbuf_add_absolute_path(struct strbuf *sb, const char *path);\n+void strbuf_add_absolute_path(struct strbuf *sb, const char *path);\n \n /**\n * Canonize `path` (make it absolute, resolve symlinks, remove extra\n@@ -475,7 +483,7 @@ extern void strbuf_add_absolute_path(struct strbuf *sb, const char *path);\n * Callers that don't mind links should use the more lightweight\n * strbuf_add_absolute_path() instead.\n */\n-extern void strbuf_add_real_path(struct strbuf *sb, const char *path);\n+void strbuf_add_real_path(struct strbuf *sb, const char *path);\n \n \n /**\n@@ -483,13 +491,13 @@ extern void strbuf_add_real_path(struct strbuf *sb, const char *path);\n * normalize_path_copy() for details. If an error occurs, the contents of \"sb\"\n * are left untouched, and -1 is returned.\n */\n-extern int strbuf_normalize_path(struct strbuf *sb);\n+int strbuf_normalize_path(struct strbuf *sb);\n \n /**\n * Strip whitespace from a buffer. The second parameter controls if\n * comments are considered contents to be removed or not.\n */\n-extern void strbuf_stripspace(struct strbuf *buf, int skip_comments);\n+void strbuf_stripspace(struct strbuf *buf, int skip_comments);\n \n static inline int strbuf_strip_suffix(struct strbuf *sb, const char *suffix)\n {\n@@ -518,8 +526,8 @@ static inline int strbuf_strip_suffix(struct strbuf *sb, const char *suffix)\n * For lighter-weight alternatives, see string_list_split() and\n * string_list_split_in_place().\n */\n-extern struct strbuf **strbuf_split_buf(const char *, size_t,\n-\t\t\t\t\tint terminator, int max);\n+struct strbuf **strbuf_split_buf(const char *str, size_t len,\n+\t\t\t\t int terminator, int max);\n \n static inline struct strbuf **strbuf_split_str(const char *str,\n \t\t\t\t\t int terminator, int max)\n@@ -528,7 +536,7 @@ static inline struct strbuf **strbuf_split_str(const char *str,\n }\n \n static inline struct strbuf **strbuf_split_max(const struct strbuf *sb,\n-\t\t\t\t\t\tint terminator, int max)\n+\t\t\t\t\t int terminator, int max)\n {\n \treturn strbuf_split_buf(sb->buf, sb->len, terminator, max);\n }\n@@ -549,23 +557,23 @@ static inline struct strbuf **strbuf_split(const struct strbuf *sb,\n * 'element1, element2, ..., elementN'\n * to str. If only one element, just write \"element1\" to str.\n */\n-extern void strbuf_add_separated_string_list(struct strbuf *str,\n-\t\t\t\t\t const char *sep,\n-\t\t\t\t\t struct string_list *slist);\n+void strbuf_add_separated_string_list(struct strbuf *str,\n+\t\t\t\t const char *sep,\n+\t\t\t\t struct string_list *slist);\n \n /**\n * Free a NULL-terminated list of strbufs (for example, the return\n * values of the strbuf_split*() functions).\n */\n-extern void strbuf_list_free(struct strbuf **);\n+void strbuf_list_free(struct strbuf **list);\n \n /**\n * Add the abbreviation, as generated by find_unique_abbrev, of `sha1` to\n * the strbuf `sb`.\n */\n-extern void strbuf_add_unique_abbrev(struct strbuf *sb,\n-\t\t\t\t const struct object_id *oid,\n-\t\t\t\t int abbrev_len);\n+void strbuf_add_unique_abbrev(struct strbuf *sb,\n+\t\t\t const struct object_id *oid,\n+\t\t\t int abbrev_len);\n \n /**\n * Launch the user preferred editor to edit a file and fill the buffer\n@@ -574,15 +582,21 @@ extern void strbuf_add_unique_abbrev(struct strbuf *sb,\n * run in. If the buffer is NULL the editor is launched as usual but the\n * file's contents are not read into the buffer upon completion.\n */\n-extern int launch_editor(const char *path, struct strbuf *buffer, const char *const *env);\n+int launch_editor(const char *path,\n+\t\t struct strbuf *buffer,\n+\t\t const char *const *env);\n \n-extern void strbuf_add_lines(struct strbuf *sb, const char *prefix, const char *buf, size_t size);\n+void strbuf_add_lines(struct strbuf *sb,\n+\t\t const char *prefix,\n+\t\t const char *buf,\n+\t\t size_t size);\n \n /**\n * Append s to sb, with the characters '<', '>', '&' and '\"' converted\n * into XML entities.\n */\n-extern void strbuf_addstr_xml_quoted(struct strbuf *sb, const char *s);\n+void strbuf_addstr_xml_quoted(struct strbuf *sb,\n+\t\t\t const char *s);\n \n /**\n * \"Complete\" the contents of `sb` by ensuring that either it ends with the\n@@ -612,8 +626,8 @@ static inline void strbuf_complete_line(struct strbuf *sb)\n * If \"allowed\" is non-zero, restrict the set of allowed expansions. See\n * interpret_branch_name() for details.\n */\n-extern void strbuf_branchname(struct strbuf *sb, const char *name,\n-\t\t\t unsigned allowed);\n+void strbuf_branchname(struct strbuf *sb, const char *name,\n+\t\t unsigned allowed);\n \n /*\n * Like strbuf_branchname() above, but confirm that the result is\n@@ -621,15 +635,15 @@ extern void strbuf_branchname(struct strbuf *sb, const char *name,\n *\n * The return value is \"0\" if the result is valid, and \"-1\" otherwise.\n */\n-extern int strbuf_check_branch_ref(struct strbuf *sb, const char *name);\n+int strbuf_check_branch_ref(struct strbuf *sb, const char *name);\n \n-extern void strbuf_addstr_urlencode(struct strbuf *, const char *,\n-\t\t\t\t int reserved);\n+void strbuf_addstr_urlencode(struct strbuf *sb, const char *name,\n+\t\t\t int reserved);\n \n __attribute__((format (printf,1,2)))\n-extern int printf_ln(const char *fmt, ...);\n+int printf_ln(const char *fmt, ...);\n __attribute__((format (printf,2,3)))\n-extern int fprintf_ln(FILE *fp, const char *fmt, ...);\n+int fprintf_ln(FILE *fp, const char *fmt, ...);\n \n char *xstrdup_tolower(const char *);\n char *xstrdup_toupper(const char *);\n-- \n2.19.0\n\n"},{"id":"359242","messageId":"CAN0heSq4-CXo5dzWdeBiR8nFdRzc-+YXE6h4w7WuB8qEiSBf6g@mail.gmail.com","threadId":"49400","inReplyTo":"xmqq5zypefcp.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] Documentation/CodingGuidelines: How to document new APIs","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-09-28T19:47:23Z","receivedAt":"2018-09-28T19:47:38Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Fri, 28 Sep 2018 at 18:51, Junio C Hamano wrote:\n> We recommend documenting in the header over documenting near the\n> implementation to encourage people to write the docs that are\n> readable without peeking at the implemention.\n\ns/implemention/implementation/\n\n> - - When you come up with an API, document it.\n> + - When you come up with an API, document it the functions and the\n\ns/it the/the/\n\n> + structures in the header file that exposes the API to its callers.\n> + Use what is in \"strbuf.h\" as a model to decide the appropriate tone\n> + in which the description is given, and the level of details to put\n> + in the description.\n\nMartin\n"},{"id":"359243","messageId":"xmqqr2hdbdp8.fsf@gitster-ct.c.googlers.com","threadId":"49400","inReplyTo":"20180928173033.159192-1-sbeller@google.com","subject":"Re: [PATCH] strbuf.h: format according to coding guidelines","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-28T19:54:11Z","receivedAt":"2018-09-28T19:54:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> So let's format strbuf.h in a way that we'd like to see:\n> * omit the extern keyword from function declarations\n\nOK\n\n> * name all parameters (usually the parameters are obvious from its type,\n> but consider exceptions like\n> `int strbuf_getwholeline_fd(struct strbuf *, int, int);`\n\nI am mildly against giving names to obvious ones.\n"},{"id":"359245","messageId":"xmqqin2pbcwh.fsf@gitster-ct.c.googlers.com","threadId":"49400","inReplyTo":"xmqqr2hdbdp8.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] strbuf.h: format according to coding guidelines","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-28T20:11:26Z","receivedAt":"2018-09-28T20:11:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\nJunio C Hamano writes:\n\n> Stefan Beller writes:\n>\n>> So let's format strbuf.h in a way that we'd like to see:\n>> * omit the extern keyword from function declarations\n>\n> OK\n>\n>> * name all parameters (usually the parameters are obvious from its type,\n>> but consider exceptions like\n>> `int strbuf_getwholeline_fd(struct strbuf *, int, int);`\n>\n> I am mildly against giving names to obvious ones.\n\nJust to make sure nobody listening from sidelines do not\nmisunderstand, ones like getwholeline_fd() that takes more than one\nparameter with the same time is a prime example of a function that\ntake non-obvious parameters that MUST be named. \n\nWhat I am mildly against is the rule that says \"name ALL\nparameters\". I tend to think these names make headers harder to\nread, and prefer to keep them to the minimum.\n\nI actually do not mind the rule to be more like\n\n * Use the same parameter names used in the function declaration when\n the description in the API documentation refers the parameter.\n\nThat _forces_ people to write\n\n\t/**\n\t * Read a whole line up to the terminating character \n\t * TERM (typically LF or NUL) from the file descriptor FD\n\t * and replace the contents of the strbuf SB with the\n\t * result ...\n\t */\n\tint strbuf_getwholeline_fd(struct strbuf *sb, int fd, int term);\n\nwhich is mostly equivalent to having a rule to always name all\nparameters, while still allowing \"sb\" to be omitted by rephrasing\n\"the contents of the given strbuf with the result ...\" and I\nconsider that a good flexibility to have.\n\n"},{"id":"359251","messageId":"xmqq8t3lb8uu.fsf@gitster-ct.c.googlers.com","threadId":"49400","inReplyTo":"xmqqin2pbcwh.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] strbuf.h: format according to coding guidelines","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-28T21:38:49Z","receivedAt":"2018-09-28T21:38:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> I actually do not mind the rule to be more like\n>\n> * Use the same parameter names used in the function declaration when\n> the description in the API documentation refers the parameter.\n\nAssuming that we adopt the above guideline, let's extending it to\nthe original patch's review.\n\nThe following is a good example. FIRST and SECOND would have been\nupcased if this followed my earlier illustration to make them stand\nout as references to the parameters, but it is already readable\nwithout upcasing _and_ naming parameters is helping here.\n\n /**\n * Compare two buffers. Returns an integer less than, equal to, or greater\n * than zero if the first buffer is found, respectively, to be less than,\n * to match, or be greater than the second buffer.\n */\n-extern int strbuf_cmp(const struct strbuf *, const struct strbuf *);\n+int strbuf_cmp(const struct strbuf *first, const struct strbuf *second);\n\n\n\nThe next one could be improved and say something like: \"Remove LEN\nbytes in the strbuf SB starting at offset POS\", as it already had\n'pos' and 'len' that are readily usable. Notice that \"Remove LEN\nbytes starting at offset POS\" is a sufficiently clear description\nand that is why I do not think we should require all parameters to\nbe named.\n\n /**\n * Remove given amount of data from a given position of the buffer.\n */\n-extern void strbuf_remove(struct strbuf *, size_t pos, size_t len);\n+void strbuf_remove(struct strbuf *sb, size_t pos, size_t len);\n \n\nThe last example is a job half-done. The original had pos and len\nparameters and referred to them in the text, but just said \"with the\ngiven data\". Now we have data and data_len, \"the given data\" can\nand should be clarified by referring to them.\n\n /**\n * Remove the bytes between `pos..pos+len` and replace it with the given\n * data.\n */\n-extern void strbuf_splice(struct strbuf *, size_t pos, size_t len,\n-\t\t\t const void *, size_t);\n+void strbuf_splice(struct strbuf *sb, size_t pos, size_t len,\n+\t\t const void *data, size_t data_len);\n\n"},{"id":"359252","messageId":"xmqq4le9b8p6.fsf@gitster-ct.c.googlers.com","threadId":"49400","inReplyTo":"20180928173033.159192-1-sbeller@google.com","subject":"Re: [PATCH] strbuf.h: format according to coding guidelines","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-28T21:42:13Z","receivedAt":"2018-09-28T21:42:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> The previous patch suggested the strbuf header to be the leading example\n> of how we would want our APIs to be documented. This may lead to some\n> scrutiny of that code and the coding style (which is different from the\n> API documentation style) and hence might be taken as an example on how\n> to format code as well.\n>\n> So let's format strbuf.h in a way that we'd like to see:\n> * omit the extern keyword from function declarations\n> * name all parameters (usually the parameters are obvious from its type,\n> but consider exceptions like\n> `int strbuf_getwholeline_fd(struct strbuf *, int, int);`\n> * break overly long lines\n>\n> Signed-off-by: Stefan Beller \n> ---\n>\n> Maybe this on top of Junios guideline patch?\n\nIf we were to do this, I'd rather see us do a very good job on this\nfile first, with \"We are going to use this file as the best current\npractice model for an API header file\" to begin its log message,\nand then recommend its use as the model on top.\n"},{"id":"359253","messageId":"CAGZ79kbV6QjsFKcD2uG_P9j1AvzSNQSi-_jXGQ9w0YU9fjhEGg@mail.gmail.com","threadId":"49400","inReplyTo":"xmqq4le9b8p6.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] strbuf.h: format according to coding guidelines","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-28T21:54:24Z","receivedAt":"2018-09-28T21:54:39Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Sep 28, 2018 at 2:42 PM Junio C Hamano wrote:\n>\n> Stefan Beller writes:\n>\n> > The previous patch suggested the strbuf header to be the leading example\n> > of how we would want our APIs to be documented. This may lead to some\n> > scrutiny of that code and the coding style (which is different from the\n> > API documentation style) and hence might be taken as an example on how\n> > to format code as well.\n> >\n> > So let's format strbuf.h in a way that we'd like to see:\n> > * omit the extern keyword from function declarations\n> > * name all parameters (usually the parameters are obvious from its type,\n> > but consider exceptions like\n> > `int strbuf_getwholeline_fd(struct strbuf *, int, int);`\n> > * break overly long lines\n> >\n> > Signed-off-by: Stefan Beller \n> > ---\n> >\n> > Maybe this on top of Junios guideline patch?\n>\n> If we were to do this, I'd rather see us do a very good job on this\n> file first, with \"We are going to use this file as the best current\n> practice model for an API header file\" to begin its log message,\n> and then recommend its use as the model on top.\n\nI started going through that file and undoing \"the naming all parameters\"\nand additionally started to remove any obvious parameter name\n(mostly the first argument, that is of struct strbuf, and sometimes\nwas called sb or out)\n\nBut it seems in addition to all this we also want to re-read the\ndocumentation and make sure it reflects the API accurately and\ndescribes it well.\n"},{"id":"359275","messageId":"20180929073827.GD2174@sigill.intra.peff.net","threadId":"49400","inReplyTo":"xmqqin2pbcwh.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] strbuf.h: format according to coding guidelines","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-09-29T07:38:27Z","receivedAt":"2018-09-29T07:38:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 28, 2018 at 01:11:26PM -0700, Junio C Hamano wrote:\n\n> > I am mildly against giving names to obvious ones.\n> \n> Just to make sure nobody listening from sidelines do not\n> misunderstand, ones like getwholeline_fd() that takes more than one\n> parameter with the same time is a prime example of a function that\n> take non-obvious parameters that MUST be named. \n> \n> What I am mildly against is the rule that says \"name ALL\n> parameters\". I tend to think these names make headers harder to\n> read, and prefer to keep them to the minimum.\n> \n> I actually do not mind the rule to be more like\n> \n> * Use the same parameter names used in the function declaration when\n> the description in the API documentation refers the parameter.\n\nYes, I agree very much with that rule (and your genera line of\nthinking).\n\nI am not personally against just naming every parameter, but I simply\ndon't care either way.\n\n-Peff\n"},{"id":"359276","messageId":"20180929074118.GE2174@sigill.intra.peff.net","threadId":"49400","inReplyTo":"CAGZ79kZzR+n+8ODs3SBhOEXFbOmccugOampzkpFKGoEXjggEwQ@mail.gmail.com","subject":"Re: [PATCH] Documentation/CodingGuidelines: How to document new APIs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-09-29T07:41:18Z","receivedAt":"2018-09-29T07:41:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 28, 2018 at 10:14:12AM -0700, Stefan Beller wrote:\n\n> > I think other high-level concepts that are _not_ APIs (e.g., file\n> > formats, protocol, etc) could go into technical/.\n> \n> That is what I meant with high level concepts. Anything that talks\n> about or implies the existence of a function is clearly header level.\n\nAh, OK. I thought you meant the \"this is how strbuf roughly works\"\noverview, which IMHO should remain in strbuf.h.\n\nI think we are on the same page, then.\n\n> > (Though actually, those are the thing that I would not mind at all if\n> > they get formatted into real manpages and shipped to end users. We do\n> > not expect most users to dissect our file-formats, but they could at\n> > least be useful to somebody poking around).\n> \n> Formats are sensible thing to present to the end user. I was also thinking\n> about partial-clone, which is a concept rather than a format.\n\nYeah, definitely. Another good example is technical/api-credentials.\nThe section on credential helpers there probably ought to be in more\nuser-visible documentation (probably gitcredentials(7)).\n\n-Peff\n"},{"id":"359277","messageId":"20180929074603.GF2174@sigill.intra.peff.net","threadId":"49400","inReplyTo":"xmqq5zypefcp.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] Documentation/CodingGuidelines: How to document new APIs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-09-29T07:46:04Z","receivedAt":"2018-09-29T07:46:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 28, 2018 at 09:50:14AM -0700, Junio C Hamano wrote:\n\n> -- >8 --\n> Subject: CodingGuidelines: document the API in *.h files\n> \n> It makes it harder to let the API description and the reality drift\n> apart if the doc is kept close to the implementation or the header\n> of the API. We have been slowly migrating API docs out of the\n> Documentation/technical/api-* to *.h files, and the development\n> community generally considers that how inline docs in strbuf.h is\n> done the best current practice.\n> \n> We recommend documenting in the header over documenting near the\n> implementation to encourage people to write the docs that are\n> readable without peeking at the implemention.\n\nYeah, I agree with all of that rationale.\n\n> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\n> index 6d265327c9..e87090c849 100644\n> --- a/Documentation/CodingGuidelines\n> +++ b/Documentation/CodingGuidelines\n> @@ -385,7 +385,11 @@ For C programs:\n> string_list for sorted string lists, a hash map (mapping struct\n> objects) named \"struct decorate\", amongst other things.\n> \n> - - When you come up with an API, document it.\n> + - When you come up with an API, document it the functions and the\n> + structures in the header file that exposes the API to its callers.\n> + Use what is in \"strbuf.h\" as a model to decide the appropriate tone\n> + in which the description is given, and the level of details to put\n> + in the description.\n\nI like the general idea here. I had trouble parsing the \"in which the\ndescription is given\". Maybe just:\n\n When you come up with an API, document its functions and structures in\n the header file that exposes the API to its callers. Use what is in\n \"strbuf.h\" as a model for the appropriate tone and level of detail.\n\nI like the idea you mentioned elsewhere of polishing up strbuf.h to\nserve as the model (but I don't want to hold up this much simpler patch\nif that seems likely to drag on).\n\nThanks for pushing this towards a concrete conclusion.\n\n-Peff\n"}]}