Re: [PATCH v3 4/4] submodule_needs_pushing() NEEDSWORK when we can not answer this question

From: Heiko Voigt <hvoigt@hvoigt.net>
Date: Nov 16, 2016, 14:26 UTC
Message-ID: <20161116142649.GA32087@book.hvoigt.net>
In-Reply-To: <xmqqtwb8s2m8.fsf@gitster.mtv.corp.google.com>

On Tue, Nov 15, 2016 at 04:13:51PM -0800, Junio C Hamano wrote:

Show 15 quoted lines

> 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*].

I think you have a point here. If I rephrase it like this: "We do consider it safe as no one in their sane mind *could* have changed the submodule pointers without having the submodule around..."

Since its actually hard to create such a situation without the submodule commit around I agree here.

Show 6 quoted lines

> *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'".

Yes saying that would be wrong. I was rather suggesting that we tell the user that we could not find the submodule commits to and that if he wants to proceed he should either pass --recurse-submodules=no or initialize the submodule.

But I think the above reasoning obsoletes my suggestion. I would adjust the comment accordingly but still keep the patch so we have documentation that this behavior is on purpose.

Cheers Heiko