[PATCH] path: refactor normalize_path_copy_len()

[PATCH] path: refactor normalize_path_copy_len()

[Pushkar Singh]

Refactor normalize_path_copy_len() by extracting helpers for skipping
slashes, handling dot components, and stripping the previous path
component, making the control flow easier to follow.

This is a mechanical refactor only; there are no functional changes.
Behavior is unchanged, as verified by t0060-path-utils.sh.

Signed-off-by: Pushkar Singh <pushkarkumarsingh1970@gmail.com>
---
 path.c | 105 ++++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 67 insertions(+), 38 deletions(-)

diff --git a/path.c b/path.c
index d726537622..00845cc03f 100644
--- a/path.c
+++ b/path.c
@@ -1112,6 +1112,63 @@ const char *remove_leading_path(const char *in, const char *prefix)
  * end with a '/', then the callers need to be fixed up accordingly.
  *
  */
+
+static const char *skip_slashes(const char *p)
+{
+	while (is_dir_sep(*p))
+		p++;
+	return p;
+}
+
+static int handle_dot_component(const char **src)
+{
+	const char *s = *src;
+
+	if (*s != '.')
+		return 0;
+
+	if (!s[1]) {
+		*src = s + 1;
+		return 1;
+	}
+
+	if (is_dir_sep(s[1])) {
+		*src = skip_slashes(s + 2);
+		return 1;
+	}
+
+	if (s[1] == '.') {
+		if (!s[2]) {
+			*src = s + 2;
+			return 2;
+		}
+		if (is_dir_sep(s[2])) {
+			*src = skip_slashes(s + 3);
+			return 2;
+		}
+	}
+
+	return 0;
+}
+
+static int strip_last_component(char **dst, char *dst0, int *prefix_len)
+{
+	char *d = *dst;
+
+	d--;
+	if (d <= dst0)
+		return -1;
+
+	while (dst0 < d && d[-1] != '/')
+		d--;
+
+	if (prefix_len && *prefix_len > d - dst0)
+		*prefix_len = d - dst0;
+
+	*dst = d;
+	return 0;
+}
+
 int normalize_path_copy_len(char *dst, const char *src, int *prefix_len)
 {
 	char *dst0;
@@ -1129,8 +1186,7 @@ int normalize_path_copy_len(char *dst, const char *src, int *prefix_len)
 	}
 	dst0 = dst;
 
-	while (is_dir_sep(*src))
-		src++;
+	src = skip_slashes(src);
 
 	for (;;) {
 		char c = *src;
@@ -1143,29 +1199,14 @@ int normalize_path_copy_len(char *dst, const char *src, int *prefix_len)
 		 * (3) ".." and ends  -- strip one and terminate.
 		 * (4) "../"          -- strip one, eat slash and continue.
 		 */
-		if (c == '.') {
-			if (!src[1]) {
-				/* (1) */
-				src++;
-			} else if (is_dir_sep(src[1])) {
-				/* (2) */
-				src += 2;
-				while (is_dir_sep(*src))
-					src++;
-				continue;
-			} else if (src[1] == '.') {
-				if (!src[2]) {
-					/* (3) */
-					src += 2;
-					goto up_one;
-				} else if (is_dir_sep(src[2])) {
-					/* (4) */
-					src += 3;
-					while (is_dir_sep(*src))
-						src++;
-					goto up_one;
-				}
-			}
+		int dot = handle_dot_component(&src);
+
+		if (dot == 1)
+			continue;
+		if (dot == 2) {
+			if (strip_last_component(&dst, dst0, prefix_len))
+				return -1;
+			continue;
 		}
 
 		/* copy up to the next '/', and eat all '/' */
@@ -1180,20 +1221,8 @@ int normalize_path_copy_len(char *dst, const char *src, int *prefix_len)
 			break;
 		continue;
 
-	up_one:
-		/*
-		 * dst0..dst is prefix portion, and dst[-1] is '/';
-		 * go up one level.
-		 */
-		dst--;	/* go to trailing '/' */
-		if (dst <= dst0)
-			return -1;
-		/* Windows: dst[-1] cannot be backslash anymore */
-		while (dst0 < dst && dst[-1] != '/')
-			dst--;
-		if (prefix_len && *prefix_len > dst - dst0)
-			*prefix_len = dst - dst0;
 	}
+
 	*dst = '\0';
 	return 0;
 }
-- 
2.43.0

Re: [PATCH] path: refactor normalize_path_copy_len()

[Junio C Hamano]

Pushkar Singh <pushkarkumarsingh1970@gmail.com> writes:

> Refactor normalize_path_copy_len() by extracting helpers for skipping
> slashes, handling dot components, and stripping the previous path
> component, making the control flow easier to follow.

The new helper for skip_slashes() may be a clear win as it extracts
away verbosity from 3 places.

Moving the logic to the "handle_dot_component()" helper, however,
dissociates the actual code from the explanation on the 4 special
cases in the comment, and at least to me, made it a lot harder to
understand what is being done and why.  Also the "goto up_one" logic
was easier to follow in the original than with the magic return
values given by the new helper.  Quite honestly, use of that helper
function looked like worsening the readablity of the logic.

Giving a descriptive name to what is done at the up_one label by
using a single-shot helper function strip_last_component() may be an
improvement, but I do not think it is a clear win.  Adding a
single-liner /* strip the last component */ comment without moving
the code may have made the result even easier to follow without
disrupting the flow with an extra helper function.

 path.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git i/path.c w/path.c
index d726537622..53a87ab67a 100644
--- i/path.c
+++ w/path.c
@@ -1182,6 +1182,8 @@ int normalize_path_copy_len(char *dst, const char *src, int *prefix_len)
 
 	up_one:
 		/*
+		 * strip the last component
+		 *
 		 * dst0..dst is prefix portion, and dst[-1] is '/';
 		 * go up one level.
 		 */

[PATCH v2] path: factor out skip_slashes() in normalize_path_copy_len()

[Pushkar Singh]

Hi Junio,

Thanks for the detailed feedback.

This version keeps skip_slashes(), but drops handle_dot_component() and
restores the original control flow around the four dot-component cases.
The up_one logic is kept inline, with a short explanatory comment as
suggested.

Changes since v1:
  - Keep skip_slashes() helper.
  - Restore inline dot-component handling.
  - Remove handle_dot_component() helper.
  - Keep up_one logic inline and add a brief comment.

Thanks for the review.

Pushkar
---
 path.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/path.c b/path.c
index d726537622..1772fcb21c 100644
--- a/path.c
+++ b/path.c
@@ -1112,6 +1112,14 @@ const char *remove_leading_path(const char *in, const char *prefix)
  * end with a '/', then the callers need to be fixed up accordingly.
  *
  */
+
+static const char *skip_slashes(const char *p)
+{
+	while (is_dir_sep(*p))
+		p++;
+	return p;
+}
+
 int normalize_path_copy_len(char *dst, const char *src, int *prefix_len)
 {
 	char *dst0;
@@ -1129,8 +1137,7 @@ int normalize_path_copy_len(char *dst, const char *src, int *prefix_len)
 	}
 	dst0 = dst;
 
-	while (is_dir_sep(*src))
-		src++;
+	src = skip_slashes(src);
 
 	for (;;) {
 		char c = *src;
@@ -1150,8 +1157,7 @@ int normalize_path_copy_len(char *dst, const char *src, int *prefix_len)
 			} else if (is_dir_sep(src[1])) {
 				/* (2) */
 				src += 2;
-				while (is_dir_sep(*src))
-					src++;
+				src = skip_slashes(src);
 				continue;
 			} else if (src[1] == '.') {
 				if (!src[2]) {
@@ -1161,8 +1167,7 @@ int normalize_path_copy_len(char *dst, const char *src, int *prefix_len)
 				} else if (is_dir_sep(src[2])) {
 					/* (4) */
 					src += 3;
-					while (is_dir_sep(*src))
-						src++;
+					src = skip_slashes(src);
 					goto up_one;
 				}
 			}
@@ -1182,6 +1187,8 @@ int normalize_path_copy_len(char *dst, const char *src, int *prefix_len)
 
 	up_one:
 		/*
+		 * strip the last component
+		 *
 		 * dst0..dst is prefix portion, and dst[-1] is '/';
 		 * go up one level.
 		 */
-- 
2.43.0

Re: [PATCH v2] path: factor out skip_slashes() in normalize_path_copy_len()

[Pushkar Singh]

Hi Junio,

Just checking back on the v2 in case it got missed.
Let me know if you’d like me to tweak anything.

Thanks,
Pushkar

Re: [PATCH v2] path: factor out skip_slashes() in normalize_path_copy_len()

[Junio C Hamano]

Pushkar Singh <pushkarkumarsingh1970@gmail.com> writes:

> Hi Junio,
>
> Just checking back on the v2 in case it got missed.
> Let me know if you’d like me to tweak anything.
>
> Thanks,
> Pushkar

Sorry, I saw it, I didn't think it was meant for application (it
didn't have a proper log message like v1 used to describve its
changes, which I expected to be updated to match the smaller scope
of what v2 made---all it had was something akin to a cover letter)
and did not comment on it when I saw it, and then completely forgot
about it.

[PATCH v3] path: factor out skip_slashes() in normalize_path_copy_len()

[Pushkar Singh]

Extract skip_slashes() to avoid repeating the same is_dir_sep()
loop in multiple places inside normalize_path_copy_len().

Keep the dot-component handling inline to preserve the original
control flow and readability, as suggested in review.

No functional changes. Behavior verified with t0060-path-utils.sh.

Signed-off-by: Pushkar Singh <pushkarkumarsingh1970@gmail.com>
---
Changes since v2:
- Clarify commit message to reflect reduced scope.
- Make intent explicit and ready for application.

 path.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/path.c b/path.c
index d726537622..1772fcb21c 100644
--- a/path.c
+++ b/path.c
@@ -1112,6 +1112,14 @@ const char *remove_leading_path(const char *in, const char *prefix)
  * end with a '/', then the callers need to be fixed up accordingly.
  *
  */
+
+static const char *skip_slashes(const char *p)
+{
+	while (is_dir_sep(*p))
+		p++;
+	return p;
+}
+
 int normalize_path_copy_len(char *dst, const char *src, int *prefix_len)
 {
 	char *dst0;
@@ -1129,8 +1137,7 @@ int normalize_path_copy_len(char *dst, const char *src, int *prefix_len)
 	}
 	dst0 = dst;
 
-	while (is_dir_sep(*src))
-		src++;
+	src = skip_slashes(src);
 
 	for (;;) {
 		char c = *src;
@@ -1150,8 +1157,7 @@ int normalize_path_copy_len(char *dst, const char *src, int *prefix_len)
 			} else if (is_dir_sep(src[1])) {
 				/* (2) */
 				src += 2;
-				while (is_dir_sep(*src))
-					src++;
+				src = skip_slashes(src);
 				continue;
 			} else if (src[1] == '.') {
 				if (!src[2]) {
@@ -1161,8 +1167,7 @@ int normalize_path_copy_len(char *dst, const char *src, int *prefix_len)
 				} else if (is_dir_sep(src[2])) {
 					/* (4) */
 					src += 3;
-					while (is_dir_sep(*src))
-						src++;
+					src = skip_slashes(src);
 					goto up_one;
 				}
 			}
@@ -1182,6 +1187,8 @@ int normalize_path_copy_len(char *dst, const char *src, int *prefix_len)
 
 	up_one:
 		/*
+		 * strip the last component
+		 *
 		 * dst0..dst is prefix portion, and dst[-1] is '/';
 		 * go up one level.
 		 */
-- 
2.43.0