Re: [PATCH v2 2/5] fetch-pack: prepare updated shallow file before fetching the pack

[Junio C Hamano <gitster@pobox.com>]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> index-pack --strict looks up and follows parent commits. If shallow
> information is not ready by the time index-pack is run, index-pack may
> be lead to non-existent objects. Make fetch-pack save shallow file to
> disk before invoking index-pack.
>
> A new environment variable GIT_SHALLOW_FILE is used to inform
> index-pack to use an alternate shallow file. This variable is for
> internal use and thus not advertised in git.txt.

The idea to tell the index-pack to use a different shallow file is
sound, but is the environment variable the best way to go?  I am
mostly worried about it being an implicit "apply everywhere"
mechanism and while I do not particularly like the idea of doing
everything in the same process as the top-level while dealing with
submodules, I am reasonably sure people would want to "clone"
recursively inside the same top-level process in the future, and
this new environment variable adds one more thing to be cleansed
when crossing a repository boundary.

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  commit.h              |  1 +
>  fetch-pack.c          | 65 ++++++++++++++++++++++++---------------------------
>  shallow.c             | 30 ++++++++++++++++++++++--
>  t/t5500-fetch-pack.sh |  7 ++++++
>  4 files changed, 67 insertions(+), 36 deletions(-)
>
> diff --git a/commit.h b/commit.h
> index 67bd509..0cef659 100644
> --- a/commit.h
> +++ b/commit.h
> @@ -176,6 +176,7 @@ extern int for_each_commit_graft(each_commit_graft_fn, void *);
>  extern int is_repository_shallow(void);
>  extern struct commit_list *get_shallow_commits(struct object_array *heads,
>  		int depth, int shallow_flag, int not_shallow_flag);
> +extern void check_shallow_file_for_update(void);
>  
>  int is_descendant_of(struct commit *, struct commit_list *);
>  int in_merge_bases(struct commit *, struct commit *);
> diff --git a/fetch-pack.c b/fetch-pack.c
> index f156dd4..17cfa88 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -20,6 +20,7 @@ static int no_done;
>  static int fetch_fsck_objects = -1;
>  static int transfer_fsck_objects = -1;
>  static int agent_supported;
> +static struct lock_file shallow_lock;
>  
>  #define COMPLETE	(1U << 0)
>  #define COMMON		(1U << 1)
> @@ -779,6 +780,26 @@ static int cmp_ref_by_name(const void *a_, const void *b_)
>  	return strcmp(a->name, b->name);
>  }
>  
> +static void setup_alternate_shallow(void)
> +{
> +	struct strbuf sb = STRBUF_INIT;
> +	int fd;
> +
> +	check_shallow_file_for_update();
> +	fd = hold_lock_file_for_update(&shallow_lock, git_path("shallow"),
> +				       LOCK_DIE_ON_ERROR);
> +	if (write_shallow_commits(&sb, 0)) {
> +		if (write_in_full(fd, sb.buf, sb.len) != sb.len)
> +			die_errno("failed to write to %s", shallow_lock.filename);
> +		if (setenv("GIT_SHALLOW_FILE", shallow_lock.filename, 1))
> +			die_errno("failed to set GIT_SHALLOW_FILE");
> +	} else {
> +		if (setenv("GIT_SHALLOW_FILE", "", 1))
> +			die_errno("failed to set GIT_SHALLOW_FILE");
> +	}
> +	strbuf_release(&sb);
> +}
> +
>  static struct ref *do_fetch_pack(struct fetch_pack_args *args,
>  				 int fd[2],
>  				 const struct ref *orig_ref,
> @@ -858,6 +879,8 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
>  
>  	if (args->stateless_rpc)
>  		packet_flush(fd[1]);
> +	if (args->depth > 0)
> +		setup_alternate_shallow();
>  	if (get_pack(args, fd, pack_lockfile))
>  		die("git fetch-pack: fetch failed.");
>  
> @@ -936,15 +959,9 @@ struct ref *fetch_pack(struct fetch_pack_args *args,
>  		       struct ref **sought, int nr_sought,
>  		       char **pack_lockfile)
>  {
> -	struct stat st;
>  	struct ref *ref_cpy;
>  
>  	fetch_pack_setup();
> -	if (args->depth > 0) {
> -		if (stat(git_path("shallow"), &st))
> -			st.st_mtime = 0;
> -	}
> -
>  	if (nr_sought)
>  		nr_sought = remove_duplicates_in_refs(sought, nr_sought);
>  
> @@ -955,34 +972,14 @@ struct ref *fetch_pack(struct fetch_pack_args *args,
>  	ref_cpy = do_fetch_pack(args, fd, ref, sought, nr_sought, pack_lockfile);
>  
>  	if (args->depth > 0) {
> -		static struct lock_file lock;
> -		struct cache_time mtime;
> -		struct strbuf sb = STRBUF_INIT;
> -		char *shallow = git_path("shallow");
> -		int fd;
> -
> -		mtime.sec = st.st_mtime;
> -		mtime.nsec = ST_MTIME_NSEC(st);
> -		if (stat(shallow, &st)) {
> -			if (mtime.sec)
> -				die("shallow file was removed during fetch");
> -		} else if (st.st_mtime != mtime.sec
> -#ifdef USE_NSEC
> -				|| ST_MTIME_NSEC(st) != mtime.nsec
> -#endif
> -			  )
> -			die("shallow file was changed during fetch");
> -
> -		fd = hold_lock_file_for_update(&lock, shallow,
> -					       LOCK_DIE_ON_ERROR);
> -		if (!write_shallow_commits(&sb, 0)
> -		 || write_in_full(fd, sb.buf, sb.len) != sb.len) {
> -			unlink_or_warn(shallow);
> -			rollback_lock_file(&lock);
> -		} else {
> -			commit_lock_file(&lock);
> -		}
> -		strbuf_release(&sb);
> +		struct stat st;
> +		if (!fstat(shallow_lock.fd, &st) &&
> +		    st.st_size == 0) {
> +			unlink_or_warn(git_path("shallow"));
> +			rollback_lock_file(&shallow_lock);
> +		} else
> +			commit_lock_file(&shallow_lock);
> +		unsetenv("GIT_SHALLOW_FILE");
>  	}
>  
>  	reprepare_packed_git();
> diff --git a/shallow.c b/shallow.c
> index 6be915f..9c03806 100644
> --- a/shallow.c
> +++ b/shallow.c
> @@ -3,6 +3,7 @@
>  #include "tag.h"
>  
>  static int is_shallow = -1;
> +static struct stat shallow_stat;
>  
>  int register_shallow(const unsigned char *sha1)
>  {
> @@ -21,12 +22,15 @@ int is_repository_shallow(void)
>  {
>  	FILE *fp;
>  	char buf[1024];
> +	const char *path;
>  
>  	if (is_shallow >= 0)
>  		return is_shallow;
>  
> -	fp = fopen(git_path("shallow"), "r");
> -	if (!fp) {
> +	path = getenv("GIT_SHALLOW_FILE");
> +	if (!path)
> +		path = git_path("shallow");
> +	if (stat(path, &shallow_stat) || (fp = fopen(path, "r")) == NULL) {
>  		is_shallow = 0;
>  		return is_shallow;
>  	}
> @@ -108,3 +112,25 @@ struct commit_list *get_shallow_commits(struct object_array *heads, int depth,
>  
>  	return result;
>  }
> +
> +void check_shallow_file_for_update(void)
> +{
> +	struct stat st;
> +
> +	if (getenv("GIT_SHALLOW_FILE"))
> +		die("GIT_SHALLOW_FILE should not be manually set");
> +
> +	if (!is_shallow)
> +		return;
> +	else if (is_shallow == -1)
> +		die("BUG: shallow must be initialized by now");
> +
> +	if (stat(git_path("shallow"), &st))
> +		die("shallow file was removed during fetch");
> +	else if (st.st_mtime != shallow_stat.st_mtime
> +#ifdef USE_NSEC
> +		 || ST_MTIME_NSEC(st) != ST_MTIME_NSEC(shallow_stat)
> +#endif
> +		   )
> +		die("shallow file was changed during fetch");
> +}
> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
> index d574085..557b073 100755
> --- a/t/t5500-fetch-pack.sh
> +++ b/t/t5500-fetch-pack.sh
> @@ -135,6 +135,13 @@ test_expect_success 'clone shallow depth 1' '
>  	test "`git --git-dir=shallow0/.git rev-list --count HEAD`" = 1
>  '
>  
> +test_expect_success 'clone shallow depth 1 with fsck' '
> +	git config --global fetch.fsckobjects true &&
> +	git clone --no-single-branch --depth 1 "file://$(pwd)/." shallow0fsck &&
> +	test "`git --git-dir=shallow0fsck/.git rev-list --count HEAD`" = 1 &&
> +	git config --global --unset fetch.fsckobjects
> +'
> +
>  test_expect_success 'clone shallow' '
>  	git clone --no-single-branch --depth 2 "file://$(pwd)/." shallow
>  '