* Re: [PATCH v4 12/15] replay: disallow revision specific options and pathspecs
From: Johannes Schindelin @ 2023-09-07 10:24 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,
Christian Couder
In-Reply-To: <20230907092521.733746-13-christian.couder@gmail.com>
Hi Christian & Elijah,
On Thu, 7 Sep 2023, Christian Couder wrote:
> A previous commit changed `git replay` to make it accept standard
> revision ranges using the setup_revisions() function. While this is a
> good thing to make this command more standard and more flexible, it has
> the downside of enabling many revision related options accepted and eaten
> by setup_revisions().
>
> Some of these options might make sense, but others, like those
> generating non-contiguous history, might not. Anyway those we might want
> to allow should probably be tested and perhaps documented a bit, which
> could be done in future work.
>
> For now it is just simpler and safer to just disallow all of them, so
> let's do that.
>
> Other commands, like `git fast-export`, currently allow all these
> revision specific options even though some of them might not make sense,
> as these commands also use setup_revisions() but do not check the
> options that might be passed to this function.
>
> So a way to fix those commands as well as git replay could be to improve
> or refactor the setup_revisions() mechanism to let callers allow and
> disallow options in a relevant way for them. Such improvements are
> outside the scope of this work though.
>
> Pathspecs, which are also accepted and eaten by setup_revisions(), are
> likely to result in disconnected history. That could perhaps be useful,
> but that would need tests and documentation, which can be added in
> future work. So, while at it, let's disallow them too.
As pointed out elsewhere in this mail thread, I consider this patch to do
more harm than good. After switching the command to plumbingmanipulators
it should be possible to just forego all command-line validation and leave
that job to the caller.
Therefore I would love to see this patch dropped.
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH 11/14] replay: use standard revision ranges
From: Johannes Schindelin @ 2023-09-07 10:22 UTC (permalink / raw)
To: Christian Couder
Cc: Elijah Newren, Derrick Stolee, git, Junio C Hamano,
Patrick Steinhardt, John Cai, Christian Couder
In-Reply-To: <CAP8UFD2KMikjJJQ0FGXO1EYYHRNWHH2n70UATU_o61PJ411f2Q@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 4502 bytes --]
Hi Christian,
On Thu, 7 Sep 2023, Christian Couder wrote:
> On Sun, Sep 3, 2023 at 5:47 PM Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> > On Sat, 29 Apr 2023, Elijah Newren wrote:
> >
> > > On Mon, Apr 24, 2023 at 8:23 AM Derrick Stolee <derrickstolee@github.com> wrote:
>
> > > > Basically, I'm not super thrilled about exposing options that are
> > > > unlikely to be valuable to users and instead are more likely to cause
> > > > confusion due to changes that won't successfully apply.
> > >
> > > Oh, I got thrown by the "right now" portion of your comment; I
> > > couldn't see how time or future changes would affect anything to make
> > > it less (or more) confusing for users.
> > >
> > > Quick clarification, though: while you correctly point out the type of
> > > confusion the user would experience without my overriding, my
> > > overriding of rev.reverse (after setup_revisions() returns, not before
> > > it is called) precludes that experience. The override means none of
> > > the above happens, and they would instead just wonder why their option
> > > is being ignored.
> >
> > FWIW here is my view on the matter: `git replay`, at least in its current
> > incarnation, is a really low-level tool. As such, I actually do not want
> > to worry much about protecting users from nonsensical invocations.
> >
> > In that light, I would like to see that code rejecting all revision
> > options except `--diff-algorithm` be dropped. Should we ever decide to add
> > a non-low-level mode to `git replay`, we can easily add some user-friendly
> > sanity check of the options then, and only for that non-low-level code.
> > For now, I feel that it's just complicating things, and `git replay` is in
> > the experimental phase anyway.
>
> I would be Ok with removing the patch (called "replay: disallow
> revision specific options and pathspecs")
> that rejects all revision options and pathspecs if there is a
> consensus for that.
Well, since you talk about "consensus" and I already made my strong
preference known, how about you add yours?
> It might not simplify things too much if there is still an exception for
> `--diff-algorithm` though. Also it's not clear if you are Ok with
> allowing pathspecs or not.
I want to remove all the rev-list-option-disallowing code. Including the
pathspec one.
> The idea with disallowing all of them was to later add back those that
> make sense along with tests and maybe docs to explain them in the
> context of this command. It was not to disallow them permanently. So I
> would think the best path forward would be a patch series on top of this
> one that would revert the patch disallowing these options and maybe
> pathspecs, and instead allow most of them and document and test things a
> bit.
I understand that that was the reasoning.
What I would like to suggest is that we should treat `git replay` as a
low-level (or: "plumbing" in Git Speak) command.
That would allow us to leave the option for stricter command-line
parameter validation to an _additional_ option, to be added later (or
never), and to stop worrying about this tool being used in ways that make
no sense (or make no sense _to us_, _now_).
> > And further, I would even like to see that `--reverse` override go, and
> > turn it into `revs.reverse = !revs.reverse` instead. (And yes, I can
> > easily think of instances where I would have wanted to reverse a series of
> > patches...).
>
> I think this might deserve docs and tests too,
I agree.
> so it might want to be part of a separate patch series once the existing
> one has graduated.
Here, I disagree. This is a bug, from my perspective, and needs to be
fixed before graduating.
> At this point I don't think it's worth delaying this patch series for
> relatively small issues like this. There are many different ways this
> new command can be polished and improved. The important thing is that
> it looks like we all agree that the new command makes sense and should
> have roughly the basic set of features that Elijah originally
> implemented, so let's go with this, and then we can improve and
> iterate on top of this.
If it were for a small issue such as a typo, sure.
But it is a bug that `--no-reverse` is hard-coded (in a confusing manner
so because the `reverse` attribute is set), and the
`--no-reverse`/`--reverse` options are ignored, silently so.
Ciao,
Johannes
^ permalink raw reply
* Re: Is "bare"ness in the context of multiple worktrees weird? Bitmap error in git gc.
From: Tao Klerks @ 2023-09-07 4:53 UTC (permalink / raw)
To: Junio C Hamano
Cc: Kristoffer Haugsbakk, Sergey Organov, Eric Sunshine,
Johannes Schindelin, Taylor Blau, Patrick Steinhardt, git
In-Reply-To: <xmqqledjm4k2.fsf@gitster.g>
On Wed, Sep 6, 2023 at 10:26 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Tao Klerks <tao@klerks.biz> writes:
>
> > I like the nomenclature, I like the simple "zero (i.e. bare) or one
> > inline worktree, zero or more attached worktrees" explanation.
>
> We have used "main worktree" to refer to the working tree part (plus
> the repository) of a non-bare repository. And it makes sense to
> explain it together with the concept of "worktree", as the primary
> one is very much special in that it cannot be removed. You can see
> that "git worktree remove" would stop you from removing it with an
> error message:
>
> fatal: '../there' is a main working tree.
>
> It probably does not add much value to introduce a new term
> "inline". Here is what "git worktree --help" has to say about it.
>
> A repository has one main worktree (if it's not a bare repository) and
> zero or more linked worktrees.
I've definitely changed my mind about "inline", I agree "main" is
better. I'm not convinced it's the best word we could come up with,
but if it's well-established, I'm happy with it.
The problem I (now) see with "inline" is that it seems to imply a
spatial proximity that doesn't necessarily hold true, with
"--separate-git-dir" or other ways to separate the main worktree from
its usual "just above the .git directory" location. "Inline" is still
a reasonable qualification of the main worktree's *metadata* in that
situation (index, etc), but I think the word would not be sufficiently
clear/representative overall.
>
> I applaud whoever wrote this sentence for packing so much good
> information in a concise and easy-to-understand description.
I also like this sentence, it's basically equivalent to Sergey's sentence above.
>
> We can read that (1) a non-bare repository itself is considered
> its "main worktree", (2) a bare repository, by inference, has no
> main worktree (otherwise we wouldn't have said "if it's not"), and
> (3) both bare and non-bare repositories can have linked worktrees
> (again, otherwise we wouldn't have brought up a bare repository in
> the description).
>
> Perhaps we should borrow it to update the glossary, like so?
>
Looks good to me, but that leaves me with a different nitpick: we say
'One "worktree" consists of a "working tree" and repository metadata,
most of which are shared among other worktrees of a single repository,
and some of which are maintained separately per worktree'
This claims that the *shared metadata* (presumably the refs, the
branch reflogs, the objects, the config, etc) are *part of the
worktree* (a worktree "consists of" them and other things). That seems
like a very strange way to conceive of things, to me.
I would find it reasonable to state that the main worktree is part of
the repo - certainly that's now most everyday users would think of it,
if they were made to think of the worktree concept at all - but not
that the shared repo metadata is part of the worktree, and especially
not that the shared repo metadata is part of the attached worktrees.
I imagine that this weird phrasing intends to allude to the fact that
a worktree is "broken" without the repository metadata folder that
contains both its worktree-specific metadata and the shared metadata
that it depends just as much on... but can we come up with better
"relationship words here?
* A repository "has" zero or more worktrees
* If it "has" a "main" worktree it is not a bare repository, otherwise it is.
* It can have any number of "attached" worktrees
If a repo "has" these worktrees, is it in the sense that I "have" arms
and legs, and I "consist of" a person with arms and legs and other
body parts, or is it in the sense that I "have" a lifetime, opinions,
legal rights, and other things that I have as a consequence of being a
person, but are not "part of" me?
Similarly, do we have a term for "the directory that contains the
'refs' and 'objects' folders and stuff", regardless of whether it is
in fact the entire bare repository, typically with a name other than
".git", or it is nested in a main worktree in the usual fashion as
".git", or it is separated (and again, typically differently-named) in
a "--separate-git-dir" topology? I called it a "repository metadata
folder" above, but I'm not sure whether there is a correct, succinct
term for it.
Wrt to the "shared among other worktrees" bit specifically, I agree
with Sergey that "shared among all" would be clearer, but it's still
weird, because all of that shared metadata is "inherent to the repo"
beyond any and all worktrees. If this happens to be a bare repo, and
we remove all the attached worktrees, the metadata is still just as
meaningful - so saying that it was "shared among the worktrees", while
true, seems to be unnecessarily implying a smaller purpose/meaning
than appropriate.
Sorry to continue nitpicking - I would love to see a clear
nomenclature and description of these parts and their relationships
for people (with less git experience) to "get it" more easily.
>
> Documentation/glossary-content.txt | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git c/Documentation/glossary-content.txt w/Documentation/glossary-content.txt
> index 5a537268e2..d9ba3bab88 100644
> --- c/Documentation/glossary-content.txt
> +++ w/Documentation/glossary-content.txt
> @@ -694,8 +694,8 @@ The most notable example is `HEAD`.
> plus any local changes that you have made but not yet committed.
>
> [[def_worktree]]worktree::
> - A repository can have zero (i.e. bare repository) or one or
> - more worktrees attached to it. One "worktree" consists of a
> + A repository has one main worktree (if it's not a bare
> + repository) and zero or more linked worktrees. One "worktree" consists of a
> "working tree" and repository metadata, most of which are
> shared among other worktrees of a single repository, and
> some of which are maintained separately per worktree
^ permalink raw reply
* Re: Is "bare"ness in the context of multiple worktrees weird? Bitmap error in git gc.
From: Sergey Organov @ 2023-09-06 22:34 UTC (permalink / raw)
To: Junio C Hamano
Cc: Tao Klerks, Kristoffer Haugsbakk, Eric Sunshine,
Johannes Schindelin, Taylor Blau, Patrick Steinhardt, git
In-Reply-To: <xmqqy1hjkkxc.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Sergey Organov <sorganov@gmail.com> writes:
>
>> I agree "inline" is not much better than "main", nor "attached" is
>> better than "linked". I just pulled mine out of thin air, and what's
>> already there is probably fine.
>
> Heh, the initial draft of my message you are responding to used
> "primary" (and "attached"), because they are the word I am
> accustomed to use (out of thin air) on the list a few times, before
> checking with the existing documentation to realize that we use
> "main" for that.
>
>> That said, to be picky, "main" suggests
>> that linked worktrees are somehow inferior. Are they?
>
> I'd say that 'main' is different, not necessarily superiour, from
> all others and they are equally useful and usable. The difference
> is that it cannot be removed. There may be other differences I am
> forgetting, but I do not think it is about which is superiour and
> which is inferiour.
Well, if worktree created by "git clone/init" is not superior compared
to that created by "git worktree", just different, then "main" might be
not the best choice, but then, provided it's already in use, it's
probably not that big deal either.
As a note, "primary" also suggests the rest are "secondary", and then
"primary" one might not be there in the first place, leaving us with a
set of "secondary" without "primary", that is a bit confusing.
"Embedded", "integrated", or even "default" come to mind as
alternatives. However, if "attached" is decided upon, "inline" just
follows naturally.
^ permalink raw reply
* Re: Is "bare"ness in the context of multiple worktrees weird? Bitmap error in git gc.
From: Junio C Hamano @ 2023-09-06 22:15 UTC (permalink / raw)
To: Sergey Organov
Cc: Tao Klerks, Kristoffer Haugsbakk, Eric Sunshine,
Johannes Schindelin, Taylor Blau, Patrick Steinhardt, git
In-Reply-To: <878r9juflz.fsf@osv.gnss.ru>
Sergey Organov <sorganov@gmail.com> writes:
> I agree "inline" is not much better than "main", nor "attached" is
> better than "linked". I just pulled mine out of thin air, and what's
> already there is probably fine.
Heh, the initial draft of my message you are responding to used
"primary" (and "attached"), because they are the word I am
accustomed to use (out of thin air) on the list a few times, before
checking with the existing documentation to realize that we use
"main" for that.
> That said, to be picky, "main" suggests
> that linked worktrees are somehow inferior. Are they?
I'd say that 'main' is different, not necessarily superiour, from
all others and they are equally useful and usable. The difference
is that it cannot be removed. There may be other differences I am
forgetting, but I do not think it is about which is superiour and
which is inferiour.
^ permalink raw reply
* Re: Is "bare"ness in the context of multiple worktrees weird? Bitmap error in git gc.
From: Sergey Organov @ 2023-09-06 22:11 UTC (permalink / raw)
To: Junio C Hamano
Cc: Kristoffer Haugsbakk, Eric Sunshine, Tao Klerks,
Johannes Schindelin, Taylor Blau, Patrick Steinhardt, git
In-Reply-To: <xmqqwmx3m82l.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> "Kristoffer Haugsbakk" <code@khaugsbakk.name> writes:
>>
>>> But not with “worktree”:
>>>
>>> “ A repository can have zero (i.e. bare repository) or one or more
>>> worktrees attached to it. ...
>>>
>>> Since this entry claims that “bare repository” and “zero worktrees” are
>>> equivalent.
>>
>> I wrote that "(i.e. bare repository)" in 2df5387e (glossary:
>> describe "worktree", 2022-02-09) but did not mean that way.
>>
>> A non-bare repository can reduce the number of its worktrees, but it
>> cannot go below one, because the directory with working tree files
>> and the .git/ subdirectory, i.e. its primary worktree, must exist
>> for it to be a non-bare repository. Consequently a repository with
>> zero worktree is by definition a bare repository.
>>
>> But that does not have to mean all bare repositories can have no
>> worktrees.
>
> I re-read the glossary entry and I think the current text is mostly
> OK, except that it does not even have to mention "bare" at that
> position in the sentence. A bare repository with zero worktrees is
> totally uninteresting in the explanation of the worktree.
Sounds reasonable.
>
> We need to say that the repository data (configuration, refs and
> objecs) are mostly shared among worktrees while some data are kept
> per-worktree, which the current text adequately covers, and what is
> missing with respect to a bare repository is that we do not say
> worktrees can be attached after the fact to a repository that was
> created bare.
Why? Worktree could be attached after the fact to any repository. I
don't see why we need to mention bareness here, as it's not special in
this regard.
>
> So, perhaps something along this line?
>
> Documentation/glossary-content.txt | 16 +++++++++-------
> 1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git c/Documentation/glossary-content.txt w/Documentation/glossary-content.txt
> index 5a537268e2..6dba68ffc0 100644
> --- c/Documentation/glossary-content.txt
> +++ w/Documentation/glossary-content.txt
> @@ -694,10 +694,12 @@ The most notable example is `HEAD`.
> plus any local changes that you have made but not yet committed.
>
> [[def_worktree]]worktree::
> - A repository can have zero (i.e. bare repository) or one or
> - more worktrees attached to it. One "worktree" consists of a
> - "working tree" and repository metadata, most of which are
> - shared among other worktrees of a single repository, and
> - some of which are maintained separately per worktree
> - (e.g. the index, HEAD and pseudorefs like MERGE_HEAD,
> - per-worktree refs and per-worktree configuration file).
> + A repository can have zero or more worktrees attached to it.
> + One "worktree" consists of a "working tree" and repository
> + metadata, most of which are shared among other worktrees of
> + a single repository, and some of which are maintained
> + separately per worktree (e.g. the index, HEAD and pseudorefs
> + like MERGE_HEAD, per-worktree refs and per-worktree
> + configuration file).
> ++
> +Note that worktrees can be attached to an existing bare repository.
"shared among other worktrees" -> "shared among all worktrees"?
Also, if we do have "main worktree" and "linked worktree" as concepts,
they need to be at least mentioned in the glossary, I believe.
Finally, if we do have "linked worktrees", then the phrasing should
better use "linked" instead of "attached"? Alternatively, if "attached"
fits better, let's call them "attached worktrees"?
--
Sergey Organov
^ permalink raw reply
* Re: Is "bare"ness in the context of multiple worktrees weird? Bitmap error in git gc.
From: Sergey Organov @ 2023-09-06 22:00 UTC (permalink / raw)
To: Junio C Hamano
Cc: Tao Klerks, Kristoffer Haugsbakk, Eric Sunshine,
Johannes Schindelin, Taylor Blau, Patrick Steinhardt, git
In-Reply-To: <xmqqledjm4k2.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Tao Klerks <tao@klerks.biz> writes:
>
>> I like the nomenclature, I like the simple "zero (i.e. bare) or one
>> inline worktree, zero or more attached worktrees" explanation.
>
[...]
> It probably does not add much value to introduce a new term
> "inline". Here is what "git worktree --help" has to say about it.
>
> A repository has one main worktree (if it's not a bare repository) and
> zero or more linked worktrees.
>
> I applaud whoever wrote this sentence for packing so much good
> information in a concise and easy-to-understand description.
I agree "inline" is not much better than "main", nor "attached" is
better than "linked". I just pulled mine out of thin air, and what's
already there is probably fine. That said, to be picky, "main" suggests
that linked worktrees are somehow inferior. Are they?
--
Sergey Organov
^ permalink raw reply
* Re: [PATCH v4 0/7] rebase -i: impove handling of failed commands
From: Junio C Hamano @ 2023-09-06 21:01 UTC (permalink / raw)
To: Phillip Wood via GitGitGadget
Cc: git, Johannes Schindelin, Stefan Haller, Phillip Wood,
Eric Sunshine, Glen Choo, Phillip Wood
In-Reply-To: <pull.1492.v4.git.1694013771.gitgitgadget@gmail.com>
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> This series fixes several bugs in the way we handle a commit cannot be
> picked because it would overwrite an untracked file.
>
> * after a failed pick "git rebase --continue" will happily commit any
> staged changes even though no commit was picked.
>
> * the commit of the failed pick is recorded as rewritten even though no
> commit was picked.
>
> * the "done" file used by "git status" to show the recently executed
> commands contains an incorrect entry.
>
> Thanks to Eric, Glen and Junio for their comments on v2. Here are the
> changes since v2:
>
> Patch 1 - Reworded the commit message.
>
> Patch 2 - Reworded the commit message, added a test and fixed error message
> pointed out by Glen.
>
> Patch 3 - New cleanup.
>
> Patch 4 - Reworded the commit message, now only increments
> todo_list->current if there is no error.
>
> Patch 5 - Swapped with next patch. Reworded the commit message, stopped
> testing implementation (suggested by Glen). Expanded post-rewrite hook test.
>
> Patch 6 - Reworded the commit message, now uses the message file rather than
> the author script to check if "rebase --continue" should commit staged
> changes. Junio suggested using a separate file for this but I think that
> would end up being more involved as we'd need to be careful about creating
> and removing it.
>
> Patch 7 - Reworded the commit message.
Thanks. This version looks good to me (although I am not as
familiar with this part of the codebase as others on the Cc: line).
^ permalink raw reply
* Re: [PATCH] [diff-lib] Fix check_removed when fsmonitor is on
From: Jonathan Tan @ 2023-09-06 20:37 UTC (permalink / raw)
To: Josip Sokcevic; +Cc: Jonathan Tan, git, git
In-Reply-To: <20230906060241.944886-2-sokcevic@google.com>
Josip Sokcevic <sokcevic@google.com> writes:
> git-diff-index may return incorrect deleted entries when fsmonitor is used in a
> repository with git submodules. This can be observed on Mac machines, but it
> can affect all other supported platforms too.
>
> If fsmonitor is used, `stat *st` may not be initialized. Since `lstat` calls
> aren't not desired when fsmonitor is on, skip the entire gitlink check using
> the same condition used to initialize `stat *st`.
Ah, that's a great catch. Thanks for narrowing it down to
check_removed().
I see from "git blame" that this was introduced in 4f3d6d0261
(fsmonitor: skip lstat deletion check during git diff-index, 2021-03-
17). In that commit, subsequent code in check_removed() already used
"st" when it was possibly uninitialized, so perhaps no one noticed it.
I don't think anyone discussed it either on the mailing list thread that
introduced this patch [1].
[1] https://lore.kernel.org/git/pull.903.git.1615760258.gitgitgadget@gmail.com/
Looking at this function in more detail, though, I see that the solution
in this patch set is incomplete - both before and after this patch,
there are code paths in which "st" is never initialized, but "st->mode"
is used (not in check_removed(), but in its callers).
I think a more complete solution needs to look something like this:
diff --git a/diff-lib.c b/diff-lib.c
index d8aa777a73..5637237003 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -39,10 +39,20 @@
static int check_removed(const struct index_state *istate, const struct cache_entry *ce, struct stat *st)
{
assert(is_fsmonitor_refreshed(istate));
- if (!(ce->ce_flags & CE_FSMONITOR_VALID) && lstat(ce->name, st) < 0) {
- if (!is_missing_file_error(errno))
- return -1;
- return 1;
+ if (ce->ce_flags & CE_FSMONITOR_VALID) {
+ /*
+ * Both check_removed() and its callers expect lstat() to have
+ * happened and, in particular, the st_mode field to be set.
+ * Simulate this with the contents of ce.
+ */
+ memset(st, 0, sizeof(*st));
+ st->st_mode = ce->ce_mode;
+ } else {
+ if (lstat(ce->name, st) < 0) {
+ if (!is_missing_file_error(errno))
+ return -1;
+ return 1;
+ }
}
if (has_symlink_leading_path(ce->name, ce_namelen(ce)))
return 1;
Also, regarding the commit message, the title should be formatted like:
diff-lib: simulate stat when using fsmonitor
or something like that (file, then colon, then message).
You'll also need a sign-off. See the [[sign-off]] section in
Documentation/SubmittingPatches for more information.
> diff --git a/t/t7527-builtin-fsmonitor.sh b/t/t7527-builtin-fsmonitor.sh
> index 0c241d6c14..894a80fbe8 100755
> --- a/t/t7527-builtin-fsmonitor.sh
> +++ b/t/t7527-builtin-fsmonitor.sh
> @@ -824,6 +824,10 @@ test_expect_success 'submodule always visited' '
>
> create_super super &&
> create_sub sub &&
> + git -C super --no-optional-locks diff-index --name-status HEAD >actual.with &&
> + git -C super --no-optional-locks -c core.fsmonitor=false \
> + diff-index --name-status HEAD >actual.without &&
> + test_cmp actual.with actual.without &&
>
> git -C super submodule add ../sub ./dir_1/dir_2/sub &&
> git -C super commit -m "add sub" &&
Any reason why these additions are here instead of where I put them (in
my_match_and_clean())? I think it makes more sense where I put them,
as then they would automatically apply to all tests.
> @@ -837,6 +841,8 @@ test_expect_success 'submodule always visited' '
> # some dirt in the submodule and confirm matching output.
>
> # Completely clean status.
> + echo Now running for clean status &&
> +
> my_match_and_clean &&
>
> # .M S..U
You can remove this - I included this to show exactly where in the code
the test fails.
^ permalink raw reply
* Re: Is "bare"ness in the context of multiple worktrees weird? Bitmap error in git gc.
From: Junio C Hamano @ 2023-09-06 20:26 UTC (permalink / raw)
To: Tao Klerks
Cc: Kristoffer Haugsbakk, Sergey Organov, Eric Sunshine,
Johannes Schindelin, Taylor Blau, Patrick Steinhardt, git
In-Reply-To: <CAPMMpohgkH3h1zC_Q7O-07gYw8_7mdSsyX7vu1K1u5+CxKUaUQ@mail.gmail.com>
Tao Klerks <tao@klerks.biz> writes:
> I like the nomenclature, I like the simple "zero (i.e. bare) or one
> inline worktree, zero or more attached worktrees" explanation.
We have used "main worktree" to refer to the working tree part (plus
the repository) of a non-bare repository. And it makes sense to
explain it together with the concept of "worktree", as the primary
one is very much special in that it cannot be removed. You can see
that "git worktree remove" would stop you from removing it with an
error message:
fatal: '../there' is a main working tree.
It probably does not add much value to introduce a new term
"inline". Here is what "git worktree --help" has to say about it.
A repository has one main worktree (if it's not a bare repository) and
zero or more linked worktrees.
I applaud whoever wrote this sentence for packing so much good
information in a concise and easy-to-understand description.
We can read that (1) a non-bare repository itself is considered
its "main worktree", (2) a bare repository, by inference, has no
main worktree (otherwise we wouldn't have said "if it's not"), and
(3) both bare and non-bare repositories can have linked worktrees
(again, otherwise we wouldn't have brought up a bare repository in
the description).
Perhaps we should borrow it to update the glossary, like so?
Documentation/glossary-content.txt | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git c/Documentation/glossary-content.txt w/Documentation/glossary-content.txt
index 5a537268e2..d9ba3bab88 100644
--- c/Documentation/glossary-content.txt
+++ w/Documentation/glossary-content.txt
@@ -694,8 +694,8 @@ The most notable example is `HEAD`.
plus any local changes that you have made but not yet committed.
[[def_worktree]]worktree::
- A repository can have zero (i.e. bare repository) or one or
- more worktrees attached to it. One "worktree" consists of a
+ A repository has one main worktree (if it's not a bare
+ repository) and zero or more linked worktrees. One "worktree" consists of a
"working tree" and repository metadata, most of which are
shared among other worktrees of a single repository, and
some of which are maintained separately per worktree
^ permalink raw reply related
* Re: Is "bare"ness in the context of multiple worktrees weird? Bitmap error in git gc.
From: Junio C Hamano @ 2023-09-06 19:10 UTC (permalink / raw)
To: Kristoffer Haugsbakk
Cc: Eric Sunshine, Tao Klerks, Johannes Schindelin, Taylor Blau,
Patrick Steinhardt, git
In-Reply-To: <xmqq5y4nnq9b.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> "Kristoffer Haugsbakk" <code@khaugsbakk.name> writes:
>
>> But not with “worktree”:
>>
>> “ A repository can have zero (i.e. bare repository) or one or more
>> worktrees attached to it. ...
>>
>> Since this entry claims that “bare repository” and “zero worktrees” are
>> equivalent.
>
> I wrote that "(i.e. bare repository)" in 2df5387e (glossary:
> describe "worktree", 2022-02-09) but did not mean that way.
>
> A non-bare repository can reduce the number of its worktrees, but it
> cannot go below one, because the directory with working tree files
> and the .git/ subdirectory, i.e. its primary worktree, must exist
> for it to be a non-bare repository. Consequently a repository with
> zero worktree is by definition a bare repository.
>
> But that does not have to mean all bare repositories can have no
> worktrees.
I re-read the glossary entry and I think the current text is mostly
OK, except that it does not even have to mention "bare" at that
position in the sentence. A bare repository with zero worktrees is
totally uninteresting in the explanation of the worktree.
We need to say that the repository data (configuration, refs and
objecs) are mostly shared among worktrees while some data are kept
per-worktree, which the current text adequately covers, and what is
missing with respect to a bare repository is that we do not say
worktrees can be attached after the fact to a repository that was
created bare.
So, perhaps something along this line?
Documentation/glossary-content.txt | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git c/Documentation/glossary-content.txt w/Documentation/glossary-content.txt
index 5a537268e2..6dba68ffc0 100644
--- c/Documentation/glossary-content.txt
+++ w/Documentation/glossary-content.txt
@@ -694,10 +694,12 @@ The most notable example is `HEAD`.
plus any local changes that you have made but not yet committed.
[[def_worktree]]worktree::
- A repository can have zero (i.e. bare repository) or one or
- more worktrees attached to it. One "worktree" consists of a
- "working tree" and repository metadata, most of which are
- shared among other worktrees of a single repository, and
- some of which are maintained separately per worktree
- (e.g. the index, HEAD and pseudorefs like MERGE_HEAD,
- per-worktree refs and per-worktree configuration file).
+ A repository can have zero or more worktrees attached to it.
+ One "worktree" consists of a "working tree" and repository
+ metadata, most of which are shared among other worktrees of
+ a single repository, and some of which are maintained
+ separately per worktree (e.g. the index, HEAD and pseudorefs
+ like MERGE_HEAD, per-worktree refs and per-worktree
+ configuration file).
++
+Note that worktrees can be attached to an existing bare repository.
^ permalink raw reply related
* Re: Is "bare"ness in the context of multiple worktrees weird? Bitmap error in git gc.
From: Kristoffer Haugsbakk @ 2023-09-06 18:08 UTC (permalink / raw)
To: Junio C Hamano
Cc: Eric Sunshine, Tao Klerks, Johannes Schindelin, Taylor Blau,
Patrick Steinhardt, git
In-Reply-To: <xmqq5y4nnq9b.fsf@gitster.g>
On Wed, Sep 6, 2023, at 19:52, Junio C Hamano wrote:
> I wrote that "(i.e. bare repository)" in 2df5387e (glossary:
> describe "worktree", 2022-02-09) but did not mean that way.
>
> A non-bare repository can reduce the number of its worktrees, but it
> cannot go below one, because the directory with working tree files
> and the .git/ subdirectory, i.e. its primary worktree, must exist
> for it to be a non-bare repository. Consequently a repository with
> zero worktree is by definition a bare repository.
>
> But that does not have to mean all bare repositories can have no
> worktrees.
I see. Zero worktrees implies bare repository, but bare repository does
not imply zero worktrees. I got my logical connectives mixed up.
Thanks
--
Kristoffer Haugsbakk
^ permalink raw reply
* Re: Is "bare"ness in the context of multiple worktrees weird? Bitmap error in git gc.
From: Tao Klerks @ 2023-09-06 18:04 UTC (permalink / raw)
To: Kristoffer Haugsbakk
Cc: Sergey Organov, Eric Sunshine, Johannes Schindelin, Taylor Blau,
Patrick Steinhardt, git
In-Reply-To: <c0a10738-86ba-4b3a-9e74-2568cc407621@app.fastmail.com>
On Wed, Sep 6, 2023 at 8:00 PM Kristoffer Haugsbakk
<code@khaugsbakk.name> wrote:
>
> On Wed, Sep 6, 2023, at 18:39, Sergey Organov wrote:
<SNIP>
> >
> > As I see it, in fact a repository can have zero (i.e. bare repository)
> > or one inline worktree, as well as zero or more attached worktrees.
>
> Ah, thank you. I felt like the glossary/nomenclature was missing a few
> words and these ones seem to fill things in nicely.
>
Yes, I agree!!
I like the nomenclature, I like the simple "zero (i.e. bare) or one
inline worktree, zero or more attached worktrees" explanation.
^ permalink raw reply
* Re: Is "bare"ness in the context of multiple worktrees weird? Bitmap error in git gc.
From: Kristoffer Haugsbakk @ 2023-09-06 17:59 UTC (permalink / raw)
To: Sergey Organov
Cc: Eric Sunshine, Tao Klerks, Johannes Schindelin, Taylor Blau,
Patrick Steinhardt, git
In-Reply-To: <87edjbuugw.fsf@osv.gnss.ru>
On Wed, Sep 6, 2023, at 18:39, Sergey Organov wrote:
>> -- >8 --
>> Subject: [PATCH] Try to reword what a worktree is
>>
>> ---
>> Documentation/glossary-content.txt | 6 +++++-
>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt
>> index 5a537268e2..5e192fb5dc 100644
>> --- a/Documentation/glossary-content.txt
>> +++ b/Documentation/glossary-content.txt
>> @@ -694,10 +694,14 @@ The most notable example is `HEAD`.
>> plus any local changes that you have made but not yet committed.
>>
>> [[def_worktree]]worktree::
>> - A repository can have zero (i.e. bare repository) or one or
>> + A repository can have zero or one or
>> more worktrees attached to it. One "worktree" consists of a
>> "working tree" and repository metadata, most of which are
>> shared among other worktrees of a single repository, and
>> some of which are maintained separately per worktree
>> (e.g. the index, HEAD and pseudorefs like MERGE_HEAD,
>> per-worktree refs and per-worktree configuration file).
>> ++
>> +Note that the directory tree of a <<def_bare_repository,bare_repository>>
>> +may have linked worktrees, but cannot itself be a worktree since it has no
>> +working tree.
>
> Reading this with a fresh eye, I wonder if we'd better distinguish
> between "inline" worktree and "attached" worktrees?
>
> As I see it, in fact a repository can have zero (i.e. bare repository)
> or one inline worktree, as well as zero or more attached worktrees.
Ah, thank you. I felt like the glossary/nomenclature was missing a few
words and these ones seem to fill things in nicely.
Now I'm just skeptical of the other wording issue about “bare repository”,
which might be somewhat out of place in the face of zero-to-multiple
worktrees. Going back to my example in the previous email:
• `repository.git` is a *bare repository* which has no *inline worktree*
and three *attached worktrees* [I really like how inline/attached work
here]
• `a` is an *attached worktree* of `repository.git`
• `a`, `b`, `c` are all the *worktrees* of the *bare repository*
`repository.git` [“bare” here just emphasizes that `repository.git` does
not have a worktree (“what about the worktree in `repository.git`?”)]
Does that sound right? (Asking no one in particular.) Personally I think
that it sounds more coherent than before I wrote it (than I thought it
would).
--
Kristoffer Haugsbakk
^ permalink raw reply
* Re: Is "bare"ness in the context of multiple worktrees weird? Bitmap error in git gc.
From: Junio C Hamano @ 2023-09-06 17:52 UTC (permalink / raw)
To: Kristoffer Haugsbakk
Cc: Eric Sunshine, Tao Klerks, Johannes Schindelin, Taylor Blau,
Patrick Steinhardt, git
In-Reply-To: <2ba66542-9ae2-4b13-ae6b-f37dec6b72c7@app.fastmail.com>
"Kristoffer Haugsbakk" <code@khaugsbakk.name> writes:
> But not with “worktree”:
>
> “ A repository can have zero (i.e. bare repository) or one or more
> worktrees attached to it. ...
>
> Since this entry claims that “bare repository” and “zero worktrees” are
> equivalent.
I wrote that "(i.e. bare repository)" in 2df5387e (glossary:
describe "worktree", 2022-02-09) but did not mean that way.
A non-bare repository can reduce the number of its worktrees, but it
cannot go below one, because the directory with working tree files
and the .git/ subdirectory, i.e. its primary worktree, must exist
for it to be a non-bare repository. Consequently a repository with
zero worktree is by definition a bare repository.
But that does not have to mean all bare repositories can have no
worktrees.
^ permalink raw reply
* Re: Is "bare"ness in the context of multiple worktrees weird? Bitmap error in git gc.
From: Kristoffer Haugsbakk @ 2023-09-06 17:29 UTC (permalink / raw)
To: Tao Klerks
Cc: Eric Sunshine, git, Johannes Schindelin, Taylor Blau,
Patrick Steinhardt, Junio C Hamano
In-Reply-To: <CAPMMpogm2tr0dy1nsV9NtF4O8-JS=_L3J0+yKRc7KbyAJ-PNbQ@mail.gmail.com>
On Tue, Sep 5, 2023, at 18:25, Tao Klerks wrote:
> Fwiw, Eric's reading was my intended one. The people I have spoken
> with, as well as myself, have started using "git worktree" by doing
> the former, and only later felt really transgressive when placing the
> worktrees explicitly on a higher level, on equal footing with the
> "main worktree". To me it seemed natural that the "nested worktrees"
> approach was the expected one, as otherwise it gets even harder to
> explain/justify the operational difference between the "main worktree"
> and the other worktrees - then leading to the bare+worktrees approach
> to eliminate that operational difference.
Let's consider a use-case that `git-worktrees` can replace: just cloning
the repository again to do some particular task.
My coworkers have to work on some branch `divergent` which requires a
complete rebuild of the project compared to the main branch. But they also
need to work on small derivative branches of the main branch. In order to
not rebuild all the time they simply cloned the project again and work in
*that* repository when working on `divergent`. And since this is an
Intellij project it was cloned in the same directory as the original
clone.
Replacing this use-case with `git worktrees` would be:
git worktree ../divergent-project
In this case, `git worktrees` is a more streamlined and better version of
cloning a separate repository:
1. Only one object store
2. No need to remember to fetch from both
3. No risk of forgetting that you have some n-iary clone on your machine
with original work
But crucially they also had the luxury of just cloning the project again
since it is less than 1GB; people with larger repositories (like
yourself) might have to immediately use the streamlined approaches like
`git worktree` since the straightforward approaches are too costly.
So I think the sibling directory tree makes sense when you arrive at this
command/workflow from the brute-force approach. But maybe that's not the
case when you have to design the proper way of doing it up-front(?)
> Is there a manual for "expected typical usage of git worktree" somewhere?
The example in “Description” of `man git-worktree` uses a sibling
directory: `git worktree ../hotfix`.
>> Even though deriving the worktree(s) from a separate and protected
>> bare repositories does protect you from total disaster caused by
>> removing "rm -fr" and bypassing "git worktree remove", it still
>> should be discouraged, as the per-worktree states left behind in the
>> repository interfere with the operations in surviving worktrees.
>
> Right, that's fine. Of course you're going to encourage deleting the
> worktrees carefully... but equally of-course, some people *will* do
> "rm -fr that-worktree-I-dont-know-how-to-clean", and when they do,
> telling them "just 'git worktree repair'" is much easier than telling
> them to "recover deleted files 'cause your local branches just
> evaporated"
>
>> Teaching folks not to do "rm -fr" would be the first step to a more
>> pleasant end-user experience, I would think.
>
> The less arcane trivia you *need* to teach users for them to be
> effective, the better the experience is for everyone.
>
> The fact that "deleting a standalone git repo only deletes what's in
> that standalone git repo the way you've done your whole life, but in
> this environment what look like multiple repos are actually
> 'worktrees', if you ever delete one your life *might*, if you choose
> the wrong one, suddenly be very unpleasant" is arcane trivia, in my
> opinion. Better to set things up so they *can't* shoot themselves in
> the foot with a bullet of that caliber.
I don't see how the principle of respecting the level of abstraction
doesn't apply here.
Before you might be able to delete a branch `b` by doing `git rm
.git/refs/heads/b` and be content when that either finishes successfully
or when it complains that the file doesn't exist; you can feel confident
that there is no more `b` branch. Now though the ref `b` might be a
*packed ref*, so you cannot do that and be sure that the ref was removed.
So if you create a worktree with `git worktree`, you should probably
remove it with the same command.
(Am I missing something? I probably am. I might not have understood the
context for wanting to run rm(1) on these directories.)
Personally I like to conceptualize worktrees as having equal status to
each other as far as me (the user) is concerned, since there isn't
anything you can do in the main worktree–or at least I haven't found
anything like that—that I *can't* do in a worktree *at that level of
abstraction* (directly manipulating files in the `.git` or
`repository.git` directory doesn't apply to this level of abstraction, so
that the linked worktrees only have a gitfile is irrelevant here).
--
Kristoffer Haugsbakk
^ permalink raw reply
* Re: [PATCH] show doc: redirect user to git log manual instead of git diff-tree
From: Junio C Hamano @ 2023-09-06 17:26 UTC (permalink / raw)
To: Han Young; +Cc: git
In-Reply-To: <CAG1j3zFYbeXokd9kwN06RG7Wih_WsLVBf6jSi-JKtUpa=i4QJg@mail.gmail.com>
Han Young <hanyang.tony@bytedance.com> writes:
>> Strictly speaking, "options to control how the changes are shown"
>> are options that are meant for "diff" command (e.g. "--stat", "-w"),
>> but "log" understands some of the "diff" command options, the
>> updated text is *not* incorrect.
>
> On a closer look, the manual page of git show does lists
> all the "diff" options by including diff-options.txt.
> The options omitted are revision parsing related.
The primary difference between "log" and "show" is that the latter
is not about walking a range of commits. Both grok "diff" related
options that control how the changes introduced by each commit are
shown equally well. But in the context of "show", options that
control revision traversal (i.e. which commits are chosen to be
shown and in what order) do not make much sense.
So ...
> Perhaps we remove the line
>
>> The command takes options applicable to the git diff-tree command to
>> control how the changes the commit introduces are shown.
>
> And rephrase the line
>
>> This manual page describes only the most frequently used options.
>
> to
>
>> This manual page describes only the most frequently used options.
>> Some options that `git rev-list` command understands can be used to
>> control how commits are shown.
... I do not find the above an improvement.
Perhaps we should start with just "diff-tree" -> "log" and doing no
other damage to the existing text. That will give us some baseline
that is a strict improvement.
Thanks.
^ permalink raw reply
* Re: [PATCH 7/8] builtin/repack.c: drop `DELETE_PACK` macro
From: Taylor Blau @ 2023-09-06 17:22 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King, Patrick Steinhardt
In-Reply-To: <xmqqh6o7nsf2.fsf@gitster.g>
On Wed, Sep 06, 2023 at 10:05:37AM -0700, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> > Signed-off-by: Taylor Blau <me@ttaylorr.com>
> > ---
> > builtin/repack.c | 18 ++++++++++--------
> > 1 file changed, 10 insertions(+), 8 deletions(-)
>
> The reason being...?
Wow, I have no idea how this got sent out without a commit message! At
least it was signed off ;-).
Here's a replacement that I cooked up, with your Helped-by. Let me know
if you want to replace this patch manually, or if you'd prefer a reroll
of the series. Either is fine with me! :-)
--- 8< ---
Subject: [PATCH] builtin/repack.c: treat string_list_item util as booleans
The `->util` field corresponding to each string_list_item used to track
the existence of some pack at the beginning of a repack operation was
originally intended to be used as a bitfield.
This bitfield tracked:
- (1 << 0): whether or not the pack should be deleted
- (1 << 1): whether or not the pack is cruft
The previous commit removed the use of the second bit, meaning that we
can treat this field as a boolean instead of a bitset.
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
builtin/repack.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index 478fab96c9..575e69b020 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -26,7 +26,7 @@
#define LOOSEN_UNREACHABLE 2
#define PACK_CRUFT 4
-#define DELETE_PACK 1
+#define DELETE_PACK ((void*)(uintptr_t)1)
static int pack_everything;
static int delta_base_offset = 1;
@@ -96,6 +96,10 @@ static int repack_config(const char *var, const char *value,
struct existing_packs {
struct string_list kept_packs;
+ /*
+ * for both non_kept_packs, and cruft_packs, a non-NULL
+ * 'util' field indicates the pack should be deleted.
+ */
struct string_list non_kept_packs;
struct string_list cruft_packs;
};
@@ -130,7 +134,7 @@ static void mark_packs_for_deletion_1(struct string_list *names,
* (if `-d` was given).
*/
if (!string_list_has_string(names, sha1))
- item->util = (void*)(uintptr_t)((size_t)item->util | DELETE_PACK);
+ item->util = DELETE_PACK;
}
}
@@ -158,7 +162,7 @@ static void remove_redundant_packs_1(struct string_list *packs)
{
struct string_list_item *item;
for_each_string_list_item(item, packs) {
- if (!((uintptr_t)item->util & DELETE_PACK))
+ if (!item->util)
continue;
remove_redundant_pack(packdir, item->string);
}
@@ -695,20 +699,20 @@ static void midx_included_packs(struct string_list *include,
for_each_string_list_item(item, &existing->cruft_packs) {
/*
- * no need to check DELETE_PACK, since we're not
- * doing an ALL_INTO_ONE repack
+ * no need to check for deleted packs, since we're
+ * not doing an ALL_INTO_ONE repack
*/
string_list_insert(include, xstrfmt("%s.idx", item->string));
}
} else {
for_each_string_list_item(item, &existing->non_kept_packs) {
- if ((uintptr_t)item->util & DELETE_PACK)
+ if (item->util)
continue;
string_list_insert(include, xstrfmt("%s.idx", item->string));
}
for_each_string_list_item(item, &existing->cruft_packs) {
- if ((uintptr_t)item->util & DELETE_PACK)
+ if (item->util)
continue;
string_list_insert(include, xstrfmt("%s.idx", item->string));
}
--
2.38.0.16.g393fd4c6db
--- >8 ---
Thanks,
Taylor
^ permalink raw reply related
* Re: [PATCH 7/8] builtin/repack.c: drop `DELETE_PACK` macro
From: Junio C Hamano @ 2023-09-06 17:05 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Jeff King, Patrick Steinhardt
In-Reply-To: <559b487e2ab056c79367a673188764e4cdce3c96.1693946195.git.me@ttaylorr.com>
Taylor Blau <me@ttaylorr.com> writes:
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
> builtin/repack.c | 18 ++++++++++--------
> 1 file changed, 10 insertions(+), 8 deletions(-)
The reason being...?
> @@ -130,7 +132,7 @@ static void mark_packs_for_deletion_1(struct string_list *names,
> * (if `-d` was given).
> */
> if (!string_list_has_string(names, sha1))
> - item->util = (void*)(uintptr_t)((size_t)item->util | DELETE_PACK);
> + item->util = (void*)1;
Presumably the literal "1" is promoted to an appropriate type
(uintptr_t) here?
We originally planned to use the .util member to store a bitset
to represent various attributes for each pack, and defined
DELETE_PACK as one of the possible bits, but over the N years of
its existence, we never found the need for the second bit.
or something?
It is not a bad idea to demote .util from a set of bits to just a
"is it NULL?" boolean, but updating the above to something like
#define DELETE_PACK ((void*)(uintptr_t)1)
item->util = DELETE_PACK;
may still reap the same simplification benefit without introducing
the "what is that _one_ about? can it be _two_ without changing the
meaning or what?" puzzlement.
Other than that, everything else in the series looked quite
straight-forward and sensible.
Thanks.
^ permalink raw reply
* Re: Is "bare"ness in the context of multiple worktrees weird? Bitmap error in git gc.
From: Sergey Organov @ 2023-09-06 16:39 UTC (permalink / raw)
To: Kristoffer Haugsbakk
Cc: Eric Sunshine, Tao Klerks, Johannes Schindelin, Taylor Blau,
Patrick Steinhardt, git
In-Reply-To: <2ba66542-9ae2-4b13-ae6b-f37dec6b72c7@app.fastmail.com>
"Kristoffer Haugsbakk" <code@khaugsbakk.name> writes:
[...]
> -- >8 --
> Subject: [PATCH] Try to reword what a worktree is
>
> ---
> Documentation/glossary-content.txt | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt
> index 5a537268e2..5e192fb5dc 100644
> --- a/Documentation/glossary-content.txt
> +++ b/Documentation/glossary-content.txt
> @@ -694,10 +694,14 @@ The most notable example is `HEAD`.
> plus any local changes that you have made but not yet committed.
>
> [[def_worktree]]worktree::
> - A repository can have zero (i.e. bare repository) or one or
> + A repository can have zero or one or
> more worktrees attached to it. One "worktree" consists of a
> "working tree" and repository metadata, most of which are
> shared among other worktrees of a single repository, and
> some of which are maintained separately per worktree
> (e.g. the index, HEAD and pseudorefs like MERGE_HEAD,
> per-worktree refs and per-worktree configuration file).
> ++
> +Note that the directory tree of a <<def_bare_repository,bare_repository>>
> +may have linked worktrees, but cannot itself be a worktree since it has no
> +working tree.
Reading this with a fresh eye, I wonder if we'd better distinguish
between "inline" worktree and "attached" worktrees?
As I see it, in fact a repository can have zero (i.e. bare repository)
or one inline worktree, as well as zero or more attached worktrees.
--
Sergey Organov
^ permalink raw reply
* Re: Is "bare"ness in the context of multiple worktrees weird? Bitmap error in git gc.
From: Kristoffer Haugsbakk @ 2023-09-06 16:00 UTC (permalink / raw)
To: Eric Sunshine
Cc: Tao Klerks, Johannes Schindelin, Taylor Blau, Patrick Steinhardt,
git
In-Reply-To: <CAPig+cQoiqeZF52Jr45an+cZF+ZQbHPXtLVn+VmyegjMQaJqCg@mail.gmail.com>
On Tue, Sep 5, 2023, at 02:38, Eric Sunshine wrote:
> Speaking as a person involved in the implementation of worktrees,
> including support for them in combination with bare repositories, my
> reading of this is perhaps biased so that I understand its intent.
> However, if I squint hard, I suppose I can see how you could read it
> as meaning that a bare repository can't have any worktrees associated
> with it. So, perhaps, the documentation could use a bit of touch up.
My interpretation of the documentation leads to contradictions. So I
thought of another one: A bare repository *can* have worktrees, but if it
does will in that case only have *linked* worktrees, since the
`repository.git` directory by definition does not have a working tree and
it therefore cannot be considered a worktree itself. By extension, a
linked worktree might be linked to a bare repository. Thus there is no
contradiction.
For example: there are four directory trees associated with the `repo`
repository:
1. `repo.git`: the directory tree with the bare repository; no worktree in
*that* directory tree
2. `a`: worktree with a gitfile that points to `repo.git`
3. `b`: worktree with a gitfile that points to `repo.git`
4. `c`: worktree with a gitfile that points to `repo.git`
So you have:
• One repository
• Four directory trees
• Three worktrees
This interpretation seems completely in line with “bare repository” in the
glossary:
“ A bare repository is normally an appropriately named directory with a
.git suffix that does not have a locally checked-out copy of any of
the files under revision control. That is, all of the Git
administrative and control files that would normally be present in the
hidden .git sub-directory are directly present in the repository.git
directory instead, and no other files are present and checked
out. Usually publishers of public repositories make bare repositories
available.
But not with “worktree”:
“ A repository can have zero (i.e. bare repository) or one or more
worktrees attached to it. ...
Since this entry claims that “bare repository” and “zero worktrees” are
equivalent.
Nothwithstanding any implementation/documentation disagreement, I think
that this interpretation at least is coherent.
But note how (for me) it is a bit awkward to refer to a “bare repository”
in this context since I need to add “the directory tree” in order to
emphasize that we are talking about `repo.git`; normally you can kind of
loosely talk about “the repository” and still get the precise meaning that
you intend across, but in this case we have four directory trees which are
all *the same repository*. (Right?) So just saying “the bare repository”
can be misleading since it might hint that the three worktrees are not
part of the repository. (Perhaps there isn't enough nomenclature to
clearly talk about this particular case/setup?)
But with all of that in mind, perhaps the glossary could read something
like this instead (no reflowing):[1][2]
(`man git-worktree` might also need to be updated.)
† 1: Applied onto 1fc548b2d6 (The sixth batch, 2023-09-05)
🔗 2: https://github.com/git/git/compare/master...LemmingAvalanche:git:bare-and-worktrees?expand=1
Cheers
Kristoffer
-- >8 --
Subject: [PATCH] Try to reword what a worktree is
---
Documentation/glossary-content.txt | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt
index 5a537268e2..5e192fb5dc 100644
--- a/Documentation/glossary-content.txt
+++ b/Documentation/glossary-content.txt
@@ -694,10 +694,14 @@ The most notable example is `HEAD`.
plus any local changes that you have made but not yet committed.
[[def_worktree]]worktree::
- A repository can have zero (i.e. bare repository) or one or
+ A repository can have zero or one or
more worktrees attached to it. One "worktree" consists of a
"working tree" and repository metadata, most of which are
shared among other worktrees of a single repository, and
some of which are maintained separately per worktree
(e.g. the index, HEAD and pseudorefs like MERGE_HEAD,
per-worktree refs and per-worktree configuration file).
++
+Note that the directory tree of a <<def_bare_repository,bare_repository>>
+may have linked worktrees, but cannot itself be a worktree since it has no
+working tree.
--
2.42.0
^ permalink raw reply related
* Re: [PATCH 1/1] doc/diff-options: fix link to generating patch section
From: Junio C Hamano @ 2023-09-06 15:58 UTC (permalink / raw)
To: Sergey Organov; +Cc: git, John Cai
In-Reply-To: <87tts7pzl2.fsf@osv.gnss.ru>
Sergey Organov <sorganov@gmail.com> writes:
> In fact I just realized that removing conditionals in fact *fixes* those
> documents by providing proper link in them as well, so I'll think of
> better description taking into account your observation as well, and
> then will re-roll.
Thanks.
^ permalink raw reply
* [PATCH v4 7/7] rebase -i: fix adding failed command to the todo list
From: Phillip Wood via GitGitGadget @ 2023-09-06 15:22 UTC (permalink / raw)
To: git
Cc: Johannes Schindelin, Junio C Hamano, Stefan Haller, Phillip Wood,
Eric Sunshine, Glen Choo, Phillip Wood, Phillip Wood
In-Reply-To: <pull.1492.v4.git.1694013771.gitgitgadget@gmail.com>
From: Phillip Wood <phillip.wood@dunelm.org.uk>
When rebasing commands are moved from the todo list in "git-rebase-todo"
to the "done" file (which is used by "git status" to show the recently
executed commands) just before they are executed. This means that if a
command fails because it would overwrite an untracked file it has to be
added back into the todo list before the rebase stops for the user to
fix the problem.
Unfortunately when a failed command is added back into the todo list the
command preceding it is erroneously appended to the "done" file. This
means that when rebase stops after "pick B" fails the "done" file
contains
pick A
pick B
pick A
instead of
pick A
pick B
This happens because save_todo() updates the "done" file with the
previous command whenever "git-rebase-todo" is updated. When we add the
failed pick back into "git-rebase-todo" we do not want to update
"done". Fix this by adding a "reschedule" parameter to save_todo() which
prevents the "done" file from being updated when adding a failed command
back into the "git-rebase-todo" file. A couple of the existing tests are
modified to improve their coverage as none of them trigger this bug or
check the "done" file.
Reported-by: Stefan Haller <lists@haller-berlin.de>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
sequencer.c | 12 ++++++------
t/t3404-rebase-interactive.sh | 29 ++++++++++++++++++-----------
t/t3430-rebase-merges.sh | 22 ++++++++++++++++------
3 files changed, 40 insertions(+), 23 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 38b0f213157..175addb7ca6 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3397,7 +3397,8 @@ give_advice:
return -1;
}
-static int save_todo(struct todo_list *todo_list, struct replay_opts *opts)
+static int save_todo(struct todo_list *todo_list, struct replay_opts *opts,
+ int reschedule)
{
struct lock_file todo_lock = LOCK_INIT;
const char *todo_path = get_todo_path(opts);
@@ -3407,7 +3408,7 @@ static int save_todo(struct todo_list *todo_list, struct replay_opts *opts)
* rebase -i writes "git-rebase-todo" without the currently executing
* command, appending it to "done" instead.
*/
- if (is_rebase_i(opts))
+ if (is_rebase_i(opts) && !reschedule)
next++;
fd = hold_lock_file_for_update(&todo_lock, todo_path, 0);
@@ -3420,7 +3421,7 @@ static int save_todo(struct todo_list *todo_list, struct replay_opts *opts)
if (commit_lock_file(&todo_lock) < 0)
return error(_("failed to finalize '%s'"), todo_path);
- if (is_rebase_i(opts) && next > 0) {
+ if (is_rebase_i(opts) && !reschedule && next > 0) {
const char *done = rebase_path_done();
int fd = open(done, O_CREAT | O_WRONLY | O_APPEND, 0666);
int ret = 0;
@@ -4733,7 +4734,7 @@ static int pick_commits(struct repository *r,
const char *arg = todo_item_get_arg(todo_list, item);
int check_todo = 0;
- if (save_todo(todo_list, opts))
+ if (save_todo(todo_list, opts, reschedule))
return -1;
if (is_rebase_i(opts)) {
if (item->command != TODO_COMMENT) {
@@ -4814,8 +4815,7 @@ static int pick_commits(struct repository *r,
get_item_line_length(todo_list,
todo_list->current),
get_item_line(todo_list, todo_list->current));
- todo_list->current--;
- if (save_todo(todo_list, opts))
+ if (save_todo(todo_list, opts, reschedule))
return -1;
if (item->commit)
write_rebase_head(&item->commit->object.oid);
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index a8ad398956a..71da9c465a1 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1277,19 +1277,24 @@ test_expect_success 'todo count' '
'
test_expect_success 'rebase -i commits that overwrite untracked files (pick)' '
- git checkout --force branch2 &&
+ git checkout --force A &&
git clean -f &&
+ cat >todo <<-EOF &&
+ exec >file2
+ pick $(git rev-parse B) B
+ pick $(git rev-parse C) C
+ pick $(git rev-parse D) D
+ exec cat .git/rebase-merge/done >actual
+ EOF
(
- set_fake_editor &&
- FAKE_LINES="edit 1 2" git rebase -i A
+ set_replace_editor todo &&
+ test_must_fail git rebase -i A
) &&
- test_cmp_rev HEAD F &&
- test_path_is_missing file6 &&
- >file6 &&
- test_must_fail git rebase --continue &&
- test_cmp_rev HEAD F &&
- test_cmp_rev REBASE_HEAD I &&
- rm file6 &&
+ test_cmp_rev HEAD B &&
+ test_cmp_rev REBASE_HEAD C &&
+ head -n3 todo >expect &&
+ test_cmp expect .git/rebase-merge/done &&
+ rm file2 &&
test_path_is_missing .git/rebase-merge/patch &&
echo changed >file1 &&
git add file1 &&
@@ -1297,7 +1302,9 @@ test_expect_success 'rebase -i commits that overwrite untracked files (pick)' '
grep "error: you have staged changes in your working tree" err &&
git reset --hard HEAD &&
git rebase --continue &&
- test_cmp_rev HEAD I
+ test_cmp_rev HEAD D &&
+ tail -n3 todo >>expect &&
+ test_cmp expect actual
'
test_expect_success 'rebase -i commits that overwrite untracked files (squash)' '
diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index 804ff819782..0b0877b9846 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -128,14 +128,24 @@ test_expect_success 'generate correct todo list' '
'
test_expect_success '`reset` refuses to overwrite untracked files' '
- git checkout -b refuse-to-reset &&
+ git checkout B &&
test_commit dont-overwrite-untracked &&
- git checkout @{-1} &&
- : >dont-overwrite-untracked.t &&
- echo "reset refs/tags/dont-overwrite-untracked" >script-from-scratch &&
+ cat >script-from-scratch <<-EOF &&
+ exec >dont-overwrite-untracked.t
+ pick $(git rev-parse B) B
+ reset refs/tags/dont-overwrite-untracked
+ pick $(git rev-parse C) C
+ exec cat .git/rebase-merge/done >actual
+ EOF
test_config sequence.editor \""$PWD"/replace-editor.sh\" &&
- test_must_fail git rebase -ir HEAD &&
- git rebase --abort
+ test_must_fail git rebase -ir A &&
+ test_cmp_rev HEAD B &&
+ head -n3 script-from-scratch >expect &&
+ test_cmp expect .git/rebase-merge/done &&
+ rm dont-overwrite-untracked.t &&
+ git rebase --continue &&
+ tail -n3 script-from-scratch >>expect &&
+ test_cmp expect actual
'
test_expect_success '`reset` rejects trees' '
--
gitgitgadget
^ permalink raw reply related
* [PATCH v4 6/7] rebase --continue: refuse to commit after failed command
From: Phillip Wood via GitGitGadget @ 2023-09-06 15:22 UTC (permalink / raw)
To: git
Cc: Johannes Schindelin, Junio C Hamano, Stefan Haller, Phillip Wood,
Eric Sunshine, Glen Choo, Phillip Wood, Phillip Wood
In-Reply-To: <pull.1492.v4.git.1694013771.gitgitgadget@gmail.com>
From: Phillip Wood <phillip.wood@dunelm.org.uk>
If a commit cannot be picked because it would overwrite an untracked
file then "git rebase --continue" should refuse to commit any staged
changes as the commit was not picked. This is implemented by refusing to
commit if the message file is missing. The message file is chosen for
this check because it is only written when "git rebase" stops for the
user to resolve merge conflicts.
Existing commands that refuse to commit staged changes when continuing
such as a failed "exec" rely on checking for the absence of the author
script in run_git_commit(). This prevents the staged changes from being
committed but prints
error: could not open '.git/rebase-merge/author-script' for
reading
before the message about not being able to commit. This is confusing to
users and so checking for the message file instead improves the user
experience. The existing test for refusing to commit after a failed exec
is updated to check that we do not print the error message about a
missing author script anymore.
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
sequencer.c | 5 +++++
t/t3404-rebase-interactive.sh | 18 +++++++++++++++++-
t/t3430-rebase-merges.sh | 4 ++++
3 files changed, 26 insertions(+), 1 deletion(-)
diff --git a/sequencer.c b/sequencer.c
index 76932ab7b23..38b0f213157 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4980,6 +4980,11 @@ static int commit_staged_changes(struct repository *r,
is_clean = !has_uncommitted_changes(r, 0);
+ if (!is_clean && !file_exists(rebase_path_message())) {
+ const char *gpg_opt = gpg_sign_opt_quoted(opts);
+
+ return error(_(staged_changes_advice), gpg_opt, gpg_opt);
+ }
if (file_exists(rebase_path_amend())) {
struct strbuf rev = STRBUF_INIT;
struct object_id head, to_amend;
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 6d3788c588b..a8ad398956a 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -604,7 +604,8 @@ test_expect_success 'clean error after failed "exec"' '
echo "edited again" > file7 &&
git add file7 &&
test_must_fail git rebase --continue 2>error &&
- test_i18ngrep "you have staged changes in your working tree" error
+ test_i18ngrep "you have staged changes in your working tree" error &&
+ test_i18ngrep ! "could not open.*for reading" error
'
test_expect_success 'rebase a detached HEAD' '
@@ -1290,6 +1291,11 @@ test_expect_success 'rebase -i commits that overwrite untracked files (pick)' '
test_cmp_rev REBASE_HEAD I &&
rm file6 &&
test_path_is_missing .git/rebase-merge/patch &&
+ echo changed >file1 &&
+ git add file1 &&
+ test_must_fail git rebase --continue 2>err &&
+ grep "error: you have staged changes in your working tree" err &&
+ git reset --hard HEAD &&
git rebase --continue &&
test_cmp_rev HEAD I
'
@@ -1310,6 +1316,11 @@ test_expect_success 'rebase -i commits that overwrite untracked files (squash)'
test_cmp_rev REBASE_HEAD I &&
rm file6 &&
test_path_is_missing .git/rebase-merge/patch &&
+ echo changed >file1 &&
+ git add file1 &&
+ test_must_fail git rebase --continue 2>err &&
+ grep "error: you have staged changes in your working tree" err &&
+ git reset --hard HEAD &&
git rebase --continue &&
test $(git cat-file commit HEAD | sed -ne \$p) = I &&
git reset --hard original-branch2
@@ -1330,6 +1341,11 @@ test_expect_success 'rebase -i commits that overwrite untracked files (no ff)' '
test_cmp_rev REBASE_HEAD I &&
rm file6 &&
test_path_is_missing .git/rebase-merge/patch &&
+ echo changed >file1 &&
+ git add file1 &&
+ test_must_fail git rebase --continue 2>err &&
+ grep "error: you have staged changes in your working tree" err &&
+ git reset --hard HEAD &&
git rebase --continue &&
test $(git cat-file commit HEAD | sed -ne \$p) = I
'
diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index 4938ebb1c17..804ff819782 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -169,6 +169,10 @@ test_expect_success 'failed `merge -C` writes patch (may be rescheduled, too)' '
grep "^merge -C .* G$" .git/rebase-merge/done &&
grep "^merge -C .* G$" .git/rebase-merge/git-rebase-todo &&
test_path_is_missing .git/rebase-merge/patch &&
+ echo changed >file1 &&
+ git add file1 &&
+ test_must_fail git rebase --continue 2>err &&
+ grep "error: you have staged changes in your working tree" err &&
: fail because of merge conflict &&
git reset --hard conflicting-G &&
--
gitgitgadget
^ permalink raw reply related
* [PATCH v4 5/7] rebase: fix rewritten list for failed pick
From: Phillip Wood via GitGitGadget @ 2023-09-06 15:22 UTC (permalink / raw)
To: git
Cc: Johannes Schindelin, Junio C Hamano, Stefan Haller, Phillip Wood,
Eric Sunshine, Glen Choo, Phillip Wood, Phillip Wood
In-Reply-To: <pull.1492.v4.git.1694013771.gitgitgadget@gmail.com>
From: Phillip Wood <phillip.wood@dunelm.org.uk>
git rebase keeps a list that maps the OID of each commit before it was
rebased to the OID of the equivalent commit after the rebase. This list
is used to drive the "post-rewrite" hook that is called at the end of a
successful rebase. When a rebase stops for the user to resolve merge
conflicts the OID of the commit being picked is written to
".git/rebase-merge/stopped-sha". Then when the rebase is continued that
OID is added to the list of rewritten commits. Unfortunately if a commit
cannot be picked because it would overwrite an untracked file we still
write the "stopped-sha1" file. This means that when the rebase is
continued the commit is added into the list of rewritten commits even
though it has not been picked yet.
Fix this by not calling error_with_patch() for failed commands. The pick
has failed so there is nothing to commit and therefore we do not want to
set up the state files for committing staged changes when the rebase
continues. This change means we no-longer write a patch for the failed
command or display the error message printed by error_with_patch(). As
the command has failed the patch isn't really useful and in any case the
user can inspect the commit associated with the failed command by
inspecting REBASE_HEAD. Unless the user has disabled it we already print
an advice message that is more helpful than the message from
error_with_patch() which the user will still see. Even if the advice is
disabled the user will see the messages from the merge machinery
detailing the problem.
The code to add a failed command back into the todo list is duplicated
between pick_one_commit() and the loop in pick_commits(). Both sites
print advice about the command being rescheduled, decrement the current
item and save the todo list. To avoid duplicating this code
pick_one_commit() is modified to set a flag to indicate that the command
should be rescheduled in the main loop. This simplifies things as only
the remaining copy of the code needs to be modified to set REBASE_HEAD
rather than calling error_with_patch().
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
sequencer.c | 19 +++++---------
t/t3404-rebase-interactive.sh | 6 +++++
t/t3430-rebase-merges.sh | 4 +--
t/t5407-post-rewrite-hook.sh | 48 +++++++++++++++++++++++++++++++++++
4 files changed, 63 insertions(+), 14 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index b434c5a2570..76932ab7b23 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4158,6 +4158,7 @@ static int do_merge(struct repository *r,
if (ret < 0) {
error(_("could not even attempt to merge '%.*s'"),
merge_arg_len, arg);
+ unlink(git_path_merge_msg(r));
goto leave_merge;
}
/*
@@ -4648,7 +4649,7 @@ N_("Could not execute the todo command\n"
static int pick_one_commit(struct repository *r,
struct todo_list *todo_list,
struct replay_opts *opts,
- int *check_todo)
+ int *check_todo, int* reschedule)
{
int res;
struct todo_item *item = todo_list->items + todo_list->current;
@@ -4661,12 +4662,8 @@ static int pick_one_commit(struct repository *r,
check_todo);
if (is_rebase_i(opts) && res < 0) {
/* Reschedule */
- advise(_(rescheduled_advice),
- get_item_line_length(todo_list, todo_list->current),
- get_item_line(todo_list, todo_list->current));
- todo_list->current--;
- if (save_todo(todo_list, opts))
- return -1;
+ *reschedule = 1;
+ return -1;
}
if (item->command == TODO_EDIT) {
struct commit *commit = item->commit;
@@ -4766,7 +4763,8 @@ static int pick_commits(struct repository *r,
}
}
if (item->command <= TODO_SQUASH) {
- res = pick_one_commit(r, todo_list, opts, &check_todo);
+ res = pick_one_commit(r, todo_list, opts, &check_todo,
+ &reschedule);
if (!res && item->command == TODO_EDIT)
return 0;
} else if (item->command == TODO_EXEC) {
@@ -4820,10 +4818,7 @@ static int pick_commits(struct repository *r,
if (save_todo(todo_list, opts))
return -1;
if (item->commit)
- return error_with_patch(r,
- item->commit,
- arg, item->arg_len,
- opts, res, 0);
+ write_rebase_head(&item->commit->object.oid);
} else if (is_rebase_i(opts) && check_todo && !res &&
reread_todo_if_changed(r, todo_list, opts)) {
return -1;
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index ff0afad63e2..6d3788c588b 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1287,7 +1287,9 @@ test_expect_success 'rebase -i commits that overwrite untracked files (pick)' '
>file6 &&
test_must_fail git rebase --continue &&
test_cmp_rev HEAD F &&
+ test_cmp_rev REBASE_HEAD I &&
rm file6 &&
+ test_path_is_missing .git/rebase-merge/patch &&
git rebase --continue &&
test_cmp_rev HEAD I
'
@@ -1305,7 +1307,9 @@ test_expect_success 'rebase -i commits that overwrite untracked files (squash)'
>file6 &&
test_must_fail git rebase --continue &&
test_cmp_rev HEAD F &&
+ test_cmp_rev REBASE_HEAD I &&
rm file6 &&
+ test_path_is_missing .git/rebase-merge/patch &&
git rebase --continue &&
test $(git cat-file commit HEAD | sed -ne \$p) = I &&
git reset --hard original-branch2
@@ -1323,7 +1327,9 @@ test_expect_success 'rebase -i commits that overwrite untracked files (no ff)' '
>file6 &&
test_must_fail git rebase --continue &&
test $(git cat-file commit HEAD | sed -ne \$p) = F &&
+ test_cmp_rev REBASE_HEAD I &&
rm file6 &&
+ test_path_is_missing .git/rebase-merge/patch &&
git rebase --continue &&
test $(git cat-file commit HEAD | sed -ne \$p) = I
'
diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index 96ae0edf1e1..4938ebb1c17 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -165,12 +165,12 @@ test_expect_success 'failed `merge -C` writes patch (may be rescheduled, too)' '
test_config sequence.editor \""$PWD"/replace-editor.sh\" &&
test_tick &&
test_must_fail git rebase -ir HEAD &&
+ test_cmp_rev REBASE_HEAD H^0 &&
grep "^merge -C .* G$" .git/rebase-merge/done &&
grep "^merge -C .* G$" .git/rebase-merge/git-rebase-todo &&
- test_path_is_file .git/rebase-merge/patch &&
+ test_path_is_missing .git/rebase-merge/patch &&
: fail because of merge conflict &&
- rm G.t .git/rebase-merge/patch &&
git reset --hard conflicting-G &&
test_must_fail git rebase --continue &&
! grep "^merge -C .* G$" .git/rebase-merge/git-rebase-todo &&
diff --git a/t/t5407-post-rewrite-hook.sh b/t/t5407-post-rewrite-hook.sh
index 5f3ff051ca2..ad7f8c6f002 100755
--- a/t/t5407-post-rewrite-hook.sh
+++ b/t/t5407-post-rewrite-hook.sh
@@ -17,6 +17,12 @@ test_expect_success 'setup' '
git checkout A^0 &&
test_commit E bar E &&
test_commit F foo F &&
+ git checkout B &&
+ git merge E &&
+ git tag merge-E &&
+ test_commit G G &&
+ test_commit H H &&
+ test_commit I I &&
git checkout main &&
test_hook --setup post-rewrite <<-EOF
@@ -173,6 +179,48 @@ test_fail_interactive_rebase () {
)
}
+test_expect_success 'git rebase with failed pick' '
+ clear_hook_input &&
+ cat >todo <<-\EOF &&
+ exec >bar
+ merge -C merge-E E
+ exec >G
+ pick G
+ exec >H 2>I
+ pick H
+ fixup I
+ EOF
+
+ (
+ set_replace_editor todo &&
+ test_must_fail git rebase -i D D 2>err
+ ) &&
+ grep "would be overwritten" err &&
+ rm bar &&
+
+ test_must_fail git rebase --continue 2>err &&
+ grep "would be overwritten" err &&
+ rm G &&
+
+ test_must_fail git rebase --continue 2>err &&
+ grep "would be overwritten" err &&
+ rm H &&
+
+ test_must_fail git rebase --continue 2>err &&
+ grep "would be overwritten" err &&
+ rm I &&
+
+ git rebase --continue &&
+ echo rebase >expected.args &&
+ cat >expected.data <<-EOF &&
+ $(git rev-parse merge-E) $(git rev-parse HEAD~2)
+ $(git rev-parse G) $(git rev-parse HEAD~1)
+ $(git rev-parse H) $(git rev-parse HEAD)
+ $(git rev-parse I) $(git rev-parse HEAD)
+ EOF
+ verify_hook_input
+'
+
test_expect_success 'git rebase -i (unchanged)' '
git reset --hard D &&
clear_hook_input &&
--
gitgitgadget
^ permalink raw reply related
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