{"thread":{"id":"6264","subject":"Moving initialization of log_all_ref_updates","startedAt":"2007-01-07T09:24:41Z","lastAt":"2007-01-07T10:19:28Z","messageCount":5,"participants":["Junio C Hamano","Shawn O. Pearce"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"31023","messageId":"7vbqlb2nqe.fsf@assigned-by-dhcp.cox.net","threadId":"6264","inReplyTo":null,"subject":"Moving initialization of log_all_ref_updates","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-01-07T09:24:41Z","receivedAt":"2007-01-07T09:24:41Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The patches to prevent Porcelainish that require working tree\nfrom doing any damage in a bare repository make a lot of sense,\nand I want to make the is_bare_git_dir() function more reliable.\n\nIn order to allow the repository owner override the heuristic\nimplemented in is_bare_git_dir() if/when it misidentifies a\nparticular repository, it would make sense to introduce a new\nconfiguration variable \"[core] bare = true/false\", and make\nis_bare_git_dir() notice it.\n\nThe scripts would do a 'repo-config --bool --get core.bare' and\niff the command fails (i.e. there is no such variable in the\nconfiguration file), it would do the heuristic you implemented\nin your RFC patch [*1*].\n\nHowever, setup_git_env() which is called a lot earlier than we\neven read from the repository configuration currently makes a\ncall to is_bare_git_dir(), in order to change the default\nsetting for log_all_ref_updates.  It somehow feels that this is\na hack, and I am considering the following patch.  What do you\nthink?\n\nBy the way, [*1*] is another thing I hate about the current\nconfig mechanism.  \"git-repo-config --get\" does not know what\nthe possible configuration variables are, let alone what the\ndefault values for them are.  It allows us not to maintain a\ncentralized configuration table, which makes it easy to\nintroduce ad-hoc variables and gives a warm fuzzy feeling of\nbeing modular, but my feeling is that it is turning out to be a\nrather high price to pay for scripts.\n\n---\n\ndiff --git a/environment.c b/environment.c\nindex 09976c7..ee33bb1 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -15,7 +15,7 @@ int use_legacy_headers = 1;\n int trust_executable_bit = 1;\n int assume_unchanged;\n int prefer_symlink_refs;\n-int log_all_ref_updates;\n+int log_all_ref_updates = -1; /* unspecified */\n int warn_ambiguous_refs = 1;\n int repository_format_version;\n char *git_commit_encoding;\n@@ -51,10 +51,9 @@ static void setup_git_env(void)\n \tgit_graft_file = getenv(GRAFT_ENVIRONMENT);\n \tif (!git_graft_file)\n \t\tgit_graft_file = xstrdup(git_path(\"info/grafts\"));\n-\tlog_all_ref_updates = !is_bare_git_dir(git_dir);\n }\n \n-int is_bare_git_dir (const char *dir)\n+int is_bare_git_dir(const char *dir)\n {\n \tconst char *s;\n \tif (!strcmp(dir, DEFAULT_GIT_DIR_ENVIRONMENT))\ndiff --git a/refs.c b/refs.c\nindex 5205745..b5eee11 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -923,6 +923,9 @@ static int log_ref_write(struct ref_lock *lock,\n \tchar *logrec;\n \tconst char *committer;\n \n+\tif (log_all_ref_updates < 0)\n+\t\tlog_all_ref_updates = !is_bare_git_dir(get_git_dir());\n+\n \tif (log_all_ref_updates &&\n \t    (!strncmp(lock->ref_name, \"refs/heads/\", 11) ||\n \t     !strncmp(lock->ref_name, \"refs/remotes/\", 13))) {\n"},{"id":"31026","messageId":"20070107093725.GB10351@spearce.org","threadId":"6264","inReplyTo":"7vbqlb2nqe.fsf@assigned-by-dhcp.cox.net","subject":"Re: Moving initialization of log_all_ref_updates","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-01-07T09:37:26Z","receivedAt":"2007-01-07T09:37:26Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio C Hamano <junkio@cox.net> wrote:\n> The patches to prevent Porcelainish that require working tree\n> from doing any damage in a bare repository make a lot of sense,\n> and I want to make the is_bare_git_dir() function more reliable.\n> \n> In order to allow the repository owner override the heuristic\n> implemented in is_bare_git_dir() if/when it misidentifies a\n> particular repository, it would make sense to introduce a new\n> configuration variable \"[core] bare = true/false\", and make\n> is_bare_git_dir() notice it.\n\nYes, this is an excellent idea.\n \n> The scripts would do a 'repo-config --bool --get core.bare' and\n> iff the command fails (i.e. there is no such variable in the\n> configuration file), it would do the heuristic you implemented\n> in your RFC patch [*1*].\n\nWhich will come up right most of the time.  And when it doesn't,\ncore.bare is there to bail the user out.  I like it.\n\nDo we want to consider having init-db/clone set core.bare based on\nwhat they are being asked to do?\n\n> However, setup_git_env() which is called a lot earlier than we\n> even read from the repository configuration currently makes a\n> call to is_bare_git_dir(), in order to change the default\n> setting for log_all_ref_updates.  It somehow feels that this is\n> a hack, and I am considering the following patch.  What do you\n> think?\n\nAck'd.  There's little we can do here then to change the order things\nget invoked.  That's harder and uglier to change than to just declare\n\"we don't know what this right now\" and guess later.  I'd go with\nthe patch you have now.  Maybe post 1.5.0 we can cleanup some of the\ninit order to get the config file parsing for at least some really\nvery core properties to occur at the same time as setup_git_env().\n\nOne thing I don't like about the config file parsing is that\napplications can easily skip doing the core.* property setup.\nWith my sp/mmap changes this means core.packedGit{Limit,WindowSize}\nmay not be changed away from their defaults, and yet the application\nwill perform pack file access.  IMHO core.* is core; if you are\ngoing after pack files or loose objects you better also recognize\nand honor what core.* says!\n \n> By the way, [*1*] is another thing I hate about the current\n> config mechanism.  \"git-repo-config --get\" does not know what\n> the possible configuration variables are, let alone what the\n> default values for them are.  It allows us not to maintain a\n> centralized configuration table, which makes it easy to\n> introduce ad-hoc variables and gives a warm fuzzy feeling of\n> being modular, but my feeling is that it is turning out to be a\n> rather high price to pay for scripts.\n\nI was thinking the other day (after reading an earlier email from\nyou stating the exact same thing) that perhaps we should do a\nconfig registry within the repository.  E.g. if you want to access\na property in the config file you should also load its documentation\ninto a file like config.desc:\n\n  $ cat .git/config.desc\n  [core \"bare\"]\n    default = \n    package = core-git\n    documentation = \"Indicates ....\"\n\n  [gui \"fontdiff\"]\n    default = -family VT100 -size 9 -weight normal\n    package = git-gui\n    documentation = \"The font for display of a file diff.\"\n\nI'm not entirely happy with the above format; its just an example.\n\nObviously we can't change the current repo-config behavior, but\nwe could add a new \"--declared\" option which requires that the\nproperty being get/set is also described in .git/config.desc,\nfailing if it isn't.\n\n-- \nShawn.\n"},{"id":"31028","messageId":"7v4pr32mcn.fsf@assigned-by-dhcp.cox.net","threadId":"6264","inReplyTo":"20070107093725.GB10351@spearce.org","subject":"Re: Moving initialization of log_all_ref_updates","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-01-07T09:54:32Z","receivedAt":"2007-01-07T09:54:32Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" <spearce@spearce.org> writes:\n\n> Do we want to consider having init-db/clone set core.bare based on\n> what they are being asked to do?\n\nHere is what I have as a follow-up patch to the one you are\nresponding to.\n\n--\n\ndiff --git a/builtin-init-db.c b/builtin-init-db.c\nindex 97fd82f..e78222f 100644\n--- a/builtin-init-db.c\n+++ b/builtin-init-db.c\n@@ -252,9 +252,13 @@ static int create_default_files(const char *git_dir, const char *template_path)\n \t}\n \tgit_config_set(\"core.filemode\", filemode ? \"true\" : \"false\");\n \n-\t/* Enable logAllRefUpdates if a working tree is attached */\n-\tif (!is_bare_git_dir(git_dir))\n+\tif (is_bare_repository()) {\n+\t\tgit_config_set(\"core.bare\", \"true\");\n+\t}\n+\telse {\n+\t\tgit_config_set(\"core.bare\", \"false\");\n \t\tgit_config_set(\"core.logallrefupdates\", \"true\");\n+\t}\n \treturn reinit;\n }\n \ndiff --git a/cache.h b/cache.h\nindex 36be64e..cff2569 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -127,7 +127,8 @@ extern int cache_errno;\n #define CONFIG_LOCAL_ENVIRONMENT \"GIT_CONFIG_LOCAL\"\n #define EXEC_PATH_ENVIRONMENT \"GIT_EXEC_PATH\"\n \n-extern int is_bare_git_dir(const char *dir);\n+extern int is_bare_repository_cfg;\n+extern int is_bare_repository(void);\n extern const char *get_git_dir(void);\n extern char *get_object_directory(void);\n extern char *get_refs_directory(void);\ndiff --git a/config.c b/config.c\nindex 5cbd130..20e6ecc 100644\n--- a/config.c\n+++ b/config.c\n@@ -269,6 +269,11 @@ int git_default_config(const char *var, const char *value)\n \t\treturn 0;\n \t}\n \n+\tif (!strcmp(var, \"core.bare\")) {\n+\t\tis_bare_repository_cfg = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n+\n \tif (!strcmp(var, \"core.ignorestat\")) {\n \t\tassume_unchanged = git_config_bool(var, value);\n \t\treturn 0;\ndiff --git a/environment.c b/environment.c\nindex 64245e7..54c22f8 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -15,6 +15,7 @@ int use_legacy_headers = 1;\n int trust_executable_bit = 1;\n int assume_unchanged;\n int prefer_symlink_refs;\n+int is_bare_repository_cfg = -1; /* unspecified */\n int log_all_ref_updates = -1; /* unspecified */\n int warn_ambiguous_refs = 1;\n int repository_format_version;\n@@ -53,9 +54,13 @@ static void setup_git_env(void)\n \t\tgit_graft_file = xstrdup(git_path(\"info/grafts\"));\n }\n \n-int is_bare_git_dir(const char *dir)\n+int is_bare_repository(void)\n {\n-\tconst char *s;\n+\tconst char *dir, *s;\n+\tif (0 <= is_bare_repository_cfg)\n+\t\treturn is_bare_repository_cfg;\n+\n+\tdir = get_git_dir();\n \tif (!strcmp(dir, DEFAULT_GIT_DIR_ENVIRONMENT))\n \t\treturn 0;\n \ts = strrchr(dir, '/');\ndiff --git a/refs.c b/refs.c\nindex b5eee11..499086b 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -924,7 +924,7 @@ static int log_ref_write(struct ref_lock *lock,\n \tconst char *committer;\n \n \tif (log_all_ref_updates < 0)\n-\t\tlog_all_ref_updates = !is_bare_git_dir(get_git_dir());\n+\t\tlog_all_ref_updates = !is_bare_repository();\n \n \tif (log_all_ref_updates &&\n \t    (!strncmp(lock->ref_name, \"refs/heads/\", 11) ||\n"},{"id":"31029","messageId":"20070107100559.GC10351@spearce.org","threadId":"6264","inReplyTo":"7v4pr32mcn.fsf@assigned-by-dhcp.cox.net","subject":"Re: Moving initialization of log_all_ref_updates","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-01-07T10:05:59Z","receivedAt":"2007-01-07T10:05:59Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio C Hamano <junkio@cox.net> wrote:\n> \"Shawn O. Pearce\" <spearce@spearce.org> writes:\n> \n> > Do we want to consider having init-db/clone set core.bare based on\n> > what they are being asked to do?\n> \n> Here is what I have as a follow-up patch to the one you are\n> responding to.\n\nI like.  :-)\n\nWorthy of 1.5.0 me thinks.\n\n-- \nShawn.\n"},{"id":"31030","messageId":"7vy7of16mn.fsf_-_@assigned-by-dhcp.cox.net","threadId":"6264","inReplyTo":"20070107100559.GC10351@spearce.org","subject":"[PATCH] git-fetch: allow updating the current branch in a bare repository.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-01-07T10:19:28Z","receivedAt":"2007-01-07T10:19:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sometimes, people have only fetch access into a bare repository\nthat is used as a back-up location (or a distribution point) but\ndoes not have a push access for networking reasons, e.g. one end\nbeing behind a firewall, and updating the \"current branch\" in\nsuch a case is perfectly fine.\n\nThis allows such a fetch without --update-head-ok, which is a\nflag that should never be used by end users otherwise.\n\nSigned-off-by: Junio C Hamano <junkio@cox.net>\n---\n\n \"Shawn O. Pearce\" <spearce@spearce.org> writes:\n\n > Junio C Hamano <junkio@cox.net> wrote:\n >> ...\n >> Here is what I have as a follow-up patch to the one you are\n >> responding to.\n >\n > I like.  :-)\n >\n > Worthy of 1.5.0 me thinks.\n\n And here is an example of what you would do in the scripts.\n\n git-fetch.sh    |    9 +++++----\n git-sh-setup.sh |    8 ++++++++\n 2 files changed, 13 insertions(+), 4 deletions(-)\n\ndiff --git a/git-fetch.sh b/git-fetch.sh\nindex 466fe59..c58704d 100755\n--- a/git-fetch.sh\n+++ b/git-fetch.sh\n@@ -231,11 +231,12 @@ update_local_ref () {\n     esac\n }\n \n-case \"$update_head_ok\" in\n-'')\n+# updating the current HEAD with git-fetch in a bare\n+# repository is always fine.\n+if test -z \"$update_head_ok\" && test $(is_bare_repository) = false\n+then\n \torig_head=$(git-rev-parse --verify HEAD 2>/dev/null)\n-\t;;\n-esac\n+fi\n \n # If --tags (and later --heads or --all) is specified, then we are\n # not talking about defaults stored in Pull: line of remotes or\ndiff --git a/git-sh-setup.sh b/git-sh-setup.sh\nindex 87b939c..7fdc912 100755\n--- a/git-sh-setup.sh\n+++ b/git-sh-setup.sh\n@@ -28,6 +28,14 @@ set_reflog_action() {\n \tfi\n }\n \n+is_bare_repository () {\n+\tgit-repo-config --bool --get core.bare ||\n+\tcase \"$GIT_DIR\" in\n+\t.git | */.git) echo false ;;\n+\t*) echo true ;;\n+\tesac\n+}\n+\n if [ -z \"$LONG_USAGE\" ]\n then\n \tLONG_USAGE=\"Usage: $0 $USAGE\"\n"}]}