IDEA-157915: Fix symbolic-ref in HEAD #2 #429

illabefat · 2016-07-21T18:01:11Z

No description provided.

illabefat · 2016-07-21T18:10:34Z

@klikh Here is a fix for problem with packed-refs.
For now there is only a problem with drawing of Branch view (ctrl+V->7)
It correctly says that master is a current branch.
The problem is that you can't switch from sym-link branch to original one: you don't have it 'Local Branches' section. (See the attached image)
I don't know what is the best solution here.
And I don't know where to look for the code of this dialog =(

Example:
Branch view after executing git checkout ilzi, which points to master

klikh · 2016-07-25T15:21:47Z

OK, it seems that the problem is a bit more complex than it seemed at first. I think we have to decide on the UI first, and then make an implementation so that it matches the UI we agree on.

Here are my thoughts on this:
0. Let user checkout ilzi branch which is a link to master.

The Push dialog should treat ilzi the same as master, in particular, use master's tracked branch as ilzi's tracked branch.
1.1. Same for Update Project action.
If I checkout ilzi, I should see "Current branch: ilzi" both in the status bar and in the "Git Branches" popup footer. Here I disagree with the command line prompt that shows master.

What do you think about 2, and what other use cases there are?

klikh · 2016-07-25T15:24:24Z

Hm. Have you rebased the pull request? I don't see neither code comments, nor the pull request discussion. Or are they hidden somewhere inside github UI?

illabefat · 2016-07-25T15:29:34Z

I didn't manage to reopen old PR. Either I am github newbie, or github thinks that reopening merged PR is strange, or both of them. So I created a new PR.

klikh · 2016-07-25T15:31:46Z

Ah, my bad. I thought that you've opened a new PR, but didn't find it because I didn't look in closed PRs. Here it is for reference: #424

illabefat · 2016-07-25T15:45:30Z

Regarding usecases, as I understand there are quite few symlink usecases:

List of branches
Checkout symlink
Understand current branch
Create, update, delete symlinks (out of scope for now)

All the other time git works with normal physical branches.
I don't think that IDEA should behave in different way from 'official' git client. Official git client shows master as a current branch, and I think that we should do the same.
I would think about fixing branch view to understand symlinks in head: i.e. 'Current branch: master(ilzi)', not filtering of master from branch list, etc.
But we can also introduce some cool method getRealBranchFromHead and call it from almost everywhere...

klikh · 2016-07-25T16:14:14Z

It seems that to solve these issues, instead of treating symrefs as normal branches and resolving them while reading the repository, we'll have to introduce a special conception for them, e.g. something like GitSymbolicRef that would link an actual GitBranch (or a symbolic ref of previous level) and resolve to the hash or to the branch when needed.

Btw, could you please share your use cases: why do you use links at all, instead of operating the real branches? AFAIK they are not an official feature of Git: back in 2011 Junio said that only the HEAD and refs/remotes/origin/HEADs are officially supported links. Has something changed after that?

illabefat · 2016-07-25T16:28:41Z

Well... we have quite a long names of branches. I want to have alias stable for stable branch =)
Actually IntelliJ's branch view solves this problem, but I perform checkout from terminal, because it usually requires couple more actions.
Not sure if something has changed since Junio's answer, I just wanted to make my work a bit more comfortable.

klikh · 2016-07-26T15:07:17Z

So do you mind trying the approach with separate class for symbolic refs?

And I don't know where to look for the code of this dialog

It is GitBranchPopup. Note that multi-repository case is supported there: branches common for all repositories are displayed at the top-level.

I don't think that IDEA should behave in different way from 'official' git client. Official git client shows master as a current branch, and I think that we should do the same.

Since we see that there is no official support of symbolic refs, following git client looks less necessary. On the other hand, the approach with braces looks nice. Just in reverse order: "stable (master)". IMHO it would be weird to see "Current branch: master" after checking out "stable", wouldn't it? In addition to that, if we display master (or even "master (stable)"), it would be unclear why there is "master" in Git | Branches popup.

illabefat · 2016-07-26T15:14:12Z

Yes, I'll probably try to do something about this

Just in reverse order: "stable (master)". IMHO it would be weird to see "Current branch: master" after checking out "stable", wouldn't it?

Well, it a good point =) I've just written first to come in mind.

What is the case of multi-repository? 2 modules from different folders in one project? git repo inside other git repo?

klikh · 2016-07-26T15:25:45Z

What is the case of multi-repository? 2 modules from different folders in one project? git repo inside other git repo?

Both, and anything else you can imagine. For example, full intellij-community project consists of 3 repositories nested one into another: this one + /android + /tools-base. We have users with dozens of repositories (usually, some web frameworks with many small repositories for each of web site modules).

And we even have users with mixed VCS projects having e.g. a Git repository, a Hg repository and an SVN repository in a single project. However, this case is not related to the original issue: here we care only about multiple Git repositories.

Repositories are registered in Settings | Version Control by specifying the path and the VCS.

In IDEA some operations are synchronous for all repositories by default (Update Project, Push, Commit), some have no sync support yet (e.g. Git | Pull, Git | Stash). Branches can operate synchronously or individually depending on the value of Settings | Version Control | Git | Control repositories synchronously. If this checkbox is enabled, you can e.g. checkout a common branch in all repositories. See this blog post for details.

illabefat · 2016-08-22T20:33:16Z

@klikh
Here is a draft of fix.
Please, have a look at this.
Current branch is written as 'ilzi(master)', symbolic-refs in list of branches point at actual remote.

Push is being performed to actual remote, pull also works.

If this is OK, I will continue with some tests for it

klikh · 2016-08-26T12:54:44Z

Looks good. I'd add a space between "ilzy" and "(master)".

Will add some comments to the code.

And could you please squash 39b8698 and 7966365, to avoid unfinished work in history ;)

klikh · 2016-08-26T13:01:09Z

Please add tests at least for the following cases:

HEAD points to reflink => current branch is reflink, but its hash is correctly resolved.
reflink points to another reflink which point to a real ref
reflink points to remote branch.
cyclic reflinks don't get resolved (probably absent in the list of branches), but don't crash the whole thing :)

You may use either GitNewRepositoryReader or GitRepositoryReader approach, as you wish. Personally, I find New one more clear since it allows to prepare the case manually, and not to look into testData to understand the test case.

klikh · 2016-08-26T13:03:39Z

plugins/git4idea/src/git4idea/GitReference.java

@@ -28,7 +28,7 @@

  public static final TObjectHashingStrategy<String> BRANCH_NAME_HASHING_STRATEGY = FilePathHashingStrategy.create();

-  @NotNull protected final String myName;
+  @NotNull private final String myName;


It is good to encapsulate the name, but I'd prefer it to be made in a separate commit, since it doesn't directly apply to the symbolic-ref feature.

klikh · 2016-08-27T12:40:50Z

plugins/git4idea/src/git4idea/repo/GitRepositoryReader.java

-  private static Map<String, Hash> getResolvedHashes(@NotNull Map<String, String> data) {
-    Map<String, Hash> resolved = ContainerUtil.newHashMap();
+  private static Map<String, RefInfo> getResolvedHashes(@NotNull Map<String, String> data) {
+    Map<String, RefInfo> resolved = ContainerUtil.newHashMap();
    for (Map.Entry<String, String> entry : data.entrySet()) {
      String refName = entry.getKey();
      Hash hash = parseHash(entry.getValue());


This is not a problem of this PR (since the problem already exists), mentioning here just for reference (mostly for myself): this method is potentially buggy, since it will treat a branch name matching [a-zA-Z0-9] as a hash.

klikh · 2018-07-22T16:10:14Z

@illabefat So is this feature is still actual for you?

Ilya Zinoviev added 3 commits July 5, 2016 23:53

IDEA-157915: Fix symbolic-ref in HEAD

593217d

IDEA-157915: Fix symbolic-ref in HEAD. After code review

3a794ef

IDEA-157915: Fix symbolic-ref in HEAD. Support packed-refs

301c9e2

illabefat closed this Jul 21, 2016

illabefat reopened this Jul 21, 2016

Ilya Zinoviev added 3 commits August 8, 2016 16:21

IDEA-157915: Create GitSymbolicRef.java

39b8698

IDEA-157915: almost ready

7966365

IDEA-157915: Fix for push dialog to get actual tracked info

349556c

klikh reviewed Aug 26, 2016
View reviewed changes

klikh reviewed Aug 27, 2016
View reviewed changes

klikh self-assigned this Jul 21, 2018

klikh added the Waiting for Reply The author needs to provide additional details requested by IntelliJ IDEA developers label Jul 30, 2018

karollewandowski force-pushed the master branch from 08f2151 to d1f830e Compare April 25, 2023 19:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IDEA-157915: Fix symbolic-ref in HEAD #2 #429

IDEA-157915: Fix symbolic-ref in HEAD #2 #429

illabefat commented Jul 21, 2016

illabefat commented Jul 21, 2016

klikh commented Jul 25, 2016

klikh commented Jul 25, 2016

illabefat commented Jul 25, 2016

klikh commented Jul 25, 2016

illabefat commented Jul 25, 2016 •

edited

klikh commented Jul 25, 2016 •

edited

illabefat commented Jul 25, 2016

klikh commented Jul 26, 2016

illabefat commented Jul 26, 2016

klikh commented Jul 26, 2016

illabefat commented Aug 22, 2016

klikh commented Aug 26, 2016

klikh commented Aug 26, 2016 •

edited

klikh Aug 26, 2016

klikh Aug 27, 2016

klikh commented Jul 22, 2018

IDEA-157915: Fix symbolic-ref in HEAD #2 #429

Are you sure you want to change the base?

IDEA-157915: Fix symbolic-ref in HEAD #2 #429

Conversation

illabefat commented Jul 21, 2016

illabefat commented Jul 21, 2016

klikh commented Jul 25, 2016

klikh commented Jul 25, 2016

illabefat commented Jul 25, 2016

klikh commented Jul 25, 2016

illabefat commented Jul 25, 2016 • edited

klikh commented Jul 25, 2016 • edited

illabefat commented Jul 25, 2016

klikh commented Jul 26, 2016

illabefat commented Jul 26, 2016

klikh commented Jul 26, 2016

illabefat commented Aug 22, 2016

klikh commented Aug 26, 2016

klikh commented Aug 26, 2016 • edited

klikh Aug 26, 2016

Choose a reason for hiding this comment

klikh Aug 27, 2016

Choose a reason for hiding this comment

klikh commented Jul 22, 2018

illabefat commented Jul 25, 2016 •

edited

klikh commented Jul 25, 2016 •

edited

klikh commented Aug 26, 2016 •

edited