[PATCH 0/3] commit: fix abbrev-sha regression

[PATCH 0/3] commit: fix abbrev-sha regression

[Tay Ray Chuan]

After c197702 (pretty: Respect --abbrev option), the summary output for
git-commit began to throw up non-abbreviated SHA-1s.

For example,

  $ git commit --allow-empty -m "empty"
  [master f869bf09c8eefee54094bb21387241aaf5f10324] empty

I also notice that this happens for merges after conflict resolutions.

This afflicts 'master', but not any release.

Here's the hunk that fixed it:

-->8--
diff --git a/builtin/commit.c b/builtin/commit.c
index a4e4966..ab4a7cf 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1166,6 +1166,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
 		struct pretty_print_context ctx = {0};
 		struct strbuf buf = STRBUF_INIT;
 		ctx.date_mode = DATE_NORMAL;
+		ctx.abbrev = DEFAULT_ABBREV;
 		format_commit_message(commit, format.buf + 7, &buf, &ctx);
 		printf("%s\n", buf.buf);
 		strbuf_release(&buf);
-->8--

However, I noticed that this would also work:

-->8--
diff --git a/builtin/commit.c b/builtin/commit.c
index ab4a7cf..13a30ab 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1148,7 +1148,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.buf, &rev);
-       rev.always_show_header = 0;
+       rev.always_show_header = 1;
        rev.diffopt.detect_rename = 1;
        rev.diffopt.rename_limit = 100;
        rev.diffopt.break_opt = 0;
-->8--

This would make the code block that handled the situation in which
log_tree_commit() did not print any output redundant, so perhaps this
would also follow:

-->8--
diff --git a/builtin/commit.c b/builtin/commit.c
index ab4a7cf..e8def55 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1163,13 +1163,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
                initial_commit ? " (root-commit)" : "");

        if (!log_tree_commit(&rev, commit)) {
-               struct pretty_print_context ctx = {0};
-               struct strbuf buf = STRBUF_INIT;
-               ctx.date_mode = DATE_NORMAL;
-               ctx.abbrev = DEFAULT_ABBREV;
-               format_commit_message(commit, format.buf + 7, &buf, &ctx);
-               printf("%s\n", buf.buf);
-               strbuf_release(&buf);
+               die("unable to print summary");
        }
        strbuf_release(&format);
 }
-->8--

In a sense, it reverts parts of bf82a15 (commit: do not add extra LF at
the end of the summary.), except for the extra LF. Would this be a more
sensible route?

Contents:
[PATCH 0/3] commit: fix abbrev-sha regression
[PATCH 1/3] t7502-commit: add tests for summary output
[PATCH 2/3] t7502-commit: add summary output tests for empty and merge commits
[PATCH 3/3] commit: show abbreviated sha for commits with empty diffs

[PATCH 1/3] t7502-commit: add tests for summary output

[Tay Ray Chuan]

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---
 t/t7502-commit.sh |   36 ++++++++++++++++++++++++++++++++++++
 1 files changed, 36 insertions(+), 0 deletions(-)

diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index 844fb43..589e8e6 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -4,8 +4,44 @@ test_description='git commit porcelain-ish'
 
 . ./test-lib.sh
 
+# Arguments: [<prefix] [<commit message>]
+check_summary_oneline() {
+	test_tick &&
+	git commit -m "$2" | head -1 > act &&
+
+	# branch name
+	SUMMARY_PREFIX="$(git name-rev --name-only HEAD)" &&
+
+	# append the "special" prefix, like "root-commit", "detached HEAD"
+	if test -n "$1"
+	then
+		SUMMARY_PREFIX="$SUMMARY_PREFIX ($1)"
+	fi
+
+	# abbrev SHA-1
+	SUMMARY_POSTFIX="$(git log -1 --pretty='format:%h')"
+	echo "[$SUMMARY_PREFIX $SUMMARY_POSTFIX] $2" >exp &&
+
+	test_cmp exp act
+}
+
+test_expect_success 'output summary format' '
+
+	echo new >file1 &&
+	git add file1 &&
+	check_summary_oneline "root-commit" "initial" &&
+
+	echo change >>file1 &&
+	git add file1 &&
+	check_summary_oneline "" "a change"
+'
+
 test_expect_success 'the basics' '
 
+	# this is needed for the "partial removal" test to pass
+	git rm file1 &&
+	git commit -m "cleanup" &&
+
 	echo doing partial >"commit is" &&
 	mkdir not &&
 	echo very much encouraged but we should >not/forbid &&
-- 
1.7.1.189.g07419

[PATCH 2/3] t7502-commit: add summary output tests for empty and merge commits

[Tay Ray Chuan]

After c197702 (pretty: Respect --abbrev option), non-abbreviated hashes
began to appear, leading to failures for these tests.

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---
 t/t7502-commit.sh |   29 +++++++++++++++++++++++++++--
 1 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index 589e8e6..8a4a277 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -4,10 +4,10 @@ test_description='git commit porcelain-ish'
 
 . ./test-lib.sh
 
-# Arguments: [<prefix] [<commit message>]
+# Arguments: [<prefix] [<commit message>] [<commit options>]
 check_summary_oneline() {
 	test_tick &&
-	git commit -m "$2" | head -1 > act &&
+	echo "$3" | xargs git commit -m "$2" | head -1 > act &&
 
 	# branch name
 	SUMMARY_PREFIX="$(git name-rev --name-only HEAD)" &&
@@ -36,6 +36,31 @@ test_expect_success 'output summary format' '
 	check_summary_oneline "" "a change"
 '
 
+test_expect_failure 'output summary format for commit with an empty diff' '
+
+	check_summary_oneline "" "empty" "--allow-empty"
+'
+
+test_expect_failure 'output summary format for merges' '
+
+	git checkout -b recursive-base &&
+	test_commit base file1 &&
+
+	git checkout -b recursive-a recursive-base &&
+	test_commit commit-a file1 &&
+
+	git checkout -b recursive-b recursive-base &&
+	test_commit commit-b file1 &&
+
+	# conflict
+	git checkout recursive-a &&
+	test_must_fail git merge recursive-b &&
+	# resolve the conflict
+	echo commit-a > file1 &&
+	git add file1 &&
+	check_summary_oneline "" "Merge"
+'
+
 test_expect_success 'the basics' '
 
 	# this is needed for the "partial removal" test to pass
-- 
1.7.1.189.g07419

[PATCH 3/3] commit: show abbreviated sha for commits with empty diffs

[Tay Ray Chuan]

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---
 builtin/commit.c  |    1 +
 t/t7502-commit.sh |    4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index a4e4966..ab4a7cf 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1166,6 +1166,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
 		struct pretty_print_context ctx = {0};
 		struct strbuf buf = STRBUF_INIT;
 		ctx.date_mode = DATE_NORMAL;
+		ctx.abbrev = DEFAULT_ABBREV;
 		format_commit_message(commit, format.buf + 7, &buf, &ctx);
 		printf("%s\n", buf.buf);
 		strbuf_release(&buf);
diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index 8a4a277..109ae00 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -36,12 +36,12 @@ test_expect_success 'output summary format' '
 	check_summary_oneline "" "a change"
 '
 
-test_expect_failure 'output summary format for commit with an empty diff' '
+test_expect_success 'output summary format for commit with an empty diff' '
 
 	check_summary_oneline "" "empty" "--allow-empty"
 '
 
-test_expect_failure 'output summary format for merges' '
+test_expect_success 'output summary format for merges' '
 
 	git checkout -b recursive-base &&
 	test_commit base file1 &&
-- 
1.7.1.189.g07419

Re: [PATCH 3/3] commit: show abbreviated sha for commits with empty diffs

[Junio C Hamano]

The fix itself might be trivial, but the series seems to break 7502.20
and 7502.22

Thanks.

Re: [PATCH 2/3] t7502-commit: add summary output tests for empty and merge commits

[Junio C Hamano]

Tay Ray Chuan <rctay89@gmail.com> writes:

> After c197702 (pretty: Respect --abbrev option), non-abbreviated hashes
> began to appear, leading to failures for these tests.
>
> Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
> ---
>  t/t7502-commit.sh |   29 +++++++++++++++++++++++++++--
>  1 files changed, 27 insertions(+), 2 deletions(-)
>
> diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
> index 589e8e6..8a4a277 100755
> --- a/t/t7502-commit.sh
> +++ b/t/t7502-commit.sh
> @@ -4,10 +4,10 @@ test_description='git commit porcelain-ish'
>  
>  . ./test-lib.sh
>  
> -# Arguments: [<prefix] [<commit message>]
> +# Arguments: [<prefix] [<commit message>] [<commit options>]
>  check_summary_oneline() {
>  	test_tick &&
> -	git commit -m "$2" | head -1 > act &&
> +	echo "$3" | xargs git commit -m "$2" | head -1 > act &&

Why do you have to fork xargs?  Wouldn't/shouldn't

    git commit ${3+"$3"} -m "$2"

work?

Re: [PATCH 2/3] t7502-commit: add summary output tests for empty and merge commits

[Tay Ray Chuan]

Hi,

On Wed, May 26, 2010 at 1:07 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Tay Ray Chuan <rctay89@gmail.com> writes:
>
>> After c197702 (pretty: Respect --abbrev option), non-abbreviated hashes
>> began to appear, leading to failures for these tests.
>>
>> Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
>> ---
>>  t/t7502-commit.sh |   29 +++++++++++++++++++++++++++--
>>  1 files changed, 27 insertions(+), 2 deletions(-)
>>
>> diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
>> index 589e8e6..8a4a277 100755
>> --- a/t/t7502-commit.sh
>> +++ b/t/t7502-commit.sh
>> @@ -4,10 +4,10 @@ test_description='git commit porcelain-ish'
>>
>>  . ./test-lib.sh
>>
>> -# Arguments: [<prefix] [<commit message>]
>> +# Arguments: [<prefix] [<commit message>] [<commit options>]
>>  check_summary_oneline() {
>>       test_tick &&
>> -     git commit -m "$2" | head -1 > act &&
>> +     echo "$3" | xargs git commit -m "$2" | head -1 > act &&
>
> Why do you have to fork xargs?  Wouldn't/shouldn't
>
>    git commit ${3+"$3"} -m "$2"
>
> work?

It would. Blame it on my poor shell-fu. :)

-- 
Cheers,
Ray Chuan

Re: [PATCH 3/3] commit: show abbreviated sha for commits with empty diffs

[Tay Ray Chuan]

Hi,

On Wed, May 26, 2010 at 1:07 PM, Junio C Hamano <gitster@pobox.com> wrote:
> The fix itself might be trivial, but the series seems to break 7502.20
> and 7502.22

I tested this patch series on top of:

 - efd1311 (Merge branch 'jn/shortlog' into next), which contains
   c197702 (pretty: Respect --abbrev option), the first offending commit
   to print un-abbreviated SHA-1s.

 - b26ba11, the recent 'next'.

It both cases, #20 and #22 passed fine.

Also, I outlined another possible fix in the cover letter to this
series:

  1274694452-4200-1-git-send-email-rctay89@gmail.com
  http://thread.gmane.org/gmane.comp.version-control.git/147619

I find the alternative fix a bit more precise; I'd appreciate your
comments, if you get the chance.

-- 
Cheers,
Ray Chuan

Re: [PATCH 3/3] commit: show abbreviated sha for commits with empty diffs

[Tay Ray Chuan]

Hi,

On Wed, May 26, 2010 at 1:37 PM, Tay Ray Chuan <rctay89@gmail.com> wrote:
> Hi,
>
> On Wed, May 26, 2010 at 1:07 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> The fix itself might be trivial, but the series seems to break 7502.20
>> and 7502.22
>
> I tested this patch series on top of:
>
>  - efd1311 (Merge branch 'jn/shortlog' into next), which contains
>   c197702 (pretty: Respect --abbrev option), the first offending commit
>   to print un-abbreviated SHA-1s.
>
>  - b26ba11, the recent 'next'.
>
> It both cases, #20 and #22 passed fine.

My apologies - I've already squashed in a fix to address that locally
and I haven't sent that out yet.

-- 
Cheers,
Ray Chuan

[PATCH v2 0/3] commit: fix abbrev-sha regression

[Tay Ray Chuan]

After c197702 (pretty: Respect --abbrev option), the summary output for
git-commit began to throw up non-abbreviated SHA-1s.

For example,

  $ git commit --allow-empty -m "empty"
  [master f869bf09c8eefee54094bb21387241aaf5f10324] empty

I also notice that this happens for merges after conflict resolutions.

This afflicts 'master', but not any release.

Changes from v1:

 - move cleanup commands into separate function in patch #1.
 - add a cleanup command to patch #1 to accomodate tests #20 and #22.
 - changed shell syntax in patch #2 (based on Junio's suggestion).
 - used a more "aggressive" fix in patch #3.

Contents:
[PATCH v2 1/3] t7502-commit: add tests for summary output
[PATCH v2 2/3] t7502-commit: add summary output tests for empty and merge commits
[PATCH v2 3/3] commit::print_summary(): set rev_info.always_show_header to 1

[PATCH v2 1/3] t7502-commit: add tests for summary output

[Tay Ray Chuan]

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---

Changes from v1:
 - move cleanup commands into a separate function,
   output_tests_cleanup().
 - add a cleanup command to accomodate tests #20 and #22.

 t/t7502-commit.sh |   43 +++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 43 insertions(+), 0 deletions(-)

diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index 844fb43..478b637 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -4,8 +4,51 @@ test_description='git commit porcelain-ish'

 . ./test-lib.sh

+# Arguments: [<prefix] [<commit message>]
+check_summary_oneline() {
+	test_tick &&
+	git commit -m "$2" | head -1 > act &&
+
+	# branch name
+	SUMMARY_PREFIX="$(git name-rev --name-only HEAD)" &&
+
+	# append the "special" prefix, like "root-commit", "detached HEAD"
+	if test -n "$1"
+	then
+		SUMMARY_PREFIX="$SUMMARY_PREFIX ($1)"
+	fi
+
+	# abbrev SHA-1
+	SUMMARY_POSTFIX="$(git log -1 --pretty='format:%h')"
+	echo "[$SUMMARY_PREFIX $SUMMARY_POSTFIX] $2" >exp &&
+
+	test_cmp exp act
+}
+
+test_expect_success 'output summary format' '
+
+	echo new >file1 &&
+	git add file1 &&
+	check_summary_oneline "root-commit" "initial" &&
+
+	echo change >>file1 &&
+	git add file1 &&
+	check_summary_oneline "" "a change"
+'
+
+output_tests_cleanup() {
+	# this is needed for "do not fire editor in the presence of conflicts"
+	git checkout master &&
+
+	# this is needed for the "partial removal" test to pass
+	git rm file1 &&
+	git commit -m "cleanup"
+}
+
 test_expect_success 'the basics' '

+	output_tests_cleanup &&
+
 	echo doing partial >"commit is" &&
 	mkdir not &&
 	echo very much encouraged but we should >not/forbid &&
--
1.7.1.189.g07419

[PATCH v2 2/3] t7502-commit: add summary output tests for empty and merge commits

[Tay Ray Chuan]

After c197702 (pretty: Respect --abbrev option), non-abbreviated hashes
began to appear, leading to failures for these tests.

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---

Changes from v1:
 - changed shell syntax in check_summary_oneline(), based on Junio's
   suggestion.

 t/t7502-commit.sh |   29 +++++++++++++++++++++++++++--
 1 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index 478b637..b10541d 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -4,10 +4,10 @@ test_description='git commit porcelain-ish'

 . ./test-lib.sh

-# Arguments: [<prefix] [<commit message>]
+# Arguments: [<prefix] [<commit message>] [<commit options>]
 check_summary_oneline() {
 	test_tick &&
-	git commit -m "$2" | head -1 > act &&
+	git commit ${3+"$3"} -m "$2" | head -1 > act &&

 	# branch name
 	SUMMARY_PREFIX="$(git name-rev --name-only HEAD)" &&
@@ -36,6 +36,31 @@ test_expect_success 'output summary format' '
 	check_summary_oneline "" "a change"
 '

+test_expect_failure 'output summary format for commit with an empty diff' '
+
+	check_summary_oneline "" "empty" "--allow-empty"
+'
+
+test_expect_failure 'output summary format for merges' '
+
+	git checkout -b recursive-base &&
+	test_commit base file1 &&
+
+	git checkout -b recursive-a recursive-base &&
+	test_commit commit-a file1 &&
+
+	git checkout -b recursive-b recursive-base &&
+	test_commit commit-b file1 &&
+
+	# conflict
+	git checkout recursive-a &&
+	test_must_fail git merge recursive-b &&
+	# resolve the conflict
+	echo commit-a > file1 &&
+	git add file1 &&
+	check_summary_oneline "" "Merge"
+'
+
 output_tests_cleanup() {
 	# this is needed for "do not fire editor in the presence of conflicts"
 	git checkout master &&
--
1.7.1.189.g07419

[PATCH v2 3/3] commit::print_summary(): set rev_info.always_show_header to 1

[Tay Ray Chuan]

This attempts to fix a regression in git-commit, where non-abbreviated
SHA-1s were printed in the summary.

One possible fix would be to set ctx.abbrev to DEFAULT_ABBREV in the
`if` block. However, we remove this codeblock altogether, and set
rev.always_show_header.

This way, we use back the same show_log() mechanism (instead of
format_commit_message()).

Quoting log-tree.c:560:

	shown = log_tree_diff(opt, commit, &log);
	if (!shown && opt->loginfo && opt->always_show_header) {
		log.parent = NULL;
		show_log(opt);
		shown = 1;
	}

This is the only area that always_show_header is checked, so the
setting of this flag should only affect this area.

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---

Changed from v1:
 - used an alternative fix - see patch message for more.

 builtin/commit.c  |   13 ++++---------
 t/t7502-commit.sh |    4 ++--
 2 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index a4e4966..2884d0c 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1148,7 +1148,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.buf, &rev);
-	rev.always_show_header = 0;
+	rev.always_show_header = 1;
 	rev.diffopt.detect_rename = 1;
 	rev.diffopt.rename_limit = 100;
 	rev.diffopt.break_opt = 0;
@@ -1162,14 +1162,9 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
 				head,
 		initial_commit ? " (root-commit)" : "");

-	if (!log_tree_commit(&rev, commit)) {
-		struct pretty_print_context ctx = {0};
-		struct strbuf buf = STRBUF_INIT;
-		ctx.date_mode = DATE_NORMAL;
-		format_commit_message(commit, format.buf + 7, &buf, &ctx);
-		printf("%s\n", buf.buf);
-		strbuf_release(&buf);
-	}
+	if (!log_tree_commit(&rev, commit))
+		die("unable to print summary");
+
 	strbuf_release(&format);
 }

diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index b10541d..08c0247 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -36,12 +36,12 @@ test_expect_success 'output summary format' '
 	check_summary_oneline "" "a change"
 '

-test_expect_failure 'output summary format for commit with an empty diff' '
+test_expect_success 'output summary format for commit with an empty diff' '

 	check_summary_oneline "" "empty" "--allow-empty"
 '

-test_expect_failure 'output summary format for merges' '
+test_expect_success 'output summary format for merges' '

 	git checkout -b recursive-base &&
 	test_commit base file1 &&
--
1.7.1.189.g07419

Re: [PATCH v2 0/3] commit: fix abbrev-sha regression

[Will Palmer]

On Thu, 2010-05-27 at 23:34 +0800, Tay Ray Chuan wrote:
> After c197702 (pretty: Respect --abbrev option), the summary output for
> git-commit began to throw up non-abbreviated SHA-1s.

Please CC: me on any future versions of this series, if there are any
-- 
-- Will

Re: [PATCH v2 3/3] commit::print_summary(): set rev_info.always_show_header to 1

[Junio C Hamano]

Tay Ray Chuan <rctay89@gmail.com> writes:

> This attempts to fix a regression in git-commit, where non-abbreviated
> SHA-1s were printed in the summary.
>
> One possible fix would be to set ctx.abbrev to DEFAULT_ABBREV in the
> `if` block. However, we remove this codeblock altogether, and set
> rev.always_show_header.
>
> This way, we use back the same show_log() mechanism (instead of
> format_commit_message()).

I like the removal of the handcrafted call to f-c-m.  Thanks.

> Quoting log-tree.c:560:
>
> 	shown = log_tree_diff(opt, commit, &log);
> 	if (!shown && opt->loginfo && opt->always_show_header) {
> 		log.parent = NULL;
> 		show_log(opt);
> 		shown = 1;
> 	}
>
> This is the only area that always_show_header is checked, so the
> setting of this flag should only affect this area.

Hmm, but also setting this flag would affect anything that changes
behaviour depending on the value of log.parent, no?

> +	if (!log_tree_commit(&rev, commit))
> +		die("unable to print summary");

When always_show_header is set, what are the situations where
log_tree_commit() might return false?  I think your fix depends on the
fact that it will never return false (which I think is a correct thing to
assume---after all that is what "always_show" means ;-).

Re: [PATCH v2 3/3] commit::print_summary(): set rev_info.always_show_header to 1

[Tay Ray Chuan]

On Sat, May 29, 2010 at 9:10 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Tay Ray Chuan <rctay89@gmail.com> writes:
>> Quoting log-tree.c:560:
>>
>>       shown = log_tree_diff(opt, commit, &log);
>>       if (!shown && opt->loginfo && opt->always_show_header) {
>>               log.parent = NULL;
>>               show_log(opt);
>>               shown = 1;
>>       }
>>
>> This is the only area that always_show_header is checked, so the
>> setting of this flag should only affect this area.
>
> Hmm, but also setting this flag would affect anything that changes
> behaviour depending on the value of log.parent, no?

A cursory look at log-tree.c leads me to believe only show_log() cares
about log.parent.

In any case, if you look a few more lines up, you would notice another
log.parent = NULL:

@552
	struct log_info log;
	int shown;

	log.commit = commit;
	log.parent = NULL;
	opt->loginfo = &log;

	shown = log_tree_diff(opt, commit, &log);
	if (!shown && opt->loginfo && opt->always_show_header) {
		log.parent = NULL;
		show_log(opt);
		shown = 1;
	}

Therefore, I suspect that log.parent = NULL is a kind of
initialization for show_log() (log_tree_diff() does call show_log()
too).

>> +     if (!log_tree_commit(&rev, commit))
>> +             die("unable to print summary");
>
> When always_show_header is set, what are the situations where
> log_tree_commit() might return false?  I think your fix depends on the
> fact that it will never return false (which I think is a correct thing to
> assume---after all that is what "always_show" means ;-).

Based on my reading, I can't think of any.

-- 
Cheers,
Ray Chuan

What's cooking in git.git (Jun 2010, #01; Wed, 2)

[Junio C Hamano]

Here are the topics that have been cooking.  Commits prefixed with '-' are
only in 'pu' while commits prefixed with '+' are in 'next'.  The ones
marked with '.' do not appear in any of the integration branches, but I am
still holding onto them.

Many topics that have been cooking since 1.7.1 pre-release freeze period
are now in master; maybe it is a good time to start freezing feature
branches for 1.7.2---I think I said I'll keep this cycle shorter, no?

--------------------------------------------------
[New Topics]

* bs/userdiff-php (2010-05-23) 1 commit
 - diff: Support visibility modifiers in the PHP hunk header regexp

Will merge to 'next'.

* cc/maint-diff-CC-binary (2010-06-02) 2 commits
 - fixup
 - diff: fix "git show -C -C" output when renaming a binary file

The fix-up should probably be squashed in, as the original "fix" was
barking up a wrong tree.

* jn/gitweb-plackup (2010-05-28) 3 commits
 - git-instaweb: Add support for running gitweb via 'plackup'
 - git-instaweb: Wait for server to start before running web browser
 - git-instaweb: Remove pidfile after stopping web server
 (this branch uses ps/gitweb-soc.)

Will merge to 'next'.

* mg/status-b (2010-05-25) 2 commits
 - Documentation+t5708: document and test status -s -b
 - Show branch information in short output of git status

There are a few style violations that snuck in, but otherwise looked
sensible.

* ps/gitweb-soc (2010-05-28) 4 commits
 - git-instaweb: Configure it to work with new gitweb structure
 - git-instaweb: Put httpd logs in a "$httpd_only" subdirectory
 - gitweb: Set default destination directory for installing gitweb in Makefile
 - gitweb: Move static files into seperate subdirectory
 (this branch is used by jn/gitweb-plackup.)

Will merge to 'next'.

* tc/commit-abbrev-fix (2010-05-27) 3 commits
 - commit::print_summary(): set rev_info.always_show_header to 1
 - t7502-commit: add summary output tests for empty and merge commits
 - t7502-commit: add tests for summary output

Will merge to 'next'.  I am not quite happy about the "impossible to
trigger" die message, though.  It is a good defensive programming to catch
breakages caused by future changes that may invalidate the assumption this
patch makes, but then the message should be worded as such to state that
assumption, I think.

* ab/maint-perl-use-instlibdir (2010-05-30) 1 commit
 - Makefile: remove redundant munging of @@INSTLIBDIR@@

Will merge to 'next'.

* by/diff-graph (2010-05-29) 6 commits
 - Make --color-words work well with --graph
 - graph.c: register a callback for graph output
 - Emit a whole line in one go
 - diff.c: Output the text graph padding before each diff line
 - Output the graph columns at the end of the commit message
 - Add a prefix output callback to diff output

Will merge to 'next'.

* cc/cherry-pick-series (2010-06-02) 8 commits
 - Documentation/revert: describe passing more than one commit
 - Documentation/cherry-pick: describe passing more than one commit
 - revert: add tests to check cherry-picking many commits
 - revert: allow cherry-picking more than one commit
 - revert: change help_msg() to take no argument
 - revert: refactor code into a do_pick_commit() function
 - revert: use run_command_v_opt() instead of execv_git_cmd()
 - revert: cleanup code for -x option

* gs/usage-to-stdout (2010-05-17) 1 commit
 - print the usage string on stdout instead of stderr

Will merge to 'next'.

* gv/portable (2010-05-14) 18 commits
 - Makefile: Tru64 portability fix
 - Makefile: HP-UX 10.20 portability fixes
 - Makefile: HPUX11 portability fixes
 - Makefile: SunOS 5.6 portability fix
 - inline declaration does not work on AIX
 - Allow disabling "inline"
 - Some platforms lack socklen_t type
 - Make NO_{INET_NTOP,INET_PTON} configured independently
 - Makefile: some platforms do not have hstrerror anywhere
 - git-compat-util.h: some platforms with mmap() lack MAP_FAILED definition
 - test_cmp: do not use "diff -u" on platforms that lack one
 - fixup: do not unconditionally disable "diff -u"
 - tests: use "test_cmp", not "diff", when verifying the result
 - Do not use "diff" found on PATH while building and installing
 - enums: omit trailing comma for portability
 - Makefile: -lpthread may still be necessary when libc has only pthread stubs
 - Rewrite dynamic structure initializations to runtime assignment
 - Makefile: pass CPPFLAGS through to fllow customization

Will merge to 'next'.

* jh/diff-index-line-abbrev (2010-05-30) 1 commit
 - diff.c: Ensure "index $from..$to" line contains unambiguous SHA1s

Will merge to 'next'.

* jk/am-skip-hint (2010-05-30) 1 commit
 - git-am: suggest what to do with superfluous patches

Will merge to 'next'.

* jn/checkout-doc (2010-05-30) 1 commit
 - Documentation/checkout: clarify description

Will merge to 'next'.

* jn/fsck-ident (2010-05-26) 1 commit
 - fsck: fix bogus commit header check

Will merge to 'next'.

* jn/maint-doc-ignore (2010-03-05) 1 commit
 - gitignore.5: Clarify matching rules

Will merge to 'next'.

* jn/rebase-cmdline-fix (2010-05-31) 1 commit
 - rebase: improve error message when upstream argument is missing

Will merge to 'next'.

* ps/gitweb--browse-chrome (2010-05-30) 1 commit
 - git-web--browse: Add support for google chrome and chromium

Will merge to 'next'.

* bc/portable (2010-06-01) 5 commits
 - t/aggregate-results: accomodate systems with small max argument list length
 - t/t7006: ignore return status of shell's unset builtin
 - t/t5150: remove space from sed script
 - git-request-pull.sh: remove -e switch to shell interpreter which breaks ksh
 - t/t5800: skip if python version is older than 2.5

* jn/show-num-walks (2010-06-01) 1 commit
 - DWIM 'git show -5' to 'git show --do-walk -5'

* rr/am-help (2010-06-02) 3 commits
 - git am: Remove stray error message from sed
 - git am: Display some help text when patch is empty
 - git am: Set cmdline globally

Will merge to 'next'.

* jc/t9129-any-utf8 (2010-06-02) 1 commit
 - t9129: fix UTF-8 locale detection

--------------------------------------------------
[Stalled -- would discard unless there are some movements soon]

* jc/rev-list-ancestry-path (2010-04-20) 1 commit
 - revision: --ancestry-path

Just an illustration patch.  merge simplification logic used when
pathspecs are in effect interacts with this rather badly.

* js/rebase-origin-x (2010-02-05) 1 commit
 - [RFC w/o test and incomplete] rebase: add -x option to record original commit name

I retract my objection against the idea of -x; needs polishing before
moving forward.

* js/grep-open (2010-05-01) 3 commits
 - grep: do not ignore return value from chdir()
 - grep -O: allow optional argument specifying the pager (or editor)
 - grep: Add the option '--open-files-in-pager'
 (this branch is used by jp/hold-string-list-sanity.)

Probably needs to support --no-index mode as well.

--------------------------------------------------
[Cooking]

* em/checkout-orphan (2010-05-21) 5 commits
 - bash completion: add --orphan to 'git checkout'
 - t3200: test -l with core.logAllRefUpdates options
 - checkout --orphan: respect -l option always
 - refs: split log_ref_write logic into log_ref_setup
 - Documentation: alter checkout --orphan description

In <4BFE2461.5080501@drmicha.warpmail.net>, Michael J Gruber raised a
valid request for a better explanation of superfluous files left behind
and then are cleaned.  Other than that I think this is a sane thing to
do.

* ec/diff-noprefix-config (2010-05-02) 1 commit
 - diff: add configuration option for disabling diff prefixes.

Will merge to 'next'.

* jk/diff-m-doc (2010-05-08) 1 commit
 - docs: clarify meaning of -M for git-log

Will merge to 'next'.

* mc/maint-zoneparse (2010-05-17) 1 commit
 - Add "Z" as an alias for the timezone "UTC"

Will merge to 'next'.

* mg/notes-dry-run (2010-05-14) 1 commit
 - notes: dry-run and verbose options for prune

Will merge to 'next'.

* mg/rev-parse-lrbranches-locals (2010-05-14) 1 commit
 - revlist: Introduce --lrbranches and --locals revision specifiers
 (this branch uses mg/rev-parse-option-sifter-deprecation.)

Hmmm...

* mg/rev-parse-option-sifter-deprecation (2010-05-14) 3 commits
 - t6018: make sure all tested symbolic names are different revs
 - t6018: add tests for rev-list's --branches and --tags
 - rev-parse: deprecate use as an option sifter
 (this branch is used by mg/rev-parse-lrbranches-locals.)

Some people might interpret "Deprecate" too strongly; the intent is that
we shouldn't keep piling parsing of new rev-list options to it and
discourage the use of "option sifter" in new programs.

* ab/cvsserver (2010-05-15) 6 commits
 - git-cvsserver: test for pserver authentication support
 - git-cvsserver: document making a password without htpasswd
 - git-cvsserver: Improved error handling for pserver
 - git-cvsserver: indent & clean up authdb code
 - git-cvsserver: use a password file cvsserver pserver
 - git-cvsserver: authentication support for pserver

Will merge to 'next'.

* eb/core-eol (2010-05-19) 4 commits
 - Add "core.eol" config variable
 - Rename the "crlf" attribute "text"
 - Add per-repository eol normalization
 - Add tests for per-repository eol normalization
 (this branch uses fg/autocrlf.)

Will merge to 'next'.

* jn/remote-set-branches (2010-05-19) 1 commit
 - Add git remote set-branches

Will merge to 'next'.

* js/maint-windows (2009-07-20) 2 commits
  (merged to 'next' on 2010-05-21 at 16abe03)
 + Recent MinGW has a C99 implementation of snprintf functions
 + mingw: use _commit to implement fsync

Will merge to 'next'.

* rs/grep-binary (2010-05-22) 8 commits
 - grep: support NUL chars in search strings for -F
 - grep: use REG_STARTEND for all matching if available
 - grep: continue case insensitive fixed string search after NUL chars
 - grep: use memmem() for fixed string search
 - grep: --name-only over binary
 - grep: --count over binary
 - grep: grep: refactor handling of binary mode options
 - grep: add test script for binary file handling

Will merge to 'next'.

* ab/submodule-foreach-toplevel (2010-05-21) 1 commit
 - git-submodule foreach: Add $toplevel variable

Will merge to 'next'.

* cb/ls-files-cdup (2010-05-26) 1 commit
 - ls-files: allow relative pathspec

Doesn't write_name() quote twice when prefix_offset is non-zero?

* jk/maint-pull-dry-run-noop (2010-05-25) 1 commit
 - pull: do nothing on --dry-run

Will merge to 'next'.

* jk/maint-sha1-file-name-fix (2010-05-22) 1 commit
 - remove over-eager caching in sha1_file_name

Will merge to 'next'.

* jk/url-decode (2010-05-23) 2 commits
 - decode file:// and ssh:// URLs
 - make url-related functions reusable

Will merge to 'next'.

* bw/diff-metainfo-color (2010-05-04) 1 commit
  (merged to 'next' on 2010-05-21 at 3aa552e)
 + diff: fix coloring of extended diff headers

* ph/clone-message-reword (2010-05-09) 1 commit
  (merged to 'next' on 2010-05-21 at 1785bd0)
 + clone: reword messages to match the end-user perception

* rc/ls-remote-default (2010-05-12) 1 commit
 - ls-remote: print URL when no repo is specified

Will merge to 'next'.

* fg/autocrlf (2010-05-12) 1 commit
  (merged to 'next' on 2010-05-21 at 5f43b66)
 + autocrlf: Make it work also for un-normalized repositories
 (this branch is used by eb/core-eol.)

* tc/merge-m-log (2010-05-11) 8 commits
  (merged to 'next' on 2010-05-21 at e889876)
 + merge: --log appends shortlog to message if specified
 + fmt-merge-msg: add function to append shortlog only
 + fmt-merge-msg: refactor merge title formatting
 + fmt-merge-msg: minor refactor of fmt_merge_msg()
 + merge: rename variable
 + merge: update comment
 + t7604-merge-custom-message: show that --log doesn't append to -m
 + t7604-merge-custom-message: shift expected output creation

* jn/gitweb-fastcgi (2010-05-07) 2 commits
  (merged to 'next' on 2010-05-21 at cb1724f)
 + gitweb: Add support for FastCGI, using CGI::Fast
 + gitweb: Put all per-connection code in run() subroutine

* jn/make-header-dependency (2010-05-08) 2 commits
  (merged to 'next' on 2010-05-21 at d4ed230)
 + Makefile: let header dependency checker override COMPUTE_HEADER_DEPENDENCIES
 + Makefile: fix header dependency checker to allow NO_CURL builds

* js/try-to-free-stackable (2010-05-08) 2 commits
  (merged to 'next' on 2010-05-21 at 4c1afcb)
 + Do not call release_pack_memory in malloc wrappers when GIT_TRACE is used
 + Have set_try_to_free_routine return the previous routine

* jn/gitweb-syntax-highlight (2010-04-27) 2 commits
 - gitweb: Refactor syntax highlighting support
 - gitweb: Syntax highlighting support

Will merge to 'next'.

* jn/maint-amend-missing-name (2010-05-02) 1 commit
  (merged to 'next' on 2010-05-09 at 9023332)
 + commit --amend: cope with missing display name

* rs/diff-no-minimal (2010-05-02) 1 commit
  (merged to 'next' on 2010-05-09 at 6c74aa0)
 + git diff too slow for a file

* ab/test-cleanup (2010-05-07) 2 commits
  (merged to 'next' on 2010-05-21 at a3cbd67)
 + Turn setup code in t2007-checkout-symlink.sh into a test
 + Move t6000lib.sh to lib-*

* jn/notes-doc (2010-05-08) 8 commits
  (merged to 'next' on 2010-05-21 at 1c28059)
 + Documentation/notes: nitpicks
 + Documentation/notes: clean up description of rewriting configuration
 + Documentation/notes: simplify treatment of default display refs
 + Documentation/log: add a CONFIGURATION section
 + Documentation/notes: simplify treatment of default notes ref
 + Documentation/notes: add configuration section
 + Documentation/notes: describe content of notes blobs
 + Documentation/notes: document format of notes trees

* cb/assume-unchanged-fix (2010-05-01) 2 commits
  (merged to 'next' on 2010-05-21 at bab2342)
 + Documentation: git-add does not update files marked "assume unchanged"
 + do not overwrite files marked "assume unchanged"

* wp/pretty-enhancement (2010-05-08) 4 commits
  (merged to 'next' on 2010-05-09 at eeaa4dc)
 + pretty: initialize new cmt_fmt_map to 0
 + pretty: add aliases for pretty formats
 + pretty: add infrastructure for commit format aliases
 + pretty: make it easier to add new formats

* hg/id-munging (2010-04-06) 2 commits
 - convert: Keep foreign $Id$ on checkout.
 - convert: Safer handling of $Id$ contraction.

Will merge to 'next'.

* js/async-thread (2010-03-09) 7 commits
  (merged to 'next' on 2010-05-21 at 9d31940)
 + Enable threaded async procedures whenever pthreads is available
  (merged to 'next' on 2010-05-04 at 2644e74)
 + Dying in an async procedure should only exit the thread, not the process.
 + Reimplement async procedures using pthreads
 + Windows: more pthreads functions
 + Fix signature of fcntl() compatibility dummy
 + Make report() from usage.c public as vreportf() and use it.
 + Modernize t5530-upload-pack-error.

(all except for the tip has been in 'next' since 2010-03-20).

* ld/discovery-limit-to-fs (2010-04-04) 1 commit
 - write-index: check and warn when worktree crosses a filesystem boundary

There might have been some valid objections to this but I cannot recall.
Will merge to 'next' unless I hear something within a few days.

--------------------------------------------------
[Discarded, perhaps can be rerolled]

* jp/hold-string-list-sanity (2010-04-06) 9 commits
 . string_list: Fix argument order for string_list_append
 . Merge branch 'sr/remote-helper-export' into HEAD
 . Merge branch 'js/grep-open' into HEAD
 . Merge branch 'sb/fmt-merge-msg' into HEAD
 . string_list: Fix argument order for string_list_lookup
 . string_list: Fix argument order for string_list_insert_at_index
 . string_list: Fix argument order for string_list_insert
 . string_list: Fix argument order for for_each_string_list
 . string_list: Fix argument order for print_string_list
 (this branch uses js/grep-open.)

Building this on top of slushy codebase is not a very promising endeavor.
Good thing to do, but it came at a bad time.

Re: What's cooking in git.git (Jun 2010, #01; Wed, 2)

[Tay Ray Chuan]

Hi,

On Thu, Jun 3, 2010 at 7:36 AM, Junio C Hamano <gitster@pobox.com> wrote:
> [New Topics]
> [snip]
> * tc/commit-abbrev-fix (2010-05-27) 3 commits
>  - commit::print_summary(): set rev_info.always_show_header to 1
>  - t7502-commit: add summary output tests for empty and merge commits
>  - t7502-commit: add tests for summary output
>
> Will merge to 'next'.  I am not quite happy about the "impossible to
> trigger" die message, though.  It is a good defensive programming to catch
> breakages caused by future changes that may invalidate the assumption this
> patch makes, but then the message should be worded as such to state that
> assumption, I think.

I'll try to remember to re-send the third patch with your suggestion
about the message.

> [Cooking]
> [snip]
> * tc/merge-m-log (2010-05-11) 8 commits
>  (merged to 'next' on 2010-05-21 at e889876)
>  + merge: --log appends shortlog to message if specified
>  + fmt-merge-msg: add function to append shortlog only
>  + fmt-merge-msg: refactor merge title formatting
>  + fmt-merge-msg: minor refactor of fmt_merge_msg()
>  + merge: rename variable
>  + merge: update comment
>  + t7604-merge-custom-message: show that --log doesn't append to -m
>  + t7604-merge-custom-message: shift expected output creation

FWIW, I've been using this feature for as long as it's been cooking -
no issues thus far.

-- 
Cheers,
Ray Chuan

Re: What's cooking in git.git (Jun 2010, #01; Wed, 2)

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 03.06.2010 01:36:
> Here are the topics that have been cooking.  Commits prefixed with '-' are
> only in 'pu' while commits prefixed with '+' are in 'next'.  The ones
> marked with '.' do not appear in any of the integration branches, but I am
> still holding onto them.
> 
> Many topics that have been cooking since 1.7.1 pre-release freeze period
> are now in master; maybe it is a good time to start freezing feature
> branches for 1.7.2---I think I said I'll keep this cycle shorter, no?
> 
> --------------------------------------------------
> [New Topics]
> * mg/status-b (2010-05-25) 2 commits
>  - Documentation+t5708: document and test status -s -b
>  - Show branch information in short output of git status
> 
> There are a few style violations that snuck in, but otherwise looked
> sensible.

I tried to follow surrounding style. Or do you mean the "if(" and local
variable initialisations in David'd patch? Do you want any of us to
recheck and resubmit?

I'm running with David's -b all the time, btw, it dramatically reduced
my need for long-status. Works great.

> --------------------------------------------------
> [Stalled -- would discard unless there are some movements soon]
> 
> * jc/rev-list-ancestry-path (2010-04-20) 1 commit
>  - revision: --ancestry-path
> 
> Just an illustration patch.  merge simplification logic used when
> pathspecs are in effect interacts with this rather badly.

I think it's still useful. Do all rev-walker flags need to interact
sanely with each other? We could just discourage --ancestry-path with
merge-simplification (and later try to reconcile them).

> --------------------------------------------------
> [Cooking]
> 
> * em/checkout-orphan (2010-05-21) 5 commits
>  - bash completion: add --orphan to 'git checkout'
>  - t3200: test -l with core.logAllRefUpdates options
>  - checkout --orphan: respect -l option always
>  - refs: split log_ref_write logic into log_ref_setup
>  - Documentation: alter checkout --orphan description
> 
> In <4BFE2461.5080501@drmicha.warpmail.net>, Michael J Gruber raised a
> valid request for a better explanation of superfluous files left behind
> and then are cleaned.  Other than that I think this is a sane thing to
> do.

Got no response but had the impression that retracting myself from the
discussion may improve the willingness of the submitter :)

> * mg/rev-parse-lrbranches-locals (2010-05-14) 1 commit
>  - revlist: Introduce --lrbranches and --locals revision specifiers
>  (this branch uses mg/rev-parse-option-sifter-deprecation.)
> 
> Hmmm...

Now I know what to do ;)

Another approach would be to have something like "ref aliases". I often
use origin/next..origin/pu and origin/next@{1}..origin/next for example.
Maybe allowing something like

[refalias]
	puextra	= origin/next..origin/pu
	punew	= origin/next@{1}..origin/next
	locals	= HEAD --branches --tags
	lrbranches = --branches --remotes

would make more sense and allows for personal choices. Hmmm? ;)

> 
> * mg/rev-parse-option-sifter-deprecation (2010-05-14) 3 commits
>  - t6018: make sure all tested symbolic names are different revs
>  - t6018: add tests for rev-list's --branches and --tags
>  - rev-parse: deprecate use as an option sifter
>  (this branch is used by mg/rev-parse-lrbranches-locals.)
> 
> Some people might interpret "Deprecate" too strongly; the intent is that
> we shouldn't keep piling parsing of new rev-list options to it and
> discourage the use of "option sifter" in new programs.

So, s/deprecated/discouraged/? Is there an alternative we should suggest
there for scripts needing to sift their options?

Cheers,
Michael

[PATCH 1/2] separate quoting and relative path generation

[Clemens Buchacher]

This is in preparation of relative path support for ls-files, which
quotes a path only if the line terminator is not the NUL character.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---

On Wed, Jun 02, 2010 at 04:36:56PM -0700, Junio C Hamano wrote:

> * cb/ls-files-cdup (2010-05-26) 1 commit
>  - ls-files: allow relative pathspec
> 
> Doesn't write_name() quote twice when prefix_offset is non-zero?

Yes. Fixed in the following two patches.

 quote.c |   69 +++++++++++++++++++++++++++++++++++++++++---------------------
 quote.h |    8 ++++++-
 2 files changed, 52 insertions(+), 25 deletions(-)

diff --git a/quote.c b/quote.c
index fc93435..2ae2c1f 100644
--- a/quote.c
+++ b/quote.c
@@ -295,42 +295,63 @@ void write_name_quotedpfx(const char *pfx, size_t pfxlen,
 	fputc(terminator, fp);
 }
 
-/* quote path as relative to the given prefix */
-char *quote_path_relative(const char *in, int len,
-			  struct strbuf *out, const char *prefix)
+void write_name_quoted_relative(const char *name, size_t len,
+				const char *prefix, size_t prefix_len,
+				FILE *fp, int terminator)
 {
-	int needquote;
+	struct strbuf sb = STRBUF_INIT;
 
+	char *path = path_relative(name, len, &sb, prefix, prefix_len);
+	write_name_quoted(path, fp, terminator);
+
+	strbuf_release(&sb);
+}
+
+/* give path as relative to prefix */
+char *path_relative(const char *in, int len,
+		    struct strbuf *out, const char *prefix, int prefix_len)
+{
 	if (len < 0)
 		len = strlen(in);
+	if (prefix && prefix_len < 0)
+		prefix_len = strlen(prefix);
 
-	/* "../" prefix itself does not need quoting, but "in" might. */
-	needquote = next_quote_pos(in, len) < len;
 	strbuf_setlen(out, 0);
 	strbuf_grow(out, len);
 
-	if (needquote)
-		strbuf_addch(out, '"');
-	if (prefix) {
-		int off = 0;
-		while (prefix[off] && off < len && prefix[off] == in[off])
-			if (prefix[off] == '/') {
-				prefix += off + 1;
-				in += off + 1;
-				len -= off + 1;
-				off = 0;
-			} else
-				off++;
-
-		for (; *prefix; prefix++)
-			if (*prefix == '/')
+	if (prefix_len > 0) {
+		int off = 0, i = 0;
+		while (i < prefix_len && i < len && prefix[i] == in[i]) {
+			if (prefix[i] == '/')
+				off = i + 1;
+			i++;
+		}
+		in += off;
+		len -= off;
+
+		while (i < prefix_len) {
+			if (prefix[i] == '/')
 				strbuf_addstr(out, "../");
+			i++;
+		}
 	}
+	strbuf_add(out, in, len);
+
+	return out->buf;
+}
+
+/* quote path as relative to the given prefix */
+char *quote_path_relative(const char *in, int len,
+			  struct strbuf *out, const char *prefix)
+{
+	char *rel;
+	size_t rel_len;
 
-	quote_c_style_counted (in, len, out, NULL, 1);
+	path_relative(in, len, out, prefix, -1);
+	rel = strbuf_detach(out, &rel_len);
+	quote_c_style_counted(rel, rel_len, out, NULL, 0);
+	free(rel);
 
-	if (needquote)
-		strbuf_addch(out, '"');
 	if (!out->len)
 		strbuf_addstr(out, "./");
 
diff --git a/quote.h b/quote.h
index f83eb23..e9e0221 100644
--- a/quote.h
+++ b/quote.h
@@ -54,9 +54,15 @@ extern void quote_two_c_style(struct strbuf *, const char *, const char *, int);
 extern void write_name_quoted(const char *name, FILE *, int terminator);
 extern void write_name_quotedpfx(const char *pfx, size_t pfxlen,
                                  const char *name, FILE *, int terminator);
+extern void write_name_quoted_relative(const char *name, size_t len,
+		const char *prefix, size_t prefix_len,
+		FILE *fp, int terminator);
 
+/* give path as relative to prefix */
+extern char *path_relative(const char *in, int len,
+		struct strbuf *out, const char *prefix, int prefix_len);
 /* quote path as relative to the given prefix */
-char *quote_path_relative(const char *in, int len,
+extern char *quote_path_relative(const char *in, int len,
 			  struct strbuf *out, const char *prefix);
 
 /* quoting as a string literal for other languages */
-- 
1.7.1.2.g2a651

[PATCH 2/2] ls-files: allow relative pathspec

[Clemens Buchacher]

git ls-files used to error out if given paths which point outside the current
working directory, such as '../'. We now allow such paths and the output is
analogous to git grep -l.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---

There are two changes with respect to the previous version of this
patch.

- write_name() now uses the new write_name_quoted_relative()
- prefix_offset is renamed to prefix_len, which is much more
  intuitive

Clemens

 builtin/ls-files.c |   75 ++++++++++++++++++++++++++++-----------------------
 t/t7010-setup.sh   |   12 +++-----
 2 files changed, 45 insertions(+), 42 deletions(-)

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index c0fbcdc..0804047 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -26,8 +26,9 @@ static int show_killed;
 static int show_valid_bit;
 static int line_terminator = '\n';
 
+static const char *prefix;
+static int max_prefix_len;
 static int prefix_len;
-static int prefix_offset;
 static const char **pathspec;
 static int error_unmatch;
 static char *ps_matched;
@@ -43,10 +44,15 @@ static const char *tag_modified = "";
 static const char *tag_skip_worktree = "";
 static const char *tag_resolve_undo = "";
 
+static void write_name(const char* name, size_t len)
+{
+	write_name_quoted_relative(name, len, prefix, prefix_len, stdout,
+			line_terminator);
+}
+
 static void show_dir_entry(const char *tag, struct dir_entry *ent)
 {
-	int len = prefix_len;
-	int offset = prefix_offset;
+	int len = max_prefix_len;
 
 	if (len >= ent->len)
 		die("git ls-files: internal error - directory entry not superset of prefix");
@@ -55,7 +61,7 @@ static void show_dir_entry(const char *tag, struct dir_entry *ent)
 		return;
 
 	fputs(tag, stdout);
-	write_name_quoted(ent->name + offset, stdout, line_terminator);
+	write_name(ent->name, ent->len);
 }
 
 static void show_other_files(struct dir_struct *dir)
@@ -121,8 +127,7 @@ static void show_killed_files(struct dir_struct *dir)
 
 static void show_ce_entry(const char *tag, struct cache_entry *ce)
 {
-	int len = prefix_len;
-	int offset = prefix_offset;
+	int len = max_prefix_len;
 
 	if (len >= ce_namelen(ce))
 		die("git ls-files: internal error - cache entry not superset of prefix");
@@ -156,20 +161,19 @@ static void show_ce_entry(const char *tag, struct cache_entry *ce)
 		       find_unique_abbrev(ce->sha1,abbrev),
 		       ce_stage(ce));
 	}
-	write_name_quoted(ce->name + offset, stdout, line_terminator);
+	write_name(ce->name, ce_namelen(ce));
 }
 
 static int show_one_ru(struct string_list_item *item, void *cbdata)
 {
-	int offset = prefix_offset;
 	const char *path = item->string;
 	struct resolve_undo_info *ui = item->util;
 	int i, len;
 
 	len = strlen(path);
-	if (len < prefix_len)
+	if (len < max_prefix_len)
 		return 0; /* outside of the prefix */
-	if (!match_pathspec(pathspec, path, len, prefix_len, ps_matched))
+	if (!match_pathspec(pathspec, path, len, max_prefix_len, ps_matched))
 		return 0; /* uninterested */
 	for (i = 0; i < 3; i++) {
 		if (!ui->mode[i])
@@ -177,19 +181,19 @@ static int show_one_ru(struct string_list_item *item, void *cbdata)
 		printf("%s%06o %s %d\t", tag_resolve_undo, ui->mode[i],
 		       find_unique_abbrev(ui->sha1[i], abbrev),
 		       i + 1);
-		write_name_quoted(path + offset, stdout, line_terminator);
+		write_name(path, len);
 	}
 	return 0;
 }
 
-static void show_ru_info(const char *prefix)
+static void show_ru_info(void)
 {
 	if (!the_index.resolve_undo)
 		return;
 	for_each_string_list(show_one_ru, the_index.resolve_undo, NULL);
 }
 
-static void show_files(struct dir_struct *dir, const char *prefix)
+static void show_files(struct dir_struct *dir)
 {
 	int i;
 
@@ -243,7 +247,7 @@ static void show_files(struct dir_struct *dir, const char *prefix)
  */
 static void prune_cache(const char *prefix)
 {
-	int pos = cache_name_pos(prefix, prefix_len);
+	int pos = cache_name_pos(prefix, max_prefix_len);
 	unsigned int first, last;
 
 	if (pos < 0)
@@ -256,7 +260,7 @@ static void prune_cache(const char *prefix)
 	while (last > first) {
 		int next = (last + first) >> 1;
 		struct cache_entry *ce = active_cache[next];
-		if (!strncmp(ce->name, prefix, prefix_len)) {
+		if (!strncmp(ce->name, prefix, max_prefix_len)) {
 			first = next+1;
 			continue;
 		}
@@ -265,11 +269,16 @@ static void prune_cache(const char *prefix)
 	active_nr = last;
 }
 
-static const char *verify_pathspec(const char *prefix)
+static const char *pathspec_prefix(const char *prefix)
 {
 	const char **p, *n, *prev;
 	unsigned long max;
 
+	if (!pathspec) {
+		max_prefix_len = prefix ? strlen(prefix) : 0;
+		return prefix;
+	}
+
 	prev = NULL;
 	max = PATH_MAX;
 	for (p = pathspec; (n = *p) != NULL; p++) {
@@ -291,10 +300,7 @@ static const char *verify_pathspec(const char *prefix)
 		}
 	}
 
-	if (prefix_offset > max || memcmp(prev, prefix, prefix_offset))
-		die("git ls-files: cannot generate relative filenames containing '..'");
-
-	prefix_len = max;
+	max_prefix_len = max;
 	return max ? xmemdupz(prev, max) : NULL;
 }
 
@@ -374,7 +380,7 @@ void overlay_tree_on_cache(const char *tree_name, const char *prefix)
 	}
 }
 
-int report_path_error(const char *ps_matched, const char **pathspec, int prefix_offset)
+int report_path_error(const char *ps_matched, const char **pathspec, int prefix_len)
 {
 	/*
 	 * Make sure all pathspec matched; otherwise it is an error.
@@ -404,7 +410,7 @@ int report_path_error(const char *ps_matched, const char **pathspec, int prefix_
 			continue;
 
 		error("pathspec '%s' did not match any file(s) known to git.",
-		      pathspec[num] + prefix_offset);
+		      pathspec[num] + prefix_len);
 		errors++;
 	}
 	return errors;
@@ -456,9 +462,10 @@ static int option_parse_exclude_standard(const struct option *opt,
 	return 0;
 }
 
-int cmd_ls_files(int argc, const char **argv, const char *prefix)
+int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 {
 	int require_work_tree = 0, show_tag = 0;
+	const char *max_prefix;
 	struct dir_struct dir;
 	struct option builtin_ls_files_options[] = {
 		{ OPTION_CALLBACK, 'z', NULL, NULL, NULL,
@@ -504,7 +511,7 @@ int cmd_ls_files(int argc, const char **argv, const char *prefix)
 		{ OPTION_CALLBACK, 0, "exclude-standard", &dir, NULL,
 			"add the standard git exclusions",
 			PARSE_OPT_NOARG, option_parse_exclude_standard },
-		{ OPTION_SET_INT, 0, "full-name", &prefix_offset, NULL,
+		{ OPTION_SET_INT, 0, "full-name", &prefix_len, NULL,
 			"make the output relative to the project top directory",
 			PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL },
 		OPT_BOOLEAN(0, "error-unmatch", &error_unmatch,
@@ -516,8 +523,9 @@ int cmd_ls_files(int argc, const char **argv, const char *prefix)
 	};
 
 	memset(&dir, 0, sizeof(dir));
+	prefix = cmd_prefix;
 	if (prefix)
-		prefix_offset = strlen(prefix);
+		prefix_len = strlen(prefix);
 	git_config(git_default_config, NULL);
 
 	if (read_cache() < 0)
@@ -555,9 +563,8 @@ int cmd_ls_files(int argc, const char **argv, const char *prefix)
 	if (pathspec)
 		strip_trailing_slash_from_submodules();
 
-	/* Verify that the pathspec matches the prefix */
-	if (pathspec)
-		prefix = verify_pathspec(prefix);
+	/* Find common prefix for all pathspec's */
+	max_prefix = pathspec_prefix(prefix);
 
 	/* Treat unmatching pathspec elements as errors */
 	if (pathspec && error_unmatch) {
@@ -575,8 +582,8 @@ int cmd_ls_files(int argc, const char **argv, const char *prefix)
 	      show_killed | show_modified | show_resolve_undo))
 		show_cached = 1;
 
-	if (prefix)
-		prune_cache(prefix);
+	if (max_prefix)
+		prune_cache(max_prefix);
 	if (with_tree) {
 		/*
 		 * Basic sanity check; show-stages and show-unmerged
@@ -584,15 +591,15 @@ int cmd_ls_files(int argc, const char **argv, const char *prefix)
 		 */
 		if (show_stage || show_unmerged)
 			die("ls-files --with-tree is incompatible with -s or -u");
-		overlay_tree_on_cache(with_tree, prefix);
+		overlay_tree_on_cache(with_tree, max_prefix);
 	}
-	show_files(&dir, prefix);
+	show_files(&dir);
 	if (show_resolve_undo)
-		show_ru_info(prefix);
+		show_ru_info();
 
 	if (ps_matched) {
 		int bad;
-		bad = report_path_error(ps_matched, pathspec, prefix_offset);
+		bad = report_path_error(ps_matched, pathspec, prefix_len);
 		if (bad)
 			fprintf(stderr, "Did you forget to 'git add'?\n");
 
diff --git a/t/t7010-setup.sh b/t/t7010-setup.sh
index d8a7c79..0335a9a 100755
--- a/t/t7010-setup.sh
+++ b/t/t7010-setup.sh
@@ -103,14 +103,10 @@ test_expect_success 'git ls-files (relative #3)' '
 	git add a &&
 	(
 		cd a/b &&
-		if git ls-files "../e/f"
-		then
-			echo Gaah, should have failed
-			exit 1
-		else
-			: happy
-		fi
-	)
+		git ls-files "../e/f"
+	)  >current &&
+	echo ../e/f >expect &&
+	test_cmp expect current
 
 '
 
-- 
1.7.1.2.g2a651

Re: What's cooking in git.git (Jun 2010, #01; Wed, 2)

[Thomas Rast]

Junio C Hamano wrote:
> * ld/discovery-limit-to-fs (2010-04-04) 1 commit
>  - write-index: check and warn when worktree crosses a filesystem boundary
> 
> There might have been some valid objections to this but I cannot recall.
> Will merge to 'next' unless I hear something within a few days.

Hmm, this actually got me wondering a bit if you meant my comment on
the original series, relating to my use of tmpfs for t/ [1].  Peff
pointed me to an existing and better solution and you rightfully
ignored this.

The patch above is the one at [2] and did not get any replies, so I
think you do not even have to wait :-)

(FWIW, I think it's a good idea to warn but I didn't look at the patch
much.)


[1] http://thread.gmane.org/gmane.comp.version-control.git/142436/focus=143617
[2] http://thread.gmane.org/gmane.comp.version-control.git/142436/focus=143959

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

mg/rev-parse-option-sifter-deprecation

[Jonathan Nieder]

Hi,

Michael J Gruber wrote:
> Junio C Hamano venit, vidit, dixit 03.06.2010 01:36:

>> Some people might interpret "Deprecate" too strongly; the intent is that
>> we shouldn't keep piling parsing of new rev-list options to it and
>> discourage the use of "option sifter" in new programs.
>
> So, s/deprecated/discouraged/? Is there an alternative we should suggest
> there for scripts needing to sift their options?

Ah, I was wondering about this, too.

Most git tools use parseopt now, whether they are written in sh or C.
When I first read this deprecation, I thought, "Wait --- so rev-parse
--parseopt is going away?"  So it might make sense to mention
rev-parse --parseopt separately in the description section.

As for --revs-only and --no-revs, the problem aiui is that we have no
in-tree users for them except git filter-branch.  Worse still, they
share no code with setup_revisions().  Would it be sensible to change
this?

 - teach setup_revisions() to use an option sifter instead of parsing
   options in one pass (probably a bad idea), or

 - give setup_revisions a second identity as an option sifter, or

 - teach filter-branch to do its own sifting of filespec arguments
   from revision arguments, and let rev-parse --revs-only rot

Unfortunately, any one of these changes would take time; so to be
realistic, regardless of what else happens, the deprecation notice
seems like the right thing to do.

| You can use linkgit:git-rev-list[1] directly or
| +linkgit:git-for-each-ref[1] for scripting.

I do not understand the reference to for-each-ref here.

Thanks,
Jonathan

Re: What's cooking in git.git (Jun 2010, #01; Wed, 2)

[Eyvind Bernhardsen]

Hi,

On 3. juni 2010, at 01.36, Junio C Hamano wrote:

> * eb/core-eol (2010-05-19) 4 commits
> - Add "core.eol" config variable
> - Rename the "crlf" attribute "text"
> - Add per-repository eol normalization
> - Add tests for per-repository eol normalization
> (this branch uses fg/autocrlf.)
> 
> Will merge to 'next'.

I have a fixup for the "core.eol" commit that modifies "core.autocrlf"'s behaviour and fixes the documentation, as discussed on the list.  Unfortunately it's on a machine that's not accessible to me right now, but I'll send it tomorrow.
-- 
Eyvind

Re: [PATCH 1/2] separate quoting and relative path generation

[Junio C Hamano]

Clemens Buchacher <drizzd@aon.at> writes:

> +/* give path as relative to prefix */
> +char *path_relative(const char *in, int len,
> +		    struct strbuf *out, const char *prefix, int prefix_len)
> +{
>  	if (len < 0)
>  		len = strlen(in);
> +	if (prefix && prefix_len < 0)
> +		prefix_len = strlen(prefix);
>  
>  	strbuf_setlen(out, 0);
>  	strbuf_grow(out, len);
>  
> +	if (prefix_len > 0) {
> +		int off = 0, i = 0;
> +		while (i < prefix_len && i < len && prefix[i] == in[i]) {
> +			if (prefix[i] == '/')
> +				off = i + 1;
> +			i++;
> +		}
> +		in += off;
> +		len -= off;
> +
> +		while (i < prefix_len) {
> +			if (prefix[i] == '/')
>  				strbuf_addstr(out, "../");
> +			i++;
> +		}
>  	}
> +	strbuf_add(out, in, len);
> +
> +	return out->buf;
> +}

Hmm...  I wonder if we really want to always make a copy of the string in
the majority of the case where there is no need to add ../ and the path
does not have any funny characters that needs quoting.  In such a case,
shouldn't write_name() be just moving the pointers into the original
string to skip the $(cwd) part and writing the remainder of the string
out, without any extra allocation nor copy?  IIUC, that is what the
original did using write_name_quoted().

[PATCH] optimize path_relative()

[Clemens Buchacher]

Avoid copying to strbuf in case a subset of the original string can
be returned.

Since the strbuf is no longer guaranteed to be updated, this
function is different from quote_path_relative(). To avoid
confusion, do not export it.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
On Thu, Jun 03, 2010 at 03:16:33PM -0700, Junio C Hamano wrote:
> Clemens Buchacher <drizzd@aon.at> writes:
>
> > +	strbuf_add(out, in, len);
> > +
> > +	return out->buf;
> > +}
> 
> Hmm...  I wonder if we really want to always make a copy of the string in
> the majority of the case where there is no need to add ../ and the path
> does not have any funny characters that needs quoting.

Right. Implemented below. I noticed a few more redundancies along
the way, so the following could be squashed into 1/2 "separate
quoting and relative path generation" or applied on top of the
existing two patches.

Please let me know if you'd rather have a resend.

Clemens

 quote.c |   72 ++++++++++++++++++++++++++++++++++++--------------------------
 quote.h |    3 --
 2 files changed, 42 insertions(+), 33 deletions(-)

diff --git a/quote.c b/quote.c
index 2ae2c1f..481541a 100644
--- a/quote.c
+++ b/quote.c
@@ -295,62 +295,74 @@ void write_name_quotedpfx(const char *pfx, size_t pfxlen,
 	fputc(terminator, fp);
 }
 
+static const char *path_relative(const char *in, int len,
+				 struct strbuf *sb, const char *prefix,
+				 int prefix_len);
+
 void write_name_quoted_relative(const char *name, size_t len,
 				const char *prefix, size_t prefix_len,
 				FILE *fp, int terminator)
 {
 	struct strbuf sb = STRBUF_INIT;
 
-	char *path = path_relative(name, len, &sb, prefix, prefix_len);
-	write_name_quoted(path, fp, terminator);
+	name = path_relative(name, len, &sb, prefix, prefix_len);
+	write_name_quoted(name, fp, terminator);
 
 	strbuf_release(&sb);
 }
 
-/* give path as relative to prefix */
-char *path_relative(const char *in, int len,
-		    struct strbuf *out, const char *prefix, int prefix_len)
+/*
+ * Give path as relative to prefix.
+ *
+ * The strbuf may or may not be used, so do not assume it contains the
+ * returned path.
+ */
+static const char *path_relative(const char *in, int len,
+				 struct strbuf *sb, const char *prefix,
+				 int prefix_len)
 {
+	int off, i;
+
 	if (len < 0)
 		len = strlen(in);
 	if (prefix && prefix_len < 0)
 		prefix_len = strlen(prefix);
 
-	strbuf_setlen(out, 0);
-	strbuf_grow(out, len);
+	off = 0;
+	i = 0;
+	while (i < prefix_len && i < len && prefix[i] == in[i]) {
+		if (prefix[i] == '/')
+			off = i + 1;
+		i++;
+	}
+	in += off;
+	len -= off;
 
-	if (prefix_len > 0) {
-		int off = 0, i = 0;
-		while (i < prefix_len && i < len && prefix[i] == in[i]) {
-			if (prefix[i] == '/')
-				off = i + 1;
-			i++;
-		}
-		in += off;
-		len -= off;
+	if (i == prefix_len)
+		return in;
 
-		while (i < prefix_len) {
-			if (prefix[i] == '/')
-				strbuf_addstr(out, "../");
-			i++;
-		}
+	strbuf_reset(sb);
+	strbuf_grow(sb, len);
+
+	while (i < prefix_len) {
+		if (prefix[i] == '/')
+			strbuf_addstr(sb, "../");
+		i++;
 	}
-	strbuf_add(out, in, len);
+	strbuf_add(sb, in, len);
 
-	return out->buf;
+	return sb->buf;
 }
 
 /* quote path as relative to the given prefix */
 char *quote_path_relative(const char *in, int len,
 			  struct strbuf *out, const char *prefix)
 {
-	char *rel;
-	size_t rel_len;
-
-	path_relative(in, len, out, prefix, -1);
-	rel = strbuf_detach(out, &rel_len);
-	quote_c_style_counted(rel, rel_len, out, NULL, 0);
-	free(rel);
+	struct strbuf sb = STRBUF_INIT;
+	const char *rel = path_relative(in, len, &sb, prefix, -1);
+	strbuf_reset(out);
+	quote_c_style_counted(rel, strlen(rel), out, NULL, 0);
+	strbuf_release(&sb);
 
 	if (!out->len)
 		strbuf_addstr(out, "./");
diff --git a/quote.h b/quote.h
index e9e0221..38003bf 100644
--- a/quote.h
+++ b/quote.h
@@ -58,9 +58,6 @@ extern void write_name_quoted_relative(const char *name, size_t len,
 		const char *prefix, size_t prefix_len,
 		FILE *fp, int terminator);
 
-/* give path as relative to prefix */
-extern char *path_relative(const char *in, int len,
-		struct strbuf *out, const char *prefix, int prefix_len);
 /* quote path as relative to the given prefix */
 extern char *quote_path_relative(const char *in, int len,
 			  struct strbuf *out, const char *prefix);
-- 
1.7.1.2.ga1f6e

Re: [PATCH] optimize path_relative()

[Clemens Buchacher]

On Fri, Jun 04, 2010 at 09:44:43AM +0200, Clemens Buchacher wrote:

> +static const char *path_relative(const char *in, int len,
> +				 struct strbuf *sb, const char *prefix,
> +				 int prefix_len)
>  {
> +	int off, i;
> +
>  	if (len < 0)
>  		len = strlen(in);
>  	if (prefix && prefix_len < 0)
>  		prefix_len = strlen(prefix);
>  
> -	strbuf_setlen(out, 0);
> -	strbuf_grow(out, len);
> +	off = 0;
> +	i = 0;
> +	while (i < prefix_len && i < len && prefix[i] == in[i]) {
> +		if (prefix[i] == '/')
> +			off = i + 1;
> +		i++;
> +	}
> +	in += off;
> +	len -= off;
>  
> -	if (prefix_len > 0) {
> -		int off = 0, i = 0;
> -		while (i < prefix_len && i < len && prefix[i] == in[i]) {
> -			if (prefix[i] == '/')
> -				off = i + 1;
> -			i++;
> -		}
> -		in += off;
> -		len -= off;
> +	if (i == prefix_len)
> +		return in;
>  
> -		while (i < prefix_len) {
> -			if (prefix[i] == '/')
> -				strbuf_addstr(out, "../");
> -			i++;
> -		}
> +	strbuf_reset(sb);
> +	strbuf_grow(sb, len);
> +
> +	while (i < prefix_len) {
> +		if (prefix[i] == '/')
> +			strbuf_addstr(sb, "../");
> +		i++;
>  	}

By the way, I noticed that we rely on the fact that a non-empty
prefix ends with '/'. Is that ok?

> -	strbuf_add(out, in, len);
> +	strbuf_add(sb, in, len);
>  
> -	return out->buf;
> +	return sb->buf;
>  }

[PATCH v3 3/3] commit::print_summary(): set rev_info.always_show_header to 1

[Tay Ray Chuan]

This attempts to fix a regression in git-commit, where non-abbreviated
SHA-1s were printed in the summary.

One possible fix would be to set ctx.abbrev to DEFAULT_ABBREV in the
`if` block.

However, we remove this codeblock altogether, and set
rev.always_show_header. This way, we use back the same show_log()
mechanism (instead of format_commit_message()).

Quoting log-tree.c:560:

	shown = log_tree_diff(opt, commit, &log);
	if (!shown && opt->loginfo && opt->always_show_header) {
		log.parent = NULL;
		show_log(opt);
		shown = 1;
	}

This is the only area that always_show_header is checked, so the
setting of this flag should only affect this area.

Also, we now die() if log_tree_commit() returns false. Based on the
existing underlying codepaths (log_tree_commit(), log_tree_diff(),
log_tree_diff_flush(), to name a few), this should not occur; changes to
these codepaths may warrant a revision of our handling of this
situation. Tests #2 and #3 in t7502 should aid in detecting such
breakages.

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---

This is a reworked version of the third patch of the
'tc/commit-abbrev-fix' series; there are no changes to the first and
second patches.

Changes from v2:
 - shift around 2nd and 3rd paras.
 - mention the die() and our assumption that it won't be triggered, as
   suggested by Junio.

 builtin/commit.c  |   13 ++++---------
 t/t7502-commit.sh |    4 ++--
 2 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index a4e4966..2884d0c 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1148,7 +1148,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.buf, &rev);
-	rev.always_show_header = 0;
+	rev.always_show_header = 1;
 	rev.diffopt.detect_rename = 1;
 	rev.diffopt.rename_limit = 100;
 	rev.diffopt.break_opt = 0;
@@ -1162,14 +1162,9 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
 				head,
 		initial_commit ? " (root-commit)" : "");

-	if (!log_tree_commit(&rev, commit)) {
-		struct pretty_print_context ctx = {0};
-		struct strbuf buf = STRBUF_INIT;
-		ctx.date_mode = DATE_NORMAL;
-		format_commit_message(commit, format.buf + 7, &buf, &ctx);
-		printf("%s\n", buf.buf);
-		strbuf_release(&buf);
-	}
+	if (!log_tree_commit(&rev, commit))
+		die("unable to print summary");
+
 	strbuf_release(&format);
 }

diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index b10541d..08c0247 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -36,12 +36,12 @@ test_expect_success 'output summary format' '
 	check_summary_oneline "" "a change"
 '

-test_expect_failure 'output summary format for commit with an empty diff' '
+test_expect_success 'output summary format for commit with an empty diff' '

 	check_summary_oneline "" "empty" "--allow-empty"
 '

-test_expect_failure 'output summary format for merges' '
+test_expect_success 'output summary format for merges' '

 	git checkout -b recursive-base &&
 	test_commit base file1 &&
--
1.7.1.189.g07419

Re: [PATCH v3 3/3] commit::print_summary(): set rev_info.always_show_header to 1

[Tay Ray Chuan]

Hi Will,

sorry, I forgot to Cc you for this new version.

-- 
Cheers,
Ray Chuan

On Fri, Jun 4, 2010 at 4:21 PM, Tay Ray Chuan <rctay89@gmail.com> wrote:
> This attempts to fix a regression in git-commit, where non-abbreviated
> SHA-1s were printed in the summary.
>
> One possible fix would be to set ctx.abbrev to DEFAULT_ABBREV in the
> `if` block.
>
> However, we remove this codeblock altogether, and set
> rev.always_show_header. This way, we use back the same show_log()
> mechanism (instead of format_commit_message()).
>
> Quoting log-tree.c:560:
>
>        shown = log_tree_diff(opt, commit, &log);
>        if (!shown && opt->loginfo && opt->always_show_header) {
>                log.parent = NULL;
>                show_log(opt);
>                shown = 1;
>        }
>
> This is the only area that always_show_header is checked, so the
> setting of this flag should only affect this area.
>
> Also, we now die() if log_tree_commit() returns false. Based on the
> existing underlying codepaths (log_tree_commit(), log_tree_diff(),
> log_tree_diff_flush(), to name a few), this should not occur; changes to
> these codepaths may warrant a revision of our handling of this
> situation. Tests #2 and #3 in t7502 should aid in detecting such
> breakages.
>
> Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
> ---
>
> This is a reworked version of the third patch of the
> 'tc/commit-abbrev-fix' series; there are no changes to the first and
> second patches.
>
> Changes from v2:
>  - shift around 2nd and 3rd paras.
>  - mention the die() and our assumption that it won't be triggered, as
>   suggested by Junio.
>
>  builtin/commit.c  |   13 ++++---------
>  t/t7502-commit.sh |    4 ++--
>  2 files changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index a4e4966..2884d0c 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1148,7 +1148,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.buf, &rev);
> -       rev.always_show_header = 0;
> +       rev.always_show_header = 1;
>        rev.diffopt.detect_rename = 1;
>        rev.diffopt.rename_limit = 100;
>        rev.diffopt.break_opt = 0;
> @@ -1162,14 +1162,9 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
>                                head,
>                initial_commit ? " (root-commit)" : "");
>
> -       if (!log_tree_commit(&rev, commit)) {
> -               struct pretty_print_context ctx = {0};
> -               struct strbuf buf = STRBUF_INIT;
> -               ctx.date_mode = DATE_NORMAL;
> -               format_commit_message(commit, format.buf + 7, &buf, &ctx);
> -               printf("%s\n", buf.buf);
> -               strbuf_release(&buf);
> -       }
> +       if (!log_tree_commit(&rev, commit))
> +               die("unable to print summary");
> +
>        strbuf_release(&format);
>  }
>
> diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
> index b10541d..08c0247 100755
> --- a/t/t7502-commit.sh
> +++ b/t/t7502-commit.sh
> @@ -36,12 +36,12 @@ test_expect_success 'output summary format' '
>        check_summary_oneline "" "a change"
>  '
>
> -test_expect_failure 'output summary format for commit with an empty diff' '
> +test_expect_success 'output summary format for commit with an empty diff' '
>
>        check_summary_oneline "" "empty" "--allow-empty"
>  '
>
> -test_expect_failure 'output summary format for merges' '
> +test_expect_success 'output summary format for merges' '
>
>        git checkout -b recursive-base &&
>        test_commit base file1 &&
> --
> 1.7.1.189.g07419
>
>

Re: What's cooking in git.git (Jun 2010, #01; Wed, 2)

[Jonathan Nieder]

Junio C Hamano wrote:

> * gv/portable (2010-05-14) 18 commits
[...]
>  - Do not use "diff" found on PATH while building and installing

That patch (d1b1a919) breaks merge-one-file when run outside the test
suite as far as I can tell.

Maybe it could be made portable with ‘git diff --no-index’?

diff --git a/git-merge-one-file.sh b/git-merge-one-file.sh
index 3145009..d067894 100755
--- a/git-merge-one-file.sh
+++ b/git-merge-one-file.sh
@@ -107,7 +107,7 @@ case "${1:-.}${2:-.}${3:-.}" in
 		# remove lines that are unique to ours.
 		orig=`git-unpack-file $2`
 		sz0=`wc -c <"$orig"`
-		$DIFF -u -La/$orig -Lb/$orig $orig $src2 | git apply --no-add
+		diff -u -La/$orig -Lb/$orig $orig $src2 | git apply --no-add
 		sz1=`wc -c <"$orig"`
 
 		# If we do not have enough common material, it is not
-- 

Re: [PATCH v3 3/3] commit::print_summary(): set rev_info.always_show_header to 1

[Junio C Hamano]

Tay Ray Chuan <rctay89@gmail.com> writes:

>  - mention the die() and our assumption that it won't be triggered, as
>    suggested by Junio.

What I meant was to state that in the die() message, not in the commit
log, so that a bug reporter (or preferably people hacking on git that
introduces a regression to trigger this die()) can quickly diagnose what
assumption this new code relies on was violated.

[PATCH v2] optimize path_relative()

[Clemens Buchacher]

Avoid copying to strbuf in case a subset of the original string can
be returned.

Since the strbuf is no longer guaranteed to be updated, this
function is different from quote_path_relative(). To avoid
confusion, do not export it.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---

On Fri, Jun 04, 2010 at 09:44:43AM +0200, Clemens Buchacher wrote:

>  	if (prefix && prefix_len < 0)
>  		prefix_len = strlen(prefix);
[...]
> +	if (i == prefix_len)
> +		return in;

This should have been i >= prefix_len, in case prefix = NULL and
prefix_len = -1.  (Can only happen with git clean at the moment.)

 quote.c |   72 ++++++++++++++++++++++++++++++++++++--------------------------
 quote.h |    3 --
 2 files changed, 42 insertions(+), 33 deletions(-)

diff --git a/quote.c b/quote.c
index 2ae2c1f..63d3b01 100644
--- a/quote.c
+++ b/quote.c
@@ -295,62 +295,74 @@ void write_name_quotedpfx(const char *pfx, size_t pfxlen,
 	fputc(terminator, fp);
 }
 
+static const char *path_relative(const char *in, int len,
+				 struct strbuf *sb, const char *prefix,
+				 int prefix_len);
+
 void write_name_quoted_relative(const char *name, size_t len,
 				const char *prefix, size_t prefix_len,
 				FILE *fp, int terminator)
 {
 	struct strbuf sb = STRBUF_INIT;
 
-	char *path = path_relative(name, len, &sb, prefix, prefix_len);
-	write_name_quoted(path, fp, terminator);
+	name = path_relative(name, len, &sb, prefix, prefix_len);
+	write_name_quoted(name, fp, terminator);
 
 	strbuf_release(&sb);
 }
 
-/* give path as relative to prefix */
-char *path_relative(const char *in, int len,
-		    struct strbuf *out, const char *prefix, int prefix_len)
+/*
+ * Give path as relative to prefix.
+ *
+ * The strbuf may or may not be used, so do not assume it contains the
+ * returned path.
+ */
+static const char *path_relative(const char *in, int len,
+				 struct strbuf *sb, const char *prefix,
+				 int prefix_len)
 {
+	int off, i;
+
 	if (len < 0)
 		len = strlen(in);
 	if (prefix && prefix_len < 0)
 		prefix_len = strlen(prefix);
 
-	strbuf_setlen(out, 0);
-	strbuf_grow(out, len);
+	off = 0;
+	i = 0;
+	while (i < prefix_len && i < len && prefix[i] == in[i]) {
+		if (prefix[i] == '/')
+			off = i + 1;
+		i++;
+	}
+	in += off;
+	len -= off;
 
-	if (prefix_len > 0) {
-		int off = 0, i = 0;
-		while (i < prefix_len && i < len && prefix[i] == in[i]) {
-			if (prefix[i] == '/')
-				off = i + 1;
-			i++;
-		}
-		in += off;
-		len -= off;
+	if (i >= prefix_len)
+		return in;
 
-		while (i < prefix_len) {
-			if (prefix[i] == '/')
-				strbuf_addstr(out, "../");
-			i++;
-		}
+	strbuf_reset(sb);
+	strbuf_grow(sb, len);
+
+	while (i < prefix_len) {
+		if (prefix[i] == '/')
+			strbuf_addstr(sb, "../");
+		i++;
 	}
-	strbuf_add(out, in, len);
+	strbuf_add(sb, in, len);
 
-	return out->buf;
+	return sb->buf;
 }
 
 /* quote path as relative to the given prefix */
 char *quote_path_relative(const char *in, int len,
 			  struct strbuf *out, const char *prefix)
 {
-	char *rel;
-	size_t rel_len;
-
-	path_relative(in, len, out, prefix, -1);
-	rel = strbuf_detach(out, &rel_len);
-	quote_c_style_counted(rel, rel_len, out, NULL, 0);
-	free(rel);
+	struct strbuf sb = STRBUF_INIT;
+	const char *rel = path_relative(in, len, &sb, prefix, -1);
+	strbuf_reset(out);
+	quote_c_style_counted(rel, strlen(rel), out, NULL, 0);
+	strbuf_release(&sb);
 
 	if (!out->len)
 		strbuf_addstr(out, "./");
diff --git a/quote.h b/quote.h
index e9e0221..38003bf 100644
--- a/quote.h
+++ b/quote.h
@@ -58,9 +58,6 @@ extern void write_name_quoted_relative(const char *name, size_t len,
 		const char *prefix, size_t prefix_len,
 		FILE *fp, int terminator);
 
-/* give path as relative to prefix */
-extern char *path_relative(const char *in, int len,
-		struct strbuf *out, const char *prefix, int prefix_len);
 /* quote path as relative to the given prefix */
 extern char *quote_path_relative(const char *in, int len,
 			  struct strbuf *out, const char *prefix);
-- 
1.7.1.2.ga1f6e

[PATCH] setup: document prefix

[Clemens Buchacher]

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---

On Fri, Jun 04, 2010 at 09:50:34AM +0200, Clemens Buchacher wrote:
>
> By the way, I noticed that we rely on the fact that a non-empty
> prefix ends with '/'. Is that ok?

I checked and the prefix does always end in a slash currently, but
this guarantee is well hidden in the code. I would feel more at
ease to continue relying on this with the comment below.

Regards,
Clemens

 setup.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/setup.c b/setup.c
index 5716d90..0e4cfe6 100644
--- a/setup.c
+++ b/setup.c
@@ -519,6 +519,12 @@ int check_repository_format(void)
 	return check_repository_format_gently(NULL);
 }
 
+/*
+ * Returns the "prefix", a path to the current working directory
+ * relative to the work tree root, or NULL, if the current working
+ * directory is not a strict subdirectory of the work tree root. The
+ * prefix always ends with a '/' character.
+ */
 const char *setup_git_directory(void)
 {
 	const char *retval = setup_git_directory_gently(NULL);
-- 
1.7.1.2.ga1f6e

Re: [PATCH v2] optimize path_relative()

[Junio C Hamano]

Clemens Buchacher <drizzd@aon.at> writes:

> Avoid copying to strbuf in case a subset of the original string can
> be returned.
>
> Since the strbuf is no longer guaranteed to be updated, this
> function is different from quote_path_relative(). To avoid
> confusion, do not export it.
>
> Signed-off-by: Clemens Buchacher <drizzd@aon.at>

The version of path_relative() after this patch looks much easier to
follow.

I noticed that there is a similar function write_name_quotedpfx() defined
in the same file, and wondered if we can do something similar to avoid the
whole allocation business.  But that would only be a microoptimize useful
for write_name_quoted_relative() and not for quote_path_relative() that
has a lot more callers, so it would probably not be worth it.

Thanks.

Re: What's cooking in git.git (Jun 2010, #01; Wed, 2)

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Junio C Hamano wrote:
>
>> * gv/portable (2010-05-14) 18 commits
> [...]
>>  - Do not use "diff" found on PATH while building and installing
>
> That patch (d1b1a919) breaks merge-one-file when run outside the test
> suite as far as I can tell.

Thanks for catching.  I wonder if something like this would be better in
that it wouldn't break people who do not use custom "DIFF" while it would
help people who do at the same time...

 Makefile              |    2 ++
 git-merge-one-file.sh |    2 +-
 2 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/Makefile b/Makefile
index bcb84be..0367e8a 100644
--- a/Makefile
+++ b/Makefile
@@ -1494,6 +1494,7 @@ SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))
 PERL_PATH_SQ = $(subst ','\'',$(PERL_PATH))
 PYTHON_PATH_SQ = $(subst ','\'',$(PYTHON_PATH))
 TCLTK_PATH_SQ = $(subst ','\'',$(TCLTK_PATH))
+DIFF_SQ = $(subst ','\'',$(DIFF))
 
 LIBS = $(GITLIBS) $(EXTLIBS)
 
@@ -1582,6 +1583,7 @@ define cmd_munge_script
 $(RM) $@ $@+ && \
 sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
     -e 's|@SHELL_PATH@|$(SHELL_PATH_SQ)|' \
+    -e 's|@@DIFF@@|$(DIFF_SQ)|' \
     -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \
     -e 's/@@NO_CURL@@/$(NO_CURL)/g' \
     -e $(BROKEN_PATH_FIX) \
diff --git a/git-merge-one-file.sh b/git-merge-one-file.sh
index 3145009..b86402a 100755
--- a/git-merge-one-file.sh
+++ b/git-merge-one-file.sh
@@ -107,7 +107,7 @@ case "${1:-.}${2:-.}${3:-.}" in
 		# remove lines that are unique to ours.
 		orig=`git-unpack-file $2`
 		sz0=`wc -c <"$orig"`
-		$DIFF -u -La/$orig -Lb/$orig $orig $src2 | git apply --no-add
+		@@DIFF@@ -u -La/$orig -Lb/$orig $orig $src2 | git apply --no-add
 		sz1=`wc -c <"$orig"`
 
 		# If we do not have enough common material, it is not

Re: What's cooking in git.git (Jun 2010, #01; Wed, 2)

[Jonathan Nieder]

Jun 05, 2010 at 11:07:32AM -0700, Junio C Hamano wrote:

> @@ -1582,6 +1583,7 @@ define cmd_munge_script
>  $(RM) $@ $@+ && \
>  sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
>      -e 's|@SHELL_PATH@|$(SHELL_PATH_SQ)|' \
> +    -e 's|@@DIFF@@|$(DIFF_SQ)|' \
>      -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \
>      -e 's/@@NO_CURL@@/$(NO_CURL)/g' \
>      -e $(BROKEN_PATH_FIX) \
> diff --git a/git-merge-one-file.sh b/git-merge-one-file.sh
> index 3145009..b86402a 100755
> --- a/git-merge-one-file.sh
> +++ b/git-merge-one-file.sh
> @@ -107,7 +107,7 @@ case "${1:-.}${2:-.}${3:-.}" in
>  		# remove lines that are unique to ours.
>  		orig=`git-unpack-file $2`
>  		sz0=`wc -c <"$orig"`
> -		$DIFF -u -La/$orig -Lb/$orig $orig $src2 | git apply --no-add
> +		@@DIFF@@ -u -La/$orig -Lb/$orig $orig $src2 | git apply --no-add
>  		sz1=`wc -c <"$orig"`
>  
>  		# If we do not have enough common material, it is not

Looks good to me.  Thanks.

Re: What's cooking in git.git (Jun 2010, #01; Wed, 2)

[Sverre Rabbelier]

Heya,

On Sat, Jun 5, 2010 at 20:07, Junio C Hamano <gitster@pobox.com> wrote:
>> That patch (d1b1a919) breaks merge-one-file when run outside the test
>> suite as far as I can tell.
>
> Thanks for catching.  I wonder if something like this would be better in
> that it wouldn't break people who do not use custom "DIFF" while it would
> help people who do at the same time...
>
>  Makefile              |    2 ++
>  git-merge-one-file.sh |    2 +-

No addition to the test suite to catch this in the future?


-- 
Cheers,

Sverre Rabbelier

Re: What's cooking in git.git (Jun 2010, #01; Wed, 2)

[Junio C Hamano]

Sverre Rabbelier <srabbelier@gmail.com> writes:

> No addition to the test suite to catch this in the future?

If we had a test like this:

    test_expect_success 'run resolve' '
	... set up something to merge ...
        (
        	unset DIFF;
                git merge -s resolve ...
	)
    '

it would have caught this particular breakage, but

 (1) nobody could have anticipated before the problematic patch that $DIFF
     would be an exported variable that could cause trouble, so nobody
     could have written it before the $DIFF patch;

 (2) with the knowledge necessary to write the above test, the $DIFF patch
     wouldn't have made the misconversion, so it is unreasonable to say
     that the $DIFF patch should have come with the above test; and

 (3) having the above test in the test suite may catch exactly the same
     mistake in git-merge-one-file.sh, but that is not a guarantee that
     nobody will break another scripted Porcelain that is not triggered in
     the "resolve" codepath.

Other than doing "! git grep -e '$DIFF' 'git-\*.sh'" at the toplevel of
the source tree, I don't think it is possible to catch future bugs of this
sort.

[PATCH v4] commit::print_summary(): set rev_info.always_show_header to 1

[Tay Ray Chuan]

This attempts to fix a regression in git-commit, where non-abbreviated
SHA-1s were printed in the summary.

One possible fix would be to set ctx.abbrev to DEFAULT_ABBREV in the
`if` block.

However, we remove this codeblock altogether, and set
rev.always_show_header. This way, we use back the same show_log()
mechanism (instead of format_commit_message()).

Quoting log-tree.c:560:

	shown = log_tree_diff(opt, commit, &log);
	if (!shown && opt->loginfo && opt->always_show_header) {
		log.parent = NULL;
		show_log(opt);
		shown = 1;
	}

This is the only area that always_show_header is checked, so the
setting of this flag should only affect this area.

Also, we now die() if log_tree_commit() returns false; add a comment in
the area, which may be helpful to future git hackers (eg. diagnosisng
trigger of the die()).

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---

This is a reworked version of the third patch of the
'tc/commit-abbrev-fix' series; there are no changes to the first and
second patches.

Changes from v3:
 - shift note on the die() and our assumption from the commit message into the
   code (as a comment), as suggested by Junio.

 builtin/commit.c  |   22 +++++++++++++---------
 t/t7502-commit.sh |    4 ++--
 2 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index a4e4966..b0c266c 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1148,7 +1148,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.buf, &rev);
-	rev.always_show_header = 0;
+	rev.always_show_header = 1;
 	rev.diffopt.detect_rename = 1;
 	rev.diffopt.rename_limit = 100;
 	rev.diffopt.break_opt = 0;
@@ -1162,14 +1162,18 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
 				head,
 		initial_commit ? " (root-commit)" : "");

-	if (!log_tree_commit(&rev, commit)) {
-		struct pretty_print_context ctx = {0};
-		struct strbuf buf = STRBUF_INIT;
-		ctx.date_mode = DATE_NORMAL;
-		format_commit_message(commit, format.buf + 7, &buf, &ctx);
-		printf("%s\n", buf.buf);
-		strbuf_release(&buf);
-	}
+	/**
+	 * Based on the existing underlying codepaths (log_tree_commit(),
+	 * log_tree_diff(), log_tree_diff_flush(), to name a few), the die()
+	 * should not occur; changes to these codepaths may warrant a revision
+	 * of our handling of this situation.
+	 *
+	 * Tests #1-#3 in t7502 for output formats should aid in detecting such
+	 * breakages.
+	 */
+	if (!log_tree_commit(&rev, commit))
+		die("unable to print summary");
+
 	strbuf_release(&format);
 }

diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index b10541d..08c0247 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -36,12 +36,12 @@ test_expect_success 'output summary format' '
 	check_summary_oneline "" "a change"
 '

-test_expect_failure 'output summary format for commit with an empty diff' '
+test_expect_success 'output summary format for commit with an empty diff' '

 	check_summary_oneline "" "empty" "--allow-empty"
 '

-test_expect_failure 'output summary format for merges' '
+test_expect_success 'output summary format for merges' '

 	git checkout -b recursive-base &&
 	test_commit base file1 &&
--
1.7.1.189.g07419

[PATCH v6 3/3] commit::print_summary(): don't use format_commit_message()

[Tay Ray Chuan]

This attempts to fix a regression in git-commit, where non-abbreviated
SHA-1s were printed in the summary.

One possible fix would be to set ctx.abbrev to DEFAULT_ABBREV in the
`if` block, where format_commit_message() is used.

Instead, we do away with the format_commit_message() codeblock
altogether, replacing it with a re-run of log_tree_commit().

We re-run log_tree_commit() with rev.always_show_header set, to force
the invocation of show_log(). The effect of this flag can be seen from
this excerpt from log-tree.c:560, the only area that
rev.always_show_header is checked:

	shown = log_tree_diff(opt, commit, &log);
	if (!shown && opt->loginfo && opt->always_show_header) {
		log.parent = NULL;
		show_log(opt);
		shown = 1;
	}

We also set rev.use_terminator, so that a newline is appended at the end
of the log message. Note that callers in builtin/log.c that also set
rev.always_show_header don't have to set rev.use_terminator, but still
get a newline, because they are wrapped in a pager.

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---

This is a reworked version of the third patch of the
'tc/commit-abbrev-fix' series; there are no changes to the first and
second patches.

Changes from v5:

 - don't set rev.always_show_header immediately, so that when there is
   no diff, log_tree_commit() returns false, as before the patch;

 - when log_tree_commit() returns false (ie. no diff), set
   rev.always_show_header, and re-run log_tree_commit();

 - add a missing newline to summary output by setting
   rev.use_terminator.

See the patch message for more details.

In case you're wondering where's v5, it was sent in a private review:

  http://github.com/gitster/git/commit/c69160d

 builtin/commit.c  |   10 ++++------
 t/t7502-commit.sh |    4 ++--
 2 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index a4e4966..aa92362 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1163,13 +1163,11 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
 		initial_commit ? " (root-commit)" : "");

 	if (!log_tree_commit(&rev, commit)) {
-		struct pretty_print_context ctx = {0};
-		struct strbuf buf = STRBUF_INIT;
-		ctx.date_mode = DATE_NORMAL;
-		format_commit_message(commit, format.buf + 7, &buf, &ctx);
-		printf("%s\n", buf.buf);
-		strbuf_release(&buf);
+		rev.always_show_header = 1;
+		rev.use_terminator = 1;
+		log_tree_commit(&rev, commit);
 	}
+
 	strbuf_release(&format);
 }

diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index b10541d..08c0247 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -36,12 +36,12 @@ test_expect_success 'output summary format' '
 	check_summary_oneline "" "a change"
 '

-test_expect_failure 'output summary format for commit with an empty diff' '
+test_expect_success 'output summary format for commit with an empty diff' '

 	check_summary_oneline "" "empty" "--allow-empty"
 '

-test_expect_failure 'output summary format for merges' '
+test_expect_success 'output summary format for merges' '

 	git checkout -b recursive-base &&
 	test_commit base file1 &&
--
1.7.1.189.g07419