* Re: [PATCH 1/1] attr: add builtin objectmode values support
From: Junio C Hamano @ 2023-11-16 8:08 UTC (permalink / raw)
To: Joanna Wang; +Cc: git, sunshine, tboegi
In-Reply-To: <xmqqsf56ql14.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Other than the removal of that file, I locally applied the following
> fix-up while checking the difference relative to the previous
> iteration.
Cumulatively, aside from the removal of the t/#t* file, here is what
I ended up with so far.
Subject: [PATCH] SQUASH???
---
Documentation/gitattributes.txt | 2 +-
neue | 0
t/t0003-attributes.sh | 5 +++--
t/t6135-pathspec-with-attrs.sh | 10 ++++++----
4 files changed, 10 insertions(+), 7 deletions(-)
create mode 100644 neue
diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 784aa9d4de..201bdf5edb 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -108,7 +108,7 @@ user defined attributes under this namespace will be ignored and
trigger a warning.
`builtin_objectmode`
-^^^^^^^^^^^^^^^^^^^^
+~~~~~~~~~~~~~~~~~~~~
This attribute is for filtering files by their file bit modes (40000,
120000, 160000, 100755, 100644). e.g. ':(attr:builtin_objectmode=160000)'.
You may also check these values with `git check-attr builtin_objectmode -- <file>`.
diff --git a/neue b/neue
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index 86f8681570..774b52c298 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -580,12 +580,13 @@ test_expect_success 'builtin object mode attributes work (dir and regular paths)
'
test_expect_success POSIXPERM 'builtin object mode attributes work (executable)' '
- >exec && chmod +x exec &&
+ >exec &&
+ chmod +x exec &&
attr_check_object_mode exec 100755
'
test_expect_success SYMLINKS 'builtin object mode attributes work (symlinks)' '
- >to_sym ln -s to_sym sym &&
+ ln -s to_sym sym &&
attr_check_object_mode sym 120000
'
diff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh
index b08a32ea68..f6403ebbda 100755
--- a/t/t6135-pathspec-with-attrs.sh
+++ b/t/t6135-pathspec-with-attrs.sh
@@ -295,22 +295,24 @@ test_expect_success 'reading from .gitattributes in a subdirectory (3)' '
test_cmp expect actual
'
-test_expect_success 'pathspec with builtin_objectmode attr can be used' '
+test_expect_success POSIXPERM 'pathspec with builtin_objectmode attr can be used' '
>mode_exec_file_1 &&
git status -s ":(attr:builtin_objectmode=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 add mode_exec_file_1 &&
+ chmod +x mode_exec_file_1 &&
git status -s ":(attr:builtin_objectmode=100755)mode_exec_*" >actual &&
echo AM mode_exec_file_1 >expect &&
test_cmp expect actual
'
-test_expect_success 'builtin_objectmode attr can be excluded' '
+test_expect_success POSIXPERM 'builtin_objectmode attr can be excluded' '
>mode_1_regular &&
- >mode_1_exec && chmod +x mode_1_exec &&
+ >mode_1_exec &&
+ chmod +x mode_1_exec &&
git status -s ":(exclude,attr:builtin_objectmode=100644)" "mode_1_*" >actual &&
echo ?? mode_1_exec >expect &&
test_cmp expect actual &&
--
2.43.0-rc2
^ permalink raw reply related
* Re: [PATCH 1/1] attr: add builtin objectmode values support
From: Junio C Hamano @ 2023-11-16 7:57 UTC (permalink / raw)
To: Joanna Wang; +Cc: git, sunshine, tboegi
In-Reply-To: <20231116054437.2343549-1-jojwang@google.com>
Joanna Wang <jojwang@google.com> writes:
> diff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh
> index a9c1e4e0ec..b08a32ea68 100755
> --- a/t/t6135-pathspec-with-attrs.sh
> +++ b/t/t6135-pathspec-with-attrs.sh
> @@ -295,4 +295,29 @@ test_expect_success 'reading from .gitattributes in a subdirectory (3)' '
> test_cmp expect actual
> '
>
> +test_expect_success 'pathspec with builtin_objectmode attr can be used' '
> + >mode_exec_file_1 &&
> +
> + git status -s ":(attr:builtin_objectmode=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 &&
This again would break with filesystems that are incapable of
supporting executable bit natively. Writing one command per line,
doing something like this may help
git add mode_exec_file_1 &&
git update-index --chmod=+x mode_exec_file_1 &&
> + git status -s ":(attr:builtin_objectmode=100755)mode_exec_*" >actual &&
> + echo AM mode_exec_file_1 >expect &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'builtin_objectmode attr can be excluded' '
> + >mode_1_regular &&
> + >mode_1_exec && chmod +x mode_1_exec &&
Ditto.
> + git status -s ":(exclude,attr:builtin_objectmode=100644)" "mode_1_*" >actual &&
> + echo ?? mode_1_exec >expect &&
> + test_cmp expect actual &&
> +
> + git status -s ":(exclude,attr:builtin_objectmode=100755)" "mode_1_*" >actual &&
> + echo ?? mode_1_regular >expect &&
> + test_cmp expect actual
> +'
> +
> test_done
^ permalink raw reply
* Re: Git Rename Detection Bug
From: Elijah Newren @ 2023-11-16 6:26 UTC (permalink / raw)
To: Philip Oakley; +Cc: Jeremy Pridmore, git@vger.kernel.org, Paul Baumgartner
In-Reply-To: <781fc667-6597-4327-80d5-721fb273d2e2@iee.email>
Hi Philip,
On Wed, Nov 15, 2023 at 6:36 AM Philip Oakley <philipoakley@iee.email> wrote:
>
> Hi Elijah,
> sorry for the delay in replying.
It was only a few days; no need to apologize.
>
> On 11/11/2023 15:13, Elijah Newren wrote:
> > 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,
>
> My point was that we already had the confusion of mental models, with
> both sides essentially thinking they had an "exact rename", hence my
> thought was to add a rather distinct technical name which reflected the
> Git mind-shift. Without something to bring folks up short they'll
> continue, erroneously, with their prior mental models.
Maybe I'm missing the other mental model you're alluding to? The
mental model problems I suspected Jeremy had did not appear to hinge
on exact renames; the fact that Jeremy was trying to "undo" renames
and "redo the correct ones" suggested to me that it's an issue with
understanding detecting vs. tracking renames, something for which it
doesn't matter whether the renames are exact or inexact. (Also, the
fact that he thought it was a bug that the detected renames could
change in `git status` output by merely editing files, suggested also
a detected vs. tracked rename misunderstanding -- and in that case,
there's virtually no chance that an explanation of exact renames or
BLOBSAME or whatever could help since the editing of files in question
means that they aren't going to have the same contents anymore.)
> and 'blob' is something
> > someone new to Git is unlikely to know.
>
> I'd agree that BLOBSAME is new, but we should be proactive in ensuring
> folk do have the mind shift from the old centralised VCS misunderstandings.
I agree it's good to help folks make the mind shift, I'm just not
following how BLOBSAME or any other alternative for "exact rename"
could help in this particular case.
Maybe it'd help to understand why I brought up "exact renames" in the
first place? When most people see "rename detection bug" (as in the
subject of this thread), they're likely to assume some heuristic went
haywire. When Jeremy pointed out that the content of the "mismatched"
file was identical to the source file of interest, I thought it worth
pointing out so anyone else watching the thread would notice. It
means all the normal heuristics involved in inexact renames cannot be
the source of this particular problem. And, in particular, that the
changes that ort brought aren't relevant to this particular problem
either.
> > 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.
>
> Isn't that what I thought I'd said?
> Hash A = Hash A => identical content;
> Hash A != B => different content.
My apologies, I misread what you had written. I had somehow read you
as saying that the rename wasn't needed, but you didn't say that at
all.
Now that I've re-read what you wrote, and hopefully understand better,
I did notice something else that might be worth responding to, though.
Let me re-quote a piece of what you said and respond to it:
> >> ...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, ...
This suggests there aren't heuristics involved when we have identical
blob names. That's not quite accurate, though. When there are
multiple identical blob names that a given source file could be paired
with (e.g. old/foo.txt deleted, and all three of A/foo.txt and
B/bar.txt and C/foo.txt all added on the same side of history and all
with identical contents to the old/foo.txt), we give a higher score to
files that share the same basename (so A/foo.txt and C/foo.txt would
be picked as rename targets over B/bar.txt). If we have multiple
files with an identical blob name and the same file basename (i.e.
A/foo.txt and C/foo.txt in this case), then we pick one, basically at
random as far as the user is concerned (so old/foo.txt could be a
rename to either A/foo.txt or C/foo.txt, just depending on processing
order).
> > 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.
>
> true, the terminology hasn't kept up with the methodology for blob
> content, and the independent meta-data. In previous 'ort' discussions I
> didn't really understand what the '1/2' renames (and other
> nomenclatures) really meant with respect to paths, filenames, content
> and the ours / theirs / base distinctions.
None of that other nomenclature in 'ort' really matters; the problem
presented is almost certainly unrelated to any changes that came with
'ort'. If he had switched to git anytime in the last 15 years he'd
probably see exactly the same problem.
I'm guessing by "'1/2' renames" that you're referring to what I termed
"rename/rename(1to2)" cases, which exist in both merge-recursive and
merge-ort. Those are cases where the base version had a file named A,
and one side of history renamed A->B, while the other side of history
renamed A->C. That kind of situation is not relevant to the current
problem; for the current problem we have a file named D that was
renamed on only one side, but there are both files E and F each with
contents identical to D and rename detection has to decide whether D
was renamed to E or renamed to F on that side.
(This contrasts with "rename/rename(2to1)" cases, where the base
version has both files G & H, and then one side of history renames
G->I and the other side renames H->I. Or in short, 2to1 means two
files are renamed to one file, and 1to2 means one file is renamed to
two.)
> >> 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.
>
> definitely easy to get confused on these cases...
yeah, and apparently I shouldn't have used TREESAME this way as per
Junio's comment elsewhere in the thread. Oops.
> Thanks for the insights.
Thanks for your comments, and again, my apologies for misreading what
you wrote the first time. I hope I didn't do it again.
^ permalink raw reply
* Re: [PATCH 1/1] attr: add builtin objectmode values support
From: Junio C Hamano @ 2023-11-16 6:17 UTC (permalink / raw)
To: Joanna Wang; +Cc: git, sunshine, tboegi
In-Reply-To: <20231116054437.2343549-1-jojwang@google.com>
Joanna Wang <jojwang@google.com> writes:
> t/#t1700-split-index.sh# | 547 ++++++++++++++++++++++++++++++++
This was added by mistake and we can safely ignore it? IOW, is
everything else in the patch, other than the addition of this path,
what you intended to send out?
Other than the removal of that file, I locally applied the following
fix-up while checking the difference relative to the previous
iteration. Other than these, the updated version looked very clear
and nicely done.
Thanks.
t/t0003-attributes.sh | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index 86f8681570..774b52c298 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -580,12 +580,13 @@ test_expect_success 'builtin object mode attributes work (dir and regular paths)
'
test_expect_success POSIXPERM 'builtin object mode attributes work (executable)' '
- >exec && chmod +x exec &&
+ >exec &&
+ chmod +x exec &&
attr_check_object_mode exec 100755
'
test_expect_success SYMLINKS 'builtin object mode attributes work (symlinks)' '
- >to_sym ln -s to_sym sym &&
+ ln -s to_sym sym &&
attr_check_object_mode sym 120000
'
^ permalink raw reply related
* Re: [PATCH v2 09/10] ref-filter.c: use peeled tag for '*' format fields
From: Junio C Hamano @ 2023-11-16 5:48 UTC (permalink / raw)
To: Victoria Dye via GitGitGadget
Cc: git, Patrick Steinhardt, Øystein Walle, Kristoffer Haugsbakk,
Victoria Dye
In-Reply-To: <48254d8e161de7f0e165510c06801195f9b0a8fd.1699991638.git.gitgitgadget@gmail.com>
"Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Victoria Dye <vdye@github.com>
>
> In most builtins ('rev-parse <revision>^{}', 'show-ref --dereference'),
> "dereferencing" a tag refers to a recursive peel of the tag object. Unlike
> these cases, the dereferencing prefix ('*') in 'for-each-ref' format
> specifiers triggers only a single, non-recursive dereference of a given tag
> object. For most annotated tags, a single dereference is all that is needed
> to access the tag's associated commit or tree; "recursive" and
> "non-recursive" dereferencing are functionally equivalent in these cases.
> However, nested tags (annotated tags whose target is another annotated tag)
> dereferenced once return another tag, where a recursive dereference would
> return the commit or tree.
This may be the only potentially controversial step in the series.
> - /*
> - * NEEDSWORK: This derefs tag only once, which
> - * is good to deal with chains of trust, but
> - * is not consistent with what deref_tag() does
> - * which peels the onion to the core.
> - */
> return get_object(ref, 1, &obj, &oi_deref, err);
> }
Very nice to see an ancient comment I added at 9f613ddd (Add
git-for-each-ref: helper for language bindings, 2006-09-15) finally
go.
Thanks.
^ permalink raw reply
* [PATCH 1/1] attr: add builtin objectmode values support
From: Joanna Wang @ 2023-11-16 5:44 UTC (permalink / raw)
To: gitster; +Cc: git, jojwang, sunshine, tboegi
In-Reply-To: <xmqqttpmtnn5.fsf@gitster.g>
Gives all paths builtin objectmode 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:builtin_objectmode=160000)' could filter for submodules without
needing to have `builtin_objectmode=160000` to be set in .gitattributes
for every submodule path.
These values are also reflected in `git check-attr` results.
If the git_attr_direction is set to GIT_ATTR_INDEX or GIT_ATTR_CHECKIN
and a path is not found in the index, the value will be unspecified.
This patch also reserves the builtin_* attribute namespace for objectmode
and any future builtin attributes. Any user defined attributes using this
reserved namespace will result in a warning. This is a breaking change for
any existing builtin_* attributes.
Pathspecs with some builtin_* attribute name (excluding builtin_objectmode)
will behave like any attribute where there are no user specified values.
Signed-off-by: Joanna Wang <jojwang@google.com>
---
>The idiom is to see if it is still NULL and then call git_attr().
Got it, thank you for explaining again.
>We need a precondition here:
>test_expect_success SYMLINKS
Torsten pointed this out in a previous version,
sorry i missed this until Junio pointed it out again.
I've addressed the all the latest feedback in this update.
Documentation/gitattributes.txt | 15 +
attr.c | 71 ++++-
t/#t1700-split-index.sh# | 547 ++++++++++++++++++++++++++++++++
t/t0003-attributes.sh | 75 +++++
t/t6135-pathspec-with-attrs.sh | 25 ++
5 files changed, 730 insertions(+), 3 deletions(-)
create mode 100755 t/#t1700-split-index.sh#
diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 8c1793c148..784aa9d4de 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -100,6 +100,21 @@ for a path to `Unspecified` state. This can be done by listing
the name of the attribute prefixed with an exclamation point `!`.
+RESERVED BUILTIN_* ATTRIBUTES
+-----------------------------
+
+builtin_* is a reserved namespace for builtin attribute values. Any
+user defined attributes under this namespace will be ignored and
+trigger a warning.
+
+`builtin_objectmode`
+^^^^^^^^^^^^^^^^^^^^
+This attribute is for filtering files by their file bit modes (40000,
+120000, 160000, 100755, 100644). e.g. ':(attr:builtin_objectmode=160000)'.
+You may also check these values with `git check-attr builtin_objectmode -- <file>`.
+If the object is not in the index `git check-attr --cached` will return unspecified.
+
+
EFFECTS
-------
diff --git a/attr.c b/attr.c
index e62876dfd3..f8a94aeb23 100644
--- a/attr.c
+++ b/attr.c
@@ -17,6 +17,7 @@
#include "utf8.h"
#include "quote.h"
#include "read-cache-ll.h"
+#include "refs.h"
#include "revision.h"
#include "object-store-ll.h"
#include "setup.h"
@@ -183,6 +184,15 @@ static void all_attrs_init(struct attr_hashmap *map, struct attr_check *check)
}
}
+/*
+ * Atribute name cannot begin with "builtin_" which
+ * is a reserved namespace for built in attributes values.
+ */
+static int attr_name_reserved(const char *name)
+{
+ return starts_with(name, "builtin_");
+}
+
static int attr_name_valid(const char *name, size_t namelen)
{
/*
@@ -315,7 +325,7 @@ static const char *parse_attr(const char *src, int lineno, const char *cp,
cp++;
len--;
}
- if (!attr_name_valid(cp, len)) {
+ if (!attr_name_valid(cp, len) || attr_name_reserved(cp)) {
report_invalid_attr(cp, len, src, lineno);
return NULL;
}
@@ -379,7 +389,7 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
name += strlen(ATTRIBUTE_MACRO_PREFIX);
name += strspn(name, blank);
namelen = strcspn(name, blank);
- if (!attr_name_valid(name, namelen)) {
+ if (!attr_name_valid(name, namelen) || attr_name_reserved(name)) {
report_invalid_attr(name, namelen, src, lineno);
goto fail_return;
}
@@ -1240,6 +1250,61 @@ static struct object_id *default_attr_source(void)
return &attr_source;
}
+static const char *builtin_object_mode_attr(struct index_state *istate, const char *path)
+{
+ unsigned int mode;
+ struct strbuf sb = STRBUF_INIT;
+
+ if (direction == GIT_ATTR_CHECKIN) {
+ struct object_id oid;
+ struct stat st;
+ if (lstat(path, &st))
+ die_errno(_("unable to stat '%s'"), path);
+ mode = canon_mode(st.st_mode);
+ if (S_ISDIR(mode)) {
+ /*
+ *`path` is either a directory or it is a submodule,
+ * in which case it is already indexed as submodule
+ * or it does not exist in the index yet and we need to
+ * check if we can resolve to a ref.
+ */
+ int pos = index_name_pos(istate, path, strlen(path));
+ if (pos >= 0) {
+ if (S_ISGITLINK(istate->cache[pos]->ce_mode))
+ mode = istate->cache[pos]->ce_mode;
+ } else if (resolve_gitlink_ref(path, "HEAD", &oid) == 0) {
+ mode = S_IFGITLINK;
+ }
+ }
+ } else {
+ /*
+ * For GIT_ATTR_CHECKOUT and GIT_ATTR_INDEX we only check
+ * for mode in the index.
+ */
+ int pos = index_name_pos(istate, path, strlen(path));
+ if (pos >= 0)
+ mode = istate->cache[pos]->ce_mode;
+ else
+ return ATTR__UNSET;
+ }
+ strbuf_addf(&sb, "%06o", mode);
+ return strbuf_detach(&sb, NULL);
+}
+
+
+static const char *compute_builtin_attr(struct index_state *istate,
+ const char *path,
+ const struct git_attr *attr) {
+ static const struct git_attr *object_mode_attr;
+
+ if (!object_mode_attr)
+ object_mode_attr = git_attr("builtin_objectmode");
+
+ if (attr == object_mode_attr)
+ return builtin_object_mode_attr(istate, path);
+ return ATTR__UNSET;
+}
+
void git_check_attr(struct index_state *istate,
const char *path,
struct attr_check *check)
@@ -1253,7 +1318,7 @@ void git_check_attr(struct index_state *istate,
unsigned int n = check->items[i].attr->attr_nr;
const char *value = check->all_attrs[n].value;
if (value == ATTR__UNKNOWN)
- value = ATTR__UNSET;
+ value = compute_builtin_attr(istate, path, check->all_attrs[n].attr);
check->items[i].value = value;
}
}
diff --git a/t/#t1700-split-index.sh# b/t/#t1700-split-index.sh#
new file mode 100755
index 0000000000..a7b7263b35
--- /dev/null
+++ b/t/#t1700-split-index.sh#
@@ -0,0 +1,547 @@
+#!/bin/sh
+
+test_description='split index mode tests'
+
+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+
+. ./test-lib.sh
+
+# We need total control of index splitting here
+sane_unset GIT_TEST_SPLIT_INDEX
+
+# Testing a hard coded SHA against an index with an extension
+# that can vary from run to run is problematic so we disable
+# those extensions.
+sane_unset GIT_TEST_FSMONITOR
+sane_unset GIT_TEST_INDEX_THREADS
+
+# Create a file named as $1 with content read from stdin.
+# Set the file's mtime to a few seconds in the past to avoid racy situations.
+create_non_racy_file () {
+ cat >"$1" &&
+ test-tool chmtime =-5 "$1"
+}
+
+test_expect_success 'setup' '
+ test_oid_cache <<-EOF
+ own_v3 sha1:8299b0bcd1ac364e5f1d7768efb62fa2da79a339
+ own_v3 sha256:38a6d2925e3eceec33ad7b34cbff4e0086caa0daf28f31e51f5bd94b4a7af86b
+
+ base_v3 sha1:39d890139ee5356c7ef572216cebcd27aa41f9df
+ base_v3 sha256:c9baeadf905112bf6c17aefbd7d02267afd70ded613c30cafed2d40cb506e1ed
+
+ own_v4 sha1:432ef4b63f32193984f339431fd50ca796493569
+ own_v4 sha256:6738ac6319c25b694afa7bcc313deb182d1a59b68bf7a47b4296de83478c0420
+
+ base_v4 sha1:508851a7f0dfa8691e9f69c7f055865389012491
+ base_v4 sha256:3177d4adfdd4b6904f7e921d91d715a471c0dde7cf6a4bba574927f02b699508
+ EOF
+'
+
+test_expect_success 'enable split index' '
+ git config splitIndex.maxPercentChange 100 &&
+ git update-index --split-index &&
+ test-tool dump-split-index .git/index >actual &&
+ indexversion=$(git update-index --show-index-version) &&
+
+ # NEEDSWORK: Stop hard-coding checksums.
+ if test "$indexversion" = "4"
+ then
+ own=$(test_oid own_v4) &&
+ base=$(test_oid base_v4)
+ else
+ own=$(test_oid own_v3) &&
+ base=$(test_oid base_v3)
+ fi &&
+
+ cat >expect <<-EOF &&
+ own $own
+ base $base
+ replacements:
+ deletions:
+ EOF
+ test_cmp expect actual
+'
+
+test_expect_success 'add one file' '
+ create_non_racy_file one &&
+ git update-index --add one &&
+ git ls-files --stage >ls-files.actual &&
+ cat >ls-files.expect <<-EOF &&
+ 100644 $EMPTY_BLOB 0 one
+ EOF
+ test_cmp ls-files.expect ls-files.actual &&
+
+ test-tool dump-split-index .git/index | sed "/^own/d" >actual &&
+ cat >expect <<-EOF &&
+ base $base
+ 100644 $EMPTY_BLOB 0 one
+ replacements:
+ deletions:
+ EOF
+ test_cmp expect actual
+'
+
+test_expect_success 'disable split index' '
+ git update-index --no-split-index &&
+ git ls-files --stage >ls-files.actual &&
+ cat >ls-files.expect <<-EOF &&
+ 100644 $EMPTY_BLOB 0 one
+ EOF
+ test_cmp ls-files.expect ls-files.actual &&
+
+ BASE=$(test-tool dump-split-index .git/index | sed -n "s/^own/base/p") &&
+ test-tool dump-split-index .git/index | sed "/^own/d" >actual &&
+ cat >expect <<-EOF &&
+ not a split index
+ EOF
+ test_cmp expect actual
+'
+
+test_expect_success 'enable split index again, "one" now belongs to base index"' '
+ git update-index --split-index &&
+ git ls-files --stage >ls-files.actual &&
+ cat >ls-files.expect <<-EOF &&
+ 100644 $EMPTY_BLOB 0 one
+ EOF
+ test_cmp ls-files.expect ls-files.actual &&
+
+ test-tool dump-split-index .git/index | sed "/^own/d" >actual &&
+ cat >expect <<-EOF &&
+ $BASE
+ replacements:
+ deletions:
+ EOF
+ test_cmp expect actual
+'
+
+test_expect_success 'modify original file, base index untouched' '
+ echo modified | create_non_racy_file one &&
+ file1_blob=$(git hash-object one) &&
+ git update-index one &&
+ git ls-files --stage >ls-files.actual &&
+ cat >ls-files.expect <<-EOF &&
+ 100644 $file1_blob 0 one
+ EOF
+ test_cmp ls-files.expect ls-files.actual &&
+
+ test-tool dump-split-index .git/index | sed "/^own/d" >actual &&
+ q_to_tab >expect <<-EOF &&
+ $BASE
+ 100644 $file1_blob 0Q
+ replacements: 0
+ deletions:
+ EOF
+ test_cmp expect actual
+'
+
+test_expect_success 'add another file, which stays index' '
+ create_non_racy_file two &&
+ git update-index --add two &&
+ git ls-files --stage >ls-files.actual &&
+ cat >ls-files.expect <<-EOF &&
+ 100644 $file1_blob 0 one
+ 100644 $EMPTY_BLOB 0 two
+ EOF
+ test_cmp ls-files.expect ls-files.actual &&
+
+ test-tool dump-split-index .git/index | sed "/^own/d" >actual &&
+ q_to_tab >expect <<-EOF &&
+ $BASE
+ 100644 $file1_blob 0Q
+ 100644 $EMPTY_BLOB 0 two
+ replacements: 0
+ deletions:
+ EOF
+ test_cmp expect actual
+'
+
+test_expect_success 'remove file not in base index' '
+ git update-index --force-remove two &&
+ git ls-files --stage >ls-files.actual &&
+ cat >ls-files.expect <<-EOF &&
+ 100644 $file1_blob 0 one
+ EOF
+ test_cmp ls-files.expect ls-files.actual &&
+
+ test-tool dump-split-index .git/index | sed "/^own/d" >actual &&
+ q_to_tab >expect <<-EOF &&
+ $BASE
+ 100644 $file1_blob 0Q
+ replacements: 0
+ deletions:
+ EOF
+ test_cmp expect actual
+'
+
+test_expect_success 'remove file in base index' '
+ git update-index --force-remove one &&
+ git ls-files --stage >ls-files.actual &&
+ test_must_be_empty ls-files.actual &&
+
+ test-tool dump-split-index .git/index | sed "/^own/d" >actual &&
+ cat >expect <<-EOF &&
+ $BASE
+ replacements:
+ deletions: 0
+ EOF
+ test_cmp expect actual
+'
+
+test_expect_success 'add original file back' '
+ create_non_racy_file one &&
+ git update-index --add one &&
+ git ls-files --stage >ls-files.actual &&
+ cat >ls-files.expect <<-EOF &&
+ 100644 $EMPTY_BLOB 0 one
+ EOF
+ test_cmp ls-files.expect ls-files.actual &&
+
+ test-tool dump-split-index .git/index | sed "/^own/d" >actual &&
+ cat >expect <<-EOF &&
+ $BASE
+ 100644 $EMPTY_BLOB 0 one
+ replacements:
+ deletions: 0
+ EOF
+ test_cmp expect actual
+'
+
+test_expect_success 'add new file' '
+ create_non_racy_file two &&
+ git update-index --add two &&
+ git ls-files --stage >actual &&
+ cat >expect <<-EOF &&
+ 100644 $EMPTY_BLOB 0 one
+ 100644 $EMPTY_BLOB 0 two
+ EOF
+ test_cmp expect actual
+'
+
+test_expect_success 'unify index, two files remain' '
+ git update-index --no-split-index &&
+ git ls-files --stage >ls-files.actual &&
+ cat >ls-files.expect <<-EOF &&
+ 100644 $EMPTY_BLOB 0 one
+ 100644 $EMPTY_BLOB 0 two
+ EOF
+ test_cmp ls-files.expect ls-files.actual &&
+
+ test-tool dump-split-index .git/index | sed "/^own/d" >actual &&
+ cat >expect <<-EOF &&
+ not a split index
+ EOF
+ test_cmp expect actual
+'
+
+test_expect_success 'rev-parse --shared-index-path' '
+ test_create_repo split-index &&
+ (
+ cd split-index &&
+ git update-index --split-index &&
+ echo .git/sharedindex* >expect &&
+ git rev-parse --shared-index-path >actual &&
+ test_cmp expect actual &&
+ mkdir subdirectory &&
+ cd subdirectory &&
+ echo ../.git/sharedindex* >expect &&
+ git rev-parse --shared-index-path >actual &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success 'set core.splitIndex config variable to true' '
+ git config core.splitIndex true &&
+ create_non_racy_file three &&
+ git update-index --add three &&
+ git ls-files --stage >ls-files.actual &&
+ cat >ls-files.expect <<-EOF &&
+ 100644 $EMPTY_BLOB 0 one
+ 100644 $EMPTY_BLOB 0 three
+ 100644 $EMPTY_BLOB 0 two
+ EOF
+ test_cmp ls-files.expect ls-files.actual &&
+ BASE=$(test-tool dump-split-index .git/index | grep "^base") &&
+ test-tool dump-split-index .git/index | sed "/^own/d" >actual &&
+ cat >expect <<-EOF &&
+ $BASE
+ replacements:
+ deletions:
+ EOF
+ test_cmp expect actual
+'
+
+test_expect_success 'set core.splitIndex config variable to false' '
+ git config core.splitIndex false &&
+ git update-index --force-remove three &&
+ git ls-files --stage >ls-files.actual &&
+ cat >ls-files.expect <<-EOF &&
+ 100644 $EMPTY_BLOB 0 one
+ 100644 $EMPTY_BLOB 0 two
+ EOF
+ test_cmp ls-files.expect ls-files.actual &&
+ test-tool dump-split-index .git/index | sed "/^own/d" >actual &&
+ cat >expect <<-EOF &&
+ not a split index
+ EOF
+ test_cmp expect actual
+'
+
+test_expect_success 'set core.splitIndex config variable back to true' '
+ git config core.splitIndex true &&
+ create_non_racy_file three &&
+ git update-index --add three &&
+ BASE=$(test-tool dump-split-index .git/index | grep "^base") &&
+ test-tool dump-split-index .git/index | sed "/^own/d" >actual &&
+ cat >expect <<-EOF &&
+ $BASE
+ replacements:
+ deletions:
+ EOF
+ test_cmp expect actual &&
+ create_non_racy_file four &&
+ git update-index --add four &&
+ test-tool dump-split-index .git/index | sed "/^own/d" >actual &&
+ cat >expect <<-EOF &&
+ $BASE
+ 100644 $EMPTY_BLOB 0 four
+ replacements:
+ deletions:
+ EOF
+ test_cmp expect actual
+'
+
+test_expect_success 'check behavior with splitIndex.maxPercentChange unset' '
+ git config --unset splitIndex.maxPercentChange &&
+ create_non_racy_file five &&
+ git update-index --add five &&
+ BASE=$(test-tool dump-split-index .git/index | grep "^base") &&
+ test-tool dump-split-index .git/index | sed "/^own/d" >actual &&
+ cat >expect <<-EOF &&
+ $BASE
+ replacements:
+ deletions:
+ EOF
+ test_cmp expect actual &&
+ create_non_racy_file six &&
+ git update-index --add six &&
+ test-tool dump-split-index .git/index | sed "/^own/d" >actual &&
+ cat >expect <<-EOF &&
+ $BASE
+ 100644 $EMPTY_BLOB 0 six
+ replacements:
+ deletions:
+ EOF
+ test_cmp expect actual
+'
+
+test_expect_success 'check splitIndex.maxPercentChange set to 0' '
+ git config splitIndex.maxPercentChange 0 &&
+ create_non_racy_file seven &&
+ git update-index --add seven &&
+ BASE=$(test-tool dump-split-index .git/index | grep "^base") &&
+ test-tool dump-split-index .git/index | sed "/^own/d" >actual &&
+ cat >expect <<-EOF &&
+ $BASE
+ replacements:
+ deletions:
+ EOF
+ test_cmp expect actual &&
+ create_non_racy_file eight &&
+ git update-index --add eight &&
+ BASE=$(test-tool dump-split-index .git/index | grep "^base") &&
+ test-tool dump-split-index .git/index | sed "/^own/d" >actual &&
+ cat >expect <<-EOF &&
+ $BASE
+ replacements:
+ deletions:
+ EOF
+ test_cmp expect actual
+'
+
+test_expect_success 'shared index files expire after 2 weeks by default' '
+ create_non_racy_file ten &&
+ git update-index --add ten &&
+ test $(ls .git/sharedindex.* | wc -l) -gt 2 &&
+ just_under_2_weeks_ago=$((5-14*86400)) &&
+ test-tool chmtime =$just_under_2_weeks_ago .git/sharedindex.* &&
+ create_non_racy_file eleven &&
+ git update-index --add eleven &&
+ test $(ls .git/sharedindex.* | wc -l) -gt 2 &&
+ just_over_2_weeks_ago=$((-1-14*86400)) &&
+ test-tool chmtime =$just_over_2_weeks_ago .git/sharedindex.* &&
+ create_non_racy_file twelve &&
+ git update-index --add twelve &&
+ test $(ls .git/sharedindex.* | wc -l) -le 2
+'
+
+test_expect_success 'check splitIndex.sharedIndexExpire set to 16 days' '
+ git config splitIndex.sharedIndexExpire "16.days.ago" &&
+ test-tool chmtime =$just_over_2_weeks_ago .git/sharedindex.* &&
+ create_non_racy_file thirteen &&
+ git update-index --add thirteen &&
+ test $(ls .git/sharedindex.* | wc -l) -gt 2 &&
+ just_over_16_days_ago=$((-1-16*86400)) &&
+ test-tool chmtime =$just_over_16_days_ago .git/sharedindex.* &&
+ create_non_racy_file fourteen &&
+ git update-index --add fourteen &&
+ test $(ls .git/sharedindex.* | wc -l) -le 2
+'
+
+test_expect_success 'check splitIndex.sharedIndexExpire set to "never" and "now"' '
+ git config splitIndex.sharedIndexExpire never &&
+ just_10_years_ago=$((-365*10*86400)) &&
+ test-tool chmtime =$just_10_years_ago .git/sharedindex.* &&
+ create_non_racy_file fifteen &&
+ git update-index --add fifteen &&
+ test $(ls .git/sharedindex.* | wc -l) -gt 2 &&
+ git config splitIndex.sharedIndexExpire now &&
+ just_1_second_ago=-1 &&
+ test-tool chmtime =$just_1_second_ago .git/sharedindex.* &&
+ create_non_racy_file sixteen &&
+ git update-index --add sixteen &&
+ test $(ls .git/sharedindex.* | wc -l) -le 2
+'
+
+test_expect_success POSIXPERM 'same mode for index & split index' '
+ git init same-mode &&
+ (
+ cd same-mode &&
+ test_commit A &&
+ test_modebits .git/index >index_mode &&
+ test_must_fail git config core.sharedRepository &&
+ git -c core.splitIndex=true status &&
+ shared=$(ls .git/sharedindex.*) &&
+ case "$shared" in
+ *" "*)
+ # we have more than one???
+ false ;;
+ *)
+ test_modebits "$shared" >split_index_mode &&
+ test_cmp index_mode split_index_mode ;;
+ esac
+ )
+'
+
+while read -r mode modebits
+do
+ test_expect_success POSIXPERM "split index respects core.sharedrepository $mode" '
+ # Remove existing shared index files
+ git config core.splitIndex false &&
+ git update-index --force-remove one &&
+ rm -f .git/sharedindex.* &&
+ # Create one new shared index file
+ git config core.sharedrepository "$mode" &&
+ git config core.splitIndex true &&
+ create_non_racy_file one &&
+ git update-index --add one &&
+ echo "$modebits" >expect &&
+ test_modebits .git/index >actual &&
+ test_cmp expect actual &&
+ shared=$(ls .git/sharedindex.*) &&
+ case "$shared" in
+ *" "*)
+ # we have more than one???
+ false ;;
+ *)
+ test_modebits "$shared" >actual &&
+ test_cmp expect actual ;;
+ esac
+ '
+done <<\EOF
+0666 -rw-rw-rw-
+0642 -rw-r---w-
+EOF
+
+test_expect_success POSIXPERM,SANITY 'graceful handling when splitting index is not allowed' '
+ test_create_repo ro &&
+ (
+ cd ro &&
+ test_commit initial &&
+ git update-index --split-index &&
+ test -f .git/sharedindex.*
+ ) &&
+ cp ro/.git/index new-index &&
+ test_when_finished "chmod u+w ro/.git" &&
+ chmod u-w ro/.git &&
+ GIT_INDEX_FILE="$(pwd)/new-index" git -C ro update-index --split-index &&
+ chmod u+w ro/.git &&
+ rm ro/.git/sharedindex.* &&
+ GIT_INDEX_FILE=new-index git ls-files >actual &&
+ echo initial.t >expected &&
+ test_cmp expected actual
+'
+
+test_expect_success 'writing split index with null sha1 does not write cache tree' '
+ git config core.splitIndex true &&
+ git config splitIndex.maxPercentChange 0 &&
+ git commit -m "commit" &&
+ {
+ git ls-tree HEAD &&
+ printf "160000 commit $ZERO_OID\\tbroken\\n"
+ } >broken-tree &&
+ echo "add broken entry" >msg &&
+
+ tree=$(git mktree <broken-tree) &&
+ test_tick &&
+ commit=$(git commit-tree $tree -p HEAD <msg) &&
+ git update-ref HEAD "$commit" &&
+ GIT_ALLOW_NULL_SHA1=1 git reset --hard &&
+ test_might_fail test-tool dump-cache-tree >cache-tree.out &&
+ test_line_count = 0 cache-tree.out
+'
+
+test_expect_success 'do not refresh null base index' '
+ test_create_repo merge &&
+ (
+ cd merge &&
+ test_commit initial &&
+ git checkout -b side-branch &&
+ test_commit extra &&
+ git checkout main &&
+ git update-index --split-index &&
+ test_commit more &&
+ # must not write a new shareindex, or we wont catch the problem
+ git -c splitIndex.maxPercentChange=100 merge --no-edit side-branch 2>err &&
+ # i.e. do not expect warnings like
+ # could not freshen shared index .../shareindex.00000...
+ test_must_be_empty err
+ )
+'
+
+test_expect_success 'reading split index at alternate location' '
+ git init reading-alternate-location &&
+ (
+ cd reading-alternate-location &&
+ >file-in-alternate &&
+ git update-index --split-index --add file-in-alternate
+ ) &&
+ echo file-in-alternate >expect &&
+
+ # Should be able to find the shared index both right next to
+ # the specified split index file ...
+ GIT_INDEX_FILE=./reading-alternate-location/.git/index \
+ git ls-files --cached >actual &&
+ test_cmp expect actual &&
+
+ # ... and, for backwards compatibility, in the current GIT_DIR
+ # as well.
+ mv -v ./reading-alternate-location/.git/sharedindex.* .git &&
+ GIT_INDEX_FILE=./reading-alternate-location/.git/index \
+ git ls-files --cached >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'GIT_TEST_SPLIT_INDEX works' '
+ git init git-test-split-index &&
+ (
+ cd git-test-split-index &&
+ >file &&
+ GIT_TEST_SPLIT_INDEX=1 git update-index --add file &&
+ ls -l .git/sharedindex.* >actual &&
+ test_line_count = 1 actual
+ )
+'
+
+test_done
diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index aee2298f01..86f8681570 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -19,6 +19,20 @@ attr_check () {
test_must_be_empty err
}
+attr_check_object_mode_basic () {
+ path="$1" &&
+ expect="$2" &&
+ check_opts="$3" &&
+ git check-attr $check_opts builtin_objectmode -- "$path" >actual 2>err &&
+ echo "$path: builtin_objectmode: $expect" >expect &&
+ test_cmp expect actual
+}
+
+attr_check_object_mode () {
+ attr_check_object_mode_basic "$@" &&
+ test_must_be_empty err
+}
+
attr_check_quote () {
path="$1" quoted_path="$2" expect="$3" &&
@@ -558,4 +572,65 @@ test_expect_success EXPENSIVE 'large attributes file ignored in index' '
test_cmp expect err
'
+test_expect_success 'builtin object mode attributes work (dir and regular paths)' '
+ >normal &&
+ attr_check_object_mode normal 100644 &&
+ mkdir dir &&
+ attr_check_object_mode dir 040000
+'
+
+test_expect_success POSIXPERM 'builtin object mode attributes work (executable)' '
+ >exec && chmod +x exec &&
+ attr_check_object_mode exec 100755
+'
+
+test_expect_success SYMLINKS 'builtin object mode attributes work (symlinks)' '
+ >to_sym ln -s to_sym sym &&
+ attr_check_object_mode sym 120000
+'
+
+test_expect_success 'native object mode attributes work with --cached' '
+ >normal &&
+ git add normal &&
+ empty_blob=$(git rev-parse :normal) &&
+ git update-index --index-info <<-EOF &&
+ 100755 $empty_blob 0 exec
+ 120000 $empty_blob 0 symlink
+ EOF
+ attr_check_object_mode normal 100644 --cached &&
+ attr_check_object_mode exec 100755 --cached &&
+ attr_check_object_mode symlink 120000 --cached
+'
+
+test_expect_success 'check object mode attributes work for submodules' '
+ mkdir sub &&
+ (
+ cd sub &&
+ git init &&
+ mv .git .real &&
+ echo "gitdir: .real" >.git &&
+ test_commit first
+ ) &&
+ attr_check_object_mode sub 160000 &&
+ attr_check_object_mode sub unspecified --cached &&
+ git add sub &&
+ attr_check_object_mode sub 160000 --cached
+'
+
+test_expect_success 'we do not allow user defined builtin_* attributes' '
+ echo "foo* builtin_foo" >.gitattributes &&
+ git add .gitattributes 2>actual &&
+ echo "builtin_foo is not a valid attribute name: .gitattributes:1" >expect &&
+ test_cmp expect actual
+'
+
+test_expect_success 'user defined builtin_objectmode values are ignored' '
+ echo "foo* builtin_objectmode=12345" >.gitattributes &&
+ git add .gitattributes &&
+ >foo_1 &&
+ attr_check_object_mode_basic foo_1 100644 &&
+ echo "builtin_objectmode is not a valid attribute name: .gitattributes:1" >expect &&
+ test_cmp expect err
+'
+
test_done
diff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh
index a9c1e4e0ec..b08a32ea68 100755
--- a/t/t6135-pathspec-with-attrs.sh
+++ b/t/t6135-pathspec-with-attrs.sh
@@ -295,4 +295,29 @@ test_expect_success 'reading from .gitattributes in a subdirectory (3)' '
test_cmp expect actual
'
+test_expect_success 'pathspec with builtin_objectmode attr can be used' '
+ >mode_exec_file_1 &&
+
+ git status -s ":(attr:builtin_objectmode=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:builtin_objectmode=100755)mode_exec_*" >actual &&
+ echo AM mode_exec_file_1 >expect &&
+ test_cmp expect actual
+'
+
+test_expect_success 'builtin_objectmode attr can be excluded' '
+ >mode_1_regular &&
+ >mode_1_exec && chmod +x mode_1_exec &&
+ git status -s ":(exclude,attr:builtin_objectmode=100644)" "mode_1_*" >actual &&
+ echo ?? mode_1_exec >expect &&
+ test_cmp expect actual &&
+
+ git status -s ":(exclude,attr:builtin_objectmode=100755)" "mode_1_*" >actual &&
+ echo ?? mode_1_regular >expect &&
+ test_cmp expect actual
+'
+
test_done
--
2.43.0.rc0.421.g78406f8d94-goog
^ permalink raw reply related
* Re: [PATCH v2 04/10] ref-filter.h: add functions for filter/format & format-only
From: Junio C Hamano @ 2023-11-16 5:39 UTC (permalink / raw)
To: Victoria Dye via GitGitGadget
Cc: git, Patrick Steinhardt, Øystein Walle, Kristoffer Haugsbakk,
Victoria Dye
In-Reply-To: <187b1d6610f96ba16bb7e1ff80d1c994a67b8753.1699991638.git.gitgitgadget@gmail.com>
"Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:
> This consolidates much of the code used to filter and format refs in
> 'builtin/for-each-ref.c', 'builtin/tag.c', and 'builtin/branch.c', reducing
> duplication and simplifying the future changes needed to optimize the filter
> & format process.
>
> Signed-off-by: Victoria Dye <vdye@github.com>
> ---
> builtin/branch.c | 33 +++++++++++++++++----------------
> builtin/for-each-ref.c | 27 +--------------------------
> builtin/tag.c | 23 +----------------------
> ref-filter.c | 35 +++++++++++++++++++++++++++++++++++
> ref-filter.h | 14 ++++++++++++++
> 5 files changed, 68 insertions(+), 64 deletions(-)
The amount of existing duplication of code is rather surprising, and
this patch nicely refactors to improve. Good.
Thanks.
^ permalink raw reply
* Re: [PATCH v2 01/10] ref-filter.c: really don't sort when using --no-sort
From: Junio C Hamano @ 2023-11-16 5:29 UTC (permalink / raw)
To: Victoria Dye via GitGitGadget
Cc: git, Patrick Steinhardt, Øystein Walle, Kristoffer Haugsbakk,
Victoria Dye
In-Reply-To: <074da1ff3e85927324c42a3fa65e4239f051cd70.1699991638.git.gitgitgadget@gmail.com>
"Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Victoria Dye <vdye@github.com>
>
> When '--no-sort' is passed to 'for-each-ref', 'tag', and 'branch', the
> printed refs are still sorted by ascending refname. Change the handling of
> sort options in these commands so that '--no-sort' to truly disables
> sorting.
"to truly disables" -> "truly disables" I think?
> '--no-sort' does not disable sorting in these commands is because their
"'--no-sort' does not" -> "The reason why '--no-sort' does not", or
"is because" -> "because".
> option parsing does not distinguish between "the absence of '--sort'"
> (and/or values for tag.sort & branch.sort) and '--no-sort'. Both result in
> an empty 'sorting_options' string list, which is parsed by
> 'ref_sorting_options()' to create the 'struct ref_sorting *' for the
> command. If the string list is empty, 'ref_sorting_options()' interprets
> that as "the absence of '--sort'" and returns the default ref sorting
> structure (equivalent to "refname" sort).
>
> To handle '--no-sort' properly while preserving the "refname" sort in the
> "absence of --sort'" case, first explicitly add "refname" to the string list
> *before* parsing options. This alone doesn't actually change any behavior,
> since 'compare_refs()' already falls back on comparing refnames if two refs
> are equal w.r.t all other sort keys.
>
> Now that the string list is populated by default, '--no-sort' is the only
> way to empty the 'sorting_options' string list. Update
> 'ref_sorting_options()' to return a NULL 'struct ref_sorting *' if the
> string list is empty, and add a condition to 'ref_array_sort()' to skip the
> sort altogether if the sort structure is NULL. Note that other functions
> using 'struct ref_sorting *' do not need any changes because they already
> ignore NULL values.
Nice.
> Finally, remove the condition around sorting in 'ls-remote', since it's no
> longer necessary. Unlike 'for-each-ref' et. al., it does *not* do any
> sorting by default. This default is preserved by simply leaving its sort key
> string list empty before parsing options; if no additional sort keys are
> set, 'struct ref_sorting *' is NULL and sorting is skipped.
Doubly nice.
> diff --git a/ref-filter.c b/ref-filter.c
> index e4d3510e28e..7250089b7c6 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -3142,7 +3142,8 @@ void ref_sorting_set_sort_flags_all(struct ref_sorting *sorting,
>
> void ref_array_sort(struct ref_sorting *sorting, struct ref_array *array)
> {
> - QSORT_S(array->items, array->nr, compare_refs, sorting);
> + if (sorting)
> + QSORT_S(array->items, array->nr, compare_refs, sorting);
> }
Nice. We do allow passing NULL to ref_sorting_release(), and we can
return NULL from ref_sorting_options(), and allowing NULL to be
passed to this function makes it easier for the callers to deal with
the case where no sorting is specified.
> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> index 3182abde27f..9918ba05dec 100755
> --- a/t/t3200-branch.sh
> +++ b/t/t3200-branch.sh
> @@ -1570,9 +1570,10 @@ test_expect_success 'tracking with unexpected .fetch refspec' '
>
> test_expect_success 'configured committerdate sort' '
> git init -b main sort &&
> + test_config -C sort branch.sort "committerdate" &&
> +
> (
> cd sort &&
> - git config branch.sort committerdate &&
> test_commit initial &&
> git checkout -b a &&
> test_commit a &&
> @@ -1592,9 +1593,10 @@ test_expect_success 'configured committerdate sort' '
> '
>
> test_expect_success 'option override configured sort' '
> + test_config -C sort branch.sort "committerdate" &&
> +
> (
> cd sort &&
> - git config branch.sort committerdate &&
> git branch --sort=refname >actual &&
> cat >expect <<-\EOF &&
> a
The above two are not strictly necessary for the purpose of this
patch, in that the tests that come after these tests do not care if
the branch.sort configuration variable is set in the "sort"
repository, as they set their own value before doing their test.
But of course, cleaning up after yourself with test_config and
friends is a good idea regardless, and a handful of new tests added
after this point follow the same pattern. Good.
> @@ -1606,10 +1608,70 @@ test_expect_success 'option override configured sort' '
> )
> '
>
> +test_expect_success '--no-sort cancels config sort keys' '
> + test_config -C sort branch.sort "-refname" &&
> +
> + (
> + cd sort &&
> +
> + # objecttype is identical for all of them, so sort falls back on
> + # default (ascending refname)
Interesting.
> + git branch \
> + --no-sort \
> + --sort="objecttype" >actual &&
> + cat >expect <<-\EOF &&
> + a
> + * b
> + c
> + main
> + EOF
> + test_cmp expect actual
> + )
> +
> +'
> +
> +test_expect_success '--no-sort cancels command line sort keys' '
> + (
> + cd sort &&
> +
> + # objecttype is identical for all of them, so sort falls back on
> + # default (ascending refname)
> + git branch \
> + --sort="-refname" \
> + --no-sort \
> + --sort="objecttype" >actual &&
OK, this exercises the same "--no-sort cleans the slate" as before,
and for this one it is essential that we lack branch.sort after the
previous step is done, which is ensured thanks to the use of
test_config in the previous one. Nice.
> + cat >expect <<-\EOF &&
> + a
> + * b
> + c
> + main
> + EOF
> + test_cmp expect actual
> + )
> +'
> +
> +test_expect_success '--no-sort without subsequent --sort prints expected branches' '
> + (
> + cd sort &&
> +
> + # Sort the results with `sort` for a consistent comparison
> + # against expected
> + git branch --no-sort | sort >actual &&
> + cat >expect <<-\EOF &&
> + a
> + c
> + main
> + * b
> + EOF
> + test_cmp expect actual
> + )
> +'
> +
> test_expect_success 'invalid sort parameter in configuration' '
> + test_config -C sort branch.sort "v:notvalid" &&
> +
> (
> cd sort &&
> - git config branch.sort "v:notvalid" &&
>
> # this works in the "listing" mode, so bad sort key
> # is a dying offence.
With such an invalid configuration value set, running the command
with "--no-sort" would stop the command from failing? Is that worth
protecting with a new test, I wonder.
Overall very nicely done.
Thanks.
^ permalink raw reply
* Re: [RFC PATCH v2 2/2] send-email: remove stray characters from usage
From: Junio C Hamano @ 2023-11-16 4:59 UTC (permalink / raw)
To: Todd Zullinger
Cc: git, Jeff King, Ævar Arnfjörð Bjarmason,
Ondřej Pohořelský
In-Reply-To: <20231115173952.339303-3-tmz@pobox.com>
Todd Zullinger <tmz@pobox.com> writes:
> A few stray single quotes crept into the usage string in a2ce608244
> (send-email docs: add format-patch options, 2021-10-25). Remove them.
>
> Signed-off-by: Todd Zullinger <tmz@pobox.com>
> ---
> This is not scrictly tied to the previous commit. It just stood out
> while I was reviewing the usage output.
Thanks. Let's split this out as a docfix patch and handle it
separately.
>
> git-send-email.perl | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 94046e0fb7..cd2f0ae14e 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -28,8 +28,8 @@
>
> sub usage {
> print <<EOT;
> -git send-email' [<options>] <file|directory>
> -git send-email' [<options>] <format-patch options>
> +git send-email [<options>] <file|directory>
> +git send-email [<options>] <format-patch options>
> git send-email --dump-aliases
>
> Composing:
^ permalink raw reply
* Re: [RFC PATCH v2 1/2] send-email: avoid duplicate specification warnings
From: Junio C Hamano @ 2023-11-16 4:58 UTC (permalink / raw)
To: Todd Zullinger
Cc: git, Jeff King, Ævar Arnfjörð Bjarmason,
Ondřej Pohořelský
In-Reply-To: <20231115173952.339303-2-tmz@pobox.com>
Todd Zullinger <tmz@pobox.com> writes:
> With perl-Getopt-Long >= 2.55 a warning is issued for options which are
> specified more than once. In addition to causing users to see warnings,
> this results in test failures which compare the output. An example,
> from t9001-send-email.37:
>
> | +++ diff -u expect actual
> | --- expect 2023-11-14 10:38:23.854346488 +0000
> | +++ actual 2023-11-14 10:38:23.848346466 +0000
> | @@ -1,2 +1,7 @@
> | +Duplicate specification "no-chain-reply-to" for option "no-chain-reply-to"
> | +Duplicate specification "to-cover|to-cover!" for option "to-cover"
> | +Duplicate specification "cc-cover|cc-cover!" for option "cc-cover"
> | +Duplicate specification "no-thread" for option "no-thread"
> | +Duplicate specification "no-to-cover" for option "no-to-cover"
> | fatal: longline.patch:35 is longer than 998 characters
> | warning: no patches were sent
> | error: last command exited with $?=1
> | not ok 37 - reject long lines
>
> Remove the duplicate option specs.
>
> Teach `--git-completion-helper` to output the '--no-' options. They are
> not included in the options hash and would otherwise be lost.
Nice to see a careful handling of potential fallouts.
> A little history:
>
> Support for the '--no-' prefix was added in Getopt::Long >= 2.33, in
> commit 8ca8b48 (Negatable options (with "!") now also support the
> "no-" prefix., 2003-04-04). Getopt::Long 2.34 was included in
> perl-5.8.1 (2003-09-25), per Module::CoreList[1].
>
> We list perl-5.8 as the minimum version in INSTALL. This would leave
> users with perl-5.8.0 (2002-07-18) with non-working arguments for
> options where we're removing the explicit 'no-' variant.
>
> The explicit 'no-' opts were added in f471494303 (git-send-email.perl:
> support no- prefix with older GetOptions, 2015-01-30), specifically to
> support perl-5.8.0 which includes the older Getopt::Long.
These are all very much relevant and deserve to be in the log
message, not hidden under the three-dash line, I would think.
Thanks for digging the history. The first paragraph was a bit hard
to read as it wasn't clear "support" on which side is being
discussed, though. If it were written perhaps like so:
Getopt::Long >= 2.33 started supporting the '--no-' prefix
natively by appending '!' to the option specification string,
which was shipped with perl-5.8.1 and not present in perl-5.8.0
it would have been clear that it was talking about the support
given by Getopt module, not on our side.
> It may be time to bump the Perl requirement to 5.8.1 (2003-09-25) or
> even 5.10.0 (2007-12-18). We last bumped the requirement from 5.6 to
> 5.8 in d48b284183 (perl: bump the required Perl version to 5.8 from
> 5.6.[21], 2010-09-24).
Isn't the position this patch takes a lot stronger than "It may be
time"? If we applied this patch, it drops the support for folks
with Perl 5.8.0 (which I do not think is a bad thing, by the way).
This sounds like something that is worth describing in the log
message (and Release Notes).
> If there is a way to have our cake without any consequence, I'm happy to
> hear it. If not, I'll add a commit which bumps the requirement in
> general or notes that some git-send-email requires perl >= 5.8.1 and
> adjusts the 'use' line there to `use 5.008001;`.
Sounds like a plan.
> diff --git a/git-send-email.perl b/git-send-email.perl
> index cacdbd6bb2..94046e0fb7 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -119,13 +119,16 @@ sub completion_helper {
>
> foreach my $key (keys %$original_opts) {
> unless (exists $not_for_completion{$key}) {
> - $key =~ s/!$//;
> + my $negate = ($key =~ s/!$//);
A very minor nit, but I'd call this $negatable if I were doing this
patch.
Just to make sure I did not misunderstand what you said below the
three-dash line, if we were to take the other option that allows us
to live with 5.8.0, we would make this hunk ...
> "chain-reply-to!" => \$chain_reply_to,
> - "no-chain-reply-to" => sub {$chain_reply_to = 0},
... look more like this?
> - "chain-reply-to!" => \$chain_reply_to,
> + "chain-reply-to" => \$chain_reply_to,
> "no-chain-reply-to" => sub {$chain_reply_to = 0},
> + "nochain-reply-to" => sub {$chain_reply_to = 0},
That is, by removing the "!" suffix, we reject the native support of
"--no-*" offered by Getopt::Long, and implement the negated variants
ourselves?
Thanks.
^ permalink raw reply
* Re: Bug: "Received" misspelled in remote message
From: Junio C Hamano @ 2023-11-16 3:42 UTC (permalink / raw)
To: git; +Cc: Alan Dove
In-Reply-To: <xmqqmsvetmu3.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
>> "remote: Recieved update on checked-out branch, queueing deployment."
>>
>> "Received" is misspelled.
>
> I think it is coming from the "push-to-checkout" hook that is
> installed on your "private server", and not from what we ship as
> part of our software offering.
Googling for the misspelled message seems to indicate that it is
coming from cpanel (whatever it is). A randomly picked example is
https://essgeelabs.com/posts/cpanel-git-1/ which is a recipe that
does not involve typing the typoed message by the end-user, so
somebody (most likely cPanel) must be shipping with a hook with
typo.
Another user of cPanel reports this
https://forums.cpanel.net/threads/git-automatic-deployment-not-working-but-manual-deployment-is.679837/
where they observe post-receive hook with the typo.
Anybody with contact there at cpanel.net may want to report the bug.
Thanks.
^ permalink raw reply
* Re: Bug: "Received" misspelled in remote message
From: Junio C Hamano @ 2023-11-16 3:10 UTC (permalink / raw)
To: Alan Dove; +Cc: git
In-Reply-To: <4e76dda1009a8365a1d66d9594865a4af31ab418.camel@gmail.com>
Alan Dove <alan.dove@gmail.com> writes:
> Hello:
>
> Using Git 2.40.1 on a private server, I see this message whenever I
> push a commit:
>
> "remote: Recieved update on checked-out branch, queueing deployment."
>
> "Received" is misspelled.
I think it is coming from the "push-to-checkout" hook that is
installed on your "private server", and not from what we ship as
part of our software offering.
We are not going to include spelling correction software to massage
the output from end-user installed hooks, so it is unlikely the
typo will be fixed by us.
> I realize this is a very minor issue, but it should also be very easy
> to correct. As someone who suffers from Proofreader's Eye, this error
> deals psychic damage to me on a daily basis.
Sounds like something a world-class science writer may say ;-).
Thanks for reporting.
^ permalink raw reply
* Re: [PATCH 1/1] attr: add builtin objectmode values support
From: Junio C Hamano @ 2023-11-16 2:53 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Joanna Wang, git, tboegi
In-Reply-To: <CAPig+cSyrD71NMnfVCTHEf+K8vop9QHLet7uyOerrq=v9SVbFw@mail.gmail.com>
Eric Sunshine <sunshine@sunshineco.com> writes:
> A bit more idiomatic in this codebase would be:
>
> git update-index --index-info <<-EOF
> 100755 $empty_blob 0 exec
> 120000 $empty_blob 0 symlink
> EOF
>
> No need for `cat`.
Aye, of course. We should not cat a single-source datastream into a
pipe. Thanks for correcting me.
^ permalink raw reply
* Re: [PATCH 1/1] attr: add builtin objectmode values support
From: Eric Sunshine @ 2023-11-16 1:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Joanna Wang, git, tboegi
In-Reply-To: <xmqqy1eytrns.fsf@gitster.g>
On Wed, Nov 15, 2023 at 8:26 PM Junio C Hamano <gitster@pobox.com> wrote:
> Joanna Wang <jojwang@google.com> writes:
> > +test_expect_success 'native object mode attributes work with --cached' '
> > + >normal && attr_check_object_mode normal unspecified --cached &&
> > + git add normal && attr_check_object_mode normal 100644 --cached
> > +'
>
> For "--cached test", on the other hand, we should be able to set the
> executable bit or record a symbolic link regardless of the
> filesystem using "update-index", e.g.,
>
> empty_blob=$(git rev-parse :normal)
> cat <<-EOF | git update-index --index-info
> 100755 $empty_blob 0 exec
> 120000 $empty_blob 0 symlink
> EOF
>
> or something.
A bit more idiomatic in this codebase would be:
git update-index --index-info <<-EOF
100755 $empty_blob 0 exec
120000 $empty_blob 0 symlink
EOF
No need for `cat`.
^ permalink raw reply
* Re: [PATCH 1/1] attr: add builtin objectmode values support
From: Junio C Hamano @ 2023-11-16 1:26 UTC (permalink / raw)
To: Joanna Wang; +Cc: git, tboegi
In-Reply-To: <20231114214934.3484892-1-jojwang@google.com>
Joanna Wang <jojwang@google.com> writes:
>> static struct git_attr mode_attr;
>>
>> if (!mode_attr)
>> mode_attr = git_attr("mode");
> Is there an idiomatic way to check if this git_attr (struct or pointer)
> has been initialized?
Sorry (I think the above is a typo from the "to illustrate the idea"
code snippet in my message), the static one should be a pointer to a
struct, as git_attr() is meant to "intern" the given name and return
a singleton instance.
The idiom is to see if it is still NULL and then call git_attr().
Of course, you could repeatedly call git_attr() with the same name
and get the same singleton instance, so without the "have we
initialized?" check, your code will still be correct, but it would
incur a locked hashmap lookup (to ensure that we do not create the
second instance of the same name), so...
> +RESERVED BUILTIN_* ATTRIBUTES
> +-----------------------------
> +
> +builtin_* is a reserved namespace for builtin attribute values. Any
> +user defined attributes under this namespace will result in a warning.
warning and then? I think we would want to do "warn and ignore",
not "warn and disable all the builtin_ computation", i.e.,
... will be ignored and a warning message is given.
> +/*
> + * Atribute name cannot begin with "builtin_" which
> + * is a reserved namespace for built in attributes values.
> + */
> +static int attr_name_reserved(const char *name)
> +{
> + return !strncmp(name, "builtin_", strlen("builtin_"));
> +}
> +
You'd want to use starts_with() probably.
> @@ -1240,6 +1250,57 @@ static struct object_id *default_attr_source(void)
> return &attr_source;
> }
>
> +static const char *builtin_object_mode_attr(struct index_state *istate, const char *path)
> +{
> + unsigned int mode;
> + struct strbuf sb = STRBUF_INIT;
> +
> + if (direction == GIT_ATTR_CHECKIN) {
> + struct object_id oid;
> + struct stat st;
> + if (lstat(path, &st))
> + die_errno(_("unable to stat '%s'"), path);
> + mode = canon_mode(st.st_mode);
> + if (S_ISDIR(mode)) {
> + /* `path` is either a directory or it is a submodule,
> + * in which case it is already indexed as submodule
> + * or it does not exist in the index yet and we need to
> + * check if we can resolve to a ref.
> + */
/*
* Our multi-line comment begins and ends with
* slash-asterisk and asterisk-slash on their
* own lines without anything else.
*/
> + int pos = index_name_pos(istate, path, strlen(path));
> + if (pos >= 0) {
> + if (S_ISGITLINK(istate->cache[pos]->ce_mode))
> + mode = istate->cache[pos]->ce_mode;
> + } else if (resolve_gitlink_ref(path, "HEAD", &oid) == 0)
> + mode = S_IFGITLINK;
> + }
Give {braces} around the else if block as the other side has it
(Documentation/CodingGuidelines):
- When there are multiple arms to a conditional and some of them
require braces, enclose even a single line block in braces for
consistency.
> + } else {
> + /*
> + * For GIT_ATTR_CHECKOUT and GIT_ATTR_INDEX we only check
> + * for mode in the index.
> + */
> + int pos = index_name_pos(istate, path, strlen(path));
> + if (pos >= 0)
> + mode = istate->cache[pos]->ce_mode;
> + else
> + return ATTR__UNSET;
> + }
> + strbuf_addf(&sb, "%06o", mode);
> + return sb.buf;
> +
The struct itself will be reclaimed as it is on stack, and sb.buf is
given back to the caller, so there technically is no leak, but I think
a more kosher way to do this involves the use of strbuf_detach().
Let's lose the blank line at the end of the function that is not
serving much useful purpose.
> void git_check_attr(struct index_state *istate,
> const char *path,
> struct attr_check *check)
> @@ -1253,7 +1314,7 @@ void git_check_attr(struct index_state *istate,
> unsigned int n = check->items[i].attr->attr_nr;
> const char *value = check->all_attrs[n].value;
> if (value == ATTR__UNKNOWN)
> - value = ATTR__UNSET;
> + value = compute_builtin_attr(istate, path, check->all_attrs[n].attr);
> check->items[i].value = value;
> }
> }
Nicely done.
> diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
> index aee2298f01..25aa3fbd05 100755
> --- a/t/t0003-attributes.sh
> +++ b/t/t0003-attributes.sh
> @@ -19,6 +19,16 @@ attr_check () {
> test_must_be_empty err
> }
>
> +attr_check_object_mode () {
> + path="$1" &&
> + expect="$2" &&
> + check_opts="$3" &&
> + git check-attr $check_opts builtin_objectmode -- "$path" >actual 2>err &&
> + echo "$path: builtin_objectmode: $expect" >expect &&
> + test_cmp expect actual
> + test_must_be_empty err
> +}
> +
> attr_check_quote () {
> path="$1" quoted_path="$2" expect="$3" &&
>
> @@ -558,4 +568,38 @@ test_expect_success EXPENSIVE 'large attributes file ignored in index' '
> test_cmp expect err
> '
>
> +test_expect_success 'native object mode attributes work' '
> + >exec && chmod +x exec && attr_check_object_mode exec 100755 &&
> + >normal && attr_check_object_mode normal 100644 &&
> + mkdir dir && attr_check_object_mode dir 040000 &&
Just a style thing, but we tend to write one statement on each line.
> + >to_sym ln -s to_sym sym && attr_check_object_mode sym 120000
> +'
You do not need to_sym file, do you? Symbolic links can be left
dangling.
In any case, the above test would not work on a filesystem without
native executable bit support. Also, you cannot do "ln -s" on
certain filesystems. It is a good idea to split the above into
three tests, one that is run unconditionally (for "normal" and
"dir"), another that is run under the POSIXPERM prerequisite, and
the third that is run under the SYNLINKS prerequisite.
> +test_expect_success 'native object mode attributes work with --cached' '
> + >normal && attr_check_object_mode normal unspecified --cached &&
> + git add normal && attr_check_object_mode normal 100644 --cached
> +'
For "--cached test", on the other hand, we should be able to set the
executable bit or record a symbolic link regardless of the
filesystem using "update-index", e.g.,
>normal && git add normal &&
empty_blob=$(git rev-parse :normal)
cat <<-EOF | git update-index --index-info
100755 $empty_blob 0 exec
120000 $empty_blob 0 symlink
EOF
attr_check_object_mode normal 100644 --cached &&
attr_check_object_mode exec 100755 --cached &&
attr_check_object_mode symlink 120000 --cached
or something.
> +test_expect_success 'check object mode attributes work for submodules' '
> + mkdir sub &&
> + (
> + cd sub &&
> + git init &&
> + mv .git .real &&
> + echo "gitdir: .real" >.git &&
> + test_commit first
> + ) &&
> + attr_check_object_mode sub 160000 &&
> + attr_check_object_mode sub unspecified --cached &&
> + git add sub &&
> + attr_check_object_mode sub 160000 --cached
> +'
Sounds good.
> +test_expect_success 'we do not allow user defined builtin_* attributes' '
> + echo "foo* builtin_foo" >.gitattributes &&
> + git add .gitattributes 2>actual &&
> + echo "builtin_foo is not a valid attribute name: .gitattributes:1" >expect &&
> + test_cmp expect actual
> +'
Another thing that is worth checking is what happens when there is an
conflicting entry:
echo "foo builtin_objectmode=200" >.gitattributes &&
>foo &&
attr_check_object_mode foo 100644
> test_done
> diff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh
> index a9c1e4e0ec..b08a32ea68 100755
> --- a/t/t6135-pathspec-with-attrs.sh
> +++ b/t/t6135-pathspec-with-attrs.sh
> @@ -295,4 +295,29 @@ test_expect_success 'reading from .gitattributes in a subdirectory (3)' '
> test_cmp expect actual
> '
>
> +test_expect_success 'pathspec with builtin_objectmode attr can be used' '
> + >mode_exec_file_1 &&
> +
> + git status -s ":(attr:builtin_objectmode=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:builtin_objectmode=100755)mode_exec_*" >actual &&
> + echo AM mode_exec_file_1 >expect &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'builtin_objectmode attr can be excluded' '
> + >mode_1_regular &&
> + >mode_1_exec && chmod +x mode_1_exec &&
> + git status -s ":(exclude,attr:builtin_objectmode=100644)" "mode_1_*" >actual &&
> + echo ?? mode_1_exec >expect &&
> + test_cmp expect actual &&
> +
> + git status -s ":(exclude,attr:builtin_objectmode=100755)" "mode_1_*" >actual &&
> + echo ?? mode_1_regular >expect &&
> + test_cmp expect actual
> +'
> +
> test_done
^ permalink raw reply
* Re: [PATCH v5 0/3] rebase: support --autosquash without -i
From: Junio C Hamano @ 2023-11-16 0:27 UTC (permalink / raw)
To: Phillip Wood; +Cc: Andy Koppe, git, phillip.wood
In-Reply-To: <eb62435f-08ca-494d-bcc7-2568df2bd7fd@gmail.com>
Phillip Wood <phillip.wood123@gmail.com> writes:
> Hi Andy
> ...
> Thanks for the re-roll this version looks good to me
>
> Best Wishes
>
> Phillip
Yup, looks good. Thanks, both.
Queued.
>
>> Andy Koppe (3):
>> rebase: fully ignore rebase.autoSquash without -i
>> rebase: support --autosquash 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(-)
>>
^ permalink raw reply
* Re: [PATCH] commit-graph: disable GIT_COMMIT_GRAPH_PARANOIA by default
From: Junio C Hamano @ 2023-11-16 0:07 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Jeff King, git, Karthik Nayak
In-Reply-To: <ZVTJFOSnVonoPgZk@tanuki>
Patrick Steinhardt <ps@pks.im> writes:
>> Yeah. Just like we auto-enabled GIT_REF_PARANOIA for git-gc, etc, I
>> think we should do the same here.
>
> I'm honestly still torn on this one. There are two cases that I can
> think of where missing objects would be benign and where one wants to
> use `git rev-list --missing`:
>
> - Repositories with promisor remotes, to find the boundary of where
> we need to fetch new objects.
>
> - Quarantine directories where you only intend to list new objects
> or find the boundary.
>
> And in neither of those cases I can see a path for how the commit-graph
> would contain such missing commits when using regular tooling to perform
> repository maintenance.
I can buy the promisor remotes use case---we may expect boundary
objects missing without any repository corruption. I do not know
about the other one---where does our "rev-list --missing" start
traversing in a quarantine directory is unclear. Objects that are
still in quarantine are not (yet) made reachable from any refs, so
even "rev-list --missing --all" would not make a useful traversal,
no?
In any case, it sounds like you are not torn but are convinced that
we should leave this loose by default ;-) and after thinking it over
again, I tend to agree that it would be a better choice, as long as
the feature "rev-list --missing" has any good use case other than
corruption in repository.
So,... thanks for pushing back.
^ permalink raw reply
* What is the best way to fix divergent branches during push
From: Mun Johl @ 2023-11-15 23:43 UTC (permalink / raw)
To: git@vger.kernel.org
Hi all,
Our team recently moved from SVN to git and occasionally we get our workspace in a snarl. I could use some advice on the best way to resolve our issue.
In general, this is what happens:
1. git pull
2. bunch of edits which could last hours to days
3. git add
4. git commit
4a. sometimes there is a delay here before the next step because the user forgets a push is needed; but that is not always the case and the end results are the same
5. git push
At this point we have divergent branches. Not surprising. However, some users say no matter if the try a pull with ff-only, merge, or rebase that they get errors from git (sorry, I don't have the exact error text--but I hear 'git pull' continues to complain about divergent branches).
What is the recommended method to correct the divergent branches issue so that the 'git push' can proceed successfully?
Are the steps outlined above incorrect? Should we insert the following steps:
2.a git stash
2.b git fetch
2.c git pull
2.d git stash pop
I think the added steps may minimize the probability of divergent branches, but it is certainly not ensuring divergent branches will not occur.
Any advice in this regard would be greatly appreciated.
Best regards,
--
Mun
^ permalink raw reply
* Re: [PATCH v2 0/5] Avoid hang if curl needs eof twice + minor related improvements
From: Jonathan Tan @ 2023-11-15 23:28 UTC (permalink / raw)
To: Jiří Hruška; +Cc: Jonathan Tan, git, Jeff King, Junio C Hamano
In-Reply-To: <CAGE_+C4mUw8U6YK0m6hRvcqriv4pWdsELpyRJBCY-LrdHjWwgw@mail.gmail.com>
Jiří Hruška <jirka@fud.cz> writes:
> Proposed changes split into several commits for clarity
>
> Jiri Hruska (5):
> remote-curl: avoid hang if curl asks for more data after eof
I've already reviewed this [1] so I'll summarize what I think of the
rest.
[1] https://lore.kernel.org/git/20231115192027.2468887-1-jonathantanmy@google.com/
> remote-curl: improve readability of curl callbacks
> remote-curl: simplify rpc_out() - remove superfluous ifs
> remote-curl: simplify rpc_out() - less nesting and rename
> http: reset CURLOPT_POSTFIELDSIZE_LARGE between requests
Overall I can see how all of these make the code clearer, but in a
long-lived project like Git where it is very common to look at code
history to try to see why something was written the way it is, I'm a
bit reluctant to include 2/5 and 4/5. I think 3/5 (removes an "if"),
the part of 4/5 where we set "rpc.pos = 0;", and 5/5 (sets a parameter
that one could expect to be set) have significant benefit and should be
included, though. Having said that, I don't have strong opposition to
including all of them.
^ permalink raw reply
* Re: [PATCH] ci: avoid running the test suite _twice_
From: Josh Steadmon @ 2023-11-15 21:28 UTC (permalink / raw)
To: Jeff King
Cc: Johannes Schindelin via GitGitGadget, Phillip Wood, git,
Johannes Schindelin
In-Reply-To: <20231113184909.GB3838361@coredump.intra.peff.net>
On 2023.11.13 13:49, Jeff King wrote:
> On Mon, Nov 13, 2023 at 05:00:37PM +0000, Johannes Schindelin via GitGitGadget wrote:
>
> > This is a late amendment of 19ec39aab54 (ci: stop linking the `prove`
> > cache, 2022-07-10), fixing a bug that had been hidden so far.
>
> We don't seem to have that commit in Junio's tree; it is only in
> git-for-windows.
>
> Not that we should not fix things if they are broken, but I am trying
> to understand if git/git is experiencing the same bug. It sounds like
> not yet, though from looking at 19ec39aab54, I would expect to get these
> doubled runs any time we store the prove state. But maybe without that
> commit our state-file symlink is going somewhere invalid, and prove
> fails to actually store anything?
>
> > But starting with that commit, we run `prove` _twice_ in CI, and with
> > completely different sets of tests to run. Due to the bug, the second
> > invocation re-runs all of the tests that were already run as part of the
> > first invocation. This not only wastes build minutes, it also frequently
> > causes the `osx-*` jobs to fail because they already take a long time
> > and now are likely to run into a timeout.
> >
> > The worst part about it is that there is actually no benefit to keep
> > running with `--state=slow,save`, ever since we decided no longer to
> > try to reuse the Prove cache between CI runs.
> >
> > So let's just drop that Prove option and live happily ever after.
>
> Yes, I think this is the right thing to do regardless. If we are not
> saving the state to use between two related runs, there is no point
> storing it in the first place.
>
> I do have to wonder, though, as somebody who did not follow the
> unit-test topic closely: why are the unit tests totally separate from
> the rest of the suite? I would think we'd want them run from one or more
> t/t*.sh scripts. That would make bugs like this impossible, but also:
>
> 1. They'd be run via "make test", so developers don't have to remember
> to run them separately.
>
> 2. They can be run in parallel with all of the other tests when using
> "prove -j", etc.
The first part is easy, but I don't see a good way to get both shell
tests and unit tests executing under the same `prove` process. For shell
tests, we pass `--exec '$(TEST_SHELL_PATH_SQ)'` to prove, meaning that
we use the specified shell as an interpreter for the test files. That
will not work for unit test executables.
We could bundle all the unit tests into a single shell script, but then
we lose parallelization and add hoops to jump through to determine what
breaks. Or we could autogenerate a corresponding shell script to run
each individual unit test, but that seems gross. Of course, these are
hypothetical concerns for now, since we only have a single unit test at
the moment.
There's also the issue that the shell test arguments we pass on from
prove would be shared with the unit tests. That's fine for now, as
t-strbuf doesn't accept any runtime arguments, but it's possible that
either the framework or individual unit tests might grow to need
arguments, and it might not be convenient to stay compatible with the
shell tests.
Personally, I lean towards keeping things simple and just running a
second `prove` process as part of `make test`. If I was forced to pick a
way to get everything under one process, I'd lean towards autogenerating
individual shell script wrappers for each unit test. But I'm open to
discussion, especially if people have other approaches I haven't thought
of.
^ permalink raw reply
* Re: Bug: "Received" misspelled in remote message
From: Konstantin Ryabitsev @ 2023-11-15 19:45 UTC (permalink / raw)
To: Alan Dove; +Cc: git
In-Reply-To: <4e76dda1009a8365a1d66d9594865a4af31ab418.camel@gmail.com>
On Wed, Nov 15, 2023 at 01:46:10PM -0500, Alan Dove wrote:
> Hello:
>
> Using Git 2.40.1 on a private server, I see this message whenever I
> push a commit:
>
> "remote: Recieved update on checked-out branch, queueing deployment."
>
> "Received" is misspelled.
>
> I realize this is a very minor issue, but it should also be very easy
> to correct. As someone who suffers from Proofreader's Eye, this error
> deals psychic damage to me on a daily basis.
It's most likely coming from some CI/CD hook deployed on the server where
you're pushing, so you should reach out to your IT to have the spelling fixed.
In other words, the message is not coming from any software that is managed by
the git project itself (at least, nothing matches "recieved" in the git
repository tree).
Best regards,
-K
^ permalink raw reply
* PROPOSAL
From: BPBVI @ 2023-11-15 19:37 UTC (permalink / raw)
To: git
Hello,
Attached is a contract document for your review. Revert accordingly.
Thank You
Felix H
Project Manager
BPOPULARBVI
^ permalink raw reply
* Re: Bug: "Received" misspelled in remote message
From: Eric Wong @ 2023-11-15 19:26 UTC (permalink / raw)
To: Alan Dove; +Cc: git
In-Reply-To: <4e76dda1009a8365a1d66d9594865a4af31ab418.camel@gmail.com>
Alan Dove <alan.dove@gmail.com> wrote:
> Hello:
>
> Using Git 2.40.1 on a private server, I see this message whenever I
> push a commit:
>
> "remote: Recieved update on checked-out branch, queueing deployment."
Hi Alan, that's likely from a hook on the server itself.
Those phrases aren't a part of git at all.
^ permalink raw reply
* Re: [PATCH v2 1/5] remote-curl: avoid hang if curl asks for more data after eof
From: Jonathan Tan @ 2023-11-15 19:20 UTC (permalink / raw)
To: Jiří Hruška; +Cc: Jonathan Tan, git, Jeff King, Junio C Hamano
In-Reply-To: <CAGE_+C4JWA0DrcV4rT7J6hXsbYPL2Po31wFPvLESe_sKq_16FQ@mail.gmail.com>
Jiří Hruška <jirka@fud.cz> writes:
> It has been observed that under some circumstances, libcurl can call
> our `CURLOPT_READFUNCTION` callback `rpc_out()` again even after
> already getting a return value of zero (EOF) back once before.
>
> Because `rpc->flush_read_but_not_sent` is reset to false immediately
> the first time an EOF is returned, the repeated call goes again to
> `rpc_read_from_out()`, which tries to read more from the child process
> pipe and the whole operation gets stuck - the child process is already
> trying to read a response back and will not write anything to the
> output pipe anymore, while the parent/remote process is now blocked
> waiting to read more too and never even finishes sending the request.
>
> The bug is fixed by moving the reset of the `flush_read_but_not_sent`
> flag to `post_rpc()`, only before `rpc_out()` would be potentially used
> the next time. This makes the callback behave like fread() and return
> a zero any number of times at the end of a finished upload.
>
> Signed-off-by: Jiri Hruska <jirka@fud.cz>
Thanks for splitting this out - this makes it much easier to review. I
can see that the patch indeed works, but some things need to be clearer
(described below).
In the commit message, I think we should add a historical note,
something like:
`flush_read_but_not_sent` was introduced in a97d00799a (remote-curl:
use post_rpc() for protocol v2 also, 2019-02-21), which needed a way to
indicate that Git should not read from a certain child process anymore
once "flush" has been received from the child process. This field was
scoped to what was believed the minimum necessary - the interval between
"flush" being received and EOF being reported to Curl.
However, as described at the beginning of the commit message, we need
the scope of this to be greater than that - in case Curl continues to
ask us for more data, we still need to remember that "flush" has been
received. Therefore, change this field from `flush_read_but_not_sent` to
`flush_read`, which both is simpler and is a fix for this bug.
Feel free to use this verbatim, or adapt it as you wish (or write your
own).
As described in the historical note, I also think we need to change the
name of this field, since we are changing its semantics.
I am trying to be better at indicating when I think a prefix subset of
a patch set is worth merging on its own (so that, if some patches in
a patch set are good and some still need to be discussed, we have the
option of merging a prefix of a patch set). So, assuming my comments are
addressed, I think this patch is good to go in on its own, even if we
don't want some of the later patches. I'll review the rest of this patch
set later.
^ permalink raw reply
* [PATCH v2] RelNotes: tweak 2.43.0 release notes
From: Andy Koppe @ 2023-11-15 19:07 UTC (permalink / raw)
To: git; +Cc: Andy Koppe
In-Reply-To: <20231114223127.11292-1-andy.koppe@gmail.com>
Add some more detail on the $(decorate) log format placeholder and tweak
the wording on some other points.
Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
---
Now with 100% more signature. Sorry for the unnecessary noise.
Documentation/RelNotes/2.43.0.txt | 20 +++++++++++---------
1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/Documentation/RelNotes/2.43.0.txt b/Documentation/RelNotes/2.43.0.txt
index 770543c464e..9a68074a4a5 100644
--- a/Documentation/RelNotes/2.43.0.txt
+++ b/Documentation/RelNotes/2.43.0.txt
@@ -10,8 +10,8 @@ Backward Compatibility Notes
prefix. If you are negatively affected by this change, please use
"--subject-prefix=PATCH --rfc" as a replacement.
- * "git rev-list --stdin" learned to take non-revisions (like "--not")
- recently from the standard input, but the way such a "--not" was
+ * "git rev-list --stdin" recently learned to take non-revisions (like
+ "--not") from the standard input, but the way such a "--not" was
handled was quite confusing, which has been rethought. The updated
rule is that "--not" given from the command line only affects revs
given from the command line that comes but not revs read from the
@@ -43,10 +43,10 @@ UI, Workflows & Features
* Git GUI updates.
- * "git format-patch" learns a way to feed cover letter description,
- that (1) can be used on detached HEAD where there is no branch
- description available, and (2) also can override the branch
- description if there is one.
+ * "git format-patch" learns option "--description-file" to feed in a
+ cover letter description that can be used when no branch description
+ is available, or that can override the branch description if there is
+ one.
* Use of --max-pack-size to allow multiple packfiles to be created is
now supported even when we are sending unreachable objects to cruft
@@ -56,7 +56,9 @@ UI, Workflows & Features
"--subject-prefix" option and used "[RFC PATCH]"; now we will add
"RFC" prefix to whatever subject prefix is specified.
- * "git log --format" has been taught the %(decorate) placeholder.
+ * "git log" format strings now support a "%(decorate)" placeholder that
+ can be used to customize the symbols and the tag prefix used in ref
+ decorations.
* The default log message created by "git revert", when reverting a
commit that records a revert, has been tweaked, to encourage people
@@ -99,7 +101,7 @@ UI, Workflows & Features
* The attribute subsystem learned to honor `attr.tree` configuration
that specifies which tree to read the .gitattributes files from.
- * "git merge-file" learns a mode to read three contents to be merged
+ * "git merge-file" learns a mode to read three files to be merged
from blob objects.
@@ -127,7 +129,7 @@ Performance, Internal Implementation, Development Support etc.
* The code to keep track of existing packs in the repository while
repacking has been refactored.
- * The "streaming" interface used for bulk-checkin codepath has been
+ * The "streaming" interface used for the bulk-checkin codepath has been
narrowed to take only blob objects for now, with no real loss of
functionality.
--
2.43.0-rc1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox