Skip to content

Use git cmd to add CHANGELOG.md in merge script#3350

Merged
DasSkelett merged 1 commit intoKSP-CKAN:masterfrom
DasSkelett:fix/merge-script-windows
Apr 15, 2021
Merged

Use git cmd to add CHANGELOG.md in merge script#3350
DasSkelett merged 1 commit intoKSP-CKAN:masterfrom
DasSkelett:fix/merge-script-windows

Conversation

@DasSkelett
Copy link
Copy Markdown
Member

Problems

Initially I only wanted to fix this one:

$ python bin/ckan-merge-pr.py 3340
Fetching origin...
Fetching https://github.com/gsantos9489/CKAN.git PTBR_Translation...
Traceback (most recent call last):
  File "C:\git-reps\CKAN\bin\ckan-merge-pr.py", line 122, in <module>
    merge_pr()
  File "C:\git-reps\CKAN\bin\venv\lib\site-packages\click\core.py", line 829, in __call__
    return self.main(*args, **kwargs)
  File "C:\git-reps\CKAN\bin\venv\lib\site-packages\click\core.py", line 782, in main
    rv = self.invoke(ctx)
  File "C:\git-reps\CKAN\bin\venv\lib\site-packages\click\core.py", line 1066, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "C:\git-reps\CKAN\bin\venv\lib\site-packages\click\core.py", line 610, in invoke
    return callback(*args, **kwargs)
  File "C:\git-reps\CKAN\bin\ckan-merge-pr.py", line 118, in merge_pr
    if ckpr.merge_into(ckr)
  File "C:\git-reps\CKAN\bin\ckan-merge-pr.py", line 104, in merge_into
    repo.index.add([repo.changelog_path().as_posix()])
  File "C:\git-reps\CKAN\bin\venv\lib\site-packages\git\index\base.py", line 734, in add
    paths, entries = self._preprocess_add_items(items)
  File "C:\git-reps\CKAN\bin\venv\lib\site-packages\git\index\base.py", line 574, in _preprocess_add_items
    paths.append(self._to_relative_path(item))
  File "C:\git-reps\CKAN\bin\venv\lib\site-packages\git\index\base.py", line 561, in _to_relative_path
    raise ValueError("Absolute path %r is not in git repository at %r" % (path, self.repo.working_tree_dir))
ValueError: Absolute path 'C:/git-reps/CKAN/CHANGELOG.md' is not in git repository at 'C:\\git-reps\\CKAN'

But during merge of #3349 I accidentally committed a CHANGELOG.md with all file endings replaced with CRLF, due to the following problem:

gitpython-developers/GitPython#1189
GitPython's repo.index.add() doesn't respect core.autocrlf. Thus on Windows any files added to the index using this function will have their line endings messed up in the repository.

Changes

Now we use repo.git.add(), which makes GitPython run the git add command using the binary.
Here's where this happens, because I was curious:
https://github.com/gitpython-developers/GitPython/blob/651a81ded00eb993977bcdc6d65f157c751edb02/git/cmd.py#L540-L546

This also seems to solve the above exception, since the git binary understands POSIX paths on Windows.
Regardless I still replaced the .as_posix()with str()(which always returns the path formatted according to the current OS' standards, so double backslash on Windows), because it feels cleaner and more future proof.

Slowly but surely we replace all actual GItPython usages with a call-through to the git binary...

@DasSkelett DasSkelett added Bug Something is not working as intended Pull request Windows Issues specific for Windows Infrastructure Issues affecting everything around CKAN (the GitHub repos, build process, CI, ...) labels Apr 15, 2021
@DasSkelett DasSkelett requested a review from HebaruSan April 15, 2021 21:04
Copy link
Copy Markdown
Member

@HebaruSan HebaruSan left a comment

Choose a reason for hiding this comment

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

👌

@DasSkelett DasSkelett merged commit 630439e into KSP-CKAN:master Apr 15, 2021
@DasSkelett DasSkelett deleted the fix/merge-script-windows branch April 15, 2021 21:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug Something is not working as intended Infrastructure Issues affecting everything around CKAN (the GitHub repos, build process, CI, ...) Windows Issues specific for Windows

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants