Re: [PATCH 2/3] Die if filter is attempted without a worktree

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Sep 6, 2021, 22:09 UTC
Message-ID: <87lf49nye4.fsf@evledraar.gmail.com>
In-Reply-To: <20210906181002.625647-2-calumlikesapplepie@gmail.com>

On Mon, Sep 06 2021, Calum McConnell wrote:

Show 22 quoted lines

> As far as I know, this isn't possible.  Rather than add a bunch of
> code to workarround something that might not be possible, lets just
> halt and catch fire if it does.  This might need to be removed before
> the change goes into master
>
> Signed-off-by: Calum McConnell <calumlikesapplepie@gmail.com>
> ---
>  convert.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/convert.c b/convert.c
> index 5d64ccce57..df70c250b0 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -646,6 +646,11 @@ static int filter_buffer_or_fd(int in, int out, void *data)
>  	sq_quote_buf(&worktreePath, the_repository->worktree);
>  	dict[1].value = worktreePath.buf;
>  
> +	/* The results of a nonexistent worktree could be... weird.  Lets avoid*/
> +	if(dict[1].value == NULL){
> +		BUG("There is no worktree for this worktree substitution");
> +	}

This BUG() addition is itself buggy, elsewhere e.g. in builtin/gc.c you can see where we have conditions like:

    the_repository->worktree ? the_repository->worktree : the_repository->gitdir;

I'm not bothering much with the greater context here, but if we suppose that we have a case where worktreePath.buf is NULL, then the_repository->worktree surely must have been NULL, and if you check what sq_quote_buf() does, you'll see:

    void sq_quote_buf(struct strbuf *dst, const char *src)
    [...]
            while (*src) {

I.e. we'd segfault anyway if that "src" were to be NULL.

Even if that weren't the case then that's not the same as the worktreePath.buf being NULL, which even if we suppose sq_quote_buf() won't segfault and just returned won't AFAICT ever be the case, see the comment for strbuf_slopbuf in strbuf.c. So I think that even if you somehow reached this with a NULL worktree that BUG() won't ever be reached.

I think this can probably just be dropped, to the extent that we need some check like this it seems like it should happen a lot earlier in convert.c than here, i.e. during the early setup can't we detect & abort if we don't have a required worktree?