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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 10, 2014, 15:42 UTC
Message-ID: <xmqq1tpbawqe.fsf@gitster.dls.corp.google.com>
In-Reply-To: <alpine.DEB.1.00.1411101400050.13845@s15462909.onlinehome-server.info>

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

Show 21 quoted lines

>> By the way, if the expected use case of updateInstead is what I
>> outlined in the previous message, would it make more sense not to
>> fail with "update-index --refresh" failure (i.e. the working tree
>> files have no changes since the index)?
>> 
>> Thinking about it a bit more, checking with "update-index --refresh"
>> feels doubly wrong.  You not just want the working tree files to be
>> pristine with respect to the index, but also you do not want to see
>> any change between the index and the original HEAD, i.e.
>> 
>> 	$ git reset --hard && echo >>Makefile ; git add Makefile
>>         $ git update-index --refresh ; echo $?
>>         0
>> 
>> this is not a good state from which you would want to update the
>> working tree.
>> 
>> Wouldn't the two-tree form "read-tree -u -m" that is the equivalent
>> to branch switching do a sufficient check?
>
> That is indeed what the patched code calls.

I know that ;-), but I think I wasn't clear enough.

I am not saying you are not using two-tree read-tree. I am saying the check with update-index --refresh before read-tree seems dubious.

There are three "cleanliness" we require in various occasions:

 (1) rebase asks you to have your index and the working tree match
     HEAD exactly, as if just after "reset --hard HEAD".

 (2) merge asks you to have your index match HEAD exactly (i.e. no
     output from "diff --cached HEAD"), and have no changes in the
     working tree at paths that are involved in the operation.  It
     is OK to have local changes in the working tree (but not in the
     index) at paths that are not involved in a mergy operation.

 (3) checkout asks you to have your index and the working tree match
     either HEAD or the commit you are switching to exactly at paths
     that are involved in the operation.  It is OK to have local
     changes in the working tree and in the index at paths that are
     not different between the commits you are moving between, and
     it is also OK to have the contents from the "new" commit in the
     working tree and the index at paths that are different between
     the two.

Dying when "update-index --refresh" signals a difference is an attempt to mimic #1, but it is in line with the spirit of the reason why a user would want to use updateInstead, I think. The situation is more like the person who pushed into your repository from sideline did a "checkout -B $current_branch $new_commit" to update the HEAD, the index and the working tree, to let you pretend as if you based your work on the commit he pushed to you.

While you still need to error out when your local work does not satisfy the cleanliness criteria #3 above, I do not think you would want to stop the operation when "checkout" would not fail, e.g. you have a local change that does not interfere with the update between the two commits, with this one:

+ if (run_command(&child)) + die ("Could not refresh the index");

When refreshed the index successfully, we signal that there were differences between the index and the working tree with a non-zero return value, so "Could not refresh" is not quite right, either.

But this one that checks the exit status from two-tree read-tree

+ if (run_command(&child)) + die ("Could not merge working tree with new HEAD. Good luck.");

is checking the right condition, i.e. cleanliness #3. The disposition should not be "die", but an error return to tell the caller to abort the push as we discussed earlier.