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

cli: don't list commits abandoned by import/fetch, just print the number #2321

Merged
merged 1 commit into from
Oct 3, 2023

Conversation

yuja
Copy link
Contributor

@yuja yuja commented Oct 3, 2023

Apparently, it gets too verbose if the remote history is actively rewritten. Let's summarize the output for now. The plan is to show the list of moved refs instead of the full list of abandoned commits.

Checklist

If applicable:

  • I have updated CHANGELOG.md
  • I have updated the documentation (README.md, docs/, demos/)
  • I have updated the config schema (cli/src/config-schema.json)
  • I have added tests to cover my changes

cli/tests/test_git_push.rs Outdated Show resolved Hide resolved
@ilyagr
Copy link
Contributor

ilyagr commented Oct 3, 2023

Looking at the tests, one downside of this is that without any commit ids, the user might wonder if something important was abandoned. They might think this anyway, but with a commit id they'll see that it's an old version of a commit that they still have (in the common case) or from a branch they just deleted.

This might be helped by printing a couple of commit summaries and then saying "Abandoned XX more commits". Or we could try it as is, or perhaps another idea will appear.

@ilyagr
Copy link
Contributor

ilyagr commented Oct 3, 2023

Perhaps "Abandoned 5 no longer relevant commits" would calm people down compared to "Abandoned 5 commits."? We could try something like that and see if that's good enough.

Somewhat surprisingly, I think it'd calm me down somewhat if I was new to jj. Obviously, I'm guessing.

@yuja
Copy link
Contributor Author

yuja commented Oct 3, 2023

How about "Abandoned N obsolete commits"?

@ilyagr
Copy link
Contributor

ilyagr commented Oct 3, 2023

I think a new person might not understand the difference between "abandoned" and "obsolete". Something like "that became obsolete after the refs moved" or "that became obsolete after the remote-tracking branch was deleted" would be OK.

@ilyagr
Copy link
Contributor

ilyagr commented Oct 3, 2023

Maybe "Abandoned NN commits that became obsolete due to the changes to branches or other refs."?

You can later change the message to mention the specific refs in question.

@martinvonz
Copy link
Member

Maybe "Abandoned NN commits that became obsolete due to the changes to branches or other refs."?

Or "Abandoned N commits that are no longer reachable."? Is that accurate and clear enough?

@ilyagr
Copy link
Contributor

ilyagr commented Oct 3, 2023

Or "Abandoned N commits that are no longer reachable."? Is that accurate and clear enough?

I think that's good enough for me.

I'd prefer it if we mentioned branches or something as the cause, but my version is admittedly a bit verbose and I can't immediately come up with a better one.

Apparently, it gets too verbose if the remote history is actively rewritten.
Let's summarize the output for now. The plan is to show the list of moved refs
instead of the full list of abandoned commits.
@yuja yuja force-pushed the push-wznpvzswxrls branch from 3e243fb to 2f4ef50 Compare October 3, 2023 04:55
@ilyagr
Copy link
Contributor

ilyagr commented Oct 3, 2023

Thank you!

@yuja yuja enabled auto-merge (rebase) October 3, 2023 04:58
@yuja yuja merged commit 902a43a into jj-vcs:main Oct 3, 2023
15 checks passed
@yuja yuja deleted the push-wznpvzswxrls branch October 3, 2023 05:07
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.

3 participants