From 0bf05715f60dd8addd457274d9ed489234d598a4 Mon Sep 17 00:00:00 2001 From: Seb M'Caw Date: Fri, 22 Nov 2024 11:27:51 +0000 Subject: [PATCH] Fix updating pins on crates which track Alire-generated files (#1788) * Fix updating pins on crates which track Alire-generated files * Require user confirmation before discarding changes * Make confirmation prompt depend on location of dirty files --- src/alire/alire-user_pins.adb | 81 +++++++++++- src/alire/alire-vcss-git.adb | 85 ++++++++++--- src/alire/alire-vcss-git.ads | 21 ++++ testsuite/drivers/helpers.py | 5 +- .../tests/pin/branch-update-dirty/test.py | 118 ++++++++++++++++++ .../tests/pin/branch-update-dirty/test.yaml | 3 + .../tests/publish/private-indexes/test.py | 7 +- 7 files changed, 292 insertions(+), 28 deletions(-) create mode 100644 testsuite/tests/pin/branch-update-dirty/test.py create mode 100644 testsuite/tests/pin/branch-update-dirty/test.yaml diff --git a/src/alire/alire-user_pins.adb b/src/alire/alire-user_pins.adb index 8aedcc70..fb3992d2 100644 --- a/src/alire/alire-user_pins.adb +++ b/src/alire/alire-user_pins.adb @@ -9,6 +9,8 @@ with Alire.VFS; with AAA.Strings; +with CLIC.User_Input; + with GNAT.OS_Lib; package body Alire.User_Pins is @@ -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 @@ -223,11 +225,78 @@ 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 then + -- 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. + declare + Paths : constant AAA.Strings.Set := + VCSs.Git.Handler.Dirty_Files + (Destination, Include_Untracked => True); + + List_Sep : constant String := New_Line & " "; + Paths_List : constant String := + List_Sep & Paths.To_Vector.Flatten (List_Sep); + + Alire_Generated_Dirs_Only : constant Boolean := + (for all Path of Paths => + AAA.Strings.Has_Prefix (Path, "alire/") + or else AAA.Strings.Has_Prefix (Path, "config/")); + -- 'git status' yields '/' separated paths, even on Windows. + + Question : constant String := + "Updating the pin '" + & Crate.As_String + & "' will discard local uncommitted changes in '" + & Destination + & "' to the following:" + & Paths_List + & New_Line + & (if Alire_Generated_Dirs_Only + then + "These changes affect only Alire's automatically " + & "generated files, which are safe to overwrite." + & New_Line + & "Do you want to proceed?" + else + "These changes include files which were not " + & "automatically generated by Alire." + & New_Line + & "Are you sure you want to proceed?"); + Default : constant Answer_Kind := + (if Force or else Alire_Generated_Dirs_Only then Yes + else No); + begin + if Paths.Length not in 0 + and then Query + (Question => Question, + Valid => (Yes | No => True, others => False), + Default => Default) + = Yes + then + VCSs.Git.Discard_Uncommitted + (Repo => Destination, Discard_Untracked => True).Assert; + Result := VCSs.Git.Handler.Update (Destination, Branch); + end if; + end; + 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 diff --git a/src/alire/alire-vcss-git.adb b/src/alire/alire-vcss-git.adb index 0ef4948e..c2fd159b 100644 --- a/src/alire/alire-vcss-git.adb +++ b/src/alire/alire-vcss-git.adb @@ -292,6 +292,26 @@ package body Alire.VCSs.Git is return Alire.Errors.Get (E); end Commit_All; + ------------------------- + -- Discard_Uncommitted -- + ------------------------- + + function Discard_Uncommitted (Repo : Directory_Path; + Discard_Untracked : Boolean := False) + return Outcome + is + Guard : Directories.Guard (Directories.Enter (Repo)) with Unreferenced; + begin + Run_Git (Empty_Vector & "reset" & "-q" & "--hard" & "HEAD"); + if Discard_Untracked then + Run_Git (Empty_Vector & "clean" & "-fd"); + end if; + return Outcome_Success; + exception + when E : others => + return Alire.Errors.Get (E); + end Discard_Uncommitted; + ---------- -- Push -- ---------- @@ -611,41 +631,72 @@ package body Alire.VCSs.Git is end; end Remote_Commit; - ------------ - -- Status -- - ------------ + ----------------- + -- Dirty_Files -- + ----------------- - function Status (This : VCS; - Repo : Directory_Path) - return States + function Dirty_Files (This : VCS; + Repo : Directory_Path; + Include_Untracked : Boolean := False) + return AAA.Strings.Set is Guard : Directories.Guard (Directories.Enter (Repo)) with Unreferenced; Output : constant AAA.Strings.Vector := - Run_Git_And_Capture (Empty_Vector & "status" & "--porcelain"); + Run_Git_And_Capture (Empty_Vector & "status" & "-z" & "--no-renames"); + + Lines : constant AAA.Strings.Vector := + (if Output.Length in 0 then Empty_Vector + else Split (Output.First_Element, Character'Val (0))); + -- 'git status -z' splits "lines" on the null character, not newline. - Untracked_File : Natural := 0; - Tracked_File : Natural := 0; + Paths : AAA.Strings.Set := Empty_Set; begin + if Output.Length not in 0 | 1 then + -- 'git status -z' splits on null character, so should only yield one + -- line of output. + Raise_Checked_Error + ("Unexpected output from 'git status -z': " + & Output.Flatten (New_Line & " ")); + end if; - for Line of Output loop + for Line of Lines loop if Contains (Line, "GNAT-TEMP-") then -- Turns out the temporary file we use to capture the output of -- "git status" makes git to return a dirty tree. We filter these -- out then. null; - elsif Has_Prefix (Line, "??") then - Untracked_File := Untracked_File + 1; + elsif Line = "" then + -- The output of 'git status -z' includes a trailing null byte, + -- so the last element of Lines is an irrelevant empty string. + null; + elsif not Include_Untracked and then Has_Prefix (Line, "??") then + -- Ignore untracked files if Include_Untracked is False. + null; else - Tracked_File := Tracked_File + 1; + -- 'git status -z' yields lines of the form "XY ", where + -- "X" and "Y" are single-character status codes. + -- We therefore strip the first three characters of each line to + -- get the path. + Paths.Include (Line (Line'First + 3 .. Line'Last)); end if; end loop; + return Paths; + end Dirty_Files; - if Tracked_File /= 0 then - -- There are added/modified tracked files - return Dirty; - else + ------------ + -- Status -- + ------------ + + function Status (This : VCS; + Repo : Directory_Path) + return States + is + begin + if Dirty_Files (This, Repo, Include_Untracked => False).Length in 0 then return Clean; + else + return Dirty; end if; end Status; diff --git a/src/alire/alire-vcss-git.ads b/src/alire/alire-vcss-git.ads index 36850f5e..3a0ef639 100644 --- a/src/alire/alire-vcss-git.ads +++ b/src/alire/alire-vcss-git.ads @@ -104,6 +104,24 @@ 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. + not overriding + function Dirty_Files (This : VCS; + Repo : Directory_Path; + Include_Untracked : Boolean := False) + return AAA.Strings.Set; + -- Return the paths of any files with uncommitted changes. + -- + -- Ignored files are not included. Untracked files are not included unless + -- Include_Untracked is True. + + function Discard_Uncommitted (Repo : Directory_Path; + Discard_Untracked : Boolean := False) + return Outcome; + -- Reset all uncommitted changes to tracked files, and optionally also + -- untracked files. + -- + -- Ignored files are not discarded. + function Push (Repo : Directory_Path; Remote : String; Force : Boolean := False; @@ -184,6 +202,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. diff --git a/testsuite/drivers/helpers.py b/testsuite/drivers/helpers.py index 6585bc9c..b4679d6b 100644 --- a/testsuite/drivers/helpers.py +++ b/testsuite/drivers/helpers.py @@ -198,8 +198,9 @@ def init_git_repo(path): assert run(["git", "checkout", "-b", "master"]).returncode == 0 git_init_user() - # Workaround for Windows, where somehow we get undeletable files in temps: - with open(".gitignore", "wt") as file: + # Workaround for Windows, where somehow we get undeletable files in temps + # (we use mode 'a' because a '.gitignore' may already be present): + with open(".gitignore", "at") as file: file.write("*.tmp\n") head = commit_all(".") diff --git a/testsuite/tests/pin/branch-update-dirty/test.py b/testsuite/tests/pin/branch-update-dirty/test.py new file mode 100644 index 00000000..407ad456 --- /dev/null +++ b/testsuite/tests/pin/branch-update-dirty/test.py @@ -0,0 +1,118 @@ +""" +Check updating a branch pin when 'config/*' files are tracked or users make +manual changes to the cached clone. +""" + + +import os +import re +import shutil +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_commit_file, init_git_repo, replace_in_file + + +def run(args): + p = subprocess.run(args, capture_output=True) + p.check_returncode() + return p + + +# Create crate yyy, with git tracking the 'config/*' files +init_local_crate("yyy") +yyy_path = os.getcwd() +replace_in_file(".gitignore", "/config/\n", "") +init_git_repo(".") +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}") +run_alr("build") + +# Verify that the cached copy of yyy has a dirty repo (due to Alire's changes to +# the 'config/*' files during the build) +cached_yyy_path = os.path.join(xxx_path, "alire", "cache", "pins", "yyy") +gpr_rel_path = os.path.join("config", "yyy_config.gpr") +gpr_rel_path_pattern = r"config/yyy_config\.gpr" # with '/', as returned by Git +os.chdir(cached_yyy_path) +p = run(["git", "status", "--porcelain"]) +assert_match(rf".* M {gpr_rel_path_pattern}", 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", gpr_rel_path, "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( + ( + ".*Updating the pin 'yyy' will discard local uncommitted changes in " + f"'{re.escape(cached_yyy_path)}' to the following:" + rf".*[\r\n]+ {gpr_rel_path_pattern}" + r".*[\r\n]+These changes affect only Alire's automatically generated " + r"files, which are safe to overwrite\.[\r\n]+Do you want to proceed\?" + ), + 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, which is not dirty). +os.chdir(cached_yyy_path) +run(["git", "reset", "--hard", "HEAD~"]) +shutil.rmtree("config") # So 'alr build' auto-generates the same files as above. +os.chdir(xxx_path) +run_alr("build") +os.chdir(cached_yyy_path) +p = run(["git", "status", "--porcelain"]) +assert_match(rf".* M {gpr_rel_path_pattern}", p.stdout.decode()) +os.chdir(xxx_path) +p = run_alr("update") +assert_not_substring("will discard local uncommitted changes", p.out) +assert_in_file(os.path.join(cached_yyy_path, "test_file"), "This is a new file") + +# Reset by one commit and repeat the process again, but write to 'test_file' so +# that it is dirty and there is a conflict. Since this time the change is not in +# one of Alire's auto-generated subdirectories, the prompt should be different. +# +# This also checks that conflicts involving untracked files are treated the same +# as those with tracked files. +os.chdir(cached_yyy_path) +run(["git", "reset", "--hard", "HEAD~"]) +shutil.rmtree("config") +os.chdir(xxx_path) +run_alr("build") +os.chdir(cached_yyy_path) +with open("test_file", "w") as f: + f.write("This file is now dirty.\n") +p = run(["git", "status", "--porcelain"]) +assert_match(rf".* M {gpr_rel_path_pattern}", p.stdout.decode()) +assert_match(r".*\?\? test_file", p.stdout.decode()) +os.chdir(xxx_path) +prompt = ( + ".*Updating the pin 'yyy' will discard local uncommitted changes in " + f"'{re.escape(cached_yyy_path)}' to the following:" + rf".*[\r\n]+ {gpr_rel_path_pattern}.*[\r\n]+ test_file" + r".*[\r\n]+These changes include files which were not automatically " + r"generated by Alire\.[\r\n]+Are you sure you want to proceed\?[\r\n]+" +) +# The default response should be 'No', so 'alr -n update' will fail. +p = run_alr("update", complain_on_error=False) +assert_match(prompt + "Using default: No", p.out) +# Supplying '--force' should change the default to 'Yes'. +p = run_alr("-f", "update") +assert_match(prompt + "Using default: Yes", p.out) +assert_in_file(os.path.join(cached_yyy_path, "test_file"), "This is a new file") + + +print('SUCCESS') diff --git a/testsuite/tests/pin/branch-update-dirty/test.yaml b/testsuite/tests/pin/branch-update-dirty/test.yaml new file mode 100644 index 00000000..fa855459 --- /dev/null +++ b/testsuite/tests/pin/branch-update-dirty/test.yaml @@ -0,0 +1,3 @@ +driver: python-script +indexes: + compiler_only_index: {} diff --git a/testsuite/tests/publish/private-indexes/test.py b/testsuite/tests/publish/private-indexes/test.py index 0079076a..05788774 100644 --- a/testsuite/tests/publish/private-indexes/test.py +++ b/testsuite/tests/publish/private-indexes/test.py @@ -66,9 +66,7 @@ def test( os.chdir("remote") run_alr("init", "--bin", "xxx") os.chdir("xxx") - # Adjust the values of maintainers-logins and user.github_login if required - if github_user is not None: - run(["alr", "settings", "--set", "user.github_login", github_user]) + # Adjust the value of maintainers-logins if required if maint_logins is not None: with open("alire.toml", "a") as f: f.write(f"maintainers-logins = {maint_logins}\n") @@ -85,6 +83,9 @@ def test( os.makedirs(local_path) os.chdir(local_path) run(["git", "clone", url, local_path]) + # Adjust the value of user.github_login if required + if github_user is not None: + run_alr("settings", "--set", "user.github_login", github_user) # Run alr p = run_alr_interactive( -- 2.39.5