From mboxrd@z Thu Jan 1 00:00:00 1970 From: Karsten Blees Subject: Re: [RFC/PATCH] pager.c: replace git_config with git_config_get_string Date: Sat, 28 Jun 2014 16:29:17 +0200 Message-ID: <53AED13D.60705@gmail.com> References: <1403520105-23250-1-git-send-email-tanayabh@gmail.com> <1403520105-23250-6-git-send-email-tanayabh@gmail.com> <53AC6A7B.3040602@gmail.com> <53ADA26E.8030801@gmail.com> <53AE50A9.6010707@gmail.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="------------050504040606010403080901" Cc: Eric Sunshine , Tanay Abhra , Git List , Ramkumar Ramachandra To: Matthieu Moy X-From: git-owner@vger.kernel.org Sat Jun 28 16:29:40 2014 Return-path: Envelope-to: gcvg-git-2@plane.gmane.org Received: from vger.kernel.org ([209.132.180.67]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1X0tdL-0002BF-Tx for gcvg-git-2@plane.gmane.org; Sat, 28 Jun 2014 16:29:40 +0200 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752646AbaF1O3Y (ORCPT ); Sat, 28 Jun 2014 10:29:24 -0400 Received: from mail-wi0-f182.google.com ([209.85.212.182]:44403 "EHLO mail-wi0-f182.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752534AbaF1O3W (ORCPT ); Sat, 28 Jun 2014 10:29:22 -0400 Received: by mail-wi0-f182.google.com with SMTP id bs8so4079836wib.3 for ; Sat, 28 Jun 2014 07:29:21 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=message-id:date:from:user-agent:mime-version:to:cc:subject :references:in-reply-to:content-type; bh=JTDLonZHe2Kcd60X9sOIV77nSSokVAwfAC9IwTve/BA=; b=peRefUslVcBT9oosgCMfdZHLlYEIrgif9GnWUikeLWVZNQeIRSGS+9CkTCmtDM1sQh Ugs6/9LjvsN4iWwFMZsHdqS+XMgRpGl7Ob5tewVuHTi+sCH1EuLLqXvwDgmEvHmQSjPf V35juypga4i/hphMJoytuI8up05sxyUVvQJrgmY8Vs295rvQTxwTyEDZsbVYJjXALVjA ASf22MwHNcgNpnFFHCiGnv7AnhzpzSkDgN7JGqgg4v6yqQYaw5yPddMTlfMQ+V3A6wyI MCMNKufJ4ZAhpS7GI3CmVsV46g/CS8rT17dLF/kH9cbgJEdQcKJ21F54/qW7gzhADQGf SXqw== X-Received: by 10.180.207.9 with SMTP id ls9mr18540257wic.32.1403965761298; Sat, 28 Jun 2014 07:29:21 -0700 (PDT) Received: from [10.1.116.52] (ns.dcon.de. [77.244.111.149]) by mx.google.com with ESMTPSA id ft17sm28334778wjc.14.2014.06.28.07.29.14 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Sat, 28 Jun 2014 07:29:15 -0700 (PDT) User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20100101 Thunderbird/24.6.0 In-Reply-To: Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org Archived-At: This is a multi-part message in MIME format. --------------050504040606010403080901 Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Am 28.06.2014 08:01, schrieb Matthieu Moy: > Karsten Blees 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 : 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. --------------050504040606010403080901 Content-Type: text/plain; charset=windows-1252; name="git-config-set-callers.txt" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="git-config-set-callers.txt" 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 --------------050504040606010403080901--