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

[Junio C Hamano <gitster@pobox.com>]

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

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