* Re: [PATCH v6 00/14] Introduce new `git replay` command
From: Johannes Schindelin @ 2023-11-08 12:47 UTC (permalink / raw)
To: Christian Couder
Cc: git, Junio C Hamano, Patrick Steinhardt, Elijah Newren, John Cai,
Derrick Stolee, Phillip Wood, Calvin Wan, Toon Claes,
Dragan Simic, Linus Arver
In-Reply-To: <20231102135151.843758-1-christian.couder@gmail.com>
Hi Christian,
On Thu, 2 Nov 2023, Christian Couder wrote:
> # Range-diff between v5 and v6
>
> (Sorry it's very long mostly due to doc and test changes over a number
> of patches.)
I am grateful for the detailed range-diff that let's me focus exclusively
on what changed between iterations.
> 1: 72c34a0eba = 1: fac0a9dff4 t6429: remove switching aspects of fast-rebase
> 2: f85e6c823c ! 2: 8a605ddef8 replay: introduce new builtin
> @@ Commit message
> For now, this is just a rename from `t/helper/test-fast-rebase.c` into
> `builtin/replay.c` with minimal changes to make it build appropriately.
>
> + There is a stub documentation and a stub test script though.
> +
> Subsequent commits will flesh out its capabilities and make it a more
> standard regular builtin.
>
> @@ .gitignore
> /git-rerere
> /git-reset
>
> + ## Documentation/git-replay.txt (new) ##
> +@@
> ++git-replay(1)
> ++=============
> ++
> ++NAME
> ++----
> ++git-replay - EXPERIMENTAL: Replay commits on a new base, works with bare repos too
> ++
> ++
> ++SYNOPSIS
> ++--------
> ++[verse]
> ++'git replay' --onto <newbase> <revision-range>... # EXPERIMENTAL
Technically, at this stage `git replay` requires precisely 5 arguments, so
the `<revision>...` is incorrect and should be `<upstream> <branch>`
instead. But it's not worth a new iteration to fix this.
> ++
> ++DESCRIPTION
> ++-----------
> ++
> ++Takes a range of commits and replays them onto a new location.
> ++
> ++THIS COMMAND IS EXPERIMENTAL. THE BEHAVIOR MAY CHANGE.
> ++
> ++OPTIONS
> ++-------
> ++
> ++--onto <newbase>::
> ++ Starting point at which to create the new commits. May be any
> ++ valid commit, and not just an existing branch name.
> ++
Traditionally, this would be a place to describe the `<revision>` argument
(or, in this patch, to reflect the current state of `builtin/replay.c`,
the `<upstream> <branch>` arguments instead).
> ++EXIT STATUS
> ++-----------
> ++
> ++For a successful, non-conflicted replay, the exit status is 0. When
> ++the replay has conflicts, the exit status is 1. If the replay is not
> ++able to complete (or start) due to some kind of error, the exit status
> ++is something other than 0 or 1.
> ++
> ++GIT
> ++---
> ++Part of the linkgit:git[1] suite
> +
> ## Makefile ##
> [... snipping non-controversial changes ...]
> 6: 37d545d5d6 ! 6: edafe4846f replay: change rev walking options
> [... snipping changes Elijah already talked about ...]
> 7: 2943f08926 = 7: b81574744a replay: add an important FIXME comment about gpg signing
> 8: f81962ba41 = 8: b08ad2b2f1 replay: remove progress and info output
> 9: 236747497e ! 9: 5099c94d2e replay: remove HEAD related sanity check
> {... snipping non-controversial changes ...]
Thank you!
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH v5 00/14] Introduce new `git replay` command
From: Johannes Schindelin @ 2023-11-08 12:25 UTC (permalink / raw)
To: Christian Couder
Cc: Elijah Newren, git, Junio C Hamano, Patrick Steinhardt, John Cai,
Derrick Stolee, Phillip Wood, Calvin Wan, Toon Claes,
Dragan Simic, Linus Arver
In-Reply-To: <CAP8UFD09dZbrbebRFZvarY71q5Vc0YBfRQHbTg7A3H-qM2g8fg@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1050 bytes --]
Hi Christian,
On Thu, 2 Nov 2023, Christian Couder wrote:
> On Thu, Oct 26, 2023 at 3:44 PM Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
>
> > In addition, I am still a bit uneasy with introducing both the manual page
> > and the test script in this commit (see my comments in
> > https://lore.kernel.org/git/03460733-0219-c648-5757-db1958f8042e@gmx.de/).
> > It would be better to uphold our high standard and introduce scaffolds for
> > both files in the first commit, then populate the file contents
> > incrementally in the same the patches that introduce the corresponding
> > options/features/changes.
>
> I have tried to improve on that in the v6 I just sent, but there are
> many patches implementing changes in behavior that I think weren't
> worth documenting and testing in `test-tool fast-rebase` (which had no
> doc and no test) and that aren't worth documenting and testing
> specifically in `git replay` either.
Thank you for putting in the effort. I appreciate it very much.
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH v5 00/14] Introduce new `git replay` command
From: Johannes Schindelin @ 2023-11-08 12:25 UTC (permalink / raw)
To: Christian Couder
Cc: Elijah Newren, git, Junio C Hamano, Patrick Steinhardt, John Cai,
Derrick Stolee, Phillip Wood, Calvin Wan, Toon Claes,
Dragan Simic, Linus Arver
In-Reply-To: <CAP8UFD04PbOLgep3-s-_4_fMbqtrWMR87K1cG-DsKnbyHEBT_w@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2028 bytes --]
Hi Christian,
On Thu, 2 Nov 2023, Christian Couder wrote:
> On Sun, Oct 29, 2023 at 7:02 AM Elijah Newren <newren@gmail.com> wrote:
>
> > On Thu, Oct 26, 2023 at 6:44 AM Johannes Schindelin
> > <Johannes.Schindelin@gmx.de> wrote:
> > >
> > > As mentioned in
> > > https://lore.kernel.org/git/03460733-0219-c648-5757-db1958f8042e@gmx.de/,
> > > I would like the `EXPERIMENTAL` label to be shown prominently here.
> > > Probably not only the `SYNOPSIS` as I had originally suggested but
> > > also in the `NAME`.
>
> Ok, I have made changes in the v6 I just sent, so that there is
> EXPERIMENTAL both in the NAME and SYNOPSIS.
>
> > > Otherwise we may end up with the same situation as with the (from my
> > > perspective, failed) `git switch`/`git restore` experiment, where we
> > > wanted to explore a better user experience than the overloaded `git
> > > checkout` command, only to now be stuck with having to maintain
> > > backward-compatibility for `git switch`/`git restore` command-line options
> > > that were not meant to be set in stone but to be iterated on, instead. A
> > > real-life demonstration of [Hyrum's Law](hyrumslaw.com/), if you like. Or,
> > > from a different angle, we re-enacted https://xkcd.com/927/ in a way.
>
> Nit: Hyrum's Law says:
>
> "With a sufficient number of users of an API,
> it does not matter what you promise in the contract:
> all observable behaviors of your system
> will be depended on by somebody."
>
> The doc is part of the contract, which according to this law doesn't
> matter. So I don't see why you use this law to suggest a doc change.
You're right. In addition to the documentation (where we definitely need
to state the experimental nature of the command), we may want to consider
adding the `EXPERIMENTAL` label not only to the output of `git replay -h`,
but show also a warning for every `git replay` invocation cautioning users
against depending on the current command-line options of this command.
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH v6 00/14] Introduce new `git replay` command
From: Johannes Schindelin @ 2023-11-08 12:19 UTC (permalink / raw)
To: Elijah Newren
Cc: Christian Couder, git, Junio C Hamano, Patrick Steinhardt,
John Cai, Derrick Stolee, Phillip Wood, Calvin Wan, Toon Claes,
Dragan Simic, Linus Arver
In-Reply-To: <CABPp-BE6G2qaF50bhz-CwxSsvxGDHzwvsWtfQO4zVcX6ERppLw@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1402 bytes --]
Hi,
On Mon, 6 Nov 2023, Elijah Newren wrote:
> On Thu, Nov 2, 2023 at 6:52 AM Christian Couder
> <christian.couder@gmail.com> wrote:
> >
> [...]
>
> > @@ builtin/replay.c: int cmd_replay(int argc, const char **argv, const char *prefix
> > -
> > strvec_pushl(&rev_walk_args, "", argv[2], "--not", argv[1], NULL);
> >
> > ++ /*
> > ++ * TODO: For now, let's warn when we see an option that we are
> > ++ * going to override after setup_revisions() below. In the
> > ++ * future we might want to either die() or allow them if we
> > ++ * think they could be useful though.
> > ++ */
> > ++ for (i = 0; i < argc; i++) {
> > ++ if (!strcmp(argv[i], "--reverse") || !strcmp(argv[i], "--date-order") ||
> > ++ !strcmp(argv[i], "--topo-order") || !strcmp(argv[i], "--author-date-order") ||
> > ++ !strcmp(argv[i], "--full-history"))
> > ++ warning(_("option '%s' will be overridden"), argv[i]);
> > ++ }
> > ++
>
> [...] This seems like an inefficient way to provide this warning;
Not only inefficient: think about the false positive when looking at a
command-line containing `--grep --reverse`. In this instance, `--reverse`
is an argument of the `--grep` option, but would be mistaken for an option
in its own right.
Ciao,
Johannes
^ permalink raw reply
* Re: Error when "git mv" file in a sparsed checkout
From: Josef Wolf @ 2023-11-08 11:36 UTC (permalink / raw)
To: git
In-Reply-To: <CABPp-BEAU8rPeNHphut0ZxcLdH0pzjh+Z_CF+rg2uhvVZoZfxg@mail.gmail.com>
Thanks for the reply, Elijah!
On Tue, Nov 07, 2023 at 06:21:00PM -0800, Elijah Newren wrote:
> On Tue, Nov 7, 2023 at 5:32 AM Josef Wolf <jw@raven.inka.de> wrote:
> > I have used the procedure described below for many years. In fact,
> > this procedure is part of a script which I am using for about 10 years.
> > This procedure was definitely working with git-2-25-1 and git-2.26.2.
> >
> > Now, with git-2.34.1 (on a freshly installed ubuntu-22.04), this
> > procedure fails.
> >
> > Here is what I do:
> >
> > I want to rename a file on a branch which is not currently checked out
> > without messing/touching my current working directory.
> >
> > For this, I first create a clone of the repo with shared git-directory:
> >
> > $ SANDBOX=/var/tmp/manage-scans-X1pKZQiey
> > $ WT=$SANDBOX/wt
> > $ GIT=$SANDBOX/git
> >
> > $ mkdir -p $SANDBOX
> > $ git --work-tree $WT --git-dir $GIT clone -qns -n ~/upstream-repo $GIT
> >
> > Then, I do a sparse checkout in this clone, containing only the file
> > that is to be renamed:
> >
> > $ cd $WT
> > $ echo 'path/to/old-filename' >>$GIT/info/sparse-checkout
> > $ git --work-tree $WT --git-dir $GIT config core.sparsecheckout true
> > $ git --work-tree $WT --git-dir $GIT checkout -b the-branch remotes/origin/the-branch
> > Switched to a new branch 'the-branch'
> >
> > Next step would be to "git mv" the file:
> >
> > $ mkdir -p /path/to # already exists, but should do no harm
> > $ git --work-tree $WT --git-dir $GIT mv path/to/old-filename path/to/new-filename
>
> sparse checkouts are designed such that only files matching the
> patterns in the sparse-checkout file should be present in the working
> tree, so renaming to a path that should not be present is problematic.
> We could possibly have "git-mv" immediately remove the path from the
> working tree (while leaving the new pathname in the index), but that's
> problematic in that users often overlook the index and only look at
> the working tree and might think the file was deleted instead of
> renamed. Not immediately removing it is potentially even worse,
> because any subsequent operation (particularly ones like checkout,
> reset, merge, rebase, etc.) are likely to nuke the file from the
> working tree and the fact that the removal is delayed makes it much
> harder for users to understand and diagnose.
>
> So, Stolee fixed this to make it throw an error; see
> https://lore.kernel.org/git/pull.1018.v4.git.1632497954.gitgitgadget@gmail.com/
> for details. His description did focus on cone mode, but you'll note
> that none of my explanation here did. The logic for making this an
> error fully applies to non-cone mode for all the same reasons.
>
> If you want to interact with `path/to/new-filename` as a path within
> your sparse checkout (as suggested by your git-mv command), then that
> path should actually be part of your sparse checkout. In other words,
> you should add `path/to/new-filename` to $GIT/info/sparse-checkout and
> do so _before_ attempting your `git mv` command. If you don't like
> that for some reason, you are allowed to instead ignore the
> problematic consequences of renaming outside the sparse-checkout by
> providing the `--sparse` flag. Both of these possibilities are
> documented in the hints provided along with the error message you
> showed below:
>
> > The following paths and/or pathspecs matched paths that exist
> > outside of your sparse-checkout definition, so will not be
> > updated in the index:
> > path/to/new-filename
> > hint: If you intend to update such entries, try one of the following:
> > hint: * Use the --sparse option.
> > hint: * Disable or modify the sparsity rules.
> > hint: Disable this message with "git config advice.updateSparsePath false"
> >
> > This error is something I have not expected.
> >
> > Error message suggests, there already exists a file named "new-filename". This
> > is not true at all. There is no file named "new-filename" in the entire
> > repository. Not in any directory of any branch.
>
> You are correct; the wording of the error message here is suboptimal
> and seems to have been focused more on the git-add case (the error
> message is shared by git-add, git-mv, and git-rm). Thanks for
> pointing it out! We could improve that wording, perhaps with
> something like:
>
> The following paths and/or pathspecs match paths that are
> outside of your sparse-checkout definition, so will not be
> updated:
>
> Which is still slightly slanted towards git-add and git-rm cases, but
> I hope it works better than the current message. Thoughts?
Yes, the wording was pretty much confusing me, since i could not find a file
named "new-file" anywhere in the repo.
There are more things confusing concerning sparse mode:
- It is not clear from git-sparse-checkout(1) when changes to
$GIT_DIR/info/sparse-checkout are catched up. In my case: would it be enough
to add the new pathname just before git-mv or would a fresh git-checkout be
needed after modifying $GIT_DIR/info/sparse-checkout? You have clarified
this in your response, but shouldn't this be clear from the manpage?
- git-sparse-checkout(1) refers to "skip-worktree bit". This concept is
potentially not very familiar to the average git user which uses mostly
porcelain. Thus, edge cases remain to be unclear.
- The pathspecs refers to .gitignore (which by itself is not very clear). But
there are differences:
1. giignore is relative to containing directoy, which don't seem to make
much sense for sparse mode
2. sparse specs are the opposite of gitignore, which seems to have different
meaning in some edge-cases.
- For cone, it is not clear how the two "accepted patterns" look like what the
semantics are. I understand that specifying a directory adds siblings
recursively. But what does the "Parent" mode mean exactly and when/how is
this recognized? I guess, this is just a mis-namer? IMHO, parent of /a/b/c would
be /a/b and not /a/b/c/* (as git-sparse-chekout(1) suggests).
I guess all this is very clear to the core-developers. But for the occasional
user like myself, all this is pretty much confusing.
--
Josef Wolf
jw@raven.inka.de
^ permalink raw reply
* Re: [PATCH 1/3] t/lib-httpd: dynamically detect httpd and modules path
From: Patrick Steinhardt @ 2023-11-08 10:42 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqcywk7k0d.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 2351 bytes --]
On Wed, Nov 08, 2023 at 04:59:46PM +0900, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > In order to set up the Apache httpd server, we need to locate both the
> > httpd binary and its default module path. This is done with a hardcoded
> > list of locations that we scan. While this works okayish with distros
> > that more-or-less follow the Filesystem Hierarchy Standard, it falls
> > apart on others like NixOS that don't.
> >
> > While it is possible to specify these paths via `LIB_HTTPD_PATH` and
> > `LIB_HTTPD_MODULE_PATH`, it is not a nice experience for the developer
> > to figure out how to set those up. And in fact we can do better by
> > dynamically detecting both httpd and its module path at runtime:
> >
> > - The httpd binary can be located via PATH.
> >
> > - The module directory can (in many cases) be derived via the
> > `HTTPD_ROOT` compile-time variable.
> >
> > Refactor the code to do so. If runtime detection of the paths fails we
> > continue to fall back to the hardcoded list of paths.
>
> Hmph.
>
> I do not think we would want to punish the distros that follow the
> FHS that was created explicitly to help developers by standardizing
> locations of various things, with an approach this patch takes that
> throws everthing with bathwater and rely on $PATH first.
>
> Would it be sufficient to please NixOS if we simply append $(command
> -v apache) or whatever after the well known candidate locations?
I was a bit torn myself when writing this. I can also see a potential
future where we would drop the hardcoded list of locations altogether in
favor of always using PATH. After all we already rely on PATH to resolve
other tools as well, so why should httpd be special there?
But in the end I opted to use the more conservative approach of using
both PATH and the static list as I didn't want to break other distros. I
don't mind to make this even more conservative and resolve via PATH as a
last resort, only.
Patrick
> I know "command -v" is in POSIX, and on both bash and dash (the two
> shells most distros use), it works as this patch expects, but its
> portability is also a bit worrysome, especially because the whole
> point of this patch is to support a platform that is, eh, on the
> fringe.
>
> So, I dunno.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 2/9] for-each-ref: clarify interaction of --omit-empty & --count
From: Kristoffer Haugsbakk @ 2023-11-08 10:00 UTC (permalink / raw)
To: Øystein Walle; +Cc: Josh Soref, git, Victoria Dye
In-Reply-To: <CAFaJEquUBYUO=scjHw2qrUyP-4wZJWtdmWAtRrW0mH9x9PbZZw@mail.gmail.com>
On Wed, Nov 8, 2023, at 08:53, Øystein Walle wrote:
> On Tue, 7 Nov 2023 at 20:30, Victoria Dye <vdye@github.com> wrote:
>
>> Since the interaction isn't clearly defined at the moment, we could probably
>> still update it to work like you're describing here. I'm happy to drop this
>> patch and implement your recommendation in a follow-up series. Let me know
>> what you think!
>
> Regardless of whether the logic is changed in a follow-up series or not
> I think the current behavior is worth documenting even if it doesn't
> exist for much longer in the tree. So I am favor of having this patch as
> part of this series.
The funny thing though is that once it’s documented then you also kind of
commit yourself to it, right? That it’s how it’s supposed to behave.[1] If
you instead change the behavior (to the correct one) and document it in
the same series then there is no in-between time when people can claim to
rely on it via the documentation.
[1] Modulo “subject to change” hedging, but it seems that even
experimental commands who are documented as that are now resistant to
change in practice.
^ permalink raw reply
* Re: [PATCH 2/3] t/lib-httpd: stop using legacy crypt(3) for authentication
From: Junio C Hamano @ 2023-11-08 8:01 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <23835763002c5f5cd68db7bdc9e4c083dda3558f.1699428122.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> When setting up httpd for our tests, we also install a passwd and
> proxy-passwd file that contain the test user's credentials. These
> credentials currently use crypt(3) as the password encryption schema.
>
> This schema can be considered deprecated nowadays as it is not safe
> anymore. Quoting Apache httpd's documentation [1]:
>
>> Unix only. Uses the traditional Unix crypt(3) function with a
>> randomly-generated 32-bit salt (only 12 bits used) and the first 8
>> characters of the password. Insecure.
>
> This is starting to cause issues in modern Linux distributions. glibc
> has deprecated its libcrypt library that used to provide crypt(3) in
> favor of the libxcrypt library. This newer replacement provides a
> compile time switch to disable insecure password encryption schemata,
> which causes crypt(3) to always return `EINVAL`. The end result is that
> httpd tests that exercise authentication will fail on distros that use
> libxcrypt without these insecure encryption schematas.
>
> Regenerate the passwd files to instead use the default password
> encryption schema, which is md5. While it feels kind of funny that an
> MD5-based encryption schema should be more secure than anything else, it
> is the current default and supported by all platforms. Furthermore, it
> really doesn't matter all that much given that these files are only used
> for testing purposes anyway.
This step makes quite a lot of sense, as we are changing this not at
all for security but for portability ;-)
>
> [1]: https://httpd.apache.org/docs/2.4/misc/password_encryptions.html
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> t/lib-httpd/passwd | 2 +-
> t/lib-httpd/proxy-passwd | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/t/lib-httpd/passwd b/t/lib-httpd/passwd
> index 99a34d64874..d9c122f3482 100644
> --- a/t/lib-httpd/passwd
> +++ b/t/lib-httpd/passwd
> @@ -1 +1 @@
> -user@host:xb4E8pqD81KQs
> +user@host:$apr1$LGPmCZWj$9vxEwj5Z5GzQLBMxp3mCx1
> diff --git a/t/lib-httpd/proxy-passwd b/t/lib-httpd/proxy-passwd
> index 77c25138e07..2ad7705d9a3 100644
> --- a/t/lib-httpd/proxy-passwd
> +++ b/t/lib-httpd/proxy-passwd
> @@ -1 +1 @@
> -proxuser:2x7tAukjAED5M
> +proxuser:$apr1$RxS6MLkD$DYsqQdflheq4GPNxzJpx5.
^ permalink raw reply
* Re: [PATCH 1/3] t/lib-httpd: dynamically detect httpd and modules path
From: Junio C Hamano @ 2023-11-08 7:59 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <ac059db8fedc6493c64f703814c7db11adb4385e.1699428122.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> In order to set up the Apache httpd server, we need to locate both the
> httpd binary and its default module path. This is done with a hardcoded
> list of locations that we scan. While this works okayish with distros
> that more-or-less follow the Filesystem Hierarchy Standard, it falls
> apart on others like NixOS that don't.
>
> While it is possible to specify these paths via `LIB_HTTPD_PATH` and
> `LIB_HTTPD_MODULE_PATH`, it is not a nice experience for the developer
> to figure out how to set those up. And in fact we can do better by
> dynamically detecting both httpd and its module path at runtime:
>
> - The httpd binary can be located via PATH.
>
> - The module directory can (in many cases) be derived via the
> `HTTPD_ROOT` compile-time variable.
>
> Refactor the code to do so. If runtime detection of the paths fails we
> continue to fall back to the hardcoded list of paths.
Hmph.
I do not think we would want to punish the distros that follow the
FHS that was created explicitly to help developers by standardizing
locations of various things, with an approach this patch takes that
throws everthing with bathwater and rely on $PATH first.
Would it be sufficient to please NixOS if we simply append $(command
-v apache) or whatever after the well known candidate locations?
I know "command -v" is in POSIX, and on both bash and dash (the two
shells most distros use), it works as this patch expects, but its
portability is also a bit worrysome, especially because the whole
point of this patch is to support a platform that is, eh, on the
fringe.
So, I dunno.
^ permalink raw reply
* Re: [PATCH 2/9] for-each-ref: clarify interaction of --omit-empty & --count
From: Øystein Walle @ 2023-11-08 7:53 UTC (permalink / raw)
To: Victoria Dye; +Cc: gitgitgadget, git
In-Reply-To: <e166abeb-3566-4acf-a252-bc493ee37f41@github.com>
On Tue, 7 Nov 2023 at 20:30, Victoria Dye <vdye@github.com> wrote:
> Since the interaction isn't clearly defined at the moment, we could probably
> still update it to work like you're describing here. I'm happy to drop this
> patch and implement your recommendation in a follow-up series. Let me know
> what you think!
Regardless of whether the logic is changed in a follow-up series or not
I think the current behavior is worth documenting even if it doesn't
exist for much longer in the tree. So I am favor of having this patch as
part of this series.
I am also in favor of changing the behavior but that warrants a separate
series and discussion.
Øsse
^ permalink raw reply
* Re: first-class conflicts?
From: Elijah Newren @ 2023-11-08 7:31 UTC (permalink / raw)
To: Martin von Zweigbergk; +Cc: phillip.wood, Sandra Snan, git, Randall S. Becker
In-Reply-To: <CAESOdVDmQ85-des6Au-LH0fkUB9BZBZho0r-5=8MkPLJVA5WQQ@mail.gmail.com>
Hi Martin,
On Tue, Nov 7, 2023 at 9:38 AM Martin von Zweigbergk
<martinvonz@google.com> wrote:
>
[...]
> > One thing to think about if we ever want to implement this is what other
> > data we need to store along with the conflict trees to preserve the
> > context in which the conflict was created. For example the files that
> > are read by "git commit" when it commits a conflict resolution. For a
> > single cherry-pick/revert it would probably be fairly straight forward
> > to store CHERRY_PICK_HEAD/REVERT_HEAD and add it as a parent so it gets
> > transferred along with the conflicts. For a sequence of cherry-picks or
> > a rebase it is more complicated to preserve the context of the conflict.
> > Even "git merge" can create several files in addition to MERGE_HEAD
> > which are read when the conflict resolution is committed.
>
> Good point. We actually don't store any extra data in jj. The old
> per-path conflict model was prepared for having some label associated
> with each term of the conflict but we never actually used it.
>
> If we add such metadata, it would probably have to be something that
> makes sense even after pushing the conflict to another repo, so it
> probably shouldn't be commit ids, unless we made sure to also push
> those commits. Also note that if you `jj restore --from <commit with
> conflict>`, you can get a conflict into a commit that didn't have
> conflicts previously. Or if you already had conflicts in the
> destination commit, your root trees (the multiple root trees
> constituting the conflict) will now have conflicts that potentially
> were created by two completely unrelated operations, so you would kind
> of need different labels for different paths.
>
> https://github.com/martinvonz/jj/issues/1176 has some more discussion
> about this.
Interesting link; thanks for sharing.
I am curious more about the data you do store. My fuzzy memory is
that you store a commit header involving something of the form "A + B
- C", where those are all commit IDs. Is that correct? Is this in
addition to a normal "tree" header as in Git, or are one of A or B
found in the tree header? I think you said there was also the
possibility for more than three terms. Are those for when a
conflicted commit is merged with another branch that adds more
conflicts, or are there other cases too? (Octopus merges?)
What about recursive merges, i.e. merges where the two sides do not
have a unique merge base. What is the form of those? (Would "- C" be
replaced by "- C1 - C2 - ... - Cn"? Or would we create the virtual
merge base V and then do a " - V"? Or do we only have "A + B"?)
You previously mentioned that if someone goes to edit a commit with
conflicts, and resolves the conflicts in just one file, then you can
modify each of the trees A, B, and C such that a merging of those
trees gives the partially resolved result. How does one do that with
special conflicts, such as:
* User modifies file D on both sides of history, in conflicting
ways, and also renames D -> E on one side of history. User checks out
this conflicted commit and fixes the conflicts in E (but not other
files) and does a "git add E". When they go to commit, does the
machinery need a mapping to figure out that it needs to adjust "D" in
two of the trees while adjusting "E" in the other?
* Similar to the above, but the side that doesn't rename D renames
olddir/ -> newdir/, and the side that renames D instead renames
D->olddir/E. For this case, the file will end up at newdir/E; do we
need the backward mapping from newdir/E to both olddir/E and D?
* Slightly different than the above: User renames D -> E on one
side of history, and D -> F on the other. That's a rename/rename
(1to2) conflict. User checks out this conflicted commit and does a
"git add F", marking it as okay, but leaving E conflicted. How can
one adjust the tree such that no conflict for F appears, but one still
appears for E?
* Similar to above with an extra wrinkle: User renames D -> E on
one side of history, and on the other side both renames D -> F and
adds a slightly different file named E. That's both a rename/rename
(1to2) conflict for E & F, and an add/add conflict for E. Users
checks out this conflicted commit and resolves textual conflict in E
(in favor of the "other side"), and does a "git add E", marking it as
resolved. When they go to commit, we not only need to worry about
making sure a conflict for F appears, we also need to figure out how
to adjust the tree such that the merge result gives you the expected
value in E without affecting F. How can that be done?
On the first two bullet points, there's no such thing as a reverse
mapping from conflicted files to original files from previous commits
in current Git. Creating one, if possible, would be a fair amount of
work. But, I'm not so sure it's even possible, due to the fact that
conflicts and files do not always have one-to-one (or even one-to-many
or many-to-one) relationships; many-to-many relationship can exist, as
I've started alluding to in the last two bullet points (see also
https://github.com/git/git/blob/98009afd24e2304bf923a64750340423473809ff/Documentation/git-merge-tree.txt#L266-L271).
In fact, they can get even more complicated (e.g.
https://github.com/git/git/blob/master/t/t6422-merge-rename-corner-cases.sh#L1017-L1022).
> > > But we'd also have to be careful and think through usecases, including
> > > in the surrounding community. People would probably want to ensure
> > > that e.g. "Protected" or "Integration" branches don't get accept
> > > fetches or pushes of conflicted commits,
> >
> > I think this is a really important point, while it can be useful to
> > share conflicts so they can be collaboratively resolved we don't want to
> > propagate them into "stable" or production branches. I wonder how 'jj'
> > handles this.
>
> Agreed. `jj git push` refuses to push commits with conflicts, because
> it's very unlikely that the remote will be able to make any sense of
> it. Our commit backend at Google does support conflicts, so users can
> check out each other's conflicted commits there (except that we
> haven't even started dogfooding yet).
I'm curious to hear what happens when you do start dogfooding, on
projects with many developers and which are jj-only. Do commits with
conflicts accidentally end up in mainline branches, or are there good
ways to make sure they don't hit anything considered stable?
> > > git status would probably
> > > need some special warnings or notices, git checkout would probably
> > > benefit from additional warnings/notices checks for those cases, git
> > > log should probably display conflicted commits differently, we'd need
> > > to add special handling for higher order conflicts (e.g. a merge with
> > > conflicts is itself involved in a merge) probably similar to what jj
> > > has done, and audit a lot of other code paths to see what would be
> > > needed.
> >
> > As you point out there is a lot more to this than just being able to
> > store the conflict data in a commit - in many ways I think that is the
> > easiest part of the solution to sharing conflicts.
>
> Yes, I think it would be a very large project. Unlike jj, Git of
> course has to worry about backwards compatibility. For example, you
> would have to decide if your goal - even in the long term - is to make
> `git rebase` etc. not get interrupted due to conflicts.
...and whether to copy jj's other feature in this area in some form:
auto-rebasing any descendants when you checkout and amend an old
commit (e.g. to resolve conflicts). :-)
^ permalink raw reply
* [PATCH 3/3] t9164: fix inability to find basename(1) in hooks
From: Patrick Steinhardt @ 2023-11-08 7:30 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1699428122.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1789 bytes --]
The post-commit hook in t9164 is executed with a default environment.
To work around this issue we already write the current `PATH` value into
the hook script. But this is done at a point where we already tried to
execute non-built-in commands, namely basename(1). While this seems to
work alright on most platforms, it fails on NixOS.
Set the `PATH` variable earlier to fix this issue. Note that we do this
via two separate heredocs. This is done because in the first one we do
want to expand variables, whereas in the second one we don't.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t9164-git-svn-dcommit-concurrent.sh | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/t/t9164-git-svn-dcommit-concurrent.sh b/t/t9164-git-svn-dcommit-concurrent.sh
index c8e6c0733f4..3b64022dc57 100755
--- a/t/t9164-git-svn-dcommit-concurrent.sh
+++ b/t/t9164-git-svn-dcommit-concurrent.sh
@@ -47,9 +47,15 @@ setup_hook()
echo "cnt=$skip_revs" > "$hook_type-counter"
rm -f "$rawsvnrepo/hooks/"*-commit # drop previous hooks
hook="$rawsvnrepo/hooks/$hook_type"
- cat > "$hook" <<- 'EOF1'
+ cat >"$hook" <<-EOF
#!/bin/sh
set -e
+
+ PATH=\"$PATH\"
+ export PATH
+ EOF
+
+ cat >>"$hook" <<-'EOF'
cd "$1/.." # "$1" is repository location
exec >> svn-hook.log 2>&1
hook="$(basename "$0")"
@@ -59,11 +65,11 @@ setup_hook()
cnt="$(($cnt - 1))"
echo "cnt=$cnt" > ./$hook-counter
[ "$cnt" = "0" ] || exit 0
-EOF1
+ EOF
+
if [ "$hook_type" = "pre-commit" ]; then
echo "echo 'commit disallowed' >&2; exit 1" >>"$hook"
else
- echo "PATH=\"$PATH\"; export PATH" >>"$hook"
echo "svnconf=\"$svnconf\"" >>"$hook"
cat >>"$hook" <<- 'EOF2'
cd work-auto-commits.svn
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 2/3] t/lib-httpd: stop using legacy crypt(3) for authentication
From: Patrick Steinhardt @ 2023-11-08 7:30 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1699428122.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2129 bytes --]
When setting up httpd for our tests, we also install a passwd and
proxy-passwd file that contain the test user's credentials. These
credentials currently use crypt(3) as the password encryption schema.
This schema can be considered deprecated nowadays as it is not safe
anymore. Quoting Apache httpd's documentation [1]:
> Unix only. Uses the traditional Unix crypt(3) function with a
> randomly-generated 32-bit salt (only 12 bits used) and the first 8
> characters of the password. Insecure.
This is starting to cause issues in modern Linux distributions. glibc
has deprecated its libcrypt library that used to provide crypt(3) in
favor of the libxcrypt library. This newer replacement provides a
compile time switch to disable insecure password encryption schemata,
which causes crypt(3) to always return `EINVAL`. The end result is that
httpd tests that exercise authentication will fail on distros that use
libxcrypt without these insecure encryption schematas.
Regenerate the passwd files to instead use the default password
encryption schema, which is md5. While it feels kind of funny that an
MD5-based encryption schema should be more secure than anything else, it
is the current default and supported by all platforms. Furthermore, it
really doesn't matter all that much given that these files are only used
for testing purposes anyway.
[1]: https://httpd.apache.org/docs/2.4/misc/password_encryptions.html
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/lib-httpd/passwd | 2 +-
t/lib-httpd/proxy-passwd | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/t/lib-httpd/passwd b/t/lib-httpd/passwd
index 99a34d64874..d9c122f3482 100644
--- a/t/lib-httpd/passwd
+++ b/t/lib-httpd/passwd
@@ -1 +1 @@
-user@host:xb4E8pqD81KQs
+user@host:$apr1$LGPmCZWj$9vxEwj5Z5GzQLBMxp3mCx1
diff --git a/t/lib-httpd/proxy-passwd b/t/lib-httpd/proxy-passwd
index 77c25138e07..2ad7705d9a3 100644
--- a/t/lib-httpd/proxy-passwd
+++ b/t/lib-httpd/proxy-passwd
@@ -1 +1 @@
-proxuser:2x7tAukjAED5M
+proxuser:$apr1$RxS6MLkD$DYsqQdflheq4GPNxzJpx5.
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 1/3] t/lib-httpd: dynamically detect httpd and modules path
From: Patrick Steinhardt @ 2023-11-08 7:29 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1699428122.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2623 bytes --]
In order to set up the Apache httpd server, we need to locate both the
httpd binary and its default module path. This is done with a hardcoded
list of locations that we scan. While this works okayish with distros
that more-or-less follow the Filesystem Hierarchy Standard, it falls
apart on others like NixOS that don't.
While it is possible to specify these paths via `LIB_HTTPD_PATH` and
`LIB_HTTPD_MODULE_PATH`, it is not a nice experience for the developer
to figure out how to set those up. And in fact we can do better by
dynamically detecting both httpd and its module path at runtime:
- The httpd binary can be located via PATH.
- The module directory can (in many cases) be derived via the
`HTTPD_ROOT` compile-time variable.
Refactor the code to do so. If runtime detection of the paths fails we
continue to fall back to the hardcoded list of paths.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/lib-httpd.sh | 51 +++++++++++++++++++++++++++++++++-----------------
1 file changed, 34 insertions(+), 17 deletions(-)
diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index 2fb1b2ae561..06038f72607 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -55,25 +55,42 @@ fi
HTTPD_PARA=""
-for DEFAULT_HTTPD_PATH in '/usr/sbin/httpd' '/usr/sbin/apache2'
-do
- if test -x "$DEFAULT_HTTPD_PATH"
- then
- break
- fi
-done
-
-for DEFAULT_HTTPD_MODULE_PATH in '/usr/libexec/apache2' \
- '/usr/lib/apache2/modules' \
- '/usr/lib64/httpd/modules' \
- '/usr/lib/httpd/modules' \
- '/usr/libexec/httpd'
-do
- if test -d "$DEFAULT_HTTPD_MODULE_PATH"
+DEFAULT_HTTPD_PATH="$(command -v httpd)"
+if test -z "$DEFAULT_HTTPD_PATH" -o ! -x "$DEFAULT_HTTPD_PATH"
+then
+ for DEFAULT_HTTPD_PATH in '/usr/sbin/httpd' '/usr/sbin/apache2'
+ do
+ if test -x "$DEFAULT_HTTPD_PATH"
+ then
+ break
+ fi
+ done
+fi
+
+if test -x "$DEFAULT_HTTPD_PATH"
+then
+ HTTPD_ROOT="$("$DEFAULT_HTTPD_PATH" -V | sed -n 's/^ -D HTTPD_ROOT="\(.*\)"$/\1/p')"
+ if test -n "$HTTPD_ROOT" -a -d "$HTTPD_ROOT/modules"
then
- break
+ DEFAULT_HTTPD_MODULE_PATH="$HTTPD_ROOT/modules"
fi
-done
+ unset HTTPD_ROOT
+fi
+
+if test -z "$DEFAULT_HTTPD_MODULE_PATH" -o ! -d "$DEFAULT_HTTPD_MODULE_PATH"
+then
+ for DEFAULT_HTTPD_MODULE_PATH in '/usr/libexec/apache2' \
+ '/usr/lib/apache2/modules' \
+ '/usr/lib64/httpd/modules' \
+ '/usr/lib/httpd/modules' \
+ '/usr/libexec/httpd'
+ do
+ if test -d "$DEFAULT_HTTPD_MODULE_PATH"
+ then
+ break
+ fi
+ done
+fi
case $(uname) in
Darwin)
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 0/3] t: improve compatibility with NixOS
From: Patrick Steinhardt @ 2023-11-08 7:29 UTC (permalink / raw)
To: git
[-- Attachment #1: Type: text/plain, Size: 1440 bytes --]
Hi,
this patch series improves compatibility of our test suite with NixOS.
NixOS is somewhat special compared to more conventional Linux distros
because it doesn't follow the Filesystem Hierarchy Specification for
most of the part. Instead, packages are installed into the Nix store
in `/nix` with hashed paths that change frequently when upgrading the
system to a newer generation. Consequentially, paths cannot be hardcoded
and must instead be computed at runtime.
We have two such issues in our test harness right now:
- t/lib-httpd searches Apache httpd and its modules directory in a
list of hardcoded paths.
- t9164 doesn't propagate PATH to a script and thus cannot find the
basename(1) utility.
Both of these issues are fixed in this patch series. In addition, this
patch series fixes an upcoming issue in httpd's passwd files caused by
the deprecation of the crypt(3) function.
Patrick
Patrick Steinhardt (3):
t/lib-httpd: dynamically detect httpd and modules path
t/lib-httpd: stop using legacy crypt(3) for authentication
t9164: fix inability to find basename(1) in hooks
t/lib-httpd.sh | 51 ++++++++++++++++++---------
t/lib-httpd/passwd | 2 +-
t/lib-httpd/proxy-passwd | 2 +-
t/t9164-git-svn-dcommit-concurrent.sh | 12 +++++--
4 files changed, 45 insertions(+), 22 deletions(-)
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 8/9] for-each-ref: add option to fully dereference tags
From: Patrick Steinhardt @ 2023-11-08 7:19 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Victoria Dye, Victoria Dye via GitGitGadget, git
In-Reply-To: <xmqq4jhx7x8l.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 2521 bytes --]
On Wed, Nov 08, 2023 at 12:14:02PM +0900, Junio C Hamano wrote:
> Victoria Dye <vdye@github.com> writes:
>
> > I think `^{}fieldname` would be a good candidate, but it's *extremely*
>
> Gaah. Why? fieldname^{} I may understand, but in the prefix form?
>
> In any case, has anybody considered that we may be better off to
> declare that "*field" peeling a tag only once is a longstanding bug?
>
> IOW, can we not add "fully peel" command line option or a new syntax
> and instead just "fix" the bug to fully peel when "*field" is asked
> for?
I see where you're coming from, but I wonder whether this wouldn't break
scripts. To me, the documentation seems to explicitly state that this
will only deref tags once:
If fieldname is prefixed with an asterisk (*) and the ref points at
a tag object, use the value for the field in the object which the
tag object refers to (instead of the field in the tag object).
So changing that now would break both the documented and the actual
behaviour. Now whether anybody actually cares about such a breaking
change is of course a different question, and you're probably correct
that in practice nobody does.
Patrick
> An application that cares about handling a chain of annotatetd tags
> would want to be able to say "this is the outermost tag's
> information; one level down, the tag was signed by this person;
> another level down, the tag was signed by this person, etc." which
> would mean either
>
> * we have a syntax that shows the information from all levels
> (e.g., "**taggername" may say "Victoria\nPatrick\nGitster")
>
> * we have a syntax that allows to specify how many levels to peel,
> (e.g., "*0*taggername" may be the same as "taggername",
> "*1*taggername" may be the same as "*taggername") plus some
> programming construct like variables and loops.
>
> but the repertoire being proposed that consists only of "peel only
> once" and "peel all levels" is way too insufficient.
>
> Note that I do not advocate for allowing inspection of each levels
> separately. Quite the contrary. I would say that --format=<>
> placeholder should not be a programming language to satisify such a
> niche need. And my conclusion from that stance is "peel once" plus
> "peel all" are already one level too many, and "peel once" was a
> very flawed implementation from day one, when 9f613ddd (Add
> git-for-each-ref: helper for language bindings, 2006-09-15)
> introduced it.
>
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [RFC PATCH 2/3] tmp-objdir: introduce `tmp_objdir_repack()`
From: Patrick Steinhardt @ 2023-11-08 7:05 UTC (permalink / raw)
To: Taylor Blau
Cc: git, Jeff King, Elijah Newren, Junio C Hamano,
Johannes Schindelin
In-Reply-To: <0f19c139ba9bb5105747f545038825d0c89f2e42.1699381371.git.me@ttaylorr.com>
[-- Attachment #1: Type: text/plain, Size: 3155 bytes --]
On Tue, Nov 07, 2023 at 01:22:58PM -0500, Taylor Blau wrote:
> In the following commit, we will teach `git replay` how to write a pack
> containing the set of new objects created as a result of the `replay`
> operation.
>
> Since `replay` needs to be able to see the object(s) written
> from previous steps in order to replay each commit, the ODB transaction
> may have multiple pending packs. Migrating multiple packs back into the
> main object store has a couple of downsides:
>
> - It is error-prone to do so: each pack must be migrated in the
> correct order (with the ".idx" file staged last), and the set of
> packs themselves must be moved over in the correct order to avoid
> racy behavior.
>
> - It is a (potentially significant) performance degradation to migrate
> a large number of packs back into the main object store.
>
> Introduce a new function that combines the set of all packs in the
> temporary object store to produce a single pack which is the logical
> concatenation of all packs created during that level of the ODB
> transaction.
>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
> tmp-objdir.c | 13 +++++++++++++
> tmp-objdir.h | 6 ++++++
> 2 files changed, 19 insertions(+)
>
> diff --git a/tmp-objdir.c b/tmp-objdir.c
> index 5f9074ad1c..ef53180b47 100644
> --- a/tmp-objdir.c
> +++ b/tmp-objdir.c
> @@ -12,6 +12,7 @@
> #include "strvec.h"
> #include "quote.h"
> #include "object-store-ll.h"
> +#include "run-command.h"
>
> struct tmp_objdir {
> struct strbuf path;
> @@ -277,6 +278,18 @@ int tmp_objdir_migrate(struct tmp_objdir *t)
> return ret;
> }
>
> +int tmp_objdir_repack(struct tmp_objdir *t)
> +{
> + struct child_process cmd = CHILD_PROCESS_INIT;
> +
> + cmd.git_cmd = 1;
> +
> + strvec_pushl(&cmd.args, "repack", "-a", "-d", "-k", "-l", NULL);
> + strvec_pushv(&cmd.env, tmp_objdir_env(t));
I wonder what performance of this repack would be like in a large
repository with many refs. Ideally, I would expect that the repacking
performance should scale with the number of objects we have written into
the temporary object directory. But in practice, the repack will need to
compute reachability and thus also scales with the size of the repo
itself, doesn't it?
Patrick
> + return run_command(&cmd);
> +}
> +
> const char **tmp_objdir_env(const struct tmp_objdir *t)
> {
> if (!t)
> diff --git a/tmp-objdir.h b/tmp-objdir.h
> index 237d96b660..d00e3b3e27 100644
> --- a/tmp-objdir.h
> +++ b/tmp-objdir.h
> @@ -36,6 +36,12 @@ struct tmp_objdir *tmp_objdir_create(const char *prefix);
> */
> const char **tmp_objdir_env(const struct tmp_objdir *);
>
> +/*
> + * Combines all packs in the tmp_objdir into a single pack before migrating.
> + * Removes original pack(s) after installing the combined pack into place.
> + */
> +int tmp_objdir_repack(struct tmp_objdir *);
> +
> /*
> * Finalize a temporary object directory by migrating its objects into the main
> * object database, removing the temporary directory, and freeing any
> --
> 2.42.0.446.g0b9ef90488
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: first-class conflicts?
From: Elijah Newren @ 2023-11-08 6:31 UTC (permalink / raw)
To: phillip.wood; +Cc: Sandra Snan, git, Martin von Zweigbergk, Randall S. Becker
In-Reply-To: <ba047d38-4ad1-4440-8342-3379404f430b@gmail.com>
Hi Phillip,
On Tue, Nov 7, 2023 at 3:49 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> Hi Elijah
>
> [I've cc'd Martin to see if he has anything to add about how "jj"
> manages the issues around storing conflicts.]
+1. I'll add some other questions for him too while we're at it,
separately in this thread.
[...]
> > Martin also gave us an update at the 2023 Git Contributors summit, and
> > in particular noted a significant implementation change to not have
> > per-file storage of conflicts, but rather storing at the commit level
> > the multiple conflicting trees involved. That model might be
> > something we could implement in Git. And if we did, it'd solve
> > various issues such as people wanting to be able to stash conflicts,
> > or wanting to be able to partially resolve conflicts and fix it up
> > later, or be able to collaboratively resolve conflicts without having
> > everyone have access to the same checkout.
>
> One thing to think about if we ever want to implement this is what other
> data we need to store along with the conflict trees to preserve the
> context in which the conflict was created. For example the files that
> are read by "git commit" when it commits a conflict resolution. For a
> single cherry-pick/revert it would probably be fairly straight forward
> to store CHERRY_PICK_HEAD/REVERT_HEAD and add it as a parent so it gets
> transferred along with the conflicts.
This is a great thing to think about and bring up. However, I'm not
sure what part of it actually needs to be preserved; in fact, it's not
clear to me that any of it needs preserving -- especially not the
files read by "git commit". A commit was already created, after all.
It seems that CHERRY_PICK_HEAD/REVERT_HEAD files exist primarily to
clue in that we are in-the-middle-of-<op>, and the conflict header
(the "tree A + tree B - tree C" thing; whatever that's called)
similarly provides signal that the commit still has conflicts.
Secondarily, these files contain information about the tree we came
from and its parent tree, which allows users to investigate the diff
between those...but that information is also available from the
conflict header in the recorded commit. The CHERRY_PICK_HEAD and
REVERT_HEAD files could also be used to access the commit message, but
that would have been stored in the conflicted commit as well. Are
there any other pieces of information I'm missing?
> For a sequence of cherry-picks or
> a rebase it is more complicated to preserve the context of the conflict.
I think the big piece here is whether we also want to adopt jj's
behavior of automatically rebasing all descendant commits when
checking out and amending some historical commit (or at least having
the option of doing so). That behavior allows users to amend commits
to resolve conflicts without figuring out complicated interactive
rebases to fix all the descendant commits across all relevant
branches. Without that feature, I agree this might be a bit more
difficult, but with that feature, I'm having a hard time figuring out
what context we actually need to preserve for a sequence of
cherry-picks or a rebase.
Digging into a few briefly...
Many of the state files are about the status of the in-progress
operation (todo-list, numbers of commits done and to do, what should
be done with not-yet-handled commits, temporary refs corresponding to
temporary labels that need to be deleted, rescheduling failed execs,
dropping or keeping redundant commits, etc.), but if the operation has
completed and new commits created (potentially with multiple files
with conflict headers), I don't see how this information is useful
anymore.
There are some special state files related to half-completed
operations (e.g. squash commits when we haven't yet reached the final
one in the sequence, a file to note that we want to edit a commit
message once the user has finished resolving conflicts, whether we
need to create a new root commit), but again, the operation has
completed and commits have been created with appropriate parentage and
commit messages so I don't think these are useful anymore either.
Other state files are related to things needing to be done at the end
of the operation, like invoke the post-rewrite hook or pop the
autostash (with knowledge of what was rewritten to what). But the
operation would have been completed and those things done already, so
I don't see how this is necessary either.
Some state files are for controlling how commits are created (setting
committer date to author date, gpg signing options, whether to add
signoff), but, again, commits have already been created, and can be
further amended as the user wants (hopefully including resolving the
conflicts).
The biggest issue is perhaps that REBASE_HEAD is used in the
implementation of `git rebase --show-current-patch`, but all
information stored in that is still accessible -- the commit message
is stored in the commit, the author time is stored in the commit, and
the trees involved are in the conflict header. The only thing missing
is committer timestamp, which isn't relevant anyway.
The only ones I'm pausing a bit on are the strategy and
strategy-options. Those might be useful somehow...but I can't
currently quite put my finger on explaining how they would be useful
and I'm not sure they are.
Am I missing anything?
> Even "git merge" can create several files in addition to MERGE_HEAD
> which are read when the conflict resolution is committed.
That's a good one to bring up too, but I'm not sure I understand how
these could be useful to preserve either. Am I missing something? My
breakdown:
* MERGE_HEAD: was recorded in the commit as a second parent, so we
already have that info
* MERGE_MSG: was recorded in the commit as the commit message, so
again we already have that info
* MERGE_AUTOSTASH: irrelevant since the stashed stuff isn't part of
the commit and was in fact unstashed after the
merge-commit-with-conflicts was created
* MERGE_MODE: irrelevant since it's only used for reducing heads at
time of git-commit, and git-commit has already been run
* MERGE_RR: I think this is irrelevant; the conflict record (tree A
+ tree B - tree C) lets us redo the merge if needed to get the list of
conflicted files and textual conflicts found therein
So I don't see how any of the information in these files need to be
recorded as additional auxiliary information. However, that last item
might depend upon the strategy and strategy-options, which currently
is not recorded...hmm....
> > But we'd also have to be careful and think through usecases, including
> > in the surrounding community. People would probably want to ensure
> > that e.g. "Protected" or "Integration" branches don't get accept
> > fetches or pushes of conflicted commits,
>
> I think this is a really important point, while it can be useful to
> share conflicts so they can be collaboratively resolved we don't want to
> propagate them into "stable" or production branches. I wonder how 'jj'
> handles this.
Yeah, figuring this out might be the biggest sticking point.
> > git status would probably
> > need some special warnings or notices, git checkout would probably
> > benefit from additional warnings/notices checks for those cases, git
> > log should probably display conflicted commits differently, we'd need
> > to add special handling for higher order conflicts (e.g. a merge with
> > conflicts is itself involved in a merge) probably similar to what jj
> > has done, and audit a lot of other code paths to see what would be
> > needed.
>
> As you point out there is a lot more to this than just being able to
> store the conflict data in a commit - in many ways I think that is the
> easiest part of the solution to sharing conflicts.
Yeah, another one I just thought of is that the trees referenced in
the conflicts would also need to affect reachability computations as
well, to make sure they both don't get gc'ed and that they are
transferred when appropriate. There are lots of things that would be
involved in implementing this idea.
^ permalink raw reply
* Re: [PATCH] diff: implement config.diff.renames=copies-harder
From: Elijah Newren @ 2023-11-08 4:38 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Sam James via GitGitGadget, git, Sam James
In-Reply-To: <xmqqv8ac7utk.fsf@gitster.g>
On Tue, Nov 7, 2023 at 8:06 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Elijah Newren <newren@gmail.com> writes:
>
> >> > We probably don't want to copy all three of those sentences here, but
> >> > I think we need to make sure users can find them, thus my suggestion
> >> > to reference the `--find-copies-harder` option to git-diff so that
> >> > affected users can get the info they need to choose.
> >>
> >> "in addition to paths that are different, will look for more copies
> >> even in unmodified paths" then?
> >
> > That's much better. I still slightly prefer referencing
> > `--find-copies-harder` so that there's a link between "copies-harder"
> > and `--find-copies-harder`; but this version would also be fine.
>
> Oh, I didn't mean "use this rewrite and do not make any external
> reference". More like "external reference is a good idea and
> necessary to help motivated readers, but we should give enough
> information inline, and I think this level of details would be
> sufficient".
Ah, gotcha. Yeah, this sounds good.
^ permalink raw reply
* Re: [PATCH] diff: implement config.diff.renames=copies-harder
From: Junio C Hamano @ 2023-11-08 4:06 UTC (permalink / raw)
To: Elijah Newren; +Cc: Sam James via GitGitGadget, git, Sam James
In-Reply-To: <CABPp-BEgxKn3QvJQ+6L3Z1RN1im=c3dfApLRCrQqum_Yim44Gw@mail.gmail.com>
Elijah Newren <newren@gmail.com> writes:
>> > We probably don't want to copy all three of those sentences here, but
>> > I think we need to make sure users can find them, thus my suggestion
>> > to reference the `--find-copies-harder` option to git-diff so that
>> > affected users can get the info they need to choose.
>>
>> "in addition to paths that are different, will look for more copies
>> even in unmodified paths" then?
>
> That's much better. I still slightly prefer referencing
> `--find-copies-harder` so that there's a link between "copies-harder"
> and `--find-copies-harder`; but this version would also be fine.
Oh, I didn't mean "use this rewrite and do not make any external
reference". More like "external reference is a good idea and
necessary to help motivated readers, but we should give enough
information inline, and I think this level of details would be
sufficient".
Thanks.
^ permalink raw reply
* Re: [PATCH] diff: implement config.diff.renames=copies-harder
From: Elijah Newren @ 2023-11-08 3:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Sam James via GitGitGadget, git, Sam James
In-Reply-To: <xmqqttpx828i.fsf@gitster.g>
On Tue, Nov 7, 2023 at 5:26 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Elijah Newren <newren@gmail.com> writes:
>
> > I find that marginally better; but I still don't think it answers the
> > user's question of why they should pick one option or the other. The
> > wording for the `--find-copies-harder` does explain when it's useful:
> >
> > For performance reasons, by default, `-C` option finds copies only
> > if the original file of the copy was modified in the same
> > changeset. This flag makes the command
> > inspect unmodified files as candidates for the source of
> > copy. This is a very expensive operation for large
> > projects, so use it with caution.
> >
> > We probably don't want to copy all three of those sentences here, but
> > I think we need to make sure users can find them, thus my suggestion
> > to reference the `--find-copies-harder` option to git-diff so that
> > affected users can get the info they need to choose.
>
> "in addition to paths that are different, will look for more copies
> even in unmodified paths" then?
That's much better. I still slightly prefer referencing
`--find-copies-harder` so that there's a link between "copies-harder"
and `--find-copies-harder`; but this version would also be fine.
^ permalink raw reply
* Re: [PATCH 8/9] for-each-ref: add option to fully dereference tags
From: Junio C Hamano @ 2023-11-08 3:14 UTC (permalink / raw)
To: Victoria Dye; +Cc: Patrick Steinhardt, Victoria Dye via GitGitGadget, git
In-Reply-To: <cf691b7c-288f-4cc9-a2ac-1a43972ae446@github.com>
Victoria Dye <vdye@github.com> writes:
> I think `^{}fieldname` would be a good candidate, but it's *extremely*
Gaah. Why? fieldname^{} I may understand, but in the prefix form?
In any case, has anybody considered that we may be better off to
declare that "*field" peeling a tag only once is a longstanding bug?
IOW, can we not add "fully peel" command line option or a new syntax
and instead just "fix" the bug to fully peel when "*field" is asked
for?
An application that cares about handling a chain of annotatetd tags
would want to be able to say "this is the outermost tag's
information; one level down, the tag was signed by this person;
another level down, the tag was signed by this person, etc." which
would mean either
* we have a syntax that shows the information from all levels
(e.g., "**taggername" may say "Victoria\nPatrick\nGitster")
* we have a syntax that allows to specify how many levels to peel,
(e.g., "*0*taggername" may be the same as "taggername",
"*1*taggername" may be the same as "*taggername") plus some
programming construct like variables and loops.
but the repertoire being proposed that consists only of "peel only
once" and "peel all levels" is way too insufficient.
Note that I do not advocate for allowing inspection of each levels
separately. Quite the contrary. I would say that --format=<>
placeholder should not be a programming language to satisify such a
niche need. And my conclusion from that stance is "peel once" plus
"peel all" are already one level too many, and "peel once" was a
very flawed implementation from day one, when 9f613ddd (Add
git-for-each-ref: helper for language bindings, 2006-09-15)
introduced it.
^ permalink raw reply
* Job vacancy
From: St. Thomas' Hospital UK @ 2023-11-08 2:19 UTC (permalink / raw)
To: git
St. Thomas' Hospital UK
REF: HR/MED-004/06923
St. Thomas' Hospital UK is a large NHS teaching hospital in Central
London, England. It is one of the institutions that compose the King's
Health Partners, an academic health science Center. Administratively
part of the Guy's and St Thomas' NHS Foundation Trust, together with
Guy's Hospital and King's College Hospital, it provides the location
of the King's College London GKT School of Medical Education.
It is ranked amongst the best Ten (10) hospitals in the United Kingdom
with 840 beds. The hospital has provided healthcare freely or under
charitable auspices since the 12th century. It is one of London's most
famous hospitals, associated with names such as Sir Astley Cooper,
William Cheselden, Florence Nightingale, Linda Richards, Edmund
Montgomery, Agnes Elizabeth Jones and Sir Harold Ridley. It is a
prominent London landmark =E2=80=93 largely due to its location on the
opposite bank of the River Thames to the Houses of Parliament.
The largest not-for-profit health system in the world, we provide high
quality, personalized and compassionate care to our patients through
our dedication to safety, rigorous self-assessment, performance
improvement, corporate integrity and health service management. We are
committed to being the per-eminent provider of acute inpatient and
outpatient health care services.
DESCRIPTION: Following the COVID-19 outbreak, expansion and
development in our hospital, we are currently recruiting and employing
the services of Medical Professionals (Specialists, Consultants,
General Practitioners) with relevant experiences to fill in the
following below vacancies in our health care facility in the United
Kingdom.
AREAS OF VACANCIES:
StH1. ALLERGY & IMMUNOLOGY StH2. ANAESTHESIOLOGY StH3. ANGIOLOGY StH4.
ANTHROPOSOPHIC MEDICINE StH5. BREAST SURGERY StH6. CARDIOLOGY StH7.
CRANIOSACRAL PRACTITIONER / THERAPIST StH8. CARDIOTHORACIC SURGERY
StH9. CARDIAC SURGERY
StH10. CRITICAL CARE MEDICINE StH11. DENTISTS StH12. DENTAL SURGEON
StH13. DERMATOLOGY StH14. ENDOCRINOLOGY
StH15. EMERGENCY MEDICINE StH16. GASTROENTEROLOGY StH17. GENERAL
SURGERY StH18. GENERAL PAEDIATRICS StH19. GENERAL MEDICINE StH20.
HEMATOLOGY StH21. HYPERTENSION SPECIALIST StH22. INTERNAL MEDICINE
StH23. INFECTOLOGY StH24. MORPHOLOGY StH25. NEPHROLOGY StH26.
NEUROSURGERY StH27. NEONATOLOGY StH28. ORTHOPAEDICS StH29. ORTHOPAEDIC
SURGERY StH30. OTORHINOLARYNGOLOGY StH31. ORTHODONTIST StH32.
OCCUPATIONAL MEDICINE StH33. ORAL AND MAXILLOFACIAL SURGERY StH34.
PATHOLOGY
StH35. PLASTIC & RECONSTRUCTIVE SURGERY StH36. PNEUMOLOGY StH37.
PAEDIATRIC SURGEON StH38. PSYCHOLOGIST StH39. PHYSIOTHERAPY StH40.
PEDIATRICS StH41. PUBLIC HEALTH StH42. RADIOLOGY StH43. RHEUMATOLOGY
StH44. REHABILITATION MEDICINE StH45. RESPIRATORY MEDICINE StH46.
THORACIC SURGERY StH47. TRAUMATOLOGY StH48. TRICHOLOGIST StH49.
UROLOGY
JOB LOCATION: London, United Kingdom
JOB COMMENCEMENT: 2023
EMPLOYMENT TYPE: Contract / Full-time
EMPLOYMENT BENEFITS:
Excellent Salary and Overtime Bonus, Health/life Insurance, Relocation
expenses, Research and Educational assistance, Medical, Optical and
Dental Care, Family/Single housing accommodation, 24/7 Official
Vehicle, Scholarship for employee's dependent within UK schools.
Interested applicants are to send a detailed attachment Resume via email: recruitment@gsttsnhsuk.online
NOTE: APPLICATION IS OPEN TO INTERESTED PERSONS FROM ALL INTERNATIONAL
LOCATIONS, ALL SUCCESSFUL APPLICANTS IN OUR RECRUITMENT PROCESS MUST
BE WILLING TO RELOCATE TO THE UK FOR WORK.
Coronavirus (COVID-19)- Stay at home if you feel unwell. If you have a
fever, cough and difficulty breathing, seek medical attention and
call-in advance. Follow the directions of your local health authority.
Source: World Health Organization
Sincerely,
Julie Screaton,
St. Thomas' Hospital Uk
Guy's & St. Thomas NHS Foundation Trust
London, United Kingdom @2023
St Thomas' Hospital UK incorporated in England, UK (Reg. No: 06160266)
having its registered address at Westminster Bridge Rd, London SE1
7EH, England.
^ permalink raw reply
* Re: Error when "git mv" file in a sparsed checkout
From: Elijah Newren @ 2023-11-08 2:21 UTC (permalink / raw)
To: Josef Wolf, git
In-Reply-To: <20231107130303.GS7041@raven.inka.de>
Hi,
On Tue, Nov 7, 2023 at 5:32 AM Josef Wolf <jw@raven.inka.de> wrote:
>
> Hello,
>
> I have used the procedure described below for many years. In fact,
> this procedure is part of a script which I am using for about 10 years.
> This procedure was definitely working with git-2-25-1 and git-2.26.2.
>
> Now, with git-2.34.1 (on a freshly installed ubuntu-22.04), this
> procedure fails.
>
> Here is what I do:
>
> I want to rename a file on a branch which is not currently checked out
> without messing/touching my current working directory.
>
> For this, I first create a clone of the repo with shared git-directory:
>
> $ SANDBOX=/var/tmp/manage-scans-X1pKZQiey
> $ WT=$SANDBOX/wt
> $ GIT=$SANDBOX/git
>
> $ mkdir -p $SANDBOX
> $ git --work-tree $WT --git-dir $GIT clone -qns -n ~/upstream-repo $GIT
>
> Then, I do a sparse checkout in this clone, containing only the file
> that is to be renamed:
>
> $ cd $WT
> $ echo 'path/to/old-filename' >>$GIT/info/sparse-checkout
> $ git --work-tree $WT --git-dir $GIT config core.sparsecheckout true
> $ git --work-tree $WT --git-dir $GIT checkout -b the-branch remotes/origin/the-branch
> Switched to a new branch 'the-branch'
>
> Next step would be to "git mv" the file:
>
> $ mkdir -p /path/to # already exists, but should do no harm
> $ git --work-tree $WT --git-dir $GIT mv path/to/old-filename path/to/new-filename
sparse checkouts are designed such that only files matching the
patterns in the sparse-checkout file should be present in the working
tree, so renaming to a path that should not be present is problematic.
We could possibly have "git-mv" immediately remove the path from the
working tree (while leaving the new pathname in the index), but that's
problematic in that users often overlook the index and only look at
the working tree and might think the file was deleted instead of
renamed. Not immediately removing it is potentially even worse,
because any subsequent operation (particularly ones like checkout,
reset, merge, rebase, etc.) are likely to nuke the file from the
working tree and the fact that the removal is delayed makes it much
harder for users to understand and diagnose.
So, Stolee fixed this to make it throw an error; see
https://lore.kernel.org/git/pull.1018.v4.git.1632497954.gitgitgadget@gmail.com/
for details. His description did focus on cone mode, but you'll note
that none of my explanation here did. The logic for making this an
error fully applies to non-cone mode for all the same reasons.
If you want to interact with `path/to/new-filename` as a path within
your sparse checkout (as suggested by your git-mv command), then that
path should actually be part of your sparse checkout. In other words,
you should add `path/to/new-filename` to $GIT/info/sparse-checkout and
do so _before_ attempting your `git mv` command. If you don't like
that for some reason, you are allowed to instead ignore the
problematic consequences of renaming outside the sparse-checkout by
providing the `--sparse` flag. Both of these possibilities are
documented in the hints provided along with the error message you
showed below:
> The following paths and/or pathspecs matched paths that exist
> outside of your sparse-checkout definition, so will not be
> updated in the index:
> path/to/new-filename
> hint: If you intend to update such entries, try one of the following:
> hint: * Use the --sparse option.
> hint: * Disable or modify the sparsity rules.
> hint: Disable this message with "git config advice.updateSparsePath false"
>
> This error is something I have not expected.
>
> Error message suggests, there already exists a file named "new-filename". This
> is not true at all. There is no file named "new-filename" in the entire
> repository. Not in any directory of any branch.
You are correct; the wording of the error message here is suboptimal
and seems to have been focused more on the git-add case (the error
message is shared by git-add, git-mv, and git-rm). Thanks for
pointing it out! We could improve that wording, perhaps with
something like:
The following paths and/or pathspecs match paths that are
outside of your sparse-checkout definition, so will not be
updated:
Which is still slightly slanted towards git-add and git-rm cases, but
I hope it works better than the current message. Thoughts?
^ permalink raw reply
* Re: issue unable to commit file and folder name to upper lower case
From: Junio C Hamano @ 2023-11-08 1:48 UTC (permalink / raw)
To: Torsten Bögershausen; +Cc: chengpu lee, git
In-Reply-To: <20231107173557.GA29083@tb-raspi4>
Torsten Bögershausen <tboegi@web.de> writes:
> Yes, that is a restriction in Git, call it a bug, call it a missing feature.
> Unless someone fixes it, the recommendation is still to use a workaround:
>
> tb@pc:/tmp/ttt> git mv Abc tmp
> tb@pc:/tmp/ttt> git mv tmp abc
> tb@pc:/tmp/ttt> git status
> On branch master
> Changes to be committed:
> (use "git restore --staged <file>..." to unstage)
> renamed: Abc/.keep -> abc/.keep
>
> tb@pc:/tmp/ttt>
Correct and very helpful suggestion. Or get a better filesystem ;-)
Thanks.
^ 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