git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 11/11] Resumable clone: implement primer logic in git-clone

From
Junio C Hamano <gitster@pobox.com>
Date
Sep 16, 2016, 23:32 UTC
Message-ID
<xmqqy42rh156.fsf@gitster.mtv.corp.google.com>
In-Reply-To
<1473984742-12516-12-git-send-email-kevin.m.wern@gmail.com>
Kevin Wern <kevin.m.wern@gmail.com> writes:
Show 59 quoted lines
> Use transport_download_primer and transport_prime_clone in git clone.
> This only supports a fully connected packfile.
>
> transport_prime_clone and transport_download_primer are executed
> completely independent of transport_(get|fetch)_remote_refs, et al.
> transport_download_primer is executed based on the existence of an
> alt_resource. The idea is that the "prime clone" execution should be
> able to attempt retrieving an alternate resource without dying, as
> opposed to depending on the result of upload pack's "capabilities" to
> indicate whether or not the client can attempt it.
>
> If a resumable resource is available, execute a codepath with the
> following modular components:
> - downloading resource to a specific directory
> - using the resource (for pack, indexing and generating the bundle
>   file)
> - cleaning up the resource (if the download or use fails)
> - cleaning up the resource (if the download or use succeeds)
>
> If resume is interrupted on the client side, the alternate resource
> info is written to the RESUMABLE file in the git directory.
>
> On resume, the required info is extracted by parsing the created
> config file, and that info is used to determine the work and git
> directories. If these cannot be determined, the program exits.
> The writing of the refspec and determination of the initial git
> directories are skipped, along with transport_prime_clone.
>
> The main purpose of this series of patches is to flesh out a codepath
> for automatic resuming, manual resuming, and leaving a resumable
> directory on exit--the logic for when to do these still needs more
> work.
>
> Signed-off-by: Kevin Wern <kevin.m.wern@gmail.com>
> ---
>  Documentation/git-clone.txt |  16 ++
>  builtin/clone.c             | 590 +++++++++++++++++++++++++++++++++++++-------
>  t/t9904-git-prime-clone.sh  | 181 ++++++++++++++
>  3 files changed, 698 insertions(+), 89 deletions(-)
>  create mode 100755 t/t9904-git-prime-clone.sh
>
> diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
> index b7c467a..5934bb6 100644
> --- a/Documentation/git-clone.txt
> +++ b/Documentation/git-clone.txt
> @@ -16,6 +16,7 @@ SYNOPSIS
>  	  [--depth <depth>] [--[no-]single-branch]
>  	  [--recursive | --recurse-submodules] [--] <repository>
>  	  [<directory>]
> +'git clone --resume <resumable_dir>'
>  
>  DESCRIPTION
>  -----------
> @@ -172,6 +173,12 @@ objects from the source repository into a pack in the cloned repository.
>  	via ssh, this specifies a non-default path for the command
>  	run on the other end.
>  
> +--prime-clone <prime-clone>::
> +-p <prime-clone>::

Not many other options have single letter shorthand. Is it expected that it is worth to let this option squat on a short-and-sweet "-p", perhaps because it is so frequently used?

Show 8 quoted lines
> +--resume::
> +	Resume a partially cloned repo in a "resumable" state. This
> +	can only be specified with a single local directory (<resumable
> +	dir>). This is incompatible with all other options.
> +
> +<resumable_dir>::
> +	The directory of the partial clone. This could be either the
> +	work directory or the git directory.
I think these should be described this way:
    --resume <resumable_dir>::
            description if what resume option does and how resumable_dir
            is used by the option.
in a single bullet point.
Show 10 quoted lines
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 9ac6c01..d9a13dc 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -8,7 +8,9 @@
>   * Clone a repository into a different directory that does not yet exist.
>   */
>  
> +#include "cache.h"
>  #include "builtin.h"

I do not think you need to include cache.h if you are including builtin.h; Documentation/CodingGuidelines says:

 - The first #include in C files, except in platform specific compat/
   implementations, must be either "git-compat-util.h", "cache.h" or
   "builtin.h".  You do not have to include more than one of these.
Show 8 quoted lines
> @@ -40,17 +42,20 @@ static const char * const builtin_clone_usage[] = {
>  
>  static int option_no_checkout, option_bare, option_mirror, option_single_branch = -1;
>  static int option_local = -1, option_no_hardlinks, option_shared, option_recursive;
> +static int option_resume;
>  static char *option_template, *option_depth;
> -static char *option_origin = NULL;
> +static const char *option_origin = NULL;
Is this change related to anything you are doing here?

If you are fixing things while at it, please don't ;-) If you really want to, please also remove " = NULL", from this line and also from the next line. Also do not add " = NULL" at the end of alt_res.

>  static char *option_branch = NULL;
>  ...
> +static const struct alt_resource *alt_res = NULL;
Show 14 quoted lines
> +static char *get_filename(const char *dir)
> +{
> +	char *dir_copy = xstrdup(dir);
> +	strip_trailing_slashes(dir_copy);
> +	char *filename, *final = NULL;
> +
> +	filename = find_last_dir_sep(dir);
> +
> +	if (filename && *(++filename))
> +		final = xstrdup(filename);
> +
> +	free(dir_copy);
> +	return final;
> +}

Hmph, don't we have our own basename(3) lookalike that knows about dir-sep already?

Show 48 quoted lines
> @@ -451,6 +475,7 @@ static const char *junk_work_tree;
>  static const char *junk_git_dir;
>  static enum {
>  	JUNK_LEAVE_NONE,
> +	JUNK_LEAVE_RESUMABLE,
>  	JUNK_LEAVE_REPO,
>  	JUNK_LEAVE_ALL
>  } junk_mode = JUNK_LEAVE_NONE;
> @@ -460,6 +485,29 @@ N_("Clone succeeded, but checkout failed.\n"
>     "You can inspect what was checked out with 'git status'\n"
>     "and retry the checkout with 'git checkout -f HEAD'\n");
>  
> +static const char junk_leave_resumable_msg[] =
> +N_("Clone interrupted while copying resumable resource.\n"
> +   "Try using 'git clone --resume <new_directory>',\n"
> +   "where <new_directory> is either the new working \n"
> +   "directory or git directory.\n\n"
> +   "If this does not succeed, it could be because the\n"
> +   "resource has been moved, corrupted, or changed.\n"
> +   "If this is the case, you should remove <new_directory>\n"
> +   "and run the original command.\n");
> +
> +static void write_resumable_resource()
> +{
> +	const char *filename = git_path_resumable();
> +	struct strbuf content = STRBUF_INIT;
> +	strbuf_addf(&content, "%s\n%s\n", alt_res->url, alt_res->filetype);
> +	int fd = open(filename, O_WRONLY | O_CREAT, 0666);
> +	if (fd < 0)
> +		die_errno(_("Could not open '%s' for writing"), filename);
> +	if (write_in_full(fd, content.buf, content.len) != content.len)
> +		die_errno(_("Could not write to '%s'"), filename);
> +	close(fd);
> +}
>
>  static void remove_junk(void)
>  {
>  	struct strbuf sb = STRBUF_INIT;
> @@ -467,7 +515,11 @@ static void remove_junk(void)
>  	switch (junk_mode) {
>  	case JUNK_LEAVE_REPO:
>  		warning("%s", _(junk_leave_repo_msg));
> -		/* fall-through */
> +		return;
> +	case JUNK_LEAVE_RESUMABLE:
> +		write_resumable_resource();
> +		warning("%s", _(junk_leave_resumable_msg));
> +		return;
Nice.
Show 9 quoted lines
> @@ -562,7 +614,7 @@ static void write_remote_refs(const struct ref *local_refs)
>  		die("%s", err.buf);
>  
>  	for (r = local_refs; r; r = r->next) {
> -		if (!r->peer_ref)
> +		if (!r->peer_ref || ref_exists(r->peer_ref->name))
>  			continue;
>  		if (ref_transaction_create(t, r->peer_ref->name, r->old_oid.hash,
>  					   0, NULL, &err))
What is this change about?
Show 11 quoted lines
> @@ -820,11 +872,296 @@ static void dissociate_from_references(void)
>  	free(alternates);
>  }
>  
> +static int do_index_pack(const char *in_pack_file, const char *out_idx_file)
> +{
> +	const char *argv[] = { "index-pack", "--clone-bundle", "-v",
> +			       "--check-self-contained-and-connected", "-o",
> +			       out_idx_file, in_pack_file, NULL };
> +	return run_command_v_opt(argv, RUN_GIT_CMD|RUN_COMMAND_NO_STDOUT);
> +}
That looks vaguely familiar ;-)
Show 19 quoted lines
> +static const char *setup_and_index_pack(const char *filename)
> +{
> +	const char *primer_idx_path = NULL, *primer_bndl_path = NULL;
> +	primer_idx_path = replace_extension(filename, ".pack", ".idx");
> +	primer_bndl_path = replace_extension(filename, ".pack", ".bndl");
> +
> +	if (!(primer_idx_path && primer_bndl_path)) {
> +		warning("invalid pack filename '%s', falling back to full "
> +			"clone", filename);
> +		return NULL;
> +	}
> +
> +	if (!file_exists(primer_bndl_path)) {
> +		if (do_index_pack(filename, primer_idx_path)) {
> +			warning("could not index primer pack, falling back to "
> +				"full clone");
> +			return NULL;
> +		}
> +	}

Can it be another (undetected) failure mode that .bndl somehow already existed, but not .idx, leaving the resulting object store in an incosistent state? Can do_index_pack() fail and leave .bndl behind to get you into such a state?

Show 17 quoted lines
> +static int write_bundle_refs(const char *bundle_filename)
> +{
> +	struct ref_transaction *t;
> +	struct bundle_header history_tips;
> +	const char *temp_ref_base = "resume";
> +	struct strbuf err = STRBUF_INIT;
> +	int i;
> +
> +	init_bundle_header(&history_tips, bundle_filename);
> +	read_bundle_header(&history_tips);
> +
> +	t = ref_transaction_begin(&err);
> +	for (i = 0; i < history_tips.references.nr; i++) {
> +		struct strbuf ref_name = STRBUF_INIT;
> +		strbuf_addf(&ref_name, "refs/temp/%s/%s/temp-%s",
> +			    option_origin, temp_ref_base,
> +			    sha1_to_hex(history_tips.references.list[i].sha1));
Can we do this without polluting refs/temp/ namespace?

I am imagining that you are first fetching the .pack file from sideways when primer service is available, running index-pack on it to produce the bundle, and the step after that is to run "git fetch" against the original remote to fill the gap between the bit-stale history you got in the bundle and the reality that has progressed since the primer pack was made, and you need a way to tell to the other end that you already have the history leading to these refs when you run "git fetch". I think a bit better way to do so is to send these has ".have" while you run the "fetch".

Wouldn't it do if you add the "--advertise-bundle-tips=<bndl>" option to "git fetch", move the code to read the bundle header to it, and point the bundle's filename with the option when you spawn "git fetch"?

Previous: Kevin WernNext: Kevin Wern
Message 29 of 39 in “Resumable clone”
  1. 00/11 Resumable cloneKevin Wern, Sep 16, 2016
  2. 01/11 Resumable clone: create service git-prime-cloneKevin Wern, Sep 16, 2016
  3. Junio C HamanoSep 16, 2016
  4. Kevin WernSep 28, 2016
  5. 02/11 Resumable clone: add prime-clone endpointsKevin Wern, Sep 16, 2016
  6. Duy NguyenSep 19, 2016
  7. Kevin WernSep 28, 2016
  8. 03/11 pkt-line: create gentle packet_read_line functionsKevin Wern, Sep 16, 2016
  9. Junio C HamanoSep 16, 2016
  10. Kevin WernSep 28, 2016
  11. 04/11 Resumable clone: add prime-clone to remote-curlKevin Wern, Sep 16, 2016
  12. Duy NguyenSep 19, 2016
  13. Kevin WernSep 28, 2016
  14. 05/11 Resumable clone: add output parsing to connect.cKevin Wern, Sep 16, 2016
  15. 06/11 Resumable clone: implement transport_prime_cloneKevin Wern, Sep 16, 2016
  16. 07/11 Resumable clone: add resumable download to http/curlKevin Wern, Sep 16, 2016
  17. Junio C HamanoSep 16, 2016
  18. Kevin WernSep 28, 2016
  19. 08/11 Resumable clone: create transport_download_primerKevin Wern, Sep 16, 2016
  20. 10/11 run command: add RUN_COMMAND_NO_STDOUTKevin Wern, Sep 16, 2016
  21. Junio C HamanoSep 16, 2016
  22. Johannes SchindelinSep 18, 2016
  23. Kevin WernSep 28, 2016
  24. Junio C HamanoSep 28, 2016
  25. Kevin WernSep 28, 2016
  26. 09/11 path: add resumable markerKevin Wern, Sep 16, 2016
  27. Duy NguyenSep 19, 2016
  28. 11/11 Resumable clone: implement primer logic in git-cloneKevin Wern, Sep 16, 2016
  29. Junio C HamanoSep 16, 2016
  30. Kevin WernSep 28, 2016
  31. Duy NguyenSep 19, 2016
  32. Junio C HamanoSep 19, 2016
  33. Kevin WernSep 28, 2016
  34. Junio C HamanoSep 16, 2016
  35. Eric WongSep 27, 2016
  36. Junio C HamanoSep 27, 2016
  37. Junio C HamanoSep 28, 2016
  38. Junio C HamanoSep 28, 2016
  39. Eric WongSep 28, 2016

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.