History

Merge pull request #2588 from GitoxideLabs/fix/gix-status-phantom-submodule-modifications-2585

Follow submodule gitdir files when opening

Sebastian Thiel committed 6 months ago

Commit db925ec2 parents ba7d9a4 5a37fbd6 files · +278 −14Browse files

Unified · Split

6 files
  1. gix/src/submodule/errors.rs +7 −0
  2. gix/src/submodule/mod.rs +50 −13
  3. gix/tests/fixtures/generated-archives/make_submodules.tar +0 −0
  4. gix/tests/fixtures/make_submodules.sh +29 −1
  5. gix/tests/gix/submodule.rs +189 −0
  6. gix/tests/submodule_open.rs +4 −1
gix/src/submodule/errors.rs+7−0
@@ -91,6 +91,13 @@ pub mod git_dir_try_old_form { · 90 unmodified lines
91 pub enum Error {91 pub enum Error {
92 #[error(transparent)]92 #[error(transparent)]
93 PathConfiguration(#[from] gix_submodule::config::path::Error),93 PathConfiguration(#[from] gix_submodule::config::path::Error),
94 #[error("The gitdir file at '{}' contains an invalid gitdir target: {:?}", .gitdir_file.display(), .target)]
95 InvalidGitDirFileTarget {
96 gitdir_file: std::path::PathBuf,
97 target: Option<std::path::PathBuf>,
98 #[source]
99 source: Option<gix_discover::path::from_gitdir_file::Error>,
100 },
94 #[error(transparent)]101 #[error(transparent)]
95 GitDir(#[from] gix_validate::submodule::name::Error),102 GitDir(#[from] gix_validate::submodule::name::Error),
96 }103 }
gix/src/submodule/mod.rs+50−13
@@ -232,25 +232,56 @@ impl Submodule<'_> { · 231 unmodified lines
232 /// the superproject's worktree where it actually *is* located if the submodule in the 'old-form', thus is a directory232 /// the superproject's worktree where it actually *is* located if the submodule in the 'old-form', thus is a directory
233 /// inside of the superproject's work-tree.233 /// inside of the superproject's work-tree.
234 ///234 ///
235 /// Note that 'old-form' paths returned aren't verified, i.e. the `.git` repository might be corrupt or otherwise235 /// Note that paths returned aren't fully verified, i.e. the `.git` repository might be corrupt or otherwise
236 /// invalid - it's left to the caller to try to open it.236 /// invalid - it's left to the caller to try to open it. `.git` files are parsed when present and their target
237 /// is required to be a directory though, as a malformed gitdir file means a broken submodule checkout.
237 ///238 ///
238 /// Also note that the returned path may not actually exist.239 /// Also note that the fallback path returned for uninitialized submodules may not actually exist.
239 pub fn git_dir_try_old_form(&self) -> Result<PathBuf, git_dir_try_old_form::Error> {240 pub fn git_dir_try_old_form(&self) -> Result<PathBuf, git_dir_try_old_form::Error> {
241 self.git_dir_try_old_form_inner(true)
242 }
243
244 /// Return the best-known submodule repository path, optionally parsing a `.git` file target
245 /// and requiring it to be a directory if `validate_gitdir_file_target` is `true`.
246 ///
247 /// Validation may be skipped when callers only need coarse state, like `status_opts()` with
248 /// `Ignore::All`, which returns before opening or inspecting the submodule repository.
249 fn git_dir_try_old_form_inner(
250 &self,
251 validate_gitdir_file_target: bool,
252 ) -> Result<PathBuf, git_dir_try_old_form::Error> {
253 let git_dir = self.git_dir()?;
240 let worktree_gitdir = self.worktree_gitdir()?;254 let worktree_gitdir = self.worktree_gitdir()?;
241 let worktree_gitdir_or_modules_gitdir = if worktree_gitdir.is_dir() {255 let git_dir = if worktree_gitdir.is_dir() {
242 worktree_gitdir256 worktree_gitdir
257 } else if worktree_gitdir.is_file() {
258 if validate_gitdir_file_target {
259 let git_dir = gix_discover::path::from_gitdir_file(&worktree_gitdir).map_err(|source| {
260 git_dir_try_old_form::Error::InvalidGitDirFileTarget {
261 gitdir_file: worktree_gitdir.clone(),
262 target: None,
263 source: Some(source),
264 }
265 })?;
266 if !git_dir.is_dir() {
267 return Err(git_dir_try_old_form::Error::InvalidGitDirFileTarget {
268 gitdir_file: worktree_gitdir,
269 target: Some(git_dir),
270 source: None,
271 });
272 }
273 git_dir
274 } else {
275 gix_discover::path::from_gitdir_file(&worktree_gitdir).unwrap_or(git_dir)
276 }
243 } else {277 } else {
244 self.git_dir()?278 git_dir
245 };279 };
246 Ok(worktree_gitdir_or_modules_gitdir)280 Ok(git_dir)
247 }281 }
248 282
249 /// Query various parts of the submodule and assemble it into state information.283 fn state_inner(&self, validate_gitdir_file_target: bool) -> Result<State, state::Error> {
250 #[doc(alias = "status", alias = "git2")]284 let maybe_old_path = self.git_dir_try_old_form_inner(validate_gitdir_file_target)?;
251 pub fn state(&self) -> Result<State, state::Error> {
252 let maybe_old_path = self.git_dir_try_old_form()?;
253 let git_dir = self.git_dir()?;
254 let worktree_git = self.worktree_gitdir()?;285 let worktree_git = self.worktree_gitdir()?;
255 let superproject_configuration = self286 let superproject_configuration = self
256 .state287 .state
@@ -263,12 +294,18 @@ impl Submodule<'_> { · 6 unmodified lines
263 .any(|section| section.header().subsection_name() == Some(self.name.as_ref()));294 .any(|section| section.header().subsection_name() == Some(self.name.as_ref()));
264 Ok(State {295 Ok(State {
265 repository_exists: maybe_old_path.is_dir(),296 repository_exists: maybe_old_path.is_dir(),
266 is_old_form: maybe_old_path != git_dir,297 is_old_form: worktree_git.is_dir(),
267 worktree_checkout: worktree_git.exists(),298 worktree_checkout: worktree_git.exists(),
268 superproject_configuration,299 superproject_configuration,
269 })300 })
270 }301 }
271 302
303 /// Query various parts of the submodule and assemble it into state information.
304 #[doc(alias = "status", alias = "git2")]
305 pub fn state(&self) -> Result<State, state::Error> {
306 self.state_inner(true)
307 }
308
272 /// Open the submodule as repository, or `None` if the submodule wasn't initialized yet.309 /// Open the submodule as repository, or `None` if the submodule wasn't initialized yet.
273 ///310 ///
274 /// More states can be derived here:311 /// More states can be derived here:
@@ -375,7 +412,7 @@ pub mod status { · 100 unmodified lines
375 )412 )
376 -> crate::status::Platform<'a, gix_features::progress::Discard>,413 -> crate::status::Platform<'a, gix_features::progress::Discard>,
377 ) -> Result<Status, Error> {414 ) -> Result<Status, Error> {
378 let mut state = self.state()?;415 let mut state = self.state_inner(ignore != config::Ignore::All)?;
379 if ignore == config::Ignore::All {416 if ignore == config::Ignore::All {
380 return Ok(Status {417 return Ok(Status {
381 state,418 state,
gix/tests/fixtures/generated-archives/make_submodules.tar+0−0

Binary file not shown.

gix/tests/fixtures/make_submodules.sh+29−1
@@ -120,6 +120,34 @@ git init with-submodules · 119 unmodified lines
120 git submodule add ../module1 dir/m1120 git submodule add ../module1 dir/m1
121)121)
122 122
123git init submodule-with-divergent-gitlink
124(cd submodule-with-divergent-gitlink
125 git submodule add ../module1 outer/inner
126 git commit -m "add nested submodule"
127
128 mv .git/modules/outer/inner .git/modules/inner
129 rmdir .git/modules/outer
130 git config --file .git/modules/inner/config core.worktree ../../../outer/inner
131 printf 'gitdir: ../../.git/modules/inner\n' >outer/inner/.git
132
133 git clone --bare ../module1 .git/modules/outer/inner
134 git -C .git/modules/outer/inner update-ref HEAD "$(git -C ../module1 rev-parse @~1)"
135)
136
137git init submodule-with-missing-gitlink-target
138(cd submodule-with-missing-gitlink-target
139 git submodule add ../module1 m1
140 git commit -m "add submodule"
141 printf 'gitdir: ../missing\n' >m1/.git
142)
143
144git init submodule-with-malformed-gitlink
145(cd submodule-with-malformed-gitlink
146 git submodule add ../module1 m1
147 git commit -m "add submodule"
148 printf 'bogus\n' >m1/.git
149)
150
123cp -R with-submodules with-submodules-in-index151cp -R with-submodules with-submodules-in-index
124(cd with-submodules-in-index152(cd with-submodules-in-index
125 git add .153 git add .
@@ -167,4 +195,4 @@ git clone with-submodules not-a-submodule · 41 unmodified lines
167 git add m1 && git commit -m "no submodule in index and commit, but in configuration"195 git add m1 && git commit -m "no submodule in index and commit, but in configuration"
168)196)
169 197
170git init unborn198git init unborn
gix/tests/gix/submodule.rs+189−0
@@ -157,6 +157,195 @@ mod open { · 156 unmodified lines
157 Ok(())157 Ok(())
158 }158 }
159 159
160 #[test]
161 fn gitlink_target_takes_precedence_over_name_in_git_dir_resolution() -> crate::Result {
162 let repo = repo("submodule-with-divergent-gitlink")?;
163 let sm = repo
164 .submodules()?
165 .expect("modules present")
166 .next()
167 .expect("one submodule");
168
169 let git_dir_from_name = sm.git_dir()?;
170 assert!(
171 git_dir_from_name.ends_with("modules/outer/inner"),
172 "the name-derived path is the fallback location, what it should be per submodule configuration/name"
173 );
174
175 let git_dir_from_gitlink = sm.git_dir_try_old_form()?;
176 assert!(
177 git_dir_from_gitlink.ends_with("modules/inner"),
178 "the worktree .git file points at the relocated repository"
179 );
180 assert_ne!(
181 git_dir_from_gitlink, git_dir_from_name,
182 "the gitlink target must be authoritative even when it differs from .git/modules/<name>"
183 );
184 assert_eq!(
185 sm.state()?,
186 submodule::State {
187 repository_exists: true,
188 is_old_form: false,
189 worktree_checkout: true,
190 superproject_configuration: true,
191 },
192 "a modern gitlink remains modern even when its target differs from the name-derived path"
193 );
194
195 #[cfg(feature = "status")]
196 {
197 let status = sm.status(gix::submodule::config::Ignore::None, false)?;
198 assert_eq!(
199 status.is_dirty(),
200 Some(false),
201 "opening the gitlink target avoids a 'phantom' submodule HEAD change"
202 );
203 assert_eq!(
204 status.checked_out_head_id, status.index_id,
205 "there are no HEAD changes even though the 'phantom' at modules/outer/inner has its HEAD at @~1"
206 );
207
208 let status = sm.status(gix::submodule::config::Ignore::All, false)?;
209 assert_eq!(
210 status.state,
211 submodule::State {
212 repository_exists: true,
213 is_old_form: false,
214 worktree_checkout: true,
215 superproject_configuration: true,
216 },
217 "ignore=all still follows a parseable divergent gitdir file"
218 );
219 }
220
221 Ok(())
222 }
223
224 #[test]
225 fn broken_gitlink_target_is_reported() -> crate::Result {
226 let repo = repo("submodule-with-missing-gitlink-target")?;
227 let sm = repo
228 .submodules()?
229 .expect("modules present")
230 .next()
231 .expect("one submodule");
232
233 assert!(matches!(
234 sm.git_dir_try_old_form(),
235 Err(submodule::git_dir_try_old_form::Error::InvalidGitDirFileTarget {
236 target: Some(target),
237 source: None,
238 ..
239 }) if target.ends_with("missing")
240 ));
241 assert!(matches!(
242 sm.state(),
243 Err(submodule::state::Error::GitDirTryOldForm(
244 submodule::git_dir_try_old_form::Error::InvalidGitDirFileTarget {
245 target: Some(target),
246 source: None,
247 ..
248 }
249 )) if target.ends_with("missing")
250 ));
251 assert!(matches!(
252 sm.open(),
253 Err(submodule::open::Error::GitDir(
254 submodule::git_dir_try_old_form::Error::InvalidGitDirFileTarget {
255 target: Some(target),
256 source: None,
257 ..
258 }
259 )) if target.ends_with("missing")
260 ));
261
262 #[cfg(feature = "status")]
263 assert!(
264 matches!(
265 sm.status(gix::submodule::config::Ignore::None, false),
266 Err(submodule::status::Error::State(
267 submodule::state::Error::GitDirTryOldForm(
268 submodule::git_dir_try_old_form::Error::InvalidGitDirFileTarget {
269 target: Some(target),
270 source: None,
271 ..
272 }
273 )
274 )) if target.ends_with("missing")
275 ),
276 "ignore=none fails as some submodules can't be opened"
277 );
278
279 #[cfg(feature = "status")]
280 {
281 let status = sm.status(gix::submodule::config::Ignore::All, false)?;
282 assert_eq!(
283 status.state,
284 submodule::State {
285 repository_exists: false,
286 is_old_form: false,
287 worktree_checkout: true,
288 superproject_configuration: true,
289 },
290 "ignore=all does not inspect the broken gitdir target"
291 );
292 }
293
294 Ok(())
295 }
296
297 #[test]
298 fn malformed_gitlink_target_is_ignored_by_ignore_all_status() -> crate::Result {
299 let repo = repo("submodule-with-malformed-gitlink")?;
300 let sm = repo
301 .submodules()?
302 .expect("modules present")
303 .next()
304 .expect("one submodule");
305
306 assert!(matches!(
307 sm.git_dir_try_old_form(),
308 Err(submodule::git_dir_try_old_form::Error::InvalidGitDirFileTarget {
309 target: None,
310 source: Some(_),
311 ..
312 })
313 ));
314
315 #[cfg(feature = "status")]
316 {
317 assert!(
318 matches!(
319 sm.status(gix::submodule::config::Ignore::None, false),
320 Err(submodule::status::Error::State(
321 submodule::state::Error::GitDirTryOldForm(
322 submodule::git_dir_try_old_form::Error::InvalidGitDirFileTarget {
323 target: None,
324 source: Some(_),
325 ..
326 }
327 )
328 ))
329 ),
330 "ignore=none fails as some submodules can't be opened"
331 );
332
333 let status = sm.status(gix::submodule::config::Ignore::All, false)?;
334 assert_eq!(
335 status.state,
336 submodule::State {
337 repository_exists: true,
338 is_old_form: false,
339 worktree_checkout: true,
340 superproject_configuration: true,
341 },
342 "ignore=all does not parse the malformed gitdir file"
343 );
344 }
345
346 Ok(())
347 }
348
160 #[cfg(feature = "status")]349 #[cfg(feature = "status")]
161 mod status {350 mod status {
162 use crate::{submodule::repo, util::hex_to_id};351 use crate::{submodule::repo, util::hex_to_id};
gix/tests/submodule_open.rs+4−1
@@ -24,7 +24,10 @@ fn on_nested_symlink() -> gix_testtools::Result { · 23 unmodified lines
24 "the workdir remains relative and is available"24 "the workdir remains relative and is available"
25 );25 );
26 let sm_repo = sm.open()?.expect("repo is present and accessible");26 let sm_repo = sm.open()?.expect("repo is present and accessible");
27 assert_eq!(sm_repo.git_dir(), "../../.git/modules/m1");27 assert!(
28 sm_repo.git_dir().ends_with(".git/modules/m1"),
29 "gitlink resolution may preserve symlink-sensitive parent components"
30 );
28 assert_eq!(31 assert_eq!(
29 sm_repo.workdir().expect("worktree present as we have one"),32 sm_repo.workdir().expect("worktree present as we have one"),
30 p("../../m1")33 p("../../m1")