From: Jonathan Tan <jonathantanmy@google.com>
To: Stefan Beller <sbeller@google.com>, bmwill@google.com
Cc: git@vger.kernel.org, jrnieder@gmail.com, gitster@pobox.com
Subject: Re: [PATCH 1/4] entry.c: submodule recursing: respect force flag correctly
Date: Fri, 14 Apr 2017 11:28:56 -0700 [thread overview]
Message-ID: <15b87ca2-98c4-edfc-1e7e-7a25c28bd8da@google.com> (raw)
In-Reply-To: <20170411234923.1860-2-sbeller@google.com>
On 04/11/2017 04:49 PM, Stefan Beller wrote:
> In case of a non-forced worktree update, the submodule movement is tested
> in a dry run first, such that it doesn't matter if the actual update is
> done via the force flag. However for correctness, we want to give the
> flag is specified by the user. All callers have been inspected and updated
> if needed.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> entry.c | 8 ++++----
> unpack-trees.c | 7 ++++++-
> 2 files changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/entry.c b/entry.c
> index d2b512da90..d6b263f78e 100644
> --- a/entry.c
> +++ b/entry.c
> @@ -208,7 +208,8 @@ static int write_entry(struct cache_entry *ce,
> sub = submodule_from_ce(ce);
> if (sub)
> return submodule_move_head(ce->name,
> - NULL, oid_to_hex(&ce->oid), SUBMODULE_MOVE_HEAD_FORCE);
> + NULL, oid_to_hex(&ce->oid),
> + state->force ? SUBMODULE_MOVE_HEAD_FORCE : 0);
> break;
> default:
> return error("unknown file mode for %s in index", path);
> @@ -282,12 +283,11 @@ int checkout_entry(struct cache_entry *ce,
> unlink_or_warn(ce->name);
>
> return submodule_move_head(ce->name,
> - NULL, oid_to_hex(&ce->oid),
> - SUBMODULE_MOVE_HEAD_FORCE);
> + NULL, oid_to_hex(&ce->oid), 0);
Should we be consistent (with the "else" block below and with the
existing code) to use "state->force ? SUBMODULE_MOVE_HEAD_FORCE : 0"
instead of 0? (I glanced briefly through the code and
SUBMODULE_MOVE_HEAD_FORCE might have no effect anyway if "old" is NULL,
but it's probably still better to be consistent.)
> } else
> return submodule_move_head(ce->name,
> "HEAD", oid_to_hex(&ce->oid),
> - SUBMODULE_MOVE_HEAD_FORCE);
> + state->force ? SUBMODULE_MOVE_HEAD_FORCE : 0);
> }
>
> if (!changed)
> diff --git a/unpack-trees.c b/unpack-trees.c
> index 8333da2cc9..7316ca99c2 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -252,14 +252,18 @@ static int check_submodule_move_head(const struct cache_entry *ce,
> const char *new_id,
> struct unpack_trees_options *o)
> {
> + unsigned flags = SUBMODULE_MOVE_HEAD_DRY_RUN;
> const struct submodule *sub = submodule_from_ce(ce);
> if (!sub)
> return 0;
>
> + if (o->reset)
> + flags |= SUBMODULE_MOVE_HEAD_FORCE;
It seems to me that this is independent of the entry.c change, and might
be better in its own patch. (Or if it is not, maybe the subject should
be "entry, unpack-trees: propagate force when submodule recursing" or
something like that, containing the names of both modified components.)
Also, you mentioned in the parent message that this patch is required
for patch 3. Is only the entry.c part required, or unpack-trees.c, or both?
> +
> switch (sub->update_strategy.type) {
> case SM_UPDATE_UNSPECIFIED:
> case SM_UPDATE_CHECKOUT:
> - if (submodule_move_head(ce->name, old_id, new_id, SUBMODULE_MOVE_HEAD_DRY_RUN))
> + if (submodule_move_head(ce->name, old_id, new_id, flags))
> return o->gently ? -1 :
> add_rejected_path(o, ERROR_WOULD_LOSE_SUBMODULE, ce->name);
> return 0;
> @@ -308,6 +312,7 @@ static void unlink_entry(const struct cache_entry *ce)
> case SM_UPDATE_CHECKOUT:
> case SM_UPDATE_REBASE:
> case SM_UPDATE_MERGE:
> + /* state.force is set at the caller. */
I don't understand the relevance of this comment - it is indeed set
there, but "state" is not used there until after the invocation to
unlink_entry so it doesn't seem related.
> submodule_move_head(ce->name, "HEAD", NULL,
> SUBMODULE_MOVE_HEAD_FORCE);
> break;
>
next prev parent reply other threads:[~2017-04-14 18:29 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-04-11 23:49 [PATCH 0/4] recursive submodules: git-reset! Stefan Beller
2017-04-11 23:49 ` [PATCH 1/4] entry.c: submodule recursing: respect force flag correctly Stefan Beller
2017-04-12 11:28 ` Philip Oakley
2017-04-14 18:28 ` Jonathan Tan [this message]
2017-04-14 20:12 ` Stefan Beller
2017-04-11 23:49 ` [PATCH 2/4] submodule.c: uninitialized submodules are ignored in recursive commands Stefan Beller
2017-04-13 19:05 ` Brandon Williams
2017-04-13 19:12 ` Stefan Beller
2017-04-13 19:14 ` Brandon Williams
2017-04-11 23:49 ` [PATCH 3/4] submodule.c: harden submodule_move_head against broken submodules Stefan Beller
2017-04-12 11:32 ` Philip Oakley
2017-04-13 19:08 ` Brandon Williams
2017-04-13 19:17 ` Stefan Beller
2017-04-14 20:13 ` Jonathan Tan
2017-04-11 23:49 ` [PATCH 4/4] builtin/reset: add --recurse-submodules switch Stefan Beller
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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=15b87ca2-98c4-edfc-1e7e-7a25c28bd8da@google.com \
--to=jonathantanmy@google.com \
--cc=bmwill@google.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.