mbox series

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

Message ID cover.1588275891.git.me@ttaylorr.com (mailing list archive)
Headers show
Series shallow: extract a header file | expand

Message

Taylor Blau April 30, 2020, 7:48 p.m. UTC 
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

Comments

Taylor Blau April 30, 2020, 7:49 p.m. UTC | #1 
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/