[RFC] Detached-HEAD reminder on commit?

[RFC] Detached-HEAD reminder on commit?

[Pieter de Bie]

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

diff --git a/builtin-commit.c b/builtin-commit.c
index ec65ac5..bfe25f9 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -845,6 +845,8 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
 {
        struct rev_info rev;
        struct commit *commit;
+       unsigned char sha[20];
+       const char* head = resolve_ref("HEAD", sha, 0, NULL);
 
        commit = lookup_commit(sha1);
        if (!commit)
@@ -877,6 +879,15 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
                printf("%s\n", buf.buf);
                strbuf_release(&buf);
        }
+
+       /* Are we on a detached HEAD? */
+       if (!strcmp("HEAD", head))
+               printf("You are on a detached head, so this commit "
+                       "has not been recorded in a branch.\n"
+                       "If you don't want to lose this commit, checkout a "
+                       "branch and then run:\n"
+                       "       git merge %s\n", sha1_to_hex(sha1));
+
 }
 
 static int git_commit_config(const char *k, const char *v, void *cb)

Re: [RFC] Detached-HEAD reminder on commit?

[Robin Rosenberg]

tisdagen den 2 september 2008 21.31.45 skrev Pieter de Bie:
> 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?

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

-- robin

Re: [RFC] Detached-HEAD reminder on commit?

[Nicolas Pitre]

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

> 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

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

Re: [RFC] Detached-HEAD reminder on commit?

[Matthieu Moy]

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

> +       if (!strcmp("HEAD", head))
> +               printf("You are on a detached head, so this commit "
> +                       "has not been recorded in a branch.\n"
> +                       "If you don't want to lose this commit, checkout a "
> +                       "branch and then run:\n"
> +                       "       git merge %s\n", sha1_to_hex(sha1));

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)

-- 
Matthieu

Re: [RFC] Detached-HEAD reminder on commit?

[Nicolas Pitre]

On Tue, 2 Sep 2008, Matthieu Moy wrote:

> Pieter de Bie <pdebie@ai.rug.nl> writes:
> 
> > +       if (!strcmp("HEAD", head))
> > +               printf("You are on a detached head, so this commit "
> > +                       "has not been recorded in a branch.\n"
> > +                       "If you don't want to lose this commit, checkout a "
> > +                       "branch and then run:\n"
> > +                       "       git merge %s\n", sha1_to_hex(sha1));
> 
> 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)

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


Nicolas

Re: [RFC] Detached-HEAD reminder on commit?

[Junio C Hamano]

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

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

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

Re: [RFC] Detached-HEAD reminder on commit?

[Stephan Beyer]

Hi,

Junio C Hamano wrote:
> Pieter de Bie <pdebie@ai.rug.nl> writes:
> 
[..]
> > 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?
> 
> Unconditionally doing this is too loud for my taste.  You probably can do
> this in your post-commit hook.

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

-- 
Stephan Beyer <s-beyer@gmx.net>, PGP 0x6EDDD207FCC5040F

Re: [RFC] Detached-HEAD reminder on commit?

[Johan Herland]

On Tuesday 02 September 2008, Stephan Beyer wrote:
> Junio C Hamano wrote:
> > Pieter de Bie <pdebie@ai.rug.nl> writes:
> > > 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?
> >
> > Unconditionally doing this is too loud for my taste.  You probably can
> > do this in your post-commit hook.
>
> 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. ;)

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

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [RFC] Detached-HEAD reminder on commit?

[Jeff King]

On Tue, Sep 02, 2008 at 11:39:20PM +0200, Johan Herland 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:

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

Re: [RFC] Detached-HEAD reminder on commit?

[Jeff King]

On Tue, Sep 02, 2008 at 05:44:28PM -0400, Jeff King 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.

<gouges eyes with a rusty spoon>

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

-Peff

Re: [RFC] Detached-HEAD reminder on commit?

[Johan Herland]

On Tuesday 02 September 2008, Jeff King wrote:
> On Tue, Sep 02, 2008 at 11:39:20PM +0200, Johan Herland 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:
>
> 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.

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>
www.herland.net

Re: [RFC] Detached-HEAD reminder on commit?

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

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

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.

Re: [RFC] Detached-HEAD reminder on commit?

[Johan Herland]

On Wednesday 03 September 2008, Junio C Hamano wrote:
> Johan Herland <johan@herland.net> writes:
> > 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).
>
> 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.

Yes, I noticed that...

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

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

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [RFC] Detached-HEAD reminder on commit?

[Jeff King]

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

> 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)?

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

Re: [RFC] Detached-HEAD reminder on commit?

[Jeff King]

On Wed, Sep 03, 2008 at 09:15:07AM -0400, Jeff King 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?

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.

---
diff --git a/builtin-checkout.c b/builtin-checkout.c
index b380ad6..b2c7d3c 100644
--- a/builtin-checkout.c
+++ b/builtin-checkout.c
@@ -386,12 +386,12 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)
 	}
 
 	/*
-	 * If the new thing isn't a branch and isn't HEAD and we're
+	 * If the new thing isn't isn't HEAD and we're
 	 * not starting a new branch, and we want messages, and we
 	 * weren't on a branch, and we're moving to a new commit,
 	 * describe the old commit.
 	 */
-	if (!new->path && strcmp(new->name, "HEAD") && !opts->new_branch &&
+	if (strcmp(new->name, "HEAD") && !opts->new_branch &&
 	    !opts->quiet && !old.path && new->commit != old.commit)
 		describe_detached_head("Previous HEAD position was", old.commit);
 

Re: [RFC] Detached-HEAD reminder on commit?

[Andreas Ericsson]

Jeff King wrote:
> On Wed, Sep 03, 2008 at 09:15:07AM -0400, Jeff King 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?
> 
> 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.
> 
> ---
> diff --git a/builtin-checkout.c b/builtin-checkout.c
> index b380ad6..b2c7d3c 100644
> --- a/builtin-checkout.c
> +++ b/builtin-checkout.c
> @@ -386,12 +386,12 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)
>  	}
>  
>  	/*
> -	 * If the new thing isn't a branch and isn't HEAD and we're
> +	 * If the new thing isn't isn't HEAD and we're

"isn't isn't"

>  	 * not starting a new branch, and we want messages, and we
>  	 * weren't on a branch, and we're moving to a new commit,
>  	 * describe the old commit.
>  	 */

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

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Re: [RFC] Detached-HEAD reminder on commit?

[Daniel Barkalow]

On Wed, 3 Sep 2008, Jeff King wrote:

> On Wed, Sep 03, 2008 at 09:15:07AM -0400, Jeff King 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?
> 
> 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.

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.

> 
> ---
> diff --git a/builtin-checkout.c b/builtin-checkout.c
> index b380ad6..b2c7d3c 100644
> --- a/builtin-checkout.c
> +++ b/builtin-checkout.c
> @@ -386,12 +386,12 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)
>  	}
>  
>  	/*
> -	 * If the new thing isn't a branch and isn't HEAD and we're
> +	 * If the new thing isn't isn't HEAD and we're
>  	 * not starting a new branch, and we want messages, and we
>  	 * weren't on a branch, and we're moving to a new commit,
>  	 * describe the old commit.
>  	 */
> -	if (!new->path && strcmp(new->name, "HEAD") && !opts->new_branch &&
> +	if (strcmp(new->name, "HEAD") && !opts->new_branch &&
>  	    !opts->quiet && !old.path && new->commit != old.commit)
>  		describe_detached_head("Previous HEAD position was", old.commit);
>  
> 

Re: [RFC] Detached-HEAD reminder on commit?

[Jeff King]

On Wed, Sep 03, 2008 at 12:49:43PM -0400, Daniel Barkalow 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).

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(-)

diff --git a/builtin-checkout.c b/builtin-checkout.c
index b380ad6..efdb1e0 100644
--- a/builtin-checkout.c
+++ b/builtin-checkout.c
@@ -386,13 +386,11 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)
 	}
 
 	/*
-	 * If the new thing isn't a branch and isn't HEAD and we're
-	 * not starting a new branch, and we want messages, and we
-	 * weren't on a branch, and we're moving to a new commit,
-	 * describe the old commit.
+	 * If we were on a detached HEAD, but we are now moving to
+	 * a new commit, we want to mention the old commit once more
+	 * to remind the user that it might be lost.
 	 */
-	if (!new->path && strcmp(new->name, "HEAD") && !opts->new_branch &&
-	    !opts->quiet && !old.path && new->commit != old.commit)
+	if (!opts->quiet && !old.path && new->commit != old.commit)
 		describe_detached_head("Previous HEAD position was", old.commit);
 
 	if (!old.commit) {
-- 
1.6.0.1.208.gcc04.dirty

Re: [RFC] Detached-HEAD reminder on commit?

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Sep 03, 2008 at 12:49:43PM -0400, Daniel Barkalow 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).
>
> 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.

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

Re: [RFC] Detached-HEAD reminder on commit?

[Jeff King]

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

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

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

Re: [RFC] Detached-HEAD reminder on commit?

[Wincent Colaiuta]

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

> On Wed, Sep 03, 2008 at 09:45:08AM +0200, Johan Herland wrote:
>
>> 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)?
>
> 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.

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

Cheers,
Wincent

Re: [RFC] Detached-HEAD reminder on commit?

[Jeff King]

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

>>  Previous HEAD position was 1234abcd
>>
>> when leaving the detached HEAD state, but it seems to have disappeared.
>
> Mightn't "Previous HEAD position was 1234abcd (detached)" be even more  
> helpful?

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

Re: [RFC] Detached-HEAD reminder on commit?

[Nicolas Pitre]

On Wed, 3 Sep 2008, Johan Herland wrote:

> On Tuesday 02 September 2008, Jeff King wrote:
> > On Tue, Sep 02, 2008 at 11:39:20PM +0200, Johan Herland 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:
> >
> > 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.
> 
> 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 has been made unnecessary when the HEAD reflog started recording 
detached head states.


Nicolas

Re: [RFC] Detached-HEAD reminder on commit?

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> On Tuesday 02 September 2008, Stephan Beyer wrote:
>> Junio C Hamano wrote:
>> > Pieter de Bie <pdebie@ai.rug.nl> writes:
>> > > 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?
>> >
>> > Unconditionally doing this is too loud for my taste.  You probably can
>> > do this in your post-commit hook.
>>
>> 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. ;)
>
> 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:

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.

Re: [RFC] Detached-HEAD reminder on commit?

[Nicolas Pitre]

On Tue, 2 Sep 2008, Johan Herland wrote:

> On Tuesday 02 September 2008, Stephan Beyer wrote:
> > Junio C Hamano wrote:
> > > Pieter de Bie <pdebie@ai.rug.nl> writes:
> > > > 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?
> > >
> > > Unconditionally doing this is too loud for my taste.  You probably can
> > > do this in your post-commit hook.
> >
> > 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. ;)
> 
> 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.

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

Re: [RFC] Detached-HEAD reminder on commit?

[Avery Pennarun]

On Tue, Sep 2, 2008 at 6:53 PM, Nicolas Pitre <nico@cam.org> wrote:
> On Tue, 2 Sep 2008, Johan Herland wrote:
>> 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.
>
> 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.

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

Re: [RFC] Detached-HEAD reminder on commit?

[Junio C Hamano]

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

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

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.

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

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.

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

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.

Re: [RFC] Detached-HEAD reminder on commit?

[Junio C Hamano]

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

> "Avery Pennarun" <apenwarr@gmail.com> writes:
> ...
>> 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.
>
> 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.

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(-)

diff --git c/branch.c w/branch.c
index b1e59f2..2ec6418 100644
--- c/branch.c
+++ w/branch.c
@@ -48,6 +48,62 @@ static int should_setup_rebase(const struct tracking *tracking)
 }
 
 /*
+ * A branch is created out of "HEAD" and we would want tracking;
+ * go back the reflog to figure out where we really came from.
+ */
+static int refine_head_one(const char *name, size_t namelen,
+			   struct strbuf *found_ref)
+{
+	char *real_ref;
+	unsigned char sha1[20];
+	if (dwim_ref(name, namelen, sha1, &real_ref) != 1)
+		return 0;
+	strbuf_reset(found_ref);
+	strbuf_addstr(found_ref, real_ref);
+	return 0;
+}
+
+static int one_head_ent(unsigned char *osha1, unsigned char *nsha1,
+			const char *ident, unsigned long timestamp, int zone,
+			const char *message, void *cbdata)
+{
+	/*
+	 * Look for signs of HEAD coming from elsewhere.
+	 *
+	 * "checkout: moving from %*s to %s" done by "git checkout"
+	 * "%s: updating HEAD" done by "git reset"
+	 */
+	struct strbuf *found_ref = cbdata;
+	char *cp;
+	size_t len;
+
+	if (!prefixcmp(message, "checkout: moving from ")) {
+		cp = strstr(message, " to ");
+		if (!cp)
+			return 0;
+		cp += 4;
+		len = strlen(cp);
+		if (cp[len-1] == '\n')
+			len--;
+		return refine_head_one(cp, len, found_ref);
+	}
+
+	cp = strstr(message, ": updating HEAD");
+	if (cp && !cp[15])
+		return refine_head_one(message, cp - message, found_ref);
+	return 0;
+}
+
+static const char *refine_head_ref(void)
+{
+	struct strbuf found = STRBUF_INIT;
+
+	strbuf_addstr(&found, "HEAD");
+	for_each_reflog_ent("HEAD", one_head_ent, &found);
+	return strbuf_detach(&found, NULL);
+}
+
+/*
  * This is called when new_ref is branched off of orig_ref, and tries
  * to infer the settings for branch.<new_ref>.{remote,merge} from the
  * config.
@@ -58,6 +114,9 @@ static int setup_tracking(const char *new_ref, const char *orig_ref,
 	char key[1024];
 	struct tracking tracking;
 
+	if (!strcmp(orig_ref, "HEAD"))
+		orig_ref = refine_head_ref();
+
 	if (strlen(new_ref) > 1024 - 7 - 7 - 1)
 		return error("Tracking not set up: name too long: %s",
 				new_ref);

Re: [RFC] Detached-HEAD reminder on commit?

[Pieter de Bie]

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

> Junio C Hamano wrote:
>> Pieter de Bie <pdebie@ai.rug.nl> writes:
>>
> [..]
>>> 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?
>>
>> Unconditionally doing this is too loud for my taste.  You probably  
>> can do
>> this in your post-commit hook.
>
> 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. ;)

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.

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


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

Re: [RFC] Detached-HEAD reminder on commit?

[Jakub Narebski]

Pieter de Bie wrote:

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

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
Warsaw, Poland
ShadeHawk on #git

Re: [RFC] Detached-HEAD reminder on commit?

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

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

That sounds sensible, easy and safe enough.


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

diff --git i/wt-status.c w/wt-status.c
index 889e50f..97076e8 100644
--- i/wt-status.c
+++ w/wt-status.c
@@ -342,17 +342,17 @@ void wt_status_print(struct wt_status *s)
 
 	s->is_initial = get_sha1(s->reference, sha1) ? 1 : 0;
 	if (s->branch) {
-		const char *on_what = "On branch ";
-		const char *branch_name = s->branch;
-		if (!prefixcmp(branch_name, "refs/heads/"))
-			branch_name += 11;
-		else if (!strcmp(branch_name, "HEAD")) {
-			branch_name = "";
-			branch_color = color(WT_STATUS_NOBRANCH);
-			on_what = "Not currently on any branch.";
+		if (!prefixcmp(s->branch, "refs/heads/")) {
+			color_fprintf(s->fp, color(WT_STATUS_HEADER), "# ");
+			color_fprintf_ln(s->fp, branch_color,
+					 "On branch %s", s->branch + 11);
+		} else if (!strcmp(s->branch, "HEAD")) {
+			color_fprintf_ln(s->fp, color(WT_STATUS_HEADER), "#");
+			color_fprintf(s->fp, color(WT_STATUS_HEADER), "# ");
+			color_fprintf_ln(s->fp, color(WT_STATUS_NOBRANCH),
+					 "Not currently on any branch.");
+			color_fprintf_ln(s->fp, color(WT_STATUS_HEADER), "#");
 		}
-		color_fprintf(s->fp, color(WT_STATUS_HEADER), "# ");
-		color_fprintf_ln(s->fp, branch_color, "%s%s", on_what, branch_name);
 		if (!s->is_initial)
 			wt_status_print_tracking(s);
 	}

Re: [RFC] Detached-HEAD reminder on commit?

[Nicolas Pitre]

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

> Jakub Narebski <jnareb@gmail.com> writes:
> 
> > 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)
> 
> That sounds sensible, easy and safe enough.

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

Re: [RFC] Detached-HEAD reminder on commit?

[Pieter de Bie]

On 2 sep 2008, at 23:51, Pieter de Bie wrote:
>
> 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

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

[PATCH] Builtin-commit: show on which branch a commit was added

[Pieter de Bie]

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(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index 8165bb3..a82483d 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -878,10 +878,31 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 	return commitable ? 0 : 1;
 }
 
+static char* get_commit_format_string()
+{
+	unsigned char sha[20];
+	const char* head = resolve_ref("HEAD", sha, 0, NULL);
+	struct strbuf buf = STRBUF_INIT;
+
+	strbuf_addstr(&buf, "format:%h");
+
+	/* Are we on a detached HEAD? */
+	if (!strcmp("HEAD", head))
+		strbuf_addstr(&buf, " on detached HEAD");
+	else if (!prefixcmp(head, "refs/heads/")) {
+		strbuf_addstr(&buf, " on ");
+		strbuf_addstr(&buf, head + 11);
+	}
+	strbuf_addstr(&buf, ": %s");
+
+	return buf.buf;
+}
+
 static void print_summary(const char *prefix, const unsigned char *sha1)
 {
 	struct rev_info rev;
 	struct commit *commit;
+	char* format = get_commit_format_string();
 
 	commit = lookup_commit(sha1);
 	if (!commit)
@@ -899,7 +920,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
 
 	rev.verbose_header = 1;
 	rev.show_root_diff = 1;
-	get_commit_format("format:%h: %s", &rev);
+	get_commit_format(format, &rev);
 	rev.always_show_header = 0;
 	rev.diffopt.detect_rename = 1;
 	rev.diffopt.rename_limit = 100;
@@ -910,10 +931,11 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
 
 	if (!log_tree_commit(&rev, commit)) {
 		struct strbuf buf = STRBUF_INIT;
-		format_commit_message(commit, "%h: %s", &buf, DATE_NORMAL);
+		format_commit_message(commit, format + 7, &buf, DATE_NORMAL);
 		printf("%s\n", buf.buf);
 		strbuf_release(&buf);
 	}
+	free(format);
 }
 
 static int git_commit_config(const char *k, const char *v, void *cb)
-- 
1.6.0.1.346.g880d9.dirty

Re: [PATCH] Builtin-commit: show on which branch a commit was added

[Junio C Hamano]

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

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

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.

> diff --git a/builtin-commit.c b/builtin-commit.c
> index 8165bb3..a82483d 100644
> --- a/builtin-commit.c
> +++ b/builtin-commit.c
> @@ -878,10 +878,31 @@ int cmd_status(int argc, const char **argv, const char *prefix)
>  	return commitable ? 0 : 1;
>  }
>  
> +static char* get_commit_format_string()

Style.

	static char *get_commit_format_string(void)

> +{
> +	unsigned char sha[20];
> +	const char* head = resolve_ref("HEAD", sha, 0, NULL);

Style.

	const char *head = ...

> ...
> +	else if (!prefixcmp(head, "refs/heads/")) {
> +		strbuf_addstr(&buf, " on ");
> +		strbuf_addstr(&buf, head + 11);

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

> +	}
> +	strbuf_addstr(&buf, ": %s");
> +
> +	return buf.buf;

API violation, I think; see strbuf_detach().

> +}
> +
>  static void print_summary(const char *prefix, const unsigned char *sha1)
>  {
>  	struct rev_info rev;
>  	struct commit *commit;
> +	char* format = get_commit_format_string();

Style.

	char *format = ...

> @@ -910,10 +931,11 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
>  
>  	if (!log_tree_commit(&rev, commit)) {
>  		struct strbuf buf = STRBUF_INIT;
> -		format_commit_message(commit, "%h: %s", &buf, DATE_NORMAL);
> +		format_commit_message(commit, format + 7, &buf, DATE_NORMAL);
> 		printf("%s\n", buf.buf);
> 		strbuf_release(&buf);

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

Re: [PATCH] Builtin-commit: show on which branch a commit was added

[Junio C Hamano]

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

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

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.

[PATCH 1/2] pretty.c: add %% format specifier

[Pieter de Bie]

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:
>Junio C Hamano <gitster@pobox.com> writes:
>>I somehow suspect it might be much simpler, more contained and robust if you:
>
>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.

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(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index f18d33e..b91db86 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -127,6 +127,7 @@ The placeholders are:
 - '%m': left, right or boundary mark
 - '%n': newline
 - '%x00': print a byte from a hex code
+- '%%': print a literal '%'
 
 * 'tformat:'
 +
diff --git a/pretty.c b/pretty.c
index 8beafa0..f66d687 100644
--- a/pretty.c
+++ b/pretty.c
@@ -538,6 +538,9 @@ static size_t format_commit_item(struct strbuf *sb, const char *placeholder,
 			return 3;
 		} else
 			return 0;
+	case '%':
+		strbuf_addch(sb, '%');
+		return 1;
 	}
 
 	/* these depend on the commit */
-- 
1.6.0.1.346.g880d9.dirty

[PATCH 2/2] builtin-commit: show on which branch a commit was added

[Pieter de Bie]

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(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index 8165bb3..47b76e6 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -878,10 +878,37 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 	return commitable ? 0 : 1;
 }
 
+static char *get_commit_format_string(void)
+{
+	unsigned char sha[20];
+	const char *head = resolve_ref("HEAD", sha, 0, NULL);
+	struct strbuf buf = STRBUF_INIT;
+
+	strbuf_addstr(&buf, "format:%h");
+
+	/* Are we on a detached HEAD? */
+	if (!strcmp("HEAD", head))
+		strbuf_addstr(&buf, " on detached HEAD");
+	else if (!prefixcmp(head, "refs/heads/")) {
+		const char *cp;
+		strbuf_addstr(&buf, " on ");
+		for (cp = head + 11; *cp; cp++) {
+			if (*cp == '%')
+				strbuf_addstr(&buf, "%%");
+			else
+				strbuf_addch(&buf, *cp);
+		}
+	}
+	strbuf_addstr(&buf, ": %s");
+
+	return strbuf_detach(&buf, NULL);
+}
+
 static void print_summary(const char *prefix, const unsigned char *sha1)
 {
 	struct rev_info rev;
 	struct commit *commit;
+	char *format = get_commit_format_string();
 
 	commit = lookup_commit(sha1);
 	if (!commit)
@@ -899,7 +926,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
 
 	rev.verbose_header = 1;
 	rev.show_root_diff = 1;
-	get_commit_format("format:%h: %s", &rev);
+	get_commit_format(format, &rev);
 	rev.always_show_header = 0;
 	rev.diffopt.detect_rename = 1;
 	rev.diffopt.rename_limit = 100;
@@ -910,10 +937,11 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
 
 	if (!log_tree_commit(&rev, commit)) {
 		struct strbuf buf = STRBUF_INIT;
-		format_commit_message(commit, "%h: %s", &buf, DATE_NORMAL);
+		format_commit_message(commit, format + 7, &buf, DATE_NORMAL);
 		printf("%s\n", buf.buf);
 		strbuf_release(&buf);
 	}
+	free(format);
 }
 
 static int git_commit_config(const char *k, const char *v, void *cb)
-- 
1.6.0.1.346.g880d9.dirty

Re: [PATCH] Builtin-commit: show on which branch a commit was added

[Jeff King]

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

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

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.

---
diff --git a/builtin-commit.c b/builtin-commit.c
index 917f638..53dcde6 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -899,7 +899,7 @@ static char *get_commit_format_string(void)
 				strbuf_addch(&buf, *cp);
 		}
 	}
-	strbuf_addstr(&buf, ": %s");
+	strbuf_addstr(&buf, ":%n %s");
 
 	return strbuf_detach(&buf, NULL);
 }

Re: [PATCH] Builtin-commit: show on which branch a commit was added

[Pieter de Bie]

(Sorry for a late response)

On 21 sep 2008, at 12:42, Jeff King 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.

How about something like

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

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

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

- Pieter

Re: [PATCH] Builtin-commit: show on which branch a commit was added

[Jeff King]

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

> How about something like
>
> 	Created commit abcd1234 on widget -- "subwidget: one line summary"

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

Re: [PATCH] Builtin-commit: show on which branch a commit was added

[Andreas Ericsson]

Jeff King wrote:
> On Mon, Sep 29, 2008 at 10:09:17PM +0200, Pieter de Bie wrote:
> 
>> How about something like
>>
>> 	Created commit abcd1234 on widget -- "subwidget: one line summary"
> 
> 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 ": ".
> 

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.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Re: [PATCH] Builtin-commit: show on which branch a commit was added

[Jeff King]

On Tue, Sep 30, 2008 at 08:13:51AM +0200, Andreas Ericsson 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.

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

Re: [PATCH] Builtin-commit: show on which branch a commit was added

[Andreas Ericsson]

Jeff King wrote:
> On Tue, Sep 30, 2008 at 08:13:51AM +0200, Andreas Ericsson 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.
> 
> 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?
> 

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

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

[PATCH] git commit: Reformat output somewhat

[Andreas Ericsson]

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(-)

diff --git a/Documentation/gittutorial-2.txt b/Documentation/gittutorial-2.txt
index 6609046..8484e7a 100644
--- a/Documentation/gittutorial-2.txt
+++ b/Documentation/gittutorial-2.txt
@@ -32,22 +32,25 @@ Initialized empty Git repository in .git/
 $ echo 'hello world' > file.txt
 $ git add .
 $ git commit -a -m "initial commit"
-Created initial commit 54196cc2703dc165cbd373a65a4dcf22d50ae7f7
+Created root-commit 54196cc (initial commit) on master
  create mode 100644 file.txt
 $ echo 'hello world!' >file.txt
 $ git commit -a -m "add emphasis"
-Created commit c4d59f390b9cfd4318117afde11d601c1085f241
+Created c4d59f3 (add emphasis) on master
 ------------------------------------------------
 
-What are the 40 digits of hex that git responded to the commit with?
+What are the 7 digits of hex that git responded to the commit with?
 
 We saw in part one of the tutorial that commits have names like this.
 It turns out that every object in the git history is stored under
-such a 40-digit hex name.  That name is the SHA1 hash of the object's
+a 40-digit hex name.  That name is the SHA1 hash of the object's
 contents; among other things, this ensures that git will never store
 the same data twice (since identical data is given an identical SHA1
 name), and that the contents of a git object will never change (since
-that would change the object's name as well).
+that would change the object's name as well). The 7 char hex strings
+here are simply the abbreviation of such 40 character long strings.
+Abbreviations can be used everywhere where the 40 character strings
+can be used, so long as they are unambiguous.
 
 It is expected that the content of the commit object you created while
 following the example above generates a different SHA1 hash than
diff --git a/builtin-commit.c b/builtin-commit.c
index 161128b..f0765cc 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -884,12 +884,11 @@ static char *get_commit_format_string(void)
 	const char *head = resolve_ref("HEAD", sha, 0, NULL);
 	struct strbuf buf = STRBUF_INIT;
 
-	strbuf_addstr(&buf, "format:%h");
+	/* use shouty-caps if we're on detached HEAD */
+	strbuf_addf(&buf, "format:%s", strcmp("HEAD", head) ? "" : "DETACHED commit");
+	strbuf_addstr(&buf, "%h (%s)");
 
-	/* Are we on a detached HEAD? */
-	if (!strcmp("HEAD", head))
-		strbuf_addstr(&buf, " on detached HEAD");
-	else if (!prefixcmp(head, "refs/heads/")) {
+	if (!prefixcmp(head, "refs/heads/")) {
 		const char *cp;
 		strbuf_addstr(&buf, " on ");
 		for (cp = head + 11; *cp; cp++) {
@@ -899,7 +898,6 @@ static char *get_commit_format_string(void)
 				strbuf_addch(&buf, *cp);
 		}
 	}
-	strbuf_addstr(&buf, ": %s");
 
 	return strbuf_detach(&buf, NULL);
 }
@@ -933,7 +931,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
 	rev.diffopt.break_opt = 0;
 	diff_setup_done(&rev.diffopt);
 
-	printf("Created %scommit ", initial_commit ? "initial " : "");
+	printf("Created %s", initial_commit ? "root-commit " : "");
 
 	if (!log_tree_commit(&rev, commit)) {
 		struct strbuf buf = STRBUF_INIT;
-- 
1.6.0.2.529.g37dbc.dirty

Re: [PATCH] Builtin-commit: show on which branch a commit was added

[Wincent Colaiuta]

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

> Jeff King wrote:
>> On Mon, Sep 29, 2008 at 10:09:17PM +0200, Pieter de Bie wrote:
>>> How about something like
>>>
>>> 	Created commit abcd1234 on widget -- "subwidget: one line summary"
>> 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 ": ".
>
> Created 6207abc (subwidget: one quite long line of sum...) on <branch>
>
> "commit" is just noise.

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

Re: [PATCH] Builtin-commit: show on which branch a commit was added

[Jeff King]

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

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

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

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

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

Re: [PATCH] Builtin-commit: show on which branch a commit was added

[Andreas Ericsson]

Jeff King wrote:
> On Tue, Sep 30, 2008 at 08:37:00AM +0200, Wincent Colaiuta wrote:
> 
>> 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.
> 
> 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.
> 

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.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Re: [PATCH] Builtin-commit: show on which branch a commit was added

[Jeff King]

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

> I agree. Obvious solution is to do
>
> subj_len = term_width - (strlen(cruft) + strlen(branch_name))

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.

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

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

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

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.

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

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

---
diff --git a/builtin-commit.c b/builtin-commit.c
index 917f638..9954a81 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -876,41 +876,17 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 	rollback_index_files();
 
 	return commitable ? 0 : 1;
 }
 
-static char *get_commit_format_string(void)
-{
-	unsigned char sha[20];
-	const char *head = resolve_ref("HEAD", sha, 0, NULL);
-	struct strbuf buf = STRBUF_INIT;
-
-	strbuf_addstr(&buf, "format:%h");
-
-	/* Are we on a detached HEAD? */
-	if (!strcmp("HEAD", head))
-		strbuf_addstr(&buf, " on detached HEAD");
-	else if (!prefixcmp(head, "refs/heads/")) {
-		const char *cp;
-		strbuf_addstr(&buf, " on ");
-		for (cp = head + 11; *cp; cp++) {
-			if (*cp == '%')
-				strbuf_addstr(&buf, "%x25");
-			else
-				strbuf_addch(&buf, *cp);
-		}
-	}
-	strbuf_addstr(&buf, ": %s");
-
-	return strbuf_detach(&buf, NULL);
-}
-
 static void print_summary(const char *prefix, const unsigned char *sha1)
 {
 	struct rev_info rev;
 	struct commit *commit;
-	char *format = get_commit_format_string();
+	static const char *format = "format:%h: \"%s\"";
+	unsigned char junk_sha1[20];
+	const char *head = resolve_ref("HEAD", junk_sha1, 0, NULL);
 
 	commit = lookup_commit(sha1);
 	if (!commit)
 		die("couldn't look up newly created commit");
 	if (!commit || parse_commit(commit))
@@ -931,19 +907,20 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
 	rev.diffopt.detect_rename = 1;
 	rev.diffopt.rename_limit = 100;
 	rev.diffopt.break_opt = 0;
 	diff_setup_done(&rev.diffopt);
 
-	printf("Created %scommit ", initial_commit ? "initial " : "");
+	printf("%s%s: created ",
+		!prefixcmp(head, "refs/heads/") ? head + 11 : head,
+		initial_commit ? " (initial)" : "");
 
 	if (!log_tree_commit(&rev, commit)) {
 		struct strbuf buf = STRBUF_INIT;
 		format_commit_message(commit, format + 7, &buf, DATE_NORMAL);
 		printf("%s\n", buf.buf);
 		strbuf_release(&buf);
 	}
-	free(format);
 }
 
 static int git_commit_config(const char *k, const char *v, void *cb)
 {
 	if (!strcmp(k, "commit.template"))

Re: [PATCH] Builtin-commit: show on which branch a commit was added

[Andreas Ericsson]

Jeff King wrote:
> On Tue, Sep 30, 2008 at 11:59:25AM +0200, Andreas Ericsson wrote:
> 
>> I agree. Obvious solution is to do
>>
>> subj_len = term_width - (strlen(cruft) + strlen(branch_name))
> 
> 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.
> 
>> 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.
> 
> I like it much better than what is on next (and I thought your commit
> message summed up the issue nicely), but...
> 

Thanks. Feel free to recycle it :)

>> Another way would be to write
>> <branch>: Created <hash>: "subject line..."
> 
> I think I like this even better.

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.

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

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.

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

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                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Re: [PATCH] Builtin-commit: show on which branch a commit was added

[Shawn O. Pearce]

Andreas Ericsson <ae@op5.se> wrote:
> Jeff King wrote:
>> On Tue, Sep 30, 2008 at 11:59:25AM +0200, Andreas Ericsson wrote:
>
>>> Another way would be to write
>>> <branch>: Created <hash>: "subject line..."
>>
>> I think I like this even better.
>
> 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.

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.

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

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.

Re: [PATCH] Builtin-commit: show on which branch a commit was added

[Andreas Ericsson]

Shawn O. Pearce wrote:
> Andreas Ericsson <ae@op5.se> wrote:
>> Jeff King wrote:
>>> On Tue, Sep 30, 2008 at 11:59:25AM +0200, Andreas Ericsson wrote:
>>>> Another way would be to write
>>>> <branch>: Created <hash>: "subject line..."
>>> I think I like this even better.
>> 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.
> 
> 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.
> 
>>> 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.
>> 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.
> 
> 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.  ;-)
> 

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.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Re: [PATCH] Builtin-commit: show on which branch a commit was added

[Jeff King]

On Wed, Oct 01, 2008 at 08:10:11AM -0700, Shawn O. Pearce 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.

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.

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

Welcome to maintainership. ;P

-Peff

Re: [PATCH] Builtin-commit: show on which branch a commit was added

[Shawn O. Pearce]

Jeff King <peff@peff.net> wrote:
> On Wed, Oct 01, 2008 at 08:10:11AM -0700, Shawn O. Pearce 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
> 
> 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.

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.
 
> > 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.  ;-)
> 
> Welcome to maintainership. ;P

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

-- 
Shawn.

Re: [PATCH] Builtin-commit: show on which branch a commit was added

[Jeff King]

On Wed, Oct 01, 2008 at 08:36:37AM -0700, Shawn O. Pearce 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)

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

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

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

Re: [PATCH] Builtin-commit: show on which branch a commit was added

[Shawn O. Pearce]

Jeff King <peff@peff.net> wrote:
> On Wed, Oct 01, 2008 at 08:36:37AM -0700, Shawn O. Pearce wrote:
> 
> > *sigh*  And I just took up the #@*#@!&!*!@ paintbrush myself.
> > Dammit.  I'm putting it down now.  Really.  :-)
> 
> 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.

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.

-- 
Shawn.

[PATCH] git commit: Repaint the output format bikeshed (again)

[Andreas Ericsson]

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(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index e4e1448..3b43344 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -878,35 +878,12 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 	return commitable ? 0 : 1;
 }
 
-static char *get_commit_format_string(void)
-{
-	unsigned char sha[20];
-	const char *head = resolve_ref("HEAD", sha, 0, NULL);
-	struct strbuf buf = STRBUF_INIT;
-
-	/* use shouty-caps if we're on detached HEAD */
-	strbuf_addf(&buf, "format:%s", strcmp("HEAD", head) ? "" : "DETACHED commit");
-	strbuf_addstr(&buf, "%h (%s)");
-
-	if (!prefixcmp(head, "refs/heads/")) {
-		const char *cp;
-		strbuf_addstr(&buf, " on ");
-		for (cp = head + 11; *cp; cp++) {
-			if (*cp == '%')
-				strbuf_addstr(&buf, "%x25");
-			else
-				strbuf_addch(&buf, *cp);
-		}
-	}
-
-	return strbuf_detach(&buf, NULL);
-}
-
 static void print_summary(const char *prefix, const unsigned char *sha1)
 {
 	struct rev_info rev;
 	struct commit *commit;
-	char *format = get_commit_format_string();
+	unsigned char head_sha1[20];
+	const char *branch, *head, *format = "format:%h (%s)";
 
 	commit = lookup_commit(sha1);
 	if (!commit)
@@ -931,15 +908,27 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
 	rev.diffopt.break_opt = 0;
 	diff_setup_done(&rev.diffopt);
 
-	printf("Created %s", initial_commit ? "root-commit " : "");
+	/* a pretty rare occurrance, so let's celebrate it specially */
+	if (initial_commit)
+		printf("History has begun anew. Root-commit created.\n");
+
+	head = resolve_ref("HEAD", head_sha1, 0, NULL);
+	if (!strcmp(head, "HEAD"))
+		branch = "DETACHED HEAD";
+	else if (!prefixcmp(head, "refs/heads/"))
+		branch = &head[strlen("refs/heads/")];
+	else {
+		/* refs/git-svn, fe */
+		branch = head;
+	}
+
+	printf("%s is now at ", branch);
 
 	if (!log_tree_commit(&rev, commit)) {
 		struct strbuf buf = STRBUF_INIT;
 		format_commit_message(commit, format + 7, &buf, DATE_NORMAL);
-		printf("%s\n", buf.buf);
-		strbuf_release(&buf);
+		printf("%s\n", strbuf_detach(&buf, NULL));
 	}
-	free(format);
 }
 
 static int git_commit_config(const char *k, const char *v, void *cb)
-- 
1.6.0.2.529.g37dbc.dirty

Re: [PATCH] git commit: Repaint the output format bikeshed (again)

[Jeff King]

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

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

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

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

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.

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

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

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

Heh.

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

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

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

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

> -		printf("%s\n", buf.buf);
> -		strbuf_release(&buf);
> +		printf("%s\n", strbuf_detach(&buf, NULL));

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

Re: [PATCH] git commit: Repaint the output format bikeshed (again)

[Jeff King]

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

> I think I still like your other proposal:
> 
>   [branch] created b930c4a: "i386: Snib the sprock"

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(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index e4e1448..7a66e5a 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -878,35 +878,13 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 	return commitable ? 0 : 1;
 }
 
-static char *get_commit_format_string(void)
-{
-	unsigned char sha[20];
-	const char *head = resolve_ref("HEAD", sha, 0, NULL);
-	struct strbuf buf = STRBUF_INIT;
-
-	/* use shouty-caps if we're on detached HEAD */
-	strbuf_addf(&buf, "format:%s", strcmp("HEAD", head) ? "" : "DETACHED commit");
-	strbuf_addstr(&buf, "%h (%s)");
-
-	if (!prefixcmp(head, "refs/heads/")) {
-		const char *cp;
-		strbuf_addstr(&buf, " on ");
-		for (cp = head + 11; *cp; cp++) {
-			if (*cp == '%')
-				strbuf_addstr(&buf, "%x25");
-			else
-				strbuf_addch(&buf, *cp);
-		}
-	}
-
-	return strbuf_detach(&buf, NULL);
-}
-
 static void print_summary(const char *prefix, const unsigned char *sha1)
 {
 	struct rev_info rev;
 	struct commit *commit;
-	char *format = get_commit_format_string();
+	static const char *format = "format:%h: \"%s\"";
+	unsigned char junk_sha1[20];
+	const char *head = resolve_ref("HEAD", junk_sha1, 0, NULL);
 
 	commit = lookup_commit(sha1);
 	if (!commit)
@@ -931,7 +909,13 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
 	rev.diffopt.break_opt = 0;
 	diff_setup_done(&rev.diffopt);
 
-	printf("Created %s", initial_commit ? "root-commit " : "");
+	printf("[%s%s]: created ",
+		!prefixcmp(head, "refs/heads/") ?
+			head + 11 :
+			!strcmp(head, "HEAD") ?
+				"detached HEAD" :
+				head,
+		initial_commit ? " (root-commit)" : "");
 
 	if (!log_tree_commit(&rev, commit)) {
 		struct strbuf buf = STRBUF_INIT;
@@ -939,7 +923,6 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
 		printf("%s\n", buf.buf);
 		strbuf_release(&buf);
 	}
-	free(format);
 }
 
 static int git_commit_config(const char *k, const char *v, void *cb)
-- 
1.6.0.2.570.g2c958

Re: [PATCH] git commit: Repaint the output format bikeshed (again)

[Andreas Ericsson]

Jeff King wrote:
> On Wed, Oct 01, 2008 at 06:06:04PM -0400, Jeff King wrote:
> 
>> I think I still like your other proposal:
>>
>>   [branch] created b930c4a: "i386: Snib the sprock"
> 
> 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"
> ---

No sign-off.

>  builtin-commit.c |   37 ++++++++++---------------------------
>  1 files changed, 10 insertions(+), 27 deletions(-)
> 
> diff --git a/builtin-commit.c b/builtin-commit.c
> index e4e1448..7a66e5a 100644
> --- a/builtin-commit.c
> +++ b/builtin-commit.c
> @@ -878,35 +878,13 @@ int cmd_status(int argc, const char **argv, const char *prefix)
>  	return commitable ? 0 : 1;
>  }
>  
> -static char *get_commit_format_string(void)
> -{
> -	unsigned char sha[20];
> -	const char *head = resolve_ref("HEAD", sha, 0, NULL);
> -	struct strbuf buf = STRBUF_INIT;
> -
> -	/* use shouty-caps if we're on detached HEAD */
> -	strbuf_addf(&buf, "format:%s", strcmp("HEAD", head) ? "" : "DETACHED commit");
> -	strbuf_addstr(&buf, "%h (%s)");
> -
> -	if (!prefixcmp(head, "refs/heads/")) {
> -		const char *cp;
> -		strbuf_addstr(&buf, " on ");
> -		for (cp = head + 11; *cp; cp++) {
> -			if (*cp == '%')
> -				strbuf_addstr(&buf, "%x25");
> -			else
> -				strbuf_addch(&buf, *cp);
> -		}
> -	}
> -
> -	return strbuf_detach(&buf, NULL);
> -}
> -
>  static void print_summary(const char *prefix, const unsigned char *sha1)
>  {
>  	struct rev_info rev;
>  	struct commit *commit;
> -	char *format = get_commit_format_string();
> +	static const char *format = "format:%h: \"%s\"";
> +	unsigned char junk_sha1[20];
> +	const char *head = resolve_ref("HEAD", junk_sha1, 0, NULL);
>  
>  	commit = lookup_commit(sha1);
>  	if (!commit)
> @@ -931,7 +909,13 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
>  	rev.diffopt.break_opt = 0;
>  	diff_setup_done(&rev.diffopt);
>  
> -	printf("Created %s", initial_commit ? "root-commit " : "");
> +	printf("[%s%s]: created ",
> +		!prefixcmp(head, "refs/heads/") ?
> +			head + 11 :
> +			!strcmp(head, "HEAD") ?
> +				"detached HEAD" :
> +				head,
> +		initial_commit ? " (root-commit)" : "");
>  

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.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Re: [PATCH] git commit: Repaint the output format bikeshed (again)

[Jeff King]

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

> No sign-off.

Sorry, mistakenly omitted.

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

>> +	printf("[%s%s]: created ",
>> +		!prefixcmp(head, "refs/heads/") ?
>> +			head + 11 :
>> +			!strcmp(head, "HEAD") ?
>> +				"detached HEAD" :
>> +				head,
>> +		initial_commit ? " (root-commit)" : "");
>>  
>
> 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.

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

Re: [PATCH] git commit: Repaint the output format bikeshed (again)

[Shawn O. Pearce]

Jeff King <peff@peff.net> wrote:
> On Thu, Oct 02, 2008 at 07:40:28AM +0200, Andreas Ericsson wrote:
> > [... many many many many many things about paints ...]

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"

-- 
Shawn.

Re: [PATCH] git commit: Repaint the output format bikeshed (again)

[Jeff King]

On Thu, Oct 02, 2008 at 05:15:56PM -0700, Shawn O. Pearce 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"

Woo! Victory by attrition!

-Peff

Re: [PATCH] git commit: Repaint the output format bikeshed (again)

[Shawn O. Pearce]

Jeff King <peff@peff.net> wrote:
> On Thu, Oct 02, 2008 at 05:15:56PM -0700, Shawn O. Pearce 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"
> 
> Woo! Victory by attrition!

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.

-- 
Shawn.

Re: [PATCH] git commit: Repaint the output format bikeshed (again)

[Jeff King]

On Fri, Oct 03, 2008 at 07:09:52AM -0700, Shawn O. Pearce 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.

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(-)

diff --git a/Documentation/gittutorial-2.txt b/Documentation/gittutorial-2.txt
index 8484e7a..bab0f34 100644
--- a/Documentation/gittutorial-2.txt
+++ b/Documentation/gittutorial-2.txt
@@ -32,11 +32,13 @@ Initialized empty Git repository in .git/
 $ echo 'hello world' > file.txt
 $ git add .
 $ git commit -a -m "initial commit"
-Created root-commit 54196cc (initial commit) on master
+[master (root-commit)] created 54196cc: "initial commit"
+ 1 files changed, 1 insertions(+), 0 deletions(-)
  create mode 100644 file.txt
 $ echo 'hello world!' >file.txt
 $ git commit -a -m "add emphasis"
-Created c4d59f3 (add emphasis) on master
+[master] created c4d59f3: "add emphasis"
+ 1 files changed, 1 insertions(+), 1 deletions(-)
 ------------------------------------------------
 
 What are the 7 digits of hex that git responded to the commit with?
-- 
1.6.0.2.636.gaa7b

Re: [PATCH] git commit: Repaint the output format bikeshed (again)

[Wincent Colaiuta]

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

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

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

Re: [PATCH] Builtin-commit: show on which branch a commit was added

[Jeff King]

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

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

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

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

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

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