On Mon, Aug 24, 2015 at 01:58:04PM -0700, Junio C Hamano wrote: > The workhorse helper function that implements "we have this (short) > body of text; create a new file that contains it" has a "fatal" > parameter, to which 1 was passed by almost all callers, but to > casual readers, it was unclear what that 1 meant. The patch [3/6] > splits it to write_file() and write_file_gently() and drops this > parameter that looks mysterious at the callsites. A common helper > function write_file_v() is introduced to implement these two as thin > wrappers of it. To be honest, I think the "flags" field is more maintainable going forward. Now you have _two_ functions, and any features you add to them have to go in both places. In 4/6 you add the WRITE_FILE_BINARY flag, but I notice that callers can't actually pass it. And adding it into write_file() would take us back to square-one with source compatibility. > The patch [4/6] updates write_file_v() so that it does the "we are > writing a text file. Make sure it does not end with an incomplete > line" logic that [2/6] added only to builtin/am.c, thusly reverting > what was done to builtin/am.c in [2/6]. I notice this also converts "fatal" to "flags". It seemed weird to me that did not go into patch 3, but I guess it is OK (we know that write_file_v has no outstanding callers, since we just added it). > The patch [5/6] stops all callers that creates a single-liner file > using write_file() and write_file_gently() from including the final > LF to the format they pass. This should not change the behaviour, > but it probably makes it conceptually cleaner. You have the contents > to be placed on a single line, and the helper turns the contents > into a proper "line". Nice. > The patch [6/6] drops the final LF from the parameter to create a > multi-line file; while this does not hurt in the sense that the > callee will add a necessary LF back, I do not think it should be > applied. Conceptually, if you have a buffer that contains a bunch > of lines and throw it at a helper to create a file, you'd better > have the terminating LF yourself before asking the helper to put > them in the file. I agree we should drop this one. -Peff -- 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