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

Christian Couder [off-list ref] 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.

quoted

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)?

quoted

quoted

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.

On mardi 15 décembre 2009, Junio C Hamano wrote:

Christian Couder [off-list ref] writes:

quoted

    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.

Ok, I will drop it.

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.

Ok.

[...]

By "after the next patch, it will not fail in a bare repository",
did you mean "if the next patch blindly replaced an external call to
read-tree with an internal call to unpack_trees(), it will change the
behaviour, and we will end up allowing '--mixed in bare'.  To prevent it
from happening, cmd_reset() should check that condition upfront"?

Yes, that's what I meant.

Then you were not trying to hide regressions (which makes me happier).
But then doesn't the change belong to the next patch, not this one?

I can put it in the patch that calls unpack_trees() directly, but on the 
other hand it can also be seen as an improvement that could be applied 
to "maint" as it improves the error message.

Best regards,
Christian.