threads / rfc / 11273

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

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

## tl;dr

13 messages between Dec 13, 2007 and Dec 13, 2007.

replies: 12people: 4as markdown or json

David Kågedal· Dec 13, 2007, 13:38 UTC · lore
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
David Kågedal· Dec 13, 2007, 13:42 UTC · re: David Kågedal · lore

git config --get-regexp exit status

David Kågedal <davidk@lysator.liu.se> writes:
Show 5 quoted lines
> 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
Junio C Hamano· Dec 13, 2007, 18:06 UTC · re: David Kågedal · lore

Re: git config --get-regexp exit status

David Kågedal <davidk@lysator.liu.se> writes:
Show 11 quoted lines
> 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...
Catalin Marinas· Dec 13, 2007, 14:04 UTC · re: David Kågedal · lore

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

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
David Kågedal· Dec 13, 2007, 14:15 UTC · re: Catalin Marinas · lore

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

"Catalin Marinas" <catalin.marinas@gmail.com> writes:
Show 15 quoted lines
> 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
David Kågedal· Dec 13, 2007, 14:31 UTC · re: Catalin Marinas · lore

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

"Catalin Marinas" <catalin.marinas@gmail.com> writes:
Show 8 quoted lines
> 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
Catalin Marinas· Dec 13, 2007, 15:38 UTC · re: David Kågedal · lore

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

On 13/12/2007, David Kågedal <davidk@lysator.liu.se> wrote:
Show 16 quoted lines
> "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
David Kågedal· Dec 13, 2007, 16:08 UTC · re: Catalin Marinas · lore

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

"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
Karl Hasselström· Dec 13, 2007, 16:08 UTC · re: David Kågedal · lore

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

On 2007-12-13 15:31:24 +0100, David Kågedal wrote:
Show 5 quoted lines
> 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
Karl Hasselström· Dec 13, 2007, 16:04 UTC · re: Catalin Marinas · lore

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

On 2007-12-13 14:04:26 +0000, Catalin Marinas wrote:
Show 7 quoted lines
> 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
Catalin Marinas· Dec 13, 2007, 16:10 UTC · re: Karl Hasselström · lore

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

On 13/12/2007, Karl Hasselström <kha@treskal.com> wrote:
Show 11 quoted lines
> 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
David Kågedal· Dec 13, 2007, 16:39 UTC · re: Catalin Marinas · lore

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

"Catalin Marinas" <catalin.marinas@gmail.com> writes:
Show 16 quoted lines
> 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
Karl Hasselström· Dec 13, 2007, 17:04 UTC · re: David Kågedal · lore

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

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

← back to recent threads