Hi Chris, 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. > 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". > Sorry if this patch series is long. I can split it into two or more > series if it is prefered. It is preferred. Much. Ciao, Dscho P.S.: Even two ~40-strong patch series are *really* painful to review. I believe you can do a much better job at cutting this monster down into palatable chunks, each with its own sweet little story. -- 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
