Re: [RFC/PATCH] pager.c: replace git_config with git_config_get_string

[Karsten Blees <karsten.blees@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2186 bytes --]

Am 28.06.2014 08:01, schrieb Matthieu Moy:
> Karsten Blees <karsten.blees@gmail.com> writes:
> 
>> I still don't like that the invalidation is done in git_config_set, though, as
>> this is also used to write completely unrelated files.
> 
> I don't get it. It is used to write the config files. Yes, we trigger a
> complete reload instead of just changing this particular value in the
> hashmap, but I do not see "unrelated files" in the picture.
> 

git-cherry-pick, revert: writes '.git/sequencer/opts' (sequencer.c:save_opts)
git-mv <submodule>: writes '.gitmodules' (submodule.c:update_path_in_gitmodules)
  ...and the submodule's '.git/config' (submodule.c:connect_work_tree_and_git_dir)


Note that the typical configuration commands git-config/remote/branch seem
to call git_config_set* immediately before exit, so no need to invalidate
the config cache here either.

Which leaves clone, init and push (transport.c:set_upstreams, seems to be
just before exit as well).

I didn't look further after finding the example in cmd_clone, so with the
statistics above, it may well be the only instance of {git_config; 
git_config_set; git_config}, I don't know.

I've attached the reverse call hierarchy as generated by Eclipse - quite
useful for things like this.

>> Wouldn't it be better to have a 'git_config_refresh()' that could be
>> used in place of (or before) current 'git_config(callback)' calls? The
>> initial implementation could just invalidate the config cache. If
>> there's time and energy to spare, a more advanced version could first
>> check if any of the involved config files has changed.
> 
> That would not change the "xstrdup" vs "no xstrdup" issue, right?
> 

Right. (Unless it turns out invalidation isn't needed after all. But even
then using interned strings for the config would be a Good Thing IMO.)

>> The xstrdup() problem could be solved by interning strings (see the
>> attached patch for a trivial implementation). I.e. allocate each distinct
>> string only once (and keep it allocated).
> 
> That's an option. We need to be carefull not to modify any string
> in-place,

Hopefully an illegal non-const cast will be caught in the review process.



[-- Attachment #2: git-config-set-callers.txt --]
[-- Type: text/plain, Size: 6597 bytes --]

git_config_set_multivar_in_file(const char *, const char *, const char *, const char *, int) : int - /git/config.c
	cmd_config(int, const char * *, const char *) : int - /git/builtin/config.c (5 matches)
	git_config_set_in_file(const char *, const char *, const char *) : int - /git/config.c
		cmd_config(int, const char * *, const char *) : int - /git/builtin/config.c (2 matches)
		connect_work_tree_and_git_dir(const char *, const char *) : void - /git/submodule.c
			cmd_mv(int, const char * *, const char *) : int - /git/builtin/mv.c
		save_opts(replay_opts *) : void - /git/sequencer.c (8 matches)
			sequencer_pick_revisions(replay_opts *) : int - /git/sequencer.c
				cmd_cherry_pick(int, const char * *, const char *) : int - /git/builtin/revert.c
				cmd_revert(int, const char * *, const char *) : int - /git/builtin/revert.c
		update_path_in_gitmodules(const char *, const char *) : int - /git/submodule.c
			cmd_mv(int, const char * *, const char *) : int - /git/builtin/mv.c
	git_config_set_multivar(const char *, const char *, const char *, int) : int - /git/config.c
		add_branch(const char *, const char *, const char *, int, strbuf *) : int - /git/builtin/remote.c
			add_branches(remote *, const char * *, const char *) : int - /git/builtin/remote.c
				set_remote_branches(const char *, const char * *, int) : int - /git/builtin/remote.c
					set_branches(int, const char * *) : int - /git/builtin/remote.c
						cmd_remote(int, const char * *, const char *) : int - /git/builtin/remote.c
			add(int, const char * *) : int - /git/builtin/remote.c
				cmd_remote(int, const char * *, const char *) : int - /git/builtin/remote.c
		cmd_branch(int, const char * *, const char *) : int - /git/builtin/branch.c (2 matches)
		git_config_set(const char *, const char *) : int - /git/config.c
			add(int, const char * *) : int - /git/builtin/remote.c (3 matches)
				cmd_remote(int, const char * *, const char *) : int - /git/builtin/remote.c
			cmd_clone(int, const char * *, const char *) : int - /git/builtin/clone.c (2 matches)
			create_default_files(const char *) : int - /git/builtin/init-db.c (8 matches)
				init_db(const char *, unsigned int) : int - /git/builtin/init-db.c
					cmd_clone(int, const char * *, const char *) : int - /git/builtin/clone.c
					cmd_init_db(int, const char * *, const char *) : int - /git/builtin/init-db.c
			edit_branch_description(const char *) : int - /git/builtin/branch.c
				cmd_branch(int, const char * *, const char *) : int - /git/builtin/branch.c
			init_db(const char *, unsigned int) : int - /git/builtin/init-db.c (2 matches)
				cmd_clone(int, const char * *, const char *) : int - /git/builtin/clone.c
				cmd_init_db(int, const char * *, const char *) : int - /git/builtin/init-db.c
			install_branch_config(int, const char *, const char *, const char *) : void - /git/branch.c (3 matches)
				cmd_clone(int, const char * *, const char *) : int - /git/builtin/clone.c
				set_upstreams(transport *, ref *, int) : void - /git/transport.c
					transport_push(transport *, int, const char * *, int, unsigned int *) : int - /git/transport.c
						push_with_options(transport *, int) : int - /git/builtin/push.c
							do_push(const char *, int) : int - /git/builtin/push.c (2 matches)
								cmd_push(int, const char * *, const char *) : int - /git/builtin/push.c
				setup_tracking(const char *, const char *, enum branch_track, int) : int - /git/branch.c
					create_branch(const char *, const char *, const char *, int, int, int, int, enum branch_track) : void - /git/branch.c
						cmd_branch(int, const char * *, const char *) : int - /git/builtin/branch.c (2 matches)
						update_refs_for_switch(const checkout_opts *, branch_info *, branch_info *) : void - /git/builtin/checkout.c
							switch_branches(const checkout_opts *, branch_info *) : int - /git/builtin/checkout.c
								checkout_branch(checkout_opts *, branch_info *) : int - /git/builtin/checkout.c
									cmd_checkout(int, const char * *, const char *) : int - /git/builtin/checkout.c
				update_head(const ref *, const ref *, const char *) : void - /git/builtin/clone.c
					cmd_clone(int, const char * *, const char *) : int - /git/builtin/clone.c
			mingw_mark_as_git_dir(const char *) : void - /git/compat/mingw.c
				init_db(const char *, unsigned int) : int - /git/builtin/init-db.c
					cmd_clone(int, const char * *, const char *) : int - /git/builtin/clone.c
					cmd_init_db(int, const char * *, const char *) : int - /git/builtin/init-db.c
			mv(int, const char * *) : int - /git/builtin/remote.c
				cmd_remote(int, const char * *, const char *) : int - /git/builtin/remote.c
			rm(int, const char * *) : int - /git/builtin/remote.c
				cmd_remote(int, const char * *, const char *) : int - /git/builtin/remote.c
			set_url(int, const char * *) : int - /git/builtin/remote.c
				cmd_remote(int, const char * *, const char *) : int - /git/builtin/remote.c
			write_refspec_config(const char *, const ref *, const ref *, strbuf *) : void - /git/builtin/clone.c
				cmd_clone(int, const char * *, const char *) : int - /git/builtin/clone.c
		migrate_file(remote *) : int - /git/builtin/remote.c (3 matches)
			mv(int, const char * *) : int - /git/builtin/remote.c
				cmd_remote(int, const char * *, const char *) : int - /git/builtin/remote.c
		mv(int, const char * *) : int - /git/builtin/remote.c (2 matches)
			cmd_remote(int, const char * *, const char *) : int - /git/builtin/remote.c
		remove_all_fetch_refspecs(const char *, const char *) : int - /git/builtin/remote.c
			set_remote_branches(const char *, const char * *, int) : int - /git/builtin/remote.c
				set_branches(int, const char * *) : int - /git/builtin/remote.c
					cmd_remote(int, const char * *, const char *) : int - /git/builtin/remote.c
		set_url(int, const char * *) : int - /git/builtin/remote.c (3 matches)
			cmd_remote(int, const char * *, const char *) : int - /git/builtin/remote.c
		write_one_config(const char *, const char *, void *) : int - /git/builtin/clone.c
			write_config(string_list *) : void - /git/builtin/clone.c
				cmd_clone(int, const char * *, const char *) : int - /git/builtin/clone.c
		write_refspec_config(const char *, const ref *, const ref *, strbuf *) : void - /git/builtin/clone.c
			cmd_clone(int, const char * *, const char *) : int - /git/builtin/clone.c
	save_opts(replay_opts *) : void - /git/sequencer.c
		sequencer_pick_revisions(replay_opts *) : int - /git/sequencer.c
			cmd_cherry_pick(int, const char * *, const char *) : int - /git/builtin/revert.c
			cmd_revert(int, const char * *, const char *) : int - /git/builtin/revert.c