better handle .gitmodules merge conflicts
v2 cover better handle .gitmodules merge conflicts
Am 13.05.2011 08:19, schrieb Junio C Hamano:
Quoted 4 lines
Quoted 1 line · Jens Lehmann
+static int
gitmodules_is_unmerged;Is it too cumbersome to pass this down the callchain as an argument?
I think so. Currently it influences the behavior of fetch, status, the diff family and commit. Having for example gitmodules_config() return that value and then pass it on would touch a lot of code (and that impact would grow even further with the recursive checkout series I'm working on).
So I thought it best to have this information statically in the submodule.c file and let the call sites who need to know that call functions there to get just the information they need and hide the details in submodule.c.
All other issues raised have been addressed in this series.
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 | 31 +++++++++++++- t/t7506-status-submodule.sh | 92 +++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 117 insertions(+), 6 deletions(-)
Signature · 1 line
1.7.5.1.291.g080bb
PATCH v2 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 | 92 +++++++++++++++++++++++++++++++++++++++++-- 1 files changed, 88 insertions(+), 4 deletions(-)
t/t7506-status-submodule.sh +88 -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,84 @@ test_expect_success 'status -a clean (empty submodule dir)' ' test_i18ngrep "nothing to commit" output ' +cat >status_expect <<\EOF +AA .gitmodules +A sub1 +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 -s >../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.291.g080bb
PATCH v2 2/2 Submodules: Don't parse .gitmodules when it contains, merge conflicts
In reply to Jens LehmannCommands 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 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. Users of git gui were not shown any conflicts at all when this happened.
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 | 31 +++++++++++++++++++++++++++++-- t/t7506-status-submodule.sh | 6 +++--- 2 files changed, 32 insertions(+), 5 deletions(-)
submodule.c +29 -2
@@ -14,6 +14,15 @@ 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 +72,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 +93,24 @@ 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 <0 ) {/* .gitmodules not found or isn't merged */ + pos =-1 - pos; +if (active_nr > pos) {/* there is a .gitmodules */ +const struct cache_entry *ce = active_cache[pos]; +if (ce_namelen (ce) ==11 && + !memcmp (ce->name,".gitmodules" ,11 )) + gitmodules_is_unmerged =1 ; + } + } + +if (!gitmodules_is_unmerged) +git_config_from_file (submodule_config, gitmodules_path.buf,NULL );strbuf_release (&gitmodules_path); } }
@@ -434,7 +460,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
@@ -196,7 +196,7 @@ AA .gitmodules A sub1 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 &&
@@ -255,7 +255,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
@@ -263,7 +263,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.291.g080bb
Re: [PATCH v2 2/2] Submodules: Don't parse .gitmodules when it contains, merge conflicts
Quoted 2 lines · Jens Lehmann
Commands like "git status", "git diff" and "git fetch" would fail when the .gitmodules file contained merge conflicts because ...
Side note. .gitattributes and .gitignore likely suffer the same. Perhaps the unmerged check in this patch can be generalized for reuse.
Signature · 1 line
Duy
Re: [PATCH v2 2/2] Submodules: Don't parse .gitmodules when it contains, merge conflicts
Am 14.05.2011 18:59, schrieb Nguyen Thai Ngoc Duy:
Quoted 6 lines · Jens Lehmann
Quoted 2 lines
Commands like "git status", "git diff" and "git fetch" would fail when the .gitmodules file contained merge conflicts because ...
Side note. .gitattributes and .gitignore likely suffer the same. Perhaps the unmerged check in this patch can be generalized for reuse.
Interesting thought. But adding conflict markers in those files doesn't break anything the way it did for .gitmodules, so this looks like a non-issue.
Re: [PATCH v2 2/2] Submodules: Don't parse .gitmodules when it contains, merge conflicts
In reply to Nguyen Thai Ngoc DuyQuoted 6 lines · Nguyen Thai Ngoc Duy
Quoted 2 lines · Jens Lehmann
Commands like "git status", "git diff" and "git fetch" would fail when the .gitmodules file contained merge conflicts because ...
Side note. .gitattributes and .gitignore likely suffer the same. Perhaps the unmerged check in this patch can be generalized for reuse.
I am somewhat skeptical about that statement. These files are designed to be one-entry-per-line so that useful information in the non-conflicted parts in a conflicted file (which should be majority) can still be used. Only the .gitmodules is an oddball.
Re: [PATCH v2 2/2] Submodules: Don't parse .gitmodules when it contains, merge conflicts
Quoted 6 lines · Junio C Hamano
Quoted 4 lines · Nguyen Thai Ngoc Duy
Quoted 2 lines · Jens Lehmann
Commands like "git status", "git diff" and "git fetch" would fail when the .gitmodules file contained merge conflicts because ...
Quoted 7 lines
Quoted 2 lines
Side note. .gitattributes and .gitignore likely suffer the same. Perhaps the unmerged check in this patch can be generalized for reuse.
I am somewhat skeptical about that statement. These files are designed to be one-entry-per-line so that useful information in the non-conflicted parts in a conflicted file (which should be majority) can still be used. Only the .gitmodules is an oddball.
True for .gitignore. A line in .gitattributes is more complex, it's easier to break the parser with conflict markers. Anyway unmerged entries should not be used until they're cleaned up, or can be used with a big fat warning.
Signature · 1 line
Duy