On Fri, Jun 18, 2021 at 12:51 PM Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> wrote: > On Fri, Jun 18 2021, ZheNing Hu wrote: > > After some refactoring, I found that there are two problems: > > 1. There are a lot of codes like this in ref-filter to fill v->s: > > > > v->s = show_ref(...) > > v->s = copy_email(...) > > > > It is very difficult to modify here: We know that show_ref() > > or copy_email() will allocate a block of memory to v->s, but > > if v->s is a strbuf, what should we do? In copy_email(), we > > can pass the v->s to copy_email() and use strbuf_add()/strbuf_addstr() > > instead of xstrdup() and xmemdupz(). But show_ref() will call > > external functions like shorten_unambiguous_ref(), we don’t know > > whether it will return us NULL or a dynamically allocated memory. > > If continue to pass v->s to the inner function, it is not a feasible > > method. Or we can use strbuf_attach() + strlen(), I'm not sure > > this is a good method. If you resend this patch, it might be a good idea to add a short version of the above explanations into the commit message. [...] > > In the case of using strbuf, I don’t know how to distinguish between an empty > > strbuf and NULL. It can be easily distinguished by using c-style "const char*". Maybe this could also be part of the explanation. > Yes, sometimes it's just too much of a hassle, looking at > shorten_unambiguous_ref() which returns a xstrdup()'d value that could > indeed be strbuf_attach'd. I haven't tried the conversion myself, > perhaps it's too much hassle. > > Just a suggestion from reading your patch in isolation. Yeah, thanks for the review anyway!
