Issue5 #6
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,49 +2,71 @@ | |
|
|
||
| set -euo pipefail | ||
|
|
||
| if [[ $# -ne 3 ]]; then | ||
| echo "Invalid arguments. Use: $0 SRC TMP DST" | ||
| exit 1 | ||
| fi | ||
|
|
||
| SRC_REPOSITORY="$1" | ||
| ORDERED_REPOSITORY="$2" | ||
| CLEANED_REPOSITORY="$3" | ||
| shift 3 | ||
|
|
||
| if [[ ! -d "${SRC_REPOSITORY}" ]]; then | ||
| echo "Invalid argument. ${SRC_REPOSITORY} has to be a directory." | ||
| exit 1 | ||
| fi | ||
|
|
||
| if [[ -e "${ORDERED_REPOSITORY}" ]]; then | ||
| echo "Invalid argument. ${ORDERED_REPOSITORY} may not exist." | ||
| exit 1 | ||
| fi | ||
|
|
||
| if [[ -e "${CLEANED_REPOSITORY}" ]]; then | ||
| echo "Invalid argument. ${CLEANED_REPOSITORY} may not exist." | ||
| exit 1 | ||
| fi | ||
|
|
||
|
|
||
| BASE="$(dirname "$(readlink -f "$0")")" | ||
| SETUP_MERCURIAL="${BASE}/setup-mercurial.sh" | ||
| VIRTUALENV="${BASE}/data/py3-env" | ||
|
|
||
| if ! /bin/bash "${SETUP_MERCURIAL}"; then | ||
| echo "Error during setup." | ||
| exit 2 | ||
| fi | ||
| source "${VIRTUALENV}/bin/activate" | ||
| # Disable all extensions. | ||
| # (https://stackoverflow.com/questions/46612210/mercurial-disable-all-the-extensions-from-the-command-line) | ||
| export HGRCPATH= | ||
| export HGPLAIN= | ||
|
|
||
|
|
||
| echo "Cloning official repository" | ||
| hg clone "http://hg.fast-downward.org" "${ORDERED_REPOSITORY}" | ||
|
|
||
| if hg -R "${SRC_REPOSITORY}" incoming "${ORDERED_REPOSITORY}"; then | ||
|
There was a problem hiding this comment. 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. There was a problem hiding this comment. 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. There was a problem hiding this comment.
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. There was a problem hiding this comment. 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. There was a problem hiding this comment. 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. |
||
| 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 | ||
|
|
||
| echo "Enforce commit order" | ||
| hg -R "${ORDERED_REPOSITORY}" pull "${SRC_REPOSITORY}" | ||
|
|
||
| echo "Clean up repository" | ||
| hg \ | ||
| --config extensions.renaming_mercurial_source="${BASE}/renaming_mercurial_source.py" \ | ||
| --config extensions.hgext.convert= \ | ||
| --config format.sparse-revlog=0 \ | ||
| convert "${ORDERED_REPOSITORY}" "${CLEANED_REPOSITORY}" \ | ||
| --source-type renaming_mercurial_source \ | ||
| --authormap "${BASE}/data/downward_authormap.txt" \ | ||
| --filemap "${BASE}/data/downward_filemap.txt" \ | ||
| --splicemap "${BASE}/data/downward_splicemap.txt" \ | ||
| --branchmap "${BASE}/data/downward_branchmap.txt" | ||
|
|
||
| cd "${CLEANED_REPOSITORY}" | ||
| hg --config extensions.strip= strip "branch(issue323)" --nobackup | ||
| hg --config extensions.strip= strip "branch(ipc-2011-fixes)" --nobackup | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 DIRto his command. If the majority wants it to be temporary and automatically removed, we can change this.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 dohg -R MERCURIAL_REPOSITORY incoming hg.fast-downward.orginstead . But again, if you already discussed this, I don't want to restart that discussion.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.