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

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

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

Show 13 quoted lines

> On Thu, 13 Nov 2014, Johannes Schindelin wrote:
>
>> Due to that experience, the documentation also states pretty clearly that
>> `updateInstead` succeeds only in updating the current branch if the
>> working directory is clean.
>
> To clarify why `updateInstead` is stricter than the `merge` scenario: when
> pushing into a remote repository with attached working directory, we
> cannot simply clean up merge conflicts or other problems introduced by a
> merge. Indeed, it is even possible to push without having any option to
> fix files in the working directory afterwards. Therefore, the
> `updateInstead` feature really needs to be adamant about the working
> directory being clean.

As the stricter check (which I think is unnecessarily strict and which you don't) can be loosened later without making something the user used to be able to do in an early version unable to do later, I am OK to accept the design as-is.

But after reading this addendum, I feel the need to double check.

A "reset --keep <new-commit>", which is the same as "checkout -B <current> <new-commit>" (note the lack of "-m"), does not have any "merge conflict" issues.

To see what I mean, follow along this simple experiment.

    # Just make sure we start clean
    $ git reset --hard

    # Create a playpen
    $ git checkout -b throwaway master

    # Pick one random path that is different.  We pretend that
    # somebody pushed the commit at the tip of 'next' from the side
    $ git diff --name-only next | head -n 1
    Documentation/git-clone.txt

    # Make sure another random path used in the experiment is unchanged
    $ git diff next -- COPYING | wc -l
    0

    # Smudge a path not involved in the branch/HEAD switching
    $ echo >>COPYING

    # attempt to updateInstead to the other version succeeds.
    $ git reset --keep next; echo $?
    0
    $ git status -suno
     M COPYING
    # Notice that we did not get into a funny state.
    # You can verify it with "git diff".

    # Go back and try smudging what would need a real merge
    $ git reset --hard master
    $ echo >>Documentation/git-clone.txt

    # attempt to updateInstead to the other version
    $ git reset --keep next; echo $?
    error: Entry 'Documentation/git-clone.txt' not uptodate. Cannot merge.
    fatal: Could not reset index file to revision 'next'.
    128

    # See that HEAD did not move
    $ git log HEAD...master | wc -l
    0

    # See that the working tree change is intact
    $ git status -suno
     M Documentation/git-clone.txt
    # Notice that we did not get into a funny state.
    # You can verify it with "git diff".

The semantics is just as safe as "checkout <another-branch>" without the "-m" option; if a local change may be clobbered or needs real file-level merge, the operation fails without touching anything to preserve the local state.

It is OK for us to agree to disagree, and I am willing to accept the stricter design for an initial version because loosening it later is possible without harming users.

But I wanted to first make sure that your disagreement is an informed one. Do you still feel the need for "absolutely clean", even if you take the safety built into "reset --keep" shown in the above experiment into account?