Re: [RFC PATCH 2/2] Submodules: Don't parse .gitmodules when it contains merge conflicts

From: Junio C Hamano <gitster@pobox.com>
Date: May 13, 2011, 06:19 UTC
Message-ID: <7vmxir9l7s.fsf@alter.siamese.dyndns.org>
In-Reply-To: <4DCC4B05.9070205@web.de>

Jens Lehmann <Jens.Lehmann@web.de> writes:

> Until now commands like "git status", "git diff" and "git fetch" would

s/Until now c/C/;

Show 6 quoted lines

> fail when the .gitmodules file contained merge conflicts because the
> config parser would call die() when hitting the conflict markers:
>
>     "fatal: bad config file line <n> in <path>/.gitmodules"
>
> While this was behavior was on the safe side, it is really unhelpful to

was--was

Show 14 quoted lines

> diff --git a/submodule.c b/submodule.c
> index 5294cef..cdf844c 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -14,6 +14,13 @@ static struct string_list config_fetch_recurse_submodules_for_name;
>  static struct string_list config_ignore_for_name;
>  static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND;
>  static struct string_list changed_submodule_paths;
> +/* The following flag is set if the .gitmodules file is unmerged. We then
> + * disable recursion for all submodules where .git/config doesn't have a
> + * matching config entry because we can't guess what might be configured in
> + * .gitmodules unless the user resolves the conflict. When a command line
> + * option is given (which always overrides configuration) this flag will be
> + * ignored. */

/*
 * We write our multi-line
 * comments like
 * this.
 */

> +static int gitmodules_is_unmerged;

Is it too cumbersome to pass this down the callchain as an argument?

Show 9 quoted lines

> +		if (read_cache() < 0)
> +			die("index file corrupt");
> +		pos = cache_name_pos(".gitmodules", 11);
> +		if (pos >= -1) {
> +			/* We have a clean, untracked or missing .gitmodules, try to parse it */
> +			git_config_from_file(submodule_config, gitmodules_path.buf, NULL);
> +		} else {
> +			gitmodules_is_unmerged = 1;
> +		}

I do not think this is correct.

If pos is zero or positive, you know you found ".gitmodules" that is merged. If it is -1, that means ".gitmodules" at stage #0 would be inserted at the beginning of the index, but that may perhaps be because the first cache entry in the index may be an unmerged ".gitmodules", no?

	pos = cache_name_pos(".gitmodules", 11);
	if (0 <= pos)
		; /* .gitmodules found and is merged */
	else {
        	pos = -1 - pos;
		if (active_nr <= pos)
	        	; /* .gitmodules does not exist */
	        ce = active_cache[pos];
	        if (ce_namelen(ce) == 11 &&
                    !memcmp(ce->name, ".gitmodules", 11))
			; /* .gitmodules unmerged */
		else
			; /* there is no .gitmodules */
	}