| Message ID | 20210222172623.69313-2-periperidip@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | rm: stage submodule removal from '.gitmodules' | expand |
Shourya Shukla <periperidip@gmail.com> writes: > Currently, using 'git rm --cached <submodule>' removes the submodule > <submodule> from the index and leaves the submodule working tree > intact in the superproject working tree, but does not stage any > changes to the '.gitmodules' file, in contrast to 'git rm <submodule>', > which removes both the submodule and its configuration in '.gitmodules' > from the worktree and index. > > Fix this inconsistency by also staging the removal of the entry of the > submodule from the '.gitmodules' file, leaving the worktree copy intact, The "also" above felt a bit puzzling, as we would be removing the entry only from the indexed copy without touching the working tree (by the way, I try to be precise in terminology between worktree and working tree, and please follow suit. A working tree is what you have in a non-bare repository that let's you "less" "gcc" etc. on the files checked out. A worktree refers to the mechanism that lets you have separate working tree by borrowing from a repository, or refers to an instance of a working tree plus .git file created by the mechanism. You mean "working tree" in the above sentence), but it refers to "remove the submodules directory and also entry", so it is OK. > diff --git a/builtin/rm.c b/builtin/rm.c > index 4858631e0f..5854ef0996 100644 > --- a/builtin/rm.c > +++ b/builtin/rm.c > @@ -254,7 +254,7 @@ static struct option builtin_rm_options[] = { > int cmd_rm(int argc, const char **argv, const char *prefix) > { > struct lock_file lock_file = LOCK_INIT; > - int i; > + int i, removed = 0; > struct pathspec pathspec; > char *seen; > > @@ -365,30 +365,33 @@ int cmd_rm(int argc, const char **argv, const char *prefix) > if (show_only) > return 0; > > + for (i = 0; i < list.nr; i++) { > + const char *path = list.entry[i].name; > + if (list.entry[i].is_submodule) { > + /* > + * Then, unless we used "--cached", remove the filenames from > + * the workspace. If we fail to remove the first one, we > + * abort the "git rm" (but once we've successfully removed > + * any file at all, we'll go ahead and commit to it all: > + * by then we've already committed ourselves and can't fail > + * in the middle) > + */ > + if (!index_only) { > + struct strbuf buf = STRBUF_INIT; > strbuf_reset(&buf); > strbuf_addstr(&buf, path); > if (remove_dir_recursively(&buf, 0)) > die(_("could not remove '%s'"), path); > > removed = 1; > + strbuf_release(&buf); OK, so this part now only deals with the submodule directory. > } > + if (!remove_path_from_gitmodules(path, index_only)) > + stage_updated_gitmodules(&the_index); And the entry for it in .gitmodules is handled by the helper, whether --cached or not. This somehow feels wrong for the index_only case; doesn't the helper take contents from the .gitmodules in the working tree and add it to the index? Unless you touched stage_updated_gitmodules() not to do that in the index_only case, that is. > + continue; And that is all for what is done to a submodule. Makes sense so far. > + } > + if (!index_only) { > if (!remove_path(path)) { > removed = 1; > continue; > @@ -396,9 +399,6 @@ int cmd_rm(int argc, const char **argv, const char *prefix) > if (!removed) > die_errno("git rm: '%s'", path); > } > - strbuf_release(&buf); > - if (gitmodules_modified) > - stage_updated_gitmodules(&the_index); OK, because this should have been done where we called remove_path_from_gitmodules(). > } > > if (write_locked_index(&the_index, &lock_file, > diff --git a/submodule.c b/submodule.c > index 9767ba9893..6ce8c8d0d8 100644 > --- a/submodule.c > +++ b/submodule.c > @@ -131,7 +131,7 @@ int update_path_in_gitmodules(const char *oldpath, const char *newpath) > * path is configured. Return 0 only if a .gitmodules file was found, a section > * with the correct path=<path> setting was found and we could remove it. > */ > -int remove_path_from_gitmodules(const char *path) > +int remove_path_from_gitmodules(const char *path, int index_only) > { > struct strbuf sect = STRBUF_INIT; > const struct submodule *submodule; > @@ -149,7 +149,8 @@ int remove_path_from_gitmodules(const char *path) > } > strbuf_addstr(§, "submodule."); > strbuf_addstr(§, submodule->name); > - if (git_config_rename_section_in_file(GITMODULES_FILE, sect.buf, NULL) < 0) { > + if (git_config_rename_section_in_file(index_only ? GITMODULES_INDEX : > + GITMODULES_FILE, sect.buf, NULL) < 0) { > /* Maybe the user already did that, don't error out here */ > warning(_("Could not remove .gitmodules entry for %s"), path); > strbuf_release(§); When !index_only, do we have any guarantee that .gitmodules in the working tree and .gitmodules in the index are in sync? I somehow doubt it. I would have expected that the updated remove_path_from_gitmodules() would look more like: - only if !index_only, nuke the section for the submodule in .gitmodules in the working tree. - nuke the section for the submodule in .gitmodules in the index. IOW, there would be two git_config_rename_section_in_file() calls to remove the section in !index_only case. Doing so would also mean that you should not have the caller call stage_updated_gitmodules() at all, even in !index_only case. Imagine if the .gitmodules file in the working tree had local changes (e.g. registered a few more submodules, or updated the url field of a few submodules) that are not yet added to the index when "git rm" removed a submodule. The user does not want them to be in the index yet and "git rm" should not add these unrelated local changes to the index. Thanks.
Shourya Shukla <periperidip@gmail.com> writes: > + if (git_config_rename_section_in_file(index_only ? GITMODULES_INDEX : > + GITMODULES_FILE, sect.buf, NULL) < 0) { Also, is it really sufficient to pass GITMODULES_INDEX as the first argument to this function to tweak what is in the index? git_config_copy_or_rename_section_in_file() which is the implementation of that helper seems to always want to work with a file that is on disk, by making unconditional calls to hold_lock_file_for_update(), fopen(), fstat(), chmod(), etc. So I suspect that there are much more work needed. It seems to me that the config editing API is one of the older and hackier parts of the system and requires quite a lot of work to teach it to work with anything but a on-disk file. In the longer term, it may be a good thing to clean it up, but I suspect that it is way too much work for too little benefit to do so as a part of this topic, so an easier way out for now would be to: - write out the .gitmodules in the index to a temporary file (learn how to correctly call entry.c::checkout_entry() by studying how builtin/checkout-index.c::checkout_file() calls it, especially to a temporary file with the --temp option). - use git_config_rename_section_in_file() on that temporary file to remove the section about the submodule. - read that temporary file back into memory and write it out as a blob object by calling sha1-file.c::write_object_file(). - add that back to the index as .gitmodules (studying how builtin/update-index.c::add_cacheinfo() calls add_cache_entry() would be a good way to learn how to do this). The working tree side can stay as is, but as I said in the earlier message, I think you need to update the .gitmodules in the working tree and .gitmodules in the index separately (and without doing any equivalent of "git add .gitmodules").
Hi Junio! Really really sorry for the late reply. I was busy in some personal engagements and was travelling for the past 8-9 days. On 22/02 10:58, Junio C Hamano wrote: > Shourya Shukla <periperidip@gmail.com> writes: > > > Currently, using 'git rm --cached <submodule>' removes the submodule > > <submodule> from the index and leaves the submodule working tree > > intact in the superproject working tree, but does not stage any > > changes to the '.gitmodules' file, in contrast to 'git rm <submodule>', > > which removes both the submodule and its configuration in '.gitmodules' > > from the worktree and index. > > > > Fix this inconsistency by also staging the removal of the entry of the > > submodule from the '.gitmodules' file, leaving the worktree copy intact, > > The "also" above felt a bit puzzling, as we would be removing the > entry only from the indexed copy without touching the working tree > (by the way, I try to be precise in terminology between worktree and > working tree, and please follow suit. A working tree is what you > have in a non-bare repository that let's you "less" "gcc" etc. on > the files checked out. A worktree refers to the mechanism that lets > you have separate working tree by borrowing from a repository, or > refers to an instance of a working tree plus .git file created by > the mechanism. You mean "working tree" in the above sentence), but > it refers to "remove the submodules directory and also entry", so it > is OK. Sure. Will make it more precise and rather technically connect. > And that is all for what is done to a submodule. > > Makes sense so far. > > > + } > > + if (!index_only) { > > if (!remove_path(path)) { > > removed = 1; > > continue; > > @@ -396,9 +399,6 @@ int cmd_rm(int argc, const char **argv, const char *prefix) > > if (!removed) > > die_errno("git rm: '%s'", path); > > } > > - strbuf_release(&buf); > > - if (gitmodules_modified) > > - stage_updated_gitmodules(&the_index); > > OK, because this should have been done where we called > remove_path_from_gitmodules(). > > > } > > > > if (write_locked_index(&the_index, &lock_file, > > diff --git a/submodule.c b/submodule.c > > index 9767ba9893..6ce8c8d0d8 100644 > > --- a/submodule.c > > +++ b/submodule.c > > @@ -131,7 +131,7 @@ int update_path_in_gitmodules(const char *oldpath, const char *newpath) > > * path is configured. Return 0 only if a .gitmodules file was found, a section > > * with the correct path=<path> setting was found and we could remove it. > > */ > > -int remove_path_from_gitmodules(const char *path) > > +int remove_path_from_gitmodules(const char *path, int index_only) > > { > > struct strbuf sect = STRBUF_INIT; > > const struct submodule *submodule; > > @@ -149,7 +149,8 @@ int remove_path_from_gitmodules(const char *path) > > } > > strbuf_addstr(§, "submodule."); > > strbuf_addstr(§, submodule->name); > > - if (git_config_rename_section_in_file(GITMODULES_FILE, sect.buf, NULL) < 0) { > > + if (git_config_rename_section_in_file(index_only ? GITMODULES_INDEX : > > + GITMODULES_FILE, sect.buf, NULL) < 0) { > > /* Maybe the user already did that, don't error out here */ > > warning(_("Could not remove .gitmodules entry for %s"), path); > > strbuf_release(§); > > When !index_only, do we have any guarantee that .gitmodules in the > working tree and .gitmodules in the index are in sync? I somehow > doubt it. > > I would have expected that the updated remove_path_from_gitmodules() > would look more like: > > - only if !index_only, nuke the section for the submodule in > .gitmodules in the working tree. > > - nuke the section for the submodule in .gitmodules in the > index. > > IOW, there would be two git_config_rename_section_in_file() calls to > remove the section in !index_only case. > > Doing so would also mean that you should not have the caller call > stage_updated_gitmodules() at all, even in !index_only case. > Imagine if the .gitmodules file in the working tree had local > changes (e.g. registered a few more submodules, or updated the url > field of a few submodules) that are not yet added to the index when > "git rm" removed a submodule. The user does not want them to be in > the index yet and "git rm" should not add these unrelated local > changes to the index. Won't this be deviating from the current behaviour of 'git rm'? Currently, the above case won't process and the user will be asked to stage or undo the mods they made before moving forward. If I am not mistaken, won't we deviate from the case if we do the above? As of now, I tried this: if (!index_only) { if (git_config_rename_section_in_file(GITMODULES_FILE, sect.buf, NULL) < 0) { /* Maybe the user already did that, don't error out here */ warning(_("Could not remove .gitmodules entry for %s"), path); strbuf_release(§); return -1; } } if (git_config_rename_section_in_file(GITMODULES_INDEX , sect.buf, NULL) < 0) { /* Maybe the user already did that, don't error out here */ warning(_("Could not remove .gitmodules entry for %s"), path); strbuf_release(§); return -1; } Everything else being unchanged. Therefore, we still have the 'stage_updated_gitmodules()' call. If we don't call this function then won't we be NOT adding the updated '.gitmodules' to the staging area, something which is done as of now? Or am I mising something here? Regards, Shourya Shukla
Shourya Shukla <periperidip@gmail.com> writes: >> Doing so would also mean that you should not have the caller call >> stage_updated_gitmodules() at all, even in !index_only case. >> Imagine if the .gitmodules file in the working tree had local >> changes (e.g. registered a few more submodules, or updated the url >> field of a few submodules) that are not yet added to the index when >> "git rm" removed a submodule. The user does not want them to be in >> the index yet and "git rm" should not add these unrelated local >> changes to the index. > > Won't this be deviating from the current behaviour of 'git rm'? > Currently, the above case won't process and the user will be asked to > stage or undo the mods they made before moving forward. Ah, adding such safety to ensure that "rm" without "--cached" (i.e. update both the index and the working tree copies of .gitmodules) would stop when .gitmodules has a local mod would be a good idea, on top of the outline you are responding to, I think. Thanks.
On 22/02 11:29, Junio C Hamano wrote: > Shourya Shukla <periperidip@gmail.com> writes: > > > + if (git_config_rename_section_in_file(index_only ? GITMODULES_INDEX : > > + GITMODULES_FILE, sect.buf, NULL) < 0) { > > Also, is it really sufficient to pass GITMODULES_INDEX as the first > argument to this function to tweak what is in the index? > > git_config_copy_or_rename_section_in_file() which is the > implementation of that helper seems to always want to work with a > file that is on disk, by making unconditional calls to > hold_lock_file_for_update(), fopen(), fstat(), chmod(), etc. > > So I suspect that there are much more work needed. I am not able to comprehend _why_ we need so much more work. To me it seems to work fine. The flow now is something like: 1. If !index_only i.e., '--cached' is not passed then remove the entry of the SM from the working tree copy of '.gitmodules' i.e., GITMODULES_FILE. If there are any unstaged mods in '.gitmodules', we do not proceed with 'git rm'. 2. Now, delete the entry of the above SM from the index copy of the '.gitmodules' i.e., GITMODULES_INDEX (irrespective of the value of 'index_only'). If there are any unstaged mods in '.gitmodules', we do not proceed with 'git rm'. 3. Finally, after the deletion of the SM entry, we stage the changes using 'stage_updated_gitmodules()'. Also, before any of the above thing happens, we check if it is OK to write the '.gitmodules' using 'is_staging_gitmodules_ok()'. All the above behaviour is in line with the current behaviour of 'git rm'. What exactly do we need to change then? > It seems to me that the config editing API is one of the older and > hackier parts of the system and requires quite a lot of work to > teach it to work with anything but a on-disk file. In the longer > term, it may be a good thing to clean it up, but I suspect that it > is way too much work for too little benefit to do so as a part of > this topic, so an easier way out for now would be to: > > - write out the .gitmodules in the index to a temporary file (learn > how to correctly call entry.c::checkout_entry() by studying how > builtin/checkout-index.c::checkout_file() calls it, especially to > a temporary file with the --temp option). > > - use git_config_rename_section_in_file() on that temporary file to > remove the section about the submodule. > > - read that temporary file back into memory and write it out as a > blob object by calling sha1-file.c::write_object_file(). > > - add that back to the index as .gitmodules (studying how > builtin/update-index.c::add_cacheinfo() calls add_cache_entry() > would be a good way to learn how to do this). > > The working tree side can stay as is, but as I said in the earlier > message, I think you need to update the .gitmodules in the working > tree and .gitmodules in the index separately (and without doing any > equivalent of "git add .gitmodules"). But 'git rm' itself used to stage the changes i.e., 'git add'-ing them. Regards, Shourya Shukla
Shourya Shukla <periperidip@gmail.com> writes: > On 22/02 11:29, Junio C Hamano wrote: >> Shourya Shukla <periperidip@gmail.com> writes: >> >> > + if (git_config_rename_section_in_file(index_only ? GITMODULES_INDEX : >> > + GITMODULES_FILE, sect.buf, NULL) < 0) { >> >> Also, is it really sufficient to pass GITMODULES_INDEX as the first >> argument to this function to tweak what is in the index? >> >> git_config_copy_or_rename_section_in_file() which is the >> implementation of that helper seems to always want to work with a >> file that is on disk, by making unconditional calls to >> hold_lock_file_for_update(), fopen(), fstat(), chmod(), etc. >> >> So I suspect that there are much more work needed. > > I am not able to comprehend _why_ we need so much more work. To me it > seems to work fine. > The flow now is something like: > > 1. If !index_only i.e., '--cached' is not passed then remove the entry > of the SM from the working tree copy of '.gitmodules' i.e., > GITMODULES_FILE. If there are any unstaged mods in '.gitmodules', we do > not proceed with 'git rm'. That side is fine, especially if we are extending the "when doing 'git rm PATH' (without '--cached'), PATH must match between the index and the working tree" to "when doing 'git rm SUBMODULE', not just SUBMODULE but also '.gitmodules' must match between the index and the working tree", then adjusting the entry for SUBMODULE in '.gitmodules' in the working tree and adding the result to the index would give the same result as editing '.gitmodules' both in the index and in the working tree independently. But the problem is that there is no way "--cached" case would work with your code. > What exactly do we need to change then? Have you traced what happens when you make this call >> > + if (git_config_rename_section_in_file(index_only ? GITMODULES_INDEX : >> > + GITMODULES_FILE, sect.buf, NULL) < 0) { with index_only set? i.e. GIT_MODULES_INDEX passed as the config_filename argument? The first parameter to the git_config_rename_section_in_file() names a filename in the working tree to be edited. Writing ':.gitmodules' does not make the function magically work in-core without touching the working tree. It will make it update a file (likely not tracked) whose name is ":.gitmodules" in the working tree, no? Presumably you want to edit in-index .gitmodules without touching the working tree file, but the call is not doing that---and it would take much more work to teach it do so. And a cheaper way out would be how I outlined in the message you are responding to, i.e. write out the in-index .gitmodules to a temporary file, let git_config_rename_section_in_file() tweak that temporary file, and add it back into the index.
On 07/03 12:29, Junio C Hamano wrote: > Shourya Shukla <periperidip@gmail.com> writes: > > > On 22/02 11:29, Junio C Hamano wrote: > >> Shourya Shukla <periperidip@gmail.com> writes: > >> > >> > + if (git_config_rename_section_in_file(index_only ? GITMODULES_INDEX : > >> > + GITMODULES_FILE, sect.buf, NULL) < 0) { > >> > >> Also, is it really sufficient to pass GITMODULES_INDEX as the first > >> argument to this function to tweak what is in the index? > >> > >> git_config_copy_or_rename_section_in_file() which is the > >> implementation of that helper seems to always want to work with a > >> file that is on disk, by making unconditional calls to > >> hold_lock_file_for_update(), fopen(), fstat(), chmod(), etc. > >> > >> So I suspect that there are much more work needed. > > > > I am not able to comprehend _why_ we need so much more work. To me it > > seems to work fine. > > > The flow now is something like: > > > > 1. If !index_only i.e., '--cached' is not passed then remove the entry > > of the SM from the working tree copy of '.gitmodules' i.e., > > GITMODULES_FILE. If there are any unstaged mods in '.gitmodules', we do > > not proceed with 'git rm'. > > That side is fine, especially if we are extending the "when doing > 'git rm PATH' (without '--cached'), PATH must match between the > index and the working tree" to "when doing 'git rm SUBMODULE', not > just SUBMODULE but also '.gitmodules' must match between the index > and the working tree", then adjusting the entry for SUBMODULE in > '.gitmodules' in the working tree and adding the result to the index > would give the same result as editing '.gitmodules' both in the > index and in the working tree independently. > > But the problem is that there is no way "--cached" case would work > with your code. > > > What exactly do we need to change then? > > Have you traced what happens when you make this call > > >> > + if (git_config_rename_section_in_file(index_only ? GITMODULES_INDEX : > >> > + GITMODULES_FILE, sect.buf, NULL) < 0) { > > with index_only set? i.e. GIT_MODULES_INDEX passed as the > config_filename argument? > > The first parameter to the git_config_rename_section_in_file() names > a filename in the working tree to be edited. Writing ':.gitmodules' > does not make the function magically work in-core without touching > the working tree. It will make it update a file (likely not > tracked) whose name is ":.gitmodules" in the working tree, no? > > Presumably you want to edit in-index .gitmodules without touching > the working tree file, but the call is not doing that---and it would > take much more work to teach it do so. > > And a cheaper way out would be how I outlined in the message you are > responding to, i.e. write out the in-index .gitmodules to a > temporary file, let git_config_rename_section_in_file() tweak that > temporary file, and add it back into the index. Ahhh. Understood and will work on it. BTW then when does GITMODULES_INDEX even fulfill its purpose? Its name can confuse anyone into thinking what it made me think: it is the index copy of the gitmodules. Is it something which is to be changed in the near future?
Shourya Shukla <periperidip@gmail.com> writes: > Ahhh. Understood and will work on it. BTW then when does > GITMODULES_INDEX even fulfill its purpose? Its name can confuse anyone > into thinking what it made me think: it is the index copy of the > gitmodules. I do not offhand know where in our codebase we use it (I am not a submodule person). Perhaps get_sha1(":.gitmodules", sha1)? Even then, I'd probably prefer to see it spelled as get_sha1(":" GITMODULES_FILE, sha1) with token concatenation. > Is it something which is to be changed in the near future? Sorry, I do not understand the question.
diff --git a/builtin/rm.c b/builtin/rm.c index 4858631e0f..5854ef0996 100644 --- a/builtin/rm.c +++ b/builtin/rm.c @@ -254,7 +254,7 @@ static struct option builtin_rm_options[] = { int cmd_rm(int argc, const char **argv, const char *prefix) { struct lock_file lock_file = LOCK_INIT; - int i; + int i, removed = 0; struct pathspec pathspec; char *seen; @@ -365,30 +365,33 @@ int cmd_rm(int argc, const char **argv, const char *prefix) if (show_only) return 0; - /* - * Then, unless we used "--cached", remove the filenames from - * the workspace. If we fail to remove the first one, we - * abort the "git rm" (but once we've successfully removed - * any file at all, we'll go ahead and commit to it all: - * by then we've already committed ourselves and can't fail - * in the middle) - */ - if (!index_only) { - int removed = 0, gitmodules_modified = 0; - struct strbuf buf = STRBUF_INIT; - for (i = 0; i < list.nr; i++) { - const char *path = list.entry[i].name; - if (list.entry[i].is_submodule) { + for (i = 0; i < list.nr; i++) { + const char *path = list.entry[i].name; + if (list.entry[i].is_submodule) { + /* + * Then, unless we used "--cached", remove the filenames from + * the workspace. If we fail to remove the first one, we + * abort the "git rm" (but once we've successfully removed + * any file at all, we'll go ahead and commit to it all: + * by then we've already committed ourselves and can't fail + * in the middle) + */ + if (!index_only) { + struct strbuf buf = STRBUF_INIT; strbuf_reset(&buf); strbuf_addstr(&buf, path); if (remove_dir_recursively(&buf, 0)) die(_("could not remove '%s'"), path); removed = 1; - if (!remove_path_from_gitmodules(path)) - gitmodules_modified = 1; - continue; + strbuf_release(&buf); } + if (!remove_path_from_gitmodules(path, index_only)) + stage_updated_gitmodules(&the_index); + + continue; + } + if (!index_only) { if (!remove_path(path)) { removed = 1; continue; @@ -396,9 +399,6 @@ int cmd_rm(int argc, const char **argv, const char *prefix) if (!removed) die_errno("git rm: '%s'", path); } - strbuf_release(&buf); - if (gitmodules_modified) - stage_updated_gitmodules(&the_index); } if (write_locked_index(&the_index, &lock_file, diff --git a/submodule.c b/submodule.c index 9767ba9893..6ce8c8d0d8 100644 --- a/submodule.c +++ b/submodule.c @@ -131,7 +131,7 @@ int update_path_in_gitmodules(const char *oldpath, const char *newpath) * path is configured. Return 0 only if a .gitmodules file was found, a section * with the correct path=<path> setting was found and we could remove it. */ -int remove_path_from_gitmodules(const char *path) +int remove_path_from_gitmodules(const char *path, int index_only) { struct strbuf sect = STRBUF_INIT; const struct submodule *submodule; @@ -149,7 +149,8 @@ int remove_path_from_gitmodules(const char *path) } strbuf_addstr(§, "submodule."); strbuf_addstr(§, submodule->name); - if (git_config_rename_section_in_file(GITMODULES_FILE, sect.buf, NULL) < 0) { + if (git_config_rename_section_in_file(index_only ? GITMODULES_INDEX : + GITMODULES_FILE, sect.buf, NULL) < 0) { /* Maybe the user already did that, don't error out here */ warning(_("Could not remove .gitmodules entry for %s"), path); strbuf_release(§); diff --git a/submodule.h b/submodule.h index 4ac6e31cf1..4d8707d911 100644 --- a/submodule.h +++ b/submodule.h @@ -43,7 +43,7 @@ int is_gitmodules_unmerged(const struct index_state *istate); int is_writing_gitmodules_ok(void); int is_staging_gitmodules_ok(struct index_state *istate); int update_path_in_gitmodules(const char *oldpath, const char *newpath); -int remove_path_from_gitmodules(const char *path); +int remove_path_from_gitmodules(const char *path, int index_only); void stage_updated_gitmodules(struct index_state *istate); void set_diffopt_flags_from_submodule_config(struct diff_options *, const char *path); diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh index 7547f11a5c..c0ca4be5a1 100755 --- a/t/t3600-rm.sh +++ b/t/t3600-rm.sh @@ -390,16 +390,14 @@ test_expect_success 'rm of a populated submodule with different HEAD fails unles test_must_fail git config -f .gitmodules submodule.sub.path ' -test_expect_success 'rm --cached leaves work tree of populated submodules and .gitmodules alone' ' +test_expect_success 'rm --cached leaves work tree of populated submodules alone' ' git reset --hard && git submodule update && git rm --cached submod && test_path_is_dir submod && test_path_is_file submod/.git && git status -s -uno >actual && - test_cmp expect.cached actual && - git config -f .gitmodules submodule.sub.url && - git config -f .gitmodules submodule.sub.path + test_cmp expect.cached actual ' test_expect_success 'rm --dry-run does not touch the submodule or .gitmodules' '
Currently, using 'git rm --cached <submodule>' removes the submodule <submodule> from the index and leaves the submodule working tree intact in the superproject working tree, but does not stage any changes to the '.gitmodules' file, in contrast to 'git rm <submodule>', which removes both the submodule and its configuration in '.gitmodules' from the worktree and index. Fix this inconsistency by also staging the removal of the entry of the submodule from the '.gitmodules' file, leaving the worktree copy intact, a behaviour which is more in line with what might be expected when using '--cached'. Achieve this by modifying the function 'remove_path_from_gitmodules()' to also take in the parameter 'index_only' denoting the presence of the '--cached' option. If present, remove the submodule entry from the copy of the '.gitmodules' in the index otherwise, do the same for the working tree copy. While at it, also change the test 46 of the test script 't3600-rm.sh' to incorporate for the above changes. Reported-by: Javier Mora <javier.moradesambricio@rtx.com> Helped-by: Phillipe Blain <levraiphilippeblain@gmail.com> Helped-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Shourya Shukla <periperidip@gmail.com> --- builtin/rm.c | 42 +++++++++++++++++++++--------------------- submodule.c | 5 +++-- submodule.h | 2 +- t/t3600-rm.sh | 6 ++---- 4 files changed, 27 insertions(+), 28 deletions(-)