Re: [PATCH v5 1/7] reset: do not accept a mixed reset in a .git dir

[Christian Couder <chriscool@tuxfamily.org>]

On samedi 12 décembre 2009, Junio C Hamano wrote:
> Christian Couder <chriscool@tuxfamily.org> writes:
> > It is strange and fragile that a mixed reset is disallowed in a bare
> > repo but is allowed in a .git dir. So this patch simplifies things
> > by only allowing soft resets when not in a working tree.
>
> I would not mind what you said were "I find the difference strange", as
> it is a fairly subjective word.  But if you say "fragile", you would need
> to defend the use of the word better.  What kind of misuse does it
> invite, and what grave consequences such misuses would cause?  I do not
> see any fragility and I would want to understand it better so that I can
> write about it in the Release Note to warn people and encourage them to
> upgrade.
>
> More importantly, I think the difference between the two actually makes
> sense.  A bare repository by definition does _not_ have any work tree so
> there is no point in having the index file in there.  On the other hand,
> even though it is not necessary to "cd .git && git reset HEAD^", I don't
> see a strong reason why it needs to be disallowed.

Ok. I have the following patch now:

---------- 8< ---------------
commit c20f969db6e565f2fe854b95202c3ef95ad0ff42
Author: Christian Couder <chriscool@tuxfamily.org>
Date:   Thu Dec 10 22:10:07 2009 +0100

    reset: improve mixed reset error message when in a bare repo

    When running a "git reset --mixed" in a bare repository, the
    message displayed is:

    fatal: This operation must be run in a work tree
    fatal: Could not reset index file to revision 'HEAD^'.

    This message is a little bit misleading as a mixed reset is ok in
    a git directory, so it is not absolutely needed to run it in a
    work tree.

    So this patch improves upon the above by changing the message to:

    fatal: mixed reset is not allowed in a bare repository

    This patch is also needed to speed up "git reset" by using
    unpack_tree() directly (instead of execing "git read-tree"). A
    following patch will do just that.

    While at it, instead of disallowing "git reset --option" outside
    a work tree only when option is "hard" or "merge", we now disallow
    it except when option is "soft" or "mixed", as it is safer if we
    ever add options to "git reset".

diff --git a/builtin-reset.c b/builtin-reset.c
index 11d1c6e..ac3505b 100644
--- a/builtin-reset.c
+++ b/builtin-reset.c
@@ -286,11 +286,15 @@ int cmd_reset(int argc, const char **argv, const char 
*pre
        if (reset_type == NONE)
                reset_type = MIXED; /* by default */

-       if ((reset_type == HARD || reset_type == MERGE)
-           && !is_inside_work_tree())
+       if (reset_type != SOFT && reset_type != MIXED
+            && !is_inside_work_tree())
                die("%s reset requires a work tree",
                    reset_type_names[reset_type]);

+       if (reset_type == MIXED && is_bare_repository())
+               die("%s reset is not allowed in a bare repository",
+                   reset_type_names[reset_type]);
+
        /* Soft reset does not touch the index file nor the working tree
         * at all, but requires them in a good order.  Other resets reset
         * the index file to the tree object we are switching to. */
---------- 8< ---------------

> On the other hand, I don't see a strong reason why such a use needs to be
> supported, other than "we've allowed it for a long time, and people may
> have hooks (they typically start in $GIT_DIR) that rely on it", which by
> itself is not something strong enough to veto the change.  It is only a
> minor incompatibility and I can be persuaded to listen to a pros-and-cons
> argument.
>
> An honest justification might have been "This change to disallow a mixed
> reset in $GIT_DIR of a repository with a work tree will break existing
> scripts, but I think it is not widely used _for such and such reasons_,
> and can easily be worked around.  On the other hand, this change vastly
> simplifies the reimplementation of 'reset' _because X and Y and Z_".

My opinion is that it works this way just by accident not by design (that's 
why I said "fragile"). But if we don't want to take any risk of regression, 
then let's use the new patch above.

> > This patch is also needed to speed up "git reset" by using
> > unpack_tree() directly (instead of execing "git read-tree").
>
> It is very unclear _why_ it is "needed" from this description.

The reason is that after the next patch, it will not fail in a bare 
repository, so if we don't change anything, the test that checks that it 
fails in a bare repo will fail. I will add this explanation to the new 
patch.

Best regards,
Christian.