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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 7, 2008, 05:27 UTC
Message-ID: <7vzlmkpltb.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <1220634785-55543-1-git-send-email-pdebie@ai.rug.nl>

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

Show 9 quoted lines

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

Show 9 quoted lines

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

Show 7 quoted lines

> +}
> +
>  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 = ...

Show 8 quoted lines

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