Moving initialization of log_all_ref_updates

Moving initialization of log_all_ref_updates

[Junio C Hamano]

The patches to prevent Porcelainish that require working tree
from doing any damage in a bare repository make a lot of sense,
and I want to make the is_bare_git_dir() function more reliable.

In order to allow the repository owner override the heuristic
implemented in is_bare_git_dir() if/when it misidentifies a
particular repository, it would make sense to introduce a new
configuration variable "[core] bare = true/false", and make
is_bare_git_dir() notice it.

The scripts would do a 'repo-config --bool --get core.bare' and
iff the command fails (i.e. there is no such variable in the
configuration file), it would do the heuristic you implemented
in your RFC patch [*1*].

However, setup_git_env() which is called a lot earlier than we
even read from the repository configuration currently makes a
call to is_bare_git_dir(), in order to change the default
setting for log_all_ref_updates.  It somehow feels that this is
a hack, and I am considering the following patch.  What do you
think?

By the way, [*1*] is another thing I hate about the current
config mechanism.  "git-repo-config --get" does not know what
the possible configuration variables are, let alone what the
default values for them are.  It allows us not to maintain a
centralized configuration table, which makes it easy to
introduce ad-hoc variables and gives a warm fuzzy feeling of
being modular, but my feeling is that it is turning out to be a
rather high price to pay for scripts.

---

diff --git a/environment.c b/environment.c
index 09976c7..ee33bb1 100644
--- a/environment.c
+++ b/environment.c
@@ -15,7 +15,7 @@ int use_legacy_headers = 1;
 int trust_executable_bit = 1;
 int assume_unchanged;
 int prefer_symlink_refs;
-int log_all_ref_updates;
+int log_all_ref_updates = -1; /* unspecified */
 int warn_ambiguous_refs = 1;
 int repository_format_version;
 char *git_commit_encoding;
@@ -51,10 +51,9 @@ static void setup_git_env(void)
 	git_graft_file = getenv(GRAFT_ENVIRONMENT);
 	if (!git_graft_file)
 		git_graft_file = xstrdup(git_path("info/grafts"));
-	log_all_ref_updates = !is_bare_git_dir(git_dir);
 }
 
-int is_bare_git_dir (const char *dir)
+int is_bare_git_dir(const char *dir)
 {
 	const char *s;
 	if (!strcmp(dir, DEFAULT_GIT_DIR_ENVIRONMENT))
diff --git a/refs.c b/refs.c
index 5205745..b5eee11 100644
--- a/refs.c
+++ b/refs.c
@@ -923,6 +923,9 @@ static int log_ref_write(struct ref_lock *lock,
 	char *logrec;
 	const char *committer;
 
+	if (log_all_ref_updates < 0)
+		log_all_ref_updates = !is_bare_git_dir(get_git_dir());
+
 	if (log_all_ref_updates &&
 	    (!strncmp(lock->ref_name, "refs/heads/", 11) ||
 	     !strncmp(lock->ref_name, "refs/remotes/", 13))) {

Re: Moving initialization of log_all_ref_updates

[Shawn O. Pearce]

Junio C Hamano <junkio@cox.net> wrote:
> The patches to prevent Porcelainish that require working tree
> from doing any damage in a bare repository make a lot of sense,
> and I want to make the is_bare_git_dir() function more reliable.
> 
> In order to allow the repository owner override the heuristic
> implemented in is_bare_git_dir() if/when it misidentifies a
> particular repository, it would make sense to introduce a new
> configuration variable "[core] bare = true/false", and make
> is_bare_git_dir() notice it.

Yes, this is an excellent idea.
 
> The scripts would do a 'repo-config --bool --get core.bare' and
> iff the command fails (i.e. there is no such variable in the
> configuration file), it would do the heuristic you implemented
> in your RFC patch [*1*].

Which will come up right most of the time.  And when it doesn't,
core.bare is there to bail the user out.  I like it.

Do we want to consider having init-db/clone set core.bare based on
what they are being asked to do?

> However, setup_git_env() which is called a lot earlier than we
> even read from the repository configuration currently makes a
> call to is_bare_git_dir(), in order to change the default
> setting for log_all_ref_updates.  It somehow feels that this is
> a hack, and I am considering the following patch.  What do you
> think?

Ack'd.  There's little we can do here then to change the order things
get invoked.  That's harder and uglier to change than to just declare
"we don't know what this right now" and guess later.  I'd go with
the patch you have now.  Maybe post 1.5.0 we can cleanup some of the
init order to get the config file parsing for at least some really
very core properties to occur at the same time as setup_git_env().

One thing I don't like about the config file parsing is that
applications can easily skip doing the core.* property setup.
With my sp/mmap changes this means core.packedGit{Limit,WindowSize}
may not be changed away from their defaults, and yet the application
will perform pack file access.  IMHO core.* is core; if you are
going after pack files or loose objects you better also recognize
and honor what core.* says!
 
> By the way, [*1*] is another thing I hate about the current
> config mechanism.  "git-repo-config --get" does not know what
> the possible configuration variables are, let alone what the
> default values for them are.  It allows us not to maintain a
> centralized configuration table, which makes it easy to
> introduce ad-hoc variables and gives a warm fuzzy feeling of
> being modular, but my feeling is that it is turning out to be a
> rather high price to pay for scripts.

I was thinking the other day (after reading an earlier email from
you stating the exact same thing) that perhaps we should do a
config registry within the repository.  E.g. if you want to access
a property in the config file you should also load its documentation
into a file like config.desc:

  $ cat .git/config.desc
  [core "bare"]
    default = 
    package = core-git
    documentation = "Indicates ...."

  [gui "fontdiff"]
    default = -family VT100 -size 9 -weight normal
    package = git-gui
    documentation = "The font for display of a file diff."

I'm not entirely happy with the above format; its just an example.

Obviously we can't change the current repo-config behavior, but
we could add a new "--declared" option which requires that the
property being get/set is also described in .git/config.desc,
failing if it isn't.

-- 
Shawn.

Re: Moving initialization of log_all_ref_updates

[Junio C Hamano]

"Shawn O. Pearce" <spearce@spearce.org> writes:

> Do we want to consider having init-db/clone set core.bare based on
> what they are being asked to do?

Here is what I have as a follow-up patch to the one you are
responding to.

--

diff --git a/builtin-init-db.c b/builtin-init-db.c
index 97fd82f..e78222f 100644
--- a/builtin-init-db.c
+++ b/builtin-init-db.c
@@ -252,9 +252,13 @@ static int create_default_files(const char *git_dir, const char *template_path)
 	}
 	git_config_set("core.filemode", filemode ? "true" : "false");
 
-	/* Enable logAllRefUpdates if a working tree is attached */
-	if (!is_bare_git_dir(git_dir))
+	if (is_bare_repository()) {
+		git_config_set("core.bare", "true");
+	}
+	else {
+		git_config_set("core.bare", "false");
 		git_config_set("core.logallrefupdates", "true");
+	}
 	return reinit;
 }
 
diff --git a/cache.h b/cache.h
index 36be64e..cff2569 100644
--- a/cache.h
+++ b/cache.h
@@ -127,7 +127,8 @@ extern int cache_errno;
 #define CONFIG_LOCAL_ENVIRONMENT "GIT_CONFIG_LOCAL"
 #define EXEC_PATH_ENVIRONMENT "GIT_EXEC_PATH"
 
-extern int is_bare_git_dir(const char *dir);
+extern int is_bare_repository_cfg;
+extern int is_bare_repository(void);
 extern const char *get_git_dir(void);
 extern char *get_object_directory(void);
 extern char *get_refs_directory(void);
diff --git a/config.c b/config.c
index 5cbd130..20e6ecc 100644
--- a/config.c
+++ b/config.c
@@ -269,6 +269,11 @@ int git_default_config(const char *var, const char *value)
 		return 0;
 	}
 
+	if (!strcmp(var, "core.bare")) {
+		is_bare_repository_cfg = git_config_bool(var, value);
+		return 0;
+	}
+
 	if (!strcmp(var, "core.ignorestat")) {
 		assume_unchanged = git_config_bool(var, value);
 		return 0;
diff --git a/environment.c b/environment.c
index 64245e7..54c22f8 100644
--- a/environment.c
+++ b/environment.c
@@ -15,6 +15,7 @@ int use_legacy_headers = 1;
 int trust_executable_bit = 1;
 int assume_unchanged;
 int prefer_symlink_refs;
+int is_bare_repository_cfg = -1; /* unspecified */
 int log_all_ref_updates = -1; /* unspecified */
 int warn_ambiguous_refs = 1;
 int repository_format_version;
@@ -53,9 +54,13 @@ static void setup_git_env(void)
 		git_graft_file = xstrdup(git_path("info/grafts"));
 }
 
-int is_bare_git_dir(const char *dir)
+int is_bare_repository(void)
 {
-	const char *s;
+	const char *dir, *s;
+	if (0 <= is_bare_repository_cfg)
+		return is_bare_repository_cfg;
+
+	dir = get_git_dir();
 	if (!strcmp(dir, DEFAULT_GIT_DIR_ENVIRONMENT))
 		return 0;
 	s = strrchr(dir, '/');
diff --git a/refs.c b/refs.c
index b5eee11..499086b 100644
--- a/refs.c
+++ b/refs.c
@@ -924,7 +924,7 @@ static int log_ref_write(struct ref_lock *lock,
 	const char *committer;
 
 	if (log_all_ref_updates < 0)
-		log_all_ref_updates = !is_bare_git_dir(get_git_dir());
+		log_all_ref_updates = !is_bare_repository();
 
 	if (log_all_ref_updates &&
 	    (!strncmp(lock->ref_name, "refs/heads/", 11) ||

Re: Moving initialization of log_all_ref_updates

[Shawn O. Pearce]

Junio C Hamano <junkio@cox.net> wrote:
> "Shawn O. Pearce" <spearce@spearce.org> writes:
> 
> > Do we want to consider having init-db/clone set core.bare based on
> > what they are being asked to do?
> 
> Here is what I have as a follow-up patch to the one you are
> responding to.

I like.  :-)

Worthy of 1.5.0 me thinks.

-- 
Shawn.

[PATCH] git-fetch: allow updating the current branch in a bare repository.

[Junio C Hamano]

Sometimes, people have only fetch access into a bare repository
that is used as a back-up location (or a distribution point) but
does not have a push access for networking reasons, e.g. one end
being behind a firewall, and updating the "current branch" in
such a case is perfectly fine.

This allows such a fetch without --update-head-ok, which is a
flag that should never be used by end users otherwise.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---

 "Shawn O. Pearce" <spearce@spearce.org> writes:

 > Junio C Hamano <junkio@cox.net> wrote:
 >> ...
 >> Here is what I have as a follow-up patch to the one you are
 >> responding to.
 >
 > I like.  :-)
 >
 > Worthy of 1.5.0 me thinks.

 And here is an example of what you would do in the scripts.

 git-fetch.sh    |    9 +++++----
 git-sh-setup.sh |    8 ++++++++
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/git-fetch.sh b/git-fetch.sh
index 466fe59..c58704d 100755
--- a/git-fetch.sh
+++ b/git-fetch.sh
@@ -231,11 +231,12 @@ update_local_ref () {
     esac
 }
 
-case "$update_head_ok" in
-'')
+# updating the current HEAD with git-fetch in a bare
+# repository is always fine.
+if test -z "$update_head_ok" && test $(is_bare_repository) = false
+then
 	orig_head=$(git-rev-parse --verify HEAD 2>/dev/null)
-	;;
-esac
+fi
 
 # If --tags (and later --heads or --all) is specified, then we are
 # not talking about defaults stored in Pull: line of remotes or
diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index 87b939c..7fdc912 100755
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -28,6 +28,14 @@ set_reflog_action() {
 	fi
 }
 
+is_bare_repository () {
+	git-repo-config --bool --get core.bare ||
+	case "$GIT_DIR" in
+	.git | */.git) echo false ;;
+	*) echo true ;;
+	esac
+}
+
 if [ -z "$LONG_USAGE" ]
 then
 	LONG_USAGE="Usage: $0 $USAGE"