Re: [PATCH 4/5] status: show the ref that is checked out, even if it's detached
- From
Duy Nguyen <pclouds@gmail.com>
- Date
- Mar 4, 2013, 12:17 UTC
- Message-ID
- <CACsJy8A+gw9oJYrJqa-b8noKM7qJTkZEd7ovmxqR68i6miCatA@mail.gmail.com>
- In-Reply-To
- <7vy5e49l3i.fsf@alter.siamese.dyndns.org>
On Mon, Mar 4, 2013 at 5:25 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 15 quoted lines
>> When a remote ref or a tag is checked out, HEAD is automatically >> detached. There is no user friendly way to find out what ref is >> checked out in this case. This patch digs in reflog for this >> information and shows "Detached from origin/master" or "Detached from >> v1.8.0" instead of "Currently not on any branch". > > "Detached from" is a nice attempt to compromise in the phrasing. > > We usually say you detach HEAD at v1.8.0, but what is shown is what > started from such a state but then the user may have built more > history on top of it and may no longer be at v1.8.0, so obviously we > do not want to say "Detached at". We are in a "detached at v1.8.0 > and then possibly built one or more commits on top" state (would it > be helpful to differentiate the "nothing built on top" and "some > commits have been built on top" cases, I wonder).
Hmm.. never thought of that subtlety. Differentiating should be possible. I'm just not sure if it would be helpful. Comments people, do or not do?
Show 17 quoted lines
> Also I wonder if you could do a bit more to help the users who do: > > $ git checkout $(git merge-base HEAD nd/branch-show-rebase-bisect-state) > > aka > > $ git checkout ...nd/branch-show-rebase-bisect-state > > and then do one or more commits on top. > > Instead of punting to "Currently not on any branch", would it help > to show the place you first detached at, so that the user can then > grab that commit object name and run > > $ git log --oneline $that_commit.. > > or something?
$that_commit would be HEAD@{-1} right? Should that be used instead of grabbing random SHA-1 shown in git-status?
>> +static int grab_1st_switch(unsigned char *osha1, unsigned char *nsha1, > > Can't this be done by generalizing grab_nth_branch_switch() and then > exposing it as part of the general API?
We could share the code, yes. Initially I wanted something that resolves "@{-1}" and gives me the reflog entry. Maybe we could add a function to do that.
> I also feel uneasy about two issues this and the previous change
I assume "previous change" is 1/5, reflog format change.
Show 15 quoted lines
> 2) The previous one records this sequence in a funny way: > > : start from branch A > git checkout B > git checkout C > > The resulting reflog entries result in > > checkout: moving from A to refs/heads/B > checkout: moving from B to refs/heads/C > > even though existing code and tools are expecting to read > > checkout: moving from A to B > checkout: moving from B to C
It does not record exactly user input, but the "to" part is basically extended sha-1 and I don't think current tools parse them manually. Instead just they should just pass them to git-rev-parse to do the job. So this change is not really bad. But again the recent '!' incident in attr.c shows that I know nothing about real world usage. We could do dwim when parsing the reflog, which introduces no changes in reflog format.
Show 16 quoted lines
> By the way, even though the title of this patch is "status: show the > ref that is checked out, even if it's detached", a quick check with > > $ cd ../linux-3.0 > $ git describe > v3.8-rc7 > $ ../git.git/git-checkout v3.8-rc7 > $ tail -n 1 .git/logs/HEAD | sed -e 's/.*checkout/checkout/' > checkout: moving from master to refs/tags/v3.8-rc7 > $ ../git.git/git-status | head -n 1 > # Not currently on any branch. > $ ../git.git/git-branch -v > * (no branch) 836dc9e Linux 3.8-rc7 > master 836dc9e Linux 3.8-rc7 > > does not seem to give me anything more helpful.
Yeah. I blame myself for copying from interpret_nth_prior_checkout() without full understanding, and git's poor documentation (there's no description about for_each_recent_reflog_ent, and that the caller is supposed to call for_each_reflog_ent in some case; but I think this convention is unintuitive, for_each_recent_reflog_ent should do that itself)
-- Duy