version-control-system #45

iamanandkr95 · 2017-07-16T13:55:35Z

No description provided.

kaustubh-karkare

Decent progress. However, you should start working on actual code, since there will be thing you cant anticipate while writing this spec.

kaustubh-karkare · 2017-07-18T18:08:47Z

solutions/anand-kumar/vcs.md

+
+The tree class has the following method(s):
+- `get_hash_value` method returns a SHA-1 hash based on the contents of the tree entry.
+- `get_content` method returns the tree entries.


These lines look duplicated. Mention a base class that these 3 types have to extend.

kaustubh-karkare · 2017-07-18T18:09:49Z

solutions/anand-kumar/vcs.md

+
+#### Binary Large Objects(BLOB)
+
+The blob class stores the following:


You talk about classes, but how does this translate to serialized data in the file?

kaustubh-karkare · 2017-07-18T18:10:57Z

solutions/anand-kumar/vcs.md

+The vcs internals includes the following methods:
+
+- `init` method takes input a directory path or by default uses the current directory path to create the .vcs directory. The init method initializes the HEAD file pointer reference as `None`.
+- `get_files_and_directories` method takes input a commit tree and recursively traverses the tree to return a list of file and directory paths along with their SHA-1 hash as stored in the `.vcs/objects/`.


You might want to make a distinction between public api and internal methods. This feels internal.

kaustubh-karkare

First of all, excellent job with writing this in a language you didn't know!
One concern I have is that your code isn't object oriented enough. You can make it a lot more intuitive if you had a common base class that these 3 types extended from.
I'm not going to insist on an actual CLI, but you do need to provide unit tests.

kaustubh-karkare · 2017-08-16T23:22:04Z

solutions/anand-kumar/src/vcs_internals.rb

+def get_sha_hash(header, content)
+    sha1 = Digest::SHA1.new
+    sha1.update(header.gsub(WINDOWS_LINE_END, UNIX_LINE_END))
+    if not content.include? NULL and File.file? content


This "content" needs a better name like "path_or_content"

kaustubh-karkare · 2017-08-16T23:22:40Z

solutions/anand-kumar/src/vcs_internals.rb

+end
+
+
+def hash_object(header, content, sha_hash)


Need a better name here. "create_object_file" ?

kaustubh-karkare · 2017-08-16T23:23:57Z

solutions/anand-kumar/src/vcs_internals.rb

+                        break
+                    end
+                    output_file.write(
+                        data.gsub(WINDOWS_LINE_END, UNIX_LINE_END)


This gsub thing looks inconvenient. Why do you even have to bother?

kaustubh-karkare · 2017-08-16T23:24:19Z

solutions/anand-kumar/src/vcs_internals.rb

+                end
+            end
+        else
+            output_file.write(content.gsub(WINDOWS_LINE_END, UNIX_LINE_END))


kaustubh-karkare · 2017-08-16T23:27:37Z

solutions/anand-kumar/src/blob.rb

+    open(input_file_path, "r") do |input_file|
+        open(output_file_path, "w") do |output_file|
+            data = ""
+            while not data.include? NULL


I've seen this pattern in a bunch of places. You should probably extract it into a utility method.

kaustubh-karkare · 2017-08-16T23:41:39Z

solutions/anand-kumar/src/vcs.rb

+                original_file
+            )
+        end
+            find_diff(original_file, modified_file)


Weird indenting? What linter are you using?

kaustubh-karkare · 2017-08-16T23:41:55Z

solutions/anand-kumar/src/vcs.rb

+    modified_files = get_modified_files()
+    modified_files.each do |modified_file, modified_file_sha|
+        puts File.basename(modified_file)
+        original_file = File.join(Dir.getwd, ".vcs", "temporary_file")


Ruby doesn't have a standard make to make temporary files?

kaustubh-karkare · 2017-08-16T23:42:45Z

solutions/anand-kumar/src/diff-utility.rb

+                end
+                old_lines.push(data)
+            end
+        end


Make a get_lines method :P

kaustubh-karkare · 2017-08-16T23:43:42Z

solutions/anand-kumar/src/diff-utility.rb

+                lcs[ii][jj] = NodeValue.new(
+                    lcs[ii - 1][jj - 1].key + 1,
+                    ii - 1,
+                    jj - 1


Trailing commas?

kaustubh-karkare · 2017-08-16T23:44:59Z

solutions/anand-kumar/src/diff-utility.rb

+        current_x = parent_x
+        current_y = parent_y
+    end
+    return lcs_old, lcs_new


I dont really understand these names.

kaustubh-karkare

I've not reviewed the whole thing, because the comments until now were signficant enough to require large changes.
You have circular dependencies in your require statements (vcs_internals -> file_storage -> vcs_internals). Even though ruby allows it, this isn't a good abstraction. Ensure a DAG.
Sorry about the delay, the cyclic dependencies made things harder to understand, which meant that I had a few false starts before I was able to review this.

kaustubh-karkare · 2017-09-14T00:14:56Z

solutions/anand-kumar/src/vcs_internals.rb

+NULL = "\0"
+
+
+class FilePath


This is basically equivalent to global variables. It isn't taking advantage of OOPs.
I want to be able to run commands on 2 separate repos at the same time.

kaustubh-karkare · 2017-09-14T00:31:54Z

solutions/anand-kumar/src/vcs_internals.rb

+end
+
+
+def extract_header(input_file)


You should clarify whether this is a file name vs file handle. Better naming.

However, I'd have recommended that you have a File class, with this method on it.

kaustubh-karkare · 2017-09-14T00:34:18Z

solutions/anand-kumar/src/vcs_internals.rb

+end
+
+
+def type(sha_hash)


This is pretty unhelpful. get_type would be a better name.
Or ObjectFile.from_hash(hash).type

kaustubh-karkare · 2017-09-14T00:35:11Z

solutions/anand-kumar/src/vcs_internals.rb

+end
+
+
+def create_object_file(header, path_or_content, sha_hash)


Why is the hash an input here? Cant you compute it based on path_or_content?

kaustubh-karkare · 2017-09-14T00:36:48Z

solutions/anand-kumar/src/vcs_internals.rb

+end
+
+
+def get_files_in_directory(directory_path=Dir.getwd)


This is a lie.

kaustubh-karkare · 2017-09-14T00:37:56Z

solutions/anand-kumar/src/vcs_internals.rb

+end
+
+
+def get_files_in_tree(sha_hash, directory_path=Dir.getwd)


kaustubh-karkare · 2017-09-14T00:38:48Z

solutions/anand-kumar/src/vcs_internals.rb

+end
+
+
+def get_modified_files(**kwargs)


Unless you are forwarding arguments, avoid kwargs. Use default argument values instead.

Alternatively:
commit = Commit.get_current()
print(commit.diff(other_commit))

kaustubh-karkare · 2017-09-14T00:41:03Z

solutions/anand-kumar/src/vcs.rb

+EOL = "\n"
+
+
+def init(directory_path=Dir.getwd)


I've seen this (getwd) in a lot of places. It makes me extremely uncomfortable, because it isn't clear whether that method gets that argument or not. Default arguments are good when you're dealing with external users, who are unreliable, but within your own code, things should be much stricter.

kaustubh-karkare · 2017-09-14T00:42:46Z

solutions/anand-kumar/src/file_storage.rb

+        return sha_hash
+    end
+
+    def self.restore_blob(sha_hash, output_file_path)


Ehh ... using static methods defeats the point of objects too.
blob = Blob.create(content); Blob(hash).restore() is how this should look.

kaustubh-karkare · 2017-09-14T00:43:40Z

solutions/anand-kumar/src/diff-utility.rb

+            equivalent_line_in_new_file[old_index] == 0 &&
+            equivalent_line_in_old_file[new_index] == 0
+        )
+            old_index, new_index, line_number_new = display_diff_change(


IMO display_diff_change can handle the display_diff_append/display_diff_delete cases too, so you shouldn't need them.

kaustubh-karkare

IMO you mostly have the overall structure right. The main concern is that the abstraction aren't completely clean. I'm not entirely sure what is the best way to fix this, because I dont want to keep requesting changes based on vague advice, so here's a summary of how my code is organized:

Basic Operations Module

Generate the diff between 2 strings (ie - content from 2 files)
Apply the given diff on a string (ie - content from original file)

Advanced Operations Module (using the above)

Generate the diff between 2 directories
Apply a diff to a directory.

(no mention of VCS in the above)
VCS Objects

Base Object, extended by Commit, Tree, Blob
Each of the subclasses has methods:
- static create(...) = based on working directory
- update() = apply this to working directory
- Class specific methods like commit.get_message()
Each subclass also maintains a reference to the VCS instance.

VCS Class

It is an augmented version of your VCSPaths class.
Includes more API like "get/set_option" (in the config file).

BaseCommandClass

Extended by the various commands that implement logic.
Here's an example snippet of how my checkout command looks:

class CheckoutCommand(BaseCommand):
  """Update the repository to specified commit."""

  @classmethod
  def initialize_subparser(cls, subparser):
    subparser.add_argument('hash')

  def execute(self, args):
    self.vcs.ensure_initialized()
    self.vcs.ensure_clean()
    old_commit = self.vcs.get_commit(head=True)
    new_commit = self.vcs.get_commit(args.hash)
    old_commit.update(new_commit)
    self.vcs.set_option('HEAD', new_commit.hash)
    self.stdout.write("Checked-out commit: %s\n" % new_commit.hash)

kaustubh-karkare · 2017-10-04T00:42:05Z

solutions/anand-kumar/src/diff-utility.rb

+                jj -= 1
+            end
+        end
+        return old_line_equivalent_in_new, new_line_equivalent_in_old


You might want to add comments explaining these names, because it wasn't until I read the rest of the class that I truly understood what was going on.

kaustubh-karkare · 2017-10-04T00:51:38Z

solutions/anand-kumar/src/diff-utility.rb

+                :range_end_old=>range_end_old,
+                :range_start_new=>range_start_new,
+                :range_end_new=>range_end_new,
+            })


Dont you have to add/subtract lines in this case?

kaustubh-karkare · 2017-10-04T00:52:24Z

solutions/anand-kumar/src/diff-utility.rb

+            @hunks_with_context[last_hunk][:range_end_new] = range_end_new
+        else
+            @hunks_with_context[last_hunk][:range_end_old] += lines
+            @hunks_with_context[last_hunk][:range_end_new] += lines


Why not do this in the if-clause too?

kaustubh-karkare · 2017-10-04T00:53:28Z

solutions/anand-kumar/src/diff-utility.rb

+        new_lines = get_lines(@new_file_path)
+        puts "diff a/#{@file_name} b/#{@file_name}"
+        puts "--- a/#{@file_name}"
+        puts "+++ b/#{@file_name}"


Never print stuff from inside utility modules. Instead, return strings, and defer it as much as possible, so it is handled by your CLI logic.

kaustubh-karkare · 2017-10-04T00:54:54Z

solutions/anand-kumar/src/diff-utility.rb

@@ -0,0 +1,202 @@
+class Diff


Tbh, I wasn't able to follow along with all of your logic here, so I'm not convinced that it is correct yet.
Can you write a unit test for this specific file? Show me the edge cases.

kaustubh-karkare · 2017-10-04T01:29:28Z

solutions/anand-kumar/src/file_storage.rb

+                    Blob.new(file_hash).restore(file_path)
+                end
+            else
+                Blob.new(file_hash).restore(file_path)


You're not dealing with Trees here, because your other utility methods have already flattened the results.
But wouldn't recursively calling restore be easier?

kaustubh-karkare · 2017-10-04T01:40:12Z

solutions/anand-kumar/src/file_storage.rb

+        config_parameters = {}
+        open(VcsPath.new().config, "r") do |config_file|
+            config_parameters = JSON.load(config_file)
+        end


Reading config should probably be through API methods on a VCS class, instead of reading the file directly.

kaustubh-karkare · 2017-10-04T01:41:53Z

solutions/anand-kumar/src/file_storage.rb

+    def get_commit_content()
+        commit_parameters = {}
+        object_file_path = File.join(VcsPath.new().objects, @hash)
+        open(object_file_path, "rb") do |object_file|


More of these loops :P

kaustubh-karkare · 2017-10-04T01:42:59Z

solutions/anand-kumar/src/vcs.rb

+    print "Author: #{commit_content.fetch("author")}#{EOL}"
+    print "Author Email: #{commit_content.fetch("email")}#{EOL}"
+    print "Time: #{commit_content.fetch("time")}#{EOL}"
+    print "Message: #{commit_content.fetch("message")}#{EOL}#{EOL}"


All of this isn't command specific. This should be part of the commit functionality.

kaustubh-karkare · 2017-10-04T01:46:11Z

solutions/anand-kumar/src/vcs.rb

+        if not files_in_commit.key? file_path
+            puts "\tuntracked:\t#{File.basename(file_path)}"
+        end
+    end


I'm reasonable sure the problem statement mentioned that we wont be dealing with staging areas, which is what this concept of "untracked" files deals with.

Additionally, you shouldn't be listing out these cases in the implementation of a command.

modified: solutions/anand-kumar/src/diff_utility.rb modified: solutions/anand-kumar/src/file_utilities.rb modified: solutions/anand-kumar/src/vcs.rb deleted: solutions/anand-kumar/src/vcs_commands.rb copied: solutions/anand-kumar/src/vcs.rb -> solutions/anand-kumar/src/vcs_internals.rb modified: solutions/anand-kumar/src/vcs_objects.rb

kaustubh-karkare · 2019-08-17T16:23:02Z

Inactive for too long.

Fwiw, this was good progress.

Create vcs.md

8545230

iamanandkr95 changed the title ~~Create vcs.md~~ version-control-system Jul 16, 2017

kaustubh-karkare requested changes Jul 18, 2017

View reviewed changes

iamanandkr added 7 commits August 17, 2017 00:50

Create vcs.rb

31bc7b1

Create vcs_internals.rb

30df542

Create blob.rb

4c05fbf

Create tree.rb

d7d87d0

Create commit.rb

5332694

Create diff-utility.rb

d1f1658

Update vcs.rb

338179b

kaustubh-karkare requested changes Aug 16, 2017

View reviewed changes

iamanandkr added 10 commits August 29, 2017 19:59

Update vcs.rb

4bfc151

Update vcs_internals.rb

d95ac6b

Create file_storage.rb

0bfea34

Delete blob.rb

d4bb34e

Delete commit.rb

4c00cc7

Delete tree.rb

c42013e

Update vcs_internals.rb

f062559

Update vcs_internals.rb

4a923c2

Update diff-utility.rb

64d6b09

Create vcs_test.rb

1fe0d6f

kaustubh-karkare requested changes Sep 14, 2017

View reviewed changes

iamanandkr added 4 commits October 4, 2017 03:44

Update vcs.rb

4fd8216

Update vcs_internals.rb

b9b2cf3

Update file_storage.rb

fe852d3

Update diff-utility.rb

147ee00

kaustubh-karkare requested changes Oct 4, 2017

View reviewed changes

iamanandkr added 3 commits October 25, 2017 19:49

Create lcs.rb

c7ab2ad

Create file_diff.rb

7829c50

Delete diff-utility.rb

73c03a2

iamanandkr added 17 commits October 30, 2017 03:28

Update file_diff.rb

bc025fe

Rename file_diff.rb to diff_utility.rb

4dc4f10

Update and rename file_storage.rb to vcs_objects.rb

293ebfa

Update vcs.rb

d53985d

Delete vcs_internals.rb

256db12

Create vcs_commands.rb

31bdf8e

Create exceptions.rb

a71f996

Create file_utilities.rb

d0325b2

Update vcs_commands.rb

b4aca5f

Update vcs.rb

a89399d

Update diff_utility.rb

398bed5

Update file_utilities.rb

cce4e35

Update vcs_internals.rb

4f2a6f3

Update vcs.rb

5710482

Update vcs_objects.rb

34900b6

Update vcs.rb

0c4c349

kaustubh-karkare closed this Aug 17, 2019


		#### Binary Large Objects(BLOB)

		The blob class stores the following:

		end


		def create_object_file(header, path_or_content, sha_hash)

		end


		def get_files_in_tree(sha_hash, directory_path=Dir.getwd)

version-control-system #45

version-control-system #45

Conversation

iamanandkr95 commented Jul 16, 2017

kaustubh-karkare left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaustubh-karkare left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaustubh-karkare left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaustubh-karkare left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaustubh-karkare commented Aug 17, 2019

kaustubh-karkare left a comment •

edited

Loading