git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Stefan Beller <sbeller@google.com>
Cc: git@vger.kernel.org, bmwill@google.com, jrnieder@gmail.com
Subject: Re: [PATCH 3/7] submodule: is_submodule_active to differentiate between new and old mode
Date: Thu, 16 Aug 2018 10:37:17 -0700
Message-ID: <xmqq600afc1e.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <20180816023100.161626-4-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

> The change a086f921a72 (submodule: decouple url and submodule interest,
> 2017-03-17) enables us to do more than originally thought.
> As the url setting was used both to actually set the url where to
> obtain the submodule from, as well as used as a boolean flag later
> to see if it was active, we would need to keep the url around.
>
> Now that submodules can be activated using the submodule.[<name>.]active
> setting, we could remove the url if the submodule is activated via that
> setting.

... as the cloned submodule repository has $GIT_DIR/config which
knows its own origin, we do not need to record URL in superproject's
$GIT_DIR/config.  Back before we started using a directory under
$GIT_DIR/modules/ as a more permanent location to store submodule,
and point at it by a gitdir file in $path/.git to allow removal of a
submodule from the working tree of the superproject without having
to reclone when resurrecting the same submodule, it may have been
useful to keep custom URL somewhere in the superproject, but that no
longer is the case.

> In preparation to do so, pave the way by providing an easy way to see
> if a submodule is considered active via the new .active setting or via
> the old .url setting.

Makes sense.

>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  submodule.c | 5 +----
>  submodule.h | 6 ++++++
>  2 files changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/submodule.c b/submodule.c
> index 6e14547e9e0..d56350ed094 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -221,9 +221,6 @@ int option_parse_recurse_submodules_worktree_updater(const struct option *opt,
>  	return 0;
>  }
>  
> -/*
> - * Determine if a submodule has been initialized at a given 'path'
> - */
>  int is_submodule_active(struct repository *repo, const char *path)
>  {
>  	int ret = 0;
> @@ -267,7 +264,7 @@ int is_submodule_active(struct repository *repo, const char *path)
>  
>  	/* fallback to checking if the URL is set */
>  	key = xstrfmt("submodule.%s.url", module->name);
> -	ret = !repo_config_get_string(repo, key, &value);
> +	ret = !repo_config_get_string(repo, key, &value) ? 2 : 0;
>
>  	free(value);
>  	free(key);
> diff --git a/submodule.h b/submodule.h
> index 4644683e6cb..bfc070e4629 100644
> --- a/submodule.h
> +++ b/submodule.h
> @@ -45,6 +45,12 @@ extern int git_default_submodule_config(const char *var, const char *value, void
>  struct option;
>  int option_parse_recurse_submodules_worktree_updater(const struct option *opt,
>  						     const char *arg, int unset);
> +/*
> + * Determine if a submodule has been initialized at a given 'path'.
> + * Returns 1 if it is considered active via the submodule.[<name>.]active
> + * setting, or return 2 if it is active via the older submodule.url setting.
> + */
> +#define SUBMODULE_ACTIVE_VIA_URL 2
>  extern int is_submodule_active(struct repository *repo, const char *path);
>  /*
>   * Determine if a submodule has been populated at a given 'path' by checking if

This change assumes that all the existing return sites in the
is_submodule_active() function signals true with 1 (or at least some
value that is different from 2).

But the part that uses submodule.active as pathspec list calls
match_pathspec() and uses its return value to signal if the module
is active.  match_pathspec() in turn uses dir.c::do_match_pathspec()
which signals _how_ the set of pathspec list elements matched to the
given name by returning various forms of true, like MATCHED_FNMATCH,
etc.

So I think this patch is insufficient, and needs to at least change
the "submodule.active" codepath to return !!ret; otherwise, a caller
that now expects 0 (not active), 1 (active but can lose URL) and 2
(active and the presence of URL makes it so) will be confused when
one of the MATCHED_* constants from dir.h comes back.




  reply index

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-08-16  2:30 [RFC PATCH 0/7] Unset the submodule URL in the superproject when no longer needed Stefan Beller
2018-08-16  2:30 ` [PATCH 1/7] t7410: update to new style Stefan Beller
2018-08-16 17:06   ` Junio C Hamano
2018-08-16  2:30 ` [PATCH 2/7] builtin/submodule--helper: remove stray new line Stefan Beller
2018-08-16  2:30 ` [PATCH 3/7] submodule: is_submodule_active to differentiate between new and old mode Stefan Beller
2018-08-16 17:37   ` Junio C Hamano [this message]
2018-08-20 19:50     ` Stefan Beller
2018-08-21 21:39       ` Junio C Hamano
2018-08-16  2:30 ` [PATCH 4/7] submodule sync: omit setting submodule URL in config if possible Stefan Beller
2018-08-16  2:30 ` [PATCH 5/7] submodule--helper: factor out allocation of callback cookie Stefan Beller
2018-08-16  2:30 ` [PATCH 6/7] submodule--helper, update_clone: store index to update_clone instead of ce Stefan Beller
2018-08-16  2:31 ` [PATCH 7/7] builtin/submodule--helper: unset submodule url if possible Stefan Beller
2018-08-16 15:12 ` [RFC PATCH 0/7] Unset the submodule URL in the superproject when no longer needed Junio C Hamano
2018-08-16 15:45   ` Stefan Beller

Reply instructions:

You may reply publically 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=xmqq600afc1e.fsf@gitster-ct.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=bmwill@google.com \
    --cc=git@vger.kernel.org \
    --cc=jrnieder@gmail.com \
    --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

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox