# [GSoC] Git Blog 5 3 messages from 2021-06-20 to 2021-06-21. Participants: ZheNing Hu, Christian Couder. Thread: https://gitlist.dev/t/55970 ## ZheNing Hu, 2021-06-20 16:53 Subject: [GSoC] Git Blog 5 Message-ID: URL: https://gitlist.dev/e/CAOLTT8TZqKQ1vkRX7apvzAJ04Cv9aHJ9kF1%2BwJcEvyi-pibOOw%40mail.gmail.com ``` My fifth week blog finished: The web version is here: https://adlternative.github.io/GSOC-Git-Blog-5/ ## Week5: Tempting apple This week, I spent a lot of time working on the digital circuit course design of the school. So this week's patches for git was completed in a hurry. This week `Ævar Arnfjörð Bjarmason` gave a lot of useful suggestions for the patch I wrote earlier. Some are related to code style improvements, and some are better design ideas. Before, I wanted to use a `` style string in `atom_value` to help copy and compare data containing '\0'. Like this: ˋˋˋc struct atom_value { const char *s; size_t s_size; ... }; ˋˋˋ But `Ævar Arnfjörð Bjarmason` thinks it is more reasonable to use `strbuf` instead of ``. Like this: ˋˋˋc struct atom_value { struct strbuf s; ... }; ˋˋˋ Since the `strbuf` API has a natural ``, we can add data that may contain '\0' through `strbuf_add()`, `strbuf_addbuf()`, and `strbuf_addf()` can also be used to fill strbuf with format string. Use `strbuf_addstr()` to replace `xstrdup()`, `strbuf_add()` to replace `xmemdupz()`, this is indeed a very tempting choice. But in my actual refactoring process, this is not very easy to achieve. For example, the original interface is like this, ˋˋˋc v->s = copy_email(wholine, &used_atom[i]); ˋˋˋ `v->s` will be filled with the data dynamically allocated by `copy_email()`. ˋˋˋc static const char *copy_email(const char *buf, struct used_atom *atom) { ... if (!eoemail) return xstrdup(""); return xmemdupz(email, eoemail - email); } ˋˋˋ Then if we want use `strbuf` type `v->s`, we should change the `copy_email()` interface parameters and return value. ˋˋˋc static void copy_email(struct strbuf *str, const char *buf, struct used_atom *atom) { ... if (!eoemail) return; return strbuf_add(str, email, eoemail - email); } ˋˋˋ Then the caller can do: ˋˋˋc copy_email(&v->s, wholine, &used_atom[i]); ˋˋˋ This is in line with our expectations. But something like `fill_remote_ref_details()`, things gradually become complicated and difficult. `fill_remote_ref_details()`. Just consider the `show_ref()` called in `fill_remote_ref_details()`, `show_ref()` may call `shorten_unambiguous_ref()` internally, and then another function is called internally in `shorten_unambiguous_ref()`... This makes us fail the method of passing `v->s` in parameters like `copy_email()` does. Another way of thinking: what if I can "attach" the data directly? Using `strbuf_attach()` may be a viable option, but... ˋˋˋc size_t len; void *data = shorten_unambiguous_ref(refname, warn_ambiguous_refs); len = strlen(data); strbuf_attach(&str, data, len, len); ˋˋˋ ...we need to get the length of the data, but this is not easy to do, `strlen()` can only be used on data that does not contain '\0', and we are not sure whether a function like `shorten_unambiguous_ref()` will return a `NULL`. Well, this is one of the reasons why I cannot move on. On the other hand, look at the following piece of code, it appears in `populate_value()`. ˋˋˋc 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) return strbuf_addf_ret(err, -1, _("missing object %s for %s"), oid_to_hex(&ref->objectname), ref->refname); } ˋˋˋ We need to determine whether `v->s` equals to `NULL`. We can use c-style strings to easily distinguish between empty strings(`xstrdup("")`) and `NULL`, but if we use strbuf, it is not easy to distinguish, because an empty strbuf has the following characteristics: `s.buf == strbuf_slopbuf` or `sb->buf[0] == '\0'`. It can be said that we shouldn’t even use `NULL` to assign to `s.buf`. So in the end, I rejected this seemingly very attractive solution, and use the previous strategy: ``, unless someone can think of a better solution later `;-)`. * FootNote: 1. Why is there no `strbuf_empty()` in `strbuf` API? I think this may be a very important thing. ˋˋˋc #define strbuf_empty(sb) \ (sb->buf == strbuf_slopbuf) ? \ (!strbuf_slopbuf[0]) : \ (sb->buf[0] == '\0') ˋˋˋ 2. Another thing worth mentioning is: I will have the school final exam between July 7th and July 14th, I may be busy during this period. Thanks for Git community, reviewers and mentors. -- ZheNing Hu ``` ## Christian Couder, 2021-06-21 12:07 Subject: Re: [GSoC] Git Blog 5 Message-ID: URL: https://gitlist.dev/e/CAP8UFD0sHevUHB-ryUZm9Sxm%3DdaKMwbqe6_Q7kc-pZAZN-eHWA%40mail.gmail.com In-Reply-To: ``` On Sun, Jun 20, 2021 at 6:53 PM ZheNing Hu wrote: > > My fifth week blog finished: > The web version is here: > https://adlternative.github.io/GSOC-Git-Blog-5/ Great, thanks! > ## Week5: Tempting apple > > This week, I spent a lot of time working on the digital circuit > course design of the school. So this week's patches for git > was completed in a hurry. This week `Ævar Arnfjörð Bjarmason` s/was/were/ > gave a lot of useful suggestions for the patch I wrote earlier. Maybe a link could help. > Some are related to code style improvements, and some are > better design ideas. [...] > But something like `fill_remote_ref_details()`, things gradually > become complicated and difficult. `fill_remote_ref_details()`. I think you can remove the above `fill_remote_ref_details()` as you are talking about it later. > Just consider the `show_ref()` called in `fill_remote_ref_details()`, > `show_ref()` may call `shorten_unambiguous_ref()` internally, and > then another function is called internally in `shorten_unambiguous_ref()`... > Well, this is one of the reasons why I cannot move on. > > On the other hand, look at the following piece of code, it appears > in `populate_value()`. > > ˋˋˋc > 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) > return strbuf_addf_ret(err, -1, _("missing object %s for %s"), > oid_to_hex(&ref->objectname), ref->refname); > } > ˋˋˋ > > We need to determine whether `v->s` equals to `NULL`. We can use s/equals to/equals/ > c-style strings to easily distinguish between empty strings(`xstrdup("")`) > and `NULL`, but if we use strbuf, it is not easy to distinguish, because > an empty strbuf has the following characteristics: `s.buf == strbuf_slopbuf` > or `sb->buf[0] == '\0'`. It can be said that we shouldn’t even use `NULL` > to assign to `s.buf`. > > So in the end, I rejected this seemingly very attractive solution, and use the > previous strategy: ``, unless someone can think of a better > solution later `;-)`. > > * FootNote: > 1. Why is there no `strbuf_empty()` in `strbuf` API? I think this may be > a very important thing. There is a 'size_t len' field in 'struct strbuf', so doing `buf->len == 0` works. > 2. Another thing worth mentioning is: I will have the school final exam > between July 7th and July 14th, I may be busy during this period. Ok, thanks for telling us in advance. ``` ## ZheNing Hu, 2021-06-21 13:17 Subject: Re: [GSoC] Git Blog 5 Message-ID: URL: https://gitlist.dev/e/CAOLTT8TYn0x_wuDz7Nn1sKn0VKMyF320p1urxG1KHc%2Bse4aiNA%40mail.gmail.com In-Reply-To: ``` Christian Couder 于2021年6月21日周一 下午8:07写道： > > On Sun, Jun 20, 2021 at 6:53 PM ZheNing Hu wrote: > > > > My fifth week blog finished: > > The web version is here: > > https://adlternative.github.io/GSOC-Git-Blog-5/ > > Great, thanks! > > > ## Week5: Tempting apple > > > > This week, I spent a lot of time working on the digital circuit > > course design of the school. So this week's patches for git > > was completed in a hurry. This week `Ævar Arnfjörð Bjarmason` > > s/was/were/ > > > gave a lot of useful suggestions for the patch I wrote earlier. > > Maybe a link could help. > > > Some are related to code style improvements, and some are > > better design ideas. > > [...] > > > But something like `fill_remote_ref_details()`, things gradually > > become complicated and difficult. `fill_remote_ref_details()`. > > I think you can remove the above `fill_remote_ref_details()` as you > are talking about it later. > OK. > > * FootNote: > > 1. Why is there no `strbuf_empty()` in `strbuf` API? I think this may be > > a very important thing. > > There is a 'size_t len' field in 'struct strbuf', so doing `buf->len > == 0` works. > Really? So I think for reader who is learning strbuf api for the first time, the characteristics of empty strbuf are not so obvious. For example, `strbuf_slopbuf` may mean "empty". In any case, adding a macro is absolutely reasonable and will not affect performance. #define strbuf_empty(sb) (sb->len == 0) > > 2. Another thing worth mentioning is: I will have the school final exam > > between July 7th and July 14th, I may be busy during this period. > > Ok, thanks for telling us in advance. Correct the time deviation, the examination time is from July 1st to July 12th. -- ZheNing Hu ```