On Mon, Mar 28, 2022 at 7:23 PM Junio C Hamano <gitster@xxxxxxxxx> wrote: > > Glen Choo <chooglen@xxxxxxxxxx> writes: > > > Hm, what do you think of an alternate approach of storing of the > > matching remotes in a string_list, something like: [...] > > then construct the advice message in setup_tracking()? To my untrained > > eye, "case 2" requires a bit of extra work to understand. Interestingly, that was what I had in the original RFC. I started using the strbuf later, after Ævar confirmed that a single "advise()" call is the way to go. I understood building the string as we go to lead to simpler code, as it meant one less loop. On the other hand I understand Junio is more concerned about performance than the existence of a second loop that we should almost never hit. I'm very happy to switch from strbuf-building to string_list-appending, but I'm curious to understand how/why the performance of strbuf_addf() would be notably worse than that of string_list_append(). Is there public doc about this somewhere? > Having said that, as long as you do that lazily not to penalize > those who have sane setting without the need for advice/error to > trigger, I do not particularly care how the list of matching remote > names are kept. Having string_list_append() unconditionally like > the above patch has, even for folks with just a single match without > need for the advice/error message is suboptimal, I would think. Again, I'm new here, and not a great coder to start with, but I'm having a hard time understanding why the single extra/gratuitous strbuf_addf() or string_list_append() call that we stand to optimize (I haven't understood whether there is a significant difference between them) would ever be noticeable in the context of creating a branch. I of course completely understand optimizing anything that will end up looping, but this is a max of 1 iteration's savings; I would have thought that at these levels, readability/maintainability (and succinctness) of the code would trump any marginal performance savings. To that end, I'd understand going back to string_list_append() as Glen proposes, and building a formatted string in a single place (setup_tracking()) only when required - both for readability, and in case some aspect of strbuf_addf() is non-trivially expensive - but is the "only append to the string_list() on the rare second pass" optimization really worth the increase in amount of code? Is "performance over succinctness" a general principle that could or should be noted somewhere?
