Re: [PATCH 4/4] config: allow including config from repository blobs

On Thu, Jan 26, 2012 at 10:25:32AM +0100, Johannes Sixt wrote:

Am 1/26/2012 8:42, schrieb Jeff King:

quoted

+static int handle_ref_include(const char *ref, void *data)
+{
+	unsigned char sha1[20];
+	char *buf;
+	unsigned long size;
+	enum object_type type;
+	int ret;
+
+	if (get_sha1(ref, sha1))
+		return 0;
+	buf = read_sha1_file(sha1, &type, &size);
+	if (!buf)
+		return error("unable to read include ref '%s'", ref);
+	if (type != OBJ_BLOB)
+		return error("include ref '%s' is not a blob", ref);
+
+	ret = git_config_from_buffer(git_config_include, data, ref, buf, size);
+	free(buf);
+	return ret;
+}

What happens if a ref cannot be resolved, for example due to repository
corruption? Does git just emit an error and then carries on, or does it
always die? Can I run at least git-fsck in such a case?

Names which do not resolve are explicitly ignored, because I wanted to
flexibility in specifying the includes. E.g., you might say put:

  [include]
          ref = refs/config

in your ~/.gitconfig, and then only use the feature in some of your
repositories (I'm not sure if that is a good idea yet in practice or
not. But as I said before, I think of this is a building block, and I'd
like people to experiment and see if it fills their needs).

Obviously the trade-off is that we would silently ignore a typo in the
object name.

However, I did explicitly return an error for a failure to find a sha1,
or a non-blob sha1, as those are more severe configuration errors (where
the former is basically repository corruption). We only return an error
here, but git_config will eventually die() because of it, noting the
file and line number where the include happened[1].

It's then up to you to fix the config file before you can continue using
git. You can do so by hand, but I think using "git config" to do so will
not work; even though it correctly does not expand includes while
writing, the git wrapper incidentally reads the config before actually
running the config command.

We already have similar problems where setting a bool option to a
non-bool value will cause many git commands git to die (e.g., try
setting "color.ui" to "foo"). But it's less often an issue, because
unless the config option you have messed up is very basic (like
core.bare), you can still run "git config".

So it's certainly recoverable if you are comfortable editing the config
file. But we could also make it a little friendlier by turning those
errors into warnings, at the minor cost of making errors less
noticeable.

-Peff

[1] The error reporting just shows the source of the deepest file,
    because git_parse_file will actually call die(). So if I have a
    .git/config that includes a ref that includes another ref that has
    an error, I see only:

      $ git config foo.value
      error: include ref 'HEAD:subdir' is not a blob
      fatal: bad config file line 5 in HEAD:config

    But solving the problem without git is hard. I know the problem is
    in the ref, but I can't edit the ref. The source of the include
    chain has to come from a file I can edit outside of git (since we
    always start from the files), but I'm not told which file included
    it.

    So it would be a little nicer to say something like:

      error: include ref 'HEAD:subdir' is not a blob (at HEAD:config, line 5)
      error: included file 'HEAD:config' had errors at .git/config, line 9
      fatal: unable to parse configuration

    which shows the complete trail, and you know to edit .git/config. In
    practice, I don't know if it is much of an issue. There are only 3
    places that git actually reads config from, and it is likely that
    you just edited one.