threads / discuss / 29617

What's cooking in git.git (Feb 2012, #05; Mon, 13)

Subject: What's cooking in git.git (Feb 2012, #05; Mon, 13)

## tl;dr

23 messages between Feb 13, 2012 and Feb 17, 2012.

replies: 22people: 2as markdown or json

Junio C Hamano· Feb 13, 2012, 20:42 UTC · lore

Here are the topics that have been cooking. Commits prefixed with '-' are only in 'pu' (proposed updates) while commits prefixed with '+' are in 'next'.

This round mosty consists of topics to fix new features introduced in 1.7.9, in preparation for 1.7.9.1 maintenance release. There are still a few more to come.

You can find the changes described here in the integration branches of the repositories listed at

    http://git-blame.blogspot.com/p/git-public-repositories.html

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

* bl/gitweb-project-filter (2012-02-12) 1 commit
  (merged to 'next' on 2012-02-13 at 35366b8)
 + gitweb: Harden and improve $project_filter page title

An update to the new feature to "gitweb" to show list of projects for intermediate levels in directory hierarchies, already slated for 1.7.10.

* dp/i18n-libcharset (2012-02-13) 1 commit
  (merged to 'next' on 2012-02-13 at 528de77)
 + Makefile: introduce CHARSET_LIB to link with -lcharset
Some systems need to explicitly link -lcharset to get locale_charset().
* lt/pull-no-edit (2012-02-12) 1 commit
  (merged to 'next' on 2012-02-13 at 352f0cb)
 + "git pull" doesn't know "--edit"

For 1.7.10 where "git merge" by default spawns the editor when it can automatically commit to record the resulting merge.

* mh/war-on-extra-refs (2012-02-12) 7 commits
  (merged to 'next' on 2012-02-13 at adb7353)
 + refs: remove the extra_refs API
 + clone: do not add alternate references to extra_refs
 + everything_local(): mark alternate refs as complete
 + fetch-pack.c: inline insert_alternate_refs()
 + fetch-pack.c: rename some parameters from "path" to "refname"
 + clone.c: move more code into the "if (refs)" conditional
 + t5700: document a failure of alternates to affect fetch
Internal API clean-up that is very cleanly done.

-------------------------------------------------- [Graduated to "master"]

* bw/inet-pton-ntop-compat (2012-02-05) 1 commit
  (merged to 'next' on 2012-02-06 at 61303e6)
 + Drop system includes from inet_pton/inet_ntop compatibility wrappers
The inclusion order of header files bites Solaris again and this fixes it.
* fc/zsh-completion (2012-02-06) 3 commits
  (merged to 'next' on 2012-02-06 at c94dd12)
 + completion: simplify __gitcomp and __gitcomp_nl implementations
 + completion: use ls -1 instead of rolling a loop to do that ourselves
 + completion: work around zsh option propagation bug
Fix git subcommand completion for zsh (in contrib/completion).
* jc/checkout-out-of-unborn (2012-02-06) 1 commit
  (merged to 'next' on 2012-02-07 at 60eb328)
 + git checkout -b: allow switching out of an unborn branch
"checkout -b" did not allow switching out of an unborn branch.
* jc/maint-commit-ignore-i-t-a (2012-02-07) 1 commit
  (merged to 'next' on 2012-02-10 at e0040cf)
 + commit: ignore intent-to-add entries instead of refusing

Replaces the nd/commit-ignore-i-t-a series that was made unnecessary complicated by bad suggestions I made earlier.

* jc/maint-mailmap-output (2012-02-06) 1 commit
  (merged to 'next' on 2012-02-06 at 0a21425)
 + mailmap: always return a plain mail address from map_user()

map_user() was not rewriting its output correctly, which resulted in the user visible symptom that "git blame -e" sometimes showed excess '>' at the end of email addresses.

* jk/maint-tag-show-fixes (2012-02-08) 3 commits
  (merged to 'next' on 2012-02-08 at 18459c4)
 + tag: do not show non-tag contents with "-n"
 + tag: die when listing missing or corrupt objects
 + tag: fix output of "tag -n" when errors occur
Bugfixes to "git tag -n" that lacked much error checking.
* jk/prompt-fallback-to-tty (2012-02-03) 2 commits
  (merged to 'next' on 2012-02-06 at c0c995a)
 + prompt: fall back to terminal if askpass fails
 + prompt: clean up strbuf usage

The code to ask for password did not fall back to the terminal input when GIT_ASKPASS is set but does not work (e.g. lack of X with GUI askpass helper).

* jn/gitweb-search-utf-8 (2012-02-03) 1 commit
  (merged to 'next' on 2012-02-05 at 055e446)
 + gitweb: Allow UTF-8 encoded CGI query parameters and path_info
Search box in "gitweb" did not accept non-ASCII characters correctly.
* jn/merge-no-edit-fix (2012-02-09) 1 commit
  (merged to 'next' on 2012-02-10 at 014eec9)
 + merge: do not launch an editor on "--no-edit $tag"
In 1.7.9, "merge --no-edit $tag" incorrectly ignored --no-edit.
* mm/empty-loose-error-message (2012-02-06) 1 commit
  (merged to 'next' on 2012-02-07 at f119cac)
 + fsck: give accurate error message on empty loose object files
Updates the error message emitted when we see an empty loose object.
* mp/make-cleanse-x-for-exe (2012-02-09) 1 commit
  (merged to 'next' on 2012-02-09 at 35cc89d)
 + Explicitly set X to avoid potential build breakage

The makefile allowed environment variable X seep into it result in command names suffixed with unnecessary strings.

* nd/cache-tree-api-refactor (2012-02-07) 1 commit
  (merged to 'next' on 2012-02-08 at a9abbca)
 + cache-tree: update API to take abitrary flags
Code cleanup.
* nd/diffstat-gramnum (2012-02-03) 1 commit
  (merged to 'next' on 2012-02-05 at 7335ecc)
 + Use correct grammar in diffstat summary line

The commands in the "git diff" family and "git apply --stat" that count the number of files changed and the number of lines inserted/deleted have been updated to match the output from "diffstat". This also opens the door to i18n this line.

* nd/find-pack-entry-recent-cache-invalidation (2012-02-01) 2 commits
  (merged to 'next' on 2012-02-01 at e26aed0)
 + find_pack_entry(): do not keep packed_git pointer locally
 + sha1_file.c: move the core logic of find_pack_entry() into fill_pack_entry()
* nk/ctype-for-perf (2012-02-10) 2 commits
  (merged to 'next' on 2012-02-10 at b41c6bb)
 + ctype: implement islower/isupper macro
 + ctype.c only wants git-compat-util.h
* tt/profile-build-fix (2012-02-09) 2 commits
  (merged to 'next' on 2012-02-09 at 1c183af)
 + Makefile: fix syntax for older make
  (merged to 'next' on 2012-02-07 at c8c5f3f)
 + Fix build problems related to profile-directed optimization

-------------------------------------------------- [Stalled]

* jc/advise-push-default (2011-12-18) 1 commit
 - push: hint to use push.default=upstream when appropriate

Peff had a good suggestion outlining an updated code structure so that somebody new can try to dip his or her toes in the development. Any takers?

* ss/git-svn-prompt-sans-terminal (2012-01-04) 3 commits
 - fixup! 15eaaf4
 - git-svn, perl/Git.pm: extend Git::prompt helper for querying users
 - perl/Git.pm: "prompt" helper to honor GIT_ASKPASS and SSH_ASKPASS

The bottom one has been replaced with a rewrite based on comments from Ævar. The second one needs more work, both in perl/Git.pm and prompt.c, to give precedence to tty over SSH_ASKPASS when terminal is available.

* jc/split-blob (2012-01-24) 6 commits
 - chunked-object: streaming checkout
 - chunked-object: fallback checkout codepaths
 - bulk-checkin: support chunked-object encoding
 - bulk-checkin: allow the same data to be multiply hashed
 - new representation types in the packstream
 - varint-in-pack: refactor varint encoding/decoding
Not ready.

I finished the streaming checkout codepath, but as explained in 127b177 (bulk-checkin: support chunked-object encoding, 2011-11-30), these are still early steps of a long and painful journey. At least pack-objects and fsck need to learn the new encoding for the series to be usable locally, and then index-pack/unpack-objects needs to learn it to be used remotely.

Given that I heard a lot of noise that people want large files, and that I was asked by somebody at GitTogether'11 privately for an advice on how to pay developers (not me) to help adding necessary support, I am somewhat dissapointed that the original patch series that was sent almost two months ago still remains here without much comments and updates from the developer community. I even made the interface to the logic that decides where to split chunks easily replaceable, and I deliberately made the logic in the original patch extremely stupid to entice others, especially the "bup" fanboys, to come up with a better logic, thinking that giving people an easy target to shoot for, they may be encouraged to help out. The plan is not working :-(.

* nd/columns (2012-02-08) 15 commits
 - column: Fix some compiler and sparse warnings
 - column: add a corner-case test to t3200
 - columns: minimum coding style fixes
 - tag: add --column
 - column: support piping stdout to external git-column process
 - status: add --column
 - branch: add --column
 - help: reuse print_columns() for help -a
 - column: add column.ui for default column output settings
 - column: support columns with different widths
 - column: add columnar layout
 - Stop starting pager recursively
 - Add git-column and column mode parsing
 - column: add API to print items in columns
 - Save terminal width before setting up pager

The "show list of ..." mode of a handful of commands learn to produce column-oriented output.

Expecting a reroll.

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

* jn/ancient-meld-support (2012-02-10) 1 commit
  (merged to 'next' on 2012-02-13 at 28aca31)
 + mergetools/meld: Use --help output to detect --output support

More reliably tell if the given version of "meld" supports --output option.

* jk/config-include (2012-02-06) 2 commits
  (merged to 'next' on 2012-02-13 at 307ddf6)
 + config: add include directive
 + docs: add a basic description of the config API

An assignment to the include.path pseudo-variable causes the named file to be included in-place when Git looks up configuration variables.

* jk/userdiff-config-simplify (2012-02-07) 1 commit
  (merged to 'next' on 2012-02-10 at e9854c1)
 + drop odd return value semantics from userdiff_config
Code cleanup.
* tg/tag-points-at (2012-02-13) 2 commits
  (merged to 'next' on 2012-02-13 at a8f4046)
 + builtin/tag.c: Fix a sparse warning
  (merged to 'next' on 2012-02-10 at 4bff88f)
 + tag: add --points-at list option
* jl/maint-submodule-relative (2012-02-09) 2 commits
 - submodules: always use a relative path from gitdir to work tree
 - submodules: always use a relative path to gitdir
The second one looked iffy.
* ld/git-p4-expanded-keywords (2012-02-09) 2 commits
 - git-p4: initial demonstration of possible RCS keyword fixup
 - git-p4: add test case for RCS keywords
Waiting for reviews and user reports.
* jk/grep-binary-attribute (2012-02-02) 9 commits
  (merged to 'next' on 2012-02-05 at 9dffa7e)
 + grep: pre-load userdiff drivers when threaded
 + grep: load file data after checking binary-ness
 + grep: respect diff attributes for binary-ness
 + grep: cache userdiff_driver in grep_source
 + grep: drop grep_buffer's "name" parameter
 + convert git-grep to use grep_source interface
 + grep: refactor the concept of "grep source" into an object
 + grep: move sha1-reading mutex into low-level code
 + grep: make locking flag global

Fixes a longstanding bug that there was no way to tell "git grep" that a path may look like text but it is not, which "git diff" can do using the attributes system. Now "git grep" honors the same "binary" (or "-diff") attribute.

Will merge to 'master' after 1.7.9.1 settles.
* jk/git-dir-lookup (2012-02-02) 1 commit
  (merged to 'next' on 2012-02-05 at 1856d74)
 + standardize and improve lookup rules for external local repos

When you have both .../foo and .../foo.git, "git clone .../foo" did not favor the former but the latter.

Will merge to 'master' after 1.7.9.1 settles.
* nd/pack-objects-parseopt (2012-02-01) 3 commits
  (merged to 'next' on 2012-02-05 at d0dc25d)
 + pack-objects: convert to use parse_options()
 + pack-objects: remove bogus comment
 + pack-objects: do not accept "--index-version=version,"

"pack-objects" learned use parse-options, losing custom command line parsing code.

Will merge to 'master' after 1.7.9.1 settles.
Junio C Hamano· Feb 14, 2012, 07:22 UTC · re: Junio C Hamano · lore

An incremental update to "What's cooking"

Here is the tonight's snapshot as an incremental update to the issue #5 of "What's cooking" for this month.

I'd like to merge jn/merge-no-edit-fix topic to 'maint' and release the first maintenance release 1.7.9.1 soonish. The topic is merged to 'master' already, and it is very much appreciated if people can test and eyeball it to make sure we fixed what is broken in the vanilla 1.7.9 release without causing regression in unexpected places.

Thanks, and goodnight.
----------------------------------------------------------------
Born topics
[New Topics]
 * cb/maint-rev-list-verify-object (2012-02-13) 1 commit
  - git rev-list: fix invalid typecast
 
 Fixes an obscure bug in "rev-list --verify" that skipped verification
 depending on the phase of the moon, which dates back to 1.7.8.x series.
 * cb/maint-t5541-make-server-port-portable (2012-02-13) 1 commit
  - t5541: check error message against the real port number used
 
 Test fix.
 * cb/receive-pack-keep-errors (2012-02-13) 1 commit
  - do not override receive-pack errors
 
 One hunk and the word "override" in the description were a bit iffy.
 * cb/transfer-no-progress (2012-02-13) 1 commit
  - push/fetch/clone --no-progress suppresses progress output
 
 The transport programs semi-ignored --no-progress and showed progress when
 sending their output to a terminal.
 * jk/diff-highlight (2012-02-13) 5 commits
  - diff-highlight: document some non-optimal cases
  - diff-highlight: match multi-line hunks
  - diff-highlight: refactor to prepare for multi-line hunks
  - diff-highlight: don't highlight whole lines
  - diff-highlight: make perl strict and warnings fatal
 
 Updates diff-highlight (in contrib/).
 * zj/decimal-width (2012-02-13) 1 commit
  - (sign-off???) make lineno_width() from blame reusable for others
  (this branch is used by zj/diff-stat-dyncol.)
 
 Refactoring.
 * zj/term-columns (2012-02-13) 1 commit
  - pager: find out the terminal width before spawning the pager
  (this branch is used by zj/diff-stat-dyncol.)
 
 Fixes "git -p cmd" for any subcommand that cares about the true terminal
 width.
 * hv/submodule-recurse-push (2012-02-13) 3 commits
  - push: teach --recurse-submodules the on-demand option
  - Refactor submodule push check to use string list instead of integer
  - Teach revision walking machinery to walk multiple times sequencially
 
 The bottom one was not clearly explained.
 * zj/diff-stat-dyncol (2012-02-13) 2 commits
  - diff --stat: use the full terminal width
  - Merge branch 'zj/term-columns' into zj/diff-stat-dyncol
  (this branch uses zj/decimal-width and zj/term-columns.)
 
 This breaks tests. Perhaps it is not worth using the decimal-width stuff
 for this series, at least initially.

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

-* ld/git-p4-expanded-keywords (2012-02-09) 2 commits
+* ld/git-p4-expanded-keywords (2012-02-13) 3 commits
+ - git-p4: more RCS tests
  - git-p4: initial demonstration of possible RCS keyword fixup
  - git-p4: add test case for RCS keywords
 
-Waiting for reviews and user reports.
+Waiting for the dust to settle.
Jeff King· Feb 14, 2012, 21:47 UTC · re: Junio C Hamano · lore

Re: What's cooking in git.git (Feb 2012, #05; Mon, 13)

On Mon, Feb 13, 2012 at 12:42:48PM -0800, Junio C Hamano wrote:
Show 7 quoted lines
> * jk/config-include (2012-02-06) 2 commits
>   (merged to 'next' on 2012-02-13 at 307ddf6)
>  + config: add include directive
>  + docs: add a basic description of the config API
> 
> An assignment to the include.path pseudo-variable causes the named file
> to be included in-place when Git looks up configuration variables.

Yikes. I was planning to re-roll this, but got sidetracked discussing David's git-cola case. Besides a few minor tweaks in the documentation patch, the actual include patch is buggy, and accidentally turns on includes for "git config --list".

Do you want to revert and re-do to make master pretty, or should I just build on top?

-Peff
Junio C Hamano· Feb 14, 2012, 22:05 UTC · re: Jeff King · lore

Re: What's cooking in git.git (Feb 2012, #05; Mon, 13)

Jeff King <peff@peff.net> writes:
Show 14 quoted lines
> On Mon, Feb 13, 2012 at 12:42:48PM -0800, Junio C Hamano wrote:
>
>> * jk/config-include (2012-02-06) 2 commits
>>   (merged to 'next' on 2012-02-13 at 307ddf6)
>>  + config: add include directive
>>  + docs: add a basic description of the config API
>> 
>> An assignment to the include.path pseudo-variable causes the named file
>> to be included in-place when Git looks up configuration variables.
>
> Yikes. I was planning to re-roll this, but got sidetracked discussing
> David's git-cola case. Besides a few minor tweaks in the documentation
> patch, the actual include patch is buggy, and accidentally turns on
> includes for "git config --list".

Hmm, I thought t1305 covered "config --list", and ... oops, it makes sure the output contains the inclusion.

> Do you want to revert and re-do to make master pretty, or should I just
> build on top?
Do you mean 'next'?
Jeff King· Feb 14, 2012, 22:09 UTC · re: Junio C Hamano · lore

Re: What's cooking in git.git (Feb 2012, #05; Mon, 13)

On Tue, Feb 14, 2012 at 02:05:16PM -0800, Junio C Hamano wrote:
Show 7 quoted lines
> > Yikes. I was planning to re-roll this, but got sidetracked discussing
> > David's git-cola case. Besides a few minor tweaks in the documentation
> > patch, the actual include patch is buggy, and accidentally turns on
> > includes for "git config --list".
> 
> Hmm, I thought t1305 covered "config --list", and ... oops, it makes sure
> the output contains the inclusion.

Yes. It should include it (and does correctly) when not using any per-file options, but does not correctly turn it off for the per-file case (because we bail to regular git_config instead of custom lookup code).

> > Do you want to revert and re-do to make master pretty, or should I just
> > build on top?
> 
> Do you mean 'next'?

I meant "revert from next and re-reroll, so that when the re-roll gets merged to master, the result there will look pretty".

-Peff
Jeff King· Feb 16, 2012, 08:01 UTC · re: Jeff King · lore

[PATCH 0/8] config-include fixes

On Tue, Feb 14, 2012 at 05:09:53PM -0500, Jeff King wrote:
Show 15 quoted lines
> > Hmm, I thought t1305 covered "config --list", and ... oops, it makes sure
> > the output contains the inclusion.
> 
> Yes. It should include it (and does correctly) when not using any
> per-file options, but does not correctly turn it off for the per-file
> case (because we bail to regular git_config instead of custom lookup
> code).
> 
> > > Do you want to revert and re-do to make master pretty, or should I just
> > > build on top?
> > 
> > Do you mean 'next'?
> 
> I meant "revert from next and re-reroll, so that when the re-roll gets
> merged to master, the result there will look pretty".

The fix for this is pretty straightforward. However, I took the opportunity to do some cleanup and fix some minor bugs while I was in the area, and ended up with quite a big series.

I prepared this on top of what you have queued in jk/config-include. However, all of the cleanup is semantically independent of the topic (though there are a few minor textual conflicts). If I were re-rolling, I would put it all at the front, then squash patch 8 into my prior "implement config includes" patch.

The patches are:
  [1/8]: t1300: add missing &&-chaining
Obvious cleanup.
  [2/8]: config: copy the return value of prefix_filename
Very minor bug.
  [3/8]: config: teach git_config_set_multivar_in_file a default path
  [4/8]: config: teach git_config_rename_section a file argument
  [5/8]: config: provide a version of git_config with more options
  [6/8]: config: stop using config_exclusive_filename
  [7/8]: config: eliminate config_exclusive_filename

This is all cleanup which makes config_exclusive_filename go away. It's not strictly necessary for this series, but it's something I've been wanting to clean up for a while. And it does fix a few minor bugs (see patch 6/8). And the refactoring in 5/8 lays the groundwork for 8/8.

  [8/8]: config: do not respect includes for single-file --list
The actual fix for the regression in my config-include patch.
-Peff
Jeff King· Feb 16, 2012, 08:02 UTC · re: Jeff King · lore

[PATCH 1/8] t1300: add missing &&-chaining

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t1300-repo-config.sh |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 0690e0e..6de46bb 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -451,7 +451,7 @@ test_expect_success 'refer config from subdirectory' '
 	mkdir x &&
 	(
 		cd x &&
-		echo strasse >expect
+		echo strasse >expect &&
 		git config --get --file ../other-config ein.bahn >actual &&
 		test_cmp expect actual
 	)
-- 
1.7.9.1.4.g8ffed
Jeff King· Feb 16, 2012, 08:03 UTC · re: Jeff King · lore

[PATCH 2/8] config: copy the return value of prefix_filename

The prefix_filename function returns a pointer to a static buffer which may be overwritten by subsequent calls. Since we are going to keep the result around for a while, let's be sure to duplicate it for safety.

I don't think this can be triggered as a bug in the current code, but it's a good idea to be defensive, as any resulting bug would be quite subtle.

Signed-off-by: Jeff King <peff@peff.net>
---
I was tempted to simply use OPT_FILENAME to get the argument to "-f",
which would handle the prefixing for us. However, we can also get the
value from $GIT_CONFIG. That value doesn't actually run through this
code currently, but I think it should (and I fix this in patch 6).
 builtin/config.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/builtin/config.c b/builtin/config.c
index 09bf778..5a43a3c 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -391,9 +391,10 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		config_exclusive_filename = git_pathdup("config");
 	else if (given_config_file) {
 		if (!is_absolute_path(given_config_file) && prefix)
-			config_exclusive_filename = prefix_filename(prefix,
-								    strlen(prefix),
-								    given_config_file);
+			config_exclusive_filename =
+				xstrdup(prefix_filename(prefix,
+							strlen(prefix),
+							given_config_file));
 		else
 			config_exclusive_filename = given_config_file;
 	}
-- 
1.7.9.1.4.g8ffed
Jeff King· Feb 16, 2012, 08:04 UTC · re: Jeff King · lore

[PATCH 3/8] config: teach git_config_set_multivar_in_file a default path

The git_config_set_multivar_in_file function takes a filename argument to specify the file into which the values should be written. Currently, this value must be non-NULL. Callers which want to write to the default location must use the regular, non-"in_file" version, which will either write to config_exclusive_filename, or to the repo config if the exclusive filename is NULL.

Let's migrate the "default to using repo config" logic into the "in_file" form. That will let callers get the same default-if-NULL behavior as one gets with config_exclusive_filename, but without having to use the global variable.

Signed-off-by: Jeff King <peff@peff.net>
---
 config.c |   20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)
diff --git a/config.c b/config.c
index e3fcf75..a9eec58 100644
--- a/config.c
+++ b/config.c
@@ -1300,6 +1300,7 @@ int git_config_set_multivar_in_file(const char *config_filename,
 	int fd = -1, in_fd;
 	int ret;
 	struct lock_file *lock = NULL;
+	char *filename_buf = NULL;
 
 	/* parse-key returns negative; flip the sign to feed exit(3) */
 	ret = 0 - git_config_parse_key(key, &store.key, &store.baselen);
@@ -1308,6 +1309,8 @@ int git_config_set_multivar_in_file(const char *config_filename,
 
 	store.multi_replace = multi_replace;
 
+	if (!config_filename)
+		config_filename = filename_buf = git_pathdup("config");
 
 	/*
 	 * The lock serves a purpose in addition to locking: the new
@@ -1477,6 +1480,7 @@ int git_config_set_multivar_in_file(const char *config_filename,
 out_free:
 	if (lock)
 		rollback_lock_file(lock);
+	free(filename_buf);
 	return ret;
 
 write_err_out:
@@ -1488,19 +1492,9 @@ write_err_out:
 int git_config_set_multivar(const char *key, const char *value,
 			const char *value_regex, int multi_replace)
 {
-	const char *config_filename;
-	char *buf = NULL;
-	int ret;
-
-	if (config_exclusive_filename)
-		config_filename = config_exclusive_filename;
-	else
-		config_filename = buf = git_pathdup("config");
-
-	ret = git_config_set_multivar_in_file(config_filename, key, value,
-					value_regex, multi_replace);
-	free(buf);
-	return ret;
+	return git_config_set_multivar_in_file(config_exclusive_filename,
+					       key, value, value_regex,
+					       multi_replace);
 }
 
 static int section_name_match (const char *buf, const char *name)
-- 
1.7.9.1.4.g8ffed
Jeff King· Feb 16, 2012, 08:04 UTC · re: Jeff King · lore

[PATCH 4/8] config: teach git_config_rename_section a file argument

The other config-writing functions (git_config_set and git_config_set_multivar) each have an -"in_file" version to write a specific file. Let's add one for rename_section, with the eventual goal of moving away from the magic config_exclusive_filename global.

Signed-off-by: Jeff King <peff@peff.net>
---
 cache.h  |    1 +
 config.c |   20 +++++++++++++-------
 2 files changed, 14 insertions(+), 7 deletions(-)
diff --git a/cache.h b/cache.h
index 94f3eaf..7e375ce 100644
--- a/cache.h
+++ b/cache.h
@@ -1130,6 +1130,7 @@ extern int git_config_parse_key(const char *, char **, int *);
 extern int git_config_set_multivar(const char *, const char *, const char *, int);
 extern int git_config_set_multivar_in_file(const char *, const char *, const char *, const char *, int);
 extern int git_config_rename_section(const char *, const char *);
+extern int git_config_rename_section_in_file(const char *, const char *, const char *);
 extern const char *git_etc_gitconfig(void);
 extern int check_repository_format_version(const char *var, const char *value, void *cb);
 extern int git_env_bool(const char *, int);
diff --git a/config.c b/config.c
index a9eec58..c456600 100644
--- a/config.c
+++ b/config.c
@@ -1537,19 +1537,19 @@ static int section_name_match (const char *buf, const char *name)
 }
 
 /* if new_name == NULL, the section is removed instead */
-int git_config_rename_section(const char *old_name, const char *new_name)
+int git_config_rename_section_in_file(const char *config_filename,
+				      const char *old_name, const char *new_name)
 {
 	int ret = 0, remove = 0;
-	char *config_filename;
+	char *filename_buf = NULL;
 	struct lock_file *lock = xcalloc(sizeof(struct lock_file), 1);
 	int out_fd;
 	char buf[1024];
 	FILE *config_file;
 
-	if (config_exclusive_filename)
-		config_filename = xstrdup(config_exclusive_filename);
-	else
-		config_filename = git_pathdup("config");
+	if (!config_filename)
+		config_filename = filename_buf = git_pathdup("config");
+
 	out_fd = hold_lock_file_for_update(lock, config_filename, 0);
 	if (out_fd < 0) {
 		ret = error("could not lock config file %s", config_filename);
@@ -1613,10 +1613,16 @@ unlock_and_out:
 	if (commit_lock_file(lock) < 0)
 		ret = error("could not commit config file %s", config_filename);
 out:
-	free(config_filename);
+	free(filename_buf);
 	return ret;
 }
 
+int git_config_rename_section(const char *old_name, const char *new_name)
+{
+	return git_config_rename_section_in_file(config_exclusive_filename,
+						 old_name, new_name);
+}
+
 /*
  * Call this to report error for your variable that should not
  * get a boolean value (i.e. "[my] var" means "true").
-- 
1.7.9.1.4.g8ffed
Jeff King· Feb 16, 2012, 08:05 UTC · re: Jeff King · lore

[PATCH 5/8] config: provide a version of git_config with more options

Callers may want to provide a specific version of a file in which to look for config. Right now this can be done by setting the magic global config_exclusive_filename variable. By providing a version of git_config that takes a filename, we can take a step towards making this magic global go away.

Furthermore, by providing a more "advanced" interface, we now have a a natural place to add new options for callers like git-config, which care about tweaking the specifics of config lookup, without disturbing the large number of "simple" users (i.e., every other part of git).

The astute reader of this patch may notice that the logic for handling config_exclusive_filename was taken out of git_config_early, but added into git_config. This means that git_config_early will no longer respect config_exclusive_filename. That's OK, because the only other caller of git_config_early is check_repository_format_gently, but the only function which sets config_exclusive_filename is cmd_config, which does not call check_repository_format_gently (and if it did, it would have been a bug, anyway, as we would be checking the repository format in the wrong file).

Signed-off-by: Jeff King <peff@peff.net>
---
Obviously this could also learn about a respect_includes flag, and it in
fact does in patch 8. I didn't include it in the initial version because
this part of the series is really about refactoring the
config_exclusive_filename stuff (and is semantically independent of the
notion of included files).
 cache.h  |    1 +
 config.c |   22 +++++++++++++++++-----
 2 files changed, 18 insertions(+), 5 deletions(-)
diff --git a/cache.h b/cache.h
index 7e375ce..7cb8874 100644
--- a/cache.h
+++ b/cache.h
@@ -1115,6 +1115,7 @@ extern int git_config_from_file(config_fn_t fn, const char *, void *);
 extern void git_config_push_parameter(const char *text);
 extern int git_config_from_parameters(config_fn_t fn, void *data);
 extern int git_config(config_fn_t fn, void *);
+extern int git_config_with_options(config_fn_t fn, void *, const char *filename);
 extern int git_config_early(config_fn_t fn, void *, const char *repo_config);
 extern int git_parse_ulong(const char *, unsigned long *);
 extern int git_config_int(const char *, const char *);
diff --git a/config.c b/config.c
index c456600..fbf883d 100644
--- a/config.c
+++ b/config.c
@@ -942,9 +942,6 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)
 	int ret = 0, found = 0;
 	const char *home = NULL;
 
-	/* Setting $GIT_CONFIG makes git read _only_ the given config file. */
-	if (config_exclusive_filename)
-		return git_config_from_file(fn, config_exclusive_filename, data);
 	if (git_config_system() && !access(git_etc_gitconfig(), R_OK)) {
 		ret += git_config_from_file(fn, git_etc_gitconfig(),
 					    data);
@@ -980,7 +977,8 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)
 	return ret == 0 ? found : ret;
 }
 
-int git_config(config_fn_t fn, void *data)
+int git_config_with_options(config_fn_t fn, void *data,
+			    const char *filename)
 {
 	char *repo_config = NULL;
 	int ret;
@@ -988,14 +986,28 @@ int git_config(config_fn_t fn, void *data)
 
 	inc.fn = fn;
 	inc.data = data;
+	fn = git_config_include;
+	data = &inc;
+
+	/*
+	 * If we have a specific filename, use it. Otherwise, follow the
+	 * regular lookup sequence.
+	 */
+	if (filename)
+		return git_config_from_file(fn, filename, data);
 
 	repo_config = git_pathdup("config");
-	ret = git_config_early(git_config_include, &inc, repo_config);
+	ret = git_config_early(fn, data, repo_config);
 	if (repo_config)
 		free(repo_config);
 	return ret;
 }
 
+int git_config(config_fn_t fn, void *data)
+{
+	return git_config_with_options(fn, data, config_exclusive_filename);
+}
+
 /*
  * Find all the stuff for git_config_set() below.
  */
-- 
1.7.9.1.4.g8ffed
Jeff King· Feb 16, 2012, 08:07 UTC · re: Jeff King · lore

[PATCH 6/8] config: stop using config_exclusive_filename

The git-config command sometimes operates on the default set of config files (either reading from all, or writing to repo config), and sometimes operates on a specific file. In the latter case, we set the magic global config_exclusive_filename, and the code in config.c does the right thing.

Instead, let's have git-config use the "advanced" variants of config.c's functions which let it specify an individual filename (or NULL for the default). This makes the code a lot more obvious, and fixes two small bugs:

  1. A relative path specified by GIT_CONFIG=foo will look
     in the wrong directory if we have to chdir as part of
     repository setup. We already handle this properly for
     "git config -f foo", but the GIT_CONFIG lookup used
     config_exclusive_filename directly. By dropping to a
     single magic variable, the GIT_CONFIG case now just
     works.
  2. Calling "git config -f foo --edit" would not respect
     core.editor. This is because just before editing, we
     called git_config, which would respect the
     config_exclusive_filename setting, even though this
     particular git_config call was not about looking in the
     user's specified file, but rather about loading actual
     git config, just as any other git program would.
Signed-off-by: Jeff King <peff@peff.net>
---
The diffstat is slightly disappointing. It really is a one-for-one
conversion in almost every case, but the functions and variable names
are so bloody long that I had to wrap the calls to stay under 80
characters.
 builtin/config.c       |   61 ++++++++++++++++++++++++++++-------------------
 t/t1300-repo-config.sh |   25 +++++++++++++++++++
 2 files changed, 61 insertions(+), 25 deletions(-)
diff --git a/builtin/config.c b/builtin/config.c
index 5a43a3c..8901dd9 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -167,7 +167,7 @@ static int get_value(const char *key_, const char *regex_)
 	config_fn_t fn;
 	void *data;
 
-	local = config_exclusive_filename;
+	local = given_config_file;
 	if (!local) {
 		const char *home = getenv("HOME");
 		local = repo_config = git_pathdup("config");
@@ -315,7 +315,8 @@ static void get_color(const char *def_color)
 {
 	get_color_found = 0;
 	parsed_color[0] = '\0';
-	git_config(git_get_color_config, NULL);
+	git_config_with_options(git_get_color_config, NULL,
+				given_config_file);
 
 	if (!get_color_found && def_color)
 		color_parse(def_color, "command line", parsed_color);
@@ -342,7 +343,8 @@ static int get_colorbool(int print)
 {
 	get_colorbool_found = -1;
 	get_diff_color_found = -1;
-	git_config(git_get_colorbool_config, NULL);
+	git_config_with_options(git_get_colorbool_config, NULL,
+				given_config_file);
 
 	if (get_colorbool_found < 0) {
 		if (!strcmp(get_colorbool_slot, "color.diff"))
@@ -365,7 +367,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 	int nongit = !startup_info->have_repository;
 	char *value;
 
-	config_exclusive_filename = getenv(CONFIG_ENVIRONMENT);
+	given_config_file = getenv(CONFIG_ENVIRONMENT);
 
 	argc = parse_options(argc, argv, prefix, builtin_config_options,
 			     builtin_config_usage,
@@ -380,27 +382,27 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		char *home = getenv("HOME");
 		if (home) {
 			char *user_config = xstrdup(mkpath("%s/.gitconfig", home));
-			config_exclusive_filename = user_config;
+			given_config_file = user_config;
 		} else {
 			die("$HOME not set");
 		}
 	}
 	else if (use_system_config)
-		config_exclusive_filename = git_etc_gitconfig();
+		given_config_file = git_etc_gitconfig();
 	else if (use_local_config)
-		config_exclusive_filename = git_pathdup("config");
+		given_config_file = git_pathdup("config");
 	else if (given_config_file) {
 		if (!is_absolute_path(given_config_file) && prefix)
-			config_exclusive_filename =
+			given_config_file =
 				xstrdup(prefix_filename(prefix,
 							strlen(prefix),
 							given_config_file));
 		else
-			config_exclusive_filename = given_config_file;
+			given_config_file = given_config_file;
 	}
 
 	if (respect_includes == -1)
-		respect_includes = !config_exclusive_filename;
+		respect_includes = !given_config_file;
 
 	if (end_null) {
 		term = '\0';
@@ -438,28 +440,29 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 
 	if (actions == ACTION_LIST) {
 		check_argc(argc, 0, 0);
-		if (git_config(show_all_config, NULL) < 0) {
-			if (config_exclusive_filename)
+		if (git_config_with_options(show_all_config, NULL,
+					    given_config_file) < 0) {
+			if (given_config_file)
 				die_errno("unable to read config file '%s'",
-					  config_exclusive_filename);
+					  given_config_file);
 			else
 				die("error processing config file(s)");
 		}
 	}
 	else if (actions == ACTION_EDIT) {
 		check_argc(argc, 0, 0);
-		if (!config_exclusive_filename && nongit)
+		if (!given_config_file && nongit)
 			die("not in a git directory");
 		git_config(git_default_config, NULL);
-		launch_editor(config_exclusive_filename ?
-			      config_exclusive_filename : git_path("config"),
+		launch_editor(given_config_file ?
+			      given_config_file : git_path("config"),
 			      NULL, NULL);
 	}
 	else if (actions == ACTION_SET) {
 		int ret;
 		check_argc(argc, 2, 2);
 		value = normalize_value(argv[0], argv[1]);
-		ret = git_config_set(argv[0], value);
+		ret = git_config_set_in_file(given_config_file, argv[0], value);
 		if (ret == CONFIG_NOTHING_SET)
 			error("cannot overwrite multiple values with a single value\n"
 			"       Use a regexp, --add or --replace-all to change %s.", argv[0]);
@@ -468,17 +471,20 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 	else if (actions == ACTION_SET_ALL) {
 		check_argc(argc, 2, 3);
 		value = normalize_value(argv[0], argv[1]);
-		return git_config_set_multivar(argv[0], value, argv[2], 0);
+		return git_config_set_multivar_in_file(given_config_file,
+						       argv[0], value, argv[2], 0);
 	}
 	else if (actions == ACTION_ADD) {
 		check_argc(argc, 2, 2);
 		value = normalize_value(argv[0], argv[1]);
-		return git_config_set_multivar(argv[0], value, "^$", 0);
+		return git_config_set_multivar_in_file(given_config_file,
+						       argv[0], value, "^$", 0);
 	}
 	else if (actions == ACTION_REPLACE_ALL) {
 		check_argc(argc, 2, 3);
 		value = normalize_value(argv[0], argv[1]);
-		return git_config_set_multivar(argv[0], value, argv[2], 1);
+		return git_config_set_multivar_in_file(given_config_file,
+						       argv[0], value, argv[2], 1);
 	}
 	else if (actions == ACTION_GET) {
 		check_argc(argc, 1, 2);
@@ -499,18 +505,22 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 	else if (actions == ACTION_UNSET) {
 		check_argc(argc, 1, 2);
 		if (argc == 2)
-			return git_config_set_multivar(argv[0], NULL, argv[1], 0);
+			return git_config_set_multivar_in_file(given_config_file,
+							       argv[0], NULL, argv[1], 0);
 		else
-			return git_config_set(argv[0], NULL);
+			return git_config_set_in_file(given_config_file,
+						      argv[0], NULL);
 	}
 	else if (actions == ACTION_UNSET_ALL) {
 		check_argc(argc, 1, 2);
-		return git_config_set_multivar(argv[0], NULL, argv[1], 1);
+		return git_config_set_multivar_in_file(given_config_file,
+						       argv[0], NULL, argv[1], 1);
 	}
 	else if (actions == ACTION_RENAME_SECTION) {
 		int ret;
 		check_argc(argc, 2, 2);
-		ret = git_config_rename_section(argv[0], argv[1]);
+		ret = git_config_rename_section_in_file(given_config_file,
+							argv[0], argv[1]);
 		if (ret < 0)
 			return ret;
 		if (ret == 0)
@@ -519,7 +529,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 	else if (actions == ACTION_REMOVE_SECTION) {
 		int ret;
 		check_argc(argc, 1, 1);
-		ret = git_config_rename_section(argv[0], NULL);
+		ret = git_config_rename_section_in_file(given_config_file,
+							argv[0], NULL);
 		if (ret < 0)
 			return ret;
 		if (ret == 0)
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 6de46bb..5f249f6 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -458,6 +458,14 @@ test_expect_success 'refer config from subdirectory' '
 
 '
 
+test_expect_success 'refer config from subdirectory via GIT_CONFIG' '
+	(
+		cd x &&
+		GIT_CONFIG=../other-config git config --get ein.bahn >actual &&
+		test_cmp expect actual
+	)
+'
+
 cat > expect << EOF
 [ein]
 	bahn = strasse
@@ -960,4 +968,21 @@ test_expect_success 'git -c complains about empty key and value' '
 	test_must_fail git -c "" rev-parse
 '
 
+test_expect_success 'git config --edit works' '
+	git config -f tmp test.value no &&
+	echo test.value=yes >expect &&
+	GIT_EDITOR="echo [test]value=yes >" git config -f tmp --edit &&
+	git config -f tmp --list >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'git config --edit respects core.editor' '
+	git config -f tmp test.value no &&
+	echo test.value=yes >expect &&
+	test_config core.editor "echo [test]value=yes >" &&
+	git config -f tmp --edit &&
+	git config -f tmp --list >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
1.7.9.1.4.g8ffed
Jeff King· Feb 16, 2012, 08:09 UTC · re: Jeff King · lore

[PATCH 7/8] config: eliminate config_exclusive_filename

This is a magic global variable that was intended as an override to the usual git-config lookup process. Once upon a time, you could specify GIT_CONFIG to any git program, and it would look only at that file. This turned out to be confusing and cause a lot of bugs for little gain. As a result, dc87183 (Only use GIT_CONFIG in "git config", not other programs, 2008-06-30) took this away for all callers except git-config.

Since git-config no longer uses it either, the variable can just go away. As the diff shows, nobody was setting to anything except NULL, so we can just replace any sites where it was read with NULL.

Signed-off-by: Jeff King <peff@peff.net>
---
This could be squashed into the last patch (really, all of the last few
patches could be squashed). But I was able to "git grep
config_exclusive_filename" at this state and see that yes, indeed, the
variable is now totally useless.
 cache.h  |    2 --
 config.c |   10 +++-------
 2 files changed, 3 insertions(+), 9 deletions(-)
diff --git a/cache.h b/cache.h
index 7cb8874..411c60d 100644
--- a/cache.h
+++ b/cache.h
@@ -1150,8 +1150,6 @@ struct config_include_data {
 #define CONFIG_INCLUDE_INIT { 0 }
 extern int git_config_include(const char *name, const char *value, void *data);
 
-extern const char *config_exclusive_filename;
-
 #define MAX_GITNAME (1000)
 extern char git_default_email[MAX_GITNAME];
 extern char git_default_name[MAX_GITNAME];
diff --git a/config.c b/config.c
index fbf883d..e1d6857 100644
--- a/config.c
+++ b/config.c
@@ -26,8 +26,6 @@ static config_file *cf;
 
 static int zlib_compression_seen;
 
-const char *config_exclusive_filename = NULL;
-
 #define MAX_INCLUDE_DEPTH 10
 static const char include_depth_advice[] =
 "exceeded maximum include depth (%d) while including\n"
@@ -1005,7 +1003,7 @@ int git_config_with_options(config_fn_t fn, void *data,
 
 int git_config(config_fn_t fn, void *data)
 {
-	return git_config_with_options(fn, data, config_exclusive_filename);
+	return git_config_with_options(fn, data, NULL);
 }
 
 /*
@@ -1504,8 +1502,7 @@ write_err_out:
 int git_config_set_multivar(const char *key, const char *value,
 			const char *value_regex, int multi_replace)
 {
-	return git_config_set_multivar_in_file(config_exclusive_filename,
-					       key, value, value_regex,
+	return git_config_set_multivar_in_file(NULL, key, value, value_regex,
 					       multi_replace);
 }
 
@@ -1631,8 +1628,7 @@ out:
 
 int git_config_rename_section(const char *old_name, const char *new_name)
 {
-	return git_config_rename_section_in_file(config_exclusive_filename,
-						 old_name, new_name);
+	return git_config_rename_section_in_file(NULL, old_name, new_name);
 }
 
 /*
-- 
1.7.9.1.4.g8ffed
Jeff King· Feb 16, 2012, 08:10 UTC · re: Jeff King · lore

[PATCH 8/8] config: do not respect includes for single-file --list

The original include implementation tried not to impact calls to "git config" that look at a single file. However, since we called into git_config in a few places (e.g., --list), our respect_includes flag was not supported.

This patch teaches git_config_with_options a flag to respect includes (instead of doing so by default), and teaches git-config to use it.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/config.c          |    7 ++++---
 cache.h                   |    3 ++-
 config.c                  |   14 ++++++++------
 t/t1305-config-include.sh |    8 ++++++++
 4 files changed, 22 insertions(+), 10 deletions(-)
diff --git a/builtin/config.c b/builtin/config.c
index 8901dd9..d41a9bf 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -316,7 +316,7 @@ static void get_color(const char *def_color)
 	get_color_found = 0;
 	parsed_color[0] = '\0';
 	git_config_with_options(git_get_color_config, NULL,
-				given_config_file);
+				given_config_file, respect_includes);
 
 	if (!get_color_found && def_color)
 		color_parse(def_color, "command line", parsed_color);
@@ -344,7 +344,7 @@ static int get_colorbool(int print)
 	get_colorbool_found = -1;
 	get_diff_color_found = -1;
 	git_config_with_options(git_get_colorbool_config, NULL,
-				given_config_file);
+				given_config_file, respect_includes);
 
 	if (get_colorbool_found < 0) {
 		if (!strcmp(get_colorbool_slot, "color.diff"))
@@ -441,7 +441,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 	if (actions == ACTION_LIST) {
 		check_argc(argc, 0, 0);
 		if (git_config_with_options(show_all_config, NULL,
-					    given_config_file) < 0) {
+					    given_config_file,
+					    respect_includes) < 0) {
 			if (given_config_file)
 				die_errno("unable to read config file '%s'",
 					  given_config_file);
diff --git a/cache.h b/cache.h
index 411c60d..ff54d6f 100644
--- a/cache.h
+++ b/cache.h
@@ -1115,7 +1115,8 @@ extern int git_config_from_file(config_fn_t fn, const char *, void *);
 extern void git_config_push_parameter(const char *text);
 extern int git_config_from_parameters(config_fn_t fn, void *data);
 extern int git_config(config_fn_t fn, void *);
-extern int git_config_with_options(config_fn_t fn, void *, const char *filename);
+extern int git_config_with_options(config_fn_t fn, void *,
+				   const char *filename, int respect_includes);
 extern int git_config_early(config_fn_t fn, void *, const char *repo_config);
 extern int git_parse_ulong(const char *, unsigned long *);
 extern int git_config_int(const char *, const char *);
diff --git a/config.c b/config.c
index e1d6857..ad03908 100644
--- a/config.c
+++ b/config.c
@@ -976,16 +976,18 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)
 }
 
 int git_config_with_options(config_fn_t fn, void *data,
-			    const char *filename)
+			    const char *filename, int respect_includes)
 {
 	char *repo_config = NULL;
 	int ret;
 	struct config_include_data inc = CONFIG_INCLUDE_INIT;
 
-	inc.fn = fn;
-	inc.data = data;
-	fn = git_config_include;
-	data = &inc;
+	if (respect_includes) {
+		inc.fn = fn;
+		inc.data = data;
+		fn = git_config_include;
+		data = &inc;
+	}
 
 	/*
 	 * If we have a specific filename, use it. Otherwise, follow the
@@ -1003,7 +1005,7 @@ int git_config_with_options(config_fn_t fn, void *data,
 
 int git_config(config_fn_t fn, void *data)
 {
-	return git_config_with_options(fn, data, NULL);
+	return git_config_with_options(fn, data, NULL, 1);
 }
 
 /*
diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh
index 0a27ec4..f3e03a0 100755
--- a/t/t1305-config-include.sh
+++ b/t/t1305-config-include.sh
@@ -59,6 +59,14 @@ test_expect_success 'single file lookup does not expand includes by default' '
 	test_cmp expect actual
 '
 
+test_expect_success 'single file list does not expand includes by default' '
+	echo "[test]one = 1" >one &&
+	echo "[include]path = one" >.gitconfig &&
+	echo "include.path=one" >expect &&
+	git config -f .gitconfig --list >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'writing config file does not expand includes' '
 	echo "[test]one = 1" >one &&
 	echo "[include]path = one" >.gitconfig &&
-- 
1.7.9.1.4.g8ffed
Junio C Hamano· Feb 16, 2012, 20:11 UTC · re: Jeff King · lore

Re: [PATCH 0/8] config-include fixes

Jeff King <peff@peff.net> writes:
Show 5 quoted lines
> I prepared this on top of what you have queued in jk/config-include.
> However, all of the cleanup is semantically independent of the topic
> (though there are a few minor textual conflicts). If I were re-rolling,
> I would put it all at the front, then squash patch 8 into my prior
> "implement config includes" patch.

Sorry for being late in answering the "revert or build on top" question; I was mostly offline yesterday afternoon.

Show 10 quoted lines
>   [7/8]: config: eliminate config_exclusive_filename
>
> This is all cleanup which makes config_exclusive_filename go away. It's
> not strictly necessary for this series, but it's something I've been
> wanting to clean up for a while. And it does fix a few minor bugs (see
> patch 6/8). And the refactoring in 5/8 lays the groundwork for 8/8.
>
>   [8/8]: config: do not respect includes for single-file --list
>
> The actual fix for the regression in my config-include patch.
Thanks.
Looking at the rebased result, it strikes me that with_options version
    Furthermore, by providing a more "advanced" interface, we
    now have a a natural place to add new options for callers
    like git-config, which care about tweaking the specifics of
    config lookup, without disturbing the large number of
    "simple" users (i.e., every other part of git).

perhaps wants to get a pointer to struct config_lookup_options, instead of us having to add a new parameter to all callsites every time a new need is discovered.

Jeff King· Feb 17, 2012, 00:14 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/8] config-include fixes

On Thu, Feb 16, 2012 at 12:11:52PM -0800, Junio C Hamano wrote:
Show 10 quoted lines
> Jeff King <peff@peff.net> writes:
> 
> > I prepared this on top of what you have queued in jk/config-include.
> > However, all of the cleanup is semantically independent of the topic
> > (though there are a few minor textual conflicts). If I were re-rolling,
> > I would put it all at the front, then squash patch 8 into my prior
> > "implement config includes" patch.
> 
> Sorry for being late in answering the "revert or build on top" question; I
> was mostly offline yesterday afternoon.

No problem. I did it as build-on-top because it's much easier to squash and reorder the commits later than it is to pick a re-roll apart into multiple commits.

Which way did you want me to go with it?
Show 11 quoted lines
> Looking at the rebased result, it strikes me that with_options version
> 
>     Furthermore, by providing a more "advanced" interface, we
>     now have a a natural place to add new options for callers
>     like git-config, which care about tweaking the specifics of
>     config lookup, without disturbing the large number of
>     "simple" users (i.e., every other part of git).
> 
> perhaps wants to get a pointer to struct config_lookup_options, instead of
> us having to add a new parameter to all callsites every time a new need
> is discovered.

I considered that, but I noticed that the only callers who are really going to care about the _with_options version will be in builtin/config.c, and they all care about every option. And dealing with creating a struct for each call seemed like more hassle.

OTOH, I could probably just make a single static global config_lookup_options, and have all of the option parsing tweak it directly (i.e., replace the given_config_file and respect_includes static globals).

-Peff
Junio C Hamano· Feb 17, 2012, 02:50 UTC · re: Jeff King · lore

Re: [PATCH 0/8] config-include fixes

Jeff King <peff@peff.net> writes:
Show 18 quoted lines
> On Thu, Feb 16, 2012 at 12:11:52PM -0800, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > I prepared this on top of what you have queued in jk/config-include.
>> > However, all of the cleanup is semantically independent of the topic
>> > (though there are a few minor textual conflicts). If I were re-rolling,
>> > I would put it all at the front, then squash patch 8 into my prior
>> > "implement config includes" patch.
>> 
>> Sorry for being late in answering the "revert or build on top" question; I
>> was mostly offline yesterday afternoon.
>
> No problem. I did it as build-on-top because it's much easier to squash
> and reorder the commits later than it is to pick a re-roll apart into
> multiple commits.
>
> Which way did you want me to go with it?

I'll push out the re-roll that follows your outline above in 'pu'; I think I got all the conflict resolved correctly, but please eyeball the result.

Show 15 quoted lines
>> Looking at the rebased result, it strikes me that with_options version
>> 
>>     Furthermore, by providing a more "advanced" interface, we
>>     now have a a natural place to add new options for callers
>>     like git-config, which care about tweaking the specifics of
>>     config lookup, without disturbing the large number of
>>     "simple" users (i.e., every other part of git).
>> 
>> perhaps wants to get a pointer to struct config_lookup_options, instead of
>> us having to add a new parameter to all callsites every time a new need
>> is discovered.
>
> I considered that, but I noticed that the only callers who are really
> going to care about the _with_options version will be in
> builtin/config.c, and they all care about every option.
Ok.
Thanks.
Jeff King· Feb 17, 2012, 03:17 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/8] config-include fixes

On Thu, Feb 16, 2012 at 06:50:40PM -0800, Junio C Hamano wrote:
> I'll push out the re-roll that follows your outline above in 'pu'; I think
> I got all the conflict resolved correctly, but please eyeball the result.
Will do.

Since you are re-rolling, these are the documentation fixes I had squashed in based on your earlier review (though come to think of it, the new patches should now also describe `git_config_with_options`).

---
diff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt
index f428c5c..01f64d1 100644
--- a/Documentation/technical/api-config.txt
+++ b/Documentation/technical/api-config.txt
@@ -11,9 +11,9 @@ General Usage
 Config files are parsed linearly, and each variable found is passed to a
 caller-provided callback function. The callback function is responsible
 for any actions to be taken on the config option, and is free to ignore
-some options (it is not uncommon for the configuration to be parsed
+some options. It is not uncommon for the configuration to be parsed
 several times during the run of a git program, with different callbacks
-picking out different variables useful to themselves).
+picking out different variables useful to themselves.
 
 A config callback function takes three parameters:
 
@@ -47,11 +47,12 @@ will first feed the user-wide one to the callback, and then the
 repo-specific one; by overwriting, the higher-priority repo-specific
 value is left at the end).
 
-There is a special version of `git_config` called `git_config_early`
-that takes an additional parameter to specify the repository config.
-This should be used early in a git program when the repository location
-has not yet been determined (and calling the usual lazy-evaluation
-lookup rules would yield an incorrect location).
+There is a special version of `git_config` called `git_config_early`.
+This version takes an additional parameter to specify the repository
+config, instead of having it looked up via `git_path`. This is useful
+early in a git program before the repository has been found. Unless
+you're working with early setup code, you probably don't want to use
+this.
 
 Reading Specific Files
 ----------------------
Jeff King· Feb 17, 2012, 03:23 UTC · re: Jeff King · lore

Re: [PATCH 0/8] config-include fixes

On Thu, Feb 16, 2012 at 10:17:23PM -0500, Jeff King wrote:
Show 6 quoted lines
> On Thu, Feb 16, 2012 at 06:50:40PM -0800, Junio C Hamano wrote:
> 
> > I'll push out the re-roll that follows your outline above in 'pu'; I think
> > I got all the conflict resolved correctly, but please eyeball the result.
> 
> Will do.
I see you just pushed it out. The result looks good to me.
> Since you are re-rolling, these are the documentation fixes I had
> squashed in based on your earlier review (though come to think of it,
> the new patches should now also describe `git_config_with_options`).

Looks like you just reverted the "include" patch instead of the merge of the whole topic. I'll prepare a few documentation fixups on top, then.

-Peff
Jeff King· Feb 17, 2012, 08:17 UTC · re: Jeff King · lore

[PATCH 0/2] api-config documentation leftovers

On Thu, Feb 16, 2012 at 10:23:25PM -0500, Jeff King wrote:
Show 6 quoted lines
> > Since you are re-rolling, these are the documentation fixes I had
> > squashed in based on your earlier review (though come to think of it,
> > the new patches should now also describe `git_config_with_options`).
> 
> Looks like you just reverted the "include" patch instead of the merge of
> the whole topic.  I'll prepare a few documentation fixups on top, then.

Here they are, on top of what you have in jk/config-include. Squashing would involve breaking apart the second one into the "introduce git_config_with_options" part and the "and now it learns respect_includes" part. So it's probably not worth the effort.

  [1/2]: docs/api-config: minor clarifications
  [2/2]: docs/api-config: describe git_config_with_options
-Peff
Junio C Hamano· Feb 17, 2012, 17:04 UTC · re: Jeff King · lore

Re: [PATCH 0/2] api-config documentation leftovers

Jeff King <peff@peff.net> writes:
Show 7 quoted lines
> Here they are, on top of what you have in jk/config-include. Squashing
> would involve breaking apart the second one into the "introduce
> git_config_with_options" part and the "and now it learns
> respect_includes" part. So it's probably not worth the effort.
>
>   [1/2]: docs/api-config: minor clarifications
>   [2/2]: docs/api-config: describe git_config_with_options
Thanks.
Jeff King· Feb 17, 2012, 08:18 UTC · re: Jeff King · lore

[PATCH 1/2] docs/api-config: minor clarifications

The first change simply drops some parentheses to make a statement more clear. The seconds clarifies that almost nobody wants to call git_config_early.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/technical/api-config.txt |   15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt
index c60b6b3..67984da 100644
--- a/Documentation/technical/api-config.txt
+++ b/Documentation/technical/api-config.txt
@@ -11,9 +11,9 @@ General Usage
 Config files are parsed linearly, and each variable found is passed to a
 caller-provided callback function. The callback function is responsible
 for any actions to be taken on the config option, and is free to ignore
-some options (it is not uncommon for the configuration to be parsed
+some options. It is not uncommon for the configuration to be parsed
 several times during the run of a git program, with different callbacks
-picking out different variables useful to themselves).
+picking out different variables useful to themselves.
 
 A config callback function takes three parameters:
 
@@ -47,11 +47,12 @@ will first feed the user-wide one to the callback, and then the
 repo-specific one; by overwriting, the higher-priority repo-specific
 value is left at the end).
 
-There is a special version of `git_config` called `git_config_early`
-that takes an additional parameter to specify the repository config.
-This should be used early in a git program when the repository location
-has not yet been determined (and calling the usual lazy-evaluation
-lookup rules would yield an incorrect location).
+There is a special version of `git_config` called `git_config_early`.
+This version takes an additional parameter to specify the repository
+config, instead of having it looked up via `git_path`. This is useful
+early in a git program before the repository has been found. Unless
+you're working with early setup code, you probably don't want to use
+this.
 
 Reading Specific Files
 ----------------------
-- 
1.7.9.9.gcf58
Jeff King· Feb 17, 2012, 08:18 UTC · re: Jeff King · lore

[PATCH 2/2] docs/api-config: describe git_config_with_options

This function was added recently but not documented.
Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/technical/api-config.txt |   16 ++++++++++++++++
 1 file changed, 16 insertions(+)
diff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt
index 67984da..3dbf9fd 100644
--- a/Documentation/technical/api-config.txt
+++ b/Documentation/technical/api-config.txt
@@ -47,6 +47,22 @@ will first feed the user-wide one to the callback, and then the
 repo-specific one; by overwriting, the higher-priority repo-specific
 value is left at the end).
 
+The `git_config_with_options` function lets the caller examine config
+while adjusting some of the default behavior of `git_config`. It should
+almost never be used by "regular" git code that is looking up
+configuration variables. It is intended for advanced callers like
+`git-config`, which are intentionally tweaking the normal config-lookup
+process. It takes two options:
+
+`filename`::
+If this parameter is non-NULL, it specifies the name of a file to
+parse for configuration, rather than looking in the usual files. Regular
+`git_config` defaults to `NULL`.
+
+`respect_includes`::
+Specify whether include directives should be followed in parsed files.
+Regular `git_config` defaults to `1`.
+
 There is a special version of `git_config` called `git_config_early`.
 This version takes an additional parameter to specify the repository
 config, instead of having it looked up via `git_path`. This is useful
-- 
1.7.9.9.gcf58

← back to recent threads