| Message ID | 20210906181002.625647-2-calumlikesapplepie@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] Add support for new %w wildcard in checkout filter | expand |
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?
On 07/09/21 01.10, Calum McConnell wrote: > + /* 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"); > + } > + Why don't simply print that error message without BUG() (aka using die(_("message"))? It can be l10n-ed if you using the approach.
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
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"); + } + /* expand all %f or %w with the quoted path */ strbuf_expand(&cmd, params->cmd, strbuf_expand_dict_cb, &dict); strbuf_release(&filePath);
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(+)