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

Be robust with regard to branch names that contain quotation marks (finishes up on #27) #62

Merged
merged 4 commits into from Jan 7, 2021

Conversation

hartwork
Copy link
Member

@hartwork hartwork commented Jan 4, 2021

Finishes up on pull request #27 by rebasing onto latest master and by adding a simple test.

Sits on top of #50 to avoid merge conflicts for the moment.

@hartwork hartwork requested a review from esc January 4, 2021 15:26
@hartwork hartwork force-pushed the azarkevich-fix-refname-escaping branch from 2293d77 to bce2c11 Compare January 4, 2021 15:28
@hartwork hartwork changed the title Be robust with regard to branch names that contain single ticks ("'") (finishes up on #27) Be robust with regard to branch names that contain tick characters ("'") (finishes up on #27) Jan 4, 2021
@hartwork hartwork added the bug label Jan 4, 2021
@hartwork hartwork force-pushed the azarkevich-fix-refname-escaping branch from bce2c11 to 9f84c62 Compare January 4, 2021 21:19
@hartwork
Copy link
Member Author

hartwork commented Jan 4, 2021

Rebased off of #50 and onto master just now.

@hartwork hartwork marked this pull request as draft January 5, 2021 14:00
@hartwork
Copy link
Member Author

hartwork commented Jan 5, 2021

Turned it back into a draft because:

@esc I just learned about --python from man git-for-each-ref. That would work with all kinds of quotes, even triple ''' and """ because Git does the escaping for us:

git for-each-ref --python --format="[%(objectname), %(*objectname), %(objecttype), %(refname)]"

I haven't checked yet when Git started to offer switch --python. What do you think about --python?

@esc
Copy link
Contributor

esc commented Jan 5, 2021

@hartwork I vaguely recall using that a few years ago, I think I never really understood at the time, what the advantage is, but perhaps the magic is in the quoting?

@hartwork
Copy link
Member Author

hartwork commented Jan 5, 2021

[..] the magic is in the quoting?

Exactly:

# git for-each-ref --python --format="[%(objectname), %(*objectname), %(objecttype), %(refname)]"
[..]
['XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX', '', 'commit', 'refs/heads/double"tick']
[..]
['XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX', '', 'commit', 'refs/heads/single\'tick']
[..]
['XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX', '', 'commit', 'refs/heads/triple"""tick']
[..]
['XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX', '', 'commit', 'refs/heads/triple\'\'\'tick']
[..]

@esc
Copy link
Contributor

esc commented Jan 5, 2021

@hartwork indeed, in that case perhaps this is a good idea?

@hartwork
Copy link
Member Author

hartwork commented Jan 5, 2021

@hartwork indeed, in that case perhaps this is a good idea?

I just checked for the availability of --python in "the real world", see below. Yes, let's go with --python, I'll adjust the pull request shortly.

# docker run --rm -it ubuntu:18.04 bash -c 'sed "\/usr\/share\/man/d" -i /etc/dpkg/dpkg.cfg.d/excludes && apt-get update && apt-get install -y --no-install-recommends git man && sed -e 2p -e d /etc/os-release && git --version && git for-each-ref --help | fgrep -i python'
[..]
VERSION="18.04.4 LTS (Bionic Beaver)"
git version 2.17.1
       git for-each-ref [--count=<count>] [--shell|--perl|--python|--tcl]
       --shell, --perl, --python, --tcl
# docker run --rm -it ubuntu:16.04 bash -c 'apt-get update && apt-get install -y --no-install-recommends git man && sed -e 2p -e d /etc/os-release && git --version && git for-each-ref --help | fgrep -i python'
[..]
VERSION="16.04.6 LTS (Xenial Xerus)"
git version 2.7.4
       git for-each-ref [--count=<count>] [--shell|--perl|--python|--tcl]
       --shell, --perl, --python, --tcl
# docker run --rm -it ubuntu:14.04 bash -c 'apt-get update && apt-get install -y --no-install-recommends git man && sed -e 2p -e d /etc/os-release && git --version && git for-each-ref --help | fgrep -i python'
[..]
VERSION="14.04.6 LTS, Trusty Tahr"
git version 1.9.1
       git for-each-ref [--count=<count>] [--shell|--perl|--python|--tcl]
       --shell, --perl, --python, --tcl

@hartwork hartwork force-pushed the azarkevich-fix-refname-escaping branch from 9f84c62 to 13193a1 Compare January 5, 2021 15:51
@hartwork hartwork marked this pull request as ready for review January 5, 2021 15:51
@hartwork
Copy link
Member Author

hartwork commented Jan 5, 2021

I'll adjust the pull request shortly.

Done.

.. and support branch names with e.g. single tick characters ("'")
properly.

To reproduce (with releases 0.9.0 to 0.10.1):

$ git init
$ git commit -m 'Empty first commit' --allow-empty
$ git branch "',subprocess.call('echo'+chr(32)+'owned',shell=True),'"
$ git-big-picture --graphviz 2>/dev/null  # or similar
owned
@hartwork hartwork force-pushed the azarkevich-fix-refname-escaping branch from 13193a1 to 67eeb4e Compare January 7, 2021 14:59
Copy link
Contributor

@esc esc left a comment

Choose a reason for hiding this comment

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

Asking a question about the test, looks good otherwise!

@@ -125,3 +125,10 @@ try profiling
stats
$ rm stats file.svg
$ ls

be robust with regard to branch names that contain special characters
Copy link
Contributor

Choose a reason for hiding this comment

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

This look convenient, but seems to be somewhat misusing the scruf testing framework since it's not strictly testing features of the command line interface. But, this is probably a lot harder/laborious to do in "regular" python test code with unitest and friends?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's mostly a crash test for now, true, yes. We can make a deal about re-writing it outside of scruf after 1.0.0 if you like.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, let's include it since it solves the problem in a pragmatic way!

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you!

Copy link
Contributor

@esc esc left a comment

Choose a reason for hiding this comment

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

Looks good. Crash tests confirms that it works.

@hartwork hartwork merged commit e840dfb into master Jan 7, 2021
@hartwork hartwork deleted the azarkevich-fix-refname-escaping branch January 7, 2021 17:44
@hartwork hartwork added this to the 1.0.0 milestone Jan 11, 2021
@hartwork hartwork changed the title Be robust with regard to branch names that contain tick characters ("'") (finishes up on #27) Be robust with regard to branch names that contain quotation marks (finishes up on #27) Jan 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants