Checkout of index entries into a directory #315

AP2008 · 2022-01-23T07:48:36Z

No description provided.

Byron

It might be worth taking a look at the reference in #301 to see what it means for git to handle multi-threading correctly. For one, these probably require different code paths, furthermore special care has to be taken to handle file systems with case folding correctly.

Besides that, it's neat to see what rayon could do, even though ultimately any parallel code has to make due with git_features::parallel as these allow to turn parallelism off entirely for completely single-threaded builds.

Furthermore I would have expected object ids from entries to be used to load contents to store in destination files, along with a more test-driven approach. For instance, it's unclear how this code gets executed and validated.

Byron · 2022-01-24T00:18:32Z

As an addition, I think it would be best to reduce the abstraction level to the bare minimum without forcing to create a Worktree abstraction just yet. What's happening here can happen in a function, without the need for any intermediate state.

Furthermore, I think it's beneficial to implement this without threads at first and strive for correctness. From there it should be possible to derive a multi-threaded checkout to reach parity with what git can already do.

For examples on how to avoid dependencies and provide facilities to obtain objects, the git-traverse and git-diff crates are probably helpful examples.

Thanks

AP2008 · 2022-01-24T03:54:05Z

Should I retrieve the contents of an ObjectId using git_odb::Handle and git_odb::FindExt where the git_odb::Handle will be passed by the caller ?
What should be the behaviour when the mode of an entry is a DIR or COMMIT ?
The data retrieved using git_odb::Handle can be copied to the new files when the mode is a FILE or FILE_EXECUTABLE. In the case of SYMLINK, I am creating the symlink using os-specific functions (#cfg[target_family = "unix" or "windows")

Byron · 2022-01-24T10:49:32Z

Should I retrieve the contents of an ObjectId using git_odb::Handle and git_odb::FindExt where the git_odb::Handle will be passed by the caller ?

As mentioned earlier, git-traverse leads the way. In tests, it's ok to pass a closure that uses an object database handle of sorts as you see fit.

What should be the behaviour when the mode of an entry is a DIR or COMMIT ?

For DIR I think a todo!() is appropriate as it requires sparse index handling (I think), and for COMMIT it could be whatever git does. Does it create this as empty directory maybe?

In the case of SYMLINK, I am creating the symlink using os-specific functions (#cfg[target_family = "unix" or "windows")

For handling symlinks and executables I think it's important provide options that mirror options of git itself, which allows to completely ignore the file mode for instance which is typically the case on windows. There is certainly special handling for symlinks on windows as well, along with options that could be passed by the caller. Of course, ultimately there is no way around gating certain functionality depending on the platform or OS, but on top of that git provides options.

AP2008 · 2022-01-24T12:01:54Z

As mentioned earlier, git-traverse leads the way. In tests, it's ok to pass a closure that uses an object database handle of sorts as you see fit.

The tests in git-traverse use git-odb as well. I was planning to do something like this (tried it and it was working):

use git_index::{entry::Mode, State};
use git_odb::{FindExt, Handle};

pub fn copy_index<P: AsRef<Path>>(state: &State, object_handle: &Handle, path: P) -> Result<()> {
    let entries = state.entries();
    let mut buf = Vec::new();
    for entry in entries {
        let obj = object_handle.find(entry.id, &mut buf)?;
        ...
    }
   ...
}

Byron · 2022-01-24T13:41:33Z

And that kind of code is ok in tests, but there can't be a dependency of git-worktree to git-odb. Instead, this is abstracted into a closure like this.

AP2008 · 2022-01-25T07:15:31Z

(Pushed new code.) I have implemented the SYMLINK and FILE modes.
Created an Options struct that can be used to pass options to copy_index.
Implemented the symlinks option to be in accordance with git's behaviour.
Wrote tests.
Not using git-odb any more (except in the tests).

TODO:

Implement Commit, FileExecutable and Dir modes
Make it multithreaded

Am I on the right path ?

Byron

I did a quick review to provide some guidance, while also pointing out the complexity that is in something that seems deceptively simple at first.

Thus it's probably best to take note of everything that is entailed in checking out a git file, and decide what to implement in this PR and maybe what to push into the next one to avoid long-lived PRs.

I strongly recommend to ditch multi-threading in the first iteration, and push it back once everything else works with a focus on correctness, ideally with an encompassing test for certain special behavior. As this can become complex quickly, putting less in each PR is probably more.

Even though it doesn't help to reduce complexity, it should help scoping this PR by looking at the requirements to checking out an entry coming from the respective code in git. There is a lot of cases to handle and to test for.

git-worktree/Cargo.toml

git-worktree/src/lib.rs

git-worktree/src/options.rs

git-worktree/tests/mod.rs

Makefile

git-worktree/src/lib.rs

AP2008 · 2022-01-25T12:27:43Z

The FILE and SYMLINK implementations are pretty much the same as they are in git.
I didn't find much about FILE_EXECUTABLE in git's implementation. Should it just be made executable ?
The other implementations may require a submodule implementation(https://github.com/git/git/blob/main/entry.c#L377:L377) (Does gitoxide have one ?).
Or is there some other way ?

Byron · 2022-01-25T13:31:44Z

I didn't find much about FILE_EXECUTABLE in git's implementation. Should it just be made executable ?

I think here is where the executable bit is set - it's done while opening/creating the file. Maybe something similar is possible with Rust as well.

The other implementations may require a submodule implementation(https://github.com/git/git/blob/main/entry.c#L377:L377) (Does gitoxide have one ?).

This should probably be outsourced into a function that is passed as parameter, that task cannot be performed by the git-worktree crate.

Generally I recommend looking and noting all features that are currently missing here (but available in git) maybe along with references to the git source code. That way it's possible to determine how far this PR should go with its implementation, and what could be next to get to an MVP level of correctness and capability.

AP2008 · 2022-01-25T16:01:55Z

I think here is where the executable bit is set - it's done while opening/creating the file. Maybe something similar is possible with Rust as well.

Done! Had to modify the tests a bit. I set the executable bit only on unix-based systems. On windows, a regular file is created.

Byron · 2022-01-25T23:49:06Z

In order to get this PR on track I think it's best to have an in-person chat on how to proceed, so I reached out to you by email and hope you can make time.

Thanks a lot.

AP2008 · 2022-01-26T08:21:24Z

Should files marked with the flag SKIP_WORKTREE be skipped while copying.
Should the copied files have the timestamps present in the entries ?

Byron · 2022-01-26T08:45:26Z

Should files marked with the flag SKIP_WORKTREE be skipped while copying.

I think so, but can't look up the git source right now.

Should the copied files have the timestamps present in the entries ?

I think it's the other way around, the index entries are adjusted to have the new files stat information.

AP2008 · 2022-01-26T12:31:26Z

What do the secs and nsecs fields in entry::Time represent ? (Unix time ?)

Byron · 2022-01-26T13:03:50Z

Yes, it's something that can be converted into std::time::SystemTime.

AP2008 · 2022-01-27T03:50:07Z

How do I change the timestamps of the entries as the entries function returns an immutable reference ?

Byron · 2022-01-27T08:10:48Z

Feel free to add a method that allows mutable access.

AP2008 · 2022-01-27T11:45:29Z

Why is the time stored in seconds and nanoseconds as u32 ? The nanoseconds will overflow a u32. The std lib stores and returns them as u64 and u128 respectively.
Does the nanos field represent the fractional part of the seconds or does it represent the actual nanoseconds since epoch ?

Byron · 2022-01-27T13:22:24Z

Does the nanos field represent the fractional part of the seconds or does it represent the actual nanoseconds since epoch ?

That's a great question, and I think it's save to assume that it's the fractional part of each second with nanosecond precision. I assume the question arises from having SystemTime that needs to brought down to u32 to match the in-memory representation git uses. In theory, nsec is never bigger than 1000000000, one billion.

AP2008 · 2022-01-27T15:11:13Z

Implemented the skip-worktree and timestamp behaviours. Please take a look.

git-worktree/Cargo.toml

git-worktree/src/lib.rs

AP2008 · 2022-01-28T05:15:33Z

The review changes have been fixed and implemented. Please take a look.

AP2008 · 2022-01-29T13:23:54Z

Fixed. Please take a look.

Byron

Maybe let me resolve conversation items next time so I get to decide if it's resolved or not.

Something you could do to mark them as done is to give it some sort of reaction - that's how I do it in one of my PRs, facilitating the reviewers work who otherwise would have to find and go through resolved comments otherwise before reviewing everything again.

git-index/src/lib.rs

git-worktree/Cargo.toml

git-index/src/lib.rs

git-worktree/src/lib.rs

AP2008 · 2022-01-30T15:28:08Z

Committed. Please take a look.

Byron

It's getting there. As a general note, production code should never use unwrap().

git-index/src/lib.rs

git-worktree/src/lib.rs

Byron · 2022-01-31T11:37:04Z

git-worktree/tests/copy_index/mod.rs

+    for (file1, file2) in repo_files.iter().zip(copy_files.iter()) {
+        assert!(fs::read(file1)? == fs::read(file2)?);
+        #[cfg(unix)]
+        assert!(fs::symlink_metadata(file1)?.mode() & 0b111 << 6 == fs::symlink_metadata(file2)?.mode() & 0b111 << 6);


If there is equality, use assert_eq!. It's odd that clippy doesn't warn about this.
Especially with assertions that aren't entirely obvious to the untrained eye, can you use the third argument to say what it is testing?

Especially with assertions that aren't entirely obvious to the untrained eye, can you use the third argument to say what it is testing?

What about the third argument, a message describing what it is supposed to test?

Done: 25a9dc1

git-worktree/tests/copy_index/mod.rs

AP2008 · 2022-02-02T15:03:32Z

Committed. Please take a look.

Byron · 2022-02-03T07:52:34Z

git-worktree/src/lib.rs

+quick_error! {
+    #[derive(Debug)]
+    pub enum Error {
+        Utf8Error(err: git_object::bstr::Utf8Error) {


The individual variants never repeat the word Error.

Fixed: eaee855

Byron · 2022-02-03T08:45:54Z

git-worktree/src/lib.rs

+            from()
+            display("IO error while writing blob or reading file metadata or changing filetype: {}", err)
+        }
+        FindOidError(oid: git_hash::ObjectId, path: std::path::PathBuf) {


For consistency, this should be called NotFound { oid: git_hash::ObjectId, path: PathBuf}.

Fixed: eaee855

Byron · 2022-02-03T08:48:36Z

git-worktree/src/lib.rs

+            display("IO error while writing blob or reading file metadata or changing filetype: {}", err)
+        }
+        FindOidError(oid: git_hash::ObjectId, path: std::path::PathBuf) {
+            display("unable to read sha1 file of {} ({})", path.display(), oid.to_hex())


There is no such thing as a SHA1 file, for all you know an object couldn't be found. This text should be reworded to reflect that.
I like that the path at which the object would be checked out at is mentioned as well.

The same error is used in the git source code you linked: https://github.com/git/git/blob/main/entry.c#L297:L299

This message originates from a commit made 11 years ago, possibly from a time where packs weren't quite a thing yet, nor SHA256. gitoxide doesn't transcribe git to Rust, but does its best to improve on it.

Done: a4b880c

git-worktree/src/lib.rs

Byron · 2022-02-03T09:02:10Z

git-worktree/src/lib.rs

+                let met = std::fs::symlink_metadata(&dest)?;
+                let ctime = met
+                    .created()
+                    .map_or(Ok(Duration::from_secs(0)), |x| x.duration_since(std::time::UNIX_EPOCH));


This should be let ctime = met.created()?.duration_since(std::time::UNIX_EPOCH)?;

This will fail on Unix as unix does not store ctime. My implementation just sets the ctime to 0 if it does not exist as you had previously suggested.

That's a great catch!

I always felt a bit estranged about Ok(Duration::from_secs(0)) which can be transformed into (this time with correct error handling) …

let ctime = met .created() .ok() .map(|x| x.duration_since(std::time::UNIX_EPOCH)) .transpose()? .unwrap_or_default();

… but it's definitely a bit wordy then.

Let's change Ok(Duration::from_secs(0)) into Ok(Duration::default()) at least.

Done: a4b880c

Byron · 2022-02-03T09:03:47Z

git-worktree/tests/copy_index/mod.rs

+    for (file1, file2) in repo_files.iter().zip(copy_files.iter()) {
+        assert!(fs::read(file1)? == fs::read(file2)?);
+        #[cfg(unix)]
+        assert!(fs::symlink_metadata(file1)?.mode() & 0b111 << 6 == fs::symlink_metadata(file2)?.mode() & 0b111 << 6);


Especially with assertions that aren't entirely obvious to the untrained eye, can you use the third argument to say what it is testing?

What about the third argument, a message describing what it is supposed to test?

Byron

It's important to stop amending to the same commit, instead, create as many commits on top of this one and push these instead, ideally with more descriptive messages than implemented git-worktree

Every time there is a change I have to look through everything and match all my comments, the time this takes is considerable. This gets frustrating as it seems no great effort is made to fully address all change requests either by applying a fix or by proposing alternatives.

Lastly, it would be great to see this rebased onto the latest main branch to resolve the conflict.

Thank you

git-worktree/tests/copy_index/mod.rs

git-worktree/src/lib.rs

Byron

I think it's time to merge main back and resolve conflicts in the process.

Can't be long now till it's ready for a merge.

Byron · 2022-02-07T05:17:38Z

git-worktree/src/lib.rs

+            display("IO error while writing blob or reading file metadata or changing filetype: {}", err)
+        }
+        FindOidError(oid: git_hash::ObjectId, path: std::path::PathBuf) {
+            display("unable to read sha1 file of {} ({})", path.display(), oid.to_hex())


This message originates from a commit made 11 years ago, possibly from a time where packs weren't quite a thing yet, nor SHA256. gitoxide doesn't transcribe git to Rust, but does its best to improve on it.

Byron · 2022-02-07T05:31:58Z

git-worktree/src/lib.rs

+                let met = std::fs::symlink_metadata(&dest)?;
+                let ctime = met
+                    .created()
+                    .map_or(Ok(Duration::from_secs(0)), |x| x.duration_since(std::time::UNIX_EPOCH));


That's a great catch!

I always felt a bit estranged about Ok(Duration::from_secs(0)) which can be transformed into (this time with correct error handling) …

let ctime = met .created() .ok() .map(|x| x.duration_since(std::time::UNIX_EPOCH)) .transpose()? .unwrap_or_default();

… but it's definitely a bit wordy then.

Let's change Ok(Duration::from_secs(0)) into Ok(Duration::default()) at least.

Byron · 2022-02-07T05:33:44Z

git-worktree/tests/fixtures/make_repo.sh

+set -eu -o pipefail
+
+git init -q
+


Here must be git config commit.gpgsign false for this to work everywhere.

Fixed: a4b880c

Byron · 2022-02-07T05:54:41Z

git-worktree/tests/copy_index/mod.rs

+            assert_eq!(
+                fs::symlink_metadata(file1)?.mode() & 0b111 << 6,
+                fs::symlink_metadata(file2)?.mode() & 0b111 << 6,
+                "Testing if the permissions of {} and {} are the same",


There is no need to prefix with "testing if".

To me it's still unclear what these permissions should mean. Is this a normal file, or an executable? Maybe the mode can be represented in octal to be more meaningful to those who used chmod on the commandline before?

All prior assert_eq!(…, "msg") messages should be removed, they are sufficiently clear.

It just checks the bit that represents wether a file is executable or normal and sees if both the files have the same bit.
Changed it to octal: a4b880c

Byron · 2022-02-07T05:58:37Z

git-worktree/tests/copy_index/mod.rs

+    let repo_files = dir_structure(&path);
+    let copy_files = dir_structure(output);
+
+    let srepo_files: Vec<_> = repo_files.iter().flat_map(|p| p.strip_prefix(&path)).collect();


Why prefix repo_files with an s? What does s mean? Or asked differently, could these just be inline to avoid having to make up a new name?

If you think it should be clear these are stripped, maybe create a little closure like and use it like this:

assert_eq!(strip_root_path(path, repo_files), strip_root_path(path, copy_files));

Inlined: a4b880c

AP2008 · 2022-02-07T10:23:25Z

Fixed and resolved conflicts!
How should I handle the DIRECTORY and COMMIT cases now ? I have been looking into git's source code. It copies it as a submodule. Will a new crate like git-submodule be required ?

Byron · 2022-02-07T13:19:36Z

Thanks a lot for your first contribution! There is a lot of potential, and I think it's best channeled after I did my due diligence with analyzing and understanding git-worktree myself. This should lead to smaller tasks that are by themselves better understood - please feel free to subscribe to the tracking issue for updates, and I will ping you once I think it's ready to start.

I also recommend looking at the crate as it is now.

Byron · 2022-02-07T13:25:17Z

I am closing the PR as github seems to not recognize it's already merged.

ErichDonGubler · 2022-03-11T10:37:55Z

@Byron (CC @AP2008): RE: #315 (review): I would strongly recommend git's git rebase --autosquash functionality for alleviating this pain. It makes it a lot easier to track the completion of feedback items, often letting the reviewee point to Git commit hashes instead of just stating, "I promise I fixed this up!" and leaving you to manually diff code yourself.

If you'd like me to give some more concrete examples of how this workflow might apply here, it might be the motivation I need to finally commit to writing a blog post about it. :) I've been dealing with similar review pain points in my professional work at @NZXTCorp.

Byron · 2022-03-12T02:04:20Z

Thanks @ErichDonGubler , that's a great suggestion and a feature I wasn't aware off. A quick search brought me to this article which revealed everything one would want to know on how to use it effectively.
Maybe that's the blog post you'd write, or even the one you have written in the mean while :).

As a summary, I have a learned a lot when trying to create PRs myself and when reviewing them, and it's not exactly working perfectly out of the box as it requires some experience for both involved parties. --autosquash along with commit --fixup can really make the difference here, so I am looking forward to apply it in practice.

Lastly, I think it would be great if GitHub would automatically detect --fixup commits and maybe if provided with the URL of the comment they are fixing, adds this information to the discussion automatically. Is it that sophisticated? I will definitely try to find out :).

ErichDonGubler · 2022-03-22T02:16:52Z

Lastly, I think it would be great if GitHub would automatically detect --fixup commits and maybe if provided with the URL of the comment they are fixing, adds this information to the discussion automatically. Is it that sophisticated? I will definitely try to find out :).

I wish that this was the case, but AFAIK GitHub has no functionality built around a --fixup flow -- just happiness from keeping history append-only during critical points of a review. 😅

Byron reviewed Jan 23, 2022

View reviewed changes

AP2008 force-pushed the implement-worktree branch from 8c5e2f4 to 5751a8e Compare January 25, 2022 07:06

AP2008 force-pushed the implement-worktree branch from 5751a8e to 82c7867 Compare January 25, 2022 07:22

Byron reviewed Jan 25, 2022

View reviewed changes

AP2008 force-pushed the implement-worktree branch from 82c7867 to c4ca4bc Compare January 25, 2022 11:49

AP2008 force-pushed the implement-worktree branch from c4ca4bc to 2aeb56b Compare January 25, 2022 15:59

AP2008 force-pushed the implement-worktree branch from 2aeb56b to d3b0ea7 Compare January 26, 2022 03:09

AP2008 force-pushed the implement-worktree branch from d3b0ea7 to 7f76c4f Compare January 27, 2022 15:10

Byron reviewed Jan 28, 2022

View reviewed changes

AP2008 force-pushed the implement-worktree branch from 7f76c4f to 91c064e Compare January 28, 2022 05:13

Byron requested changes Jan 30, 2022

View reviewed changes

AP2008 force-pushed the implement-worktree branch from 3cdb11d to e975b75 Compare January 30, 2022 15:26

Byron reviewed Jan 31, 2022

View reviewed changes

Implemented git-worktree

4177d72

AP2008 force-pushed the implement-worktree branch from e975b75 to 4177d72 Compare February 2, 2022 11:51

Byron reviewed Feb 3, 2022

View reviewed changes

Byron requested changes Feb 3, 2022

View reviewed changes

git-worktree/tests/copy_index/mod.rs Outdated Show resolved Hide resolved

git-worktree/src/lib.rs Outdated Show resolved Hide resolved

Byron changed the title ~~Implemented git-worktree~~ Checkout of index entries into a directory Feb 3, 2022

AP2008 added 3 commits February 3, 2022 16:55

Refactor errors and remove unwraps

eaee855

Reduce io calls

e838eaa

Refactored tests

25a9dc1

Byron reviewed Feb 7, 2022

View reviewed changes

AP2008 added 3 commits February 7, 2022 15:06

Refactored code and tests

a4b880c

Merge branch 'main' of https://github.com/Byron/gitoxide into Byron-main

2abacf9

Merge branch 'Byron-main' into implement-worktree

73a063a

Byron closed this Feb 7, 2022

AP2008 deleted the implement-worktree branch February 7, 2022 14:32

ErichDonGubler mentioned this pull request Mar 19, 2022

Blog post series: my git workflow (git-rebase --autosquash, git-absorb, git-rerere, git-assembler) ErichDonGubler/ErichDonGubler#5

Open

Checkout of index entries into a directory #315

Checkout of index entries into a directory #315

Conversation

AP2008 commented Jan 23, 2022

Byron left a comment

Choose a reason for hiding this comment

Byron commented Jan 24, 2022

AP2008 commented Jan 24, 2022 • edited Loading

Byron commented Jan 24, 2022 • edited Loading

AP2008 commented Jan 24, 2022 • edited Loading

Byron commented Jan 24, 2022

AP2008 commented Jan 25, 2022

Byron left a comment

Choose a reason for hiding this comment

AP2008 commented Jan 25, 2022

Byron commented Jan 25, 2022

AP2008 commented Jan 25, 2022

Byron commented Jan 25, 2022

AP2008 commented Jan 26, 2022

Byron commented Jan 26, 2022

AP2008 commented Jan 26, 2022

Byron commented Jan 26, 2022

AP2008 commented Jan 27, 2022 • edited Loading

Byron commented Jan 27, 2022

AP2008 commented Jan 27, 2022 • edited Loading

Byron commented Jan 27, 2022

AP2008 commented Jan 27, 2022

AP2008 commented Jan 28, 2022

AP2008 commented Jan 29, 2022

Byron left a comment

Choose a reason for hiding this comment

AP2008 commented Jan 30, 2022

Byron 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

AP2008 commented Feb 2, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Byron left a comment

Choose a reason for hiding this comment

Byron 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

AP2008 commented Feb 7, 2022 • edited Loading

Byron commented Feb 7, 2022

Byron commented Feb 7, 2022

ErichDonGubler commented Mar 11, 2022 • edited Loading

Byron commented Mar 12, 2022

ErichDonGubler commented Mar 22, 2022

AP2008 commented Jan 24, 2022 •

edited

Loading

Byron commented Jan 24, 2022 •

edited

Loading

AP2008 commented Jan 24, 2022 •

edited

Loading

AP2008 commented Jan 27, 2022 •

edited

Loading

AP2008 commented Jan 27, 2022 •

edited

Loading

AP2008 commented Feb 7, 2022 •

edited

Loading

ErichDonGubler commented Mar 11, 2022 •

edited

Loading