Subcommand show-dependencies

Development
1

H there.

Yesterday, I have pushed yesterday a first patch to introduce a new subcommand, that is show-dependencies. The goal is to output a .dot files which can be rendered with dot -Tpng for instance.

The first version of this subcommand takes the same arguments as pijul changes. My goal is to add several other way to use it, such as:

  • --hash <hash> to only print one patch and its direct dependencies
  • --hash <hash> --depth <x>, same as before, but recursively with x levels

Do you have other ideas or wishes?

2

Great! I’d prefer it without the --hash argument, so that pijul show-dependencies <hash> would show the dependencies of that patch. That would be consistent with pijul apply.

3

By the way, I know all the other commands do it, but I think

let target = find_repo_root(&wd).ok_or(Error::NotInARepository)?;

is better than the match statement. For example, it saves two levels of indentation for the bulk of the function.

1 Like
4

(FYI, you can edit your post on discourse. It is the pen button at the bottom of your message)

Thanks for your feedback!

I would agree if <hash> is optional and if the tool print the whole graph in case of its absence.

And thanks for the tip, I am still a little rusty with the Rust language, I didn’t write any program for two years, so any advice is welcome.

5

Completely agree - I have caught myself a couple of times going through the commands/*.rs files just to rewrite that match statement :smile:

1 Like
6

That’s the most intuitive behavior, IMO.

7

I will do that, then, and push accordingly.

@pmeunier @flobec: I think you can merge @joeneeman patches before mine. I saw he has made some refactoring on how commands are implemented and I can adapt my code to that “new way” before my patch is merged.

8

So I push a complete and functional version of show-dependencies to the Nest. I use the opportunity to fix a conflict in graph.rs (in a second patch).

@laumann I totally forgot your suggestion… I am very sorry about that.

@pmeunier @flobec The patches are:

Do you have a way to refuse/discard the two other patches I have pushed before and are now irrelevant (sorry about that)?

9

No worries, my input was only stylistic anyways :slight_smile:

I had a few more stylistic considerations: The label_sanitize() function caught my eye. From where it’s used, shouldn’t it be possible to do:

fn label_sanitize(name: &String) -> String {
    name.replace("\"", "\\\"")
}

and then

label_sanitize(&patch.header().name)

to avoid cloning name? A similar consideration might apply for hash_sanitize.

It also surprised me to read:

let target = &find_repo_root(&wd).ok_or(Error::NotInARepository)?;
let repo_dir = pristine_dir(target);

vs

let target = find_repo_root(&wd).ok_or(Error::NotInARepository)?;
let repo_dir = pristine_dir(&target);

I think the second form is more idiomatic, but that’s just my opinion :slight_smile:

10

Oh, and another stylistic thing. Instead of

String::from("foo")

consider

"foo".to_owned()
11

Thanks! I will try to address that and, this time, will come back to you all before pushing to the nest. I was eager to finish that, sorry.

12

Yet another version of the patch here. Thanks again for you help, everyone.