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

Trim git output #26

Merged
merged 2 commits into from
Oct 13, 2021
Merged

Trim git output #26

merged 2 commits into from
Oct 13, 2021

Conversation

Jakski
Copy link
Contributor

@Jakski Jakski commented Oct 11, 2021

Solves #25

fatal: credential url cannot be parsed: https://github.com/janet-lang/spork.git

is actually output from https://github.com/git/git/blob/106298f7f9cca4158a980de149ef217751e1f943/credential.c#L512 which apparently tries to retrieve credentials upon seeing newline.

Copy link
Member

@pepe pepe left a comment

Choose a reason for hiding this comment

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

LGTM

@sogaiu
Copy link

sogaiu commented Oct 11, 2021

The trimming seems like a good idea to me.

I am not so sure about the conversion to a string. I guess we'll see :)

@sogaiu
Copy link

sogaiu commented Oct 11, 2021

On second thought, I think if trimming and conversion to a string is ok, I think string/trimr is more appropriate.

Sometimes there is whitespace on the left I think one doesn't want lost.

As an example, some git subcommands encode info in the left "columns" of output which sometimes has non-whitespace and sometimes whitespace, e.g. git branch output might look like:

  dispatch-by-backslash
  doc-thread-new-loop
  ev
  hotfix-new-style-math-bindings
* master
  pr/672
  pr/726

I think it's easier to parse this sort of thing if the left-most whitespace is not removed. (In this example output, IIUC, the 2 leading spaces from the first row would be lost.)

Just my 2 cents.

@bakpakin bakpakin merged commit 087fbe2 into janet-lang:master Oct 13, 2021
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.

4 participants