On Sun, 28 Oct 2018 at 20:01, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote: > > On Sun, Oct 28, 2018 at 11:32 AM Martin Ågren <martin.agren@xxxxxxxxx> wrote: > > [...] > > Let's be explicit about breaking out of the loop. This helps the > > compiler grok our intention. As a bonus, it might make it (even) more > > obvious to human readers that the loop stops at the first space. > > This did come up in review[1,2]. I had a hard time understanding the > loop because it looked idiomatic but wasn't (and we have preconceived > notions about behavior of things which look idiomatic). > > [1]: https://public-inbox.org/git/CAPig+cQbG2s-LrAo9+7C7=dXifbWFJ3SzuNa-QePHDk7egK=jg@xxxxxxxxxxxxxx/ > [2]: https://public-inbox.org/git/CAPig+cRjU6niXpT2FrDWZ0x1HmGf1ojVZj3uk2qXEGe-S7i_HQ@xxxxxxxxxxxxxx/ Hmm, I should have been able to dig those up myself. Thanks for the pointers. > > /* Determine the length of the label */ > > + for (i = 0; i < len; i++) { > > + if (isspace(name[i])) { > > len = i; > > + break; > > + } > > + } > > strbuf_addf(&ref_name, "refs/rewritten/%.*s", len, name); > > At least for me, this would be more idiomatic if it was coded like this: > > for (i = 0; i < len; i++) > if (isspace(name[i])) > break; > strbuf_addf(..., i, name); > > or, perhaps (less concise): > > for (i = 0; i < len; i++) > if (isspace(name[i])) > break; > len = i; > strbuf_addf(..., len, name); This second one is more true to the original in that it updates `len` to the new, shorter length. Which actually seems to be needed -- towards the very end of the function, `len` is used, so the first of these suggestions would change the behavior. Thanks a lot for a review. I'll wait for any additional comments and will try a v2 with your second suggestion. Martin
