* Re: [PATCH 0/4] checkout: cleanup --conflict=
From: phillip.wood123 @ 2024-03-08 16:07 UTC (permalink / raw)
To: Junio C Hamano, Phillip Wood via GitGitGadget
Cc: git, Phillip Wood, Johannes Schindelin
In-Reply-To: <xmqqplw4hgnq.fsf@gitster.g>
Hi Junio
[cc Johannes for the gitgitgadget issue]
On 08/03/2024 15:44, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> Here is a place to say why this series exists. Saying things like
> "'checkout --conflict=bad' gives a wrong error message, as if the
> inalid conflict style were given by a configuration variable, and
> this is to fix that bug".
Sorry, I'm not sure what happen there, I definitely entered a
cover-letter on the gitgitgadget PR:
Passing an invalid conflict style name such as "--conflict=bad" to "git
checkout" gives the error message
error: unknown style 'bad' given for 'merge.conflictstyle'
which is unfortunate as it talks about a config setting rather than the
option given on the command line. This series refactors the
implementation to pass the conflict style down the call chain to the
merge machinery rather than abusing the config setting.
Best Wishes
Phillip
>> Phillip Wood (4):
>> xdiff-interface: refactor parsing of merge.conflictstyle
>> merge-ll: introduce LL_MERGE_OPTIONS_INIT
>> merge options: add a conflict style member
>> checkout: cleanup --conflict=<style> parsing
>>
>> builtin/checkout.c | 40 +++++++++++++++++++++-------------------
>> merge-ll.c | 6 ++++--
>> merge-ll.h | 5 +++++
>> merge-ort.c | 3 ++-
>> merge-recursive.c | 5 ++++-
>> merge-recursive.h | 1 +
>> t/t7201-co.sh | 6 ++++++
>> xdiff-interface.c | 29 ++++++++++++++++++-----------
>> xdiff-interface.h | 1 +
>> 9 files changed, 62 insertions(+), 34 deletions(-)
>>
>>
>> base-commit: b387623c12f3f4a376e4d35a610fd3e55d7ea907
>> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1684%2Fphillipwood%2Frefactor-conflict-style-v1
>> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1684/phillipwood/refactor-conflict-style-v1
>> Pull-Request: https://github.com/gitgitgadget/git/pull/1684
^ permalink raw reply
* Re: [PATCH 3/4] merge options: add a conflict style member
From: phillip.wood123 @ 2024-03-08 16:13 UTC (permalink / raw)
To: Junio C Hamano, Phillip Wood via GitGitGadget
Cc: git, Phillip Wood, Elijah Newren
In-Reply-To: <xmqqle6shgkw.fsf@gitster.g>
Hi Junio
On 08/03/2024 15:46, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> diff --git a/merge-ll.c b/merge-ll.c
>> index 6570707297d..bf1077ae092 100644
>> --- a/merge-ll.c
>> +++ b/merge-ll.c
>> ..
>> -#define LL_MERGE_OPTIONS_INIT {0}
>> +#define LL_MERGE_OPTIONS_INIT { .conflict_style = -1 }
>
> Makes sense, and this obviously makes the previous step worth doing.
>
> It looks quite wrong that low-level merge options definition is
> hosted in a file whose name is merge low-level. Is it too late to
> rename the file to fix this, by the way?
I agree it is confusing, Elijah renamed it from ll-merge.c relatively
recently 6723899932e (merge-ll: rename from ll-merge, 2023-05-16). It
looks like the idea was to group it with the other merge* files:
merge-ll: rename from ll-merge
A long term (but rather minor) pet-peeve of mine was the name
ll-merge.[ch]. I thought it made it harder to realize what stuff was
related to merging when I was working on the merge machinery and trying
to improve it.
Further, back in d1cbe1e6d8a ("hash-ll.h: split out of hash.h to remove
dependency on repository.h", 2023-04-22), we have split the portions of
hash.h that do not depend upon repository.h into a "hash-ll.h" (due to
the recommendation to use "ll" for "low-level" in its name[1], but which
I used as a suffix precisely because of my distaste for "ll-merge").
When we discussed adding additional "*-ll.h" files, a request was made
that we use "ll" consistently as either a prefix or a suffix. Since it
is already in use as both a prefix and a suffix, the only way to do so
is to rename some files.
Besides my distaste for the ll-merge.[ch] name, let me also note that
the files
ll-fsmonitor.h, ll-hash.h, ll-merge.h, ll-object-store.h, ll-read-cache.h
would have essentially nothing to do with each other and make no sense
to group. But giving them the common "ll-" prefix would group them. Using
"-ll" as a suffix thus seems just much more logical to me. Rename
ll-merge.[ch] to merge-ll.[ch] to achieve this consistency, and to
ensure we get a more logical grouping of files.
[1] https://lore.kernel.org/git/kl6lsfcu1g8w.fsf@chooglen-macbookpro.roam.corp.google.com/
Best Wishes
Phillip
^ permalink raw reply
* Re: [PATCH 4/4] checkout: cleanup --conflict=<style> parsing
From: Junio C Hamano @ 2024-03-08 16:15 UTC (permalink / raw)
To: Phillip Wood via GitGitGadget; +Cc: git, Phillip Wood
In-Reply-To: <317bb7a70d023278087f4370b843d7f28f9ee2f6.1709907271.git.gitgitgadget@gmail.com>
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> @@ -91,7 +91,8 @@ struct checkout_opts {
> int new_branch_log;
> enum branch_track track;
> struct diff_options diff_options;
> - char *conflict_style;
> + char *conflict_style_name;
> + int conflict_style;
Does the conflict_style_name need to be a member of this struct?
> @@ -1628,7 +1635,7 @@ static struct option *add_common_options(struct checkout_opts *opts,
> PARSE_OPT_OPTARG, option_parse_recurse_submodules_worktree_updater),
> OPT_BOOL(0, "progress", &opts->show_progress, N_("force progress reporting")),
> OPT_BOOL('m', "merge", &opts->merge, N_("perform a 3-way merge with the new branch")),
> - OPT_STRING(0, "conflict", &opts->conflict_style, N_("style"),
> + OPT_STRING(0, "conflict", &opts->conflict_style_name, N_("style"),
> N_("conflict style (merge, diff3, or zdiff3)")),
> OPT_END()
> };
Ah, the options[] definition is not in the same scope as where the
parse_options() is called, and that is the reason why we need to
carry the extra member that we do not need after we are done with
parsing (we use "int conflict_style") in the struct. Otherwise we
would have just received OPT_STRING() into a local variable, called
parse_options(), and post-processed the string into
opts->conflict_style.
Yucky. I do not care much about wasted 8 bytes in the structure,
but I find it disturbing that those functions called later with this
struct has to know that conflict_style_name is a useless member and
they are supposed to use conflict_style exclusively.
We could use OPT_CALLBACK() to accept the incoming string, parse it
and store it in opts->conflict_style and that would be a way to
avoid the extra member.
> + opts->conflict_style =
> + parse_conflict_style(opts->conflict_style_name);
When I saw the change to xdiff-interface in an earlier step, I
thought parse_conflict_style() was a potentially confusing name.
You can imagine a function that is fed a file with conflict markers
and say "ah, this uses diff3 style with common ancestor version" vs
"this uses merge style with only two sides" to have such a name.
parse_conflict_style_name() that takes a name and returns
conflict_style enumeration constant would not risk such a confusion,
I guess.
Thanks.
^ permalink raw reply
* Re: [PATCH 11/15] find multi-byte comment chars in unterminated buffers
From: Phillip Wood @ 2024-03-08 16:20 UTC (permalink / raw)
To: Junio C Hamano
Cc: René Scharfe, Jeff King, git, Dragan Simic,
Kristoffer Haugsbakk, Manlio Perillo
In-Reply-To: <xmqqfrx0hg01.fsf@gitster.g>
On 08/03/2024 15:58, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
>
>> I agree with your analysis. I do wonder though if we should reject
>> whitespace and control characters when parsing core.commentChar, it
>> feels like accepting them is a bug waiting to happen. If
>> comment_line_char starts with ' ' or '\t' that part will be eaten by
>> the strspn() above and so starts_with_mem() wont match. Also we will
>> never match a comment if comment_line_str contains '\n'.
>
> Another thing I was wondering is what we want to do a random
> byte-sequence that may match from the middle of a multi-byte UTF-8
> character.
>
> The reason I haven't mentioned these "nonsense input" is because
> they will at worst only lead to self-denial-of-service to those who
> are too curious, and will fall into "don't do it then" category.
We could certainly leave it as-is and tell users they are only hurting
themselves if they complain when it does not work.
> Also, what exactly is the definition of "nonsense" will become can
> of worms. I can sympathise if somebody wants to use "#\t" to give
> themselves a bit more room than usual on the left for visibility,
> for example, so there might be a case to want whitespace characters.
That's fair, maybe we could just ban leading whitespace if we do decide
to restrict core.commentChar
Best Wishes
Phillip
>>> Not sure why lines that start with CR are considered comment lines,
>>> though.
>>
>> I think it is a lazy way of looking for an empty line ending in CR LF,
>> it should really be
>>
>> || (bol[0] == '\r' && bol[1] == '\n') ||
>
> My recollection matches your speculation.
>
> IIRC the lazy persono was probably me but I didn't run "git blame".
^ permalink raw reply
* Re: [PATCH 4/4] checkout: cleanup --conflict=<style> parsing
From: phillip.wood123 @ 2024-03-08 16:22 UTC (permalink / raw)
To: Junio C Hamano, Phillip Wood via GitGitGadget; +Cc: git, Phillip Wood
In-Reply-To: <xmqq1q8khf8t.fsf@gitster.g>
Hi Junio
On 08/03/2024 16:15, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> We could use OPT_CALLBACK() to accept the incoming string, parse it
> and store it in opts->conflict_style and that would be a way to
> avoid the extra member.
>
>> + opts->conflict_style =
>> + parse_conflict_style(opts->conflict_style_name);
>
> When I saw the change to xdiff-interface in an earlier step, I
> thought parse_conflict_style() was a potentially confusing name.
> You can imagine a function that is fed a file with conflict markers
> and say "ah, this uses diff3 style with common ancestor version" vs
> "this uses merge style with only two sides" to have such a name.
>
> parse_conflict_style_name() that takes a name and returns
> conflict_style enumeration constant would not risk such a confusion,
> I guess.
Those are both good suggestions - I'll re-roll next week
Thanks
Phillip
^ permalink raw reply
* Re: [PATCH 3/4] merge options: add a conflict style member
From: Junio C Hamano @ 2024-03-08 16:48 UTC (permalink / raw)
To: phillip.wood123
Cc: Phillip Wood via GitGitGadget, git, Phillip Wood, Elijah Newren
In-Reply-To: <48b2d5b1-a59f-44c3-94ba-e7f81913b7f5@gmail.com>
phillip.wood123@gmail.com writes:
> I agree it is confusing, Elijah renamed it from ll-merge.c relatively
> recently 6723899932e (merge-ll: rename from ll-merge, 2023-05-16). It
> looks like the idea was to group it with the other merge* files:
That reasoning cuts both ways. When you are only interested in the
upper level API functions, being able to skip ll-anything as "low
level details" is a powerful thing. merge-ll & hash-ll separated
far apart will make it impossible.
^ permalink raw reply
* Re: [PATCH] wt-status: Don't find scissors line beyond buf len
From: Florian Schmidt @ 2024-03-08 17:43 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jonathan Davies, Phillip Wood, Denton Liu, Linus Arver
In-Reply-To: <xmqqedckiw2r.fsf@gitster.g>
On 08/03/2024 15:26, Junio C Hamano wrote:
> It would be the technically correct way to do so, but as a short-cut
> to reduce a round-trip, if you are happy with the version I queued
> (should be found by fetching the 'seen' branch from any of the
> mirrors), you can just say "that looks fine" and we can be done with
> this patch.
I had a look, and it looks fine (though there's the one typo already
pointed out by Eric: s/exeption/exception/). You can go ahead and use
that version of the patch.
Cheers,
flosch
^ permalink raw reply
* Re: [PATCH 2/3] format-patch: teach `--header-cmd`
From: Kristoffer Haugsbakk @ 2024-03-08 18:30 UTC (permalink / raw)
To: git
In-Reply-To: <f405a0140b5655bc66a0a7a603517a421d7669cf.1709841147.git.code@khaugsbakk.name>
On Thu, Mar 7, 2024, at 20:59, Kristoffer Haugsbakk wrote:
> +test_expect_success '--header-cmd with no output works' '
> + write_script cmd <<-\EOF &&
> + exit 0
> + EOF
> + git format-patch --header-cmd=./cmd --stdout main..side
> +'
This can be simplified to `--header-cmd=true`.
^ permalink raw reply
* Re: What's cooking in git.git (Feb 2024, #09; Tue, 27)
From: Linus Arver @ 2024-03-08 19:11 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqsf1a7wse.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>
>>> Doh, please wait for my v6 reroll (will send to the list in the next
>>> half hour) to clean up the commit messages. Thanks.
>>
>> If we had communication gap and the topic was prematurely merged and
>> was caught within a day or two, I would be a bit more sympathetic,
>> but given that this was merged more than a week ago,
Doh, I didn't realize this when I posted my message in this thread.
>> that's totally
>> unacceptable.
Completely understandable. In the future I will check "how long has the
topic been in 'next'?" before asking to replace it with a newer
revision.
>> If you have improvements, please do so as incremental patches on
>> top. I'll hold the topic in 'next' until we are ready.
>
> Well, I changed my mind. As we haven't rewound the tip of 'next'
> post release, let's eject what is in 'next' and queue the latest,
> pretending that the earlier round weren't in 'next' at all.
>
> Thanks.
Ah I see --- thanks for stating for the record (and not leaving any room
for me to get confused). Much appreciated.
^ permalink raw reply
* [BUG] cannot git clone with includeif onbranch
From: Angelo Dureghello @ 2024-03-08 19:25 UTC (permalink / raw)
To: git
Hi,
below the bug report, not totally sure this is a bug btw.
---
Thank you for filling out a Git bug report!
Please answer the following questions to help us understand your issue.
What did you do before the bug happened? (Steps to reproduce your issue)
Perform a git clone https with includeif onbranch in the .gitconfig
Create a .gitconfig
with
[includeIf "onbranch:wip/pippo/**"]
path = ~/.gitconfig.pippo.inc
git clone https://github.com/analogdevicesinc/no-OS.git
Cloning into 'no-OS'...
BUG: refs.c:2083: reference backend is unknown
error: git-remote-https died of signal 6
What did you expect to happen? (Expected behavior)
Proper clone
What happened instead? (Actual behavior)
Error above
What's different between what you expected and what actually happened?
I cannot clone
Anything else you want to add:
I am assuming that cloning is outside from any gitdir so should ingnore
onbranch.
Please review the rest of the bug report below.
You can delete any lines you don't wish to share.
[System Info]
git version:
git version 2.44.0
cpu: x86_64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
uname: Linux 6.5.0-rc2-devel-00345-gc2782531397f #2 SMP PREEMPT_DYNAMIC
Sun Jul 23 12:25:41 CEST 2023 x86_64
compiler info: gnuc: 13.2
libc info: glibc: 2.39
$SHELL (typically, interactive shell): /bin/zsh
[Enabled Hooks]
not run from a git repository - no hooks to show
^ permalink raw reply
* Re: [BUG] cannot git clone with includeif onbranch
From: brian m. carlson @ 2024-03-08 20:10 UTC (permalink / raw)
To: Angelo Dureghello; +Cc: git, Patrick Steinhardt
In-Reply-To: <72771da0-a0ef-4fd9-8071-6467cd7b6a6b@kernel-space.org>
[-- Attachment #1: Type: text/plain, Size: 2064 bytes --]
On 2024-03-08 at 19:25:52, Angelo Dureghello wrote:
> Hi,
>
> below the bug report, not totally sure this is a bug btw.
>
> ---
>
> Thank you for filling out a Git bug report!
> Please answer the following questions to help us understand your issue.
>
> What did you do before the bug happened? (Steps to reproduce your issue)
>
> Perform a git clone https with includeif onbranch in the .gitconfig
>
> Create a .gitconfig
> with
> [includeIf "onbranch:wip/pippo/**"]
> path = ~/.gitconfig.pippo.inc
>
> git clone https://github.com/analogdevicesinc/no-OS.git
>
> Cloning into 'no-OS'...
> BUG: refs.c:2083: reference backend is unknown
> error: git-remote-https died of signal 6
Thanks for the report.
I can definitely confirm this with a local Git 2.44.0 built out of my
working tree. It seems to trigger as long as there's a `path` entry
whether the path exists or not. It _doesn't_ seem to trigger with a
`gitdir` check, but does trigger for `onbranch`. v2.43.0 is not
affected.
I do definitely think this is a bug. First of all, we should not
trigger BUG conditions, even if the user has done something naughty, so
we should fix it for that reason. Second of all, this seems like a
completely reasonable thing to want to do, and considering it triggers
for existing files, and that it worked just fine in v2.43.0, I don't see
a reason we shouldn't have this work.
A bisection[0] leads us to 0fcc285c5e ("refs: refactor logic to look up
storage backends", 2023-12-29). I've CCed the author of that commit,
who hopefully can provide some more helpful context.
I have some guesses about what's going on here, but I haven't poked
further into the situation, so I'll refrain from speculating for now.
[0] git bisect run sh -c 'make -j12 && cd $TMPDIR && rm -fr no-OS && PATH="$HOME/checkouts/git/bin-wrappers:$PATH" git clone https://github.com/analogdevicesinc/no-OS.git; RET=$?; [ "$RET" -eq 128 ] && RET=1; exit $RET'
--
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]
^ permalink raw reply
* Re: [BUG] cannot git clone with includeif onbranch
From: Angelo Dureghello @ 2024-03-08 20:27 UTC (permalink / raw)
To: brian m. carlson, git, Patrick Steinhardt
In-Reply-To: <Zetw0I0NHgABR_PX@tapette.crustytoothpaste.net>
Hi Brian,
On 08/03/24 9:10 PM, brian m. carlson wrote:
> On 2024-03-08 at 19:25:52, Angelo Dureghello wrote:
>> Hi,
>>
>> below the bug report, not totally sure this is a bug btw.
>>
>> ---
>>
>> Thank you for filling out a Git bug report!
>> Please answer the following questions to help us understand your issue.
>>
>> What did you do before the bug happened? (Steps to reproduce your issue)
>>
>> Perform a git clone https with includeif onbranch in the .gitconfig
>>
>> Create a .gitconfig
>> with
>> [includeIf "onbranch:wip/pippo/**"]
>> path = ~/.gitconfig.pippo.inc
>>
>> git clone https://github.com/analogdevicesinc/no-OS.git
>>
>> Cloning into 'no-OS'...
>> BUG: refs.c:2083: reference backend is unknown
>> error: git-remote-https died of signal 6
>
> Thanks for the report.
>
> I can definitely confirm this with a local Git 2.44.0 built out of my
> working tree. It seems to trigger as long as there's a `path` entry
> whether the path exists or not. It _doesn't_ seem to trigger with a
> `gitdir` check, but does trigger for `onbranch`. v2.43.0 is not
> affected.
>
> I do definitely think this is a bug. First of all, we should not
> trigger BUG conditions, even if the user has done something naughty, so
> we should fix it for that reason. Second of all, this seems like a
> completely reasonable thing to want to do, and considering it triggers
> for existing files, and that it worked just fine in v2.43.0, I don't see
> a reason we shouldn't have this work.
>
> A bisection[0] leads us to 0fcc285c5e ("refs: refactor logic to look up
> storage backends", 2023-12-29). I've CCed the author of that commit,
> who hopefully can provide some more helpful context.
>
> I have some guesses about what's going on here, but I haven't poked
> further into the situation, so I'll refrain from speculating for now.
>
> [0] git bisect run sh -c 'make -j12 && cd $TMPDIR && rm -fr no-OS && PATH="$HOME/checkouts/git/bin-wrappers:$PATH" git clone https://github.com/analogdevicesinc/no-OS.git; RET=$?; [ "$RET" -eq 128 ] && RET=1; exit $RET'
Thanks a lot.
Actually i can work with git clone git@github.com:.. that works,
issue is only on http.
Regards, and if i can help, welcome.
angelo
^ permalink raw reply
* [PATCH 0/2] Loosening safe.bareRepository=explicit even further
From: Junio C Hamano @ 2024-03-08 21:19 UTC (permalink / raw)
To: git; +Cc: Kyle Lippincott, Glen Choo
In-Reply-To: <xmqqv85zqniu.fsf@gitster.g>
Earlier 45bb9162 (setup: allow cwd=.git w/ bareRepository=explicit,
2024-01-20) loosened safe.bareRepository=explicit in such a way that
working inside the ".git/" directory (or its subdirectories) of a
repository that is not bare can be done without an explicit GIT_DIR
or "git --git-dir=<path>". The code needed for its change was
almost trivial---when it looks like we encountered a bare
repository, if the last path component of the discovered "$GIT_DIR"
is ".git", then it cannot be anything but the $GIT_DIR of a non-bare
repository, the root of whose working tree is the parent directory
of that ".git" directory. This is because projects cannot create a
".git" directory in their working tree and cause clone/checkout to
extract them in the victim's working tree.
This almost works, until somebody starts using "git worktree add" to
create a secondary worktree. Their $GIT_DIR resides inside the
$GIT_DIR of the primary worktree of the same repository, at
$GIT_DIR/worktree/$name where $name is the name of the secondary
worktree, which is not ".git".
These two patches are to extend the "if you can work in its working
tree, you should be able to work in its $GIT_DIR" for secondary
worktrees.
Junio C Hamano (2):
setup: detect to be in $GIT_DIR with a new helper
setup: make bareRepository=explicit work in GIT_DIR of a secondary worktree
setup.c | 57 ++++++++++++++++++++++++++++++++-
t/t0035-safe-bare-repository.sh | 8 ++++-
2 files changed, 63 insertions(+), 2 deletions(-)
--
2.44.0-165-ge09f1254c5
^ permalink raw reply
* [PATCH 2/2] setup: make bareRepository=explicit work in GIT_DIR of a secondary worktree
From: Junio C Hamano @ 2024-03-08 21:19 UTC (permalink / raw)
To: git; +Cc: Kyle Lippincott
In-Reply-To: <20240308211957.3758770-1-gitster@pobox.com>
If you have /var/tmp/primary/ as a repository, and if you create a
secondary worktree of it at /var/tmp/secondary/, the layout would
look like this:
$ cd /var/tmp/
$ git init primary
$ cd primary
$ pwd
/var/tmp/primary
$ git worktree add ../secondary
$ cat ../seconary/.git
gitdir: /var/tmp/primary/.git/worktrees/secondary
$ ls /var/tmp/primary/.git/worktrees/secondary
commondir gitdir HEAD index refs
$ cat /var/tmp/primary/.git/worktrees/secondary/gitdir
/var/tmp/secondary/.git
When the configuration variable 'safe.bareRepository=explicit' is
set to explicit, the change made by 45bb9162 (setup: allow cwd=.git
w/ bareRepository=explicit, 2024-01-20) allows you to work in the
/var/tmp/primary/.git directory (i.e., $GIT_DIR of the primary
worktree). The idea is that if it is safe to work in the repository
in its working tree, it should be equally safe to work in the
".git/" directory of that working tree, too.
Now, for the same reason, let's allow command execution from within
the $GIT_DIR directory of a secondary worktree. This is useful for
tools working with secondary worktrees when the 'bareRepository'
setting is set to 'explicit'.
In the previous commit, we created a helper function to house the
logic that checks if a directory that looks like a bare repository
is actually a part of a non-bare repository. Extend the helper
function to also check if the apparent bare-repository is a $GIT_DIR
of a secondary worktree, by checking three things:
* The path to the $GIT_DIR must be a subdirectory of
".git/worktrees/", which is the primary worktree [*].
* Such $GIT_DIR must have file "gitdir", that records the path of
the ".git" file that is at the root level of the secondary
worktree.
* That ".git" file in turn points back at the $GIT_DIR we are
inspecting.
The latter two points are merely for checking sanity. The security
lies in the first requirement.
Remember that a tree object with an entry whose pathname component
is ".git" is forbidden at various levels (fsck, object transfer and
checkout), so malicious projects cannot cause users to clone and
checkout a crafted ".git" directory in a shell directory that
pretends to be a working tree with that ".git" thing at its root
level. That is where 45bb9162 (setup: allow cwd=.git w/
bareRepository=explicit, 2024-01-20) draws its security guarantee
from. And the solution for secondary worktrees in this commit draws
its security guarantee from the same place.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
setup.c | 52 ++++++++++++++++++++++++++++++++-
t/t0035-safe-bare-repository.sh | 8 ++++-
2 files changed, 58 insertions(+), 2 deletions(-)
diff --git a/setup.c b/setup.c
index 3081be4970..68860dcd18 100644
--- a/setup.c
+++ b/setup.c
@@ -1231,9 +1231,59 @@ static const char *allowed_bare_repo_to_string(
return NULL;
}
+static int is_git_dir_of_secondary_worktree(const char *path)
+{
+ int result = 0; /* assume not */
+ struct strbuf gitfile_here = STRBUF_INIT;
+ struct strbuf gitfile_there = STRBUF_INIT;
+ const char *gitfile_contents;
+ int error_code = 0;
+
+ /*
+ * We should be a subdirectory of /.git/worktrees inside
+ * the $GIT_DIR of the primary worktree.
+ *
+ * NEEDSWORK: some folks create secondary worktrees out of a
+ * bare repository; they don't count ;-), at least not yet.
+ */
+ if (!strstr(path, "/.git/worktrees/"))
+ goto out;
+
+ /*
+ * Does gitdir that points at the ".git" file at the root of
+ * the secondary worktree roundtrip here?
+ */
+ strbuf_addf(&gitfile_here, "%s/gitdir", path);
+ if (!file_exists(gitfile_here.buf))
+ goto out;
+ if (strbuf_read_file(&gitfile_there, gitfile_here.buf, 0) < 0)
+ goto out;
+ strbuf_trim_trailing_newline(&gitfile_there);
+
+ gitfile_contents = read_gitfile_gently(gitfile_there.buf, &error_code);
+ if ((!gitfile_contents) || strcmp(gitfile_contents, path))
+ goto out;
+
+ /* OK, we are happy */
+ result = 1;
+
+out:
+ strbuf_release(&gitfile_here);
+ strbuf_release(&gitfile_there);
+ return result;
+}
+
static int is_repo_with_working_tree(const char *path)
{
- return ends_with_path_components(path, ".git");
+ /* $GIT_DIR immediately below the primary working tree */
+ if (ends_with_path_components(path, ".git"))
+ return 1;
+
+ /* Are we in $GIT_DIR of a secondary worktree? */
+ if (is_git_dir_of_secondary_worktree(path))
+ return 1;
+
+ return 0;
}
/*
diff --git a/t/t0035-safe-bare-repository.sh b/t/t0035-safe-bare-repository.sh
index 8048856379..62cdfcefc1 100755
--- a/t/t0035-safe-bare-repository.sh
+++ b/t/t0035-safe-bare-repository.sh
@@ -31,7 +31,9 @@ expect_rejected () {
test_expect_success 'setup bare repo in worktree' '
git init outer-repo &&
- git init --bare outer-repo/bare-repo
+ git init --bare outer-repo/bare-repo &&
+ git -C outer-repo worktree add ../outer-secondary &&
+ test_path_is_dir outer-secondary
'
test_expect_success 'safe.bareRepository unset' '
@@ -86,4 +88,8 @@ test_expect_success 'no trace when "bare repository" is a subdir of .git' '
expect_accepted_implicit -C outer-repo/.git/objects
'
+test_expect_success 'no trace in $GIT_DIR of secondary worktree' '
+ expect_accepted_implicit -C outer-repo/.git/worktrees/outer-secondary
+'
+
test_done
--
2.44.0-165-ge09f1254c5
^ permalink raw reply related
* [PATCH 1/2] setup: detect to be in $GIT_DIR with a new helper
From: Junio C Hamano @ 2024-03-08 21:19 UTC (permalink / raw)
To: git; +Cc: Kyle Lippincott
In-Reply-To: <20240308211957.3758770-1-gitster@pobox.com>
Earlier, 45bb9162 (setup: allow cwd=.git w/ bareRepository=explicit,
2024-01-20) loosened the "safe.bareRepository=explicit" to allow Git
operations inside ".git/" directory in the root level of a working
tree of a non-bare repository. It used the fact that the $GIT_DIR
you discover has ".git" as the last path component, if you descended
into ".git" of a non-bare repository.
Let's move the logic into a separate helper function. We can
enhance this to detect the case where we are inside $GIT_DIR of a
secondary worktree (where "ends with .git" trick does not work) in
the next commit.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
setup.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/setup.c b/setup.c
index a09b7b87ec..3081be4970 100644
--- a/setup.c
+++ b/setup.c
@@ -1231,6 +1231,11 @@ static const char *allowed_bare_repo_to_string(
return NULL;
}
+static int is_repo_with_working_tree(const char *path)
+{
+ return ends_with_path_components(path, ".git");
+}
+
/*
* We cannot decide in this function whether we are in the work tree or
* not, since the config can only be read _after_ this function was called.
@@ -1360,7 +1365,7 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
if (is_git_directory(dir->buf)) {
trace2_data_string("setup", NULL, "implicit-bare-repository", dir->buf);
if (get_allowed_bare_repo() == ALLOWED_BARE_REPO_EXPLICIT &&
- !ends_with_path_components(dir->buf, ".git"))
+ !is_repo_with_working_tree(dir->buf))
return GIT_DIR_DISALLOWED_BARE;
if (!ensure_valid_ownership(NULL, NULL, dir->buf, report))
return GIT_DIR_INVALID_OWNERSHIP;
--
2.44.0-165-ge09f1254c5
^ permalink raw reply related
* RE: [BUG] cannot git clone with includeif onbranch
From: rsbecker @ 2024-03-08 21:20 UTC (permalink / raw)
To: 'brian m. carlson', 'Angelo Dureghello'
Cc: git, 'Patrick Steinhardt'
In-Reply-To: <Zetw0I0NHgABR_PX@tapette.crustytoothpaste.net>
On Friday, March 8, 2024 3:11 PM, brian m. carlson wrote:
>On 2024-03-08 at 19:25:52, Angelo Dureghello wrote:
>> Hi,
>>
>> below the bug report, not totally sure this is a bug btw.
>>
>> ---
>>
>> Thank you for filling out a Git bug report!
>> Please answer the following questions to help us understand your issue.
>>
>> What did you do before the bug happened? (Steps to reproduce your
>> issue)
>>
>> Perform a git clone https with includeif onbranch in the .gitconfig
>>
>> Create a .gitconfig
>> with
>> [includeIf "onbranch:wip/pippo/**"]
>> path = ~/.gitconfig.pippo.inc
>>
>> git clone https://github.com/analogdevicesinc/no-OS.git
>>
>> Cloning into 'no-OS'...
>> BUG: refs.c:2083: reference backend is unknown
>> error: git-remote-https died of signal 6
>
>Thanks for the report.
>
>I can definitely confirm this with a local Git 2.44.0 built out of my working tree. It seems to trigger as long as there's a `path` entry
>whether the path exists or not. It _doesn't_ seem to trigger with a `gitdir` check, but does trigger for `onbranch`. v2.43.0 is not
>affected.
>
>I do definitely think this is a bug. First of all, we should not trigger BUG conditions, even if the user has done something naughty, so we
>should fix it for that reason. Second of all, this seems like a completely reasonable thing to want to do, and considering it triggers for
>existing files, and that it worked just fine in v2.43.0, I don't see a reason we shouldn't have this work.
>
>A bisection[0] leads us to 0fcc285c5e ("refs: refactor logic to look up storage backends", 2023-12-29). I've CCed the author of that
>commit, who hopefully can provide some more helpful context.
>
>I have some guesses about what's going on here, but I haven't poked further into the situation, so I'll refrain from speculating for now.
>
>[0] git bisect run sh -c 'make -j12 && cd $TMPDIR && rm -fr no-OS && PATH="$HOME/checkouts/git/bin-wrappers:$PATH" git clone
>https://github.com/analogdevicesinc/no-OS.git; RET=$?; [ "$RET" -eq 128 ] && RET=1; exit $RET'
I was wondering whether this should only work on a clone with -b wip/pippo/... argument, although it should not fail the clone, just not apply the path. I can see how it would be somewhat confusing if the global config defaults to master and remote only serves up main - or vise-versa.
--Randall
^ permalink raw reply
* Re: [PATCH 4/4] checkout: cleanup --conflict=<style> parsing
From: Junio C Hamano @ 2024-03-08 21:40 UTC (permalink / raw)
To: phillip.wood123; +Cc: Phillip Wood via GitGitGadget, git, Phillip Wood
In-Reply-To: <1fb1c84a-b6b3-4e3e-896f-5fb93cff57a6@gmail.com>
phillip.wood123@gmail.com writes:
> Hi Junio
>
> On 08/03/2024 16:15, Junio C Hamano wrote:
>> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
>> We could use OPT_CALLBACK() to accept the incoming string, parse it
>> and store it in opts->conflict_style and that would be a way to
>> avoid the extra member.
>>
>>> + opts->conflict_style =
>>> + parse_conflict_style(opts->conflict_style_name);
>> When I saw the change to xdiff-interface in an earlier step, I
>> thought parse_conflict_style() was a potentially confusing name.
>> You can imagine a function that is fed a file with conflict markers
>> and say "ah, this uses diff3 style with common ancestor version" vs
>> "this uses merge style with only two sides" to have such a name.
>> parse_conflict_style_name() that takes a name and returns
>> conflict_style enumeration constant would not risk such a confusion,
>> I guess.
>
> Those are both good suggestions - I'll re-roll next week
Thanks.
^ permalink raw reply
* Re: [PATCH 1/1] Documentation/user-manual.txt: example for generating object hashes
From: Dirk Gouders @ 2024-03-08 22:11 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqil1wiw5l.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Dirk Gouders <dirk@gouders.net> writes:
>
>> May I ask what you meant by "modulo coding style", e.g. where I should
>> look at to make the code of similar style?
>
> Documentation/CodingGuidelines would be a good start, but
>
> * A here-doc for a single liner is probably an overkill. Why not
>
> echo "Hello, world" >file
>
> In either way, in our codebase a redirection operator '>' (or
> '<') has one whitespace before it, and no whitespace after it
> before the file.
>
> * printf piped to "cat - file" whose output feeds another pipe
> looked unusual. More usual way novices write may be
>
> { printf ... ; cat file; } | sha1sum
>
> were the two things I noticed.
>
>> I would also add that git-hash-object(1) could be used to verify the
>> result if you think that is OK.
>
> git hash-object can be used to replace that whole thing ;-)
>
>> In addition to a suggestion in another mail, the commit would
>> consist of substantial content you suggested and perhaps, you could tell
>> me how to express this; would a Helped-By be correct?
>
> I think many may prefer to downcase B in By, but if it is
> "substantial", probably. I do not think I gave much in this
> discussion to become a substantial addition to the original, though.
Thank you for the explanation (some of which I should have found by
myself).
I will send the prepared v2 when I solved my struggling with range-diffs;
that concept is new to me and I have a slow brain -- if one at all.
Dirk
^ permalink raw reply
* Re: [PATCH v2] git-difftool--helper: honor `--trust-exit-code` with `--dir-diff`
From: SZEDER Gábor @ 2024-03-08 22:12 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Jean-Rémy Falleri, Junio C Hamano
In-Reply-To: <0fac668f8fc021af9f9c4df5134da59816307ccc.1708423309.git.ps@pks.im>
On Tue, Feb 20, 2024 at 11:08:25AM +0100, Patrick Steinhardt wrote:
> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
> index 6a36be1e63..96ae5d5880 100755
> --- a/t/t7800-difftool.sh
> +++ b/t/t7800-difftool.sh
> @@ -91,58 +91,67 @@ test_expect_success 'difftool forwards arguments to diff' '
> rm for-diff
> '
>
> -test_expect_success 'difftool ignores exit code' '
> - test_config difftool.error.cmd false &&
> - git difftool -y -t error branch
> -'
> +for opt in '' '--dir-diff'
> +do
> + test_expect_success "difftool ${opt} ignores exit code" "
> + test_config difftool.error.cmd false &&
> + git difftool ${opt} -y -t error branch
> + "
>
> -test_expect_success 'difftool forwards exit code with --trust-exit-code' '
> - test_config difftool.error.cmd false &&
> - test_must_fail git difftool -y --trust-exit-code -t error branch
> -'
> + test_expect_success "difftool ${opt} forwards exit code with --trust-exit-code" "
> + test_config difftool.error.cmd false &&
> + test_must_fail git difftool ${opt} -y --trust-exit-code -t error branch
> + "
>
> -test_expect_success 'difftool forwards exit code with --trust-exit-code for built-ins' '
> - test_config difftool.vimdiff.path false &&
> - test_must_fail git difftool -y --trust-exit-code -t vimdiff branch
> -'
> + test_expect_success "difftool ${opt} forwards exit code with --trust-exit-code for built-ins" "
> + test_config difftool.vimdiff.path false &&
> + test_must_fail git difftool ${opt} -y --trust-exit-code -t vimdiff branch
> + "
>
> -test_expect_success 'difftool honors difftool.trustExitCode = true' '
> - test_config difftool.error.cmd false &&
> - test_config difftool.trustExitCode true &&
> - test_must_fail git difftool -y -t error branch
> -'
> + test_expect_success "difftool ${opt} honors difftool.trustExitCode = true" "
> + test_config difftool.error.cmd false &&
> + test_config difftool.trustExitCode true &&
> + test_must_fail git difftool ${opt} -y -t error branch
> + "
>
> -test_expect_success 'difftool honors difftool.trustExitCode = false' '
> - test_config difftool.error.cmd false &&
> - test_config difftool.trustExitCode false &&
> - git difftool -y -t error branch
> -'
> + test_expect_success "difftool ${opt} honors difftool.trustExitCode = false" "
> + test_config difftool.error.cmd false &&
> + test_config difftool.trustExitCode false &&
> + git difftool ${opt} -y -t error branch
> + "
>
> -test_expect_success 'difftool ignores exit code with --no-trust-exit-code' '
> - test_config difftool.error.cmd false &&
> - test_config difftool.trustExitCode true &&
> - git difftool -y --no-trust-exit-code -t error branch
> -'
> + test_expect_success "difftool ${opt} ignores exit code with --no-trust-exit-code" "
> + test_config difftool.error.cmd false &&
> + test_config difftool.trustExitCode true &&
> + git difftool ${opt} -y --no-trust-exit-code -t error branch
> + "
>
> -test_expect_success 'difftool stops on error with --trust-exit-code' '
> - test_when_finished "rm -f for-diff .git/fail-right-file" &&
> - test_when_finished "git reset -- for-diff" &&
> - write_script .git/fail-right-file <<-\EOF &&
> - echo failed
> - exit 1
> - EOF
> - >for-diff &&
> - git add for-diff &&
> - test_must_fail git difftool -y --trust-exit-code \
> - --extcmd .git/fail-right-file branch >actual &&
> - test_line_count = 1 actual
> -'
> + test_expect_success "difftool ${opt} stops on error with --trust-exit-code" "
> + test_when_finished 'rm -f for-diff .git/fail-right-file' &&
> + test_when_finished 'git reset -- for-diff' &&
> + write_script .git/fail-right-file <<-\EOF &&
> + echo failed
> + exit 1
> + EOF
> + >for-diff &&
> + git add for-diff &&
> + test_must_fail git difftool ${opt} -y --trust-exit-code \
> + --extcmd .git/fail-right-file branch >actual &&
> + test_line_count = 1 actual
> + "
>
> -test_expect_success 'difftool honors exit status if command not found' '
> - test_config difftool.nonexistent.cmd i-dont-exist &&
> - test_config difftool.trustExitCode false &&
> - test_must_fail git difftool -y -t nonexistent branch
> -'
> + test_expect_success "difftool ${opt} honors exit status if command not found" "
> + test_config difftool.nonexistent.cmd i-dont-exist &&
> + test_config difftool.trustExitCode false &&
> + if test "${opt}" = '--dir-diff'
The quoting doesn't quite work here. When $opt is empty, this results
in:
expecting success of 7800.14 'difftool honors exit status if command not found':
test_config difftool.nonexistent.cmd i-dont-exist &&
test_config difftool.trustExitCode false &&
if test = '--dir-diff'
then
expected_code=127
else
expected_code=128
fi &&
test_expect_code ${expected_code} git difftool -y -t nonexistent branch
+ test_config difftool.nonexistent.cmd i-dont-exist
+ test_config difftool.trustExitCode false
+ test = --dir-diff
./t7800-difftool.sh: 14: test: =: unexpected operator
> + then
> + expected_code=127
> + else
> + expected_code=128
> + fi &&
> + test_expect_code \${expected_code} git difftool ${opt} -y -t nonexistent branch
> + "
> +done
>
> test_expect_success 'difftool honors --gui' '
> difftool_test_setup &&
> --
> 2.44.0-rc1
>
^ permalink raw reply
* Re: [PATCH v4 2/2] index-pack: --fsck-objects to take an optional argument for fsck msgs
From: SZEDER Gábor @ 2024-03-08 22:24 UTC (permalink / raw)
To: John Cai via GitGitGadget; +Cc: git, Jonathan Tan, Patrick Steinhardt, John Cai
In-Reply-To: <f29ab9136fb4c23c5700a73731a5e220f92b7c30.1706751483.git.gitgitgadget@gmail.com>
On Thu, Feb 01, 2024 at 01:38:02AM +0000, John Cai via GitGitGadget wrote:
> diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh
> index 496fffa0f8a..a58f91035d1 100755
> --- a/t/t5300-pack-object.sh
> +++ b/t/t5300-pack-object.sh
> @@ -441,8 +441,7 @@ test_expect_success 'index-pack with --strict' '
> )
> '
>
> -test_expect_success 'index-pack with --strict downgrading fsck msgs' '
> - test_when_finished rm -rf strict &&
> +test_expect_success 'setup for --strict and --fsck-objects downgrading fsck msgs' '
> git init strict &&
> (
> cd strict &&
> @@ -457,12 +456,32 @@ test_expect_success 'index-pack with --strict downgrading fsck msgs' '
>
> EOF
> git hash-object --literally -t commit -w --stdin <commit >commit_list &&
> - PACK=$(git pack-objects test <commit_list) &&
> - test_must_fail git index-pack --strict "test-$PACK.pack" &&
> - git index-pack --strict="missingEmail=ignore" "test-$PACK.pack"
> + git pack-objects test <commit_list >pack-name
> )
> '
>
> +test_with_bad_commit () {
> + must_fail_arg="$1" &&
> + must_pass_arg="$2" &&
> + (
> + cd strict &&
> + test_expect_fail git index-pack "$must_fail_arg" "test-$(cat pack-name).pack"
There is no such command as 'test_expect_fail', resulting in:
expecting success of 5300.34 'index-pack with --strict downgrading fsck msgs':
test_with_bad_commit --strict --strict="missingEmail=ignore"
+ test_with_bad_commit --strict --strict=missingEmail=ignore
+ must_fail_arg=--strict
+ must_pass_arg=--strict=missingEmail=ignore
+ cd strict
+ cat pack-name
+ test_expect_fail git index-pack --strict test-e4e1649155bf444fbd9cd85e376628d6eaf3d3bd.pack
./t5300-pack-object.sh: 468: eval: test_expect_fail: not found
+ cat pack-name
+ git index-pack --strict=missingEmail=ignore test-e4e1649155bf444fbd9cd85e376628d6eaf3d3bd.pack
e4e1649155bf444fbd9cd85e376628d6eaf3d3bd
ok 34 - index-pack with --strict downgrading fsck msgs
The missing command should fail the test, but it doesn't, because the
&&-chain is broken on this line as well.
^ permalink raw reply
* Re: [PATCH 2/2] setup: make bareRepository=explicit work in GIT_DIR of a secondary worktree
From: Junio C Hamano @ 2024-03-08 22:30 UTC (permalink / raw)
To: git; +Cc: Kyle Lippincott
In-Reply-To: <20240308211957.3758770-3-gitster@pobox.com>
Junio C Hamano <gitster@pobox.com> writes:
> In the previous commit, we created a helper function to house the
> logic that checks if a directory that looks like a bare repository
> is actually a part of a non-bare repository. Extend the helper
> function to also check if the apparent bare-repository is a $GIT_DIR
> of a secondary worktree, by checking three things:
>
> * The path to the $GIT_DIR must be a subdirectory of
> ".git/worktrees/", which is the primary worktree [*].
>
> * Such $GIT_DIR must have file "gitdir", that records the path of
> the ".git" file that is at the root level of the secondary
> worktree.
>
> * That ".git" file in turn points back at the $GIT_DIR we are
> inspecting.
>
> The latter two points are merely for checking sanity. The security
> lies in the first requirement.
>
> Remember that a tree object with an entry whose pathname component
> is ".git" is forbidden at various levels (fsck, object transfer and
> checkout), so malicious projects cannot cause users to clone and
> checkout a crafted ".git" directory in a shell directory that
> pretends to be a working tree with that ".git" thing at its root
> level. That is where 45bb9162 (setup: allow cwd=.git w/
> bareRepository=explicit, 2024-01-20) draws its security guarantee
> from. And the solution for secondary worktrees in this commit draws
> its security guarantee from the same place.
I wrote the "[*]" mark but forgot to add a footnote with an
additional information for it. Something like this was what I had
in mind to write there:
[Footnote]
* This does not help folks who create a new worktree out of a bare
repository, because in their set-up, there won't be "/.git/" in
front of "worktrees" directory. It is fundamentally impossible
to lift this limitation, as long as safe.bareRepository is
considered to be a meaningful security measure. The security of
both the loosening for a secondary worktree's GIT_DIR as well as
the loosening for the GIT_DIR of the primary worktree, hinge on
the fact that ".git/" directory is impossible to create as
payload to be cloned.
^ permalink raw reply
* Re: [PATCH v2] git-difftool--helper: honor `--trust-exit-code` with `--dir-diff`
From: Junio C Hamano @ 2024-03-08 22:36 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: Patrick Steinhardt, git, Jean-Rémy Falleri
In-Reply-To: <20240308221229.GA1908@szeder.dev>
SZEDER Gábor <szeder.dev@gmail.com> writes:
>> -test_expect_success 'difftool honors exit status if command not found' '
>> - test_config difftool.nonexistent.cmd i-dont-exist &&
>> - test_config difftool.trustExitCode false &&
>> - test_must_fail git difftool -y -t nonexistent branch
>> -'
>> + test_expect_success "difftool ${opt} honors exit status if command not found" "
>> + test_config difftool.nonexistent.cmd i-dont-exist &&
>> + test_config difftool.trustExitCode false &&
>> + if test "${opt}" = '--dir-diff'
>
> The quoting doesn't quite work here.
Thanks for looking at them carefully.
In general, when you want to interpolate an variable that exists
outside test_expect_success, you should write it this way:
for var in a "b c"
do
test_expect_success "message with $var interpolated" '
command and "$var" as its argument
'
done
The last parameter to test_expect_{success,failure} is eval'ed, so
enclose it within a pair of single quotes, and let the eval to
interpolate references to $variables at runtime (as opposed to when
the parameters to test_expect_success are formulated) avoids a lot
of surprises and headaches.
Perhaps we should have something like the above as a hint in
t/README?
^ permalink raw reply
* Re: [PATCH 2/2] setup: make bareRepository=explicit work in GIT_DIR of a secondary worktree
From: Kyle Lippincott @ 2024-03-08 23:10 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <20240308211957.3758770-3-gitster@pobox.com>
On Fri, Mar 8, 2024 at 1:20 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> If you have /var/tmp/primary/ as a repository, and if you create a
> secondary worktree of it at /var/tmp/secondary/, the layout would
> look like this:
>
> $ cd /var/tmp/
> $ git init primary
> $ cd primary
> $ pwd
> /var/tmp/primary
> $ git worktree add ../secondary
> $ cat ../seconary/.git
Nit: typo, should be `secondary` (missing the `d`)
> gitdir: /var/tmp/primary/.git/worktrees/secondary
> $ ls /var/tmp/primary/.git/worktrees/secondary
> commondir gitdir HEAD index refs
> $ cat /var/tmp/primary/.git/worktrees/secondary/gitdir
> /var/tmp/secondary/.git
>
> When the configuration variable 'safe.bareRepository=explicit' is
> set to explicit, the change made by 45bb9162 (setup: allow cwd=.git
> w/ bareRepository=explicit, 2024-01-20) allows you to work in the
> /var/tmp/primary/.git directory (i.e., $GIT_DIR of the primary
> worktree). The idea is that if it is safe to work in the repository
> in its working tree, it should be equally safe to work in the
> ".git/" directory of that working tree, too.
>
> Now, for the same reason, let's allow command execution from within
> the $GIT_DIR directory of a secondary worktree. This is useful for
> tools working with secondary worktrees when the 'bareRepository'
> setting is set to 'explicit'.
>
> In the previous commit, we created a helper function to house the
> logic that checks if a directory that looks like a bare repository
> is actually a part of a non-bare repository. Extend the helper
> function to also check if the apparent bare-repository is a $GIT_DIR
> of a secondary worktree, by checking three things:
>
> * The path to the $GIT_DIR must be a subdirectory of
> ".git/worktrees/", which is the primary worktree [*].
>
> * Such $GIT_DIR must have file "gitdir", that records the path of
> the ".git" file that is at the root level of the secondary
> worktree.
>
> * That ".git" file in turn points back at the $GIT_DIR we are
> inspecting.
>
> The latter two points are merely for checking sanity. The security
> lies in the first requirement.
>
> Remember that a tree object with an entry whose pathname component
> is ".git" is forbidden at various levels (fsck, object transfer and
> checkout), so malicious projects cannot cause users to clone and
> checkout a crafted ".git" directory in a shell directory that
> pretends to be a working tree with that ".git" thing at its root
> level. That is where 45bb9162 (setup: allow cwd=.git w/
> bareRepository=explicit, 2024-01-20) draws its security guarantee
> from. And the solution for secondary worktrees in this commit draws
> its security guarantee from the same place.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> setup.c | 52 ++++++++++++++++++++++++++++++++-
> t/t0035-safe-bare-repository.sh | 8 ++++-
> 2 files changed, 58 insertions(+), 2 deletions(-)
>
> diff --git a/setup.c b/setup.c
> index 3081be4970..68860dcd18 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -1231,9 +1231,59 @@ static const char *allowed_bare_repo_to_string(
> return NULL;
> }
>
> +static int is_git_dir_of_secondary_worktree(const char *path)
> +{
> + int result = 0; /* assume not */
> + struct strbuf gitfile_here = STRBUF_INIT;
> + struct strbuf gitfile_there = STRBUF_INIT;
> + const char *gitfile_contents;
> + int error_code = 0;
> +
> + /*
> + * We should be a subdirectory of /.git/worktrees inside
> + * the $GIT_DIR of the primary worktree.
> + *
> + * NEEDSWORK: some folks create secondary worktrees out of a
> + * bare repository; they don't count ;-), at least not yet.
> + */
> + if (!strstr(path, "/.git/worktrees/"))
Do we need to be concerned about path separators being different on
Windows? Or have they already been normalized here?
> + goto out;
> +
> + /*
> + * Does gitdir that points at the ".git" file at the root of
> + * the secondary worktree roundtrip here?
> + */
What loss of security do we have if we don't have as stringent of a
check? i.e. if we just did `return !!strstr(path, "/.git/worktrees/)`?
Or maybe we even combine the existing ends_with(.git) check with this
and do something like:
static int is_under_dotgit_dir(const char *path) {
char *dotgit = strstr(path, "/.git");
return dotgit && (dotgit[5] == '\0' || dotgit[5] == '/');
}
> + strbuf_addf(&gitfile_here, "%s/gitdir", path);
> + if (!file_exists(gitfile_here.buf))
> + goto out;
> + if (strbuf_read_file(&gitfile_there, gitfile_here.buf, 0) < 0)
> + goto out;
> + strbuf_trim_trailing_newline(&gitfile_there);
> +
> + gitfile_contents = read_gitfile_gently(gitfile_there.buf, &error_code);
> + if ((!gitfile_contents) || strcmp(gitfile_contents, path))
> + goto out;
> +
> + /* OK, we are happy */
> + result = 1;
> +
> +out:
> + strbuf_release(&gitfile_here);
> + strbuf_release(&gitfile_there);
> + return result;
> +}
> +
> static int is_repo_with_working_tree(const char *path)
> {
> - return ends_with_path_components(path, ".git");
> + /* $GIT_DIR immediately below the primary working tree */
> + if (ends_with_path_components(path, ".git"))
> + return 1;
> +
> + /* Are we in $GIT_DIR of a secondary worktree? */
> + if (is_git_dir_of_secondary_worktree(path))
> + return 1;
> +
> + return 0;
> }
>
> /*
> diff --git a/t/t0035-safe-bare-repository.sh b/t/t0035-safe-bare-repository.sh
> index 8048856379..62cdfcefc1 100755
> --- a/t/t0035-safe-bare-repository.sh
> +++ b/t/t0035-safe-bare-repository.sh
> @@ -31,7 +31,9 @@ expect_rejected () {
>
> test_expect_success 'setup bare repo in worktree' '
> git init outer-repo &&
> - git init --bare outer-repo/bare-repo
> + git init --bare outer-repo/bare-repo &&
> + git -C outer-repo worktree add ../outer-secondary &&
> + test_path_is_dir outer-secondary
> '
>
> test_expect_success 'safe.bareRepository unset' '
> @@ -86,4 +88,8 @@ test_expect_success 'no trace when "bare repository" is a subdir of .git' '
> expect_accepted_implicit -C outer-repo/.git/objects
> '
>
> +test_expect_success 'no trace in $GIT_DIR of secondary worktree' '
> + expect_accepted_implicit -C outer-repo/.git/worktrees/outer-secondary
> +'
> +
> test_done
> --
> 2.44.0-165-ge09f1254c5
>
^ permalink raw reply
* Re: [PATCH 2/2] setup: make bareRepository=explicit work in GIT_DIR of a secondary worktree
From: Junio C Hamano @ 2024-03-08 23:32 UTC (permalink / raw)
To: Kyle Lippincott; +Cc: git
In-Reply-To: <CAO_smVjrKJeKr7QgQWryZRErStFk=Y+1T=dwrR_boXQD_X9_Mg@mail.gmail.com>
Kyle Lippincott <spectral@google.com> writes:
>> $ cat ../seconary/.git
>
> Nit: typo, should be `secondary` (missing the `d`)
Good eyes. Thanks.
>> + /*
>> + * We should be a subdirectory of /.git/worktrees inside
>> + * the $GIT_DIR of the primary worktree.
>> + *
>> + * NEEDSWORK: some folks create secondary worktrees out of a
>> + * bare repository; they don't count ;-), at least not yet.
>> + */
>> + if (!strstr(path, "/.git/worktrees/"))
>
> Do we need to be concerned about path separators being different on
> Windows? Or have they already been normalized here?
I am not certain offhand, but if they need to tolerate different
separators, they can send in patches.
>> + goto out;
>> +
>> + /*
>> + * Does gitdir that points at the ".git" file at the root of
>> + * the secondary worktree roundtrip here?
>> + */
>
> What loss of security do we have if we don't have as stringent of a
> check? i.e. if we just did `return !!strstr(path, "/.git/worktrees/)`?
No loss of security.
These "keep result the status we want to return if we want to return
immediately here, and always jump to the out label instead of
returning" patterns is mere a disciplined way to make it easier to
write code that does not leak. The very first one may not have to
do the "goto out" and instead immediately return, but by writing
this way, I do not need to be always looking out to shoot down
patches that adds new check and/or allocations before this
condition and early "out".
> Or maybe we even combine the existing ends_with(.git) check with this
At the mechanical level, perhaps, but I'd want logically separate
things treated as distinct cases. One is about being inside
$GIT_DIR of the primary worktrees (where more than majority of users
will encounter) and the new one is about being inside $GIT_DIR of
secondaries.
^ permalink raw reply
* Re: [PATCH 2/2] setup: make bareRepository=explicit work in GIT_DIR of a secondary worktree
From: Kyle Lippincott @ 2024-03-09 0:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqy1ase1vo.fsf@gitster.g>
On Fri, Mar 8, 2024 at 3:32 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Kyle Lippincott <spectral@google.com> writes:
>
> >> $ cat ../seconary/.git
> >
> > Nit: typo, should be `secondary` (missing the `d`)
>
> Good eyes. Thanks.
>
> >> + /*
> >> + * We should be a subdirectory of /.git/worktrees inside
> >> + * the $GIT_DIR of the primary worktree.
> >> + *
> >> + * NEEDSWORK: some folks create secondary worktrees out of a
> >> + * bare repository; they don't count ;-), at least not yet.
> >> + */
> >> + if (!strstr(path, "/.git/worktrees/"))
> >
> > Do we need to be concerned about path separators being different on
> > Windows? Or have they already been normalized here?
>
> I am not certain offhand, but if they need to tolerate different
> separators, they can send in patches.
>
> >> + goto out;
> >> +
> >> + /*
> >> + * Does gitdir that points at the ".git" file at the root of
> >> + * the secondary worktree roundtrip here?
> >> + */
> >
> > What loss of security do we have if we don't have as stringent of a
> > check? i.e. if we just did `return !!strstr(path, "/.git/worktrees/)`?
>
> No loss of security.
Then should we just do that?
+ /* Assumption: `path` points to the root of a $GIT_DIR. */
static int is_repo_with_working_tree(const char *path)
{
- return ends_with_path_components(path, ".git");
+ /* $GIT_DIR immediately below the primary working tree */
+ if (ends_with_path_components(path, ".git"))
+ return 1;
+
+ /*
+ * Also allow $GIT_DIRs in secondary worktrees.
+ * These do not end in .git, but are still considered safe because
+ * of the .git component in the path.
+ */
+ if (strstr(path, "/.git/worktrees/"))
+ return 1;
+
+ return 0;
}
>
> These "keep result the status we want to return if we want to return
> immediately here, and always jump to the out label instead of
> returning" patterns is mere a disciplined way to make it easier to
> write code that does not leak. The very first one may not have to
> do the "goto out" and instead immediately return, but by writing
> this way, I do not need to be always looking out to shoot down
> patches that adds new check and/or allocations before this
> condition and early "out".
>
> > Or maybe we even combine the existing ends_with(.git) check with this
>
> At the mechanical level, perhaps, but I'd want logically separate
> things treated as distinct cases. One is about being inside
> $GIT_DIR of the primary worktrees (where more than majority of users
> will encounter) and the new one is about being inside $GIT_DIR of
> secondaries.
^ 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