{"thread":{"id":"53354","subject":"[PATCH 0/5] shallow: extract a header file","startedAt":"2020-04-29T22:39:32Z","lastAt":"2020-04-30T19:32:38Z","messageCount":22,"participants":["Taylor Blau","Eric Sunshine","Jonathan Tan","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"396619","messageId":"cover.1588199705.git.me@ttaylorr.com","threadId":"53354","inReplyTo":null,"subject":"[PATCH 0/5] shallow: extract a header file","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-29T22:39:25Z","receivedAt":"2020-04-29T22:39:32Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi,\n\nThese few patches are based on a suggestion in [1] from Jonathan Nieder\nand in [2] Junio. The gist of this series is to do the following three\nthings:\n\n 1. Extract a new 'shallow.h' header to collect all of the functions\n that are related to shallow-ness, but defined in 'commit.h'.\n\n 2. Add some documentation from the previous series where it could have\n benefited from it.\n\n 3. Finally, introduce a thin wrapper type 'struct shallow_lock' which\n makes it a type-level error to unlock a non-shallow lock with\n '{commit,rollback}_shallow_file'.\n\nThe remainder of suggestions in [1] about the structure of t5537 I\ndecided to not address in this series, since I anticipate there is more\nwork there than I want to take on in this series alone.\n\nThanks in advance for your review!\n\n[1]: https://lore.kernel.org/git/20200423011444.GG140314@google.com/\n[2]: https://lore.kernel.org/git/xmqqy2qnidyy.fsf@gitster.c.googlers.com/\n\nTaylor Blau (5):\n commit: make 'commit_graft_pos' non-static\n shallow: extract a header file for shallow-related functions\n commit: move 'unregister_shallow' to 'shallow.h'\n shallow.h: document '{commit,rollback}_shallow_file'\n shallow: use struct 'shallow_lock' for additional safety\n\n builtin.h | 1 +\n builtin/receive-pack.c | 2 +-\n commit.c | 16 ++-------\n commit.h | 49 +-------------------------\n fetch-pack.c | 3 +-\n send-pack.c | 1 +\n shallow.c | 36 +++++++++++++------\n shallow.h | 78 ++++++++++++++++++++++++++++++++++++++++++\n upload-pack.c | 1 +\n 9 files changed, 112 insertions(+), 75 deletions(-)\n create mode 100644 shallow.h\n\n--\n2.26.0.113.ge9739cdccc\n"},{"id":"396620","messageId":"8ac4c63735f2e48dc83e9f31c9fbbbfb47d7e79d.1588199705.git.me@ttaylorr.com","threadId":"53354","inReplyTo":"cover.1588199705.git.me@ttaylorr.com","subject":"[PATCH 1/5] commit: make 'commit_graft_pos' non-static","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-29T22:39:29Z","receivedAt":"2020-04-29T22:39:35Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In the next patch, some functions will be moved from 'commit.c' to have\nprototypes in a new 'shallow.h' and their implementations in\n'shallow.c'.\n\nThree functions in 'commit.c' use 'commit_graft_pos()' (they are\n'register_commit_graft()', 'lookup_commit_graft()', and\n'unregister_shallow()'). The first two of these will stay in 'commit.c',\nbut the latter will move to 'shallow.c', and thus needs\n'commit_graft_pos' to be non-static.\n\nPrepare for that by making 'commit_graft_pos' non-static so that it can\nbe called from both 'commit.c' and 'shallow.c'.\n\nSigned-off-by: Taylor Blau \n---\n commit.c | 2 +-\n commit.h | 1 +\n 2 files changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/commit.c b/commit.c\nindex c7099daeac..b76f7d72be 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -110,7 +110,7 @@ static const unsigned char *commit_graft_sha1_access(size_t index, void *table)\n \treturn commit_graft_table[index]->oid.hash;\n }\n \n-static int commit_graft_pos(struct repository *r, const unsigned char *sha1)\n+int commit_graft_pos(struct repository *r, const unsigned char *sha1)\n {\n \treturn sha1_pos(sha1, r->parsed_objects->grafts,\n \t\t\tr->parsed_objects->grafts_nr,\ndiff --git a/commit.h b/commit.h\nindex ab91d21131..eb42e8b6d2 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -236,6 +236,7 @@ struct commit_graft {\n typedef int (*each_commit_graft_fn)(const struct commit_graft *, void *);\n \n struct commit_graft *read_graft_line(struct strbuf *line);\n+int commit_graft_pos(struct repository *r, const unsigned char *sha1);\n int register_commit_graft(struct repository *r, struct commit_graft *, int);\n void prepare_commit_graft(struct repository *r);\n struct commit_graft *lookup_commit_graft(struct repository *r, const struct object_id *oid);\n-- \n2.26.0.113.ge9739cdccc\n\n"},{"id":"396621","messageId":"0b93abe072aa35ab96ba3b97118caa8ffe8e439d.1588199705.git.me@ttaylorr.com","threadId":"53354","inReplyTo":"cover.1588199705.git.me@ttaylorr.com","subject":"[PATCH 2/5] shallow: extract a header file for shallow-related functions","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-29T22:39:34Z","receivedAt":"2020-04-29T22:39:39Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"There are many functions in commit.h that are more related to shallow\nrepositories than they are to any sort of generic commit machinery.\nLikely this began when there were only a few shallow-related functions,\nand commit.h seemed a reasonable enough place to put them.\n\nBut, now there are a good number of shallow-related functions, and\nplacing them all in 'commit.h' doesn't make sense.\n\nThis patch extracts a 'shallow.h', which takes all of the headers from\n'commit.h' for functions which already exist in 'shallow.c'. We will\nbring the remaining shallow-related functions defined in 'commit.c' in a\nsubsequent patch.\n\nFor now, move only the ones that already are implemented in 'shallow.c',\nand update the necessary includes.\n\nSigned-off-by: Taylor Blau \n---\n builtin.h | 1 +\n commit.c | 1 +\n commit.h | 48 ---------------------------------------\n fetch-pack.c | 1 +\n send-pack.c | 1 +\n shallow.c | 1 +\n shallow.h | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++\n upload-pack.c | 1 +\n 8 files changed, 69 insertions(+), 48 deletions(-)\n create mode 100644 shallow.h\n\ndiff --git a/builtin.h b/builtin.h\nindex 2b25a80cde..2e701a023c 100644\n--- a/builtin.h\n+++ b/builtin.h\n@@ -5,6 +5,7 @@\n #include \"strbuf.h\"\n #include \"cache.h\"\n #include \"commit.h\"\n+#include \"shallow.h\"\n \n /*\n * builtin API\ndiff --git a/commit.c b/commit.c\nindex b76f7d72be..eebfbeb45d 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -20,6 +20,7 @@\n #include \"refs.h\"\n #include \"commit-reach.h\"\n #include \"run-command.h\"\n+#include \"shallow.h\"\n \n static struct commit_extra_header *read_commit_extra_header_lines(const char *buf, size_t len, const char **);\n \ndiff --git a/commit.h b/commit.h\nindex eb42e8b6d2..5cd984939b 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -248,55 +248,7 @@ struct commit *get_fork_point(const char *refname, struct commit *commit);\n \n struct oid_array;\n struct ref;\n-int register_shallow(struct repository *r, const struct object_id *oid);\n-int unregister_shallow(const struct object_id *oid);\n-int commit_shallow_file(struct repository *r, struct lock_file *lk);\n-void rollback_shallow_file(struct repository *r, struct lock_file *lk);\n int for_each_commit_graft(each_commit_graft_fn, void *);\n-int is_repository_shallow(struct repository *r);\n-struct commit_list *get_shallow_commits(struct object_array *heads,\n-\t\t\t\t\tint depth, int shallow_flag, int not_shallow_flag);\n-struct commit_list *get_shallow_commits_by_rev_list(\n-\t\tint ac, const char **av, int shallow_flag, int not_shallow_flag);\n-void set_alternate_shallow_file(struct repository *r, const char *path, int override);\n-int write_shallow_commits(struct strbuf *out, int use_pack_protocol,\n-\t\t\t const struct oid_array *extra);\n-void setup_alternate_shallow(struct lock_file *shallow_lock,\n-\t\t\t const char **alternate_shallow_file,\n-\t\t\t const struct oid_array *extra);\n-const char *setup_temporary_shallow(const struct oid_array *extra);\n-void advertise_shallow_grafts(int);\n-\n-/*\n- * Initialize with prepare_shallow_info() or zero-initialize (equivalent to\n- * prepare_shallow_info with a NULL oid_array).\n- */\n-struct shallow_info {\n-\tstruct oid_array *shallow;\n-\tint *ours, nr_ours;\n-\tint *theirs, nr_theirs;\n-\tstruct oid_array *ref;\n-\n-\t/* for receive-pack */\n-\tuint32_t **used_shallow;\n-\tint *need_reachability_test;\n-\tint *reachable;\n-\tint *shallow_ref;\n-\tstruct commit **commits;\n-\tint nr_commits;\n-};\n-\n-void prepare_shallow_info(struct shallow_info *, struct oid_array *);\n-void clear_shallow_info(struct shallow_info *);\n-void remove_nonexistent_theirs_shallow(struct shallow_info *);\n-void assign_shallow_commits_to_refs(struct shallow_info *info,\n-\t\t\t\t uint32_t **used,\n-\t\t\t\t int *ref_status);\n-int delayed_reachability_test(struct shallow_info *si, int c);\n-#define PRUNE_SHOW_ONLY 1\n-#define PRUNE_QUICK 2\n-void prune_shallow(unsigned options);\n-extern struct trace_key trace_shallow;\n \n int interactive_add(int argc, const char **argv, const char *prefix, int patch);\n int run_add_interactive(const char *revision, const char *patch_mode,\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex a618f3b029..401d028e41 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -22,6 +22,7 @@\n #include \"connected.h\"\n #include \"fetch-negotiator.h\"\n #include \"fsck.h\"\n+#include \"shallow.h\"\n \n static int transfer_unpack_limit = -1;\n static int fetch_unpack_limit = -1;\ndiff --git a/send-pack.c b/send-pack.c\nindex 0407841ae8..e0ccfef75a 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -15,6 +15,7 @@\n #include \"sha1-array.h\"\n #include \"gpg-interface.h\"\n #include \"cache.h\"\n+#include \"shallow.h\"\n \n int option_parse_push_signed(const struct option *opt,\n \t\t\t const char *arg, int unset)\ndiff --git a/shallow.c b/shallow.c\nindex 5010a6c732..d0191c7609 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -14,6 +14,7 @@\n #include \"commit-slab.h\"\n #include \"list-objects.h\"\n #include \"commit-reach.h\"\n+#include \"shallow.h\"\n \n void set_alternate_shallow_file(struct repository *r, const char *path, int override)\n {\ndiff --git a/shallow.h b/shallow.h\nnew file mode 100644\nindex 0000000000..14dd748625\n--- /dev/null\n+++ b/shallow.h\n@@ -0,0 +1,63 @@\n+#ifndef SHALLOW_H\n+#define SHALLOW_H\n+\n+#include \"lockfile.h\"\n+#include \"object.h\"\n+#include \"repository.h\"\n+#include \"strbuf.h\"\n+\n+void set_alternate_shallow_file(struct repository *r, const char *path, int override);\n+int register_shallow(struct repository *r, const struct object_id *oid);\n+int is_repository_shallow(struct repository *r);\n+int commit_shallow_file(struct repository *r, struct lock_file *lk);\n+void rollback_shallow_file(struct repository *r, struct lock_file *lk);\n+\n+struct commit_list *get_shallow_commits(struct object_array *heads,\n+\t\t\t\t\tint depth, int shallow_flag, int not_shallow_flag);\n+struct commit_list *get_shallow_commits_by_rev_list(\n+\t\tint ac, const char **av, int shallow_flag, int not_shallow_flag);\n+int write_shallow_commits(struct strbuf *out, int use_pack_protocol,\n+\t\t\t const struct oid_array *extra);\n+\n+void setup_alternate_shallow(struct lock_file *shallow_lock,\n+\t\t\t const char **alternate_shallow_file,\n+\t\t\t const struct oid_array *extra);\n+\n+const char *setup_temporary_shallow(const struct oid_array *extra);\n+\n+void advertise_shallow_grafts(int);\n+\n+#define PRUNE_SHOW_ONLY 1\n+#define PRUNE_QUICK 2\n+void prune_shallow(unsigned options);\n+\n+/*\n+ * Initialize with prepare_shallow_info() or zero-initialize (equivalent to\n+ * prepare_shallow_info with a NULL oid_array).\n+ */\n+struct shallow_info {\n+\tstruct oid_array *shallow;\n+\tint *ours, nr_ours;\n+\tint *theirs, nr_theirs;\n+\tstruct oid_array *ref;\n+\n+\t/* for receive-pack */\n+\tuint32_t **used_shallow;\n+\tint *need_reachability_test;\n+\tint *reachable;\n+\tint *shallow_ref;\n+\tstruct commit **commits;\n+\tint nr_commits;\n+};\n+\n+void prepare_shallow_info(struct shallow_info *, struct oid_array *);\n+void clear_shallow_info(struct shallow_info *);\n+void remove_nonexistent_theirs_shallow(struct shallow_info *);\n+void assign_shallow_commits_to_refs(struct shallow_info *info,\n+\t\t\t\t uint32_t **used,\n+\t\t\t\t int *ref_status);\n+int delayed_reachability_test(struct shallow_info *si, int c);\n+\n+extern struct trace_key trace_shallow;\n+\n+#endif /* SHALLOW_H */\ndiff --git a/upload-pack.c b/upload-pack.c\nindex c53249cac1..e71b068291 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -26,6 +26,7 @@\n #include \"serve.h\"\n #include \"commit-graph.h\"\n #include \"commit-reach.h\"\n+#include \"shallow.h\"\n \n /* Remember to update object flag allocation in object.h */\n #define THEY_HAVE\t(1u << 11)\n-- \n2.26.0.113.ge9739cdccc\n\n"},{"id":"396622","messageId":"65ecfeaba56807926fbe532f94921c98298e50d5.1588199705.git.me@ttaylorr.com","threadId":"53354","inReplyTo":"cover.1588199705.git.me@ttaylorr.com","subject":"[PATCH 3/5] commit: move 'unregister_shallow' to 'shallow.h'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-29T22:39:38Z","receivedAt":"2020-04-29T22:39:44Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In the last commit, we introduced a header for the functions defined in\n'shallow.c'. There is one remaining shallow-related function in\ncommit.c which should be moved, too. This patch moves that function.\n\nSigned-off-by: Taylor Blau \n---\n commit.c | 13 -------------\n shallow.c | 13 +++++++++++++\n shallow.h | 1 +\n 3 files changed, 14 insertions(+), 13 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex eebfbeb45d..87686a7055 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -246,19 +246,6 @@ int for_each_commit_graft(each_commit_graft_fn fn, void *cb_data)\n \treturn ret;\n }\n \n-int unregister_shallow(const struct object_id *oid)\n-{\n-\tint pos = commit_graft_pos(the_repository, oid->hash);\n-\tif (pos < 0)\n-\t\treturn -1;\n-\tif (pos + 1 < the_repository->parsed_objects->grafts_nr)\n-\t\tMOVE_ARRAY(the_repository->parsed_objects->grafts + pos,\n-\t\t\t the_repository->parsed_objects->grafts + pos + 1,\n-\t\t\t the_repository->parsed_objects->grafts_nr - pos - 1);\n-\tthe_repository->parsed_objects->grafts_nr--;\n-\treturn 0;\n-}\n-\n struct commit_buffer {\n \tvoid *buffer;\n \tunsigned long size;\ndiff --git a/shallow.c b/shallow.c\nindex d0191c7609..76e00893fe 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -39,6 +39,19 @@ int register_shallow(struct repository *r, const struct object_id *oid)\n \treturn register_commit_graft(r, graft, 0);\n }\n \n+int unregister_shallow(const struct object_id *oid)\n+{\n+\tint pos = commit_graft_pos(the_repository, oid->hash);\n+\tif (pos < 0)\n+\t\treturn -1;\n+\tif (pos + 1 < the_repository->parsed_objects->grafts_nr)\n+\t\tMOVE_ARRAY(the_repository->parsed_objects->grafts + pos,\n+\t\t\t the_repository->parsed_objects->grafts + pos + 1,\n+\t\t\t the_repository->parsed_objects->grafts_nr - pos - 1);\n+\tthe_repository->parsed_objects->grafts_nr--;\n+\treturn 0;\n+}\n+\n int is_repository_shallow(struct repository *r)\n {\n \tFILE *fp;\ndiff --git a/shallow.h b/shallow.h\nindex 14dd748625..b50a85ed7e 100644\n--- a/shallow.h\n+++ b/shallow.h\n@@ -8,6 +8,7 @@\n \n void set_alternate_shallow_file(struct repository *r, const char *path, int override);\n int register_shallow(struct repository *r, const struct object_id *oid);\n+int unregister_shallow(const struct object_id *oid);\n int is_repository_shallow(struct repository *r);\n int commit_shallow_file(struct repository *r, struct lock_file *lk);\n void rollback_shallow_file(struct repository *r, struct lock_file *lk);\n-- \n2.26.0.113.ge9739cdccc\n\n"},{"id":"396623","messageId":"7828def924a767e54dd43b6c107e8f98c1698f55.1588199705.git.me@ttaylorr.com","threadId":"53354","inReplyTo":"cover.1588199705.git.me@ttaylorr.com","subject":"[PATCH 4/5] shallow.h: document '{commit,rollback}_shallow_file'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-29T22:39:42Z","receivedAt":"2020-04-29T22:39:47Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When 'commit_shallow_file()' and 'rollback_shallow_file()' were\nintroduced, they did not have an documenting comment, when they could\nhave benefited from one.\n\nAdd a brief note about what these functions do, and make a special note\nthat they reset stat-validity checks.\n\nSigned-off-by: Taylor Blau \n---\n shallow.h | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/shallow.h b/shallow.h\nindex b50a85ed7e..08e1bc4fd0 100644\n--- a/shallow.h\n+++ b/shallow.h\n@@ -10,6 +10,10 @@ void set_alternate_shallow_file(struct repository *r, const char *path, int over\n int register_shallow(struct repository *r, const struct object_id *oid);\n int unregister_shallow(const struct object_id *oid);\n int is_repository_shallow(struct repository *r);\n+/*\n+ * {commit,rollback}_shallow_file commits or performs a rollback to the\n+ * '.git/shallow' file, respectively, and resets stat-validity checks.\n+ */\n int commit_shallow_file(struct repository *r, struct lock_file *lk);\n void rollback_shallow_file(struct repository *r, struct lock_file *lk);\n \n-- \n2.26.0.113.ge9739cdccc\n\n"},{"id":"396624","messageId":"c0dc5024a9b368dfbca99b81bc843f66d725f3d7.1588199705.git.me@ttaylorr.com","threadId":"53354","inReplyTo":"cover.1588199705.git.me@ttaylorr.com","subject":"[PATCH 5/5] shallow: use struct 'shallow_lock' for additional safety","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-29T22:39:46Z","receivedAt":"2020-04-29T22:39:50Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In previous patches, the functions 'commit_shallow_file' and\n'rollback_shallow_file' were introduced to reset the shallowness\nvalidity checks on a repository after potentially modifying\n'.git/shallow'.\n\nThese functions can be made safer by wrapping the 'struct lockfile *' in\na new type, 'shallow_lock', so that they cannot be called with a raw\nlock (and potentially misused by other code that happens to possess a\nlockfile, but has nothing to do with shallowness).\n\nThis patch introduces that type as a thin wrapper around 'struct\nlockfile', and updates the two aforementioned functions and their\ncallers to use it.\n\nSuggested-by: Junio C Hamano \nSigned-off-by: Taylor Blau \n---\n builtin/receive-pack.c | 2 +-\n fetch-pack.c | 2 +-\n shallow.c | 22 +++++++++++-----------\n shallow.h | 16 +++++++++++++---\n 4 files changed, 26 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 652661fa99..10fd2c52ec 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -855,7 +855,7 @@ static void refuse_unconfigured_deny_delete_current(void)\n static int command_singleton_iterator(void *cb_data, struct object_id *oid);\n static int update_shallow_ref(struct command *cmd, struct shallow_info *si)\n {\n-\tstruct lock_file shallow_lock = LOCK_INIT;\n+\tstruct shallow_lock shallow_lock = SHALLOW_LOCK_INIT;\n \tstruct oid_array extra = OID_ARRAY_INIT;\n \tstruct check_connected_options opt = CHECK_CONNECTED_INIT;\n \tuint32_t mask = 1 << (cmd->index % 32);\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 401d028e41..27d2d08cc0 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -35,7 +35,7 @@ static int fetch_fsck_objects = -1;\n static int transfer_fsck_objects = -1;\n static int agent_supported;\n static int server_supports_filtering;\n-static struct lock_file shallow_lock;\n+static struct shallow_lock shallow_lock;\n static const char *alternate_shallow_file;\n static struct strbuf fsck_msg_types = STRBUF_INIT;\n \ndiff --git a/shallow.c b/shallow.c\nindex 76e00893fe..1acf8ce95b 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -92,16 +92,16 @@ static void reset_repository_shallow(struct repository *r)\n \tstat_validity_clear(r->parsed_objects->shallow_stat);\n }\n \n-int commit_shallow_file(struct repository *r, struct lock_file *lk)\n+int commit_shallow_file(struct repository *r, struct shallow_lock *lk)\n {\n-\tint res = commit_lock_file(lk);\n+\tint res = commit_lock_file(&lk->lk);\n \treset_repository_shallow(r);\n \treturn res;\n }\n \n-void rollback_shallow_file(struct repository *r, struct lock_file *lk)\n+void rollback_shallow_file(struct repository *r, struct shallow_lock *lk)\n {\n-\trollback_lock_file(lk);\n+\trollback_lock_file(&lk->lk);\n \treset_repository_shallow(r);\n }\n \n@@ -366,22 +366,22 @@ const char *setup_temporary_shallow(const struct oid_array *extra)\n \treturn \"\";\n }\n \n-void setup_alternate_shallow(struct lock_file *shallow_lock,\n+void setup_alternate_shallow(struct shallow_lock *shallow_lock,\n \t\t\t const char **alternate_shallow_file,\n \t\t\t const struct oid_array *extra)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n \tint fd;\n \n-\tfd = hold_lock_file_for_update(shallow_lock,\n+\tfd = hold_lock_file_for_update(&shallow_lock->lk,\n \t\t\t\t git_path_shallow(the_repository),\n \t\t\t\t LOCK_DIE_ON_ERROR);\n \tcheck_shallow_file_for_update(the_repository);\n \tif (write_shallow_commits(&sb, 0, extra)) {\n \t\tif (write_in_full(fd, sb.buf, sb.len) < 0)\n \t\t\tdie_errno(\"failed to write to %s\",\n-\t\t\t\t get_lock_file_path(shallow_lock));\n-\t\t*alternate_shallow_file = get_lock_file_path(shallow_lock);\n+\t\t\t\t get_lock_file_path(&shallow_lock->lk));\n+\t\t*alternate_shallow_file = get_lock_file_path(&shallow_lock->lk);\n \t} else\n \t\t/*\n \t\t * is_repository_shallow() sees empty string as \"no\n@@ -414,7 +414,7 @@ void advertise_shallow_grafts(int fd)\n */\n void prune_shallow(unsigned options)\n {\n-\tstruct lock_file shallow_lock = LOCK_INIT;\n+\tstruct shallow_lock shallow_lock = SHALLOW_LOCK_INIT;\n \tstruct strbuf sb = STRBUF_INIT;\n \tunsigned flags = SEEN_ONLY;\n \tint fd;\n@@ -428,14 +428,14 @@ void prune_shallow(unsigned options)\n \t\tstrbuf_release(&sb);\n \t\treturn;\n \t}\n-\tfd = hold_lock_file_for_update(&shallow_lock,\n+\tfd = hold_lock_file_for_update(&shallow_lock.lk,\n \t\t\t\t git_path_shallow(the_repository),\n \t\t\t\t LOCK_DIE_ON_ERROR);\n \tcheck_shallow_file_for_update(the_repository);\n \tif (write_shallow_commits_1(&sb, 0, NULL, flags)) {\n \t\tif (write_in_full(fd, sb.buf, sb.len) < 0)\n \t\t\tdie_errno(\"failed to write to %s\",\n-\t\t\t\t get_lock_file_path(&shallow_lock));\n+\t\t\t\t get_lock_file_path(&shallow_lock.lk));\n \t\tcommit_shallow_file(the_repository, &shallow_lock);\n \t} else {\n \t\tunlink(git_path_shallow(the_repository));\ndiff --git a/shallow.h b/shallow.h\nindex 08e1bc4fd0..d12096fbc4 100644\n--- a/shallow.h\n+++ b/shallow.h\n@@ -10,12 +10,22 @@ void set_alternate_shallow_file(struct repository *r, const char *path, int over\n int register_shallow(struct repository *r, const struct object_id *oid);\n int unregister_shallow(const struct object_id *oid);\n int is_repository_shallow(struct repository *r);\n+\n+/*\n+ * shallow_lock is a thin wrapper around 'struct lock_file' in order to restrict\n+ * which locks can be used with '{commit,rollback}_shallow_file()'.\n+ */\n+struct shallow_lock {\n+\tstruct lock_file lk;\n+};\n+#define SHALLOW_LOCK_INIT { LOCK_INIT }\n+\n /*\n * {commit,rollback}_shallow_file commits or performs a rollback to the\n * '.git/shallow' file, respectively, and resets stat-validity checks.\n */\n-int commit_shallow_file(struct repository *r, struct lock_file *lk);\n-void rollback_shallow_file(struct repository *r, struct lock_file *lk);\n+int commit_shallow_file(struct repository *r, struct shallow_lock *lk);\n+void rollback_shallow_file(struct repository *r, struct shallow_lock *lk);\n \n struct commit_list *get_shallow_commits(struct object_array *heads,\n \t\t\t\t\tint depth, int shallow_flag, int not_shallow_flag);\n@@ -24,7 +34,7 @@ struct commit_list *get_shallow_commits_by_rev_list(\n int write_shallow_commits(struct strbuf *out, int use_pack_protocol,\n \t\t\t const struct oid_array *extra);\n \n-void setup_alternate_shallow(struct lock_file *shallow_lock,\n+void setup_alternate_shallow(struct shallow_lock *shallow_lock,\n \t\t\t const char **alternate_shallow_file,\n \t\t\t const struct oid_array *extra);\n \n-- \n2.26.0.113.ge9739cdccc\n"},{"id":"396626","messageId":"CAPig+cS4K=crmPqdb0xeoD0H9FGCYdxb4GhQLCoRPEs-uurz6g@mail.gmail.com","threadId":"53354","inReplyTo":"0b93abe072aa35ab96ba3b97118caa8ffe8e439d.1588199705.git.me@ttaylorr.com","subject":"Re: [PATCH 2/5] shallow: extract a header file for shallow-related functions","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-04-29T22:48:51Z","receivedAt":"2020-04-29T22:49:07Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Apr 29, 2020 at 6:39 PM Taylor Blau wrote:\n> There are many functions in commit.h that are more related to shallow\n> repositories than they are to any sort of generic commit machinery.\n> Likely this began when there were only a few shallow-related functions,\n> and commit.h seemed a reasonable enough place to put them.\n>\n> But, now there are a good number of shallow-related functions, and\n> placing them all in 'commit.h' doesn't make sense.\n>\n> This patch extracts a 'shallow.h', which takes all of the headers from\n\nDid you mean s/headers/declarations/ ?\n\n> 'commit.h' for functions which already exist in 'shallow.c'. We will\n> bring the remaining shallow-related functions defined in 'commit.c' in a\n> subsequent patch.\n>\n> For now, move only the ones that already are implemented in 'shallow.c',\n> and update the necessary includes.\n>\n> Signed-off-by: Taylor Blau \n"},{"id":"396627","messageId":"CAPig+cRd4cfig5N29hkKTS8jnD+Vr96a0Tx5p0=DFZtg9KpXLw@mail.gmail.com","threadId":"53354","inReplyTo":"7828def924a767e54dd43b6c107e8f98c1698f55.1588199705.git.me@ttaylorr.com","subject":"Re: [PATCH 4/5] shallow.h: document '{commit,rollback}_shallow_file'","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-04-29T22:53:16Z","receivedAt":"2020-04-29T22:53:31Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Apr 29, 2020 at 6:39 PM Taylor Blau wrote:\n> When 'commit_shallow_file()' and 'rollback_shallow_file()' were\n> introduced, they did not have an documenting comment, when they could\n> have benefited from one.\n\n\"an\" seems wrong; did you mean \"a\" or \"any\"? Or, rewrite as: \"...they\nwere not documented...\".\n\n> Add a brief note about what these functions do, and make a special note\n> that they reset stat-validity checks.\n>\n> Signed-off-by: Taylor Blau \n> ---\n> diff --git a/shallow.h b/shallow.h\n> @@ -10,6 +10,10 @@ void set_alternate_shallow_file(struct repository *r, const char *path, int over\n> int register_shallow(struct repository *r, const struct object_id *oid);\n> int unregister_shallow(const struct object_id *oid);\n> int is_repository_shallow(struct repository *r);\n> +/*\n> + * {commit,rollback}_shallow_file commits or performs a rollback to the\n> + * '.git/shallow' file, respectively, and resets stat-validity checks.\n> + */\n> int commit_shallow_file(struct repository *r, struct lock_file *lk);\n> void rollback_shallow_file(struct repository *r, struct lock_file *lk);\n\nOr, simpler:\n\n /* commit .git/shallow and reset stat-validity checks */\n int commit_shallow_file(...);\n /* rollback .git/shallow and reset stat-validity checks */\n void rollback_shallow_file(...);\n"},{"id":"396628","messageId":"CAPig+cTvX1wT8kAha22uUQ5PDQ0mjeoDGQdA6VtMzfnAELpTaA@mail.gmail.com","threadId":"53354","inReplyTo":"c0dc5024a9b368dfbca99b81bc843f66d725f3d7.1588199705.git.me@ttaylorr.com","subject":"Re: [PATCH 5/5] shallow: use struct 'shallow_lock' for additional safety","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-04-29T23:03:48Z","receivedAt":"2020-04-29T23:04:04Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Apr 29, 2020 at 6:39 PM Taylor Blau wrote:\n> [...]\n> This patch introduces that type as a thin wrapper around 'struct\n> lockfile', and updates the two aforementioned functions and their\n> callers to use it.\n>\n> Signed-off-by: Taylor Blau \n> ---\n> diff --git a/shallow.h b/shallow.h\n> @@ -10,12 +10,22 @@ void set_alternate_shallow_file(struct repository *r, const char *path, int over\n> +/*\n> + * shallow_lock is a thin wrapper around 'struct lock_file' in order to restrict\n> + * which locks can be used with '{commit,rollback}_shallow_file()'.\n> + */\n> +struct shallow_lock {\n> + struct lock_file lk;\n> +};\n\nThe documentation comment for 'shallow_lock' may help newcomers to C\nbut probably doesn't add much value for seasoned programmers. If this\nis the sort of idiom we want to introduce (or exists already in this\ncodebase) -- declaring a specific C type to avoid accidental use of an\nunrelated lock -- then it's probably better documented in\nCodingGuidelines rather than repeating it at each point in the code\nwhich employs the idiom.\n"},{"id":"396633","messageId":"20200429235146.GA20588@syl.local","threadId":"53354","inReplyTo":"CAPig+cTvX1wT8kAha22uUQ5PDQ0mjeoDGQdA6VtMzfnAELpTaA@mail.gmail.com","subject":"Re: [PATCH 5/5] shallow: use struct 'shallow_lock' for additional safety","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-29T23:51:46Z","receivedAt":"2020-04-29T23:51:52Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Apr 29, 2020 at 07:03:48PM -0400, Eric Sunshine wrote:\n> On Wed, Apr 29, 2020 at 6:39 PM Taylor Blau wrote:\n> > [...]\n> > This patch introduces that type as a thin wrapper around 'struct\n> > lockfile', and updates the two aforementioned functions and their\n> > callers to use it.\n> >\n> > Signed-off-by: Taylor Blau \n> > ---\n> > diff --git a/shallow.h b/shallow.h\n> > @@ -10,12 +10,22 @@ void set_alternate_shallow_file(struct repository *r, const char *path, int over\n> > +/*\n> > + * shallow_lock is a thin wrapper around 'struct lock_file' in order to restrict\n> > + * which locks can be used with '{commit,rollback}_shallow_file()'.\n> > + */\n> > +struct shallow_lock {\n> > + struct lock_file lk;\n> > +};\n>\n> The documentation comment for 'shallow_lock' may help newcomers to C\n> but probably doesn't add much value for seasoned programmers. If this\n> is the sort of idiom we want to introduce (or exists already in this\n> codebase) -- declaring a specific C type to avoid accidental use of an\n> unrelated lock -- then it's probably better documented in\n> CodingGuidelines rather than repeating it at each point in the code\n> which employs the idiom.\n\nFair enough; I definitely weighed the usefulness of this comment a\nlittle bit when I was writing it. I figured that I didn't want to\ncommit to updating CodingGuidelines in this series, but that I didn't\nwant to leave the explanation only in the commit message.\n\nI figure that it's probably fine to document it in the commit message,\nand leave it at that for now.\n\nThanks for this suggestion (and the earlier ones, too). I have applied\nthem locally, and I'll sit on them for a day or two before sending out\nv2. Thanks again.\n\n\nThanks,\nTaylor\n"},{"id":"396636","messageId":"20200430002154.231855-1-jonathantanmy@google.com","threadId":"53354","inReplyTo":"0b93abe072aa35ab96ba3b97118caa8ffe8e439d.1588199705.git.me@ttaylorr.com","subject":"Re: [PATCH 2/5] shallow: extract a header file for shallow-related functions","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-04-30T00:21:54Z","receivedAt":"2020-04-30T00:21:59Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"First of all, I couldn't apply these patches neither on latest master\n(86ab15cb15 (\"The fourth batch\", 2020-04-28)) nor on its 2 immediate\nancestors - what is the base of these patches? (Or is there something\nwrong with my workflow?)\n\nI'll review based on the patches themselves, but what I've seen looks\ngood so far.\n\n> diff --git a/builtin.h b/builtin.h\n> index 2b25a80cde..2e701a023c 100644\n> --- a/builtin.h\n> +++ b/builtin.h\n> @@ -5,6 +5,7 @@\n> #include \"strbuf.h\"\n> #include \"cache.h\"\n> #include \"commit.h\"\n> +#include \"shallow.h\"\n\nIt's a pity that builtin.h has to be modified in this way, but I see\nthat it's necessary - a few files that include builtin.h (at least git.c\nand builtin/bisect--helper.c) assume that they have shallow function\naccess through this header.\n\nOnce I manage to apply these patches myself, I'll verify with\n--color-moved that the lines moved as I expect, but otherwise patch 1\nand 2 look good.\n"},{"id":"396637","messageId":"20200430003059.236985-1-jonathantanmy@google.com","threadId":"53354","inReplyTo":"c0dc5024a9b368dfbca99b81bc843f66d725f3d7.1588199705.git.me@ttaylorr.com","subject":"Re: [PATCH 5/5] shallow: use struct 'shallow_lock' for additional safety","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-04-30T00:30:59Z","receivedAt":"2020-04-30T00:31:08Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> @@ -428,14 +428,14 @@ void prune_shallow(unsigned options)\n> \t\tstrbuf_release(&sb);\n> \t\treturn;\n> \t}\n> -\tfd = hold_lock_file_for_update(&shallow_lock,\n> +\tfd = hold_lock_file_for_update(&shallow_lock.lk,\n> \t\t\t\t git_path_shallow(the_repository),\n> \t\t\t\t LOCK_DIE_ON_ERROR);\n> \tcheck_shallow_file_for_update(the_repository);\n> \tif (write_shallow_commits_1(&sb, 0, NULL, flags)) {\n> \t\tif (write_in_full(fd, sb.buf, sb.len) < 0)\n> \t\t\tdie_errno(\"failed to write to %s\",\n> -\t\t\t\t get_lock_file_path(&shallow_lock));\n> +\t\t\t\t get_lock_file_path(&shallow_lock.lk));\n> \t\tcommit_shallow_file(the_repository, &shallow_lock);\n> \t} else {\n> \t\tunlink(git_path_shallow(the_repository));\n\nI was hoping that the inner lock (\"lk\") wouldn't need to be exposed, so\nthat it wouldn't be possible to inadvertently commit the lock without\ngoing through commit_shallow_file(), but this patch is still a step\ntowards that.\n\n> diff --git a/shallow.h b/shallow.h\n> index 08e1bc4fd0..d12096fbc4 100644\n> --- a/shallow.h\n> +++ b/shallow.h\n> @@ -10,12 +10,22 @@ void set_alternate_shallow_file(struct repository *r, const char *path, int over\n> int register_shallow(struct repository *r, const struct object_id *oid);\n> int unregister_shallow(const struct object_id *oid);\n> int is_repository_shallow(struct repository *r);\n> +\n> +/*\n> + * shallow_lock is a thin wrapper around 'struct lock_file' in order to restrict\n> + * which locks can be used with '{commit,rollback}_shallow_file()'.\n> + */\n> +struct shallow_lock {\n> +\tstruct lock_file lk;\n> +};\n\nAs far as I know, we don't use many abbreviations in struct members -\nmaybe s/lk/lock/.\n\nI couldn't find any other issues with all 5 patches in this patch\nseries, but I would like to be able to apply the patches to be sure.\n"},{"id":"396638","messageId":"20200430005945.GA23090@syl.local","threadId":"53354","inReplyTo":"20200430002154.231855-1-jonathantanmy@google.com","subject":"Re: [PATCH 2/5] shallow: extract a header file for shallow-related functions","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-30T00:59:45Z","receivedAt":"2020-04-30T00:59:50Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Jonathan,\n\nOn Wed, Apr 29, 2020 at 05:21:54PM -0700, Jonathan Tan wrote:\n> First of all, I couldn't apply these patches neither on latest master\n> (86ab15cb15 (\"The fourth batch\", 2020-04-28)) nor on its 2 immediate\n> ancestors - what is the base of these patches? (Or is there something\n> wrong with my workflow?)\n\nYes, I should have mentioned in the cover letter that this is based on\n'tb/reset-shallow' which is should be part of Junio's next push to\nmaster. I didn't mention it because I figured that it would be on master\nby the time anyone wanted to look at these patches ;).\n\nAnyhow, apologies. It should apply cleanly on any branch that has\n'tb/reset-shallow' already.\n\n> I'll review based on the patches themselves, but what I've seen looks\n> good so far.\n>\n> > diff --git a/builtin.h b/builtin.h\n> > index 2b25a80cde..2e701a023c 100644\n> > --- a/builtin.h\n> > +++ b/builtin.h\n> > @@ -5,6 +5,7 @@\n> > #include \"strbuf.h\"\n> > #include \"cache.h\"\n> > #include \"commit.h\"\n> > +#include \"shallow.h\"\n>\n> It's a pity that builtin.h has to be modified in this way, but I see\n> that it's necessary - a few files that include builtin.h (at least git.c\n> and builtin/bisect--helper.c) assume that they have shallow function\n> access through this header.\n\nYeah :/.\n\n> Once I manage to apply these patches myself, I'll verify with\n> --color-moved that the lines moved as I expect, but otherwise patch 1\n> and 2 look good.\n\nThanks.\n\nThanks,\nTaylor\n"},{"id":"396645","messageId":"20200430031138.GC115238@google.com","threadId":"53354","inReplyTo":"c0dc5024a9b368dfbca99b81bc843f66d725f3d7.1588199705.git.me@ttaylorr.com","subject":"Re: [PATCH 5/5] shallow: use struct 'shallow_lock' for additional safety","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-04-30T03:11:38Z","receivedAt":"2020-04-30T03:11:43Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nTaylor Blau wrote:\n\n> In previous patches, the functions 'commit_shallow_file' and\n> 'rollback_shallow_file' were introduced to reset the shallowness\n> validity checks on a repository after potentially modifying\n> '.git/shallow'.\n>\n> These functions can be made safer by wrapping the 'struct lockfile *' in\n> a new type, 'shallow_lock', so that they cannot be called with a raw\n> lock (and potentially misused by other code that happens to possess a\n> lockfile, but has nothing to do with shallowness).\n>\n> This patch introduces that type as a thin wrapper around 'struct\n> lockfile', and updates the two aforementioned functions and their\n> callers to use it.\n>\n> Suggested-by: Junio C Hamano \n> Signed-off-by: Taylor Blau \n> ---\n> builtin/receive-pack.c | 2 +-\n> fetch-pack.c | 2 +-\n> shallow.c | 22 +++++++++++-----------\n> shallow.h | 16 +++++++++++++---\n> 4 files changed, 26 insertions(+), 16 deletions(-)\n\nYay! Thanks for indulging the suggestion.\n\n[...]\n> --- a/shallow.h\n> +++ b/shallow.h\n> @@ -10,12 +10,22 @@ void set_alternate_shallow_file(struct repository *r, const char *path, int over\n> int register_shallow(struct repository *r, const struct object_id *oid);\n> int unregister_shallow(const struct object_id *oid);\n> int is_repository_shallow(struct repository *r);\n> +\n> +/*\n> + * shallow_lock is a thin wrapper around 'struct lock_file' in order to restrict\n> + * which locks can be used with '{commit,rollback}_shallow_file()'.\n> + */\n> +struct shallow_lock {\n> +\tstruct lock_file lk;\n> +};\n> +#define SHALLOW_LOCK_INIT { LOCK_INIT }\n\nI think I disagree with Eric here: it's useful to have a comment here\nto describe the purpose of the struct (i.e., the \"why\" as opposed to\nthe \"what\").\n\nI wonder if we can go further, though --- when using a shallow_lock,\nhow should I think of it as a caller? In some sense, the use of\n'struct lock_file' is an implementation detail, so we could say:\n\n\t/*\n\t * Lock for updating the $GIT_DIR/shallow file.\n\t *\n\t * Use `commit_shallow_file()` to commit an update, or\n\t * `rollback_shallow_file()` to roll it back. In either case,\n\t * any in-memory cached information about which commits are\n\t * shallow will be appropriately invalidated so that future\n\t * operations reflect the new state.\n\t */\n\nWhat do you think?\n\n[...]\n> --- a/shallow.c\n> +++ b/shallow.c\n[...]\n> @@ -366,22 +366,22 @@ const char *setup_temporary_shallow(const struct oid_array *extra)\n> \treturn \"\";\n> }\n> \n> -void setup_alternate_shallow(struct lock_file *shallow_lock,\n> +void setup_alternate_shallow(struct shallow_lock *shallow_lock,\n> \t\t\t const char **alternate_shallow_file,\n> \t\t\t const struct oid_array *extra)\n> {\n> \tstruct strbuf sb = STRBUF_INIT;\n> \tint fd;\n> \n> -\tfd = hold_lock_file_for_update(shallow_lock,\n> +\tfd = hold_lock_file_for_update(&shallow_lock->lk,\n> \t\t\t\t git_path_shallow(the_repository),\n> \t\t\t\t LOCK_DIE_ON_ERROR);\n\nThis is peeking into the underlying lock_file, so I should ask myself\nwhether it's hinting at some missing function in the shallow_lock\nAPI. My feeling is \"no\": setup_alternate_shallow is itself that\nfunction. :)\n\n[...]\n> @@ -414,7 +414,7 @@ void advertise_shallow_grafts(int fd)\n> */\n> void prune_shallow(unsigned options)\n> {\n> -\tstruct lock_file shallow_lock = LOCK_INIT;\n> +\tstruct shallow_lock shallow_lock = SHALLOW_LOCK_INIT;\n> \tstruct strbuf sb = STRBUF_INIT;\n> \tunsigned flags = SEEN_ONLY;\n> \tint fd;\n> @@ -428,14 +428,14 @@ void prune_shallow(unsigned options)\n> \t\tstrbuf_release(&sb);\n> \t\treturn;\n> \t}\n> -\tfd = hold_lock_file_for_update(&shallow_lock,\n> +\tfd = hold_lock_file_for_update(&shallow_lock.lk,\n> \t\t\t\t git_path_shallow(the_repository),\n> \t\t\t\t LOCK_DIE_ON_ERROR);\n> \tcheck_shallow_file_for_update(the_repository);\n> \tif (write_shallow_commits_1(&sb, 0, NULL, flags)) {\n> \t\tif (write_in_full(fd, sb.buf, sb.len) < 0)\n> \t\t\tdie_errno(\"failed to write to %s\",\n> -\t\t\t\t get_lock_file_path(&shallow_lock));\n> +\t\t\t\t get_lock_file_path(&shallow_lock.lk));\n> \t\tcommit_shallow_file(the_repository, &shallow_lock);\n\nThere's no obvious helper to extract here either, so this looks good\nto me.\n\nWith whatever tweaks based on Eric's and Jonathan's reviews seem\nappropriate,\n\nReviewed-by: Jonathan Nieder \n\nThanks.\n"},{"id":"396646","messageId":"20200430031350.GD115238@google.com","threadId":"53354","inReplyTo":"65ecfeaba56807926fbe532f94921c98298e50d5.1588199705.git.me@ttaylorr.com","subject":"Re: [PATCH 3/5] commit: move 'unregister_shallow' to 'shallow.h'","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-04-30T03:13:50Z","receivedAt":"2020-04-30T03:13:55Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Taylor Blau wrote:\n\n> In the last commit, we introduced a header for the functions defined in\n> 'shallow.c'. There is one remaining shallow-related function in\n> commit.c which should be moved, too. This patch moves that function.\n> \n> Signed-off-by: Taylor Blau \n> ---\n> commit.c | 13 -------------\n> shallow.c | 13 +++++++++++++\n> shallow.h | 1 +\n> 3 files changed, 14 insertions(+), 13 deletions(-)\n\nYes, an obviously good thing to do.\n\nPerhaps could be squashed with patch 1 (but also see my review of that\none).\n\nReviewed-by: Jonathan Nieder \n\nShould this take a \"struct repository\" parameter?\n"},{"id":"396647","messageId":"20200430031949.GE115238@google.com","threadId":"53354","inReplyTo":"0b93abe072aa35ab96ba3b97118caa8ffe8e439d.1588199705.git.me@ttaylorr.com","subject":"Re: [PATCH 2/5] shallow: extract a header file for shallow-related functions","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-04-30T03:19:49Z","receivedAt":"2020-04-30T03:19:54Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Taylor Blau wrote:\n\n> There are many functions in commit.h that are more related to shallow\n> repositories than they are to any sort of generic commit machinery.\n> Likely this began when there were only a few shallow-related functions,\n> and commit.h seemed a reasonable enough place to put them.\n>\n> But, now there are a good number of shallow-related functions, and\n> placing them all in 'commit.h' doesn't make sense.\n\nSure. For me, there are a few additional sources of motivation:\n\n- shallow clone is a bit of a thorny feature, so I like having the\n indication of which source files are interacting with it\n\n- this will give us a good place to put any overview documentation on\n the shallow API\n\n> This patch extracts a 'shallow.h', which takes all of the headers from\n> 'commit.h' for functions which already exist in 'shallow.c'. We will\n> bring the remaining shallow-related functions defined in 'commit.c' in a\n> subsequent patch.\n>\n> For now, move only the ones that already are implemented in 'shallow.c',\n> and update the necessary includes.\n\nIt's probably worth a mention of the builtin.h part here.\n\n(By the way, I wouldn't be against propagating that to the callers,\nto better match what https://include-what-you-use.org/ would enforce.)\n\nThanks,\nJonathan\n"},{"id":"396648","messageId":"20200430032611.GF115238@google.com","threadId":"53354","inReplyTo":"8ac4c63735f2e48dc83e9f31c9fbbbfb47d7e79d.1588199705.git.me@ttaylorr.com","subject":"Re: [PATCH 1/5] commit: make 'commit_graft_pos' non-static","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-04-30T03:26:11Z","receivedAt":"2020-04-30T03:26:16Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nTaylor Blau wrote:\n\n\n> [...] by making 'commit_graft_pos' non-static so that it can\n> be called from both 'commit.c' and 'shallow.c'.\n>\n> Signed-off-by: Taylor Blau \n> ---\n> commit.c | 2 +-\n> commit.h | 1 +\n> 2 files changed, 2 insertions(+), 1 deletion(-)\n> \n> diff --git a/commit.h b/commit.h\n> index ab91d21131..eb42e8b6d2 100644\n> --- a/commit.h\n> +++ b/commit.h\n> @@ -236,6 +236,7 @@ struct commit_graft {\n> typedef int (*each_commit_graft_fn)(const struct commit_graft *, void *);\n> \n> struct commit_graft *read_graft_line(struct strbuf *line);\n> +int commit_graft_pos(struct repository *r, const unsigned char *sha1);\n\nNow that this function isn't file-local, its name becomes more\nsignificant. What array does this represent a position in? What does\nit return if the graft isn't found? From a call site it's not\nnecessarily obvious.\n\nIdeas:\n\n- could include a comment saying that it's an index into\n r->parsed_objects->grafts\n\n- I'm usually loathe to suggest unnecessary duplication of code, but\n it might make sense to duplicate the function into shallow.c. Or\n even to inline it there (in the single call site, that ends up\n being pretty readable).\n\nThoughts?\n\nThanks,\nJonathan\n"},{"id":"396652","messageId":"CAPig+cQOMJJQBi2KMqzzZQs-S1KVwzAxdfzEaUvSNskMtYkCrw@mail.gmail.com","threadId":"53354","inReplyTo":"20200430031138.GC115238@google.com","subject":"Re: [PATCH 5/5] shallow: use struct 'shallow_lock' for additional safety","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-04-30T05:32:34Z","receivedAt":"2020-04-30T05:32:50Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Apr 29, 2020 at 11:11 PM Jonathan Nieder wrote:\n> Taylor Blau wrote:\n> > +/*\n> > + * shallow_lock is a thin wrapper around 'struct lock_file' in order to restrict\n> > + * which locks can be used with '{commit,rollback}_shallow_file()'.\n> > + */\n>\n> I think I disagree with Eric here: it's useful to have a comment here\n> to describe the purpose of the struct (i.e., the \"why\" as opposed to\n> the \"what\").\n\nI'm not, in general, opposed to the structure being documented; it's\njust that the comment, as presented, doesn't seem to add value.\n\n> I wonder if we can go further, though --- when using a shallow_lock,\n> how should I think of it as a caller? In some sense, the use of\n> 'struct lock_file' is an implementation detail, so we could say:\n>\n> /*\n> * Lock for updating the $GIT_DIR/shallow file.\n> *\n> * Use `commit_shallow_file()` to commit an update, or\n> * `rollback_shallow_file()` to roll it back. In either case,\n> * any in-memory cached information about which commits are\n> * shallow will be appropriately invalidated so that future\n> * operations reflect the new state.\n> */\n>\n> What do you think?\n\nThis comment makes more sense and wouldn't have led to me questioning\nits usefulness. Thanks.\n"},{"id":"396677","messageId":"20200430182302.1969-1-jonathantanmy@google.com","threadId":"53354","inReplyTo":"20200430005945.GA23090@syl.local","subject":"Re: [PATCH 2/5] shallow: extract a header file for shallow-related functions","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-04-30T18:23:02Z","receivedAt":"2020-04-30T18:23:08Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Yes, I should have mentioned in the cover letter that this is based on\n> 'tb/reset-shallow' which is should be part of Junio's next push to\n> master. I didn't mention it because I figured that it would be on master\n> by the time anyone wanted to look at these patches ;).\n> \n> Anyhow, apologies. It should apply cleanly on any branch that has\n> 'tb/reset-shallow' already.\n\nThanks. Now that I'm able to apply the patches, I see that the prototype\nfor unregister_shallow() was removed with no corresponding addition.\nThis causes a compile error when running \"make\" with DEVELOPER=1. Other\nthan that, things look fine.\n"},{"id":"396679","messageId":"20200430192251.GA6280@syl.local","threadId":"53354","inReplyTo":"20200430032611.GF115238@google.com","subject":"Re: [PATCH 1/5] commit: make 'commit_graft_pos' non-static","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-30T19:22:51Z","receivedAt":"2020-04-30T19:22:56Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Apr 29, 2020 at 08:26:11PM -0700, Jonathan Nieder wrote:\n> Hi,\n>\n> Taylor Blau wrote:\n>\n>\n> > [...] by making 'commit_graft_pos' non-static so that it can\n> > be called from both 'commit.c' and 'shallow.c'.\n> >\n> > Signed-off-by: Taylor Blau \n> > ---\n> > commit.c | 2 +-\n> > commit.h | 1 +\n> > 2 files changed, 2 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/commit.h b/commit.h\n> > index ab91d21131..eb42e8b6d2 100644\n> > --- a/commit.h\n> > +++ b/commit.h\n> > @@ -236,6 +236,7 @@ struct commit_graft {\n> > typedef int (*each_commit_graft_fn)(const struct commit_graft *, void *);\n> >\n> > struct commit_graft *read_graft_line(struct strbuf *line);\n> > +int commit_graft_pos(struct repository *r, const unsigned char *sha1);\n>\n> Now that this function isn't file-local, its name becomes more\n> significant. What array does this represent a position in? What does\n> it return if the graft isn't found? From a call site it's not\n> necessarily obvious.\n>\n> Ideas:\n>\n> - could include a comment saying that it's an index into\n> r->parsed_objects->grafts\n\nThis and the below are both good ideas to me. I prefer this one, since\nwe'd have to duplicate yet another static function\n('commit_graft_sha1_access()' directly above) that is called by this\none.\n\n> - I'm usually loathe to suggest unnecessary duplication of code, but\n> it might make sense to duplicate the function into shallow.c. Or\n> even to inline it there (in the single call site, that ends up\n> being pretty readable).\n\nI am not at all offended by duplication of code where it makes sense to\ndo so, but having to duplicate two functions seems like we'd be better\noff simply documenting the function in commit.h.\n\n> Thoughts?\n>\n> Thanks,\n> Jonathan\n\nThanks,\nTaylor\n"},{"id":"396680","messageId":"20200430192931.GB6280@syl.local","threadId":"53354","inReplyTo":"20200430031350.GD115238@google.com","subject":"Re: [PATCH 3/5] commit: move 'unregister_shallow' to 'shallow.h'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-30T19:29:31Z","receivedAt":"2020-04-30T19:29:35Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Apr 29, 2020 at 08:13:50PM -0700, Jonathan Nieder wrote:\n> Taylor Blau wrote:\n>\n> > In the last commit, we introduced a header for the functions defined in\n> > 'shallow.c'. There is one remaining shallow-related function in\n> > commit.c which should be moved, too. This patch moves that function.\n> >\n> > Signed-off-by: Taylor Blau \n> > ---\n> > commit.c | 13 -------------\n> > shallow.c | 13 +++++++++++++\n> > shallow.h | 1 +\n> > 3 files changed, 14 insertions(+), 13 deletions(-)\n>\n> Yes, an obviously good thing to do.\n>\n> Perhaps could be squashed with patch 1 (but also see my review of that\n> one).\n\nDo you mean patch 2/5? If so, I think that it probably makes more sense\nto go there than in the first patch.\n\n(I originally thought that it would be useful for people reading the\ndiff to have this change broken out into its own patch, but I think that\nit's more juggling to do in terms of figuring out which headers to\ninclude than is worth.)\n\n> Reviewed-by: Jonathan Nieder \n>\n> Should this take a \"struct repository\" parameter?\n\nProbably, but I'd rather leave this series to be just focused on moving\nthis code around while being minimally invasive.\n\nThanks,\nTaylor\n"},{"id":"396681","messageId":"20200430193233.GC6280@syl.local","threadId":"53354","inReplyTo":"CAPig+cQOMJJQBi2KMqzzZQs-S1KVwzAxdfzEaUvSNskMtYkCrw@mail.gmail.com","subject":"Re: [PATCH 5/5] shallow: use struct 'shallow_lock' for additional safety","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-30T19:32:33Z","receivedAt":"2020-04-30T19:32:38Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Apr 30, 2020 at 01:32:34AM -0400, Eric Sunshine wrote:\n> On Wed, Apr 29, 2020 at 11:11 PM Jonathan Nieder wrote:\n> > Taylor Blau wrote:\n> > > +/*\n> > > + * shallow_lock is a thin wrapper around 'struct lock_file' in order to restrict\n> > > + * which locks can be used with '{commit,rollback}_shallow_file()'.\n> > > + */\n> >\n> > I think I disagree with Eric here: it's useful to have a comment here\n> > to describe the purpose of the struct (i.e., the \"why\" as opposed to\n> > the \"what\").\n>\n> I'm not, in general, opposed to the structure being documented; it's\n> just that the comment, as presented, doesn't seem to add value.\n>\n> > I wonder if we can go further, though --- when using a shallow_lock,\n> > how should I think of it as a caller? In some sense, the use of\n> > 'struct lock_file' is an implementation detail, so we could say:\n> >\n> > /*\n> > * Lock for updating the $GIT_DIR/shallow file.\n> > *\n> > * Use `commit_shallow_file()` to commit an update, or\n> > * `rollback_shallow_file()` to roll it back. In either case,\n> > * any in-memory cached information about which commits are\n> > * shallow will be appropriately invalidated so that future\n> > * operations reflect the new state.\n> > */\n> >\n> > What do you think?\n>\n> This comment makes more sense and wouldn't have led to me questioning\n> its usefulness. Thanks.\n\nMe either, thanks for the suggestion.\n\nThanks,\nTaylor\n"}]}