{"thread":{"id":"12020","subject":"[PATCH] Fix 'git cvsexportcommit -w $cvsdir ...' when used with relative $GIT_DIR","startedAt":"2008-02-11T01:28:05Z","lastAt":"2008-02-13T09:26:57Z","messageCount":8,"participants":["Johan Herland","Junio C Hamano","Robin Rosenberg","Jakub Narebski"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"68315","messageId":"200802110228.05233.johan@herland.net","threadId":"12020","inReplyTo":null,"subject":"[PATCH] Fix 'git cvsexportcommit -w $cvsdir ...' when used with relative $GIT_DIR","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2008-02-11T01:28:05Z","receivedAt":"2008-02-11T01:28:05Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"When using the '-w $cvsdir' option to cvsexportcommit, it will chdir into\n$cvsdir before executing several other git commands. If $GIT_DIR is set to\na relative path (e.g. '.'), the git commands executed by cvsexportcommit\nwill naturally fail with \"fatal: Not a git repository\".\n\nTherefore, if $GIT_DIR is relative, prepend $PWD to $GIT_DIR before the\nchdir to $cvsdir.\n\nSigned-off-by: Johan Herland <johan@herland.net>\n---\n\nI'm working on an update hook script for automatically propagating commits \nto a CVS server. My script calls 'git cvsexportcommit -w $cvsdir ...'. \nHowever, the hooks infrastructure invokes the update hook script with the \n(bare, in my case) Git repo as $PWD and sets up GIT_DIR=\".\" in the script's \nenvironment. For now, I work around this bug in my script by forcing \n$GIT_DIR to $PWD, but this really seems like a bug in cvsexportcommit.\n\nIn my case, this bug can also be fixed by making sure the hook \ninfrastructure set up an _absolute_ GIT_DIR for the update hook, something \nI'd consider more polite than the current GIT_DIR=\".\". Whether we want to \nfix this as well should be discussed. But the patch below is needed in any \ncase, since cvsexportcommit may not only be called from hook scripts.\n\n\nHave fun! :)\n\n...Johan\n\n\n git-cvsexportcommit.perl |   15 +++++++++------\n 1 files changed, 9 insertions(+), 6 deletions(-)\n\ndiff --git a/git-cvsexportcommit.perl b/git-cvsexportcommit.perl\nindex d2e50c3..fd4fbfb 100755\n--- a/git-cvsexportcommit.perl\n+++ b/git-cvsexportcommit.perl\n@@ -15,15 +15,18 @@ $opt_h && usage();\n die \"Need at least one commit identifier!\" unless @ARGV;\n \n if ($opt_w) {\n+\t# Remember where GIT_DIR is before changing to CVS checkout\n \tunless ($ENV{GIT_DIR}) {\n-\t\t# Remember where our GIT_DIR is before changing to CVS checkout\n+\t\t# Oops no GIT_DIR set. Figure out for ourselves\n \t\tmy $gd =`git-rev-parse --git-dir`;\n \t\tchomp($gd);\n-\t\tif ($gd eq '.git') {\n-\t\t\tmy $wd = `pwd`;\n-\t\t\tchomp($wd);\n-\t\t\t$gd = $wd.\"/.git\"\t;\n-\t\t}\n+\t\t$ENV{GIT_DIR} = $gd;\n+\t}\n+\tunless ($ENV{GIT_DIR} =~ m[^/]) {\n+\t\t# GIT_DIR is relative. Prepend $PWD\n+\t\tmy $wd = `pwd`;\n+\t\tchomp($wd);\n+\t\tmy $gd = $wd.\"/\".$ENV{GIT_DIR};\n \t\t$ENV{GIT_DIR} = $gd;\n \t}\n \n-- \n1.5.4.2.g41ac4\n"},{"id":"68435","messageId":"7vhcgf8da6.fsf@gitster.siamese.dyndns.org","threadId":"12020","inReplyTo":"200802110228.05233.johan@herland.net","subject":"Re: [PATCH] Fix 'git cvsexportcommit -w $cvsdir ...' when used with relative $GIT_DIR","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-11T20:10:41Z","receivedAt":"2008-02-11T20:10:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland <johan@herland.net> writes:\n\n> When using the '-w $cvsdir' option to cvsexportcommit, it will chdir into\n> $cvsdir before executing several other git commands. If $GIT_DIR is set to\n> a relative path (e.g. '.'), the git commands executed by cvsexportcommit\n> will naturally fail with \"fatal: Not a git repository\".\n>\n> Therefore, if $GIT_DIR is relative, prepend $PWD to $GIT_DIR before the\n> chdir to $cvsdir.\n\nThat sounds like a correct thing to do.  Robin?\n"},{"id":"68461","messageId":"200802112158.16830.robin.rosenberg.lists@dewire.com","threadId":"12020","inReplyTo":"200802110228.05233.johan@herland.net","subject":"Re: [PATCH] Fix 'git cvsexportcommit -w $cvsdir ...' when used with relative $GIT_DIR","fromName":"Robin Rosenberg","fromEmail":"robin.rosenberg.lists@dewire.com","sentAt":"2008-02-11T20:58:16Z","receivedAt":"2008-02-11T20:58:16Z","isPatch":true,"sender":{"key":"robin.rosenberg@dewire.com","avatar":"https://avatars.githubusercontent.com/u/46357?v=4"},"body":"måndagen den 11 februari 2008 skrev Johan Herland:\n> When using the '-w $cvsdir' option to cvsexportcommit, it will chdir into\n> $cvsdir before executing several other git commands. If $GIT_DIR is set to\n> a relative path (e.g. '.'), the git commands executed by cvsexportcommit\n> will naturally fail with \"fatal: Not a git repository\".\n[...]\n\n>  die \"Need at least one commit identifier!\" unless @ARGV;\n>  \n>  if ($opt_w) {\n> +\t# Remember where GIT_DIR is before changing to CVS checkout\n>  \tunless ($ENV{GIT_DIR}) {\n> -\t\t# Remember where our GIT_DIR is before changing to CVS checkout\n> +\t\t# Oops no GIT_DIR set. Figure out for ourselves\n\nThat's not an \"Oops\". It's perfectly normal not to have GIT_DIR set.\n\n>  \t\tmy $gd =`git-rev-parse --git-dir`;\n>  \t\tchomp($gd);\n> -\t\tif ($gd eq '.git') {\n> -\t\t\tmy $wd = `pwd`;Kopia\n> -\t\t\tchomp($wd);\n> -\t\t\t$gd = $wd.\"/.git\"\t;\n> -\t\t}\n> +\t\t$ENV{GIT_DIR} = $gd;\n> +\t}\n> +\tunless ($ENV{GIT_DIR} =~ m[^/]) {\n> +\t\t# GIT_DIR is relative. Prepend $PWD\nHmm. C:/foo? You should probably use rel2abs in the File::Spec\nmodule.\n\n-- robin\n"},{"id":"68463","messageId":"200802120043.41610.johan@herland.net","threadId":"12020","inReplyTo":"200802112158.16830.robin.rosenberg.lists@dewire.com","subject":"[PATCH] Fix 'git cvsexportcommit -w $cvsdir ...' when used with relative $GIT_DIR","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2008-02-11T23:43:41Z","receivedAt":"2008-02-11T23:43:41Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"When using the '-w $cvsdir' option to cvsexportcommit, it will chdir into\n$cvsdir before executing several other git commands. If $GIT_DIR is set to\na relative path (e.g. '.'), the git commands executed by cvsexportcommit\nwill naturally fail.\n\nTherefore, ensure that $GIT_DIR is absolute before the chdir to $cvsdir.\n\nSigned-off-by: Johan Herland <johan@herland.net>\n---\n\nOn Monday 11 February 2008, Robin Rosenberg wrote:\n> måndagen den 11 februari 2008 skrev Johan Herland:\n> > -\t\t# Remember where our GIT_DIR is before changing to CVS checkout\n> > +\t\t# Oops no GIT_DIR set. Figure out for ourselves\n> \n> That's not an \"Oops\". It's perfectly normal not to have GIT_DIR set.\n\nOf course not. Fixed.\n\n> > +\tunless ($ENV{GIT_DIR} =~ m[^/]) {\n> \n> Hmm. C:/foo? You should probably use rel2abs in the File::Spec \n> module.\n\nThanks. Fixed.\n\n\nHave fun! :)\n\n...Johan\n\n\n git-cvsexportcommit.perl |   11 +++++------\n 1 files changed, 5 insertions(+), 6 deletions(-)\n\ndiff --git a/git-cvsexportcommit.perl b/git-cvsexportcommit.perl\nindex d2e50c3..2a8ad1e 100755\n--- a/git-cvsexportcommit.perl\n+++ b/git-cvsexportcommit.perl\n@@ -5,6 +5,7 @@ use Getopt::Std;\n use File::Temp qw(tempdir);\n use Data::Dumper;\n use File::Basename qw(basename dirname);\n+use File::Spec;\n \n our ($opt_h, $opt_P, $opt_p, $opt_v, $opt_c, $opt_f, $opt_a, $opt_m, $opt_d, $opt_u, $opt_w);\n \n@@ -15,17 +16,15 @@ $opt_h && usage();\n die \"Need at least one commit identifier!\" unless @ARGV;\n \n if ($opt_w) {\n+\t# Remember where GIT_DIR is before changing to CVS checkout\n \tunless ($ENV{GIT_DIR}) {\n-\t\t# Remember where our GIT_DIR is before changing to CVS checkout\n+\t\t# No GIT_DIR set. Figure it out for ourselves\n \t\tmy $gd =`git-rev-parse --git-dir`;\n \t\tchomp($gd);\n-\t\tif ($gd eq '.git') {\n-\t\t\tmy $wd = `pwd`;\n-\t\t\tchomp($wd);\n-\t\t\t$gd = $wd.\"/.git\"\t;\n-\t\t}\n \t\t$ENV{GIT_DIR} = $gd;\n \t}\n+\t# Make sure GIT_DIR is absolute\n+\t$ENV{GIT_DIR} = File::Spec->rel2abs($ENV{GIT_DIR});\n \n \tif (! -d $opt_w.\"/CVS\" ) {\n \t\tdie \"$opt_w is not a CVS checkout\";\n-- \n1.5.4.2.g41ac4\n"},{"id":"68546","messageId":"200802122141.35594.robin.rosenberg.lists@dewire.com","threadId":"12020","inReplyTo":"200802120043.41610.johan@herland.net","subject":"Re: [PATCH] Fix 'git cvsexportcommit -w $cvsdir ...' when used with relative $GIT_DIR","fromName":"Robin Rosenberg","fromEmail":"robin.rosenberg.lists@dewire.com","sentAt":"2008-02-12T20:41:34Z","receivedAt":"2008-02-12T20:41:34Z","isPatch":true,"sender":{"key":"robin.rosenberg@dewire.com","avatar":"https://avatars.githubusercontent.com/u/46357?v=4"},"body":"\nLooks ok. Something for the test suite?\n\n-- robin\n"},{"id":"68596","messageId":"200802130411.22093.johan@herland.net","threadId":"12020","inReplyTo":"200802122141.35594.robin.rosenberg.lists@dewire.com","subject":"[PATCH] Add testcase for 'git cvsexportcommit -w $cvsdir ...' with relative $GIT_DIR","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2008-02-13T03:11:22Z","receivedAt":"2008-02-13T03:11:22Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"The testcase verifies that 'git cvsexportcommit' functions correctly when\nthe '-w' option is used, and GIT_DIR is set to a relative path (e.g. '.').\n\nSigned-off-by: Johan Herland <johan@herland.net>\n---\n\nOn Tuesday 12 February 2008, Robin Rosenberg wrote:\n> Looks ok. Something for the test suite?\n\nLike this?\n\n\nHave fun! :)\n\n...Johan\n\n\n t/t9200-git-cvsexportcommit.sh |   18 +++++++++++++++++-\n 1 files changed, 17 insertions(+), 1 deletions(-)\n\ndiff --git a/t/t9200-git-cvsexportcommit.sh b/t/t9200-git-cvsexportcommit.sh\nindex a15222c..5dae88c 100755\n--- a/t/t9200-git-cvsexportcommit.sh\n+++ b/t/t9200-git-cvsexportcommit.sh\n@@ -2,7 +2,7 @@\n #\n # Copyright (c) Robin Rosenberg\n #\n-test_description='CVS export comit. '\n+test_description='Test export of commits to CVS'\n \n . ./test-lib.sh\n \n@@ -246,4 +246,20 @@ test_expect_success \\\n \t;;\n esac\n \n+test_expect_success \\\n+     '-w option should work with relative GIT_DIR' \\\n+     'mkdir W &&\n+      echo foobar >W/file1.txt &&\n+      echo bazzle >W/file2.txt &&\n+      git add W/file1.txt &&\n+      git add W/file2.txt &&\n+      git commit -m \"More updates\" &&\n+      id=$(git rev-list --max-count=1 HEAD) &&\n+      (cd \"$GIT_DIR\" &&\n+      GIT_DIR=. git cvsexportcommit -w \"$CVSWORK\" -c $id &&\n+      check_entries \"$CVSWORK/W\" \"file1.txt/1.1/|file2.txt/1.1/\" &&\n+      diff -u \"$CVSWORK/W/file1.txt\" ../W/file1.txt &&\n+      diff -u \"$CVSWORK/W/file2.txt\" ../W/file2.txt\n+      )'\n+\n test_done\n-- \n1.5.4.2.g41ac4\n"},{"id":"68606","messageId":"7vabm5s2xs.fsf@gitster.siamese.dyndns.org","threadId":"12020","inReplyTo":"200802130411.22093.johan@herland.net","subject":"Re: [PATCH] Add testcase for 'git cvsexportcommit -w $cvsdir ...' with relative $GIT_DIR","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-13T07:58:23Z","receivedAt":"2008-02-13T07:58:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland <johan@herland.net> writes:\n\n> The testcase verifies that 'git cvsexportcommit' functions correctly when\n> the '-w' option is used, and GIT_DIR is set to a relative path (e.g. '.').\n>\n> Signed-off-by: Johan Herland <johan@herland.net>\n> ---\n>\n> On Tuesday 12 February 2008, Robin Rosenberg wrote:\n>> Looks ok. Something for the test suite?\n>\n> Like this?\n\nYeah, looks good.\n\nExcept that I'll reorder the patches so that\n\n (1) this test comes first, but with test_expect_failure instead;\n\n (2) then your earlier fix patch, but with a change that changes\n     the test_expect_failure to test_expect_success.\n\nThanks.\n\nPerhaps it would be a good idea to require fix-up patches to\nalways come with a test case to prevent future regression.\n"},{"id":"68619","messageId":"m3odalcill.fsf@localhost.localdomain","threadId":"12020","inReplyTo":"7vabm5s2xs.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Add testcase for 'git cvsexportcommit -w $cvsdir ...' with relative $GIT_DIR","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-02-13T09:26:57Z","receivedAt":"2008-02-13T09:26:57Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Except that I'll reorder the patches so that\n> \n>  (1) this test comes first, but with test_expect_failure instead;\n> \n>  (2) then your earlier fix patch, but with a change that changes\n>      the test_expect_failure to test_expect_success.\n> \n> Thanks.\n> \n> Perhaps it would be a good idea to require fix-up patches to\n> always come with a test case to prevent future regression.\n\nPerhaps we should add this hint to SubmittingPatches, on top\nof your additions? And to t/README?\n\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"}]}