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

[Calum McConnell <calumlikesapplepie@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3272 bytes --]

On Tue, 2021-09-07 at 00:09 +0200, Ævar Arnfjörð Bjarmason wrote:
> 
> 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?

Part of the reason I inserted this check was because I wasn't sure about
the ordering on a checkout into an empty directory (eg, would there
actually be the filter script when it ran?).  On deeper thought, however,
that problem wouldn't even be solved by this.  Not to mention how you
would need to include the script on the initial commit, and such.

Since the check doesn't even do what I thought it would do, I thought of a
better approach: rather than having it expand to the working tree, it
expands to the git directory.  This means that you can place your scripts
in a much better location: since .git/config must be modified anyways to
use a filter, it doesn't entail a loss of features.

Why I ever thought to use the worktree is beyond me. Patch V2 coming in a
few days.

Calum McConnell


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]