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

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 15, 2009, 20:17 UTC
Message-ID: <7vskbcyot5.fsf@alter.siamese.dyndns.org>
In-Reply-To: <200912152041.36194.chriscool@tuxfamily.org>

Christian Couder <chriscool@tuxfamily.org> writes:

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

This still doesn't seem to explain anything that the part you added after your patch.

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

I fail to see any sane logic behind this reasoning; you cannot decide if you need to allow or disallow the new --option with unspecified semantics until you have that --option, and you are saying

Hmm, "reset --option" that does not work when it should work is a bug, just like "reset --option" that does not refuse to work when it should refuse is, and you cannot decide if you should allow a new --option until you have it. Your "disallowing everything the code does not know about by default" doesn't particularly sound safer to me. I'd suggest dropping it from this patch.

It is perfectly fine to have a change like that, if it makes the logic easier to follow with the updated repertoire when a new --option is added, but not before.

Show 8 quoted lines

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

And do you still think it is accident after I explained the difference between the two for you (or perhaps you didn't read it)?

Show 7 quoted lines

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

That sounds as if you want to change the definition of what the expected behaviour is early, because you want to claim a regression you will later introduce is not a regression. I hope that is not the case.