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

main: use actual line number in combined pattern fields #2705

Merged
merged 3 commits into from
Nov 20, 2020

Conversation

AmaiKinono
Copy link
Member

See #2691. This makes it straightforward for client tools to "search the nearest occurence of pattern around the line", making good use of both the line number and search pattern.

I'll update #2698 once this is merged.

See universal-ctags#2691. This makes it straightforward for client tools to "search the
nearest occurence of pattern around the line", making good use of both
the line number and search pattern.
@coveralls
Copy link

coveralls commented Nov 18, 2020

Coverage Status

Coverage increased (+0.0003%) to 87.079% when pulling a876022 on AmaiKinono:combined-v2-excmd into 06279cb on universal-ctags:master.

@masatake
Copy link
Member

Though I cannot find any strong reason, I would like to avoid a special character within the name of excmd: how about naming combineV2 instead of combine(v2)?

@AmaiKinono
Copy link
Member Author

Done.

@masatake
Copy link
Member

It seems that you forgot to run make -C man QUICK=1 update-docs.

@masatake
Copy link
Member

@AmaiKinono, when merging, please use the "Squash and merge" method for this change.

@AmaiKinono
Copy link
Member Author

OK.

Do we have a criteria of when to use which kind of merging?

@codecov
Copy link

codecov bot commented Nov 18, 2020

Codecov Report

Merging #2705 (a876022) into master (06279cb) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2705   +/-   ##
=======================================
  Coverage   86.99%   86.99%           
=======================================
  Files         185      185           
  Lines       39440    39441    +1     
=======================================
+ Hits        34309    34310    +1     
  Misses       5131     5131           
Impacted Files Coverage Δ
main/writer-ctags.c 98.70% <100.00%> (ø)
main/writer.c 96.36% <100.00%> (ø)
parsers/verilog.c 98.86% <0.00%> (+<0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 06279cb...a876022. Read the comment docs.

@masatake
Copy link
Member

About the difference between "Create and merge" or "Rebase and merge", I have no word.
So I will write here about whether we should use "Squash merge" or not.

I would like to you make a pull request only with semantically meaningful commits. /* I cannot good word(s) for this. */
We should not make a pull request including a commit just for recording your development process.

Though I requested updating, 6649d2b semantically meaningful commit.
acf5e2e represents your (our) development process.

When merging I would like you to unify acf5e2e and 6649d2b into one commit.
You can do it with "git rebsae -i master" from the command line or "Squash merge" on the web.
"fixup!" at the beginning of the commit header of acf5e2e implies you know what I wrote here.
Yes! "git rebase -i --autosquash msater" helps you. "git commit --fixup ..." is also useful. You used "--fixup", don't you?
When doing "git log", you will see I merged some "fixup!" commits. I merged them mistakenly. I should not do so.

About a876022, I wonder how we should do.
If you regard it as part of 6649d2b. In this case, we should unify a876022, acf5e2e, and 6649d2 into one commit.
You can use the commit log of 6649d2 as is.

If you regard the updating of the man page as a separate work, you can keep a876022 as a separate commit from the unification commit of acf5e2e and 6649d2. HOWEVER, when keeping a876022 as a separate commit, I would like you to update the commit log. "fixup: make manpage" is not good. This should be something like "docs(man): update the description for --excmd=combine for v2 pattern".

"squashing" tasks can be done just before merging.
However, I would like to do it incrementally during reviewing. So, during reviewing the commits, I can imagine what kind of commits will be introduced when merging. In that case, you can use "git commit --force push".

The very basic assumption is that one commit does one thing. This policy makes reviewing easier. It also makes rearranging commits and resolving conflicts when rebasing easier. See #2707 as examples.

I guess you already know about much of what I wrote here.
In my experience, when I wrote much to you, you did much much more good things. So I write much to you when I find time:-)

@AmaiKinono
Copy link
Member Author

"git rebase -i --autosquash master" helps you. "git commit --fixup ..." is also useful. You used "--fixup", don't you?

I didn't know these before. Thanks for the tip!

I didn't use --fixup, but typed them manually. I did this since I've saw that prefix in other PRs before. So, I use fixup: but not fixup! for a876022, because I thought it is a hint for human, not git.

I guess you already know about much of what I wrote here.

Indeed. Actually I didn't put my question clear. I know we want these changes to go into a single commit, and it can only be done by "squash and merge" through the GitHub web UI. But using the command line, we could rearrange this branch first, then merge; or we could interactively rebase master against this branch. The difference is whether:

  • we have an "actual" commit, plus a merge commit, and we keep the fact that this work is done on a separate branch. Or,

  • we have only one commit, and the history is linear, looks like the work is done directly on master.

I use the second style in my projects, but I don't know which is preferred by u-ctags when the change is small. That's my question.

In my experience, when I wrote much to you, you did much much more good things. So I write much to you when I find time:-)

That does happen when people work together. You are helping me know how are things done in this repo, and that increases the possibility of me trying to work more/deeper on u-ctags ;)

@masatake
Copy link
Member

I don't know which is preferred by u-ctags when the change is small. That's my question.

I don't care either way. Maybe there are merits and demerits. But I don't know them.

@AmaiKinono AmaiKinono merged commit 4a9f00a into universal-ctags:master Nov 20, 2020
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