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

jacobpfickes · 2022-04-30T03:19:48Z

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

fanquake · 2022-04-30T19:34:35Z

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

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

jacobpfickes · 2022-04-30T22:21:12Z

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

test/lint/git-subtree-check.py

danielabrozzoni

light tACK 2d1d27a - 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)

test/lint/git-subtree-check.py

laanwj · 2022-05-05T18:16:15Z

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 · 2022-05-05T18:34:47Z

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?

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

laanwj · 2022-05-09T09:18:57Z

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.

jacobpfickes · 2022-05-09T11:23:39Z

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

jacobpfickes · 2022-05-11T01:18:13Z

@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.

bitcoin/test/lint/git-subtree-check.py

Lines 64 to 69 in bb17f72

    
           # get the tree in the current commit 
        
           tree_contents = subprocess.Popen(["git", "ls-tree", "-d", args.COMMIT, args.DIR], stdout=subprocess.PIPE, universal_newlines=True, encoding="utf8") 
        
           tree_actual = subprocess.check_output(["head",  "-n 1"], stdin=tree_contents.stdout, universal_newlines=True, encoding="utf8") 
        
           if not tree_actual: 
        
               print(f"FAIL: subtree directory {args.DIR} not found in {args.COMMIT}") 
        
               sys.exit(1)

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

bitcoin/test/lint/git-subtree-check.py

Lines 35 to 59 in bb17f72

    
           def find_latest_squash(dir, commit): 
        
               sq = main = sub = None 
        
               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") 
        
               for line in stdout.splitlines(): 
        
                   if "START" in line: 
        
                       sq = line.lstrip().split(' ')[1] 
        
                   if line.startswith("git-subtree-mainline:"): 
        
                       main = line.split(':', 2)[1].strip(' ') 
        
                   if line.startswith("git-subtree-split:"): 
        
                       sub = line.split(':', 2)[1].strip(' ') 
        
                   if "END" in line: 
        
                       if sub: 
        
                           if main: 
        
                               sq = sub 
        
                           return(f"{sq} {sub}") 
        
                       sq = main = sub = None 
        
           def main(): 
        
               args = parse_args() 
        
               # find the latest subtree update 
        
               latest_squash = find_latest_squash(args.DIR.rstrip('/'), args.COMMIT) 
        
               if not latest_squash: 
        
                   print(f"Error: {args.DIR} is not a subtree") 
        
                   sys.exit(2)

Also with the use of the following lines the -d flag will only return trees

bitcoin/test/lint/git-subtree-check.py

Lines 65 to 66 in bb17f72

    
           tree_contents = subprocess.Popen(["git", "ls-tree", "-d", args.COMMIT, args.DIR], stdout=subprocess.PIPE, universal_newlines=True, encoding="utf8") 
        
           tree_actual = subprocess.check_output(["head",  "-n 1"], stdin=tree_contents.stdout, universal_newlines=True, encoding="utf8")

bitcoin/test/lint/git-subtree-check.py

Lines 71 to 72 in bb17f72

    
           tree_actual_type = tree_actual.split()[1] 
        
           tree_actual_tree = tree_actual.split()[2]

which makes this conditional redundant

bitcoin/test/lint/git-subtree-check.py

Lines 74 to 76 in bb17f72

    
           if tree_actual_type != "tree": 
        
               print(f"FAIL: subtree directory {args.DIR} is not a tree in {args.COMMIT}") 
        
               sys.exit(1)

laanwj · 2022-05-11T11:39:06Z

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 · 2022-05-11T11:46:04Z

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 · 2022-06-15T10:07:46Z

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 · 2022-06-16T13:21:56Z

🐙 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".}

fanquake · 2022-06-16T13:45:28Z

@jacobpfickes are you still working on this?

jacobpfickes · 2022-06-18T14:56:26Z

@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 · 2022-10-12T19:19:08Z

Are you still working on this?

achow101 · 2022-11-10T17:13:57Z

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.

DrahtBot added the Tests label Apr 30, 2022

jacobpfickes force-pushed the convert_git_subtree_check branch 2 times, most recently from a7c6d82 to c09765e Compare April 30, 2022 20:38

laanwj mentioned this pull request May 1, 2022

Port lint scripts to Python #24783

Closed

25 tasks

laanwj reviewed May 2, 2022

View reviewed changes