* RE: [PATCH 07/16] update submodules: introduce submodule_is_interesting
From: David Turner @ 2016-11-16 0:14 UTC (permalink / raw)
To: 'Stefan Beller'
Cc: git@vger.kernel.org, bmwill@google.com, gitster@pobox.com,
jrnieder@gmail.com, mogulguy10@gmail.com
In-Reply-To: <20161115230651.23953-8-sbeller@google.com>
> -----Original Message-----
> From: Stefan Beller [mailto:sbeller@google.com]
> Sent: Tuesday, November 15, 2016 6:07 PM
> Cc: git@vger.kernel.org; bmwill@google.com; gitster@pobox.com;
> jrnieder@gmail.com; mogulguy10@gmail.com; David Turner; Stefan Beller
> Subject: [PATCH 07/16] update submodules: introduce
> submodule_is_interesting
> +int submodule_is_interesting(const char *path, const unsigned char
> +*sha1) {
This is apparently only ever (in this series) called with null_sha1. So either this arg is unnecessary, or there are bugs elsewhere in the code.
^ permalink raw reply
* Re: [PATCH 11/16] teach unpack_trees() to remove submodule contents
From: Brandon Williams @ 2016-11-16 0:14 UTC (permalink / raw)
To: Stefan Beller; +Cc: git, gitster, jrnieder, mogulguy10, David.Turner
In-Reply-To: <20161115230651.23953-12-sbeller@google.com>
On 11/15, Stefan Beller wrote:
> Extend rmdir_or_warn() to remove the directories of those submodules which
> are scheduled for removal. Also teach verify_clean_submodule() to check
> that a submodule configured to be removed is not modified before scheduling
> it for removal.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> unpack-trees.c | 6 ++----
> wrapper.c | 4 ++++
> 2 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/unpack-trees.c b/unpack-trees.c
> index ea6bdd2..576e1d5 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -9,6 +9,7 @@
> #include "refs.h"
> #include "attr.h"
> #include "split-index.h"
> +#include "submodule.h"
> #include "dir.h"
>
> /*
> @@ -1361,15 +1362,12 @@ static void invalidate_ce_path(const struct cache_entry *ce,
> /*
> * Check that checking out ce->sha1 in subdir ce->name is not
> * going to overwrite any working files.
> - *
> - * Currently, git does not checkout subprojects during a superproject
> - * checkout, so it is not going to overwrite anything.
> */
> static int verify_clean_submodule(const struct cache_entry *ce,
> enum unpack_trees_error_types error_type,
> struct unpack_trees_options *o)
> {
> - return 0;
> + return submodule_is_interesting(ce->name, null_sha1) && is_submodule_modified(ce->name, 0);
> }
So what does the return value from this function meant to mean? Is '1'
mean the submodule is clean while '0' indicates it is dirty or is it the
reverse of that? Reading this it seems to me a value of '1' means "yes
the submodule is clean!" but the way the return value is calculated
tells a different story. Either I'm understanding it incorrectly or I
think the return should be something like this:
return submodule_is_interesting(ce->name, null_sha1) && !is_submodule_modified(ce->name, 0);
Where we return '1' if the submodule is interesting and it hasn't been
modified.
>
> static int verify_clean_subdirectory(const struct cache_entry *ce,
> diff --git a/wrapper.c b/wrapper.c
> index e7f1979..17c08de 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -2,6 +2,7 @@
> * Various trivial helper wrappers around standard functions
> */
> #include "cache.h"
> +#include "submodule.h"
>
> static void do_nothing(size_t size)
> {
> @@ -592,6 +593,9 @@ int unlink_or_warn(const char *file)
>
> int rmdir_or_warn(const char *file)
> {
> + if (submodule_is_interesting(file, null_sha1)
> + && depopulate_submodule(file))
> + return -1;
> return warn_if_unremovable("rmdir", file, rmdir(file));
> }
It seems weird to me that rmdir is doing checks to see if the file being
removed is a submodule. Shouldn't those checks have occurred before
calling rmdir?
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH v3 4/4] submodule_needs_pushing() NEEDSWORK when we can not answer this question
From: Junio C Hamano @ 2016-11-16 0:13 UTC (permalink / raw)
To: Stefan Beller
Cc: Heiko Voigt, git@vger.kernel.org, Jeff King, Jens Lehmann,
Fredrik Gustafsson, Leandro Lucarella
In-Reply-To: <CAGZ79kYyyjP7W7gWq6WomVSkhRtMbZZMKYQPFszko4_f9oprgg@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
>> "We do not know" ...
>
> ... because there is no way to check for us as we don't have the
> submodule commits.
>
> " We do consider it safe as no one in their sane mind would
> have changed the submodule pointers without having the
> submodule around. If a user did however change the submodules
> without having the submodule commits around, this indicates an
> expert who knows what they were doing."
I didn't think it through myself to arrive at such a conclusion, but
to me the above sounds like a sensible reasoning [*1*].
>> We currently
>> + * proceed pushing here as if the submodules commits are
>> + * available on a remote. Since we can not check the
>> + * remote availability for this submodule we should
>> + * consider changing this behavior to: Stop here and
>> + * tell the user how to skip this check if wanted.
>> + */
>> return 0;
>
> Thanks for adding the NEEDSWORK, I just wrote the above lines
> to clarify my thought process, not as a suggestion for change.
One thing I would suggest would be "Stop here, explain the situation
and then tell the user how to skip". I am not convinced that it
would _help_ users and make it _safer_ if we stopped here, though.
> Overall the series looks good to me; the nits are minor IMHO.
Ditto.
[Footnote]
*1* My version was more like "we do not know if they would get into
a situation where they do not have enough submodule commits if
we pushed our superproject, but more importantly, we DO KNOW
that it would not help an iota if we pushed our submodule to
them, so there is no point stopping the push of superproject
saying 'no, no, no, you must push the submodule first'".
^ permalink raw reply
* RE: [PATCH 09/16] update submodules: add scheduling to update submodules
From: David Turner @ 2016-11-16 0:07 UTC (permalink / raw)
To: 'Brandon Williams', Stefan Beller
Cc: git@vger.kernel.org, gitster@pobox.com, jrnieder@gmail.com,
mogulguy10@gmail.com
In-Reply-To: <20161116000236.GF66382@google.com>
> -----Original Message-----
> From: Brandon Williams [mailto:bmwill@google.com]
> > +struct scheduled_submodules_update_type {
> > + const char *path;
> > + const struct object_id *oid;
> > + /*
> > + * Do we need to perform a complete checkout or just incremental
> > + * update?
> > + */
> > + unsigned is_new:1;
> > +} *scheduled_submodules;
> > +#define SCHEDULED_SUBMODULES_INIT {NULL, NULL}
>
> I may not know enough about these types of initializors but that Init
> macro only has 2 entries while there are three entries in the struct
> itself.
In fact, only the first NULL is necessary; unspecified initializer entries in C default to zero.
^ permalink raw reply
* Re: [PATCH v3 4/6] grep: optionally recurse into submodules
From: Stefan Beller @ 2016-11-16 0:07 UTC (permalink / raw)
To: Brandon Williams; +Cc: git@vger.kernel.org, Jonathan Tan, Junio C Hamano
In-Reply-To: <1478908273-190166-5-git-send-email-bmwill@google.com>
On Fri, Nov 11, 2016 at 3:51 PM, Brandon Williams <bmwill@google.com> wrote:
> Allow grep to recognize submodules and recursively search for patterns in
> each submodule. This is done by forking off a process to recursively
> call grep on each submodule. The top level --super-prefix option is
> used to pass a path to the submodule which can in turn be used to
> prepend to output or in pathspec matching logic.
>
> Recursion only occurs for submodules which have been initialized and
> checked out by the parent project. If a submodule hasn't been
> initialized and checked out it is simply skipped.
>
> In order to support the existing multi-threading infrastructure in grep,
> output from each child process is captured in a strbuf so that it can be
> later printed to the console in an ordered fashion.
>
> To limit the number of theads that are created, each child process has
> half the number of threads as its parents (minimum of 1), otherwise we
> potentailly have a fork-bomb.
>
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
> Documentation/git-grep.txt | 5 +
> builtin/grep.c | 300 ++++++++++++++++++++++++++++++++++---
> git.c | 2 +-
> t/t7814-grep-recurse-submodules.sh | 99 ++++++++++++
> 4 files changed, 385 insertions(+), 21 deletions(-)
> create mode 100755 t/t7814-grep-recurse-submodules.sh
>
> diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
> index 0ecea6e..17aa1ba 100644
> --- a/Documentation/git-grep.txt
> +++ b/Documentation/git-grep.txt
> @@ -26,6 +26,7 @@ SYNOPSIS
> [--threads <num>]
> [-f <file>] [-e] <pattern>
> [--and|--or|--not|(|)|-e <pattern>...]
> + [--recurse-submodules]
> [ [--[no-]exclude-standard] [--cached | --no-index | --untracked] | <tree>...]
> [--] [<pathspec>...]
>
> @@ -88,6 +89,10 @@ OPTIONS
> mechanism. Only useful when searching files in the current directory
> with `--no-index`.
>
> +--recurse-submodules::
> + Recursively search in each submodule that has been initialized and
> + checked out in the repository.
> +
> -a::
> --text::
> Process binary files as if they were text.
> diff --git a/builtin/grep.c b/builtin/grep.c
> index 8887b6a..1fd292f 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -18,12 +18,20 @@
> #include "quote.h"
> #include "dir.h"
> #include "pathspec.h"
> +#include "submodule.h"
>
> static char const * const grep_usage[] = {
> N_("git grep [<options>] [-e] <pattern> [<rev>...] [[--] <path>...]"),
> NULL
> };
>
> +static const char *super_prefix;
> +static int recurse_submodules;
> +static struct argv_array submodule_options = ARGV_ARRAY_INIT;
> +
> +static int grep_submodule_launch(struct grep_opt *opt,
> + const struct grep_source *gs);
> +
> #define GREP_NUM_THREADS_DEFAULT 8
> static int num_threads;
>
> @@ -174,7 +182,10 @@ static void *run(void *arg)
> break;
>
> opt->output_priv = w;
> - hit |= grep_source(opt, &w->source);
> + if (w->source.type == GREP_SOURCE_SUBMODULE)
> + hit |= grep_submodule_launch(opt, &w->source);
> + else
> + hit |= grep_source(opt, &w->source);
> grep_source_clear_data(&w->source);
> work_done(w);
> }
> @@ -300,6 +311,10 @@ static int grep_sha1(struct grep_opt *opt, const unsigned char *sha1,
> if (opt->relative && opt->prefix_length) {
> quote_path_relative(filename + tree_name_len, opt->prefix, &pathbuf);
> strbuf_insert(&pathbuf, 0, filename, tree_name_len);
> + } else if (super_prefix) {
> + strbuf_add(&pathbuf, filename, tree_name_len);
> + strbuf_addstr(&pathbuf, super_prefix);
> + strbuf_addstr(&pathbuf, filename + tree_name_len);
> } else {
> strbuf_addstr(&pathbuf, filename);
> }
> @@ -328,10 +343,13 @@ static int grep_file(struct grep_opt *opt, const char *filename)
> {
> struct strbuf buf = STRBUF_INIT;
>
> - if (opt->relative && opt->prefix_length)
> + if (opt->relative && opt->prefix_length) {
> quote_path_relative(filename, opt->prefix, &buf);
> - else
> + } else {
> + if (super_prefix)
> + strbuf_addstr(&buf, super_prefix);
> strbuf_addstr(&buf, filename);
> + }
>
> #ifndef NO_PTHREADS
> if (num_threads) {
> @@ -378,31 +396,258 @@ static void run_pager(struct grep_opt *opt, const char *prefix)
> exit(status);
> }
>
> -static int grep_cache(struct grep_opt *opt, const struct pathspec *pathspec, int cached)
> +static void compile_submodule_options(const struct grep_opt *opt,
> + const struct pathspec *pathspec,
> + int cached, int untracked,
> + int opt_exclude, int use_index,
> + int pattern_type_arg)
> +{
> + struct grep_pat *pattern;
> + int i;
> +
> + if (recurse_submodules)
> + argv_array_push(&submodule_options, "--recurse-submodules");
> +
> + if (cached)
> + argv_array_push(&submodule_options, "--cached");
> + if (!use_index)
> + argv_array_push(&submodule_options, "--no-index");
> + if (untracked)
> + argv_array_push(&submodule_options, "--untracked");
> + if (opt_exclude > 0)
> + argv_array_push(&submodule_options, "--exclude-standard");
> +
> + if (opt->invert)
> + argv_array_push(&submodule_options, "-v");
> + if (opt->ignore_case)
> + argv_array_push(&submodule_options, "-i");
> + if (opt->word_regexp)
> + argv_array_push(&submodule_options, "-w");
> + switch (opt->binary) {
> + case GREP_BINARY_NOMATCH:
> + argv_array_push(&submodule_options, "-I");
> + break;
> + case GREP_BINARY_TEXT:
> + argv_array_push(&submodule_options, "-a");
> + break;
> + default:
> + break;
> + }
> + if (opt->allow_textconv)
> + argv_array_push(&submodule_options, "--textconv");
> + if (opt->max_depth != -1)
> + argv_array_pushf(&submodule_options, "--max-depth=%d",
> + opt->max_depth);
> + if (opt->linenum)
> + argv_array_push(&submodule_options, "-n");
> + if (!opt->pathname)
> + argv_array_push(&submodule_options, "-h");
> + if (!opt->relative)
> + argv_array_push(&submodule_options, "--full-name");
> + if (opt->name_only)
> + argv_array_push(&submodule_options, "-l");
> + if (opt->unmatch_name_only)
> + argv_array_push(&submodule_options, "-L");
> + if (opt->null_following_name)
> + argv_array_push(&submodule_options, "-z");
> + if (opt->count)
> + argv_array_push(&submodule_options, "-c");
> + if (opt->file_break)
> + argv_array_push(&submodule_options, "--break");
> + if (opt->heading)
> + argv_array_push(&submodule_options, "--heading");
> + if (opt->pre_context)
> + argv_array_pushf(&submodule_options, "--before-context=%d",
> + opt->pre_context);
> + if (opt->post_context)
> + argv_array_pushf(&submodule_options, "--after-context=%d",
> + opt->post_context);
> + if (opt->funcname)
> + argv_array_push(&submodule_options, "-p");
> + if (opt->funcbody)
> + argv_array_push(&submodule_options, "-W");
> + if (opt->all_match)
> + argv_array_push(&submodule_options, "--all-match");
> + if (opt->debug)
> + argv_array_push(&submodule_options, "--debug");
> + if (opt->status_only)
> + argv_array_push(&submodule_options, "-q");
> +
> + switch (pattern_type_arg) {
> + case GREP_PATTERN_TYPE_BRE:
> + argv_array_push(&submodule_options, "-G");
> + break;
> + case GREP_PATTERN_TYPE_ERE:
> + argv_array_push(&submodule_options, "-E");
> + break;
> + case GREP_PATTERN_TYPE_FIXED:
> + argv_array_push(&submodule_options, "-F");
> + break;
> + case GREP_PATTERN_TYPE_PCRE:
> + argv_array_push(&submodule_options, "-P");
> + break;
> + case GREP_PATTERN_TYPE_UNSPECIFIED:
> + break;
> + }
> +
> + for (pattern = opt->pattern_list; pattern != NULL;
> + pattern = pattern->next) {
> + switch (pattern->token) {
> + case GREP_PATTERN:
> + argv_array_pushf(&submodule_options, "-e%s",
> + pattern->pattern);
> + break;
> + case GREP_AND:
> + case GREP_OPEN_PAREN:
> + case GREP_CLOSE_PAREN:
> + case GREP_NOT:
> + case GREP_OR:
> + argv_array_push(&submodule_options, pattern->pattern);
> + break;
> + /* BODY and HEAD are not used by git-grep */
> + case GREP_PATTERN_BODY:
> + case GREP_PATTERN_HEAD:
> + break;
> + }
> + }
> +
> + /*
> + * Limit number of threads for child process to use.
> + * This is to prevent potential fork-bomb behavior of git-grep as each
> + * submodule process has its own thread pool.
> + */
> + if (num_threads)
> + argv_array_pushf(&submodule_options, "--threads=%d",
> + (num_threads + 1) / 2);
I think you would want to pass --threads=%d unconditionally,
as it also serves as a weak defusal for fork bombs. Is it possible to come here
with num_threads=0? (i.e. what happens if the user doesn't specify the number
of threads or such, do we fall back to some default or is it just 0?)
I have seen some other places that check for num_threads unequal to 0,
as e.g. no mutex needs to be locked then (assuming we don't have any
thread but grep within the main process), but as you intend to use this also
as a helper to not blow up the number of threads recursively, we'd need to
pass at a number != 0 here?
> +
> + git grep -e "bar" --and -e "foo" --recurse-submodules > actual &&
nit here and in the tests below:
We prefer to have no white space between > and the file piped to.
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'grep and multiple patterns' '
> + cat >expect <<-\EOF &&
> + b/b:bar
> + EOF
> +
> + git grep -e "bar" --and --not -e "foo" --recurse-submodules > actual &&
Otherwise, this patch looks good.
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCH 10/16] update submodules: is_submodule_checkout_safe
From: Brandon Williams @ 2016-11-16 0:06 UTC (permalink / raw)
To: Stefan Beller; +Cc: git, gitster, jrnieder, mogulguy10, David.Turner
In-Reply-To: <20161115230651.23953-11-sbeller@google.com>
On 11/15, Stefan Beller wrote:
> In later patches we introduce the options and flag for commands
> that modify the working directory, e.g. git-checkout.
>
> This piece of code will answer the question:
> "Is it safe to change the submodule to this new state?"
> e.g. is it overwriting untracked files or are there local
> changes that would be overwritten?
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> submodule.c | 22 ++++++++++++++++++++++
> submodule.h | 2 ++
> 2 files changed, 24 insertions(+)
>
> diff --git a/submodule.c b/submodule.c
> index 2773151..2149ef7 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1201,6 +1201,28 @@ int ok_to_remove_submodule(const char *path)
> return ok_to_remove;
> }
>
> +int is_submodule_checkout_safe(const char *path, const struct object_id *oid)
> +{
> + struct child_process cp = CHILD_PROCESS_INIT;
> +
> + if (!is_submodule_populated(path))
> + /* The submodule is not populated, it's safe to check it out */
> + /*
> + * TODO: When git learns to re-populate submodules, a check must be
> + * added here to assert that no local files will be overwritten.
> + */
When you mean local files do you mean in the situation where we want to
checkout a submodule at path 'foo' but there already exists a file at
path 'foo'?
> + return 1;
> +
> + argv_array_pushl(&cp.args, "read-tree", "-n", "-m", "HEAD",
> + sha1_to_hex(oid->hash), NULL);
> +
> + prepare_submodule_repo_env(&cp.env_array);
> + cp.git_cmd = 1;
> + cp.no_stdin = 1;
> + cp.dir = path;
> + return run_command(&cp) == 0;
> +}
> +
> static int find_first_merges(struct object_array *result, const char *path,
> struct commit *a, struct commit *b)
> {
> diff --git a/submodule.h b/submodule.h
> index f01f87c..a7fa634 100644
> --- a/submodule.h
> +++ b/submodule.h
> @@ -74,6 +74,8 @@ extern unsigned is_submodule_modified(const char *path, int ignore_untracked);
> extern int is_submodule_populated(const char *path);
> extern int submodule_uses_gitfile(const char *path);
> extern int ok_to_remove_submodule(const char *path);
> +extern int is_submodule_checkout_safe(const char *path,
> + const struct object_id *oid);
> extern int merge_submodule(unsigned char result[20], const char *path,
> const unsigned char base[20],
> const unsigned char a[20],
> --
> 2.10.1.469.g00a8914
>
--
Brandon Williams
^ permalink raw reply
* Re: gitweb html validation
From: Junio C Hamano @ 2016-11-16 0:05 UTC (permalink / raw)
To: Ralf Thielow, Jakub Narębski; +Cc: Raphaël Gertz, git
In-Reply-To: <20161115182632.GA17539@gmail.com>
Ralf Thielow <ralf.thielow@gmail.com> writes:
> Only block level elements are
> allowed to be inside form tags, according to
> https://www.w3.org/2010/04/xhtml10-strict.html#elem_form
> ...
> I think it's better to just move the <form>-Tag outside of the
> surrounding div?
> Something like this perhaps, I didn't test it myself yet.
That sounds like a sensible update to me (no, I do not run gitweb
myself). Is this the only <form> we have in the UI, or is it the
only one that is problematic?
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 7cf68f07b..33d7c154f 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -5531,8 +5531,8 @@ sub git_project_search_form {
> $limit = " in '$project_filter/'";
> }
>
> - print "<div class=\"projsearch\">\n";
> print $cgi->start_form(-method => 'get', -action => $my_uri) .
> + "<div class=\"projsearch\">\n" .
> $cgi->hidden(-name => 'a', -value => 'project_list') . "\n";
> print $cgi->hidden(-name => 'pf', -value => $project_filter). "\n"
> if (defined $project_filter);
> @@ -5544,11 +5544,11 @@ sub git_project_search_form {
> -checked => $search_use_regexp) .
> "</span>\n" .
> $cgi->submit(-name => 'btnS', -value => 'Search') .
> - $cgi->end_form() . "\n" .
> $cgi->a({-href => href(project => undef, searchtext => undef,
> project_filter => $project_filter)},
> esc_html("List all projects$limit")) . "<br />\n";
> - print "</div>\n";
> + print "</div>\n" .
> + $cgi->end_form() . "\n";
> }
>
> # entry for given @keys needs filling if at least one of keys in list
> diff --git a/gitweb/static/gitweb.css b/gitweb/static/gitweb.css
> index 321260103..507740b6a 100644
> --- a/gitweb/static/gitweb.css
> +++ b/gitweb/static/gitweb.css
> @@ -539,7 +539,7 @@ div.projsearch {
> margin: 20px 0px;
> }
>
> -div.projsearch form {
> +form div.projsearch {
> margin-bottom: 2px;
> }
>
^ permalink raw reply
* Re: [PATCH 09/16] update submodules: add scheduling to update submodules
From: Brandon Williams @ 2016-11-16 0:02 UTC (permalink / raw)
To: Stefan Beller; +Cc: git, gitster, jrnieder, mogulguy10, David.Turner
In-Reply-To: <20161115230651.23953-10-sbeller@google.com>
On 11/15, Stefan Beller wrote:
> +static int update_submodule(const char *path, const struct object_id *oid,
> + int force, int is_new)
> +{
> + const char *git_dir;
> + struct child_process cp = CHILD_PROCESS_INIT;
> + const struct submodule *sub = submodule_from_path(null_sha1, path);
> +
> + if (!sub || !sub->name)
> + return -1;
> +
> + git_dir = resolve_gitdir(git_common_path("modules/%s", sub->name));
> +
> + if (!git_dir)
> + return -1;
> +
> + if (is_new)
> + connect_work_tree_and_git_dir(path, git_dir);
> +
> + /* update index via `read-tree --reset sha1` */
> + argv_array_pushl(&cp.args, "read-tree",
> + force ? "--reset" : "-m",
> + "-u", sha1_to_hex(oid->hash), NULL);
> + prepare_submodule_repo_env(&cp.env_array);
> + cp.git_cmd = 1;
> + cp.no_stdin = 1;
> + cp.dir = path;
> + if (run_command(&cp)) {
> + warning(_("reading the index in submodule '%s' failed"), path);
> + child_process_clear(&cp);
> + return -1;
> + }
> +
> + /* write index to working dir */
> + child_process_clear(&cp);
> + child_process_init(&cp);
> + argv_array_pushl(&cp.args, "checkout-index", "-a", NULL);
> + cp.git_cmd = 1;
> + cp.no_stdin = 1;
> + cp.dir = path;
> + if (force)
> + argv_array_push(&cp.args, "-f");
> +
> + if (run_command(&cp)) {
> + warning(_("populating the working directory in submodule '%s' failed"), path);
> + child_process_clear(&cp);
> + return -1;
> + }
> +
> + /* get the HEAD right */
> + child_process_clear(&cp);
> + child_process_init(&cp);
> + argv_array_pushl(&cp.args, "checkout", "--recurse-submodules", NULL);
> + cp.git_cmd = 1;
> + cp.no_stdin = 1;
> + cp.dir = path;
> + if (force)
> + argv_array_push(&cp.args, "-f");
> + argv_array_push(&cp.args, sha1_to_hex(oid->hash));
> +
> + if (run_command(&cp)) {
> + warning(_("setting the HEAD in submodule '%s' failed"), path);
> + child_process_clear(&cp);
> + return -1;
> + }
> +
> + child_process_clear(&cp);
> + return 0;
> +}
If run command is successful then it handles the clearing of the child
process struct, correct? Is there a negative to having all the explicit
clears when the child was successful?
> +
> int depopulate_submodule(const char *path)
> {
> int ret = 0;
> @@ -1336,3 +1405,51 @@ void prepare_submodule_repo_env(struct argv_array *out)
> }
> argv_array_push(out, "GIT_DIR=.git");
> }
> +
> +struct scheduled_submodules_update_type {
> + const char *path;
> + const struct object_id *oid;
> + /*
> + * Do we need to perform a complete checkout or just incremental
> + * update?
> + */
> + unsigned is_new:1;
> +} *scheduled_submodules;
> +#define SCHEDULED_SUBMODULES_INIT {NULL, NULL}
I may not know enough about these types of initializors but that Init
macro only has 2 entries while there are three entries in the struct
itself.
> +
> +int scheduled_submodules_nr, scheduled_submodules_alloc;
Should these globals be static since they should be scoped to only this
file?
> +
> +void schedule_submodule_for_update(const struct cache_entry *ce, int is_new)
> +{
> + struct scheduled_submodules_update_type *ssu;
> + ALLOC_GROW(scheduled_submodules,
> + scheduled_submodules_nr + 1,
> + scheduled_submodules_alloc);
> + ssu = &scheduled_submodules[scheduled_submodules_nr++];
> + ssu->path = ce->name;
> + ssu->oid = &ce->oid;
> + ssu->is_new = !!is_new;
> +}
> +
> +int update_submodules(int force)
> +{
> + int i;
> + gitmodules_config();
> +
> + /*
> + * NEEDSWORK: As submodule updates can potentially take some
> + * time each and they do not overlap (i.e. no d/f conflicts;
> + * this can be parallelized using the run_commands.h API.
> + */
> + for (i = 0; i < scheduled_submodules_nr; i++) {
> + struct scheduled_submodules_update_type *ssu =
> + &scheduled_submodules[i];
> +
> + if (submodule_is_interesting(ssu->path, null_sha1))
> + update_submodule(ssu->path, ssu->oid,
> + force, ssu->is_new);
> + }
> +
> + scheduled_submodules_nr = 0;
> + return 0;
> +}
nit: organization wise it makes more sense to me to have the
'update_submodule' helper function be located more closely to the
'update_submodules' function.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH v3 1/6] submodules: add helper functions to determine presence of submodules
From: Stefan Beller @ 2016-11-15 23:49 UTC (permalink / raw)
To: Brandon Williams; +Cc: git@vger.kernel.org, Jonathan Tan, Junio C Hamano
In-Reply-To: <1478908273-190166-2-git-send-email-bmwill@google.com>
On Fri, Nov 11, 2016 at 3:51 PM, Brandon Williams <bmwill@google.com> wrote:
> Add two helper functions to submodules.c.
> `is_submodule_initialized()` checks if a submodule has been initialized
> at a given path and `is_submodule_populated()` check if a submodule
> has been checked out at a given path.
This reminds me to write the documentation patch explaining the
concepts of submodules (specifically that overview page would state
all the possible states of submodules)
This patch looks good,
Stefan
> +
> + if (module) {
> + char *key = xstrfmt("submodule.%s.url", module->name);
> + char *value = NULL;
minor nit:
In case a reroll is needed, you could replace `value` by
`not_needed` or `unused` to make it easier to follow.
Hence it also doesn't need initialization (Doh, it does
for free() to work, nevermind).
^ permalink raw reply
* Re: [PATCH 08/16] update submodules: add depopulate_submodule
From: Brandon Williams @ 2016-11-15 23:44 UTC (permalink / raw)
To: Stefan Beller; +Cc: git, gitster, jrnieder, mogulguy10, David.Turner
In-Reply-To: <20161115230651.23953-9-sbeller@google.com>
On 11/15, Stefan Beller wrote:
> Implement the functionality needed to enable work tree manipulating
> commands to that a deleted submodule should not only affect the index
"to that a deleted" did you mean "so that a deleted"
> (leaving all the files of the submodule in the work tree) but also to
> remove the work tree of the superproject (including any untracked
> files).
>
> To do so, we need an equivalent of "rm -rf", which is already found in
> entry.c, so expose that and for clarity add a suffix "_or_dir" to it.
>
> That will only work properly when the submodule uses a gitfile instead of
> a .git directory and no untracked files are present. Otherwise the removal
> will fail with a warning (which is just what happened until now).
So if a submodule uses a .git directory then it will be ignored during
the checkout? All other submodules will actually be removed? Couldn't
you end up in an undesirable state with a checkout effecting one
submodule but not another?
> diff --git a/cache.h b/cache.h
> index a50a61a..65c47e4 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -2018,4 +2018,6 @@ void sleep_millisec(int millisec);
> */
> void safe_create_dir(const char *dir, int share);
>
> +void remove_subtree_or_die(const char *path);
> +
> #endif /* CACHE_H */
Should probably place an explicit 'extern' in the function prototype.
> +int depopulate_submodule(const char *path)
> +{
> + int ret = 0;
> + char *dot_git = xstrfmt("%s/.git", path);
> +
> + /* Is it populated? */
> + if (!resolve_gitdir(dot_git))
> + goto out;
> +
> + /* Does it have a .git directory? */
> + if (!submodule_uses_gitfile(path)) {
> + warning(_("cannot remove submodule '%s' because it (or one of "
> + "its nested submodules) uses a .git directory"),
> + path);
> + ret = -1;
> + goto out;
> + }
> +
> + remove_subtree_or_die(path);
> +
> +out:
> + free(dot_git);
> + return ret;
> +}
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH 07/16] update submodules: introduce submodule_is_interesting
From: Brandon Williams @ 2016-11-15 23:34 UTC (permalink / raw)
To: Stefan Beller; +Cc: git, gitster, jrnieder, mogulguy10, David.Turner
In-Reply-To: <20161115230651.23953-8-sbeller@google.com>
On 11/15, Stefan Beller wrote:
> +/**
> + * When updating the working tree, do we need to check if the submodule needs
> + * updating. We do not require a check if we are already sure that the
> + * submodule doesn't need updating, e.g. when we are not interested in submodules
> + * or the submodule is marked uninteresting by being not initialized.
> + */
The first sentence seems a bit awkward. It seems like its worded as a
question, maybe drop the 'do'?
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH 04/16] update submodules: add is_submodule_populated
From: Brandon Williams @ 2016-11-15 23:20 UTC (permalink / raw)
To: Stefan Beller; +Cc: git, gitster, jrnieder, mogulguy10, David.Turner
In-Reply-To: <20161115230651.23953-5-sbeller@google.com>
On 11/15, Stefan Beller wrote:
> This is nearly same as Brandon sent out.
> (First patch of origin/bw/grep-recurse-submodules,
> will drop this patch once Brandons series is stable
> enough to build on).
I would be thrilled to see more reviews on that series :D
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH v15 13/27] bisect--helper: `bisect_start` shell function partially in C
From: Stephan Beyer @ 2016-11-15 23:19 UTC (permalink / raw)
To: Pranit Bauva, git
In-Reply-To: <01020157c38b1ad3-ea75ed97-2514-427e-8e57-9f10efd4e6e9-000000@eu-west-1.amazonses.com>
Hi,
On 10/14/2016 04:14 PM, Pranit Bauva wrote:
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 6a5878c..1d3e17f 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -24,6 +27,8 @@ static const char * const git_bisect_helper_usage[] = {
> N_("git bisect--helper --bisect-check-and-set-terms <command> <TERM_GOOD> <TERM_BAD>"),
> N_("git bisect--helper --bisect-next-check [<term>] <TERM_GOOD> <TERM_BAD"),
> N_("git bisect--helper --bisect-terms [--term-good | --term-old | --term-bad | --term-new]"),
> + N_("git bisect--helper --bisect start [--term-{old,good}=<term> --term-{new,bad}=<term>]"
> + "[--no-checkout] [<bad> [<good>...]] [--] [<paths>...]"),
Typo: "--bisect start" with space instead of "-"
> @@ -403,6 +408,205 @@ static int bisect_terms(struct bisect_terms *terms, const char **argv, int argc)
> return 0;
> }
>
> +static int bisect_start(struct bisect_terms *terms, int no_checkout,
> + const char **argv, int argc)
> +{
> + int i, has_double_dash = 0, must_write_terms = 0, bad_seen = 0;
> + int flags, pathspec_pos, retval = 0;
> + struct string_list revs = STRING_LIST_INIT_DUP;
> + struct string_list states = STRING_LIST_INIT_DUP;
> + struct strbuf start_head = STRBUF_INIT;
> + struct strbuf bisect_names = STRBUF_INIT;
> + struct strbuf orig_args = STRBUF_INIT;
> + const char *head;
> + unsigned char sha1[20];
> + FILE *fp = NULL;
> + struct object_id oid;
> +
> + if (is_bare_repository())
> + no_checkout = 1;
> +
> + for (i = 0; i < argc; i++) {
> + if (!strcmp(argv[i], "--")) {
> + has_double_dash = 1;
> + break;
> + }
> + }
> +
> + for (i = 0; i < argc; i++) {
> + const char *commit_id = xstrfmt("%s^{commit}", argv[i]);
> + const char *arg = argv[i];
> + if (!strcmp(argv[i], "--")) {
> + has_double_dash = 1;
This is without effect since has_double_dash is already set to 1 by the
loop above. I think you can remove this line.
> + break;
> + } else if (!strcmp(arg, "--no-checkout")) {
> + no_checkout = 1;
> + } else if (!strcmp(arg, "--term-good") ||
> + !strcmp(arg, "--term-old")) {
> + must_write_terms = 1;
> + terms->term_good = xstrdup(argv[++i]);
> + } else if (skip_prefix(arg, "--term-good=", &arg)) {
> + must_write_terms = 1;
> + terms->term_good = xstrdup(arg);
> + } else if (skip_prefix(arg, "--term-old=", &arg)) {
> + must_write_terms = 1;
> + terms->term_good = xstrdup(arg);
I think you can join the last two branches:
+ } else if (skip_prefix(arg, "--term-good=", &arg) ||
+ skip_prefix(arg, "--term-old=", &arg)) {
+ must_write_terms = 1;
+ terms->term_good = xstrdup(arg);
> + } else if (!strcmp(arg, "--term-bad") ||
> + !strcmp(arg, "--term-new")) {
> + must_write_terms = 1;
> + terms->term_bad = xstrdup(argv[++i]);
> + } else if (skip_prefix(arg, "--term-bad=", &arg)) {
> + must_write_terms = 1;
> + terms->term_bad = xstrdup(arg);
> + } else if (skip_prefix(arg, "--term-new=", &arg)) {
> + must_write_terms = 1;
> + terms->term_good = xstrdup(arg);
This has to be terms->term_bad = ...
Also, you can join the last two branches, again, ie,
+ } else if (skip_prefix(arg, "--term-bad=", &arg) ||
+ skip_prefix(arg, "--term-new=", &arg)) {
+ must_write_terms = 1;
+ terms->term_bad = xstrdup(arg);
> + } else if (starts_with(arg, "--") &&
> + !one_of(arg, "--term-good", "--term-bad", NULL)) {
> + die(_("unrecognised option: '%s'"), arg);
[...]
> + /*
> + * Verify HEAD
> + */
> + head = resolve_ref_unsafe("HEAD", 0, sha1, &flags);
> + if (!head)
> + if (get_sha1("HEAD", sha1))
> + die(_("Bad HEAD - I need a HEAD"));
> +
> + if (!is_empty_or_missing_file(git_path_bisect_start())) {
You were so eager to re-use the comments from the shell script, but you
forgot the "Check if we are bisecting." comment above this line ;-)
> + /* Reset to the rev from where we started */
> + strbuf_read_file(&start_head, git_path_bisect_start(), 0);
> + strbuf_trim(&start_head);
> + if (!no_checkout) {
> + struct argv_array argv = ARGV_ARRAY_INIT;
[...]
> + if (must_write_terms)
> + if (write_terms(terms->term_bad, terms->term_good)) {
> + retval = -1;
> + goto finish;
> + }
> +
bisect_start() is a pretty big function.
I think it can easily be decomposed in some smaller parts, for example,
the following lines ...
> + fp = fopen(git_path_bisect_log(), "a");
> + if (!fp)
> + return -1;
> +
> + if (fprintf(fp, "git bisect start") < 1) {
> + retval = -1;
> + goto finish;
> + }
> +
> + sq_quote_argv(&orig_args, argv, 0);
> + if (fprintf(fp, "%s", orig_args.buf) < 0) {
> + retval = -1;
> + goto finish;
> + }
> + if (fprintf(fp, "\n") < 1) {
> + retval = -1;
> + goto finish;
> + }
... could be in a function like
static int bisect_append_log(const char **argv)
{
FILE *fp = fopen(git_path_bisect_log(), "a");
struct strbuf orig_args = STRBUF_INIT;
if (!fp)
return -1;
if (fprintf(fp, "git bisect start") < 1) {
retval = -1;
goto finish;
}
sq_quote_argv(&orig_args, argv, 0);
if (fprintf(fp, "%s", orig_args.buf) < 0 ||
fprintf(fp, "\n") < 1) {
retval = -1;
goto finish;
}
finish:
if (fp)
fclose(fp);
strbuf_release(&orig_args);
return retval;
}
and then simply call
retval = bisect_append_log(argv);
in bisect_start()... (This is totally untested.)
If you do not want that for some reason, you should at least fix
> + if (!fp)
> + return -1;
to retval = 1; goto finish; such that the other lists and strings are
released.
> + goto finish;
> +finish:
The "goto finish" right above the "finish" label is unnecessary.
> + if (fp)
> + fclose(fp);
> + string_list_clear(&revs, 0);
> + string_list_clear(&states, 0);
> + strbuf_release(&start_head);
> + strbuf_release(&bisect_names);
> + strbuf_release(&orig_args);
> + return retval;
> +}
> +
> int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> {
> enum {
By the way, there are two spaghetti-ish ways to get rid of the
retval = -1;
goto finish;
line pair:
goto fail;
and below the "return retval;" add
fail:
retval = -1;
goto finish;
and you can feel the touch of His Noodly Appendage. *scnr*
The other way is to keep the "goto finish" I deemed unnecessary (right
above the label), and expand it to:
goto finish;
fail:
retval = -1;
finish:
...
Cheers
Stephan
^ permalink raw reply
* [PATCH 15/16] completion: add '--recurse-submodules' to checkout
From: Stefan Beller @ 2016-11-15 23:06 UTC (permalink / raw)
Cc: git, bmwill, gitster, jrnieder, mogulguy10, David.Turner,
Stefan Beller
In-Reply-To: <20161115230651.23953-1-sbeller@google.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
contrib/completion/git-completion.bash | 2 +-
t/t9902-completion.sh | 1 +
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 21016bf..28acfdb 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1068,7 +1068,7 @@ _git_checkout ()
--*)
__gitcomp "
--quiet --ours --theirs --track --no-track --merge
- --conflict= --orphan --patch
+ --conflict= --orphan --patch --recurse-submodules
"
;;
*)
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 2ba62fb..d2d1102 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -447,6 +447,7 @@ test_expect_success 'double dash "git checkout"' '
--conflict=
--orphan Z
--patch Z
+ --recurse-submodules Z
EOF
'
--
2.10.1.469.g00a8914
^ permalink raw reply related
* [PATCH 05/16] update submodules: add submodule config parsing
From: Stefan Beller @ 2016-11-15 23:06 UTC (permalink / raw)
Cc: git, bmwill, gitster, jrnieder, mogulguy10, David.Turner,
Stefan Beller
In-Reply-To: <20161115230651.23953-1-sbeller@google.com>
Similar as in b33a15b08 (push: add recurseSubmodules config option,
2015-11-17) and 027771fcb1 (submodule: allow erroneous values for the
fetchRecurseSubmodules option, 2015-08-17), we add submodule-config code
that is later used to parse whether we are interested in updating
submodules.
We need the `die_on_error` parameter to be able to call this parsing
function for the config file as well, which if incorrect let's Git die.
As we're just touching the header file, also mark all functions extern.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
submodule-config.c | 22 ++++++++++++++++++++++
submodule-config.h | 17 +++++++++--------
2 files changed, 31 insertions(+), 8 deletions(-)
diff --git a/submodule-config.c b/submodule-config.c
index 098085b..4b5297e 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -234,6 +234,28 @@ int parse_fetch_recurse_submodules_arg(const char *opt, const char *arg)
return parse_fetch_recurse(opt, arg, 1);
}
+static int parse_update_recurse(const char *opt, const char *arg,
+ int die_on_error)
+{
+ switch (git_config_maybe_bool(opt, arg)) {
+ case 1:
+ return RECURSE_SUBMODULES_ON;
+ case 0:
+ return RECURSE_SUBMODULES_OFF;
+ default:
+ if (!strcmp(arg, "checkout"))
+ return RECURSE_SUBMODULES_ON;
+ if (die_on_error)
+ die("bad %s argument: %s", opt, arg);
+ return RECURSE_SUBMODULES_ERROR;
+ }
+}
+
+int parse_update_recurse_submodules_arg(const char *opt, const char *arg)
+{
+ return parse_update_recurse(opt, arg, 1);
+}
+
static int parse_push_recurse(const char *opt, const char *arg,
int die_on_error)
{
diff --git a/submodule-config.h b/submodule-config.h
index d05c542..992bfbe 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -22,13 +22,14 @@ struct submodule {
int recommend_shallow;
};
-int parse_fetch_recurse_submodules_arg(const char *opt, const char *arg);
-int parse_push_recurse_submodules_arg(const char *opt, const char *arg);
-int parse_submodule_config_option(const char *var, const char *value);
-const struct submodule *submodule_from_name(const unsigned char *commit_sha1,
- const char *name);
-const struct submodule *submodule_from_path(const unsigned char *commit_sha1,
- const char *path);
-void submodule_free(void);
+extern int parse_fetch_recurse_submodules_arg(const char *opt, const char *arg);
+extern int parse_update_recurse_submodules_arg(const char *opt, const char *arg);
+extern int parse_push_recurse_submodules_arg(const char *opt, const char *arg);
+extern int parse_submodule_config_option(const char *var, const char *value);
+extern const struct submodule *submodule_from_name(
+ const unsigned char *commit_sha1, const char *name);
+extern const struct submodule *submodule_from_path(
+ const unsigned char *commit_sha1, const char *path);
+extern void submodule_free(void);
#endif /* SUBMODULE_CONFIG_H */
--
2.10.1.469.g00a8914
^ permalink raw reply related
* [PATCH 10/16] update submodules: is_submodule_checkout_safe
From: Stefan Beller @ 2016-11-15 23:06 UTC (permalink / raw)
Cc: git, bmwill, gitster, jrnieder, mogulguy10, David.Turner,
Stefan Beller
In-Reply-To: <20161115230651.23953-1-sbeller@google.com>
In later patches we introduce the options and flag for commands
that modify the working directory, e.g. git-checkout.
This piece of code will answer the question:
"Is it safe to change the submodule to this new state?"
e.g. is it overwriting untracked files or are there local
changes that would be overwritten?
Signed-off-by: Stefan Beller <sbeller@google.com>
---
submodule.c | 22 ++++++++++++++++++++++
submodule.h | 2 ++
2 files changed, 24 insertions(+)
diff --git a/submodule.c b/submodule.c
index 2773151..2149ef7 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1201,6 +1201,28 @@ int ok_to_remove_submodule(const char *path)
return ok_to_remove;
}
+int is_submodule_checkout_safe(const char *path, const struct object_id *oid)
+{
+ struct child_process cp = CHILD_PROCESS_INIT;
+
+ if (!is_submodule_populated(path))
+ /* The submodule is not populated, it's safe to check it out */
+ /*
+ * TODO: When git learns to re-populate submodules, a check must be
+ * added here to assert that no local files will be overwritten.
+ */
+ return 1;
+
+ argv_array_pushl(&cp.args, "read-tree", "-n", "-m", "HEAD",
+ sha1_to_hex(oid->hash), NULL);
+
+ prepare_submodule_repo_env(&cp.env_array);
+ cp.git_cmd = 1;
+ cp.no_stdin = 1;
+ cp.dir = path;
+ return run_command(&cp) == 0;
+}
+
static int find_first_merges(struct object_array *result, const char *path,
struct commit *a, struct commit *b)
{
diff --git a/submodule.h b/submodule.h
index f01f87c..a7fa634 100644
--- a/submodule.h
+++ b/submodule.h
@@ -74,6 +74,8 @@ extern unsigned is_submodule_modified(const char *path, int ignore_untracked);
extern int is_submodule_populated(const char *path);
extern int submodule_uses_gitfile(const char *path);
extern int ok_to_remove_submodule(const char *path);
+extern int is_submodule_checkout_safe(const char *path,
+ const struct object_id *oid);
extern int merge_submodule(unsigned char result[20], const char *path,
const unsigned char base[20],
const unsigned char a[20],
--
2.10.1.469.g00a8914
^ permalink raw reply related
* [PATCH 02/16] submodule: modernize ok_to_remove_submodule to use argv_array
From: Stefan Beller @ 2016-11-15 23:06 UTC (permalink / raw)
Cc: git, bmwill, gitster, jrnieder, mogulguy10, David.Turner,
Stefan Beller
In-Reply-To: <20161115230651.23953-1-sbeller@google.com>
Instead of constructing the NULL terminated array ourselves, we
should make use of the argv_array infrastructure.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
submodule.c | 10 ++--------
1 file changed, 2 insertions(+), 8 deletions(-)
diff --git a/submodule.c b/submodule.c
index 6f7d883..53a6dbb 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1022,13 +1022,6 @@ int ok_to_remove_submodule(const char *path)
{
ssize_t len;
struct child_process cp = CHILD_PROCESS_INIT;
- const char *argv[] = {
- "status",
- "--porcelain",
- "-u",
- "--ignore-submodules=none",
- NULL,
- };
struct strbuf buf = STRBUF_INIT;
int ok_to_remove = 1;
@@ -1038,7 +1031,8 @@ int ok_to_remove_submodule(const char *path)
if (!submodule_uses_gitfile(path))
return 0;
- cp.argv = argv;
+ argv_array_pushl(&cp.args, "status", "--porcelain", "-uall",
+ "--ignore-submodules=none", NULL);
prepare_submodule_repo_env(&cp.env_array);
cp.git_cmd = 1;
cp.no_stdin = 1;
--
2.10.1.469.g00a8914
^ permalink raw reply related
* [PATCH 04/16] update submodules: add is_submodule_populated
From: Stefan Beller @ 2016-11-15 23:06 UTC (permalink / raw)
Cc: git, bmwill, gitster, jrnieder, mogulguy10, David.Turner,
Stefan Beller
In-Reply-To: <20161115230651.23953-1-sbeller@google.com>
This is nearly same as Brandon sent out.
(First patch of origin/bw/grep-recurse-submodules,
will drop this patch once Brandons series is stable
enough to build on).
Signed-off-by: Stefan Beller <sbeller@google.com>
---
submodule.c | 11 +++++++++++
submodule.h | 1 +
2 files changed, 12 insertions(+)
diff --git a/submodule.c b/submodule.c
index c9d22e5..97eaf7c 100644
--- a/submodule.c
+++ b/submodule.c
@@ -914,6 +914,17 @@ int fetch_populated_submodules(const struct argv_array *options,
return spf.result;
}
+int is_submodule_populated(const char *path)
+{
+ int retval = 0;
+ struct strbuf gitdir = STRBUF_INIT;
+ strbuf_addf(&gitdir, "%s/.git", path);
+ if (resolve_gitdir(gitdir.buf))
+ retval = 1;
+ strbuf_release(&gitdir);
+ return retval;
+}
+
unsigned is_submodule_modified(const char *path, int ignore_untracked)
{
ssize_t len;
diff --git a/submodule.h b/submodule.h
index afc58d0..d44b4f1 100644
--- a/submodule.h
+++ b/submodule.h
@@ -61,6 +61,7 @@ extern int fetch_populated_submodules(const struct argv_array *options,
const char *prefix, int command_line_option,
int quiet, int max_parallel_jobs);
extern unsigned is_submodule_modified(const char *path, int ignore_untracked);
+extern int is_submodule_populated(const char *path);
extern int submodule_uses_gitfile(const char *path);
extern int ok_to_remove_submodule(const char *path);
extern int merge_submodule(unsigned char result[20], const char *path,
--
2.10.1.469.g00a8914
^ permalink raw reply related
* [PATCH 03/16] submodule: use absolute path for computing relative path connecting
From: Stefan Beller @ 2016-11-15 23:06 UTC (permalink / raw)
Cc: git, bmwill, gitster, jrnieder, mogulguy10, David.Turner,
Stefan Beller
In-Reply-To: <20161115230651.23953-1-sbeller@google.com>
This addresses a similar concern as in f8eaa0ba98b (submodule--helper,
module_clone: always operate on absolute paths, 2016-03-31)
When computing the relative path from one to another location, we
need to provide both locations as absolute paths to make sure the
computation of the relative path is correct.
While at it, change `real_work_tree` to be non const as we own
the memory.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
submodule.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/submodule.c b/submodule.c
index 53a6dbb..c9d22e5 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1221,23 +1221,25 @@ void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir)
{
struct strbuf file_name = STRBUF_INIT;
struct strbuf rel_path = STRBUF_INIT;
- const char *real_work_tree = xstrdup(real_path(work_tree));
+ char *real_git_dir = xstrdup(real_path(git_dir));
+ char *real_work_tree = xstrdup(real_path(work_tree));
/* Update gitfile */
strbuf_addf(&file_name, "%s/.git", work_tree);
write_file(file_name.buf, "gitdir: %s",
- relative_path(git_dir, real_work_tree, &rel_path));
+ relative_path(real_git_dir, real_work_tree, &rel_path));
/* Update core.worktree setting */
strbuf_reset(&file_name);
- strbuf_addf(&file_name, "%s/config", git_dir);
+ strbuf_addf(&file_name, "%s/config", real_git_dir);
git_config_set_in_file(file_name.buf, "core.worktree",
- relative_path(real_work_tree, git_dir,
+ relative_path(real_work_tree, real_git_dir,
&rel_path));
strbuf_release(&file_name);
strbuf_release(&rel_path);
- free((void *)real_work_tree);
+ free(real_work_tree);
+ free(real_git_dir);
}
int parallel_submodules(void)
--
2.10.1.469.g00a8914
^ permalink raw reply related
* [PATCH 07/16] update submodules: introduce submodule_is_interesting
From: Stefan Beller @ 2016-11-15 23:06 UTC (permalink / raw)
Cc: git, bmwill, gitster, jrnieder, mogulguy10, David.Turner,
Stefan Beller
In-Reply-To: <20161115230651.23953-1-sbeller@google.com>
In later patches we introduce the --recurse-submodule flag for commands
that modify the working directory, e.g. git-checkout.
It is potentially expensive to check if a submodule needs an update,
because a common theme to interact with submodules is to spawn a child
process for each interaction.
So let's introduce a function that pre checks if a submodule needs
to be checked for an update.
I am not particular happy with the name `submodule_is_interesting`,
in internal iterations I had `submodule_requires_check_for_update`
and `submodule_needs_update`, but I was even less happy with those
names. Maybe `submodule_interesting_for_update`?
Generally this is to answer "Am I allowed to touch the submodule
at all?" or: "Does the user expect me to touch it?"
which includes all of creation/deletion/update.
This patch is based off a prior attempt by Jens Lehmann to add
submodules to checkout.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
submodule.c | 37 +++++++++++++++++++++++++++++++++++++
submodule.h | 8 ++++++++
2 files changed, 45 insertions(+)
diff --git a/submodule.c b/submodule.c
index 38b0573..d34b721 100644
--- a/submodule.c
+++ b/submodule.c
@@ -500,6 +500,43 @@ void set_config_update_recurse_submodules(int value)
config_update_recurse_submodules = value;
}
+int submodules_interesting_for_update(void)
+{
+ /*
+ * Update can't be "none", "merge" or "rebase",
+ * treat any value as OFF, except an explicit ON.
+ */
+ return config_update_recurse_submodules == RECURSE_SUBMODULES_ON;
+}
+
+int submodule_is_interesting(const char *path, const unsigned char *sha1)
+{
+ /*
+ * If we cannot load a submodule config, we cannot get the name
+ * of the submodule, so we'd need to follow the gitlink file
+ */
+ const struct submodule *sub;
+
+ if (!submodules_interesting_for_update())
+ return 0;
+
+ sub = submodule_from_path(sha1, path);
+ if (!sub)
+ return 0;
+
+ switch (sub->update_strategy.type) {
+ case SM_UPDATE_UNSPECIFIED:
+ case SM_UPDATE_CHECKOUT:
+ return 1;
+ case SM_UPDATE_REBASE:
+ case SM_UPDATE_MERGE:
+ case SM_UPDATE_NONE:
+ case SM_UPDATE_COMMAND:
+ return 0;
+ }
+ return 0;
+}
+
static int has_remote(const char *refname, const struct object_id *oid,
int flags, void *cb_data)
{
diff --git a/submodule.h b/submodule.h
index 185ad18..3df6881 100644
--- a/submodule.h
+++ b/submodule.h
@@ -57,6 +57,14 @@ extern void show_submodule_inline_diff(FILE *f, const char *path,
const struct diff_options *opt);
extern void set_config_fetch_recurse_submodules(int value);
extern void set_config_update_recurse_submodules(int value);
+/**
+ * When updating the working tree, do we need to check if the submodule needs
+ * updating. We do not require a check if we are already sure that the
+ * submodule doesn't need updating, e.g. when we are not interested in submodules
+ * or the submodule is marked uninteresting by being not initialized.
+ */
+extern int submodule_is_interesting(const char *path, const unsigned char *sha1);
+extern int submodules_interesting_for_update(void);
extern void check_for_new_submodule_commits(unsigned char new_sha1[20]);
extern int fetch_populated_submodules(const struct argv_array *options,
const char *prefix, int command_line_option,
--
2.10.1.469.g00a8914
^ permalink raw reply related
* [PATCH 16/16] checkout: add config option to recurse into submodules by default
From: Stefan Beller @ 2016-11-15 23:06 UTC (permalink / raw)
Cc: git, bmwill, gitster, jrnieder, mogulguy10, David.Turner,
Stefan Beller
In-Reply-To: <20161115230651.23953-1-sbeller@google.com>
To make it easier for the user, who doesn't want to give the
`--recurse-submodules` option whenever they run checkout, have an
option for to set the default behavior for checkout to recurse into
submodules.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
Documentation/config.txt | 6 ++++++
Documentation/git-checkout.txt | 5 +++--
submodule.c | 6 +++---
t/t2013-checkout-submodule.sh | 19 +++++++++++++++++++
4 files changed, 31 insertions(+), 5 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index a0ab66a..67e0714 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -949,6 +949,12 @@ clean.requireForce::
A boolean to make git-clean do nothing unless given -f,
-i or -n. Defaults to true.
+checkout.recurseSubmodules::
+ This option can be set to a boolean value.
+ When set to true checkout will recurse into submodules and
+ update them. When set to false, which is the default, checkout
+ will leave submodules untouched.
+
color.branch::
A boolean to enable/disable color in the output of
linkgit:git-branch[1]. May be set to `always`,
diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
index a0ea2c5..819c430 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt
@@ -260,8 +260,9 @@ section of linkgit:git-add[1] to learn how to operate the `--patch` mode.
Using --recurse-submodules will update the content of all initialized
submodules according to the commit recorded in the superproject. If
local modifications in a submodule would be overwritten the checkout
- will fail until `-f` is used. If nothing (or --no-recurse-submodules)
- is used, the work trees of submodules will not be updated.
+ will fail until `-f` is used. If `--no-recurse-submodules` is used,
+ the work trees of submodules will not be updated. If no command line
+ argument is given, `checkout.recurseSubmodules` is used as a default.
<branch>::
Branch to checkout; if it refers to a branch (i.e., a name that,
diff --git a/submodule.c b/submodule.c
index 2149ef7..0c807d9 100644
--- a/submodule.c
+++ b/submodule.c
@@ -160,10 +160,10 @@ int submodule_config(const char *var, const char *value, void *cb)
return 0;
} else if (starts_with(var, "submodule."))
return parse_submodule_config_option(var, value);
- else if (!strcmp(var, "fetch.recursesubmodules")) {
+ else if (!strcmp(var, "fetch.recursesubmodules"))
config_fetch_recurse_submodules = parse_fetch_recurse_submodules_arg(var, value);
- return 0;
- }
+ else if (!strcmp(var, "checkout.recursesubmodules"))
+ config_update_recurse_submodules = parse_update_recurse_submodules_arg(var, value);
return 0;
}
diff --git a/t/t2013-checkout-submodule.sh b/t/t2013-checkout-submodule.sh
index 60f6987..788c59d 100755
--- a/t/t2013-checkout-submodule.sh
+++ b/t/t2013-checkout-submodule.sh
@@ -149,6 +149,25 @@ test_expect_success '"checkout --recurse-submodules" repopulates submodule' '
)
'
+test_expect_success 'option checkout.recurseSubmodules updates submodule' '
+ test_config -C super checkout.recurseSubmodules 1 &&
+ (
+ cd super &&
+ git checkout base &&
+ git checkout -b advanced-base &&
+ git -C submodule commit --allow-empty -m "empty commit" &&
+ git add submodule &&
+ git commit -m "advance submodule" &&
+ git checkout base &&
+ git diff-files --quiet &&
+ git diff-index --quiet --cached base &&
+ git checkout advanced-base &&
+ git diff-files --quiet &&
+ git diff-index --quiet --cached advanced-base &&
+ git checkout --recurse-submodules base
+ )
+'
+
test_expect_success '"checkout --recurse-submodules" repopulates submodule in existing directory' '
(
cd super &&
--
2.10.1.469.g00a8914
^ permalink raw reply related
* RE: [PATCH 02/16] submodule: modernize ok_to_remove_submodule to use argv_array
From: David Turner @ 2016-11-15 23:11 UTC (permalink / raw)
To: 'Stefan Beller'
Cc: git@vger.kernel.org, bmwill@google.com, gitster@pobox.com,
jrnieder@gmail.com, mogulguy10@gmail.com
In-Reply-To: <20161115230651.23953-3-sbeller@google.com>
> - "-u",
...
> + argv_array_pushl(&cp.args, "status", "--porcelain", "-uall",
This also changes -u to -uall, which is not mentioned in the commit message. That should probably be called out.
^ permalink raw reply
* [PATCH 13/16] submodule: teach unpack_trees() to update submodules
From: Stefan Beller @ 2016-11-15 23:06 UTC (permalink / raw)
Cc: git, bmwill, gitster, jrnieder, mogulguy10, David.Turner,
Stefan Beller
In-Reply-To: <20161115230651.23953-1-sbeller@google.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
entry.c | 7 +++--
unpack-trees.c | 97 ++++++++++++++++++++++++++++++++++++++++++++++++----------
unpack-trees.h | 1 +
3 files changed, 86 insertions(+), 19 deletions(-)
diff --git a/entry.c b/entry.c
index 2330b6e..dd829ec 100644
--- a/entry.c
+++ b/entry.c
@@ -270,7 +270,7 @@ int checkout_entry(struct cache_entry *ce,
if (!check_path(path.buf, path.len, &st, state->base_dir_len)) {
unsigned changed = ce_match_stat(ce, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE);
- if (!changed)
+ if (!changed && (!S_ISDIR(st.st_mode) || !S_ISGITLINK(ce->ce_mode)))
return 0;
if (!state->force) {
if (!state->quiet)
@@ -287,9 +287,10 @@ int checkout_entry(struct cache_entry *ce,
* just do the right thing)
*/
if (S_ISDIR(st.st_mode)) {
- /* If it is a gitlink, leave it alone! */
- if (S_ISGITLINK(ce->ce_mode))
+ if (S_ISGITLINK(ce->ce_mode)) {
+ schedule_submodule_for_update(ce, 1);
return 0;
+ }
if (!state->force)
return error("%s is a directory", path.buf);
remove_subtree(&path);
diff --git a/unpack-trees.c b/unpack-trees.c
index 576e1d5..c5c22ed 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -29,6 +29,9 @@ static const char *unpack_plumbing_errors[NB_UNPACK_TREES_ERROR_TYPES] = {
/* ERROR_NOT_UPTODATE_DIR */
"Updating '%s' would lose untracked files in it",
+ /* ERROR_NOT_UPTODATE_SUBMODULE */
+ "Updating submodule '%s' would lose modifications in it",
+
/* ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN */
"Untracked working tree file '%s' would be overwritten by merge.",
@@ -80,6 +83,8 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
msgs[ERROR_NOT_UPTODATE_DIR] =
_("Updating the following directories would lose untracked files in it:\n%s");
+ msgs[ERROR_NOT_UPTODATE_SUBMODULE] =
+ _("Updating the following submodules would lose modifications in it:\n%s");
if (!strcmp(cmd, "checkout"))
msg = advice_commit_before_merge
@@ -1315,19 +1320,18 @@ static int verify_uptodate_1(const struct cache_entry *ce,
return 0;
if (!lstat(ce->name, &st)) {
- int flags = CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE;
- unsigned changed = ie_match_stat(o->src_index, ce, &st, flags);
- if (!changed)
- return 0;
- /*
- * NEEDSWORK: the current default policy is to allow
- * submodule to be out of sync wrt the superproject
- * index. This needs to be tightened later for
- * submodules that are marked to be automatically
- * checked out.
- */
- if (S_ISGITLINK(ce->ce_mode))
- return 0;
+ if (!S_ISGITLINK(ce->ce_mode)) {
+ int flags = CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE;
+ unsigned changed = ie_match_stat(o->src_index, ce, &st, flags);
+ if (!changed)
+ return 0;
+ } else {
+ if (!submodule_is_interesting(ce->name, null_sha1))
+ return 0;
+ if (ce_stage(ce) ? is_submodule_checkout_safe(ce->name, &ce->oid)
+ : !is_submodule_modified(ce->name, 1))
+ return 0;
+ }
errno = 0;
}
if (errno == ENOENT)
@@ -1350,6 +1354,59 @@ static int verify_uptodate_sparse(const struct cache_entry *ce,
return verify_uptodate_1(ce, o, ERROR_SPARSE_NOT_UPTODATE_FILE);
}
+/*
+ * When a submodule gets turned into an unmerged entry, we want it to be
+ * up-to-date regarding the merge changes.
+ */
+static int verify_uptodate_submodule(const struct cache_entry *old,
+ const struct cache_entry *new,
+ struct unpack_trees_options *o)
+{
+ struct stat st;
+
+ if (o->index_only
+ || (!((old->ce_flags & CE_VALID) || ce_skip_worktree(old))
+ && (o->reset || ce_uptodate(old))))
+ return 0;
+ if (!submodule_is_interesting(new->name, null_sha1))
+ return 0;
+ if (!lstat(old->name, &st)) {
+ unsigned changed = ie_match_stat(o->src_index, old, &st,
+ CE_MATCH_IGNORE_VALID|
+ CE_MATCH_IGNORE_SKIP_WORKTREE);
+ if (!changed) {
+ /* old is always a submodule */
+ if (S_ISGITLINK(new->ce_mode)) {
+ /*
+ * new is also a submodule, so check if we care
+ * and then if can checkout the new sha1 safely
+ */
+ if (submodule_is_interesting(old->name, null_sha1)
+ && is_submodule_checkout_safe(new->name, &new->oid))
+ return 0;
+ } else {
+ /*
+ * new is not a submodule any more, so only
+ * care if we care:
+ */
+ if (submodule_is_interesting(old->name, null_sha1)
+ && ok_to_remove_submodule(old->name))
+ return 0;
+ }
+ } else {
+ if (S_ISGITLINK(new->ce_mode))
+ return !is_submodule_checkout_safe(new->name, &new->oid);
+ else
+ return !ok_to_remove_submodule(old->name);
+ }
+ errno = 0;
+ }
+ if (errno == ENOENT)
+ return 0;
+ return o->gently ? -1 :
+ add_rejected_path(o, ERROR_NOT_UPTODATE_SUBMODULE, old->name);
+}
+
static void invalidate_ce_path(const struct cache_entry *ce,
struct unpack_trees_options *o)
{
@@ -1608,9 +1665,17 @@ static int merged_entry(const struct cache_entry *ce,
copy_cache_entry(merge, old);
update = 0;
} else {
- if (verify_uptodate(old, o)) {
- free(merge);
- return -1;
+ if (S_ISGITLINK(old->ce_mode) ||
+ S_ISGITLINK(merge->ce_mode)) {
+ if (verify_uptodate_submodule(old, merge, o)) {
+ free(merge);
+ return -1;
+ }
+ } else {
+ if (verify_uptodate(old, o)) {
+ free(merge);
+ return -1;
+ }
}
/* Migrate old flags over */
update |= old->ce_flags & (CE_SKIP_WORKTREE | CE_NEW_SKIP_WORKTREE);
diff --git a/unpack-trees.h b/unpack-trees.h
index 36a73a6..bee8740 100644
--- a/unpack-trees.h
+++ b/unpack-trees.h
@@ -15,6 +15,7 @@ enum unpack_trees_error_types {
ERROR_WOULD_OVERWRITE = 0,
ERROR_NOT_UPTODATE_FILE,
ERROR_NOT_UPTODATE_DIR,
+ ERROR_NOT_UPTODATE_SUBMODULE,
ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN,
ERROR_WOULD_LOSE_UNTRACKED_REMOVED,
ERROR_BIND_OVERLAP,
--
2.10.1.469.g00a8914
^ permalink raw reply related
* [PATCH 14/16] checkout: recurse into submodules if asked to
From: Stefan Beller @ 2016-11-15 23:06 UTC (permalink / raw)
Cc: git, bmwill, gitster, jrnieder, mogulguy10, David.Turner,
Stefan Beller
In-Reply-To: <20161115230651.23953-1-sbeller@google.com>
Allow checkout to recurse into submodules via
the command line option --[no-]recurse-submodules.
The flag for recurse-submodules in its current form
could be an OPT_BOOL, but eventually we may want to have
it as:
git checkout --recurse-submodules=rebase|merge| \
cherry-pick|checkout|none
which resembles the submodule.<name>.update options,
so naturally a value such as
"as-configured-or-X-as-fallback" would also come in handy.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
Documentation/git-checkout.txt | 7 +
builtin/checkout.c | 31 +++-
t/lib-submodule-update.sh | 10 +-
t/t2013-checkout-submodule.sh | 325 ++++++++++++++++++++++++++++++++++++++++-
4 files changed, 362 insertions(+), 11 deletions(-)
diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
index 8e2c066..a0ea2c5 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt
@@ -256,6 +256,13 @@ section of linkgit:git-add[1] to learn how to operate the `--patch` mode.
out anyway. In other words, the ref can be held by more than one
worktree.
+--[no-]recurse-submodules::
+ Using --recurse-submodules will update the content of all initialized
+ submodules according to the commit recorded in the superproject. If
+ local modifications in a submodule would be overwritten the checkout
+ will fail until `-f` is used. If nothing (or --no-recurse-submodules)
+ is used, the work trees of submodules will not be updated.
+
<branch>::
Branch to checkout; if it refers to a branch (i.e., a name that,
when prepended with "refs/heads/", is a valid ref), then that
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 9b2a5b3..2a626a3 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -21,12 +21,31 @@
#include "submodule-config.h"
#include "submodule.h"
+static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
+
static const char * const checkout_usage[] = {
N_("git checkout [<options>] <branch>"),
N_("git checkout [<options>] [<branch>] -- <file>..."),
NULL,
};
+int option_parse_recurse_submodules(const struct option *opt,
+ const char *arg, int unset)
+{
+ if (unset) {
+ recurse_submodules = RECURSE_SUBMODULES_OFF;
+ return 0;
+ }
+ if (arg)
+ recurse_submodules =
+ parse_update_recurse_submodules_arg(opt->long_name,
+ arg);
+ else
+ recurse_submodules = RECURSE_SUBMODULES_ON;
+
+ return 0;
+}
+
struct checkout_opts {
int patch_mode;
int quiet;
@@ -826,7 +845,8 @@ static int switch_branches(const struct checkout_opts *opts,
parse_commit_or_die(new->commit);
}
- ret = merge_working_tree(opts, &old, new, &writeout_error);
+ ret = merge_working_tree(opts, &old, new, &writeout_error) ||
+ update_submodules(opts->force);
if (ret) {
free(path_to_free);
return ret;
@@ -1160,6 +1180,9 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
N_("second guess 'git checkout <no-such-branch>'")),
OPT_BOOL(0, "ignore-other-worktrees", &opts.ignore_other_worktrees,
N_("do not check if another worktree is holding the given ref")),
+ { OPTION_CALLBACK, 0, "recurse-submodules", &recurse_submodules,
+ "checkout", "control recursive updating of submodules",
+ PARSE_OPT_OPTARG, option_parse_recurse_submodules },
OPT_BOOL(0, "progress", &opts.show_progress, N_("force progress reporting")),
OPT_END(),
};
@@ -1190,6 +1213,12 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
git_xmerge_config("merge.conflictstyle", conflict_style, NULL);
}
+ if (recurse_submodules != RECURSE_SUBMODULES_OFF) {
+ git_config(submodule_config, NULL);
+ if (recurse_submodules != RECURSE_SUBMODULES_DEFAULT)
+ set_config_update_recurse_submodules(recurse_submodules);
+ }
+
if ((!!opts.new_branch + !!opts.new_branch_force + !!opts.new_orphan_branch) > 1)
die(_("-b, -B and --orphan are mutually exclusive"));
diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
index 79cdd34..e0773c6 100755
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -634,7 +634,13 @@ test_submodule_forced_switch () {
########################## Modified submodule #########################
# Updating a submodule sha1 doesn't update the submodule's work tree
- test_expect_success "$command: modified submodule does not update submodule work tree" '
+ if test "$KNOWN_FAILURE_RECURSE_SUBMODULE_SERIES_BREAKS_REPLACE_SUBMODULE_TEST" = 1
+ then
+ RESULT="failure"
+ else
+ RESULT="success"
+ fi
+ test_expect_$RESULT "$command: modified submodule does not update submodule work tree" '
prolog &&
reset_work_tree_to add_sub1 &&
(
@@ -649,7 +655,7 @@ test_submodule_forced_switch () {
'
# Updating a submodule to an invalid sha1 doesn't update the
# submodule's work tree, subsequent update will fail
- test_expect_success "$command: modified submodule does not update submodule work tree to invalid commit" '
+ test_expect_$RESULT "$command: modified submodule does not update submodule work tree to invalid commit" '
prolog &&
reset_work_tree_to add_sub1 &&
(
diff --git a/t/t2013-checkout-submodule.sh b/t/t2013-checkout-submodule.sh
index 6847f75..60f6987 100755
--- a/t/t2013-checkout-submodule.sh
+++ b/t/t2013-checkout-submodule.sh
@@ -7,15 +7,21 @@ test_description='checkout can handle submodules'
test_expect_success 'setup' '
mkdir submodule &&
- (cd submodule &&
- git init &&
- test_commit first) &&
- git add submodule &&
+ (
+ cd submodule &&
+ git init &&
+ test_commit first
+ ) &&
+ echo first >file &&
+ git add file submodule &&
test_tick &&
git commit -m superproject &&
- (cd submodule &&
- test_commit second) &&
- git add submodule &&
+ (
+ cd submodule &&
+ test_commit second
+ ) &&
+ echo second > file &&
+ git add file submodule &&
test_tick &&
git commit -m updated.superproject
'
@@ -37,7 +43,8 @@ test_expect_success '"checkout <submodule>" updates the index only' '
git checkout HEAD^ submodule &&
test_must_fail git diff-files --quiet &&
git checkout HEAD submodule &&
- git diff-files --quiet
+ git diff-files --quiet &&
+ git diff-index --quiet --cached HEAD
'
test_expect_success '"checkout <submodule>" honors diff.ignoreSubmodules' '
@@ -63,8 +70,310 @@ test_expect_success '"checkout <submodule>" honors submodule.*.ignore from .git/
! test -s actual
'
+# TODO: hardcode $2
+# use flag instead of checking for directory.
+submodule_creation_must_succeed() {
+ # checkout base ($1)
+ git checkout -f --recurse-submodules $1 &&
+ git diff-files --quiet &&
+ git diff-index --quiet --cached $1 &&
+
+ # checkout target ($2)
+ if test -d submodule; then
+ echo change >>submodule/first.t &&
+ test_must_fail git checkout --recurse-submodules $2 &&
+ git checkout -f --recurse-submodules $2
+ else
+ git checkout --recurse-submodules $2
+ fi &&
+ test -e submodule/.git &&
+ test -f submodule/first.t &&
+ test -f submodule/second.t &&
+ git diff-files --quiet &&
+ git diff-index --quiet --cached $2
+}
+
+# TODO: inline this:
+submodule_removal_must_succeed() {
+
+ # checkout base ($1)
+ git checkout -f --recurse-submodules $1 &&
+ git submodule update -f . &&
+ test -e submodule/.git &&
+ git diff-files --quiet &&
+ git diff-index --quiet --cached $1 &&
+
+ # checkout target ($2)
+ echo change >>submodule/first.t &&
+ test_must_fail git checkout --recurse-submodules $2 &&
+ git checkout -f --recurse-submodules $2 &&
+ git diff-files --quiet &&
+ git diff-index --quiet --cached $2 &&
+ ! test -d submodule
+}
+
+test_expect_success '"check --recurse-submodules" removes deleted submodule' '
+ # first some setup:
+ git config -f .gitmodules submodule.submodule.path submodule &&
+ git config -f .gitmodules submodule.submodule.url ./submodule.bare &&
+ git -C submodule clone --bare . ../submodule.bare &&
+ echo submodule.bare >>.gitignore &&
+ git add .gitignore .gitmodules submodule &&
+ git commit -m "submodule registered with a gitmodules file" &&
+
+ # for testing this case, do it in a fresh clone with the submodules
+ # git dir inside the superprojects .git/modules dir.
+ git clone --recurse-submodules . super &&
+ (
+ cd super &&
+ git checkout -b base &&
+ git checkout -b delete_submodule &&
+ rm -rf submodule &&
+ git rm submodule &&
+ git commit -m "submodule deleted" &&
+ submodule_removal_must_succeed base delete_submodule
+ )
+'
+
+test_expect_success '"checkout --recurse-submodules" does not delete submodule with .git dir inside' '
+ git fetch super delete_submodule &&
+ git checkout --recurse-submodules FETCH_HEAD 2>output.err &&
+ test_i18ngrep "cannot remove submodule" output.err &&
+ test -d submodule/.git
+'
+
+test_expect_success '"checkout --recurse-submodules" repopulates submodule' '
+ (
+ cd super &&
+ submodule_creation_must_succeed delete_submodule base
+ )
+'
+
+test_expect_success '"checkout --recurse-submodules" repopulates submodule in existing directory' '
+ (
+ cd super &&
+ git checkout --recurse-submodules delete_submodule &&
+ mkdir submodule &&
+ submodule_creation_must_succeed delete_submodule base
+ )
+'
+
+test_expect_success '"checkout --recurse-submodules" replaces submodule with files' '
+ (
+ cd super
+ git checkout -f base &&
+ git checkout -b replace_submodule_with_file &&
+ git rm -f submodule &&
+ echo "file instead" >submodule &&
+ git add submodule &&
+ git commit -m "submodule replaced" &&
+ git checkout -f base &&
+ git submodule update -f . &&
+ git checkout --recurse-submodules replace_submodule_with_file &&
+ test -f submodule
+ )
+'
+
+test_expect_success '"checkout --recurse-submodules" removes files and repopulates submodule' '
+ (
+ cd super &&
+ submodule_creation_must_succeed replace_submodule_with_file base
+ )
+'
+
+test_expect_success '"checkout --recurse-submodules" replaces submodule with a directory' '
+ (
+ cd super
+ git checkout -f base &&
+ git checkout -b replace_submodule_with_dir &&
+ git rm -f submodule &&
+ mkdir -p submodule/dir &&
+ echo content >submodule/dir/file &&
+ git add submodule &&
+ git commit -m "submodule replaced with a directory (file inside)" &&
+ git checkout -f base &&
+ git submodule update -f . &&
+ git checkout --recurse-submodules replace_submodule_with_dir &&
+ test -d submodule &&
+ ! test -e submodule/.git &&
+ ! test -f submodule/first.t &&
+ ! test -f submodule/second.t &&
+ test -d submodule/dir
+ )
+'
+
+test_expect_success '"checkout --recurse-submodules" removes the directory and repopulates submodule' '
+ (
+ cd super
+ submodule_creation_must_succeed replace_submodule_with_dir base
+ )
+'
+
+test_expect_success SYMLINKS '"checkout --recurse-submodules" replaces submodule with a link' '
+ (
+ cd super
+ git checkout -f base &&
+ git checkout -b replace_submodule_with_link &&
+ git rm -f submodule &&
+ ln -s submodule &&
+ git add submodule &&
+ git commit -m "submodule replaced with a link" &&
+ git checkout -f base &&
+ git submodule update -f . &&
+ git checkout --recurse-submodules replace_submodule_with_link &&
+ test -L submodule
+ )
+'
+
+test_expect_success SYMLINKS '"checkout --recurse-submodules" removes the link and repopulates submodule' '
+ (
+ cd super
+ submodule_creation_must_succeed replace_submodule_with_link base
+ )
+'
+
+test_expect_success '"checkout --recurse-submodules" updates the submodule' '
+ (
+ cd super
+ git checkout --recurse-submodules base &&
+ git diff-files --quiet &&
+ git diff-index --quiet --cached HEAD &&
+ git checkout -b updated_submodule &&
+ (
+ cd submodule &&
+ echo x >>first.t &&
+ git add first.t &&
+ test_commit third
+ ) &&
+ git add submodule &&
+ test_tick &&
+ git commit -m updated.superproject &&
+ git checkout --recurse-submodules base &&
+ git diff-files --quiet &&
+ git diff-index --quiet --cached HEAD
+ )
+'
+
+test_expect_failure 'untracked file is not deleted' '
+ (
+ cd super &&
+ git checkout --recurse-submodules base &&
+ echo important >submodule/untracked &&
+ test_must_fail git checkout --recurse-submodules delete_submodule &&
+ git checkout -f --recurse-submodules delete_submodule
+ )
+'
+
+test_expect_success 'ignored file works just fine' '
+ (
+ cd super &&
+ git checkout --recurse-submodules base &&
+ echo important >submodule/ignored &&
+ echo ignored >.git/modules/submodule/info/exclude &&
+ git checkout --recurse-submodules delete_submodule
+ )
+'
+
+test_expect_success 'dirty file file is not deleted' '
+ (
+ cd super &&
+ git checkout --recurse-submodules base &&
+ echo important >submodule/first.t &&
+ test_must_fail git checkout --recurse-submodules delete_submodule &&
+ git checkout -f --recurse-submodules delete_submodule
+ )
+'
+
+test_expect_success 'added to index is not deleted' '
+ (
+ cd super &&
+ git checkout --recurse-submodules base &&
+ echo important >submodule/to_index &&
+ git -C submodule add to_index &&
+ test_must_fail git checkout --recurse-submodules delete_submodule &&
+ git checkout -f --recurse-submodules delete_submodule
+ )
+'
+
+# This is ok in theory, we just need to make sure
+# the garbage collection doesn't eat the commit.
+test_expect_success 'different commit prevents from deleting' '
+ (
+ cd super &&
+ git checkout --recurse-submodules base &&
+ echo important >submodule/to_index &&
+ git -C submodule add to_index &&
+ test_must_fail git checkout --recurse-submodules delete_submodule &&
+ git checkout -f --recurse-submodules delete_submodule
+ )
+'
+
+test_expect_failure '"checkout --recurse-submodules" needs -f to update a modifed submodule commit' '
+ (
+ cd submodule &&
+ git checkout --recurse-submodules HEAD^
+ ) &&
+ test_must_fail git checkout --recurse-submodules master &&
+ test_must_fail git diff-files --quiet submodule &&
+ git diff-files --quiet file &&
+ git checkout --recurse-submodules -f master &&
+ git diff-files --quiet &&
+ git diff-index --quiet --cached HEAD
+'
+
+test_expect_failure '"checkout --recurse-submodules" needs -f to update modifed submodule content' '
+ echo modified >submodule/second.t &&
+ test_must_fail git checkout --recurse-submodules HEAD^ &&
+ test_must_fail git diff-files --quiet submodule &&
+ git diff-files --quiet file &&
+ git checkout --recurse-submodules -f HEAD^ &&
+ git diff-files --quiet &&
+ git diff-index --quiet --cached HEAD &&
+ git checkout --recurse-submodules -f master &&
+ git diff-files --quiet &&
+ git diff-index --quiet --cached HEAD
+'
+
+test_expect_failure '"checkout --recurse-submodules" ignores modified submodule content that would not be changed' '
+ echo modified >expected &&
+ cp expected submodule/first.t &&
+ git checkout --recurse-submodules HEAD^ &&
+ test_cmp expected submodule/first.t &&
+ test_must_fail git diff-files --quiet submodule &&
+ git diff-index --quiet --cached HEAD &&
+ git checkout --recurse-submodules -f master &&
+ git diff-files --quiet &&
+ git diff-index --quiet --cached HEAD
+'
+
+test_expect_failure '"checkout --recurse-submodules" does not care about untracked submodule content' '
+ echo untracked >submodule/untracked &&
+ git checkout --recurse-submodules master &&
+ git diff-files --quiet --ignore-submodules=untracked &&
+ git diff-index --quiet --cached HEAD &&
+ rm submodule/untracked
+'
+
+test_expect_failure '"checkout --recurse-submodules" needs -f when submodule commit is not present (but does fail anyway)' '
+ git checkout --recurse-submodules -b bogus_commit master &&
+ git update-index --cacheinfo 160000 0123456789012345678901234567890123456789 submodule &&
+ BOGUS_TREE=$(git write-tree) &&
+ BOGUS_COMMIT=$(echo "bogus submodule commit" | git commit-tree $BOGUS_TREE) &&
+ git commit -m "bogus submodule commit" &&
+ git checkout --recurse-submodules -f master &&
+ test_must_fail git checkout --recurse-submodules bogus_commit &&
+ git diff-files --quiet &&
+ test_must_fail git checkout --recurse-submodules -f bogus_commit &&
+ test_must_fail git diff-files --quiet submodule &&
+ git diff-files --quiet file &&
+ git diff-index --quiet --cached HEAD &&
+ git checkout --recurse-submodules -f master
+'
+
+KNOWN_FAILURE_RECURSE_SUBMODULE_SERIES_BREAKS_REPLACE_SUBMODULE_TEST=1
test_submodule_switch "git checkout"
+KNOWN_FAILURE_RECURSE_SUBMODULE_SERIES_BREAKS_REPLACE_SUBMODULE_TEST=
test_submodule_forced_switch "git checkout -f"
test_done
--
2.10.1.469.g00a8914
^ permalink raw reply related
* [PATCH 12/16] entry: write_entry to write populate submodules
From: Stefan Beller @ 2016-11-15 23:06 UTC (permalink / raw)
Cc: git, bmwill, gitster, jrnieder, mogulguy10, David.Turner,
Stefan Beller
In-Reply-To: <20161115230651.23953-1-sbeller@google.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
entry.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/entry.c b/entry.c
index 019826b..2330b6e 100644
--- a/entry.c
+++ b/entry.c
@@ -2,6 +2,7 @@
#include "blob.h"
#include "dir.h"
#include "streaming.h"
+#include "submodule.h"
static void create_directories(const char *path, int path_len,
const struct checkout *state)
@@ -211,6 +212,7 @@ static int write_entry(struct cache_entry *ce,
return error("cannot create temporary submodule %s", path);
if (mkdir(path, 0777) < 0)
return error("cannot create submodule directory %s", path);
+ schedule_submodule_for_update(ce, 1);
break;
default:
return error("unknown file mode for %s in index", path);
--
2.10.1.469.g00a8914
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox