Am 28.06.2014 08:01, schrieb Matthieu Moy:
> Karsten Blees <karsten.blees@xxxxxxxxx> 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.
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