RE: [RFC PATCHv2 14/17] submodule: teach unpack_trees() to update submodules
Show 8 quoted lines
> -----Original Message-----
> From: Stefan Beller [mailto:sbeller@google.com]
> Sent: Friday, December 02, 2016 7:30 PM
> To: bmwill@google.com; David Turner
> Cc: git@vger.kernel.org; sandals@crustytoothpaste.net; hvoigt@hvoigt.net;
> gitster@pobox.com; Stefan Beller
> Subject: [RFC PATCHv2 14/17] submodule: teach unpack_trees() to update
> submodules
Show 22 quoted lines
> 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)) {
> + if (!submodule_is_interesting(ce->name))
> + return 0;
> + if (ce_stage(ce) ? is_submodule_checkout_safe(ce->name,
> &ce->oid)
> + : !is_submodule_modified(ce->name, 1))This logic is too convoluted. Do a nested if or something.
Show 6 quoted lines
> + return 0;
> + } else {
> + int flags = CE_MATCH_IGNORE_VALID |
> CE_MATCH_IGNORE_SKIP_WORKTREE;
> + if (!ie_match_stat(o->src_index, ce, &st, flags))
> + return 0;Nit: I liked the old temp var "changed" better -- it made it clear what
ie_match_stat is checking.
Show 27 quoted lines
> + }
> errno = 0;
> }
> if (errno == ENOENT)
> @@ -1355,6 +1359,38 @@ 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))
> + return 0;
> +
> + if (lstat(old->name, &st)) {We never actually use st here. Should we? If not, is this really the right error message? And should we use access() instead of lstat?
Show 7 quoted lines
> + if (errno == ENOENT)
> + return 0;
> + return o->gently ? -1 :
> + add_rejected_path(o, ERROR_NOT_UPTODATE_SUBMODULE,
> + old->name);
> + }
> +