Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A couple examples #932

Merged
merged 10 commits into from
Jul 17, 2023
Merged

A couple examples #932

merged 10 commits into from
Jul 17, 2023

Conversation

metatoaster
Copy link
Contributor

This includes a couple examples based on the git ls-tree and git log command. I would have loved to include the --patch flag, however I am completely unfamiliar with the imara-diff library for generating diffs so I am going to leave that for someone else who might be more familiar with this, but I hope this is a good enough start to get that going.

I did include structopt as an additional dev-dependencies to make passing CLI arguments more ergonomic.

Copy link
Member

@Byron Byron left a comment

Choose a reason for hiding this comment

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

Thanks a lot for helping with the examples, it's much appreciated!

For this round, I only took a cursory look and liked what I saw. However, there are a couple of bigger requests I need to make before I can wave this through.

  • Please do not start commits with gix: - this will make cargo smart-release unhappy. If it's a feature you'd like to advertise in the changelog, the feat: prefix can be used.
  • Please do not use structuopt - it has been integrated into clap for a while now and is used here already. Please take a look at src/plumbing/options/mod.rs to get an idea how to do this.

When done, I think I can make all remaining changes myself and merge.

Thanks a lot!

- Implements some of the functionality provided by `git ls-tree`.
- Given the additional parsing of arguments, copying what libgit2 did
  and use `structopt` as an additional dev-dependencies.
- A rather naive first cut.
- Also, output updated to be equivalent to the one produced by running
  `git log --full-history <commit> <path>`.
- Shouldn't have mixed author/commit time (it was always author time
  being displayed by default).
- Unless reverse is required - the git log DAG is unidirectional, the
  whole graph must be read in order for it to be reversed - libgit2 does
  this internally when the `GIT_SORT_REVERSE` is specified.

  See: https://github.com/libgit2/libgit2/blob/v1.6.4/src/libgit2/revwalk.c#L659-L669
- Include some comments that might hopefully help future readers.
- This set of filters will report (non)merge commits by filtering out
  commits with min/max number of parents
- Also refactored the path filter logic and restructure the layout of
  the code block plus add in comments to make fit as an example.
- Also remember to test for empty list of paths and permit that through
  the filter as `any()` returns false on empty - likewise this applies
  to the check on list of parent_ids.
@metatoaster
Copy link
Contributor Author

metatoaster commented Jul 16, 2023

Thanks for the very prompt review. I reworked (rebased the changes to where they belong, including the clap usage) onto every commit to ensure they can bisect cleanly with the current clippy settings (wait, guess there's more being flagged by the CI, I will get this done, probably should have read the full thing), and in the process I also fixed a minor bug that I didn't test with the latest changeset (left out the parentheses for the || of the is_empty() condition...).

…plications.

Please note that these are just examples, which aren't necessarily
production ready in terms of quality or performance.
Copy link
Member

@Byron Byron left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the fixes!

I decided to put the examples to the top-level where they can use clap naturally, which is also a nice differentiator between tiny examples in gix and possibly larger examples in gitoxide.

In any case, once CI is done this PR can be merged. Thanks again for contributing, I hope you have more ideas for examples and find time (and motivation) to implement them :).

@Byron Byron merged commit c6bfee0 into GitoxideLabs:main Jul 17, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants