Skip to content

Fix git:branch bug when there is more than one branch with equal SHA-ID #7

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.

Sign up for GitHub

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

Open
mrjaros wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Fix git:branch bug when there is more than one branch with equal SHA-ID #7

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d5103a4
Fix git:branch bug when there is more than one branch with equal SHA-ID
Jul 8, 2014
7a06a4b
Fix git:branch bug when there is more than one branch with equal SHA-ID
Jul 8, 2014
d5dd09f
Merge branch 'mstr' - use generic string:join instead of list_join
Jul 9, 2014
65905a6
Fix code to renturn just active (current) branch instead of list
Jul 9, 2014
aa7bef0
Update code to use existing functions for Branch string
Jul 9, 2014
def3bf3
Fix emarassing mistake with include_lib
Jul 11, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions src/git.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,8 +160,8 @@ branch(Repo) ->
false ->
Refs = refs(Repo),
{value, {"HEAD", 'HEAD', H}, Refs2} = lists:keytake('HEAD', 2, Refs),
[B] = [ N || {N, T, C} <- Refs2, T == head, C == H ],
{ok, B};
B = [ N || {N, T, C} <- Refs2, T == head, C == H ],
{ok, list_join(B, "; ")};

@gleber gleber Jul 9, 2014

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This function is supposed to return the same as "git branch". What does "git branch" return in the situation if HEAD has the same sha as multiple branches?

@mrjaros mrjaros Jul 9, 2014

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

But when we're operating on refs, we can't suppose to have distinct ref's sha. By doing
[B] = [ N || {N, T, C} <- Refs2, T == head, C == H ],
you are matching one-element list to refs having tag 'head' and specific sha, while refs list doesn't guarantee this. Let's take a look step-by-step. In this example, I'm on branch 'mstr', but 'master' is in the same place:
29> {ok, Repo} = file:get_cwd().
{ok,"/home/jarek/erlgit"}
30> Refs = git:refs(Repo).
[{"HEAD",'HEAD',"d5103a445892e5d6f3d893a16637368765b112db"},
{"master",head,"d5103a445892e5d6f3d893a16637368765b112db"},
{"mstr",head,"d5103a445892e5d6f3d893a16637368765b112db"},
{"origin/HEAD",remote,
"d5103a445892e5d6f3d893a16637368765b112db"},
{"origin/master",remote,
"d5103a445892e5d6f3d893a16637368765b112db"},
{"v0.3.0",tag,"be3b0ade881666286801ee8b218ecbf29da97558"},
{"v0.3.1",tag,"89f2d81be12f6034db52fd6d71d8a4b96f4ee9de"},
{"v0.3.2",tag,"3dc42981d9e4677654370614888ec3f368421240"},
{"v0.5.0",tag,"da21ef41b800d1fd0c08d7d8f05b4489a0c3d2a8"},
{"v0.7.0",tag,"172e570f39020637d1e98326c746ad776242417f"},
{"v0.7.5",tag,"ef40ae01bf0ad7c1f6b5564940d5426b1c082b4a"}]

As you can see, there are branches 'master' and 'mstr' with the same tag ('head') and sha. Let's get going:
31> {value, {"HEAD", 'HEAD', H}, Refs2} = lists:keytake('HEAD', 2, Refs).
{value,{"HEAD",'HEAD',
"d5103a445892e5d6f3d893a16637368765b112db"},
[{"master",head,"d5103a445892e5d6f3d893a16637368765b112db"},
{"mstr",head,"d5103a445892e5d6f3d893a16637368765b112db"},
{"origin/HEAD",remote,
"d5103a445892e5d6f3d893a16637368765b112db"},
{"origin/master",remote,
"d5103a445892e5d6f3d893a16637368765b112db"},
{"v0.3.0",tag,"be3b0ade881666286801ee8b218ecbf29da97558"},
{"v0.3.1",tag,"89f2d81be12f6034db52fd6d71d8a4b96f4ee9de"},
{"v0.3.2",tag,"3dc42981d9e4677654370614888ec3f368421240"},
{"v0.5.0",tag,"da21ef41b800d1fd0c08d7d8f05b4489a0c3d2a8"},
{"v0.7.0",tag,"172e570f39020637d1e98326c746ad776242417f"},
{"v0.7.5",tag,"ef40ae01bf0ad7c1f6b5564940d5426b1c082b4a"}]}

We assign our head's sha to variable H. It's okay, but when we go a little bit forward:
32> [B] = [ N || {N, T, C} <- Refs2, T == head, C == H ].
** exception error: no match of right hand side value ["master","mstr"]

That's because we have two tuples with same tag and sha, and trying to assign its branches to one-element list.

With my fix, we get just list of these branches. Function list_join becomes handy when we want to return list of branches, while we can't determine which one is active.

@gleber gleber Jul 9, 2014

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

That's wrong to return a list of branches as a string separated by semicolon. If we really want to return list of branches, it should be list of strings, i.e. ["master", "mstr"] instead of "master; mstr".

What does "git branch" command returns in this case? Maybe this function should not use "refs" and detect current branch some other way?

@mrjaros mrjaros Jul 9, 2014

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I agree. Anyway, it is still better to return such info than to crash - until we find a FINAL SOLUTION for this bug, imho. Moreover, current solution fits my needs, but it's not important for this discussion. I'll try to fix it if have more time. For now, I just updated the code with string:join what is obviously better.

@flycodepl flycodepl Jul 9, 2014

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

As mrjaros said:

Anyway, it is still better to return such info than to crash - until we find a FINAL SOLUTION for this bug, imho.

I think that repair bug by implements bad behavior is worse than this bug. Look:

branch(Repo) ->
    case status_is_detached(Repo) of
        false -> {ok, "blah"};
        true -> detached
    end.

This 'fix' causes that we not see crash, but still this is not properly behavior. This behavior is just as incorrect as {ok, "master; some_branch"} - because you can not be on two branches in one moment ;)

If this function cannot be done with use "refs" maybe you should use other git command?
e.g.:

$ git symbolic-ref --short HEAD
master

true ->
detached
end.
Expand Down Expand Up @@ -366,3 +366,7 @@ oksh(Cmd, Opts) ->
oksh(Cmd, Args, Opts) ->
{ok, Rep} = sh(Cmd, Args, Opts),
Rep.

list_join([H], _Separator) -> H;

@gleber gleber Jul 9, 2014

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Use string:join/2 instead

@mrjaros mrjaros Jul 9, 2014

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

good point

list_join([H | T], Separator) ->
[H | [[Separator, S] || S <- T]].