Hi Dscho, On Fri, May 13, 2016 at 8:32 AM, Johannes Schindelin <Johannes.Schindelin@xxxxxx> wrote: > 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. I think we should just agree that we disagree on what we think the v1 was doing and move on to v2. >> 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: Documentation/git-am.txt:--quiet:: Documentation/git-am.txt: Be quiet. Only print error messages. Documentation/git-branch.txt:--quiet:: Documentation/git-branch.txt: Be more quiet when creating or deleting a branch, suppressing Documentation/git-branch.txt- non-error messages. Documentation/git-checkout-index.txt:--quiet:: Documentation/git-checkout-index.txt: be quiet if files exist or are not in the index Documentation/git-checkout.txt:--quiet:: Documentation/git-checkout.txt- Quiet, suppress feedback messages. Documentation/git-clean.txt:--quiet:: Documentation/git-clean.txt: Be quiet, only report errors, but not the files that are Documentation/git-clean.txt- successfully removed. Documentation/git-clone.txt--q:: Documentation/git-clone.txt: Operate quietly. Progress is not reported to the standard Documentation/git-clone.txt- error stream. ocumentation/git-commit.txt:--quiet:: Documentation/git-commit.txt- Suppress commit summary message. Documentation/git-fast-import.txt:--quiet:: Documentation/git-fast-import.txt- Disable all non-fatal output, making fast-import silent when it ... 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(). On the contrary the new feature I tentatively called --silent does suppress all output including error messages from error(). 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. >> 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. Thanks, Christian. -- 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