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

2013-06-28 Thread Sebastian Schuberth 
On Thu, Jun 27, 2013 at 8:10 PM, Junio C Hamano gits...@pobox.com wrote:

  Now that I look at it more, I see that
 `git-mailinfo` was missed and there's a `git-apply` towards the
 bottom.  So I'm not sure it's helping the consistency argument.

 Hmph, true.

 Having said that, I'd still prefer to see documentation changes in a
 patch separate from a do not call git-foo form patch.

I'll send a new version of the patch next week to address this and
also use quotes when replacing a hyphenated form in prose.

-- 
Sebastian Schuberth
--
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

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

2013-06-28 Thread Sebastian Schuberth 
On Thu, Jun 27, 2013 at 8:52 PM, Johannes Schindelin
johannes.schinde...@gmx.de wrote:

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

I agree to all of your comments except this one: I did not split
'git-merge-one-file' here ...

 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.

Sorry, I can't follow you here.

-- 
Sebastian Schuberth
--
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

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

2013-06-28 Thread Johannes Schindelin 
Hi Sebastian,

On Fri, 28 Jun 2013, Sebastian Schuberth wrote:

 On Thu, Jun 27, 2013 at 8:52 PM, Johannes Schindelin
 johannes.schinde...@gmx.de wrote:
 
  --- 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.
 
 I agree to all of your comments except this one: I did not split
 'git-merge-one-file' here ...

I know. That is what I pointed out. git-merge-one-file was *not*
un-dashed. And I explained the reason, too.

But if we really want to solve the problem you described earlier, we also
must not rely on git-merge-one-file to be present in libexec.

Ciao,
Dscho
--
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

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

2013-06-28 Thread Junio C Hamano 
Sebastian Schuberth sschube...@gmail.com writes:

 On Thu, Jun 27, 2013 at 8:52 PM, Johannes Schindelin
 johannes.schinde...@gmx.de wrote:

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

 I agree to all of your comments except this one: I did not split
 'git-merge-one-file' here ...

I do not think Dscho was pointing out any problem with your patch.

He is merely pointing out that the goal of No git-foo anywhere on
the filesystem is an unworkable one, as sometimes you need to give
the path of a specific binary to commands.  It is not limited to the
-o option of git merge-index, but the above is a good example.

And I agree with Dscho that this is not a problem with your patch
per-se.

   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.

 Sorry, I can't follow you here.

The same issue above, that you have to have git-merge-one-file
somewhere in your filesystem on $GIT_EXEC_PATH, he pointed out when
he commented on the patch to git-merge-octopus.sh, can be seen here.
--
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

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

2013-06-27 Thread Sebastian Schuberth 
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.

Signed-off-by: Sebastian Schuberth sschube...@gmail.com
---
 git-am.sh|  6 ++---
 git-archimport.perl  | 68 
 git-cvsexportcommit.perl | 18 ++---
 git-cvsserver.perl   | 50 +--
 git-merge-octopus.sh |  2 +-
 git-merge-one-file.sh|  8 +++---
 git-merge-resolve.sh |  2 +-
 git-parse-remote.sh  |  2 +-
 git-pull.sh  |  2 +-
 git-stash.sh |  2 +-
 git-submodule.sh |  8 +++---
 11 files changed, 84 insertions(+), 84 deletions(-)

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
@@ -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 || {
git rerere $allow_rerere_autoupdate
die $(gettext Failed to merge in the changes.)
 }
diff --git a/git-archimport.perl b/git-archimport.perl
index 9cb123a..ed2c741 100755
--- a/git-archimport.perl
+++ b/git-archimport.perl
@@ -343,10 +343,10 @@ sub process_patchset_accurate {

 # switch to that branch if we're not already in that branch:
 if (-e $git_dir/refs/heads/$ps-{branch}) {
-   system('git-checkout','-f',$ps-{branch}) == 0 or die $! $?\n;
+   system('git','checkout','-f',$ps-{branch}) == 0 or die $! $?\n;

# remove any old stuff that got leftover:
-   my $rm = safe_pipe_capture('git-ls-files','--others','-z');
+   my $rm = safe_pipe_capture('git','ls-files','--others','-z');
rmtree(split(/\0/,$rm)) if $rm;
 }

@@ -367,7 +367,7 @@ sub process_patchset_accurate {

 # find where we are supposed to branch from
if (! -e $git_dir/refs/heads/$ps-{branch}) {
-   system('git-branch',$ps-{branch},$branchpoint) == 0 or die $! 
$?\n;
+   system('git','branch',$ps-{branch},$branchpoint) == 0 or die 
$! $?\n;

# We trust Arch with the fact that this is just a tag,
# and it does not affect the state of the tree, so
@@ -378,10 +378,10 @@ sub process_patchset_accurate {
ptag($ps-{id}, $branchpoint);
print  * Tagged $ps-{id} at $branchpoint\n;
}
-   system('git-checkout','-f',$ps-{branch}) == 0 or die $! $?\n;
+   system('git','checkout','-f',$ps-{branch}) == 0 or die $! $?\n;

 # remove any old stuff that got leftover:
-my $rm = safe_pipe_capture('git-ls-files','--others','-z');
+my $rm = safe_pipe_capture('git','ls-files','--others','-z');
 rmtree(split(/\0/,$rm)) if $rm;
 return 0;
 } else {
@@ -392,10 +392,10 @@ sub process_patchset_accurate {
 }

 # update the index with all the changes we got
-system('git-diff-files --name-only -z | '.
-'git-update-index --remove -z --stdin') == 0 or die $! $?\n;
-system('git-ls-files --others -z | '.
-'git-update-index --add -z --stdin') == 0 or die $! $?\n;
+system('git diff-files --name-only -z | '.
+'git update-index --remove -z --stdin') == 0 or die $! $?\n;
+system('git ls-files --others -z | '.
+'git update-index --add -z --stdin') == 0 or die $! $?\n;
 return 1;
 }

@@ -413,7 +413,7 @@ sub process_patchset_fast {
 unless ($import) { # skip for import
 if ( -e $git_dir/refs/heads/$ps-{branch}) {
 # we know about this branch
-system('git-checkout',$ps-{branch});
+system('git','checkout',$ps-{branch});
 } else {
 # new branch! we need to verify a few things
 die Branch on a non-tag! unless $ps-{type} eq 't';
@@ -423,7 +423,7 @@ sub process_patchset_fast {

 # find where we are supposed to branch from
if (! -e

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

2013-06-27 Thread Junio C Hamano 
Sebastian Schuberth sschube...@gmail.com writes:

 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.

In principle I have no problem with this change, if nothing other
than for consistency reasons.  We tell users to write git foo, and
we tell users to say PATH=$(git --exec-path):$PATH if they want to
write git-foo, but we do tell them we prefer 'git foo' form.  We
should do so ourselves where it is reasonable.

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.

Having said all that, I do have a huge issue with the justification
in the proposed log message.  If your plan is to make this:

$(git --exec-path)/git-ls-files

not to work with your port of Git, that implementation is _broken_
and is not a Git anymore.

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.

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.

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.


 Signed-off-by: Sebastian Schuberth sschube...@gmail.com
 ---
  git-am.sh|  6 ++---
  git-archimport.perl  | 68 
 
  git-cvsexportcommit.perl | 18 ++---
  git-cvsserver.perl   | 50 +--
  git-merge-octopus.sh |  2 +-
  git-merge-one-file.sh|  8 +++---
  git-merge-resolve.sh |  2 +-
  git-parse-remote.sh  |  2 +-
  git-pull.sh  |  2 +-
  git-stash.sh |  2 +-
  git-submodule.sh |  8 +++---
  11 files changed, 84 insertions(+), 84 deletions(-)

 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
 @@ -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 || {
   git rerere $allow_rerere_autoupdate
   die $(gettext Failed to merge in the changes.)
  }
 diff --git a/git-archimport.perl b/git-archimport.perl
 index 9cb123a..ed2c741 100755
 --- a/git-archimport.perl
 +++ b/git-archimport.perl
 @@ -343,10 +343,10 @@ sub process_patchset_accurate {

  # switch to that branch if we're not already in that branch:
  if (-e $git_dir/refs/heads/$ps-{branch}) {
 -   system('git-checkout','-f',$ps-{branch}) == 0 or die $! $?\n;
 +   system('git','checkout','-f',$ps-{branch}) == 0 or die $! $?\n;

 # remove any old stuff that got leftover:
 -   my $rm = safe_pipe_capture('git-ls-files','--others','-z');
 +   my $rm = safe_pipe_capture('git','ls-files','--others','-z');
 rmtree(split(/\0/,$rm)) if $rm;
  }

 @@ -367,7 +367,7 @@ sub process_patchset_accurate {

  # find where we are supposed to branch from
   if (! -e $git_dir/refs/heads/$ps-{branch}) {
 - system('git-branch',$ps-{branch},$branchpoint) == 0 or die $! 
 $?\n;
 + system('git','branch',$ps-{branch},$branchpoint) == 0 or die 
 $! $?\n;

   # We trust Arch with the fact that this is just a tag,
   # and it does not affect the state of the tree, so
 @@ -378,10 +378,10 @@ sub process_patchset_accurate {
   ptag($ps-{id}, $branchpoint);
   print  *

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

2013-06-27 Thread Sebastian Schuberth 
On Thu, Jun 27, 2013 at 6:11 PM, Junio C Hamano gits...@pobox.com wrote:

 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.

 In principle I have no problem with this change, if nothing other
 than for consistency reasons.  We tell users to write git foo, and
 we tell users to say PATH=$(git --exec-path):$PATH if they want to
 write git-foo, but we do tell them we prefer 'git foo' form.  We
 should do so ourselves where it is reasonable.

Consistency was indeed one of my intentions, though maybe not the primary one.

 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., I
can see how calling git-pull from a script is more efficient than
calling git pull from the same script, because git pull would
first execute git which in turn would spawn a shell to run the
git-pull script. But calling e.g. git-merge and git merge should
use almost the same code path in the git executable and not
fork/exec at all because the merge command is built in, right?

 Having said all that, I do have a huge issue with the justification
 in the proposed log message.  If your plan is to make this:

 $(git --exec-path)/git-ls-files

 not to work with your port of Git, that implementation is _broken_
 and is not a Git anymore.

I would at least like to have that option, yes, well knowing that such
a port would not be considered to be a Git anymore. A typical use-case
would be a portable Git for Windows, which is surprisingly popular.
Such a portable version usually just ships as an archive without any
kind of installer. The ZIP archive format on Windows does not support
storing any kind of file system links, which means all executables for
built-ins would need to be added as copies to the archive. While that
does not add much to the archive size because all such files are equal
to git.exe and thus compress well, the big surprise comes when
extracting that archive as all the copies of git.exe make such a
portable installation unnecessary big. On Windows, git.exe is about
1,34 MiB in size, which sums up to about 146 MiB of extra storage
required if you have copies for all the built-ins instead of file
system links.

 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?

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

 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.

-- 
Sebastian Schuberth
--
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

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

2013-06-27 Thread Junio C Hamano 
Sebastian Schuberth sschube...@gmail.com writes:

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

 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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-06-27 Thread Junio C Hamano 
John Szakmeister j...@szakmeister.net writes:

 On Thu, Jun 27, 2013 at 1:27 PM, Junio C Hamano gits...@pobox.com wrote:
 [snip]
 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?

 I initially missed it too, but `git-mailsplit` changed to `git
 mailsplit` in the help.

Ahh, OK.

  Now that I look at it more, I see that
 `git-mailinfo` was missed and there's a `git-apply` towards the
 bottom.  So I'm not sure it's helping the consistency argument.

Hmph, true.
--
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

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

2013-06-27 Thread John Keeping 
On Thu, Jun 27, 2013 at 11:05:09AM -0700, Junio C Hamano wrote:
 John Szakmeister j...@szakmeister.net writes:
 
  On Thu, Jun 27, 2013 at 1:27 PM, Junio C Hamano gits...@pobox.com wrote:
  [snip]
  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?
 
  I initially missed it too, but `git-mailsplit` changed to `git
  mailsplit` in the help.
 
 Ahh, OK.

That is rendered differently though, I don't think just having the plain
text git command is as useful.  It should either use the hyphenated form
or enclose the text in quotes.
--
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

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

2013-06-27 Thread Johannes Schindelin 
Hi Sebastian,

On Thu, 27 Jun 2013, Sebastian Schuberth wrote:

 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!

 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:

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

 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.

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

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

 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.

 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

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

2013-06-27 Thread Johannes Sixt 
Am 27.06.2013 14:47, schrieb Sebastian Schuberth:
 diff --git a/git-archimport.perl b/git-archimport.perl
 index 9cb123a..ed2c741 100755
 --- a/git-archimport.perl
 +++ b/git-archimport.perl
...
 @@ -477,8 +477,8 @@ sub process_patchset_fast {
  unlink @$del;
  while (@$del) {
  my @slice = splice(@$del, 0, 100);
 -system('git-update-index','--remove','--',@slice) == 0 or
 -die Error in git-update-index --remove: $! 
 $?\n;
 +system('git update-index','--remove','--',@slice) == 0 or
 +die Error in git update-index --remove: $! 
 $?\n;

Shouldn't this become 'git','update-index'?

  }
  }
 
 @@ -496,25 +496,25 @@ sub process_patchset_fast {
  }
  # print moving $from $to;
  rename($from, $to) or die Error renaming '$from' '$to': $!\n;
 -system('git-update-index','--remove','--',$from) == 0 or
 -die Error in git-update-index --remove: $! 
 $?\n;
 -system('git-update-index','--add','--',$to) == 0 or
 -die Error in git-update-index --add: $! $?\n;
 +system('git update-index','--remove','--',$from) == 0 or
 +die Error in git update-index --remove: $! 
 $?\n;
 +system('git update-index','--add','--',$to) == 0 or
 +die Error in git update-index --add: $! $?\n;

Twice here, too.

  }
  }
 
  if (my $add = $ps-{new_files}) {
  while (@$add) {
  my @slice = splice(@$add, 0, 100);
 -system('git-update-index','--add','--',@slice) == 0 or
 -die Error in git-update-index --add: $! $?\n;
 +system('git update-index','--add','--',@slice) == 0 or
 +die Error in git update-index --add: $! $?\n;

Again.

  }
  }
 
  if (my $mod = $ps-{modified_files}) {
  while (@$mod) {
  my @slice = splice(@$mod, 0, 100);
 -system('git-update-index','--',@slice) == 0 or
 +system('git update-index','--',@slice) == 0 or

Ditto.

-- Hannes

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