Fixes for :(optional) path code

Unlike the configuration option magic, the parseopt code also ignores empty files: compare implementations from ccfcaf399f (parseopt: values of pathname type can be prefixed with :(optional), 2025-09-28) and 749d6d166d (config: values of pathname type can be prefixed with :(optional), 2025-09-28).

Unify the 2 by not ignoring empty files, which is less surprising and the intended semantics from the first patch for config.

Suggested-by Phillip Wood <[email protected]>

Signed-off-by D. Ben Knoble <[email protected]>

---
  parse-options.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

parse-options.c +1 -1

index 5933468c19..6211b55a83 100644
@@ -226,7 +226,7 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
  		if (!value)
  			is_optional = 0;
  		value = fix_filename(p->prefix, value);
-		if (is_optional && is_empty_or_missing_file(value)) {
+		if (is_optional && is_missing_file(value)) {
  			free((char *)value);
  		} else {
  			FREE_AND_NULL(*(char **)opt->value);

Hi Ben

These all look good to me though I agree with Junio's comments on patch
3. It would be nice to get at least the fist patch merged in time for 2.52.0.

Quoted 5 lines · Phillip Wood

Hi Ben

These all look good to me though I agree with Junio's comments on patch
3. It would be nice to get at least the fist patch merged in time for 2.52.0.

Yup, let me do exactly that ;-)

Thanks, both.

Quoted 11 lines · Junio C Hamano

Quoted 5 lines · Phillip Wood

Hi Ben

These all look good to me though I agree with Junio's comments on patch
3. It would be nice to get at least the fist patch merged in time for 2.52.0.

Yup, let me do exactly that ;-)

Thanks, both.

Let me have this on top of Ben's 5-patch series.

----- >8 -----
Subject: [PATCH] parseopt: remove unreachable code

At this point in the code after running skip_prefix() on the variable and receiving the result in the same variable, the contents of the variable can never be NULL. The function either (1) updates the variable to point at a later part of the string it originally pointed at, or (2) leaves it intact if the string does not have the prefix. (1) will never make the variable NULL, and (2) cannot be the source of NULL, because the variable cannot be NULL before calling skip_prefix(), which would die immediately by dereferencing the NULL pointer in that case.

Helped-by Phillip Wood <[email protected]>

Signed-off-by Junio C Hamano <[email protected]>

---
 parse-options.c | 2 --
 1 file changed, 2 deletions(-)

parse-options.c +0 -3

index 27c1e75d53..97a55300e8 100644
@@ -223,8 +223,6 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
                        return 0;
 
                is_optional = skip_prefix(value, ":(optional)", &value);
-               if (!value)
-                       is_optional = false;
                value = fix_filename(p->prefix, value);
                if (is_optional && is_missing_file(value)) {
                        free((char *)value);
--

2.52.0-rc0-28-g4cf919bd7b

Junio C Hamano <[email protected]> writes:

Let me have this on top of Ben's 5-patch series.

----- >8 -----
Subject: [PATCH] parseopt: remove unreachable code

At this point in the code after running skip_prefix() on the variable and receiving the result in the same variable, the contents of the variable can never be NULL. The function either (1) updates the variable to point at a later part of the string it originally pointed at, or (2) leaves it intact if the string does not have the prefix. (1) will never make the variable NULL, and (2) cannot be the source of NULL, because the variable cannot be NULL before calling skip_prefix(), which would die immediately by dereferencing the NULL pointer in that case.

Helped-by Phillip Wood <[email protected]>

Signed-off-by Junio C Hamano <[email protected]>

---
  parse-options.c | 2 --
  1 file changed, 2 deletions(-)

parse-options.c +0 -2

index 27c1e75d53..97a55300e8 100644
@@ -223,8 +223,6 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
  			return 0;
 
  		is_optional = skip_prefix(value, ":(optional)", &value);
-		if (!value)
-			is_optional = false;
  		value = fix_filename(p->prefix, value);
  		if (is_optional && is_missing_file(value)) {
  			free((char *)value);

Quoted 7 lines · Junio C Hamano

Junio C Hamano <[email protected]> writes:

Let me have this on top of Ben's 5-patch series.

----- >8 -----
Subject: [PATCH] parseopt: remove unreachable code

Quoted 9 lines

At this point in the code after running skip_prefix() on the variable and receiving the result in the same variable, the contents of the variable can never be NULL. The function either (1) updates the variable to point at a later part of the string it originally pointed at, or (2) leaves it intact if the string does not have the prefix. (1) will never make the variable NULL, and (2) cannot be the source of NULL, because the variable cannot be NULL before calling skip_prefix(), which would die immediately by dereferencing the NULL pointer in that case.

Nicely explained, the changes below look good

Thanks

Phillip

Quoted 20 lines

Helped-by Phillip Wood <[email protected]>

Signed-off-by Junio C Hamano <[email protected]>

---
  parse-options.c | 2 --
  1 file changed, 2 deletions(-)

parse-options.c +0 -2

index 27c1e75d53..97a55300e8 100644
@@ -223,8 +223,6 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
  			return 0;
 
  		is_optional = skip_prefix(value, ":(optional)", &value);
-		if (!value)
-			is_optional = false;
  		value = fix_filename(p->prefix, value);
  		if (is_optional && is_missing_file(value)) {
  			free((char *)value);

Suggested-by Phillip Wood <[email protected]>

Signed-off-by D. Ben Knoble <[email protected]>

---
 parse-options.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

parse-options.c +1 -1

index 6211b55a83..197c01987e 100644
@@ -208,7 +208,7 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
 	case OPTION_FILENAME:
 	{
 		const char *value;
-		int is_optional;
+		bool is_optional;
 
 		if (unset)
 			value = NULL;

parse-options.c +0 -0

 		value = fix_filename(p->prefix, value);
 		if (is_optional && is_missing_file(value)) {
 			free((char *)value);

parse-options.c static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p, · line 227

Quoted 4 lines · D. Ben Knoble

  		is_optional = skip_prefix(value, ":(optional)", &value);
  		if (!value)

parse-options.c +1 -1

-			is_optional = 0;
+			is_optional = false;

Whether it is spelled 0 or false, I do not think this makes any sense. skip_prefix() either touches &value to point at the substring in value that comes after ":(optional)", or it does not touch it at all, so there is no way value can be NULL here (and we know value is not NULL before we call skip_prefix()).

Shouldn't you be removing the entire "if value is NULL, it is not optional" thing instead? That is exactly what Phillip pointed out in his review.

Quoted 13 lines · Junio C Hamano

parse-options.c static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p, · line 227

Quoted 4 lines · D. Ben Knoble

             is_optional = skip_prefix(value, ":(optional)", &value);
             if (!value)

parse-options.c +1 -1

-                    is_optional = 0;
+                    is_optional = false;

Whether it is spelled 0 or false, I do not think this makes any sense. skip_prefix() either touches &value to point at the substring in value that comes after ":(optional)", or it does not touch it at all, so there is no way value can be NULL here (and we know value is not NULL before we call skip_prefix()).

Quoted 3 lines

Shouldn't you be removing the entire "if value is NULL, it is not optional" thing instead? That is exactly what Phillip pointed out in his review.

Looking at this again I wonder if the intention was to error out if there wasn't a filename after the ":(optional)" prefix which I think would be a reasonable thing to do but that's not what this code actually does.

Thanks

Phillip