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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Mon, Sep 06 2021, Calum McConnell wrote:

> 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?