Sebastian Schuberth <sschuberth@xxxxxxxxx> writes: > On Thu, Jun 27, 2013 at 6:11 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote: > >> I vaguely recall that some people may have argued that git-foo is >> one less exec(2) when we left these in our scripted Porcelains, >> though, so on a platform with poorly performing fork/exec, this >> change may be seen as detrimental. I do not know it matters that >> much. > > But isn't this only true for commands that are not built-ins? I.e., ... Read "I do not know it matters that much." again ;-). >> Git can evolve over time, and if that is really your plan, the first >> step you have to take is to revive the old discussion we had after >> v1.6.0: >> >> http://thread.gmane.org/gmane.comp.version-control.git/93511/focus=93825 >> >> and see if it is now a good idea. It could be in year 2014. It was >> not in 2008. > > Can I get around that discussion by adjusting the reasoning for the > patch to mention consistency? Taking "In principle I have no problem with this change, if nothing other than for consistency reasons." and "I do have a huge issue with the justification in the proposed log message." together, I would reach the same conclusion ;-) >> I cannot apply this exact patch for an obvious and unrelated reason; >> it seems to have changes, e.g. "git am" option help, that do not >> have anything to do with the topic. > > Well, if the topic was consistency the changes would be related, or? The theme of the patch is "Do not call built-in aliases from scripts" (by the way, do not call them "alias"---it is confusing as they are not what people consider "alias"). > diff --git a/git-am.sh b/git-am.sh > index 9f44509..ad67194 100755 > --- a/git-am.sh > +++ b/git-am.sh > @@ -16,8 +16,8 @@ s,signoff add a Signed-off-by line to the commit message > u,utf8 recode into utf8 (default) > k,keep pass -k flag to git-mailinfo > keep-non-patch pass -b flag to git-mailinfo > -keep-cr pass --keep-cr flag to git-mailsplit for mbox format > -no-keep-cr do not pass --keep-cr flag to git-mailsplit > independent of am.keepcr > +keep-cr pass --keep-cr flag to git mailsplit for mbox format > +no-keep-cr do not pass --keep-cr flag to git mailsplit > independent of am.keepcr > c,scissors strip everything before a scissors line > whitespace= pass it through git-apply > ignore-space-change pass it through git-apply > As you were saying yourself, we tell users to prefer the "git foo" > form, so we should also do so in the "git am" option help, IMHO. What does the above change to the options-help have anything to do with that theme? It does not seem to say anything about "git foo" vs "git-foo"? Confused... >> For a future reroll of this patch with only "git-foo -> 'git foo'", >> I would appreciate an independent review by at least one set of >> eyes. It is very easy to blindly do this conversion with sed/perl >> and fail to spot misconversion before sending it out. > > At least the test suite (running on Linux) did not throw any failures > at me after applying this patch. Passing test may be a necessary condition to convince yourself that the patch may not be so broken, but is not sufficient proof of correctness, which can only come from careful code inspection. In any case, thanks for working on this. -- 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