rebase with --rebase-merges fails to create refs with valid names

[ROGERSM94]

• I was not able to find an open or closed issue matching what I'm seeing

Setup

• Which version of Git for Windows are you using? Is it 32-bit or 64-bit?

$ git --version --build-options

>git --version --build-options
git version 2.22.0.windows.1
cpu: x86_64
built from commit: d003d728ffa6c0006da875ec6318d3f6b28a4ddb
sizeof-long: 4
sizeof-size_t: 8

• Which version of Windows are you running? Vista, 7, 8, 10? Is it 32-bit or 64-bit?

$ cmd.exe /c ver

Microsoft Windows [Version 10.0.16299.1268]

• What options did you set as part of the installation? Or did you choose the
  defaults?

>type "C:\Program Files\Git\etc\install-options.txt"
Editor Option: Notepad++
Custom Editor Path:
Path Option: Cmd
SSH Option: OpenSSH
CURL Option: WinSSL
CRLF Option: CRLFCommitAsIs
Bash Terminal Option: MinTTY
Performance Tweaks FSCache: Enabled
Use Credential Manager: Enabled
Enable Symlinks: Enabled
Enable Builtin Interactive Add: Enabled

• Any other interesting things about your environment that might be related
  to the issue you're seeing?

Nothing that I would believe would be of interest

Details

• Which terminal/shell are you running Git from? e.g Bash/CMD/PowerShell/other

CMD

• What commands did you run to trigger this issue? If you can provide a
  Minimal, Complete, and Verifiable example
  this will help us understand the issue.

>git rebase --rebase-merges origin/master

• What did you expect to occur after running these commands?

A normal rebase procedure with or without merge conflicts

• What actually happened instead?

I got the following output :

>git rebase --rebase-merges origin/master
error: refusing to update ref with bad name 'refs/rewritten/Merged-PR-183:-Handling-for-message'
hint: Could not execute the todo command
hint:
hint:     label Merged-PR-183:-Handling-for-message
hint:
hint: It has been rescheduled; To edit the command before continuing, please
hint: edit the todo list first:
hint:
hint:     git rebase --edit-todo
hint:     git rebase --continue

Which was resolved after manually replacing all refs with the colon (':') character with a valid character. This led me to believe that the auto-generated labels led to invalid refnames, one would expect the auto-generated refs to be valid.

• If the problem was occurring with a specific repository, can you provide the
  URL to that repository to help us with testing?

[PhilipOakley]

That looks to be an awkward problem with the need to create a ref with a label that has a forbidden Windows character in it "Merged-PR-183:-Handling-for-message". (I haven't followed through to the source of that label - I presume it was a merge commit message title - not sure who auto created that).

I think it's from the line L3221 in sequencer.c } else if (ref_transaction_update(transaction, ref_name.buf, &head_oid,, which calls into refs.c L1443 for the ref_transaction_update() which has that error message.

(line counts from commit 41c74b1 on origin/vs/master).

Not sure if the labels should be sanitised as the best course of action (on Windows). They are only 'temporary' IIUC.

[ROGERSM94]

I agree with your determination of the cause. The commit message was auto-generated by Microsoft's Azure Dev Ops server.

I'm not really sure what the best course of action would be, but I strongly feel that --rebase-merges should not break due to fairly innocuous commit messages. I think the git contribution guidelines even recommend using a colon in the first characters of the commit message for submitted patches:
(from https://github.com/git-for-windows/git/blob/master/Documentation/SubmittingPatches)

The first line of the commit message should be a short description (50
characters is the soft limit, see DISCUSSION in linkgit:git-commit[1]),
and should skip the full stop. It is also conventional in most cases to
prefix the first line with "area: " where the area is a filename or
identifier for the general area of the code being modified...

[PhilipOakley]

I agree with your determination of the cause. ...

Definitely sound like the correct course would be to also report the issue upstream, perhaps with a short patch that regularises the label (I believe there is a function somewhere that will sanitise special characters, utf-8 characters and bytes may also be an issue). The justification you have noted looks solid.
At least getting confirmation regarding the approach to resolving the issue for the wider community.

[dscho]

Yes, I ran into the exact same issue a couple of times, too.

@ROGERSM94 I will explain as well as I can how to fix it, because I am too busy with core priorities at my work to fix it.

Install Git for Windows' SDK

First of all, install the SDK from https://gitforwindows.org/#download-sdk and start it. Both downloading and initializing will take a while, you may want to drink a coffee or three.

Patch the problem

sdk cd git in the Git SDK Bash (which was opened as part of the SDK's initialization, and a Desktop shortcut was installed, too) will clone the Git sources and set up everything ready to go developing.

Now, use your favorite editor to edit sequencer.c. If your favorite editor is VS Code, you may want to run contrib/vscode/init.sh first, this will set up a Build task and Intellisense niceties.

You will want to focus on these lines, where the branch labels are generated: https://github.com/git-for-windows/git/blob/v2.23.0.windows.1/sequencer.c#L4625-L4640

I would like to steer you in particular to https://github.com/git-for-windows/git/blob/v2.23.0.windows.1/sequencer.c#L4638-L4640, where all whitespace characters are turned into dashes.

And this is where we want to make the change, e.g. extending this to also replace the colon with the dash character.

Try it

Build Git

You can build it via sdk build git back in the Git SDK Bash (or via the Build task in VS Code).

Test it

To test this, I would suggest to extend [t/t3430-rebase-merges.sh](https://github.com/git-for-windows/git/blob/v2.23.0.windows.1/t/t3430-rebase-merges.sh), as you will want to have a regression test for this, anyway.

Have a look at the most recently added test case: https://github.com/git-for-windows/git/blob/v2.23.0.windows.1/t/t3430-rebase-merges.sh#L428-L442

The structure should be relatively obvious, and by convention, you can reuse whatever was initialized in the first test case labeled setup: https://github.com/git-for-windows/git/blob/v2.23.0.windows.1/t/t3430-rebase-merges.sh#L11-L19

If I were you, I would probably do something like this:

git checkout -b colon-in-label E &&
git merge -m "colon: this should work" G &&
git rebase -fr E

You can then run the test in the Git SDK Bash by switching to t/ and then running sh t3430-*.sh -i -v -x --run=1,22.

Open a Pull Request

You know how that works, don't you 😄 If not, GitHub has excellent documentation: https://help.github.com/en/articles/about-pull-requests

Please let us know how you fare.

[ROGERSM94]

I'm a bit busy at work this week, but I'll look into trying to fix it this weekend when I'll have more time.

[PhilipOakley]

@dscho do you know if is there an equivalent isBannedOnWindows() function (hopefully with a better name ;-) for use with https://github.com/git-for-windows/git/blob/v2.23.0.windows.1/sequencer.c#L4638-L4640

[dscho]

I'm a bit busy at work this week, but I'll look into trying to fix it this weekend when I'll have more time.

@ROGERSM94 sounds good!

do you know if is there an equivalent isBannedOnWindows() function

@PhilipOakley no, I don't think there is any function (yet) that describes what characters are illegal for file names on Windows. That would be a bit ambiguous, too, as the set differs between FAT and NTFS (as I found out recently, for example, + is valid on NTFS, but not on FAT).

We do have sanitize_refname_component(), though, which is commonly used to sanitize branch names (and label creates a very specific type of branch), but this is not what we need here, I think, as it would still allow colons.

I guess we could go the whole nine yards and just convert everything non-alphanumerical into dashes. That would be the simplest safe solution, in my opinion.

[PhilipOakley]

dscho wrote

I guess we could go the whole nine yards and just convert everything non-alphanumerical into dashes. That would be the simplest safe solution, in my opinion.

My one concern is if we have utf-8 in the commit subject characters - and what is the best way of sanitizing them..? (worst case they are non core ASCII, so we just get 'n' dashes, which becomes another potential hazard (lots of identical labels), never mind folks who have lots of "WIP: merge" commits ;-)

Probably just start with the basics of the core NTFS invalid chars.

[dscho]

My one concern is if we have utf-8 in the commit subject characters

Good point.

All UTF-8 characters that are not ASCII have bit 0x80 set, so we could very easily say if (!(c & 0x80) && !isalnum(c)) c = '-';. That's my preference for now.

[ROGERSM94]

The only other time that this problem presents itself which your fix wouldn't address is when you hit windows reserved filenames (e.g. CON, NuL, LPT, NUL.txt, etc.) but (after the suggested fix) this would require a commit with the oneline consisting of only those invalild filenames, it's probably unlikely to appear. but could be easily resolved by appending an '_' or other innocuous character to the end of the label name

[PhilipOakley]

An apology..

My one concern is if we have utf-8 in the commit subject characters - and what is the best way of sanitizing them..? (worst case they are non core ASCII, so we just get 'n' dashes, which becomes another potential hazard (lots of identical labels), never mind folks who have lots of "WIP: merge" commits ;-)

I was mistaken that utf-8 simply used the sign bit for the higher order bytes for code points beyond 7 bits.

That would have left the last byte with the sign bit not set, as per usage in varints etc., so my follow up to the comment was wrong (that if colon = 003A, then what about say 203A.. thought fail..)

https://en.wikipedia.org/wiki/UTF-8 "Moreover, 7-bit bytes (bytes where the most significant bit is 0) never appear in a multi-byte sequence, and no valid multi-byte sequence decodes to an ASCII code-point."

Sorry for any misunderstandings.

[dscho]

@ROGERSM94 maybe you would have time to test the latest snapshot? https://wingit.blob.core.windows.net/files/index.html

[ROGERSM94]

I should probably be able to get that done later tonight

[dscho]

I should probably be able to get that done later tonight

Were you able to test this?

[ROGERSM94]

Yeah sorry about that it seemed to work great now

…

On Tue, Sep 17, 2019, 5:54 AM Johannes Schindelin ***@***.***> wrote: I should probably be able to get that done later tonight Were you able to test this? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2307?email_source=notifications&email_token=ABLUOJQWHJ2VPYFSRQW65QDQKDARDA5CNFSM4IPPPIEKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD64IT5A#issuecomment-532187636>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABLUOJQETZ7ESD5PCZ6DBMDQKDARDANCNFSM4IPPPIEA> .