Hi Junio, On Wed, 29 Jun 2016, Junio C Hamano wrote: > Johannes Schindelin <johannes.schindelin@xxxxxx> writes: > > > A truly libified function does not die() just for fun. > > The sentence is wasting bits. After all, a helper function in > run-once-and-exit program does not die() just for fun, either. Given that I had a hard time when reviewing Christian's apply patches to drive home the message that it is not okay for library functions to call die() or exit(), I happen to disagree with your notion that this sentence is wasting bits. This sentence does not so much target *you* personally as audience, but the occasional reader of the log who wonders: "Why don't we just call die()? We would not have to worry about passing back the return value through all those long call chains..." > > As such, the recursive merge will convert all die() calls to return -1 > > instead in the next commits, giving the caller a chance at least to > > print some helpful message. > > > > Let's prepare the builtins for this fatal error condition, even if we do > > not really do more than imitating the previous die()'s exit(128): this is > > what callers of e.g. `git merge` have come to expect. > > One thing missing is your design decision and justification. > > For example, the above explanation hints that the original code in > this hunk expected merge_trees() to die() with some useful message > when there is an error condition, but merge_trees() is going to be > improved not to die() itself and return -1 instead to signal an > error, so that the caller can react more flexibly, and this is a > step to prepare for the version of merge_trees() that no longer > dies. Okay, I will replace the "As such..." paragraph with a modified version of your paraphrased explanation. > > - merge_trees(&o, new->commit->tree, work, > > + ret = merge_trees(&o, new->commit->tree, work, > > old->commit->tree, &result); > > + if (ret < 0) > > + exit(128); > > The postimage of the patch tells us that the caller is now > responsible for exiting with status 128, but neither the proposed > log message nor the above hunk tells us where the message the > original code must have given to the end user from die() inside > merge_trees(). The updated caller just exits, so a natural guess is > that the calls to die() have been changed to fprintf(stderr) with > the patch. Even more natural is it to guess that the code will call error(), just like we do almost everywhere else. But you are right, I do not have to leave the reader guessing. Better to err on the side of being slightly verbose than to be so concise that nobody understands what I mean. > But that does not mesh very well with the stated objective of the > patch. The callers want flexibility to do their own error handling, > including giving their own message, so letting merge_trees() to > still write the same message to the standard error stream would not > work well for them. A caller may want to do merge_trees() just to > see if it succeeds, without wanting to give _any_ indication of that > is happening to the user, because it has an alternate/fallback code > if merge_trees() fails, for example (analogy: "am -3" first tries a > straight patch application before fallking back to 3-way merge; it > may not want to show the error from the first attempt). > > The reader _can_ guess that this step ignores the error-message > issue, and improving it later (or keep ignoring that issue) might be > OK in the context of this patch series, but it is necessary to be > upfront to the readers what the design choices were and which one of > those choices the proposed patch adopted as its design for them to > be able to evaluate the patch series correctly. To be honest, I did not even think about the error message issue because my primary concern is to teach the sequencer to perform rebase -i's grunt work. And while we usually suppress the output of the commands in rebase -i, we do show them in case of errors. It will make things more complex, unfortunately, even if it will be straight-forward: there is already a strbuf and a flag in struct merge_options to collect output. The merge_options are simply not passed through to all of the previously die()ing functions yet. I won't have time to get this implemented this week, unfortunately. So please do not expect the next iteration of this patch series before next week. I could imagine that you wanted even more fine-grained control, where we have a range of return values indicating different error conditions. However, I already spent two weeks' worth of work to get this far, and would like to defer that task to the developer who will actually need these fine-grained return values (if we ever will need them). Ciao, Dscho P.S.: For the future, would you mind deleting the quoted remainder of my patches when there are no further comments? I deleted a footer of 73 unnecessary lines in this mail. It's no big deal if this is too tedious, but it would make my life easier. -- 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