[StGit RFC] Make "stg branch -l" faster by getting all git config information in one call

[StGit RFC] Make "stg branch -l" faster by getting all git config information in one call

[David Kågedal]

This isn't a real patch yet, but it is good enough for my usage.

I have a fair amount of branches, and I noticed that "stg branch -l"
takes ridiculously long to finish. The problem is that it creates
stack objects for all branches, and indiviudally extract information
about them one after one.  With 20 branches, it took almost 2 seconds
to run.  Compare that with the 0.01 seconds it takes to run "git
branch".

I made a patch that uses "git config --get-regexp" to get the
description and stgit.stackformatversion options for all branches at
once, and ignore the "protected" flag that I don't use. With this
change, I'm almost down to half a second, which almost makes it
usable.

There are still a bunch of redundant invokations of git, but python
startup times are hard to get around.

Maybe someone can help me find a quicker replacement for the
get_protected call?

---

 stgit/commands/branch.py |   39 ++++++++++++++++++++++++++++++---------
 1 files changed, 30 insertions(+), 9 deletions(-)


diff --git a/stgit/commands/branch.py b/stgit/commands/branch.py
index 50684bb..f4b0c33 100644
--- a/stgit/commands/branch.py
+++ b/stgit/commands/branch.py
@@ -72,21 +72,21 @@ options = [make_option('-c', '--create',
 def __is_current_branch(branch_name):
     return crt_series.get_name() == branch_name
 
-def __print_branch(branch_name, length):
+def __print_branch(branch_name, description, version, length):
     initialized = ' '
     current = ' '
     protected = ' '
 
-    branch = stack.Series(branch_name)
+    #branch = stack.Series(branch_name)
 
-    if branch.is_initialised():
+    if version != None:
         initialized = 's'
     if __is_current_branch(branch_name):
         current = '>'
-    if branch.get_protected():
-        protected = 'p'
+    #if branch.get_protected():
+    #    protected = 'p'
     out.stdout(current + ' ' + initialized + protected + '\t'
-               + branch_name.ljust(length) + '  | ' + branch.get_description())
+               + branch_name.ljust(length) + '  | ' + (description or ''))
 
 def __delete_branch(doomed_name, force = False):
     doomed = stack.Series(doomed_name)
@@ -100,6 +100,23 @@ def __delete_branch(doomed_name, force = False):
     doomed.delete(force)
     out.done()
 
+class FormatException(StgException):
+    pass
+
+def __get_all_branch_config(key):
+    key = re.escape(key)
+    lines = git.GRun('config', '--get-regexp',
+                     r'branch\..*\.'+key).returns([0,1]).output_lines()
+    val_re = re.compile(r'branch\.(.*)\.%s (.*)' % key)
+    result = {}
+    for line in lines:
+        m = val_re.match(line)
+        if not m:
+            raise FormatException("unknown output from git config")
+        branch, data = m.groups()
+        result[branch] = data
+    return result
+
 def func(parser, options, args):
 
     if options.create:
@@ -198,11 +215,15 @@ def func(parser, options, args):
         branches = git.get_heads()
         branches.sort()
 
+        descriptions = __get_all_branch_config('description')
+        versions = __get_all_branch_config('stgit.stackformatversion')
+
         if branches:
             out.info('Available branches:')
-            max_len = max([len(i) for i in branches])
-            for i in branches:
-                __print_branch(i, max_len)
+            max_len = max(len(i) for i in branches)
+            for branch in branches:
+                __print_branch(branch, descriptions.get(branch),
+                               versions.get(branch), max_len)
         else:
             out.info('No branches')
         return

git config --get-regexp exit status

[David Kågedal]

David Kågedal <davidk@lysator.liu.se> writes:

> I made a patch that uses "git config --get-regexp" to get the
> description and stgit.stackformatversion options for all branches at
> once, and ignore the "protected" flag that I don't use. With this
> change, I'm almost down to half a second, which almost makes it
> usable.

One thing that annoyed me what that "git config --get-regexp" will
return zero, one, or more matches, which are all valid reponses. But
it treats the zero-match special and return an exit status of 1.

Is that a conscious choice, or just an effect of how "git config
--get" works?

Since zero matches isn't really an error, I would like the exit status
to be 0. At least for this use case :-)

-- 
David Kågedal

Re: [StGit RFC] Make "stg branch -l" faster by getting all git config information in one call

[Catalin Marinas]

On 13/12/2007, David Kågedal <davidk@lysator.liu.se> wrote:
> I have a fair amount of branches, and I noticed that "stg branch -l"
> takes ridiculously long to finish.

I have the same problem.

> Maybe someone can help me find a quicker replacement for the
> get_protected call?

We can have the standard --list command which ignores the protected
flag or even the stgit.formatversion. Just a simple listing of the
branches (that's what I need most of the time). To get the
description, the 's' and 'p' flags, we could use --list-full or
something similar and wait a bit longer. This would also improve the
bash completion of commands taking branch names as arguments.

We had a similar issue in the past with 'series' as it was checking
whether the patch is empty. We ended up adding a '--empty' option for
this case.

-- 
Catalin

Re: [StGit RFC] Make "stg branch -l" faster by getting all git config information in one call

[David Kågedal]

"Catalin Marinas" <catalin.marinas@gmail.com> writes:

> On 13/12/2007, David Kågedal <davidk@lysator.liu.se> wrote:
>> I have a fair amount of branches, and I noticed that "stg branch -l"
>> takes ridiculously long to finish.
>
> I have the same problem.
>
>> Maybe someone can help me find a quicker replacement for the
>> get_protected call?
>
> We can have the standard --list command which ignores the protected
> flag or even the stgit.formatversion. Just a simple listing of the
> branches (that's what I need most of the time). To get the
> description, the 's' and 'p' flags, we could use --list-full or
> something similar and wait a bit longer. This would also improve the
> bash completion of commands taking branch names as arguments.

If you remove everything but the branch name, you will have identical
output to "git branch", but much slower.  Then there is no reason to
use it. The reason I used "stg branch -l" was that I found it
interesting to know which branches I had initialized with stgit, and I
could see uses for the description as well.

-- 
David Kågedal

Re: [StGit RFC] Make "stg branch -l" faster by getting all git config information in one call

[David Kågedal]

"Catalin Marinas" <catalin.marinas@gmail.com> writes:

> On 13/12/2007, David Kågedal <davidk@lysator.liu.se> wrote:
>> I have a fair amount of branches, and I noticed that "stg branch -l"
>> takes ridiculously long to finish.
>
> I have the same problem.
>
>> Maybe someone can help me find a quicker replacement for the
>> get_protected call?

Hey, why not put the "protected" flag in the config? Then we can get
it the same way as the other stuff.

Protecting a branch is a configuration action, so it makes sense to
put it in the config.

-- 
David Kågedal

Re: [StGit RFC] Make "stg branch -l" faster by getting all git config information in one call

[Catalin Marinas]

On 13/12/2007, David Kågedal <davidk@lysator.liu.se> wrote:
> "Catalin Marinas" <catalin.marinas@gmail.com> writes:
>
> > On 13/12/2007, David Kågedal <davidk@lysator.liu.se> wrote:
> >> I have a fair amount of branches, and I noticed that "stg branch -l"
> >> takes ridiculously long to finish.
> >
> > I have the same problem.
> >
> >> Maybe someone can help me find a quicker replacement for the
> >> get_protected call?
>
> Hey, why not put the "protected" flag in the config? Then we can get
> it the same way as the other stuff.
>
> Protecting a branch is a configuration action, so it makes sense to
> put it in the config.

Yes, I'm OK with this. The only problem I see is that we have to
change the stgit.formatversion and provide an upgrade in the Series
object. However, 'stg branch -l' no longer initialises the Series
objects and the upgrade won't happen.

The branch command would have to check format version and force the
upgrade if it isn't the required one.

BTW, have you run stg-prof to check where it spends most of the time?
Is it caused by Python object creation or GIT calls invoked during the
Series objects initialisation. If the latter, we can turn some
variables into properties and access them lazily.

-- 
Catalin

Re: [StGit RFC] Make "stg branch -l" faster by getting all git config information in one call

[Karl Hasselström]

On 2007-12-13 14:04:26 +0000, Catalin Marinas wrote:

> On 13/12/2007, David Kågedal <davidk@lysator.liu.se> wrote:
>
> > Maybe someone can help me find a quicker replacement for the
> > get_protected call?
>
> We can have the standard --list command which ignores the protected
> flag

Exactly what is the p flag useful for anyway?

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

Re: [StGit RFC] Make "stg branch -l" faster by getting all git config information in one call

[Karl Hasselström]

On 2007-12-13 15:31:24 +0100, David Kågedal wrote:

> Hey, why not put the "protected" flag in the config? Then we can get
> it the same way as the other stuff.
>
> Protecting a branch is a configuration action, so it makes sense to
> put it in the config.

I agree; if we are to have such a flag, the config is the right place
for it.

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

Re: [StGit RFC] Make "stg branch -l" faster by getting all git config information in one call

[David Kågedal]

"Catalin Marinas" <catalin.marinas@gmail.com> writes:

> BTW, have you run stg-prof to check where it spends most of the time?
> Is it caused by Python object creation or GIT calls invoked during the
> Series objects initialisation. If the latter, we can turn some
> variables into properties and access them lazily.

I ran with STG_SUBPROCESS_LOG=debug and noticed that there were four
invokations of git for each branch it listed, in addition to the ten
invokation before it even starts to list the branches.

So I focused on replacing those 4xN invokations with 2 invokations,
and it helped a lot.  But I'm sure there are still lots of things to
improve.

Actually, I didn't know about stg-prof.

-- 
David Kågedal

Re: [StGit RFC] Make "stg branch -l" faster by getting all git config information in one call

[Catalin Marinas]

On 13/12/2007, Karl Hasselström <kha@treskal.com> wrote:
> On 2007-12-13 14:04:26 +0000, Catalin Marinas wrote:
>
> > On 13/12/2007, David Kågedal <davidk@lysator.liu.se> wrote:
> >
> > > Maybe someone can help me find a quicker replacement for the
> > > get_protected call?
> >
> > We can have the standard --list command which ignores the protected
> > flag
>
> Exactly what is the p flag useful for anyway?

It was added so that you don't rebase the stack by mistake. Yann
suggested to have a specific policy for this and make the protected
flag freeze the stack completely.

-- 
Catalin

Re: [StGit RFC] Make "stg branch -l" faster by getting all git config information in one call

[David Kågedal]

"Catalin Marinas" <catalin.marinas@gmail.com> writes:

> On 13/12/2007, Karl Hasselström <kha@treskal.com> wrote:
>> On 2007-12-13 14:04:26 +0000, Catalin Marinas wrote:
>>
>> > On 13/12/2007, David Kågedal <davidk@lysator.liu.se> wrote:
>> >
>> > > Maybe someone can help me find a quicker replacement for the
>> > > get_protected call?
>> >
>> > We can have the standard --list command which ignores the protected
>> > flag
>>
>> Exactly what is the p flag useful for anyway?
>
> It was added so that you don't rebase the stack by mistake. Yann
> suggested to have a specific policy for this and make the protected
> flag freeze the stack completely.

I'd be much more interested in a flag that prevents me from running
"git rebase" on a stg-controlled branch by mistake...

-- 
David Kågedal

Re: [StGit RFC] Make "stg branch -l" faster by getting all git config information in one call

[Karl Hasselström]

On 2007-12-13 17:39:49 +0100, David Kågedal wrote:

> I'd be much more interested in a flag that prevents me from running
> "git rebase" on a stg-controlled branch by mistake...

In the long run, we probably want to figure out a good set of hooks in
git that would let us know when interesting stuff happens. And then
add them. The challenge will be to keep the number of hooks down.

In the short run, we need a general "undo" command. (I've got one on
the drawing board. Just need to set aside some time ...)

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

Re: git config --get-regexp exit status

[Junio C Hamano]

David Kågedal <davidk@lysator.liu.se> writes:

> David Kågedal <davidk@lysator.liu.se> writes:
> ...
> One thing that annoyed me what that "git config --get-regexp" will
> return zero, one, or more matches, which are all valid reponses. But
> it treats the zero-match special and return an exit status of 1.
>
> Is that a conscious choice, or just an effect of how "git config
> --get" works?
>
> Since zero matches isn't really an error, I would like the exit status
> to be 0. At least for this use case :-)

I would imagine "matching variant" may want to signal "I did not match
anything" as a special event, just like grep does.  As long as the
caller can distinguish "no match" from real failure cases (say, not in a
git repository and hence no configuration file), that is one valid use
of status code.

I haven't been annoyed by that myself, but I see what you mean.  "give
me all the matching ones" callers would want to consider zero match and
seven matches both the same way as non errors.  And similarity to "grep"
does not extend very far because there is no "--get-regexp" variant that
is quiet (i.e. "no output, just checking if there is a match").

So I do not mind if --get-regexp considered zero matches as non-error
myself.  If an existing script did something like:

	if list=$(git config --get-regexp '$pattern')
        then
        	# use list, without verifying if it is empty
                ...
	fi

such a change would break it, though.  But the script should have been
doing:

	LF='
	'
	if list=$(git config --get-regexp '$pattern')
        then
		IFS="$LF"
		for $list
                do
                	...
		done
	fi

anyway, so...