[PATCH v3 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.


v3
==

Make the separator check in get_worktree_id() more lenient.

 * [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


v2
==

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
==

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 |  57 ++++++++++++--
 worktree.c                 | 153 ++++++++++++++++++++++++-------------
 2 files changed, 152 insertions(+), 58 deletions(-)


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

Range-diff vs v2:

 1:  21b2878aa5 = 1:  e336afa029 worktree repair: align .git file handling with repair_gitfile()
 2:  30ac7acad3 = 2:  86021f31af worktree repair: centralize inferred backlink selection
 3:  2cc7b817ee = 3:  7087e829bb worktree repair: use read_gitfile_raw() in infer_backlink()
 4:  8c80b9ae05 = 4:  e35285a4ea worktree repair: reduce .git file reads
 5:  015602168c ! 5:  f1d5c7b5f7 worktree repair: avoid breaking unrelated .git file and gitdir
     @@ t/t2406-worktree-repair.sh: test_expect_success 'repair .git file from bare.git'
      +	test_cmp expect corrupt/.git &&
      +	test_grep "unrelated .git file" err
      +'
     ++
     ++test_expect_success 'repair moved worktree whose .git file has extra separators' '
     ++	test_when_finished "rm -rf side sidemoved && git worktree prune" &&
     ++	git worktree add --detach side &&
     ++	mv side sidemoved &&
     ++	cat sidemoved/.git >expect &&
     ++	sed s,/,//,g sidemoved/.git | sed s,$,//, >sidemoved/.newgit &&
     ++	mv sidemoved/.newgit sidemoved/.git &&
     ++	git worktree repair sidemoved 2>err &&
     ++	test_cmp expect sidemoved/.git &&
     ++	test_grep "gitdir incorrect" err
     ++'
     ++
     ++test_expect_success WINDOWS 'repair moved worktree whose .git file uses backslash' '
     ++	test_when_finished "rm -rf side sidemoved && git worktree prune" &&
     ++	git worktree add --detach side &&
     ++	mv side sidemoved &&
     ++	cat sidemoved/.git >expect &&
     ++	sed s,\/,\\\\,g sidemoved/.git >sidemoved/.newgit &&
     ++	mv sidemoved/.newgit sidemoved/.git &&
     ++	git worktree repair sidemoved 2>err &&
     ++	test_cmp expect sidemoved/.git &&
     ++	test_grep "gitdir incorrect" err
     ++'
      +
       test_expect_success 'invalid worktree path' '
       	test_must_fail git worktree repair /notvalid >out 2>err &&
     @@ t/t2406-worktree-repair.sh: test_expect_success 'repo not found; .git not refere
      
       ## worktree.c ##
      @@ worktree.c: int other_head_refs(struct repository *repo,
     - static const char *get_worktree_id(const char *dotgit_contents)
     + 	return ret;
     + }
     + 
     +-static const char *get_worktree_id(const char *dotgit_contents)
     ++static int get_worktree_id(struct strbuf *worktree_id,
     ++			   const char *dotgit_contents)
       {
     - 	const char *slash = find_last_dir_sep(dotgit_contents);
     +-	const char *slash = find_last_dir_sep(dotgit_contents);
      -	if (!slash)
     -+	const char *prefix = "/worktrees";
     +-		return "";
     +-	return slash + 1;
     ++	const char *id_end;
     ++	const char *id;
     ++	const char *prefix_end;
     ++	const char *prefix = "worktrees";
      +	int prefixlen = strlen(prefix);
     -+	if (!slash ||
     -+	    slash - dotgit_contents < prefixlen ||
     -+	    strncmp(slash - prefixlen, prefix, prefixlen))
     - 		return "";
     - 	return slash + 1;
     ++
     ++	id_end = dotgit_contents + strlen(dotgit_contents);
     ++
     ++	/* skip trailing separators */
     ++	while (dotgit_contents < id_end && is_dir_sep(*(id_end - 1)))
     ++		id_end--;
     ++	if (id_end == dotgit_contents)
     ++		return 1;
     ++
     ++	id = id_end;
     ++
     ++	/* find the last separator */
     ++	while (dotgit_contents < id && !is_dir_sep(*(id - 1)))
     ++		id--;
     ++	if (id == dotgit_contents || id == id_end)
     ++		return 1;
     ++
     ++	prefix_end = id - 1;
     ++
     ++	/* skip repeated separators */
     ++	while (dotgit_contents < prefix_end && is_dir_sep(*(prefix_end - 1)))
     ++		prefix_end--;
     ++
     ++	/* check for "/worktrees" */
     ++	if (prefix_end - dotgit_contents < prefixlen + 1 ||
     ++	    strncmp(prefix_end - prefixlen, prefix, prefixlen) ||
     ++	    !is_dir_sep(*(prefix_end - prefixlen - 1)))
     ++		return 1;
     ++
     ++	strbuf_add(worktree_id, id, id_end - id);
     ++	return 0;
       }
     + 
     + /*
     +@@ worktree.c: static void repair_gitfile(struct worktree *wt,
     + 	struct strbuf repo = STRBUF_INIT;
     + 	struct strbuf backlink = STRBUF_INIT;
     + 	struct strbuf contents = STRBUF_INIT;
     ++	struct strbuf worktree_id = STRBUF_INIT;
     + 	const char *dotgit_contents = NULL;
     + 	const char *repair = NULL;
     + 	char *path = NULL;
      @@ worktree.c: static void repair_gitfile(struct worktree *wt,
       	if (err == READ_GITFILE_ERR_NOT_A_FILE ||
       		err == READ_GITFILE_ERR_IS_A_DIR)
     @@ worktree.c: static void repair_gitfile(struct worktree *wt,
      -	else if (err || !is_git_directory(backlink.buf))
      +	else if (err)
       		repair = _(".git file broken");
     -+	else if (strcmp(get_worktree_id(dotgit_contents), wt->id))
     ++	else if (get_worktree_id(&worktree_id, dotgit_contents) ||
     ++		 strcmp(worktree_id.buf, wt->id))
      +		fn(1, wt->path, _("unrelated .git file"), cb_data);
       	else if (fspathcmp(backlink.buf, repo.buf))
       		repair = _(".git file incorrect");
       	else if (use_relative_paths == is_absolute_path(dotgit_contents))
     +@@ worktree.c: done:
     + 	strbuf_release(&gitdir);
     + 	strbuf_release(&backlink);
     + 	strbuf_release(&contents);
     ++	strbuf_release(&worktree_id);
     + }
     + 
     + static void repair_noop(int iserr UNUSED,
      @@ worktree.c: static ssize_t infer_backlink(struct repository *repo,
     - 	if (!*id)
     + 			      const char *dotgit_contents,
     + 			      struct strbuf *inferred)
     + {
     +-	const char *id;
     ++	struct strbuf worktree_id = STRBUF_INIT;
     + 
     +-	id = get_worktree_id(dotgit_contents);
     +-	if (!*id)
     ++	if (get_worktree_id(&worktree_id, dotgit_contents))
       		goto error;
     - 	repo_common_path_replace(repo, inferred, "worktrees/%s", id);
     +-	repo_common_path_replace(repo, inferred, "worktrees/%s", id);
      -	if (!is_directory(inferred->buf))
     ++	repo_common_path_replace(repo, inferred,
     ++				 "worktrees/%s", worktree_id.buf);
      +	if (!is_git_directory(inferred->buf))
       		goto error;
       
     ++	strbuf_release(&worktree_id);
       	return inferred->len;
     + error:
     + 	strbuf_reset(inferred); /* clear invalid path */
     ++	strbuf_release(&worktree_id);
     + 	return -1;
     + }
     + 
      @@ worktree.c: void repair_worktree_at_path(struct repository *repo,
       		   cb_data);
       		goto done;

-- 
gitgitgadget