On Tue, May 15, 2018 at 1:15 PM, Leif Middelschulte <leif.middelschulte@xxxxxxxxx> wrote: > Hello Stefan, > > thank you once again for your effort. > > Am 15. Mai 2018 um 22:00:34, Stefan Beller > (sbeller@xxxxxxxxxx(mailto:sbeller@xxxxxxxxxx)) schrieb: > >> This rerolls the two commits found at [1] with the feedback of Eliah >> and puts Leifs patch[2] on top, that I edited according to Eliahs feedback, >> but kept Leifs ownership. >> >> This has addressed all of Eliahs feedback AFAICT. >> You'll find a branch-diff below[3], which lacks >> the new patch of Leif in that series, but is part of the reroll? >> >> Leif, what do you think? > > Seems great to me. Thank you for picking up and improving my changes :) > One Question though: Shouldn’t an enum (like > NOTES_MERGE_VERBOSITY_DEFAULT) be used instead of numbers? Hah! I did not know that existed. $ git grep NOTES_MERGE_VERBOSITY_DEFAULT builtin/notes.c:810: o.verbosity = verbosity + NOTES_MERGE_VERBOSITY_DEFAULT; notes-merge.c:22: o->verbosity = NOTES_MERGE_VERBOSITY_DEFAULT; notes-merge.h:9: NOTES_MERGE_VERBOSITY_DEFAULT = 2, It doesn't seem to be used much, as opposed to numbers: $ git grep show -- merge-recursive.c merge-recursive.c:201:static int show(struct merge_options *o, int v) merge-recursive.c:211: if (!show(o, v)) merge-recursive.c:570: opts.show_rename_progress = o->show_rename_progress; merge-recursive.c:1096: if (show(o, 3)) { merge-recursive.c:1099: } else if (show(o, 2)) merge-recursive.c:1108: if (show(o, 3)) { merge-recursive.c:1111: } else if (show(o, 2)) merge-recursive.c:2178: if (show(o, 4) || o->call_depth) merge-recursive.c:2275: if (show(o, 4)) { merge-recursive.c:2286: if (show(o, 5)) { merge-recursive.c:2351: if (show(o, 2)) (The first two are the implementation of show/output, third is somewhat unrelated to show() and all the rest is numbers). If we'd want to use NOTES_MERGE_VERBOSITY_DEFAULT, I would suggest to send a followup series on top of this? I would think numbers are fine for now. Thanks, Stefan
