On Mon, Nov 14, 2016 at 11:23 AM, Karthik Nayak <karthik.188@xxxxxxxxx> wrote: > Hello > > On Wed, Nov 9, 2016 at 5:44 AM, Jacob Keller <jacob.keller@xxxxxxxxx> wrote: >> On Tue, Nov 8, 2016 at 12:12 PM, Karthik Nayak <karthik.188@xxxxxxxxx> wrote: >>> From: Karthik Nayak <karthik.188@xxxxxxxxx> >>> >>> 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. > Agreed, I think that it makes more sense to keep this as is. It is relatively complicated and the strings do have some duplicate code, but I think it's still ok. Thanks, Jake > -- > Regards, > Karthik Nayak