Hi Chris, On Fri, 13 May 2016, Christian Couder wrote: > On Fri, May 13, 2016 at 8:32 AM, Johannes Schindelin > <Johannes.Schindelin@xxxxxx> wrote: > > > > On Wed, 11 May 2016, Christian Couder wrote: > > > >> I consider that the apply functionality is properly libified before > >> these patches, and that they should be in a separate series, but > >> unfortunately using the libified apply in "git am" unmasks the fact > >> that "git am", since it was a shell script, has been silencing the > >> apply functionality by "futzing with file descriptors". And > >> unfortunately this makes some reviewers unhappy. > > > > It is a misrepresentation to claim that this makes some reviewers > > unhappy. Speaking for myself, I am very happy. Despite having had to > > point out that the previous iteration of this patch series had a > > serious flaw. > > > > It is also incorrect to say that the shell script had been "futzing > > with the file descriptors". You see, the shell script's *own* file > > descriptors had been left completely unaffected by the redirection of > > the spawned process' output. That was perfectly fine a thing to do, > > even if it possibly hid fatal errors. Shell scripts are simply very > > limiting. The problem was introduced by v1 of this patch series, which > > changed *the caller's file descriptors* back and forth simply because > > the called code no longer runs in a separate process. And *that* was, > > and is, improper. > > I think we should just agree that we disagree on what we think the v1 > was doing and move on to v2. It is obvious that you want to disagree. But that is counterproductive. It is important to keep in mind, and needs to be remembered in future libification efforts, that extra care is required when we no longer spawn a separate process: you no longer have the option to change global state *just* for the called process. This is very true when redirecting (global) file descriptors. I struggle with this myself in the rebase--helper branch, so it is not only you who has to face, and address, this issue. The same goes for die(), too, of course. And for statically allocated memory. And even worse: when all of a sudden reusing static lockfile structs. And, and, and. The list goes on. It really comes back at us that we originally simple did not care about cleaning up after us "because it is a short-lived process, anyway". The thing is: this is not at all the philosophical discussion as which your comment tries to color it. We *have* to address these issues when libifying code. Yes, it's hard. Yes, it's tedious. Yes, I also want to bitch about it. And yes, it must be done. > >> By the way there are no tests yet for this new feature, and I am not > >> sure at all that "--silent" and "be_silent" are good names. > > > > If you want to follow existing code's example, we typically call this > > option "quiet". > > In the documentation there is: [... snip ...] > ... > > So it looks to me that --quiet means something like "don't tell the > story of your life, but in case of problem you are allowed to > complain". In other word --quiet generally doesn't suppress error > messages from error() or die(). Right. And if you care to take a step back, this is most likely what we want in libified code. Modulo die(), of course! > On the contrary the new feature I tentatively called --silent does > suppress all output including error messages from error(). And what would be the point of that? Now that we are libifying code, in contrast to the spawned-process approach, we *can* discern between "prints to stderr" and "displays an error". I'd wager that you won't find any error() call in the code path that we want to silence. > Now if people think that it is not worth making a difference between the > different behaviors, then I am ok to rename it --quiet, though I wonder > what will happen if people later want a --quiet that does only what > --quiet usually does to the other commands. You know, I get the distinct impression that you do not want my feedback because you always want "people" to agree with my assessment. I hoped that my arguments would make sense, but I guess I should spend my time differently. > >> Sorry if this patch series is long. I can split it into two or more > >> series if it is prefered. > > > > It is preferred. Much. > > Ok, I will split it then. Thank you. Maybe you take me off of the Cc: list, too? Thanks, Dscho -- 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