* Re: [PATCH v2 3/2] credential-cache: use child_process.args
From: Jeff King @ 2020-08-27 4:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqzh6ht7fg.fsf_-_@gitster.c.googlers.com>
On Wed, Aug 26, 2020 at 02:37:39PM -0700, Junio C Hamano wrote:
> As child_process structure has an embedded strvec args for
> formulating the command line, let's use it instead of using
> an out-of-line argv[] whose length needs to be maintained
> correctly.
I forgot to mention in my other reply: I think this fails to mention the
actual functional change, which is switching from the dashed form to
using the "git" wrapper.
-Peff
^ permalink raw reply
* Re: [PATCH] clone: add remote.cloneDefault config option
From: Junio C Hamano @ 2020-08-27 4:21 UTC (permalink / raw)
To: Sean Barag; +Cc: git, gitgitgadget, stolee
In-Reply-To: <20200827033854.130005-1-sean@barag.org>
Sean Barag <sean@barag.org> writes:
>> It is somewhat sad that we have the git_config(git_default_config)
>> call so late in the control flow. I wonder if we can update the
>> start-up sequence to match the usual flow
>> ...
>> One oddity "git clone" has is that it wants to delay the reading of
>> configuration files (they are read only once, and second and
>> subsequent git_config() calls will reuse what was read before [*]) so
>> that it can read what clone.c::write_config() wrote, so if we were to
>> "fix" the start-up sequence to match the usual flow, we need to
>> satisfy what that odd arrangement wanted to achieve in some other way
>> (e.g. feed what is in option_config to git_default_config ourselves,
>> without using git_config(), as part of the "main control flow uses the
>> variable" part), but it should be doable.
>
> Sounds like a pretty big change! I'm willing to take a crack at it,
> but given that this is my first patch I'm frankly a bit intimidated :)
> How would you feel about that being a separate patch?
It definitely needs to be a separate patch.
In order to realize any new feature that needs to read the existing
(i.e. per-machine or per-user) configuration files to affect the
behaviour of "git clone", whether it is the "give default to
--origin option" or any other thing, first needs to fix the start-up
sequence so that the configuration is read once before we process
command line options, which is the norm. Only after that is done,
we can build the clone.defaultRemoteName and other features that
would be affected by the settings of clone.* variables on top, and
it is not wise to mix the foundation with a new feature that uses
the foundation. So I would imagine it would at least be 3-patch
series:
- [1/3] would be to whip the start-up sequence into the normal
order. This may be the most tricky part. I identified that the
handling of option_config might need adjustment, but there may be
others. This may not need any new tests, but if the existing
tests for "clone -c var=val" do not catch breakage when we
naively move the git_config(git_default_config) call early
without doing any other adjustment, we might need to add more
tests to cover the option. If we find things other than
option_config needs adjustment, they also need test coverage.
- [2/3] would be to tighten the error checking of option_origin
that was lost when the command was reimplemented in C (already
discussed). This would need new tests to see "--origin $bogus"
command line option is flagged as an error.
- [3/3] would be to read option_origin from the configuration file.
The start-up sequence fixed by [1/3] would allow us to write the
config callback in a more natural way, compared to what you wrote
and what I suggested to rewrite. Error checking code in [2/3]
would directly be reusable in the code added in this step. We'd
need tests similar to we add in [2/3] for the configuration, and
combination of configuration and command line (you already wrote
most of these).
Thanks.
^ permalink raw reply
* Re: [PATCH] run_command: teach API users to use embedded 'args' more
From: Jeff King @ 2020-08-27 4:21 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqmu2ht58g.fsf_-_@gitster.c.googlers.com>
On Wed, Aug 26, 2020 at 03:25:03PM -0700, Junio C Hamano wrote:
> The child_process structure has an embedded strvec for formulating
> the command line argument list these days, but code that predates
> the wide use of it prepared a separate char *argv[] array and
> manually set the child_process.argv pointer point at it.
>
> Teach these old-style code to lose the separate argv[] array.
I think the result is much nicer and less error-prone (especially the
ones that pre-size the array with NULLs and fill in the components
later). It incurs a few extra mallocs at run-time, but on top of an
execve(), that's a drop in the bucket.
I've actually considered dropping child_process.argv entirely. Having
two separate ways to do the same thing gives the potential for
confusion. But I never dug into whether any existing callers would be
made worse for it (I kind of doubt it, though; worst case they can use
strvec_pushv). There are still several left after this patch, it seems.
Likewise for child_process.env_array.
-Peff
^ permalink raw reply
* Re: [PATCH v2 3/2] credential-cache: use child_process.args
From: Jeff King @ 2020-08-27 4:22 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <20200827041328.GA3346457@coredump.intra.peff.net>
On Thu, Aug 27, 2020 at 12:13:28AM -0400, Jeff King wrote:
> On Wed, Aug 26, 2020 at 02:37:39PM -0700, Junio C Hamano wrote:
>
> > diff --git a/builtin/credential-cache.c b/builtin/credential-cache.c
> > index d0fafdeb9e..195335a783 100644
> > --- a/builtin/credential-cache.c
> > +++ b/builtin/credential-cache.c
> > @@ -42,13 +42,13 @@ static int send_request(const char *socket, const struct strbuf *out)
> > static void spawn_daemon(const char *socket)
> > {
> > struct child_process daemon = CHILD_PROCESS_INIT;
> > - const char *argv[] = { NULL, NULL, NULL };
> > char buf[128];
> > int r;
> >
> > - argv[0] = "git-credential-cache--daemon";
> > - argv[1] = socket;
> > - daemon.argv = argv;
> > + strvec_pushl(&daemon.args,
> > + "credential-cache--daemon", socket,
> > + NULL);
> > + daemon.git_cmd = 1;
>
> Yep, this makes sense. I don't recall any reason to use the dashed form
> back then, but probably it was just that I knew it was a separate
> program. Doing it this way will mean an extra parent "git" process
> hanging around, but I don't think it's that big a deal. We never try to
> kill it by PID, etc (instead we connect to the socket and ask it to
> exit). And anyway, it is becoming a builtin in a parallel topic, so that
> extra process will go away. :)
...and if I had read the diff header more carefully, I'd see that you
did this on top of that topic. :)
-Peff
^ permalink raw reply
* Re: [PATCH] pretty-options.txt: fix --no-abbrev-commit description
From: Jeff King @ 2020-08-27 4:25 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Sergey Organov, git
In-Reply-To: <xmqqr1rtt613.fsf@gitster.c.googlers.com>
On Wed, Aug 26, 2020 at 03:07:52PM -0700, Junio C Hamano wrote:
> Sergey Organov <sorganov@gmail.com> writes:
>
> >> Keeping the original sentence structure, e.g.
> >>
> >> ... and those options which imply abbreviating commit object names
> >> such as ...
> >>
> >> would have been what I wrote, instead of "either explicit or implied
> >> by", though.
> >
> > Sorry, but it'd then read:
> >
> > This negates `--abbrev-commit` and those options which imply
> > abbreviating commit object names such as "--oneline".
> >
> > that again essentially reduces to:
> >
> > This negates "--oneline"
>
> "--oneline" means a lot more than "do not use full object name", and
> I think we are on the same page with our shared goal of not negating
> everything "--oneline" means. We just want to say the option
> negates only the "do not use full object name" aspect.
>
> "and the effect of abbreviating commit objects implied by other
> options, such as '--oneline'" may be a more verbose way to say the
> same thing, I would think, but that would be overkill. I would have
> expected that with common sense readers would think it would be
> crazy for --no-abbrev to override everything --oneline means, but if
> you found that the original risks such an interpretation, perhaps we
> would need to be more verbose and explicit. I dunno.
FWIW, as a third-party observer (because you wanted more opinions,
right?), I found the result of Sergey's original patch easy to read and
understand.
I also think it's unlikely for people to misinterpret the current text,
but it does not hurt to be more precise just in case.
-Peff
^ permalink raw reply
* Re: [PATCH] run_command: teach API users to use embedded 'args' more
From: Junio C Hamano @ 2020-08-27 4:30 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20200827042157.GC3346457@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I've actually considered dropping child_process.argv entirely. Having
> two separate ways to do the same thing gives the potential for
> confusion. But I never dug into whether any existing callers would be
> made worse for it (I kind of doubt it, though; worst case they can use
> strvec_pushv). There are still several left after this patch, it seems.
>
> Likewise for child_process.env_array.
Yup, conversion similar to what I did in this patch may be too
trivial for #microproject, but would nevertheless be a good
#leftoverbits task. The removal of .argv/.env is not entirely
trivial but a good candidate for #microproject.
Thanks.
^ permalink raw reply
* Re: [PATCH v2 3/2] credential-cache: use child_process.args
From: Junio C Hamano @ 2020-08-27 4:31 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20200827041328.GA3346457@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Yep, this makes sense. I don't recall any reason to use the dashed form
> back then, but probably it was just that I knew it was a separate
> program. Doing it this way will mean an extra parent "git" process
> hanging around, but I don't think it's that big a deal. We never try to
> kill it by PID, etc (instead we connect to the socket and ask it to
> exit). And anyway, it is becoming a builtin in a parallel topic, so that
> extra process will go away. :)
Yeah, this was discovered when I tentatively merged that "don't run
built-in as git-foo" to the tip of seen. Without your slimmed-down
topic, it wouldn't have been caught. Likewise for remote-ext.
^ permalink raw reply
* Re: [PATCH] run_command: teach API users to use embedded 'args' more
From: Eric Sunshine @ 2020-08-27 4:31 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Git List
In-Reply-To: <20200827042157.GC3346457@coredump.intra.peff.net>
On Thu, Aug 27, 2020 at 12:22 AM Jeff King <peff@peff.net> wrote:
> I've actually considered dropping child_process.argv entirely. Having
> two separate ways to do the same thing gives the potential for
> confusion. [...]
>
> Likewise for child_process.env_array.
builtin/worktree.c:add_worktree() is a case in which an environment
strvec is built once and re-used for multiple run_command()
invocations by re-assigning it to child_process.env before each
run_command(). It uses child_process.env rather than
child_process.env_array because run_command() clears
child_process.env_array upon completion, which makes it difficult to
reuse a prepared environment strvec repeatedly.
Nevertheless, that isn't much of a reason to keep child_process.env.
Refactoring add_worktree() a bit to rebuild the environment strvec
on-demand wouldn't be very difficult.
^ permalink raw reply
* Re: [PATCH] pretty-options.txt: fix --no-abbrev-commit description
From: Junio C Hamano @ 2020-08-27 4:32 UTC (permalink / raw)
To: Jeff King; +Cc: Sergey Organov, git
In-Reply-To: <20200827042530.GE3346457@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Wed, Aug 26, 2020 at 03:07:52PM -0700, Junio C Hamano wrote:
>
>> Sergey Organov <sorganov@gmail.com> writes:
>>
>> >> Keeping the original sentence structure, e.g.
>> >>
>> >> ... and those options which imply abbreviating commit object names
>> >> such as ...
>> >>
>> >> would have been what I wrote, instead of "either explicit or implied
>> >> by", though.
>> >
>> > Sorry, but it'd then read:
>> >
>> > This negates `--abbrev-commit` and those options which imply
>> > abbreviating commit object names such as "--oneline".
>> >
>> > that again essentially reduces to:
>> >
>> > This negates "--oneline"
>>
>> "--oneline" means a lot more than "do not use full object name", and
>> I think we are on the same page with our shared goal of not negating
>> everything "--oneline" means. We just want to say the option
>> negates only the "do not use full object name" aspect.
>>
>> "and the effect of abbreviating commit objects implied by other
>> options, such as '--oneline'" may be a more verbose way to say the
>> same thing, I would think, but that would be overkill. I would have
>> expected that with common sense readers would think it would be
>> crazy for --no-abbrev to override everything --oneline means, but if
>> you found that the original risks such an interpretation, perhaps we
>> would need to be more verbose and explicit. I dunno.
>
> FWIW, as a third-party observer (because you wanted more opinions,
> right?), I found the result of Sergey's original patch easy to read and
> understand.
>
> I also think it's unlikely for people to misinterpret the current text,
> but it does not hurt to be more precise just in case.
Yup, I think I said the same already ;-)
^ permalink raw reply
* Re: [PATCH] run_command: teach API users to use embedded 'args' more
From: Jeff King @ 2020-08-27 4:44 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Junio C Hamano, Git List
In-Reply-To: <CAPig+cS1uMw6YDVjzb8FbBmC=iVjged-wHu0LF2+trmW-4ZfVw@mail.gmail.com>
On Thu, Aug 27, 2020 at 12:31:52AM -0400, Eric Sunshine wrote:
> On Thu, Aug 27, 2020 at 12:22 AM Jeff King <peff@peff.net> wrote:
> > I've actually considered dropping child_process.argv entirely. Having
> > two separate ways to do the same thing gives the potential for
> > confusion. [...]
> >
> > Likewise for child_process.env_array.
>
> builtin/worktree.c:add_worktree() is a case in which an environment
> strvec is built once and re-used for multiple run_command()
> invocations by re-assigning it to child_process.env before each
> run_command(). It uses child_process.env rather than
> child_process.env_array because run_command() clears
> child_process.env_array upon completion, which makes it difficult to
> reuse a prepared environment strvec repeatedly.
>
> Nevertheless, that isn't much of a reason to keep child_process.env.
> Refactoring add_worktree() a bit to rebuild the environment strvec
> on-demand wouldn't be very difficult.
I think they'd still be one-liner changes, like:
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 378f332b5d..b40f0d4cea 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -422,7 +422,7 @@ static int add_worktree(const char *path, const char *refname,
strvec_push(&cp.args, "--quiet");
}
- cp.env = child_env.v;
+ strvec_pushv(&cp.env_array, child_env.v);
ret = run_command(&cp);
if (ret)
goto done;
@@ -433,7 +433,7 @@ static int add_worktree(const char *path, const char *refname,
strvec_pushl(&cp.args, "reset", "--hard", "--no-recurse-submodules", NULL);
if (opts->quiet)
strvec_push(&cp.args, "--quiet");
- cp.env = child_env.v;
+ strvec_pushv(&cp.env_array, child_env.v);
ret = run_command(&cp);
if (ret)
goto done;
I think there are other opportunities for cleanup, too:
- the code right above the second hunk clears cp.args manually. That
shouldn't be necessary because run_command() will leave it in a
blank state (and we're already relying on that, since otherwise we'd
be left with cruft in other fields from the previous run).
- check_clean_worktree() only uses it once, and could drop the
separate child_env (and in fact appears to leak it)
-Peff
^ permalink raw reply related
* Re: [PATCH] run_command: teach API users to use embedded 'args' more
From: Eric Sunshine @ 2020-08-27 5:03 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Git List
In-Reply-To: <20200827044420.GA3360616@coredump.intra.peff.net>
On Thu, Aug 27, 2020 at 12:44 AM Jeff King <peff@peff.net> wrote:
> On Thu, Aug 27, 2020 at 12:31:52AM -0400, Eric Sunshine wrote:
> > Nevertheless, that isn't much of a reason to keep child_process.env.
> > Refactoring add_worktree() a bit to rebuild the environment strvec
> > on-demand wouldn't be very difficult.
>
> I think they'd still be one-liner changes, like:
>
> - cp.env = child_env.v;
> + strvec_pushv(&cp.env_array, child_env.v);
Nice and simple.
> I think there are other opportunities for cleanup, too:
True on both counts.
> - the code right above the second hunk clears cp.args manually. That
> shouldn't be necessary because run_command() will leave it in a
> blank state (and we're already relying on that, since otherwise we'd
> be left with cruft in other fields from the previous run).
Right. I wonder why the author of 7f44e3d1de (worktree: make setup of
new HEAD distinct from worktree population, 2015-07-17) chose to clear
cp.args manually like that.
> - check_clean_worktree() only uses it once, and could drop the
> separate child_env (and in fact appears to leak it)
Perhaps this unnecessary 'child_env' strvec was a copy/paste from
add_worktree()? But certainly cp.env_array would be simpler and avoid
the leak.
^ permalink raw reply
* [PATCH] worktree: fix leak in check_clean_worktree()
From: Jeff King @ 2020-08-27 5:25 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Junio C Hamano, Git List
In-Reply-To: <CAPig+cSA56xgNN0WP4t+YoyNU8fGf5eaz__=4Vh+s=He-tG=DA@mail.gmail.com>
On Thu, Aug 27, 2020 at 01:03:43AM -0400, Eric Sunshine wrote:
> > - the code right above the second hunk clears cp.args manually. That
> > shouldn't be necessary because run_command() will leave it in a
> > blank state (and we're already relying on that, since otherwise we'd
> > be left with cruft in other fields from the previous run).
>
> Right. I wonder why the author of 7f44e3d1de (worktree: make setup of
> new HEAD distinct from worktree population, 2015-07-17) chose to clear
> cp.args manually like that.
I wondered if we might not have cleared the array automatically back
then, but it looks like we did.
I do think this kind of child_process struct reuse is slightly sketchy
in general. Looking at child_process_clear(), we only free the memory,
but leave other fields set. And in fact we rely on that here; git_cmd
needs to remain set for both commands to work. But if the first command
had used, say, cp.in, then we'd be left with a bogus descriptor.
> > - check_clean_worktree() only uses it once, and could drop the
> > separate child_env (and in fact appears to leak it)
>
> Perhaps this unnecessary 'child_env' strvec was a copy/paste from
> add_worktree()? But certainly cp.env_array would be simpler and avoid
> the leak.
Yeah, that was my guess, too.
Most of these issues are more complex and/or should be part of a larger
cleanup effort. But let's fix the leak while we're thinking about it.
-- >8 --
Subject: [PATCH] worktree: fix leak in check_clean_worktree()
We allocate a child_env strvec but never free its memory. Instead, let's
just use the strvec that our child_process struct provides, which is
cleaned up automatically when we run the command.
And while we're moving the initialization of the child_process around,
let's switch it to use the official init function (zero-initializing it
works OK, since strvec is happy enough with that, but it sets a bad
example).
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/worktree.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 378f332b5d..df214697d2 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -924,7 +924,6 @@ static int move_worktree(int ac, const char **av, const char *prefix)
static void check_clean_worktree(struct worktree *wt,
const char *original_path)
{
- struct strvec child_env = STRVEC_INIT;
struct child_process cp;
char buf[1];
int ret;
@@ -935,15 +934,14 @@ static void check_clean_worktree(struct worktree *wt,
*/
validate_no_submodules(wt);
- strvec_pushf(&child_env, "%s=%s/.git",
+ child_process_init(&cp);
+ strvec_pushf(&cp.env_array, "%s=%s/.git",
GIT_DIR_ENVIRONMENT, wt->path);
- strvec_pushf(&child_env, "%s=%s",
+ strvec_pushf(&cp.env_array, "%s=%s",
GIT_WORK_TREE_ENVIRONMENT, wt->path);
- memset(&cp, 0, sizeof(cp));
strvec_pushl(&cp.args, "status",
"--porcelain", "--ignore-submodules=none",
NULL);
- cp.env = child_env.v;
cp.git_cmd = 1;
cp.dir = wt->path;
cp.out = -1;
--
2.28.0.751.g0834cceced
^ permalink raw reply related
* Re: post-checkout hook aborts rebase
From: Chris Torek @ 2020-08-27 5:44 UTC (permalink / raw)
To: Tom Rutherford; +Cc: Junio C Hamano, Git List
In-Reply-To: <CAHr-Uu8umDQJ=LORaBNJX+wnmaeM1hHxxpG7xROPgCqgEPrwdw@mail.gmail.com>
On Wed, Aug 26, 2020 at 5:48 PM Tom Rutherford <tmrutherford@gmail.com> wrote:
>
> Thank you for the response Junio.
>
> For what it's worth, my hook does not make changes to the repo. It's
> running a command to check that the installed version of our
> dependencies match the version specified in the commit being checked
> out, and merely warns if the two don't match (then exits with a
> nonzero return code).
I've run into this before myself. The core of the bug is that when
`git checkout` runs the post-checkout hook, whatever exit status
that hook has, `git checkout` has the same exit status.
This might be intended as a feature, but if so, the documentation
needs a tweak: the githooks docs say in part
This hook cannot affect the outcome of git checkout.
If "outcome" includes exit status -- to me, it does -- either the docs
are wrong or the code is wrong.
> For this reason it's been convenient that the hook runs during
> rebases, but I find it surprising that the nonzero return code would
> impact the rebase.
I have to agree with this, too.
(The simplest fix would be to have `git checkout` ignore the status
from the post-checkout hook, of course, and just exit 0 for success.)
Chris
^ permalink raw reply
* Re: [PATCH] worktree: fix leak in check_clean_worktree()
From: Eric Sunshine @ 2020-08-27 5:56 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Git List
In-Reply-To: <20200827052504.GA3360984@coredump.intra.peff.net>
On Thu, Aug 27, 2020 at 1:25 AM Jeff King <peff@peff.net> wrote:
> On Thu, Aug 27, 2020 at 01:03:43AM -0400, Eric Sunshine wrote:
> > Right. I wonder why the author of 7f44e3d1de (worktree: make setup of
> > new HEAD distinct from worktree population, 2015-07-17) chose to clear
> > cp.args manually like that.
>
> I wondered if we might not have cleared the array automatically back
> then, but it looks like we did.
I had the same thought and came to the same conclusion. It's possible
that the manual clearing of the array was leftover cruft as the
implementation matured before the patch was submitted. I have a vague
(perhaps false) recollection that a local argv_array was populated and
assigned to cp.argv originally, until cp.args was discovered as a
cleaner alternative and used instead. If that was the case, then the
local argv_array wouldn't have been cleared automatically by
run_command(), which would account for clearing it manually.
> I do think this kind of child_process struct reuse is slightly sketchy
> in general. Looking at child_process_clear(), we only free the memory,
> but leave other fields set. And in fact we rely on that here; git_cmd
> needs to remain set for both commands to work. But if the first command
> had used, say, cp.in, then we'd be left with a bogus descriptor.
Agreed. The current usage in worktree.c is a bit too familiar with the
current internal implementation of run_command(). Reinitializing the
child_process struct or using a separate one would be a good cleanup.
> -- >8 --
> Subject: [PATCH] worktree: fix leak in check_clean_worktree()
>
> We allocate a child_env strvec but never free its memory. Instead, let's
> just use the strvec that our child_process struct provides, which is
> cleaned up automatically when we run the command.
>
> And while we're moving the initialization of the child_process around,
> let's switch it to use the official init function (zero-initializing it
> works OK, since strvec is happy enough with that, but it sets a bad
> example).
The various memset()'s in worktree.c seem to have been inherited (and
multiplied) from Duy's original "git checkout --to" implementation
(which later became the basis for "git worktree add" after which it
mutated significantly), and "git checkout --to" predates introduction
of child_process_init().
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -924,7 +924,6 @@ static int move_worktree(int ac, const char **av, const char *prefix)
> - struct strvec child_env = STRVEC_INIT;
> @@ -935,15 +934,14 @@ static void check_clean_worktree(struct worktree *wt,
> - strvec_pushf(&child_env, "%s=%s/.git",
> + child_process_init(&cp);
> + strvec_pushf(&cp.env_array, "%s=%s/.git",
> GIT_DIR_ENVIRONMENT, wt->path);
> - strvec_pushf(&child_env, "%s=%s",
> + strvec_pushf(&cp.env_array, "%s=%s",
> GIT_WORK_TREE_ENVIRONMENT, wt->path);
> - memset(&cp, 0, sizeof(cp));
> - cp.env = child_env.v;
Looks good to me. For what it's worth:
Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>
^ permalink raw reply
* Re: [PATCH v3 11/11] doc: add corrected commit date info
From: Abhishek Kumar @ 2020-08-27 6:39 UTC (permalink / raw)
To: Jakub Narębski; +Cc: abhishekkumar8222, git, gitgitgadget, me, stolee
In-Reply-To: <85y2m6fhkm.fsf@gmail.com>
On Sun, Aug 23, 2020 at 12:20:57AM +0200, Jakub Narębski wrote:
> Hello,
>
> "Abhishek Kumar via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Abhishek Kumar <abhishekkumar8222@gmail.com>
> >
> > With generation data chunk and corrected commit dates implemented, let's
> > update the technical documentation for commit-graph.
> >
> > Signed-off-by: Abhishek Kumar <abhishekkumar8222@gmail.com>
>
> All right.
>
> > ---
> > .../technical/commit-graph-format.txt | 12 ++---
> > Documentation/technical/commit-graph.txt | 45 ++++++++++++-------
> > 2 files changed, 36 insertions(+), 21 deletions(-)
> >
> > diff --git a/Documentation/technical/commit-graph-format.txt b/Documentation/technical/commit-graph-format.txt
> > index 440541045d..71c43884ec 100644
> > --- a/Documentation/technical/commit-graph-format.txt
> > +++ b/Documentation/technical/commit-graph-format.txt
> > @@ -4,11 +4,7 @@ Git commit graph format
> > The Git commit graph stores a list of commit OIDs and some associated
> > metadata, including:
> >
> > -- The generation number of the commit. Commits with no parents have
> > - generation number 1; commits with parents have generation number
> > - one more than the maximum generation number of its parents. We
> > - reserve zero as special, and can be used to mark a generation
> > - number invalid or as "not computed".
> > +- The generation number of the commit.
>
> All right, that was duplicated information. Now that we need to talk
> about two of them, it would not make sense to duplicate that.
>
> >
> > - The root tree OID.
> >
> > @@ -88,6 +84,12 @@ CHUNK DATA:
>
> Shouldn't we also replace 'generation number' occurences in description
> of the Commit Data (CDAT) chunk with either 'topological level' or
> 'generation number v1'?
Yes, we should.
>
> > 2 bits of the lowest byte, storing the 33rd and 34th bit of the
> > commit time.
> >
> > + Generation Data (ID: {'G', 'D', 'A', 'T' }) (N * 4 bytes) [Optional]
>
> It is not exactly 'optional', as it implies that we need to turn it on
> (or that we can turn it off). It is more 'conditional', as it can be
> not present due to outside influences (mixed-version environment).
>
> > + * This list of 4-byte values store corrected commit date offsets for the
> > + commits, arranged in the same order as commit data chunk.
>
> I have just realized purely theoretical, but possible, problem with
> storing non-monotinic generation number related values like corrected
> commit date offset in constrained space. There are problems with
> clamping them.
>
> Say that somewhere in the ancestry chain there is a commit A with commit
> date far in the future by mistake, for example 2120-08-22; it is
> important for that date to be not able to be represented using uint32_t.
> Say that a later descendant commit B is malformed, and has committer
> date of 0, that is 1970-01-01. This means that the corrected commit date
> for B must be larger than 2120-08-22 - which for this commit means that
> corrected commit date offset do not fit in 32 bits, and must be clamped
> (replaced) with GENERATION_NUMBER_V2_OFFSET_MAX.
>
> Say that we have commit C that is child of B, and it has correct commit
> date. Because of mistake in commit A, it has corrected commit date of
> more than 2120-08-22 (corrected commit date degenerated into topological
> level plus constant).
>
> Now C can reach B, and B can reach A. However, if we recover corrected
> commit date of B out of its date=0 and offset=GENERATION_NUMBER_V2_OFFSET_MAX
> we get a number that is smaller than correct corrected commit date. We
> will have
>
> gen(A) > date(B) + offset(B) < gen(C)
>
> Which breaks reachability condition guarantee.
>
> If instead we use GENERATION_NUMBER_V2_MAX for commits with clamped
> corrected commit date, that is offset=GENERATION_NUMBER_V2_OFFSET_MAX,
> we would get
>
> gen(A) < GENERATION_NUMBER_V2_MAX > gen(C)
>
> And again reachability condition is broken.
>
> This is a very contrived but possible example. This shouldn't happen,
> but ufortunately it can happen.
>
Yes, that's very unfortunate.
Here's a much simpler example:
A commit P has an reasonable commit date (i.e. after release of Git to
present) D and has a child commit C with committer date 0. Now, the
corrected commiter date of C would D + 1 and the offset would be same too,
as the committer date is zero. This overflows as reasonable dates are of
the order 2 ^ 34.
>
> The question is how to deal with this issue. Ignore it as unlikely?
> Switch to storing corrected commit date, which is monotonic, so if there
> is commit with GENERATION_NUMBER_V2_MAX, then subsequent descendant
> commits will also have GENERATION_NUMBER_V2_MAX -- and pay with up to 7%
> larger commit-graph file?
>
To be honest, I would prefer storing corrected committer dates over
storing offsets.
While it is 7% of the size of commit-graph file, it is also *only* around
~3.5 MB for a repository of the size of linux kernel (and IIRC
correctly, the Windows repo has ~2M commits, it amounts to ~8 MB).
Minimizing space and memory requirements are a top priority, but
shouldn't making sure our program is correct and efficient to be a
greater priority?
I would love to hear your and Dr. Stolee's opinions on this.
> > + * This list can be later modified to store future generation number related
> > + data.
>
> How can it be later modified? There is no header, no version number.
> How would we add another generation number data?
>
We could modify the graph version in future. Here's how I think it would
work:
Graph Version 1, No GDAT -> Topological level
Graph Version 2, GDAT -> Corrected committer dates
Graph Version 3, GDAT -> Generation number v3
and so on.
Of course, we do not have to update generation number definition for
each graph version.
However, my statement could still be wrong for things that we do not
foresee (similar to how we missed the hard die on different graph version),
so I am removing the statement.
> > +
> > Extra Edge List (ID: {'E', 'D', 'G', 'E'}) [Optional]
> > This list of 4-byte values store the second through nth parents for
> > all octopus merges. The second parent value in the commit data stores
> > diff --git a/Documentation/technical/commit-graph.txt b/Documentation/technical/commit-graph.txt
> > index 808fa30b99..f27145328c 100644
> > --- a/Documentation/technical/commit-graph.txt
> > +++ b/Documentation/technical/commit-graph.txt
> > @@ -38,14 +38,27 @@ A consumer may load the following info for a commit from the graph:
> >
> > Values 1-4 satisfy the requirements of parse_commit_gently().
> >
> > -Define the "generation number" of a commit recursively as follows:
> > +There are two definitions of generation number:
> > +1. Corrected committer dates
> > +2. Topological levels
>
> Should we add versioning info, that is:
>
> +1. Corrected committer dates (generation number v2)
> +2. Topological levels (generation number v1)
>
Yes, added.
> > +
> > +Define "corrected committer date" of a commit recursively as follows:
> > +
> > + * A commit with no parents (a root commit) has corrected committer date
> > + equal to its committer date.
> > +
> > + * A commit with at least one parent has corrected committer date equal to
> > + the maximum of its commiter date and one more than the largest corrected
> > + committer date among its parents.
> > +
> > +Define the "topological level" of a commit recursively as follows:
> >
> > * A commit with no parents (a root commit) has generation number one.
>
> Shouldn't this be
>
> * A commit with no parents (a root commit) has topological level of one.
>
Thanks, fixed!
> >
> > - * A commit with at least one parent has generation number one more than
> > - the largest generation number among its parents.
> > + * A commit with at least one parent has topological level one more than
> > + the largest topological level among its parents.
> >
> > -Equivalently, the generation number of a commit A is one more than the
> > +Equivalently, the topological level of a commit A is one more than the
> > length of a longest path from A to a root commit. The recursive definition
> > is easier to use for computation and observing the following property:
>
> We should probably explicitly state that the property state applies to
> both versions of generation number, not only to topological level.
>
> >
> > @@ -67,17 +80,12 @@ numbers, the general heuristic is the following:
> > If A and B are commits with commit time X and Y, respectively, and
> > X < Y, then A _probably_ cannot reach B.
> >
> > -This heuristic is currently used whenever the computation is allowed to
> > -violate topological relationships due to clock skew (such as "git log"
> > -with default order), but is not used when the topological order is
> > -required (such as merge base calculations, "git log --graph").
> > -
>
> To be overly pedantic, this heuristic is still used, but now in much
> more rare case. In addition to what is stated above, at least one layer
> in the split commit-graph chain must have been generated by "Old" Git,
> for the date heuristic to be used.
>
> But that might be unnecessary level of detail.
>
> > In practice, we expect some commits to be created recently and not stored
> > in the commit graph. We can treat these commits as having "infinite"
> > generation number and walk until reaching commits with known generation
> > number.
> >
> > -We use the macro GENERATION_NUMBER_INFINITY = 0xFFFFFFFF to mark commits not
> > +We use the macro GENERATION_NUMBER_INFINITY to mark commits not
>
> All right.
>
> > in the commit-graph file. If a commit-graph file was written by a version
> > of Git that did not compute generation numbers, then those commits will
> > have generation number represented by the macro GENERATION_NUMBER_ZERO = 0.
> > @@ -93,12 +101,11 @@ fully-computed generation numbers. Using strict inequality may result in
> > walking a few extra commits, but the simplicity in dealing with commits
> > with generation number *_INFINITY or *_ZERO is valuable.
> >
> > -We use the macro GENERATION_NUMBER_MAX = 0x3FFFFFFF to for commits whose
> > -generation numbers are computed to be at least this value. We limit at
> > -this value since it is the largest value that can be stored in the
> > -commit-graph file using the 30 bits available to generation numbers. This
> > -presents another case where a commit can have generation number equal to
> > -that of a parent.
> > +We use the macro GENERATION_NUMBER_MAX for commits whose generation numbers
> > +are computed to be at least this value. We limit at this value since it is
> > +the largest value that can be stored in the commit-graph file using the
> > +available to generation numbers. This presents another case where a
> > +commit can have generation number equal to that of a parent.
>
> All right, though it could have been done without re-wrapping, so that
> only first line would be marked as changed.
>
> As I wrote, there is theoretical problem with this for offsets.
>
> >
> > Design Details
> > --------------
> > @@ -267,6 +274,12 @@ The merge strategy values (2 for the size multiple, 64,000 for the maximum
> > number of commits) could be extracted into config settings for full
> > flexibility.
> >
> > +We also merge commit-graph chains when we try to write a commit graph with
> > +two different generation number definitions as they cannot be compared directly.
> > +We overwrite the existing chain and create a commit-graph with the newer or more
> > +efficient defintion. For example, overwriting topological levels commit graph
> > +chain to create a corrected commit dates commit graph chain.
> > +
>
> This is more complicated than that.
>
> I think we should explicitly state that Git ensures that in split
> commit-graph chain, if there are layers without the GDAT chunk (that
> force Git to use topological levels for generation numbers), then they
> are top layers. So if there is commit-graph file created by "Old" Git,
> then when addig new layer it would also be GDAT-less.
>
> Now how to write this...
Thinking about this, I feel creating a new section called "Handling
Mixed Generation Number Chains" made more sense:
## Handling Mixed Generation Number Chains
With the introduction of generation number v2 and generation data chunk,
the following scenario is possible:
1. "New" Git writes a commit-graph with a GDAT chunk.
2. "Old" Git writes a split commit-graph on top without a GDAT chunk.
The commits in the lower layer will be interpreted as having very large
generation values (commit date plus offset) compared to the generation
numbers in the top layer (toplogical level). This violates the
expectation that the generation of a parent is strictly smaller than the
generation of a child. In such cases, we revert to using topological
levels for all layers to maintain backwards compatability.
When writing a new layer in split commit-graph, we write a GDAT chunk
only if the topmost layer has a GDAT chunk. This guarantees that if a
lyer has GDAT chunk, all lower layers must have a GDAT chunk as well.
Rewriting layers follows similar approach: if the topmost layer below
set of layers being rewriteen (in the split commit-graph chain) exists,
and it does not contain GDAT chunk, then the result of rewrite does not
have GDAT chunks either.
>
> > ## Deleting graph-{hash} files
> >
> > After a new tip file is written, some `graph-{hash}` files may no longer
>
> Best,
> --
> Jakub Narębski
Thanks
- Abhishek
^ permalink raw reply
* feature request - add --only-author option to git push
From: Toni Brkic @ 2020-08-27 7:47 UTC (permalink / raw)
To: git
Sorry if this mail list is not used for feature requests/discussions.
This was the best list I found.
Let me know if should post it somewhere else.
I would like to be able to configure git so that when doing git push
git checks that I am author of
all patches that are being pushed. If I am not authour it should not do push.
The reason for this is that a common mistake that happens when working
with gerrit (at least for me)
Person A has uploaded patch1
I need patch1 to continue development and cherry pick it to my repo.
Person A uploads new version of patch1
I have finished my patch and push to gerrit. What then happens is that
I have an older version of patch1 and thus overwrite the new version
by Person A
Maybe there is some way already to do this, but I could not find
anything when searching.
BR,
Toni
^ permalink raw reply
* [PATCH 0/5] add "git worktree repair" command
From: Eric Sunshine @ 2020-08-27 8:21 UTC (permalink / raw)
To: git; +Cc: Henré Botha, Jeff King, Eric Sunshine
The purpose of this patch series is twofold.
First, it adds a "git worktree repair" command to help users recover
from situations in which worktree administrative files can become
outdated or corrupted due to external factors. Simple examples include
(1) the user moving a worktree manually rather than via "git worktree
move" which causes the main worktree (or bare repository) to lose track
of the worktree, and (2) moving the main worktree (or bare repository)
which results in linked worktrees being unable to find the repository.
Second, it fixes two bugs with "git init --separate-git-dir" when linked
worktrees are involved (reported by [1]), both of which cause worktree
administrative files to become outdated or corrupted.
An intentional side-effect of the --separate-git-dir fix in patch [5/5]
is that it closes an additional loophole not covered by [2] which made
it illegal to use --separate-git-dir in conjunction with bare
repositories. Peff is Cc:'d because he commented on that thread.
[1]: https://lore.kernel.org/git/CAHbriek39i9NSHRw6DZm0dftk-GkeAYR74c0xyss0vbeDHu1Hw@mail.gmail.com/T/
[2]: https://lore.kernel.org/git/20200809225316.19503-1-sunshine@sunshineco.com/T/
Eric Sunshine (5):
worktree: add skeleton "repair" command
worktree: teach "repair" to fix worktree back-links to main worktree
worktree: teach "repair" to fix outgoing links to worktrees
init: teach --separate-git-dir to repair linked worktrees
init: make --separate-git-dir work from within linked worktree
Documentation/git-worktree.txt | 26 ++++-
builtin/init-db.c | 26 +++++
builtin/worktree.c | 29 ++++++
t/t0001-init.sh | 28 ++++++
t/t2406-worktree-repair.sh | 169 +++++++++++++++++++++++++++++++++
worktree.c | 127 +++++++++++++++++++++++++
worktree.h | 22 +++++
7 files changed, 425 insertions(+), 2 deletions(-)
create mode 100755 t/t2406-worktree-repair.sh
--
2.28.0.461.g40977abb40
^ permalink raw reply
* [PATCH 1/5] worktree: add skeleton "repair" command
From: Eric Sunshine @ 2020-08-27 8:21 UTC (permalink / raw)
To: git; +Cc: Henré Botha, Jeff King, Eric Sunshine
In-Reply-To: <20200827082129.56149-1-sunshine@sunshineco.com>
Worktree administrative files can become corrupted or outdated due to
external factors. Although, it is often possible to recover from such
situations by hand-tweaking these files, doing so requires intimate
knowledge of worktree internals. While information necessary to make
such repairs manually can be obtained from git-worktree.txt and
gitrepository-layout.txt, we can assist users more directly by teaching
git-worktree how to repair its administrative files itself (at least to
some extent). Therefore, add a "git worktree repair" command which
attempts to correct common problems which may arise due to factors
beyond Git's control.
At this stage, the "repair" command is a mere skeleton; subsequent
commits will flesh out the functionality.
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
Documentation/git-worktree.txt | 6 ++++++
builtin/worktree.c | 15 +++++++++++++++
t/t2406-worktree-repair.sh | 11 +++++++++++
3 files changed, 32 insertions(+)
create mode 100755 t/t2406-worktree-repair.sh
diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 6ee6ec7982..ae432d39a8 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -15,6 +15,7 @@ SYNOPSIS
'git worktree move' <worktree> <new-path>
'git worktree prune' [-n] [-v] [--expire <expire>]
'git worktree remove' [-f] <worktree>
+'git worktree repair'
'git worktree unlock' <worktree>
DESCRIPTION
@@ -110,6 +111,11 @@ and no modification in tracked files) can be removed. Unclean working
trees or ones with submodules can be removed with `--force`. The main
working tree cannot be removed.
+repair::
+
+Repair working tree administrative files, if possible, if they have
+become corrupted or outdated due to external factors.
+
unlock::
Unlock a working tree, allowing it to be pruned, moved or deleted.
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 378f332b5d..88af412d4f 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -1030,6 +1030,19 @@ static int remove_worktree(int ac, const char **av, const char *prefix)
return ret;
}
+static int repair(int ac, const char **av, const char *prefix)
+{
+ struct option options[] = {
+ OPT_END()
+ };
+ int rc = 0;
+
+ ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
+ if (ac)
+ usage_with_options(worktree_usage, options);
+ return rc;
+}
+
int cmd_worktree(int ac, const char **av, const char *prefix)
{
struct option options[] = {
@@ -1056,5 +1069,7 @@ int cmd_worktree(int ac, const char **av, const char *prefix)
return move_worktree(ac - 1, av + 1, prefix);
if (!strcmp(av[1], "remove"))
return remove_worktree(ac - 1, av + 1, prefix);
+ if (!strcmp(av[1], "repair"))
+ return repair(ac - 1, av + 1, prefix);
usage_with_options(worktree_usage, options);
}
diff --git a/t/t2406-worktree-repair.sh b/t/t2406-worktree-repair.sh
new file mode 100755
index 0000000000..cc679e1a21
--- /dev/null
+++ b/t/t2406-worktree-repair.sh
@@ -0,0 +1,11 @@
+#!/bin/sh
+
+test_description='test git worktree repair'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+ test_commit init
+'
+
+test_done
--
2.28.0.461.g40977abb40
^ permalink raw reply related
* [PATCH 5/5] init: make --separate-git-dir work from within linked worktree
From: Eric Sunshine @ 2020-08-27 8:21 UTC (permalink / raw)
To: git; +Cc: Henré Botha, Jeff King, Eric Sunshine
In-Reply-To: <20200827082129.56149-1-sunshine@sunshineco.com>
The intention of `git init --separate-work-dir=<path>` is to move the
.git/ directory to a location outside of the main worktree. When used
within a linked worktree, however, rather than moving the .git/
directory as intended, it instead incorrectly moves the worktree's
.git/worktrees/<id> directory to <path>, thus disconnecting the linked
worktree from its parent repository and breaking the worktree in the
process since its local .git file no longer points at a location at
which it can find the object database. Fix this broken behavior.
An intentional side-effect of this change is that it also closes a
loophole not caught by ccf236a23a (init: disallow --separate-git-dir
with bare repository, 2020-08-09) in which the check to prevent
--separate-git-dir being used in conjunction with a bare repository was
unable to detect the invalid combination when invoked from within a
linked worktree. Therefore, add a test to verify that this loophole is
closed, as well.
Reported-by: Henré Botha <henrebotha@gmail.com>
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
builtin/init-db.c | 24 ++++++++++++++++++++++++
t/t0001-init.sh | 21 +++++++++++++++++++--
2 files changed, 43 insertions(+), 2 deletions(-)
diff --git a/builtin/init-db.c b/builtin/init-db.c
index 7b915d88ab..6a94d20a2e 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -642,6 +642,30 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
if (!git_dir)
git_dir = DEFAULT_GIT_DIR_ENVIRONMENT;
+ /*
+ * When --separate-git-dir is used inside a linked worktree, take
+ * care to ensure that the common .git/ directory is relocated, not
+ * the worktree-specific .git/worktrees/<id>/ directory.
+ */
+ if (real_git_dir) {
+ int err;
+ const char *p;
+ struct strbuf sb = STRBUF_INIT;
+
+ p = read_gitfile_gently(git_dir, &err);
+ if (p && get_common_dir(&sb, p)) {
+ struct strbuf mainwt = STRBUF_INIT;
+
+ strbuf_addbuf(&mainwt, &sb);
+ strbuf_strip_suffix(&mainwt, "/.git");
+ if (chdir(mainwt.buf) < 0)
+ die_errno(_("cannot chdir to %s"), mainwt.buf);
+ strbuf_release(&mainwt);
+ git_dir = strbuf_detach(&sb, 0);
+ }
+ strbuf_release(&sb);
+ }
+
if (is_bare_repository_cfg < 0)
is_bare_repository_cfg = guess_repository_type(git_dir);
diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index e489eb4ddb..2f7c3dcd0f 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -329,6 +329,15 @@ test_expect_success 'implicit bare & --separate-git-dir incompatible' '
test_i18ngrep "incompatible" err
'
+test_expect_success 'bare & --separate-git-dir incompatible within worktree' '
+ test_when_finished "rm -rf bare.git linkwt seprepo" &&
+ test_commit gumby &&
+ git clone --bare . bare.git &&
+ git -C bare.git worktree add --detach ../linkwt &&
+ test_must_fail git -C linkwt init --separate-git-dir seprepo 2>err &&
+ test_i18ngrep "incompatible" err
+'
+
test_lazy_prereq GETCWD_IGNORES_PERMS '
base=GETCWD_TEST_BASE_DIR &&
mkdir -p $base/dir &&
@@ -405,15 +414,23 @@ test_expect_success SYMLINKS 're-init to move gitdir symlink' '
test_path_is_dir realgitdir/refs
'
-test_expect_success 're-init to move gitdir with linked worktrees' '
+sep_git_dir_worktree () {
test_when_finished "rm -rf mainwt linkwt seprepo" &&
git init mainwt &&
test_commit -C mainwt gumby &&
git -C mainwt worktree add --detach ../linkwt &&
- git -C mainwt init --separate-git-dir ../seprepo &&
+ git -C "$1" init --separate-git-dir ../seprepo &&
git -C mainwt rev-parse --git-common-dir >expect &&
git -C linkwt rev-parse --git-common-dir >actual &&
test_cmp expect actual
+}
+
+test_expect_success 're-init to move gitdir with linked worktrees' '
+ sep_git_dir_worktree mainwt
+'
+
+test_expect_success 're-init to move gitdir within linked worktree' '
+ sep_git_dir_worktree linkwt
'
test_expect_success MINGW '.git hidden' '
--
2.28.0.461.g40977abb40
^ permalink raw reply related
* [PATCH 4/5] init: teach --separate-git-dir to repair linked worktrees
From: Eric Sunshine @ 2020-08-27 8:21 UTC (permalink / raw)
To: git; +Cc: Henré Botha, Jeff King, Eric Sunshine
In-Reply-To: <20200827082129.56149-1-sunshine@sunshineco.com>
A linked worktree's .git file is a "gitlink" pointing at the
.git/worktrees/<id> directory within the repository. When `git init
--separate-git-dir=<path>` is used on an existing repository to relocate
the repository's .git/ directory to a different location, it neglects to
update the .git files of linked worktrees, thus breaking the worktrees
by making it impossible for them to locate the repository. Fix this by
teaching --separate-git-dir to repair the .git file of each linked
worktree to point at the new repository location.
Reported-by: Henré Botha <henrebotha@gmail.com>
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
builtin/init-db.c | 2 ++
t/t0001-init.sh | 11 +++++++++++
2 files changed, 13 insertions(+)
diff --git a/builtin/init-db.c b/builtin/init-db.c
index bbc9bc78f9..7b915d88ab 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -9,6 +9,7 @@
#include "builtin.h"
#include "exec-cmd.h"
#include "parse-options.h"
+#include "worktree.h"
#ifndef DEFAULT_GIT_TEMPLATE_DIR
#define DEFAULT_GIT_TEMPLATE_DIR "/usr/share/git-core/templates"
@@ -364,6 +365,7 @@ static void separate_git_dir(const char *git_dir, const char *git_link)
if (rename(src, git_dir))
die_errno(_("unable to move %s to %s"), src, git_dir);
+ repair_worktrees(NULL, NULL);
}
write_file(git_link, "gitdir: %s", git_dir);
diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index 50222a10c5..e489eb4ddb 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -405,6 +405,17 @@ test_expect_success SYMLINKS 're-init to move gitdir symlink' '
test_path_is_dir realgitdir/refs
'
+test_expect_success 're-init to move gitdir with linked worktrees' '
+ test_when_finished "rm -rf mainwt linkwt seprepo" &&
+ git init mainwt &&
+ test_commit -C mainwt gumby &&
+ git -C mainwt worktree add --detach ../linkwt &&
+ git -C mainwt init --separate-git-dir ../seprepo &&
+ git -C mainwt rev-parse --git-common-dir >expect &&
+ git -C linkwt rev-parse --git-common-dir >actual &&
+ test_cmp expect actual
+'
+
test_expect_success MINGW '.git hidden' '
rm -rf newdir &&
(
--
2.28.0.461.g40977abb40
^ permalink raw reply related
* [PATCH 2/5] worktree: teach "repair" to fix worktree back-links to main worktree
From: Eric Sunshine @ 2020-08-27 8:21 UTC (permalink / raw)
To: git; +Cc: Henré Botha, Jeff King, Eric Sunshine
In-Reply-To: <20200827082129.56149-1-sunshine@sunshineco.com>
The .git file in a linked worktree is a "gitlink" which points back to
the .git/worktrees/<id> entry in the main worktree or bare repository.
If a worktree's .git file is deleted or becomes corrupted or outdated,
then the linked worktree won't know how to find the repository or any of
its own administrative files (such as 'index', 'HEAD', etc.). An easy
way for the .git file to become outdated is for the user to move the
main worktree or bare repository. Although it is possible to manually
update each linked worktree's .git file to reflect the new repository
location, doing so requires a level of knowledge about worktree
internals beyond what a user should be expected to know offhand.
Therefore, teach "git worktree repair" how to repair broken or outdated
worktree .git files automatically. (For this to work, the command must
be invoked from within the main worktree or bare repository, or from
within a worktree which has not become disconnected from the repository
-- such as one which was created after the repository was moved.)
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
Documentation/git-worktree.txt | 10 ++++-
builtin/worktree.c | 11 ++++++
t/t2406-worktree-repair.sh | 72 ++++++++++++++++++++++++++++++++++
worktree.c | 53 +++++++++++++++++++++++++
worktree.h | 11 ++++++
5 files changed, 156 insertions(+), 1 deletion(-)
diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index ae432d39a8..acb0ea1c2e 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -98,7 +98,10 @@ with `--reason`.
move::
Move a working tree to a new location. Note that the main working tree
-or linked working trees containing submodules cannot be moved.
+or linked working trees containing submodules cannot be moved with this
+command. (The `git worktree repair` command, however, can reestablish
+the connection with linked working trees if you move the main working
+tree manually.)
prune::
@@ -115,6 +118,11 @@ repair::
Repair working tree administrative files, if possible, if they have
become corrupted or outdated due to external factors.
++
+For instance, if the main working tree (or bare repository) is moved,
+linked working trees will be unable to locate it. Running `repair` in
+the recently-moved main working tree will reestablish the connection
+from linked working trees back to the main working tree.
unlock::
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 88af412d4f..62e33eb7f5 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -1030,6 +1030,16 @@ static int remove_worktree(int ac, const char **av, const char *prefix)
return ret;
}
+static void repair_cb(int iserr, const char *path, const char *msg, void *cb_data)
+{
+ if (!iserr)
+ printf_ln(_("repair: %s: %s"), msg, path);
+ else {
+ fprintf_ln(stderr, _("error: %s: %s"), msg, path);
+ *(int *)cb_data = 1;
+ }
+}
+
static int repair(int ac, const char **av, const char *prefix)
{
struct option options[] = {
@@ -1040,6 +1050,7 @@ static int repair(int ac, const char **av, const char *prefix)
ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
if (ac)
usage_with_options(worktree_usage, options);
+ repair_worktrees(repair_cb, &rc);
return rc;
}
diff --git a/t/t2406-worktree-repair.sh b/t/t2406-worktree-repair.sh
index cc679e1a21..9379a63130 100755
--- a/t/t2406-worktree-repair.sh
+++ b/t/t2406-worktree-repair.sh
@@ -8,4 +8,76 @@ test_expect_success setup '
test_commit init
'
+test_expect_success 'skip missing worktree' '
+ test_when_finished "git worktree prune" &&
+ git worktree add --detach missing &&
+ rm -rf missing &&
+ git worktree repair >out 2>err &&
+ test_must_be_empty out &&
+ test_must_be_empty err
+'
+
+test_expect_success "don't clobber .git repo" '
+ test_when_finished "rm -rf repo && git worktree prune" &&
+ git worktree add --detach repo &&
+ rm -rf repo &&
+ test_create_repo repo &&
+ test_must_fail git worktree repair >out 2>err &&
+ test_must_be_empty out &&
+ test_i18ngrep ".git is not a file" err
+'
+
+test_corrupt_gitlink () {
+ butcher=$1 &&
+ problem=$2 &&
+ repairdir=${3:-.} &&
+ test_when_finished 'rm -rf corrupt && git worktree prune' &&
+ git worktree add --detach corrupt &&
+ git -C corrupt rev-parse --absolute-git-dir >expect &&
+ eval "$butcher" &&
+ git -C "$repairdir" worktree repair >out 2>err &&
+ test_i18ngrep "$problem" out &&
+ test_must_be_empty err &&
+ git -C corrupt rev-parse --absolute-git-dir >actual &&
+ test_cmp expect actual
+}
+
+test_expect_success 'repair missing .git file' '
+ test_corrupt_gitlink "rm -f corrupt/.git" ".git link broken"
+'
+
+test_expect_success 'repair bogus .git file' '
+ test_corrupt_gitlink "echo \"gitdir: /nowhere\" >corrupt/.git" \
+ ".git link broken"
+'
+
+test_expect_success 'repair incorrect .git file' '
+ test_when_finished "rm -rf other && git worktree prune" &&
+ test_create_repo other &&
+ other=$(git -C other rev-parse --absolute-git-dir) &&
+ test_corrupt_gitlink "echo \"gitdir: $other\" >corrupt/.git" \
+ ".git link incorrect"
+'
+
+test_expect_success 'repair .git file from main/.git' '
+ test_corrupt_gitlink "rm -f corrupt/.git" ".git link broken" .git
+'
+
+test_expect_success 'repair .git file from linked worktree' '
+ test_when_finished "rm -rf other && git worktree prune" &&
+ git worktree add --detach other &&
+ test_corrupt_gitlink "rm -f corrupt/.git" ".git link broken" other
+'
+
+test_expect_success 'repair .git file from bare.git' '
+ test_when_finished "rm -rf bare.git corrupt && git worktree prune" &&
+ git clone --bare . bare.git &&
+ git -C bare.git worktree add --detach ../corrupt &&
+ git -C corrupt rev-parse --absolute-git-dir >expect &&
+ rm -f corrupt/.git &&
+ git -C bare.git worktree repair &&
+ git -C corrupt rev-parse --absolute-git-dir >actual &&
+ test_cmp expect actual
+'
+
test_done
diff --git a/worktree.c b/worktree.c
index 62217b4a6b..029ce91fdf 100644
--- a/worktree.c
+++ b/worktree.c
@@ -571,3 +571,56 @@ int other_head_refs(each_ref_fn fn, void *cb_data)
free_worktrees(worktrees);
return ret;
}
+
+/*
+ * Repair worktree's /path/to/worktree/.git link if missing, corrupt, or not
+ * pointing at <repo>/worktrees/<id>.
+ */
+static void repair_dotgit(struct worktree *wt,
+ worktree_repair_cb *cb, void *cb_data)
+{
+ struct strbuf dotgit = STRBUF_INIT;
+ struct strbuf repo = STRBUF_INIT;
+ char *backlink;
+ const char *repair = NULL;
+ int err;
+
+ /* missing worktree can't be repaired */
+ if (!file_exists(wt->path))
+ return;
+
+ strbuf_realpath(&repo, git_common_path("worktrees/%s", wt->id), 1);
+ strbuf_addf(&dotgit, "%s/.git", wt->path);
+ backlink = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &err));
+
+ if (err == READ_GITFILE_ERR_NOT_A_FILE)
+ cb(1, wt->path, _(".git is not a file"), cb_data);
+ else if (err)
+ repair = _(".git link broken");
+ else if (fspathcmp(backlink, repo.buf))
+ repair = _(".git link incorrect");
+
+ if (repair) {
+ cb(0, wt->path, repair, cb_data);
+ write_file(dotgit.buf, "gitdir: %s", repo.buf);
+ }
+
+ free(backlink);
+ strbuf_release(&repo);
+ strbuf_release(&dotgit);
+}
+
+static void repair_noop_cb(int iserr, const char *path, const char *msg,
+ void *cb_data) {}
+
+void repair_worktrees(worktree_repair_cb *cb, void *cb_data)
+{
+ struct worktree **worktrees = get_worktrees();
+ struct worktree **wt = worktrees + 1; /* +1 skips main worktree */
+
+ if (!cb)
+ cb = repair_noop_cb;
+ for (; *wt; wt++)
+ repair_dotgit(*wt, cb, cb_data);
+ free_worktrees(worktrees);
+}
diff --git a/worktree.h b/worktree.h
index 516744c433..7d085c7b91 100644
--- a/worktree.h
+++ b/worktree.h
@@ -89,6 +89,17 @@ int validate_worktree(const struct worktree *wt,
void update_worktree_location(struct worktree *wt,
const char *path_);
+typedef void worktree_repair_cb(int iserr, const char *path, const char *msg,
+ void *cb_data);
+
+/*
+ * Visit each registered linked worktree and repair corruptions. For each
+ * repair made or error encountered while attempting a repair, the callback, if
+ * non-NULL, is called with the path of the worktree and a description of the
+ * repair or error, along with the callback user-data.
+ */
+void repair_worktrees(worktree_repair_cb *, void *cb_data);
+
/*
* Free up the memory for worktree(s)
*/
--
2.28.0.461.g40977abb40
^ permalink raw reply related
* [PATCH 3/5] worktree: teach "repair" to fix outgoing links to worktrees
From: Eric Sunshine @ 2020-08-27 8:21 UTC (permalink / raw)
To: git; +Cc: Henré Botha, Jeff King, Eric Sunshine
In-Reply-To: <20200827082129.56149-1-sunshine@sunshineco.com>
The .git/worktrees/<id>/gitdir file points at the location of a linked
worktree's .git file. Its content must be of the form
/path/to/worktree/.git (from which the location of the worktree itself
can be derived by stripping the "/.git" suffix). If the gitdir file is
deleted or becomes corrupted or outdated, then Git will be unable to
find the linked worktree. An easy way for the gitdir file to become
outdated is for the user to move the worktree manually (without using
"git worktree move"). Although it is possible to manually update the
gitdir file to reflect the new linked worktree location, doing so
requires a level of knowledge about worktree internals beyond what a
user should be expected to know offhand.
Therefore, teach "git worktree repair" how to repair broken or outdated
.git/worktrees/<id>/gitdir files automatically. (For this to work, the
command must either be invoked from within the worktree whose gitdir
file requires repair, or from within the main or any linked worktree by
providing the path of the broken worktree as an argument to "git
worktree repair".)
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
Documentation/git-worktree.txt | 14 ++++--
builtin/worktree.c | 7 ++-
t/t2406-worktree-repair.sh | 86 ++++++++++++++++++++++++++++++++++
worktree.c | 74 +++++++++++++++++++++++++++++
worktree.h | 11 +++++
5 files changed, 187 insertions(+), 5 deletions(-)
diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index acb0ea1c2e..a43a0af0af 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -15,7 +15,7 @@ SYNOPSIS
'git worktree move' <worktree> <new-path>
'git worktree prune' [-n] [-v] [--expire <expire>]
'git worktree remove' [-f] <worktree>
-'git worktree repair'
+'git worktree repair' [<path>...]
'git worktree unlock' <worktree>
DESCRIPTION
@@ -114,7 +114,7 @@ and no modification in tracked files) can be removed. Unclean working
trees or ones with submodules can be removed with `--force`. The main
working tree cannot be removed.
-repair::
+repair [<path>...]::
Repair working tree administrative files, if possible, if they have
become corrupted or outdated due to external factors.
@@ -123,6 +123,13 @@ For instance, if the main working tree (or bare repository) is moved,
linked working trees will be unable to locate it. Running `repair` in
the recently-moved main working tree will reestablish the connection
from linked working trees back to the main working tree.
++
+Similarly, if a linked working tree is moved without using `git worktree
+move`, the main working tree (or bare repository) will be unable to
+locate it. Running `repair` within the recently-moved working tree will
+reestablish the connection. If multiple linked working trees are moved,
+running `repair` from any working tree with each tree's new `<path>` as
+an argument, will reestablish the connection to all the specified paths.
unlock::
@@ -317,7 +324,8 @@ in the entry's directory. For example, if a linked working tree is moved
to `/newpath/test-next` and its `.git` file points to
`/path/main/.git/worktrees/test-next`, then update
`/path/main/.git/worktrees/test-next/gitdir` to reference `/newpath/test-next`
-instead.
+instead. Better yet, run `git worktree repair` to reestablish the connection
+automatically.
To prevent a `$GIT_DIR/worktrees` entry from being pruned (which
can be useful in some situations, such as when the
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 62e33eb7f5..19bbc246ad 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -1042,15 +1042,18 @@ static void repair_cb(int iserr, const char *path, const char *msg, void *cb_dat
static int repair(int ac, const char **av, const char *prefix)
{
+ const char **p;
+ const char *self[] = { ".", NULL };
struct option options[] = {
OPT_END()
};
int rc = 0;
ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
- if (ac)
- usage_with_options(worktree_usage, options);
repair_worktrees(repair_cb, &rc);
+ p = ac > 0 ? av : self;
+ for (; *p; p++)
+ repair_worktree_at_path(*p, repair_cb, &rc);
return rc;
}
diff --git a/t/t2406-worktree-repair.sh b/t/t2406-worktree-repair.sh
index 9379a63130..87bd8fc526 100755
--- a/t/t2406-worktree-repair.sh
+++ b/t/t2406-worktree-repair.sh
@@ -80,4 +80,90 @@ test_expect_success 'repair .git file from bare.git' '
test_cmp expect actual
'
+test_expect_success 'invalid worktree path' '
+ test_must_fail git worktree repair /notvalid >out 2>err &&
+ test_must_be_empty out &&
+ test_i18ngrep "not a valid path" err
+'
+
+test_expect_success 'repo not found; .git not file' '
+ test_when_finished "rm -rf not-a-worktree" &&
+ test_create_repo not-a-worktree &&
+ test_must_fail git worktree repair not-a-worktree >out 2>err &&
+ test_must_be_empty out &&
+ test_i18ngrep ".git is not a file" err
+'
+
+test_expect_success 'repo not found; .git link broken' '
+ test_when_finished "rm -rf orig moved && git worktree prune" &&
+ git worktree add --detach orig &&
+ echo /invalid >orig/.git &&
+ mv orig moved &&
+ test_must_fail git worktree repair moved >out 2>err &&
+ test_must_be_empty out &&
+ test_i18ngrep ".git link broken" err
+'
+
+test_expect_success 'repair broken gitdir' '
+ test_when_finished "rm -rf orig moved && git worktree prune" &&
+ git worktree add --detach orig &&
+ sed s,orig/\.git$,moved/.git, .git/worktrees/orig/gitdir >expect &&
+ rm .git/worktrees/orig/gitdir &&
+ mv orig moved &&
+ git worktree repair moved >out 2>err &&
+ test_cmp expect .git/worktrees/orig/gitdir &&
+ test_i18ngrep "gitdir unreadable" out &&
+ test_must_be_empty err
+'
+
+test_expect_success 'repair incorrect gitdir' '
+ test_when_finished "rm -rf orig moved && git worktree prune" &&
+ git worktree add --detach orig &&
+ sed s,orig/\.git$,moved/.git, .git/worktrees/orig/gitdir >expect &&
+ mv orig moved &&
+ git worktree repair moved >out 2>err &&
+ test_cmp expect .git/worktrees/orig/gitdir &&
+ test_i18ngrep "gitdir incorrect" out &&
+ test_must_be_empty err
+'
+
+test_expect_success 'repair gitdir (implicit) from linked worktree' '
+ test_when_finished "rm -rf orig moved && git worktree prune" &&
+ git worktree add --detach orig &&
+ sed s,orig/\.git$,moved/.git, .git/worktrees/orig/gitdir >expect &&
+ mv orig moved &&
+ git -C moved worktree repair >out 2>err &&
+ test_cmp expect .git/worktrees/orig/gitdir &&
+ test_i18ngrep "gitdir incorrect" out &&
+ test_must_be_empty err
+'
+
+test_expect_success 'unable to repair gitdir (implicit) from main worktree' '
+ test_when_finished "rm -rf orig moved && git worktree prune" &&
+ git worktree add --detach orig &&
+ cat .git/worktrees/orig/gitdir >expect &&
+ mv orig moved &&
+ git worktree repair >out 2>err &&
+ test_cmp expect .git/worktrees/orig/gitdir &&
+ test_must_be_empty out &&
+ test_must_be_empty err
+'
+
+test_expect_success 'repair multiple gitdir files' '
+ test_when_finished "rm -rf orig1 orig2 moved1 moved2 &&
+ git worktree prune" &&
+ git worktree add --detach orig1 &&
+ git worktree add --detach orig2 &&
+ sed s,orig1/\.git$,moved1/.git, .git/worktrees/orig1/gitdir >expect1 &&
+ sed s,orig2/\.git$,moved2/.git, .git/worktrees/orig2/gitdir >expect2 &&
+ mv orig1 moved1 &&
+ mv orig2 moved2 &&
+ git worktree repair moved1 moved2 >out 2>err &&
+ test_cmp expect1 .git/worktrees/orig1/gitdir &&
+ test_cmp expect2 .git/worktrees/orig2/gitdir &&
+ test_i18ngrep "gitdir incorrect:.*orig1/gitdir$" out &&
+ test_i18ngrep "gitdir incorrect:.*orig2/gitdir$" out &&
+ test_must_be_empty err
+'
+
test_done
diff --git a/worktree.c b/worktree.c
index 029ce91fdf..6ade4f0d8b 100644
--- a/worktree.c
+++ b/worktree.c
@@ -624,3 +624,77 @@ void repair_worktrees(worktree_repair_cb *cb, void *cb_data)
repair_dotgit(*wt, cb, cb_data);
free_worktrees(worktrees);
}
+
+static int is_main_worktree_path(const char *path)
+{
+ struct strbuf target = STRBUF_INIT;
+ struct strbuf main = STRBUF_INIT;
+ int cmp;
+
+ strbuf_add_real_path(&target, path);
+ strbuf_strip_suffix(&target, "/.git");
+ strbuf_add_real_path(&main, get_git_common_dir());
+ strbuf_strip_suffix(&main, "/.git");
+ cmp = fspathcmp(main.buf, target.buf);
+
+ strbuf_release(&main);
+ strbuf_release(&target);
+ return !cmp;
+}
+
+/*
+ * Repair <repo>/worktrees/<id>/gitdir if missing, corrupt, or not pointing at
+ * the worktree's path.
+ */
+void repair_worktree_at_path(const char *path,
+ worktree_repair_cb *cb, void *cb_data)
+{
+ struct strbuf dotgit = STRBUF_INIT;
+ struct strbuf realdotgit = STRBUF_INIT;
+ struct strbuf gitdir = STRBUF_INIT;
+ struct strbuf olddotgit = STRBUF_INIT;
+ char *backlink = NULL;
+ const char *repair = NULL;
+ int err;
+
+ if (!cb)
+ cb = repair_noop_cb;
+
+ if (is_main_worktree_path(path))
+ goto done;
+
+ strbuf_addf(&dotgit, "%s/.git", path);
+ if (!strbuf_realpath(&realdotgit, dotgit.buf, 0)) {
+ cb(1, path, _("not a valid path"), cb_data);
+ goto done;
+ }
+
+ backlink = xstrdup_or_null(read_gitfile_gently(realdotgit.buf, &err));
+ if (err == READ_GITFILE_ERR_NOT_A_FILE) {
+ cb(1, realdotgit.buf, _("unable to locate repository; .git is not a file"), cb_data);
+ goto done;
+ } else if (err) {
+ cb(1, realdotgit.buf, _("unable to locate repository; .git link broken"), cb_data);
+ goto done;
+ }
+
+ strbuf_addf(&gitdir, "%s/gitdir", backlink);
+ if (strbuf_read_file(&olddotgit, gitdir.buf, 0) < 0)
+ repair = _("gitdir unreadable");
+ else {
+ strbuf_rtrim(&olddotgit);
+ if (fspathcmp(olddotgit.buf, realdotgit.buf))
+ repair = _("gitdir incorrect");
+ }
+
+ if (repair) {
+ cb(0, gitdir.buf, repair, cb_data);
+ write_file(gitdir.buf, "%s", realdotgit.buf);
+ }
+done:
+ free(backlink);
+ strbuf_release(&olddotgit);
+ strbuf_release(&gitdir);
+ strbuf_release(&realdotgit);
+ strbuf_release(&dotgit);
+}
diff --git a/worktree.h b/worktree.h
index 7d085c7b91..9cb7f05741 100644
--- a/worktree.h
+++ b/worktree.h
@@ -100,6 +100,17 @@ typedef void worktree_repair_cb(int iserr, const char *path, const char *msg,
*/
void repair_worktrees(worktree_repair_cb *, void *cb_data);
+/*
+ * Repair administrative files corresponding to the worktree at the given path.
+ * The worktree's .git link pointing at the repository must be intact for the
+ * repair to succeed. Useful for re-associating an orphaned worktree with the
+ * repository if the worktree has been moved manually (without using "git
+ * worktree move"). For each repair made or error encountered while attempting
+ * a repair, the callback, if non-NULL, is called with the path of the worktree
+ * and a description of the repair or error, along with the callback user-data.
+ */
+void repair_worktree_at_path(const char *, worktree_repair_cb *, void *cb_data);
+
/*
* Free up the memory for worktree(s)
*/
--
2.28.0.461.g40977abb40
^ permalink raw reply related
* Re: git init --separate-git-dir doesn't play nice with linked working trees
From: Eric Sunshine @ 2020-08-27 8:26 UTC (permalink / raw)
To: Henré Botha; +Cc: Git List
In-Reply-To: <CAHbriek39i9NSHRw6DZm0dftk-GkeAYR74c0xyss0vbeDHu1Hw@mail.gmail.com>
On Thu, Jun 18, 2020 at 12:48 PM Henré Botha <henrebotha@gmail.com> wrote:
> I have identified two potential problems when using git init
> --separate-git-dir with a repo that has linked working trees (created
> using git worktree add).
A patch series fixing these problems has been posted[1].
[1]: https://lore.kernel.org/git/20200827082129.56149-1-sunshine@sunshineco.com/T/
^ permalink raw reply
* Re: [PATCH v2 3/3] ci: stop linking built-ins to the dashed versions
From: SZEDER Gábor @ 2020-08-27 8:30 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Junio C Hamano, Johannes Schindelin via GitGitGadget, git
In-Reply-To: <nycvar.QRO.7.76.6.2008260615280.56@tvgsbejvaqbjf.bet>
On Wed, Aug 26, 2020 at 06:19:52AM +0200, Johannes Schindelin wrote:
> Hi,
>
> On Tue, 25 Aug 2020, Junio C Hamano wrote:
>
> > SZEDER Gábor <szeder.dev@gmail.com> writes:
> >
> > > I'm afraid I don't understand this patch or the previous one (or
> > > both?). So this new Makefile knob stops hard-linking the dashed
> > > builtins _during 'make install'_, but it doesn't affect how Git is
> > > built by the default target. And our CI jobs only build Git by the
> > > default target, but don't run 'make install', so setting
> > > SKIP_DASHED_BUILT_INS wouldn't have any affect anyway.
> >
> > Very very true. Let's drop 3/3 if it is not testing anything new.
> >
> > I do understand the concern 2/3 wants to address, and it would be a
> > real one to you especially if you come from Windows. People on the
> > platform wouldn't be able to use shell scripts written in 12 years
> > ago or written with the promise we made to our users 12 years ago,
> > and unlike hardlink-capable platforms it incurs real cost to install
> > these individual binaries on disk.
>
> Actually, `SKIP_DASHED_BUILT_INS` does not _only_ have an impact on `make
> install`:
>
> $ rm git-add.exe && make
> BUILTIN git-add.exe
> BUILTIN all
> SUBDIR git-gui
> SUBDIR gitk-git
> SUBDIR templates
>
> $ rm git-add.exe && make SKIP_DASHED_BUILT_INS=1
> BUILTIN all
> SUBDIR git-gui
> SUBDIR gitk-git
> SUBDIR templates
>
> See how `git-add.exe` is linked in the first, but not in the second run?
Ah, ok, so I did indeed misunderstand the previous patch. Thanks.
^ permalink raw reply
* Re: [PATCH] patch-id: ignore newline at end of file in diff_flush_patch_id()
From: Tilman Vogel @ 2020-08-27 9:05 UTC (permalink / raw)
To: René Scharfe; +Cc: git, Junio C Hamano, Johannes Schindelin
In-Reply-To: <b67eb51d-75e8-62c5-d1c4-fc3015e13fc6@web.de>
That's great, thanks René! Looking forward to try that out!
Tilman
Am Mi., 19. Aug. 2020 um 00:09 Uhr schrieb René Scharfe <l.s.r@web.de>:
>
> Whitespace is ignored when calculating patch IDs. This is done by
> removing all whitespace from diff lines before hashing them, including
> a newline at the end of a file. If that newline is missing, however,
> diff reports that fact in a separate line containing "\ No newline at
> end of file\n", and this marker is hashed like a context line.
>
> This goes against our goal of making patch IDs independent of
> whitespace. Use the same heuristic that 2485eab55cc (git-patch-id: do
> not trip over "no newline" markers, 2011-02-17) added to git patch-id
> instead and skip diff lines that start with a backslash and a space
> and are longer than twelve characters.
>
> Reported-by: Tilman Vogel <tilman.vogel@web.de>
> Initial-test-by: Tilman Vogel <tilman.vogel@web.de>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
> diff.c | 2 ++
> t/t3500-cherry.sh | 23 +++++++++++++++++++++++
> 2 files changed, 25 insertions(+)
>
> diff --git a/diff.c b/diff.c
> index f9709de7b45..f175019eb7a 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -6044,6 +6044,8 @@ static void patch_id_consume(void *priv, char *line, unsigned long len)
> struct patch_id_t *data = priv;
> int new_len;
>
> + if (len > 12 && starts_with(line, "\\ "))
> + return;
> new_len = remove_space(line, len);
>
> the_hash_algo->update_fn(data->ctx, line, new_len);
> diff --git a/t/t3500-cherry.sh b/t/t3500-cherry.sh
> index f038f34b7c0..2b8d9cb38ed 100755
> --- a/t/t3500-cherry.sh
> +++ b/t/t3500-cherry.sh
> @@ -55,4 +55,27 @@ test_expect_success \
> expr "$(echo $(git cherry master my-topic-branch) )" : "+ [^ ]* - .*"
> '
>
> +test_expect_success 'cherry ignores whitespace' '
> + git switch --orphan=upstream-with-space &&
> + test_commit initial file &&
> + >expect &&
> + git switch --create=feature-without-space &&
> +
> + # A spaceless file on the feature branch. Expect a match upstream.
> + printf space >file &&
> + git add file &&
> + git commit -m"file without space" &&
> + git log --format="- %H" -1 >>expect &&
> +
> + # A further change. Should not match upstream.
> + test_commit change file &&
> + git log --format="+ %H" -1 >>expect &&
> +
> + git switch upstream-with-space &&
> + # Same as the spaceless file, just with spaces and on upstream.
> + test_commit "file with space" file "s p a c e" file-with-space &&
> + git cherry upstream-with-space feature-without-space >actual &&
> + test_cmp expect actual
> +'
> +
> test_done
> --
> 2.28.0
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox