Jonathan Tan <jonathantanmy@xxxxxxxxxx> writes: >> In any case, here is what I tentatively have in my tree (with heavy >> rewrite to the proposed log message). > > Junio, what are your plans over what you have in your tree? If you'd > like to hear Heba's opinion on it, then she can chime in; if you'd like > a review, then I think it's good to go in. On hold until anything like those happens ;-) A random reviewer mentioning something on a patch (either in a line-by-line critique form or "how about doing it this way instead" counterproposal form) without getting followed up by others (including the original author) is a stall review thread, and it does not change the equation if the random reviewer happens to be me. >> I didn't try it on my end. Maybe it won't help much, because we think >> we're going to use the editor right up until we realize it's not >> committable? > > And I think the answer to that is "s" is used throughout the function in > various ways (in particular, used to print statuses both to stdout and > to the message template) so any wrapping or corralling of scope would > just make things more complicated. In particular, the way Heba did it in > v2 is more unclear - at the time of setting s->hints = 0, it's done You mean "less clear" (just double checking if I got the negation right)? > within a "if (use_editor && include_status)" block, but (as far as I can > tell) the commit message template might also be used when there is no > editor - for example, as input to a hook. And more importantly, when > s->hints is reset to the config, we don't know at that point that the > next status is going to stdout. So I think it's better just to use the > v1 way. Yeah, thanks for going back to compare v1 and v2, and I agree with your assessment. > The second area of discussion I see is in the commit message. Commit > messages have to balance brevity and comprehensiveness, and this can be > a subjective matter, but I think Junio's strikes a good balance. As one side of the comparison is my own, I won't be a good judge on this, but yes I tried to strick a good balance as much as possible. I think I've merged it to 'next' yesterday, but it does not mean that much as we are in -rc and it is not such an urgent "oops we broke it in this cycle, let's fix it" issue. If we see a v3 that improves it, I do not mind at all reverting what I merged to 'next' and use the updated one instead (either way, it will be in 'master' during the next cycle at the earliest). Thanks.
