Re: [PATCH 1/2] Add a few more values for receive.denyCurrentBranch

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 10, 2014, 16:00 UTC
Message-ID: <xmqqwq739hau.fsf@gitster.dls.corp.google.com>
In-Reply-To: <alpine.DEB.1.00.1411101305510.13845@s15462909.onlinehome-server.info>

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

Show 23 quoted lines

>> I do not think of a good justification of detachInstead offhand, but
>> you must have thought things through a lot more than I did, so you
>> can come up with a work flow description that is more usable by mere
>> mortals to justify that mode.
>
> The main justification is that it is safer than updateInstead because it
> is guaranteed not to modify the working directory. I intended it for use
> by developers who are uncomfortable with updating the working directory
> through a push, and as uncomfortable with forgetting about temporary
> branches pushed, say, via "git push other-computer HEAD:refs/heads/tmp".
>
> However, I have to admit that I never used this myself after the first few
> weeks of playing with this patch series.
>
>> Without such a description to justify why detachInstead makes sense, for
>> example, I cannot even answer this simple question:
>> 
>>     Would it make sense to have a third mode, "check out if the
>>     working tree is clean, detach otherwise"?
>
> I imagine that some developer might find that useful. If you insist, I
> will implement it, even if I personally deem this mode way too complicated
> a concept for myself to be used safely.

You have been around long enough to know that adding a feature of an unknown value is the last thing I would ask, don't you?

I am essentially saying this:

    I do not see why and the patch and its documentation do not
    explain why it is useful, but Dscho wouldn't have added the
    feature without a reason better than "just because we can do
    so", so let's assume the mode is useful for some unknown reason.
    Would people find "updateInstead if able otherwise
    detachInstead" even more useful for that same unknown reason?

So a good answer would have been one of these:

 * detachInstead is not backed by a use case as useful as
   updateInstead, and was done more from "because we can while at
   it".  Let's drop it for now.

 * detachInstead is a good thing and here is an updated patch to
   better explain the readers of our documentation the workflow to
   use to the feature well.  updateIfAbleOrDetachInstead would be
   useful for the same reason stated in the updated documentation,
   but let's not make the scope of the topic too wide and stop at
   mentioning the possibility in the log message for now.

 * detachInstead is a good thing and here is an updated patch to
   better explain the readers of our documentation the workflow to
   use to the feature well.  Once a reader understands this, she
   would realize why updateIfAbleOrDetachInstead would not be
   useful, so it is not even worth mentioning the possibility.

Show 10 quoted lines

>> > +	if (run_command(&child))
>> > +		die ("Could not merge working tree with new HEAD.  Good luck.");
>> 
>> Drop "Good luck." and replace it with a more useful info.  At least,
>> tell the user what state you left her repository in by this botched
>> attempt, so that she can manually recover.
>
> The best information Git can give at that point is that the working tree
> could not be merged with the new HEAD. I stripped "Good luck." in the next
> iteration, although I mourn the loss of comedy in Git.

Being humourous is good, but "Good luck" while refusing to be helpful is not humourous; it is just being hostile to the user.

We would be showing the string as the reason for push failure to "git push" in the new code that does not die but signal the failure to our caller, so "could not merge working tree with new HEAD" is good (we shouldn't be doing the "advice" thing here).

Thanks.