Re: [PATCH] Do not call built-in aliases from scripts

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 27, 2013, 17:27 UTC
Message-ID: <7v8v1vtqfa.fsf@alter.siamese.dyndns.org>
In-Reply-To: <CAHGBnuPc+vDwejFOF_o+jr+L_mgrVo=Uxs2vC6C-Feg_uxWPpA@mail.gmail.com>

Sebastian Schuberth <sschuberth@gmail.com> writes:

Show 9 quoted lines

> On Thu, Jun 27, 2013 at 6:11 PM, Junio C Hamano <gitster@pobox.com> 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 ;-).

Show 11 quoted lines

>> 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 ;-)

Show 5 quoted lines

>> 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").

Show 17 quoted lines

> 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...

Show 7 quoted lines

>> 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.

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.