[PATCH v2 0/4] shallow: extract a header file

[PATCH v2 0/4] shallow: extract a header file

[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.

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.

         Signed-off-by: Taylor Blau <me@ttaylorr.com>

    - ## 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 <gitster@pobox.com>
    +    Helped-by: Jonathan Nieder <jrnieder@gmail.com>
         Signed-off-by: Taylor Blau <me@ttaylorr.com>

      ## 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

[Taylor Blau]

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'.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 commit.c | 2 +-
 commit.h | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/commit.c b/commit.c
index c7099daeac..b76f7d72be 100644
--- a/commit.c
+++ b/commit.c
@@ -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,
diff --git a/commit.h b/commit.h
index ab91d21131..eb42e8b6d2 100644
--- a/commit.h
+++ b/commit.h
@@ -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);
-- 
2.26.0.113.ge9739cdccc

[PATCH v2 2/4] shallow: extract a header file for shallow-related functions

[Taylor Blau]

There 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.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 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

diff --git a/builtin/fetch.c b/builtin/fetch.c
index bf6bab80fa..a292acd1c0 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -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)
 
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index dc7c58ce3f..303ae312a9 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -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)
diff --git a/builtin/prune.c b/builtin/prune.c
index 2b76872ad2..028ff786d6 100644
--- a/builtin/prune.c
+++ b/builtin/prune.c
@@ -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>...]"),
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 652661fa99..8e3f9bc35f 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -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>"),
diff --git a/builtin/repack.c b/builtin/repack.c
index 0781763b06..2550603608 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -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;
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 06056434ed..669dd2fd6f 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -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
diff --git a/commit-graph.c b/commit-graph.c
index f013a84e29..d316de5021 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -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" */
diff --git a/commit.c b/commit.c
index b76f7d72be..87686a7055 100644
--- a/commit.c
+++ b/commit.c
@@ -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;
diff --git a/commit.h b/commit.h
index eb42e8b6d2..5cd984939b 100644
--- a/commit.h
+++ b/commit.h
@@ -248,55 +248,7 @@ struct commit *get_fork_point(const char *refname, struct commit *commit);
 
 struct oid_array;
 struct ref;
-int register_shallow(struct repository *r, const struct object_id *oid);
-int unregister_shallow(const struct object_id *oid);
-int commit_shallow_file(struct repository *r, struct lock_file *lk);
-void rollback_shallow_file(struct repository *r, struct lock_file *lk);
 int for_each_commit_graft(each_commit_graft_fn, void *);
-int is_repository_shallow(struct repository *r);
-struct commit_list *get_shallow_commits(struct object_array *heads,
-					int depth, int shallow_flag, int not_shallow_flag);
-struct commit_list *get_shallow_commits_by_rev_list(
-		int ac, const char **av, int shallow_flag, int not_shallow_flag);
-void set_alternate_shallow_file(struct repository *r, const char *path, int override);
-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,
-			     const char **alternate_shallow_file,
-			     const struct oid_array *extra);
-const char *setup_temporary_shallow(const struct oid_array *extra);
-void advertise_shallow_grafts(int);
-
-/*
- * Initialize with prepare_shallow_info() or zero-initialize (equivalent to
- * prepare_shallow_info with a NULL oid_array).
- */
-struct shallow_info {
-	struct oid_array *shallow;
-	int *ours, nr_ours;
-	int *theirs, nr_theirs;
-	struct oid_array *ref;
-
-	/* for receive-pack */
-	uint32_t **used_shallow;
-	int *need_reachability_test;
-	int *reachable;
-	int *shallow_ref;
-	struct commit **commits;
-	int nr_commits;
-};
-
-void prepare_shallow_info(struct shallow_info *, struct oid_array *);
-void clear_shallow_info(struct shallow_info *);
-void remove_nonexistent_theirs_shallow(struct shallow_info *);
-void assign_shallow_commits_to_refs(struct shallow_info *info,
-				    uint32_t **used,
-				    int *ref_status);
-int delayed_reachability_test(struct shallow_info *si, int c);
-#define PRUNE_SHOW_ONLY 1
-#define PRUNE_QUICK 2
-void prune_shallow(unsigned options);
-extern struct trace_key trace_shallow;
 
 int interactive_add(int argc, const char **argv, const char *prefix, int patch);
 int run_add_interactive(const char *revision, const char *patch_mode,
diff --git a/environment.c b/environment.c
index 10c9061c43..aaca0e91ac 100644
--- a/environment.c
+++ b/environment.c
@@ -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;
diff --git a/fetch-pack.c b/fetch-pack.c
index a618f3b029..401d028e41 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -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;
diff --git a/git.c b/git.c
index b07198fe03..9b689143c6 100644
--- a/git.c
+++ b/git.c
@@ -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)
diff --git a/send-pack.c b/send-pack.c
index 0407841ae8..e0ccfef75a 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -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)
diff --git a/shallow.c b/shallow.c
index 5010a6c732..76e00893fe 100644
--- a/shallow.c
+++ b/shallow.c
@@ -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;
diff --git a/shallow.h b/shallow.h
new file mode 100644
index 0000000000..b50a85ed7e
--- /dev/null
+++ b/shallow.h
@@ -0,0 +1,64 @@
+#ifndef SHALLOW_H
+#define SHALLOW_H
+
+#include "lockfile.h"
+#include "object.h"
+#include "repository.h"
+#include "strbuf.h"
+
+void set_alternate_shallow_file(struct repository *r, const char *path, int override);
+int 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);
+int commit_shallow_file(struct repository *r, struct lock_file *lk);
+void rollback_shallow_file(struct repository *r, struct lock_file *lk);
+
+struct commit_list *get_shallow_commits(struct object_array *heads,
+					int depth, int shallow_flag, int not_shallow_flag);
+struct commit_list *get_shallow_commits_by_rev_list(
+		int ac, const char **av, int shallow_flag, int not_shallow_flag);
+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,
+			     const char **alternate_shallow_file,
+			     const struct oid_array *extra);
+
+const char *setup_temporary_shallow(const struct oid_array *extra);
+
+void advertise_shallow_grafts(int);
+
+#define PRUNE_SHOW_ONLY 1
+#define PRUNE_QUICK 2
+void prune_shallow(unsigned options);
+
+/*
+ * Initialize with prepare_shallow_info() or zero-initialize (equivalent to
+ * prepare_shallow_info with a NULL oid_array).
+ */
+struct shallow_info {
+	struct oid_array *shallow;
+	int *ours, nr_ours;
+	int *theirs, nr_theirs;
+	struct oid_array *ref;
+
+	/* for receive-pack */
+	uint32_t **used_shallow;
+	int *need_reachability_test;
+	int *reachable;
+	int *shallow_ref;
+	struct commit **commits;
+	int nr_commits;
+};
+
+void prepare_shallow_info(struct shallow_info *, struct oid_array *);
+void clear_shallow_info(struct shallow_info *);
+void remove_nonexistent_theirs_shallow(struct shallow_info *);
+void assign_shallow_commits_to_refs(struct shallow_info *info,
+				    uint32_t **used,
+				    int *ref_status);
+int delayed_reachability_test(struct shallow_info *si, int c);
+
+extern struct trace_key trace_shallow;
+
+#endif /* SHALLOW_H */
diff --git a/upload-pack.c b/upload-pack.c
index c53249cac1..e71b068291 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -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)
-- 
2.26.0.113.ge9739cdccc

[PATCH v2 3/4] shallow.h: document '{commit,rollback}_shallow_file'

[Taylor Blau]

When '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.

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 shallow.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/shallow.h b/shallow.h
index b50a85ed7e..b3bf200bf9 100644
--- a/shallow.h
+++ b/shallow.h
@@ -10,7 +10,9 @@ void set_alternate_shallow_file(struct repository *r, const char *path, int over
 int 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,
-- 
2.26.0.113.ge9739cdccc

[PATCH v2 4/4] shallow: use struct 'shallow_lock' for additional safety

[Taylor Blau]

In 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.

Suggested-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 builtin/receive-pack.c |  2 +-
 fetch-pack.c           |  2 +-
 shallow.c              | 22 +++++++++++-----------
 shallow.h              | 21 ++++++++++++++++++---
 4 files changed, 31 insertions(+), 16 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 8e3f9bc35f..73cebc2f55 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -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);
diff --git a/fetch-pack.c b/fetch-pack.c
index 401d028e41..27d2d08cc0 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -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;
 
diff --git a/shallow.c b/shallow.c
index 76e00893fe..7e0a41ea68 100644
--- a/shallow.c
+++ b/shallow.c
@@ -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));
diff --git a/shallow.h b/shallow.h
index b3bf200bf9..5b4a96dcd6 100644
--- a/shallow.h
+++ b/shallow.h
@@ -10,10 +10,25 @@ void set_alternate_shallow_file(struct repository *r, const char *path, int over
 int 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);
 
-- 
2.26.0.113.ge9739cdccc

Re: [PATCH v2 0/4] shallow: extract a header file

[Taylor Blau]

On Thu, Apr 30, 2020 at 01:48:43PM -0600, Taylor Blau wrote:
> 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

[1]: https://lore.kernel.org/git/cover.1588199705.git.me@ttaylorr.com/

Re: [PATCH v2 1/4] commit: make 'commit_graft_pos' non-static

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> -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,
> diff --git a/commit.h b/commit.h
> index ab91d21131..eb42e8b6d2 100644
> --- a/commit.h
> +++ b/commit.h
> @@ -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:

>> - could include a comment saying that it's an index into
>>   r->parsed_objects->grafts
>
> This 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.
>
>> - 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

[Taylor Blau]

On Thu, Apr 30, 2020 at 01:55:11PM -0700, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> > -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,
> > diff --git a/commit.h b/commit.h
> > index ab91d21131..eb42e8b6d2 100644
> > --- a/commit.h
> > +++ b/commit.h
> > @@ -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:
>
> >> - could include a comment saying that it's an index into
> >>   r->parsed_objects->grafts
> >
> > This 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.
> >
> >> - 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?

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'.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 commit.c | 2 +-
 commit.h | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/commit.c b/commit.c
index c7099daeac..b76f7d72be 100644
--- a/commit.c
+++ b/commit.c
@@ -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,
diff --git a/commit.h b/commit.h
index ab91d21131..0fe1e1b570 100644
--- a/commit.h
+++ b/commit.h
@@ -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