Hi Johannes, On Tue, Jun 21, 2016 at 6:34 PM, Johannes Schindelin <johannes.schindelin@xxxxxx> wrote: > - this uncovered a problem with builtin am, where it asked the diff > machinery to close the file stream, but actually called the log_tree > machinery (which might mean that this patch series inadvertently fixes > a bug where `git am --rebasing` would write the commit message to > stdout instead of the `patch` file when erroring out) Please correct me if I'm wrong: looking at log-tree.c, the commit message will not be printed when no_commit_id = 1, isn't it? This is because we do not hit the code paths that write to stdout since show_log() is not called. Also, the return value of log_tree_commit() is actually a boolean value, not an error status value, isn't it? > This last point is a bigger issue, actually. There seem to be quite a > few function calls in builtin/am.c whose return values that might > indicate errors are flatly ignored. I see two calls to run_diff_index() > whose return value then goes poof unchecked, Thanks, future-proofing the builtin/am.c code is good, in case run_diff_index() is updated to not call exit(128) on error in the future. > and several calls to > write_state_text() and write_state_bool() with the same issue. These functions will die() on error, so checking their error code is not really useful. I'm not opposed to changing them to use the write_file_gently() version though, although I don't see a need to unless builtin/am.c is being libified. > And I did > not even try to review the code to that end, all I wanted was to verify > that builtin am only has the close_file issue once (it does use it a > second time, but that one is okay because it then calls > run_diff_index(), i.e. the diff machinery). > > I am embarrassed to admit that these builtin am problems demonstrate > that I, as a mentor of the builtin am project, failed to help make the > patches as good as I expected myself to do. Sorry to disappoint you :-( Regards, Paul -- 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
