| Message ID | 20200826011718.3186597-3-gitster@pobox.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | War on dashed-git | expand |
On Tue, Aug 25, 2020 at 9:17 PM Junio C Hamano <gitster@pobox.com> wrote: > This ancient script runs "git-foo" all over the place. A strange > thing is that it has t9200 tests successfully running, even though > it does not seem to futz with PATH to prepend $(git --exec-path) > output. t/test-lib.sh takes care of that for us, doesn't it? > It is tempting to declare that the command must be unused, > but that is left to another topic. > > Signed-off-by: Junio C Hamano <gitster@pobox.com>
Eric Sunshine <sunshine@sunshineco.com> writes: > On Tue, Aug 25, 2020 at 9:17 PM Junio C Hamano <gitster@pobox.com> wrote: >> This ancient script runs "git-foo" all over the place. A strange >> thing is that it has t9200 tests successfully running, even though >> it does not seem to futz with PATH to prepend $(git --exec-path) >> output. > > t/test-lib.sh takes care of that for us, doesn't it? It generally shouldn't, I think. We have bin-wrappers directory and we shouldn't be copying things we would install in libexec/git-core/ in real life.
Hi Junio, On Tue, 25 Aug 2020, Junio C Hamano wrote: > This ancient script runs "git-foo" all over the place. A strange > thing is that it has t9200 tests successfully running, even though > it does not seem to futz with PATH to prepend $(git --exec-path) > output. It is tempting to declare that the command must be unused, > but that is left to another topic. Not surprising at all: when t9200 runs `git cvsexportcommit`, it actually runs `bin-wrappers/git`, which sets `GIT_EXEC_PATH` to the top-level directory of the Git source code, then calls the `git` executable which in turn will set up the `PATH` to prepend `GIT_EXEC_PATH`, and then look for `git-cvsexportcommit` (which does not exist in `bin-wrappers/`, but in `GIT_EXEC_PATH`). And of course then `git-rev-parse` is found on the `PATH`, too. Slightly more surprising is that my PR build did not fail. I guess we do test `git svn`, but we skip all CVS-related tests, eh? Slightly related: it might be a good time to deprecate the CVS-related Git commands... Ciao, Dscho
Eric Sunshine <sunshine@sunshineco.com> writes: > On Tue, Aug 25, 2020 at 9:17 PM Junio C Hamano <gitster@pobox.com> wrote: >> This ancient script runs "git-foo" all over the place. A strange >> thing is that it has t9200 tests successfully running, even though >> it does not seem to futz with PATH to prepend $(git --exec-path) >> output. > > t/test-lib.sh takes care of that for us, doesn't it? Actually, it is "git" itself. The tests spawn "git cvsexportcommit" via the "git" dispatcher. The dispatcher adds GIT_EXEC_PATH to PATH in exec-cmd.c::setup_path() and that is used to (1) locate "git-foo" from /usr/libexec/git-core/ that are not builtin and (2) passed to any processes we invoke. I think the latter was originally done primarily for not breaking hooks, but that is what allows this script going in this particular case. If this were only a fluke in the test that kept otherwise unrunnable script passing, I'd say it is an evidence enough that lets us drop the cvsexportcommit immediately, but now I rediscovered how it was supposed to work and saw how it actually does work as designed, I would not be surprised if it is still used in the wild. Thanks.
Junio C Hamano <gitster@pobox.com> writes: > Eric Sunshine <sunshine@sunshineco.com> writes: > >> On Tue, Aug 25, 2020 at 9:17 PM Junio C Hamano <gitster@pobox.com> wrote: >>> This ancient script runs "git-foo" all over the place. A strange >>> thing is that it has t9200 tests successfully running, even though >>> it does not seem to futz with PATH to prepend $(git --exec-path) >>> output. >> >> t/test-lib.sh takes care of that for us, doesn't it? > > Actually, it is "git" itself. > > The tests spawn "git cvsexportcommit" via the "git" dispatcher. The > dispatcher adds GIT_EXEC_PATH to PATH in exec-cmd.c::setup_path() > and that is used to (1) locate "git-foo" from /usr/libexec/git-core/ > that are not builtin and (2) passed to any processes we invoke. I > think the latter was originally done primarily for not breaking > hooks, but that is what allows this script going in this particular > case. > > If this were only a fluke in the test that kept otherwise unrunnable > script passing, I'd say it is an evidence enough that lets us drop > the cvsexportcommit immediately, but now I rediscovered how it was > supposed to work and saw how it actually does work as designed, I > would not be surprised if it is still used in the wild. So, the conclusion: the proposed log message needs a large revamp. Thanks.
diff --git a/git-cvsexportcommit.perl b/git-cvsexportcommit.perl index 0ae8bce3fb..289d4bc684 100755 --- a/git-cvsexportcommit.perl +++ b/git-cvsexportcommit.perl @@ -30,7 +30,7 @@ # Remember where GIT_DIR is before changing to CVS checkout unless ($ENV{GIT_DIR}) { # No GIT_DIR set. Figure it out for ourselves - my $gd =`git-rev-parse --git-dir`; + my $gd =`git rev-parse --git-dir`; chomp($gd); $ENV{GIT_DIR} = $gd; } @@ -66,7 +66,7 @@ # resolve target commit my $commit; $commit = pop @ARGV; -$commit = safe_pipe_capture('git-rev-parse', '--verify', "$commit^0"); +$commit = safe_pipe_capture('git', 'rev-parse', '--verify', "$commit^0"); chomp $commit; if ($?) { die "The commit reference $commit did not resolve!"; @@ -76,7 +76,7 @@ my $parent; if (@ARGV) { $parent = pop @ARGV; - $parent = safe_pipe_capture('git-rev-parse', '--verify', "$parent^0"); + $parent = safe_pipe_capture('git', 'rev-parse', '--verify', "$parent^0"); chomp $parent; if ($?) { die "The parent reference did not resolve!"; @@ -84,7 +84,7 @@ } # find parents from the commit itself -my @commit = safe_pipe_capture('git-cat-file', 'commit', $commit); +my @commit = safe_pipe_capture('git', 'cat-file', 'commit', $commit); my @parents; my $committer; my $author; @@ -162,9 +162,9 @@ close MSG; if ($parent eq $noparent) { - `git-diff-tree --binary -p --root $commit >.cvsexportcommit.diff`;# || die "Cannot diff"; + `git diff-tree --binary -p --root $commit >.cvsexportcommit.diff`;# || die "Cannot diff"; } else { - `git-diff-tree --binary -p $parent $commit >.cvsexportcommit.diff`;# || die "Cannot diff"; + `git diff-tree --binary -p $parent $commit >.cvsexportcommit.diff`;# || die "Cannot diff"; } ## apply non-binary changes @@ -178,7 +178,7 @@ print "Checking if patch will apply\n"; my @stat; -open APPLY, "GIT_INDEX_FILE=$tmpdir/index git-apply $context --summary --numstat<.cvsexportcommit.diff|" || die "cannot patch"; +open APPLY, "GIT_INDEX_FILE=$tmpdir/index git apply $context --summary --numstat<.cvsexportcommit.diff|" || die "cannot patch"; @stat=<APPLY>; close APPLY || die "Cannot patch"; my (@bfiles,@files,@afiles,@dfiles); @@ -333,7 +333,7 @@ if ($opt_W) { system("git checkout -q $commit^0") && die "cannot patch"; } else { - `GIT_INDEX_FILE=$tmpdir/index git-apply $context --summary --numstat --apply <.cvsexportcommit.diff` || die "cannot patch"; + `GIT_INDEX_FILE=$tmpdir/index git apply $context --summary --numstat --apply <.cvsexportcommit.diff` || die "cannot patch"; } print "Patch applied successfully. Adding new files and directories to CVS\n";
This ancient script runs "git-foo" all over the place. A strange thing is that it has t9200 tests successfully running, even though it does not seem to futz with PATH to prepend $(git --exec-path) output. It is tempting to declare that the command must be unused, but that is left to another topic. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- git-cvsexportcommit.perl | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)