pijul_org / pijul

#260 pijul record: description argument is ignored when recording interactively

Opened by laumann, on April 29, 2018
Good first bug
Bug
Closed
laumann commented on April 29, 2018

I tried recording like so:

$ pijul record -d "Fixes #259"
[interactively select and record patch]

The description Fixes #259 does not show up in the log afterwards.

lthms added tag
Bug
on April 29, 2018
lthms commented on April 29, 2018

Thanks for reporting this issue!

laumann commented on April 29, 2018

Well no, I unrecorded the patch without the description and made a new patch with the description :-) Sorry for the confusion.

lthms commented on April 29, 2018

Strange indeed. Maybe it is related to patch dependencies.

I have reviewed your change a bit, and I really like most of what I saw. Thanks a lot!

I have a few comment, though:

  1. I am not sure about your second patch (removing the else). From my point of view, having one or more return in the middle of the function makes its control flow harder to gasp.
  2. In case there is a description passed as argument through the CLI, then --no-editor should be assume. Other possibilities:
  • Init the .pijul/PATCH_NAME file with the description
  • Fail if the user gives a description with its editor when it already has passed one with --description

I think my favourite one is to init the PATCH_NAME file.

What do you think?

laumann commented on April 29, 2018

Thanks for your feedback!

I am not sure about your second patch (removing the else). From my point of view, having one or more return in the middle of the function makes its control flow harder to gasp.

Normally, I would agree. Typically, I prefer something like:

fn foo() -> Option<R> {
    if cond() {
        true_branch_stuff();
        None
    } else {
        Some(false_branch_stuff())
    }
}

because it exploits the expression-oriented if to avoid any returns. This makes sense when (a) both branches are not too long and (b) both branches accomplish something "meaningful". I use the other format when the if is a sanity check or an early error return:

fn foo() -> Option<R> {
    if cond() { // Establish some pre-condition
        true_branch_stuff(); // Not satisfied
        return None;
    }
    // Condition is satisfied
    let e = false_branch_stuff();
    // LOTS more code
    Some(e)
}

In this case the else is essentially the rest of the function body, and the then part is a simple sanity check. The else part also takes up more than 100 lines and doesn't fit in my screen, so I started getting confused with the multiple closing braces.

I can take it out.

In case there is a description passed as argument through the CLI, then --no-editor should be assume

OK. I was wondering how --description should interact with the editor functionality, but I'm still not sure how it should work. If the user provides --description, but not --message, what should happen? Should it essentially, be the same as when --message is provided --no-editor is assumed (with the difference that it can still ask for the message)?

laumann commented on April 29, 2018

To elaborate, the format in record::run() is:

if changes.is_empty() {
    println!("What are you doing?");
    None
} else {
    // Actually record a change in >100 lines
}

I would suggest a middle ground then:

if changes.is_empty() {
    println!("What are you doing?");
    None
} else {
    actually_record_change(...)
}

fn actually_record_change(...) -> Some<...> {
    // Actually record a change in >100 lines
}

What do you say? It gets rid of even more identation and satisfies your request :-)

lthms commented on April 29, 2018

OK. I was wondering how --description should interact with the editor functionality, but I'm still not sure how it should work. If the user provides --description, but not --message, what should happen? Should it essentially, be the same as when --message is provided --no-editor is assumed (with the difference that it can still ask for the message)?

The more I think about it, the more I believe if an editor is set and --description is called, but no message, then this description should be the initial content of PATCH_NAME (the file opened by the editor and read by pijul once the editor is closed).

And, I am in favour of an additional function, I think.

lthms commented on May 12, 2018

I was not aware you have pushed something! I will review that as soon as possible (probably tomorrow) and probably apply it.

pmeunier commented on October 22, 2018

I just fixed this. Mind that if you use bash, --description "fixes #260" is the same as --description "fixes .

pmeunier closed this discussion on October 22, 2018