Skip to content
This repository has been archived by the owner on Sep 11, 2020. It is now read-only.

Implement git log --all #1045

Merged
merged 3 commits into from
Jan 11, 2019
Merged

Implement git log --all #1045

merged 3 commits into from
Jan 11, 2019

Conversation

kuba--
Copy link
Contributor

@kuba-- kuba-- commented Jan 4, 2019

Signed-off-by: kuba-- kuba@sourced.tech

This PR implements git log --all.
It closes #1024
It's also related to #1023

git log --all pretends as if all the refs in refs/, along with HEAD, are listed on the command line as . We start from the HEAD, so From hash is ignored for --all option.

Please take a look carefully if it can be done in more optimal way.
For filter by file (git log --all --filename) we have to double check if the parent commit comes from the real commit (not just the next object returned by iterator).

@kuba-- kuba-- requested review from ajnavarro, mcuadros, smola and a team and removed request for ajnavarro and mcuadros January 4, 2019 15:54
Signed-off-by: kuba-- <kuba@sourced.tech>
@kuba--
Copy link
Contributor Author

kuba-- commented Jan 8, 2019

@jfontan - this is how it works. Imagine we have following repo:

* 6ecf0ef (HEAD -> master, origin/master, origin/HEAD) vendor stuff
| * e8d3ffa (origin/branch) some code in a branch
|/  
* 918c48b some code
* af2d6a6 some json
*   1669dce Merge branch 'master' of github.com:tyba/git-fixture
|\  
| *   a5b8b09 Merge pull request #1 from dripolles/feature
| |\  
| | * b8e471f Creating changelog
| |/  
* | 35e8510 binary file
|/  
* b029517 Initial commit

so refCommits stores all commits on branch e8d3ffa (origin/branch) some code in a branch till it finds existing (in HEAD) commit: * 918c48b some code then it inserts into the list.
At the end you will get flat list of all commits from all refs along with HEAD:

6ecf0ef (HEAD -> master, origin/master, origin/HEAD) vendor stuff
e8d3ffa (origin/branch) some code in a branch
918c48b some code
af2d6a6 some json
1669dce Merge branch 'master' of github.com:tyba/git-fixture
a5b8b09 Merge pull request #1 from dripolles/feature
35e8510 binary file
b8e471f Creating changelog
b029517 Initial commit

without --all other refs are ignored and we start from any commit (or from HEAD), so just git log would give us:

6ecf0ef (HEAD -> master, origin/master, origin/HEAD) vendor stuff
918c48b some code
af2d6a6 some json
1669dce Merge branch 'master' of github.com:tyba/git-fixture
a5b8b09 Merge pull request #1 from dripolles/feature
35e8510 binary file
b8e471f Creating changelog
b029517 Initial commit

Signed-off-by: kuba-- <kuba@sourced.tech>
// NewCommitAllIter returns a new commit iterator for all refs.
// s is a repo Storer used to get commits and references.
// fn is a commit iterator function, used to iterate through ref commits in chosen order
func NewCommitAllIter(s storage.Storer, fn func(*Commit) CommitIter) (CommitIter, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is similar to revlist.Objects but for commits.
This is just a note that, in the future, we might want to deprecate revlist.Objects in favor of NewObjectAllIter.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe worth to do it now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, but it should go in another PR (IMO)

return nil, err
}
defer refIter.Close()
err = refIter.ForEach(func(r *plumbing.Reference) error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will be great if we can split this function in something more readable

}

// NewCommitFileIterFromIter returns a commit iterator which performs diffTree between
// successive trees returned from the commit iterator from the argument. The purpose of this is
// to find the commits that explain how the files that match the path came to be.
func NewCommitFileIterFromIter(fileName string, commitIter CommitIter) CommitIter {
func NewCommitFileIterFromIter(fileName string, commitIter CommitIter, all bool) CommitIter {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Propagating here the word all maybe is not the best option since is not very meaningful

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, actually it should be something like checkParents

}

// filename matches, now check if source iterator contains all commits (from all refs)
if c.all {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This snippet need a refactor

repository.go Outdated
default:
return nil, fmt.Errorf("invalid Order=%v", o.Order)
}

if o.FileName == nil {
return commitIter, nil
if o.All {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this code can be move to a function

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What exactly do you suggest to extract?
switch-case? else-branch?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know exactly the problem is that I think is very nested and complicated code with a high cyclomatic complexity

Signed-off-by: kuba-- <kuba@sourced.tech>
@kuba--
Copy link
Contributor Author

kuba-- commented Jan 9, 2019

@mcuadros - I've refined Log function. PTAL.

@mcuadros mcuadros merged commit 434611b into src-d:master Jan 11, 2019
@kuba-- kuba-- deleted the enh-1024/log-all branch January 11, 2019 08:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose git log --all equivalent
4 participants