[RFC] Detached-HEAD reminder on commit?

Sometimes I work on a detached HEAD and then forget about it. If I then create some commits and checkout another branch, I have to dig through my reflog to find the older commits. I know that "git commit" adds has a "Not currently on any branch", but it's not very noticeable and also doesn't work when you specify a commit message on the command line.

I suggest to add some extra output to the STDOUT after a commit if we're on a detached HEAD. The quick patch below adds output like:

Vienna:git pieter$ ./git commit --allow-empty -m"test"
Created commit 6ce62c8b: test
You are on a detached head, so this commit has not been recorded in a branch.
If you don't want to lose this commit, checkout a branch and then run:
	git merge 6ce62c8bfcfb341106f3587d1c141c3955c2544c

Are there any comments to this / strong opinions against such a change?

- Pieter

tisdagen den 2 september 2008 21.31.45 skrev Pieter de Bie:

Probably doesn't hurt, but I think you should enable the git prompt. That will give you notice before committing.

-- robin

On Tue, 2 Sep 2008, Pieter de Bie wrote:

Nah.

I have nothing against the idea of an extra message, but there are other ways to preserve commits made on top of a detached head. So I'd keep only the first line.

Nicolas

Pieter de Bie <pdebie@ai.rug.nl> writes:

I'd say

+                       "If you don't want to lose this commit, run "
+                       "git branch <some-name>\n"
+			"to create a named branch for the commit you just made");

(or whatever better wording you find, but I think suggesting to name the branch makes more sense that merging it)

On Tue, 2 Sep 2008, Matthieu Moy wrote:

Agreed. Something that repeat more or less the message that was given when detaching HEAD in the first place would be best.

Nicolas

Pieter de Bie <pdebie@ai.rug.nl> writes:

Unconditionally doing this is too loud for my taste. You probably can do this in your post-commit hook.

Hi,

Junio C Hamano wrote:

[..]

Well, Pieter probably can do this in his post-commit hook. But I think this is useful for usability... especially for beginners who might not even know what a hook is. ;)

For me this felt too loud, too, especially since "git status" and "git commit" (without message option) already tells the user that she is on a detached HEAD. And "git commit -a" is usually done after a "git status", too, isn't it? (I do not use "git commit -a", I *use* the index.)

But nonetheless... Some days ago I accidentally detached my head (hihi) and at the end of the day, after switching and rebasing branches, I noticed that some commits in my branch were "lost"... Such a patch may have helped. (Yes, "git fsck --lost-found" and "git cherry-pick ..." helped.)

So I'm somewhere in between. I think this patch can be useful to minimize some annoyance for git users, but on the other hand the loud output can annoy people if they, for example, use git-commit in scripts (like git-rebase and git-rebase--interactive does).

Regards,
  Stephan

On Tuesday 02 September 2008, Stephan Beyer wrote:

I'm not sure I like this personally, but if we _really_ don't want newbies to shoot themselves in the foot, we could make "git commit" fail on a detached HEAD unless the user has indicated that s/he knows what's going on; i.e. something like this:

Vienna:git pieter$ ./git commit --allow-empty -m"test" You are on a detached head, so this commit would not be recorded in a branch. If you don't want to lose this commit, please switch to a (new) branch before committing. If you know what you're doing, and want to proceed on a detached HEAD, please enable commit.detached in your configuration (git config --global commit.detached true)

...but I sympathize with those that think this is overkill.

...Johan

On Tue, Sep 02, 2008 at 11:39:20PM +0200, Johan Herland wrote:

This was discussed to death when detached HEAD was introduced, and the decision was to go with the current behavior. Try looking in the list archives around December 2006 / January 2007 if you are truly masochistic.

-Peff

On Tue, Sep 02, 2008 at 05:44:28PM -0400, Jeff King wrote:

<gouges eyes with a rusty spoon>

Try http://thread.gmane.org/gmane.comp.version-control.git/35777

-Peff

On Tuesday 02 September 2008, Jeff King wrote:

Ok. Scratch that.

But what happened to the various suggestions in that original thread on adding a safety valve when _leaving_ the detached state (i.e. preventing the user from leaving their detached commits unreachable)?

It seems to have been suggested (in various forms) by several people and generally well-received in the original thread, but nothing seems to have come of it (at least nothing that has survived till today).

Apparently masochistic,

...Johan

Johan Herland <johan@herland.net> writes:

You are masochistic enough to have noticed that people were talking about the safety in the context of "HEAD lacks its own reflog"? Yes, we had reflogs on each refs/*, but HEAD itself did not have one.

The situation has changed --- HEAD has its own reflog these days, which not only helps this particular issue but is useful in contexts that never involves a detached HEAD.

On Wednesday 03 September 2008, Junio C Hamano wrote:

Yes, I noticed that...

...and that is certainly good, but I wonder if the people we want to protect from losing detached commits might not be aware of the existence and usage of reflogs (since we already assume they might not yet have fully grasped the branch concept). Even so, this would provide a golden opportunity to teach them about it. So, what about this:

When switching away from a detached HEAD that will cause it to become unreachable, we should warn the user of what is happening, and how to recover from this situation. I.e. something like:

  The commit you switched away from is not reachable from an existing
  ref, and will be deleted when its reflog entry expires (in $X days).
  See "git help reflog" for more information.
  If you want to keep this commit, you must bind it to a ref, for
  example by doing
    git branch <new-branch-name> $SHA1_SUM_HERE

Of course, this warning can be hidden by giving -q to git checkout.

Hmm?

Hopefully no longer masochistic,

...Johan

On Wed, Sep 03, 2008 at 09:45:08AM +0200, Johan Herland wrote:

Hrm. I thought we decided on a message like:

  Previous HEAD position was 1234abcd

when leaving the detached HEAD state, but it seems to have disappeared. Maybe with the move to builtin-checkout (sorry, I don't have time to bisect right at this second). Was that intentional?

-Peff

On Wed, Sep 03, 2008 at 09:15:07AM -0400, Jeff King wrote:

OK, I lied. I did have time to bisect it.

It never worked in builtin-checkout, and I am a bit suspicious of the code (and comment) below. Why would we not want to show such a message if moving to a branch (as long as it is not a _new_ branch)? The patch below makes more sense to me.

---

Jeff King wrote:

"isn't isn't"

Apart from the typo above, this patch makes a whole lot of sense.

On Wed, 3 Sep 2008, Jeff King wrote:

Good point. I think I confused myself with the new branch case. On the other hand, I think the "not starting a new branch" case should go as well. If you've got a detached HEAD, and you do:

$ git checkout -b foo origin/master

we probably ought to describe the old state. The reason that starting a new branch usually shouldn't give the message is that new->commit == old.commit (assuming that the defaults have gotten filled in by this point, which they should have).

(I think I included the new branch case to match the existing branch case; I'm not sure where I got the idea that switching to a branch shouldn't give the message. Hey, at least the comment makes it clear that I was actually trying to write the wrong code...)

Actually, the test for HEAD should be able to go, also, since checking out HEAD wouldn't change the current commit, although this doesn't matter to anything other than the complexity of the condition.

On Wed, Sep 03, 2008 at 12:49:43PM -0400, Daniel Barkalow wrote:

Right, I was thinking it was necessary for the starting a new branch case, but a better test is checking whether the commits are the same.

So based on what you said and thinking a bit, I came up with:

    if (!opts->quiet && !old.path && new->commit != old.commit)

and then I had the brilliant idea of checking what git-checkout.sh did. And sure enough:

    elif test -z "$oldbranch" && test "$new" != "$old"
    then
            describe_detached_head 'Previous HEAD position was' "$old"
    fi

Patch is below.

-- >8 -- checkout: fix message when leaving detached HEAD

The shell version of git checkout would print:

  Previous HEAD position was 1234abcd... commit subject line

when leaving a detached HEAD for another commit. Ths C version attempted to implement this, but got the condition wrong such that the behavior never triggered.

This patch simplifies the conditions for showing the message to the ones used by the shell version: any time we are leaving a detached HEAD and the new and old commits are not the same (this suppresses it for the "git checkout -b new" case recommended when you enter the detached state).

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin-checkout.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)

Jeff King <peff@peff.net> writes:

It was a good idea to keep the scripted ones in contrib/examples so that they are readily accessible to find out what we used to do ;-).

Thanks.  Will apply to 'maint'.

On Wed, Sep 03, 2008 at 12:36:05PM -0700, Junio C Hamano wrote:

Who's to say I didn't do:

  deleted=$(git log -1 --pretty=format:%H --diff-filter=D -- git-checkout.sh)
  git show ${deleted}^:git-checkout.sh

? :P

-Peff

PS Actually, I was about to look it up in the history when I remembered that we put it into contrib. :)

El 3/9/2008, a las 15:15, Jeff King escribió:

Mightn't "Previous HEAD position was 1234abcd (detached)" be even more helpful?

Cheers, Wincent

On Wed, Sep 03, 2008 at 04:11:32PM +0200, Wincent Colaiuta wrote:

The full message is actually:

    Previous HEAD position was 1234abcd... subject line of 1234abcd

I am not opposed to changing it, but I will let others decide on what it should say; my patch just reinstates the message itself (for which there was code, but broken code).

-Peff

On Wed, 3 Sep 2008, Johan Herland wrote:

It has been made unnecessary when the HEAD reflog started recording detached head states.

Nicolas

Johan Herland <johan@herland.net> writes:

If we _really_ don't want newbies to shoot themselves in the foot, we probably can issue a loud warning when they detach there HEAD.

Oh, wait,... we already do that.

On Tue, 2 Sep 2008, Johan Herland wrote:

This is going over board indeed.

Adding commits to a detached head is _cool_, and it is also _useful_ in many occasions. Let's not obfuscate that capability.

Adding an extra line of warning when the commit is done is fine, but more than that is too much IMHO.

Nicolas

On Tue, Sep 2, 2008 at 6:53 PM, Nicolas Pitre <nico@cam.org> wrote:

I think maybe we're looking at this the wrong way. The bad thing isn't committing to a detached HEAD; the bad thing is detaching the HEAD *by accident* in the first place.

Why do people (including me) spend so much time with a detached HEAD? I think it happens mainly for the following reasons:

1) Checking out a remote branch "git checkout origin/master" detaches
my HEAD, which is kind of bad, since it's such a common thing to want
to do.  And "git checkout -b master origin/master" is *not* actually
what I want to do, *most* of the time. What I actually want is for git
to remember that I'm on origin/master, but not let me change
origin/master, because it's a remote branch.  If I want to make
changes, I need to first make a topic branch, with "git checkout -b
topic".  git should prevent me from committing until I do.

2) git-rebase and git-am detach the HEAD while they work.  I think
this is fine, but: you shouldn't be able to *reattach* the HEAD
without first aborting the rebase or am operations.  When I've lost my
work, it's usually because I turned out to be in the middle of a
rebase or am and forgot about it, then I checked out another branch
and did some work, then ran git-rebase --abort, and oops! It moved me
somewhere else.  git should prevent me from switching branches when a
rebase or am is in progress.

3) git-submodule detaches and moves the HEAD of submodules
automatically.  This is a whole separate discussion :)

The remaining situations where someone is working on a detached HEAD (eg. checking out a particular commit, or actually implementing git-rebase like operations) seem to be pretty obviously *intentional*, and in that case, git should stay out of their way and let them do what they're doing.

I believe the reason this is such a hotly debated topic is that people confuse situations #1 and #2, and try to apply the same solution to both. But in situation #1, you want to be able to switch branches; in situation #2, you want to be able to commit. They are different situations, even though technically the fact that "I'm on a detached HEAD!" is the same.

Have fun,

Avery

"Avery Pennarun" <apenwarr@gmail.com> writes:

I do not think it is bad at all. The feature to detach HEAD was designed for that kind of usage. Start sightseeing, possibly futz with the code, and even create some snapshot commits, and then:

 * if it starts to take a usable shape, say "git checkout -b my-topic",
   from there, to give your exploration a lasting home; or
   
 * if it doesn't pan out, just discard it with "git checkout -f master"
   (or whatever you wanted to switch back to).

One thing that might help for downstream people would be to be able to say "I am making 'my-topic' branch out of a detached HEAD, but it really is meant to be a fork of origin/master that I detached my HEAD from, so please set up tracking for that one".

You could force people to say "git checkout -b my-topic origin/master" from the beginning, but that is very unreasonable and unworkable. When you are exploring, you more often than not do not know where your quest would lead to until spending some time. It is quite important to be able to delay the decision to create a local branch to keep what you did, and (more importantly) to be able to delay deciding what to name that topic.

Perhaps "git checkout -b my-topic" from a detached HEAD should inspect the HEAD reflog to see which remote (or local) branch you came from, and give that to the --track logic.

PS1='[\u@\h \W$(__git_ps1 " (%s)")]\$ ' would help here.

I do not have an objection to the general idea of forbidding people from switching branches out of detached HEAD while sequencer is in effect as a safety measure, but I didn't think through possible negative implications.

Absolutely. Being able to explore, without having to first decide what you are going to work on or what to name that branch, is a wonderful feature. It really lowers the barrier to explore.

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

So here is a patch for discussion, not heavily tested, but:

	$ git checkout origin/master
        $ git commit; hack hack hack ...
        $ git checkout --track -b mybranch

sequence should result in mybranch tracking the 'master' branch from the 'origin'.

The patch is just a proof of concept; doing this for HEAD reflog that is several megabytes long might take nontrivial amount of time (at least from performance standard of git); if we wanted to go this route, we should add an API to read the reflog entries from more recent to older.

 branch.c |   59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 59 insertions(+), 0 deletions(-)

On 2 sep 2008, at 23:05, Stephan Beyer wrote:

Exactly..I can fix this for myself, and when things go wrong I can also fix it, but there are people less with Git that don't understand what's going on.

The one in "git status" is somewhat noticeable with color highlighting, but the "git commit" one doesn't exactly stand out:

	# Please enter the commit message for your changes. Lines starting
	# with '#' will be ignored, and an empty message aborts the commit.
	# Not currently on any branch.
	# Untracked files:
	#   (use "git add <file>..." to include in what will be committed)

It's the third line, surrounded by other lines and at the bottom of the window.

How about a single line then, something like

	Vienna:git pieter$ ./git commit --allow-empty -m"test"
	Created commit 6ce62c8b: test
	Remember you are on a detached head, create a new branch to not lose  
these changes

Pieter de Bie wrote:

Perhaps instead of poposed patch we should simply put empty lines to emphasize that we are on no branch:

        # Please enter the commit message for your changes. Lines starting
        # with '#' will be ignored, and an empty message aborts the commit.
        #
        # Not currently on any branch.
        #
        # Untracked files:
        #   (use "git add <file>..." to include in what will be committed)

Jakub Narebski <jnareb@gmail.com> writes:

That sounds sensible, easy and safe enough.

 wt-status.c |   20 ++++++++++----------
 1 files changed, 10 insertions(+), 10 deletions(-)

On Tue, 2 Sep 2008, Junio C Hamano wrote:

Yep. And I would go as far as suggesting that the branch information be the first line of the lot:

# Not currently on any branch. # # Please enter the commit message for your changes. Lines starting # with '#' will be ignored, and an empty message aborts the commit. # # Untracked files: # (use "git add <file>..." to include in what will be committed) # [...]

Nicolas

On 2 sep 2008, at 23:51, Pieter de Bie wrote:

Or, as a final suggestion, something unintrusive like:

(on detached head)
	Vienna:git pieter$ ./git commit --allow-empty -m"test"
	Created commit 6ce62c8b on detached head: test
(on branch)
	Vienna:git pieter$ ./git commit --allow-empty -m"test"
	Created commit 6ce62c8b on branch 'master': test

- Pieter

This outputs the current branch on which a commit was created, just for reference. For example:

	Created commit 6d42875 on master: Fix submodule invalid command error

This also reminds the committer when he is on a detached HEAD:

	Created commit 02a7172 on detached HEAD: Also do this for 'git commit --amend'

Signed-off-by: Pieter de Bie <pdebie@ai.rug.nl>
---
 builtin-commit.c |   26 ++++++++++++++++++++++++--
 1 files changed, 24 insertions(+), 2 deletions(-)

Pieter de Bie <pdebie@ai.rug.nl> writes:

Given the recent "reminder" discussion, I suspect people without $PS1 set to show the current branch would like this, majority of others would be neutral, while some may actively hate it for cluttering the output even more. But I also suspect the initial annoyance the third camp may feel will pass rather quickly after they get used to seeing these.

Style.

	static char *get_commit_format_string(void)

Style.

	const char *head = ...

Isn't this function crafting a format string for format_commit_message()? What happens if your branch name has % in it?

API violation, I think; see strbuf_detach().

Style.

	char *format = ...

I somehow suspect it might be much simpler, more contained and robust if you:

 (1) chuck get_commit_format_string(), and leave all the existing code as-is;

 (2) format "%h: %s" into buf here;

 (3) call resolve_ref(HEAD) here to see if you are on detached HEAD (or
     otherwise what branch you are on) after (2),

 (4) find the first ':' in buf.buf and do your "on HEAD"/"on master"
     magic, using the result from (3).

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

Ah, that would not work, sorry, because this if() statement is only about an uninteresting case of --allow-empty. You need to do this the hard way and teach get_commit_format_string() quote % characters in branch names.

This adds a %% format which just prints a literal %

Signed-off-by: Pieter de Bie <pdebie@ai.rug.nl>
---

On 7 sep 2008, at 07:59, Junio C Hamano wrote:

Yes, here is patch for this. Sorry for the style issues. I also fixed the strbuf issue.

 Documentation/pretty-formats.txt |    1 +
 pretty.c                         |    3 +++
 2 files changed, 4 insertions(+), 0 deletions(-)

This outputs the current branch on which a commit was created, just for reference. For example:

	Created commit 6d42875 on master: Fix submodule invalid command error

This also reminds the committer when he is on a detached HEAD:

	Created commit 02a7172 on detached HEAD: Also do this for 'git commit --amend'

Signed-off-by: Pieter de Bie <pdebie@ai.rug.nl>
---
 builtin-commit.c |   32 ++++++++++++++++++++++++++++++--
 1 files changed, 30 insertions(+), 2 deletions(-)

On Sat, Sep 06, 2008 at 10:27:44PM -0700, Junio C Hamano wrote:

OK, I have lived with it for a little while, and I am still annoyed. ;)

My complaints are:

  1. It wastes more horizontal screen real estate, making it more likely
     that the line will wrap.

  2. In almost all of my projects (including git), I use the subject
     line convention of "subsystem: one line summary". So you end up
     with the visually confusing:

       Created commit abcd1234 on master: subsystem: one line summary

     which is even worse on a topic branch which is meaningful to the
     project:

       Created commit abcd1234 on widget: subwidget: one line summary

     which has literally left me scratching my head wondering why I put
     "widget" into the commit message.

Maybe it is better to simply break the line, which solves both problems. Something like:

  Created commit abcd1234 on master:
   subsystem: do some stuff
   1 files changes, 1 insertions(+), 0 deletions(-)

Trivial patch is below.

---

(Sorry for a late response)

On 21 sep 2008, at 12:42, Jeff King wrote:

How about something like

	Created commit abcd1234 on widget -- "subwidget: one line summary"

?

I don't like a multi-line approach.. I tried it myself, and the second line makes the first line easier to overlook

- Pieter

On Mon, Sep 29, 2008 at 10:09:17PM +0200, Pieter de Bie wrote:

I think that is probably just trading one visual problem for another. That is, there are other people will have the same problem with "--" that I had with ": ".

And of course it doesn't deal with the line length issues.

Anyway, I seem to be the only one complaining, so perhaps it should just be left as-is.

-Peff

Jeff King wrote:

Created 6207abc (subwidget: one quite long line of sum...) on <branch>

"commit" is just noise. Parentheses are often used to extemporize when using normal written language so it should work well here too.

On Tue, Sep 30, 2008 at 08:13:51AM +0200, Andreas Ericsson wrote:

I do like that better, and there is some precedent in the way we mention commits in emails (I know Junio even has an alias that formats it as $hash ($subject)).

By your "..." are you suggesting to truncate the subject?

-Peff

Jeff King wrote:

Yes, although that can be added later. I'm sending a format fix to the list in a minute or two.

Previously, we used to print something along the lines of

	Created commit abc9056 on master: Snib the sprock

but that output was sometimes confusing, as many projects use the "subsystem: message" style of commit subjects (just like this commit message does). When such improvements are done on topic-branches, it's not uncommon to name the topic-branch the same as the subsystem, leading to output like this:

	Created commit abc9056 on i386: i386: Snib the sprock

which doesn't look very nice and can be highly confusing. This patch alters the format so that the noise-word "commit" is dropped except when it makes the output read better and the commit subject is put inside parentheses. We also emphasize the detached case so that users do not overlook it in case the commit subject is long enough to extend to the next line. The end result looks thusly:

	normal case
	Created abc9056 (i386: Snib the sprock) on i386

	detached head
	Created DETACHED commit abc9056 (i386: Snib the sprock)

While we're at it, we rename "initial commit" to "root-commit" to align it with the argument to 'git log', producing this:

	initial commit
	Created root-commit abc9056 (i386: Snib the sprock) on i386

Documentation/gittutorial-2.txt is updated accordingly so that new users recognize what they're looking at.

Signed-off-by: Andreas Ericsson <ae@op5.se>
---

According to the few entries in the discussion about showing the branch we're on, this patch should probably go on top of next fairly soon.

If the code-change isn't accepted, let me know and I'll fix the documentation update to match whatever goes in builtin-commit.c.

Feel free to alter shouty-caps for detached when applying. I have no strong opinion either way, as I never commit on detached head anyway.

Thanks.

 Documentation/gittutorial-2.txt |   13 ++++++++-----
 builtin-commit.c                |   12 +++++-------
 2 files changed, 13 insertions(+), 12 deletions(-)

El 30/9/2008, a las 8:13, Andreas Ericsson escribió:

Excellent point on the noise. Independently of whether the branch info gets added the word "commit" should probably be dropped.

As far as long-line-wrapping goes, I don't really think this is a problem for Git to solve (by truncation or any other means); it's more of a user behaviour thing where one would hope that users would get into the habit of using concise subject lines and branch names.

Cheers, Wincent

On Tue, Sep 30, 2008 at 08:37:00AM +0200, Wincent Colaiuta wrote:

The branch info has already been added, if you count it being in next (in the form of "on $branch: ").

How concise must we be? I wrap my commit messages at 60 characters, which I consider quite conservative. But

  Created commit abcd1234 on jk/my-topic-branch:

takes up over half of an 80-column terminal. Is that a long branch name? Browsing "git log --grep=Merge.branch --pretty=format:%s origin/next" suggests it's not terribly out of line (at least by Junio's standards).

Dropping "commit " will help some. But given how much width is still used, and the fact that this message is really just to say "yes, I confirm that we just created the commit you asked for", I think truncating (with dots) to keep it within 80 characters is reasonable.

-Peff

Jeff King wrote:

I agree. Obvious solution is to do

subj_len = term_width - (strlen(cruft) + strlen(branch_name))

where strlen(cruft) is just 8 less if we drop 'commit ' from the cases. See the patch I just sent though. I sort of like that one.

Another way would be to write <branch>: Created <hash>: "subject line..."

As <hash> will very, very rarely match anything the user would put in his/her commit message themselves. Quoting the subject is probably a nice touch, and it can make sense to put it last as it's the least interesting of the things we print. Ah well. I'll just await commentary on the patch I've already sent before I go ahead and do something like that.

On Tue, Sep 30, 2008 at 11:59:25AM +0200, Andreas Ericsson wrote:

I think the difficulty is that the printing is sometimes done by our printf and sometimes by log_tree_commit, and there isn't a convenient way to hook into log_tree_commit to postprocess the formatted output.

I like it much better than what is on next (and I thought your commit message summed up the issue nicely), but...

I think I like this even better. My only concern is that many programs say "program: some error", so you could potentially have a confusing branch name. But I personally have never used a branch name that would cause such confusion.

Here is a patch for that format on top of next (the patch between this and what is in master is even more simple, since we are mostly removing Pieter's helper function).

---

Jeff King wrote:

Thanks. Feel free to recycle it :)

Me too, but I thought it up after I sent out the first patch. The nicest part is that the info that's always present will always end up in the same place, while my patch moves the branch-name around depending on the length of the subject line.

Let's agree here and now that the subject should be last and that "commit " should be dropped, at least for the normal cases.

A valid concern, certainly. We needn't use colons for the branch-name though, but could instead use some other delimiter, like this: [<branch>] Created <hash>: "subject line..." although I do believe we're close to nitpicking this issue to death now. It's not *that* important after all.

I don't quite like the fact that you're removing the "detached" thingie. I have coworkers that have been bitten by committing on detached head, so I'd like to have some mention of it. I'll rework it to take that into account. Otherwise, this looks good. Less code is always a good thing, imo.

Andreas Ericsson <ae@op5.se> wrote:

Actually I rather like the patch you submitted yesterday:

        normal case
        Created abc9056 (i386: Snib the sprock) on i386

        detached head
        Created DETACHED commit abc9056 (i386: Snib the sprock)

        initial commit
        Created root-commit abc9056 (i386: Snib the sprock) on i386

The detached HEAD and root-commit cases are clearly denoted at the very start of the line, where your eyes are likely to start scanning from first before you say "Doh, its just line noise because Git wants a pat on the back for doing what I asked". Thus you are likely to notice something out of the ordinary (commit on detached HEAD) pretty quick.

The "<branch>: Created <hash>: subject" format described above has the problem that a lot of errors look like "error: foo: bar" and the human eye is probably trained to glance over it. IMHO its formatted too much like an error message line.

Yup, its a bikeshed.

Right now I'm happy with your patch in next. I don't like taking the paint brush away from folks, but I also don't want to be applying a ton of commit message reformatting patches over the next week. ;-)

Shawn O. Pearce wrote:

Thanks for putting your foot down. Having thought more about it, I was quite disgusted to realize I spent brain-time on something so unimportant.

On Wed, Oct 01, 2008 at 08:10:11AM -0700, Shawn O. Pearce wrote:

I agree with your assumption that people scan the line from left to right, and that the most important stuff should come first. So that format covers _those_ cases, but not the case of "oops, I committed on a different branch than I intended." So I think it really makes sense to keep the branch name on the left side, and the commit subject last.

Welcome to maintainership. ;P

-Peff

Jeff King <peff@peff.net> wrote:

I briefly considered a format like this while replying above, but at prior day-job I used rather long branch names (sometimes with common prefixes) so it would truncate alot:

  normal case
  On i386           abc9056 (i386: Snib the sprock)

  detached head
  On DETACHED HEAD  abc9056 (i386: Snib the sprock)

  initial commit
  On i386
  Root commit       abc9056 (i386: Snib the sprock)

With the branch name field set at about 15 characters and truncated.
The initial commit case is very infrequent so burning two lines
for it to help make it stand out *and* make the branch name clear
isn't really a problem.
 

*sigh* And I just took up the #@*#@!&!*!@ paintbrush myself. Dammit. I'm putting it down now. Really. :-)

On Wed, Oct 01, 2008 at 08:36:37AM -0700, Shawn O. Pearce wrote:

I don't like this purely for the reason that it wastes horizontal space, which is one of the problems that started this discussion.

Also, there is no verb, which I think is worse. :)

Heh. I don't want to stir up trouble or drag you into a discussion you don't want to be in. But it seems like you are saying "OK, this is silly, let's just go with what is in next." But I think Andreas raised a good point about "stuff the user should check should go on the left" which is not consistent with what is in next. So I just want to confirm that you either disagree with that, or simply think it is not important enough to keep the discussion going.

-Peff

Jeff King <peff@peff.net> wrote:

I agree with "important stuff on the left". So as much as I'd like to just move on, I guess I'm willing to look at a 3rd patch that moves the branch name onto the left side.

Since we want the most important information furthest left while at the same time preserving valuable screen estate, we move the branch-name to the leftmost side of the commit result output. To make it read properly we get rid of "Created", which I just can't fit into a sentence without putting the branch-name last.

Having taken inspiration from the "git reset" command, output for the three conceivable cases now look thus:

  normal commit
  <branch> is now at b930c4a (i386: Snib the sprock)

  detached commit
  DETACHED HEAD is now at b930c4a (i386: Snib the sprock)

  initial commit
  History has begun anew. Root-commit created.
  <branch> is now at bc930c4a (i386: Snib the sprock)

As a nice side-effect, we can get rid of the get_commit_format helper function and thereby remove more code than we add.

This is a substantial rewrite of a patch originally sent by Jeff King <peff@peff.net>.

Signed-off-by: Andreas Ericsson <ae@op5.se>
---

"Created" is a problem when one wants to put branch-name before the subject line, because the subject has to follow the hash (it doesn't describe the pre-state of the branch/detached head), but the newly added commit. "Created, on branch, hash (subject)" just looks stilted and stupid, so I had to change it. Hopefully this can be accepted. If not, count me out.

I'm not sure if the last "else" case setting branch = head; can ever happen, but I figured it can't hurt to make sure. Feel free to modify commentary around it or the entire section when applying.

This is based on current next (798a2a426a).

 builtin-commit.c |   47 ++++++++++++++++++-----------------------------
 1 files changed, 18 insertions(+), 29 deletions(-)

On Wed, Oct 01, 2008 at 11:06:54PM +0200, Andreas Ericsson wrote:

All of the other proposals indicate the hash and subject as the object of creation. IOW, "created: <hash>: subject" or similar.

I think I still like your other proposal:

  [branch] created b930c4a: "i386: Snib the sprock"

better. But in the interests of just agreeing on something, I am willing to accept this. FWIW, the git-reset command doesn't use any delimiter for the message:

   <branch> is now at <hash> <subject>

So perhaps they should be the same. I don't think it overly matters.

You mentioned the shouty caps before. I think "detached HEAD" is probably caps enough, but not enough to argue for it (I just want to mention as an informal vote if Shawn wants to mark it up while applying).

Heh.

That was the reason for the helper function that was deleted. It actually created a format string like "Created %h on <branch>: %s" and properly escaped the percents in <branch>. So you would have to keep it if you wanted to interleave the data (but I think what you have is better -- the branch name or the detached status is the thing that should be first).

It should definitely be there, if only for the sanity of future expansion (and because I can technically put whatever ref I want into HEAD :) ).

This change is bogus. "release" frees a strbuf. "detach" says "Give me the buffer, and I will take care of freeing it later myself". So you introduced a leak.

-Peff

On Wed, Oct 01, 2008 at 06:06:04PM -0400, Jeff King wrote:

And here is the patch, since it was sitting uncommitted in my working tree. Feel free to ignore.

BTW, we should apply _something_ since what is currently in next has a bug: it lacks a space between "DETACHED commit" and the hash:

  Created DETACHED commit4fde0d0 (subject line)

-- >8 -- reformat informational commit message

When committing, we print a message like:

  Created [DETACHED commit] <hash> (<subject>) on <branch>

The most useful bit of information there (besides the detached status, if it is present) is which branch you made the commit on. However, it is sometimes hard to see because the subject dominates the line.

Instead, let's put the most useful information (detached status and commit branch) on the far left, with the subject (which is least likely to be interesting) on the far right.

We'll use brackets to offset the branch name so the line is not mistaken for an error line of the form "program: some sort of error". E.g.,:

  [jk/bikeshed] created bd8098f: "reformat informational commit message"
---
 builtin-commit.c |   37 ++++++++++---------------------------
 1 files changed, 10 insertions(+), 27 deletions(-)

Jeff King wrote:

No sign-off.

Personally, I'm not overly fond of things like
   something ? yay : nay_but_try ? worked_now : still_no_go
since I find them hard to read without thinking a lot.

On Thu, Oct 02, 2008 at 07:40:28AM +0200, Andreas Ericsson wrote:

Sorry, mistakenly omitted.

Signed-off-by: Jeff King <peff@peff.net>

Hmm, I find them more readable. :) And often easier to visually see that no matter what happens, the result has _some_ value (whereas with if/else, you have to make sure that all branchs set the value). But I am happy to change it to:

  const char *branch;
  ...

  if (!prefixcmp(head, "refs/heads/"))
          branch = head + 11;
  else if (!strcmp(head, "HEAD"))
          branch = "detached HEAD";
  else
          branch = head;

However, I found your mail somewhat unexpected. Rather than comments on the code, I expected rather "yes, I do like this better" or "no, I think we should go with the other one." But maybe you are just sick of weighing in. ;)

-Peff

Jeff King <peff@peff.net> wrote:

I think painting is over for now. Time to let the paint dry. I applied Jeff's patch:

  [jk/bikeshed] created bd8098f: "reformat informational commit message"

On Thu, Oct 02, 2008 at 05:15:56PM -0700, Shawn O. Pearce wrote:

Woo! Victory by attrition!

-Peff

Jeff King <peff@peff.net> wrote:

I think the hard part now is to get the user docs updated to reflect the new format. We need to get that done before this can merge over to master.

On Fri, Oct 03, 2008 at 07:09:52AM -0700, Shawn O. Pearce wrote:

Grepping only turned up the two instances that Andreas had changed for his patch, so I think that is probably it. Here's the patch.

-- >8 -- tutorial: update output of git commit

Commit c85db254 changed the format of the message produced by "git commit" when creating a commit. This patch updates the example session in the tutorial to the new format.

It also adds in the missing diffstat summary lines, which should have been added long ago.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/gittutorial-2.txt |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

El 2/10/2008, a las 0:06, Jeff King escribió:

If you're wanting to trim horizontal fat then the "is" isn't really required.

<branch> now at <hash> <subject>

Reads just as well.

Cheers, Wincent

On Wed, Oct 01, 2008 at 10:13:34AM +0200, Andreas Ericsson wrote:

Yes, I agree that those are the most important aspects, and the rest of it is just minor formatting details.

Heh. Yes, I feel a little silly discussing this so much. But it probably is _the_ most frequently seen informational message in git. So not only does it affect new users' perception of git, but I have to see it every day. ;)

I thought the all-caps "HEAD" instead of the branch name would make it stand out. But given that it isn't the common case, I don't think it is the end of the world to spend a little more screen real estate mentioning it.

I am ready to bikeshed the next patch you produce. ;)

-Peff