git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: "Orgad Shaneh via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Orgad Shaneh <orgads@gmail.com>
Subject: Re: [PATCH] fetch: do not look for submodule changes in unchanged refs
Date: Wed, 02 Sep 2020 13:26:18 -0700	[thread overview]
Message-ID: <xmqq3640apsl.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <pull.720.git.1599056635276.gitgitgadget@gmail.com> (Orgad Shaneh via GitGitGadget's message of "Wed, 02 Sep 2020 14:23:55 +0000")

"Orgad Shaneh via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Orgad Shaneh <orgads@gmail.com>
>
> This operation is very expensive, as it scans all the refs using
> setup_revisions, which resolves each ref, including checking if it
> is ambiguous, or if it is a file name etc.

Nobody can tell what "This operation" is without looking at the
patch/diff text.  Our commit message typically gives minimum
explanation of the situation and the problem it tries to solve first
to make it self sufficient.  And then we go on to order the code
base to be in a better shape.  Something along the lines of ...

    When fetching recursively with submodules, for each ref in the
    superproject, we call check_for_new_submodule_commits() to
    figure out X and Y for the object the ref was pointing at before
    the fetch in the superproject, in order to ensure Z.  This is
    expensive because of A, B and C, but it unnecessary if the fetch
    in the superproject did not update the ref (i.e. the objects
    that are required to exist in the submodule did not change).

    Check if we are making any change to the ref, and skip the check
    if we aren't.

... but I didn't fill the most important bits in the above, as by
now you, as the person who encountered the issue and figured out a
good way to solve it, would know what to fill the placeholders with
far better than I would ;-)


> There is no reason to do all that for refs that haven't changed in this
> fetch.
>
> Reported here:
> https://public-inbox.org/git/CAGHpTBKSUJzFSWc=uznSu2zB33qCSmKXM-iAjxRCpqNK5bnhRg@mail.gmail.com/
>
> Amends commit be76c2128234d94b47f7087152ee55d08bb65d88.

I am not sure what this reference is trying to achieve.  Fixing a
bug in be76c212 (fetch: ensure submodule objects fetched,
2018-12-06)?  If so, please say so more directly, perhaps like

    be76c212 (fetch: ensure submodule objects fetched, 2018-12-06)
    tried to do what we are trying to do here, but it botched the
    exectuion by forgetting the fact that ...

or somesuch.  The cited commit says

   The submodule checks were done only when a ref in the
   superproject changed,...

so it is not clear what we are really fixing with this patch,
though.  Is the assertion "checks were done only when changed"
it made incorrect and instead we were doing unnecessary check
always?

> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 0f23dd4b8c..d3f922fc89 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -958,8 +958,10 @@ 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)
> +			if (recurse_submodules != RECURSE_SUBMODULES_OFF &&
> +			    (!rm->peer_ref || !oideq(&ref->old_oid, &ref->new_oid))) {
>  				check_for_new_submodule_commits(&rm->old_oid);
> +			}

The original before be76c212 fed ref->new_oid to the check
function.  Now that we are using ref->{old,new}_oid in the
condition, would it make more sense to pass ref->new_oid
like we did before the commit, or is that an object that is
different from rm->old_oid?

Thanks.

>  			if (!strcmp(rm->name, "HEAD")) {
>  				kind = "";
>
> base-commit: e19713638985533ce461db072b49112da5bd2042

  reply	other threads:[~2020-09-02 20:26 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-02 14:23 Orgad Shaneh via GitGitGadget
2020-09-02 20:26 ` Junio C Hamano [this message]
2020-09-07 15:49   ` Orgad Shaneh
2020-09-04 13:50 ` [PATCH v2] " Orgad Shaneh via GitGitGadget

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=xmqq3640apsl.fsf@gitster.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=orgads@gmail.com \
    --subject='Re: [PATCH] fetch: do not look for submodule changes in unchanged refs' \
    /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

Code repositories for project(s) associated with this 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).