git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Karsten Blees <karsten.blees@gmail.com>
Date
Jun 28, 2014, 14:29 UTC
Message-ID
<53AED13D.60705@gmail.com>
In-Reply-To
<vpqy4whshbj.fsf@anie.imag.fr>
Am 28.06.2014 08:01, schrieb Matthieu Moy:
Show 9 quoted lines
> 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.

Show 8 quoted lines
>> 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.)

Show 6 quoted lines
>> 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.
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
Previous: Matthieu MoyNext: Matthieu Moy
Message 35 of 41 in “alias.c: replace git_config with git_config_get_string”
  1. alias.c: replace git_config with git_config_get_stringTanay Abhra, Jun 23, 2014
  2. branch.c: replace git_config with git_config_get_stringTanay Abhra, Jun 23, 2014
  3. Eric SunshineJun 25, 2014
  4. Tanay AbhraJun 26, 2014
  5. Eric SunshineJun 29, 2014
  6. imap-send.c: replace git_config with git_config_get_stringTanay Abhra, Jun 23, 2014
  7. Eric SunshineJun 25, 2014
  8. Tanay AbhraJun 26, 2014
  9. Matthieu MoyJun 26, 2014
  10. Karsten BleesJun 26, 2014
  11. notes-util.c: replace git_config with git_config_get_stringTanay Abhra, Jun 23, 2014
  12. Eric SunshineJun 25, 2014
  13. Tanay AbhraJun 26, 2014
  14. Eric SunshineJun 29, 2014
  15. Karsten BleesJun 30, 2014
  16. Eric SunshineJun 30, 2014
  17. Karsten BleesJun 30, 2014
  18. Tanay AbhraJun 30, 2014
  19. Karsten BleesJun 30, 2014
  20. Tanay AbhraJun 30, 2014
  21. Junio C HamanoJun 30, 2014
  22. Matthieu MoyJul 1, 2014
  23. notes.c: replace git_config with git_config_get_stringTanay Abhra, Jun 23, 2014
  24. Eric SunshineJun 25, 2014
  25. Tanay AbhraJun 26, 2014
  26. pager.c: replace git_config with git_config_get_stringTanay Abhra, Jun 23, 2014
  27. Eric SunshineJun 25, 2014
  28. Tanay AbhraJun 26, 2014
  29. Karsten BleesJun 26, 2014
  30. Matthieu MoyJun 27, 2014
  31. Karsten BleesJun 27, 2014
  32. Matthieu MoyJun 27, 2014
  33. Karsten BleesJun 28, 2014
  34. Matthieu MoyJun 28, 2014
  35. Karsten BleesJun 28, 2014
  36. Matthieu MoyJun 29, 2014
  37. Jonathan NiederJun 23, 2014
  38. Tanay AbhraJun 24, 2014
  39. Eric SunshineJun 25, 2014
  40. Tanay AbhraJun 26, 2014
  41. Matthieu MoyJun 26, 2014

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.