Skip to content

cmd-diff: a few misc enhancements #4253

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.

Sign up for GitHub

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

cmd-diff: a few misc enhancements #4253

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a109ea7
cmd-diff: refactor diff_cmd_output into a loop
dustymabe Aug 12, 2025
a74696c
cmd-diff: rename vars in diff_cmd_outputs
dustymabe Aug 12, 2025
4e0cf9e
cmd-diff: support `cd` strategy for diff_cmd_outputs
dustymabe Aug 12, 2025
ca158e2
cmd-diff: support `git difftool` so we can utilize vimdiff
dustymabe Aug 12, 2025
5590be4
cmd-diff: refactor metal mounting code into helper
dustymabe Aug 12, 2025
8ef5b0e
cmd-diff: support --metal-du and --metal-ls
dustymabe Aug 12, 2025
f034c31
cmd-diff: fix superfluous .stdout call
dustymabe Aug 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 54 additions & 17 deletions src/cmd-diff
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,11 +49,17 @@ TMP_REPO = 'tmp/repo'

DIFF_CACHE = 'tmp/diff-cache'

USE_DIFFTOOL = False


def main():
args = parse_args()
builds = Builds()

# Modify the USE_DIFFTOOL global based on the --difftool argument
global USE_DIFFTOOL
USE_DIFFTOOL = args.difftool
Comment on lines +59 to +61
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

Using a global variable like USE_DIFFTOOL makes the code harder to reason about and test due to hidden dependencies. It's better to make dependencies explicit.

A preferred approach would be to pass the difftool setting down through function arguments. While this might require a larger refactoring, it would improve code quality. A less disruptive alternative could be to encapsulate settings into a configuration object that is created in main() and passed to the functions that need it.

Copy link
Member Author

Choose a reason for hiding this comment

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

A preferred approach would be to pass the difftool setting down through function arguments. While this might require a larger refactoring, it would improve code quality. A less disruptive alternative could be to encapsulate settings into a configuration object that is created in main() and passed to the functions that need it.

indeed. I started this way, but passing that around to all the functions proved a bit more "unclean" than I wanted. It was part me being lazy and part just not wanting to pass it all around because it's ugly.

Though, this is ugly too.

Let's see what other people think.


latest_build = builds.get_latest()

os.makedirs(DIFF_CACHE, exist_ok=True)
Expand Down Expand Up @@ -109,6 +115,7 @@ def parse_args():
parser.add_argument("--to", dest='diff_to', help="Second build ID")
parser.add_argument("--gc", action='store_true', help="Delete cached diff content")
parser.add_argument("--arch", dest='arch', help="Architecture of builds")
parser.add_argument("--difftool", action='store_true', help="Use git difftool")

for differ in DIFFERS:
parser.add_argument("--" + differ.name, action='store_true', default=False,
Expand Down Expand Up @@ -349,7 +356,10 @@ def run_guestfs_mount(image_path, mount_target):
g.close()


def diff_metal(diff_from, diff_to):
# Generator that will mount up metal image filesystems and yield
# the paths to be used for analysis and then clean up once given back
# control.
def diff_metal_helper(diff_from, diff_to):
metal_from = get_metal_path(diff_from)
metal_to = get_metal_path(diff_to)

Expand Down Expand Up @@ -382,8 +392,8 @@ def diff_metal(diff_from, diff_to):
if not p.is_alive():
raise Exception(f"A guestfs process for {os.path.basename(d)} died unexpectedly.")

# Now that the mounts are live, we can diff them
git_diff(mount_dir_from, mount_dir_to)
# Allow the caller to operate on these values
yield mount_dir_from, mount_dir_to
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't a more appropriate model for the single yield case be to just take a function as argument?

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess. Like a callback?

Copy link
Member Author

Choose a reason for hiding this comment

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

Looked into this a little, but time boxed myself on it and ran out of time. Can we leave it and improve later?

Copy link
Member

Choose a reason for hiding this comment

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

Basically I mean something like

def diff_metal_ls(diff_from, diff_to):
	cmd = ['find', '.']
	def differ(mount_dir_from, mount_dir_to):
		diff_cmd_outputs(cmd, mount_dir_from, mount_dir_to, strategy='cd')
	diff_metal_helper(diff_from, diff_to, differ)

or did that not work?


finally:
# Unmount the FUSE binds, this will make the guestfs mount calls return
Expand All @@ -401,23 +411,46 @@ def diff_metal(diff_from, diff_to):
shutdown_process(p_to)


def diff_cmd_outputs(cmd, file_from, file_to):
with tempfile.NamedTemporaryFile(prefix=cmd[0] + '-') as f_from, \
tempfile.NamedTemporaryFile(prefix=cmd[0] + '-') as f_to:
if '{}' not in cmd:
cmd += ['{}']
idx = cmd.index('{}')
cmd_from = list(cmd)
cmd_from[idx] = file_from
subprocess.run(cmd_from, check=True, stdout=f_from).stdout
cmd_to = list(cmd)
cmd_to[idx] = file_to
subprocess.run(cmd_to, check=True, stdout=f_to).stdout
git_diff(f_from.name, f_to.name)
def diff_metal(diff_from, diff_to):
for mount_dir_from, mount_dir_to in diff_metal_helper(diff_from, diff_to):
git_diff(mount_dir_from, mount_dir_to)


def diff_metal_du(diff_from, diff_to):
for mount_dir_from, mount_dir_to in diff_metal_helper(diff_from, diff_to):
cmd = ['find', '.', '-type', 'd', '-exec', 'du', '-sh', '{}', ';']
diff_cmd_outputs(cmd, mount_dir_from, mount_dir_to, strategy='cd')


def diff_metal_ls(diff_from, diff_to):
for mount_dir_from, mount_dir_to in diff_metal_helper(diff_from, diff_to):
cmd = ['find', '.']
diff_cmd_outputs(cmd, mount_dir_from, mount_dir_to, strategy='cd')


def diff_cmd_outputs(cmd, path_from, path_to, strategy='template'):
Copy link
Member

Choose a reason for hiding this comment

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

Feels a bit weird to use a string for this. Couldn't it just be a boolean?

That avoids the assert also.

Copy link
Member Author

Choose a reason for hiding this comment

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

I kind of liked leaving this open to different strategies in the future and also being more explicit in the naming.

i.e. what would the boolean argument's name be? use_cd_not_template=False ?

Copy link
Member

Choose a reason for hiding this comment

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

Or maybe just chdir?

Or we could make it into a proper enum too like OSTreeImport if we want to keep this structure.

workingdir = os.getcwd()
Copy link
Member

Choose a reason for hiding this comment

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

Minor/optional: in situations like this, I prefer to use the signature default so that it's equivalent to not passing an argument at all in the default case. Often (like here), that's None.

Copy link
Member Author

Choose a reason for hiding this comment

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

i.e.

Suggested change
workingdir = os.getcwd()
workingdir = None

? I can make that change if you like.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, exactly.

with tempfile.NamedTemporaryFile(prefix=cmd[0] + '-') as from_output, \
tempfile.NamedTemporaryFile(prefix=cmd[0] + '-') as to_output:
for path, output in (path_from, from_output), (path_to, to_output):
c = list(cmd)
if strategy == 'template':
if '{}' not in c:
c += ['{}']
idx = c.index('{}')
c[idx] = path
else:
assert strategy == 'cd'
workingdir = path
subprocess.run(c, cwd=workingdir, check=True, stdout=output)
git_diff(from_output.name, to_output.name)


def git_diff(arg_from, arg_to):
runcmd(['git', 'diff', '--no-index', arg_from, arg_to], check=False)
subcmd = 'diff'
if USE_DIFFTOOL:
subcmd = 'difftool'
runcmd(['git', subcmd, '--no-index', arg_from, arg_to], check=False)


def cache_dir(dir):
Expand Down Expand Up @@ -457,6 +490,10 @@ DIFFERS = [
needs_ostree=OSTreeImport.NO, function=diff_metal_partitions),
Differ("metal", "Diff metal disk image content",
needs_ostree=OSTreeImport.NO, function=diff_metal),
Differ("metal-du", "Compare directory usage of metal disk image content",
needs_ostree=OSTreeImport.NO, function=diff_metal_du),
Differ("metal-ls", "Compare directory listing of metal disk image content",
needs_ostree=OSTreeImport.NO, function=diff_metal_ls),
]

if __name__ == '__main__':
Expand Down
3 changes: 3 additions & 0 deletions src/deps.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,3 +110,6 @@ python3-libguestfs

# For generating kubernetes YAML files (e.g Konflux resources)
kustomize

# For vimdiff
vim-enhanced
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, how can we structure this so that this functionality is not dependent on the $preferred_tool shipping in cosa? One option is that cosa diff --difftool in that case just outputs the command and you run it.

Or... it could also do something like systemd-run --user -t --same-dir --wait --collect git difftool ... maybe.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah. I like git-delta so being able to plug my own tool would be nice. How about we juste write the git diff output to a file and then we can invoke the tool we want?

Copy link
Member Author

Choose a reason for hiding this comment

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

cosa diff --difftool in that case just outputs the command and you run it.

Only thing about this is it implies not cleaning up after itself.

Or... it could also do something like systemd-run --user -t --same-dir --wait --collect git difftool ... maybe.

This implies functionality we don't necessarily have today. i.e. in the run via bash function case where we run inside the COSA container, we don't have things mounted into the container that would allow it to run a systemd unit on the host.

It definitely sucks installing this into the COSA container too, but it was the easiest thing to do. We don't have to do that, i'd just continue to do what I've been doing, which is cosa shell and then sudo dnf install vim before running cosa diff --difftool

Copy link
Member

Choose a reason for hiding this comment

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

Only thing about this is it implies not cleaning up after itself.

Indeed. I think it's OK in that case to tell the user they're responsible for cleanup.

This implies functionality we don't necessarily have today. i.e. in the run via bash function case where we run inside the COSA container, we don't have things mounted into the container that would allow it to run a systemd unit on the host.

Right yeah, the idea would be to add another option to the alias.

It definitely sucks installing this into the COSA container too, but it was the easiest thing to do. We don't have to do that, i'd just continue to do what I've been doing, which is cosa shell and then sudo dnf install vim before running cosa diff --difftool

Not strongly against to be clear, but I think it'd be a larger win to try to make this work for everyone.

Loading