{"thread":{"id":"20321","subject":"reflogs generated by git-cvsimport","startedAt":"2009-07-31T17:42:26Z","lastAt":"2009-07-31T20:40:23Z","messageCount":4,"participants":["Kalle Olavi Niemitalo","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"119295","messageId":"87fxccagvx.fsf@Astalo.kon.iki.fi","threadId":"20321","inReplyTo":null,"subject":"reflogs generated by git-cvsimport","fromName":"Kalle Olavi Niemitalo","fromEmail":"kon@iki.fi","sentAt":"2009-07-31T17:42:26Z","receivedAt":"2009-07-31T17:42:26Z","isPatch":false,"sender":{"key":"kon@iki.fi","avatar":null},"body":"When I run git cvsimport incrementally, it generates a reflog\nentry for each CVS commit, because it runs git-update-ref after\neach git-commit-tree.  I think it'd be nicer to make just one\nreflog entry at the end, when all CVS patchsets have been\nimported.  I could then use git log cvs/master@{1}..cvs/master\nto see all the commits on master that were imported in the\nlatest run.\n\nIf git-cvsimport.perl kept the commit IDs in Perl variables only,\nand then updated the refs once at the end, I'd get the reflogs I\nprefer.  However, if the script were interrupted in the middle,\nit would then leave the refs unchanged, and the next cvsimport\nrun would have to download the same commits again.  I suppose\nthat could be fixed with some $SIG{'INT'} handler.  But then how\nabout the git repack -a -d that git-cvsimport.perl runs every\n1024 commits: could that lose some commits that have been\nimported from CVS but not yet saved in any ref?\n\nWould it be better to create temporary refs/cvsimport/* and then\nfinally update the real refs based on those?\n\nThere's also another problem with the reflogs.  The current\nversion of git-cvsimport sets GIT_COMMITTER_DATE and related\nvariables for git-commit-tree, and then leaves them set for\ngit-update-ref.  So git-update-ref saves the author and date of\nthe CVS commit into the reflog.  It would be better to save the\nname of the person who is running git cvsimport, and the local\ntime.  That one I've already fixed in my local version, but I'm\nnot really happy with how the code turned out.\n"},{"id":"119303","messageId":"20090731191334.GA12132@coredump.intra.peff.net","threadId":"20321","inReplyTo":"87fxccagvx.fsf@Astalo.kon.iki.fi","subject":"Re: reflogs generated by git-cvsimport","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-07-31T19:13:34Z","receivedAt":"2009-07-31T19:13:34Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 31, 2009 at 08:42:26PM +0300, Kalle Olavi Niemitalo wrote:\n\n> If git-cvsimport.perl kept the commit IDs in Perl variables only,\n> and then updated the refs once at the end, I'd get the reflogs I\n> prefer.  However, if the script were interrupted in the middle,\n> it would then leave the refs unchanged, and the next cvsimport\n> run would have to download the same commits again.  I suppose\n> that could be fixed with some $SIG{'INT'} handler.  But then how\n> about the git repack -a -d that git-cvsimport.perl runs every\n> 1024 commits: could that lose some commits that have been\n> imported from CVS but not yet saved in any ref?\n> \n> Would it be better to create temporary refs/cvsimport/* and then\n> finally update the real refs based on those?\n\nThe \"rebase\" command does something similar by performing its changes on\na detached HEAD, and then writing the finished result to the real ref.\nThis gives you a detailed log in the HEAD reflog, but branch@{1} shows\nthe rebase as a single step.\n\nHowever, I'm not sure that such a strategy would work for cvsimport,\nwhich (IIRC) operates on branches that are not the HEAD. So I think\nusing refs/cvsimport/* instead of a detached HEAD makes sense.\n\nTrying to do it all internally to the script seems needlessly complex\nand error prone (and you would lose the detailed reflog, which you might\nsometimes want for debugging or whatever).\n\n> There's also another problem with the reflogs.  The current\n> version of git-cvsimport sets GIT_COMMITTER_DATE and related\n> variables for git-commit-tree, and then leaves them set for\n> git-update-ref.  So git-update-ref saves the author and date of\n> the CVS commit into the reflog.  It would be better to save the\n> name of the person who is running git cvsimport, and the local\n> time.  That one I've already fixed in my local version, but I'm\n> not really happy with how the code turned out.\n\nYeah, it probably should not munge the reflog with the CVS committer\ninformation. I suspect it would be as easy as the following (totally\nuntested, not even syntax checked) patch:\n\n---\ndiff --git a/git-cvsimport.perl b/git-cvsimport.perl\nindex e439202..a94c48d 100755\n--- a/git-cvsimport.perl\n+++ b/git-cvsimport.perl\n@@ -742,14 +742,16 @@ sub commit {\n \t}\n \n \tmy $commit_date = strftime(\"+0000 %Y-%m-%d %H:%M:%S\",gmtime($date));\n-\t$ENV{GIT_AUTHOR_NAME} = $author_name;\n-\t$ENV{GIT_AUTHOR_EMAIL} = $author_email;\n-\t$ENV{GIT_AUTHOR_DATE} = $commit_date;\n-\t$ENV{GIT_COMMITTER_NAME} = $author_name;\n-\t$ENV{GIT_COMMITTER_EMAIL} = $author_email;\n-\t$ENV{GIT_COMMITTER_DATE} = $commit_date;\n-\tmy $pid = open2(my $commit_read, my $commit_write,\n-\t\t'git-commit-tree', $tree, @commit_args);\n+\tmy $pid = do {\n+\t\tlocal $ENV{GIT_AUTHOR_NAME} = $author_name;\n+\t\tlocal $ENV{GIT_AUTHOR_EMAIL} = $author_email;\n+\t\tlocal $ENV{GIT_AUTHOR_DATE} = $commit_date;\n+\t\tlocal $ENV{GIT_COMMITTER_NAME} = $author_name;\n+\t\tlocal $ENV{GIT_COMMITTER_EMAIL} = $author_email;\n+\t\tlocal $ENV{GIT_COMMITTER_DATE} = $commit_date;\n+\t\topen2(my $commit_read, my $commit_write,\n+\t\t\t'git-commit-tree', $tree, @commit_args);\n+\t}\n \n \t# compatibility with git2cvs\n \tsubstr($logmsg,32767) = \"\" if length($logmsg) > 32767;\n"},{"id":"119305","messageId":"87bpn0a9t9.fsf@Astalo.kon.iki.fi","threadId":"20321","inReplyTo":"20090731191334.GA12132@coredump.intra.peff.net","subject":"Re: reflogs generated by git-cvsimport","fromName":"Kalle Olavi Niemitalo","fromEmail":"kon@iki.fi","sentAt":"2009-07-31T20:15:14Z","receivedAt":"2009-07-31T20:15:14Z","isPatch":false,"sender":{"key":"kon@iki.fi","avatar":null},"body":"Jeff King <peff@peff.net> writes:\n\n> Yeah, it probably should not munge the reflog with the CVS committer\n> information. I suspect it would be as easy as the following (totally\n> untested, not even syntax checked) patch:\n\nThat patch does not work because the $commit_read and\n$commit_write file handles fall out of scope too early.\nThose and $pid could be returned from the do {...} as\na list, but I think it's easier to remove the \"do\", declare\nthe variables above the block, and assign them in the block.\n\nAlso, there's $ENV{'TZ'}=\"UTC\" at the beginning of the script\nand it affects the reflogs too.  This is the annoying part.\nThe script runs numerous subprocesses and it is not clear to\nme which of those need TZ=UTC and which ones should use the\noriginal TZ:\n\n- git config: doesn't matter?\n- cvs: UTC?\n- rsh: UTC?\n- git rev-parse --verify: depends on whether $name looks in reflog\n- git-init: doesn't matter?\n- git-read-tree: doesn't matter\n- git-symbolic-ref: original if this can write to reflog\n- git-rev-parse --verify HEAD: doesn't matter\n- git-for-each-ref: doesn't matter\n- cvsps: UTC?\n- git-update-index: doesn't matter\n- git-write-tree: doesn't matter\n- git-commit-tree: doesn't matter because GIT_COMMITTER_DATE and\n  GIT_COMMITTER_DATE already specify \"+0000\".  (Might be nice to\n  have author-specific time zones there though.)\n- git-update-ref: original.  (Also, -m cvsimport could be added.)\n- git-tag: doesn't matter because cvsimport never uses git tag -a.\n- git update-ref: original\n- git-hash-object: doesn't matter\n- git repack: doesn't matter?\n- git-count-objects: doesn't matter\n- git-merge: original\n- git checkout: doesn't matter?\n"},{"id":"119306","messageId":"20090731204023.GB28226@coredump.intra.peff.net","threadId":"20321","inReplyTo":"87bpn0a9t9.fsf@Astalo.kon.iki.fi","subject":"Re: reflogs generated by git-cvsimport","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-07-31T20:40:23Z","receivedAt":"2009-07-31T20:40:23Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 31, 2009 at 11:15:14PM +0300, Kalle Olavi Niemitalo wrote:\n\n> That patch does not work because the $commit_read and\n> $commit_write file handles fall out of scope too early.\n> Those and $pid could be returned from the do {...} as\n> a list, but I think it's easier to remove the \"do\", declare\n> the variables above the block, and assign them in the block.\n\nOops, indeed, I clearly did not look closely. But I see you understood\nwhat I was trying to say, and I think you are right that it is probably\ncleaner to just \"my\" them right before the block.\n\n> Also, there's $ENV{'TZ'}=\"UTC\" at the beginning of the script\n> and it affects the reflogs too.  This is the annoying part.\n\nSince that is covering the whole script, it is obviously a harder issue\nand should probably be a separate patch from the GIT_COMMITTER_*\ninformation.\n\n> The script runs numerous subprocesses and it is not clear to\n> me which of those need TZ=UTC and which ones should use the\n> original TZ:\n\nSadly, there is nothing useful in the commit history as the TZ setting\ngoes all the way back to the script being added. I would guess it is\nthere to convince cvs to give us a consistent time, since its log output\nusually comes out in the local timezone (though since cvsimport is based\non cvsps, I would assume cvsps handles this sanely).\n\nI suspect if you set it for cvs and cvsps, that would be sufficient. The\nrest of git should use the original.\n\n-Peff\n"}]}