Re: [PATCH 5/7] git mv: Support moving submodules

Petr Baudis <pasky@suse.cz> writes:

I do not think use of path_list->util is particularly bad. It is a data structure to store a bag of random pointers that can be indexed with string keys, which is exactly what you are doing here.

This one certainly is ugly beyond words.

By the way, why is it even necessary to rehash the contents when you run "mv"?

IOW,

	$ >foo
        $ git add foo
        $ echo 1 >foo
        $ git mv foo bar

makes "foo" disappear from the index and adds "bar", but it makes the local change added to the index at the same time.

	Side note. It actually is a lot worse than that.  When you move
	something to another existing entry, the code does not even add
	the object name of moved entry recorded in the index, nor rehashes
	the moved file.  This is totally inconsistent!

I personally think these buggy behaviours you are trying to inherit are making this patch more complex than necessary. Perhaps this needs to be fixed up even further (you did some fix with the first patch) before adding new features? For example, I think it is a bug that the "overwrite" codepath does not work with symlinks.

But let's assume that we do not want to "fix" any of these existing (mis)behaviour, because doing so would change the semantics.

There are a few cases:

 (1) gitlink exists and so is directory but no repository (i.e. the user is
     not interested);

 (2) gitlink exists and there is a repository.  Its HEAD matches what
     gitlink records;

 (3) gitlink exists and there is a repository.  Its HEAD does not match what
     gitlink records;

The (mis)behaviour that automatically adds moved files to the index we saw earlier suggests that in case (3) you would want to update the gitlink in the index with whatever HEAD the submodule repository has to be consistent.

So instead of adding a index_path_src_sha1 hack like that, how about

 * When you see ce_is_gitlink(j), you keep the original gitlink object
   name.  That is very good in order to preserve it for not just (1) but
   also for (3) once we decide to fix this "auto adding" misbehaviour in
   the later round.  But you do not have to do this for case (2) if you
   are going to rehash anyway, don't you?

   So when you see ce_is_gitlink(j), you check if it has repository and
   HEAD, and record the path_list->util only when it is case (1).

 * Then, only for case (1), you do not call add_file_to_cache() -- because
   you know you do not have anything you can rehash; instead, factor out
   the codepath "git-update-index --cacheinfo" uses and call that.