Bug in git citool when staging symlink as replacement for submodule

Bug in git citool when staging symlink as replacement for submodule

[Adam Spiers]

I found some strange behaviour in git citool when trying to stage a
symlink into the index as a replacement for a (non-registered) submodule.
I just reproduced with latest master (1.7.11.rc0.55.gb2478aa):

    mkdir repo
    cd repo
    git init
    echo foo > one
    git add one
    git commit -m'first commit'

    mkdir two
    cd two
    git init
    echo foo > submodule-file
    git add submodule-file
    git commit -m'first submodule commit'

    cd ..

    git add two
    git commit -m'second commit'

    rm -rf two
    ln -s one two

    git citool

At this point, git citool outputs:

    error: Unhandled submodule diff marker: {@@ }
    error: Unhandled submodule diff marker: {+on}

Now if I press Control-T to try to stage 'two' into the index, I get a new
dialog which says:

    Updating the Git index failed. A rescan will be automatically
    started to resynchronize git-gui.

    error: two is already a gitlink, not replacing
    fatal: Unable to process path two


                              [Unlock Index] [Continue]

I can work around via 'git add two', but it would be nice if citool
handled this correctly.

Thanks!
Adam

[PATCH] git-gui: recognize submodule diff when replaced by file

[Heiko Voigt]

When coloring the diff in submodule mode we previously just looked
for the submodule change markers and not normal diff markers. Since
a submodule can be replaced by a file and vice versa lets also support
hunk, + and - lines.

Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
Hi,

On Fri, Jun 01, 2012 at 11:31:26AM +0100, Adam Spiers wrote:
> At this point, git citool outputs:
> 
>     error: Unhandled submodule diff marker: {@@ }
>     error: Unhandled submodule diff marker: {+on}

This fixes that error message (and is hopefully doing the right thing).

There will be another patch fixing the error popup.

Cheers Heiko

 git-gui/lib/diff.tcl | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/git-gui/lib/diff.tcl b/git-gui/lib/diff.tcl
index ec44055..a3c997b 100644
--- a/git-gui/lib/diff.tcl
+++ b/git-gui/lib/diff.tcl
@@ -467,8 +467,16 @@ proc read_diff {fd conflict_size cont_info} {
 				{  >} {set tags d_+}
 				{  W} {set tags {}}
 				default {
-					puts "error: Unhandled submodule diff marker: {$op}"
-					set tags {}
+					set op [string index $line 0]
+					switch -- $op {
+						{-} {set tags d_-}
+						{+} {set tags d_+}
+						{@} {set tags d_@}
+						default {
+							puts "error: Unhandled submodule diff marker: {$op}"
+							set tags {}
+						}
+					}
 				}
 				}
 			}
-- 
1.7.10.2.522.g93b45fe

[PATCH] update-index: allow overwriting existing submodule index entries

[Heiko Voigt]

In commit e01105 Linus introduced gitlinks to update-index. He explains
that he thinks it is not the right thing to replace a gitlink with
something else.

That commit is from the very first beginnings of submodule support.
Since then we have gotten a lot closer to being able to remove a
submodule without losing its history. This check prevents such a use
case, so I think this assumption has changed.

Additionally in the git add codepath we do not have such a check, so for
consistency reasons I think removing this check is the correct thing to
do.

Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
Hi,

On Fri, Jun 01, 2012 at 11:31:26AM +0100, Adam Spiers wrote:
> Now if I press Control-T to try to stage 'two' into the index, I get a new
> dialog which says:
> 
>     Updating the Git index failed. A rescan will be automatically
>     started to resynchronize git-gui.
> 
>     error: two is already a gitlink, not replacing
>     fatal: Unable to process path two
> 
> 
>                               [Unlock Index] [Continue]
> 
> I can work around via 'git add two', but it would be nice if citool
> handled this correctly.

This is because git gui uses plumbing (update-index) for updating the
index.

This patch fixes that popup. Testsuite passes here.

Is it ok this way?

Cheers Heiko

 builtin/update-index.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/builtin/update-index.c b/builtin/update-index.c
index 5f038d6..5a4e9ea 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -211,12 +211,6 @@ static int process_path(const char *path)
 	if (S_ISDIR(st.st_mode))
 		return process_directory(path, len, &st);
 
-	/*
-	 * Process a regular file
-	 */
-	if (ce && S_ISGITLINK(ce->ce_mode))
-		return error("%s is already a gitlink, not replacing", path);
-
 	return add_one_path(ce, path, len, &st);
 }
 
-- 
1.7.10.2.522.g93b45fe

Re: [PATCH] update-index: allow overwriting existing submodule index entries

[Junio C Hamano]

Heiko Voigt <hvoigt@hvoigt.net> writes:

> In commit e01105 Linus introduced gitlinks to update-index. He explains
> that he thinks it is not the right thing to replace a gitlink with
> something else.
>
> That commit is from the very first beginnings of submodule support.
> Since then we have gotten a lot closer to being able to remove a
> submodule without losing its history. This check prevents such a use
> case, so I think this assumption has changed.

Yeah, I think we should remove it if only to make it consistent with
"add" (if anything, the Porcelain level "add" should be the one that
is more strict and the plumbing level should be able to let you
shoot in the foot, not the other way around), but we need to make
sure "closer to" becomes reality.  Can we remove and the resurrect
automatically when checking out a branch with a submodule when you
are on a branch with a directory and vice versa?

Re: [PATCH] update-index: allow overwriting existing submodule index entries

[Jens Lehmann]

Am 11.06.2012 17:03, schrieb Junio C Hamano:
> Heiko Voigt <hvoigt@hvoigt.net> writes:
> 
>> In commit e01105 Linus introduced gitlinks to update-index. He explains
>> that he thinks it is not the right thing to replace a gitlink with
>> something else.
>>
>> That commit is from the very first beginnings of submodule support.
>> Since then we have gotten a lot closer to being able to remove a
>> submodule without losing its history. This check prevents such a use
>> case, so I think this assumption has changed.
> 
> Yeah, I think we should remove it if only to make it consistent with
> "add" (if anything, the Porcelain level "add" should be the one that
> is more strict and the plumbing level should be able to let you
> shoot in the foot, not the other way around), but we need to make
> sure "closer to" becomes reality. Can we remove and the resurrect
> automatically when checking out a branch with a submodule when you
> are on a branch with a directory and vice versa?

Even while I suspect most of the time a submodule <=> directory
transition will occur (moving directory content into a submodule
or vice versa; and then there will be no replacement of a gitlink
with something else as only the files inside the directory will be
recorded) there is no reason why submodule <=> file or submodule
<=> link transitions shouldn't work just fine. So yes, we can ;-)
(See the recursive_submodule_checkout branch in my GitHub repo for
current state of affairs, even though not all transitions work yet
some do just fine)

If you want I can keep this patch in my GitHub repo until "closer
to" becomes reality. Me too thinks that plumbing should not enforce
this restriction, but I wouldn't mind to hold this patch back until
then.

Re: [PATCH] update-index: allow overwriting existing submodule index entries

[Heiko Voigt]

Hi,

On Mon, Jun 11, 2012 at 11:23:15PM +0200, Jens Lehmann wrote:
> Am 11.06.2012 17:03, schrieb Junio C Hamano:
> > Heiko Voigt <hvoigt@hvoigt.net> writes:
> > 
> >> In commit e01105 Linus introduced gitlinks to update-index. He explains
> >> that he thinks it is not the right thing to replace a gitlink with
> >> something else.
> >>
> >> That commit is from the very first beginnings of submodule support.
> >> Since then we have gotten a lot closer to being able to remove a
> >> submodule without losing its history. This check prevents such a use
> >> case, so I think this assumption has changed.
> > 
> > Yeah, I think we should remove it if only to make it consistent with
> > "add" (if anything, the Porcelain level "add" should be the one that
> > is more strict and the plumbing level should be able to let you
> > shoot in the foot, not the other way around), but we need to make
> > sure "closer to" becomes reality. Can we remove and the resurrect
> > automatically when checking out a branch with a submodule when you
> > are on a branch with a directory and vice versa?
> 
> Even while I suspect most of the time a submodule <=> directory
> transition will occur (moving directory content into a submodule
> or vice versa; and then there will be no replacement of a gitlink
> with something else as only the files inside the directory will be
> recorded) there is no reason why submodule <=> file or submodule
> <=> link transitions shouldn't work just fine. So yes, we can ;-)
> (See the recursive_submodule_checkout branch in my GitHub repo for
> current state of affairs, even though not all transitions work yet
> some do just fine)

This is what works currently with submodule update in master:

file => submodule
symlink => submodule
directory => submodule
	These are the simplest cases. It currently works fine with
	submodule update. The checkout will remove the files of the
	directory and submodule update will just populate the submodule
	if it needs to.


submodule => file
submodule => symlink
submodule => directory
	These ones are the trickier ones.
	A checkout would currently not touch the submodule. It will be
	left in place. I have just tried it and in case the same files
	exist checkout will stop with an error. If there are no
	overlapping files it will happily checkout the files.  Now
	status displays the files contained in the submodule as
	untracked.

	This behavior could use some improvement. *)

	So if we were to implement submodule removal with submodule
	update the first problem here is when transitioning from a
	submodule to files we first should skip checking out files from
	that path.

	Then when using submodule update we need to detect whether a
	paths situation was brought to you by a checkout. Remember if
	the files are skipped by checkout the submodule is still in place.
	Comparing the working copy with whats in the database looks
	as if the user has replaced the directory/file with a submodule.

	To cleanup the situation with submodule update we could just do
	some security checks**) and in case they are successful remove
	the submodule and checkout the files.

	One real problem I see here is when displaying status if some
	files checkout has been skipped. Now you will potentially
	(directory case) see a lot of files looking as if they were
	deleted.

*) I also found that replacing a submodule with a directory using git
add does not work directly. The directory and it files will simply be
ignored by add.

**) Check that the .git is a gitfile that points outside of the
submodules directory. Check that there are no untracked changes in the
submodule.

The current plan to solve the submodule => file(s) transition is to
extend checkout with a --recurse-submodules option and then let it do
the above transition during checkout.

But unless we are going to have recursive checkout for populated
submodules always on (no config) we need to be able to deal with the
non-recursive outdated submodules situation.

Here a quick idea of what we could do:

We could mark a path as transitioned from submodule (similar to the
assume unchanged bit) if files were skipped due to removal of a
submodule and have submodule update clear that flag. That way we could
teach diff, status and so on to only show one entry for a submodule to
be removed and replaced with something else.

Thinking further: We could actually prevent adding such an out-of-date
submodule as a safeguard. Which in fact was something which happened by
mistake to some of our users. The story is that when you see a *changed*
submodule in a merge conflict it can be easily mistaken for needing
resolution and added to the merge. Most times this rewinds your
submodule to some old state

If we agree on how to handle the submodule => file(s) cases I think the
implementation in the submodule script possibly requires less work than
the fully fledged recursive checkout and could also be used for gaining
some experience with such transitions.

So the first step would be to extend submodule update to support the
removal of submodules. The next step is then that we finish the fully
automatic recursive submodule checkout.

What do you think of such a two step plan?

Cheers Heiko

Re: [PATCH] update-index: allow overwriting existing submodule index entries

[Jens Lehmann]

Am 12.06.2012 22:33, schrieb Heiko Voigt:
> Here a quick idea of what we could do:
> 
> We could mark a path as transitioned from submodule (similar to the
> assume unchanged bit) if files were skipped due to removal of a
> submodule and have submodule update clear that flag. That way we could
> teach diff, status and so on to only show one entry for a submodule to
> be removed and replaced with something else.
> 
> Thinking further: We could actually prevent adding such an out-of-date
> submodule as a safeguard. Which in fact was something which happened by
> mistake to some of our users. The story is that when you see a *changed*
> submodule in a merge conflict it can be easily mistaken for needing
> resolution and added to the merge. Most times this rewinds your
> submodule to some old state
> 
> If we agree on how to handle the submodule => file(s) cases I think the
> implementation in the submodule script possibly requires less work than
> the fully fledged recursive checkout and could also be used for gaining
> some experience with such transitions.
> 
> So the first step would be to extend submodule update to support the
> removal of submodules. The next step is then that we finish the fully
> automatic recursive submodule checkout.
> 
> What do you think of such a two step plan?

Hmm, I suspect the real problem here is that "git submodule update" is
run *after* the actual checkout, merge, reset or whatever. So if for
example you want to replace a submodule with a plain directory containing
the same files the submodule update would not only have to remove the now
obsolete submodule but also has to remember to check out all files in the
former submodule directory again. IMO this should be part of the initial
unpack_trees(), not done in a later script. Imagine a submodule having
local modifications which would be overwritten by the checkout, then the
later submodule update would have to fail (when used without -f) to
protect the users changes. The only sane thing to do would be to not allow
such a checkout in the first place (when the user chose to automatically
update submodules that is) but abort just like we do for changes to
regular files right now telling the user to save his changes.

And I suspect you would have to teach at least status and diff to give
meaningful output in that "submodule should be removed and replaced with
something else but submodule update hasn't run yet" case, and apart from
the change to add you mentioned above some other commands might need
safeguards too.

So me thinks we should skip step one and implement recursive checkout
right away. Adding much more complexity to the submodule script for an
intermediate solution doesn't sound like a good idea to me.