git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jonathan Tan <jonathantanmy@google.com>
To: sbeller@google.com
Cc: git@vger.kernel.org, jonathantanmy@google.com
Subject: Re: [PATCH 9/9] fetch: try fetching submodules if needed objects were not fetched
Date: Tue,  4 Dec 2018 17:07:04 -0800	[thread overview]
Message-ID: <20181205010704.84790-1-jonathantanmy@google.com> (raw)
In-Reply-To: <20181129002756.167615-10-sbeller@google.com>

> Try fetching a submodule by object id if the object id that the
> superproject points to, cannot be found.

Mention here the consequences of what happens when this attempt to fetch
fails. Also, this seems to be a case of "do or do not, there is no try"
- maybe it's better to say "Fetch commits by ID from the submodule's
origin if the submodule doesn't already contain the commit that the
superproject references" (note that there is no "Try" word, since the
consequence is to fail the entire command).

Also mention that this fetch is always from the origin.

> builtin/fetch used to only inspect submodules when they were fetched
> "on-demand", as in either on/off case it was clear whether the submodule
> needs to be fetched. However to know whether we need to try fetching the
> object ids, we need to identify the object names, which is done in this
> function check_for_new_submodule_commits(), so we'll also run that code
> in case the submodule recursion is set to "on".
> 
> The submodule checks were done only when a ref in the superproject
> changed, these checks were extended to also be performed when fetching
> into FETCH_HEAD for completeness, and add a test for that too.

"inspect submodules" and "submodule checks" are unnecessarily vague to
me - might be better to just say "A list of new submodule commits are
already generated in certain conditions (by
check_for_new_submodule_commits()); this new feature invokes that
function in more situations".

> -		if ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&
> -		    (recurse_submodules != RECURSE_SUBMODULES_ON))
> -			check_for_new_submodule_commits(&ref->new_oid);
>  		r = s_update_ref(msg, ref, 0);
>  		format_display(display, r ? '!' : '*', what,
>  			       r ? _("unable to update local ref") : NULL,
> @@ -779,9 +776,6 @@ static int update_local_ref(struct ref *ref,
>  		strbuf_add_unique_abbrev(&quickref, &current->object.oid, DEFAULT_ABBREV);
>  		strbuf_addstr(&quickref, "..");
>  		strbuf_add_unique_abbrev(&quickref, &ref->new_oid, DEFAULT_ABBREV);
> -		if ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&
> -		    (recurse_submodules != RECURSE_SUBMODULES_ON))
> -			check_for_new_submodule_commits(&ref->new_oid);
>  		r = s_update_ref("fast-forward", ref, 1);
>  		format_display(display, r ? '!' : ' ', quickref.buf,
>  			       r ? _("unable to update local ref") : NULL,
> @@ -794,9 +788,6 @@ static int update_local_ref(struct ref *ref,
>  		strbuf_add_unique_abbrev(&quickref, &current->object.oid, DEFAULT_ABBREV);
>  		strbuf_addstr(&quickref, "...");
>  		strbuf_add_unique_abbrev(&quickref, &ref->new_oid, DEFAULT_ABBREV);
> -		if ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&
> -		    (recurse_submodules != RECURSE_SUBMODULES_ON))
> -			check_for_new_submodule_commits(&ref->new_oid);
>  		r = s_update_ref("forced-update", ref, 1);
>  		format_display(display, r ? '!' : '+', quickref.buf,
>  			       r ? _("unable to update local ref") : _("forced update"),
> @@ -892,6 +883,8 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
>  				ref->force = rm->peer_ref->force;
>  			}
>  
> +			if (recurse_submodules != RECURSE_SUBMODULES_OFF)
> +				check_for_new_submodule_commits(&rm->old_oid);

As discussed above, indeed, check_for_new_submodule_commits() is now
invoked in more situations (not just when there is a local ref, and also
when recurse_submodules is _ON).

> @@ -1231,8 +1231,14 @@ struct submodule_parallel_fetch {
>  	int result;
>  
>  	struct string_list changed_submodule_names;
> +
> +	/* The submodules to fetch in */
> +	struct fetch_task **oid_fetch_tasks;
> +	int oid_fetch_tasks_nr, oid_fetch_tasks_alloc;
>  };

Better to document as "Pending fetches by OIDs", I think. (These are not
fetches by default refspec, and are not already in progress.)

> +struct fetch_task {
> +	struct repository *repo;
> +	const struct submodule *sub;
> +	unsigned free_sub : 1; /* Do we need to free the submodule? */
> +
> +	/* fetch specific oids if set, otherwise fetch default refspec */
> +	struct oid_array *commits;
> +};

I would document this as "Fetch in progress (if callback data) or
pending (if in oid_fetch_tasks in struct submodule_parallel_fetch)".
This potential confusion is why I wanted 2 separate types, as I wrote in
[1].

[1] https://public-inbox.org/git/20181026204106.132296-1-jonathantanmy@google.com/

> +/**
> + * When a submodule is not defined in .gitmodules, we cannot access it
> + * via the regular submodule-config. Create a fake submodule, which we can
> + * work on.
> + */
> +static const struct submodule *get_non_gitmodules_submodule(const char *path)

Thanks, this is a good explanation.

>  static int get_next_submodule(struct child_process *cp,
>  			      struct strbuf *err, void *data, void **task_cb)
>  {
> -	int ret = 0;
>  	struct submodule_parallel_fetch *spf = data;
>  
>  	for (; spf->count < spf->r->index->cache_nr; spf->count++) {
> -		struct strbuf submodule_prefix = STRBUF_INIT;
> +		int recurse_config;

Unnecessary local variable recurse_config, but that's not a big deal.

[snip the part where we use a heap-allocated task instead of a few
variables on the stack]

> -			repo_clear(repo);
> -			free(repo);
> -			ret = 1;
> +			spf->count++;
> +			*task_cb = task;

And here we see why we need the heap-allocated task - it needs to go
into the callback data.

> +	if (spf->oid_fetch_tasks_nr) {
> +		struct fetch_task *task =
> +			spf->oid_fetch_tasks[spf->oid_fetch_tasks_nr - 1];
> +		struct strbuf submodule_prefix = STRBUF_INIT;
> +		spf->oid_fetch_tasks_nr--;
> +
> +		strbuf_addf(&submodule_prefix, "%s%s/",
> +			    spf->prefix, task->sub->path);
> +
> +		child_process_init(cp);
> +		prepare_submodule_repo_env_in_gitdir(&cp->env_array);
> +		cp->git_cmd = 1;
> +		cp->dir = task->repo->gitdir;
> +
> +		argv_array_init(&cp->args);
> +		argv_array_pushv(&cp->args, spf->args.argv);
> +		argv_array_push(&cp->args, "on-demand");
> +		argv_array_push(&cp->args, "--submodule-prefix");
> +		argv_array_push(&cp->args, submodule_prefix.buf);
> +
> +		/* NEEDSWORK: have get_default_remote from submodule--helper */
> +		argv_array_push(&cp->args, "origin");
> +		oid_array_for_each_unique(task->commits,
> +					  append_oid_to_argv, &cp->args);
> +
> +		*task_cb = task;
>  		strbuf_release(&submodule_prefix);
> -		if (ret) {
> -			spf->count++;
> -			return 1;
> -		}
> +		return 1;
>  	}

And if we ran out of submodules but have pending fetch-by-OID tasks, we
execute them.

> +static int commit_exists_in_sub(const struct object_id *oid, void *data)
> +{
> +	struct repository *subrepo = data;
> +
> +	enum object_type type = oid_object_info(subrepo, oid, NULL);
> +
> +	return type != OBJ_COMMIT;
> +}

Should be commit_missing_in_sub.

> +	/* Is this the second time we process this submodule? */
> +	if (task->commits)
> +		return 0;

Should be goto out, to clean up properly?

> +test_expect_success "fetch new submodule commits on-demand outside standard refspec" '
> +	# add a second submodule and ensure it is around in downstream first
> +	git clone submodule sub1 &&
> +	git submodule add ./sub1 &&
> +	git commit -m "adding a second submodule" &&
> +	git -C downstream pull &&
> +	git -C downstream submodule update --init --recursive &&
> +
> +	git checkout --detach &&
> +
> +	C=$(git -C submodule commit-tree -m "new change outside refs/heads" HEAD^{tree}) &&
> +	git -C submodule update-ref refs/changes/1 $C &&
> +	git update-index --cacheinfo 160000 $C submodule &&
> +	test_tick &&
> +
> +	D=$(git -C sub1 commit-tree -m "new change outside refs/heads" HEAD^{tree}) &&
> +	git -C sub1 update-ref refs/changes/2 $D &&
> +	git update-index --cacheinfo 160000 $D sub1 &&
> +
> +	git commit -m "updated submodules outside of refs/heads" &&
> +	E=$(git rev-parse HEAD) &&
> +	git update-ref refs/changes/3 $E &&
> +	(
> +		cd downstream &&
> +		git fetch --recurse-submodules origin refs/changes/3:refs/heads/my_branch &&
> +		git -C submodule cat-file -t $C &&
> +		git -C sub1 cat-file -t $D &&
> +		git checkout --recurse-submodules FETCH_HEAD
> +	)
> +'

It would be nicer if all these tests started from scratch (that is, an
empty repository), but they are understandable - thanks.

In addition to the tests here, I would like a test that checks that
submodule commits pointed to by interior superproject commits also work.
For example:

  submodule:
  B   C
   \ /
    A

  superproject:

  current HEAD -> a commit -> the ref we're fetching
  gitlink=A       gitlink=B   gitlink=C

When fetching recursively in the superproject, we should make sure that
both B and C are fetched in the submodule.

  reply	other threads:[~2018-12-05  1:07 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-11-29  0:27 [PATCHv2 0/9] Resending sb/submodule-recursive-fetch-gets-the-tip Stefan Beller
2018-11-29  0:27 ` [PATCH 1/9] sha1-array: provide oid_array_filter Stefan Beller
2018-11-29  0:27 ` [PATCH 2/9] submodule.c: fix indentation Stefan Beller
2018-11-29  0:27 ` [PATCH 3/9] submodule.c: sort changed_submodule_names before searching it Stefan Beller
2018-12-05  0:12   ` Jonathan Tan
2018-11-29  0:27 ` [PATCH 4/9] submodule.c: tighten scope of changed_submodule_names struct Stefan Beller
2018-11-29  0:27 ` [PATCH 5/9] submodule: store OIDs in changed_submodule_names Stefan Beller
2018-11-29  0:27 ` [PATCH 6/9] repository: repo_submodule_init to take a submodule struct Stefan Beller
2018-11-29  0:27 ` [PATCH 7/9] submodule: migrate get_next_submodule to use repository structs Stefan Beller
2018-12-05  0:17   ` Jonathan Tan
2019-02-02  1:58   ` Jonathan Nieder
2018-11-29  0:27 ` [PATCH 8/9] submodule.c: fetch in submodules git directory instead of in worktree Stefan Beller
2018-12-05  0:38   ` Jonathan Tan
2018-11-29  0:27 ` [PATCH 9/9] fetch: try fetching submodules if needed objects were not fetched Stefan Beller
2018-12-05  1:07   ` Jonathan Tan [this message]
2018-12-06 21:26     ` [PATCH] fetch: ensure submodule objects fetched Stefan Beller
2018-12-09  1:57       ` Junio C Hamano
2018-12-05  3:10 ` [PATCHv2 0/9] Resending sb/submodule-recursive-fetch-gets-the-tip Junio C Hamano
2018-12-06 21:59   ` Stefan Beller
2018-12-07  0:25 ` Josh Steadmon
2019-01-15  1:38   ` Jonathan Nieder

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20181205010704.84790-1-jonathantanmy@google.com \
    --to=jonathantanmy@google.com \
    --cc=git@vger.kernel.org \
    --cc=sbeller@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).