Re: [PATCH 5/6] status: do not depend on flaky reflog messages

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 16, 2013, 05:57 UTC
Message-ID: <7vip1ek37o.fsf@alter.siamese.dyndns.org>
In-Reply-To: <CALkWK0=R8h0_FUYMUFQ_qGC1Lf_Xvu84qigznYUK5MtzP8usPQ@mail.gmail.com>

Ramkumar Ramachandra <artagnon@gmail.com> writes:

Show 13 quoted lines

> Junio C Hamano wrote:
>> Two possibilities:
>>
>>  (1) Assume that the other thread will produce a more reasonable
>>      semantics when finished; perhaps the first line will go away
>>      entirely, or maybe it would say something like "# Rebasing;
>>      head at $commit".
>>
>>      Your topic does not _care_ what it would say, so you tweak the
>>      "status" test that is done during "rebase" so that they
>>      ignore the first lines; or
>
> You said you didn't want to regress to show senseless information,...

Go back and read it again.

    If you want to avoid regression, the codepath in wt-status.c
    should compensate for the change to "rebase" so that it checks
    $dotest/onto and show what is recorded in there.

> I agreed with that.  What is wrong with the patch I showed in the
> previous email?

Which previous? My message you are responding to was a response to your non-patch message, and tangling the In-reply-to backwards will reach your original patch.

Puzzled....

> Smudging is a bad hack, and must only be used as a
> last resort: when an another topics updates status to say something
> sensible, it will have to unsmudge the tests.

Yes. I just got an impression, from your incessant arguing, that you are resisting the "ignore the top" as "extra work that is not interesting to _me_", while I was trying to see what is the best way to go forward for the _project_.

Honestly, I didn't want to waste my time and was trying to come up with a compromise, which would be a small regression to the tests that we will need to fix with the other topic. Because you already said that you are not interested in that topic, I was anticipating that the work to polish the topic would be a lot easier than anything I would have to do with you in this topic, because others are a lot easier to collaborate with than you are, and that is where that suggestion comes from.

Show 11 quoted lines

> diff --git a/wt-status.c b/wt-status.c
> index bf84a86..99c55e3 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -1182,7 +1182,7 @@ void wt_status_print(struct wt_status *s)
>                                 if (!get_sha1("HEAD", sha1) &&
>                                     !hashcmp(sha1, state.detached_sha1))
>                                         on_what = _("HEAD detached at ");
> -                               else
> +                               else if (!state.rebase_in_progress)
>                                         on_what = _("HEAD detached from ");

Is this supposed to be on top of your original patch?

The primary reason I suggested to special case "rebase-in-progress" in the "how about this" patch was because that way, you can have the "have builtin/commit.c honor reflog-action" much earlier in the series, because what this part of the code would say will not be affected by what is recorded in the reflog. If the above, together with the original patch, makes the code match the expectation of the "rebase stops in the middle and then status runs" tests, that's also fine. The other topic needs to redo it in either case anyway.

Show 5 quoted lines

> Unless we change the first line drastically to say: "rebase in
> progress: rebasing onto $ONTO" (or something), I don't think this
> makes sense.  And if we were to do that, why not do it properly like
> "rebase ($N/$M): onto $ONTO, upstream $UPSTREAM, branch $BRANCH"?
> Other people on a different thread are already handling that,...

That is the thread I was referring to.

Show 8 quoted lines

>
> So, you have three simple choices now:
>
> 1. Accept the simple patch I proposed above.
> 2. Propose an alternative patch quickly.  *Patch*.  No more English.
> 3. Reject all patches, and leave me no choice but to smudge.
>
> Which one is it going to be?

None of the above. After going back and forth this long, I won't want to pick an incremental from the middle of discussion, so (1) is out. This is not my itch and I am only helping you in a way that the result will not hurt the project, so (2) is not it. But assuming that "checkout that is done as an implementation detail in rebase is _not_ checkout from end user's point of view" is where we want to go, discarding this series is not something we want to see, either.

A rerolled series that does:

 - Tweak wt-status.c to take information not from reflog, when
   detached during rebase (this may need to tweak existing tests,
   as different "rebase" modes may use 'checkout' liberally in
   different ways);

 - Teach builtin/commit.c to pay attention to reflog-action; thanks
   to the previous step, this will not have to change the tests;

 - Update the reflog message rebase uses, but again this will not
   affect wt-status.c at all.

would be one way to go.