shallow: extract a header file
v2 cover shallow: extract a header file
Hi,
Here's a reroll of my series to introduce 'shallow.h' after some very helpful review from yesterday evening and early this morning.
Not a ton has changed since last time, but here are the main points. A range-diff since v1 is included below for a more detailed look at the changes.
* The #include in 'builtin.h' was dropped and instead the burden of including 'shallow.h' falls on the callers. Six builtins needed to be updated to include 'shallow.h', so this seems a net positive for the others that don't need to include it.
* Comments were updated in the final two patches for increased readability and helpfulness.
* The third patch was folded into the second patch to make the refactoring occur in a single step.
* A comment was added to 'commit_graft_pos' to indicate what position is being returned.
Like last time, this is based on my earlier series 'tb/reset-shallow', which should be on master shortly (perhaps in the next push-out? Not sure.).
Thanks in advance for a re-review :).
Taylor Blau (4):
commit: make 'commit_graft_pos' non-static
shallow: extract a header file for shallow-related functions shallow.h: document '{commit,rollback}_shallow_file'
shallow: use struct 'shallow_lock' for additional safety
builtin/fetch.c | 1 + builtin/pack-objects.c | 1 + builtin/prune.c | 1 + builtin/receive-pack.c | 3 +- builtin/repack.c | 1 + builtin/rev-parse.c | 1 + commit-graph.c | 1 + commit.c | 16 ++------- commit.h | 49 +------------------------ environment.c | 1 + fetch-pack.c | 3 +- git.c | 1 + send-pack.c | 1 + shallow.c | 36 +++++++++++++------ shallow.h | 81 ++++++++++++++++++++++++++++++++++++++++++ upload-pack.c | 1 + 16 files changed, 123 insertions(+), 75 deletions(-) create mode 100644 shallow.h
Range-diff against v1:
1: 8ac4c63735 = 1: cb8dde2ae2 commit: make 'commit_graft_pos' non-static 2: 8ee7ed0666 < -: ---------- shallow: take 'unregister_shallow' from 'commit.c' 3: 3fb71045b6 ! 2: 0631e2a87d shallow: extract a header file for shallow-related functions
@@ Commit message
But, now there are a good number of shallow-related functions, and placing them all in 'commit.h' doesn't make sense.
- This patch extracts a 'shallow.h', which takes all of the headers from
- 'commit.h' for functions which already exist in 'shallow.c'. We will
- bring the remaining shallow-related functions defined in 'commit.c' in a
- subsequent patch.
+ This patch extracts a 'shallow.h', which takes all of the declarations
+ from 'commit.h' for functions which already exist in 'shallow.c'. We
+ will bring the remaining shallow-related functions defined in 'commit.c'
+ in a subsequent patch.
For now, move only the ones that already are implemented in 'shallow.c', and update the necessary includes.
- ## builtin.h ##
+ ## builtin/fetch.c ##
@@
- #include "strbuf.h"
- #include "cache.h"
- #include "commit.h"
+ #include "branch.h"
+ #include "promisor-remote.h"
+ #include "commit-graph.h"
+#include "shallow.h"
- /*
- * builtin API
+ #define FORCED_UPDATES_DELAY_WARNING_IN_MS (10 * 1000) +
+
+ ## builtin/pack-objects.c ##
+@@
+ #include "dir.h"
+ #include "midx.h"
+ #include "trace2.h"
++#include "shallow.h"
+
+ #define IN_PACK(obj) oe_in_pack(&to_pack, obj)
+ #define SIZE(obj) oe_size(&to_pack, obj)
+
+ ## builtin/prune.c ##
+@@
+ #include "parse-options.h"
+ #include "progress.h"
+ #include "object-store.h"
++#include "shallow.h"
+
+ static const char * const prune_usage[] = {
+ N_("git prune [-n] [-v] [--progress] [--expire <time>] [--] [<head>...]"),
+
+ ## builtin/receive-pack.c ##
+@@
+ #include "protocol.h"
+ #include "commit-reach.h"
+ #include "worktree.h"
++#include "shallow.h"
+
+ static const char * const receive_pack_usage[] = {
+ N_("git receive-pack <git-dir>"),
+
+ ## builtin/repack.c ##
+@@
+ #include "packfile.h"
+ #include "object-store.h"
+ #include "promisor-remote.h"
++#include "shallow.h"
+
+ static int delta_base_offset = 1;
+ static int pack_kept_objects = -1;
+
+ ## builtin/rev-parse.c ##
+@@
+ #include "split-index.h"
+ #include "submodule.h"
+ #include "commit-reach.h"
++#include "shallow.h"
+
+ #define DO_REVS 1
+ #define DO_NOREV 2
## commit-graph.c ##
@@
@@ commit.c
static struct commit_extra_header *read_commit_extra_header_lines(const char *buf, size_t len, const char **);
+@@ commit.c: int for_each_commit_graft(each_commit_graft_fn fn, void *cb_data)
+ return ret;
+ }
+
+-int unregister_shallow(const struct object_id *oid)
+-{
+- int pos = commit_graft_pos(the_repository, oid->hash);
+- if (pos < 0)
+- return -1;
+- if (pos + 1 < the_repository->parsed_objects->grafts_nr)
+- MOVE_ARRAY(the_repository->parsed_objects->grafts + pos,
+- the_repository->parsed_objects->grafts + pos + 1,
+- the_repository->parsed_objects->grafts_nr - pos - 1);
+- the_repository->parsed_objects->grafts_nr--;
+- return 0;
+-}
+-
+ struct commit_buffer {
+ void *buffer;
+ unsigned long size;
## commit.h ##
@@ commit.h: struct commit *get_fork_point(const char *refname, struct commit *commit);
@@ fetch-pack.c
static int transfer_unpack_limit = -1;
static int fetch_unpack_limit = -1;
+ ## git.c ##
+@@
+ #include "help.h"
+ #include "run-command.h"
+ #include "alias.h"
++#include "shallow.h"
+
+ #define RUN_SETUP (1<<0)
+ #define RUN_SETUP_GENTLY (1<<1)
+
## send-pack.c ##
@@
#include "sha1-array.h"
@@ shallow.c
void set_alternate_shallow_file(struct repository *r, const char *path, int override) {
+@@ shallow.c: int register_shallow(struct repository *r, const struct object_id *oid)
+ return register_commit_graft(r, graft, 0);
+ }
+
++int unregister_shallow(const struct object_id *oid)
++{
++ int pos = commit_graft_pos(the_repository, oid->hash);
++ if (pos < 0)
++ return -1;
++ if (pos + 1 < the_repository->parsed_objects->grafts_nr)
++ MOVE_ARRAY(the_repository->parsed_objects->grafts + pos,
++ the_repository->parsed_objects->grafts + pos + 1,
++ the_repository->parsed_objects->grafts_nr - pos - 1);
++ the_repository->parsed_objects->grafts_nr--;
++ return 0;
++}
++
+ int is_repository_shallow(struct repository *r)
+ {
+ FILE *fp;
## shallow.h (new) ##
@@
4: ff3620d50c < -: ---------- shallow.h: document '{commit,rollback}_shallow_file'
-: ---------- > 3: c251cf3ef9 shallow.h: document '{commit,rollback}_shallow_file'
5: 839d5d0d8e ! 4: 08d8a915a0 shallow: use struct 'shallow_lock' for additional safety
@@ Commit message
callers to use it.
Suggested-by: Junio C Hamano <[email protected]>
+ Helped-by: Jonathan Nieder <[email protected]>
Signed-off-by: Taylor Blau <[email protected]>
## builtin/receive-pack.c ##
@@ shallow.c: static void reset_repository_shallow(struct repository *r)
+int commit_shallow_file(struct repository *r, struct shallow_lock *lk)
{
- int res = commit_lock_file(lk);
-+ int res = commit_lock_file(&lk->lk);
++ int res = commit_lock_file(&lk->lock);
reset_repository_shallow(r);
return res;
}
@@ shallow.c: static void reset_repository_shallow(struct repository *r)
+void rollback_shallow_file(struct repository *r, struct shallow_lock *lk)
{
- rollback_lock_file(lk);
-+ rollback_lock_file(&lk->lk);
++ rollback_lock_file(&lk->lock);
reset_repository_shallow(r);
}
@@ shallow.c: const char *setup_temporary_shallow(const struct oid_array *extra)
int fd;
- fd = hold_lock_file_for_update(shallow_lock,
-+ fd = hold_lock_file_for_update(&shallow_lock->lk,
++ fd = hold_lock_file_for_update(&shallow_lock->lock,
git_path_shallow(the_repository),
LOCK_DIE_ON_ERROR);
check_shallow_file_for_update(the_repository);
@@ shallow.c: const char *setup_temporary_shallow(const struct oid_array *extra)
die_errno("failed to write to %s",
- get_lock_file_path(shallow_lock));
- *alternate_shallow_file = get_lock_file_path(shallow_lock);
-+ get_lock_file_path(&shallow_lock->lk));
-+ *alternate_shallow_file = get_lock_file_path(&shallow_lock->lk);
++ get_lock_file_path(&shallow_lock->lock));
++ *alternate_shallow_file = get_lock_file_path(&shallow_lock->lock);
} else
/*
* is_repository_shallow() sees empty string as "no
@@ shallow.c: void prune_shallow(unsigned options)
return;
}
- fd = hold_lock_file_for_update(&shallow_lock,
-+ fd = hold_lock_file_for_update(&shallow_lock.lk,
++ fd = hold_lock_file_for_update(&shallow_lock.lock,
git_path_shallow(the_repository),
LOCK_DIE_ON_ERROR);
check_shallow_file_for_update(the_repository);
@@ shallow.c: void prune_shallow(unsigned options)
if (write_in_full(fd, sb.buf, sb.len) < 0)
die_errno("failed to write to %s",
- get_lock_file_path(&shallow_lock));
-+ get_lock_file_path(&shallow_lock.lk));
++ get_lock_file_path(&shallow_lock.lock));
commit_shallow_file(the_repository, &shallow_lock);
} else {
unlink(git_path_shallow(the_repository));
@@ shallow.h: void set_alternate_shallow_file(struct repository *r, const char *pat
int is_repository_shallow(struct repository *r);
+
+/*
-+ * shallow_lock is a thin wrapper around 'struct lock_file' in order to restrict
-+ * which locks can be used with '{commit,rollback}_shallow_file()'.
++ * Lock for updating the $GIT_DIR/shallow file.
++ *
++ * Use `commit_shallow_file()` to commit an update, or
++ * `rollback_shallow_file()` to roll it back. In either case, any
++ * in-memory cached information about which commits are shallow will be
++ * appropriately invalidated so that future operations reflect the new
++ * state.
+ */
+struct shallow_lock {
-+ struct lock_file lk;
++ struct lock_file lock;
+};
+#define SHALLOW_LOCK_INIT { LOCK_INIT }
+
- /*
- * {commit,rollback}_shallow_file commits or performs a rollback to the
- * '.git/shallow' file, respectively, and resets stat-validity checks.
- */
+ /* commit $GIT_DIR/shallow and reset stat-validity checks */
-int commit_shallow_file(struct repository *r, struct lock_file *lk);
--void rollback_shallow_file(struct repository *r, struct lock_file *lk);
+int commit_shallow_file(struct repository *r, struct shallow_lock *lk);
+ /* rollback $GIT_DIR/shallow and reset stat-validity checks */
+-void rollback_shallow_file(struct repository *r, struct lock_file *lk);
+void rollback_shallow_file(struct repository *r, struct shallow_lock *lk);
struct commit_list *get_shallow_commits(struct object_array *heads,--
2.26.0.113.ge9739cdccc
PATCH v2 1/4 commit: make 'commit_graft_pos' non-static
In the next patch, some functions will be moved from 'commit.c' to have prototypes in a new 'shallow.h' and their implementations in 'shallow.c'.
Three functions in 'commit.c' use 'commit_graft_pos()' (they are 'register_commit_graft()', 'lookup_commit_graft()', and 'unregister_shallow()'). The first two of these will stay in 'commit.c', but the latter will move to 'shallow.c', and thus needs 'commit_graft_pos' to be non-static.
Prepare for that by making 'commit_graft_pos' non-static so that it can be called from both 'commit.c' and 'shallow.c'.
--- commit.c | 2 +- commit.h | 1 + 2 files changed, 2 insertions(+), 1 deletion(-)
commit.c +1 -1
@@ -110,7 +110,7 @@ static const unsigned char *commit_graft_sha1_access(size_t index, void *table)return commit_graft_table[index]->oid.hash; } -static int commit_graft_pos (struct repository *r,const unsigned char *sha1) +int commit_graft_pos (struct repository *r,const unsigned char *sha1) { return sha1_pos(sha1, r->parsed_objects->grafts, r->parsed_objects->grafts_nr,
commit.h +1 -0
@@ -236,6 +236,7 @@ struct commit_graft {typedef int (*each_commit_graft_fn )(const struct commit_graft *,void *);struct commit_graft *read_graft_line (struct strbuf *line); +int commit_graft_pos (struct repository *r,const unsigned char *sha1);int register_commit_graft (struct repository *r,struct commit_graft *,int );void prepare_commit_graft (struct repository *r);struct commit_graft *lookup_commit_graft (struct repository *r,const struct object_id *oid);
Signature · 1 line
2.26.0.113.ge9739cdccc
Re: [PATCH v2 1/4] commit: make 'commit_graft_pos' non-static
Quoted 10 lines · Taylor Blau
commit.c +1
-1-static int commit_graft_pos (struct repository *r,const unsigned char *sha1) +int commit_graft_pos (struct repository *r,const unsigned char *sha1) { return sha1_pos(sha1, r->parsed_objects->grafts, r->parsed_objects->grafts_nr,commit.h +0
-0
commit.h struct commit_graft { · line 239
Quoted 4 lines
commit.h +1
-0@@ -236,6 +236,7 @@ struct commit_graft {typedef int (*each_commit_graft_fn )(const struct commit_graft *,void *);struct commit_graft *read_graft_line (struct strbuf *line); +int commit_graft_pos (struct repository *r,const unsigned char *sha1);
In an earlier exchange, I saw this:
Quoted 8 lines
Quoted 2 lines
- could include a comment saying that it's an index into
r->parsed_objects->graftsThis and the below are both good ideas to me. I prefer this one, since we'd have to duplicate yet another static function ('
commit_graft_sha1_access()' directly above) that is called by this one.
Quoted 8 lines
Quoted 4 lines
- I'm usually loathe to suggest unnecessary duplication of code, but it might make sense to duplicate the function into
shallow.c. Or even to inline it there (in the single call site, that ends up being pretty readable).I am not at all offended by duplication of code where it makes sense to do so, but having to duplicate two functions seems like we'd be better off simply documenting the function in
commit.h.
and I think I agree with that direction. Forgot to add those comments?
Re: [PATCH v2 1/4] commit: make 'commit_graft_pos' non-static
Quoted 32 lines · Junio C Hamano
Quoted 10 lines · Taylor Blau
(quoted diff) +1
-1-static int commit_graft_pos(struct repository *r, const unsigned char *sha1) +int commit_graft_pos(struct repository *r, const unsigned char *sha1) { return sha1_pos(sha1, r->parsed_objects->grafts, r->parsed_objects->grafts_nr,commit.h +0
-0commit.h struct commit_graft { · line 239
Quoted 4 lines
commit.h +1
-0@@ -236,6 +236,7 @@ struct commit_graft {typedef int (*each_commit_graft_fn )(const struct commit_graft *,void *);struct commit_graft *read_graft_line (struct strbuf *line); +int commit_graft_pos (struct repository *r,const unsigned char *sha1);In an earlier exchange, I saw this:
Quoted 13 lines
Quoted 2 lines
- could include a comment saying that it's an index into
r->parsed_objects->graftsThis and the below are both good ideas to me. I prefer this one, since we'd have to duplicate yet another static function ('
commit_graft_sha1_access()' directly above) that is called by this one.Quoted 4 lines
- I'm usually loathe to suggest unnecessary duplication of code, but it might make sense to duplicate the function into
shallow.c. Or even to inline it there (in the single call site, that ends up being pretty readable).
Quoted 6 lines
Quoted 3 lines
I am not at all offended by duplication of code where it makes sense to do so, but having to duplicate two functions seems like we'd be better off simply documenting the function in
commit.h.and I think I agree with that direction. Forgot to add those comments?
Thanks for remembering. I did apply this locally, but must've dropped it on the floor during a rebase. In any case, please swap in this patch instead:
-- >8 --
Subject: [PATCH] commit: make 'commit_graft_pos' non-static
In the next patch, some functions will be moved from 'commit.c' to have prototypes in a new 'shallow.h' and their implementations in 'shallow.c'.
Three functions in 'commit.c' use 'commit_graft_pos()' (they are 'register_commit_graft()', 'lookup_commit_graft()', and 'unregister_shallow()'). The first two of these will stay in 'commit.c', but the latter will move to 'shallow.c', and thus needs 'commit_graft_pos' to be non-static.
Prepare for that by making 'commit_graft_pos' non-static so that it can be called from both 'commit.c' and 'shallow.c'.
--- commit.c | 2 +- commit.h | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-)
commit.c +1 -1
@@ -110,7 +110,7 @@ static const unsigned char *commit_graft_sha1_access(size_t index, void *table)return commit_graft_table[index]->oid.hash; } -static int commit_graft_pos (struct repository *r,const unsigned char *sha1) +int commit_graft_pos (struct repository *r,const unsigned char *sha1) { return sha1_pos(sha1, r->parsed_objects->grafts, r->parsed_objects->grafts_nr,
commit.h +2 -0
@@ -236,6 +236,8 @@ struct commit_graft {typedef int (*each_commit_graft_fn )(const struct commit_graft *,void *);struct commit_graft *read_graft_line (struct strbuf *line); +/* commit_graft_pos returns an index into r->parsed_objects->grafts. */ +int commit_graft_pos (struct repository *r,const unsigned char *sha1);int register_commit_graft (struct repository *r,struct commit_graft *,int );void prepare_commit_graft (struct repository *r);struct commit_graft *lookup_commit_graft (struct repository *r,const struct object_id *oid);
--
2.26.0.113.ge9739cdccc
PATCH v2 2/4 shallow: extract a header file for shallow-related functions
In reply to Taylor BlauThere are many functions in commit.h that are more related to shallow repositories than they are to any sort of generic commit machinery. Likely this began when there were only a few shallow-related functions, and commit.h seemed a reasonable enough place to put them.
But, now there are a good number of shallow-related functions, and placing them all in 'commit.h' doesn't make sense.
This patch extracts a 'shallow.h', which takes all of the declarations from 'commit.h' for functions which already exist in 'shallow.c'. We will bring the remaining shallow-related functions defined in 'commit.c' in a subsequent patch.
For now, move only the ones that already are implemented in 'shallow.c', and update the necessary includes.
--- builtin/fetch.c | 1 + builtin/pack-objects.c | 1 + builtin/prune.c | 1 + builtin/receive-pack.c | 1 + builtin/repack.c | 1 + builtin/rev-parse.c | 1 + commit-graph.c | 1 + commit.c | 14 +-------- commit.h | 48 ------------------------------- environment.c | 1 + fetch-pack.c | 1 + git.c | 1 + send-pack.c | 1 + shallow.c | 14 +++++++++ shallow.h | 64 ++++++++++++++++++++++++++++++++++++++++++ upload-pack.c | 1 + 16 files changed, 91 insertions(+), 61 deletions(-) create mode 100644 shallow.h
builtin/fetch.c +1 -0
@@ -27,6 +27,7 @@#include "branch.h" #include "promisor-remote.h" #include "commit-graph.h" +#include "shallow.h" #define FORCED_UPDATES_DELAY_WARNING_IN_MS (10 * 1000)
builtin/pack-objects.c +1 -0
@@ -34,6 +34,7 @@#include "dir.h" #include "midx.h" #include "trace2.h" +#include "shallow.h" #define IN_PACK (obj) oe_in_pack(&to_pack, obj)#define SIZE (obj) oe_size(&to_pack, obj)
builtin/prune.c +1 -0
@@ -7,6 +7,7 @@#include "parse-options.h" #include "progress.h" #include "object-store.h" +#include "shallow.h" static const char *const prune_usage[] = {N_ ("git prune [-n] [-v] [--progress] [--expire <time>] [--] [<head>...]" ),
builtin/receive-pack.c +1 -0
@@ -28,6 +28,7 @@#include "protocol.h" #include "commit-reach.h" #include "worktree.h" +#include "shallow.h" static const char *const receive_pack_usage[] = {N_ ("git receive-pack <git-dir>" ),
builtin/repack.c +1 -0
@@ -12,6 +12,7 @@#include "packfile.h" #include "object-store.h" #include "promisor-remote.h" +#include "shallow.h" static int delta_base_offset =1 ;static int pack_kept_objects =-1 ;
builtin/rev-parse.c +1 -0
@@ -16,6 +16,7 @@#include "split-index.h" #include "submodule.h" #include "commit-reach.h" +#include "shallow.h" #define DO_REVS 1#define DO_NOREV 2
commit-graph.c +1 -0
@@ -16,6 +16,7 @@#include "hashmap.h" #include "replace-object.h" #include "progress.h" +#include "shallow.h" #define GRAPH_SIGNATURE 0x43475048/* "CGPH" */ #define GRAPH_CHUNKID_OIDFANOUT 0x4f494446/* "OIDF" */
commit.c +1 -13
@@ -20,6 +20,7 @@#include "refs.h" #include "commit-reach.h" #include "run-command.h" +#include "shallow.h" static struct commit_extra_header *read_commit_extra_header_lines (const char *buf,size_t len,const char **);
@@ -245,19 +246,6 @@ int for_each_commit_graft(each_commit_graft_fn fn, void *cb_data)return ret; } -int unregister_shallow (const struct object_id *oid) -{ -int pos =commit_graft_pos (the_repository, oid->hash); -if (pos <0 ) -return -1 ; -if (pos +1 < the_repository->parsed_objects->grafts_nr) -MOVE_ARRAY (the_repository->parsed_objects->grafts + pos, - the_repository->parsed_objects->grafts + pos +1 , - the_repository->parsed_objects->grafts_nr - pos -1 ); - the_repository->parsed_objects->grafts_nr--; -return 0 ; -} -struct commit_buffer {void *buffer;unsigned long size;
commit.h +0 -48
55 lines not on this page · raw message
environment.c +1 -0
@@ -17,6 +17,7 @@#include "argv-array.h" #include "object-store.h" #include "chdir-notify.h" +#include "shallow.h" int trust_executable_bit =1 ;int trust_ctime =1 ;
fetch-pack.c +1 -0
@@ -22,6 +22,7 @@#include "connected.h" #include "fetch-negotiator.h" #include "fsck.h" +#include "shallow.h" static int transfer_unpack_limit =-1 ;static int fetch_unpack_limit =-1 ;
git.c +1 -0
@@ -4,6 +4,7 @@#include "help.h" #include "run-command.h" #include "alias.h" +#include "shallow.h" #define RUN_SETUP (1<<0)#define RUN_SETUP_GENTLY (1<<1)
send-pack.c +1 -0
@@ -15,6 +15,7 @@#include "sha1-array.h" #include "gpg-interface.h" #include "cache.h" +#include "shallow.h" int option_parse_push_signed (const struct option *opt,const char *arg,int unset)
shallow.c +14 -0
@@ -14,6 +14,7 @@#include "commit-slab.h" #include "list-objects.h" #include "commit-reach.h" +#include "shallow.h" void set_alternate_shallow_file (struct repository *r,const char *path,int override) {
@@ -38,6 +39,19 @@ int register_shallow(struct repository *r, const struct object_id *oid)return register_commit_graft (r, graft,0 ); } +int unregister_shallow (const struct object_id *oid) +{ +int pos =commit_graft_pos (the_repository, oid->hash); +if (pos <0 ) +return -1 ; +if (pos +1 < the_repository->parsed_objects->grafts_nr) +MOVE_ARRAY (the_repository->parsed_objects->grafts + pos, + the_repository->parsed_objects->grafts + pos +1 , + the_repository->parsed_objects->grafts_nr - pos -1 ); + the_repository->parsed_objects->grafts_nr--; +return 0 ; +} +int is_repository_shallow (struct repository *r) {FILE *fp;
shallow.h new file +64 -0
64 lines not on this page · raw message
upload-pack.c +1 -0
@@ -26,6 +26,7 @@#include "serve.h" #include "commit-graph.h" #include "commit-reach.h" +#include "shallow.h" /* Remember to update object flag allocation in object.h */ #define THEY_HAVE (1u << 11)
Signature · 1 line
2.26.0.113.ge9739cdccc
PATCH v2 3/4 shallow.h: document '{commit,rollback}_shallow_file'
In reply to Taylor BlauWhen 'commit_shallow_file()' and 'rollback_shallow_file()' were introduced, they did not have a documenting comment, when they could have benefited from one.
Add a brief note about what these functions do, and make a special note that they reset stat-validity checks.
--- shallow.h | 2 ++ 1 file changed, 2 insertions(+)
shallow.h +2 -0
@@ -10,7 +10,9 @@ void set_alternate_shallow_file(struct repository *r, const char *path, int overint register_shallow (struct repository *r,const struct object_id *oid);int unregister_shallow (const struct object_id *oid);int is_repository_shallow (struct repository *r); +/* commit $GIT_DIR/shallow and reset stat-validity checks */ int commit_shallow_file (struct repository *r,struct lock_file *lk); +/* rollback $GIT_DIR/shallow and reset stat-validity checks */ void rollback_shallow_file (struct repository *r,struct lock_file *lk); struct commit_list *get_shallow_commits(struct object_array *heads,
Signature · 1 line
2.26.0.113.ge9739cdccc
PATCH v2 4/4 shallow: use struct 'shallow_lock' for additional safety
In reply to Taylor BlauIn previous patches, the functions 'commit_shallow_file' and 'rollback_shallow_file' were introduced to reset the shallowness validity checks on a repository after potentially modifying '.git/shallow'.
These functions can be made safer by wrapping the 'struct lockfile *' in a new type, 'shallow_lock', so that they cannot be called with a raw lock (and potentially misused by other code that happens to possess a lockfile, but has nothing to do with shallowness).
This patch introduces that type as a thin wrapper around 'struct lockfile', and updates the two aforementioned functions and their callers to use it.
--- builtin/receive-pack.c | 2 +- fetch-pack.c | 2 +- shallow.c | 22 +++++++++++----------- shallow.h | 21 ++++++++++++++++++--- 4 files changed, 31 insertions(+), 16 deletions(-)
builtin/receive-pack.c +1 -1
@@ -856,7 +856,7 @@ static void refuse_unconfigured_deny_delete_current(void)static int command_singleton_iterator (void *cb_data,struct object_id *oid);static int update_shallow_ref (struct command *cmd,struct shallow_info *si) { -struct lock_file shallow_lock =LOCK_INIT ; +struct shallow_lock shallow_lock =SHALLOW_LOCK_INIT ;struct oid_array extra =OID_ARRAY_INIT ;struct check_connected_options opt =CHECK_CONNECTED_INIT ;uint32_t mask =1 << (cmd->index %32 );
fetch-pack.c +1 -1
@@ -35,7 +35,7 @@ static int fetch_fsck_objects = -1;static int transfer_fsck_objects =-1 ;static int agent_supported;static int server_supports_filtering; -static struct lock_file shallow_lock; +static struct shallow_lock shallow_lock;static const char *alternate_shallow_file;static struct strbuf fsck_msg_types =STRBUF_INIT ;
shallow.c +11 -11
@@ -92,16 +92,16 @@ static void reset_repository_shallow(struct repository *r)stat_validity_clear (r->parsed_objects->shallow_stat); } -int commit_shallow_file (struct repository *r,struct lock_file *lk) +int commit_shallow_file (struct repository *r,struct shallow_lock *lk) { -int res =commit_lock_file (lk); +int res =commit_lock_file (&lk->lock);reset_repository_shallow (r);return res; } -void rollback_shallow_file (struct repository *r,struct lock_file *lk) +void rollback_shallow_file (struct repository *r,struct shallow_lock *lk) { -rollback_lock_file (lk); +rollback_lock_file (&lk->lock);reset_repository_shallow (r); }
@@ -366,22 +366,22 @@ const char *setup_temporary_shallow(const struct oid_array *extra)return "" ; } -void setup_alternate_shallow (struct lock_file *shallow_lock, +void setup_alternate_shallow (struct shallow_lock *shallow_lock,const char **alternate_shallow_file,const struct oid_array *extra) {struct strbuf sb =STRBUF_INIT ;int fd; - fd =hold_lock_file_for_update (shallow_lock, + fd =hold_lock_file_for_update (&shallow_lock->lock,git_path_shallow (the_repository),LOCK_DIE_ON_ERROR );check_shallow_file_for_update (the_repository);if (write_shallow_commits (&sb,0 , extra)) {if (write_in_full (fd, sb.buf, sb.len) <0 )die_errno ("failed to write to %s" , -get_lock_file_path (shallow_lock)); - *alternate_shallow_file =get_lock_file_path (shallow_lock); +get_lock_file_path (&shallow_lock->lock)); + *alternate_shallow_file =get_lock_file_path (&shallow_lock->lock); }else /* * is_repository_shallow() sees empty string as "no
@@ -414,7 +414,7 @@ void advertise_shallow_grafts(int fd)*/ void prune_shallow (unsigned options) { -struct lock_file shallow_lock =LOCK_INIT ; +struct shallow_lock shallow_lock =SHALLOW_LOCK_INIT ;struct strbuf sb =STRBUF_INIT ;unsigned flags =SEEN_ONLY ;int fd;
@@ -428,14 +428,14 @@ void prune_shallow(unsigned options)strbuf_release (&sb);return ; } - fd =hold_lock_file_for_update (&shallow_lock, + fd =hold_lock_file_for_update (&shallow_lock.lock,git_path_shallow (the_repository),LOCK_DIE_ON_ERROR );check_shallow_file_for_update (the_repository);if (write_shallow_commits_1 (&sb,0 ,NULL , flags)) {if (write_in_full (fd, sb.buf, sb.len) <0 )die_errno ("failed to write to %s" , -get_lock_file_path (&shallow_lock)); +get_lock_file_path (&shallow_lock.lock));commit_shallow_file (the_repository, &shallow_lock); }else {unlink (git_path_shallow (the_repository));
shallow.h +18 -3
@@ -10,10 +10,25 @@ void set_alternate_shallow_file(struct repository *r, const char *path, int overint register_shallow (struct repository *r,const struct object_id *oid);int unregister_shallow (const struct object_id *oid);int is_repository_shallow (struct repository *r); + +/* +* Lock for updating the $GIT_DIR/shallow file. +* +* Use `commit_shallow_file()` to commit an update, or +* `rollback_shallow_file()` to roll it back. In either case, any +* in-memory cached information about which commits are shallow will be +* appropriately invalidated so that future operations reflect the new +* state. +*/ +struct shallow_lock { +struct lock_file lock; +}; +#define SHALLOW_LOCK_INIT { LOCK_INIT } +/* commit $GIT_DIR/shallow and reset stat-validity checks */ -int commit_shallow_file (struct repository *r,struct lock_file *lk); +int commit_shallow_file (struct repository *r,struct shallow_lock *lk);/* rollback $GIT_DIR/shallow and reset stat-validity checks */ -void rollback_shallow_file (struct repository *r,struct lock_file *lk); +void rollback_shallow_file (struct repository *r,struct shallow_lock *lk);struct commit_list *get_shallow_commits (struct object_array *heads,int depth,int shallow_flag,int not_shallow_flag);
@@ -22,7 +37,7 @@ struct commit_list *get_shallow_commits_by_rev_list(int write_shallow_commits (struct strbuf *out,int use_pack_protocol,const struct oid_array *extra); -void setup_alternate_shallow (struct lock_file *shallow_lock, +void setup_alternate_shallow (struct shallow_lock *shallow_lock,const char **alternate_shallow_file,const struct oid_array *extra);
Signature · 1 line
2.26.0.113.ge9739cdccc
Quoted 4 lines · Taylor Blau
Hi,
Here's a reroll of my series to introduce '
shallow.h' after some very helpful review from yesterday evening and early this morning.
Oops. Forgot to set --in-reply-to to the v1 of this series, which can be found in [1]. Since I already sent the cover letter and patches, let's just pick it up from here.
Thanks,
Taylor