t/git-cgi.t: fix race condition

[git.ikiwiki.info.git] / doc / todo / headless_git_branches.mdwn

diff --git a/doc/todo/headless_git_branches.mdwn b/doc/todo/headless_git_branches.mdwn
index e97b599007b383074b5ed6e78de4ac9e2d57e95a..bedf21d0ca5c07d68c11e8eb566957224b5d3d1e 100644 (file)
--- a/doc/todo/headless_git_branches.mdwn
+++ b/doc/todo/headless_git_branches.mdwn
@@ -6,49 +6,100 @@ Ikiwiki should really survive being asked to work with a git branch that has no
     git clone barerepo.git srcdir
     ikiwiki --rcs=git srcdir destdir
 
     git clone barerepo.git srcdir
     ikiwiki --rcs=git srcdir destdir
 

-I've fixed this initial construction case, and, based on my testing, I've also fixed the post-update executing on a new master, and ikiwiki.cgi executing on a non-existent master cases. Unfortunately these cases still generate a lot of warnings from recentchanges; I suspect a much more invasive patch would be needed to shut those up.
+I've fixed this initial construction case, and, based on my testing, I've
+also fixed the post-update executing on a new master, and ikiwiki.cgi
+executing on a non-existent master cases.

 
 Please commit so my users stop whining at me about having clean branches to push to, the big babies.
 
 
 Please commit so my users stop whining at me about having clean branches to push to, the big babies.
 

-Summary: Change three scary loud failure cases related to empty branches into three scary loud success cases.
+Summary: Change three scary loud failure cases related to empty branches into three mostly quiet success cases.

 
 [[!tag patch]]
 
 
 [[!tag patch]]
 

+> FWIW, [[The_TOVA_Company]] apparently wants this feature (and I hope
+> I don't mind that I mention they were willing to pay someone for it,
+> but I told them I'd not done any of the work. :) )
+> 
+> Code review follows, per hunk.. --[[Joey]] 
+

 <pre>
 diff --git a/IkiWiki/Plugin/git.pm b/IkiWiki/Plugin/git.pm
 <pre>
 diff --git a/IkiWiki/Plugin/git.pm b/IkiWiki/Plugin/git.pm

-index cf7fbe9..5b1e334 100644
+index cf7fbe9..e5bafcf 100644

 --- a/IkiWiki/Plugin/git.pm
 +++ b/IkiWiki/Plugin/git.pm
 --- a/IkiWiki/Plugin/git.pm
 +++ b/IkiWiki/Plugin/git.pm

-@@ -439,10 +439,13 @@ sub git_commit_info ($;$) {
+@@ -439,17 +439,21 @@ sub git_commit_info ($;$) {

  
        my @opts;
        push @opts, "--max-count=$num" if defined $num;
  
        my @opts;
        push @opts, "--max-count=$num" if defined $num;

-+        my @raw_lines;
- 
+-

 -      my @raw_lines = run_or_die('git', 'log', @opts,
 -              '--pretty=raw', '--raw', '--abbrev=40', '--always', '-c',
 -              '-r', $sha1, '--', '.');
 -      my @raw_lines = run_or_die('git', 'log', @opts,
 -              '--pretty=raw', '--raw', '--abbrev=40', '--always', '-c',
 -              '-r', $sha1, '--', '.');

-+        if (-e $config{srcdir} . "/.git/refs/heads/" . $config{gitmaster_branch}) {
-+                @raw_lines = run_or_die('git', 'log', @opts,
-+                        '--pretty=raw', '--raw', '--abbrev=40', '--always', '-c',
-+                        '-r', $sha1, '--', '.');
-+        };
- 
+-
++      my @raw_lines;

        my @ci;
        my @ci;

-       while (my $parsed = parse_diff_tree(\@raw_lines)) {
-@@ -474,7 +477,10 @@ sub rcs_update () {
+-      while (my $parsed = parse_diff_tree(\@raw_lines)) {
+-              push @ci, $parsed;
+-      }
++        
++      # Test to see if branch actually exists yet.
++      if (run_or_non('git', 'show-ref', '--quiet', '--verify', '--', 'refs/heads/' . $config{gitmaster_branch}) ) {
++              @raw_lines = run_or_die('git', 'log', @opts,
++                      '--pretty=raw', '--raw', '--abbrev=40', '--always', '-c',
++                      '-r', $sha1, '--', '.');
++
++              while (my $parsed = parse_diff_tree(\@raw_lines)) {
++                      push @ci, $parsed;
++              }
+ 
+-      warn "Cannot parse commit info for '$sha1' commit" if !@ci;
++              warn "Cannot parse commit info for '$sha1' commit" if !@ci;
++      };
+ 
+       return wantarray ? @ci : $ci[0];
+ }
+</pre>
+
+My concern is that this adds a bit of slowdown (git show-ref is fast, but
+It's still extra work) to a very hot code path that is run to eg,
+update recentchanges after every change. 
+
+Seems not ideal to do extra work every time to handle a case
+that will literally happen a maximum of once in the entire lifecycle of a
+wiki (and zero times more typically, since the setup automator puts in a
+.gitignore file that works around this problem).
+
+So as to not just say "no" ... what if it always tried to run git log,
+and if it failed (or returned no parsed lines), then it could look 
+at git show-ref to deduce whether to throw an error or not.
+--[[Joey]] 
+
+> Ah, but then git-log would still complain "bad revision 'HEAD'"
+> --[[Joey]] 
+
+<pre>
+@@ -474,7 +478,10 @@ sub rcs_update () {

        # Update working directory.
  
        if (length $config{gitorigin_branch}) {
 -              run_or_cry('git', 'pull', '--prune', $config{gitorigin_branch});
        # Update working directory.
  
        if (length $config{gitorigin_branch}) {
 -              run_or_cry('git', 'pull', '--prune', $config{gitorigin_branch});

-+                run_or_cry('git', 'fetch', '--prune', $config{gitorigin_branch});
-+                if (-e $config{srcdir} . '/.git/refs/remotes/' . $config{gitorigin_branch} . '/' . $config{gitmaster_branch}) {
-+                        run_or_cry('git', 'merge', $config{gitorigin_branch} . '/' . $config{gitmaster_branch});
-+                }
++              run_or_cry('git', 'fetch', '--prune', $config{gitorigin_branch});
++              if (run_or_non('git', 'show-ref', '--quiet', '--verify', '--', 'refs/remotes/' . $config{gitorigin_branch} . '/' . $config{gitmaster_branch}) ) {
++                      run_or_cry('git', 'merge', $config{gitorigin_branch} . '/' . $config{gitmaster_branch});
++              }

        }
  }
        }
  }

- 
-@@ -559,7 +565,7 @@ sub rcs_commit_helper (@) {
+
+</pre>
+
+Same concern here about extra work. Code path is nearly as hot, being
+called on every refresh. Probably could be dealt with similarly as above.
+
+Also, is there any point in breaking the pull up into a
+fetch followed by a merge? --[[Joey]] 
+
+<pre>
+@@ -559,7 +566,7 @@ sub rcs_commit_helper (@) {

        # So we should ignore its exit status (hence run_or_non).
        if (run_or_non('git', 'commit', '-m', $params{message}, '-q', @opts)) {
                if (length $config{gitorigin_branch}) {
        # So we should ignore its exit status (hence run_or_non).
        if (run_or_non('git', 'commit', '-m', $params{message}, '-q', @opts)) {
                if (length $config{gitorigin_branch}) {


@@ -56,4 +107,7 @@ index cf7fbe9..5b1e334 100644
 +                      run_or_cry('git', 'push', $config{gitorigin_branch}, $config{gitmaster_branch});
                }
        }
 +                      run_or_cry('git', 'push', $config{gitorigin_branch}, $config{gitmaster_branch});
                }
        }

+       

 </pre>
 </pre>

+
+This seems fine to apply. --[[Joey]]