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

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> 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.