[StGit PATCH 00/13] Eliminate 'top' and 'bottom' files

[StGit PATCH 00/13] Eliminate 'top' and 'bottom' files

[David Kågedal]

The following series removes the 'bottom' and 'top' files for each
patch, and instead uses the commit objects to keep track of the
patches.

The patches are based on kha/safe
24a81d7a94cd7c9ad2fc741b0179db5b830cce78 and will conflict with the
'conflict' series in at least one place.

The first ten patches are actually only cleanups and refactoring that
could go in regardless of the last ones.  Some of the changes are not
really necessary, but I did them while digging into the the code, and
believe they are improvements. They should not change the way stg
behaves in any way.

The eleventh one is just a sanity check before applying the last two
that removes the top and bottom files.

The last two patches do the final cleansing.  Obviously, this changes
the format, and the format version should be increased and and update
function be written.  So it's not really ready to go in yet.

Also, there are some documentation changes not included in this.

Maybe I should have sent the first ten patches separately, but this
makes it clearer why I did it.

---

David Kågedal (13):
      Remove the 'top' field
      Remove the 'bottom' field
      Check bottom and invariants
      Refactor Series.new_patch
      Clear up the semantics of Series.new_patch
      Add a 'bottom' parameter to Series.refresh_patch and use it
      Clean up Series.refresh_patch
      Refactor Series.push_patch
      Remove dead code from push_empty_patch
      Split Series.push_patch in two
      Moved that status function to the status command file
      Clear up semantics of tree_status
      Add some more tests of "stg status" output


 stgit/commands/common.py   |    2 
 stgit/commands/status.py   |   42 +++++++++-
 stgit/commands/sync.py     |    1 
 stgit/commands/uncommit.py |    1 
 stgit/git.py               |   61 +++-----------
 stgit/stack.py             |  189 ++++++++++++++++++++++----------------------
 t/t0002-status.sh          |   36 ++++++++
 7 files changed, 185 insertions(+), 147 deletions(-)

-- 
Signature

[StGit PATCH 01/13] Add some more tests of "stg status" output

[David Kågedal]

Signed-off-by: David Kågedal <davidk@lysator.liu.se>
---

 t/t0002-status.sh |   28 ++++++++++++++++++++++++++++
 1 files changed, 28 insertions(+), 0 deletions(-)


diff --git a/t/t0002-status.sh b/t/t0002-status.sh
index ce3b688..790b9fb 100755
--- a/t/t0002-status.sh
+++ b/t/t0002-status.sh
@@ -85,6 +85,12 @@ test_expect_success 'Status after refresh' '
     diff -u expected.txt output.txt
 '
 
+test_expect_success 'Add another file' '
+    echo lajbans > fie &&
+    stg add fie &&
+    stg refresh
+'
+
 test_expect_success 'Make a conflicting patch' '
     stg pop &&
     stg new -m "third patch" &&
@@ -105,6 +111,28 @@ test_expect_success 'Status after conflicting push' '
 '
 
 cat > expected.txt <<EOF
+C foo/bar
+EOF
+test_expect_success 'Status of file' '
+    stg status foo/bar > output.txt &&
+    diff -u expected.txt output.txt
+'
+
+cat > expected.txt <<EOF
+EOF
+test_expect_success 'Status of dir' '
+    stg status foo > output.txt &&
+    diff -u expected.txt output.txt
+'
+
+cat > expected.txt <<EOF
+EOF
+test_expect_success 'Status of other file' '
+    stg status fie > output.txt &&
+    diff -u expected.txt output.txt
+'
+
+cat > expected.txt <<EOF
 M foo/bar
 EOF
 test_expect_success 'Status after resolving the push' '

[StGit PATCH 02/13] Clear up semantics of tree_status

[David Kågedal]

The tree_status() function does a few slightly different things
depending on the arguments.  This patch adds checks that the arguments
are consistent and that the returned value looks good.

It also changes the semantics slightly.  If the 'files' parameter is
None, it will run status on all files.  If 'files' is a list, it will
run status on only those files.  This changes two things:

 1) If 'files' is the empty list, it will run status on no files.
 2) It 'files' is a list, it will never return the status for other files

Clearing this up will make it easier to understand code that is using
this function.

Signed-off-by: David Kågedal <davidk@lysator.liu.se>
---

 stgit/commands/status.py |    3 +++
 stgit/git.py             |   44 +++++++++++++++++++++++---------------------
 2 files changed, 26 insertions(+), 21 deletions(-)


diff --git a/stgit/commands/status.py b/stgit/commands/status.py
index 156552b..de88ba0 100644
--- a/stgit/commands/status.py
+++ b/stgit/commands/status.py
@@ -82,6 +82,9 @@ def func(parser, options, args):
         else:
             diff_flags = []
 
+        # No args means all files
+        if not args:
+            args = None
         git.status(args, options.modified, options.new, options.deleted,
                    options.conflict, options.unknown, options.noexclude,
                    diff_flags = diff_flags)
diff --git a/stgit/git.py b/stgit/git.py
index 539d699..8dda1ed 100644
--- a/stgit/git.py
+++ b/stgit/git.py
@@ -167,15 +167,20 @@ def exclude_files():
 
 def tree_status(files = None, tree_id = 'HEAD', unknown = False,
                   noexclude = True, verbose = False, diff_flags = []):
-    """Returns a list of pairs - (status, filename)
+    """Get the status of all changed files, or of a selected set of
+    files. Returns a list of pairs - (status, filename).
+
+    If 'files' is None, it will check all files, and optionally all
+    unknown files.  If 'files' is a list, it will only check the files
+    in the list.
     """
+    assert files == None or not unknown
+
     if verbose:
         out.start('Checking for changes in the working directory')
 
     refresh_index()
 
-    if not files:
-        files = []
     cache_files = []
 
     # unknown files
@@ -197,11 +202,14 @@ def tree_status(files = None, tree_id = 'HEAD', unknown = False,
     conflicts = get_conflicts()
     if not conflicts:
         conflicts = []
-    cache_files += [('C', filename) for filename in conflicts]
+    cache_files += [('C', filename) for filename in conflicts
+                    if files == None or filename in files]
 
     # the rest
-    for line in GRun('git-diff-index', *(diff_flags + [tree_id, '--'] + files)
-                     ).output_lines():
+    args = diff_flags + [tree_id]
+    if files != None:
+        args += ['--'] + files
+    for line in GRun('git-diff-index', *args).output_lines():
         fs = tuple(line.rstrip().split(' ',4)[-1].split('\t',1))
         if fs[1] not in conflicts:
             cache_files.append(fs)
@@ -209,6 +217,7 @@ def tree_status(files = None, tree_id = 'HEAD', unknown = False,
     if verbose:
         out.done()
 
+    assert files == None or set(f for s,f in cache_files) <= set(files)
     return cache_files
 
 def local_changes(verbose = True):
@@ -538,9 +547,6 @@ def committer():
 def update_cache(files = None, force = False):
     """Update the cache information for the given files
     """
-    if not files:
-        files = []
-
     cache_files = tree_status(files, verbose = False)
 
     # everything is up-to-date
@@ -569,8 +575,6 @@ def commit(message, files = None, parents = None, allowempty = False,
            committer_name = None, committer_email = None):
     """Commit the current tree to repository
     """
-    if not files:
-        files = []
     if not parents:
         parents = []
 
@@ -712,14 +716,13 @@ def status(files = None, modified = False, new = False, deleted = False,
            diff_flags = []):
     """Show the tree status
     """
-    if not files:
-        files = []
-
-    cache_files = tree_status(files, unknown = True, noexclude = noexclude,
-                                diff_flags = diff_flags)
-    all = not (modified or new or deleted or conflict or unknown)
+    cache_files = tree_status(files,
+                              unknown = (files == None),
+                              noexclude = noexclude,
+                              diff_flags = diff_flags)
+    filtered = (modified or new or deleted or conflict or unknown)
 
-    if not all:
+    if filtered:
         filestat = []
         if modified:
             filestat.append('M')
@@ -735,9 +738,8 @@ def status(files = None, modified = False, new = False, deleted = False,
         cache_files = [x for x in cache_files if x[0] in filestat]
 
     for fs in cache_files:
-        if files and not fs[1] in files:
-            continue
-        if all:
+        assert files == None or fs[1] in files
+        if not filtered:
             out.stdout('%s %s' % (fs[0], fs[1]))
         else:
             out.stdout('%s' % fs[1])

[StGit PATCH 03/13] Moved that status function to the status command file

[David Kågedal]

The git.status() was more of a command than a library function, and
was only used in one place.

Signed-off-by: David Kågedal <davidk@lysator.liu.se>
---

 stgit/commands/status.py |   39 ++++++++++++++++++++++++++++++++++++---
 stgit/git.py             |   33 ---------------------------------
 t/t0002-status.sh        |    8 ++++++++
 3 files changed, 44 insertions(+), 36 deletions(-)


diff --git a/stgit/commands/status.py b/stgit/commands/status.py
index de88ba0..bbfb5df 100644
--- a/stgit/commands/status.py
+++ b/stgit/commands/status.py
@@ -65,6 +65,39 @@ options = [make_option('-m', '--modified',
                        action = 'store_true')]
 
 
+def status(files = None, modified = False, new = False, deleted = False,
+           conflict = False, unknown = False, noexclude = False,
+           diff_flags = []):
+    """Show the tree status
+    """
+    cache_files = git.tree_status(files,
+                                  unknown = (files == None),
+                                  noexclude = noexclude,
+                                  diff_flags = diff_flags)
+    filtered = (modified or new or deleted or conflict or unknown)
+
+    if filtered:
+        filestat = []
+        if modified:
+            filestat.append('M')
+        if new:
+            filestat.append('A')
+            filestat.append('N')
+        if deleted:
+            filestat.append('D')
+        if conflict:
+            filestat.append('C')
+        if unknown:
+            filestat.append('?')
+        cache_files = [x for x in cache_files if x[0] in filestat]
+
+    for fs in cache_files:
+        assert files == None or fs[1] in files
+        if not filtered:
+            out.stdout('%s %s' % (fs[0], fs[1]))
+        else:
+            out.stdout('%s' % fs[1])
+
 def func(parser, options, args):
     """Show the tree status
     """
@@ -85,6 +118,6 @@ def func(parser, options, args):
         # No args means all files
         if not args:
             args = None
-        git.status(args, options.modified, options.new, options.deleted,
-                   options.conflict, options.unknown, options.noexclude,
-                   diff_flags = diff_flags)
+        status(args, options.modified, options.new, options.deleted,
+               options.conflict, options.unknown, options.noexclude,
+               diff_flags = diff_flags)
diff --git a/stgit/git.py b/stgit/git.py
index 8dda1ed..b112787 100644
--- a/stgit/git.py
+++ b/stgit/git.py
@@ -711,39 +711,6 @@ def merge(base, head1, head2, recursive = False):
     if errors:
         raise GitException, 'GIT index merging failed (possible conflicts)'
 
-def status(files = None, modified = False, new = False, deleted = False,
-           conflict = False, unknown = False, noexclude = False,
-           diff_flags = []):
-    """Show the tree status
-    """
-    cache_files = tree_status(files,
-                              unknown = (files == None),
-                              noexclude = noexclude,
-                              diff_flags = diff_flags)
-    filtered = (modified or new or deleted or conflict or unknown)
-
-    if filtered:
-        filestat = []
-        if modified:
-            filestat.append('M')
-        if new:
-            filestat.append('A')
-            filestat.append('N')
-        if deleted:
-            filestat.append('D')
-        if conflict:
-            filestat.append('C')
-        if unknown:
-            filestat.append('?')
-        cache_files = [x for x in cache_files if x[0] in filestat]
-
-    for fs in cache_files:
-        assert files == None or fs[1] in files
-        if not filtered:
-            out.stdout('%s %s' % (fs[0], fs[1]))
-        else:
-            out.stdout('%s' % fs[1])
-
 def diff(files = None, rev1 = 'HEAD', rev2 = None, diff_flags = []):
     """Show the diff between rev1 and rev2
     """
diff --git a/t/t0002-status.sh b/t/t0002-status.sh
index 790b9fb..d0c31b2 100755
--- a/t/t0002-status.sh
+++ b/t/t0002-status.sh
@@ -60,6 +60,14 @@ test_expect_success 'Status with an added file' '
 '
 
 cat > expected.txt <<EOF
+foo/bar
+EOF
+test_expect_success 'Status with an added file and -n option' '
+    stg status -n > output.txt &&
+    diff -u expected.txt output.txt
+'
+
+cat > expected.txt <<EOF
 EOF
 test_expect_success 'Status after refresh' '
     stg new -m "first patch" &&

[StGit PATCH 04/13] Split Series.push_patch in two

[David Kågedal]

The push_patch() function has a complex control flow and actually does
two different things depending on the 'empty' parameter. This patch
splits in in two functions without other code changes.

Later patches will refactor the code to simplify it.

Signed-off-by: David Kågedal <davidk@lysator.liu.se>
---

 stgit/commands/common.py |    2 +
 stgit/stack.py           |   62 ++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 52 insertions(+), 12 deletions(-)


diff --git a/stgit/commands/common.py b/stgit/commands/common.py
index f3fa89d..eaaf5fc 100644
--- a/stgit/commands/common.py
+++ b/stgit/commands/common.py
@@ -184,7 +184,7 @@ def push_patches(patches, check_merged = False):
         out.start('Pushing patch "%s"' % p)
 
         if p in merged:
-            crt_series.push_patch(p, empty = True)
+            crt_series.push_empty_patch(p)
             out.done('merged upstream')
         else:
             modified = crt_series.push_patch(p)
diff --git a/stgit/stack.py b/stgit/stack.py
index 906e6b1..79d6cf3 100644
--- a/stgit/stack.py
+++ b/stgit/stack.py
@@ -1022,7 +1022,55 @@ class Series(PatchSet):
 
         return merged
 
-    def push_patch(self, name, empty = False):
+    def push_empty_patch(self, name):
+        """Pushes an empty patch on the stack
+        """
+        unapplied = self.get_unapplied()
+        assert(name in unapplied)
+
+        patch = self.get_patch(name)
+
+        head = git.get_head()
+        bottom = patch.get_bottom()
+        top = patch.get_top()
+
+        ex = None
+        modified = False
+
+        # top != bottom always since we have a commit for each patch
+        # just make an empty patch (top = bottom = HEAD). This
+        # option is useful to allow undoing already merged
+        # patches. The top is updated by refresh_patch since we
+        # need an empty commit
+        patch.set_bottom(head, backup = True)
+        patch.set_top(head, backup = True)
+        modified = True
+
+        append_string(self.__applied_file, name)
+
+        unapplied.remove(name)
+        write_strings(self.__unapplied_file, unapplied)
+
+        # head == bottom case doesn't need to refresh the patch
+        if not ex:
+            # if the merge was OK and no conflicts, just refresh the patch
+            # The GIT cache was already updated by the merge operation
+            if modified:
+                log = 'push(m)'
+            else:
+                log = 'push'
+            self.refresh_patch(cache_update = False, log = log)
+        else:
+            # we store the correctly merged files only for
+            # tracking the conflict history. Note that the
+            # git.merge() operations should always leave the index
+            # in a valid state (i.e. only stage 0 files)
+            self.refresh_patch(cache_update = False, log = 'push(c)')
+            raise StackException, str(ex)
+
+        return modified
+
+    def push_patch(self, name):
         """Pushes a patch on the stack
         """
         unapplied = self.get_unapplied()
@@ -1038,15 +1086,7 @@ class Series(PatchSet):
         modified = False
 
         # top != bottom always since we have a commit for each patch
-        if empty:
-            # just make an empty patch (top = bottom = HEAD). This
-            # option is useful to allow undoing already merged
-            # patches. The top is updated by refresh_patch since we
-            # need an empty commit
-            patch.set_bottom(head, backup = True)
-            patch.set_top(head, backup = True)
-            modified = True
-        elif head == bottom:
+        if head == bottom:
             # reset the backup information. No need for logging
             patch.set_bottom(bottom, backup = True)
             patch.set_top(top, backup = True)
@@ -1079,7 +1119,7 @@ class Series(PatchSet):
         write_strings(self.__unapplied_file, unapplied)
 
         # head == bottom case doesn't need to refresh the patch
-        if empty or head != bottom:
+        if head != bottom:
             if not ex:
                 # if the merge was OK and no conflicts, just refresh the patch
                 # The GIT cache was already updated by the merge operation

[StGit PATCH 05/13] Remove dead code from push_empty_patch

[David Kågedal]

Since the split from push_patch, the push_empty_patch contains some
code that fills no purpose. Remove it and simplify the code.

Signed-off-by: David Kågedal <davidk@lysator.liu.se>
---

 stgit/stack.py |   33 +++------------------------------
 1 files changed, 3 insertions(+), 30 deletions(-)


diff --git a/stgit/stack.py b/stgit/stack.py
index 79d6cf3..f31f308 100644
--- a/stgit/stack.py
+++ b/stgit/stack.py
@@ -1029,46 +1029,19 @@ class Series(PatchSet):
         assert(name in unapplied)
 
         patch = self.get_patch(name)
-
         head = git.get_head()
-        bottom = patch.get_bottom()
-        top = patch.get_top()
-
-        ex = None
-        modified = False
 
-        # top != bottom always since we have a commit for each patch
-        # just make an empty patch (top = bottom = HEAD). This
-        # option is useful to allow undoing already merged
-        # patches. The top is updated by refresh_patch since we
-        # need an empty commit
+        # The top is updated by refresh_patch since we need an empty
+        # commit
         patch.set_bottom(head, backup = True)
         patch.set_top(head, backup = True)
-        modified = True
 
         append_string(self.__applied_file, name)
 
         unapplied.remove(name)
         write_strings(self.__unapplied_file, unapplied)
 
-        # head == bottom case doesn't need to refresh the patch
-        if not ex:
-            # if the merge was OK and no conflicts, just refresh the patch
-            # The GIT cache was already updated by the merge operation
-            if modified:
-                log = 'push(m)'
-            else:
-                log = 'push'
-            self.refresh_patch(cache_update = False, log = log)
-        else:
-            # we store the correctly merged files only for
-            # tracking the conflict history. Note that the
-            # git.merge() operations should always leave the index
-            # in a valid state (i.e. only stage 0 files)
-            self.refresh_patch(cache_update = False, log = 'push(c)')
-            raise StackException, str(ex)
-
-        return modified
+        self.refresh_patch(cache_update = False, log = 'push(m)')
 
     def push_patch(self, name):
         """Pushes a patch on the stack

[StGit PATCH 06/13] Refactor Series.push_patch

[David Kågedal]

Refactor the Series.push_patch function to make the code flow
simpler. It identifies the simple case and handles it early, and
reduces the number of if statements.

Most changes are simply indentation changes.

Signed-off-by: David Kågedal <davidk@lysator.liu.se>
---

 stgit/stack.py |   87 ++++++++++++++++++++++++++++++--------------------------
 1 files changed, 46 insertions(+), 41 deletions(-)


diff --git a/stgit/stack.py b/stgit/stack.py
index f31f308..9b8ed6e 100644
--- a/stgit/stack.py
+++ b/stgit/stack.py
@@ -1054,60 +1054,65 @@ class Series(PatchSet):
         head = git.get_head()
         bottom = patch.get_bottom()
         top = patch.get_top()
-
-        ex = None
-        modified = False
-
         # top != bottom always since we have a commit for each patch
+
         if head == bottom:
-            # reset the backup information. No need for logging
+            # A fast-forward push. Just reset the backup
+            # information. No need for logging
             patch.set_bottom(bottom, backup = True)
             patch.set_top(top, backup = True)
 
             git.switch(top)
-        else:
-            # new patch needs to be refreshed.
-            # The current patch is empty after merge.
-            patch.set_bottom(head, backup = True)
-            patch.set_top(head, backup = True)
-
-            # Try the fast applying first. If this fails, fall back to the
-            # three-way merge
-            if not git.apply_diff(bottom, top):
-                # if git.apply_diff() fails, the patch requires a diff3
-                # merge and can be reported as modified
-                modified = True
-
-                # merge can fail but the patch needs to be pushed
-                try:
-                    git.merge(bottom, head, top, recursive = True)
-                except git.GitException, ex:
-                    out.error('The merge failed during "push".',
-                              'Use "refresh" after fixing the conflicts or'
-                              ' revert the operation with "push --undo".')
+            append_string(self.__applied_file, name)
+
+            unapplied.remove(name)
+            write_strings(self.__unapplied_file, unapplied)
+            return False
+
+        # Need to create a new commit an merge in the old patch
+        ex = None
+        modified = False
+
+        # new patch needs to be refreshed.
+        # The current patch is empty after merge.
+        patch.set_bottom(head, backup = True)
+        patch.set_top(head, backup = True)
+
+        # Try the fast applying first. If this fails, fall back to the
+        # three-way merge
+        if not git.apply_diff(bottom, top):
+            # if git.apply_diff() fails, the patch requires a diff3
+            # merge and can be reported as modified
+            modified = True
+
+            # merge can fail but the patch needs to be pushed
+            try:
+                git.merge(bottom, head, top, recursive = True)
+            except git.GitException, ex:
+                out.error('The merge failed during "push".',
+                          'Use "refresh" after fixing the conflicts or'
+                          ' revert the operation with "push --undo".')
 
         append_string(self.__applied_file, name)
 
         unapplied.remove(name)
         write_strings(self.__unapplied_file, unapplied)
 
-        # head == bottom case doesn't need to refresh the patch
-        if head != bottom:
-            if not ex:
-                # if the merge was OK and no conflicts, just refresh the patch
-                # The GIT cache was already updated by the merge operation
-                if modified:
-                    log = 'push(m)'
-                else:
-                    log = 'push'
-                self.refresh_patch(cache_update = False, log = log)
+        if not ex:
+            # if the merge was OK and no conflicts, just refresh the patch
+            # The GIT cache was already updated by the merge operation
+            if modified:
+                log = 'push(m)'
             else:
-                # we store the correctly merged files only for
-                # tracking the conflict history. Note that the
-                # git.merge() operations should always leave the index
-                # in a valid state (i.e. only stage 0 files)
-                self.refresh_patch(cache_update = False, log = 'push(c)')
-                raise StackException, str(ex)
+                log = 'push'
+            self.refresh_patch(cache_update = False, log = log)
+        else:
+            # we store the correctly merged files only for
+            # tracking the conflict history. Note that the
+            # git.merge() operations should always leave the index
+            # in a valid state (i.e. only stage 0 files)
+            self.refresh_patch(cache_update = False, log = 'push(c)')
+            raise StackException, str(ex)
 
         return modified
 

[StGit PATCH 07/13] Clean up Series.refresh_patch

[David Kågedal]

This patch does some minor simplifications of the code and updates the
documentation string of Series.refresh_patch.

Signed-off-by: David Kågedal <davidk@lysator.liu.se>
---

 stgit/stack.py |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)


diff --git a/stgit/stack.py b/stgit/stack.py
index 9b8ed6e..03ce218 100644
--- a/stgit/stack.py
+++ b/stgit/stack.py
@@ -749,14 +749,12 @@ class Series(PatchSet):
                       committer_name = None, committer_email = None,
                       backup = False, sign_str = None, log = 'refresh',
                       notes = None):
-        """Generates a new commit for the given patch
+        """Generates a new commit for the topmost patch
         """
-        name = self.get_current()
-        if not name:
+        patch = self.get_current_patch()
+        if not patch:
             raise StackException, 'No patches applied'
 
-        patch = self.get_patch(name)
-
         descr = patch.get_description()
         if not (message or descr):
             edit = True
@@ -767,7 +765,7 @@ class Series(PatchSet):
         if not message and edit:
             descr = edit_file(self, descr.rstrip(), \
                               'Please edit the description for patch "%s" ' \
-                              'above.' % name, show_patch)
+                              'above.' % patch.get_name(), show_patch)
 
         if not author_name:
             author_name = patch.get_authname()

[StGit PATCH 08/13] Add a 'bottom' parameter to Series.refresh_patch and use it

[David Kågedal]

By specifying a bottom for the new patch commit, it is no longer
necessary to update the bottom of the patch before calling
refresh_patch.  This ensures that the patch top always correspond to a
commit object, and the bottom to its parent.

Signed-off-by: David Kågedal <davidk@lysator.liu.se>
---

 stgit/stack.py |   24 ++++++++----------------
 1 files changed, 8 insertions(+), 16 deletions(-)


diff --git a/stgit/stack.py b/stgit/stack.py
index 03ce218..fd19a82 100644
--- a/stgit/stack.py
+++ b/stgit/stack.py
@@ -748,7 +748,7 @@ class Series(PatchSet):
                       author_date = None,
                       committer_name = None, committer_email = None,
                       backup = False, sign_str = None, log = 'refresh',
-                      notes = None):
+                      notes = None, bottom = None):
         """Generates a new commit for the topmost patch
         """
         patch = self.get_current_patch()
@@ -780,7 +780,8 @@ class Series(PatchSet):
 
         descr = add_sign_line(descr, sign_str, committer_name, committer_email)
 
-        bottom = patch.get_bottom()
+        if not bottom:
+            bottom = patch.get_bottom()
 
         commit_id = git.commit(files = files,
                                message = descr, parents = [bottom],
@@ -1026,20 +1027,15 @@ class Series(PatchSet):
         unapplied = self.get_unapplied()
         assert(name in unapplied)
 
-        patch = self.get_patch(name)
+        # patch = self.get_patch(name)
         head = git.get_head()
 
-        # The top is updated by refresh_patch since we need an empty
-        # commit
-        patch.set_bottom(head, backup = True)
-        patch.set_top(head, backup = True)
-
         append_string(self.__applied_file, name)
 
         unapplied.remove(name)
         write_strings(self.__unapplied_file, unapplied)
 
-        self.refresh_patch(cache_update = False, log = 'push(m)')
+        self.refresh_patch(bottom = head, cache_update = False, log = 'push(m)')
 
     def push_patch(self, name):
         """Pushes a patch on the stack
@@ -1071,11 +1067,6 @@ class Series(PatchSet):
         ex = None
         modified = False
 
-        # new patch needs to be refreshed.
-        # The current patch is empty after merge.
-        patch.set_bottom(head, backup = True)
-        patch.set_top(head, backup = True)
-
         # Try the fast applying first. If this fails, fall back to the
         # three-way merge
         if not git.apply_diff(bottom, top):
@@ -1103,13 +1094,14 @@ class Series(PatchSet):
                 log = 'push(m)'
             else:
                 log = 'push'
-            self.refresh_patch(cache_update = False, log = log)
+            self.refresh_patch(bottom = head, cache_update = False, log = log)
         else:
             # we store the correctly merged files only for
             # tracking the conflict history. Note that the
             # git.merge() operations should always leave the index
             # in a valid state (i.e. only stage 0 files)
-            self.refresh_patch(cache_update = False, log = 'push(c)')
+            self.refresh_patch(bottom = head, cache_update = False,
+                               log = 'push(c)')
             raise StackException, str(ex)
 
         return modified

[StGit PATCH 09/13] Clear up the semantics of Series.new_patch

[David Kågedal]

This patch adds a number of assertions to document and verify the
complex restrictions of the input parameters to the Series.new_patch
function. It also adds the requirement that 'before_existing' and
'commit' cannot be true at the same time when calling it, instead of
updating 'commit' inside the function.

Signed-off-by: David Kågedal <davidk@lysator.liu.se>
---

 stgit/commands/uncommit.py |    1 +
 stgit/stack.py             |   14 ++++++++++----
 2 files changed, 11 insertions(+), 4 deletions(-)


diff --git a/stgit/commands/uncommit.py b/stgit/commands/uncommit.py
index 3cf2f0a..0cd0fb0 100644
--- a/stgit/commands/uncommit.py
+++ b/stgit/commands/uncommit.py
@@ -129,6 +129,7 @@ def func(parser, options, args):
                      name_email_date(commit.get_author())
         crt_series.new_patch(patchname,
                              can_edit = False, before_existing = True,
+                             commit = False,
                              top = commit_id, bottom = parent,
                              message = commit.get_log(),
                              author_name = author_name,
diff --git a/stgit/stack.py b/stgit/stack.py
index fd19a82..733a241 100644
--- a/stgit/stack.py
+++ b/stgit/stack.py
@@ -833,9 +833,16 @@ class Series(PatchSet):
                   author_name = None, author_email = None, author_date = None,
                   committer_name = None, committer_email = None,
                   before_existing = False):
-        """Creates a new patch
+        """Creates a new patch, either pointing to an existing commit object,
+        or by creating a new commit object.
         """
 
+        assert commit or (top and bottom)
+        assert not before_existing or (top and bottom)
+        assert not (commit and before_existing)
+        assert (top and bottom) or (not top and not bottom)
+        assert not top or (bottom == git.get_commit(top).get_parent())
+
         if name != None:
             self.__patch_name_valid(name)
             if self.patch_exists(name):
@@ -873,9 +880,6 @@ class Series(PatchSet):
 
         if before_existing:
             insert_string(self.__applied_file, patch.get_name())
-            # no need to commit anything as the object is already
-            # present (mainly used by 'uncommit')
-            commit = False
         elif unapplied:
             patches = [patch.get_name()] + self.get_unapplied()
             write_strings(self.__unapplied_file, patches)
@@ -900,6 +904,8 @@ class Series(PatchSet):
                                    committer_email = committer_email)
             # set the patch top to the new commit
             patch.set_top(commit_id)
+        else:
+            assert top != bottom
 
         self.log_patch(patch, 'new')
 

[StGit PATCH 10/13] Refactor Series.new_patch

[David Kågedal]

This shuffles some code so that the top and bottom never need to be
set to anything other than a valid commit and its parent.

Signed-off-by: David Kågedal <davidk@lysator.liu.se>
---

 stgit/stack.py |   17 +++++++++--------
 1 files changed, 9 insertions(+), 8 deletions(-)


diff --git a/stgit/stack.py b/stgit/stack.py
index 733a241..fdff5a4 100644
--- a/stgit/stack.py
+++ b/stgit/stack.py
@@ -864,13 +864,6 @@ class Series(PatchSet):
         patch = self.get_patch(name)
         patch.create()
 
-        if not bottom:
-            bottom = head
-        if not top:
-            top = head
-
-        patch.set_bottom(bottom)
-        patch.set_top(top)
         patch.set_description(descr)
         patch.set_authname(author_name)
         patch.set_authemail(author_email)
@@ -889,10 +882,15 @@ class Series(PatchSet):
             set_head = True
 
         if commit:
+            if top:
+                top_commit = git.get_commit(top)
+            else:
+                bottom = head
+                top_commit = git.get_commit(head)
+
             # create a commit for the patch (may be empty if top == bottom);
             # only commit on top of the current branch
             assert(unapplied or bottom == head)
-            top_commit = git.get_commit(top)
             commit_id = git.commit(message = descr, parents = [bottom],
                                    cache_update = False,
                                    tree_id = top_commit.get_tree(),
@@ -903,9 +901,12 @@ class Series(PatchSet):
                                    committer_name = committer_name,
                                    committer_email = committer_email)
             # set the patch top to the new commit
+            patch.set_bottom(bottom)
             patch.set_top(commit_id)
         else:
             assert top != bottom
+            patch.set_bottom(bottom)
+            patch.set_top(top)
 
         self.log_patch(patch, 'new')
 

[StGit PATCH 11/13] Check bottom and invariants

[David Kågedal]

This code adds some checks that the bottom is actually always the
parent of top.

It also checks that the top is the same as what the patch ref points
to.

This is only do to ensure that the next patches are correct.

Signed-off-by: David Kågedal <davidk@lysator.liu.se>
---

 stgit/stack.py |   17 +++++++++++++++--
 1 files changed, 15 insertions(+), 2 deletions(-)


diff --git a/stgit/stack.py b/stgit/stack.py
index fdff5a4..d9a2e56 100644
--- a/stgit/stack.py
+++ b/stgit/stack.py
@@ -197,9 +197,15 @@ class Patch(StgitObject):
             self.__update_top_ref(top)
 
     def get_old_bottom(self):
-        return self._get_field('bottom.old')
+        old_bottom = self._get_field('bottom.old')
+        old_top = self.get_old_top()
+        assert old_bottom == git.get_commit(old_top).get_parent()
+        return old_bottom
 
     def get_bottom(self):
+        bottom = self._get_field('bottom')
+        top = self.get_top()
+        assert bottom == git.get_commit(top).get_parent()
         return self._get_field('bottom')
 
     def set_bottom(self, value, backup = False):
@@ -212,7 +218,13 @@ class Patch(StgitObject):
         return self._get_field('top.old')
 
     def get_top(self):
-        return self._get_field('top')
+        top = self._get_field('top')
+        try:
+            ref = git.rev_parse(self.__top_ref)
+        except:
+            ref = None
+        assert not ref or top == ref
+        return top
 
     def set_top(self, value, backup = False):
         if backup:
@@ -220,6 +232,7 @@ class Patch(StgitObject):
             self._set_field('top.old', curr)
         self._set_field('top', value)
         self.__update_top_ref(value)
+        self.get_bottom() # check the assert
 
     def restore_old_boundaries(self):
         bottom = self._get_field('bottom.old')

[StGit PATCH 12/13] Remove the 'bottom' field

[David Kågedal]

The bottom is instead always calculated from the top by getting its
parent commit.

Signed-off-by: David Kågedal <davidk@lysator.liu.se>
---

 stgit/commands/sync.py |    1 -
 stgit/stack.py         |   29 +++--------------------------
 2 files changed, 3 insertions(+), 27 deletions(-)


diff --git a/stgit/commands/sync.py b/stgit/commands/sync.py
index 580b5bd..c18d7e0 100644
--- a/stgit/commands/sync.py
+++ b/stgit/commands/sync.py
@@ -148,7 +148,6 @@ def func(parser, options, args):
 
         # reset the patch backup information. That's needed in case we
         # undo the sync but there were no changes made
-        patch.set_bottom(bottom, backup = True)
         patch.set_top(top, backup = True)
 
         # the actual merging (either from a branch or an external file)
diff --git a/stgit/stack.py b/stgit/stack.py
index d9a2e56..00b91c6 100644
--- a/stgit/stack.py
+++ b/stgit/stack.py
@@ -158,7 +158,6 @@ class Patch(StgitObject):
 
     def create(self):
         os.mkdir(self._dir())
-        self.create_empty_field('bottom')
         self.create_empty_field('top')
 
     def delete(self):
@@ -197,22 +196,10 @@ class Patch(StgitObject):
             self.__update_top_ref(top)
 
     def get_old_bottom(self):
-        old_bottom = self._get_field('bottom.old')
-        old_top = self.get_old_top()
-        assert old_bottom == git.get_commit(old_top).get_parent()
-        return old_bottom
+        return git.get_commit(self.get_old_top()).get_parent()
 
     def get_bottom(self):
-        bottom = self._get_field('bottom')
-        top = self.get_top()
-        assert bottom == git.get_commit(top).get_parent()
-        return self._get_field('bottom')
-
-    def set_bottom(self, value, backup = False):
-        if backup:
-            curr = self._get_field('bottom')
-            self._set_field('bottom.old', curr)
-        self._set_field('bottom', value)
+        return git.get_commit(self.get_top()).get_parent()
 
     def get_old_top(self):
         return self._get_field('top.old')
@@ -232,14 +219,11 @@ class Patch(StgitObject):
             self._set_field('top.old', curr)
         self._set_field('top', value)
         self.__update_top_ref(value)
-        self.get_bottom() # check the assert
 
     def restore_old_boundaries(self):
-        bottom = self._get_field('bottom.old')
         top = self._get_field('top.old')
 
-        if top and bottom:
-            self._set_field('bottom', bottom)
+        if top:
             self._set_field('top', top)
             self.__update_top_ref(top)
             return True
@@ -806,7 +790,6 @@ class Series(PatchSet):
                                committer_name = committer_name,
                                committer_email = committer_email)
 
-        patch.set_bottom(bottom, backup = backup)
         patch.set_top(commit_id, backup = backup)
         patch.set_description(descr)
         patch.set_authname(author_name)
@@ -914,11 +897,8 @@ class Series(PatchSet):
                                    committer_name = committer_name,
                                    committer_email = committer_email)
             # set the patch top to the new commit
-            patch.set_bottom(bottom)
             patch.set_top(commit_id)
         else:
-            assert top != bottom
-            patch.set_bottom(bottom)
             patch.set_top(top)
 
         self.log_patch(patch, 'new')
@@ -972,7 +952,6 @@ class Series(PatchSet):
             if head == bottom:
                 # reset the backup information. No logging since the
                 # patch hasn't changed
-                patch.set_bottom(head, backup = True)
                 patch.set_top(top, backup = True)
 
             else:
@@ -1000,7 +979,6 @@ class Series(PatchSet):
                                      committer_name = committer_name,
                                      committer_email = committer_email)
 
-                    patch.set_bottom(head, backup = True)
                     patch.set_top(top, backup = True)
 
                     self.log_patch(patch, 'push(f)')
@@ -1073,7 +1051,6 @@ class Series(PatchSet):
         if head == bottom:
             # A fast-forward push. Just reset the backup
             # information. No need for logging
-            patch.set_bottom(bottom, backup = True)
             patch.set_top(top, backup = True)
 
             git.switch(top)

[StGit PATCH 13/13] Remove the 'top' field

[David Kågedal]

The top is instead implicitly defined by the patch ref.

Signed-off-by: David Kågedal <davidk@lysator.liu.se>
---

 stgit/stack.py |   26 +++++++++-----------------
 1 files changed, 9 insertions(+), 17 deletions(-)


diff --git a/stgit/stack.py b/stgit/stack.py
index 00b91c6..94d3eee 100644
--- a/stgit/stack.py
+++ b/stgit/stack.py
@@ -158,7 +158,6 @@ class Patch(StgitObject):
 
     def create(self):
         os.mkdir(self._dir())
-        self.create_empty_field('top')
 
     def delete(self):
         for f in os.listdir(self._dir()):
@@ -190,11 +189,6 @@ class Patch(StgitObject):
     def __update_log_ref(self, ref):
         git.set_ref(self.__log_ref, ref)
 
-    def update_top_ref(self):
-        top = self.get_top()
-        if top:
-            self.__update_top_ref(top)
-
     def get_old_bottom(self):
         return git.get_commit(self.get_old_top()).get_parent()
 
@@ -205,26 +199,18 @@ class Patch(StgitObject):
         return self._get_field('top.old')
 
     def get_top(self):
-        top = self._get_field('top')
-        try:
-            ref = git.rev_parse(self.__top_ref)
-        except:
-            ref = None
-        assert not ref or top == ref
-        return top
+        return git.rev_parse(self.__top_ref)
 
     def set_top(self, value, backup = False):
         if backup:
-            curr = self._get_field('top')
+            curr = self.get_top()
             self._set_field('top.old', curr)
-        self._set_field('top', value)
         self.__update_top_ref(value)
 
     def restore_old_boundaries(self):
         top = self._get_field('top.old')
 
         if top:
-            self._set_field('top', top)
             self.__update_top_ref(top)
             return True
         else:
@@ -436,7 +422,13 @@ class Series(PatchSet):
                 patch = patch.strip()
                 os.rename(os.path.join(branch_dir, patch),
                           os.path.join(patch_dir, patch))
-                Patch(patch, patch_dir, refs_base).update_top_ref()
+                topfield = os.path.join(patch_dir, patch, 'top')
+                if os.path.isfile(topfield):
+                    top = read_string(topfield, False)
+                else:
+                    top = None
+                if top:
+                    git.set_ref(refs_base + '/' + patch, top)
             set_format_version(1)
 
         # Update 1 -> 2.

Re: [StGit PATCH 03/13] Moved that status function to the status command file

[David Kågedal]

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

> diff --git a/t/t0002-status.sh b/t/t0002-status.sh
> index 790b9fb..d0c31b2 100755
> --- a/t/t0002-status.sh
> +++ b/t/t0002-status.sh
> @@ -60,6 +60,14 @@ test_expect_success 'Status with an added file' '
>  '
>  
>  cat > expected.txt <<EOF
> +foo/bar
> +EOF
> +test_expect_success 'Status with an added file and -n option' '
> +    stg status -n > output.txt &&
> +    diff -u expected.txt output.txt
> +'
> +
> +cat > expected.txt <<EOF
>  EOF
>  test_expect_success 'Status after refresh' '
>      stg new -m "first patch" &&
>

Oops, that should have been in the first patch.

-- 
David Kågedal

Re: [StGit PATCH 13/13] Remove the 'top' field

[Karl Hasselström]

On 2007-09-15 00:32:15 +0200, David Kågedal wrote:

> @@ -436,7 +422,13 @@ class Series(PatchSet):
>                  patch = patch.strip()
>                  os.rename(os.path.join(branch_dir, patch),
>                            os.path.join(patch_dir, patch))
> -                Patch(patch, patch_dir, refs_base).update_top_ref()
> +                topfield = os.path.join(patch_dir, patch, 'top')
> +                if os.path.isfile(topfield):
> +                    top = read_string(topfield, False)
> +                else:
> +                    top = None
> +                if top:
> +                    git.set_ref(refs_base + '/' + patch, top)
>              set_format_version(1)
>  
>          # Update 1 -> 2.

And remove the top file, maybe? (Or I may be mistaken; I don't have a
copy of the surrounding code handy.)

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

Re: [StGit PATCH 00/13] Eliminate 'top' and 'bottom' files

[Karl Hasselström]

On 2007-09-15 00:31:09 +0200, David Kågedal wrote:

> The following series removes the 'bottom' and 'top' files for each
> patch, and instead uses the commit objects to keep track of the
> patches.

Wonderful! Does this ensure that there's a bijection between patches
and commits at _all_ times, or am I missing something?

> The last two patches do the final cleansing. Obviously, this changes
> the format, and the format version should be increased and and
> update function be written. So it's not really ready to go in yet.

It's a trivial format update, though: just delete those two files and
increase the number from 2 to 3.

Hmm, wait, no. Right. We also have to create commits for those patches
that don't have exactly one commit object. Not that there'll be many
of them, but better not make assumptions ...

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

Re: [StGit PATCH 00/13] Eliminate 'top' and 'bottom' files

[Catalin Marinas]

On 16/09/2007, Karl Hasselström <kha@treskal.com> wrote:
> On 2007-09-15 00:31:09 +0200, David Kågedal wrote:
>
> > The following series removes the 'bottom' and 'top' files for each
> > patch, and instead uses the commit objects to keep track of the
> > patches.
>
> Wonderful! Does this ensure that there's a bijection between patches
> and commits at _all_ times, or am I missing something?

We should get rid of top.old and bottom.old as well.

My question - does this conflict with the DAG patches in any way? I
intend to include the them at some point, once I get a chance to test
the performance penalty with a big tree like the Linux kernel.

> Hmm, wait, no. Right. We also have to create commits for those patches
> that don't have exactly one commit object. Not that there'll be many
> of them, but better not make assumptions ...

Is there any patch which consists of more than one commit? Maybe only
uncommit could generate one but I think we put some tests in place.

-- 
Catalin

Re: [StGit PATCH 13/13] Remove the 'top' field

[David Kågedal]

16 sep 2007 kl. 01.36 skrev Karl Hasselström:

> On 2007-09-15 00:32:15 +0200, David Kågedal wrote:
>
>> @@ -436,7 +422,13 @@ class Series(PatchSet):
>>                  patch = patch.strip()
>>                  os.rename(os.path.join(branch_dir, patch),
>>                            os.path.join(patch_dir, patch))
>> -                Patch(patch, patch_dir, refs_base).update_top_ref()
>> +                topfield = os.path.join(patch_dir, patch, 'top')
>> +                if os.path.isfile(topfield):
>> +                    top = read_string(topfield, False)
>> +                else:
>> +                    top = None
>> +                if top:
>> +                    git.set_ref(refs_base + '/' + patch, top)
>>              set_format_version(1)
>>
>>          # Update 1 -> 2.
>
> And remove the top file, maybe? (Or I may be mistaken; I don't have a
> copy of the surrounding code handy.)

No, this is the code that updates from version 0 to version 1. The  
problem was that the update functionality used the update_top_ref()  
function in the Patch class which I changed. So I had to inline the  
code instead.
-- 
David Kågedal
davidk@lysator.liu.se

Re: [StGit PATCH 00/13] Eliminate 'top' and 'bottom' files

[David Kågedal]

16 sep 2007 kl. 01.42 skrev Karl Hasselström:

> On 2007-09-15 00:31:09 +0200, David Kågedal wrote:
>
>> The following series removes the 'bottom' and 'top' files for each
>> patch, and instead uses the commit objects to keep track of the
>> patches.
>
> Wonderful! Does this ensure that there's a bijection between patches
> and commits at _all_ times, or am I missing something?

That's the intention, at least. As far as I could tell, the biject  
already held, except temporarily in the middle of executing some  
commands.  So I had to refactor them to never create that intermedate  
state.

>> The last two patches do the final cleansing. Obviously, this changes
>> the format, and the format version should be increased and and
>> update function be written. So it's not really ready to go in yet.
>
> It's a trivial format update, though: just delete those two files and
> increase the number from 2 to 3.
>
> Hmm, wait, no. Right. We also have to create commits for those patches
> that don't have exactly one commit object. Not that there'll be many
> of them, but better not make assumptions ...

I haven't seen any such patches, but I haven't tried everything.  
Neither do the test suite test everything.

-- 
David Kågedal
davidk@lysator.liu.se

Re: [StGit PATCH 00/13] Eliminate 'top' and 'bottom' files

[David Kågedal]

16 sep 2007 kl. 09.28 skrev Catalin Marinas:

> On 16/09/2007, Karl Hasselström <kha@treskal.com> wrote:
>> On 2007-09-15 00:31:09 +0200, David Kågedal wrote:
>>
>>> The following series removes the 'bottom' and 'top' files for each
>>> patch, and instead uses the commit objects to keep track of the
>>> patches.
>>
>> Wonderful! Does this ensure that there's a bijection between patches
>> and commits at _all_ times, or am I missing something?
>
> We should get rid of top.old and bottom.old as well.
>
> My question - does this conflict with the DAG patches in any way? I
> intend to include the them at some point, once I get a chance to test
> the performance penalty with a big tree like the Linux kernel.

My refactoring of the push_patch function will conflict because of  
refactoring, but it doesn't change how the appliedness is used, so it  
should be pretty simple to resolve.

Or I could try to redo the patches so it only has the minimal changes  
to actually remove the top and bottom files.

>> Hmm, wait, no. Right. We also have to create commits for those  
>> patches
>> that don't have exactly one commit object. Not that there'll be many
>> of them, but better not make assumptions ...
>
> Is there any patch which consists of more than one commit? Maybe only
> uncommit could generate one but I think we put some tests in place.

I haven't seen any such case. Can uncommit create one? Or did it use  
to do that before? I added checks to detect it, and no test case  
caught it at least.

-- 
David Kågedal
davidk@lysator.liu.se

Re: [StGit PATCH 13/13] Remove the 'top' field

[Karl Hasselström]

On 2007-09-16 12:22:28 +0200, David Kågedal wrote:

> 16 sep 2007 kl. 01.36 skrev Karl Hasselström:
>
> > And remove the top file, maybe? (Or I may be mistaken; I don't
> > have a copy of the surrounding code handy.)
>
> No, this is the code that updates from version 0 to version 1. The
> problem was that the update functionality used the update_top_ref()
> function in the Patch class which I changed. So I had to inline the
> code instead.

Ah. Right, you did say that you hadn't built an update function yet.

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

Re: [StGit PATCH 00/13] Eliminate 'top' and 'bottom' files

[Karl Hasselström]

On 2007-09-16 08:28:53 +0100, Catalin Marinas wrote:

> We should get rid of top.old and bottom.old as well.

Yeah, I guess that data could be computed from the patch log?

> My question - does this conflict with the DAG patches in any way? I
> intend to include the them at some point, once I get a chance to
> test the performance penalty with a big tree like the Linux kernel.

I haven't been able to get rid of all the expensive DAG walking, so
I've been considering a different approach: continue using the current
applied and unapplied files if the existing HEAD == top patch check
passes, and letting the assimilate command do a full DAG walk to
regenerate those files. (And by "full DAG walk", I mean walking from
HEAD down to the first commit with parents != 1; the patches we see
are applied (in the order we see them), and the rest are unapplied.)

> Is there any patch which consists of more than one commit? Maybe
> only uncommit could generate one but I think we put some tests in
> place.

Uncommit does not generate such patches, unless I've made a thinko; I
don't approve of them. But I always assumed they could exist, since
the code (at least in places) seems careful to not assume anything
about the number of commits between top and bottom.

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

Re: [StGit PATCH 09/13] Clear up the semantics of Series.new_patch

[Catalin Marinas]

On 14/09/2007, David Kågedal <davidk@lysator.liu.se> wrote:
> This patch adds a number of assertions to document and verify the
> complex restrictions of the input parameters to the Series.new_patch
> function. It also adds the requirement that 'before_existing' and
> 'commit' cannot be true at the same time when calling it, instead of
> updating 'commit' inside the function.
[...]
> --- a/stgit/stack.py
> +++ b/stgit/stack.py
> @@ -833,9 +833,16 @@ class Series(PatchSet):
>                    author_name = None, author_email = None, author_date = None,
>                    committer_name = None, committer_email = None,
>                    before_existing = False):
> -        """Creates a new patch
> +        """Creates a new patch, either pointing to an existing commit object,
> +        or by creating a new commit object.
>          """
>
> +        assert commit or (top and bottom)
> +        assert not before_existing or (top and bottom)
> +        assert not (commit and before_existing)
> +        assert (top and bottom) or (not top and not bottom)
> +        assert not top or (bottom == git.get_commit(top).get_parent())

The last assertion here prevents the use of 'stg pick --reverse'. This
command creates an unapplied patch with top and bottom reversed and
pushes it to force a three-way merge.

It seems to work OK if I comment it out but I wonder whether it will
break in the future with the planned removal of the top and bottom
files.

Thanks.

-- 
Catalin

Re: [StGit PATCH 09/13] Clear up the semantics of Series.new_patch

[Karl Hasselström]

On 2007-10-08 14:16:10 +0100, Catalin Marinas wrote:

> On 14/09/2007, David Kågedal <davidk@lysator.liu.se> wrote:
>
> > +        assert commit or (top and bottom)
> > +        assert not before_existing or (top and bottom)
> > +        assert not (commit and before_existing)
> > +        assert (top and bottom) or (not top and not bottom)
> > +        assert not top or (bottom == git.get_commit(top).get_parent())
>
> The last assertion here prevents the use of 'stg pick --reverse'.
> This command creates an unapplied patch with top and bottom reversed
> and pushes it to force a three-way merge.
>
> It seems to work OK if I comment it out but I wonder whether it will
> break in the future with the planned removal of the top and bottom
> files.

I think the assert represents a real constraint, namely that there has
to be a 1:1 correspondance between patches and commits.

Couldn't "stg pick --reverse" create a new commit and use that? That
is, given that we want to revert commit C, create a new commit C* with

  tree(C*)   := tree(parent(C))
  parent(C*) := C

Creating just one new commit object seems like a cheap thing to do.

And shouldn't there be a test for this? :-)

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

Re: [StGit PATCH 09/13] Clear up the semantics of Series.new_patch

[Catalin Marinas]

On 08/10/2007, Karl Hasselström <kha@treskal.com> wrote:
> On 2007-10-08 14:16:10 +0100, Catalin Marinas wrote:
>
> > On 14/09/2007, David Kågedal <davidk@lysator.liu.se> wrote:
> >
> > > +        assert commit or (top and bottom)
> > > +        assert not before_existing or (top and bottom)
> > > +        assert not (commit and before_existing)
> > > +        assert (top and bottom) or (not top and not bottom)
> > > +        assert not top or (bottom == git.get_commit(top).get_parent())
> >
> > The last assertion here prevents the use of 'stg pick --reverse'.
> > This command creates an unapplied patch with top and bottom reversed
> > and pushes it to force a three-way merge.
> >
> > It seems to work OK if I comment it out but I wonder whether it will
> > break in the future with the planned removal of the top and bottom
> > files.
>
> I think the assert represents a real constraint, namely that there has
> to be a 1:1 correspondance between patches and commits.
>
> Couldn't "stg pick --reverse" create a new commit and use that? That
> is, given that we want to revert commit C, create a new commit C* with

Series.new_patch already creates a commit, why should we move the
functionality to 'pick'? The only call to new_patch with commit=False
seems to be from 'uncommit' (and it makes sense indeed).

> And shouldn't there be a test for this? :-)

Yes :-). I think there are many other tests needed. It would be useful
to do a code coverage with the existing tests to see what we are
missing. Unit testing might be useful as well but we all have limited
spare time.

-- 
Catalin

Re: [StGit PATCH 09/13] Clear up the semantics of Series.new_patch

[David Kågedal]

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

> On 08/10/2007, Karl Hasselström <kha@treskal.com> wrote:
>> On 2007-10-08 14:16:10 +0100, Catalin Marinas wrote:
>>
>> > On 14/09/2007, David Kågedal <davidk@lysator.liu.se> wrote:
>> >
>> > > +        assert commit or (top and bottom)
>> > > +        assert not before_existing or (top and bottom)
>> > > +        assert not (commit and before_existing)
>> > > +        assert (top and bottom) or (not top and not bottom)
>> > > +        assert not top or (bottom == git.get_commit(top).get_parent())
>> >
>> > The last assertion here prevents the use of 'stg pick --reverse'.
>> > This command creates an unapplied patch with top and bottom reversed
>> > and pushes it to force a three-way merge.
>> >
>> > It seems to work OK if I comment it out but I wonder whether it will
>> > break in the future with the planned removal of the top and bottom
>> > files.
>>
>> I think the assert represents a real constraint, namely that there has
>> to be a 1:1 correspondance between patches and commits.

Yes, that was the point of the series.

>> Couldn't "stg pick --reverse" create a new commit and use that? That
>> is, given that we want to revert commit C, create a new commit C* with
>
> Series.new_patch already creates a commit, why should we move the
> functionality to 'pick'? The only call to new_patch with commit=False
> seems to be from 'uncommit' (and it makes sense indeed).

It might be true that the assertion could be amended so that if
commit=True, then it is allowed to have top/bottom that doesn't
correspond to a commit and its parent. But it's hard to be sure
without looking at the code much harder than I did just now.  Testing
would also help.

>> And shouldn't there be a test for this? :-)
>
> Yes :-). I think there are many other tests needed. It would be useful
> to do a code coverage with the existing tests to see what we are
> missing. Unit testing might be useful as well but we all have limited
> spare time.

-- 
David Kågedal

Re: [StGit PATCH 09/13] Clear up the semantics of Series.new_patch

[Karl Hasselström]

On 2007-10-09 22:01:44 +0100, Catalin Marinas wrote:

> On 08/10/2007, Karl Hasselström <kha@treskal.com> wrote:
>
> > On 2007-10-08 14:16:10 +0100, Catalin Marinas wrote:
> >
> > > It seems to work OK if I comment it out but I wonder whether it
> > > will break in the future with the planned removal of the top and
> > > bottom files.
> >
> > I think the assert represents a real constraint, namely that there
> > has to be a 1:1 correspondance between patches and commits.
> >
> > Couldn't "stg pick --reverse" create a new commit and use that?
> > That is, given that we want to revert commit C, create a new
> > commit C* with
>
> Series.new_patch already creates a commit, why should we move the
> functionality to 'pick'?

I didn't say that. :-) You could accomplish the commit creation by
calling Series.new_patch if you like.

> The only call to new_patch with commit=False seems to be from
> 'uncommit' (and it makes sense indeed).

Yes. For uncommit anything else would be insane.

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

Re: [StGit PATCH 09/13] Clear up the semantics of Series.new_patch

[Karl Hasselström]

On 2007-10-10 09:45:17 +0200, Karl Hasselström wrote:

> On 2007-10-09 22:01:44 +0100, Catalin Marinas wrote:
>
> > Series.new_patch already creates a commit, why should we move the
> > functionality to 'pick'?
>
> I didn't say that. :-) You could accomplish the commit creation by
> calling Series.new_patch if you like.

Ummm ... which was presumably the case already, and broke because of
the new assertion. Just ignore me. :-)

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

Re: [StGit PATCH 09/13] Clear up the semantics of Series.new_patch

[Catalin Marinas]

On 10/10/2007, David Kågedal <davidk@lysator.liu.se> wrote:
> "Catalin Marinas" <catalin.marinas@gmail.com> writes:
> > On 08/10/2007, Karl Hasselström <kha@treskal.com> wrote:
> >> Couldn't "stg pick --reverse" create a new commit and use that? That
> >> is, given that we want to revert commit C, create a new commit C* with
> >
> > Series.new_patch already creates a commit, why should we move the
> > functionality to 'pick'? The only call to new_patch with commit=False
> > seems to be from 'uncommit' (and it makes sense indeed).
>
> It might be true that the assertion could be amended so that if
> commit=True, then it is allowed to have top/bottom that doesn't
> correspond to a commit and its parent.

I'll fix this and add a test for pick --reverse.

-- 
Catalin