Hello On Wed, Nov 9, 2016 at 5:44 AM, Jacob Keller <jacob.kel...@gmail.com> wrote: > On Tue, Nov 8, 2016 at 12:12 PM, Karthik Nayak <karthik....@gmail.com> wrote: >> From: Karthik Nayak <karthik....@gmail.com> >> >> Port branch.c to use ref-filter APIs for printing. This clears out >> most of the code used in branch.c for printing and replaces them with >> calls made to the ref-filter library. > > Nice. This looks correct based on checking against the current > branch.c implementation by hand. There was one minor change I > suggested but I'm not really sure it buys is that much. >
Thanks for this review. More down. >> + if (filter->verbose > 1) >> + strbuf_addf(&local, >> "%%(if)%%(upstream)%%(then)[%s%%(upstream:short)%s%%(if)%%(upstream:track)" >> + "%%(then): >> %%(upstream:track,nobracket)%%(end)] %%(end)%%(contents:subject)", >> + branch_get_color(BRANCH_COLOR_UPSTREAM), >> branch_get_color(BRANCH_COLOR_RESET)); > > When we have extra verbose, we check whether we have an upstream, and > if so, we print the short name of that upstream inside brackets. If we > have tracking information, we print that without brackets, and then we > end this section. Finally we print the subject. > > We could almost re-use the code for the subject bits, but I'm not sure > it's worth it. Maybe drop the %contents:subject part and add it > afterwards since we always want it? It would remove some duplication > but overall not sure it's actually worth it. > If you see that's the last part we add to the 'local' strbuf in the verbose case. If we want to remove the duplication we'll end up adding one more strbuf_addf(...). So I guess its better this way. -- Regards, Karthik Nayak
