Re: [PATCH v5 4/5] setup: Add 'abspath_part_inside_repo' function

Martin Erik Werner [off-list ref] writes:

The path being exactly equal to the work tree is handled separately,
since then there is no directory separator between the work tree and
in-repo part.

What is an "in-repo part"?  Whatever it is, I am not sure if I
follow that logic.  After the while (*path) loop checks each level,
you check the whole path---wouldn't that code handle that special
case already?

Because it is probably the normal case not to have any symbolic link
in the leading part (hence not having to dereference them), I think
checking "is work_tree[] a prefix of path[]" early is justified from
performance point of view, though.

 /*
+ * No checking if the path is in fact an absolute path is done, and it must
+ * already be normalized.

This is not quite parsable to me.

+ * Find the part of an absolute path that lies inside the work tree by
+ * dereferencing symlinks outside the work tree, for example:
+ * /dir1/repo/dir2/file   (work tree is /dir1/repo)      -> dir2/file
+ * /dir/file              (work tree is /)               -> dir/file
+ * /dir/symlink1/symlink2 (symlink1 points to work tree) -> symlink2
+ * /dir/repolink/file     (repolink points to /dir/repo) -> file
+ * /dir/repo              (exactly equal to work tree)   -> (empty string)
+ */
+static inline int abspath_part_inside_repo(char *path)

It looks a bit too big to be marked "inline"; better leave it to the
compiler to notice that there is only a single callsite and decide
to (or not to) inline it.

+{
+	size_t len;
+	size_t wtlen;
+	char *path0;
+	int off;
+
+	const char* work_tree = get_git_work_tree();
+	if (!work_tree)
+		return -1;
+	wtlen = strlen(work_tree);
+	len = strlen(path);

I expect that this is called from a callsite that _knows_ how long
path[] is.  Shouldn't len be a parameter to this function instead?

+	off = 0;
+
+	/* check if work tree is already the prefix */
+	if (strncmp(path, work_tree, wtlen) == 0) {

I wonder if we want to be explicit and compare wtlen and len before
even attempting strncmp().  If work_tree is longer than path, it
cannot be a prefix of path, right?

In other words, also with a style-fix to prefer !XXcmp() over
XXcmp() == 0:

	if (wtlen <= len && !strncmp(path, work_tree, wtlen)) {

perhaps?  That would make it clear why referring to path[wtlen] on
the next line is permissible (it is obvious that it won't access
past the end of path[]):

+		if (path[wtlen] == '/') {
+			memmove(path, path + wtlen + 1, len - wtlen);
+			return 0;
+		} else if (path[wtlen - 1] == '/' || path[wtlen] == '\0') {
+			/* work tree is the root, or the whole path */
+			memmove(path, path + wtlen, len - wtlen + 1);

If work_tree[] == "/", path[] == "/a", then len == 2, wtlen == 1,
path[wtlen - 1] == '/' and this shifts path[] to the left by one,
leaving path[] = "a", which is what we want.  OK.

If work_tree[] == path[] == "/a", then len == wtlen == 2,
path[wtlen] == '\0', and this becomes equivalent to path[0] = '\0'.
Hmph....  How do our callers treat an empty path?  In other words,
should these three be equivalent?

	cd /a && git ls-files /a
	cd /a && git ls-files ""
	cd /a && git ls-files .

+			return 0;
+		}
+		/* work tree might match beginning of a symlink to work tree */
+		off = wtlen;
+	}
+	path0 = path;
+	path += offset_1st_component(path) + off;
+
+	/* check each level */
+	while (*path) {
+		path++;
+		if (*path == '/') {
+			*path = '\0';
+			if (strcmp(real_path(path0), work_tree) == 0) {
+				memmove(path0, path + 1, len - (path - path0));
+				return 0;
+			}
+			*path = '/';
+		}
+	}
+
+	/* check whole path */
+	if (strcmp(real_path(path0), work_tree) == 0) {
+		*path0 = '\0';
+		return 0;
+	}
+
+	return -1;
+}
+
+/*
  * Normalize "path", prepending the "prefix" for relative paths. If
  * remaining_prefix is not NULL, return the actual prefix still
  * remains in the path. For example, prefix = sub1/sub2/ and path is