* Re: [PATCH v2 5/7] bulk-checkin: introduce `index_blob_bulk_checkin_incore()`
From: Junio C Hamano @ 2023-10-18 2:18 UTC (permalink / raw)
To: Taylor Blau
Cc: git, Elijah Newren, Eric W. Biederman, Jeff King,
Patrick Steinhardt
In-Reply-To: <239bf39bfb21ef621a15839bade34446dcbc3103.1697560266.git.me@ttaylorr.com>
Taylor Blau <me@ttaylorr.com> writes:
> bulk-checkin.c | 118 +++++++++++++++++++++++++++++++++++++++++++++++++
> bulk-checkin.h | 4 ++
> 2 files changed, 122 insertions(+)
Unlike the previous four, which were very clear refactoring to
create reusable helper functions, this step leaves a bad aftertaste
after reading twice, and I think what is disturbing is that many new
lines are pretty much literally copied from stream_blob_to_pack().
I wonder if we can introduce an "input" source abstraction, that
replaces "fd" and "size" (and "path" for error reporting) parameters
to the stream_blob_to_pack(), so that the bulk of the implementation
of stream_blob_to_pack() can call its .read() method to read bytes
up to "size" from such an abstracted interface? That would be a
good sized first half of this change. Then in the second half, you
can add another "input" source that works with in-core "buf" and
"size", whose .read() method will merely be a memcpy().
That way, we will have two functions, one for stream_obj_to_pack()
that reads from an open file descriptor, and the other for
stream_obj_to_pack_incore() that reads from an in-core buffer,
sharing the bulk of the implementation that is extracted from the
current code, which hopefully be easier to audit.
> diff --git a/bulk-checkin.c b/bulk-checkin.c
> index f4914fb6d1..25cd1ffa25 100644
> --- a/bulk-checkin.c
> +++ b/bulk-checkin.c
> @@ -140,6 +140,69 @@ static int already_written(struct bulk_checkin_packfile *state, struct object_id
> return 0;
> }
>
> +static int stream_obj_to_pack_incore(struct bulk_checkin_packfile *state,
> + git_hash_ctx *ctx,
> + off_t *already_hashed_to,
> + const void *buf, size_t size,
> + enum object_type type,
> + const char *path, unsigned flags)
> +{
> + git_zstream s;
> + unsigned char obuf[16384];
> + unsigned hdrlen;
> + int status = Z_OK;
> + int write_object = (flags & HASH_WRITE_OBJECT);
> +
> + git_deflate_init(&s, pack_compression_level);
> +
> + hdrlen = encode_in_pack_object_header(obuf, sizeof(obuf), type, size);
> + s.next_out = obuf + hdrlen;
> + s.avail_out = sizeof(obuf) - hdrlen;
> +
> + if (*already_hashed_to < size) {
> + size_t hsize = size - *already_hashed_to;
> + if (hsize) {
> + the_hash_algo->update_fn(ctx, buf, hsize);
> + }
> + *already_hashed_to = size;
> + }
> + s.next_in = (void *)buf;
> + s.avail_in = size;
> +
> + while (status != Z_STREAM_END) {
> + status = git_deflate(&s, Z_FINISH);
> + if (!s.avail_out || status == Z_STREAM_END) {
> + if (write_object) {
> + size_t written = s.next_out - obuf;
> +
> + /* would we bust the size limit? */
> + if (state->nr_written &&
> + pack_size_limit_cfg &&
> + pack_size_limit_cfg < state->offset + written) {
> + git_deflate_abort(&s);
> + return -1;
> + }
> +
> + hashwrite(state->f, obuf, written);
> + state->offset += written;
> + }
> + s.next_out = obuf;
> + s.avail_out = sizeof(obuf);
> + }
> +
> + switch (status) {
> + case Z_OK:
> + case Z_BUF_ERROR:
> + case Z_STREAM_END:
> + continue;
> + default:
> + die("unexpected deflate failure: %d", status);
> + }
> + }
> + git_deflate_end(&s);
> + return 0;
> +}
> +
> /*
> * Read the contents from fd for size bytes, streaming it to the
> * packfile in state while updating the hash in ctx. Signal a failure
> @@ -316,6 +379,50 @@ static void finalize_checkpoint(struct bulk_checkin_packfile *state,
> }
> }
>
> +static int deflate_obj_contents_to_pack_incore(struct bulk_checkin_packfile *state,
> + git_hash_ctx *ctx,
> + struct hashfile_checkpoint *checkpoint,
> + struct object_id *result_oid,
> + const void *buf, size_t size,
> + enum object_type type,
> + const char *path, unsigned flags)
> +{
> + struct pack_idx_entry *idx = NULL;
> + off_t already_hashed_to = 0;
> +
> + /* Note: idx is non-NULL when we are writing */
> + if (flags & HASH_WRITE_OBJECT)
> + CALLOC_ARRAY(idx, 1);
> +
> + while (1) {
> + prepare_checkpoint(state, checkpoint, idx, flags);
> + if (!stream_obj_to_pack_incore(state, ctx, &already_hashed_to,
> + buf, size, type, path, flags))
> + break;
> + truncate_checkpoint(state, checkpoint, idx);
> + }
> +
> + finalize_checkpoint(state, ctx, checkpoint, idx, result_oid);
> +
> + return 0;
> +}
> +
> +static int deflate_blob_to_pack_incore(struct bulk_checkin_packfile *state,
> + struct object_id *result_oid,
> + const void *buf, size_t size,
> + const char *path, unsigned flags)
> +{
> + git_hash_ctx ctx;
> + struct hashfile_checkpoint checkpoint = {0};
> +
> + format_object_header_hash(the_hash_algo, &ctx, &checkpoint, OBJ_BLOB,
> + size);
> +
> + return deflate_obj_contents_to_pack_incore(state, &ctx, &checkpoint,
> + result_oid, buf, size,
> + OBJ_BLOB, path, flags);
> +}
> +
> static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
> struct object_id *result_oid,
> int fd, size_t size,
> @@ -396,6 +503,17 @@ int index_blob_bulk_checkin(struct object_id *oid,
> return status;
> }
>
> +int index_blob_bulk_checkin_incore(struct object_id *oid,
> + const void *buf, size_t size,
> + const char *path, unsigned flags)
> +{
> + int status = deflate_blob_to_pack_incore(&bulk_checkin_packfile, oid,
> + buf, size, path, flags);
> + if (!odb_transaction_nesting)
> + flush_bulk_checkin_packfile(&bulk_checkin_packfile);
> + return status;
> +}
> +
> void begin_odb_transaction(void)
> {
> odb_transaction_nesting += 1;
> diff --git a/bulk-checkin.h b/bulk-checkin.h
> index aa7286a7b3..1b91daeaee 100644
> --- a/bulk-checkin.h
> +++ b/bulk-checkin.h
> @@ -13,6 +13,10 @@ int index_blob_bulk_checkin(struct object_id *oid,
> int fd, size_t size,
> const char *path, unsigned flags);
>
> +int index_blob_bulk_checkin_incore(struct object_id *oid,
> + const void *buf, size_t size,
> + const char *path, unsigned flags);
> +
> /*
> * Tell the object database to optimize for adding
> * multiple objects. end_odb_transaction must be called
^ permalink raw reply
* Re: Is there any interest in localizing term delimiters in git messages?
From: Jiang Xin @ 2023-10-18 2:01 UTC (permalink / raw)
To: Junio C Hamano
Cc: Alexander Shopov, Git List, jmas, alexhenrie24, ralf.thielow,
matthias.ruester, phillip.szelat, vyruss, christopher.diaz.riv,
jn.avila, flashcode, bagasdotme,
Ævar Arnfjörð Bjarmason, alessandro.menti,
elongbug, cwryu, uneedsihyeon, arek_koz, dacs.git,
insolor@gmail.com, peter, bitigchi, ark, kate,
vnwildman@gmail.com, pclouds, dyroneteng@gmail.com,
oldsharp@gmail.com, lilydjwg@gmail.com, me, pan93412@gmail.com,
franklin@goodhorse.idv.tw
In-Reply-To: <xmqqzg0gx6k9.fsf@gitster.g>
On Wed, Oct 18, 2023 at 5:50 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Alexander Shopov <ash@kambanaria.org> writes:
>
> > Typical example:
> > ORIGINAL
> > msgid " (use \"git rm --cached <file>...\" to unstage)"
> >
> > TRANSLATION
> > msgstr ""
> > " (използвайте „git rm --cached %s ФАЙЛ…“, за да извадите ФАЙЛа от индекса)"
> >
> > The important part are the `<' and `>' delimiters of the term "file"
> >
> > Instead of using them - I omit them and capitalize the term. As if `<'
> > and `>' are declared as localizable and then I translate them as `',
> > `'
>
> Is it because it is more common in your target language to omit <>
> around the placeholder word, or is it just your personal preference?
>
> Whichever is the case, I am not sure how it affects ...
>
> > So I am asking - is there any interest from other localizers to have
> > such a feature? Would the additional maintenance be OK for the
> > developers?
>
> ... the maintenance burden for developers. Perhaps I am not getting
> what you are proposing, but we are not going to change the message
> in "C" locale (the original you see in msgid). In untranslated Git,
> we will keep the convention to highlight the placeholder word by
> having <> around it, so the "(use \"git rm --cached <file>...\" to
> unstage)" message will be spelled with "<file>". You can translate
> that to a msgstr without <> markings without asking anybody's
> permission, and I do not think of a reason why it would burden
> developers to do so.
Starting with the release of git 2.34.0 two years ago, we had a new
l10n pipeline and the git-po-helper tool as part of our l10n workflow.
The first version of git-po-helper introduced a validator to protect
git command parameters and variable names in megid. E.g. In pull
request 541 (https://github.com/git-l10n/git-po/pull/541), a
mismatched variable name "new_index" was reported in bg.po as below:
level=warning msg="mismatch variable names in msgstr: new_index"
level=warning msg=">> msgid: unable to write new_index file"
level=warning msg=">> msgstr: новият индекс не може да бъде записан"
And po/bg.po changed as below:
msgid "unable to write new_index file"
msgstr "новият индекс (new_index) не може да бъде записан"
Later, more validators were introduced into git-po-helper for checking
git config name, place holders, etc. "git-po-helper" used a list of
regular expressions to find git config names, placeholders, and there
are some false positive cases need to be ignored. So I added tweaks in
smarge tables in "dict/*.go" of git-po-helper. E.g. For German
translation, there are two exceptions that need to be ignored:
"e.g." was translated to "z.B.",
"you@example.com" was translated to "ihre@emailadresse.de"
In pull request 593 (https://github.com/git-l10n/git-po/pull/593), it
was the first time I know that in Bulgarian translations, markers
around <placeholder> were not suitable for Bulgarian. So I decided to
add more tweaks for Bulgarian by adding more exception rules in
"dict/smudge-bg.go".
I wonder if Bulgarian can use some unique characters to wrap the
placeholders (e.g. Chinese can use wrappers around placeholders
like「placeholder」，【placeholder】，etc). It will be much simpler to
define exception rules for Bulgarian. Otherwize, maybe I can add
filters for validators in "po-helper", and Bulgarian can bypass some
validators to suppress warnings in pull requests.
--
Jiang Xin
^ permalink raw reply
* Re: [PATCH v2 2/2] Prevent git from rehashing 4GiB files
From: brian m. carlson @ 2023-10-18 0:42 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, Taylor Blau, Jason Hatton
In-Reply-To: <20231017000019.GB551672@coredump.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 2403 bytes --]
On 2023-10-17 at 00:00:19, Jeff King wrote:
> On Thu, Oct 12, 2023 at 04:09:30PM +0000, brian m. carlson wrote:
>
> > +static unsigned int munge_st_size(off_t st_size) {
> > + unsigned int sd_size = st_size;
> > +
> > + /*
> > + * If the file is an exact multiple of 4 GiB, modify the value so it
> > + * doesn't get marked as racily clean (zero).
> > + */
> > + if (!sd_size && st_size)
> > + return 0x80000000;
> > + else
> > + return sd_size;
> > +}
>
> Coverity complained that the "true" side of this conditional is
> unreachable, since sd_size is assigned from st_size, so the two values
> cannot be both true and false. But obviously we are depending here on
> the truncation of off_t to "unsigned int". I'm not sure if Coverity is
> just dumb, or if it somehow has a different size for off_t.
Technically, on 32-bit machines, you can have a 32-bit off_t if you
don't compile with -D_FILE_OFFSET_BITS=64. However, such a program is
not very useful on modern systems, since it wouldn't be able to handle
files that are 2 GiB or larger, and so everyone considers that a silly
and buggy way to compile programs.
> I don't _think_ this would ever cause confusion in a real compiler, as
> assignment from a larger type to a smaller has well-defined truncation,
> as far as I know.
>
> But I do wonder if an explicit "& 0xFFFFFFFF" would make it more obvious
> what is happening (which would also do the right thing if in some
> hypothetical platform "unsigned int" ended up larger than 32 bits).
Such a hypothetical platform is of course allowed by the C standard, but
it's also going to run near zero real Unix programs or kernels. I am at
this point reflecting on the prudent decision Rust made to make
almost everything an explicit bit size (e.g., u32, i64).
Nonetheless, I'll reroll this with the other things you mentioned, and
I'll switch that "unsigned int" above to "uint32_t", which I think makes
this more obvious.
I don't, however, intend to change the constant from 0x8000000 to 1,
because I think that's a poorer choice, but I'll try to explain it
better in the commit message. (I had originally aimed to avoid editing
it as much as possible since it's not my name on the commit to avoid
Jason getting blamed unnecessarily for any mistakes I might make.)
--
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]
^ permalink raw reply
* Re: Supporting `git add -a <exclude submodules>`
From: Junio C Hamano @ 2023-10-17 23:02 UTC (permalink / raw)
To: Joanna Wang; +Cc: git, Joanna Wang
In-Reply-To: <CAMmZTi8swsSMcLUcW+YwUDg8GcrY_ks2+i35-nsHE3o9MNpsUQ@mail.gmail.com>
Joanna Wang <jojwang@google.com> writes:
> I will dive into the code more to confirm, but that's my current
> high-level plan, in case you immediately see something very wrong.
Nothing I immediately see glaringly wrong in there ;-)
Thanks for digging!
^ permalink raw reply
* Re: Supporting `git add -a <exclude submodules>`
From: Joanna Wang @ 2023-10-17 22:48 UTC (permalink / raw)
To: gitster; +Cc: git, Joanna Wang, Joanna Wang
> (2) does collect_some_attrs() or git_check_attr() leave enough
> information in check[] to tell between [a] the attr stack had
> no opinion on "bits" attribute and [b] the attr stack
> explicitly said "bits" attribute is unspecified?
I will double check this but IIUC, I could change this part
(https://github.com/git/git/blob/master/attr.c#L1100-L1105),
such that:
```
if (n == ATTR__UNKNOWN && v = NULL):
// keep n equal to ATTR_UNKNOWN
```
And here: https://github.com/git/git/blob/master/attr.c#L1238,
if value == 'ATTR__UNKNOWN', then keep it at ATTR__UNKNOWN
> (1) where in that callchain would we intercept and insert our own
> "bits" value based on the filesystem property?
I was planning to do that somewhere around here:
https://github.com/git/git/blob/master/pathspec.c#L764-L767
and possibly combine it with adding a new match_mode (e.g.
MATCH_BITS) which would be set by parse_pathspec().
Something like:
```
else if (ATTR_UNKNOWN(value))
matched = (match_mode == MATCH_BITS &&
!strcmp(item->attr_match[i].value, get_mode(path)));
```
I will dive into the code more to confirm, but that's my current
high-level plan, in case you immediately see something very wrong.
^ permalink raw reply
* Re: [PATCH v2 1/1] [OUTREACHY] builtin/add.c: clean up die() messages
From: Junio C Hamano @ 2023-10-17 22:16 UTC (permalink / raw)
To: Naomi Ibe; +Cc: git
In-Reply-To: <20231017221323.352-1-naomi.ibeh69@gmail.com>
Naomi Ibe <naomi.ibeh69@gmail.com> writes:
> As described in the CodingGuidelines document, a single line
> message given to die() and its friends should not capitalize its
> first word, and should not add full-stop at the end.
>
> Signed-off-by: Naomi Ibe <naomi.ibeh69@gmail.com>
> ---
> builtin/add.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
Looks good. Will queue. Thanks.
^ permalink raw reply
* [PATCH v2 1/1] [OUTREACHY] builtin/add.c: clean up die() messages
From: Naomi Ibe @ 2023-10-17 22:13 UTC (permalink / raw)
To: git; +Cc: Naomi Ibe
As described in the CodingGuidelines document, a single line
message given to die() and its friends should not capitalize its
first word, and should not add full-stop at the end.
Signed-off-by: Naomi Ibe <naomi.ibeh69@gmail.com>
---
builtin/add.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/builtin/add.c b/builtin/add.c
index c27254a5cd..5126d2ede3 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -182,7 +182,7 @@ static int edit_patch(int argc, const char **argv, const char *prefix)
git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
if (repo_read_index(the_repository) < 0)
- die(_("Could not read the index"));
+ die(_("could not read the index"));
repo_init_revisions(the_repository, &rev, prefix);
rev.diffopt.context = 7;
@@ -200,15 +200,15 @@ static int edit_patch(int argc, const char **argv, const char *prefix)
die(_("editing patch failed"));
if (stat(file, &st))
- die_errno(_("Could not stat '%s'"), file);
+ die_errno(_("could not stat '%s'"), file);
if (!st.st_size)
- die(_("Empty patch. Aborted."));
+ die(_("empty patch. aborted"));
child.git_cmd = 1;
strvec_pushl(&child.args, "apply", "--recount", "--cached", file,
NULL);
if (run_command(&child))
- die(_("Could not apply '%s'"), file);
+ die(_("could not apply '%s'"), file);
unlink(file);
free(file);
@@ -568,7 +568,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
finish:
if (write_locked_index(&the_index, &lock_file,
COMMIT_LOCK | SKIP_IF_UNCHANGED))
- die(_("Unable to write new index file"));
+ die(_("unable to write new index file"));
dir_clear(&dir);
clear_pathspec(&pathspec);
--
2.36.1.windows.1
^ permalink raw reply related
* [PATCH v2 1/1] [OUTREACHY] add: standardize die() messages output.
From: Naomi Ibe @ 2023-10-17 22:06 UTC (permalink / raw)
To: git; +Cc: Naomi Ibe
builtin/add.c: clean up die() messages
As described in the CodingGuidelines document, a single line
message given to die() and its friends should not capitalize its
first word, and should not add full-stop at the end.
Signed-off-by: Naomi Ibe <naomi.ibeh69@gmail.com>
---
builtin/add.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/builtin/add.c b/builtin/add.c
index c27254a5cd..5126d2ede3 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -182,7 +182,7 @@ static int edit_patch(int argc, const char **argv, const char *prefix)
git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
if (repo_read_index(the_repository) < 0)
- die(_("Could not read the index"));
+ die(_("could not read the index"));
repo_init_revisions(the_repository, &rev, prefix);
rev.diffopt.context = 7;
@@ -200,15 +200,15 @@ static int edit_patch(int argc, const char **argv, const char *prefix)
die(_("editing patch failed"));
if (stat(file, &st))
- die_errno(_("Could not stat '%s'"), file);
+ die_errno(_("could not stat '%s'"), file);
if (!st.st_size)
- die(_("Empty patch. Aborted."));
+ die(_("empty patch. aborted"));
child.git_cmd = 1;
strvec_pushl(&child.args, "apply", "--recount", "--cached", file,
NULL);
if (run_command(&child))
- die(_("Could not apply '%s'"), file);
+ die(_("could not apply '%s'"), file);
unlink(file);
free(file);
@@ -568,7 +568,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
finish:
if (write_locked_index(&the_index, &lock_file,
COMMIT_LOCK | SKIP_IF_UNCHANGED))
- die(_("Unable to write new index file"));
+ die(_("unable to write new index file"));
dir_clear(&dir);
clear_pathspec(&pathspec);
--
2.36.1.windows.1
^ permalink raw reply related
* Re: Is there any interest in localizing term delimiters in git messages?
From: Junio C Hamano @ 2023-10-17 21:49 UTC (permalink / raw)
To: Alexander Shopov
Cc: Git List, jmas, alexhenrie24, ralf.thielow, matthias.ruester,
phillip.szelat, vyruss, christopher.diaz.riv, jn.avila, flashcode,
bagasdotme, Ævar Arnfjörð Bjarmason,
alessandro.menti, elongbug, cwryu, uneedsihyeon, arek_koz,
dacs.git, insolor@gmail.com, peter, bitigchi, ark, kate,
vnwildman@gmail.com, pclouds, dyroneteng@gmail.com,
oldsharp@gmail.com, lilydjwg@gmail.com, me, Xin Jiang,
pan93412@gmail.com, franklin@goodhorse.idv.tw
In-Reply-To: <CAP6f5Mmi=f4DPcFwfvEiJMdKMa0BUyZ019mc8uFXyOufgD4NjA@mail.gmail.com>
Alexander Shopov <ash@kambanaria.org> writes:
> Typical example:
> ORIGINAL
> msgid " (use \"git rm --cached <file>...\" to unstage)"
>
> TRANSLATION
> msgstr ""
> " (използвайте „git rm --cached %s ФАЙЛ…“, за да извадите ФАЙЛа от индекса)"
>
> The important part are the `<' and `>' delimiters of the term "file"
>
> Instead of using them - I omit them and capitalize the term. As if `<'
> and `>' are declared as localizable and then I translate them as `',
> `'
Is it because it is more common in your target language to omit <>
around the placeholder word, or is it just your personal preference?
Whichever is the case, I am not sure how it affects ...
> So I am asking - is there any interest from other localizers to have
> such a feature? Would the additional maintenance be OK for the
> developers?
... the maintenance burden for developers. Perhaps I am not getting
what you are proposing, but we are not going to change the message
in "C" locale (the original you see in msgid). In untranslated Git,
we will keep the convention to highlight the placeholder word by
having <> around it, so the "(use \"git rm --cached <file>...\" to
unstage)" message will be spelled with "<file>". You can translate
that to a msgstr without <> markings without asking anybody's
permission, and I do not think of a reason why it would burden
developers to do so.
As long as the target audience of your translation wants to see
<file> to be translated to ФАЙЛ without <> around the word, I do
not think there is any problem doing so. I of course am assuming
that using capitalized placeholder is the norm for all users who use
Bulgarian translated Git---if it is not some users want to see <>
around the placeholder word just like "C" locale, then you'd need to
answer your users wish first, or course, but that would not need to
concern the developers who write the "C" locale messages.
Thanks for helping Git easier to use for users with your language.
^ permalink raw reply
* Re: Supporting `git add -a <exclude submodules>`
From: Junio C Hamano @ 2023-10-17 21:36 UTC (permalink / raw)
To: Joanna Wang; +Cc: git, Joanna Wang
In-Reply-To: <CAMmZTi-xOWxpzL1SvErgri0_gwvED5Vu2NfeGVcW7jCN8gyEDQ@mail.gmail.com>
Joanna Wang <jojwang@google.com> writes:
>> choose among attr:submodule, attr:type=<what>, attr:bits=<permbits>, decide what keyword to use
> Whatever we choose, do we want to block these keywords from being used
> by folks in their .gitattributes files?
> That would break any existing usage of the keywords. Is this a concern?
> Option A: To keep things working, we could add this support for attr,
> but then always prioritize whatever is set in .gitattributes. So this
> new behavior would only be triggered if the keywords (e.g.
> "submodule", "type", "bits") aren't set in .gitattributes (or w/e list
> of attributes are being read).
Without thinking things through, I think this sounds easier to
explain to the users. I have to wonder how one would implement such
override, though. Suppose we are doing attr:bits=160000, so we ask
git_check_attr() about "bits" attribute. In collect_some_attrs(), we
will have "bits" in the check[] array. We prepare the attr_stack
and fill(), which would go grab whatever is defined in the
attributes system. We'll lstat() or do the equivalent conversion
from the tree_entry.mode to permission bits only if the attributes
system has nothing to say for that "bits" attribute. I think a few
key things that needs to be thought out are:
(1) where in that callchain would we intercept and insert our own
"bits" value based on the filesystem property?
(2) does collect_some_attrs() or git_check_attr() leave enough
information in check[] to tell between [a] the attr stack had
no opinion on "bits" attribute and [b] the attr stack
explicitly said "bits" attribute is unspecified?
^ permalink raw reply
* [PATCH v2] upload-pack: add tracing for fetches
From: Robert Coup via GitGitGadget @ 2023-10-17 21:12 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Jeff King, Robert Coup, Robert Coup
In-Reply-To: <pull.1598.git.1697040242703.gitgitgadget@gmail.com>
From: Robert Coup <robert@coup.net.nz>
Information on how users are accessing hosted repositories can be
helpful to server operators. For example, being able to broadly
differentiate between fetches and initial clones; the use of shallow
repository features; or partial clone filters.
a29263c (fetch-pack: add tracing for negotiation rounds, 2022-08-02)
added some information on have counts to fetch-pack itself to help
diagnose negotiation; but from a git-upload-pack (server) perspective,
there's no means of accessing such information without using
GIT_TRACE_PACKET to examine the protocol packets.
Improve this by emitting a Trace2 JSON event from upload-pack with
summary information on the contents of a fetch request.
* haves, wants, and want-ref counts can help determine (broadly) between
fetches and clones, and the use of single-branch, etc.
* shallow clone depth, tip counts, and deepening options.
* any partial clone filter type.
Signed-off-by: Robert Coup <robert@coup.net.nz>
---
upload-pack: add tracing for fetches
Changes since V1
================
* Don't generate the JSON event unless Trace2 is active.
* Code style fix.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1598%2Frcoup%2Frc-upload-pack-trace-info-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1598/rcoup/rc-upload-pack-trace-info-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1598
Range-diff vs v1:
1: f9157979848 ! 1: 9290d586b08 upload-pack: add tracing for fetches
@@ upload-pack.c: static int parse_have(const char *line, struct oid_array *haves)
+ struct json_writer jw = JSON_WRITER_INIT;
+
+ jw_object_begin(&jw, 0);
-+ {
-+ jw_object_intmax(&jw, "haves", data->haves.nr);
-+ jw_object_intmax(&jw, "wants", data->want_obj.nr);
-+ jw_object_intmax(&jw, "want-refs", data->wanted_refs.nr);
-+ jw_object_intmax(&jw, "depth", data->depth);
-+ jw_object_intmax(&jw, "shallows", data->shallows.nr);
-+ jw_object_bool(&jw, "deepen-since", data->deepen_since);
-+ jw_object_intmax(&jw, "deepen-not", data->deepen_not.nr);
-+ jw_object_bool(&jw, "deepen-relative", data->deepen_relative);
-+ if (data->filter_options.choice)
-+ jw_object_string(&jw, "filter", list_object_filter_config_name(data->filter_options.choice));
-+ else
-+ jw_object_null(&jw, "filter");
-+ }
++ jw_object_intmax(&jw, "haves", data->haves.nr);
++ jw_object_intmax(&jw, "wants", data->want_obj.nr);
++ jw_object_intmax(&jw, "want-refs", data->wanted_refs.nr);
++ jw_object_intmax(&jw, "depth", data->depth);
++ jw_object_intmax(&jw, "shallows", data->shallows.nr);
++ jw_object_bool(&jw, "deepen-since", data->deepen_since);
++ jw_object_intmax(&jw, "deepen-not", data->deepen_not.nr);
++ jw_object_bool(&jw, "deepen-relative", data->deepen_relative);
++ if (data->filter_options.choice)
++ jw_object_string(&jw, "filter", list_object_filter_config_name(data->filter_options.choice));
++ else
++ jw_object_null(&jw, "filter");
+ jw_end(&jw);
+
+ trace2_data_json("upload-pack", the_repository, "fetch-info", &jw);
@@ upload-pack.c: static void process_args(struct packet_reader *request,
if (request->status != PACKET_READ_FLUSH)
die(_("expected flush after fetch arguments"));
+
-+ trace2_fetch_info(data);
++ if (trace2_is_enabled())
++ trace2_fetch_info(data);
}
static int process_haves(struct upload_pack_data *data, struct oid_array *common)
t/t5500-fetch-pack.sh | 38 +++++++++++++++++++++++++++-----------
upload-pack.c | 28 ++++++++++++++++++++++++++++
2 files changed, 55 insertions(+), 11 deletions(-)
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index d18f2823d86..bb15ac34f77 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -132,13 +132,18 @@ test_expect_success 'single branch object count' '
'
test_expect_success 'single given branch clone' '
- git clone --single-branch --branch A "file://$(pwd)/." branch-a &&
- test_must_fail git --git-dir=branch-a/.git rev-parse origin/B
+ GIT_TRACE2_EVENT="$(pwd)/branch-a/trace2_event" \
+ git clone --single-branch --branch A "file://$(pwd)/." branch-a &&
+ test_must_fail git --git-dir=branch-a/.git rev-parse origin/B &&
+ grep \"fetch-info\".*\"haves\":0 branch-a/trace2_event &&
+ grep \"fetch-info\".*\"wants\":1 branch-a/trace2_event
'
test_expect_success 'clone shallow depth 1' '
- git clone --no-single-branch --depth 1 "file://$(pwd)/." shallow0 &&
- test "$(git --git-dir=shallow0/.git rev-list --count HEAD)" = 1
+ GIT_TRACE2_EVENT="$(pwd)/shallow0/trace2_event" \
+ git clone --no-single-branch --depth 1 "file://$(pwd)/." shallow0 &&
+ test "$(git --git-dir=shallow0/.git rev-list --count HEAD)" = 1 &&
+ grep \"fetch-info\".*\"depth\":1 shallow0/trace2_event
'
test_expect_success 'clone shallow depth 1 with fsck' '
@@ -235,7 +240,10 @@ test_expect_success 'add two more (part 2)' '
test_expect_success 'deepening pull in shallow repo' '
(
cd shallow &&
- git pull --depth 4 .. B
+ GIT_TRACE2_EVENT="$(pwd)/trace2_event" \
+ git pull --depth 4 .. B &&
+ grep \"fetch-info\".*\"depth\":4 trace2_event &&
+ grep \"fetch-info\".*\"shallows\":2 trace2_event
)
'
@@ -306,9 +314,12 @@ test_expect_success 'fetch --depth --no-shallow' '
test_expect_success 'turn shallow to complete repository' '
(
cd shallow &&
- git fetch --unshallow &&
+ GIT_TRACE2_EVENT="$(pwd)/trace2_event" \
+ git fetch --unshallow &&
! test -f .git/shallow &&
- git fsck --full
+ git fsck --full &&
+ grep \"fetch-info\".*\"shallows\":2 trace2_event &&
+ grep \"fetch-info\".*\"depth\":2147483647 trace2_event
)
'
@@ -826,13 +837,15 @@ test_expect_success 'clone shallow since ...' '
'
test_expect_success 'fetch shallow since ...' '
- git -C shallow11 fetch --shallow-since "200000000 +0700" origin &&
+ GIT_TRACE2_EVENT=$(pwd)/shallow11/trace2_event \
+ git -C shallow11 fetch --shallow-since "200000000 +0700" origin &&
git -C shallow11 log --pretty=tformat:%s origin/main >actual &&
cat >expected <<-\EOF &&
three
two
EOF
- test_cmp expected actual
+ test_cmp expected actual &&
+ grep \"fetch-info\".*\"deepen-since\":true shallow11/trace2_event
'
test_expect_success 'clone shallow since selects no commits' '
@@ -987,13 +1000,16 @@ test_expect_success 'filtering by size' '
test_config -C server uploadpack.allowfilter 1 &&
test_create_repo client &&
- git -C client fetch-pack --filter=blob:limit=0 ../server HEAD &&
+ GIT_TRACE2_EVENT=$(pwd)/client/trace2_event \
+ git -C client fetch-pack --filter=blob:limit=0 ../server HEAD &&
# Ensure that object is not inadvertently fetched
commit=$(git -C server rev-parse HEAD) &&
blob=$(git hash-object server/one.t) &&
git -C client rev-list --objects --missing=allow-any "$commit" >oids &&
- ! grep "$blob" oids
+ ! grep "$blob" oids &&
+
+ grep \"fetch-info\".*\"filter\":\"blob:limit\" client/trace2_event
'
test_expect_success 'filtering by size has no effect if support for it is not advertised' '
diff --git a/upload-pack.c b/upload-pack.c
index 83f3d2651ab..ea234ab6a45 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -33,6 +33,7 @@
#include "commit-reach.h"
#include "shallow.h"
#include "write-or-die.h"
+#include "json-writer.h"
/* Remember to update object flag allocation in object.h */
#define THEY_HAVE (1u << 11)
@@ -1552,6 +1553,30 @@ static int parse_have(const char *line, struct oid_array *haves)
return 0;
}
+static void trace2_fetch_info(struct upload_pack_data *data)
+{
+ struct json_writer jw = JSON_WRITER_INIT;
+
+ jw_object_begin(&jw, 0);
+ jw_object_intmax(&jw, "haves", data->haves.nr);
+ jw_object_intmax(&jw, "wants", data->want_obj.nr);
+ jw_object_intmax(&jw, "want-refs", data->wanted_refs.nr);
+ jw_object_intmax(&jw, "depth", data->depth);
+ jw_object_intmax(&jw, "shallows", data->shallows.nr);
+ jw_object_bool(&jw, "deepen-since", data->deepen_since);
+ jw_object_intmax(&jw, "deepen-not", data->deepen_not.nr);
+ jw_object_bool(&jw, "deepen-relative", data->deepen_relative);
+ if (data->filter_options.choice)
+ jw_object_string(&jw, "filter", list_object_filter_config_name(data->filter_options.choice));
+ else
+ jw_object_null(&jw, "filter");
+ jw_end(&jw);
+
+ trace2_data_json("upload-pack", the_repository, "fetch-info", &jw);
+
+ jw_release(&jw);
+}
+
static void process_args(struct packet_reader *request,
struct upload_pack_data *data)
{
@@ -1640,6 +1665,9 @@ static void process_args(struct packet_reader *request,
if (request->status != PACKET_READ_FLUSH)
die(_("expected flush after fetch arguments"));
+
+ if (trace2_is_enabled())
+ trace2_fetch_info(data);
}
static int process_haves(struct upload_pack_data *data, struct oid_array *common)
base-commit: aab89be2eb6ca51eefeb8c8066f673f447058856
--
gitgitgadget
^ permalink raw reply related
* Is there any interest in localizing term delimiters in git messages?
From: Alexander Shopov @ 2023-10-17 21:09 UTC (permalink / raw)
To: Git List
Cc: jmas, alexhenrie24, ralf.thielow, matthias.ruester,
phillip.szelat, vyruss, christopher.diaz.riv, jn.avila, flashcode,
bagasdotme, Ævar Arnfjörð Bjarmason,
alessandro.menti, elongbug, cwryu, uneedsihyeon, arek_koz,
dacs.git, insolor@gmail.com, peter, bitigchi, ark, kate,
vnwildman@gmail.com, pclouds, dyroneteng@gmail.com,
oldsharp@gmail.com, lilydjwg@gmail.com, me, Xin Jiang,
pan93412@gmail.com, franklin@goodhorse.idv.tw, Junio C Hamano
Hello all,
Is there any interest in being able to change the delimiters of the
changeable terms in git messages?
Typical example:
ORIGINAL
msgid " (use \"git rm --cached <file>...\" to unstage)"
TRANSLATION
msgstr ""
" (използвайте „git rm --cached %s ФАЙЛ…“, за да извадите ФАЙЛа от индекса)"
The important part are the `<' and `>' delimiters of the term "file"
Instead of using them - I omit them and capitalize the term. As if `<'
and `>' are declared as localizable and then I translate them as `',
`'
This has the following benefits:
1. The translation gets shorter
2. We skip potentially dangerous shell characters (<> redirect IN/OUT)
3. Readability improves for some strings, ex:
- git pack-objects [<options>] <base-name> [< <ref-list> | < <object-list>]
- git mailinfo [<options>] <msg> <patch> < mail >info
On the other hand - this can increase the maintenance burden of
messages and tests and the shortening benefit is applicable to
languages using capitalization or some other form of letter changing
that preserves readability (I understand there are many languages with
lots of speakers that are not like that). They might decide to convey
`<' and `>' as `«', `»' to get benefits 2 and 3.
So I am asking - is there any interest from other localizers to have
such a feature? Would the additional maintenance be OK for the
developers?
It is possible that no one besides me is interested in this - in which
case I will rework the Bulgarian translation as:
- More and more messages containing only the term automatically add
the `<' and `>'
- I need to keep adding smudge rules to the git-po-helper tool
(https://github.com/git-l10n/git-po-helper).
Kind regards:
al_shopov
^ permalink raw reply
* Re: Supporting `git add -a <exclude submodules>`
From: Joanna Wang @ 2023-10-17 20:59 UTC (permalink / raw)
To: gitster; +Cc: git, Joanna Wang
> choose among attr:submodule, attr:type=<what>, attr:bits=<permbits>, decide what keyword to use
Whatever we choose, do we want to block these keywords from being used
by folks in their .gitattributes files?
That would break any existing usage of the keywords. Is this a concern?
Option A: To keep things working, we could add this support for attr,
but then always prioritize whatever is set in .gitattributes. So this
new behavior would only be triggered if the keywords (e.g.
"submodule", "type", "bits") aren't set in .gitattributes (or w/e list
of attributes are being read).
Option B: Or we could add this new behavior for `attr-bits:<permbits>`
or just `bits:<permbits>`.
I personally don't see big UX differences between A and B. Any opinions on this?
^ permalink raw reply
* Re: [PATCH 0/8] t7900: untangle test dependencies
From: Junio C Hamano @ 2023-10-17 20:49 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: git, stolee
In-Reply-To: <8cd788dc-7d16-4cfd-9f70-7889dcaa7199@app.fastmail.com>
"Kristoffer Haugsbakk" <code@khaugsbakk.name> writes:
> On Tue, Oct 17, 2023, at 21:59, Junio C Hamano wrote:
>> It is kind-of surprising that with only 8 patches you can reach such
>> a state, but ...
>>
>>> # The tests that used to depend on each other should still pass
>>> # when run together
>>> ./t7900-maintenance.sh --quiet --run=setup,30,31 &&
>>
>> ... this puzzles me. What does it mean for tests to "depend on each
>> other"? Does this mean running #31 with or without running #30 runs
>> under different condition and potentially run different things?
>
> What I mean is that some preceding test has a side-effect that a test
> depends on.
I see. And 31 used to depend on the side effect of having ran 30,
but in the updated test, the precondition 31 depends on is created
by itself without relying on what 30 did (and in fact, perhaps in
the updated test, 30 may rewind what it did as part of the clean-up
process using test_when_finished). That makes sense.
> I don't know what the policy is. :) My motivation was that I was working
> on something else which seemed to break the suite, then I tried to reduce
> the tests that were run to get rid of the noise (`--verbose`), but then it
> got confusing because I didn't know if I had really broken some tests
> myself or if more tests would start failing by only running a subset of
> them.
Yeah, it is a laudable goal, but I am not sure how practical it is
to expect developers to maintain that propertly. Unless there is
some automated test to enforce the independence of the tests, that
is.
Thanks.
^ permalink raw reply
* Re: [PATCH] grep: die gracefully when outside repository
From: Junio C Hamano @ 2023-10-17 20:39 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: git, ks1322
In-Reply-To: <087c92e3904dd774f672373727c300bf7f5f6369.1697317276.git.code@khaugsbakk.name>
Kristoffer Haugsbakk <code@khaugsbakk.name> writes:
> diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
> index 39d6d713ecb..b976f81a166 100755
> --- a/t/t7810-grep.sh
> +++ b/t/t7810-grep.sh
> @@ -1234,6 +1234,19 @@ test_expect_success 'outside of git repository with fallbackToNoIndex' '
> )
> '
>
> +test_expect_success 'outside of git repository with pathspec outside the directory tree' '
> + test_when_finished rm -fr non &&
> + rm -fr non &&
> + mkdir -p non/git/sub &&
> + (
> + GIT_CEILING_DIRECTORIES="$(pwd)/non" &&
> + export GIT_CEILING_DIRECTORIES &&
> + cd non/git &&
> + test_expect_code 128 git grep --no-index search .. 2>error &&
> + grep "is outside the directory tree" error
> + )
> +'
> +
So you create non/git/sub, go to non/git (so there is sub/ directory),
and try running "..".
If you had a directory non/tig next to non/git and used ../tig
instead of .. as the path given to "git grep", it would also
correctly fail. Searching in a non-existing path, ../non, dies in a
different error, with an error message that is not technically
wrong, but it probably can be improved. It has been a while since I
looked at the pathspec matching code, but if we are lucky, it might
be just the matter of swapping the order of checking (in other
words, check "is it outside" first and then "does it exist" next, or
something like that)?
t/t7810-grep.sh | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git c/t/t7810-grep.sh w/t/t7810-grep.sh
index b976f81a16..84838c0fe1 100755
--- c/t/t7810-grep.sh
+++ w/t/t7810-grep.sh
@@ -1234,16 +1234,30 @@ test_expect_success 'outside of git repository with fallbackToNoIndex' '
)
'
-test_expect_success 'outside of git repository with pathspec outside the directory tree' '
+test_expect_success 'no repository with path outside $cwd' '
test_when_finished rm -fr non &&
rm -fr non &&
- mkdir -p non/git/sub &&
+ mkdir -p non/git/sub non/tig &&
(
GIT_CEILING_DIRECTORIES="$(pwd)/non" &&
export GIT_CEILING_DIRECTORIES &&
cd non/git &&
test_expect_code 128 git grep --no-index search .. 2>error &&
grep "is outside the directory tree" error
+ ) &&
+ (
+ GIT_CEILING_DIRECTORIES="$(pwd)/non" &&
+ export GIT_CEILING_DIRECTORIES &&
+ cd non/git &&
+ test_expect_code 128 git grep --no-index search ../tig 2>error &&
+ grep "is outside the directory tree" error
+ ) &&
+ (
+ GIT_CEILING_DIRECTORIES="$(pwd)/non" &&
+ export GIT_CEILING_DIRECTORIES &&
+ cd non/git &&
+ test_expect_code 128 git grep --no-index search ../non 2>error &&
+ grep "no such path in the working tree" error
)
'
^ permalink raw reply related
* Re: [PATCH] grep: die gracefully when outside repository
From: Junio C Hamano @ 2023-10-17 20:25 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: git, ks1322, peff
In-Reply-To: <f8a2abc0f610912af3eb56536ed217b8f90db2f9.1697571664.git.code@khaugsbakk.name>
Kristoffer Haugsbakk <code@khaugsbakk.name> writes:
> On Tue, Oct 17, 2023, at 18:42, Junio C Hamano wrote:
>> It is curious that the original has two sources of hint_path (i.e.,
>> get_git_dir() is used as a fallback for get_git_work_tree()). Are
>> we certain that the check is at the right place? If we do not have
>> a repository, then both would fail by returning NULL, so it should
>> not matter if we add the new check before we check either or both,
>> or even after we checked both before dying.
>>
>> I wonder if
>>
>> const char *hint_path = get_git_work_tree();
>>
>> if (!hint_path)
>> hint_path = get_git_dir();
>> if (hint_path)
>> die(_("%s: '%s' is outside repository at '%s'"),
>> elt, copyfrom, absolute_path(hint_path));
>> else
>> die(_("%s: '%s' is outside the directory tree"),
>> elt, copyfrom);
>>
>> makes the intent of the code clearer.
>
> That doesn't work since `get_git_dir()` triggers `BUG` instead of
> returning `NULL`.
Ah, interesting.
> The `hint_path` declaration has to be at the start because of style
> rules. But we can initialize it after.
Yes, what you have below (but please leave a blank line between the
last line of decl and the first line of statement for readablility)
looks very readable and sensible.
> I can also have a second look at the test since I am using `grep` to
> test the failure output and not the translation string variant.
That is not necessary, as we no longer run under phoney i18n that
required us to use test_i18ngrep. It is OK to assume that the tests
are run under "C" locale.
Thanks.
> -- >8 --
> Subject: [PATCH] fixup! grep: die gracefully when outside repository
>
> ---
> pathspec.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/pathspec.c b/pathspec.c
> index e115832f17a..0c1061fad11 100644
> --- a/pathspec.c
> +++ b/pathspec.c
> @@ -467,10 +467,11 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,
> match = prefix_path_gently(prefix, prefixlen,
> &prefixlen, copyfrom);
> if (!match) {
> - const char *hint_path = get_git_work_tree();
> + const char *hint_path;
> if (!have_git_dir())
> die(_("'%s' is outside the directory tree"),
> copyfrom);
> + hint_path = get_git_work_tree();
> if (!hint_path)
> hint_path = get_git_dir();
> die(_("%s: '%s' is outside repository at '%s'"), elt,
^ permalink raw reply
* Re: none
From: Junio C Hamano @ 2023-10-17 20:21 UTC (permalink / raw)
To: Dorcas Litunya; +Cc: christian.couder, git
In-Reply-To: <ZS2ESFGP2H3CTJSK@dorcaslitunya-virtual-machine>
Dorcas Litunya <anonolitunya@gmail.com> writes:
> Bcc:
> Subject: Re: [PATCH] t/t7601: Modernize test scripts using functions
> Reply-To:
> In-Reply-To: <xmqq1qdumrto.fsf@gitster.g>
What are these lines doing here?
> So should I replace this in the next version or leave this as is?
Perhaps I was not clear enough, but I found the commit title and
description need to be updated to clearly record the intent of the
change with a handful of points, so I will not be accepting the
patch as-is.
These two sections may be of help.
Documentation/MyFirstContribution.txt::now-what
Documentation/MyFirstContribution.txt::reviewing
Thanks.
^ permalink raw reply
* Re: [PATCH 2/8] t7900: setup and tear down clones
From: Kristoffer Haugsbakk @ 2023-10-17 20:20 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, stolee
In-Reply-To: <xmqqwmvlgg71.fsf@gitster.g>
On Tue, Oct 17, 2023, at 22:13, Junio C Hamano wrote:
> As I already said in my response to the cover letter, while I am
> surprised that the series managed to make each step (and it alone)
> succeed after the set-up (applaud!), I am not sure if it is really
> worth doing. As the business of test scripts is to test git, and it
> means that we should always assume that we are dealing with a
> potentially broken version of git. By running so many git
> subcommands in test_when_finished, each of them may be from a buggy
> implementation of git, we cannot be really sure that we are
> resetting the environment to the pristine state. We should strive
> to do as little as possible in test_when_finished.
I'll have to think more about this part in order to understand the
ramifications. Thanks for the feedback.
> This is even worse; it has to redo much of what the previous test
> did. Developers cannot be reasonably expected to maintain this
> duplication when we need to change the earlier test.
>
> While I am impressed that "set-up + individual single test" was made
> to work, I am not convinced that the changes that took us to get
> there are reasonable. The end result looks much less maintainable
> and more wasteful with duplicated steps.
>
> Thanks.
I can rewrite this one—as well as others—to use the `setup` keyword in the
original test instead.
But dropping the series is also fine. I am still very new to this test
suite.
Cheers
--
Kristoffer Haugsbakk
^ permalink raw reply
* Re: [PATCH 0/8] t7900: untangle test dependencies
From: Kristoffer Haugsbakk @ 2023-10-17 20:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, stolee
In-Reply-To: <xmqqbkcxhvf9.fsf@gitster.g>
On Tue, Oct 17, 2023, at 21:59, Junio C Hamano wrote:
> It is kind-of surprising that with only 8 patches you can reach such
> a state, but ...
>
>> # The tests that used to depend on each other should still pass
>> # when run together
>> ./t7900-maintenance.sh --quiet --run=setup,30,31 &&
>
> ... this puzzles me. What does it mean for tests to "depend on each
> other"? Does this mean running #31 with or without running #30 runs
> under different condition and potentially run different things?
What I mean is that some preceding test has a side-effect that a test
depends on. Or that the test depends on some test *not* having done
something; patch 9/8 changes `maintenance.auto config option` to delete
and init the repository since it depends on the preceding tests *not*
having run `git maintenance register`, since that turns off the default
`true` value of `maintenance.auto`.
(Maybe those last meta-tests with combining tests like number 30 and 31
was a bit silly.)
> One might argue that, in an ideal world, our tests should work when
> any non-setup tests are omitted (so, instead of $i above, you'll
> have an arbitrary subsequence of 1..42 and your tests still pass),
> and it may be a worthy goal, but at the same time, it may be a bit
> impractical, as setting things up is costly, but what you can do in
> the common "setup" will be very small. Or you'll have so much
> "recovering from damage" in test_when_finished for each test that
> makes such untangling of dependencies too costly.
I don't know what the policy is. :) My motivation was that I was working
on something else which seemed to break the suite, then I tried to reduce
the tests that were run to get rid of the noise (`--verbose`), but then it
got confusing because I didn't know if I had really broken some tests
myself or if more tests would start failing by only running a subset of
them.
That last patch 9/8 deals with what I discovered when I added two tests
before `maintenance.auto config option`; the test started failing, which
made me think that my changes might have some side-effect on what the test
is testing. But it was just an invisible dependency on `git maintenance
register` *not* having been run in the whole suite up until that point.
Cheers
^ permalink raw reply
* Re: [PATCH 2/8] t7900: setup and tear down clones
From: Junio C Hamano @ 2023-10-17 20:13 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: git, stolee
In-Reply-To: <e3987cda75e4db72393f85de4bbb71d2ebaa097b.1697319294.git.code@khaugsbakk.name>
Kristoffer Haugsbakk <code@khaugsbakk.name> writes:
> Test `loose-objects task` depends on the two clones setup in `prefetch
> multiple remotes`.
>
> Reuse the two clones setup and tear down the clones afterwards in both
> tests.
>
> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
> ---
> t/t7900-maintenance.sh | 22 ++++++++++++++++++++++
> 1 file changed, 22 insertions(+)
>
> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> index ca86b2ba687..ebc207f1a58 100755
> --- a/t/t7900-maintenance.sh
> +++ b/t/t7900-maintenance.sh
> @@ -145,6 +145,12 @@ test_expect_success 'run --task=prefetch with no remotes' '
> '
>
> test_expect_success 'prefetch multiple remotes' '
> + test_when_finished rm -r clone1 &&
> + test_when_finished rm -r clone2 &&
> + test_when_finished git remote remove remote1 &&
> + test_when_finished git remote remove remote2 &&
> + test_when_finished git tag --delete one &&
> + test_when_finished git tag --delete two &&
> git clone . clone1 &&
> git clone . clone2 &&
> git remote add remote1 "file://$(pwd)/clone1" &&
As I already said in my response to the cover letter, while I am
surprised that the series managed to make each step (and it alone)
succeed after the set-up (applaud!), I am not sure if it is really
worth doing. As the business of test scripts is to test git, and it
means that we should always assume that we are dealing with a
potentially broken version of git. By running so many git
subcommands in test_when_finished, each of them may be from a buggy
implementation of git, we cannot be really sure that we are
resetting the environment to the pristine state. We should strive
to do as little as possible in test_when_finished.
> @@ -175,6 +181,22 @@ test_expect_success 'prefetch multiple remotes' '
> '
>
> test_expect_success 'loose-objects task' '
> + test_when_finished rm -r clone1 &&
> + test_when_finished rm -r clone2 &&
> + test_when_finished git remote remove remote1 &&
> + test_when_finished git remote remove remote2 &&
> + test_when_finished git tag --delete one &&
> + test_when_finished git tag --delete two &&
Ditto.
> + git clone . clone1 &&
> + git clone . clone2 &&
> + git remote add remote1 "file://$(pwd)/clone1" &&
> + git remote add remote2 "file://$(pwd)/clone2" &&
> + git -C clone1 switch -c one &&
> + git -C clone2 switch -c two &&
> + test_commit -C clone1 one &&
> + test_commit -C clone2 two &&
> + git fetch --all &&
This is even worse; it has to redo much of what the previous test
did. Developers cannot be reasonably expected to maintain this
duplication when we need to change the earlier test.
While I am impressed that "set-up + individual single test" was made
to work, I am not convinced that the changes that took us to get
there are reasonable. The end result looks much less maintainable
and more wasteful with duplicated steps.
Thanks.
^ permalink raw reply
* [PATCH v3 3/3] subtree: adding test to validate fix
From: Zach FettersMoore @ 2023-10-17 20:02 UTC (permalink / raw)
To: gitgitgadget, git; +Cc: zach.fetters
In-Reply-To: <pull.1587.v3.git.1696019580.gitgitgadget@gmail.com>
From: "Zach FettersMoore via GitGitGadget" <gitgitgadget@gmail.com>
> From: Zach FettersMoore <zach.fetters@apollographql.com>
>
> Adding a test to validate that the proposed fix
> solves the issue.
>
> The test accomplishes this by checking the output
> of the split command to ensure the output from
> the progress of 'process_split_commit' function
> that represents the 'extracount' of commits
> processed does not increment.
>
> This was tested against the original functionality
> to show the test failed, and then with this fix
> to show the test passes.
>
> This illustrated that when using multiple subtrees,
> A and B, when doing a split on subtree B, the
> processing does not traverse the entire history
> of subtree A which is unnecessary and would cause
> the 'extracount' of processed commits to climb
> based on the number of commits in the history of
> subtree A.
>
> Signed-off-by: Zach FettersMoore <zach.fetters@apollographql.com>
> ---
> contrib/subtree/t/t7900-subtree.sh | 41 ++++++++++++++++++++++++++++++
> 1 file changed, 41 insertions(+)
>
> diff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh
> index 49a21dd7c9c..57c12e9f924 100755
> --- a/contrib/subtree/t/t7900-subtree.sh
> +++ b/contrib/subtree/t/t7900-subtree.sh
> @@ -385,6 +385,47 @@ test_expect_success 'split sub dir/ with --rejoin' '
> )
> '
>
> +test_expect_success 'split with multiple subtrees' '
> + subtree_test_create_repo "$test_count" &&
> + subtree_test_create_repo "$test_count/subA" &&
> + subtree_test_create_repo "$test_count/subB" &&
> + test_create_commit "$test_count" main1 &&
> + test_create_commit "$test_count/subA" subA1 &&
> + test_create_commit "$test_count/subA" subA2 &&
> + test_create_commit "$test_count/subA" subA3 &&
> + test_create_commit "$test_count/subB" subB1 &&
> + (
> + cd "$test_count" &&
> + git fetch ./subA HEAD &&
> + git subtree add --prefix=subADir FETCH_HEAD
> + ) &&
> + (
> + cd "$test_count" &&
> + git fetch ./subB HEAD &&
> + git subtree add --prefix=subBDir FETCH_HEAD
> + ) &&
> + test_create_commit "$test_count" subADir/main-subA1 &&
> + test_create_commit "$test_count" subBDir/main-subB1 &&
> + (
> + cd "$test_count" &&
> + git subtree split --prefix=subADir --squash --rejoin -m "Sub A Split 1"
> + ) &&
> + (
> + cd "$test_count" &&
> + git subtree split --prefix=subBDir --squash --rejoin -m "Sub B Split 1"
> + ) &&
> + test_create_commit "$test_count" subADir/main-subA2 &&
> + test_create_commit "$test_count" subBDir/main-subB2 &&
> + (
> + cd "$test_count" &&
> + git subtree split --prefix=subADir --squash --rejoin -m "Sub A Split 2"
> + ) &&
> + (
> + cd "$test_count" &&
> + test "$(git subtree split --prefix=subBDir --squash --rejoin -d -m "Sub B Split 1" 2>&1 | grep -w "\[1\]")" = ""
> + )
> +'
> +
> test_expect_success 'split sub dir/ with --rejoin from scratch' '
> subtree_test_create_repo "$test_count" &&
> test_create_commit "$test_count" main1 &&
> --
Checking to see if there is something else I need to do with this
to get it across the finish line, wasn't sure if something else
was needed since it's been dormant since my last push a few
weeks ago.
Thanks
^ permalink raw reply
* Re: [PATCH 0/8] t7900: untangle test dependencies
From: Junio C Hamano @ 2023-10-17 19:59 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: git, stolee
In-Reply-To: <cover.1697319294.git.code@khaugsbakk.name>
Kristoffer Haugsbakk <code@khaugsbakk.name> writes:
> #!/bin/sh
> cd t
> # Every test run together with `setup` should pass
> for i in $(seq 1 42)
> do
> ./t7900-maintenance.sh --quiet --run=setup,$i || return 1
> done &&
It is kind-of surprising that with only 8 patches you can reach such
a state, but ...
> # The tests that used to depend on each other should still pass
> # when run together
> ./t7900-maintenance.sh --quiet --run=setup,30,31 &&
... this puzzles me. What does it mean for tests to "depend on each
other"? Does this mean running #31 with or without running #30 runs
under different condition and potentially run different things?
One might argue that, in an ideal world, our tests should work when
any non-setup tests are omitted (so, instead of $i above, you'll
have an arbitrary subsequence of 1..42 and your tests still pass),
and it may be a worthy goal, but at the same time, it may be a bit
impractical, as setting things up is costly, but what you can do in
the common "setup" will be very small. Or you'll have so much
"recovering from damage" in test_when_finished for each test that
makes such untangling of dependencies too costly.
^ permalink raw reply
* Re: [PATCH] grep: die gracefully when outside repository
From: Kristoffer Haugsbakk @ 2023-10-17 19:51 UTC (permalink / raw)
To: gitster; +Cc: Kristoffer Haugsbakk, git, ks1322, peff
In-Reply-To: <xmqqmswhjj48.fsf@gitster.g>
On Tue, Oct 17, 2023, at 18:42, Junio C Hamano wrote:
> It is curious that the original has two sources of hint_path (i.e.,
> get_git_dir() is used as a fallback for get_git_work_tree()). Are
> we certain that the check is at the right place? If we do not have
> a repository, then both would fail by returning NULL, so it should
> not matter if we add the new check before we check either or both,
> or even after we checked both before dying.
>
> I wonder if
>
> const char *hint_path = get_git_work_tree();
>
> if (!hint_path)
> hint_path = get_git_dir();
> if (hint_path)
> die(_("%s: '%s' is outside repository at '%s'"),
> elt, copyfrom, absolute_path(hint_path));
> else
> die(_("%s: '%s' is outside the directory tree"),
> elt, copyfrom);
>
> makes the intent of the code clearer.
That doesn't work since `get_git_dir()` triggers `BUG` instead of
returning `NULL`.
The `hint_path` declaration has to be at the start because of style
rules. But we can initialize it after.
I can also have a second look at the test since I am using `grep` to
test the failure output and not the translation string variant.
-- >8 --
Subject: [PATCH] fixup! grep: die gracefully when outside repository
---
pathspec.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/pathspec.c b/pathspec.c
index e115832f17a..0c1061fad11 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -467,10 +467,11 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,
match = prefix_path_gently(prefix, prefixlen,
&prefixlen, copyfrom);
if (!match) {
- const char *hint_path = get_git_work_tree();
+ const char *hint_path;
if (!have_git_dir())
die(_("'%s' is outside the directory tree"),
copyfrom);
+ hint_path = get_git_work_tree();
if (!hint_path)
hint_path = get_git_dir();
die(_("%s: '%s' is outside repository at '%s'"), elt,
--
2.42.0.2.g879ad04204
^ permalink raw reply related
* Re: [PATCH v2 1/1] [OUTREACHY] add: standardize die() messages output.
From: Junio C Hamano @ 2023-10-17 19:41 UTC (permalink / raw)
To: Naomi Ibe; +Cc: git
In-Reply-To: <20231017113946.747-1-naomi.ibeh69@gmail.com>
Naomi Ibe <naomi.ibeh69@gmail.com> writes:
> builtin/add.c: clean up die() messages
>
> As described in the CodingGuidelines document, a single line
> message given to die() and its friends should not capitalize its
> first word, and should not add full-stop at the end.
>
> Signed-off-by: Naomi Ibe <naomi.ibeh69@gmail.com>
That's a strange formatting. I think you meant to make the first
line (i.e. "builtin/add.c:...") as the Subject: of the e-mail, and
the three lines of text as the body, without any indentation.
Will queue with such a fix locally to save time, but please make
sure you know what in your workflow to produce a patch and send it
out caused this strange formatting, so that you can fix it and send
your patches correctly when you need to do so for real.
Thanks.
^ permalink raw reply
* Re: [PATCH] commit: detect commits that exist in commit-graph but not in the ODB
From: Junio C Hamano @ 2023-10-17 18:34 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Karthik Nayak, Taylor Blau
In-Reply-To: <ZS4rmtBTYnp2RMiY@tanuki>
Patrick Steinhardt <ps@pks.im> writes:
> Fair point indeed. The following is a worst-case scenario benchmark of
> of the change where we do a full topological walk of all reachable
> commits in the graph, executed in linux.git. We parse commit parents via
> `repo_parse_commit_gently()`, so the new code path now basically has to
> check for object existence of every reachable commit:
> ...
> The added check does lead to a performance regression indeed, which is
> not all that unexpected. That being said, the commit-graph still results
> in a significant speedup compared to the case where we don't have it.
Yeah, I agree that both points are expected. An extra check that is
much cheaper than the full parsing is paying a small price to be a
bit more careful than before. The question is if the price is small
enough. I am still not sure if the extra carefulness is warranted
for all normal cases to spend 30% extra cycles
Yeah, I agree that both points are expected. An extra check that is
much cheaper than the full parsing is paying a small price to be a
bit more careful than before. The question is if the price is small
enough. I am still not sure if the extra carefulness is warranted
for all normal cases to spend 30% extra cycles.
Thanks.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox