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

git_resource: ignore stderr in get_patch_count when calling git rev-list #2654

Merged

Conversation

hmmr
Copy link

@hmmr hmmr commented Nov 29, 2021

When trying to build one of our projects on dockerhub, we saw the automatic build failing with a badarg:

#12 129.8 ===> Uncaught error in rebar_core. Run with DIAGNOSTIC=1 to see stacktrace or consult rebar3.crashdump
#12 129.8 ===> Uncaught error: badarg
#12 129.8 ===> Stack trace to the error location:
#12 129.8 [{erlang,list_to_integer,["warning: refname '3.0.0rc4' is ambiguous.\n0"],[]},
#12 129.8 {rebar_git_resource,get_patch_count,2,

It appears that in a situation after git checkout -b <TAG>, when rebar_git_resource:get_patch_count/2 is called, the cloned repo is in a state where it has a branch and a tag of the same name. This causes git rev-list <TAG>..HEAD to print "warning: refname '' is ambiguous." to stderr, which in turn confuses a list_to_integer at the end of that function and causes a badarg. Redirecting stderr to /dev/null seems to be a sufficient and harmless way to deal with this issue.

@ferd
Copy link
Collaborator

ferd commented Nov 29, 2021

Would it make sense to actually detect and surface the warning to the end user so they know the type of ref they chose is ambiguous?

@hmmr hmmr force-pushed the hmmr/master/ignore_stderr_on_git-rev-list branch from 2b68822 to fafa86c Compare November 30, 2021 11:24
@hmmr
Copy link
Author

hmmr commented Nov 30, 2021

Amended, now with the git warning forwarded to the user.

hmmr pushed a commit to TI-Tokyo/stanchion that referenced this pull request Nov 30, 2021
@hmmr
Copy link
Author

hmmr commented Nov 30, 2021

Hold on, there seems to be an issue with the patch. It appears that Warning and PatchLines sometimes get the stderr and stdout lines the wrong way.

…ount/2

In a situation after `git checkout -b <TAG>`, When
rebar_git_resource:get_patch_count/2 is called, the cloned repo is in
a state where it has a branch and a tag of the same name. This causes
`git rev-list <TAG>..HEAD` to print "warning: refname '<TAG>' is
ambiguous." to stdout, which in turn confuses the list_to_integer at
the end of that function and causes a badarg. Redirecting stderr to
/dev/null is sufficient to deal with this issue.

print the warning from git rev-list, if any
@hmmr hmmr force-pushed the hmmr/master/ignore_stderr_on_git-rev-list branch from fafa86c to c537205 Compare November 30, 2021 12:31
@hmmr
Copy link
Author

hmmr commented Nov 30, 2021

Amended again, on account of string:split/2 idiosyncratic behaviour (not the stderr/stdout nondeterminism as I suspected in my previous comment):

Erlang/OTP 22 [erts-10.7] [source] [64-bit] [smp:8:8] [ds:8:8:10] [async-threads:1] [hipe]

Eshell V10.7  (abort with ^G)
1> string:split("aa\nbb\n", "\n").
["aa","bb\n"]
2> string:split("aabb\n", "\n").  
["aabb",[]]
3> 

(note the inconsistency in handling the \n character in one vs two lines) while in python, we have

Python 3.9.9 (main, Nov 26 2021, 16:55:59) 
[GCC 11.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import string
>>> "aa\nbb\n".split("\n")
['aa', 'bb', '']
>>> "aabb\n".split("\n")
['aabb', '']
>>> 

hmmr pushed a commit to TI-Tokyo/stanchion that referenced this pull request Nov 30, 2021
@ferd
Copy link
Collaborator

ferd commented Nov 30, 2021

Yeah that's a thing that can be set. https://www.erlang.org/doc/man/string.html#split-2 mentions supporting default arguments with leading, trailing, or all, and it defaults to leading. Specifying all would have made it work the way you thought, but the weird state of backwards compatibility modules we have prevents us from doing it.

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.

2 participants