Issue5 #6

PatrickFerber · 2020-07-16T11:20:20Z

No description provided.

run-order.sh

FlorianPommerening

I saw one problem in run-all-steps.sh, the other comments are minor. Could you do the changes and ping me again. I'd like to wait with testing this until the issues are fixed.

README.md

run-all-steps.sh

run-order.sh

PatrickFerber · 2020-07-17T11:27:03Z

I'd also do the same for run-conversion.sh for consistency.

In the other 2 scripts, we used it multiple times. Here we use it only once. Therefore, I would prefer it to be at the location, we actually use it. But, if the others want this changed, we can do that.

FlorianPommerening · 2020-07-17T11:33:16Z

I'd also do the same for run-conversion.sh for consistency.

In the other 2 scripts, we used it multiple times. Here we use it only once. Therefore, I would prefer it to be at the location, we actually use it. But, if the others want this changed, we can do that.

I think it still makes sense. When reading the scripts, it is easier to understand what is going on if it is the same as in the other scripts. (actually, if there is a lot o shared code, we should think about factoring it out completely, but I don't think this is necessary here).

PatrickFerber · 2020-07-17T11:38:34Z

I'd also do the same for run-conversion.sh for consistency.

In the other 2 scripts, we used it multiple times. Here we use it only once. Therefore, I would prefer it to be at the location, we actually use it. But, if the others want this changed, we can do that.

I think it still makes sense. When reading the scripts, it is easier to understand what is going on if it is the same as in the other scripts. (actually, if there is a lot o shared code, we should think about factoring it out completely, but I don't think this is necessary here).

To make this discussion short: now HG* is also exported in run-conversion.sh

PatrickFerber · 2020-07-22T07:54:37Z

This tests the symptoms that Florian mentioned, not what we actually want to test (that all commits from hg.fast-downward.org are present). For example, there are other branches that we also expect to be there, and just because all branches we expect to be there are there, it doesn't mean that we have the right commits. The error message is also not very helpful about telling people what they can do to address the problem. I would instead test the return value of hg incoming http://hg.fast-downward.org to check if the repository has all commits from that repository. From the help page (and some minimal testing on my side), it looks like you can test this with
if hg incoming http://hg.fast-downward.org; then
    echo 1>&2 "Your repository is missing commits from http://hg.fast-downward.org."
    echo 1>&2 "You must pull from http://hg.fast-downward.org first."
    exit 3
fi```

(BTW, using an exit code of 3 seems unusual to me; "1" would be more customary.)

We decided to add Maltes test. It is now added to the code.

About the exit codes. I used different exit codes for different exit reasons:
1: error with the script arguments
2: error with the setup (mercurial/fast-export) of tools
3: other errors (the incoming changes test is the first of this kind, thus, I do not know a better name). This can be seen as setup and be changed to a '2'.

If the majority decides to have no feedback on the exit codes, we can change them all to 1 otherwise, we should document the meaning of the codes in the README.md

maltehelmert · 2020-07-22T08:00:21Z

For me having and documenting different exit codes apart from the de-factor standard (usage errors: 2, other errors: 1) is over-engineering. It's not like we expect many people to write complex scripts around our scripts that need this distinction. But also changing the exit codes now is probably unnecessary bike-shedding, so perhaps the best thing is to leave things the way they are, but not give details in the README file. For READMEs, focus on the important things is a virtue.

jendrikseipp · 2020-07-22T12:02:35Z

@PatrickFerber I just tried to convert a repository with the run-all-steps method. It said "Cloning official repo" and then for a long time nothing happened. Then it checked for incoming changesets and the script aborted with

Your repository is missing commits from http://hg.fast-downward.org.
You must pull from http://hg.fast-downward.org first.

Can't we do the check for incoming changesets before cloning the repo?

PatrickFerber · 2020-07-22T13:13:44Z

@jendrikseipp:
This is because those two things happen in different steps. The cloning of the repository happens in run-order.sh. There we do not require the user to have pulled in all changes. The abort happens in run-cleanup.sh.
Solutions:

change nothing (the user gets a late error message)
adding the requirement that the repository has pulled all incoming changes to the run-all-steps.sh script (as check prior to any real work)
remove the stripping of new commits in the run-order.sh script (the run-order.sh script makes a new clone of Fast Downward and strips commits aheads of your repository (thus, you will have no issues with changes on the default branch)).

As we have decided for the run-cleanup.sh script that the user repository has to be up to date to our final mercurial repository, 3 is a possible solution. Although I am still a bit hesitant just pulling changes automatically into the user's repository. Therefore, I would prefer solution 2

jendrikseipp · 2020-07-22T20:29:49Z

I don't really grasp what is the status quo and what is the proposed change for option 3. Maybe we can chat about this tomorrow morning.

PatrickFerber · 2020-07-23T10:34:19Z

The script run-order.sh is now merged into run-cleanup.sh. This also solves the problem described by Jendrik, that he would like to received the feedback about pulling earlier.

jendrikseipp · 2020-07-23T11:34:19Z

Awesome, thanks!

silvansievers · 2020-07-24T10:16:56Z

I just tested this and was a bit confused when I read "error", but the script still continued (and the result is a compatible repo):
error: The branch 'merge-sccs' is not fully merged.
If you are sure you want to delete it, run 'git branch -D merge-sccs'.
I think I only understood what is going on because I know that we want to delete branches which have been closed and merged into another branch, but not those that have only been merged but are still open. Maybe let's not call it an "error" since the script continues?

PatrickFerber · 2020-07-24T10:22:01Z

I agree, we should change this to 'warning'.

It also sounds like you think the message would not be explanatory enough for its own. Do you suggest to add more information? e.g.
warning: The branch 'merge-sccs' is not fully merged and will not be automatically deleted.
If you are sure you want to delete it, run 'git branch -D merge-sccs'

silvansievers · 2020-07-24T12:01:17Z

Yes, warning sounds good. The point that is not clear is "fully merged". Let's stick with correct mercurial terminology:

warning: The branch 'merge-sccs' is not closed and hence will not be automatically deleted.
If you are sure you want to delete it, run 'git branch -D merge-sccs' in the converted repository.

PatrickFerber · 2020-07-24T12:58:19Z

With your new message you confused me. We do not automatically delete open branches. We delete branches that are closed and merged. We detected that this can be problematic for branches that are closed and merged into non-main branches.

What kind of merge/unmerged open/closed status does your branch merge-sccs have?

Furthermore, I looked into the script. The current message comes from git directly. We do not have the possibilities to change those. Of course, we could swallow them, but then some other error happens and we swallow that one too, or even report it wrongly. Therefore, I would keep it with the current error produced by git.
We already state in the readme what you shall do in the case of closed branches that are merged into non-main branches, but do not state that those will be reported as errors. We could add a hint for this.

silvansievers · 2020-07-24T13:03:10Z

Please read my comment again. We do automatically delete closed branches, don't we? I didn't want to suggest anything else. Your suggestion was "is not fully merged" and I basically said that I don't know what "fully merged" means and therefore would prefer "not closed", or, if you prefer, "not closed and merged" or "merged but not closed" or "merged but still open". My branch is merged into another one, but I didn't close it. If we can't change the message, then why did you suggest to change it? :)

PatrickFerber · 2020-07-24T13:42:41Z

My warning was speaking about not 'fully merge' branches (I guess fully merged comes from git to speak about branches that are not merged into the main branch).
We do not delete branches that are not merged into the main branch.

You suggested
warning: The branch 'merge-sccs' is not closed and hence will not be automatically deleted.
This sounds like we attempt to delete a branch that is open and fail. We do NOT delete open branches.

We try to delete all branches that are 1. closed and 2. merged. And as far we know this fails with the mentioned error if we the branch is merged into a non-main branch.

-> You said the branch is merged and opened. That we try to delete such a branch is strange and should not happen.
Can you provide the output for the following commands please (if necessary remove issue* branches)
hg branch --template "{branch}
and
hg branch --template "{branch} --closed
. Our script tries to delete set(all_branches) - set(open_branches) - set(unmerged_branches). The set(all_branches) - set(open_branches) should not contain your branch anymore. Therefore, I would like to investigate this.

Why did I suggest changing the message if it is (not easily possible)? I said it sounds like you would like more information in that message and indeed I though about updating it. Not knowing that this is not easily possible. 2 hours later (after your reply) I looked at how we could do this and just then detected that changing the error message is most likely not worth the effort. And therefore, informed you about this development.

silvansievers · 2020-07-24T14:48:29Z

You suggested
warning: The branch 'merge-sccs' is not closed and hence will not be automatically deleted.
This sounds like we attempt to delete a branch that is open and fail. We do NOT delete open branches.

I think this clearly says that we could not delete the branch because it was open. Not that we attempted to delete an open branch. Anyway, if it isn't clear, we should change it.

We try to delete all branches that are 1. closed and 2. merged. And as far we know this fails with the mentioned error if we the branch is merged into a non-main branch.

I think the problem was that it is a branch which is merged into another branch which is not the main branch. Sorry for the confusion. So it is a branch which is open and not merged into main. But still, apparently, the script detected it as something which should normally be deleted. I don't know how you decided to handle such cases.

-> You said the branch is merged and opened. That we try to delete such a branch is strange and should not happen.
Can you provide the output for the following commands please (if necessary remove issue* branches)
hg branch --template "{branch}
and
hg branch --template "{branch} --closed

My mercurial doesn't recognize the option "--template" for hg branch. Do I need a newer version for this?

Why did I suggest changing the message if it is (not easily possible)? I said it sounds like you would like more information in that message and indeed I though about updating it. Not knowing that this is not easily possible. 2 hours later (after your reply) I looked at how we could do this and just then detected that changing the error message is most likely not worth the effort. And therefore, informed you about this development.

Sorry, I must have overlooked this.

PatrickFerber · 2020-07-24T14:56:17Z

We try to delete all branches that are 1. closed and 2. merged. And as far we know this fails with the mentioned error if we the branch is merged into a non-main branch.

I think the problem was that it is a branch which is merged into another branch which is not the main branch. Sorry for the confusion. So it is a branch which is open and not merged into main. But still, apparently, the script detected it as something which should normally be deleted. I don't know how you decided to handle such cases.

If it is open, we do not want to delete it and have to fix the script for that.

-> You said the branch is merged and opened. That we try to delete such a branch is strange and should not happen.
Can you provide the output for the following commands please (if necessary remove issue* branches)
hg branch --template "{branch}
and
hg branch --template "{branch} --closed

My mercurial doesn't recognize the option "--template" for hg branch. Do I need a newer version for this?

My bad, it is not hg branch, but hg branches. Please try
hg branches --template "{branch} "
and
hg branches --template "{branch} " --closed

silvansievers · 2020-07-24T15:03:24Z

We try to delete all branches that are 1. closed and 2. merged. And as far we know this fails with the mentioned error if we the branch is merged into a non-main branch.

I think the problem was that it is a branch which is merged into another branch which is not the main branch. Sorry for the confusion. So it is a branch which is open and not merged into main. But still, apparently, the script detected it as something which should normally be deleted. I don't know how you decided to handle such cases.

If it is open, we do not want to delete it and have to fix the script for that.

Turns out the branch is actually closed - I assumed it was open from the original "error" message, not understanding that the problem the error pointed me to is that the branch is not integrated into the main branch. But anyway, if we can't change the message easily, let's leave it as it is and hope that other users look into the README when they encounter this error.

PatrickFerber · 2020-07-24T15:20:08Z

I update the ReadMe slightly. In the 'Warnings' section we already spoke about the discussed problem. I added now the error message printed by git for the reader to know ah, this warning belongs to this git error message.

silvansievers · 2020-07-24T15:46:01Z

Ok, sounds good.

FlorianPommerening · 2020-07-28T11:57:22Z

README.md

+  The CLEANED_MERCURIAL_REPOSITORY is a location where the intermediate cleaned
+  up Mercurial repository will be written to:
+
+    ./run-cleanup.sh MERCURIAL_REPOSITORY ORDERED_REPOSITORY CLEANED_MERCURIAL_REPOSITORY


I think it is strange to have two intermediate directories as parameters to a single step. It seem to disagree with the decision to merge the two steps into one. What I mean is that passing two directories here makes it seem like two steps that just happen to both be executed by a single script. I'm fine with leaving it like this but it feels odd to me.

I see this as 1 intermediate repository (ORDERED_REPOSITORY). CLEANED_* is the intended output of this script, but I also dislike interface. We do not see another way to solve what we want to do. We need to 1. order the commits and 2. cleanup the repository. We do NOT want to modify the MERCURIAL_REPOSITORY.
To order the commits, we have to clone hg.fast-downward.org anew. This requires a directory and pull the users changes into it. The cleanup (hg convert) cannot be done inplace. Thus, we have to create a second directory (we do not overwrite the users repository).

Executing this script (instead of the run-all-steps.sh) shall give the user the ability to investigate what could have gone wrong. Thus, I do not create a temporary directory and delete at the end of the cleanup.

That all sounds like an argument for having two separate steps for ordering and cleanup. What was the reasoning merging the steps? If the point was that the ordering is something internal, then we can make it more internal by using a temporary directory for it and not bothering the user with this. But if we want to keep the ordered repository after the script runs, it sounds like we should have two steps again.

The ordering is more an internal thing for the cleanup. We observed the bad design, when Jendrik ran the ordering which took some time and then the cleanup failed because its requirement (all changes pulled in) were not satisfied. The user should see this directly. Adding the check that all changes are pulled in to the run-all-steps.sh script would be a hack. Instead we made the ordering internal to the cleanup.

We could make the ordered repository a temporary one. For me this script is used by a user who wants to have full control. Therefore, I kept the ordered repository. The user can investigate into it or just add rm DIR to his command. If the majority wants it to be temporary and automatically removed, we can change this.

I don't know, it still sounds like this should be separate steps to me. If there are conditions that the input (MERCURIAL_REPOSITORY) has to satisfy, they should be check at the start of the first step. If the cloning takes too long to check if there are incoming changes, you could do hg -R MERCURIAL_REPOSITORY incoming hg.fast-downward.org instead . But again, if you already discussed this, I don't want to restart that discussion.

I also disagree with keeping an intermediate result around, and even making it a command-line argument. Like Florian said it breaks the abstraction. If we want to facilitate debugging if things go wrong, for me the logical thing is simply to not go out of our way and delete intermediate results in cases where things go wrong, but they should not be kept around if things go right.

I also agree it makes no sense to first clone and then test and fail if there are incoming commits. The test about incoming commits should be made right at the start if we consider it a reason to abort.

We also agreed on these points in the discussion with Silvan and Jendrik.

For what it's worth, regarding debugging: unless there are network errors or similar basic issues, there is no way in which the initial clone-and-pull steps could go wrong.

If you're OK with it, I'll push a suggested change -- if you don't like it, we can strip it or back it out.

README.md

FlorianPommerening · 2020-07-28T12:07:11Z

run-cleanup.sh

+echo "Cloning official repository"
+hg clone "http://hg.fast-downward.org" "${ORDERED_REPOSITORY}"
+
+if hg -R "${SRC_REPOSITORY}" incoming "${ORDERED_REPOSITORY}"; then


Is this test still necessary? After the pull in line 56 the condition will be satisfied in the new repository, even if it wasn't before. I think I suggested this test but this was when we were still removing parts from the official clone before pulling.

I don't see how removing this test would cause any problems.

I think this test is still necessary. It enforces that all current changes from the last Mercurial master are pulled in by the user prior to executing our script. Any 'multiple head on the same branch' issues (or other issues the pull could cause and we do not know about) can be seen in the user's repository.

It enforces that all current changes from the last Mercurial master are pulled in by the user prior to executing our script.

I don't see why that would be necessary. The next step after this test is a pull and after this pull the condition that we test here is satisfied even if it wasn't before.

If our assumption is that the repository does not have multiple heads on a single branch, we should test that assumption explicitly instead of testing something unrelated that would force the user to do a step that generates a warning (not an error) when the assumption is not satisfied.
(Here is an example for testing this but note the comment about the test in line #19: https://gist.github.com/FernFerret/3178035)

Our problem is, we do not exactly know when fast-export succeeds and when it fails. Malte tested it and said there are cases when multiple heads on the same branch work. We simply do not know for what exactly we have to test (and additionally, what else we do not know what else could fast-export to fail). We do not want to say you are not allowed to convert this repository, although it would convert perfectly fine.

therefore, we kept the check that all changes have to be pulled. If fast-export tells you know "you have multiple heads on branch X" or "fix that". Those issues are in the user's repository and he can fix them there.
If we would pull the newest changes ourselves, then 1. he could not find the problems that caused the error in his repository and 2. could not fix them.
(yes, he could find and fix them by pulling himself, but for me it is more naturally: "I get an error, I look into my repository" and not: "I get an error, let's look through all the temporary files")

I think fast-export doesn't produce an error in this case, only a warning and an incompatible repository, But if you already discussed this, I'm fine with leaving things as they are.

…on detail

FlorianPommerening

I looked over the changes again and it looks good to merge to me.

FlorianPommerening · 2020-07-29T12:39:22Z

run-cleanup.sh

+
+echo "Looking for missing commits"
+
+if hg -R "${SRC_REPOSITORY}" incoming http://hg.fast-downward.org; then


I just got the following output when I accidentally ran cleanup on a repository that was already cleaned:

Looking for missing commits comparing with http://hg.fast-downward.org/ searching for changes abort: repository is unrelated Cloning official repository

It looks like the error (abort: repository is unrelated) still had the exit code of 0 (no commits to pull) and the script continued with cloning the official repository. Should we check for this?

I wouldn't bother. This is not a script meant to be run by many people many times, and we cannot predict all user errors. I think users that make this error are likely to find out eventually what went wrong, so all this would do is potentially save them a bit of time.

Also, we should merge this soon; as far as I understand, the main branch is still on code that does no reordering and is not likely to work.

PatrickFerber added 5 commits July 16, 2020 12:06

[issue5] rename setup files

39b8e31

[issue5] Update setup-* calls

f34c2b4

[issue5] add run-order.sh script

db083c7

[issue5] add run-order.sh to summary script and update README

a8f9256

rename run-cleanup-and-conversion.sh to run-all-steps

5679fa0

PatrickFerber mentioned this pull request Jul 16, 2020

Fix order of commits #5

Closed

FlorianPommerening reviewed Jul 16, 2020

View reviewed changes

run-order.sh Outdated Show resolved Hide resolved

FlorianPommerening requested changes Jul 17, 2020

View reviewed changes

README.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

run-all-steps.sh Show resolved Hide resolved

run-order.sh Outdated Show resolved Hide resolved

PatrickFerber added 4 commits July 17, 2020 11:54

[issue5] add HGPLAIN

e75f58f

[issue5] move -R option of hg to front

bd442cb

[issue5] made indentation consistent

6840573

[issue5] fix run-all-steps.sh usage of wrong directory

eac81b0

FlorianPommerening reviewed Jul 17, 2020

View reviewed changes

run-order.sh Outdated Show resolved Hide resolved

PatrickFerber added 2 commits July 17, 2020 13:15

[issue5] export HGRCPATH= HGPLAIN

3fac1a2

[issue5] add echo

941c445

PatrickFerber added 2 commits July 17, 2020 13:36

[issue5] add troubleshooting to README.md

1272f3a

[issue5] also export HG* in run-conversion.sh

01a28d7

PatrickFerber added 2 commits July 20, 2020 10:31

[issue5] add branch check to cleanup script

e9e40c2

[issue5] update repository check for cleanup

b6c42d5

[issue5] merged run-order.sh into run-cleanup.sh

18084ee

[issue5] update Warning in ReadMe.md

e981f6c

FlorianPommerening requested changes Jul 28, 2020

View reviewed changes

PatrickFerber and others added 2 commits July 28, 2020 15:09

update README.md

9c856a6

[issue5] fail early on incoming changes; make reordering implementati…

16bbca3

…on detail

FlorianPommerening approved these changes Jul 29, 2020

View reviewed changes

FlorianPommerening reviewed Jul 29, 2020

View reviewed changes

PatrickFerber merged commit 8f4a459 into master Aug 6, 2020

PatrickFerber deleted the issue5 branch August 6, 2020 11:19


		echo "Looking for missing commits"

		if hg -R "${SRC_REPOSITORY}" incoming http://hg.fast-downward.org; then

Issue5 #6

Issue5 #6

Conversation

PatrickFerber commented Jul 16, 2020

FlorianPommerening left a comment

Choose a reason for hiding this comment

PatrickFerber commented Jul 17, 2020

FlorianPommerening commented Jul 17, 2020

PatrickFerber commented Jul 17, 2020 • edited

PatrickFerber commented Jul 22, 2020

maltehelmert commented Jul 22, 2020

jendrikseipp commented Jul 22, 2020 • edited

PatrickFerber commented Jul 22, 2020 • edited

jendrikseipp commented Jul 22, 2020

PatrickFerber commented Jul 23, 2020

jendrikseipp commented Jul 23, 2020

silvansievers commented Jul 24, 2020

PatrickFerber commented Jul 24, 2020

silvansievers commented Jul 24, 2020

PatrickFerber commented Jul 24, 2020

silvansievers commented Jul 24, 2020

PatrickFerber commented Jul 24, 2020

silvansievers commented Jul 24, 2020 • edited

PatrickFerber commented Jul 24, 2020

silvansievers commented Jul 24, 2020

PatrickFerber commented Jul 24, 2020

silvansievers commented Jul 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FlorianPommerening left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PatrickFerber commented Jul 17, 2020 •

edited

jendrikseipp commented Jul 22, 2020 •

edited

PatrickFerber commented Jul 22, 2020 •

edited

silvansievers commented Jul 24, 2020 •

edited