public inbox for git@vger.kernel.org 
 help / color / mirror / Atom feed

From: Junio C Hamano <gitster@pobox•com>
To: "Francesco Guastella via GitGitGadget" <gitgitgadget@gmail•com>
Cc: git@vger•kernel.org,
	 Francesco Guastella <guastella.francesco@gmail•com>
Subject: Re: [PATCH] builtin/worktree: support relative paths for worktrees
Date: Fri, 20 Sep 2024 18:35:02 -0700	[thread overview]
Message-ID: <xmqqikupbxh5.fsf@gitster.g> (raw)
In-Reply-To: <pull.1783.git.git.1726880551891.gitgitgadget@gmail.com> (Francesco Guastella via GitGitGadget's message of "Sat, 21 Sep 2024 01:02:31 +0000")

"Francesco Guastella via GitGitGadget" <gitgitgadget@gmail•com>
writes:

> From: Francesco Guastella <guastella.francesco@gmail•com>
>
> Add a new configuration option, `worktree.useRelativePaths`, which allows
> users to choose between storing relative or absolute paths in the files
> Git uses to maintain links between the main worktree and any additional
> ones.

I may be misremembering things, but wasn't the use of absolute paths
a concious design decision?  

When the source repository and an attached worktree know each other
with relative location, moving merely one of them would make it
impossible to locate the other.

On the other hand, if they know the other peer's absolute location,
at least the one that was moved would still be able to locate the
one that did not move, which means that it is possible to find from
the one that moved the other one that did not move, and teach the
latter where the new location of the moved one is.

The only case where it may be an improvement to have them refer to
each other with relative locations is when both of them move in
unison without breaking their relative location.

There may be problems other than the design choice I mentioned above
in the resulting code after applying this huge patch, but as it
stands, the patch does a bit too many things at once to be sensibly
reviewable.  I cannot comment much on the implementation (at least
in this message) in its current form.

For example, the refactoring of t/t2400 into t/lib-worktree might be
an excellent thing to do, but it looks like that the change to the
tests alone deserves to be split into at least a few patches (one to
refactor the test script without changing any functionality, and one
or more patches to add or improve test helpers, and another that
comes with code and behaviour change that add tests for the new
behaviour when configured, or something like that).

I might be tempted to come back to it later, but style violations in
the t/lib-worktree.sh were annoying enough to discourage me from
reviewing it (if it followed Documentation/CodingGuidelines, it
wouldn't have repelled my eyes).

Thanks.

next prev parent reply	other threads:[~2024-09-21  1:35 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-09-21  1:02 [PATCH] builtin/worktree: support relative paths for worktrees Francesco Guastella via GitGitGadget
2024-09-21  1:35 ` Junio C Hamano [this message]
2024-09-21 14:58   ` Francesco Guastella
2024-09-24  8:06 ` Eric Sunshine

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqikupbxh5.fsf@gitster.g \
    --to=gitster@pobox$(echo .)com \
    --cc=git@vger$(echo .)kernel.org \
    --cc=gitgitgadget@gmail$(echo .)com \
    --cc=guastella.francesco@gmail$(echo .)com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox