Eric, On Wed, Jun 11, 2014 at 04:09:17AM -0400, Eric Sunshine wrote: > On Mon, Jun 9, 2014 at 6:19 PM, Jeremiah Mahler <jmmahler@xxxxxxxxx> wrote: > > Version 2 of the patch set to add strbuf_set operations. > > > > Includes suggestions from Eric Sunshine [1]: > > > > [...snip...] > > ... > > Food for thought: This patch series has now likely entered the > territory of unnecessary code churn. Quoting from > Documentation/CodingGuidelines: > > Fixing style violations while working on a real change as a > preparatory clean-up step is good, but otherwise avoid useless > code churn for the sake of conforming to the style. > > "Once it _is_ in the tree, it's not really worth the patch noise > to go and fix it up." > Cf. http://article.gmane.org/gmane.linux.kernel/943020 > > Rewriting code merely for the sake of replacing strbuf_reset() + > strbuf_add() with strbuf_set(), while not exactly a style cleanup, > falls into the same camp. (The patch which introduces strbuf_set(), > however, is not churn.) Each change you make can potentially conflict > with other topics already in-flight (see [1], for example), thus > making more work for Junio. > > Also, these sorts of apparently innocuous "mechanical" changes can > have hidden costs [2], so it's useful to weigh carefully how lengthy > you want to make this patch series. > > Having said that, patch 4/19, particularly the changes to > builtin/remote.c:mv(), in which multiple strbufs are reused > repeatedly, does seem to make the code a bit easier to read and likely > reduces cognitive load. > > [1]: http://thread.gmane.org/gmane.comp.version-control.git/245133/focus=245147 > [2]: http://thread.gmane.org/gmane.comp.version-control.git/245133/focus=245144 > I did not realize this "code churn" aspect at the time but it makes sense now. These "mechanical" changes can be more trouble than they are worth. I will try to re-focus my changes to only those that have a substantial benefit. -- Jeremiah Mahler jmmahler@xxxxxxxxx http://github.com/jmahler -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html