[PATCH v2 0/5] worktree repair: avoid breaking unrelated .git file and gitdir

["Yoichi NAKAYAMA via GitGitGadget" <gitgitgadget@gmail.com>]

'git worktree repair' does not sufficiently validate the cross-references
between a linked working tree and its administrative data before repairing
them. This can cause the repair to modify the wrong .git file or gitdir in
certain situations.


v2 (0156021)
============

Split the first commit of v1 to make the changes easier to understand. Fold
long lines.

 * [1/5] align .git file handling with repair_gitfile()
 * [2/5] centralize inferred backlink selection
 * [3/5] use read_gitfile_raw() in infer_backlink()
 * [4/5] reduce .git file reads
 * [5/5] avoid breaking unrelated .git file and gitdir


v1 (99aa341)
============

This series first refactors the code to read the .git file once and extract
the worktree ID, then uses that information to validate the repair target
before modifying the cross-references.

 * [1/2] Refactor the code without changing functionality before making the
   fix
 * [2/2] Validate the worktree ID and inferred gitdir path before repairing

Yoichi NAKAYAMA (5):
  worktree repair: align .git file handling with repair_gitfile()
  worktree repair: centralize inferred backlink selection
  worktree repair: use read_gitfile_raw() in infer_backlink()
  worktree repair: reduce .git file reads
  worktree repair: avoid breaking unrelated .git file and gitdir

 t/t2406-worktree-repair.sh |  33 +++++++++--
 worktree.c                 | 113 +++++++++++++++++++++----------------
 2 files changed, 91 insertions(+), 55 deletions(-)


base-commit: 6de20f6092dcf9bdb1c8efe03db4b70c82b423dd
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2225%2Fyoichi%2Fworktree-repair-keep-unrelated-gitfile-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2225/yoichi/worktree-repair-keep-unrelated-gitfile-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/2225

Range-diff vs v1:

 -:  ---------- > 1:  21b2878aa5 worktree repair: align .git file handling with repair_gitfile()
 1:  dc7ebb427b ! 2:  30ac7acad3 worktree repair: refactor and reduce .git file reads
     @@ Metadata
      Author: Yoichi NAKAYAMA <yoichi.nakayama@gmail.com>
      
       ## Commit message ##
     -    worktree repair: refactor and reduce .git file reads
     +    worktree repair: centralize inferred backlink selection
      
     -    Remove the file reading and trimming logic from `infer_backlink()`,
     -    and instead read the .git file once in its caller,
     -    `repair_worktree_at_path()`, using `read_gitfile_raw()`. Since
     -    `read_gitfile_gently()` is replaced with `read_gitfile_raw()`, restore
     -    the logic for constructing the absolute path and replace the
     -    READ_GITFILE_ERR_NOT_A_REPO handling with a check using
     -    `is_git_directory()`. Simplify the logic for prioritizing
     -    'inferred_backlink' over 'backlink'.
     -
     -    Extract `get_worktree_id()` to get the worktree ID from the contents
     -    of the .git file. We are going to modify and use this function in
     -    subsequent commits.
     +    Reorder the processing steps to centralize the logic for preferring
     +    the inferred_backlink.
      
          Signed-off-by: Yoichi NAKAYAMA <yoichi.nakayama@gmail.com>
      
       ## worktree.c ##
     -@@ worktree.c: int other_head_refs(struct repository *repo,
     - 	return ret;
     - }
     - 
     -+static const char *get_worktree_id(const char *dotgit_contents)
     -+{
     -+	const char *slash = find_last_dir_sep(dotgit_contents);
     -+	if (!slash)
     -+		return "";
     -+	return slash + 1;
     -+}
     -+
     - /*
     -  * Repair worktree's /path/to/worktree/.git file if missing, corrupt, or not
     -  * pointing at <repo>/worktrees/<id>.
     -@@ worktree.c: static int is_main_worktree_path(struct repository *repo, const char *path)
     -  * Returns -1 on failure and strbuf.len on success.
     -  */
     - static ssize_t infer_backlink(struct repository *repo,
     --			      const char *gitfile,
     -+			      const char *dotgit_contents,
     - 			      struct strbuf *inferred)
     - {
     --	struct strbuf actual = STRBUF_INIT;
     - 	const char *id;
     - 
     --	if (strbuf_read_file(&actual, gitfile, 0) < 0)
     --		goto error;
     --	if (!starts_with(actual.buf, "gitdir:"))
     --		goto error;
     --	if (!(id = find_last_dir_sep(actual.buf)))
     --		goto error;
     --	strbuf_trim(&actual);
     --	id++; /* advance past '/' to point at <id> */
     -+	id = get_worktree_id(dotgit_contents);
     - 	if (!*id)
     - 		goto error;
     - 	repo_common_path_replace(repo, inferred, "worktrees/%s", id);
     - 	if (!is_directory(inferred->buf))
     - 		goto error;
     - 
     --	strbuf_release(&actual);
     - 	return inferred->len;
     - error:
     --	strbuf_release(&actual);
     - 	strbuf_reset(inferred); /* clear invalid path */
     - 	return -1;
     - }
      @@ worktree.c: void repair_worktree_at_path(struct repository *repo,
     - 	struct strbuf inferred_backlink = STRBUF_INIT;
       	struct strbuf gitdir = STRBUF_INIT;
       	struct strbuf olddotgit = STRBUF_INIT;
     --	char *dotgit_contents = NULL;
     -+	struct strbuf contents = STRBUF_INIT;
     + 	struct strbuf contents = STRBUF_INIT;
      +	const char *dotgit_contents = NULL;
       	const char *repair = NULL;
       	int err;
     @@ worktree.c: void repair_worktree_at_path(struct repository *repo,
       
      -	infer_backlink(repo, dotgit.buf, &inferred_backlink);
      -	strbuf_realpath_forgiving(&inferred_backlink, inferred_backlink.buf, 0);
     --	dotgit_contents = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &err));
     --	if (dotgit_contents) {
     --		strbuf_addstr(&backlink, dotgit_contents);
     --	} else if (err == READ_GITFILE_ERR_NOT_A_FILE ||
     --			err == READ_GITFILE_ERR_IS_A_DIR) {
     -+	err = read_gitfile_raw(&contents, dotgit.buf);
     -+	if (err == READ_GITFILE_ERR_NOT_A_FILE ||
     -+	    err == READ_GITFILE_ERR_IS_A_DIR) {
     - 		fn(1, dotgit.buf, _("unable to locate repository; .git is not a file"), cb_data);
     + 	err = read_gitfile_raw(&contents, dotgit.buf);
     +-	if (!err) {
     +-		const char *dotgit_contents = contents.buf;
     +-		if (is_absolute_path(dotgit_contents)) {
     +-			strbuf_addstr(&backlink, dotgit_contents);
     +-		} else {
     +-			strbuf_addbuf(&backlink, &dotgit);
     +-			strbuf_strip_suffix(&backlink, ".git");
     +-			strbuf_addstr(&backlink, dotgit_contents);
     +-			strbuf_realpath_forgiving(&backlink, backlink.buf, 0);
     +-		}
     +-	}
     + 	if (err == READ_GITFILE_ERR_NOT_A_FILE ||
     + 	    err == READ_GITFILE_ERR_IS_A_DIR) {
     + 		fn(1, dotgit.buf,
     + 		   _("unable to locate repository; .git is not a file"),
     + 		   cb_data);
       		goto done;
     --	} else if (err == READ_GITFILE_ERR_NOT_A_REPO) {
     +-	} else if (!err && !is_git_directory(backlink.buf)) {
     +-		strbuf_release(&backlink);
      -		if (inferred_backlink.len) {
      -			/*
      -			 * Worktree's .git file does not point at a repository
     @@ worktree.c: void repair_worktree_at_path(struct repository *repo,
      -			 */
      -			strbuf_swap(&backlink, &inferred_backlink);
      -		} else {
     --			fn(1, dotgit.buf, _("unable to locate repository; .git file does not reference a repository"), cb_data);
     +-			fn(1, dotgit.buf,
     +-			   _("unable to locate repository; .git file does not reference a repository"),
     +-			   cb_data);
      -			goto done;
      -		}
     --	} else {
     -+	} else if (err) {
     - 		fn(1, dotgit.buf, _("unable to locate repository; .git file broken"), cb_data);
     + 	} else if (err) {
     + 		fn(1, dotgit.buf,
     + 		   _("unable to locate repository; .git file broken"),
     +@@ worktree.c: void repair_worktree_at_path(struct repository *repo,
       		goto done;
       	}
       
      +	dotgit_contents = contents.buf;
     -+	infer_backlink(repo, dotgit_contents, &inferred_backlink);
     ++	infer_backlink(repo, dotgit.buf, &inferred_backlink);
      +	strbuf_realpath_forgiving(&inferred_backlink, inferred_backlink.buf, 0);
      +
      +	if (is_absolute_path(dotgit_contents)) {
     @@ worktree.c: void repair_worktree_at_path(struct repository *repo,
      +	}
      +
      +	if (!is_git_directory(backlink.buf) && !inferred_backlink.len) {
     -+		fn(1, dotgit.buf, _("unable to locate repository; .git file does not reference a repository"), cb_data);
     ++		fn(1, dotgit.buf,
     ++		   _("unable to locate repository; .git file does not reference a repository"),
     ++		   cb_data);
      +		goto done;
      +	}
      +
     @@ worktree.c: void repair_worktree_at_path(struct repository *repo,
       	 */
       	if (inferred_backlink.len && fspathcmp(backlink.buf, inferred_backlink.buf))
       		strbuf_swap(&backlink, &inferred_backlink);
     -@@ worktree.c: void repair_worktree_at_path(struct repository *repo,
     - 					     gitdir.buf, use_relative_paths);
     - 	}
     - done:
     --	free(dotgit_contents);
     - 	strbuf_release(&olddotgit);
     - 	strbuf_release(&backlink);
     - 	strbuf_release(&inferred_backlink);
     - 	strbuf_release(&gitdir);
     - 	strbuf_release(&dotgit);
     -+	strbuf_release(&contents);
     - }
     - 
     - int should_prune_worktree(struct repository *repo,
 -:  ---------- > 3:  2cc7b817ee worktree repair: use read_gitfile_raw() in infer_backlink()
 -:  ---------- > 4:  8c80b9ae05 worktree repair: reduce .git file reads
 2:  99aa34135c ! 5:  015602168c worktree repair: avoid breaking unrelated .git file and gitdir
     @@ worktree.c: static ssize_t infer_backlink(struct repository *repo,
       
       	return inferred->len;
      @@ worktree.c: void repair_worktree_at_path(struct repository *repo,
     - 		fn(1, dotgit.buf, _("unable to locate repository; .git file does not reference a repository"), cb_data);
     + 		   cb_data);
       		goto done;
       	}
      +	if (!inferred_backlink.len) {
     -+		fn(1, dotgit.buf, _("unable to locate repository; .git file is not for a linked worktree"), cb_data);
     ++		fn(1, dotgit.buf,
     ++		   _("unable to locate repository; .git file is not for a linked worktree"),
     ++		   cb_data);
      +		goto done;
      +	}
       

-- 
gitgitgadget