git promise type must just continue if we have unicode error

[basvandervlies]

closes #99

[olehermanse]

@basvandervlies Thanks. It seems this will work at least for this case since the command being run is git diff and what we do with it is check if the output is empty. However, for other uses of the _git function, I think your solution could be problematic. How about taking the output, attempting to decode to utf-8, if that fails, fall back to filtered ascii? I.e. we can loop through the array and replace anything which is not ascii with ? mark. That seems to mostly preserve the output, to make a nicer log message, and to preserve the emptyness / length of the output as well.

[basvandervlies]

@olehermanse yes I noticed it to check if there is output. This output can be huge and for me it was a surprise that is crashes with an unicode error. I think this will only occurs with the git diff command. The second thing is are we interested in the diff output? It makes more sense to do this:

 13:41 mgmt1:/data/cfengine3 (master)
root# git diff  --shortstat ..origin/master

13:41 mgmt1:/data/cfengine3 (master)
root# git diff  --shortstat ..origin/revert-c7019895
 501 files changed, 4483 insertions(+), 18101 deletions(-)

Another thing is why do we not check the exit code of the command to report when it failed or is this done differently?

[basvandervlies]

@olehermanse yes I noticed it to check if there is output. This output can be huge and for me it was a surprise that is crashes with an unicode error. I think this will only occurs with the git diff command. The second thing is are we interested in the diff output? It makes more sense to do this: (or --numstat)

 13:41 mgmt1:/data/cfengine3 (master)
root# git diff  --shortstat ..origin/master

13:41 mgmt1:/data/cfengine3 (master)
root# git diff  --shortstat ..origin/revert-c7019895
 501 files changed, 4483 insertions(+), 18101 deletions(-)

Another thing is why do we not check the exit code of the command to report when it failed or is this done differently?

[basvandervlies]

@olehermanse Some feedback will be nice!! We have this patch in our prod environment and no issues at all.

[olehermanse]

ACK. Your improvement makes sense, and fixes the issue, I was just thinking that these functions and their handling of exit code and output is a bit "iffy" (i.e. might cause problem if more git commands are used in the future), but you did not introduce these issues.

[olehermanse]

Updating promise-type-git to version 0.2.5 here: cfengine/build-index#516

[olehermanse]

@basvandervlies Thank you for your contribution, it has been released as version 0.2.5 of the module: https://build.cfengine.com/modules/promise-type-git/0.2.5/

[basvandervlies]

@basvandervlies Thank you for your contribution, it has been released as version 0.2.5 of the module: https://build.cfengine.com/modules/promise-type-git/0.2.5/

Your welcome. I already have update the git promise type!. It easy with cfbs.