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

Override file modifiedTime by git commit history #26

Merged
merged 1 commit into from
Oct 15, 2023

Conversation

lawrenceching
Copy link
Contributor

@lawrenceching lawrenceching commented Oct 18, 2020

Override the modifiedTime by git commit history.

  1. Add option "enableShallowClone" to enable/disable shallow clone
  2. When enableShallowClone is false, clone without "--depth 1" so that we can retrieve real commit time for a file

Copy link
Owner

@stevetweeddale stevetweeddale left a comment

Choose a reason for hiding this comment

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

Thanks for this PR! If you could remove the non-functional formatting changes that would make reviewing this a little easier.

I like the idea though. In fact to be honest, assuming I understand what you're doing here, I'm not sure if anyone would ever really want the local file modified time in the graph. I'd be tempted to a) make this the default, and b) strip out the option and role this into the next API-changing release.

@lawrenceching
Copy link
Contributor Author

Hi @stevetweeddale ,

I do think so.

But the problem is, git does not record last modified time for a file.
The only way(If I didn't get it wrong) is to get the timestramp for file latest commit.

In the README, you expects a shallow clone (git clone --depth 1) to speed up buidling a project. That makes sense.
However, shallow commit lost all commit history.

The simple-git git.log(file) method returns the latest commit for given file. If you do a shallow clone, it returns the the latest commit for the whole repository, instead of file.

// https://github.com/stevetweeddale/gatsby-source-git/pull/26/files#diff-a6f3904c41211be170d9b0eb87e49ea8d02d0fd5a7224d6772174b425dacf90fR109
const latest = log.latest;
const {date, message, author_name} = latest;

So, are you expecting to enable this feature by default, and have a longer git clone time?
I'm open for any decision.

BR,
Lawrence

@stevetweeddale
Copy link
Owner

Ah righto, I hadn't connected that a shallow clone doesn't give you accurate commit times. That is significant as the original project this plugin was written for had a big history including binary files so a full clone was prohibitively slow, and I can imagine other users needing the clones to be shallow.

However, I've got recollections of other pull requests that wanted a full checkout rather than shallow clone. It makes sense for there to be an option to switch between a full clone/shallow clone where necessary, though given Gatsby's emphasis on speed my instinct would still be for "fast by default", and only doing a full clone when necessary.

@lawrenceching
Copy link
Contributor Author

Hi @stevetweeddale ,

I updated my change, could you have a look when you have time? Thank you.

@lawrenceching
Copy link
Contributor Author

@stevetweeddale, hope you can grep a chance to merge, and publish the new package to NPM.
Or would it be easier if to transfor the ownership as we discuss in this issue https://github.com/stevetweeddale/gatsby-source-git/issues/38?

@itsmejoeeey
Copy link

I encountered this problem today - I'd love to see these changes merged!

Are any further changes required or is this good-to-go?

@darthsteven
Copy link
Collaborator

We should probably document this in the readme?

Copy link
Collaborator

@darthsteven darthsteven left a comment

Choose a reason for hiding this comment

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

Thanks for your work on this, and your patience!

The code to update the modifiedTime seems to work well for me in testing and seems like a great addition to the source plugin.

Rather than the slightly awkward enableShallowClone: false type construct, which makes me stop and think about exactly what the implication is, let's simply expose the 'fetch depth' as per other tools that do this sort of thing.

So, is there any chance that you could please:

  • Rework this to add a fetchDepth option that defaults to 1, and the pass it all the way through.
  • Add some documentation to the readme about the option, and saying why you might want to set it to something like 0. Also can we document the message and authorName fields too please?

@lawrenceching
Copy link
Contributor Author

Finally I see some progress here.
I will update my PR accordingly this week.

@lawrenceching
Copy link
Contributor Author

lawrenceching commented Aug 5, 2023

Test Evidence

  1. fetchDepth is absent
    All files' modifiedTime is incorrect(the time of my latest commit)
    image

  2. fetchDepth is 0
    modifiedTime and message reflect to correct commit
    image

@darthsteven, good to merge?

@lawrenceching
Copy link
Contributor Author

Hi, any comment on this PR?

if no major concern on this change, I suggest we can merge this change into master.
And leave the minor issues to another PR.

Another round of review&change may delay this change to 2024...

@darthsteven darthsteven merged commit 8be0db3 into stevetweeddale:master Oct 15, 2023
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.

None yet

4 participants