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

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

[Junio C Hamano]

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.

An incremental update to "What's cooking"

[Junio C Hamano]

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.

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

[Jeff King]

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

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

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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

[Jeff King]

On Tue, Feb 14, 2012 at 02:05:16PM -0800, Junio C Hamano wrote:

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

[PATCH 0/8] config-include fixes

[Jeff King]

On Tue, Feb 14, 2012 at 05:09:53PM -0500, Jeff King wrote:

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

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

[Jeff King]

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

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

[Jeff King]

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

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

[Jeff King]

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

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

[Jeff King]

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

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

[Jeff King]

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

[PATCH 6/8] config: stop using config_exclusive_filename

[Jeff King]

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

[PATCH 7/8] config: eliminate config_exclusive_filename

[Jeff King]

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

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

[Jeff King]

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

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

[Junio C Hamano]

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.

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

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

[Jeff King]

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?

> 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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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

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

[Jeff King]

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

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

[Jeff King]

On Thu, Feb 16, 2012 at 10:17:23PM -0500, Jeff King wrote:

> 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

[PATCH 0/2] api-config documentation leftovers

[Jeff King]

On Thu, Feb 16, 2012 at 10:23:25PM -0500, Jeff King wrote:

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

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

[Jeff King]

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

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

[Jeff King]

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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