Re: [PATCH 2/2] Let deny.currentBranch=updateInstead ignore submodules

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 13, 2014, 19:48 UTC
Message-ID: <xmqq4mu2285w.fsf@gitster.dls.corp.google.com>
In-Reply-To: <alpine.DEB.1.00.1411131850510.13845@s15462909.onlinehome-server.info>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Show 14 quoted lines

> Thanks for mentioning this. I would like to ask not to loosen this later.
> Let me try to explain in more detail than before why I think it would make
> *my* life hard if it ever were loosened.
> ...
> And now when I try to push, Git complains that the working directory is
> not clean, which is *just* fine by me, because after further inspection it
> turns out that the uncommitted changes – which are in a different file
> than the changes introducing my new feature – would have borked the
> production system rather thoroughly.
> ...
> In my mind, when (and if!) a less strict version is desired, it should be
> added as another denyCurrentBranch value and 'updateInstead' should be
> unaffected, otherwise, speaking for me personally, all my work in this
> patch series would be for naught.

Thanks for an explanation why the updateInstead mode must require a pristine working tree (and the index). I *now* agree why such a mode would be useful, *after* reading it. I did not understand why *after* reading only the patch, the documentation updates and the log message.

That tells us something, doesn't it? ;-)

Also the failure case test must protect us from making a change you fear in the future. The interdiff you sent in a separate message was to smudge path2 that is modified in the 'fourth' commit, which should fail with either your "OK only if really clean" requirement or the other "OK as long as it does not interfere with the switch" criterion. Checking that is a good step, but you would want to have a separate "Smudge a path that is unchanged with the switch and see the push updateInstead fail" to protect the current semantics.

I agree that a new value "mergeInstead" or something should be invented when/if different workflows want a looser semantics. People would rely not only on "being able to push when clean" but also on "being safely prevented from pushing when not" (and that is where my earlier comment to test both sides comes from). Loosening the check to an existing "updateInstead" would break users who have been using updateInstead.

Also I suspect that people would want to send a patch to add "-q" to your "update-index --refresh" invocation, at which time you would need to add a call to "diff-files" to check that the working tree and the index match. You may want to add that "diff-files" now, or at least have a test that is designed to break when "-q" is added to "update-index --refresh" without adding the necessary "diff-files".

Thanks.