Skip to content

Conversation

vegar
Copy link
Contributor

@vegar vegar commented Sep 9, 2020

Changes

  • Add note on escaping curly braces in PowerShell when doing revision selection

Context

I've been struggling with the revision selection syntax HEAD@{...} and finally found out it's because of curly braces special meaning in PowerShell.

I noticed the NOTE on special handling of caret in cmd.exe on Windows. A similar note about curly braces in PowerShell would have saved me some time and I hope to save fellow developers some time by adding in such a note.

Copy link
Contributor

@HonkingGoose HonkingGoose left a comment

Choose a reason for hiding this comment

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

Thanks for contributing! 👍

I have made some suggestions to improve the text. Let me know if you like/hate those. 😄

Proposed changes:

  • Rewrite first sentence to flow better, and to explain the bracket situation a bit better.
  • Remove "either" from the second sentence, as I think the sentence is better without that word. The "or" in the middle of the sentence makes it clear that there are two choices to deal with the bracket issue. So "either" is not really adding value.

How to merge these changes into your pull:

If you like my changes, you can put them in your pull request via the GitHub pull interface, read the
GitHub docs, incorporating feedback in your pull request for more on this.

@vegar
Copy link
Contributor Author

vegar commented Sep 9, 2020

I’m totally fine having my non-native English improved. 😄

One note: PowerShell has become cross-platform. I would guess the same issue is present in powershell on Linux, but I’m not sure. Maybe this should be checked out, or maybe this is good for now and removing the ‘on Windows’ part can be left for an improvement in the future.

@HonkingGoose
Copy link
Contributor

HonkingGoose commented Sep 9, 2020

I’m totally fine having my non-native English improved. 😄

I'm a non-native with English as well, so I'm also learning new things each day.

One note: PowerShell has become cross-platform. I would guess the same issue is present in powershell on Linux, but I’m not sure. Maybe this should be checked out, or maybe this is good for now and removing the ‘on Windows’ part can be left for an improvement in the future.

I've just tried this with PowerShell on Linux. And I can confirm, you need to escape the braces there as well.

@vegar
Copy link
Contributor Author

vegar commented Sep 9, 2020

@HonkingGoose , It ended up being quite a few minor change commits 😆 Do you know how it will be merged - if it gets merged? Will it be squashed?

@HonkingGoose

This comment has been minimized.

@vegar
Copy link
Contributor Author

vegar commented Sep 9, 2020

Wow. That was quite some instructions. Hope :-D
I'm quite used to rebasing, but not the co-author ting. I read about it for the first time a couple of days ago, actually.
Anyway, without looking at your proposal, I would have kept my original commit and then one additional commit with all your improvements. Wouldn't that be ok? Just to keep the history of the original vs the improved note?

I guess the comments in this pull request will go a little bananas after the force push, though...

vegar and others added 3 commits September 9, 2020 22:58
I've been struggling with the revision selection syntax `HEAD@{...}` and finally found out it's because of curly braces special meaning in PowerShell.

I noticed the NOTE on special handling of caret in cmd.exe on Windows. A similar note about curly braces in PowerShell would have saved me some time and I hope to save fellow developers some time by adding in such a note.

Thanks to HonkingGoose for his minor changes.

Co-authored-by: HonkingGoose <34918129+HonkingGoose@users.noreply.github.com>
Correct usage of braces vs brackets
Rewrite first sentence to flow better, and to explain the braces situation a bit better.
Remove "either" from the second sentence, as I think the sentence is better without that word. The "or" in the middle of the sentence makes it clear that there are two choices to deal with the bracket issue. So "either" is not really adding value.

Co-authored-by: HonkingGoose <34918129+HonkingGoose@users.noreply.github.com>
PowerShell on Linux requires the same escapeing of braces as on Windows

Co-authored-by: HonkingGoose <34918129+HonkingGoose@users.noreply.github.com>
@vegar
Copy link
Contributor Author

vegar commented Sep 9, 2020

Had to push it twice to be sure I added the extra line feed. Looks like the double line feed doesn't show, but the co-author seems to work.

Hope you're satisfied with the three commits. I kinda like to keep things separate, so I extracted the linux vs windows thingy into a separate commit just to make it clear that this is for both operating systems.

@HonkingGoose
Copy link
Contributor

Had to push it twice to be sure I added the extra line feed. Looks like the double line feed doesn't show, but the co-author seems to work.

Yeah the double line feed does not really show up in the GitHub commit interface, but the co-author thing works. 👍

Hope you're satisfied with the three commits. I kinda like to keep things separate, so I extracted the linux vs windows thingy into a separate commit just to make it clear that this is for both operating systems.

I'm happy with your 3 commits. I like your commit messages.

Anyway, without looking at your proposal, I would have kept my original commit and then one additional commit with all your improvements. Wouldn't that be ok? Just to keep the history of the original vs the improved note?

For me, I like to squash everything into one commit, that single commit contains all changes. That way there is no chance of ending up with a invalid state when doing a git revert on part of the work but not on all of the work.
But your way is clear and valid too. 👍 If anybody wants to revert the work, they'll need to do the revert on the merge commit. 😄

@vegar
Copy link
Contributor Author

vegar commented Sep 10, 2020

Unless they only want to revet e.g. the linux vs windows part. :-)
I guess this is part of the reason why I like git so much: It's not just a tool to stash away old source code. It's a tool for communicating around code. If anyone reviews this change and thinks 'hey - powershell is a windows thing and the text should say so', they now have a commit telling that no, it's not a windows only thing. So even though the end result is the same it gives the opportunity to see a little bit of the thought process behind the end result as well.

@HonkingGoose
Copy link
Contributor

I'd say we're ready for a review by the head maintainer.

Hi @ben! 😄 👋 Can you take a look and tell us what you think?

Copy link
Member

@ben ben left a comment

Choose a reason for hiding this comment

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

It's been a long journey, but this looks great. Thanks! ✨

@ben ben merged commit e8c5402 into progit:master Sep 25, 2020
@TerriTauranga
Copy link

``

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