Re: [PATCH] read_gitfile_gently(): return non-repo path on error

[Patrick Steinhardt <ps@pks.im>]

On Tue, Jun 02, 2026 at 02:11:59AM -0400, Jeff King wrote:
[snip]
> Two other points of interest.
> 
> One, I'm not sure how useful printing the pointed-to directory is. We
> _could_ just say:
> 
>   fatal: gitfile does not point to a valid repository: /path/to/.git
> 
> which is enough for somebody to investigate themselves. That would
> certainly make the patch smaller.

I have to agree that the patch is somewhat gross, and I myself don't
really see much of an issue to move to an error message like the above
if it ends up simplifying the logic.

> And two, I ran into this running doc-diff:
> 
>   $ ./doc-diff HEAD^ HEAD
>   fatal: not a git repository: (null)
> 
> The correct output (which this patch produces) is:
> 
>   fatal: not a git repository: /home/peff/compile/git/.git/worktrees/worktree3
> 
> And indeed, that path is missing. But why? I feel like I've run into
> this same problem occasionally over the last year or so, but never
> before. Did we get more aggressive about removing worktrees at some
> point? I haven't been able to reproduce whatever is killing off the
> worktree directory, and by the time I see the error it is long gone.

Both git-gc(1) and git-maintenance(1) prune orphaned worktrees that are
older than three months by default, which can be configured via
"gc.worktreePruneExpire". That logic has changed in 4dda60c9df (Merge
branch 'ps/maintenance-missing-tasks', 2025-05-15), which would kind of
match your timeline.

But rereading that patch series I cannot really see how it could result
in more aggressive pruning of worktrees. We used `git worktree prune
--expire <expiry>` before that series, and we still use that logic now.

Hum.

> diff --git a/setup.c b/setup.c
> index 075bf89fa9..2df6fbf595 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -1641,9 +1650,11 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
>  					return GIT_DIR_INVALID_GITFILE;
>  			default:
>  				if (die_on_error)
> -					read_gitfile_error_die(error_code, dir->buf, NULL);
> -				else
> +					read_gitfile_error_die(error_code, dir->buf, error_dst);
> +				else {
> +					free(error_dst);
>  					return GIT_DIR_INVALID_GITFILE;
> +				}

The `if` branch should also gain some curly braces here.

Patrick