Re: [PATCH v3 2/4] 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.
>
> git learns new global option --shallow-file to pass on the alternate
> shallow file path. Undocumented (and not even support --shallow-file=
> syntax) because it's unlikely to be used again elsewhere.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  commit.h              |  2 ++
>  fetch-pack.c          | 69 +++++++++++++++++++++++++--------------------------
>  git.c                 |  5 ++++
>  shallow.c             | 45 +++++++++++++++++++++++++++++++--
>  t/t5500-fetch-pack.sh |  7 ++++++
>  5 files changed, 91 insertions(+), 37 deletions(-)
>
> diff --git a/commit.h b/commit.h
> index 67bd509..6e9c7cd 100644
> --- a/commit.h
> +++ b/commit.h
> @@ -176,6 +176,8 @@ 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);
> +extern void set_alternate_shallow_file(const char *path);
>  
>  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..1ca4f6b 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -20,6 +20,8 @@ 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;
> +static const char *alternate_shallow_file;
>  
>  #define COMPLETE	(1U << 0)
>  #define COMMON		(1U << 1)
> @@ -683,7 +685,7 @@ static int get_pack(struct fetch_pack_args *args,
>  		    int xd[2], char **pack_lockfile)
>  {
>  	struct async demux;
> -	const char *argv[20];
> +	const char *argv[22];
>  	char keep_arg[256];
>  	char hdr_arg[256];
>  	const char **av;
> @@ -724,6 +726,11 @@ static int get_pack(struct fetch_pack_args *args,
>  			do_keep = 1;
>  	}
>  
> +	if (alternate_shallow_file) {
> +		*av++ = "--shallow-file";
> +		*av++ = alternate_shallow_file;
> +	}
> +
>  	if (do_keep) {
>  		if (pack_lockfile)
>  			cmd.out = -1;
> @@ -779,6 +786,23 @@ 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);
> +		alternate_shallow_file = shallow_lock.filename;
> +	} else
> +		alternate_shallow_file = "";

This "empty string, not NULL" trick needs to be commented here to
match the other place that uses this as a cue to do things
differently.

> +	strbuf_release(&sb);
> +}
> +
>  static struct ref *do_fetch_pack(struct fetch_pack_args *args,
>  				 int fd[2],
>  				 const struct ref *orig_ref,
> @@ -858,6 +882,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 +962,9 @@ struct ref *fetch_pack(struct fetch_pack_args *args,
>  		       struct ref **sought, int nr_sought,
>  		       char **pack_lockfile)
>  {
>  	struct ref *ref_cpy;
>  
>  	fetch_pack_setup();
>  	if (nr_sought)
>  		nr_sought = remove_duplicates_in_refs(sought, nr_sought);
>  
> @@ -955,34 +975,13 @@ 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) {
> +		struct stat st;
> +		if (!fstat(shallow_lock.fd, &st) &&
> +		    st.st_size == 0) {
> +			unlink_or_warn(git_path("shallow"));

Are we unlinking the right file here?

> +			rollback_lock_file(&shallow_lock);
> +		} else
> +			commit_lock_file(&shallow_lock);
>  	}
>  
>  	reprepare_packed_git();
> diff --git a/git.c b/git.c
> index 1ada169..6450a38 100644
> --- a/git.c
> +++ b/git.c
> @@ -4,6 +4,7 @@
>  #include "help.h"
>  #include "quote.h"
>  #include "run-command.h"
> +#include "commit.h"
>  
>  const char git_usage_string[] =
>  	"git [--version] [--help] [-c name=value]\n"
> @@ -146,6 +147,10 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
>  			setenv(GIT_LITERAL_PATHSPECS_ENVIRONMENT, "0", 1);
>  			if (envchanged)
>  				*envchanged = 1;
> +		} else if (!strcmp(cmd, "--shallow-file")) {
> +			(*argv)++;
> +			(*argc)--;
> +			set_alternate_shallow_file((*argv)[0]);

Should this count as "envchanged"?

> diff --git a/shallow.c b/shallow.c
> index 6be915f..bdae988 100644
> --- a/shallow.c
> +++ b/shallow.c
> @@ -3,6 +3,16 @@
>  #include "tag.h"
>  
>  static int is_shallow = -1;
> +static struct stat shallow_stat;
> +static char *alternate_shallow_file;
> +
> +void set_alternate_shallow_file(const char *path)
> +{
> +	if (is_shallow != -1)
> +		die("BUG: is_repository_shallow must not be called before set_alternate_shallow_file");
> +	free(alternate_shallow_file);
> +	alternate_shallow_file = path ? xstrdup(path) : NULL;
> +}
>  
>  int register_shallow(const unsigned char *sha1)
>  {
> @@ -21,12 +31,21 @@ int is_repository_shallow(void)
>  {
>  	FILE *fp;
>  	char buf[1024];
> +	const char *path = alternate_shallow_file;
>  
>  	if (is_shallow >= 0)
>  		return is_shallow;
>  
> -	fp = fopen(git_path("shallow"), "r");
> -	if (!fp) {
> +	if (!path)
> +		path = git_path("shallow");
> +	/*
> +	 * fetch-pack set '--shallow-file ""' as an indicator that no

s/set/&s/?

> +	 * shallow file should be used. We could just open it and it
> +	 * will likely fail. But let's do an explicit check instead.
> +	 */
> +	if (!*path ||
> +	    stat(path, &shallow_stat) ||
> +	    (fp = fopen(path, "r")) == NULL) {
>  		is_shallow = 0;
>  		return is_shallow;
>  	}
> @@ -108,3 +127,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");

Forgot to remove this?

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