[Diff] Changed repo protection to worktree mechanism

- As suggested by @matthijskooijman on #443
- This is cleaner than trying to use stash push/pop
- The old method is available

CHANGELOG.md

@@ -32,6 +32,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   - `field_power` Name/s of the field/s used for the power raiting
   - `invalidate_pcb_text_cache` controls if we reset the text variables cached
     in the PCB file.
+  - `git_diff_strategy` selects how we preserve the current repo state.
+    (See #443)
 - Filters:
   - New `value_split` to extract information from the Value field and put it in
     separated fields. I.e. tolerance, voltage, etc.
@@ -53,11 +55,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Command line:
   - `--list` also lists groups
 - KiCad v6/7 schematic:
-  - The hierarchy is expanded only if needed, i.e. value of an instance changed
+  - When saving an schematic the hierarchy is expanded only if needed,
+    i.e. value of an instance changed
 - List actions:
   - Now you must explicitly ask to configure outputs. Otherwise isn't needed.
     As a result you no longer need to have an SCH/PCB. Use `--config-outs` to
     get the old behavior.
+- Git diff link file name:
+  - Now we default to using worktrees instead of stash push/pop. As a side
+    effect the names of the git points are chnaged. This is because main/master
+    only applies to the main worktree. So the names now refer to the closest
+    tag.
 
 ### Fixed
 - KiCad v6/7 schematic:

README.md

@@ -840,6 +840,10 @@ global:
         - `number`: [number=0] Error number we want to exclude.
         - `regex`: [string=''] Regular expression to match the text for the error we want to exclude.
         - *regexp*: Alias for regex.
+    - `git_diff_strategy`: [string='worktree'] [worktree,stash] When computing a PCB/SCH diff it configures how do we preserve the current
+                           working state. The *worktree* mechanism creates a separated worktree, that then is just removed.
+                           The *stash* mechanism uses *git stash push/pop* to save the current changes. Using *worktree*
+                           is the preferred mechanism.
     - `hide_excluded`: [boolean=false] Default value for the `hide_excluded` option of various PCB outputs.
     - `impedance_controlled`: [boolean=false] The PCB needs specific dielectric characteristics.
                               KiCad 6: you should set this in the Board Setup -> Physical Stackup.

kibot/globals.py

@@ -343,6 +343,11 @@ class Globals(FiltersOptions):
                 variables update when using `set_text_variables`. You might want to disable it when applying some
                 changes to the PCB and create a new copy to send to somebody without changing the cached values.
                 The `auto` value will remove the cached values only when using `set_text_variables` """
+            self.git_diff_strategy = 'worktree'
+            """ [worktree,stash] When computing a PCB/SCH diff it configures how do we preserve the current
+                working state. The *worktree* mechanism creates a separated worktree, that then is just removed.
+                The *stash* mechanism uses *git stash push/pop* to save the current changes. Using *worktree*
+                is the preferred mechanism """
         self.set_doc('filters', " [list(dict)] KiBot warnings to be ignored ")
         self._filter_what = 'KiBot warnings'
         self.filters = FilterOptionsKiBot

kibot/gs.py

@@ -151,6 +151,7 @@ class GS(object):
     global_field_tolerance = None
     global_field_voltage = None
     global_hide_excluded = None
+    global_git_diff_strategy = None
     global_impedance_controlled = None
     global_invalidate_pcb_text_cache = None
     global_kiauto_time_out_scale = None

kibot/out_diff.py

@@ -154,51 +154,51 @@ class DiffOptions(BaseOptions):
         self.name_used_for_cache = name
         run_command(cmd)
 
-    def cache_pcb(self, name):
+    def cache_pcb(self, name, force_exist):
         if name:
-            if not os.path.isfile(name):
+            if not os.path.isfile(name) and not force_exist:
                 raise KiPlotConfigurationError('Missing file to compare: `{}`'.format(name))
         else:
             GS.check_pcb()
             name = GS.pcb_file
-            if not os.path.isfile(name):
-                if self.always_fail_if_missing:
-                    raise KiPlotConfigurationError('Missing file to compare: `{}`'.format(name))
-                with NamedTemporaryFile(mode='w', suffix='.kicad_pcb', delete=False) as f:
-                    f.write("(kicad_pcb (version 20171130) (host pcbnew 5.1.5))\n")
-                    name = f.name
-                    self._to_remove.append(name)
+        if not os.path.isfile(name):
+            if self.always_fail_if_missing:
+                raise KiPlotConfigurationError('Missing file to compare: `{}`'.format(name))
+            with NamedTemporaryFile(mode='w', suffix='.kicad_pcb', delete=False) as f:
+                f.write("(kicad_pcb (version 20171130) (host pcbnew 5.1.5))\n")
+                name = f.name
+                self._to_remove.append(name)
         hash = self.get_digest(name)
         self.add_to_cache(name, hash)
         return hash
 
-    def cache_sch(self, name):
+    def cache_sch(self, name, force_exist):
         if name:
-            if not os.path.isfile(name):
+            if not os.path.isfile(name) and not force_exist:
                 raise KiPlotConfigurationError('Missing file to compare: `{}`'.format(name))
         else:
             GS.check_sch()
             name = GS.sch_file
+        if not os.path.isfile(name):
+            if self.always_fail_if_missing:
+                raise KiPlotConfigurationError('Missing file to compare: `{}`'.format(name))
             ext = os.path.splitext(name)[1]
-            if not os.path.isfile(name):
-                if self.always_fail_if_missing:
-                    raise KiPlotConfigurationError('Missing file to compare: `{}`'.format(name))
-                with NamedTemporaryFile(mode='w', suffix=ext, delete=False) as f:
-                    logger.debug('Creating empty schematic: '+f.name)
-                    if ext == '.kicad_sch':
-                        f.write("(kicad_sch (version 20211123) (generator eeschema))\n")
-                    else:
-                        f.write("EESchema Schematic File Version 4\nEELAYER 30 0\nEELAYER END\n$Descr A4 11693 8268\n"
-                                "$EndDescr\n$EndSCHEMATC\n")
-                    name = f.name
-                    self._to_remove.append(name)
-                if ext != '.kicad_sch':
-                    lib_name = os.path.splitext(name)[0]+'-cache.lib'
-                    if not os.path.isfile(lib_name):
-                        logger.debug('Creating dummy cache lib: '+lib_name)
-                        with open(lib_name, 'w') as f:
-                            f.write("EESchema-LIBRARY Version 2.4\n#\n#End Library\n")
-                        self._to_remove.append(lib_name)
+            with NamedTemporaryFile(mode='w', suffix=ext, delete=False) as f:
+                logger.debug('Creating empty schematic: '+f.name)
+                if ext == '.kicad_sch':
+                    f.write("(kicad_sch (version 20211123) (generator eeschema))\n")
+                else:
+                    f.write("EESchema Schematic File Version 4\nEELAYER 30 0\nEELAYER END\n$Descr A4 11693 8268\n"
+                            "$EndDescr\n$EndSCHEMATC\n")
+                name = f.name
+                self._to_remove.append(name)
+            if ext != '.kicad_sch':
+                lib_name = os.path.splitext(name)[0]+'-cache.lib'
+                if not os.path.isfile(lib_name):
+                    logger.debug('Creating dummy cache lib: '+lib_name)
+                    with open(lib_name, 'w') as f:
+                        f.write("EESchema-LIBRARY Version 2.4\n#\n#End Library\n")
+                    self._to_remove.append(lib_name)
         # Schematics can have sub-sheets
         sch = load_any_sch(name, os.path.splitext(os.path.basename(name))[0])
         files = sch.get_files()
@@ -210,9 +210,9 @@ class DiffOptions(BaseOptions):
         self.add_to_cache(name, hash)
         return hash
 
-    def cache_file(self, name=None):
+    def cache_file(self, name=None, force_exist=False):
         self.git_hash = 'Current' if not name else 'FILE'
-        return self.cache_pcb(name) if self.pcb else self.cache_sch(name)
+        return self.cache_pcb(name, force_exist) if self.pcb else self.cache_sch(name, force_exist)
 
     def run_git(self, cmd, cwd=None, just_raise=False):
         if cwd is None:
@@ -240,7 +240,7 @@ class DiffOptions(BaseOptions):
         logger.debug('Git submodules '+str(subs))
         return subs
 
-    def undo_git(self):
+    def undo_git_use_stash(self):
         if self.checkedout:
             logger.debug('Restoring point '+self.branch)
             self.run_git(['checkout', '--force', '--recurse-submodules', self.branch])
@@ -303,23 +303,23 @@ class DiffOptions(BaseOptions):
             return self.solve_kibot_magic(name, 'KIBOT_TAG')
         return name
 
-    def get_git_point_desc(self, user_name):
+    def get_git_point_desc(self, user_name, cwd=None):
         # Are we at a tagged point?
         name = None
         try:
-            name = self.run_git(['describe', '--exact-match', '--tags', 'HEAD'], just_raise=True)
+            name = self.run_git(['describe', '--exact-match', '--tags', 'HEAD'], cwd=cwd, just_raise=True)
             if user_name == 'Dirty':
                 name += '-dirty'
         except CalledProcessError:
             logger.debug("Not at a tag point")
         if name is None:
             # Are we at the HEAD of a branch?
-            branch = self.run_git(['rev-parse', '--abbrev-ref', 'HEAD'])
+            branch = self.run_git(['rev-parse', '--abbrev-ref', 'HEAD'], cwd=cwd)
             if branch == 'HEAD':
                 # Detached state
                 # Try to find the name relative to a tag
                 try:
-                    name = self.run_git(['describe', '--tags', '--dirty'], just_raise=True)
+                    name = self.run_git(['describe', '--tags', '--dirty'], cwd=cwd, just_raise=True)
                 except CalledProcessError:
                     logger.debug("Can't find a tag name")
                 if not name:
@@ -331,19 +331,19 @@ class DiffOptions(BaseOptions):
                 # Do we have a tag in this branch
                 extra = None
                 try:
-                    extra = self.run_git(['describe', '--tags', '--abbrev=0', '--dirty'], just_raise=True)
+                    extra = self.run_git(['describe', '--tags', '--abbrev=0', '--dirty'], cwd=cwd, just_raise=True)
                 except CalledProcessError:
                     logger.debug("Can't find a tag name")
                 if extra:
                     name += '['+extra+']'
                 elif user_name == 'Dirty':
                     name += '-dirty'
-        return '{}({})'.format(self.run_git(['rev-parse', '--short', 'HEAD']), name)
+        return '{}({})'.format(self.run_git(['rev-parse', '--short', 'HEAD'], cwd=cwd), name)
 
     def git_dirty(self):
         return self.run_git(['status', '--porcelain', '-uno'])
 
-    def cache_git(self, name):
+    def cache_git_use_stash(self, name):
         self.stashed = False
         self.checkedout = False
         # Which file
@@ -385,9 +385,49 @@ class DiffOptions(BaseOptions):
             # A short version of the current hash
             self.git_hash = self.get_git_point_desc(name_ori)
         finally:
-            self.undo_git()
+            self.undo_git_use_stash()
         return hash
 
+    def cache_git(self, name):
+        self.stashed = False
+        self.checkedout = False
+        # Which file
+        if self.pcb:
+            GS.check_pcb()
+            self.file = GS.pcb_file
+        else:
+            GS.check_sch()
+            self.file = GS.sch_file
+        # Place where we know we have a repo
+        self.repo_dir = os.path.dirname(os.path.abspath(self.file))
+        if name:
+            # Checkout the target
+            name_ori = name
+            name = self.solve_git_name(name)
+            git_tmp_wd = mkdtemp()
+            logger.debug('Checking out '+name+' to '+git_tmp_wd)
+            self.run_git(['worktree', 'add', git_tmp_wd, name])
+            self._worktrees_to_remove.append(git_tmp_wd)
+            self.run_git(['submodule', 'init'], cwd=git_tmp_wd)
+            self.run_git(['submodule', 'update'], cwd=git_tmp_wd)
+            name_copy = self.run_git(['ls-files', '--full-name', self.file])
+            name_copy = os.path.join(git_tmp_wd, name_copy)
+            logger.debug('- Using temporal copy: '+name_copy)
+            hash = self.cache_file(name_copy, force_exist=True)
+            cwd = git_tmp_wd
+        else:
+            name_ori = 'Dirty' if self.git_dirty() else 'HEAD'
+            # Populate the cache
+            hash = self.cache_file()
+            cwd = self.repo_dir
+        # A short version of the current hash
+        self.git_hash = self.get_git_point_desc(name_ori, cwd)
+        return hash
+
+    def remove_git_worktree(self, name):
+        logger.debug('Removing temporal checkout at '+name)
+        self.run_git(['worktree', 'remove', '--force', name])
+
     @staticmethod
     def check_output_type(out, must_be):
         if out.type != must_be:
@@ -422,16 +462,16 @@ class DiffOptions(BaseOptions):
             fname, dir_name = VariantOptions.save_tmp_dir_board('diff')
         else:
             dir_name = mkdtemp()
-            self.dirs_to_remove.append(dir_name)
             fname = GS.sch.save_variant(dir_name)
             GS.copy_project_sch(dir_name)
+        self.dirs_to_remove.append(dir_name)
         res = self.cache_file(os.path.join(dir_name, fname))
         self.git_hash = 'Current'
         return res
 
     def cache_obj(self, name, type):
         if type == 'git':
-            return self.cache_git(name)
+            return self.cache_git(name) if GS.global_git_diff_strategy == 'worktree' else self.cache_git_use_stash(name)
         if type == 'file':
             return self.cache_file(name)
         if type == 'current':
@@ -498,6 +538,7 @@ class DiffOptions(BaseOptions):
     def run(self, name):
         self.command = self.ensure_tool('KiDiff')
         self._to_remove = []
+        self._worktrees_to_remove = []
         if self.old_type == 'git' or self.new_type == 'git':
             self.git_command = self.ensure_tool('Git')
         if not self.pcb:
@@ -543,6 +584,9 @@ class DiffOptions(BaseOptions):
                 os.remove(self.incl_file)
             for f in self._to_remove:
                 os.remove(f)
+            # Remove any git worktree that we created
+            for w in self._worktrees_to_remove:
+                self.remove_git_worktree(w)
 
 
 @output_class

tests/test_plot/test_misc.py

@@ -1400,7 +1400,8 @@ def test_diff_git_2(test_dir):
         msg = f.read()
     assert msg == 'Bye!\n'
     # Check the link
-    assert glob(os.path.join(ctx.output_dir, prj+'-diff_pcb_*(v1)-*(master[[]v1[]]).pdf'))
+    assert (glob(os.path.join(ctx.output_dir, prj+'-diff_pcb_*(v1)-*(master[[]v1[]]).pdf')) +
+            glob(os.path.join(ctx.output_dir, prj+'-diff_pcb_*(v1)-*(v1-2-*).pdf'))), "Can't find link"
     ctx.clean_up(keep_project=True)