I finally got around to looking at Alex' patch to teach update-index about 
gitlinks too, so that "git commit -a" along with any other explicit 
update-index scripts can work.

I don't think there was anything wrong with Alex' patch, but the code he 
patched I felt was just so ugly that the added cases just pushed it over 
the edge. Especially as I don't think that patch necessarily did the right 
thing for a gitlink entry that already existed in the index, but that 
wasn't actually a real git repository in the working tree (just an empty 
subdirectory or a non-git snapshot because it hadn't wanted to track that 
particular subproject).

So I ended up deciding to clean up the git-update-index handling the same 
way I tackled the directory traversal used by git-add earlier: by 
splitting the different cases up into multiple smaller functions, and just 
making the code easier to read (and adding more comments about the 
different cases).

So this replaces the old "process_file()" with a new "process_path()" 
function that then just calls out to different helper functions depending 
on what kind of path it is. Processing a nondirectory ends up being just 
one of the simpler cases.

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---

Alex, I'd appreciate it if you gave this a look-over. I think the logic is 
fairly well-documented and the flow is fairly obvious (the patch is not 
quite as easy to read as the end result, but you can get the gist of it 
from the patch too if you're as used to unified diffs as I am..)

Junio - if you prefer Alex' patch instead, I won't be upset. This is 
definitely a bigger re-org, and while I think it makes sense as a patch 
even *aside* from the gitlink support, it's probably largely a matter of 
taste.

 builtin-update-index.c |  200 +++++++++++++++++++++++++++++++++---------------
 1 files changed, 138 insertions(+), 62 deletions(-)

diff --git a/builtin-update-index.c b/builtin-update-index.c
index 89aa0b0..022d5cc ...
[ message continues ]

I find the result of applying this patch much easier to read

I may be missing the obvious but if I have a subproject at
"path/S" and I say "update-index path/S/Makefile", what should
happen?  There is ISDIRLNK entry at path/S and add_one_path()
would allow removal of "path/S" to make room for
path/S/Makefile, when --replace is given.


-

[ message continues ]

Good catch. I think we should quite possibly make that be a check in 
"add_cache_entry()", along with the check_file_directory_conflict() stuff.

I don't think anything protects against it as-is.

Of course, we could do it inside builtin-update-index, but we've generally 
had the rule that "add_cache_entry()" is the thing that *enforces* any 
index consistency rules, and the callers are just doing their own sanity 
checks.

			Linus
-

[ message continues ]

return error("%s: cannot add to the index%s", path, "\0 - missing
--add option?" + !allow_add);

Makes me uncomfortable too. Is it be possible anyone is depending
on it? Maybe it still can be changed?
-

[ message continues ]

Maybe not. It actually makes sense: the old file (now a directory)
is actually removed, so a plain --remove in its old meaning still
_is_ correct: remove the file from index if it is removed from
working directory.
And it breaks t1001.
-

[ message continues ]