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

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