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

README: Update git subrepo pull with --force flag #24

Merged
merged 1 commit into from
Oct 3, 2023

Conversation

joverlee521
Copy link
Contributor

In walking through git subrepo pull with @j23414 for monkeypox, we ran into unexpected merge conflicts.¹

Reading through the git subrepo docs,² I found that we can just make a clean "reclone" of the subrepo with the --force flag. Instead of working through the conflict resolution, the forced reclone made sense for just getting the latest version of the ingest repo.

If we are operating on a "pull-only" model for vendoring the ingest repo, using the --force flag seems like a reasonable default.

¹ https://gist.github.com/joverlee521/b00739c3dcbff66ca99828d43cb80a42
² https://github.com/ingydotnet/git-subrepo#commands

Checklist

  • Checks pass
  • If adding a script, add an entry for it in the README.

@joverlee521 joverlee521 requested a review from a team September 20, 2023 20:55
Copy link
Member

Choose a reason for hiding this comment

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

(non-blocking, just answering my own question of "why was there a merge conflict?")

I was hoping that merge conflicts could be avoided if commits were applied sequentially from the last time the remote was pulled, but now I realize that's not what git subrepo is doing (it doesn't store the last commit pulled from the remote). For example, if 6e955d7 and 6f196f7 were applied in that order, there should be no merge conflict. However, it looks like the latest contents are fetched and attempted as a single commit, thus failing on any conflicts.

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, I don't fully understand why there was a merge conflict. I just pulled in nextstrain/rsv#40 without the --force flag and did not run into any conflicts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, must have something to do with the parent commit in the monkeypox repo. I was able to pull in without the --force flag after changing the parent commit in this test branch.

Copy link
Member

Choose a reason for hiding this comment

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

@joverlee521 that's weird! A manual search reveals that setting parent to anything nextstrain/mpox@f96fa06 or later prevents merge conflict.

I'd think it's related to my force-push in nextstrain/mpox#175, but the commit where it starts having conflicts is slightly off...

$ git log --oneline --graph  # with my own annotations in parentheses
…
* 3037946 (WORKS) update parent
*   b766498 (WORKS) Merge pull request #175: Use centralized scripts for NCBI Virus
|\  
| * 871817b (WORKS) Use centralized scripts for NCBI Virus
| * afeec9c (WORKS) git subrepo pull (merge) ingest/vendored
* |   f96fa06 (WORKS) Merge pull request #176: Update CI workflow triggers
|\ \  
| |/  
|/|   
| * 00f324e (CONFLICTS) Update CI workflow triggers
|/  
*   fb81750 (CURRENT-CONFLICTS) Merge pull request #173: Use default config and partial overrides
|\  
| * 4140aff (CONFLICTS) Set hmpxv1-focused defaults
| * 55f7569 Move common values to default config, allow partial overrides
|/  
*   12ccb42 Merge pull request #170 from nextstrain/remove-augur-recursion-limit
…

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is making me think that we don't need to use --force as default, but maybe just document it as a work-around for merge conflicts.

Copy link
Member

Choose a reason for hiding this comment

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

That sounds fine to me.

Now that we're actively using git subrepo, looks like it's not as seamless as we initially hoped... but I think still fine to use for now.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe it'd be better to centralize the currently decentralized usage snippets by putting it in nextstrain/ingest and linking to it from pathogen repo READMEs. That way we only need to make these doc updates in one place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Force pushed with --force as a documented workaround instead of the default.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe it'd be better to centralize the currently decentralized usage snippets by putting it in nextstrain/ingest and linking to it from pathogen repo READMEs. That way we only need to make these doc updates in one place.

Done (I think):

README.md Show resolved Hide resolved
In walking through `git subrepo pull` with @j23414 for monkeypox, we
ran into unexpected merge conflicts.¹

Reading through the git subrepo docs,² I found that we can just make a
clean "reclone" of the subrepo with the `--force` flag. Instead of
working through the conflict resolution, the forced reclone made sense
for just getting the latest version of the ingest repo.

¹ https://gist.github.com/joverlee521/b00739c3dcbff66ca99828d43cb80a42
² https://github.com/ingydotnet/git-subrepo#commands
@joverlee521 joverlee521 merged commit 7617c39 into main Oct 3, 2023
2 checks passed
@joverlee521 joverlee521 deleted the git-subrepo-force branch October 3, 2023 20:27
@joverlee521 joverlee521 mentioned this pull request Oct 16, 2023
2 tasks
joverlee521 added a commit to nextstrain/pathogen-repo-guide that referenced this pull request Oct 16, 2023
Point users to the centralized usage docs of the vendored ingest repo
instead of maintaining a different copy of snippets that may become
out of sync.

Originally suggested by @victorlin in a nextstrain/ingest PR comment¹

¹ nextstrain/ingest#24 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

2 participants