[PATCH 0/3] path: clean up few things

[PATCH 0/3] path: clean up few things

[K Jayatheerth]

While reviewing path.c in preparation for the upcoming git repo info path expansions,
I noticed a few areas of accumulated technical debt.

This series cleans up the file by removing an unused header, enforcing proper
size_t typing for path lengths, and eliminating redundant settings evaluations
to keep the underlying path API clean.

K Jayatheerth (3):
  path: remove unused header
  path: use the right datatype
  path: remove redundant function calls

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

-- 
2.53.0

[PATCH 1/3] path: remove unused header

[K Jayatheerth]

The "environment.h" header is included in "path.c", but none of the
functions or macros it provides are used in this file.

Signed-off-by: K Jayatheerth <jayatheerthkulkarni2005@gmail.com>
---
 path.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/path.c b/path.c
index d726537622..f613d8bbd1 100644
--- a/path.c
+++ b/path.c
@@ -4,7 +4,6 @@
 
 #include "git-compat-util.h"
 #include "abspath.h"
-#include "environment.h"
 #include "gettext.h"
 #include "repository.h"
 #include "strbuf.h"
-- 
2.53.0

[PATCH 2/3] path: use the right datatype

[K Jayatheerth]

The strlen() function returns a size_t
Storing this in a standard signed int is a bad practice
that invites overflow vulnerabilities if paths get absurdly long.

Signed-off-by: K Jayatheerth <jayatheerthkulkarni2005@gmail.com>
---
 path.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/path.c b/path.c
index f613d8bbd1..56be5e1726 100644
--- a/path.c
+++ b/path.c
@@ -58,7 +58,7 @@ static void strbuf_cleanup_path(struct strbuf *sb)
 
 static int dir_prefix(const char *buf, const char *dir)
 {
-	int len = strlen(dir);
+	size_t len = strlen(dir);
 	return !strncmp(buf, dir, len) &&
 		(is_dir_sep(buf[len]) || buf[len] == '\0');
 }
-- 
2.53.0

Re: [PATCH 2/3] path: use the right datatype

[Patrick Steinhardt]

On Mon, Mar 02, 2026 at 07:51:37PM +0530, K Jayatheerth wrote:
> The strlen() function returns a size_t

Micronit: missing punctuation.

> Storing this in a standard signed int is a bad practice
> that invites overflow vulnerabilities if paths get absurdly long.
> 
> Signed-off-by: K Jayatheerth <jayatheerthkulkarni2005@gmail.com>
> ---
>  path.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/path.c b/path.c
> index f613d8bbd1..56be5e1726 100644
> --- a/path.c
> +++ b/path.c
> @@ -58,7 +58,7 @@ static void strbuf_cleanup_path(struct strbuf *sb)
>  
>  static int dir_prefix(const char *buf, const char *dir)
>  {
> -	int len = strlen(dir);
> +	size_t len = strlen(dir);
>  	return !strncmp(buf, dir, len) &&
>  		(is_dir_sep(buf[len]) || buf[len] == '\0');

Makes sense. What's left out in the commit message is an explanation
that this change is safe to do without any further changes. But judging
by the diff it's used in contexts where we already expect a `size_t`
anyway, so it is.

Patrick

Re: [PATCH 2/3] path: use the right datatype

[Junio C Hamano]

K Jayatheerth <jayatheerthkulkarni2005@gmail.com> writes:

> The strlen() function returns a size_t
> Storing this in a standard signed int is a bad practice
> that invites overflow vulnerabilities if paths get absurdly long.
>
> Signed-off-by: K Jayatheerth <jayatheerthkulkarni2005@gmail.com>
> ---
>  path.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/path.c b/path.c
> index f613d8bbd1..56be5e1726 100644
> --- a/path.c
> +++ b/path.c
> @@ -58,7 +58,7 @@ static void strbuf_cleanup_path(struct strbuf *sb)
>  
>  static int dir_prefix(const char *buf, const char *dir)
>  {
> -	int len = strlen(dir);
> +	size_t len = strlen(dir);
>  	return !strncmp(buf, dir, len) &&
>  		(is_dir_sep(buf[len]) || buf[len] == '\0');
>  }

Obviously correct.

We also could tell it to return "bool" without disrupting much else,
as this is a file-scope static function that are only used inside
"if (...)" conditions without its return value stored in any
variable, if we are interested in type kosherness.

I have to wonder if it is easier to read if we used our standard
helper functions, e.g.,

        const char *tail;

        return (skip_prefix(buf, dir, &tail) &&
		(!*tail || is_dir_sep(*tail)));

but probably not.

Thanks.

[PATCH 3/3] path: remove redundant function calls

[K Jayatheerth]

We fetch the exact same setting up to four times.
We fix this by evaluating it once, storing it in a local variable,
and referencing that variable.

Signed-off-by: K Jayatheerth <jayatheerthkulkarni2005@gmail.com>
---
 path.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/path.c b/path.c
index 56be5e1726..5cd38b2a16 100644
--- a/path.c
+++ b/path.c
@@ -741,18 +741,18 @@ int calc_shared_perm(struct repository *repo,
 		     int mode)
 {
 	int tweak;
-
-	if (repo_settings_get_shared_repository(repo) < 0)
-		tweak = -repo_settings_get_shared_repository(repo);
+	int shared_repo = repo_settings_get_shared_repository(repo);
+	if (shared_repo < 0)
+		tweak = -shared_repo;
 	else
-		tweak = repo_settings_get_shared_repository(repo);
+		tweak = shared_repo;
 
 	if (!(mode & S_IWUSR))
 		tweak &= ~0222;
 	if (mode & S_IXUSR)
 		/* Copy read bits to execute bits */
 		tweak |= (tweak & 0444) >> 2;
-	if (repo_settings_get_shared_repository(repo) < 0)
+	if (shared_repo < 0)
 		mode = (mode & ~0777) | tweak;
 	else
 		mode |= tweak;
-- 
2.53.0

Re: [PATCH 3/3] path: remove redundant function calls

[Patrick Steinhardt]

On Mon, Mar 02, 2026 at 07:51:38PM +0530, K Jayatheerth wrote:
> We fetch the exact same setting up to four times.
> We fix this by evaluating it once, storing it in a local variable,

Micronit: we typically write this as if instructing the code itself to
change. So this would rather be something like "Fix this by storing it
in a local variable.".

> and referencing that variable.
> 
> Signed-off-by: K Jayatheerth <jayatheerthkulkarni2005@gmail.com>
> ---
>  path.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/path.c b/path.c
> index 56be5e1726..5cd38b2a16 100644
> --- a/path.c
> +++ b/path.c
> @@ -741,18 +741,18 @@ int calc_shared_perm(struct repository *repo,
>  		     int mode)
>  {
>  	int tweak;
> -
> -	if (repo_settings_get_shared_repository(repo) < 0)
> -		tweak = -repo_settings_get_shared_repository(repo);
> +	int shared_repo = repo_settings_get_shared_repository(repo);
> +	if (shared_repo < 0)
> +		tweak = -shared_repo;
>  	else
> -		tweak = repo_settings_get_shared_repository(repo);
> +		tweak = shared_repo;
>  
>  	if (!(mode & S_IWUSR))
>  		tweak &= ~0222;
>  	if (mode & S_IXUSR)
>  		/* Copy read bits to execute bits */
>  		tweak |= (tweak & 0444) >> 2;
> -	if (repo_settings_get_shared_repository(repo) < 0)
> +	if (shared_repo < 0)
>  		mode = (mode & ~0777) | tweak;
>  	else
>  		mode |= tweak;

I agree with the fix itself though. Probably doesn't matter much as we
simply retrieve a value from the repo settings, but this also removes
some mental overhead in my mind.

Thanks!

Patrick

Re: [PATCH 3/3] path: remove redundant function calls

[Junio C Hamano]

K Jayatheerth <jayatheerthkulkarni2005@gmail.com> writes:

> We fetch the exact same setting up to four times.
> We fix this by evaluating it once, storing it in a local variable,
> and referencing that variable.
>
> Signed-off-by: K Jayatheerth <jayatheerthkulkarni2005@gmail.com>
> ---
>  path.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)

The function body is guarded with "we initialize this just once and
return the value stored in a structure member", so 3 among four of
these calls incur only cost for a no-op call/return, but using a
temporary variable on this caller's side makes it clear that we are
not expecting any recomputation in the callee.

> diff --git a/path.c b/path.c
> index 56be5e1726..5cd38b2a16 100644
> --- a/path.c
> +++ b/path.c
> @@ -741,18 +741,18 @@ int calc_shared_perm(struct repository *repo,
>  		     int mode)
>  {
>  	int tweak;
> -
> -	if (repo_settings_get_shared_repository(repo) < 0)
> -		tweak = -repo_settings_get_shared_repository(repo);
> +	int shared_repo = repo_settings_get_shared_repository(repo);
> +	if (shared_repo < 0)
> +		tweak = -shared_repo;
>  	else
> -		tweak = repo_settings_get_shared_repository(repo);
> +		tweak = shared_repo;
>  
>  	if (!(mode & S_IWUSR))
>  		tweak &= ~0222;
>  	if (mode & S_IXUSR)
>  		/* Copy read bits to execute bits */
>  		tweak |= (tweak & 0444) >> 2;
> -	if (repo_settings_get_shared_repository(repo) < 0)
> +	if (shared_repo < 0)
>  		mode = (mode & ~0777) | tweak;
>  	else
>  		mode |= tweak;

[PATCH v2 0/3] clean up a few things

[K Jayatheerth]

Changes since v1:
 - Update commit messages of patches 2 and 3 to better explain the changes

While reviewing path.c in preparation for the upcoming git repo info path expansions,
I noticed a few areas of accumulated technical debt.

This series cleans up the file by removing an unused header, enforcing proper
size_t typing for path lengths, and eliminating redundant settings evaluations
to keep the underlying path API clean.

K Jayatheerth (3):
  path: remove unused header
  path: use size_t for dir_prefix length
  path: remove redundant function calls

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

-- 
2.53.0

[PATCH v2 1/3] path: remove unused header

[K Jayatheerth]

The "environment.h" header is included in "path.c", but none of the
functions or macros it provides are used in this file.

Signed-off-by: K Jayatheerth <jayatheerthkulkarni2005@gmail.com>
---
 path.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/path.c b/path.c
index d726537622..f613d8bbd1 100644
--- a/path.c
+++ b/path.c
@@ -4,7 +4,6 @@
 
 #include "git-compat-util.h"
 #include "abspath.h"
-#include "environment.h"
 #include "gettext.h"
 #include "repository.h"
 #include "strbuf.h"
-- 
2.53.0

[PATCH v2 2/3] path: use size_t for dir_prefix length

[K Jayatheerth]

The strlen() function returns a size_t. Storing this in a standard
signed int is a bad practice that invites overflow vulnerabilities if
paths get absurdly long.

Switch the variable to size_t. This is safe to do because 'len' is
strictly used as an argument to strncmp() (which expects size_t) and
as a positive array index, involving no signed arithmetic that could
rely on negative values.

Signed-off-by: K Jayatheerth <jayatheerthkulkarni2005@gmail.com>
---
 path.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/path.c b/path.c
index f613d8bbd1..56be5e1726 100644
--- a/path.c
+++ b/path.c
@@ -58,7 +58,7 @@ static void strbuf_cleanup_path(struct strbuf *sb)
 
 static int dir_prefix(const char *buf, const char *dir)
 {
-	int len = strlen(dir);
+	size_t len = strlen(dir);
 	return !strncmp(buf, dir, len) &&
 		(is_dir_sep(buf[len]) || buf[len] == '\0');
 }
-- 
2.53.0

Re: [PATCH v2 2/3] path: use size_t for dir_prefix length

[Junio C Hamano]

K Jayatheerth <jayatheerthkulkarni2005@gmail.com> writes:

> The strlen() function returns a size_t. Storing this in a standard
> signed int is a bad practice that invites overflow vulnerabilities if
> paths get absurdly long.

"a standard signed int" -> "an int variable".  There is no
"nonstandard signed int" anyway ;-)

If we were doing malloc(len) using length truncated due to integer
wraparound and then strcpy() the whole string, it would make us
write beyond the end of the allocation, but in this case, the worst
thing that can happen is that we stop comparing prematurely, which
may make us declare that buf is a path inside the directory dir when
it isn't.  The two callers of this function do not use this
miscalculated len to carry out what they do, so there is no other
damage.  It indeed would be computing a wrong result, but "overflow
vulnerabilities" is a slight exaggeration in the context of this
patch, I think.

"overflow vulnerabilities" -> "bugs due to integer wraparound".

> Switch the variable to size_t. This is safe to do because 'len' is
> strictly used as an argument to strncmp() (which expects size_t) and
> as a positive array index, involving no signed arithmetic that could
> rely on negative values.
>
> Signed-off-by: K Jayatheerth <jayatheerthkulkarni2005@gmail.com>
> ---
>  path.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/path.c b/path.c
> index f613d8bbd1..56be5e1726 100644
> --- a/path.c
> +++ b/path.c
> @@ -58,7 +58,7 @@ static void strbuf_cleanup_path(struct strbuf *sb)
>  
>  static int dir_prefix(const char *buf, const char *dir)
>  {
> -	int len = strlen(dir);
> +	size_t len = strlen(dir);
>  	return !strncmp(buf, dir, len) &&
>  		(is_dir_sep(buf[len]) || buf[len] == '\0');
>  }

[PATCH v2 3/3] path: remove redundant function calls

[K Jayatheerth]

repo_settings_get_shared_repository() is invoked multiple times in
calc_shared_perm(). While the function internally caches the value,
repeated calls still add unnecessary noise.

Store the result in a local variable and reuse it instead. This makes
it explicit that the value is expected to remain constant and avoids
repeated calls in the same scope.

Signed-off-by: K Jayatheerth <jayatheerthkulkarni2005@gmail.com>
---
 path.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/path.c b/path.c
index 56be5e1726..5cd38b2a16 100644
--- a/path.c
+++ b/path.c
@@ -741,18 +741,18 @@ int calc_shared_perm(struct repository *repo,
 		     int mode)
 {
 	int tweak;
-
-	if (repo_settings_get_shared_repository(repo) < 0)
-		tweak = -repo_settings_get_shared_repository(repo);
+	int shared_repo = repo_settings_get_shared_repository(repo);
+	if (shared_repo < 0)
+		tweak = -shared_repo;
 	else
-		tweak = repo_settings_get_shared_repository(repo);
+		tweak = shared_repo;
 
 	if (!(mode & S_IWUSR))
 		tweak &= ~0222;
 	if (mode & S_IXUSR)
 		/* Copy read bits to execute bits */
 		tweak |= (tweak & 0444) >> 2;
-	if (repo_settings_get_shared_repository(repo) < 0)
+	if (shared_repo < 0)
 		mode = (mode & ~0777) | tweak;
 	else
 		mode |= tweak;
-- 
2.53.0

[PATCH v3 0/3] clean up a few things

[K Jayatheerth]

Changes since v2:
 - Update commit message of patch 2.

While reviewing path.c in preparation for the upcoming git repo info path expansions,
I noticed a few areas of accumulated technical debt.

This series cleans up the file by removing an unused header, enforcing proper
size_t typing for path lengths, and eliminating redundant settings evaluations
to keep the underlying path API clean.

K Jayatheerth (3):
  path: remove unused header
  path: use size_t for dir_prefix length
  path: remove redundant function calls

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

-- 
2.53.0

[PATCH v3 1/3] path: remove unused header

[K Jayatheerth]

The "environment.h" header is included in "path.c", but none of the
functions or macros it provides are used in this file.

Signed-off-by: K Jayatheerth <jayatheerthkulkarni2005@gmail.com>
---
 path.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/path.c b/path.c
index d726537622..f613d8bbd1 100644
--- a/path.c
+++ b/path.c
@@ -4,7 +4,6 @@
 
 #include "git-compat-util.h"
 #include "abspath.h"
-#include "environment.h"
 #include "gettext.h"
 #include "repository.h"
 #include "strbuf.h"
-- 
2.53.0

[PATCH v3 2/3] path: use size_t for dir_prefix length

[K Jayatheerth]

The strlen() function returns a size_t. Storing this in a standard
signed int is a bad practice that invites overflow vulnerabilities if
paths get absurdly long.

Switch the variable to size_t. This is safe to do because 'len' is
strictly used as an argument to strncmp() (which expects size_t) and
as a positive array index, involving no signed arithmetic that could
rely on negative values.

Signed-off-by: K Jayatheerth <jayatheerthkulkarni2005@gmail.com>
---
 path.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/path.c b/path.c
index f613d8bbd1..56be5e1726 100644
--- a/path.c
+++ b/path.c
@@ -58,7 +58,7 @@ static void strbuf_cleanup_path(struct strbuf *sb)
 
 static int dir_prefix(const char *buf, const char *dir)
 {
-	int len = strlen(dir);
+	size_t len = strlen(dir);
 	return !strncmp(buf, dir, len) &&
 		(is_dir_sep(buf[len]) || buf[len] == '\0');
 }
-- 
2.53.0

[PATCH v3 3/3] path: remove redundant function calls

[K Jayatheerth]

repo_settings_get_shared_repository() is invoked multiple times in
calc_shared_perm(). While the function internally caches the value,
repeated calls still add unnecessary noise.

Store the result in a local variable and reuse it instead. This makes
it explicit that the value is expected to remain constant and avoids
repeated calls in the same scope.

Signed-off-by: K Jayatheerth <jayatheerthkulkarni2005@gmail.com>
---
 path.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/path.c b/path.c
index 56be5e1726..5cd38b2a16 100644
--- a/path.c
+++ b/path.c
@@ -741,18 +741,18 @@ int calc_shared_perm(struct repository *repo,
 		     int mode)
 {
 	int tweak;
-
-	if (repo_settings_get_shared_repository(repo) < 0)
-		tweak = -repo_settings_get_shared_repository(repo);
+	int shared_repo = repo_settings_get_shared_repository(repo);
+	if (shared_repo < 0)
+		tweak = -shared_repo;
 	else
-		tweak = repo_settings_get_shared_repository(repo);
+		tweak = shared_repo;
 
 	if (!(mode & S_IWUSR))
 		tweak &= ~0222;
 	if (mode & S_IXUSR)
 		/* Copy read bits to execute bits */
 		tweak |= (tweak & 0444) >> 2;
-	if (repo_settings_get_shared_repository(repo) < 0)
+	if (shared_repo < 0)
 		mode = (mode & ~0777) | tweak;
 	else
 		mode |= tweak;
-- 
2.53.0

Re: [PATCH v3 0/3] clean up a few things

[Junio C Hamano]

K Jayatheerth <jayatheerthkulkarni2005@gmail.com> writes:

> Changes since v2:
>  - Update commit message of patch 2.

Hmph, they look identical to me, and more importantly, the previous
round has already been merged to 'next'.

Thanks.

Re: [PATCH v3 0/3] clean up a few things

[K Jayatheerth]

On Fri, Mar 6, 2026 at 1:06 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> K Jayatheerth <jayatheerthkulkarni2005@gmail.com> writes:
>
> > Changes since v2:
> >  - Update commit message of patch 2.
>
> Hmph, they look identical to me, and more importantly, the previous

The only change was the words "a standard signed int" -> "an int variable"
"overflow vulnerabilities" -> "bugs due to integer wraparound"


> round has already been merged to 'next'.
>

Whoops, I didn't check the what's cooking chart,
Thanks for the info Junio ;-)

Regards
- Jayatheerth

Re: [PATCH v3 0/3] clean up a few things

[K Jayatheerth]

On Fri, Mar 6, 2026 at 1:06 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> K Jayatheerth <jayatheerthkulkarni2005@gmail.com> writes:
>
> > Changes since v2:
> >  - Update commit message of patch 2.
>
> Hmph, they look identical to me, and more importantly, the previous
> round has already been merged to 'next'.
>
> Thanks.


I just noticed the commit message actually had not change,
that was a mistake.

Either way, since it is already merged to next
I will consider it acceptable.

Thanks again

Regards
- Jayatheerth

Re: [PATCH v3 0/3] clean up a few things

[Junio C Hamano]

K Jayatheerth <jayatheerthkulkarni2005@gmail.com> writes:

>> Hmph, they look identical to me, and more importantly, the previous
>> round has already been merged to 'next'.
> ...
> I just noticed the commit message actually had not change,
> that was a mistake.

Whew.  I was afraid I was hallucinating.