[PATCH 0/5] am state file fix with write_file() clean-up
So here is an solution based on the write_file() is primarily to produce text, so it should be able to correct the incomplete line at the end approach. The first one is Peff's idea to consolidate callers in am, in a more concrete form. The second is the fix to $gmane/276238. The remainder is to clean up write_file() helper function. All callers except for two were passing 1 as one parameter, whose meaning was not all obvious to a casual reader. In patch 3/5, we flip the default behaviour of write_file() to die upon error unless explicitly asked not to with WRITE_FILE_GENTLY flag, and change the two oddball callers to pass this new flag. In patch 4/5, we enhance the default behaviour of write_file() to complete an incomplete line at the end, unless asked not to with WRITE_FILE_BINARY flag; nobody passes this because all existing callers want to produce a text file. In patch 5/5, the transitional noise left by patches 3 and 4 are cleaned up by updating the non-binary callers not to add LF themselves and by changing the callers that pass 1 as flags parameter to pass 0 (as bit (10) is a no-op since patch 3/5). The series is built on top of b5e8235, the current tip of the pt/am-builtin-options topic. Junio C Hamano (5): builtin/am: introduce write_state_*() helper functions builtin/am: make sure state files are text write_file(): introduce an explicit WRITE_FILE_GENTLY request write_file(): do not leave incomplete line at the end write_file(): clean up transitional mess of flag words and terminating LF builtin/am.c | 68 -- builtin/init-db.c | 2 +- builtin/worktree.c | 10 cache.h| 16 - daemon.c | 2 +- setup.c| 2 +- submodule.c| 2 +- transport.c| 2 +- wrapper.c | 13 +-- 9 files changed, 77 insertions(+), 40 deletions(-) -- 2.5.0-568-g53a3e28 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/5] am state file fix with write_file() clean-up
On Mon, Aug 24, 2015 at 10:09:41AM -0700, Junio C Hamano wrote: So here is an solution based on the write_file() is primarily to produce text, so it should be able to correct the incomplete line at the end approach. This all looks good to me. The topics-in-flight compatibility stuff in patches 3 and 5 is neatly done. Usually I would just cheat and change the order of arguments to make the compiler notice such problems, but that's hard to do here because of the varargs (you cannot just bump flags to the end). -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/5] am state file fix with write_file() clean-up
On Mon, Aug 24, 2015 at 11:15:55AM -0700, Junio C Hamano wrote: This all looks good to me. The topics-in-flight compatibility stuff in patches 3 and 5 is neatly done. Usually I would just cheat and change the order of arguments to make the compiler notice such problems, but that's hard to do here because of the varargs (you cannot just bump flags to the end). Actually, I think my compatibility stuff is worthless. It would not catch new callers that wants to only probe and do their own error handling by passing 0 (and besides, assert() is a shoddy way to do this---there is no guarantee that tests will trigger all the codepaths in the first place). Oh, hrm, you're right. I was focused on making sure the common 1-passers were not broken, but patch 3 does break 0-passers (obviously, because they needed updated in the same patch ;) ). And I do agree that build-time assertions are much better than run-time ones. We should deprecate and remove write_file() by renaming the one with the updated semantics to something else, possibly with a backward compatiblity thin wrapper around it that is called write_file(), or without it to force a link-time error. That sounds reasonable. Maybe format_to_file or something? -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/5] am state file fix with write_file() clean-up
Jeff King p...@peff.net writes: On Mon, Aug 24, 2015 at 10:09:41AM -0700, Junio C Hamano wrote: So here is an solution based on the write_file() is primarily to produce text, so it should be able to correct the incomplete line at the end approach. This all looks good to me. The topics-in-flight compatibility stuff in patches 3 and 5 is neatly done. Usually I would just cheat and change the order of arguments to make the compiler notice such problems, but that's hard to do here because of the varargs (you cannot just bump flags to the end). Actually, I think my compatibility stuff is worthless. It would not catch new callers that wants to only probe and do their own error handling by passing 0 (and besides, assert() is a shoddy way to do this---there is no guarantee that tests will trigger all the codepaths in the first place). We should deprecate and remove write_file() by renaming the one with the updated semantics to something else, possibly with a backward compatiblity thin wrapper around it that is called write_file(), or without it to force a link-time error. Thanks for a dose of sanity. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/5] am state file fix with write_file() clean-up
Jeff King p...@peff.net writes: On Mon, Aug 24, 2015 at 11:15:55AM -0700, Junio C Hamano wrote: This all looks good to me. The topics-in-flight compatibility stuff in patches 3 and 5 is neatly done. Usually I would just cheat and change the order of arguments to make the compiler notice such problems, but that's hard to do here because of the varargs (you cannot just bump flags to the end). Actually, I think my compatibility stuff is worthless. It would not catch new callers that wants to only probe and do their own error handling by passing 0 (and besides, assert() is a shoddy way to do this---there is no guarantee that tests will trigger all the codepaths in the first place). Oh, hrm, you're right. I was focused on making sure the common 1-passers were not broken, but patch 3 does break 0-passers (obviously, because they needed updated in the same patch ;) ). And I do agree that build-time assertions are much better than run-time ones. We should deprecate and remove write_file() by renaming the one with the updated semantics to something else, possibly with a backward compatiblity thin wrapper around it that is called write_file(), or without it to force a link-time error. That sounds reasonable. Maybe format_to_file or something? I am going into a slightly different tangent. Binary support is not something we need right now, so I'll keep the door open for that in the future by - drop the int fatal altogether from write_file() without adding unsigned flags; - add write_file_gently(), again without unsigned flags; - make them call write_file_v() that takes unsigned flags and va_list. I earlier said there were 2 oddball callers, one of them being suspicious, but it turns out that there are 3 callers that want write_file_gently(). Only the one in the setup codepath is asking for fatal=0 while discarding the error return, which is suspicious; other two are handling an error themselves and are OK. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
