commit: fix abbrev-sha regression

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

index a4e4966..ab4a7cf 100644
@@ -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--

index ab4a7cf..13a30ab 100644
@@ -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--

index ab4a7cf..e8def55 100644
@@ -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

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

index 844fb43..589e8e6 100755
@@ -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

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

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

index 589e8e6..8a4a277 100755
@@ -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

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

index a4e4966..ab4a7cf 100644
@@ -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);

index 8a4a277..109ae00 100755
@@ -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

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

Thanks.

Hi,

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:

[email protected]
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

Hi,

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

Cheers,
Ray Chuan

Hi,

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

Cheers,
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

---

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

index 844fb43..478b637 100755
@@ -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

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

---

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

index 478b637..b10541d 100755
@@ -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

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.

---

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

index a4e4966..2884d0c 100644
@@ -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);
 }
 

index b10541d..08c0247 100755
@@ -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

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

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

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

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

---

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

index a4e4966..aa92362 100644
@@ -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);
 }
 

index b10541d..08c0247 100755
@@ -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

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

-- Will

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 <[email protected]>, 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.

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.

---

Yes. Fixed in the following two patches.

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

index fc93435..2ae2c1f 100644
@@ -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, "./");
 

index f83eb23..e9e0221 100644
@@ -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

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.

---

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

index c0fbcdc..0804047 100644
@@ -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");
 

index d8a7c79..0335a9a 100755
@@ -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

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.

---

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

index 2ae2c1f..481541a 100644
@@ -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, "./");

index e9e0221..38003bf 100644
@@ -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

---

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

index 5716d90..0e4cfe6 100644
@@ -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

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.

---

[...]

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

index 2ae2c1f..63d3b01 100644
@@ -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, "./");

index e9e0221..38003bf 100644
@@ -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

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.

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

Junio C Hamano venit, vidit, dixit 03.06.2010 01:36:

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.

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

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

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

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

Cheers,
Michael

Hi,

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

Hi,

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

Hi,

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

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

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

---

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

index a4e4966..2884d0c 100644
@@ -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);
 }
 

index b10541d..08c0247 100755
@@ -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

Hi Will,

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

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

---

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

index a4e4966..b0c266c 100644
@@ -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);
 }
 

index b10541d..08c0247 100755
@@ -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

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.

[...]

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

index 3145009..d067894 100755
@@ -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

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

index bcb84be..0367e8a 100644
@@ -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) \

index 3145009..b86402a 100755
@@ -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

Heya,

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

Cheers,

Sverre Rabbelier

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.