patch, 6 partsrebase: prepare to write reflog message for checkout

Hi,

I'm happy to report that I have found a reasonable solution to the problem: see [5/6].

The larger problem still persists: in my opinion, b397ea4 takes the wrong approach to the problem it is attempting to solve; nobody cares _how_ I got to a detached HEAD state; what is important is that I'm stuck in such a state and need useful information.

The correct approach, in my opinion, is already taken by my prompt: use git describe.

  artagnon|checkout-dash=$ git checkout @~1
  artagnon|(checkout-dash~1):~/src/git$

Now compare this with the approach taken by the patch:

  artagnon|(checkout-dash~1):~/src/git$ git status
  # HEAD detached at 7aa7992
  nothing added to commit but untracked files present

Completely useless.

Unfortunately, it is too late to revert b397ea4, as too much stuff already depends on it now (see builtin/branch.c for example). Reworking the code to use describe is not an easy task at all: describe has no exposed API, and is polluted with die() statements. Nevertheless, it can be a fruitful exercise for someone who is willing to take on the challenge.

Thanks.

Ramkumar Ramachandra (6):
  t/checkout-last: checkout - doesn't work after rebase
  rebase: prepare to write reflog message for checkout
  rebase -i: prepare to write reflog message for checkout
  wt-status: remove unused field in grab_1st_switch_cbdata
  status: do not depend on flaky reflog messages
  checkout: respect GIT_REFLOG_ACTION

 builtin/checkout.c         | 11 ++++++++---
 git-rebase--interactive.sh |  2 ++
 git-rebase.sh              |  2 ++
 t/t2012-checkout-last.sh   | 16 ++++++++++++++++
 t/t7512-status-help.sh     | 37 +++++++++++++++++--------------------
 wt-status.c                | 13 ++++---------
 6 files changed, 49 insertions(+), 32 deletions(-)

The following command

  $ git checkout -

does not work as expected after a rebase. Every kind of rebase must behave in the exactly same way: for the purposes of checkout -, the rebase event should be inconsequential.

Add two failing tests documenting this bug: one for a normal rebase, and another for an interactive rebase.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t2012-checkout-last.sh | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Ramkumar Ramachandra <artagnon@gmail.com> writes:

Why two?

After the discussion, I would have expected to see the two argument form:

	git rebase [-i] master other
        
started on the 'other' branch and also started on a branch that is
not 'master' or 'other', also be tested to specify the desired
behaviour in these cases.

Junio C Hamano wrote:

What breaks checkout - is the initial HEAD detachment (which writes that "checkout: " message), before anything else happens. None of <onto>, <upstream>, and <branch> make any difference: I'm testing exactly the code that I patched.

I have recently been told that I should be testing "end-user behavior" by treating the programs as black-boxes, instead of "implementation". What is your opinion on the issue? Should I write more tests?

Ramkumar Ramachandra <artagnon@gmail.com> writes:

Of course, the behaviours that should be observable by end-users need be spelled out. Also, an impementation detail that cannot be observed or make any difference to the end-user experience should not be cast in stone.

The guideline is a good one, but you need to realize that there is a difference between "do not test implementation details" and "do not look at implementation when designing tests". Only the former is necessary and correct: it lets you avoid over-specifying the behaviour.

Sometimes you can trivially tell that some obvious implementations that may be different from what you have can break the expectation you are setting. For example, in v1.5.0, "rebase A B" literally ran "git-checkout B" as the first thing, and if you want the end-user to expect that @{-$n} does not resolve to B because such internal "checkout" is not an user action, covering "rebase A B", even if you know that the current implemention happens to be immune to such a form of breakage, would be a good way to future-proof your code.

That still is testing the behaviour. You are just taking advantage of the fact that you know the implementation and can anticipate how future changes by careless people could break the behaviour.

rebase should never write "checkout: " messages to the reflog, since it is highly confusing to the end user, and breaks grab_nth_branch_checkout(), as demonstrated by failing tests in t/checkout-last. Set a sensible GIT_REFLOG_ACTION: checkout does not respect GIT_REFLOG_ACTION yet, but this defect will be addressed in a future patch.

When the defect is addressed, rebase will write the following line to the reflog when started:

  rebase: checkout master

This is much better than the confusing message it currently writes:

  checkout: moving from master to 1462b67

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 git-rebase.sh | 2 ++
 1 file changed, 2 insertions(+)

Ramkumar Ramachandra <artagnon@gmail.com> writes:

"breaks" because "the branch flipping rebase internally does is not 'checkout' as far as the end user is concerned", which essentially is the tautology with the first part "should never write".

I would say

    The branch flipping rebase internally does is not 'checkout' as
    far as the end user is concerned; rebase should never write
    "checkout: " messages to the reflog.  Instead it should say
    "rebase: checkout master".

That is not a defect; it did not have to so far, until this patch.

I suspect doing 6/6 before this patch may make more sense.

Junio C Hamano wrote:

Will fix those.

Yeah, I'd done it like that in the original (early preview thing). Allow me to explain why I flipped the ordering.

The problem I am facing is that 6/6 causes very major breakages, and 5/6 attempts to minimize that fallout and make life for 6/6 easier. The problem with putting this patch (and the rebase -i) after those two is simple: it calls set_reflog_action, but never explicitly indicates that it wants to set the reflog message for checkout. As a result, the reflog messages are merely accidental and will look like:

  rebase
  rebase -i (start)

in both the critical patches (5/6 and 6/6). This was an absolute debugging disaster for me, and I didn't know what wt-status was trying to tell me with its cryptic "detached HEAD to" and "detached HEAD from" messages.

Makes sense?

Interactive rebase should never write "checkout: " messages to the reflog, since it is highly confusing to the end user, and breaks grab_nth_branch_checkout(), as demonstrated by failing tests in t/checkout-last. Set a sensible GIT_REFLOG_ACTION: checkout does not respect GIT_REFLOG_ACTION yet, but this defect will be addressed in a future patch.

When the defect is addressed, rebase -i will write the following line to the reflog when started:

  rebase -i (start): checkout master

This is much better than the confusing message it currently writes:

  checkout: moving from master to 1462b67

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 git-rebase--interactive.sh | 2 ++
 1 file changed, 2 insertions(+)

The same comment as 2/6 applies to this one. What these two do makes sense, but I think having 6/6 before them would make the series easier to follow.

The struct grab_1st_switch_cbdata has the field "found", which is set in grab_1st_switch() when a match is found. This information is redundant and unused by any caller: the return value of the function serves to communicate this information anyway. Remove the field.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 wt-status.c | 2 --
 1 file changed, 2 deletions(-)

Ramkumar Ramachandra <artagnon@gmail.com> writes:

Good.

b397ea4 (status: show more info than "currently not on any branch", 2013-03-13) made the output of 'git status' richer in the case of a detached HEAD. Before this patch, with a detached HEAD:

  $ git status
  # Not currently on any branch.

After the patch:

  $ git checkout v1.8.2
  # HEAD is now detached
  $ git status
  # HEAD detached at v1.8.2.

It works by digging the reflog for the most recent message of the form "checkout: moving from xxxx to yyyy". It then asserts that HEAD and "yyyy" are the same, and displays this message. When they aren't equal, it displays:

  $ git status
  # HEAD detached from fe11db.

At this point, the utility of such a message is in question. Moreover, there are several tests in t/status-help that explicitly rely on rebase writing "checkout: " messages to the reflog. As evidenced by the failing tests in t/checkout-last, these messages are completely unintended and flaky. Relying on them only makes it harder to improve the reflog messages written by scripts. As a reasonable compromise, remove the logic to display the "HEAD detached from ..." message, and fallback to "Not currently on any branch." in this case. Update the tests, giving scripts some breathing space.

This issue is not isolated to rebase at all. Several other scripts like bisect write (confusing) "checkout: " messages to the reflog. Fixing them is left as an exercise to other contributors.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t7512-status-help.sh | 24 +++++++++++-------------
 wt-status.c            | 11 ++++-------
 2 files changed, 15 insertions(+), 20 deletions(-)

Ramkumar Ramachandra <artagnon@gmail.com> writes:

You can question, but I am not convinced the answer is an unambiguous "not useful"

You were at 1.8.2 but no longer are, so in the following sequence:

    $ git checkout v1.8.2
    $ git status
    $ git reset --hard HEAD^
    $ git status
    
the former would say "detached at v1.8.2" while the latter should
*not*, because we are no longer at v1.8.2.  "detached from v1.8.2"
is too subtle a way to express the state, and is confusing, but I
would not be surprised if people find it useful to be able to learn
"v1.8.2" even after you strayed away.

The above only helps to convince me that "rebase should not affect what the last checked-out branch was by letting 'checkout' it internally calls to write reflog entries for it" With patches 6, 2, and 3, I thought you fixed that issue.

So I am not convinced that is a good argument to justify to regress "HEAD detached from" message to "Not on any branch".

At least, not just yet.

Any scripted Porcelain that use "checkout", with patch 6, should be able to do so with reflog-action environment variable, right? And 2 and 3 are examples of such fixes to two of them, which argues even more strongly that 6 should be earier in the series, I think.

Junio C Hamano wrote:

I'd just like to point out quickly that I first attempted to write 6/6 without this patch. It is absolutely impossible, because the "detached HEAD from/to" messages no longer make any sense when checkout starts respecting GIT_REFLOG_ACTION. At that point, I'm was just monkeying around the trash-directory running describes to somehow try and make the expected output equal to the actual output. There was no method to the madness, and I was literally losing my mind.

This is _the_ patch that makes this series possible.

If you want to be convinced, please attempt to drop this patch and fix the tests in 6/6. You will see what I mean.

Junio C Hamano wrote:

I am not arguing for an unambiguous "not useful". I am arguing for a practical compromise: this patch locks things up too tightly, and makes life hell for contributors who want to improve reflog messages. To be clear: the problem is not the feature, but rather in the _implementation_ of the feature.

What is wrong with git describe?  Is this cheaper, or am I missing something?

I also thought of ignoring the first line in the actual output ("HEAD detached from/to"), and comparing the rest to make the tests pass. At that point, you start to wonder: what is this fantastic feature that we are bending over backwards for?

Ramkumar Ramachandra <artagnon@gmail.com> writes:

OK, but are we discussing the same "feature" (see below)?

I think what you are missing is that the "detached from" is not about your current HEAD after you flipped it around with many resets and commits. It is about what tag or what specific commit you detached your HEAD at originally.

"You can ask the same to describe" is wrong and is not a valid argument. Once you replace the HEAD^ with a distant commit (e.g. HEAD~400) in the third step in the example, "describe" will not talk about v1.8.2 at all.

Your argument can be that it is not a useful piece of information, and as you can probably guess from my "I wouldn't be surprised" above, I am not sure how useful it would be and in what situation [*1*].

But the original commit thought it was necessary and that was done for a reason; we need to be careful here. With a good justification why it is not necessary (or misleading to the user), I do not think we cannot change it.

As various tools that use detached "intermediate" states leave enough clues for "status" to notice what is going on, I actually think it is a mistake to focus on what happens when we are in such a detached "intermediate" status with this codepath. For example:

	$ git rebase master
        ... replays some but stops
        $ git status

currently uses that "HEAD detached from" codepath, but I think that is a mistake. We could not just tell the HEAD is detached, but the reason _why_ the HEAD is detached (i.e. we are in the middle of a rebase). The prompt script can do it, "status" should be able to do the same, and do a lot more sensible thing than unconditionally showing that "HEAD detached from" and then say "You are currently rebasing" on a separate line. Most likely we do not want to even say "Not currently on any branch" but just say "You are currently rebasing branch X on top of Y" (and perhaps "N commits remaining to be replayed").

[Footnote]

*1* One thing I could think of is to start sightseeing or (more
    realistically) manually bisecting at a given release point,
    reset the detached HEAD around several times, and then want to
    be reminded where the session started from.  I do not think it
    is particularly a very good example, though.

Junio C Hamano wrote:

No, it is about what tag of specific commit you detached your HEAD from, *without using checkout*. If you used checkout, you'd get the "detached at" message, and I haven't changed that.

You're missing the point: how do I, as the end-user, detach my HEAD *without using checkout*? The hypothetical example you have given is:

  $ git checkout HEAD^
  $ git update-ref HEAD $(git rev-parse HEAD~400)

Which end-user executes that?

Precisely. It is a poorly thought-out feature that locks things up too tightly, and makes life hell for contributors. It must therefore be removed.

We cannot reverse-engineer intents. All we can do is look at the evidence in front of us. Read the commit message, and look at the newly added test. There is absolutely no indication about why this "detached from" is useful, and where.

That information is available in .git/{rebase-apply,rebase-merge}, and your suggestion pertains to improving show_rebase_in_progress(). The first line is about the state of HEAD, and is completely orthogonal to the issue at hand.

  artagnon|rebase-rev-failure|REBASE-i 2/3:~/src/git$ git status
  # HEAD detached from a7e9fd4
  # You are currently editing a commit while rebasing branch
'rebase-rev-failure' on '9926f66'.
  #
  nothing to commit, working directory clean

That first line about "HEAD detached from ..." is completely useless. And yes, my prompt is more useful. No prizes for guessing how often I use the long form of git status.

The example you have given now is:

  $ git checkout @^
  # or whatever bisect command to detach HEAD
  $ git reset @~3
  ...
  $ git reset @^
  ...
  $ git reset @~5
  ....
  # when was HEAD originally detached?

Yes, it is a contrived example where this feature arguably has some utility. Is it worth putting the information in the status for such an esoteric example? If one really wants this information, they can open up the reflog and grep for "checkout: ".

Ramkumar Ramachandra <artagnon@gmail.com> writes:

The part you stripped from your quote looked like this:

And your justification to make the latter "git status" to say "Not on any branch" instead of "detached from" was "what is wrong with describe".

The user used "checkout" to detach the HEAD, and the user stayed in that detached state and jumped around. Where is this "without using checkout" coming from?

Junio C Hamano wrote:

Apologies for the lack of clarity.

In this example, it is inconsequential whether I run:

  $ git checkout v1.8.2^

or:

  $ git checkout v1.8.2
  $ git reset --hard @^

as far as describe is concerned. It will give me the same good consistent answer in either case.

The point I was trying to make is:

  $ git checkout v1.8.2
  $ git checkout @^

will give a different result once again. As the end-user, I use checkout to inspect detached HEAD states, and reset to update worktree/refs.

Did my response to your examples make sense? Are you convinced that this feature should be removed yet?

Ramkumar Ramachandra <artagnon@gmail.com> writes:

Yes, "describe HEAD" is about the location in the history of the commit at the current HEAD.

But the commit shown on "detached at/detached from" is for people who care about the differences between the initial "checkout" you did in these two examples: "checkout v1.8.2^" and "checkout v1.8.2".

That is different from the question you ask to "describe", which is the location your HEAD happens to be in the commit ancestry graph after you detached the HEAD with "checkout" and possibly jumping around. If you say "Not on any branch" instead of saying "detached from v1.8.2" (or v1.8.2^), you are losing information, aren't you?

As I said (twice), you can argue that that particular piece of information is not useful (at least to you), but why it is not useful has to be justified, against the justification given by b397ea4863a1 (status: show more info than "currently not on any branch", 2013-03-13) and people who have been using that information in the status output, no?

The last checkout is for HEAD^ while at v1.8.2 in this case, isn't it? Don't you want to show it?

Junio C Hamano wrote:

Yes, Junio.  I cannot justify that "detached HEAD from" is completely
useless, because it is not.  I argued for a practical compromise, and
have tried to show the huge negative impact against the marginal gain.
 If you argue that we absolutely must not cause an
information-regression against all odds and are unwilling to
compromise, I cannot improve the reflog messages written by various
scripts (and fix git checkout -).  I don't know any other way forward.

Ramkumar Ramachandra <artagnon@gmail.com> writes:

First of all, during a stopped "git rebase", I think "git status" that says "# HEAD detached at" or "# HEAD detached from" is not something we want in the end result anyway. In either case, that comes from the internal use of "git checkout" by the command to switch out of a concrete branch, which is *not* user initiated.

If you first update "git checkout" so that it will pay attention to a custom reflog-action exported by Porcelain scripts that may want to internally use it to flip branches (and without a custom one, it will still record "checkout: moving from A to B"), without exporting custom reflog-action from "rebase" and other Porcelain scripts, that would not affect any externally visible behaviour.

When you teach "git rebase" to pass custom reflog-action when it detaches the HEAD at "$onto^0" (because that is not a user-initiated action) as the next step, nth_prior_checkout and 1st_switch would stop counting them as "checkout" events. That fix will give you the desired result of the topic of this series, i.e. "git checkout @{-1}" ignores the internal "git checkout" done by "rebase".

That step may change the output from "git status" but I do not think the output from "git status" in this sequence:

	git rebase ;# stops upon conflict
        git status

is anything sacred. Tests modified by b397ea4863a1 (status: show more info than "currently not on any branch", 2013-03-13) do change the original "# Not currently on any branch" to "detached at", but I do not think b397ea4863a1 _wanted_ to say where the $onto commit was during the rebase (or was it?), so if they have to change to "detached from", I do not think it is a problem at all. I think it is very sane to update these selected "detached at" to "detached from" as needed.

That would be a sensible compromise, because we do not want to see either "detached at" or "detached from" during "rebase" in the longer term. We would rather want to see the message start with something like

	# Rebasing X on top of Y (23 commits remaining to be replayed)

which is under discussion in a separate thread lately. The change to expected output (i.e. "detached at" to "detached from") will have to be changed again in that separate topic.

The very last test that the b397ea4863a1 added, which wants to see "detached at atag", should be kept, and also there should be another test added to reset one step back to make sure we see "detached from" after it. It tests what the user sees after a "git checkout" initiated by the end-user.

Junio C Hamano wrote:

rebase already sets a custom reflog-action; in fact, 'set_reflog_action rebase' is executed after it sources sh-setup. I have to go out of my way to deliberately unset it before every checkout invocation to avoid breaking all the tests in t/status-help. Most unnatural way to go about writing a series, if you ask me.

... and you have missed the point yet again.

I previously wrote:

Let me try and explain this once again: to determine "HEAD detached from/to", wt-status must look for a previous "checkout: " message in the reflog. Since a rebase no longer executes a checkout, the "detached HEAD from" message no longer has anything to do with the rebase itself; it is completely _random_ and _incidental_, based on what checkout operation was executed last (eg. in a previous test). To change the "detached HEAD to" messages to "detached HEAD from" messages, I have to turn myself into a monkey that goes around running describes on the test-directory. After enduring hours of this mind-numbing job, the final result will be a terrible mess; tests can no longer be added, removed, reordered, or even tweaked. Everything depends on the exact ordering of the tests, and the incidental checkout invocations they contain.

How is it a compromise?  It is most uncompromising.

Orthogonal. I want checkout-dash now, and I should not have to be asked to do something as unrelated as redoing 'git status' output to get what I want.

Since you have made it clear that you will defend every bit of b397ea against all odds, and since I want checkout-dash now, we have to do something. I have a proposal: I will smudge the first line of the 'git status' output, before comparing it to make tests pass. Never mind that the line is incidental and Wrong; since I have no say in the matter, I will just sidestep the issue. I don't use the long-form 'git status' anyway, and am never going to see this ugliness. We both win. Okay?

Ramkumar Ramachandra <artagnon@gmail.com> writes:

Ah, OK, that is what I missed, so updating the "checkout" itself will affect the test Duy's commit updated.

But that does not change anything. The thing is, I fully agree with this:

because at the end of this series, we will not be recording the internal "checkout" as a checkout event in the reflog. What the top-line of "git status" says does not matter.

If I were doing this series, the first commit before doing anything else would be to strip that line from the expected output from "git status" for tests Duy's commit changed, with the justification perhaps like this:

    b397ea4863a1 (status: show more info than "currently not on any
    branch", 2013-03-13) wanted to make sure that after a checkout
    to detach HEAD, the user can see where the HEAD was originally
    detached from.

    The last test added by that commit to t7512 shows one example,
    immediately after HEAD is detached.  Add another test to show
    "detached HEAD from" form that should be shown when the user
    further resetted to another commit.

    The majority of tests in t7512 that commit updated, however, are
    looking at the status output while in "rebase", and the exact
    content of that first line relies on the fact that "git rebase"
    used "git checkout" to first detach HEAD before doing anything
    else, and allowed that to be recorded in the reflog just like a
    user initiated "checkout".  As far as the user is concerned,
    however, this operation is not a "checkout" that the user would
    want to count when later issuing "git checkout @{-1}", etc.

    We are going to fix "git rebase" not to let reflog record these
    internal events as "checkout"; these first lines will start
    saying different (and useless) things.  Improving them to give
    a message more appropriate during "rebase" (e.g. "# Rebasing
    branch X on top of Y", instead of "# Not currently on any
    branch" or "# HEAD detached from Z") is left to a separate
    future series, but until that happens, stop checking what
    appears on that first line.

and would keep the last one that shows "detached HEAD at". That is what that commit wantd to achieve. This first commit in the series would be a good place to add the missing one after that to show "detached HEAD from", doing "reset HEAD^". In other words, we can make this tirst step as a pure clean-up.

If stripping the first line to compare is too much, then I think it is an acceptable compromise to update the "# HEAD detached from $ONTO" Duy's patch changed from "# Not currently on ..." to whatever your updated "rebase" happens to produce, with the same (but weaker) justification as the last sentence in my "perhaps like this" above.

Ramkumar Ramachandra <artagnon@gmail.com> writes:

You are misreading me.  I am not defending every bit at all.

We both agree that what b397ea4863a1 (status: show more info than "currently not on any branch", 2013-03-13) expects from "git status" during an interrupted "git rebase" is _not_ sacred, especially when we are about to fix what "git rebase" records in the reflog.

I am only saying that the last test the commit adds must be kept unbroken. I am also saying that, even though that commit did not add a test for "detached from" case, we should add something like the attached to protect the behaviour. These two are sacred.

What happens to be shown during a stopped "git rebase" is not.

I have been assuming the "main" thing Duy wanted to do was the last test (and the one below), but was this meant as an improvement for "git status" output during that state? Showing $ONTO certainly makes some sense, and from that point of view, the change we are discussing _will_ be a regression if it just shows a random thing. 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.

But in practice, as far as we know other people are working on it in the near-by thread, I do not think this particular regression (if it was) matters that much. At the worst, we can queue that on top of this topic to fix "rebase ; checkout -" and have them graduate at the same time to avoid it.

 t/t7512-status-help.sh | 8 ++++++++
 1 file changed, 8 insertions(+)

Junio C Hamano <gitster@pobox.com> writes:

And it might be just the matter of doing something like this (totally untested, of course). Then the people who want to improve "status" to give more detailed state during "rebase" can further enhance the logic in this part to give more detailed information.

I have a feeling that without changing anything in t7512 but applying the change to commit.c in your [6/6], most tests in there would still pass, and you may even uncover latent *bug* that expected to show a wrong $ONTO value in "# HEAD detached from/at" line.

 wt-status.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Junio C Hamano wrote:

Good. You have proposed a solution to the problem. However, it is wrong for the following reasons:

1. It shows a pseudo "HEAD detached at" message.  Everywhere else,
when the user sees a "HEAD detached at $committish" message, git
rev-parse $committish = git rev-parse HEAD.  A rebase-in-progress
seems to be the only exception, and the user has no idea this is
happened.

2. The following no longer updates status:

  # in the middle of a rebase
  $ git reset @~2

The constant "HEAD detached at $onto" message is misleading and Bad. Besides, wasn't this the primary usecase you wanted?

You previously wrote:

Whether the HEAD is detached by bisect or rebase is irrelevant.

3. The problem is not unique to rebase at all; yet you have
special-cased it.  If this isn't a band-aid, what is?  The larger
problem, as I have stated previously, is that 'git status' output
depends on the _implementations_ of various commands (do they write a
"checkout: " message to the reflog or not?).  Therefore, a future
contributor who updates a command to write more sensible reflog
messages will have to apply a similar band-aid.

If you want to take the band-aid approach, I think this is the right way to do it:

Ramkumar Ramachandra <artagnon@gmail.com> writes:

See the other message. The first line from status in the middle of a rebase is secondary. End-user initiated "checkout" to detach is the primary thing.

You read and reacted to "bisect" too literally and missed "manually". The scenario I had in mind does _not_ use "git bisect" command, which may _internally_ run "git checkout" to detach, but for exactly the same reason why you want to touch "git rebase" in this series, its use of "checkout" should not count for the next "checkout -".

The sequence of "manually bisecting" is like

	$ git checkout v1.8.3
        ... test
        $ git status
        $ git log --oneline --first-parent v1.8.2..$detached_from
        ... pick a likely point
        $ git reset --hard $that_point
        ... go back to test further

which I actually do fairly often, as I tend to know more about the likely place something may have been broken than a mechanical "split the history into about the same number of commits" bisection.

It is an illustration for approach (2) in the other message.

In the longer term, you would want richer status output for various detached state, and that "how about this" patch shows where new code to support other cases should be added.

Junio C Hamano wrote:

Fine. If that is what you want, we'll have to special-case every known script-in-progress to stop writing the usual "HEAD detached from" message. That'll leave out only the "end-user cases", where you argue that the message is helpful.

Junio C Hamano wrote:

On what basis are you making that assumption? b397ea4 did not add a test for "detached HEAD from", and there is no evidence of what the author wanted in the commit message. What has happened has happened; the question is: what do _you_ want now?

Read the various "HEAD detached from/to" messages in the rebase tests in t/status-help and tell me that they make some sense. It does _not_ show a constant "HEAD detached from $onto", contrary to your assumption (evidenced by your patch in the later email). Look at these hunk added by b397ea4, and tell me that it's not a monkey-patch:

Ramkumar Ramachandra <artagnon@gmail.com> writes:

It should be fairly clear what _I_ want now. Re-read the first four lines you quoted above.

Again, re-read the part you did _not_ quote before those four lines. I do not think all of them make lot of sense; some of them may.

Unless you are also tackling the topic of the other thread:

  http://thread.gmane.org/gmane.comp.version-control.git/227432/focus=227469

(which I would not suggest to do in this topic), what these lines say during "rebase" does not really matter, as far as your topic is concerned.

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

 (2) Starting from the same assumption as above, but try to minimize
     the semantics change to user-visible behaviour this series
     makes.

     That means that even though the _primary_ thing you want to do
     is to tweak "rebase" and its internal use of "checkout" in such
     a way that reflog will not record the implementation-detail
     checkout (because that will affect the next "checkout -"), make
     sure that "status" while doing "rebase" reports where the
     internal "checkout" of $ONTO detached HEAD from/at.

The "something along this line" patch was an illustration of how the beginning of the effort for (2) would look like.

As I already said, I could see us going either way. If we can do (2), that may be better, but at end it would not matter that much as long as we will be doing the other topic, too.

Junio C Hamano wrote:

You said you didn't want to regress to show senseless information, and I agreed with that. What is wrong with the patch I showed in the previous email? 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.

Ramkumar Ramachandra <artagnon@gmail.com> writes:

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.

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

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.

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.

That is the thread I was referring to.

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.

Junio C Hamano wrote:

I have no desire to argue incessantly.  I just want a solution to the problem!

Please be more specific, and tell me what it should print when a rebase is in progress. Would a constant

  # rebase in progress; onto $ONTO

be sufficient?

Where did builtin/commit.c enter into the equation? Doesn't it already respect GIT_REFLOG_ACTION? See line 1461.

Okay.

Ramkumar Ramachandra <artagnon@gmail.com> writes:

List archive helps.

http://thread.gmane.org/gmane.comp.version-control.git/217342/focus=217444

Not just that single message, but a few messages in the discussion that led to the design shows that we wanted both "at" and "from" to mean something sensible, when a "checkout" initiated by the end-user detached HEAD and we are not on any branch, even after making commits or doing resets.

GIT_REFLOG_ACTION is an environment variable specifying the reflog message to write after an action is completed. Several other commands including merge, reset, and commit respect it.

Fix the failing tests in t/checkout-last by making checkout respect it too. You can now expect

  $ git checkout -

to work as expected after any rebase operation.

Also update the tests in t/status-help that rely on rebase writing "checkout: " messages to the reflog.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/checkout.c       | 11 ++++++++---
 t/t2012-checkout-last.sh |  4 ++--
 t/t7512-status-help.sh   | 13 ++++++-------
 3 files changed, 16 insertions(+), 12 deletions(-)