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

object.WalkCommitHistory should walk parents in reverse order #223

Closed
mvdan opened this issue Jan 24, 2017 · 10 comments
Closed

object.WalkCommitHistory should walk parents in reverse order #223

mvdan opened this issue Jan 24, 2017 · 10 comments

Comments

@mvdan
Copy link
Contributor

mvdan commented Jan 24, 2017

When you do git log and there has been a merge, you see something like:

  • merge commit
  • merged commit
  • rest of history

But the way WalkCommitHistory is implemented, the first parent is walked first. This means that the history is traversed as:

  • merge commit
  • rest of history
  • merged commit

I see no advantage in doing it this way. Using it the other way is more intuitive, and should also use less memory as merged histories are often much shorter (e.g. PRs) than the main ones.

@mvdan
Copy link
Contributor Author

mvdan commented Jan 24, 2017

@mcuadros
Copy link
Contributor

@smola

@smola
Copy link
Collaborator

smola commented Jan 25, 2017

@mvdan Thank you for taking the time to open the issue.

git log by default uses only one of the many ways that there is to walk the commit history. There are many others supported by options. So we'll be happy to accept a PR adding the behavior you propose, but as an option, since we do have use cases that require walking parents in their original order.

@mvdan
Copy link
Contributor Author

mvdan commented Jan 25, 2017

Assuming the API is frozen and that you'd prefer a separate func, I propose my implementation above with a name like WalkCommitHistoryReverse, explaining what is special about it in its godoc.

@mvdan
Copy link
Contributor Author

mvdan commented Jan 25, 2017

Also, cheers for doing nice Go work from Spain :) I moved away from Barcelona recently.

@smola
Copy link
Collaborator

smola commented Jan 26, 2017

@mvdan The API for v4 is not frozen yet, it will be frozen only once we release v4.0.0.

@mcuadros Any thoughts about the API for this?

WRT working in Spain, note that we're hiring ;)

@mvdan
Copy link
Contributor Author

mvdan commented Jan 26, 2017

Ah, sounds exciting, but not planning to move to Madrid anytime soon :)

The API for v4 is not frozen yet, it will be frozen only once we release v4.0.0.

This is surprising. The docs suggest using v4, so I was assuming it was already stable.

@mvdan
Copy link
Contributor Author

mvdan commented Feb 22, 2017

Status of this? Should I open a pull request?

@mcuadros
Copy link
Contributor

The docs suggest use v4, because the v3 is very limited in terms of functionality. I could thing in the API, but of course a pull request can be open and we can discuss about it.

@mvdan
Copy link
Contributor Author

mvdan commented Feb 22, 2017

Sorry for the confusion, I indeed meant the patch, not the version in the docs.

I'll open a pull request.

@smola smola closed this as completed in 33db30d Mar 27, 2017
gsalingu-ovhus pushed a commit to gsalingu-ovhus/go-git that referenced this issue Mar 28, 2019
fix error shadowing in addFile preprocessor
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants