better handle .gitmodules merge conflicts
cover better handle .gitmodules merge conflicts
This series contains a test to reproduce and a first attempt to fix the problem that "git diff", "git status" and "git fetch" die early when the .gitmodules file contains merge conflict markers. As I am not aware of any bug reports yet it looks like that either doesn't happen very often in the wild ("git diff" parses the .gitmodules file since 1.7.3, but even as heavy submodule users we only hit this once just recently) or the users encountering this problem just know what to do: resolve the conflict and carry on.
But that is no excuse to behave so unfriendly, especially as this can happen in a completely normal workflow, when e.g. two users are adding different submodules in separate branches and they get merged. So this RFC patch is my first attempt to avoid those commands dying.
In a second step a merge helper capable of merging inifiles would make sense. This should be enabled for the .gitmodules file by default and avoid most of the merge conflicts, e.g. when two users add different submodules in separate branches. But that is a different patch ...
Heiko Voigt (1):
test that git status works with merge conflict in .gitmodules
Jens Lehmann (1):
Submodules: Don't parse .gitmodules when it contains merge conflicts
submodule.c | 23 +++++++++- t/t7506-status-submodule.sh | 100 +++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 117 insertions(+), 6 deletions(-)
Signature · 1 line
1.7.5.1.251.ga75dd
RFC PATCH 1/2 test that git status works with merge conflict in .gitmodules
From: Heiko Voigt <[email protected]>
For example: Two users independently adding a submodule will result in a merge conflict in .gitmodules. Since configuration of the status and diff machinery depends on the file being parseable they currently fail to produce useable output in case .gitmodules is marked with a merge conflict.
--- t/t7506-status-submodule.sh | 100 +++++++++++++++++++++++++++++++++++++++++-- 1 files changed, 96 insertions(+), 4 deletions(-)
t/t7506-status-submodule.sh +96 -4
@@ -4,17 +4,21 @@ test_description='git status for submodule' . ./test-lib.sh -test_expect_success 'setup' ' - test_create_repo sub && +test_create_repo_with_commit () { + test_create_repo $1 && ( - cd sub && + cd $1 && : >bar && git add bar && git commit -m " Add bar" && : >foo && git add foo && git commit -m " Add foo" - ) && + ) +} + +test_expect_success 'setup' ' + test_create_repo_with_commit sub && echo output > .gitignore && git add sub .gitignore && git commit -m "Add submodule sub"
@@ -187,4 +191,92 @@ test_expect_success 'status -a clean (empty submodule dir)' ' test_i18ngrep "nothing to commit" output ' +cat > status_expect << EOF +# On branch merge_conflict_gitmodules +# Changes to be committed: +# +# new file: sub1 +# +# Unmerged paths: +# (use "git add/rm <file>..." as appropriate to mark resolution) +# +# both added: .gitmodules +# +EOF + +test_expect_failure 'status with merge conflict in .gitmodules' ' + git clone . super && + test_create_repo_with_commit sub1 && + test_tick && + test_create_repo_with_commit sub2 && + ( + cd super && + prev=$(git rev-parse HEAD) && + git checkout -b add_sub1 && + git submodule add ../sub1 && + git commit -m "add sub1" && + git checkout -b add_sub2 $prev && + git submodule add ../sub2 && + git commit -m "add sub2" && + git checkout -b merge_conflict_gitmodules && + test_must_fail git merge add_sub1 && + git status > ../status_actual 2>&1 + ) && + test_cmp status_actual status_expect +' + +sha1_merge_sub1=$(cd sub1 && git rev-parse HEAD) +sha1_merge_sub2=$(cd sub2 && git rev-parse HEAD) +short_sha1_merge_sub1=$(cd sub1 && git rev-parse --short HEAD) +short_sha1_merge_sub2=$(cd sub2 && git rev-parse --short HEAD) +cat > diff_expect << EOF +diff --cc .gitmodules +index badaa4c,44f999a..0000000 +--- a/.gitmodules ++++ b/.gitmodules +@@@ -1,3 -1,3 +1,9 @@@ +++<<<<<<< HEAD + +[submodule "sub2"] + + path = sub2 + + url = ../sub2 +++======= ++ [submodule "sub1"] ++ path = sub1 ++ url = ../sub1 +++>>>>>>> add_sub1 +EOF + +cat > diff_submodule_expect << EOF +diff --cc .gitmodules +index badaa4c,44f999a..0000000 +--- a/.gitmodules ++++ b/.gitmodules +@@@ -1,3 -1,3 +1,9 @@@ +++<<<<<<< HEAD + +[submodule "sub2"] + + path = sub2 + + url = ../sub2 +++======= ++ [submodule "sub1"] ++ path = sub1 ++ url = ../sub1 +++>>>>>>> add_sub1 +EOF + +test_expect_failure 'diff with merge conflict in .gitmodules' ' + ( + cd super && + git diff > ../diff_actual 2>&1 + ) && + test_cmp diff_actual diff_expect +' + +test_expect_failure 'diff --submodule with merge conflict in .gitmodules' ' + ( + cd super && + git diff --submodule > ../diff_submodule_actual 2>&1 + ) && + test_cmp diff_submodule_actual diff_submodule_expect +' + test_done
Signature · 1 line
1.7.5.1.251.ga75dd
Re: [RFC PATCH 1/2] test that git status works with merge conflict in .gitmodules
Quoted 9 lines · Jens Lehmann
From: Heiko Voigt <[email protected]>
For example: Two users independently adding a submodule will result in a merge conflict in .gitmodules. Since configuration of the status and diff machinery depends on the file being parseable they currently fail to produce useable output in case .gitmodules is marked with a merge conflict.
Signed-off-by Heiko Voigt <[email protected]>
The basic idea (both patches) seems sound.
Quoted 5 lines
t/t7506-status-submodule.sh +0
-0
t/t7506-status-submodule.sh test_description='git status for submodule' · line 10
Quoted 10 lines
t/t7506-status-submodule.sh +3
-3@@ -4,17 +4,21 @@ test_description='git status for submodule' . ./test-lib.sh -test_expect_success 'setup' ' - test_create_repo sub && +test_create_repo_with_commit () { + test_create_repo $1 && ( - cd sub && + cd $1 &&
Missing dq, i.e. "$1" (two instances).
t/t7506-status-submodule.sh test_expect_success 'status -a clean (empty submodule dir)' ' · line 194
Quoted 1 line
t/t7506-status-submodule.sh +1
-0+cat > status_expect << EOF
cat >status_expect <<\EOF
Unless you actively mean the here-text has variables that need to be substituted, always quote the end-of-here-text delimiter. That would reduce the mental burden of people who have to read the script, as they do not have to look for a potential substitution that does not exist.
t/t7506-status-submodule.sh test_expect_success 'status -a clean (empty submodule dir)' ' · line 205
Quoted 11 lines
t/t7506-status-submodule.sh +11
-0+# On branch merge_conflict_gitmodules +# Changes to be committed: +# +# new file: sub1 +# +# Unmerged paths: +# (use "git add/rm <file>..." as appropriate to mark resolution) +# +# both added: .gitmodules +# +EOF
Is it absolutely necessary to run this test by comparing the output of human readable version of "git status" output? It makes it impossible to test under gettext-poison. Can you use "git status -s" output instead?
t/t7506-status-submodule.sh test_expect_success 'status -a clean (empty submodule dir)' ' · line 280
Quoted 8 lines
...
t/t7506-status-submodule.sh +7
-0+test_expect_failure 'diff --submodule with merge conflict in .gitmodules' ' + ( + cd super && + git diff --submodule > ../diff_submodule_actual 2>&1 + ) && + test_cmp diff_submodule_actual diff_submodule_expect +'
You have too many ">" redirect followed by an unnecessary SP. There are existing examples in the test, please do not make things worse.
RFC PATCH 2/2 Submodules: Don't parse .gitmodules when it contains merge conflicts
In reply to Jens LehmannUntil now commands like "git status", "git diff" and "git fetch" would 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 the user to have commands like status and diff fail, as these are needed to find out what's going on. And the error message is only mildly helpful, as it points to the right file but doesn't mention that it is unmerged.
Improve the situation by checking if the index records .gitmodules as unmerged. When that is the case we can't make any assumptions about the configuration to be found there after the merge conflict is resolved by the user, so assume that all recursion is disabled unless .git/config or the global config say otherwise.
As soon as the merge conflict is resolved and the .gitmodules file has been staged subsequent commands again honor any configuration done there.
--- submodule.c | 23 +++++++++++++++++++++-- t/t7506-status-submodule.sh | 6 +++--- 2 files changed, 24 insertions(+), 5 deletions(-)
submodule.c +21 -2
@@ -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. */ +static int gitmodules_is_unmerged;static int add_submodule_odb (const char *path) {
@@ -63,6 +70,8 @@ void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt, ignore_option =unsorted_string_list_lookup (&config_ignore_for_name, path_option->util);if (ignore_option)handle_ignore_submodules_arg (diffopt, ignore_option->util); +else if (gitmodules_is_unmerged) +DIFF_OPT_SET (diffopt,IGNORE_SUBMODULES ); } }
@@ -82,9 +91,18 @@ void gitmodules_config(void)const char *work_tree =get_git_work_tree ();if (work_tree) {struct strbuf gitmodules_path =STRBUF_INIT ; +int pos;strbuf_addstr (&gitmodules_path, work_tree);strbuf_addstr (&gitmodules_path,"/.gitmodules" ); -git_config_from_file (submodule_config, gitmodules_path.buf,NULL ); +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 ; + }strbuf_release (&gitmodules_path); } }
@@ -434,7 +452,8 @@ int fetch_populated_submodules(int num_options, const char **options, default_argv ="on-demand" ; } }else { -if (config_fetch_recurse_submodules ==RECURSE_SUBMODULES_OFF ) +if ((config_fetch_recurse_submodules ==RECURSE_SUBMODULES_OFF ) || + gitmodules_is_unmerged)continue ;if (config_fetch_recurse_submodules ==RECURSE_SUBMODULES_ON_DEMAND ) {if (!unsorted_string_list_lookup (&changed_submodule_paths, ce->name))
t/t7506-status-submodule.sh +3 -3
@@ -204,7 +204,7 @@ cat > status_expect << EOF # EOF -test_expect_failure 'status with merge conflict in .gitmodules' ' +test_expect_success 'status with merge conflict in .gitmodules' ' git clone . super && test_create_repo_with_commit sub1 && test_tick &&
@@ -263,7 +263,7 @@ index badaa4c,44f999a..0000000 ++>>>>>>> add_sub1 EOF -test_expect_failure 'diff with merge conflict in .gitmodules' ' +test_expect_success 'diff with merge conflict in .gitmodules' ' ( cd super && git diff > ../diff_actual 2>&1
@@ -271,7 +271,7 @@ test_expect_failure 'diff with merge conflict in .gitmodules' ' test_cmp diff_actual diff_expect ' -test_expect_failure 'diff --submodule with merge conflict in .gitmodules' ' +test_expect_success 'diff --submodule with merge conflict in .gitmodules' ' ( cd super && git diff --submodule > ../diff_submodule_actual 2>&1
Signature · 1 line
1.7.5.1.251.ga75dd
Re: [RFC PATCH 2/2] Submodules: Don't parse .gitmodules when it contains merge conflicts
Quoted 1 line · Jens Lehmann
Until now commands like "git status", "git diff" and "git fetch" would
s/Until now c/C/;
Quoted 6 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
Quoted 5 lines
submodule.c +0
-0
submodule.c static struct string_list config_fetch_recurse_submodules_for_name; · line 22
Quoted 9 lines
submodule.c +6
-0@@ -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.
*/
submodule.c static struct string_list config_fetch_recurse_submodules_for_name; · line 23
Quoted 1 line
submodule.c +1
-0+static int gitmodules_is_unmerged;
Is it too cumbersome to pass this down the callchain as an argument?
submodule.c void gitmodules_config(void) · line 105
Quoted 9 lines
submodule.c +9
-0+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 */
}Quoted 8 lines · Jens Lehmann
This series contains a test to reproduce and a first attempt to fix the problem that "git diff", "git status" and "git fetch" die early when the .gitmodules file contains merge conflict markers. As I am not aware of any bug reports yet it looks like that either doesn't happen very often in the wild ("git diff" parses the .gitmodules file since 1.7.3, but even as heavy submodule users we only hit this once just recently) or the users encountering this problem just know what to do: resolve the conflict and carry on.
I fell in the latter camp when I ran into this.
I like to examine conflicts with "git gui" but IIRC it doesn't display any conflicting files when there's a conflict in .gitmodules (it won't even display conflicts in other files, presumably because some plumbing failed early on the .gitmodules conflict). That was quite a "WTF?" moment...
Quoted 4 lines
But that is no excuse to behave so unfriendly, especially as this can happen in a completely normal workflow, when e.g. two users are adding different submodules in separate branches and they get merged. So this RFC patch is my first attempt to avoid those commands dying.
Yup -- thanks for addressing this!
M.