Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix updating pins on crates which track Alire-generated files #1788

Merged
merged 4 commits into from
Nov 22, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 41 additions & 6 deletions src/alire/alire-user_pins.adb
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ with Alire.VFS;

with AAA.Strings;

with CLIC.User_Input;

with GNAT.OS_Lib;

package body Alire.User_Pins is
Expand Down Expand Up @@ -199,7 +201,7 @@ package body Alire.User_Pins is
Trace.Detail ("Checking out pin " & Utils.TTY.Name (Crate) & " at "
& TTY.URL (Destination));

-- If the fetch URL has been changed, fall back to checkout
-- If the fetch URL has been changed, do a fresh 'git clone'.
--
-- Note that VCSs.Git.Clone converts the URL to a git-friendly form
-- with VCSs.Repo, so this is what the output of 'git config' should
Expand All @@ -223,11 +225,44 @@ package body Alire.User_Pins is
Put_Info ("Pulling " & Utils.TTY.Name (Crate)
& " branch " & TTY.URL (Branch) & "...");

if not VCSs.Git.Handler.Update (Destination, Branch).Success then
Raise_Checked_Error
("Update of repository at " & TTY.URL (Destination)
& " failed, re-run with -vv -d for details");
end if;
declare
use CLIC.User_Input;
Result : Outcome := VCSs.Git.Handler.Update (Destination, Branch);
begin
if not Result.Success
and then VCSs.Git.Handler.Status (Destination) in VCSs.Git.Dirty
and then Query
(Question =>
"Do you want to discard local uncommitted changes in '"
& Destination
& "'?"
& New_Line
& "These changes were probably generated by Alire, so "
& "select 'Yes' unless you made them yourself.",
Valid => (Yes | No => True, others => False),
Default => Yes)
= Yes
then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't feel comfortable with telling the user it is probably OK to go ahead. What about:

  1. Print something like "The update will overwrite these changed files in pinned crate blah:" and the output of git status.

  2. If changes are confined to alire/ or config/, say something like "changes affect Alire autogenerated files that are safe to overwrite" and default to Yes.

  3. Else, say something like "changes affect user files, are you sure?" and default to No.

If detecting 2) is too complex, just say that changes within alire and config are OK to ignore and to make sure there are no other changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My concern with going down this route was that the file location is only a rough heuristic for the source of the changes; changes made by actions like e.g. Alire's version_patcher will be categorised as "user files", while conversely users might make manual changes to the files in config/ which would then be confidently asserted as "safe to overwrite".

In particular, defaulting to No means that alr -n update will fail when actions change "user files", which may require workarounds in non-interactive use cases like CI pipelines.

I'm happy to implement as requested, but just checking you've considered these drawbacks.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually what we (I) have done in the past is always default to Yes when --force is applied, so we could do the same here.

Editions inside config are normally overwritten (without warning in the normal use case!) anyway, so the only problem I can see here is having a disabled configuration but still using config for manual files, or extra files in there, which certainly could happen, as misguided as it sounds (to me)... We operate under the assumption that config/* is fair game, and we could narrow it even more to the actual files Alire is generating only. Still, it seems to me a corner case not meriting complicating life for the regular case, and the warning when interactive will happen anyway.

For actions touching user files under version control, I normally would want to review those, no matter that I didn't do the edition manually (e.g., something like version_patcher), although I haven't encountered this situation in actual development (changes by version_patcher don't go into VC), so again to me the warning as suggested is legit.

As for CI uses, I expect that's not the regular situation in which a user is editing several crates via pins for which changes are to be preserved. Again, I have not seen this happening (pins suggest a temporary situation anyway).

In the end, this has never happened to me, so if you encountered the situation, you may have a better grasp of the actual user expectations. We can also ping @Fabien-Chouteau for a third opinion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, that all sounds good. The only occasion I've encountered this is the linked issue, so the possibility of users making their own changes is entirely speculative.

I'll make the default Yes if --forceed, and otherwise implement as suggested above.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sound like a good compromise to me 👍

-- One reason why the update may fail is if there are
-- uncommitted changes in the local clone which would conflict
-- with the update, so we discard such changes and try again.
--
-- This generally happens if the crate's repo tracks
-- Alire-generated files (e.g. those in 'config/'). However, it
-- is conceivable the user might have made changes to the
-- checkout in the 'alire/cache/pins' directory themselves, so
-- we require user confirmation before discarding anything.

VCSs.Git.Discard_Uncommitted (Repo => Destination).Assert;
Result := VCSs.Git.Handler.Update (Destination, Branch);
end if;

if not Result.Success then
Raise_Checked_Error
("Update of repository at " & TTY.URL (Destination)
& " failed, re-run with -vv -d for details");
end if;
end;
end Update;

begin
Expand Down
15 changes: 15 additions & 0 deletions src/alire/alire-vcss-git.adb
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,21 @@ package body Alire.VCSs.Git is
return Alire.Errors.Get (E);
end Commit_All;

-------------------------
-- Discard_Uncommitted --
-------------------------

function Discard_Uncommitted (Repo : Directory_Path) return Outcome
is
Guard : Directories.Guard (Directories.Enter (Repo)) with Unreferenced;
begin
Run_Git (Empty_Vector & "reset" & "-q" & "--hard" & "HEAD");
return Outcome_Success;
exception
when E : others =>
return Alire.Errors.Get (E);
end Discard_Uncommitted;

----------
-- Push --
----------
Expand Down
6 changes: 6 additions & 0 deletions src/alire/alire-vcss-git.ads
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,9 @@ package Alire.VCSs.Git is
-- Add and commit all changes in a given repo; commiter will be set to the
-- user email stored in our config.

function Discard_Uncommitted (Repo : Directory_Path) return Outcome;
-- Reset all uncommitted changes to tracked files

function Push (Repo : Directory_Path;
Remote : String;
Force : Boolean := False;
Expand Down Expand Up @@ -184,6 +187,9 @@ package Alire.VCSs.Git is
return Outcome;
-- Update and track Branch, if given.
--
-- Does not discard uncommitted changes, so will fail if there are local
-- changes which conflict with the update.
--
-- Raises Checked_Error when the repo has multiple remotes configured and
-- Branch is not the same as the current HEAD.

Expand Down
82 changes: 82 additions & 0 deletions testsuite/tests/pin/branch-update-dirty/test.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
"""
Check updating a branch pin when 'config/*' files are tracked.
"""


import os
import re
import subprocess

from drivers.alr import run_alr, alr_pin, init_local_crate
from drivers.asserts import assert_match, assert_in_file, assert_not_substring
from drivers.helpers import git_branch, git_commit_file, init_git_repo


# Create crate yyy, with git tracking the 'config/*' files
init_local_crate("yyy")
yyy_path = os.getcwd()
with open(".gitignore") as f:
gitignore_content = f.read()
gitignore_content = gitignore_content.replace("/config/\n", "")
with open(".gitignore", "w") as f:
f.write(gitignore_content)
init_git_repo(".")
default_branch = git_branch()
os.chdir("..")

# Create and build another crate, with yyy's default branch added as a pin
init_local_crate()
xxx_path = os.getcwd()
alr_pin("yyy", url=f"git+file:{yyy_path}", branch=default_branch)
run_alr("build")

# Verify that the cached copy of yyy has a dirty repo (due to changes to the
# 'config/*' files)
cached_yyy_path = os.path.join(xxx_path, "alire", "cache", "pins", "yyy")
os.chdir(cached_yyy_path)
p = subprocess.run(["git", "status"], capture_output=True)
p.check_returncode()
assert_match(r".*modified:\s*config/yyy_config\.gpr", p.stdout.decode())

# Add commits to yyy's default branch, including a change to a file in 'config/'
os.chdir(yyy_path)
git_commit_file(
"Change_config", "config/yyy_config.gpr", "This is a new addition\n", "a"
)
git_commit_file("Add_test_file", "test_file", "This is a new file\n")

# Check that the dirty repo doesn't prevent updating the pin (subject to user
# confirmation)
os.chdir(xxx_path)
p = run_alr("update")
assert_match(
(
".*Do you want to discard local uncommitted changes in '"
rf"{re.escape(cached_yyy_path)}'\?"
".*These changes were probably generated by Alire, so select 'Yes' "
r"unless you made them yourself\."
),
p.out
)

# Check that the update was successful
assert_in_file(os.path.join(cached_yyy_path, "test_file"), "This is a new file")

# Reset the cached clone by only one commit and repeat the above. This time the
# update should work without user confirmation, as there will be no conflict
# (only 'test_file' needs to be updated).
os.chdir(cached_yyy_path)
p = subprocess.run(["git", "reset", "--hard", "HEAD~"]).check_returncode()
os.chdir(xxx_path)
run_alr("build")
os.chdir(cached_yyy_path)
p = subprocess.run(["git", "status"], capture_output=True)
p.check_returncode()
assert_match(r".*modified:\s*config/yyy_config\.gpr", p.stdout.decode())
os.chdir(xxx_path)
p = run_alr("update")
assert_not_substring("Do you want to discard local uncommitted changes", p.out)
assert_in_file(os.path.join(cached_yyy_path, "test_file"), "This is a new file")


print('SUCCESS')
3 changes: 3 additions & 0 deletions testsuite/tests/pin/branch-update-dirty/test.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
driver: python-script
indexes:
compiler_only_index: {}
Loading