[PATCH 00/10] Misc. cvsserver, dashless, and test suite patches

[PATCH 00/10] Misc. cvsserver, dashless, and test suite patches

[Matthew Ogilvie]

This series of small patches fixes a few minor issues I noticed
while researching what it would take to for cvsserver to support
cvs's "-r" flags properly.  No real progress on the "-r" front,
but see comments in the "cvsserver: add comments about database ..."
email.

The "run test suite without dashed..." patch has wide scope,
even though is is fairly small.  It (along with fixing the things it
found in t2300 and t3409) was inspired by a desire to avoid future
regressions of the bug fixed in "cvsserver: run dashless 'git commands'"
patch, where git-cvsserver was trying to run dashed commands that
are not in the default PATH...

As this patch series was going to press, I noticed that
the "run test suite without dashed..." patch probably conflicts with
recently posted valgrind test suite patches.  See extra comments in
the email for possible resolutions.

The other patches in this series are mostly independent of each other.

When working on the "git-shell" patch, I noticed a couple of other
things related to "git-shell" that maybe ought to be changed (change
the argc==2 special case, and document the best way(s) to
configure an account to use git-shell).  See also extra information in
the email.

--
Matthew Ogilvie   [mmogilvi_git@miniinfo.net]

Matthew Ogilvie (10):
  cvsserver: removed unused sha1Or-k mode from kopts_from_path
  cvsserver: add comments about database schema/usage
  cvsserver: remove unused functions _headrev and gethistory
  git-shell: allow running git-cvsserver, not just cvs
  cvsserver: run dashless "git command"s to access plumbing
  t2300: use documented technique to invoke git-sh-setup
  t3409: use dashless "git commit" instead of "git-commit"
  run test suite without dashed git-commands in PATH
  Revert "adapt git-cvsserver manpage to dash-free syntax"
  cvsserver doc: emphasize using CVS_SERVER= phrase within CVSROOT

 .gitignore                        |    1 +
 Documentation/git-cvsserver.txt   |   29 +++++--
 Documentation/git-shell.txt       |    5 +-
 Makefile                          |   40 +++++++---
 git-cvsserver.perl                |  163 ++++++++++++++++---------------------
 shell.c                           |    1 +
 t/t2300-cd-to-toplevel.sh         |    2 +-
 t/t3409-rebase-preserve-merges.sh |    6 +-
 t/t9400-git-cvsserver-server.sh   |   88 ++++++++++++++++++++
 t/test-lib.sh                     |   14 +++-
 test-bin-wrapper.sh               |   12 +++
 11 files changed, 246 insertions(+), 115 deletions(-)
 create mode 100644 test-bin-wrapper.sh

[PATCH 01/10] cvsserver: removed unused sha1Or-k mode from kopts_from_path

[Matthew Ogilvie]

sha1Or-k was a vestige from an early, never-released
attempt to handle some oddball cases of CRLF conversion (-k option).
Ultimately it wasn't needed, and I should have gotten rid of it
before submitting the CRLF patch in the first place.

See also 90948a42892779 (add ability to guess -kb from contents).

Signed-off-by: Matthew Ogilvie <mmogilvi_git@miniinfo.net>
---
 git-cvsserver.perl |   38 +++++---------------------------------
 1 files changed, 5 insertions(+), 33 deletions(-)

diff --git a/git-cvsserver.perl b/git-cvsserver.perl
index fef7faf..1de0c1e 100755
--- a/git-cvsserver.perl
+++ b/git-cvsserver.perl
@@ -2355,42 +2355,14 @@ sub kopts_from_path
         }
         elsif( ($cfg->{gitcvs}{allbinary} =~ /^\s*guess\s*$/i) )
         {
-            if( $srcType eq "sha1Or-k" &&
-                !defined($name) )
+            if( is_binary($srcType,$name) )
             {
-                my ($ret)=$state->{entries}{$path}{options};
-                if( !defined($ret) )
-                {
-                    $ret=$state->{opt}{k};
-                    if(defined($ret))
-                    {
-                        $ret="-k$ret";
-                    }
-                    else
-                    {
-                        $ret="";
-                    }
-                }
-                if( ! ($ret=~/^(|-kb|-kkv|-kkvl|-kk|-ko|-kv)$/) )
-                {
-                    print "E Bad -k option\n";
-                    $log->warn("Bad -k option: $ret");
-                    die "Error: Bad -k option: $ret\n";
-                }
-
-                return $ret;
+                $log->debug("... as binary");
+                return "-kb";
             }
             else
             {
-                if( is_binary($srcType,$name) )
-                {
-                    $log->debug("... as binary");
-                    return "-kb";
-                }
-                else
-                {
-                    $log->debug("... as text");
-                }
+                $log->debug("... as text");
             }
         }
     }
@@ -2497,7 +2469,7 @@ sub open_blob_or_die
             die "Unable to open file $name: $!\n";
         }
     }
-    elsif( $srcType eq "sha1" || $srcType eq "sha1Or-k" )
+    elsif( $srcType eq "sha1" )
     {
         unless ( defined ( $name ) and $name =~ /^[a-zA-Z0-9]{40}$/ )
         {
-- 
1.6.1.81.g9833d.dirty

[PATCH 02/10] cvsserver: add comments about database schema/usage

[Matthew Ogilvie]

No functionality changes, but these comments should make it easier to
understand how it works.

Signed-off-by: Matthew Ogilvie <mmogilvi_git@miniinfo.net>
---

This is the result of some initial research into what it would
take to support switching branches/tags/commithashes for the entire
sandbox using "cvs update -r TAG".  This seems like the one remaining
big missing feature that makes git-cvsserver a substantial downgrade
compared to a real CVS server.

git-cvsserver's support for cvs's "-r" arguments is kind of hit
or miss:
   - Some commands ignore it completely.  This includes "checkout",
     but checkout does have a kind of workaround: you can checkout
     any branch by specifying "cvs checkout branch".  (Normally
     it is "cvs checkout module".)  But that sandbox is then locked
     to that branch, you can't switch to another.
   - Some commands (e.g. update) allow you to specify CVS-style
     version numbers with -r, but that is generally only useful if
     you are examining a single file.
   - Some of those commands also recognize git commit hashes.  But
     due to the internal database model, the commithash form will only
     work for files that were actually modified in the requested
     commit, and for which cvsserver has previously decided that
     the commit is on the same branch that the sandbox
     is locked to (it has already assigned CVS revision numbers
     for it).  Other commands (including update) do not recognize
     the commithash form at all (ignores it if it doesn't look like
     a CVS version number, even though the underlying getmeta()
     function is more a little more general).

Even the relatively simple goal of being able to update to an
older version on the same branch involves some significant costs:
   - Perhaps store O(numCommits*totalNumFiles) entries in the
     revision table, instead of the current more reasonable
     O(numCommits*averageNumChangedFilesPerCommit).  Perhaps
     this could be combined with a significant schema change that
     involves assigning short ints to distinct commithash
     values and distinct filenames, looking up author/mode
     information directly in git on demand, and just storing the
     short ints in the revisions table to shrink each row
     as much as possible.
   - Fork off extra git commands to find the most recent
     commit (relative to the supplied commit) that modified each
     file, then look that up in the DB.  For many files, it
     is likely to have to search quite a ways, and if it the search
     is done with a separate command per file, it could
     get expensive.  Also watch out for "other"
     branches that won't be in the DB.  Sounds like a lot
     of extra CPU usage re-parsing commit history for every
     file...

Thoughts about full branch support in cvsserver:
   - One way would be to add a "branch" column to the "revisions"
     and "headers" tables.  Also add a branch table (details TBD)?
   - Alternatively, drop the DB and see if maybe we could store
     file revision number information using git's core data structures:
     "refs/cvsfileversions/{branch}", with a tree structure for storing
     file revision number to commithash/path mappings that is
     specifically designed to use the strengths of the DAG
     representation and pack files.
   - I have vague notions of other techniques as well.
   - Recognise that names of branches may change as heads advance
     and things get merged, but the CVS branch /number/ needs
     to remain consistent.
   - Recognize that in general you can not reliably regenerate
     the DB.  Depending on what sequence of git-cvsserver and
     git-merge commands, versions of files can end up with
     different CVS revision numbers.
   - Has anyone thought about implementing server emulators for
     other version control systems (git-svnserver and similar)?

--
Matthew Ogilvie   [mmogilvi_git@miniinfo.net]

 git-cvsserver.perl |   46 ++++++++++++++++++++++++++++++++++++++++++----
 1 files changed, 42 insertions(+), 4 deletions(-)

diff --git a/git-cvsserver.perl b/git-cvsserver.perl
index 1de0c1e..f7891b8 100755
--- a/git-cvsserver.perl
+++ b/git-cvsserver.perl
@@ -2045,6 +2045,10 @@ sub statecleanup
     $state->{entries} = {};
 }
 
+# Return working directory revision int "X" from CVS revision "1.X" out
+# of the the working directory "entries" state, for the given filename.
+# Return negative "X" to represent the file is scheduled for removal
+# when it is committed.
 sub revparse
 {
     my $filename = shift;
@@ -2775,6 +2779,16 @@ sub new
     }
 
     # Construct the revision table if required
+    # The revision table stores an entry for each file, each time that file
+    # changes.
+    #   numberOfRecords = O( numCommits * averageNumChangedFilesPerCommit )
+    # This is not sufficient to support "-r {commithash}" for any
+    # files except files that were modified by that commit (also,
+    # some places in the code ignore/effectively strip out -r in
+    # some cases, before it gets passed to getmeta()).
+    # The "filehash" field typically has a git blob hash, but can also
+    # be set to "dead" to indicate that the given version of the file
+    # should not exist in the sandbox.
     unless ( $self->{tables}{$self->tablename("revision")} )
     {
         my $tablename = $self->tablename("revision");
@@ -2802,6 +2816,15 @@ sub new
     }
 
     # Construct the head table if required
+    # The head table (along with the "last_commit" entry in the property
+    # table) is the persisted working state of the "sub update" subroutine.
+    # All of it's data is read entirely first, and completely recreated
+    # last, every time "sub update" runs.
+    # This is also used by "sub getmeta" when it is asked for the latest
+    # version of a file (as opposed to some specific version).
+    # Another way of thinking about it is as a single slice out of
+    # "revisions", giving just the most recent revision information for
+    # each file.
     unless ( $self->{tables}{$self->tablename("head")} )
     {
         my $tablename = $self->tablename("head");
@@ -2824,6 +2847,7 @@ sub new
     }
 
     # Construct the properties table if required
+    #  - "last_commit" - Used by "sub update".
     unless ( $self->{tables}{$self->tablename("properties")} )
     {
         my $tablename = $self->tablename("properties");
@@ -2836,6 +2860,11 @@ sub new
     }
 
     # Construct the commitmsgs table if required
+    # The commitmsgs table is only used for merge commits, since
+    # "sub update" will only keep one branch of parents.  Shortlogs
+    # for ignored commits (i.e. not on the chosen branch) will be used
+    # to construct a replacement "collapsed" merge commit message,
+    # which will be stored in this table.  See also "sub commitmessage".
     unless ( $self->{tables}{$self->tablename("commitmsgs")} )
     {
         my $tablename = $self->tablename("commitmsgs");
@@ -2867,6 +2896,14 @@ sub tablename
 
 =head2 update
 
+Bring the database up to date with the latest changes from
+the git repository.
+
+Internal working state is read out of the "head" table and the
+"last_commit" property, then it updates "revisions" based on that, and
+finally it writes the new internal state back to the "head" table
+so it can be used as a starting point the next time update is called.
+
 =cut
 sub update
 {
@@ -2980,17 +3017,18 @@ sub update
                 next;
             } elsif (@{$commit->{parents}} > 1) {
                 # it is a merge commit, for each parent that is
-                # not $lastpicked, see if we can get a log
+                # not $lastpicked (not given a CVS revision number),
+                # see if we can get a log
                 # from the merge-base to that parent to put it
                 # in the message as a merge summary.
                 my @parents = @{$commit->{parents}};
                 foreach my $parent (@parents) {
-                    # git-merge-base can potentially (but rarely) throw
-                    # several candidate merge bases. let's assume
-                    # that the first one is the best one.
                     if ($parent eq $lastpicked) {
                         next;
                     }
+                    # git-merge-base can potentially (but rarely) throw
+                    # several candidate merge bases. let's assume
+                    # that the first one is the best one.
 		    my $base = eval {
 			    safe_pipe_capture('git-merge-base',
 						 $lastpicked, $parent);
-- 
1.6.1.81.g9833d.dirty

[PATCH 03/10] cvsserver: remove unused functions _headrev and gethistory

[Matthew Ogilvie]

Remove:
   - _headrev() - cvsserver already uses similar functionality
     from getmeta() and gethead().
   - gethistory() - cvsserver already uses similar functions
     gethistorydense() and getlog().
   - #'annotate' comment  - Uncommented annotate line is 2 lines earlier.

Signed-off-by: Matthew Ogilvie <mmogilvi_git@miniinfo.net>
---
 git-cvsserver.perl |   37 ++++---------------------------------
 1 files changed, 4 insertions(+), 33 deletions(-)

diff --git a/git-cvsserver.perl b/git-cvsserver.perl
index f7891b8..fe23b49 100755
--- a/git-cvsserver.perl
+++ b/git-cvsserver.perl
@@ -78,7 +78,6 @@ my $methods = {
     'editors'         => \&req_EMPTY,
     'annotate'        => \&req_annotate,
     'Global_option'   => \&req_Globaloption,
-    #'annotate'        => \&req_CATCHALL,
 };
 
 ##############################################
@@ -3310,19 +3309,6 @@ sub insert_head
     $insert_head->execute($name, $revision, $filehash, $commithash, $modified, $author, $mode);
 }
 
-sub _headrev
-{
-    my $self = shift;
-    my $filename = shift;
-    my $tablename = $self->tablename("head");
-
-    my $db_query = $self->{dbh}->prepare_cached("SELECT filehash, revision, mode FROM $tablename WHERE name=?",{},1);
-    $db_query->execute($filename);
-    my ( $hash, $revision, $mode ) = $db_query->fetchrow_array;
-
-    return ( $hash, $revision, $mode );
-}
-
 sub _get_prop
 {
     my $self = shift;
@@ -3382,6 +3368,8 @@ sub gethead
 
 =head2 getlog
 
+See also gethistorydense().
+
 =cut
 
 sub getlog
@@ -3467,25 +3455,6 @@ sub commitmessage
     return $message;
 }
 
-=head2 gethistory
-
-This function takes a filename (with path) argument and returns an arrayofarrays
-containing revision,filehash,commithash ordered by revision descending
-
-=cut
-sub gethistory
-{
-    my $self = shift;
-    my $filename = shift;
-    my $tablename = $self->tablename("revision");
-
-    my $db_query;
-    $db_query = $self->{dbh}->prepare_cached("SELECT revision, filehash, commithash FROM $tablename WHERE name=? ORDER BY revision DESC",{},1);
-    $db_query->execute($filename);
-
-    return $db_query->fetchall_arrayref;
-}
-
 =head2 gethistorydense
 
 This function takes a filename (with path) argument and returns an arrayofarrays
@@ -3495,6 +3464,8 @@ This version of gethistory skips deleted entries -- so it is useful for annotate
 The 'dense' part is a reference to a '--dense' option available for git-rev-list
 and other git tools that depend on it.
 
+See also getlog().
+
 =cut
 sub gethistorydense
 {
-- 
1.6.1.81.g9833d.dirty

[PATCH 04/10] git-shell: allow running git-cvsserver, not just cvs

[Matthew Ogilvie]

Add the ability to ask a git-shell restricted account to explicitly run
git-cvsserver, instead of requesting a fake "cvs".  With this, an account
could be reconfigured to use/not use git-shell without requiring
changes to client side configuration.  It also avoids confusing
users about which way cvs should be configured; if cvs is accessing
a git repository, always set CVS_SERVER=git-cvsserver.

Signed-off-by: Matthew Ogilvie <mmogilvi_git@miniinfo.net>
---

--------------
Question 1:

There is a surprising shortage of solid information about the
best way(s) restrict what commands can be run by ssh accounts,
both in general and for git specifically.  Is
putting git-shell in /etc/passwd the best way?  Are there other ways,
just as good or better?

Perhaps someone who really knows could add something to the git-shell.txt
man page describing the possible way(s) to configure an account to
be restricted to git-shell.

--------------
Question 2:

As near as I can tell, git-shell as run by sshd is typically
invoked such that it ends up with an argv with 3 arguments of the
form ("git-shell","-c","command arguments"). 
But the (argc==2 "cvs server") special case near line 69 of shell.c
appears to allow the middle argument ("-c") to be missing, but
only for "cvs server".

My question is, why was this special case put there?  Is a remnant of
an imperfect ad-hoc test framework someone used, or is there a
legitimate configuration where the "-c" is missing?

It seems to me that either this missing "-c" case should be
eliminated, or it should be made more general: If some
configuration of sshd leaves out the "-c", then it probably
leaves it out for anything you try to invoke, not just
"cvs server".

--------------
Hints about testing:

In addition to the test cases that are included in the patch, I tested
this by setting up an extra user account with the following
shell script set as the shell in /etc/passwd (yes, a shell script
is my shell...).  This allows me to tweak enviornment variables,
log how it is invoked, maybe tee off the stdin/stdout, etc:

---CUT---
#!/bin/sh

log()
{ echo "$1" >> remote.log
}

log "------------"
log ""
log "cmd:$0"
for arg in "$@" ; do
  log "arg:$arg"
done

PATH="/path/to/version/to/test:$PATH"
log "path: $PATH"
#tee input.out | git-shell "$@" | tee output.out
exec git-shell "$@"
---CUT---

But note that you probably do NOT want to route through a
wrapper like this for production...

--
Matthew Ogilvie   [mmogilvi_git@miniinfo.net]

 Documentation/git-cvsserver.txt |    7 ++-
 Documentation/git-shell.txt     |    5 +-
 shell.c                         |    1 +
 t/t9400-git-cvsserver-server.sh |   88 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 97 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-cvsserver.txt b/Documentation/git-cvsserver.txt
index 785779e..e32ad7b 100644
--- a/Documentation/git-cvsserver.txt
+++ b/Documentation/git-cvsserver.txt
@@ -113,9 +113,12 @@ cvs -d ":ext;CVS_SERVER=git cvsserver:user@server/path/repo.git" co <HEAD_name>
 ------
 This has the advantage that it will be saved in your 'CVS/Root' files and
 you don't need to worry about always setting the correct environment
-variable.  SSH users restricted to 'git-shell' don't need to override the default
-with CVS_SERVER (and shouldn't) as 'git-shell' understands `cvs` to mean
+variable.  SSH users restricted to 'git-shell' do not need to
+override CVS_SERVER as 'git-cvsserver' because
+'git-shell' understands `cvs` to mean
 'git-cvsserver' and pretends that the other end runs the real 'cvs' better.
+But restricted users can still override it for consistency and to avoid
+reconfiguration if they are later promoted to full SSH access.
 --
 2. For each repo that you want accessible from CVS you need to edit config in
    the repo and add the following section.
diff --git a/Documentation/git-shell.txt b/Documentation/git-shell.txt
index 3f8d973..68cb834 100644
--- a/Documentation/git-shell.txt
+++ b/Documentation/git-shell.txt
@@ -18,8 +18,9 @@ of server-side GIT commands implementing the pull/push functionality.
 The commands can be executed only by the '-c' option; the shell is not
 interactive.
 
-Currently, only three commands are permitted to be called, 'git-receive-pack'
-'git-upload-pack' with a single required argument or 'cvs server' (to invoke
+Currently, only four commands are permitted to be called, 'git-receive-pack'
+'git-upload-pack' with a single required argument,
+'git-cvsserver server', or 'cvs server' (to invoke
 'git-cvsserver').
 
 Author
diff --git a/shell.c b/shell.c
index e339369..6ed960f 100644
--- a/shell.c
+++ b/shell.c
@@ -40,6 +40,7 @@ static struct commands {
 } cmd_list[] = {
 	{ "git-receive-pack", do_generic_cmd },
 	{ "git-upload-pack", do_generic_cmd },
+	{ "git-cvsserver", do_cvs_cmd },
 	{ "cvs", do_cvs_cmd },
 	{ NULL },
 };
diff --git a/t/t9400-git-cvsserver-server.sh b/t/t9400-git-cvsserver-server.sh
index 6a37f71..b23a774 100755
--- a/t/t9400-git-cvsserver-server.sh
+++ b/t/t9400-git-cvsserver-server.sh
@@ -501,4 +501,92 @@ test_expect_success 'cvs annotate' '
     test_cmp ../expect ../actual
 '
 
+#------------
+# access server through git-shell:
+#------------
+
+cd "$WORKDIR"
+test_expect_success 'setup fake_ssh' '
+    cd cvswork &&
+    ( echo "#!/bin/sh" &&
+      echo "echo \"cmd:\$*\" >> fake_ssh_sh.log" &&
+      echo "if test x\"\$1 \$2 \$3\" = x\"-l user machine\" ; then" &&
+      echo "  shift ; shift ; shift" &&
+      echo "  exec /bin/sh -c \"\$*\"" &&
+      echo "fi" &&
+      echo "echo FAIL" &&
+      echo "exit 1"
+    ) > fake_ssh_sh &&
+    ( echo "#!/bin/sh" &&
+      echo "echo \"cmd:\$*\" >> fake_ssh_git-shell.log" &&
+      echo "if test x\"\$1 \$2 \$3\" = x\"-l user machine\" ; then" &&
+      echo "  shift ; shift ; shift" &&
+      echo "  exec git-shell -c \"\$*\""
+      echo "fi" &&
+      echo "echo FAIL" &&
+      echo "exit 1"
+    ) > fake_ssh_git-shell &&
+    chmod +x fake_ssh_sh fake_ssh_git-shell
+'
+
+cd "$WORKDIR"
+test_expect_success 'fake_ssh ... sh git-cvsserver' '
+    ( cd cvswork &&
+      CVS_SERVER="git-cvsserver" &&
+      CVS_RSH="./fake_ssh_sh" &&
+      CVSROOT=":ext:user@machine$SERVERDIR" &&
+      export CVS_SERVER CVS_RSH CVSROOT &&
+      GIT_CONFIG="$git_config" cvs -d "$CVSROOT" up -p status.file > ../out &&
+      cmp status.file ../out
+    )
+'
+
+cd "$WORKDIR"
+test_expect_success 'fake_ssh ... sh git cvsserver' '
+    ( cd cvswork &&
+      CVS_SERVER="git cvsserver" &&
+      CVS_RSH="./fake_ssh_sh" &&
+      CVSROOT=":ext:user@machine$SERVERDIR" &&
+      export CVS_SERVER CVS_RSH CVSROOT &&
+      GIT_CONFIG="$git_config" cvs -d "$CVSROOT" up -p status.file > ../out &&
+      cmp status.file ../out
+    )
+'
+
+cd "$WORKDIR"
+test_expect_success 'fake_ssh ... git-shell git-cvsserver' '
+    ( cd cvswork &&
+      CVS_SERVER="git-cvsserver" &&
+      CVS_RSH="./fake_ssh_git-shell" &&
+      CVSROOT=":ext:user@machine$SERVERDIR" &&
+      export CVS_SERVER CVS_RSH CVSROOT &&
+      GIT_CONFIG="$git_config" cvs -d "$CVSROOT" up -p status.file > ../out &&
+      cmp status.file ../out
+    )
+'
+
+cd "$WORKDIR"
+test_expect_success 'fake_ssh ... git-shell git cvsserver' '
+    ( cd cvswork &&
+      CVS_SERVER="git cvsserver" &&
+      CVS_RSH="./fake_ssh_git-shell" &&
+      CVSROOT=":ext:user@machine$SERVERDIR" &&
+      export CVS_SERVER CVS_RSH CVSROOT &&
+      GIT_CONFIG="$git_config" cvs -d "$CVSROOT" up -p status.file > ../out &&
+      cmp status.file ../out
+    )
+'
+
+cd "$WORKDIR"
+test_expect_success 'fake_ssh ... git-shell cvs' '
+    ( cd cvswork &&
+      CVS_SERVER="cvs" &&
+      CVS_RSH="./fake_ssh_git-shell" &&
+      CVSROOT=":ext:user@machine$SERVERDIR" &&
+      export CVS_SERVER CVS_RSH CVSROOT &&
+      GIT_CONFIG="$git_config" cvs -d "$CVSROOT" up -p status.file > ../out &&
+      cmp status.file ../out
+    )
+'
+
 test_done
-- 
1.6.1.81.g9833d.dirty

[PATCH 05/10] cvsserver: run dashless "git command"s to access plumbing

[Matthew Ogilvie]

git-cvsserver has been installed in the default $(bindir)
since 1df2a1ce806, but if you actually invoke it that way
(CVS_SERVER=git-cvsserver), it was failing because most of the
dashed-form plumbing commands cvsserver invokes are not in the PATH.

It seems best not to just switch to pure CVS_SERVER="git cvsserver",
which does not work with cvs's ":fork:" connection scheme.  In
this case "cvs" presumably uses the whole value as argv[0] for
exec(), and of course no execuable named "git cvsserver" exists.

But both settings worked under some circumstances:

CVS_SERVER="git cvsserver" works OK for cvs's ":ext:" scheme (at
least with ssh), presumably because of how sshd invokes
/bin/sh -c "{args}" with limited quoting, and "git"
would set up the PATH so that cvsserver could run dashed commands
correctly.

Also, CVS_SERVER="git-cvsserver" has been working OK under git's test
framework, because the test framework currently always sets up a PATH
that includes the dashed forms.

Signed-off-by: Matthew Ogilvie <mmogilvi_git@miniinfo.net>
---

Note: I only changed the actual command invocations; I left error
messages and comments as-is.  Should they be changed as well?

--
Matthew Ogilvie   [mmogilvi_git@miniinfo.net]

 git-cvsserver.perl |   40 ++++++++++++++++++++--------------------
 1 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/git-cvsserver.perl b/git-cvsserver.perl
index fe23b49..8e9e659 100755
--- a/git-cvsserver.perl
+++ b/git-cvsserver.perl
@@ -283,7 +283,7 @@ sub req_Root
        return 0;
     }
 
-    my @gitvars = `git-config -l`;
+    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 \n";
@@ -700,7 +700,7 @@ sub req_Modified
     # Save the file data in $state
     $state->{entries}{$state->{directory}.$data}{modified_filename} = $filename;
     $state->{entries}{$state->{directory}.$data}{modified_mode} = $mode;
-    $state->{entries}{$state->{directory}.$data}{modified_hash} = `git-hash-object $filename`;
+    $state->{entries}{$state->{directory}.$data}{modified_hash} = `git hash-object $filename`;
     $state->{entries}{$state->{directory}.$data}{modified_hash} =~ s/\s.*$//s;
 
     #$log->debug("req_Modified : file=$data mode=$mode size=$size");
@@ -1287,7 +1287,7 @@ sub req_ci
 
 	# do a checkout of the file if it is part of this tree
         if ($wrev) {
-            system('git-checkout-index', '-f', '-u', $filename);
+            system('git', 'checkout-index', '-f', '-u', $filename);
             unless ($? == 0) {
                 die "Error running git-checkout-index -f -u $filename : $!";
             }
@@ -1329,15 +1329,15 @@ sub req_ci
         {
             $log->info("Removing file '$filename'");
             unlink($filename);
-            system("git-update-index", "--remove", $filename);
+            system("git", "update-index", "--remove", $filename);
         }
         elsif ( $addflag )
         {
             $log->info("Adding file '$filename'");
-            system("git-update-index", "--add", $filename);
+            system("git", "update-index", "--add", $filename);
         } else {
             $log->info("Updating file '$filename'");
-            system("git-update-index", $filename);
+            system("git", "update-index", $filename);
         }
     }
 
@@ -1349,7 +1349,7 @@ sub req_ci
         return;
     }
 
-    my $treehash = `git-write-tree`;
+    my $treehash = `git write-tree`;
     chomp $treehash;
 
     $log->debug("Treehash : $treehash, Parenthash : $parenthash");
@@ -1366,7 +1366,7 @@ sub req_ci
     }
     close $msg_fh;
 
-    my $commithash = `git-commit-tree $treehash -p $parenthash < $msg_filename`;
+    my $commithash = `git commit-tree $treehash -p $parenthash < $msg_filename`;
     chomp($commithash);
     $log->info("Commit hash : $commithash");
 
@@ -1819,7 +1819,7 @@ sub req_annotate
 	# TODO: if we got a revision from the client, use that instead
 	# to look up the commithash in sqlite (still good to default to
 	# the current head as we do now)
-	system("git-read-tree", $lastseenin);
+	system("git", "read-tree", $lastseenin);
 	unless ($? == 0)
 	{
 	    print "E error running git-read-tree $lastseenin $ENV{GIT_INDEX_FILE} $!\n";
@@ -1828,7 +1828,7 @@ sub req_annotate
 	$log->info("Created index '$ENV{GIT_INDEX_FILE}' with commit $lastseenin - exit status $?");
 
         # do a checkout of the file
-        system('git-checkout-index', '-f', '-u', $filename);
+        system('git', 'checkout-index', '-f', '-u', $filename);
         unless ($? == 0) {
             print "E error running git-checkout-index -f -u $filename : $!\n";
             return;
@@ -1859,7 +1859,7 @@ sub req_annotate
         close ANNOTATEHINTS
             or (print "E failed to write $a_hints: $!\n"), return;
 
-        my @cmd = (qw(git-annotate -l -S), $a_hints, $filename);
+        my @cmd = (qw(git annotate -l -S), $a_hints, $filename);
         if (!open(ANNOTATE, "-|", @cmd)) {
             print "E error invoking ". join(' ',@cmd) .": $!\n";
             return;
@@ -2080,17 +2080,17 @@ sub transmitfile
 
     die "Need filehash" unless ( defined ( $filehash ) and $filehash =~ /^[a-zA-Z0-9]{40}$/ );
 
-    my $type = `git-cat-file -t $filehash`;
+    my $type = `git cat-file -t $filehash`;
     chomp $type;
 
     die ( "Invalid type '$type' (expected 'blob')" ) unless ( defined ( $type ) and $type eq "blob" );
 
-    my $size = `git-cat-file -s $filehash`;
+    my $size = `git cat-file -s $filehash`;
     chomp $size;
 
     $log->debug("transmitfile($filehash) size=$size, type=$type");
 
-    if ( open my $fh, '-|', "git-cat-file", "blob", $filehash )
+    if ( open my $fh, '-|', "git", "cat-file", "blob", $filehash )
     {
         if ( defined ( $options->{targetfile} ) )
         {
@@ -2942,7 +2942,7 @@ sub update
         push @git_log_params, $self->{module};
     }
     # git-rev-list is the backend / plumbing version of git-log
-    open(GITLOG, '-|', 'git-rev-list', @git_log_params) or die "Cannot call git-rev-list: $!";
+    open(GITLOG, '-|', 'git', 'rev-list', @git_log_params) or die "Cannot call git-rev-list: $!";
 
     my @commits;
 
@@ -3029,7 +3029,7 @@ sub update
                     # several candidate merge bases. let's assume
                     # that the first one is the best one.
 		    my $base = eval {
-			    safe_pipe_capture('git-merge-base',
+			    safe_pipe_capture('git', 'merge-base',
 						 $lastpicked, $parent);
 		    };
 		    # The two branches may not be related at all,
@@ -3041,7 +3041,7 @@ sub update
                     if ($base) {
                         my @merged;
                         # print "want to log between  $base $parent \n";
-                        open(GITLOG, '-|', 'git-log', '--pretty=medium', "$base..$parent")
+                        open(GITLOG, '-|', 'git', 'log', '--pretty=medium', "$base..$parent")
 			  or die "Cannot call git-log: $!";
                         my $mergedhash;
                         while (<GITLOG>) {
@@ -3083,7 +3083,7 @@ 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 : $!");
 	    local ($/) = "\0";
             while ( <FILELIST> )
             {
@@ -3157,7 +3157,7 @@ sub update
             # this is used to detect files removed from the repo
             my $seen_files = {};
 
-            my $filepipe = open(FILELIST, '-|', 'git-ls-tree', '-z', '-r', $commit->{hash}) or die("Cannot call git-ls-tree : $!");
+            my $filepipe = open(FILELIST, '-|', 'git', 'ls-tree', '-z', '-r', $commit->{hash}) or die("Cannot call git-ls-tree : $!");
 	    local $/ = "\0";
             while ( <FILELIST> )
             {
@@ -3448,7 +3448,7 @@ sub commitmessage
         return $message;
     }
 
-    my @lines = safe_pipe_capture("git-cat-file", "commit", $commithash);
+    my @lines = safe_pipe_capture("git", "cat-file", "commit", $commithash);
     shift @lines while ( $lines[0] =~ /\S/ );
     $message = join("",@lines);
     $message .= " " if ( $message =~ /\n$/ );
-- 
1.6.1.81.g9833d.dirty

[PATCH 06/10] t2300: use documented technique to invoke git-sh-setup

[Matthew Ogilvie]

This is needed to allow the test suite to run against a minimal bin
directory instead of GIT_EXEC_PATH.

Signed-off-by: Matthew Ogilvie <mmogilvi_git@miniinfo.net>
---
 t/t2300-cd-to-toplevel.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/t/t2300-cd-to-toplevel.sh b/t/t2300-cd-to-toplevel.sh
index e42cbfe..eac348a 100755
--- a/t/t2300-cd-to-toplevel.sh
+++ b/t/t2300-cd-to-toplevel.sh
@@ -8,7 +8,7 @@ test_cd_to_toplevel () {
 	test_expect_success "$2" '
 		(
 			cd '"'$1'"' &&
-			. git-sh-setup &&
+			. "$(git --exec-path)"/git-sh-setup &&
 			cd_to_toplevel &&
 			[ "$(unset PWD; /bin/pwd)" = "$TOPLEVEL" ]
 		)
-- 
1.6.1.81.g9833d.dirty

[PATCH 07/10] t3409: use dashless "git commit" instead of "git-commit"

[Matthew Ogilvie]

This is needed to allow test suite to run against a minimal bin
directory instead of GIT_EXEC_PATH.

Signed-off-by: Matthew Ogilvie <mmogilvi_git@miniinfo.net>
---

Semi-related:

I noticed that there are actually two test cases numbered
t3409: t3409-rebase-hook.sh and t3409-rebase-preserve-merges.sh.
Should one of them be renumbered, or is it good enough as-is?  I'm
just pointing this out; I don't intend to change it myself.

--
Matthew Ogilvie   [mmogilvi_git@miniinfo.net]

 t/t3409-rebase-preserve-merges.sh |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t3409-rebase-preserve-merges.sh b/t/t3409-rebase-preserve-merges.sh
index e6c8327..8878771 100755
--- a/t/t3409-rebase-preserve-merges.sh
+++ b/t/t3409-rebase-preserve-merges.sh
@@ -32,14 +32,14 @@ export GIT_AUTHOR_EMAIL
 test_expect_success 'setup for merge-preserving rebase' \
 	'echo First > A &&
 	git add A &&
-	git-commit -m "Add A1" &&
+	git commit -m "Add A1" &&
 	git checkout -b topic &&
 	echo Second > B &&
 	git add B &&
-	git-commit -m "Add B1" &&
+	git commit -m "Add B1" &&
 	git checkout -f master &&
 	echo Third >> A &&
-	git-commit -a -m "Modify A2" &&
+	git commit -a -m "Modify A2" &&
 
 	git clone ./. clone1 &&
 	cd clone1 &&
-- 
1.6.1.81.g9833d.dirty

[PATCH 08/10] run test suite without dashed git-commands in PATH

[Matthew Ogilvie]

Exclude GIT_EXEC_PATH from PATH, to emulate the default user environment,
and ensure all the programs (especially scripts) run correctly in
such an environment.

This works by creating a test-bin directory with wrapper scripts for
the programs normally installed in "bin", and only including that
directory in the PATH.

Signed-off-by: Matthew Ogilvie <mmogilvi_git@miniinfo.net>
---

Valgrind conflict:  As I was preparing this email, I noticed that a patch
for running the test suite under valgrind has been under discussion
in the mailing list.  The valgrind patch probably conflicts with this
one.  I've thought of some possible ways to resolve it:

   1. Extend this patch to support valgrind by adding
environment-variable controlled hook to test-bin-wrapper.sh,
and enhance the makefile rules to disable the valgrind hook
for instances of the script that are wrapping other scripts.
(This allows dashless, valgrind, or both.)

   2. Or change the valgrind patch to only add executables typically
found in bindir (not GIT_EXEC_PATH) to the symlink directory.  To
support dashless by itself, it would need to do a similar symlink
when asked for dashless without valgrind.

   3. Or keep the core of both patches, and just fix up the setup
in test-lib.sh to only enable one or the other (never both).

Thoughts?

--
Matthew Ogilvie   [mmogilvi_git@miniinfo.net]

 .gitignore          |    1 +
 Makefile            |   42 +++++++++++++++++++++++++++++++-----------
 t/test-lib.sh       |   14 +++++++++++++-
 test-bin-wrapper.sh |   12 ++++++++++++
 4 files changed, 57 insertions(+), 12 deletions(-)
 create mode 100644 test-bin-wrapper.sh

diff --git a/.gitignore b/.gitignore
index d9adce5..13a0d33 100644
--- a/.gitignore
+++ b/.gitignore
@@ -143,6 +143,7 @@ git-write-tree
 git-core-*/?*
 gitk-wish
 gitweb/gitweb.cgi
+test-bin
 test-chmtime
 test-date
 test-delta
diff --git a/Makefile b/Makefile
index b4d9cb4..197a6f0 100644
--- a/Makefile
+++ b/Makefile
@@ -330,6 +330,15 @@ ALL_PROGRAMS = $(PROGRAMS) $(SCRIPTS)
 # what 'all' will build but not install in gitexecdir
 OTHER_PROGRAMS = git$X gitweb/gitweb.cgi
 
+# what test wrappers are needed and 'install' will install, in bindir
+BINDIR_PROGRAMS_NEED_X += git
+BINDIR_PROGRAMS_NEED_X += git-upload-pack
+BINDIR_PROGRAMS_NEED_X += git-receive-pack
+BINDIR_PROGRAMS_NEED_X += git-upload-archive
+BINDIR_PROGRAMS_NEED_X += git-shell
+
+BINDIR_PROGRAMS_NO_X += git-cvsserver
+
 # Set paths to tools early so that they can be used for version tests.
 ifndef SHELL_PATH
 	SHELL_PATH = /bin/sh
@@ -1356,17 +1365,25 @@ endif
 
 ### Testing rules
 
-TEST_PROGRAMS += test-chmtime$X
-TEST_PROGRAMS += test-ctype$X
-TEST_PROGRAMS += test-date$X
-TEST_PROGRAMS += test-delta$X
-TEST_PROGRAMS += test-genrandom$X
-TEST_PROGRAMS += test-match-trees$X
-TEST_PROGRAMS += test-parse-options$X
-TEST_PROGRAMS += test-path-utils$X
-TEST_PROGRAMS += test-sha1$X
+TEST_PROGRAMS_NEED_X += test-chmtime
+TEST_PROGRAMS_NEED_X += test-ctype
+TEST_PROGRAMS_NEED_X += test-date
+TEST_PROGRAMS_NEED_X += test-delta
+TEST_PROGRAMS_NEED_X += test-genrandom
+TEST_PROGRAMS_NEED_X += test-match-trees
+TEST_PROGRAMS_NEED_X += test-parse-options
+TEST_PROGRAMS_NEED_X += test-path-utils
+TEST_PROGRAMS_NEED_X += test-sha1
+
+TEST_PROGRAMS = $(patsubst %,%$X,$(TEST_PROGRAMS_NEED_X))
 
-all:: $(TEST_PROGRAMS)
+test_bindir_programs := $(patsubst %,test-bin/%,$(BINDIR_PROGRAMS_NEED_X) $(BINDIR_PROGRAMS_NO_X) $(TEST_PROGRAMS_NEED_X))
+
+all:: $(TEST_PROGRAMS) $(test_bindir_programs)
+
+test-bin/%: test-bin-wrapper.sh
+	@mkdir -p test-bin
+	$(QUIET_GEN)sed -e 's|__GIT_EXEC_PATH__|$(shell pwd)|' -e 's|__PROG__|$(@F)|' < $< > $@ && chmod +x $@
 
 # GNU make supports exporting all variables by "export" without parameters.
 # However, the environment gets quite big, and some programs have problems
@@ -1425,11 +1442,13 @@ endif
 gitexec_instdir_SQ = $(subst ','\'',$(gitexec_instdir))
 export gitexec_instdir
 
+install_bindir_programs := $(patsubst %,%$X,$(BINDIR_PROGRAMS_NEED_X)) $(BINDIR_PROGRAMS_NO_X)
+
 install: all
 	$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(bindir_SQ)'
 	$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
 	$(INSTALL) $(ALL_PROGRAMS) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
-	$(INSTALL) git$X git-upload-pack$X git-receive-pack$X git-upload-archive$X git-shell$X git-cvsserver '$(DESTDIR_SQ)$(bindir_SQ)'
+	$(INSTALL) $(install_bindir_programs) '$(DESTDIR_SQ)$(bindir_SQ)'
 	$(MAKE) -C templates DESTDIR='$(DESTDIR_SQ)' install
 	$(MAKE) -C perl prefix='$(prefix_SQ)' DESTDIR='$(DESTDIR_SQ)' install
 ifndef NO_TCLTK
@@ -1533,6 +1552,7 @@ clean:
 		$(LIB_FILE) $(XDIFF_LIB)
 	$(RM) $(ALL_PROGRAMS) $(BUILT_INS) git$X
 	$(RM) $(TEST_PROGRAMS)
+	$(RM) -r test-bin
 	$(RM) *.spec *.pyc *.pyo */*.pyc */*.pyo common-cmds.h TAGS tags cscope*
 	$(RM) -r autom4te.cache
 	$(RM) config.log config.mak.autogen config.mak.append config.status config.cache
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 41d5a59..2f42b5b 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -89,6 +89,8 @@ do
 		verbose=t; shift ;;
 	-q|--q|--qu|--qui|--quie|--quiet)
 		quiet=t; shift ;;
+	--with-dashes)
+		with_dashes=t; shift ;;
 	--no-color)
 		color=; shift ;;
 	--no-python)
@@ -467,8 +469,18 @@ test_done () {
 # Test the binaries we have just built.  The tests are kept in
 # t/ subdirectory and are run in 'trash directory' subdirectory.
 TEST_DIRECTORY=$(pwd)
-PATH=$TEST_DIRECTORY/..:$PATH
 GIT_EXEC_PATH=$(pwd)/..
+git_bin_dir="$TEST_DIRECTORY/../test-bin"
+if ! test -x "$git_bin_dir/git" ; then
+	if test -z "$with_dashes" ; then
+		say "$git_bin_dir/git is not executable; using GIT_EXEC_PATH"
+	fi
+	with_dashes=t
+fi
+PATH="$git_bin_dir:$PATH"
+if test -n "$with_dashes" ; then
+	PATH="$TEST_DIRECTORY/..:$PATH"
+fi
 GIT_TEMPLATE_DIR=$(pwd)/../templates/blt
 unset GIT_CONFIG
 GIT_CONFIG_NOSYSTEM=1
diff --git a/test-bin-wrapper.sh b/test-bin-wrapper.sh
new file mode 100644
index 0000000..199d098
--- /dev/null
+++ b/test-bin-wrapper.sh
@@ -0,0 +1,12 @@
+#!/bin/sh
+
+# test-bin-wrapper.sh: Template for git executable wrapper scripts
+# to run test suite against sandbox, but with only bindir-installed
+# executables in PATH.  The Makefile copies this into various
+# files in test-bin, substituting __GIT_EXEC_PATH__ and __PROG__.
+
+GIT_EXEC_PATH="__GIT_EXEC_PATH__"
+GIT_TEMPLATE_DIR="__GIT_EXEC_PATH__/templates/blt"
+export GIT_EXEC_PATH GIT_TEMPLATE_DIR
+
+exec "${GIT_EXEC_PATH}/__PROG__" "$@"
-- 
1.6.1.81.g9833d.dirty

[PATCH 09/10] Revert "adapt git-cvsserver manpage to dash-free syntax"

[Matthew Ogilvie]

This reverts commit da9973c6f9600d90e64aac647f3ed22dfd692f70,
and a couple of other remaining references to "git cvsserver".

When da9973c6f9600 was committed, we did not install git-cvsserver
in $(bindir) by default, but that situation did not last long (see
1df2a1ce806 "Install git-cvsserver in $(bindir)").  Also, a recent
commit (cvsserver: use dashless "git command"s to access plumbing)
fixed an issue with git-cvsserver trying to run dashed forms
without setting up the PATH for it.

The dashed form is arguably better in this case, since it is
required for cvs's ":fork:" connection method (although ":ext:"
usually doesn't need dashes), and it can minimize the need to escape
characters when setting up CVS_SERVER and/or CVSROOT.

Signed-off-by: Matthew Ogilvie <mmogilvi_git@miniinfo.net>
---

Note: It might conceivably be worth holding off on this patch for a
release, in case someone tries to use a newer man page from the web with
an older install of git, where git-cvsserver still has the PATH bug
mentioned in the commit message above.  But I doubt this is really
common enough to worry about.

--
Matthew Ogilvie   [mmogilvi_git@miniinfo.net]

 Documentation/git-cvsserver.txt |   10 +++++-----
 git-cvsserver.perl              |    2 +-
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-cvsserver.txt b/Documentation/git-cvsserver.txt
index e32ad7b..6f8cd88 100644
--- a/Documentation/git-cvsserver.txt
+++ b/Documentation/git-cvsserver.txt
@@ -11,7 +11,7 @@ SYNOPSIS
 SSH:
 
 [verse]
-export CVS_SERVER="git cvsserver"
+export CVS_SERVER=git-cvsserver
 'cvs' -d :ext:user@server/path/repo.git co <HEAD_name>
 
 pserver (/etc/inetd.conf):
@@ -22,7 +22,7 @@ cvspserver stream tcp nowait nobody /usr/bin/git-cvsserver git-cvsserver pserver
 Usage:
 
 [verse]
-'git cvsserver' [options] [pserver|server] [<directory> ...]
+'git-cvsserver' [options] [pserver|server] [<directory> ...]
 
 OPTIONS
 -------
@@ -109,7 +109,7 @@ Note: Newer CVS versions (>= 1.12.11) also support specifying
 CVS_SERVER directly in CVSROOT like
 
 ------
-cvs -d ":ext;CVS_SERVER=git cvsserver:user@server/path/repo.git" co <HEAD_name>
+cvs -d ":ext;CVS_SERVER=git-cvsserver:user@server/path/repo.git" co <HEAD_name>
 ------
 This has the advantage that it will be saved in your 'CVS/Root' files and
 you don't need to worry about always setting the correct environment
@@ -161,7 +161,7 @@ allowing access over SSH.
 --
 ------
      export CVSROOT=:ext:user@server:/var/git/project.git
-     export CVS_SERVER="git cvsserver"
+     export CVS_SERVER=git-cvsserver
 ------
 --
 4. For SSH clients that will make commits, make sure their server-side
@@ -286,7 +286,7 @@ To get a checkout with the Eclipse CVS client:
 Protocol notes: If you are using anonymous access via pserver, just select that.
 Those using SSH access should choose the 'ext' protocol, and configure 'ext'
 access on the Preferences->Team->CVS->ExtConnection pane. Set CVS_SERVER to
-"'git cvsserver'". Note that password support is not good when using 'ext',
+'git-cvsserver'. Note that password support is not good when using 'ext',
 you will definitely want to have SSH keys setup.
 
 Alternatively, you can just use the non-standard extssh protocol that Eclipse
diff --git a/git-cvsserver.perl b/git-cvsserver.perl
index 8e9e659..4b817ce 100755
--- a/git-cvsserver.perl
+++ b/git-cvsserver.perl
@@ -100,7 +100,7 @@ my $work =
 $log->info("--------------- STARTING -----------------");
 
 my $usage =
-    "Usage: git cvsserver [options] [pserver|server] [<directory> ...]\n".
+    "Usage: git-cvsserver [options] [pserver|server] [<directory> ...]\n".
     "    --base-path <path>  : Prepend to requested CVSROOT\n".
     "    --strict-paths      : Don't allow recursing into subdirectories\n".
     "    --export-all        : Don't check for gitcvs.enabled in config\n".
-- 
1.6.1.81.g9833d.dirty

[PATCH 10/10] cvsserver doc: emphasize using CVS_SERVER= phrase within CVSROOT

[Matthew Ogilvie]

Add information about CVSROOT=":ext;CVS_SERVER=git-cvsserver:..." to
"SYNOPSIS" section.  This information was/is already spelled out deep in
the "INSTALLATION" section, but this new method is far superior to
the old backwards-compatible option (separate environment variable),
so try to emphasize it's use for versions of CVS that support it.

Also mention how to connect to a local git repository using
git-cvsserver.

Signed-off-by: Matthew Ogilvie <mmogilvi_git@miniinfo.net>
---
 Documentation/git-cvsserver.txt |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/Documentation/git-cvsserver.txt b/Documentation/git-cvsserver.txt
index 6f8cd88..d1dc5be 100644
--- a/Documentation/git-cvsserver.txt
+++ b/Documentation/git-cvsserver.txt
@@ -14,6 +14,18 @@ SSH:
 export CVS_SERVER=git-cvsserver
 'cvs' -d :ext:user@server/path/repo.git co <HEAD_name>
 
+SSH with newer CVS versions (>= 1.12.11):
+
+[verse]
+export CVSROOT=":ext;CVS_SERVER=git-cvsserver:user@server/path/repo.git"
+'cvs' -d "$CVSROOT" co <HEAD_name>
+
+Local repository:
+
+[verse]
+export CVSROOT=":fork;CVS_SERVER=git-cvsserver:/path/repo.git"
+'cvs' -d "$CVSROOT" co <HEAD_name>
+
 pserver (/etc/inetd.conf):
 
 [verse]
-- 
1.6.1.81.g9833d.dirty

Re: [PATCH 04/10] git-shell: allow running git-cvsserver, not just cvs

[Johannes Schindelin]

Hi,

On Sat, 24 Jan 2009, Matthew Ogilvie wrote:

> diff --git a/shell.c b/shell.c
> index e339369..6ed960f 100644
> --- a/shell.c
> +++ b/shell.c
> @@ -40,6 +40,7 @@ static struct commands {
>  } cmd_list[] = {
>  	{ "git-receive-pack", do_generic_cmd },
>  	{ "git-upload-pack", do_generic_cmd },
> +	{ "git-cvsserver", do_cvs_cmd },
>  	{ "cvs", do_cvs_cmd },
>  	{ NULL },
>  };

I do not see any value in this.

Ciao,
Dscho

Re: [PATCH 08/10] run test suite without dashed git-commands in PATH

[Johannes Schindelin]

Hi,

On Sat, 24 Jan 2009, Matthew Ogilvie wrote:

>  .gitignore          |    1 +
>  Makefile            |   42 +++++++++++++++++++++++++++++++-----------
>  t/test-lib.sh       |   14 +++++++++++++-
>  test-bin-wrapper.sh |   12 ++++++++++++
>  4 files changed, 57 insertions(+), 12 deletions(-)
>  create mode 100644 test-bin-wrapper.sh

I am strongly opposed to a patch this big, just for something as 3rd class 
as CVS server faking.  We already have a big fallout from all that bending 
over for Windows support, and I do not like it at all.

Note: I do not even have to look further than the diffstat to see that it 
is wrong.

The point is: if cvsserver wants to pretend that it is in a fake bin where 
almost none of the other Git programs are, fine, let's do that _in the 
test for cvsserver_.

Let's not fsck up the whole test suite just for one user.

Ciao,
Dscho

Re: [PATCH 08/10] run test suite without dashed git-commands in PATH

[Matthew Ogilvie]

Hi,

On Sun, Jan 25, 2009 at 02:59:53AM +0100, Johannes Schindelin wrote:
> Hi,
> 
> On Sat, 24 Jan 2009, Matthew Ogilvie wrote:
> 
> >  .gitignore          |    1 +
> >  Makefile            |   42 +++++++++++++++++++++++++++++++-----------
> >  t/test-lib.sh       |   14 +++++++++++++-
> >  test-bin-wrapper.sh |   12 ++++++++++++
> >  4 files changed, 57 insertions(+), 12 deletions(-)
> >  create mode 100644 test-bin-wrapper.sh
> 
> I am strongly opposed to a patch this big, just for something as 3rd class 
> as CVS server faking.  We already have a big fallout from all that bending 
> over for Windows support, and I do not like it at all.
> 
> Note: I do not even have to look further than the diffstat to see that it 
> is wrong.
> 
> The point is: if cvsserver wants to pretend that it is in a fake bin where 
> almost none of the other Git programs are, fine, let's do that _in the 
> test for cvsserver_.
> 
> Let's not fsck up the whole test suite just for one user.
> 
> Ciao,
> Dscho

Since by default git is installed such that most of the dashed-form
commands are not in a user's default PATH, my thought was that
it would make sense for the test suite to mimick that environment
as much as possible.  This could detect regressions in any
installed/tested git script that erroneously assumes the dashed
form commands are in the PATH, not just git-cvsserver.

I can think of ways it could be made smaller, but they seem uglier than
the current patch to me:

    1. Perhaps just list the executables for the fake bin directory
separately, but then it is all too easy for the list to get out of sync
with what the final install environment will be.

    2. Perhaps strip off the $X's (.exe on windows; currently empty
elsewhere) from the words of the existing variables, in the rule
for building the "fake bin" directory.  But generally I'ld rather
not assume that pattern-matching replacement of
$X's will never conflict with a part of a script name.

    3. Perhaps just use symlinks or hardlinks instead of a wrapper
script.  This might have some promise, except that links are more
likely to fail on windows, and the wrappers generally give you
more flexibility for testing odd scenarios.

    4. The test-bin-wrapper.sh script does not actually need to
set environment variables (GIT_EXEC_DIT and templates) for
purposes of this patch.  But my thought was that in this form
you could run things straight out of the test-bin directory
to manually try out new code without needing to actually install
a build or mess with the environment variables yourself.  It could
also be extended to handle other global wrapper needs
relatively easily, such as valgrind.

Any other ideas?

--
Matthew Ogilvie   [mmogilvi_git@miniinfo.net]

Re: [PATCH 08/10] run test suite without dashed git-commands in PATH

[Johannes Schindelin]

Hi,

On Sun, 25 Jan 2009, Matthew Ogilvie wrote:

> On Sun, Jan 25, 2009 at 02:59:53AM +0100, Johannes Schindelin wrote:
> 
> > On Sat, 24 Jan 2009, Matthew Ogilvie wrote:
> > 
> > >  .gitignore          |    1 +
> > >  Makefile            |   42 +++++++++++++++++++++++++++++++-----------
> > >  t/test-lib.sh       |   14 +++++++++++++-
> > >  test-bin-wrapper.sh |   12 ++++++++++++
> > >  4 files changed, 57 insertions(+), 12 deletions(-)
> > >  create mode 100644 test-bin-wrapper.sh
> > 
> > I am strongly opposed to a patch this big, just for something as 3rd 
> > class as CVS server faking.  We already have a big fallout from all 
> > that bending over for Windows support, and I do not like it at all.
> > 
> > Note: I do not even have to look further than the diffstat to see that 
> > it is wrong.
> > 
> > The point is: if cvsserver wants to pretend that it is in a fake bin 
> > where almost none of the other Git programs are, fine, let's do that 
> > _in the test for cvsserver_.
> 
> Since by default git is installed such that most of the dashed-form 
> commands are not in a user's default PATH, my thought was that it would 
> make sense for the test suite to mimick that environment as much as 
> possible.

This sounds very generic, but you hid it in cvsserver-specific patch 
series.

So maybe I was wrong to assume that this is cvsserver specific, but then, 
you made that mistake rather easy to make.

> This could detect regressions in any installed/tested git script that 
> erroneously assumes the dashed form commands are in the PATH, not just 
> git-cvsserver.

The major point is that these scripts _will_ run if you call _them_ using 
the dash-less form, as GIT_EXEC_PATH will be added to the PATH by the Git 
wrapper.

>     3. Perhaps just use symlinks or hardlinks instead of a wrapper 
>        script.  This might have some promise, except that links are more 
>        likely to fail on windows, and the wrappers generally give you 
>        more flexibility for testing odd scenarios.

Not likely.  Sure as hell.

>     4. The test-bin-wrapper.sh script does not actually need to set 
>        environment variables (GIT_EXEC_DIT and templates) for purposes 
>        of this patch.  But my thought was that in this form you could 
>        run things straight out of the test-bin directory to manually try 
>        out new code without needing to actually install a build or mess 
>        with the environment variables yourself.  It could also be 
>        extended to handle other global wrapper needs relatively easily, 
>        such as valgrind.

Umm.

You missed the valgrind patch series.

Ciao,
Dscho

Re: [PATCH 08/10] run test suite without dashed git-commands in PATH

[Matthew Ogilvie]

On Mon, Jan 26, 2009 at 12:06:08PM +0100, Johannes Schindelin wrote:
> So maybe I was wrong to assume that this is cvsserver specific, but then, 
> you made that mistake rather easy to make.

Yes, in retrospect I probably should have split off patches 6, 7,
8, and maybe 5 (5-7 fix issues that patch 8 exposes in the test suite)
into a separate patch series.  When or if a v2 is needed, should
I split them off then?

> >     4. The test-bin-wrapper.sh script does not actually need to set 
> >        environment variables (GIT_EXEC_DIT and templates) for purposes 
> >        of this patch.  But my thought was that in this form you could 
> >        run things straight out of the test-bin directory to manually try 
> >        out new code without needing to actually install a build or mess 
> >        with the environment variables yourself.  It could also be 
> >        extended to handle other global wrapper needs relatively easily, 
> >        such as valgrind.
> 
> Umm.
> 
> You missed the valgrind patch series.

Actually, I'm (poorly) alluding to some comments in the original patch
8 email, where I pointed out an expected conflict with the valgrind
patches.  To briefly recap, there are at least 3 possible strategies
of resolving such a conflict, and I'm not sure which makes the most
sense: Keep valgrind design, and extend it with limited bindir
support.  Keep limited bindir design and extend it with valgrind
support.  Or keep both, and have the runtime setup logic make
them mutually exclusive.

--
Matthew Ogilvie   [mmogilvi_git@miniinfo.net]