Re: [PATCH] Do not call built-in aliases from scripts
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Jun 27, 2013, 18:52 UTC
- Message-ID
- <alpine.DEB.1.00.1306272030291.28957@s15462909.onlinehome-server.info>
- In-Reply-To
- <CAHGBnuNUjaWH2UDsa6jGjf32M+b8-iezw4pKXR985mROFSLOKQ@mail.gmail.com>
Hi Sebastian,
On Thu, 27 Jun 2013, Sebastian Schuberth wrote:
Show 5 quoted lines
> Call built-in commands via the main executable (non-dashed form) without > relying on the aliases (dashed form) to be present. On some platforms, > e.g. those that do not properly support file system links, it is > inconvenient to ship the built-in aliases, so do not depend on their > presence.
A laudable goal!
Show 11 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
That looks to me as if an overly-long line in the diff (not your fault) was wrapped in the mailer...
> +keep-cr pass --keep-cr flag to git mailsplit for mbox format
At first, I wondered what changed in this line. But then I realized that you separated "git-mailsplit" into "git mailsplit". This is purely nitpicking and I am almost ashamed to do so, but I think it might be *slightly* easier to read if the "git mailsplit" was quoted.
Having said that, I think it is an important change.
It is a different change, philosophically, though, from changes like this one:
Show 6 quoted lines
> @@ -174,7 +174,7 @@ It does not apply to blobs recorded in its index.")"
> then
> GIT_MERGE_VERBOSITY=0 && export GIT_MERGE_VERBOSITY
> fi
> - git-merge-recursive $orig_tree -- HEAD $his_tree || {
> + git merge-recursive $orig_tree -- HEAD $his_tree || {This change is code while the former change is documentation. It might be prudent to split this commit into two, one for calls in scripts, one for documentation. That way, we could carry the documentation change (which I whole-heartedly agree with) in Git for Windows should upstream stop before the fence.
> diff --git a/git-archimport.perl b/git-archimport.perl
This file's diff would be less long if the code was a bit more DRY. But your changes are sound.
Show 10 quoted lines
> diff --git a/git-cvsexportcommit.perl b/git-cvsexportcommit.perl > index d13f02d..6718bad 100755 > --- a/git-cvsexportcommit.perl > +++ b/git-cvsexportcommit.perl > @@ -18,7 +18,7 @@ $opt_h && usage(); > > die "Need at least one commit identifier!" unless @ARGV; > > -# Get git-config settings > +# Get git config settings
This is not as clear-cut as the changes above. I would tend to call it a "documentation" change, though.
Show 16 quoted lines
> diff --git a/git-cvsserver.perl b/git-cvsserver.perl
> index a0d796e..53c136f 100755
> --- a/git-cvsserver.perl
> +++ b/git-cvsserver.perl
> @@ -358,10 +358,10 @@ sub req_Root
>
> my @gitvars = `git config -l`;
> if ($?) {
> - print "E problems executing git-config on the server -- this
> is not a git repository or the PATH is not set correctly.\n";
> + print "E problems executing git config on the server -- this
> is not a git repository or the PATH is not set correctly.\n";
> print "E \n";
> print "error 1 - problem executing git-config\n";
> - return 0;
> + return 0;Please don't. I know, it is a whitespace error. But it makes reviewing more tedious...
Show 10 quoted lines
> @@ -3936,14 +3936,14 @@ sub update
>
> if ( defined ( $lastpicked ) )
> {
> - my $filepipe = open(FILELIST, '-|', 'git', 'diff-tree',
> '-z', '-r', $lastpicked, $commit->{hash}) or die("Cannot call
> git-diff-tree : $!");
> + my $filepipe = open(FILELIST, '-|', 'git', 'diff-tree',
> '-z', '-r', $lastpicked, $commit->{hash}) or die("Cannot call git
> diff-tree : $!");Likewise, this would be a documentation change. It is funny that the open() did not require a change: apparently, your intended code fixes were already started at some point, but not finished.
Show 10 quoted lines
> diff --git a/git-merge-octopus.sh b/git-merge-octopus.sh > index 8643f74..ec1d65b 100755 > --- a/git-merge-octopus.sh > +++ b/git-merge-octopus.sh > @@ -97,7 +97,7 @@ do > if test $? -ne 0 > then > echo "Simple merge did not work, trying automatic merge." > - git-merge-index -o git-merge-one-file -a || > + git merge-index -o git-merge-one-file -a ||
This is a problem. 'git-merge-one-file' cannot be split here AFAICT.
Of course, we could teach merge-index to read *two* parameters instead of one when it encounters "git" as the <merge-program>. But that would be as hacky as the whole dashed-form business to begin with.
Show 10 quoted lines
> diff --git a/git-merge-resolve.sh b/git-merge-resolve.sh > index c9da747..343fe7b 100755 > --- a/git-merge-resolve.sh > +++ b/git-merge-resolve.sh > @@ -45,7 +45,7 @@ then > exit 0 > else > echo "Simple merge failed, trying Automatic merge." > - if git-merge-index -o git-merge-one-file -a > + if git merge-index -o git-merge-one-file -a
As above, with -octopus.
Apart from the split and the merge-one-file problem, absolutely no objections from my side, but appraisals!
Ciao, Dscho
-- -- *** 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.