pijul_org / pijul

#277 Show that a patch is a tag from the cli

Opened by yory, on May 14, 2018
Feature
Closed
yory commented on May 14, 2018

I just noticed that:

  1. in the log, tags cannot be distinguished from actual patches.
  2. we don't have a command to list all the tags present in a branch or repo. The only place I found to know if a patch is a tag is show-dependencies on the patch.

I'm currently trying to implement it myself, so please wait for me.

yory commented on May 14, 2018

Implemented 1. 2 will be trickier for me, but I'll try later.

pmeunier commented on May 14, 2018

Thanks for the patch, I'm going to apply it now. There is currently no table of tags (we try to keep the number of tables as small as possible), so the best you can do for 2 is traverse the patches, collect the tags, and output that.

yory commented on May 14, 2018

yes, that's what I planned to do! Thank'you for accepting the other patch, I need it for my gui viewer

yory commented on May 14, 2018

Finished!

EDIT: compiles fine, so if you like it, you can merge.

lthms commented on May 14, 2018

Regarding this second patch, I would rather use the occasion to create a new command show, with two subcommands, dependencies and tags (and why not branches?). We could deprecate show-dependencies.

(also, glad to see that having several branches in the repository now is useful :D)

yory commented on May 14, 2018

@lthms Not sure about this (though definitely show-dependencies is very ugly).

  1. more to type: pijul show --dependencies
  2. why show? I'd expect show to print info on a specific patch (metadata, content, dependencies).

While I'm in favor of merging patch and show-dependencies into show, I think that branches and tags belong each to their own subcommands

lthms commented on May 14, 2018

I don't know. pijul patch is actually used as-is for remote stuff, so we should probably let it at least for backward compatible reason (or brace ourselves for impact of curious users who cannot use the nest). Also, I like the subcommand approach better, but there is no rational here, so…

On a side note, I’ve reviewed your patch, and you should use the is_tag() function. Currently, there is only one flag defined, but this might change in the future, and if so, your patch won't have the expected behaviour.

is_patch is defined in libpijul/src/patch.rs:299, and is used in show-dependencies.

yory commented on May 15, 2018

Fixed! I actually knew about is_tag(), but ended up not using it because it's only available for Patch, not PatchHeader, which I preferred because it seems it might be more performant (though I really don't know) and is used in pijul log.

I added a "--label-only" flag to only display the label.

lthms commented on May 15, 2018

is_tag could (should?) probably be implemented for PatchHeader too (and the is_tag of Patch would be a call to is_tag of PatchHeader). This way, you shouldn’t have to load the entire patch, and only the header would be sufficient.

Also, wouldn’t it be better to just use the same display than for pijul log? (and don't remember just now where it is, maybe in ask.rs)

(Sorry for the incremental review, I’m still new at this)

yory commented on May 15, 2018

I implemented it that way because I understand tags as labels, not patches. But maybe now that we have --label-only I could implement the default using the same as log. What do you think? (Yes, it's in ask.rs; it would be very easy to use that, I just thought it's not fitting for the purpose of this command)

I too believe that is_tag should be implemented for PathHeader, it makes more sense.

yory commented on May 15, 2018

You know what? I'll implement it as you suggested. It's better. Only doubt: what should be the default view? compact or verbose?

yory commented on May 15, 2018

Done! Let me know if there's something else to fix

lthms commented on May 15, 2018

Neat! I think we are almost good to go, I’d just call setup_pager once (before the for).

yory commented on May 15, 2018

Ops, missed that =D Fixed now.

lthms commented on May 16, 2018

Yay!

Let me try that this evening, just to be sure, and then I will happily apply your patch. Thanks a lot!

pmeunier commented on May 16, 2018

Some of the patches in this discussion conflict. @yory, could you please remove the extra ones?

Also, would you care to write one test (just to make sure our coverage doesn't decrease too much, these things are hard to get back up).

yory commented on May 16, 2018

The extras should be gone now. It's only one patch: 6rWFPzF7AF3XXvUVXJpwG54TxYHnzy7LDmMBnichUNsT9jiTxqgynZ9xcJAmZafGnFWrGDjdW5PYsWWcPFDLqix8

As for the test, I'll see to it later, either today or tomorrow.

yory commented on May 16, 2018

How can I tell bats that some parts of the output can change from run to run (in my case, the hash, internal id and timestamp)?

pmeunier commented on May 16, 2018

The way it's done in other tests is to parse the output of the command, the hash is printed.

yory commented on May 17, 2018

I can't find how to make sed work. This command seems totally legit but doesn't work: pijul tags | sed -e 's/Hash: .*/Hash: <witheld>/g'

yory commented on May 17, 2018
yory commented on May 17, 2018

I suspect it's because pijul is sending this:

Hash:(B 78i3cUK4BE3eX3j5T3juPe7ktbP9wD7SJBE3FfBkED4Kmdh15W4BXbokjS5xyZ5ij2uusn1hvNfT6uhrG244ppaX
lthms commented on May 17, 2018

Maybe use NOPAGER=1 pijul ...?

yory commented on May 17, 2018
yory commented on May 18, 2018
yory commented on May 19, 2018
yory commented on May 19, 2018

Done! whew

yory commented on May 19, 2018

All ready for merging.

lthms commented on May 19, 2018

Thanks a lot for your dedication. Unfortunately, I will not be able to review and apply your patch right now, but I will probably be able to do it this evening.

Again, thanks!

yory commented on May 19, 2018

No probs, take your time!

lthms commented on May 21, 2018

The test fails in my case. I will try to have a closer look in order to understand why.

yory commented on May 21, 2018

Strange! I ran it using pijul from master, with this command: ./run_tests.sh cases/tags.bats.

Maybe is the perl version (i'm using it for the regex)? mine is v5.26.2

lthms closed this discussion on March 2, 2019