Re: [PATCH 2/8] [GSOC] ref-filter: add %(raw) atom

From: ZheNing Hu <adlternative@gmail.com>
Date: Jun 17, 2021, 16:14 UTC
Message-ID: <CAOLTT8SGkObKWMZax-k4KXgsN+7ezvOMkRU-zt+c2zon2Ta3pA@mail.gmail.com>
In-Reply-To: <87r1h0wnwg.fsf@evledraar.gmail.com>

Ævar Arnfjörð Bjarmason <avarab@gmail.com> 于2021年6月17日周四 下午10:45写道：

Show 13 quoted lines

>
>
> On Thu, Jun 17 2021, ZheNing Hu wrote:
>
> >
> > Yes, strbuf is a suitable choice when using <str,len> pair.
> > But if replace v->s with strbuf, the possible changes will be larger.
>
> I for one would like to see it done that way, those changes are usually
> easy to read. Also it seems a large part of 2/8 is extra new code
> because we didn't do that, e.g. getting length differently if something
> is a strbuf or not, passing char*/size_t pairs to new functions etc.
>

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.

2. See:

-       for (i = 0; i < used_atom_cnt; i++) {
+       for (i = 0; i < used_atom_cnt; i++) {
                struct atom_value *v = &ref->value[i];
-               if (v->s == NULL && used_atom[i].source == SOURCE_NONE)
+               if (v->s.len == 0 && used_atom[i].source == SOURCE_NONE)
                        return strbuf_addf_ret(err, -1, _("missing
object %s for %s"),

oid_to_hex(&ref->objectname), ref->refname);
        }

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*".

Show 8 quoted lines

> >
> > Not python safe. See [1].
> > Regarding the perl language, I support Junio's point of view: it can be
> > re-supported in the future.
>
> Ah, I'd missed that. Anyway, if it's easy it seems you discovered that
> Perl deals with it correctly, so we could just have it support this.
>

Well, it's ok, support for perl will be put in a separate commit.

Show 37 quoted lines

> >>
> >> > +test_expect_success 'basic atom: refs/tags/testtag *raw' '
> >> > +     git cat-file commit refs/tags/testtag^{} >expected &&
> >> > +     git for-each-ref --format="%(*raw)" refs/tags/testtag >actual &&
> >> > +     sanitize_pgp <expected >expected.clean &&
> >> > +     sanitize_pgp <actual >actual.clean &&
> >> > +     echo "" >>expected.clean &&
> >>
> >> Just "echo" will do, ditto for the rest. Also odd to go back and forth
> >> between populating expected.clean & actual.clean.
> >>
> >
> > Are you saying that sanitize_pgp is not needed?
>
> No that instead of:
>
>     echo "" >x
>
> You can do:
>
>     echo >x
>
> And also that going back and forth between populating different files is
> confusing, i.e. this:
>
>
>     echo a >x
>     echo c >y
>     echo b >>x
>
> is better as:
>
>     echo a >x
>     echo b >>x
>     echo c >y
>
>

Thanks, I get what you meant now.

Show 28 quoted lines

> >>
> >> > +test_expect_success 'set up refs pointing to binary blob' '
> >> > +     printf "a\0b\0c" >blob1 &&
> >> > +     printf "a\0c\0b" >blob2 &&
> >> > +     printf "\0a\0b\0c" >blob3 &&
> >> > +     printf "abc" >blob4 &&
> >> > +     printf "\0 \0 \0 " >blob5 &&
> >> > +     printf "\0 \0a\0 " >blob6 &&
> >> > +     printf "  " >blob7 &&
> >> > +     >blob8 &&
> >> > +     git hash-object blob1 -w | xargs git update-ref refs/myblobs/blob1 &&
> >> > +     git hash-object blob2 -w | xargs git update-ref refs/myblobs/blob2 &&
> >> > +     git hash-object blob3 -w | xargs git update-ref refs/myblobs/blob3 &&
> >> > +     git hash-object blob4 -w | xargs git update-ref refs/myblobs/blob4 &&
> >> > +     git hash-object blob5 -w | xargs git update-ref refs/myblobs/blob5 &&
> >> > +     git hash-object blob6 -w | xargs git update-ref refs/myblobs/blob6 &&
> >> > +     git hash-object blob7 -w | xargs git update-ref refs/myblobs/blob7 &&
> >> > +     git hash-object blob8 -w | xargs git update-ref refs/myblobs/blob8
> >>
> >> Hrm, xargs just to avoid:
> >>
> >>     git update-ref ... $(git hash-object) ?
> >>
> >
> > I didn’t think about it, just for convenience.
>
> *nod*, Junio had a good suggestion.
>

ok.

Thanks. -- ZheNing Hu