Skip to content
This repository

Index - remove empty directories #158

Closed
wants to merge 3 commits into from

2 participants

Jonathan Johnson Brandon Keepers
Jonathan Johnson

I ran into a problem while using grit where I couldn't get it to create a commit that deletes a directory. It seemed to me that using index.read_tree(repo.commits.first.id) as a starting point I should be able to use index.delete(file) to remove all files from a directory and that the directory would also be removed. Instead what was happening was that the commit would include an empty directory (tree) which isn't even valid. Even using index.delete(directory_name) specifically wouldn't remove the directory (due to the add method pruning trailing '/' and then the write_tree method later expecting a trailing '/' for directory removals).

I changed the Index class so that the write_tree method recursively merges the pre-existing tree (from read_tree) with whatever changes have been made using the add and delete methods in such a way that empty directories are pruned from the resulting tree hierarchy used for the commit. I also added a test to test_rubygit_index to test the functionality.

added some commits February 23, 2013
Jonathan Johnson index - don't allow empty directories in commits
If all files are removed from a particular directory
using the index.delete method, the directory will also
be removed. This better matches the command line
interface to git and what I initially expected from
grit.

This is accomplished by recursively merging the new
index items (tree of hash objects created via the
add and delete methods) with the previous tree that
the commit builds from (read in via index.read_tree)
44023ba
Jonathan Johnson add test for deleting directories from index
deleting all files in a directory should delete the
directory itself
71405d6
Jonathan Johnson add test for deleting a whole directory at once
Calling delete('directory_name') should delete the
directory 'directory_name' and all its contents.
(without the need to delete each member of the
directory's tree individually)
00a2415
Jonathan Johnson

I just noticed issue #54 which is a similar problem. This patch fixes that issue by allowing directories to be deleted using Index#delete('directory_name') but also goes further by ensuring that directories that have had all files removed from them also get removed automatically, enforcing the rule that you can't have empty directories in a git repository. Without either this fix or the patch in #54, it seems it is impossible to create a commit that deletes a directory from an existing tree read in via index#read_tree.

Brandon Keepers
Collaborator

Grit is no longer maintained. See #183 and check out libgit2/rugged.

Brandon Keepers bkeepers closed this February 03, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Showing 3 unique commits by 1 author.

Feb 23, 2013
Jonathan Johnson index - don't allow empty directories in commits
If all files are removed from a particular directory
using the index.delete method, the directory will also
be removed. This better matches the command line
interface to git and what I initially expected from
grit.

This is accomplished by recursively merging the new
index items (tree of hash objects created via the
add and delete methods) with the previous tree that
the commit builds from (read in via index.read_tree)
44023ba
Jonathan Johnson add test for deleting directories from index
deleting all files in a directory should delete the
directory itself
71405d6
Feb 25, 2013
Jonathan Johnson add test for deleting a whole directory at once
Calling delete('directory_name') should delete the
directory 'directory_name' and all its contents.
(without the need to delete each member of the
directory's tree individually)
00a2415
This page is out of date. Refresh to see the latest.
1  lib/grit.rb
@@ -8,6 +8,7 @@
8 8
 require 'timeout'
9 9
 require 'logger'
10 10
 require 'digest/sha1'
  11
+require 'set'
11 12
 
12 13
 # third party
13 14
 
67  lib/grit/index.rb
@@ -164,19 +164,16 @@ def commit(message, parents = nil, actor = nil, last_tree = nil, head = 'master'
164 164
     # Returns the String SHA1 String of the tree.
165 165
     def write_tree(tree = nil, now_tree = nil)
166 166
       tree = self.tree if !tree
167  
-      tree_contents = {}
168  
-
169  
-      # fill in original tree
170  
-      now_tree = read_tree(now_tree) if(now_tree && now_tree.is_a?(String))
171  
-      now_tree.contents.each do |obj|
172  
-        sha = [obj.id].pack("H*")
173  
-        k = obj.name
174  
-        k += '/' if (obj.class == Grit::Tree)
175  
-        tmode = obj.mode.to_i.to_s  ## remove zero-padding
176  
-        tree_contents[k] = "%s %s\0%s" % [tmode, obj.name, sha]
177  
-      end if now_tree
178  
-
179  
-      # overwrite with new tree contents
  167
+
  168
+      # keep blobs and dirs separate to enforce order
  169
+      tree_contents_blobs = {}
  170
+      tree_contents_dirs = {}
  171
+
  172
+      # merge original and new trees recursively
  173
+      now_tree = read_tree(now_tree) if (now_tree && now_tree.is_a?(String))
  174
+      tree = (merge_tree_and_hash(now_tree, tree) || {}) if now_tree
  175
+
  176
+      # prepare raw object output
180 177
       tree.each do |k, v|
181 178
         case v
182 179
           when Array
@@ -186,25 +183,26 @@ def write_tree(tree = nil, now_tree = nil)
186 183
               mode = mode.to_i.to_s  # leading 0s not allowed
187 184
               k = k.split('/').last  # slashes not allowed
188 185
               str = "%s %s\0%s" % [mode, k, sha]
189  
-              tree_contents[k] = str
  186
+              if mode == '40000'
  187
+                tree_contents_dirs[k] = str
  188
+              else
  189
+                tree_contents_blobs[k] = str
  190
+              end
190 191
             end
191 192
           when String
192 193
             sha = write_blob(v)
193 194
             sha = [sha].pack("H*")
194 195
             str = "%s %s\0%s" % ['100644', k, sha]
195  
-            tree_contents[k] = str
  196
+            tree_contents_blobs[k] = str
196 197
           when Hash
197  
-            ctree = now_tree/k if now_tree
198  
-            sha = write_tree(v, ctree)
  198
+            sha = write_tree(v)
199 199
             sha = [sha].pack("H*")
200 200
             str = "%s %s\0%s" % ['40000', k, sha]
201  
-            tree_contents[k + '/'] = str
202  
-          when false
203  
-            tree_contents.delete(k)
  201
+            tree_contents_dirs[k] = str
204 202
         end
205 203
       end
206 204
 
207  
-      tr = tree_contents.sort.map { |k, v| v }.join('')
  205
+      tr = [tree_contents_blobs, tree_contents_dirs].map {|tree_contents| tree_contents.sort.map { |k, v| v }.join('')}.inject(:+)
208 206
       @last_tree_size = tr.size
209 207
       self.repo.git.put_raw_object(tr, 'tree')
210 208
     end
@@ -217,6 +215,33 @@ def write_tree(tree = nil, now_tree = nil)
217 215
     def write_blob(data)
218 216
       self.repo.git.put_raw_object(data, 'blob')
219 217
     end
  218
+
  219
+    # Merge now_tree and tree for write_tree
  220
+    #
  221
+    # tree - a Grit::Tree representing previous state
  222
+    # hash - a hash representing the new state to merge in
  223
+    #        (created by the add and delete methods)
  224
+    def merge_tree_and_hash(tree, hash)
  225
+      result = {}
  226
+      merge_keys = Set.new
  227
+      tree.contents.each do |object|
  228
+        k = object.name
  229
+        if hash.has_key?(k)
  230
+          v = hash[k]
  231
+          if v.kind_of?(Hash) && object.kind_of?(Grit::Tree)
  232
+            result[k] = merge_tree_and_hash(object, v)
  233
+            merge_keys.add k
  234
+          end
  235
+        else
  236
+          result[object.name] = [object.id, object.mode]
  237
+        end
  238
+      end
  239
+      result.merge!(hash.select {|k, v| !merge_keys.include?(k)})
  240
+      result = false if result.select {|k,v| v}.empty?
  241
+      result
  242
+    end
  243
+    private :merge_tree_and_hash
  244
+
220 245
   end # Index
221 246
 
222 247
 end # Grit
26  test/test_rubygit_index.rb
@@ -121,4 +121,28 @@ def test_modify_file
121 121
     b = @git.commits.first.tree/'README.txt'
122 122
     assert_equal 'e45d6b418e34951ddaa3e78e4fc4d3d92a46d3d1', b.id
123 123
   end
124  
-end
  124
+
  125
+  # deleting the contents of a directory should delete the directory itself
  126
+  def test_delete_dirs
  127
+    i = @git.index
  128
+    i.read_tree(@git.commits.first.tree.id)
  129
+    %w(pack.rb loose.rb raw_object.rb mmap.rb).each {|f| i.delete(File.join('lib/grit/git-ruby/internal', f))}
  130
+    i.commit('message', [@git.commits.first], @user, nil, 'master')
  131
+    assert !(@git.commits.first.tree / 'lib/grit/git-ruby').contents.map {|c| c.name}.include?('internal')
  132
+        
  133
+    i = @git.index
  134
+    i.read_tree(@git.commits.first.tree.id)
  135
+    %w(object.rb repository.rb).each {|f| i.delete(File.join('lib/grit/git-ruby', f))}
  136
+    i.commit('message', [@git.commits.first], @user, nil, 'master')
  137
+    assert !(@git.commits.first.tree / 'lib/grit').contents.map {|c| c.name}.include?('git-ruby')
  138
+  end
  139
+
  140
+  def test_delete_whole_dir
  141
+    i = @git.index
  142
+    i.read_tree(@git.commits.first.tree.id)
  143
+    i.delete('lib/grit/git-ruby/internal')
  144
+    i.commit('message', [@git.commits.first], @user, nil, 'master')
  145
+    assert !(@git.commits.first.tree / 'lib/grit/git-ruby').contents.map {|c| c.name}.include?('internal')
  146
+  end
  147
+
  148
+end
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.