Skip to content

Commit db925ec

Browse files
ByronByron
authored
Merge pull request #2588 from GitoxideLabs/fix/gix-status-phantom-submodule-modifications-2585
Follow submodule gitdir files when opening
2 parents ba7d9a4 + 5a37fbd commit db925ec

6 files changed

Lines changed: 279 additions & 15 deletions

File tree

‎gix/src/submodule/errors.rs‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -91,6 +91,13 @@ pub mod git_dir_try_old_form {
9191
pub enum Error {
9292
#[error(transparent)]
9393
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+
},
94101
#[error(transparent)]
95102
GitDir(#[from] gix_validate::submodule::name::Error),
96103
}

‎gix/src/submodule/mod.rs‎

Lines changed: 50 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -232,25 +232,56 @@ impl Submodule<'_> {
232232
/// the superproject's worktree where it actually *is* located if the submodule in the 'old-form', thus is a directory
233233
/// inside of the superproject's work-tree.
234234
///
235-
/// Note that 'old-form' paths returned aren't verified, i.e. the `.git` repository might be corrupt or otherwise
236-
/// invalid - it's left to the caller to try to open it.
235+
/// 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. `.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.
237238
///
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.
239240
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()?;
240254
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() {
242256
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+
}
243277
} else {
244-
self.git_dir()?
278+
git_dir
245279
};
246-
Ok(worktree_gitdir_or_modules_gitdir)
280+
Ok(git_dir)
247281
}
248282

249-
/// Query various parts of the submodule and assemble it into state information.
250-
#[doc(alias = "status", alias = "git2")]
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()?;
283+
fn state_inner(&self, validate_gitdir_file_target: bool) -> Result<State, state::Error> {
284+
let maybe_old_path = self.git_dir_try_old_form_inner(validate_gitdir_file_target)?;
254285
let worktree_git = self.worktree_gitdir()?;
255286
let superproject_configuration = self
256287
.state
@@ -263,12 +294,18 @@ impl Submodule<'_> {
263294
.any(|section| section.header().subsection_name() == Some(self.name.as_ref()));
264295
Ok(State {
265296
repository_exists: maybe_old_path.is_dir(),
266-
is_old_form: maybe_old_path != git_dir,
297+
is_old_form: worktree_git.is_dir(),
267298
worktree_checkout: worktree_git.exists(),
268299
superproject_configuration,
269300
})
270301
}
271302

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+
272309
/// Open the submodule as repository, or `None` if the submodule wasn't initialized yet.
273310
///
274311
/// More states can be derived here:
@@ -375,7 +412,7 @@ pub mod status {
375412
)
376413
-> crate::status::Platform<'a, gix_features::progress::Discard>,
377414
) -> Result<Status, Error> {
378-
let mut state = self.state()?;
415+
let mut state = self.state_inner(ignore != config::Ignore::All)?;
379416
if ignore == config::Ignore::All {
380417
return Ok(Status {
381418
state,

‎gix/tests/fixtures/generated-archives/make_submodules.tar‎

460 KB
Binary file not shown.

‎gix/tests/fixtures/make_submodules.sh‎

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,34 @@ git init with-submodules
120120
git submodule add ../module1 dir/m1
121121
)
122122

123+
git 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+
137+
git 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+
144+
git 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+
123151
cp -R with-submodules with-submodules-in-index
124152
(cd with-submodules-in-index
125153
git add .
@@ -167,4 +195,4 @@ git clone with-submodules not-a-submodule
167195
git add m1 && git commit -m "no submodule in index and commit, but in configuration"
168196
)
169197

170-
git init unborn
198+
git init unborn

‎gix/tests/gix/submodule.rs‎

Lines changed: 189 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -157,6 +157,195 @@ mod open {
157157
Ok(())
158158
}
159159

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+
160349
#[cfg(feature = "status")]
161350
mod status {
162351
use crate::{submodule::repo, util::hex_to_id};

‎gix/tests/submodule_open.rs‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,10 @@ fn on_nested_symlink() -> gix_testtools::Result {
2424
"the workdir remains relative and is available"
2525
);
2626
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+
);
2831
assert_eq!(
2932
sm_repo.workdir().expect("worktree present as we have one"),
3033
p("../../m1")

0 commit comments

Comments
 (0)