* Re: [RFC PATCH 1/3] merge-ort.c: finalize ODB transactions after each step
From: Elijah Newren @ 2023-11-11 3:45 UTC (permalink / raw)
To: Taylor Blau
Cc: git, Jeff King, Patrick Steinhardt, Junio C Hamano,
Johannes Schindelin
In-Reply-To: <c615a61d32644b64ef8f47feb47ec909286c56b3.1699381371.git.me@ttaylorr.com>
On Tue, Nov 7, 2023 at 10:22 AM Taylor Blau <me@ttaylorr.com> wrote:
>
> In a previous commit, the ORT merge backend learned how to use the
> bulk-checkin mechanism to emit a single pack containing any new objects
> created during the merge. This functionality was implemented by setting
> up a new ODB transaction, and finalizing it at the end of the merge via
> `process_entries()`.
>
> In a future commit, we will extend this functionality to the new `git
> replay` command, which needs to see objects from previous steps in order
> to replay each commit.
>
> As a step towards implementing this, teach the ORT backend to flush the
> ODB transaction at the end of each step in `process_entries()`, and then
> finalize the result with `end_odb_transaction()` when calling
> `merge_finalize()`.
process_entries() contains a for loop inside it, so "end of each step
in `process_entries()`" sounds like you are flushing after entry, i.e.
several times per commit being replayed.
Perhaps "at the end of `process_entries()` (thus creating one pack per
commit)" ?
(Of course, the fact that we need this change earlier, in the other
series, kinda makes this point moot. Just thought I'd mention it
anyway in case it comes back up in your restructuring.)
^ permalink raw reply
* [PATCH] RelNotes: minor wording fixes in 2.43.0 release notes
From: Elijah Newren via GitGitGadget @ 2023-11-11 4:02 UTC (permalink / raw)
To: git; +Cc: Elijah Newren, Elijah Newren
From: Elijah Newren <newren@gmail.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
RelNotes: minor wording fixes in 2.43.0 release notes
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1611%2Fnewren%2Frelnotes-wording-improvements-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1611/newren/relnotes-wording-improvements-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1611
Documentation/RelNotes/2.43.0.txt | 36 +++++++++++++++----------------
1 file changed, 18 insertions(+), 18 deletions(-)
diff --git a/Documentation/RelNotes/2.43.0.txt b/Documentation/RelNotes/2.43.0.txt
index 74f304b1b2b..770543c464e 100644
--- a/Documentation/RelNotes/2.43.0.txt
+++ b/Documentation/RelNotes/2.43.0.txt
@@ -27,14 +27,14 @@ UI, Workflows & Features
a branch that is checked out and protect it. Rephrase the message
to say that the branch is in use.
- * Hourly and other schedule of "git maintenance" jobs are randomly
+ * Hourly and other schedules of "git maintenance" jobs are randomly
distributed now.
* "git cmd -h" learned to signal which options can be negated by
listing such options like "--[no-]opt".
- * The way authentication related data other than passwords (e.g.
- oath token and password expiration data) are stored in libsecret
+ * The way authentication related data other than passwords (e.g.,
+ oauth token and password expiration data) are stored in libsecret
keyrings has been rethought.
* Update the libsecret and wincred credential helpers to correctly
@@ -60,7 +60,7 @@ UI, Workflows & Features
* The default log message created by "git revert", when reverting a
commit that records a revert, has been tweaked, to encourage people
- describe complex "revert of revert of revert" situation better in
+ to describe complex "revert of revert of revert" situations better in
their own words.
* The command-line completion support (in contrib/) learned to
@@ -77,8 +77,8 @@ UI, Workflows & Features
* The command line completion script (in contrib/) can be told to
complete aliases by including ": git <cmd> ;" in the alias to tell
- it that the alias should be completed similar to how "git <cmd>" is
- completed. The parsing code for the alias as been loosened to
+ it that the alias should be completed in a similar way to how "git <cmd>" is
+ completed. The parsing code for the alias has been loosened to
allow ';' without an extra space before it.
* "git for-each-ref" and friends learned to apply mailmap to
@@ -117,8 +117,8 @@ Performance, Internal Implementation, Development Support etc.
* Flaky "git p4" tests, as well as "git svn" tests, are now skipped
in the (rather expensive) sanitizer CI job.
- * Tests with LSan from time to time seem to emit harmless message
- that makes our tests unnecessarily flaky; we work it around by
+ * Tests with LSan from time to time seem to emit harmless messages
+ that make our tests unnecessarily flaky; we work around it by
filtering the uninteresting output.
* Unused parameters to functions are marked as such, and/or removed,
@@ -135,7 +135,7 @@ Performance, Internal Implementation, Development Support etc.
* Test coverage for trailers has been improved.
- * The code to iterate over loose references have been optimized to
+ * The code to iterate over loose references has been optimized to
reduce the number of lstat() system calls.
* The codepaths that read "chunk" formatted files have been corrected
@@ -157,7 +157,7 @@ Fixes since v2.42
branch tips at the same time will not waste building and testing
the same thing twice.
- * The commit-graph verification code that detects mixture of zero and
+ * The commit-graph verification code that detects a mixture of zero and
non-zero generation numbers has been updated.
* "git diff -w --exit-code" with various options did not work
@@ -170,20 +170,20 @@ Fixes since v2.42
the sequencer code has been cleaned up for consistency.
* "git diff --no-such-option" and other corner cases around the exit
- status of the "diff" command has been corrected.
+ status of the "diff" command have been corrected.
* "git for-each-ref --sort='contents:size'" sorts the refs according
to size numerically, giving a ref that points at a blob twelve-byte
(12) long before showing a blob hundred-byte (100) long.
- * We now limit depth of the tree objects and maximum length of
+ * We now limit the depth of the tree objects and maximum length of
pathnames recorded in tree objects.
(merge 4d5693ba05 jk/tree-name-and-depth-limit later to maint).
* Various fixes to the behavior of "rebase -i" when the command got
interrupted by conflicting changes.
- * References from description of the `--patch` option in various
+ * References from a description of the `--patch` option in various
manual pages have been simplified and improved.
* "git grep -e A --no-or -e B" is accepted, even though the negation
@@ -203,8 +203,8 @@ Fixes since v2.42
information for a file when fsmonitor knows it is clean and ended
up behaving as if it is not clean, which has been corrected.
- * Clarify how "alias.foo = : git cmd ; aliased-command-string" should
- be spelled with necessary whitespaces around punctuation marks to
+ * Clarify how "alias.foo = : git cmd ; aliased-command-string" should be
+ spelled with necessary whitespace around punctuation marks to
work.
* HTTP Header redaction code has been adjusted for a newer version of
@@ -256,9 +256,9 @@ Fixes since v2.42
by "git stash create" now errors out.
(merge d9b6634589 jc/fail-stash-to-store-non-stash later to maint).
- * The index file has room only for lower 32-bit of the file size in
+ * The index file has room only for the lower 32-bit of the file size in
the cached stat information, which means cached stat information
- will have 0 in its sd_size member for a file whose size is multiple
+ will have 0 in its sd_size member for a file whose size is a multiple
of 4GiB. This is mistaken for a racily clean path. Avoid it by
storing a bogus sd_size value instead for such files.
(merge 5143ac07b1 bc/racy-4gb-files later to maint).
@@ -281,7 +281,7 @@ Fixes since v2.42
20 months or so, which has been corrected.
* "git send-email" did not have certain pieces of data computed yet
- when it tried to validate the outging messages and its recipient
+ when it tried to validate the outgoing messages and its recipient
addresses, which has been sorted out.
* "git bugreport" learned to complain when it received a command line
base-commit: dadef801b365989099a9929e995589e455c51fed
--
gitgitgadget
^ permalink raw reply related
* [PATCH 1/1] attr: add native file mode values support
From: Joanna Wang @ 2023-11-11 4:03 UTC (permalink / raw)
To: git; +Cc: Joanna Wang
Gives all paths inherent 'mode' attribute values based on the paths'
modes (one of 100644, 100755, 120000, 040000, 160000). Users may use
this feature to filter by file types. For example a pathspec such as
':(attr:mode=160000)' could filter for submodules without needing
`mode=160000` to actually be specified for each subdmoule path in
.gitattributes. The native mode values are also reflected in
`git check-attr` results.
If there is any existing mode attribute for a path (e.g. there is
!mode, -mode, mode, mode=<value> in .gitattributes) that setting will
take precedence over the native mode value.
---
I went with 'mode' because that is the word used in documentation
(e.g. https://git-scm.com/book/sv/v2/Git-Internals-Git-Objects)
and in the code (e.g. `ce_mode`). Please let me know what you think
of this UX.
The implementation happens within git_check_attr() method which is
the common mathod called for getting a pathspec attribute value.
The previous discussed idea did not work with `git check-attr`.
(https://lore.kernel.org/all/CAMmZTi8swsSMcLUcW+YwUDg8GcrY_ks2+i35-nsHE3o9MNpsUQ@mail.gmail.com/).
There are no tests for excluding based on pathspec attrs in subdirectories
due to an existing bug. Since it is not specific to native mode, I thought
it would be better to fix separately.
https://lore.kernel.org/all/CAMmZTi89Un+bsLXdEdYs44oT8eLNp8y=Pm8ywaurcQ7ccRKGdQ@mail.gmail.com/
Documentation/gitattributes.txt | 10 +++++++
attr.c | 42 ++++++++++++++++++++++++--
t/t0003-attributes.sh | 40 +++++++++++++++++++++++++
t/t6135-pathspec-with-attrs.sh | 53 +++++++++++++++++++++++++++++++++
4 files changed, 143 insertions(+), 2 deletions(-)
diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 8c1793c148..bb3c11f151 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -156,6 +156,16 @@ Unspecified::
Any other value causes Git to act as if `text` has been left
unspecified.
+`mode`
+^^^^^
+
+This attribute is for filtering files by their file bit modes
+(40000, 120000, 160000, 100755, 100644) and has native support in git,
+meaning values for this attribute are natively set (e.g. mode=100644) by
+git without having to specify them in .gitattributes. However, if
+'mode' is set in .gitattributest for some path, that value takes precendence,
+whether it is 'set', 'unset', 'unspecified', or some other value.
+
`eol`
^^^^^
diff --git a/attr.c b/attr.c
index e62876dfd3..95dc1cf695 100644
--- a/attr.c
+++ b/attr.c
@@ -1240,20 +1240,58 @@ static struct object_id *default_attr_source(void)
return &attr_source;
}
+
+/*
+ * This implements native file mode attr support.
+ * We always return the current mode of the path, not the mode stored
+ * in the index, unless the path is a directory where we check the index
+ * to see if it is a GITLINK. It is ok to rely on the index for GITLINK
+ * modes because a path cannot become a GITLINK (which is a git concept only)
+ * without having it indexed with a GITLINK mode in git.
+ */
+static unsigned int native_mode_attr(struct index_state *istate, const char *path)
+{
+ struct stat st;
+ unsigned int mode;
+ if (lstat(path, &st))
+ die_errno(_("unable to stat '%s'"), path);
+ mode = canon_mode(st.st_mode);
+ if (S_ISDIR(mode)) {
+ int pos = index_name_pos(istate, path, strlen(path));
+ if (pos >= 0)
+ if (S_ISGITLINK(istate->cache[pos]->ce_mode))
+ return istate->cache[pos]->ce_mode;
+ }
+ return mode;
+}
+
+
void git_check_attr(struct index_state *istate,
const char *path,
struct attr_check *check)
{
int i;
const struct object_id *tree_oid = default_attr_source();
+ struct strbuf sb = STRBUF_INIT;
collect_some_attrs(istate, tree_oid, path, check);
for (i = 0; i < check->nr; i++) {
unsigned int n = check->items[i].attr->attr_nr;
const char *value = check->all_attrs[n].value;
- if (value == ATTR__UNKNOWN)
- value = ATTR__UNSET;
+ if (value == ATTR__UNKNOWN){
+ if (strcmp(check->all_attrs[n].attr->name, "mode") == 0) {
+ /*
+ * If value is ATTR_UNKNOWN then it has not been set
+ * anywhere with -mode (ATTR_FALSE), !mode (ATTR_UNSET),
+ * mode (ATTR_TRUE), or an explicit value. We fill
+ * value with the native mode value.
+ */
+ strbuf_addf(&sb, "%06o", native_mode_attr(istate, path));
+ value = sb.buf;
+ } else
+ value = ATTR__UNSET;
+ }
check->items[i].value = value;
}
}
diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index aee2298f01..9c2603d8e2 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -19,6 +19,15 @@ attr_check () {
test_must_be_empty err
}
+attr_check_mode () {
+ path="$1" expect="$2" git_opts="$3" &&
+
+ git $git_opts check-attr mode -- "$path" >actual 2>err &&
+ echo "$path: mode: $expect" >expect &&
+ test_cmp expect actual &&
+ test_must_be_empty err
+}
+
attr_check_quote () {
path="$1" quoted_path="$2" expect="$3" &&
@@ -558,4 +567,35 @@ test_expect_success EXPENSIVE 'large attributes file ignored in index' '
test_cmp expect err
'
+test_expect_success 'submodule with .git file' '
+ mkdir sub &&
+ (cd sub &&
+ git init &&
+ mv .git .real &&
+ echo "gitdir: .real" >.git &&
+ test_commit first) &&
+ git update-index --add -- sub
+'
+
+test_expect_success 'native mode attributes work' '
+ >exec && chmod +x exec && attr_check_mode exec 100755 &&
+ >normal && attr_check_mode normal 100644 &&
+ mkdir dir && attr_check_mode dir 040000 &&
+ ln -s normal normal_sym && attr_check_mode normal_sym 120000 &&
+ attr_check_mode sub 160000
+'
+
+test_expect_success '.gitattributes mode values take precedence' '
+ (
+ echo "mode_value* mode=myownmode" &&
+ echo "mode_set* mode" &&
+ echo "mode_unset* -mode" &&
+ echo "mode_unspecified* !mode"
+ ) >.gitattributes &&
+ >mode_value && attr_check_mode mode_value myownmode &&
+ >mode_unset && attr_check_mode mode_unset unset &&
+ >mode_unspecified && attr_check_mode mode_unspecified unspecified &&
+ >mode_set && attr_check_mode mode_set set
+'
+
test_done
diff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh
index a9c1e4e0ec..fd9569d39b 100755
--- a/t/t6135-pathspec-with-attrs.sh
+++ b/t/t6135-pathspec-with-attrs.sh
@@ -64,6 +64,9 @@ test_expect_success 'setup .gitattributes' '
fileSetLabel label
fileValue label=foo
fileWrongLabel label☺
+ mode_set* mode=1234
+ mode_unset* -mode
+ mode_unspecified* !mode
EOF
echo fileSetLabel label1 >sub/.gitattributes &&
git add .gitattributes sub/.gitattributes &&
@@ -295,4 +298,54 @@ test_expect_success 'reading from .gitattributes in a subdirectory (3)' '
test_cmp expect actual
'
+test_expect_success 'mode attr is handled correctly for overriden values' '
+ >mode_set_1 &&
+ >mode_unset_1 &&
+ >mode_unspecified_1 &&
+ >mode_regular_file_1 &&
+
+ git status -s ":(attr:mode=1234)mode*" >actual &&
+ cat <<-\EOF >expect &&
+ ?? mode_set_1
+ EOF
+ test_cmp expect actual &&
+
+ git status -s ":(attr:-mode)mode*" >actual &&
+ echo ?? mode_unset_1 >expect &&
+ test_cmp expect actual &&
+
+ git status -s ":(attr:!mode)mode*" >actual &&
+ echo ?? mode_unspecified_1 >expect &&
+ test_cmp expect actual &&
+
+ git status -s ":(attr:mode=100644)mode*" >actual &&
+ echo ?? mode_regular_file_1 >expect &&
+ test_cmp expect actual
+'
+
+test_expect_success 'mode attr values are current file modes, not indexed modes' '
+ >mode_exec_file_1 &&
+
+ git status -s ":(attr:mode=100644)mode_exec_*" >actual &&
+ echo ?? mode_exec_file_1 >expect &&
+ test_cmp expect actual &&
+
+ git add mode_exec_file_1 && chmod +x mode_exec_file_1 &&
+ git status -s ":(attr:mode=100755)mode_exec_*" >actual &&
+ echo AM mode_exec_file_1 >expect &&
+ test_cmp expect actual
+'
+
+test_expect_success 'mode attr can be excluded' '
+ >mode_1_regular &&
+ >mode_1_exec && chmod +x mode_1_exec &&
+ git status -s ":(exclude,attr:mode=100644)" "mode_1_*" >actual &&
+ echo ?? mode_1_exec >expect &&
+ test_cmp expect actual &&
+
+ git status -s ":(exclude,attr:mode=100755)" "mode_1_*" >actual &&
+ echo ?? mode_1_regular >expect &&
+ test_cmp expect actual
+'
+
test_done
--
2.42.0.869.gea05f2083d-goog
^ permalink raw reply related
* Re: [RFC PATCH 0/3] replay: implement support for writing new objects to a pack
From: Elijah Newren @ 2023-11-11 4:04 UTC (permalink / raw)
To: Taylor Blau
Cc: git, Jeff King, Patrick Steinhardt, Junio C Hamano,
Johannes Schindelin
In-Reply-To: <cover.1699381371.git.me@ttaylorr.com>
On Tue, Nov 7, 2023 at 10:22 AM Taylor Blau <me@ttaylorr.com> wrote:
>
> (Based on a combination of Christian's cc/git-replay and my
> tb/merge-tree-write-pack branches).
>
> This RFC demonstrates extending the new `--write-pack` option that
> `merge-tree` recently learned to the `replay` builtin as well.
>
> The approach is as follows:
>
> - write a pack out after each step in the replay operation, so that
> subsequent steps may see any new object(s) created during previous
> steps
>
> - combine those packs into one before migrating them back into the
> main object store
>
> This is accomplished with a combination of the bulk-checkin and
> tmp-objdir APIs, with some minor modifications made to when we flush out
> and finalize bulk-checkin transactions.
Just thought I'd note that I finished reading all of this series as
well as the five-commit series it's based on. Just wanted to note
that any commits I didn't comment on from either series looked good to
me.
^ permalink raw reply
* Re: [PATCH 1/1] attr: add native file mode values support
From: Junio C Hamano @ 2023-11-11 4:48 UTC (permalink / raw)
To: Joanna Wang; +Cc: git
In-Reply-To: <20231111040309.2848560-1-jojwang@google.com>
Joanna Wang <jojwang@google.com> writes:
> +/*
> + * This implements native file mode attr support.
> + * We always return the current mode of the path, not the mode stored
> + * in the index, unless the path is a directory where we check the index
> + * to see if it is a GITLINK. It is ok to rely on the index for GITLINK
> + * modes because a path cannot become a GITLINK (which is a git concept only)
> + * without having it indexed with a GITLINK mode in git.
> + */
> +static unsigned int native_mode_attr(struct index_state *istate, const char *path)
> +{
> + struct stat st;
> + unsigned int mode;
Please have a blank line here between decls and the first statement.
> + if (lstat(path, &st))
> + die_errno(_("unable to stat '%s'"), path);
For checking in, this is probably OK, but don't we need to switch
between lstat() on the filesystem entity and inspecting ce_mode()
for the in-index cache_entry, depending on the git_attr_direction?
> + mode = canon_mode(st.st_mode);
> + if (S_ISDIR(mode)) {
> + int pos = index_name_pos(istate, path, strlen(path));
> + if (pos >= 0)
> + if (S_ISGITLINK(istate->cache[pos]->ce_mode))
> + return istate->cache[pos]->ce_mode;
Even if we assume that this code is currently meant to work only
with GIT_ATTR_CHECKIN, I do not think this is what you want. When
asked to perform "git add . ':(exclude,mode=160000)'", you not only
want to exclude the submodules that are already known to this
superproject, but also a repository that _can_ become a submodule of
this superproject when added, no? You are missing "if (pos < 0)"
case where you'd probably want to see if the directory at the path
looks like the top of a working tree with ".git" subdirectory that
is a valid repository, or something like that to detect such a case.
On the other hand, the GIT_ATTR_CHECKOUT direction is hopefully much
simpler. You'd see what the path in the index is, among a gitlink,
a regular non-executable file, an executable file, or a symlink.
> + }
> + return mode;
> +}
> +
> +
> void git_check_attr(struct index_state *istate,
> const char *path,
> struct attr_check *check)
> {
> int i;
> const struct object_id *tree_oid = default_attr_source();
> + struct strbuf sb = STRBUF_INIT;
>
> collect_some_attrs(istate, tree_oid, path, check);
>
> for (i = 0; i < check->nr; i++) {
> unsigned int n = check->items[i].attr->attr_nr;
> const char *value = check->all_attrs[n].value;
> - if (value == ATTR__UNKNOWN)
> - value = ATTR__UNSET;
> + if (value == ATTR__UNKNOWN){
Style. Missing SP between "){".
> + if (strcmp(check->all_attrs[n].attr->name, "mode") == 0) {
Style. We avoid comparison with 0; so say
if (!strcmp(check->all_attrs[n].attr->name, "mode") == 0) {
instead.
It is disturbing that we always need to perform a string comparison
in this loop (and the function is called repeatedly while traversing
the tree looking for paths that match pathspec). The attribute objects
are designed to be interned, so at least you should be able to do
mode_attr = git_attr("mode");
before the loop and then compare check->all_attrs[n].attr and
mode_attr as two addresses.
> + /*
> + * If value is ATTR_UNKNOWN then it has not been set
> + * anywhere with -mode (ATTR_FALSE), !mode (ATTR_UNSET),
> + * mode (ATTR_TRUE), or an explicit value. We fill
> + * value with the native mode value.
> + */
> + strbuf_addf(&sb, "%06o", native_mode_attr(istate, path));
> + value = sb.buf;
Or even better yet, as this is not a place to write too many lines
for a single oddball attribute, it might make even more sense to
introduce a helper function and make a call to it like so:
if (value == ATTR__UNKNOWN)
- value = ATTR__UNSET;
+ value = compute_attr_from_path(istate, path, check_items[i].attr);
with the new helper function do all the heavy lifting, e.g.,
static const char *compute_attr_from_path(struct index_state *istate,
const char *path,
const struct git_attr *attr) {
static struct git_attr mode_attr;
if (!mode_attr)
mode_attr = git_attr("mode");
if (attr == mode_attr) {
call native_mode_attr(), stringify, and
return it;
}
return ATTR__UNSET;
}
which will allow us to easily add in the future special attribute
similar to "mode" whose fallback value is computed.
Otherwise, looking pretty good. Thanks for working on this.
^ permalink raw reply
* Re: Git Rename Detection Bug
From: Elijah Newren @ 2023-11-11 5:46 UTC (permalink / raw)
To: Jeremy Pridmore; +Cc: git@vger.kernel.org, Paul Baumgartner
In-Reply-To: <LO6P265MB6736F5F9E8368A9DE95D294FFAA9A@LO6P265MB6736.GBRP265.PROD.OUTLOOK.COM>
Hi Jeremy,
On Fri, Nov 10, 2023 at 3:28 AM Jeremy Pridmore <jpridmore@rdt.co.uk> wrote:
>
> Hi Elijah,
>
> Many thanks for your reply, the detail is much appreciated. I was aware, from recently read articles, that git doesn't record renames as such, hence my investigations into the rename detection, but I also found some interesting points in your email, such as the "git status --no-renames" flag.
The fact that you were trying to "undo" renames and "redo the correct
ones" suggested there's something you still didn't understand about
rename detection, though. The fact that you were worried that "git
status" showing the "wrong renames" and the implication that it needed
to show the right ones before you committed also suggested there's
something that was still not being understood. Likewise, the fact
that the renames reported by "git status" change even when you haven't
renamed files further but have simply made additional changes to the
contents of some files suggests there is something that was still not
being understood about the phrase that "renames are detected rather
than recorded".
While renames are used in the merge algorithm in order to know what
files to match up for three-way content merges (so that changes from
both sides to the "same" file can be incorporated into the end
result), once the merge stops to ask you to resolve conflicts, the
detection of renames doesn't matter beyond that point. The fact that
renames aren't recorded means whatever renames are shown is only there
as a guide to help you understand. All that matters to Git is that
all files have the intended content. If some of the files have the
wrong content, then by all means go and correct it. If "git mv"
commands help you do that, great. If simply editing all files of
interest (including adding and deleting files) until they match the
expected contents works, that's fine too. Once all files have the
correct content, commit it. Git will have no way whatsoever of
knowing which of those two routes you picked, and won't behave any
differently in the future based on which way you ended up with the
right contents in each file. You could have even done the extreme of
"git merge -s ours --no-commit ${OTHER_BRANCH}" (merge the other
branch but completely ignore *every* change the other side made and
then stop for user to make further changes), followed by opening and
editing every relevant file to include changes made by the other side,
deleting and adding files as relevant, to end up with the right
contents in all files and then committed, and Git wouldn't know the
difference and no one who pulled your merge commit would be able to
tell the difference either.
> I think the issue I'm encountering is described by what you say here:
> "Exact renames are detected first, before any other method of rename
> detection is employed, and other than giving a preference to files
> with the same basename, if there are multiple choices it just picks
> one (what I'd call at random, though technically based on what the
> internal processing order happens to be)"
>
> That is close to the behaviour I'm seeing. As I mentioned, git seems to think a file has been deleted and then as it continues to detect renames, it's as if it is going through lists of "Local-Base" and "Base-Remote" changes trying to match them up, but the directories of the files being matched are "offset", as highlighted by this list of mismatches:
>
> (I'd put the paths in a table for easier analysis, but for some reason the emails need to be plain text)
> > Incorrect path match: Landscape/Deployment/PowershellScripts/pre-req/Rdt.BatchProcessingService.Setup/pre-req.ps1 -> Landscape/src/Deployment/PowershellScripts/pre-req/Landscape.Net/pre-req.ps1
> > Incorrect path match: Landscape/Deployment/PowershellScripts/pre-req/Workflow/pre-req.ps1 -> Landscape/src/Deployment/PowershellScripts/pre-req/Rdt.BatchProcessingService.Setup/pre-req.ps1
> > Incorrect name match: Landscape/Documentation/Rdt.Documentation.UI/Properties/licenses.licx -> Landscape/src/Deployment/PowershellScripts/pre-req/Workflow/pre-req.ps1
> > Incorrect path match: Landscape/Documentation/uiDocumentation/licenses.licx -> Landscape/src/Documentation/Rdt.Documentation.UI/Properties/licenses.licx
> > Incorrect path match: Landscape/Import/uiImport/My Project/licenses.licx -> Landscape/src/Documentation/uiDocumentation/licenses.licx
> > Incorrect path match: Landscape/Main/uiMain.Workflow/My Project/licenses.licx -> Landscape/src/Import/uiImport/My Project/licenses.licx
> > Incorrect path match: Landscape/Main/uiMain/My Project/licenses.licx -> Landscape/src/Main/uiMain.Workflow/My Project/licenses.licx
>
> Given git compares both the content and the directory\filenames,
For exact renames, it only will look at filenames if there are
multiple 100% matches. If one match is 100%, and the other 99.9999%
match, the 100% match is taken.
(Since we think that exact renames seem to be describing your problem,
there's no point discussing the inexact rename handling.)
> and as the repositories have unrelated histories,
What do you mean by this? If there's no common point in history (i.e.
no merge base), rename detection doesn't even get invoked, so you must
mean something different by this phrase than what I would normally
take it to mean.
Any chance, if you're still in the middle of the merge, that you could run
git merge-base --all HEAD MERGE_HEAD
and report the output? It'll be commit hashes that don't mean a lot
to anyone who doesn't have your repository, but I'm interested in how
many commit hashes it responds with (0, 1, or more than 1).
> the "Base" file is going to be empty, therefore, even if Local and Remote are identical, they are both 100% different to Base.
I assume by "Base" you are referring to the commit in history common
to both branches being merged (i.e. what we call the "merge base"), or
to the relevant file from that commit. Is that right?
Even if I'm right about that, this comment has me lost. Could you
clarify it, with one particular example? For example (I'm making
stuff up since I'm not familiar with your repo, but showing how to
clarify for a given set of path(s)):
* In this repository, with the renames detected above,
Landscape/Main/somefile.licx is an empty file in the merge base.
* On my local side, I renamed Landscape/Main/somefile.licx to
Landscape/src/Main/somefile.licx and populated it with some content
(with hash A). There is also another new file on the local side (at
least new relative to the merge base) named
Landscape/src/Other/somefile.licx that happens to have hash A.
* On the remote side, Landscape/Main/somefile.licx was left in
place but populated with some content (with hash A).
* Git is detecting the rename as Landscape/Main/somefile.licx ->
Landscape/src/Other/somefile.licx, when I wanted it to detect a rename
to Landscape/src/Main/somefile.licx.
I'm pretty sure this example is not what you're seeing, even if
components of it are, because the empty file thing is impossible with
the rest of the story.
> That given, I'm not sure why git would state that Landscape/Documentation/Rdt.Documentation.UI/Properties/licenses.licx and Landscape/src/Deployment/PowershellScripts/pre-req/Workflow/pre-req.ps1 are a "both added" conflict given their file names and paths are completely different. Any ideas?
The fact that Landscape/Documentation/Rdt.Documentation.UI/Properties/licenses.licx
is marked as "both added" means that this file
* did not exist in the merge base
* did exist on your local side
* did exist on the remote side
* the version of this file on the local and remote sides do not match
Basically, both sides added a new file and they are not the same, so
you have a conflict.
The answer for Landscape/src/Deployment/PowershellScripts/pre-req/Workflow/pre-req.ps1,
is an identical set of bullet points; it was a file added by both
sides of history that did not exist in the merge base, and the sides
are different so you have a conflict in this file too.
> I wrote a script to resolve the conflicts best I can which categorises the files into sets according to the file status (i.e. "added by them", "added by us" etc), and then either does a "git checkout head --
<file>" or a "git rm <file>" based upon which set the file is in and
whether it is in another set or not. This has worked really well and
helped me through the large changeset with 3k conflicts.
So, you're simply throwing away the changes made by the remote side?
I mean, that's one way to merge, and it might be right in your case,
but to someone unfamiliar with your repo it smells like a hack to just
ignore conflicts and throw away other people's changes in order to
complete the merge.
> As git only needs to try and match files in the "deleted by us" and "deleted by them" sets (although including the "deleted in both" set would allow matching renames/moves on both sides),
"deleted by us" and "deleted by them" means no rename was detected for
the file (at least on the side that the delete is reported for). So,
a "deleted in both" only happens when neither side detects a rename,
and if the file isn't renamed on either side and both removed it, then
there's no conflict -- just delete the file.
> an idea for a potential improvement to the matching algorithm (where you say there's a comment "too many alternatives, pick one") could be to compute a "difference value" for the path\filename of those files in one of the other sets (i.e. "added by us", "added by them" or "added in both"), and chose a potential rename based upon the smallest calculated difference. The difference value would be the number of differences in folder names, e.g.
>
> deleted in both: Landscape/Deployment/PowershellScripts/pre-req/Rdt.BatchProcessingService.Setup/pre-req.ps1
>
> added in both: Landscape/src/Deployment/PowershellScripts/pre-req/Landscape.Net/pre-req.ps1
> (path\name difference = 2)
> added in both: Landscape/src/Deployment/PowershellScripts/pre-req/Rdt.BatchProcessingService.Setup/pre-req.ps1
> (path\name difference = 1)
> added in both: Landscape/src/Deployment/PowershellScripts/pre-req/Workflow/pre-req.ps1
> (path\name difference = 2)
>
> So, given the above, git would chose the second "added in both" entry.
Three problems here:
* If git were to report "deleted in both" for one path and "added in
both" in another as you suggest, that would only be because the files
are dissimilar. Not only would they not be an exact match, their
contents would have less than 50% similarity. Thus augmenting exact
matching like this just wouldn't work, because the files aren't
matches at all. The only correct thing to do would be to not report
the "deleted in both" because that file is not a rename of anything
else and has simply been deleted by both sides.
* filename similarity is extraordinarily expensive compared to exact
renames, and if not carefully handled, can sometimes rival the cost of
file content similarity computations given our spanhash
representations. Exact renames are tasked with finding renames even
if they are known to not be relevant, simply because exact renames can
do so very quickly. If we change that, we throw a monkey wrench in
our performance handling elsewhere and have to rethink a number of
other things.
* While I was optimizing rename detection while investigating the new
merge backend, I actually attempted a few versions of filename
similarity looking for something that was predictive and useful.
While I think the idea was potentially helpful for some repositories,
it has a significant risk of hurting merges in other repositories.
While what I tried was far from an exhaustive checking of all filename
similarity ideas, I came away doubting there was a useful heuristic
other than exact matches of basenames (i.e. exact matches of
everything in the filename after the final slash). If someone else
wants to try more ideas, and do a study on various existing
repositories, they can go ahead, but I suspect most work here is going
to end up at a dead end and I'm unwilling to put further time of my
own into it.
> Food for thought? Happy to discuss the idea further.
So, I've occasionally seen repositories that have something like the following:
* base version: directory named library-x-1.7/
* stable branch: many changes to files under library-x-1.7/
* development branch: library-x-1.7/ no longer exists. However,
library-x-1.8/ and library-x-1.9/ both do. Both are obviously
"similar" to library-x-1.7/ but both have many changes.
What happens when someone tries to merge the stable branch into the
development branch? There are two obvious guesses:
* Changes from library-x-1.7/ on the stable branch are applied to
library-x-1.8/
* Changes from library-x-1.7/ on the stable branch are applied to
library-x-1.9/
Either answer can be suboptimal depending on your viewpoint.
(Applying the changes to both directories would also have other
suboptimal effects even if it might sound right based for this exact
problem as I've worded it. But git doesn't do copy detection as part
of merges so Git won't choose this third choice ever.) So, which of
those two happens? Well, since renames are detected based upon file
similarity, the changes will go to whatever file is most similar.
What does that mean? It means that both answers above are wrong.
Instead:
* Some of the changes from library-x-1.7/ on the stable branch are
applied to files from library-x-1.8/, while others are applied to
files from library-x-1.9/, and to determine which files from which
directory are matched up is an individual file choice based on which
file in library-x-1.8/ or library-x-1.9/ is most similar to the file
from the base version in library-x-1.7/.
This answer is clearly worse than either of the two above, and is
virtually never what people would want. But it's also fundamental to
the idea of matching up files and detecting renames individually based
upon file similarity. It's part of both the old and new merge
backends in Git, because both were based upon this fundamental idea.
So, if you have this kind of situation, or even something like it
where files from one old directory could match files from multiple
other directories, it's just something you have to be aware of.
All that said, here's something that might help:
*** Hack to workaround rename detection in special cases where there
are directories of multiple possible matches ***
1. Get back to a clean slate from before the merge.
$ git merge --abort
OR
$ cd ${OTHER_DIRECTORY} && git clone ${url} && cd ${REPONAME} && git
checkout ${relevant_branch}
2. Temporarily undo your local renames and make a temporary commit
$ git mv Landscape/src/* Landscape/
$ git commit -m "TEMPORARY COMMIT"
3. Perform the merge (files will be in Landscape/ instead of
Landscape/src/ for now). Don't worry, we'll fix the merge commit
later.
$ git merge ${REMOTE_BRANCH}
[...fix up any conflicts and commit, if needed...]
4. Rename Landscape/ back to Landscape/src/ and make another (temporary) commit.
5. Create a corrected merge commit with the current tree, the commit
message from your merge commit, and the correct parents:
$ git commit-tree -p HEAD~3 -p HEAD~1^2 -F $(git log -1 --format=%B
HEAD~1) HEAD^{tree}
[...the above command will print out a new commit id for a corrected
merge commit. You can inspect it first, but we just need to pass this
to reset --hard...]
6. Reset your branch to this corrected merge commit (which will orphan
the temporary commits from steps 2, 3, and 4 so they can later be
garbage collected)
$ git reset --hard [...output of commit-tree command...]
Hope that helps,
Elijah
^ permalink raw reply
* Re: first-class conflicts?
From: Sandra Snan @ 2023-11-11 7:48 UTC (permalink / raw)
To: git
In-Reply-To: <xmqqh6ltrs6t.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Correct but with a caveat: it is too easy for lazy folks to
> circumvent the safety by mistake with "commit -a".
Lazy and ignorant like myself because I didn't know -a was that
dangerous. Thank you both!
^ permalink raw reply
* Re: [RFC PATCH] status: avoid reporting worktrees as "Untracked files"
From: Edmundo Carmona Antoranz @ 2023-11-11 9:22 UTC (permalink / raw)
To: Junio C Hamano, Eric Sunshine; +Cc: git
In-Reply-To: <xmqqjzqygg3i.fsf@gitster.g>
Hey, guys! Thanks Junio and Eric for sharing your thoughts.
And I candidly thought this was going to be an "easy sell".... :-D
On Sat, Nov 4, 2023 at 7:58 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> What problem are you trying to solve? "git add foo" where "foo" is
> actually a different worktree of the repository would add it as a
> submodule that causes confusion? If that is the case, I think the
> right solution is not to get into such a state, i.e. not create a
> worktree of the repository inside a different worktree in the first
> place.
>
I am not against the idea of creating worktrees outside of the
repository... however, I like them to be _inside_ the repository. Am I
the only one? IDK. I might be! It feels completely natural, if you ask
me.... but that's just my opinion, I acknowledge that.
While I was running a couple of quick tests to add more information
about git behaviour with "my use case" I think I found something to
work on so more RFCs might be on the way in the next few days or
weeks.
About adding an error message when 'git add' will skip doing something
because it is working on a different worktree, I think it makes
sense.... will probably work on that too.
Thanks again! BR!
^ permalink raw reply
* [PATCH v4 0/4] rebase: support --autosquash without -i
From: Andy Koppe @ 2023-11-11 13:27 UTC (permalink / raw)
To: git; +Cc: gitster, newren, Andy Koppe
In-Reply-To: <20231105000808.10171-1-andy.koppe@gmail.com>
Make rebase --autosquash work without --interactive, but limit
rebase.autoSquash's effects to interactive mode, and improve testing
and documentation.
Changes from v3:
- Separate commits for rebase.autoSquash, --autosquash and testing.
- Go back to interpreting rebase.autoSquash as a boolean, but document
it as only affecting interactive mode.
- In the --autosquash documentation, bring back partial matching as a
fallback and show what todo list command each squash marker
corresponds to.
Thanks very much to Junio and Phillip for their reviews. I made plenty
of use of non-interactive autosquash in preparing this version. :)
Andy Koppe (4):
rebase: fully ignore rebase.autoSquash without -i
rebase: support --autosquash without -i
rebase: test autosquash with and without -i
rebase: rewrite --(no-)autosquash documentation
Documentation/config/rebase.txt | 4 ++-
Documentation/git-rebase.txt | 34 +++++++++++++----------
builtin/rebase.c | 17 +++++-------
t/t3415-rebase-autosquash.sh | 38 +++++++++++++++++++-------
t/t3422-rebase-incompatible-options.sh | 12 --------
5 files changed, 58 insertions(+), 47 deletions(-)
--
2.43.0-rc1
^ permalink raw reply
* [PATCH v4 2/4] rebase: support --autosquash without -i
From: Andy Koppe @ 2023-11-11 13:27 UTC (permalink / raw)
To: git; +Cc: gitster, newren, Andy Koppe
In-Reply-To: <20231111132720.78877-1-andy.koppe@gmail.com>
The --autosquash option prevents preemptive fast-forwarding and triggers
conflicts with amend backend options, yet it only actually performs
auto-squashing when combined with the --interactive (or -i) option.
Remove the latter restriction and tweak the --autosquash description
accordingly.
Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
---
Documentation/git-rebase.txt | 2 +-
builtin/rebase.c | 4 +---
2 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index b4526ca246..10548e715c 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -592,7 +592,7 @@ See also INCOMPATIBLE OPTIONS below.
When the commit log message begins with "squash! ..." or "fixup! ..."
or "amend! ...", and there is already a commit in the todo list that
matches the same `...`, automatically modify the todo list of
- `rebase -i`, so that the commit marked for squashing comes right after
+ `rebase`, so that the commit marked for squashing comes right after
the commit to be modified, and change the action of the moved commit
from `pick` to `squash` or `fixup` or `fixup -C` respectively. A commit
matches the `...` if the commit subject matches, or if the `...` refers
diff --git a/builtin/rebase.c b/builtin/rebase.c
index a73de7892b..9f8192e0a5 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -710,10 +710,8 @@ static int run_specific_rebase(struct rebase_options *opts)
if (opts->type == REBASE_MERGE) {
/* Run sequencer-based rebase */
setenv("GIT_CHERRY_PICK_HELP", resolvemsg, 1);
- if (!(opts->flags & REBASE_INTERACTIVE_EXPLICIT)) {
+ if (!(opts->flags & REBASE_INTERACTIVE_EXPLICIT))
setenv("GIT_SEQUENCE_EDITOR", ":", 1);
- opts->autosquash = 0;
- }
if (opts->gpg_sign_opt) {
/* remove the leading "-S" */
char *tmp = xstrdup(opts->gpg_sign_opt + 2);
--
2.43.0-rc1
^ permalink raw reply related
* [PATCH v4 1/4] rebase: fully ignore rebase.autoSquash without -i
From: Andy Koppe @ 2023-11-11 13:27 UTC (permalink / raw)
To: git; +Cc: gitster, newren, Andy Koppe
In-Reply-To: <20231111132720.78877-1-andy.koppe@gmail.com>
Setting the rebase.autoSquash config variable to true implies a couple
of restrictions: it prevents preemptive fast-forwarding and it triggers
conflicts with amend backend options. However, it only actually results
in auto-squashing when combined with the --interactive (or -i) option,
due to code in run_specific_rebase() that disables auto-squashing unless
the REBASE_INTERACTIVE_EXPLICIT flag is set.
Doing autosquashing for rebase.autoSquash without --interactive would be
problematic in terms of backward compatibility, but conversely, there is
no need for the aforementioned restrictions without --interactive.
So drop the options.config_autosquash check from the conditions for
clearing allow_preemptive_ff, as the case where it is combined with
--interactive is already covered by the REBASE_INTERACTIVE_EXPLICIT
flag check above it.
Also drop the "apply options are incompatible with rebase.autoSquash"
error, because it is unreachable if it is restricted to --interactive,
as apply options already cause an error when used with --interactive.
Drop the tests for the error from t3422-rebase-incompatible-options.sh,
which has separate tests for the conflicts of --interactive with apply
options.
When neither --autosquash nor --no-autosquash are given, only set
options.autosquash to true if rebase.autosquash is combined with
--interactive.
Don't initialize options.config_autosquash to -1, as there is no need to
distinguish between rebase.autoSquash being unset or explicitly set to
false.
Finally, amend the rebase.autoSquash documentation to say it only
affects interactive mode.
Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
---
Documentation/config/rebase.txt | 4 +++-
builtin/rebase.c | 13 ++++++-------
t/t3422-rebase-incompatible-options.sh | 12 ------------
3 files changed, 9 insertions(+), 20 deletions(-)
diff --git a/Documentation/config/rebase.txt b/Documentation/config/rebase.txt
index 9c248accec..d59576dbb2 100644
--- a/Documentation/config/rebase.txt
+++ b/Documentation/config/rebase.txt
@@ -9,7 +9,9 @@ rebase.stat::
rebase. False by default.
rebase.autoSquash::
- If set to true enable `--autosquash` option by default.
+ If set to true, enable the `--autosquash` option of
+ linkgit:git-rebase[1] by default for interactive mode.
+ This can be overridden with the `--no-autosquash` option.
rebase.autoStash::
When set to true, automatically create a temporary stash entry
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 043c65dccd..a73de7892b 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -149,7 +149,6 @@ struct rebase_options {
.reapply_cherry_picks = -1, \
.allow_empty_message = 1, \
.autosquash = -1, \
- .config_autosquash = -1, \
.rebase_merges = -1, \
.config_rebase_merges = -1, \
.update_refs = -1, \
@@ -1405,7 +1404,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
if ((options.flags & REBASE_INTERACTIVE_EXPLICIT) ||
(options.action != ACTION_NONE) ||
(options.exec.nr > 0) ||
- (options.autosquash == -1 && options.config_autosquash == 1) ||
options.autosquash == 1) {
allow_preemptive_ff = 0;
}
@@ -1508,8 +1506,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
if (is_merge(&options))
die(_("apply options and merge options "
"cannot be used together"));
- else if (options.autosquash == -1 && options.config_autosquash == 1)
- die(_("apply options are incompatible with rebase.autoSquash. Consider adding --no-autosquash"));
else if (options.rebase_merges == -1 && options.config_rebase_merges == 1)
die(_("apply options are incompatible with rebase.rebaseMerges. Consider adding --no-rebase-merges"));
else if (options.update_refs == -1 && options.config_update_refs == 1)
@@ -1529,10 +1525,13 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
options.rebase_merges = (options.rebase_merges >= 0) ? options.rebase_merges :
((options.config_rebase_merges >= 0) ? options.config_rebase_merges : 0);
- if (options.autosquash == 1)
+ if (options.autosquash == 1) {
imply_merge(&options, "--autosquash");
- options.autosquash = (options.autosquash >= 0) ? options.autosquash :
- ((options.config_autosquash >= 0) ? options.config_autosquash : 0);
+ } else if (options.autosquash == -1) {
+ options.autosquash =
+ options.config_autosquash &&
+ (options.flags & REBASE_INTERACTIVE_EXPLICIT);
+ }
if (options.type == REBASE_UNSPECIFIED) {
if (!strcmp(options.default_backend, "merge"))
diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
index 2eba00bdf5..b40f26250b 100755
--- a/t/t3422-rebase-incompatible-options.sh
+++ b/t/t3422-rebase-incompatible-options.sh
@@ -100,12 +100,6 @@ test_rebase_am_only () {
test_must_fail git rebase $opt --root A
"
- test_expect_success "$opt incompatible with rebase.autosquash" "
- git checkout B^0 &&
- test_must_fail git -c rebase.autosquash=true rebase $opt A 2>err &&
- grep -e --no-autosquash err
- "
-
test_expect_success "$opt incompatible with rebase.rebaseMerges" "
git checkout B^0 &&
test_must_fail git -c rebase.rebaseMerges=true rebase $opt A 2>err &&
@@ -118,12 +112,6 @@ test_rebase_am_only () {
grep -e --no-update-refs err
"
- test_expect_success "$opt okay with overridden rebase.autosquash" "
- test_when_finished \"git reset --hard B^0\" &&
- git checkout B^0 &&
- git -c rebase.autosquash=true rebase --no-autosquash $opt A
- "
-
test_expect_success "$opt okay with overridden rebase.rebaseMerges" "
test_when_finished \"git reset --hard B^0\" &&
git checkout B^0 &&
--
2.43.0-rc1
^ permalink raw reply related
* [PATCH v4 4/4] docs: rewrite rebase --(no-)autosquash description
From: Andy Koppe @ 2023-11-11 13:27 UTC (permalink / raw)
To: git; +Cc: gitster, newren, Andy Koppe
In-Reply-To: <20231111132720.78877-1-andy.koppe@gmail.com>
Rewrite the description of the rebase --(no-)autosquash options to try
to make it a bit clearer. Don't use "the '...'" to refer to part of a
commit message, mention how --interactive can be used to review the
todo list, and add a bit more detail on commit --squash/amend.
Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
---
Documentation/git-rebase.txt | 34 ++++++++++++++++++++--------------
1 file changed, 20 insertions(+), 14 deletions(-)
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 10548e715c..1dd6555f66 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -589,21 +589,27 @@ See also INCOMPATIBLE OPTIONS below.
--autosquash::
--no-autosquash::
- When the commit log message begins with "squash! ..." or "fixup! ..."
- or "amend! ...", and there is already a commit in the todo list that
- matches the same `...`, automatically modify the todo list of
- `rebase`, so that the commit marked for squashing comes right after
- the commit to be modified, and change the action of the moved commit
- from `pick` to `squash` or `fixup` or `fixup -C` respectively. A commit
- matches the `...` if the commit subject matches, or if the `...` refers
- to the commit's hash. As a fall-back, partial matches of the commit
- subject work, too. The recommended way to create fixup/amend/squash
- commits is by using the `--fixup`, `--fixup=amend:` or `--fixup=reword:`
- and `--squash` options respectively of linkgit:git-commit[1].
+ Automatically squash commits with specially formatted messages into
+ previous commits being rebased. If a commit message starts with
+ "squash! ", "fixup! " or "amend! ", the remainder of the subject line
+ is taken as a commit specifier, which matches a previous commit if it
+ matches the subject line or the hash of that commit. If no commit
+ matches fully, matches of the specifier with the start of commit
+ subjects are considered.
+
-If the `--autosquash` option is enabled by default using the
-configuration variable `rebase.autoSquash`, this option can be
-used to override and disable this setting.
+In the rebase todo list, the actions of squash, fixup and amend commits are
+changed from `pick` to `squash`, `fixup` or `fixup -C`, respectively, and they
+are moved right after the commit they modify. The `--interactive` option can
+be used to review and edit the todo list before proceeding.
++
+The recommended way to create commits with squash markers is by using the
+`--squash`, `--fixup`, `--fixup=amend:` or `--fixup=reword:` options of
+linkgit:git-commit[1], which take the target commit as an argument and
+automatically fill in the subject line of the new commit from that.
++
+Settting configuration variable `rebase.autoSquash` to true enables
+auto-squashing by default for interactive rebase. The `--no-autosquash`
+option can be used to override that setting.
+
See also INCOMPATIBLE OPTIONS below.
--
2.43.0-rc1
^ permalink raw reply related
* [PATCH v4 3/4] rebase: test autosquash with and without -i
From: Andy Koppe @ 2023-11-11 13:27 UTC (permalink / raw)
To: git; +Cc: gitster, newren, Andy Koppe
In-Reply-To: <20231111132720.78877-1-andy.koppe@gmail.com>
Amend t3415-rebase-autosquash.sh to test the --autosquash option and
rebase.autoSquash config with and without -i.
Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
---
t/t3415-rebase-autosquash.sh | 38 ++++++++++++++++++++++++++----------
1 file changed, 28 insertions(+), 10 deletions(-)
diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
index a364530d76..fcc40d6fe1 100755
--- a/t/t3415-rebase-autosquash.sh
+++ b/t/t3415-rebase-autosquash.sh
@@ -43,7 +43,7 @@ test_auto_fixup () {
git tag $1 &&
test_tick &&
- git rebase $2 -i HEAD^^^ &&
+ git rebase $2 HEAD^^^ &&
git log --oneline >actual &&
if test -n "$no_squash"
then
@@ -61,15 +61,24 @@ test_auto_fixup () {
}
test_expect_success 'auto fixup (option)' '
- test_auto_fixup final-fixup-option --autosquash
+ test_auto_fixup fixup-option --autosquash &&
+ test_auto_fixup fixup-option-i "--autosquash -i"
'
-test_expect_success 'auto fixup (config)' '
+test_expect_success 'auto fixup (config true)' '
git config rebase.autosquash true &&
- test_auto_fixup final-fixup-config-true &&
+ test_auto_fixup ! fixup-config-true &&
+ test_auto_fixup fixup-config-true-i -i &&
test_auto_fixup ! fixup-config-true-no --no-autosquash &&
+ test_auto_fixup ! fixup-config-true-i-no "-i --no-autosquash"
+'
+
+test_expect_success 'auto fixup (config false)' '
git config rebase.autosquash false &&
- test_auto_fixup ! final-fixup-config-false
+ test_auto_fixup ! fixup-config-false &&
+ test_auto_fixup ! fixup-config-false-i -i &&
+ test_auto_fixup fixup-config-false-yes --autosquash &&
+ test_auto_fixup fixup-config-false-i-yes "-i --autosquash"
'
test_auto_squash () {
@@ -87,7 +96,7 @@ test_auto_squash () {
git commit -m "squash! first" -m "extra para for first" &&
git tag $1 &&
test_tick &&
- git rebase $2 -i HEAD^^^ &&
+ git rebase $2 HEAD^^^ &&
git log --oneline >actual &&
if test -n "$no_squash"
then
@@ -105,15 +114,24 @@ test_auto_squash () {
}
test_expect_success 'auto squash (option)' '
- test_auto_squash final-squash --autosquash
+ test_auto_squash squash-option --autosquash &&
+ test_auto_squash squash-option-i "--autosquash -i"
'
-test_expect_success 'auto squash (config)' '
+test_expect_success 'auto squash (config true)' '
git config rebase.autosquash true &&
- test_auto_squash final-squash-config-true &&
+ test_auto_squash ! squash-config-true &&
+ test_auto_squash squash-config-true-i -i &&
test_auto_squash ! squash-config-true-no --no-autosquash &&
+ test_auto_squash ! squash-config-true-i-no "-i --no-autosquash"
+'
+
+test_expect_success 'auto squash (config false)' '
git config rebase.autosquash false &&
- test_auto_squash ! final-squash-config-false
+ test_auto_squash ! squash-config-false &&
+ test_auto_squash ! squash-config-false-i -i &&
+ test_auto_squash squash-config-false-yes --autosquash &&
+ test_auto_squash squash-config-false-i-yes "-i --autosquash"
'
test_expect_success 'misspelled auto squash' '
--
2.43.0-rc1
^ permalink raw reply related
* [PATCH v4 4/4] rebase: rewrite --(no-)autosquash documentation
From: Andy Koppe @ 2023-11-11 13:27 UTC (permalink / raw)
To: git; +Cc: gitster, newren, Andy Koppe
In-Reply-To: <20231111132720.78877-1-andy.koppe@gmail.com>
Rewrite the description of the rebase --(no-)autosquash options to try
to make it a bit clearer. Don't use "the '...'" to refer to part of a
commit message, mention how --interactive can be used to review the
todo list, and add a bit more detail on commit --squash/amend.
Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
---
Documentation/git-rebase.txt | 34 ++++++++++++++++++++--------------
1 file changed, 20 insertions(+), 14 deletions(-)
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 10548e715c..1dd6555f66 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -589,21 +589,27 @@ See also INCOMPATIBLE OPTIONS below.
--autosquash::
--no-autosquash::
- When the commit log message begins with "squash! ..." or "fixup! ..."
- or "amend! ...", and there is already a commit in the todo list that
- matches the same `...`, automatically modify the todo list of
- `rebase`, so that the commit marked for squashing comes right after
- the commit to be modified, and change the action of the moved commit
- from `pick` to `squash` or `fixup` or `fixup -C` respectively. A commit
- matches the `...` if the commit subject matches, or if the `...` refers
- to the commit's hash. As a fall-back, partial matches of the commit
- subject work, too. The recommended way to create fixup/amend/squash
- commits is by using the `--fixup`, `--fixup=amend:` or `--fixup=reword:`
- and `--squash` options respectively of linkgit:git-commit[1].
+ Automatically squash commits with specially formatted messages into
+ previous commits being rebased. If a commit message starts with
+ "squash! ", "fixup! " or "amend! ", the remainder of the subject line
+ is taken as a commit specifier, which matches a previous commit if it
+ matches the subject line or the hash of that commit. If no commit
+ matches fully, matches of the specifier with the start of commit
+ subjects are considered.
+
-If the `--autosquash` option is enabled by default using the
-configuration variable `rebase.autoSquash`, this option can be
-used to override and disable this setting.
+In the rebase todo list, the actions of squash, fixup and amend commits are
+changed from `pick` to `squash`, `fixup` or `fixup -C`, respectively, and they
+are moved right after the commit they modify. The `--interactive` option can
+be used to review and edit the todo list before proceeding.
++
+The recommended way to create commits with squash markers is by using the
+`--squash`, `--fixup`, `--fixup=amend:` or `--fixup=reword:` options of
+linkgit:git-commit[1], which take the target commit as an argument and
+automatically fill in the subject line of the new commit from that.
++
+Settting configuration variable `rebase.autoSquash` to true enables
+auto-squashing by default for interactive rebase. The `--no-autosquash`
+option can be used to override that setting.
+
See also INCOMPATIBLE OPTIONS below.
--
2.43.0-rc1
^ permalink raw reply related
* Re: [PATCH v4 4/4] docs: rewrite rebase --(no-)autosquash description
From: Andy Koppe @ 2023-11-11 13:33 UTC (permalink / raw)
To: git; +Cc: gitster, newren
In-Reply-To: <20231111132720.78877-5-andy.koppe@gmail.com>
Disregard this one in favour of the other patch 4/4 please:
"[PATCH v4 4/4] rebase: rewrite --(no-)autosquash documentation"
Sorry I failed to notice that I still had this lying around from the
previous iteration.
On 11/11/2023 13:27, Andy Koppe wrote:
> Rewrite the description of the rebase --(no-)autosquash options to try
> to make it a bit clearer. Don't use "the '...'" to refer to part of a
> commit message, mention how --interactive can be used to review the
> todo list, and add a bit more detail on commit --squash/amend.
>
> Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
> ---
> Documentation/git-rebase.txt | 34 ++++++++++++++++++++--------------
> 1 file changed, 20 insertions(+), 14 deletions(-)
>
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index 10548e715c..1dd6555f66 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -589,21 +589,27 @@ See also INCOMPATIBLE OPTIONS below.
>
> --autosquash::
> --no-autosquash::
> - When the commit log message begins with "squash! ..." or "fixup! ..."
> - or "amend! ...", and there is already a commit in the todo list that
> - matches the same `...`, automatically modify the todo list of
> - `rebase`, so that the commit marked for squashing comes right after
> - the commit to be modified, and change the action of the moved commit
> - from `pick` to `squash` or `fixup` or `fixup -C` respectively. A commit
> - matches the `...` if the commit subject matches, or if the `...` refers
> - to the commit's hash. As a fall-back, partial matches of the commit
> - subject work, too. The recommended way to create fixup/amend/squash
> - commits is by using the `--fixup`, `--fixup=amend:` or `--fixup=reword:`
> - and `--squash` options respectively of linkgit:git-commit[1].
> + Automatically squash commits with specially formatted messages into
> + previous commits being rebased. If a commit message starts with
> + "squash! ", "fixup! " or "amend! ", the remainder of the subject line
> + is taken as a commit specifier, which matches a previous commit if it
> + matches the subject line or the hash of that commit. If no commit
> + matches fully, matches of the specifier with the start of commit
> + subjects are considered.
> +
> -If the `--autosquash` option is enabled by default using the
> -configuration variable `rebase.autoSquash`, this option can be
> -used to override and disable this setting.
> +In the rebase todo list, the actions of squash, fixup and amend commits are
> +changed from `pick` to `squash`, `fixup` or `fixup -C`, respectively, and they
> +are moved right after the commit they modify. The `--interactive` option can
> +be used to review and edit the todo list before proceeding.
> ++
> +The recommended way to create commits with squash markers is by using the
> +`--squash`, `--fixup`, `--fixup=amend:` or `--fixup=reword:` options of
> +linkgit:git-commit[1], which take the target commit as an argument and
> +automatically fill in the subject line of the new commit from that.
> ++
> +Settting configuration variable `rebase.autoSquash` to true enables
> +auto-squashing by default for interactive rebase. The `--no-autosquash`
> +option can be used to override that setting.
> +
> See also INCOMPATIBLE OPTIONS below.
>
^ permalink raw reply
* Re: [PATCH v3 1/2] rebase: support non-interactive autosquash
From: Andy Koppe @ 2023-11-11 14:08 UTC (permalink / raw)
To: phillip.wood, git; +Cc: gitster, newren
In-Reply-To: <8c2bb219-127c-4128-99ed-158bc64b1dab@gmail.com>
On 06/11/2023 11:06, Phillip Wood wrote:
> On 05/11/2023 00:08, Andy Koppe wrote:
>> So far, the rebase --autosquash option and rebase.autoSquash=true
>> config setting are quietly ignored when used without --interactive,
>
> Thanks for working on this, I agree that "--autosquash" being ignored
> without "--interactive" is something that we should address. I think
> there are several possible solutions
>
> 1 - make "--autosquash" imply "--interactive". This has the advantage
> that the user gets to check the commits are going to be reordered as
> they expect when they edit the todo list. It is hard to see how to
> accommodate the config setting though - I don't think we want
> "rebase.autosquash=true" to imply "--interactive".
>
> 2 - make "--autosquash" without "--interactive" an error. This would
> prevent the user being surprised that their commits are not squashed
> by a non-interactive rebase. Users who have set
> "rebase.autosquash=true" would have to pass "--no-autosquash" to
> perform any form of non-interactive rebase. This is similar to the
> current behavior where the user has to pass "--no-autosquash" if
> they want to use the apply backend with "rebase.autosquash=true".
>
> 3 - make "--autosquash" rearrange and squash commits without
> "--interactive". This is convenient but there is a risk in that the
> user does not get a chance to check the todo list before the commits
> are reordered and squashed. I think that risk is fairly small with
> an explicit "--autosquash" on the commandline. This is the approach
> taken by this patch. I do have some concerns about extending the
> config setting to non-interactive rebases though. If the user has
> commits that look like
>
> fixup! foo (HEAD)
> foo bar
> foo
>
> and runs "git -c rebase.autosquash=non-interactive rebase HEAD~2"
> then we'll silently squash the fixup into the wrong commit due to a
> prefix subject match.
Good analysis. My order of preference is 3 (obviously), 1, 2.
>> except that they prevent fast-forward and that they trigger conflicts
>> with --apply and relatives, which is less than helpful particularly for
>> the config setting.
>
> The behavior to make the config setting incompatible with the apply
> backend was implemented to avoid users being surprised that their
> commits are not squashed by that backend even when they have set
> "rebase.autosquash=true"[1]. I think one could consider "--autosquash"
> being silently ignored without "--interactive" to be an oversight in
> 796abac7e1 (rebase: add coverage of other incompatible options,
> 2023-01-25) that introduced that change.
>
> [1]
> https://lore.kernel.org/git/pull.1466.v5.git.1674619434.gitgitgadget@gmail.com/
>
>> Since the "merge" backend used for interactive rebase also is the
>> default for non-interactive rebase, there doesn't appear to be a
>> reason not to do --autosquash without --interactive, so support that.
>
> I think making "--autosquash" on the commandline work for
> non-interactive rebases is reasonable but I would be open to the
> argument that it would be better to make it an error and require
> "--interactive" to allow the user to check that the commits are going to
> be reordered as they expect.
I found that once I got used to and started trusting the feature,
particularly in connection with the corresponding git-commit support, I
no longer felt the need to check the todo list as I'd inspect the log
afterwards anyway. And of course there's always resetting to ORIG_HEAD
when things do go wrong.
So I think users should be trusted with this, especially as it's not a
particularly dangerous feature, given it requires the squash markers to
be present in the first place to do anything.
>> Turn rebase.autoSquash into a comma-separated list of flags, with
>> "interactive" or "i" enabling auto-squashing with --interactive, and
>> "no-interactive" or "no-i" enabling it without. Make boolean true mean
>> "interactive" for backward compatibility.
>
> Please, please, please don't introduce abbreviated config settings, it
> just makes the interface more complicated. The user only has to set this
> once so I think the short names just add confusion.
Duly noted.
> I also think
> "non-interactive" would be a better name for the config setting
> corresponding to non-interactive rebases. Does this mean the user can
> request that commits are only rearranged when they do not pass
> "--interactive"?
Yes. That doesn't seem useful.
> As I said above I do have some concerns that the
> "rebase.autosquash=non-interactive" setting will catch people out.
I think you're right, so I've gone back to interpreting it as a boolean,
but officially make it affect interactive mode only.
> Having said that ignoring "rebase.autosquash=true" without
> "--interactive" as we do now is inconsistent with the behavior of
> "rebase.autosquash=true" with "--apply". One possibility would be to
> introduce "rebase.autosquash=interactive" which would not cause an error
> with "--apply" and always require an explicit "--autosquash" on the
> commandline to squash fixups without "--interactive"
I don't think different error behaviour is worth a separate setting, as
we can't change rebase.autosquash=true to do auto-squashing without
--interactive without surprising people.
>> Don't prevent fast-forwards or report conflicts with --apply options
>> when auto-squashing is not active.
>
> I think this change deserves to be in a separate commit (which probably
> means separating out the config changes into that commit) as it is not
> directly related to fixing "--autosquash" without "--interactive" on the
> commandline.
Done in v4.
> In summary I like "--autosquash" working without "--interactive" but I'm
> unsure about the config changes.
Thanks very much for the thoughtful review!
Regards,
Andy
^ permalink raw reply
* Re: [PATCH v2 1/2] rebase: support non-interactive autosquash
From: Andy Koppe @ 2023-11-11 14:26 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, newren
In-Reply-To: <xmqqcywng0wu.fsf@gitster.g>
On 06/11/2023 00:50, Junio C Hamano wrote:
> Andy Koppe <andy.koppe@gmail.com> writes:
> Our log
> message convention is to first describe what happens in the system
> in the present tense to illustrate why it is suboptimal, to prepare
> readers' minds to anticipate the solution, which is described next.
>
> When asking reviews on a new iteration [PATCH v(N+1)], please
> summarize the differences relative to [PATCH vN]. For explaining
> such incremental changes for individual patches, here between the
> three-dash line and the diffstat is the place to do so. When you
> have a cover letter [PATCH 0/X], it can be done in that messaage.
> Either way is OK. Doing both is also helpful as long as the
> explanation done in two places do not contradict with each other.
> If you tried to format the documentation before sending this patch,
> you'd have seen the second paragraph formatted as if it were a code
> snippet. Dedent the second paragraph (and later ones if you had
> more than one extra paragraphs), and turn the blank line between the
> paragraphs into a line with "+" (and nothing else) on it. See the
> description of `--autosquash` option in Documentation/git-rebase.txt
> for an example.
Sad thing is that I knew most of that from reading the contribution
guidelines and previous experience, but obviously I don't always
remember. So thanks for you patience in re-explaining that.
> OK, by clearing opts->config_autosquash in this function, you keep
> the rebase.autosquash to be "the last one wins" as a whole. If a
> configuration file with lower precedence (e.g., /etc/gitconfig) says
> "[rebase] autosquash" to set it to "interactive,no-interactive", a
> separate setting in your ~/.gitconfig "[rebase] autosquash = false"
> would override both bits.
>
> A more involved design may let the users override these bits
> independently by allowing something like "!no-i" (take whatever the
> lower precedence configuration file says for the interactive case,
> but disable autosquash when running a non-interactive rebase) as the
> value, but I think the approach taken by this patch to allow replacing
> as a whole is OK. It is simpler to explain.
>
> Giving short-hands for often used command line options is one thing,
> but I do not think a short-hand is warranted here, especially when
> the other one needs to be a less-than-half legible "no-i" that does
> not allow "no-int" and friends, for configuration variable values.
> I'd strongly suggest dropping them.
Dropped in v4, along with the attempt to expand rebase.autoSquash,
following Phillip's review.
Regards,
Andy
^ permalink raw reply
* Re: Git Rename Detection Bug
From: Elijah Newren @ 2023-11-11 15:13 UTC (permalink / raw)
To: Philip Oakley; +Cc: Jeremy Pridmore, git@vger.kernel.org, Paul Baumgartner
In-Reply-To: <9baca4af-a570-4b7a-a1ee-de91b809e79c@iee.email>
Hi,
On Sat, Nov 11, 2023 at 3:08 AM Philip Oakley <philipoakley@iee.email> wrote:
>
> Hi all,
>
> On 11/11/2023 05:46, Elijah Newren wrote:
> > The fact that you were trying to "undo" renames and "redo the correct
> > ones" suggested there's something you still didn't understand about
> > rename detection, though.
>
>
> Could I suggest that we are missing a piece of terminology, to wit,
> BLOBSAME. It's a compatriot to TREESAME, as used in `git log` for
> history simplification (based on a tree's pathspec, most commonly a
> commit's top level path).
We could add it, but I'm not sure how it helps. We already had 'exact
rename' which seems to fit the bill as well, and 'blob' is something
someone new to Git is unlikely to know.
Perhaps it's useful in some other context, though?
> File rename, at it's most basic, is when the blob associated with that
> changed path is identical, i.e. BLOBSAME. There is no need to 'record'
> the action of renaming, moving or whatever, the content sameness is
> right there, in plain sight, as an identical blob name. After that
> (files with slight variations) it is a load of heuristics, but starting
> with BLOBSAME we see how easy the basic rename detection is, and why
> renames (and de-dup) don't need recording.
This is incorrect. Let's say you have a file foo:
* base version: foo has hash A
* our version: foo has been renamed to bar, but bar still has hash A
* their version: foo has been modified; it now has hash B
The foo->bar is an exact rename (or they are BLOBSAME if you prefer),
but the renaming/moving/whatever is a critical piece of information
because the changes to foo in 'their' version need to be applied to
bar to get the correct end results.
I do not know if in Jeremy's case foo has been modified on the
unrenamed side. But the following hypothetical is exactly the type of
problem Jeremy is hitting: what should happen when 'our' version has
both a new 'bar' and a new 'baz' file that each have hash A? In that
case, to which one was foo renamed? It's inherently ambiguous.
> The heuristics of 'rename with small change' is trickier, but for a
> basic understanding, starting at BLOBSAME (and TREESAME for directory
> renames) should make it easier to grasp the concepts.
Interesting; TREESAME isn't used within directory rename detection
currently; it is only used currently when two (or three) trees with
the same name are TREESAME, in order to potentially avoid recursing
into the tree. But even then, having two trees with the same name be
TREESAME isn't enough on its own to avoid recursing into that tree,
because the other side could have added files within the same-named
tree and we need to know about those added files because they could be
part of renames involving other files outside that tree. There would
probably be similar challenges to attempting to apply the concept of
TREESAME to directory rename detection to two trees of different
names, but it's at least an interesting idea. Hmm....
^ permalink raw reply
* rev-list default order sometimes very slow (size and metadata dependent)
From: Kevin Lyles @ 2023-11-11 16:17 UTC (permalink / raw)
To: git@vger.kernel.org; +Cc: allred.sean@gmail.com
[-- Attachment #1.1: Type: text/plain, Size: 5028 bytes --]
Hello,
As part of my company's migration from SVN to git, we discovered a
performance issue in rev-list with large repositories. The issue appears to
be metadata-dependent; we were able to work around it (completely avoiding
any performance penalty) by changing the date of certain commits.
The general structure of our repository is I think fairly normal (if large
-- we have >5.5 million commits total). We have a handful of trunk
branches, and ~10k total refs. To reduce the ref count (we hit other
performance issues when we had significantly more refs), we remove refs as
we're done with them. Any code that doesn't make it into a trunk is
preserved in an archive branch. The archive branch has no content, and
consists entirely of octopus merges with 50-500 parents.
If the archive branch is created with author/commit dates older than the
rest of the repository, we're able to run:
$ git rev-list --count --all
in ~9-10 seconds on a mirror clone with a commit-graph. However, if the
archive branch is instead created with author/commit dates newer than the
rest of the repository, it takes 4-5 minutes.
Using any order other than the default or --reverse removes the disparity.
All orders except --author-date-order bring things much closer to the ~9-10
seconds we see with the workaround, and --author-date-order is still under a
minute (though not by much).
System info from git bugreport:
[System Info]
git version:
git version 2.42.0.windows.2
cpu: x86_64
built from commit: 2f819d1670fff9a1818f63b6722e9959405378e3
sizeof-long: 4
sizeof-size_t: 8
shell-path: /bin/sh
feature: fsmonitor--daemon
uname: Windows 10.0 19044
compiler info: gnuc: 13.2
libc info: no libc information available
$SHELL (typically, interactive shell): C:\Program Files\Git\usr\bin\bash.exe
(no enabled hooks)
Note that we first realized this was an issue on our GitLab instance, which
runs on Linux, so this is not a Windows-specific bug.
I created a bash script to create very similar repositories that are/are not
affected by the issue; it follows. The issue starts to become visible at 1
million commits (the default), where the difference is ~2x. 5 million
commits is roughly equivalent performance-wise to what we saw in our
repository, with a difference of ~33x. Note that with 5 million commits,
each repository is ~1.2 GB and takes 7-8 minutes to create on an i9-9900
with NVMe storage.
Once you create a fast and a slow repo with the script, try the following
commands in each one:
# Shows the performance difference
$ time git rev-list --count –all
# Shows very similar performance across both repos
$ time git rev-list --count --all --topo-order
Thank you,
Kevin Lyles
klyles@epic.com <mailto:klyles@epic.com>
----------------------------------------
#!/bin/bash
usage="Usage: $0 <destination folder> <--fast|--slow> [Number of commits (default: 1000000)]"
destinationFolder=${1:?$usage}
oldTimestamp=315554400 # 1980-01-01 midnight
newTimestamp=1672552800 # 2023-01-01 midnight
if [ "$2" == "--fast" ]
then
archiveTimestamp=$oldTimestamp
elif [ "$2" == "--slow" ]
then
archiveTimestamp=$newTimestamp
else
echo "$usage" >&2
exit 1
fi
numberOfCommits=${3:-1000000}
if ! [[ "$numberOfCommits" =~ ^[0-9]+$ ]]
then
echo "$usage" >&2
exit 1
fi
increment=$(( (newTimestamp - oldTimestamp) / (numberOfCommits + 2) ))
timestamp=$oldTimestamp
rm -rf "$destinationFolder"
git init "$destinationFolder"
echo "Fast-importing repo, please wait..."
{
echo "feature done"
echo "reset refs/heads/main"
echo ""
for count in $(seq "$numberOfCommits")
do
timestamp=$(( timestamp + increment ))
echo "commit refs/heads/main"
echo "mark :$count"
echo "committer Test Test <test@test.com <mailto:test@test.com> > $timestamp -0500"
echo "data <<|"
echo "Main branch commit #$count"
echo "|"
echo ""
done
parentMark=0
echo "reset refs/archive"
for count in $(seq $(( numberOfCommits / 1000 )))
do
echo "commit refs/archive"
echo "committer Test Test <test@test.com <mailto:test@test.com> > $archiveTimestamp -0500"
echo "data <<|"
echo "Archive branch commit #$count"
echo "|"
for parentCount in {1..50}
do
parentMark=$(( (parentMark + 99991) % numberOfCommits + 1 ))
echo "merge :$parentMark"
done
echo ""
done
echo "done"
} | git -C "$destinationFolder" fast-import
git -C "$destinationFolder" commit-graph write
[-- Attachment #1.2: Type: text/html, Size: 12590 bytes --]
[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 6034 bytes --]
^ permalink raw reply
* Re: Git Rename Detection Bug
From: Philip Oakley @ 2023-11-11 11:08 UTC (permalink / raw)
To: Elijah Newren, Jeremy Pridmore; +Cc: git@vger.kernel.org, Paul Baumgartner
In-Reply-To: <CABPp-BHEX+SyophEfgRqDbNdrAS3=bptt_cKzHLBSutnBAxexw@mail.gmail.com>
Hi all,
On 11/11/2023 05:46, Elijah Newren wrote:
> The fact that you were trying to "undo" renames and "redo the correct
> ones" suggested there's something you still didn't understand about
> rename detection, though.
Could I suggest that we are missing a piece of terminology, to wit,
BLOBSAME. It's a compatriot to TREESAME, as used in `git log` for
history simplification (based on a tree's pathspec, most commonly a
commit's top level path).
File rename, at it's most basic, is when the blob associated with that
changed path is identical, i.e. BLOBSAME. There is no need to 'record'
the action of renaming, moving or whatever, the content sameness is
right there, in plain sight, as an identical blob name. After that
(files with slight variations) it is a load of heuristics, but starting
with BLOBSAME we see how easy the basic rename detection is, and why
renames (and de-dup) don't need recording.
The heuristics of 'rename with small change' is trickier, but for a
basic understanding, starting at BLOBSAME (and TREESAME for directory
renames) should make it easier to grasp the concepts.
--
Philip
^ permalink raw reply
* [PATCH] fuzz: add new oss-fuzz fuzzer for date.c / date.h
From: Arthur Chan via GitGitGadget @ 2023-11-11 17:39 UTC (permalink / raw)
To: git; +Cc: Arthur Chan, Arthur Chan
From: Arthur Chan <arthur.chan@adalogics.com>
Signed-off-by: Arthur Chan <arthur.chan@adalogics.com>
---
fuzz: add new oss-fuzz fuzzer for date.c / date.h
This patch is aimed to add a new oss-fuzz fuzzer to the oss-fuzz
directory for fuzzing date.c / date.h in the base directory.
The .gitignore of the oss-fuzz directory and the Makefile have been
modified to accommodate the new fuzzer fuzz-date.c.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1612%2Farthurscchan%2Fnew-fuzzer-date-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1612/arthurscchan/new-fuzzer-date-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1612
Makefile | 1 +
oss-fuzz/.gitignore | 1 +
oss-fuzz/fuzz-date.c | 75 ++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 77 insertions(+)
create mode 100644 oss-fuzz/fuzz-date.c
diff --git a/Makefile b/Makefile
index 03adcb5a480..c9fe99a8c88 100644
--- a/Makefile
+++ b/Makefile
@@ -752,6 +752,7 @@ ETAGS_TARGET = TAGS
FUZZ_OBJS += oss-fuzz/fuzz-commit-graph.o
FUZZ_OBJS += oss-fuzz/fuzz-pack-headers.o
FUZZ_OBJS += oss-fuzz/fuzz-pack-idx.o
+FUZZ_OBJS += oss-fuzz/fuzz-date.o
.PHONY: fuzz-objs
fuzz-objs: $(FUZZ_OBJS)
diff --git a/oss-fuzz/.gitignore b/oss-fuzz/.gitignore
index 9acb74412ef..2375e77b108 100644
--- a/oss-fuzz/.gitignore
+++ b/oss-fuzz/.gitignore
@@ -1,3 +1,4 @@
fuzz-commit-graph
fuzz-pack-headers
fuzz-pack-idx
+fuzz-date
diff --git a/oss-fuzz/fuzz-date.c b/oss-fuzz/fuzz-date.c
new file mode 100644
index 00000000000..29bcaf595e4
--- /dev/null
+++ b/oss-fuzz/fuzz-date.c
@@ -0,0 +1,75 @@
+#include "git-compat-util.h"
+#include "date.h"
+
+int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size);
+
+int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)
+{
+ int type;
+ int time;
+ int num;
+ char *str;
+ timestamp_t ts;
+ enum date_mode_type dmtype;
+ struct date_mode *dm;
+
+ if (size <= 8)
+ {
+ return 0;
+ }
+
+ type = (*((int *)data)) % 8;
+ data += 4;
+ size -= 4;
+
+ time = abs(*((int *)data));
+ data += 4;
+ size -= 4;
+
+ str = (char *)malloc(size+1);
+ if (!str)
+ {
+ return 0;
+ }
+ memcpy(str, data, size);
+ str[size] = '\0';
+
+ ts = approxidate_careful(str, &num);
+ free(str);
+
+ switch(type)
+ {
+ case 0: default:
+ dmtype = DATE_NORMAL;
+ break;
+ case 1:
+ dmtype = DATE_HUMAN;
+ break;
+ case 2:
+ dmtype = DATE_SHORT;
+ break;
+ case 3:
+ dmtype = DATE_ISO8601;
+ break;
+ case 4:
+ dmtype = DATE_ISO8601_STRICT;
+ break;
+ case 5:
+ dmtype = DATE_RFC2822;
+ break;
+ case 6:
+ dmtype = DATE_RAW;
+ break;
+ case 7:
+ dmtype = DATE_UNIX;
+ break;
+ }
+
+ dm = date_mode_from_type(dmtype);
+ dm->local = 1;
+ show_date(ts, time, dm);
+
+ date_mode_release(dm);
+
+ return 0;
+}
base-commit: dadef801b365989099a9929e995589e455c51fed
--
gitgitgadget
^ permalink raw reply related
* [PATCH] checkout: add config variable checkout.autoDetach
From: Andy Koppe @ 2023-11-11 22:42 UTC (permalink / raw)
To: git; +Cc: pclouds, Andy Koppe
The git-checkout command without pathspecs automatically detaches HEAD
when switching to something other than a branch, whereas git-switch
requires the --detach option to do so.
Add configuration variable checkout.autoDetach to choose the behavior
for both: true for automatic detaching, false for requiring --detach.
Amend their documentation and tests accordingly.
Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
---
I like to use git-switch and git-restore instead of git-checkout, and
also recommend them when new users ask how to switch branches or undo
local changes. But I do miss git-checkout's auto-detaching, as I like
to avoid creating local versions of remote branches when I don't intend
to make changes to them.
Hence this patch, which turned out to be very simple in terms of the
actual source change, as the checkout_opts.implicit_detach field
already controls the necessary functionality. That was added in commit
7968bef06b by Nguyễn Thái Ngọc Duy, with the following explanation:
> "git checkout <commit>" will checkout the commit in question and
> detach HEAD from the current branch. It is naturally a right thing to
> do once you get git references. But detached HEAD is a scary concept
> to new users because we show a lot of warnings and stuff, and it could
> be hard to get out of (until you know better).
>
> To keep switch a bit more friendly to new users, we only allow
> entering detached HEAD mode when --detach is given.
I think that makes plenty of sense, and I don't think it conflicts with
the config setting proposed here, as new users are unlikely to mess with
such settings. In fact, the point where "you get git references" would
probably be a good time to enable checkout.autoDetach.
Conversely, admins might want to set checkout.autoDetach to false in the
system-wide config to prevent accidental decapitation by git-checkout
and the resulting support requests.
Documentation/config/checkout.txt | 8 ++++++++
Documentation/git-checkout.txt | 3 +++
Documentation/git-switch.txt | 3 +++
builtin/checkout.c | 4 ++++
t/t2020-checkout-detach.sh | 14 ++++++++++++++
t/t2060-switch.sh | 7 +++++++
6 files changed, 39 insertions(+)
diff --git a/Documentation/config/checkout.txt b/Documentation/config/checkout.txt
index a323022993..6827ee74d5 100644
--- a/Documentation/config/checkout.txt
+++ b/Documentation/config/checkout.txt
@@ -17,6 +17,14 @@ and by linkgit:git-worktree[1] when `git worktree add` refers to a
remote branch. This setting might be used for other checkout-like
commands or functionality in the future.
+checkout.autoDetach::
+ If set to true, `git checkout` and `git switch` automatically detach
+ HEAD when switching to something other than a branch. If set to false,
+ they require the `--detach` option to detach HEAD.
++
+If this setting is not specified, `git checkout` defaults to automatic
+detaching, whereas `git switch` defaults to requiring `--detach`.
+
checkout.guess::
Provides the default value for the `--guess` or `--no-guess`
option in `git checkout` and `git switch`. See
diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
index 240c54639e..23f90c15ac 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt
@@ -79,6 +79,9 @@ When the `<commit>` argument is a branch name, the `--detach` option can
be used to detach `HEAD` at the tip of the branch (`git checkout
<branch>` would check out that branch without detaching `HEAD`).
+
+If the `checkout.autoDetach` config variable is set to false, the `--detach`
+option is required even if the `<commit>` argument is not a branch name.
++
Omitting `<branch>` detaches `HEAD` at the tip of the current branch.
'git checkout' [-f|--ours|--theirs|-m|--conflict=<style>] [<tree-ish>] [--] <pathspec>...::
diff --git a/Documentation/git-switch.txt b/Documentation/git-switch.txt
index c60fc9c138..f6b925c43b 100644
--- a/Documentation/git-switch.txt
+++ b/Documentation/git-switch.txt
@@ -83,6 +83,9 @@ $ git switch <new-branch>
Switch to a commit for inspection and discardable
experiments. See the "DETACHED HEAD" section in
linkgit:git-checkout[1] for details.
++
+If the `checkout.autoDetach` configuration variable is set to true,
+`--detach` can be omitted when the target is not a branch.
--guess::
--no-guess::
diff --git a/builtin/checkout.c b/builtin/checkout.c
index f02434bc15..d042638bb0 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1205,6 +1205,10 @@ static int git_checkout_config(const char *var, const char *value,
handle_ignore_submodules_arg(&opts->diff_options, value);
return 0;
}
+ if (!strcmp(var, "checkout.autodetach")) {
+ opts->implicit_detach = git_config_bool(var, value);
+ return 0;
+ }
if (!strcmp(var, "checkout.guess")) {
opts->dwim_new_local_branch = git_config_bool(var, value);
return 0;
diff --git a/t/t2020-checkout-detach.sh b/t/t2020-checkout-detach.sh
index 8202ef8c74..b842b6cc89 100755
--- a/t/t2020-checkout-detach.sh
+++ b/t/t2020-checkout-detach.sh
@@ -69,12 +69,26 @@ test_expect_success 'checkout ref^0 detaches' '
check_detached
'
+test_expect_success 'checkout of tag with autoDetach=false fails' '
+ reset &&
+ test_config checkout.autoDetach false &&
+ test_must_fail git checkout tag &&
+ check_not_detached
+'
+
test_expect_success 'checkout --detach detaches' '
reset &&
git checkout --detach branch &&
check_detached
'
+test_expect_success 'checkout --detach with autoDetach=false detaches' '
+ reset &&
+ test_config checkout.autoDetach false &&
+ git checkout --detach branch &&
+ check_detached
+'
+
test_expect_success 'checkout --detach without branch name' '
reset &&
git checkout --detach &&
diff --git a/t/t2060-switch.sh b/t/t2060-switch.sh
index e247a4735b..69ff197d11 100755
--- a/t/t2060-switch.sh
+++ b/t/t2060-switch.sh
@@ -50,6 +50,13 @@ test_expect_success 'switch and detach current branch' '
test_must_fail git symbolic-ref HEAD
'
+test_expect_success 'switch with checkout.autoDetach=true' '
+ test_when_finished git switch main &&
+ test_config checkout.autoDetach true &&
+ git switch main^{commit} &&
+ test_must_fail git symbolic-ref HEAD
+'
+
test_expect_success 'switch and create branch' '
test_when_finished git switch main &&
git switch -c temp main^ &&
--
2.43.0-rc1
^ permalink raw reply related
* Re: [PATCH] RelNotes: minor wording fixes in 2.43.0 release notes
From: Junio C Hamano @ 2023-11-12 0:59 UTC (permalink / raw)
To: Elijah Newren via GitGitGadget; +Cc: git, Elijah Newren
In-Reply-To: <pull.1611.git.1699675340400.gitgitgadget@gmail.com>
"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Elijah Newren <newren@gmail.com>
>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
> RelNotes: minor wording fixes in 2.43.0 release notes
Thanks, will directly apply on 'master'.
^ permalink raw reply
* Re: [PATCH] fuzz: add new oss-fuzz fuzzer for date.c / date.h
From: Junio C Hamano @ 2023-11-12 5:59 UTC (permalink / raw)
To: Arthur Chan via GitGitGadget; +Cc: git, Arthur Chan
In-Reply-To: <pull.1612.git.1699724379458.gitgitgadget@gmail.com>
"Arthur Chan via GitGitGadget" <gitgitgadget@gmail.com> writes:
> diff --git a/Makefile b/Makefile
> index 03adcb5a480..c9fe99a8c88 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -752,6 +752,7 @@ ETAGS_TARGET = TAGS
> FUZZ_OBJS += oss-fuzz/fuzz-commit-graph.o
> FUZZ_OBJS += oss-fuzz/fuzz-pack-headers.o
> FUZZ_OBJS += oss-fuzz/fuzz-pack-idx.o
> +FUZZ_OBJS += oss-fuzz/fuzz-date.o
The same comment applies to .gitignore but I think the existing
entries are sorted and fuzz-date should be added between
fuzz-commit-graph and fuzz-pack-headers.
> diff --git a/oss-fuzz/fuzz-date.c b/oss-fuzz/fuzz-date.c
> new file mode 100644
> index 00000000000..29bcaf595e4
> --- /dev/null
> +++ b/oss-fuzz/fuzz-date.c
> @@ -0,0 +1,75 @@
> +#include "git-compat-util.h"
> +#include "date.h"
> +
> +int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size);
> +
> +int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)
> +{
> + int type;
> + int time;
> + int num;
> + char *str;
> + timestamp_t ts;
> + enum date_mode_type dmtype;
> + struct date_mode *dm;
> +
> + if (size <= 8)
> + {
> + return 0;
> + }
How much do we care about sticking to our coding style for source
files in this directory? If we do (and I do not see a strong reason
not to), let's lose the {unneeded braces} around a single statement
block.
> + type = (*((int *)data)) % 8;
> + data += 4;
> + size -= 4;
I'd prefer to avoid these hardcoded and unexplained constants. I
think "8" relates to the number of case arms below? I am not sure
if "4" is justifiable without "we assume everybody's int is four
bytes long", but if that is what is going on, perhaps use uint32_t
or something?
Also, is data[] guaranteed to be aligned well to allow us to do the
above? As we only need to spread to DATE_UNIX-1 types (because we
exclude DATE_STRFTIME), it is sufficient to look at the lower nibble
of a single byte. The upper nibble could be used to fuzz the .local
bit if you wanted to, e.g. so I wonder
local_bit = !!(*data & 0x10);
dmtype = (enum date_mode_type)(*data % DATE_UNIX);
if (dmtype == DATE_STRFTIME)
return 0;
data++;
size--;
> + time = abs(*((int *)data));
> + data += 4;
> + size -= 4;
Ditto. Rename "time" because the second parameter to show_date() is
*not* "time" but "tz". The valid range of "tz" comfortably fits in
16-bit signed int, but note that there are valid negative values in
the range.
Are we assuming that the "tz" is attacker controlled? Why are you
limiting its value to non-negative, yet you are not rejecting absurd
timezone offsets? Good values lie in a range much narrower than
between -2400 and 2400. Subjecting "tz" to fuzzer is perfectly
fine, but then limiting its value to non-negative contradicts with
it, so I am not sure what your intention is.
As I used the first byte to fuzz dmtype and .local, let's use the
next three bytes to allow feeding overly wild timezone values to the
machinery and see what breaks, perhaps like so:
tz = *data++; /* int tz; */
tz = (tz << 8) | *data++;
tz = (tz << 8) | *data++;
size -= 3;
Now the upfront length check needs to reject any input shorter than
4 bytes, so do so with a comment accordingly, perhaps like
if (size < 4)
/*
* we use the first byte to fuzz dmtype and local,
* then the next three bytes to fuzz tz offset,
* and the remainder is fed as end-user input to
* approxidate().
*/
return 0;
before everything I wrote so far.
> + str = (char *)malloc(size+1);
(char *)malloc(size + 1);
> + if (!str)
> + {
> + return 0;
> + }
Ditto on {unnecessary braces}.
> + memcpy(str, data, size);
> + str[size] = '\0';
> +
> + ts = approxidate_careful(str, &num);
> + free(str);
> +
> + switch(type)
> + {
> + case 0: default:
> + dmtype = DATE_NORMAL;
> + break;
Style. In our codebase, "switch" and "case" align at the same
column, and case arms are written one per line, i.e.,
switch (type) {
case 0:
default:
...
The way dmtype is handled in a switch() below tells me that you do
not consider it is a potential attack vector (e.g., an attacker
cannot force us to use dmtype==DATE_STRFTIME without the format and
cause us to die). Am I reading your intention correctly?
If so, I'd just do the "use the lower nibble of the first byte" as I
shown earlier, and this large switch statement will go away.
> + case 1:
> + dmtype = DATE_HUMAN;
> + break;
> + case 2:
> + dmtype = DATE_SHORT;
> + break;
> + case 3:
> + dmtype = DATE_ISO8601;
> + break;
> + case 4:
> + dmtype = DATE_ISO8601_STRICT;
> + break;
> + case 5:
> + dmtype = DATE_RFC2822;
> + break;
> + case 6:
> + dmtype = DATE_RAW;
> + break;
> + case 7:
> + dmtype = DATE_UNIX;
> + break;
> + }
> +
> + dm = date_mode_from_type(dmtype);
> + dm->local = 1;
Don't we want to allow the incoming data to fuzz the local bit, too?
> + show_date(ts, time, dm);
> +
> + date_mode_release(dm);
> +
> + return 0;
> +}
>
> base-commit: dadef801b365989099a9929e995589e455c51fed
^ permalink raw reply
* Re: [PATCH] checkout: add config variable checkout.autoDetach
From: Junio C Hamano @ 2023-11-12 6:04 UTC (permalink / raw)
To: Andy Koppe; +Cc: git, pclouds
In-Reply-To: <20231111224253.1923-1-andy.koppe@gmail.com>
Andy Koppe <andy.koppe@gmail.com> writes:
> The git-checkout command without pathspecs automatically detaches HEAD
> when switching to something other than a branch, whereas git-switch
> requires the --detach option to do so.
>
> Add configuration variable checkout.autoDetach to choose the behavior
> for both: true for automatic detaching, false for requiring --detach.
>
> Amend their documentation and tests accordingly.
>
> Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
> ---
"switch" was meant to be an experimental command to sort out this
kind of UI ideas, and I think the fact that it requires a more
explicit "--detach", where experienced users might just say "git
checkout that-branch^0", has established itself as a more friendly
and good thing to help new users. I do not know how others react to
this kind of proliferation of configuration variables, but I do not
mind this particular variable existing.
^ 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