lint: convert git-subtree-check.sh to Python

jacobpfickes commented at 3:19 am on April 30, 2022: contributor

Converts test/lint/git-subtree-check.sh to a Python script test/lint/git-subtree-check.py as requested in #24783

DrahtBot added the label Tests on Apr 30, 2022

fanquake commented at 7:34 pm on April 30, 2022: member

https://github.com/bitcoin/bitcoin/pull/25039/checks?check_run_id=6237619420:

0./ci/lint/06_script.sh: line 18: test/lint/git-subtree-check.sh: No such file or directory

jacobpfickes force-pushed on Apr 30, 2022

jacobpfickes commented at 10:21 pm on April 30, 2022: contributor

@fanquake Sorry about that oversight. Fixed the issue and squashed. All checks pass now.

in test/lint/git-subtree-check.py:35 in c09765e78e outdated

30+    sq = main = sub = None
31+    stdout = subprocess.check_output(["git", "log", "--grep=^git-subtree-dir: " + dir + "/*$", "--pretty=format: START %H%n%s%n%n%b%nEND%n", commit], universal_newlines=True, encoding="utf8")
32+    for line in stdout.splitlines():
33+        if "START" in line:
34+            sq = line.lstrip().split(' ')[1]
35+        if "git-subtree-mainline:" in line:

laanwj commented at 8:46 am on May 2, 2022:

line.startswith("git-subtree-mainline:")? (also the one below)

in test/lint/git-subtree-check.py:36 in c09765e78e outdated

31+    stdout = subprocess.check_output(["git", "log", "--grep=^git-subtree-dir: " + dir + "/*$", "--pretty=format: START %H%n%s%n%n%b%nEND%n", commit], universal_newlines=True, encoding="utf8")
32+    for line in stdout.splitlines():
33+        if "START" in line:
34+            sq = line.lstrip().split(' ')[1]
35+        if "git-subtree-mainline:" in line:
36+            main = line.split(' ')[1]

laanwj commented at 8:48 am on May 2, 2022:

line.split(':', 2)[1].strip(' ') maybe? (represents the intent of splitting header/value better, even if something is fudged with spaces, or there are more of them than expected)

in test/lint/git-subtree-check.py:59 in c09765e78e outdated

54+    else:
55+        old = latest_squash.split(' ')[0]
56+        rev = latest_squash.split(' ')[1]
57+
58+    # get the tree in the current commit
59+    tree_actual = subprocess.check_output(["git", "ls-tree", "-d", args.commit, args.dir, "| head -n 1"], universal_newlines=True, encoding="utf8")

laanwj commented at 9:13 am on May 2, 2022:

whoops: | head -n 1 is not going to work here, please implement this in Python

jacobpfickes commented at 12:32 pm on May 2, 2022:

fixed by piping output to new subprocess

jacobpfickes force-pushed on May 2, 2022

fanquake requested review from laanwj on May 5, 2022

in test/lint/git-subtree-check.py:56 in 2d1d27afae outdated

51+    if not latest_squash:
52+        print(f"Error: {args.dir} is not a subtree")
53+        sys.exit(2)
54+    else:
55+        old = latest_squash.split(' ')[0]
56+        rev = latest_squash.split(' ')[1]

danielabrozzoni commented at 2:35 pm on May 5, 2022:

tiny nit: you can avoid putting else if you’re exiting in the if branch:

0    if not latest_squash:
1        print(f"Error: {args.dir} is not a subtree")
2        sys.exit(2)
3    old = latest_squash.split(' ')[0]
4    rev = latest_squash.split(' ')[1]

This reduces indentation, but here it’s basically irrelevant :)

jacobpfickes commented at 3:23 pm on May 5, 2022:

Very good point. I thought it helped with readability a bit but can easily go through and remove the else statements after exiting

danielabrozzoni commented at 3:24 pm on May 5, 2022:

As you wish, if you prefer to leave it like that it’s ok for me :)

jacobpfickes commented at 4:13 pm on May 5, 2022:

I decided to remove the else statements. Definitely cleans up the code a bit and should still be easy enough to read. Thanks for the feedback!

danielabrozzoni approved

danielabrozzoni commented at 2:47 pm on May 5, 2022: contributor

light tACK 2d1d27afaefb050196e92e584170b20641668c9b - The code looks good, I tested locally and they produce the same output.

(My tACK is light as I’m not really a bash expert, it’s possible I missed some small differences between the two scripts)

jacobpfickes force-pushed on May 5, 2022

in test/lint/git-subtree-check.py:36 in a53d24779b outdated

31+    stdout = subprocess.check_output(["git", "log", "--grep=^git-subtree-dir: " + dir + "/*$", "--pretty=format: START %H%n%s%n%n%b%nEND%n", commit], universal_newlines=True, encoding="utf8")
32+    for line in stdout.splitlines():
33+        if "START" in line:
34+            sq = line.lstrip().split(' ')[1]
35+        if line.startswith("git-subtree-mainline:"):
36+            main = line.split(':', 2)[1].split(' ')

laanwj commented at 6:13 pm on May 5, 2022:

I don’t think this is correct. What I suggested was .strip(' ') not .split(' ') (see #25039 (review)) Currently you end up with an array in sub which is likely not what you want.

jacobpfickes commented at 6:30 pm on May 5, 2022:

Sorry about that. You’re correct. I completely misread it and just pushed a fix

laanwj commented at 6:16 pm on May 5, 2022: member

Thanks for addressing my comments.

I think the difficulty is properly testing this script. There’s quite some logic in there that isn’t exercised by the CI, and would require some different git aerobics to check.

jacobpfickes force-pushed on May 5, 2022

jacobpfickes commented at 6:34 pm on May 5, 2022: contributor

I think the difficulty is properly testing this script. There’s quite some logic in there that isn’t exercised by the CI, and would require some different git aerobics to check.

Would writing some sort of test script to hit all of the use cases be useful? and if so where is the best place to include something like that?

its0x08 approved

jacobpfickes force-pushed on May 6, 2022

converts git-subtree-check.sh to Python

Converts test/lint/git-subtree-check.sh to a Python script test/lint/git-subtree-check.py.

bb17f72331

jacobpfickes force-pushed on May 6, 2022

laanwj commented at 9:18 am on May 9, 2022: member

Would writing some sort of test script to hit all of the use cases be useful? and if so where is the best place to include something like that?

I don’t know. From my point of view, testing the cases just once is fine, just to make sure the conditionals in the script are hit and do what they are supposed to. Could publish it in a gist or similar.

I don’t think there’s need to run the test for this script say, as part of the CI every time.

laanwj renamed this:
~~lint: converts git-subtree-check.sh to Python~~
lint: convert git-subtree-check.sh to Python
on May 9, 2022

jacobpfickes commented at 11:23 am on May 9, 2022: contributor

Ok makes sense. I’ll start putting together some of the commands used to hit all of the conditionals.

jacobpfickes commented at 1:18 am on May 11, 2022: contributor

@laanwj

I am currently trying to write a test guide but wanted to get your thoughts on a few of the conditionals. I am by no means a git expert so I might not be understanding correctly. I think with the way the script is written a couple of these failure conditionals will never be hit.

The following conditional produces an error if the subtree directory is not found in the commit. https://github.com/bitcoin/bitcoin/blob/bb17f7233100d6660231d99fa5c2e2fc17a7747c/test/lint/git-subtree-check.py#L64-L69 But before it ever gets to that point it will try to find the latest squash (seen below) which will be empty if the directory is not found in the commit and will fail at that point https://github.com/bitcoin/bitcoin/blob/bb17f7233100d6660231d99fa5c2e2fc17a7747c/test/lint/git-subtree-check.py#L35-L59

Also with the use of the following lines the -d flag will only return trees https://github.com/bitcoin/bitcoin/blob/bb17f7233100d6660231d99fa5c2e2fc17a7747c/test/lint/git-subtree-check.py#L65-L66 https://github.com/bitcoin/bitcoin/blob/bb17f7233100d6660231d99fa5c2e2fc17a7747c/test/lint/git-subtree-check.py#L71-L72 which makes this conditional redundant https://github.com/bitcoin/bitcoin/blob/bb17f7233100d6660231d99fa5c2e2fc17a7747c/test/lint/git-subtree-check.py#L74-L76

laanwj commented at 11:39 am on May 11, 2022: member

Thanks for looking into it. To be honest, I’m not really up to speed with git subtree internals either. You could very well be right! Seems the script could be simplified quite a bit, then (but out of scope for this PR). I guess some of it is hardening to protect against git itself doing unexpected things. It’s ok not to have a test plan for those cases.

jacobpfickes commented at 11:46 am on May 11, 2022: contributor

Sure thing! Ok that makes sense. Yeah maybe it is checking for some outliers. I just wanted to make sure I wasn’t missing something or someone with some more knowledge might know better. I wrap up how to trigger the other conditionals then.

DrahtBot commented at 10:07 am on June 15, 2022: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25369 (Unsubtree Univalue by fanquake)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot commented at 1:21 pm on June 16, 2022: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

DrahtBot added the label Needs rebase on Jun 16, 2022

fanquake commented at 1:45 pm on June 16, 2022: member

@jacobpfickes are you still working on this?

jacobpfickes commented at 2:56 pm on June 18, 2022: contributor

@fanquake yes, I am still working on this. Sorry I haven’t had much free time lately. I should be able to dive back in this weekend though. Most of the development work is done but was just going to write a test plan to ensure its right and reviewers can easily validate

achow101 commented at 7:19 pm on October 12, 2022: member

Are you still working on this?

achow101 commented at 5:13 pm on November 10, 2022: member

Closing this as it has not had any activity in a while. If you are interested in continuing work on this, please leave a comment so that it can be reopened.

achow101 closed this on Nov 10, 2022

maflcko added the label Up for grabs on Nov 10, 2022

maflcko removed the label Up for grabs on Nov 10, 2022

maflcko added the label Up for grabs on Nov 10, 2022

bitcoin locked this on Nov 10, 2023

maflcko removed the label Up for grabs on Dec 12, 2023

lint: convert git-subtree-check.sh to Python #25039

Conflicts