From: Stefan Beller <sbeller@google.com>
To: Jonathan Tan <jonathantanmy@google.com>
Cc: Brandon Williams <bmwill@google.com>,
"git@vger.kernel.org" <git@vger.kernel.org>,
Jonathan Nieder <jrnieder@gmail.com>,
Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH 1/4] entry.c: submodule recursing: respect force flag correctly
Date: Fri, 14 Apr 2017 13:12:22 -0700 [thread overview]
Message-ID: <CAGZ79kYQpVKc1GGj4hxw6t-6eDo8zPjwf=CtEPFTc9b4St1EOQ@mail.gmail.com> (raw)
In-Reply-To: <15b87ca2-98c4-edfc-1e7e-7a25c28bd8da@google.com>
On Fri, Apr 14, 2017 at 11:28 AM, Jonathan Tan <jonathantanmy@google.com> wrote:
>> @@ -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.)
ok, will do.
>>
>> + 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.)
eh. I realize the patch evolved after writing the commit message initially.
Maybe:
fix all submodule_move_head force flags
Audit all callers of submodule_move_head and make sure the
force flag is handled correctly.
>
> 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.
Well we would have wanted to put
state->force ? SUBMODULE_MOVE_HEAD_FORCE : 0
here, but state is not passed into this function, so just make a comment
why we keep it at force all the time.
Thanks,
Stefan
next prev parent reply other threads:[~2017-04-14 20:12 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
2017-04-14 20:12 ` Stefan Beller [this message]
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
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='CAGZ79kYQpVKc1GGj4hxw6t-6eDo8zPjwf=CtEPFTc9b4St1EOQ@mail.gmail.com' \
--to=sbeller@google.com \
--cc=bmwill@google.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=jonathantanmy@google.com \
--cc=jrnieder@gmail.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.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).