* Re: Defining a platform support policy (Was: [DISCUSS] Introducing Rust into the Git project)
From: Elijah Newren @ 2024-01-24 7:54 UTC (permalink / raw)
To: Emily Shaffer
Cc: Randall S. Becker, Taylor Blau, Junio C Hamano, Dragan Simic,
Git List, Johannes Schindelin
In-Reply-To: <CAJoAoZnHGTFhfR6e6r=GMSfVbSNgLoHF-opaWYLbHppiuzi+Rg@mail.gmail.com>
On Mon, Jan 22, 2024 at 3:18 PM Emily Shaffer <nasamuffin@google.com> wrote:
>
> On Wed, Jan 10, 2024 at 3:52 PM <rsbecker@nexbridge.com> wrote:
> >
[...]
> > Unfortunately, none of the compiler frontends listed previously can be built for NonStop. These appear to all require gcc either directly or transitively, which cannot be ported to NonStop. I do not expect this to change any time soon - and is outside of my control anyway. An attempt was made to port Rust but it did not succeed primarily because of that dependency. Similarly, Golang is also not portable to NonStop because of architecture assumptions made by the Go team that cannot be satisfied on NonStop at this time. If some of the memory/pointer issues are the primary concern, c11 might be something acceptable with smart pointers. C17 will eventually be deployable, but is not available on most currently supported OS versions on the platform.
>
> I hope y'all don't mind me hijacking this part of the thread ;)
Of course not. :-)
[...]
> Does it make sense for us to formalize a support policy?
Some hurdles that may need to be overcome if we want to do so:
* For a significant number of the discussions I remember, a
significant challenge was that we don't even know which platforms Git
is used on. That's why we sometimes agree to weather balloon patches
that attempt to use some new option, in a way that is really easy to
remove...and if no one complains for a long enough time, then we
presume all platforms support it and start adding hard dependencies on
it.
* We are often happy to try to fix issues on even obscure platforms if
we get a detailed enough description showing exactly what the problem
is
* However, when reports don't come with a complete diagnosis, we often
will tell people who are reporting issues that we don't have access to
such a platform and someone else will have to dig further. This
happens more often for exotic platforms (AIX, NonStop, etc.) but also
happens with mainstream platforms (Mac, Windows, and I think I've even
seen it happen with Linux).
* Even when folks report that they can't help the reporter, the work
doesn't always go back to the reporter, because someone else on the
list may respond and dig in; that happens more for mainstream
platforms but can happen with the exotic platforms as well.
* How exactly can we even enforce continued platform support? What's
the actual mechanism? I think the only route available to us is
people who care and try to provide reports, testing, patches, new
tools (e.g. our CI runs and gitgitgadget providing reports across
several of the more common platforms, with lots of work to investigate
the occasional weird build issues and flakes so it continues to be
fairly reliable), but what happens if some of those developers start
caring less...and yet we still have an encoded policy that their
platforms are supported?
I generally think we value portability fairly highly, but it clearly
has bounds...fuzzy and even unknown-by-us bounds. I don't know how to
translate that into a policy, and I'm curious if trying to apply nice
sharp boundaries risks unreasonable expectations on either or both
sides.
Also...
[...]
> I see a few axes we can play with:
> * which architectures/kernels/OS (do we care about more than the
> usual suspects of Linux/Mac/Windows // x86/amd/arm //
> POSIX-compliant?)
> * age of architectures/kernels (do we care to offer full support for
> a 10 or 15 year old OS?)
> * new feature compatibility guarantees vs. core
> functionality/security fix guarantees (which do we really define
> "support" as?)
> * test provisioning (do we require a VM we can run CI on, or is a
> report generated from a nightly build and mailed to the list OK?)
> * test/breakage timing (should the above tests run on every commit to
> 'next'? every merge to 'master'? every RC?)
> * who provides the support (is it the patch author's responsibility
> to fix late-breaking platform support bugs? is it the reporter's
> responsibility? and especially, how does this interplay with test
> provisioning and frequency above?)
That's a great list of questions, but to me it does seem to lean
towards "whatever is supported is supported equally". I don't know if
that was intended, or just the way I read it. But if it was intended,
I'd say that while equal support may be an ideal, I suspect it is
pragmatically just too expensive as evidenced by the many optional
features we already have, many (all?) of which have roots in platform
support or the lack thereof:
* gitk (NO_TCLTK)
* dumb http(s) transport (NO_EXPAT)
* smart http(s) transport (NO_CURL)
* perl regexes (USE_LIBPRCRE)
* translations (NO_GETTEXT)
* charset conversions (NO_ICONV)
* p4 support (NO_PYTHON, affected other scripts in the past too)
* svn, send-email, gitweb support (NO_PERL, affected other stuff in
the past too)
* fsmonitor (FSMONITOR_DAEMON_BACKEND)
Also, this list isn't just an "exotic" vs. "mainstream" platform
thing, since even Linux is "second class" in the final category[1].
So, I think if we create a "supported platforms" policy, it should
address optional features as well (though perhaps as simply as "the
support policy only applies to non-optional parts of Git").
[1] https://lore.kernel.org/git/pull.1352.v5.git.git.1670882286.gitgitgadget@gmail.com/
> If we had clearer answers to these questions, it'd be much simpler to
> determine whether experimentation with Rust is possible or useful.
How so?
^ permalink raw reply
* Re: [PATCH v3 2/2] revision: Implement `git log --merge` also for rebase/cherry_pick/revert
From: Elijah Newren @ 2024-01-24 7:06 UTC (permalink / raw)
To: Michael Lohmann; +Cc: gitster, git, phillip.wood123, Johannes Sixt
In-Reply-To: <20240117081405.14012-2-mi.al.lohmann@gmail.com>
On Wed, Jan 17, 2024 at 12:14 AM Michael Lohmann
<mi.al.lohmann@gmail.com> wrote:
>
> Co-authored-by: Johannes Sixt <j6t@kdbg.org>
> Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>
> ---
>
> On 12. Jan 2024, at 21:18, Junio C Hamano <gitster@pobox.com> wrote:
> > I like the way your other_merge_candidate() loops over an array of
> > possible candidates, which makes it a lot easier to extend, though,
> > admittedly the "die()" message needs auto-generating if we really
> > wanted to make it maintenance free ;-)
>
> I would certainly like that but I did not find an easy way of achieving
> this in C. Help wanted.
>
> Changes with respect to v2:
> - use read_ref_full instead of refs_resolve_ref_unsafe
> - check for symbolic ref
> - extract "other_name" in this patch, instead of patch 1
>
> revision.c | 31 +++++++++++++++++++++++--------
> 1 file changed, 23 insertions(+), 8 deletions(-)
>
> diff --git a/revision.c b/revision.c
> index aa4c4dc778..c778413c7d 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -1961,11 +1961,31 @@ static void add_pending_commit_list(struct rev_info *revs,
> }
> }
>
> +static const char *lookup_other_head(struct object_id *oid)
> +{
> + int i;
> + static const char *const other_head[] = {
> + "MERGE_HEAD", "REBASE_HEAD", "CHERRY_PICK_HEAD", "REVERT_HEAD"
> + };
> +
> + for (i = 0; i < ARRAY_SIZE(other_head); i++)
> + if (!read_ref_full(other_head[i],
> + RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
> + oid, NULL)) {
> + if (is_null_oid(oid))
> + die("%s is a symbolic ref???", other_head[i]);
> + return other_head[i];
> + }
> +
> + die("--merge without MERGE_HEAD, REBASE_HEAD, CHERRY_PICK_HEAD or REVERT_HEAD?");
> +}
> +
> static void prepare_show_merge(struct rev_info *revs)
> {
> struct commit_list *bases;
> struct commit *head, *other;
> struct object_id oid;
> + const char *other_name;
> const char **prune = NULL;
> int i, prune_num = 1; /* counting terminating NULL */
> struct index_state *istate = revs->repo->index;
> @@ -1973,15 +1993,10 @@ static void prepare_show_merge(struct rev_info *revs)
> if (repo_get_oid(the_repository, "HEAD", &oid))
> die("--merge without HEAD?");
> head = lookup_commit_or_die(&oid, "HEAD");
> - if (read_ref_full("MERGE_HEAD",
> - RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
> - &oid, NULL))
> - die("--merge without MERGE_HEAD?");
> - if (is_null_oid(&oid))
> - die("MERGE_HEAD is a symbolic ref???");
> - other = lookup_commit_or_die(&oid, "MERGE_HEAD");
> + other_name = lookup_other_head(&oid);
> + other = lookup_commit_or_die(&oid, other_name);
> add_pending_object(revs, &head->object, "HEAD");
> - add_pending_object(revs, &other->object, "MERGE_HEAD");
> + add_pending_object(revs, &other->object, other_name);
> bases = repo_get_merge_bases(the_repository, head, other);
> add_rev_cmdline_list(revs, bases, REV_CMD_MERGE_BASE, UNINTERESTING | BOTTOM);
> add_pending_commit_list(revs, bases, UNINTERESTING | BOTTOM);
> --
> 2.39.3 (Apple Git-145)
I had to go look up previous versions to see the discussion of why
this was useful for things other than merge. I agree with Phillip
from https://lore.kernel.org/git/648774b5-5208-42d3-95c7-e0cba4d6a159@gmail.com/,
that the commit message _needs_ to explain this, likely using some of
Junio's explanation.
Also, what about cases where users do a cherry-pick in the middle of a
rebase, so that both REBASE_HEAD and CHERRY_PICK_HEAD exist? What
then?
^ permalink raw reply
* [PATCH v4 2/2] patch-id: replace `atoi()` with `strtol_i_updated()`
From: Mohit Marathe via GitGitGadget @ 2024-01-24 6:55 UTC (permalink / raw)
To: git; +Cc: Mohit Marathe, Mohit Marathe
In-Reply-To: <pull.1646.v4.git.1706079304.gitgitgadget@gmail.com>
From: Mohit Marathe <mohitmarathe23@gmail.com>
The change is made to improve the error-handling capabilities
during the conversion of string representations to integers.
The `strtol_i_updated(` function offers a more robust mechanism for
converting strings to integers by providing enhanced error
detection. Unlike `atoi(`, `strtol_i_updated(` allows the code to
differentiate between a valid conversion and an invalid one,
offering better resilience against potential issues such as
reading hunk header of a corrupted patch.
Signed-off-by: Mohit Marathe <mohitmarathe@proton.me>
---
builtin/patch-id.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/builtin/patch-id.c b/builtin/patch-id.c
index 3894d2b9706..88db178c905 100644
--- a/builtin/patch-id.c
+++ b/builtin/patch-id.c
@@ -1,3 +1,4 @@
+#include "git-compat-util.h"
#include "builtin.h"
#include "config.h"
#include "diff.h"
@@ -29,14 +30,18 @@ static int scan_hunk_header(const char *p, int *p_before, int *p_after)
{
static const char digits[] = "0123456789";
const char *q, *r;
+ char *endp;
int n;
q = p + 4;
n = strspn(q, digits);
if (q[n] == ',') {
q += n + 1;
- *p_before = atoi(q);
+ if (strtol_i_updated(q, 10, p_before, &endp) != 0)
+ return 0;
n = strspn(q, digits);
+ if (endp != q + n)
+ return 0;
} else {
*p_before = 1;
}
@@ -48,8 +53,11 @@ static int scan_hunk_header(const char *p, int *p_before, int *p_after)
n = strspn(r, digits);
if (r[n] == ',') {
r += n + 1;
- *p_after = atoi(r);
+ if (strtol_i_updated(r, 10, p_after, &endp) != 0)
+ return 0;
n = strspn(r, digits);
+ if (endp != r + n)
+ return 0;
} else {
*p_after = 1;
}
--
gitgitgadget
^ permalink raw reply related
* [PATCH v4 1/2] git-compat-util: add strtol_i_updated()
From: Mohit Marathe via GitGitGadget @ 2024-01-24 6:55 UTC (permalink / raw)
To: git; +Cc: Mohit Marathe, Mohit Marathe
In-Reply-To: <pull.1646.v4.git.1706079304.gitgitgadget@gmail.com>
From: Mohit Marathe <mohitmarathe23@gmail.com>
This function is an updated version of strtol_i() function. It will
give more control to handle parsing of the characters after the
integer and better error handling while parsing numbers.
Signed-off-by: Mohit Marathe <mohitmarathe@proton.me>
---
git-compat-util.h | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
diff --git a/git-compat-util.h b/git-compat-util.h
index 7c2a6538e5a..b38d7c7f8f1 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1309,6 +1309,29 @@ static inline int strtol_i(char const *s, int base, int *result)
return 0;
}
+#define strtol_i(s,b,r) strtol_i_updated((s), (b), (r), NULL)
+static inline int strtol_i_updated(char const *s, int base, int *result, char **endp)
+{
+ long ul;
+ char *dummy = NULL;
+
+ if (!endp)
+ endp = &dummy;
+ errno = 0;
+ ul = strtol(s, endp, base);
+ if (errno ||
+ /*
+ * if we are told to parse to the end of the string by
+ * passing NULL to endp, it is an error to have any
+ * remaining character after the digits.
+ */
+ (dummy && *dummy) ||
+ *endp == s || (int) ul != ul)
+ return -1;
+ *result = ul;
+ return 0;
+}
+
void git_stable_qsort(void *base, size_t nmemb, size_t size,
int(*compar)(const void *, const void *));
#ifdef INTERNAL_QSORT
--
gitgitgadget
^ permalink raw reply related
* [PATCH v4 0/2] Replace atoi() with strtol_i_updated()
From: Mohit Marathe via GitGitGadget @ 2024-01-24 6:55 UTC (permalink / raw)
To: git; +Cc: Mohit Marathe
In-Reply-To: <pull.1646.v3.git.1706078885.gitgitgadget@gmail.com>
Hello,
This patch series replaces atoi() with an updated version of strtol_i()
called strtol_i_updated (Credits: Junio C Hamano). The reasoning behind this
is to improve error handling by not allowing non-numerical characters in the
hunk header (which might happen in case of a corrupt patch, although
rarely).
There is still a change to be made, as Junio says: "A corrupt patch may be
getting a nonsense patch-ID with the current code and hopefully is not
matching other patches that are not corrupt, but with such a change, a
corrupt patch may not be getting any patch-ID and a loop that computes
patch-ID for many files and try to match them up might need to be rewritten
to take the new failure case into account." I'm not sure where this change
needs to me made (maybe get_one_patchid()?). It would be great if anyone
could point me to the correct place.
Thanks, Mohit Marathe
Mohit Marathe (2):
git-compat-util: add strtol_i_updated()
patch-id: replace `atoi()` with `strtol_i_updated()`
builtin/patch-id.c | 12 ++++++++++--
git-compat-util.h | 23 +++++++++++++++++++++++
2 files changed, 33 insertions(+), 2 deletions(-)
base-commit: e02ecfcc534e2021aae29077a958dd11c3897e4c
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1646%2Fmohit-marathe%2Fupdate-strtol_i-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1646/mohit-marathe/update-strtol_i-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/1646
Range-diff vs v3:
1: 60ea85a701a = 1: 60ea85a701a git-compat-util: add strtol_i_updated()
2: 0e117198d01 ! 2: 17f2dda4907 patch-id: replace `atoi()` with `strtol_i_updated()`
@@ builtin/patch-id.c: static int scan_hunk_header(const char *p, int *p_before, in
+ if (strtol_i_updated(r, 10, p_after, &endp) != 0)
+ return 0;
n = strspn(r, digits);
-+ if (endp != q + n)
++ if (endp != r + n)
+ return 0;
} else {
*p_after = 1;
--
gitgitgadget
^ permalink raw reply
* Re: [PATCH] precious-files.txt: new document proposing new precious file type
From: Elijah Newren @ 2024-01-24 6:50 UTC (permalink / raw)
To: phillip.wood
Cc: Junio C Hamano, Sebastian Thiel, Elijah Newren via GitGitGadget,
git, Josh Triplett
In-Reply-To: <7fc35078-a165-4b3c-96e2-37fbe55e109d@gmail.com>
Hi Phillip,
On Fri, Jan 19, 2024 at 8:53 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
[...]
> >> * Which one between "'git add .' adds '.config' that users did not
> >> want to add" and "'git clean -f' removes '.config' together with
> >> other files" a larger problem to the users, who participate in a
> >> project that already decided to use the new .gitignore feature to
> >> mark ".config" as "precious", of older versions of Git that
> >> predate "precious"?
> >
> > Accidental "git add ." comes with 3 opportunities to correct the
> > problem before it becomes permanent: before commiting, after
> > committing but before pushing, and after publishing for patch review
> > (where it can even be caught by third parties) but before the
> > patch/PR/MR is accepted and included. At each stage there's a chance
> > to go back and correct the problem.
>
> If you've added a secret then catching it after you've published the
> patch for review is likely to be too late. I agree there are a couple of
> chances to catch it before that though.
Ah, good point.
> > Accidental nuking of a file (via either git clean or git checkout or
> > git merge or whatever), cannot be reviewed or corrected; it's
> > immediately too late.
>
> Indeed, though "git clean" requires the user to pass a flag before it
> will delete anything does have a dry-run mode to check what's going to
> happen so there is an opportunity for users to avoid accidental deletions.
Yes, good point again for "git clean"; it does have one level of check
before the operation users can take advantage of. The same cannot be
said for the files nuked by checkout/merge/rebase/cherry-pick, though.
> > [...]
> > However, on a closely related note, in my response to Sebastian I
> > point out that the '$' syntax permits individual teams to prioritize
> > avoiding either accidental deletions or accidental adds on a filename
> > or glob granularity, so if folks are concerned with handling by older
> > Git versions or are just extra concerned with certain files, they can
> > optimize accordingly.
>
> That is an advantage. I do worry that the '$' syntax is unintuitive and
> will further add to the impression that git is hard to use. I think the
> choice comes down how much we are worried about the way older versions
> of git treat ".gitignore" files with the new syntax.
Interesting, I thought the mixture of '!' as a prefix and '#(keep)' as
a previous-line directive would be somewhat inconsistent and add
further to the impression that git is hard to use, though I can also
see your point that '$' as a prefix can as well.
> While I can see it would be helpful to settle the syntax question I
> think parsing the new syntax is a relatively small part of the work that
> needs to be done to implement precious files.
Oh, I agree it's a small part of the work, but as stated previously,
I'm not doing that work (Sebastian is). I'm just trying to help avoid
getting unintended consequences in the design, and to me this is an
important edge case to consider, get an agreement on, and document in
some fashion.
Anyway, Junio seems to have weighed in with a tentative path forward,
and everyone has been very good about bringing up additional
considerations around this issue that are worth documenting in the
design document, so I'll try to put together an update soon-ish.
^ permalink raw reply
* [PATCH v3 2/2] patch-id: replace `atoi()` with `strtol_i_updated()`
From: Mohit Marathe via GitGitGadget @ 2024-01-24 6:48 UTC (permalink / raw)
To: git; +Cc: Mohit Marathe, Mohit Marathe
In-Reply-To: <pull.1646.v3.git.1706078885.gitgitgadget@gmail.com>
From: Mohit Marathe <mohitmarathe23@gmail.com>
The change is made to improve the error-handling capabilities
during the conversion of string representations to integers.
The `strtol_i_updated(` function offers a more robust mechanism for
converting strings to integers by providing enhanced error
detection. Unlike `atoi(`, `strtol_i_updated(` allows the code to
differentiate between a valid conversion and an invalid one,
offering better resilience against potential issues such as
reading hunk header of a corrupted patch.
Signed-off-by: Mohit Marathe <mohitmarathe@proton.me>
---
builtin/patch-id.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/builtin/patch-id.c b/builtin/patch-id.c
index 3894d2b9706..2c00d45cb2c 100644
--- a/builtin/patch-id.c
+++ b/builtin/patch-id.c
@@ -1,3 +1,4 @@
+#include "git-compat-util.h"
#include "builtin.h"
#include "config.h"
#include "diff.h"
@@ -29,14 +30,18 @@ static int scan_hunk_header(const char *p, int *p_before, int *p_after)
{
static const char digits[] = "0123456789";
const char *q, *r;
+ char *endp;
int n;
q = p + 4;
n = strspn(q, digits);
if (q[n] == ',') {
q += n + 1;
- *p_before = atoi(q);
+ if (strtol_i_updated(q, 10, p_before, &endp) != 0)
+ return 0;
n = strspn(q, digits);
+ if (endp != q + n)
+ return 0;
} else {
*p_before = 1;
}
@@ -48,8 +53,11 @@ static int scan_hunk_header(const char *p, int *p_before, int *p_after)
n = strspn(r, digits);
if (r[n] == ',') {
r += n + 1;
- *p_after = atoi(r);
+ if (strtol_i_updated(r, 10, p_after, &endp) != 0)
+ return 0;
n = strspn(r, digits);
+ if (endp != q + n)
+ return 0;
} else {
*p_after = 1;
}
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3 1/2] git-compat-util: add strtol_i_updated()
From: Mohit Marathe via GitGitGadget @ 2024-01-24 6:48 UTC (permalink / raw)
To: git; +Cc: Mohit Marathe, Mohit Marathe
In-Reply-To: <pull.1646.v3.git.1706078885.gitgitgadget@gmail.com>
From: Mohit Marathe <mohitmarathe23@gmail.com>
This function is an updated version of strtol_i() function. It will
give more control to handle parsing of the characters after the
integer and better error handling while parsing numbers.
Signed-off-by: Mohit Marathe <mohitmarathe@proton.me>
---
git-compat-util.h | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
diff --git a/git-compat-util.h b/git-compat-util.h
index 7c2a6538e5a..b38d7c7f8f1 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1309,6 +1309,29 @@ static inline int strtol_i(char const *s, int base, int *result)
return 0;
}
+#define strtol_i(s,b,r) strtol_i_updated((s), (b), (r), NULL)
+static inline int strtol_i_updated(char const *s, int base, int *result, char **endp)
+{
+ long ul;
+ char *dummy = NULL;
+
+ if (!endp)
+ endp = &dummy;
+ errno = 0;
+ ul = strtol(s, endp, base);
+ if (errno ||
+ /*
+ * if we are told to parse to the end of the string by
+ * passing NULL to endp, it is an error to have any
+ * remaining character after the digits.
+ */
+ (dummy && *dummy) ||
+ *endp == s || (int) ul != ul)
+ return -1;
+ *result = ul;
+ return 0;
+}
+
void git_stable_qsort(void *base, size_t nmemb, size_t size,
int(*compar)(const void *, const void *));
#ifdef INTERNAL_QSORT
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3 0/2] Replace atoi() with strtol_i_updated()
From: Mohit Marathe via GitGitGadget @ 2024-01-24 6:48 UTC (permalink / raw)
To: git; +Cc: Mohit Marathe
In-Reply-To: <pull.1646.v2.git.1706077977.gitgitgadget@gmail.com>
Hello,
This patch series replaces atoi() with an updated version of strtol_i()
called strtol_i_updated (Credits: Junio C Hamano). The reasoning behind this
is to improve error handling by not allowing non-numerical characters in the
hunk header (which might happen in case of a corrupt patch, although
rarely).
There is still a change to be made, as Junio says: "A corrupt patch may be
getting a nonsense patch-ID with the current code and hopefully is not
matching other patches that are not corrupt, but with such a change, a
corrupt patch may not be getting any patch-ID and a loop that computes
patch-ID for many files and try to match them up might need to be rewritten
to take the new failure case into account." I'm not sure where this change
needs to me made (maybe get_one_patchid()?). It would be great if anyone
could point me to the correct place.
Thanks, Mohit Marathe
Mohit Marathe (2):
git-compat-util: add strtol_i_updated()
patch-id: replace `atoi()` with `strtol_i_updated()`
builtin/patch-id.c | 12 ++++++++++--
git-compat-util.h | 23 +++++++++++++++++++++++
2 files changed, 33 insertions(+), 2 deletions(-)
base-commit: e02ecfcc534e2021aae29077a958dd11c3897e4c
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1646%2Fmohit-marathe%2Fupdate-strtol_i-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1646/mohit-marathe/update-strtol_i-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1646
Range-diff vs v2:
1: 60ea85a701a = 1: 60ea85a701a git-compat-util: add strtol_i_updated()
2: f3a03d68211 ! 2: 0e117198d01 patch-id: replace `atoi()` with `strtol_i_updated()`
@@ builtin/patch-id.c: static int scan_hunk_header(const char *p, int *p_before, in
+ if (strtol_i_updated(q, 10, p_before, &endp) != 0)
+ return 0;
n = strspn(q, digits);
++ if (endp != q + n)
++ return 0;
} else {
*p_before = 1;
+ }
@@ builtin/patch-id.c: static int scan_hunk_header(const char *p, int *p_before, int *p_after)
n = strspn(r, digits);
if (r[n] == ',') {
@@ builtin/patch-id.c: static int scan_hunk_header(const char *p, int *p_before, in
+ if (strtol_i_updated(r, 10, p_after, &endp) != 0)
+ return 0;
n = strspn(r, digits);
++ if (endp != q + n)
++ return 0;
} else {
*p_after = 1;
+ }
--
gitgitgadget
^ permalink raw reply
* [PATCH v2 2/2] patch-id: replace `atoi()` with `strtol_i_updated()`
From: Mohit Marathe via GitGitGadget @ 2024-01-24 6:32 UTC (permalink / raw)
To: git; +Cc: Mohit Marathe, Mohit Marathe
In-Reply-To: <pull.1646.v2.git.1706077977.gitgitgadget@gmail.com>
From: Mohit Marathe <mohitmarathe23@gmail.com>
The change is made to improve the error-handling capabilities
during the conversion of string representations to integers.
The `strtol_i_updated(` function offers a more robust mechanism for
converting strings to integers by providing enhanced error
detection. Unlike `atoi(`, `strtol_i_updated(` allows the code to
differentiate between a valid conversion and an invalid one,
offering better resilience against potential issues such as
reading hunk header of a corrupted patch.
Signed-off-by: Mohit Marathe <mohitmarathe@proton.me>
---
builtin/patch-id.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/builtin/patch-id.c b/builtin/patch-id.c
index 3894d2b9706..e513b6aed3f 100644
--- a/builtin/patch-id.c
+++ b/builtin/patch-id.c
@@ -1,3 +1,4 @@
+#include "git-compat-util.h"
#include "builtin.h"
#include "config.h"
#include "diff.h"
@@ -29,13 +30,15 @@ static int scan_hunk_header(const char *p, int *p_before, int *p_after)
{
static const char digits[] = "0123456789";
const char *q, *r;
+ char *endp;
int n;
q = p + 4;
n = strspn(q, digits);
if (q[n] == ',') {
q += n + 1;
- *p_before = atoi(q);
+ if (strtol_i_updated(q, 10, p_before, &endp) != 0)
+ return 0;
n = strspn(q, digits);
} else {
*p_before = 1;
@@ -48,7 +51,8 @@ static int scan_hunk_header(const char *p, int *p_before, int *p_after)
n = strspn(r, digits);
if (r[n] == ',') {
r += n + 1;
- *p_after = atoi(r);
+ if (strtol_i_updated(r, 10, p_after, &endp) != 0)
+ return 0;
n = strspn(r, digits);
} else {
*p_after = 1;
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 0/2] Replace atoi() with strtol_i_updated()
From: Mohit Marathe via GitGitGadget @ 2024-01-24 6:32 UTC (permalink / raw)
To: git; +Cc: Mohit Marathe
In-Reply-To: <pull.1646.git.1705913519.gitgitgadget@gmail.com>
Hello,
This patch series replaces atoi() with an updated version of strtol_i()
called strtol_i_updated (Credits: Junio C Hamano). The reasoning behind this
is to improve error handling by not allowing non-numerical characters in the
hunk header (which might happen in case of a corrupt patch, although
rarely).
There is still a change to be made, as Junio says: "A corrupt patch may be
getting a nonsense patch-ID with the current code and hopefully is not
matching other patches that are not corrupt, but with such a change, a
corrupt patch may not be getting any patch-ID and a loop that computes
patch-ID for many files and try to match them up might need to be rewritten
to take the new failure case into account." I'm not sure where this change
needs to me made (maybe get_one_patchid()?). It would be great if anyone
could point me to the correct place.
Thanks, Mohit Marathe
Mohit Marathe (2):
git-compat-util: add strtol_i_updated()
patch-id: replace `atoi()` with `strtol_i_updated()`
builtin/patch-id.c | 8 ++++++--
git-compat-util.h | 23 +++++++++++++++++++++++
2 files changed, 29 insertions(+), 2 deletions(-)
base-commit: e02ecfcc534e2021aae29077a958dd11c3897e4c
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1646%2Fmohit-marathe%2Fupdate-strtol_i-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1646/mohit-marathe/update-strtol_i-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1646
Range-diff vs v1:
1: 4e2b03cdd4f ! 1: 60ea85a701a git-compat-util: add strtol_i2
@@ Metadata
Author: Mohit Marathe <mohitmarathe23@gmail.com>
## Commit message ##
- git-compat-util: add strtol_i2
+ git-compat-util: add strtol_i_updated()
- This function is an updated version of strtol_i function. It will
+ This function is an updated version of strtol_i() function. It will
give more control to handle parsing of the characters after the
integer and better error handling while parsing numbers.
@@ git-compat-util.h: static inline int strtol_i(char const *s, int base, int *resu
return 0;
}
-+#define strtol_i(s,b,r) strtol_i2((s), (b), (r), NULL)
-+static inline int strtol_i2(char const *s, int base, int *result, char **endp)
++#define strtol_i(s,b,r) strtol_i_updated((s), (b), (r), NULL)
++static inline int strtol_i_updated(char const *s, int base, int *result, char **endp)
+{
+ long ul;
+ char *dummy = NULL;
2: 1ece724b1ca ! 2: f3a03d68211 patch-id: replace `atoi()` with `strtol_i2()`
@@ Metadata
Author: Mohit Marathe <mohitmarathe23@gmail.com>
## Commit message ##
- patch-id: replace `atoi()` with `strtol_i2()`
+ patch-id: replace `atoi()` with `strtol_i_updated()`
The change is made to improve the error-handling capabilities
during the conversion of string representations to integers.
- The `strtol_i2(` function offers a more robust mechanism for
+ The `strtol_i_updated(` function offers a more robust mechanism for
converting strings to integers by providing enhanced error
- detection. Unlike `atoi(`, `strtol_i2(` allows the code to
+ detection. Unlike `atoi(`, `strtol_i_updated(` allows the code to
differentiate between a valid conversion and an invalid one,
offering better resilience against potential issues such as
reading hunk header of a corrupted patch.
@@ builtin/patch-id.c: static int scan_hunk_header(const char *p, int *p_before, in
if (q[n] == ',') {
q += n + 1;
- *p_before = atoi(q);
-+ if (strtol_i2(q, 10, p_before, &endp) != 0)
++ if (strtol_i_updated(q, 10, p_before, &endp) != 0)
+ return 0;
n = strspn(q, digits);
} else {
*p_before = 1;
- }
-
-- if (n == 0 || q[n] != ' ' || q[n+1] != '+')
-+ if (q[n] != ' ' || q[n+1] != '+')
- return 0;
-
- r = q + n + 2;
+@@ builtin/patch-id.c: static int scan_hunk_header(const char *p, int *p_before, int *p_after)
n = strspn(r, digits);
if (r[n] == ',') {
r += n + 1;
- *p_after = atoi(r);
-- n = strspn(r, digits);
-+ if (strtol_i2(r, 10, p_after, &endp) != 0)
++ if (strtol_i_updated(r, 10, p_after, &endp) != 0)
+ return 0;
+ n = strspn(r, digits);
} else {
*p_after = 1;
- }
-- if (n == 0)
-- return 0;
--
- return 1;
- }
-
--
gitgitgadget
^ permalink raw reply
* [PATCH v2 1/2] git-compat-util: add strtol_i_updated()
From: Mohit Marathe via GitGitGadget @ 2024-01-24 6:32 UTC (permalink / raw)
To: git; +Cc: Mohit Marathe, Mohit Marathe
In-Reply-To: <pull.1646.v2.git.1706077977.gitgitgadget@gmail.com>
From: Mohit Marathe <mohitmarathe23@gmail.com>
This function is an updated version of strtol_i() function. It will
give more control to handle parsing of the characters after the
integer and better error handling while parsing numbers.
Signed-off-by: Mohit Marathe <mohitmarathe@proton.me>
---
git-compat-util.h | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
diff --git a/git-compat-util.h b/git-compat-util.h
index 7c2a6538e5a..b38d7c7f8f1 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1309,6 +1309,29 @@ static inline int strtol_i(char const *s, int base, int *result)
return 0;
}
+#define strtol_i(s,b,r) strtol_i_updated((s), (b), (r), NULL)
+static inline int strtol_i_updated(char const *s, int base, int *result, char **endp)
+{
+ long ul;
+ char *dummy = NULL;
+
+ if (!endp)
+ endp = &dummy;
+ errno = 0;
+ ul = strtol(s, endp, base);
+ if (errno ||
+ /*
+ * if we are told to parse to the end of the string by
+ * passing NULL to endp, it is an error to have any
+ * remaining character after the digits.
+ */
+ (dummy && *dummy) ||
+ *endp == s || (int) ul != ul)
+ return -1;
+ *result = ul;
+ return 0;
+}
+
void git_stable_qsort(void *base, size_t nmemb, size_t size,
int(*compar)(const void *, const void *));
#ifdef INTERNAL_QSORT
--
gitgitgadget
^ permalink raw reply related
* Re: [DISCUSS] Introducing Rust into the Git project
From: Dragan Simic @ 2024-01-24 5:14 UTC (permalink / raw)
To: Elijah Newren; +Cc: Taylor Blau, git
In-Reply-To: <CABPp-BGfPXKtdHaz0u5273AwUfBnYRKfMa2VHPFohv5fOtwJtg@mail.gmail.com>
Hello Elijah,
On 2024-01-24 05:15, Elijah Newren wrote:
> On Wed, Jan 17, 2024 at 1:30 PM Dragan Simic <dsimic@manjaro.org>
> wrote:
>> On 2024-01-11 17:57, Elijah Newren wrote:
>>> On Wed, Jan 10, 2024 at 9:39 PM Dragan Simic <dsimic@manjaro.org>
>>> wrote:
>> and refrain myself
>> from expressing my opinions in this thread.
>
> ...but that's not what my advice was. My advice was that you'd be
> more persuasive if you expressed your opinions differently. Some
> possible examples:
>
> * Stating that you are worried about the codebase becoming more
> complicated or more fragmented (without dismissing the points Taylor
> raised)
> * Arguing that you believe various points others raised aren't as
> much of an advantage as they perceive, or even potentially aren't even
> an advantage at all, not by mere assertion but by providing additional
> details on the topic (statistics, anecdotes, war stories,
> counter-examples, old commit messages, etc.) that back up your point
> * Stating that you don't understand why others think that advantages
> they state are as significant as they pose and ask for clarification.
>
> I think there's potentially some good points behind your positions,
> and I don't want to discourage them. I want to encourage lively,
> friendly debate so that we can have the best information possible when
> decisions are made.
Oh, I once again totally agree! I really love the way you expressed it,
which I'm once again thankful for.
I always support making improvements and major changes, introducing
new technologies, augmenting or even replacing old technologies, etc.
In the end, that's how progress is made, but such major changes also
need to be performed very carefully, in a controlled way that provides
a backup plan, and based on solid facts and past experiences.
In this specific case, please be aware that my health wasn't in great
shape, because I was (and still am) recovering from some nasty flu,
which has also effectively diminished my mental capacities. That's
the primary reason why I provided only terse comments, without backing
them up with more specific details. That's not the way I usually
operate, and I apologize for that.
^ permalink raw reply
* Re: [PATCH 2/2] patch-id: replace `atoi()` with `strtol_i2()`
From: Mohit Marathe @ 2024-01-24 4:22 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Mohit Marathe via GitGitGadget, git, Mohit Marathe
In-Reply-To: <xmqq8r4htb8r.fsf@gitster.g>
On Tuesday, January 23rd, 2024 at 1:02 AM, Junio C Hamano <gitster@pobox.com> wrote:
> "Mohit Marathe via GitGitGadget" gitgitgadget@gmail.com writes:
>
> > static const char digits[] = "0123456789";
> > const char *q, *r;
> > + char *endp;
> > int n;
> >
> > q = p + 4;
> > n = strspn(q, digits);
> > if (q[n] == ',') {
> > q += n + 1;
> > - *p_before = atoi(q);
> > + if (strtol_i2(q, 10, p_before, &endp) != 0)
> > + return 0;
> > n = strspn(q, digits);
> > } else {
> > *p_before = 1;
> > }
>
>
> Looking at this code again, because we upfront run strspn() to make
> sure q[] begins with a run of digits and followed by a comma
> (which is not a digit), I think it is safe to use atoi() and assume
> it would slurp all the digits. So the lack of another check the use
> of new helper allows us to do, namely
>
> if (endp != q + n)
> return 0;
>
> is probably OK, but that is one of the two reasons why you would
> favor the use of new helper over atoi(), so the upside of this
> change is not all that great as I originally hoped for X-<.
>
> Not your fault, of course. We would still catch when the digit
> string that starts q[] is too large to fit in an int, which is an
> upside.
>
> > - if (n == 0 || q[n] != ' ' || q[n+1] != '+')
> > + if (q[n] != ' ' || q[n+1] != '+')
> > return 0;
>
>
> When we saw q[] that begins with ',' upon entry to this function, we
> used to say *p_before = 1 and then saw n==0 and realized it is not a
> good input and returned 0 from the function.
Uh oh, I just looked at the `if` block and concluded that it was just
to check if it has numbers after the ',', which`strtol_i2()` already
does. But I totally missed this one.
> Now we instead peek q[0] and the check says q[0] is not SP so we
> will return 0 the same way so there is no behaviour change from the
> upper hunk? The conversion may be correct, but it wasn't explained
> in the proposed commit log message.
>
> How are the change to stop caring about n==0 here ...
>
> > r = q + n + 2;
> > n = strspn(r, digits);
> > if (r[n] == ',') {
> > r += n + 1;
> > - *p_after = atoi(r);
> > - n = strspn(r, digits);
> > + if (strtol_i2(r, 10, p_after, &endp) != 0)
> > + return 0;
> > } else {
> > *p_after = 1;
> > }
> > - if (n == 0)
> > - return 0;
>
>
> ... and this change here, linked to the switch from atoi() to
> strtul_i2()[*]?
>
> It looks like an unrelated behaviour change that is left
> unexplained.
>
> > return 1;
> > }
>
>
> Thanks for working on this one.
>
>
> [Footnote]
>
> * by the way, what a horrible name for a public function. Yuck.
Yeah, I thought so too /:D How does `strtol_i_updated` sounds?
Thanks for you feedback! I will send v2 with the corrections soon.
^ permalink raw reply
* Re: [DISCUSS] Introducing Rust into the Git project
From: Elijah Newren @ 2024-01-24 4:15 UTC (permalink / raw)
To: Dragan Simic; +Cc: Taylor Blau, git
In-Reply-To: <45bfda3a350b4040a28a25993a2b22e0@manjaro.org>
Hi Dragan,
On Wed, Jan 17, 2024 at 1:30 PM Dragan Simic <dsimic@manjaro.org> wrote:
>
> On 2024-01-11 17:57, Elijah Newren wrote:
> > Hi Dragan,
>
> I apologize for my delayed response.
No worries; I'm often hit or miss on my responses these days as well.
> > On Wed, Jan 10, 2024 at 9:39 PM Dragan Simic <dsimic@manjaro.org>
> > wrote:
> >>
> >> On 2024-01-11 01:33, Elijah Newren wrote:
> >> > On Wed, Jan 10, 2024 at 1:57 PM Dragan Simic <dsimic@manjaro.org>
> >> > wrote:
> >> >>
> >> >> Thus, Git should probably follow the same approach of not converting
> >> >> the
> >> >> already existing code
> >> >
> >> > I disagree with this. I saw significant performance improvements
> >> > through converting some existing Git code to Rust. Granted, it was
> >> > only a small amount of code, but the performance benefits I saw
> >> > suggested we'd see more by also doing similar conversions elsewhere.
> >> > (Note that I kept the old C code and then conditionally compiled
> >> > either Rust or C versions of what I was converting.)
> >>
> >> Well, it's also possible that improving the old C code could also
> >> result
> >> in some performance improvements. Thus, quite frankly, I don't see
> >> that
> >> as a valid argument to rewrite some existing C code in Rust.
> >
> > Yes, and I've made many performance improvements in the C code in git.
> > Sometimes I make some of the code 5% or 20% faster. Sometimes 1-3
> > orders of magnitude faster. Once over 60 orders of magnitude
> > faster.[1] Look around in git's history; I've done a fair amount of
> > performance stuff.
>
> Thank you very much for your work!
>
> > And I'm specifically arguing that I feel limited in some of the
> > performance work that can be done by remaining in C. Part of my
> > reason for interest in Rust is exactly because I think it can help us
> > improve performance in ways that are far more difficult to achieve in
> > C. And this isn't just guesswork, I've done some trials with it.
> > Further, I even took the time to document some of these reasons
> > elsewhere in this thread[2]. Arguing that some performance
> > improvements can be done in C is thus entirely missing the point.
> >
> > If you want to dismiss the performance angle of argument for Rust, you
> > should take the time to address the actual reasons raised for why it
> > could make it easier to improve performance relative to continuing in
> > C.
> >
> > Also, as a heads up since you seem to be relatively new to the list:
> > your position will probably carry more weight with others if you take
> > the time to understand, acknowledge, and/or address counterpoints of
> > the other party. It is certainly fine to simply express some concerns
> > without doing so (Randall and Patrick did a good job of this in this
> > thread), but when you simply assert that the benefits others point out
> > simply don't exist (e.g. your "Quite frankly, that would _only_
> > complicate things and cause fragmentation." (emphasis added) from your
> > first email in this thread[3], and which this latest email of yours
> > somewhat looks like as well), others may well start applying a
> > discount to any positions you state. Granted, it's totally up to you,
> > but I'm just giving a hint about how I think you might be able to be
> > more persuasive.
>
> I totally agree with your suggestions, and I'm thankful for the time it
> took you to write it all down. I'll take your advice
Great!
> and refrain myself
> from expressing my opinions in this thread.
...but that's not what my advice was. My advice was that you'd be
more persuasive if you expressed your opinions differently. Some
possible examples:
* Stating that you are worried about the codebase becoming more
complicated or more fragmented (without dismissing the points Taylor
raised)
* Arguing that you believe various points others raised aren't as
much of an advantage as they perceive, or even potentially aren't even
an advantage at all, not by mere assertion but by providing additional
details on the topic (statistics, anecdotes, war stories,
counter-examples, old commit messages, etc.) that back up your point
* Stating that you don't understand why others think that advantages
they state are as significant as they pose and ask for clarification.
I think there's potentially some good points behind your positions,
and I don't want to discourage them. I want to encourage lively,
friendly debate so that we can have the best information possible when
decisions are made.
^ permalink raw reply
* Re: [PATCH 3/7] refs: convert AUTO_MERGE to become a normal pseudo-ref
From: Elijah Newren @ 2024-01-24 3:19 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Junio C Hamano, git
In-Reply-To: <Za5HOkWM3IQIiKDJ@tanuki>
On Mon, Jan 22, 2024 at 2:45 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Fri, Jan 19, 2024 at 11:28:10AM -0800, Junio C Hamano wrote:
> > Patrick Steinhardt <ps@pks.im> writes:
> >
> > > In 70c70de616 (refs: complete list of special refs, 2023-12-14) we have
> > > inrtoduced a new `is_special_ref()` function that classifies some refs
> >
> > "introduced"
> >
> > > @@ -4687,10 +4685,17 @@ void merge_switch_to_result(struct merge_options *opt,
> > > trace2_region_leave("merge", "record_conflicted", opt->repo);
> > >
> > > trace2_region_enter("merge", "write_auto_merge", opt->repo);
> > > - filename = git_path_auto_merge(opt->repo);
> > > - fp = xfopen(filename, "w");
> > > - fprintf(fp, "%s\n", oid_to_hex(&result->tree->object.oid));
> > > - fclose(fp);
> > > + if (refs_update_ref(get_main_ref_store(opt->repo), "", "AUTO_MERGE",
> > > + &result->tree->object.oid, NULL, REF_NO_DEREF,
> > > + UPDATE_REFS_MSG_ON_ERR)) {
> > > + /* failure to function */
> > > + opt->priv = NULL;
> > > + result->clean = -1;
> > > + merge_finalize(opt, result);
> > > + trace2_region_leave("merge", "write_auto_merge",
> > > + opt->repo);
> > > + return;
> > > + }
> > > trace2_region_leave("merge", "write_auto_merge", opt->repo);
> > > }
> > > if (display_update_msgs)
> >
> > We used to ignore errors while updating AUTO_MERGE, implying that it
> > is an optional nicety that does not have to block the merge. Now we
> > explicitly mark the resulting index unclean. While my gut feeling
> > says that it should not matter all that much (as such a failure
> > would be rare enough that the user may want to inspect and double
> > check the situation before going forward), I am not 100% sure if the
> > change is behaviour is acceptable by everybody (cc'ed Elijah for
> > second opinion).
>
> We only ignored _some_ errors when updating AUTO_MERGE. Most notably we
> die when we fail to create the file, but we succeed in case its contents
> aren't written. This does not make much sense to me -- my expectation
> would be that we should verify either the complete operation or nothing
> of it and ignore all failures. Gracefully leaving an empty file behind
> is a weird in-between state, so I'd claim it's more or less an oversight
> that we did not perform proper error checking here.
I can confirm it was indeed just an oversight. I like your change to
make this code more careful.
^ permalink raw reply
* Re: Subject: [PATCH] CoC: whitespace fix
From: Elijah Newren @ 2024-01-24 3:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Kristoffer Haugsbakk, Taylor Blau, Brian Lyles
In-Reply-To: <xmqqmssvnb8d.fsf_-_@gitster.g>
On Tue, Jan 23, 2024 at 10:41 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> > For example, CODE_OF_CONDUCT.md has these two (shown with $$$)
> > I think can be removed without breaking markdown:
> >
> > Community Impact Guidelines were inspired by $$$
> > [Mozilla's code of conduct enforcement ladder][Mozilla CoC].
> >
> > For answers to common questions about this code of conduct, see the FAQ at
> > [https://www.contributor-covenant.org/faq][FAQ]. Translations are available $$$
> > at [https://www.contributor-covenant.org/translations][translations].
>
>
> Before I forget...
>
> ------ >8 ----------- >8 ----------- >8 ----------- >8 ------
> Subject: [PATCH] CoC: whitespace fix
>
> Fix two lines with trailing whitespaces.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> CODE_OF_CONDUCT.md | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/CODE_OF_CONDUCT.md b/CODE_OF_CONDUCT.md
> index 0215b1fd4c..e58917c50a 100644
> --- a/CODE_OF_CONDUCT.md
> +++ b/CODE_OF_CONDUCT.md
> @@ -130,11 +130,11 @@ This Code of Conduct is adapted from the [Contributor Covenant][homepage],
> version 2.0, available at
> [https://www.contributor-covenant.org/version/2/0/code_of_conduct.html][v2.0].
>
> -Community Impact Guidelines were inspired by
> +Community Impact Guidelines were inspired by
> [Mozilla's code of conduct enforcement ladder][Mozilla CoC].
>
> For answers to common questions about this code of conduct, see the FAQ at
> -[https://www.contributor-covenant.org/faq][FAQ]. Translations are available
> +[https://www.contributor-covenant.org/faq][FAQ]. Translations are available
> at [https://www.contributor-covenant.org/translations][translations].
>
> [homepage]: https://www.contributor-covenant.org
> --
I'm always happy to see trailing whitespace removed. :-) LGTM.
^ permalink raw reply
* Re: Fwd: Unexpected behavior of ls-files command when using --others --exclude-from, and a .gitignore file which resides in a subdirectory
From: Elijah Newren @ 2024-01-24 2:58 UTC (permalink / raw)
To: Jeff King
Cc: Junio C Hamano, Raúl Núñez de Arenas Coronado, git
In-Reply-To: <20240122215954.GA813833@coredump.intra.peff.net>
On Mon, Jan 22, 2024 at 1:59 PM Jeff King <peff@peff.net> wrote:
>
> On Mon, Jan 22, 2024 at 01:45:05PM -0800, Junio C Hamano wrote:
>
> > Jeff King <peff@peff.net> writes:
> >
> > > PS I hadn't realized that --exclude-per-directory had been marked as
> > > deprecated. I do agree with e750951e74 (ls-files: guide folks to
> > > --exclude-standard over other --exclude* options, 2023-01-13) in its
> > > goal of guiding people to the easiest option, but I don't know that
> > > there has been any discussion about removing the other ones.
I'm not aware of any discussion either. I certainly had no plans to remove it.
> > I do not think there is any value in _removing_ the perfectly well
> > working --exclude* options, even though I think --exclude-standard
> > should be what users and scriptors should be using if they want to
> > emulate what Git does internally.
>
> Yeah, mostly I was surprised that e750951e74 used as strong a word as
> "deprecated".
Well, here's what I was thinking. First, based on wikipedia's
definition of deprecated:
```
In several fields, especially computing, deprecation is the
discouragement of use of some terminology, feature, design, or
practice, typically because it has been superseded or is no longer
considered efficient or safe, without completely removing it or
prohibiting its use. Typically, deprecated materials are not
completely removed to ensure legacy compatibility or back up practice
in case new methods are not functional in an odd scenario.
It can also imply that a feature, design, or practice will be removed
or discontinued entirely in the future.
```
Since "can also imply" != "does also imply", and based on the commit
message of 491a7575f (particularly the part that quotes dcf0c16ef1,
both of which were prior work that informed the under discussion
e750951e74), I thought the use of deprecated was perfectly applicable
here.
^ permalink raw reply
* Re: Fwd: Unexpected behavior of ls-files command when using --others --exclude-from, and a .gitignore file which resides in a subdirectory
From: Jeff King @ 2024-01-24 1:09 UTC (permalink / raw)
To: Raúl Núñez de Arenas Coronado; +Cc: git
In-Reply-To: <CAGF1KhWaOrnV8d2f_vajOT0hFK2N_8m7-p_vnZhw92ZdhXfsag@mail.gmail.com>
On Tue, Jan 23, 2024 at 06:40:10AM +0100, Raúl Núñez de Arenas Coronado wrote:
> > Do you get different results from that toy repo? If not, then what is
> > different about your main repo? Do you perhaps have a stray "*" match
> > somewhere in .git/info/exclude, etc? Or are you still providing
> > --exclude-from in addition to --exclude-standard?
>
> The difference lies in how I deal in my computer with ignored files. I
> have some files in all my repos which have to be ignored always, so I
> have that pattern in my core.excludes file. BUT those files have to be
> backed up on my system, just in case my local copy of the repo is lost
> for some reason, as they are files I need for development in my
> personal machine.
>
> If I use --exclude-standard, no matter if those files are being
> ignored by core.excludes OR .git/info/exclude, they won't appear in
> that 'list other files' command output.
Ah, OK. I understand your case better now. Then yes, I think
--exclude-per-directory is the right thing for you, since it lets you
just trigger parts of the exclusion mechanism, and not all. I would say
to ignore the deprecation mention in the manpage. ;)
(I think this also gives an interesting use case arguing for continuing
to support those more-specific exclusion options for ls-files).
If you are mostly just using core.excludesFile (and not
.git/info/exclude), then I suspect:
git -c core.excludesFile /dev/null ls-files -o --exclude-standard
would work for you, too (but it sounds like you might also be using
.git/info/exclude).
-Peff
^ permalink raw reply
* [PATCH] transport-helper: re-examine object dir after fetching
From: Jeff King @ 2024-01-24 1:00 UTC (permalink / raw)
To: Josh Steadmon; +Cc: git, bcmills
In-Reply-To: <ZbAqsf-2DWaXbN7K@google.com>
On Tue, Jan 23, 2024 at 01:08:01PM -0800, Josh Steadmon wrote:
> At $DAYJOB, we got a bug report that Git 2.21.0 breaks Go's CI due to
> not fetching all tags in the history. The bug reporter (Bryan, CCed) was
> kind enough to bisect this failure down to 61c7711cfe (sha1-file: use
> loose object cache for quick existence check, 2018-11-12). This was only
> recently discovered because Go's CI was using Git v2.17.6.
>
> More details can be found in the original bug report [1] and Go's issue
> for the CI breakage [2].
Thanks, I was able to reproduce it. Besides using the v0 protocol, two
key elements are that the server is http and the use of --depth.
The patch below explains what's going on and should fix it. I prepared
the patch on top of 'master', but it can also be applied directly on
61c7711cfe or on v2.21.0, modulo some minor textual conflicts in the
test script (modern t5551 has some more tests, and no longer calls
stop_httpd manually).
-- >8 --
Subject: transport-helper: re-examine object dir after fetching
This patch fixes a bug where fetch over http (or any helper) using the
v0 protocol may sometimes fail to auto-follow tags. The bug comes from
61c7711cfe (sha1-file: use loose object cache for quick existence check,
2018-11-12). But to explain why (and why this is the right fix), let's
take a step back.
After fetching a pack, the object database has changed, but we may still
hold in-memory caches that are now out of date. Traditionally this was
just the packed_git list, but 61c7711cfe started using a loose-object
cache, as well.
Usually these caches are invalidated automatically. When an expected
object cannot be found, the low-level object lookup routines call
reprepare_packed_git(), which re-scans the set of packs (and thanks to
some preparatory patches ahead of 61c7711cfe, throws away the loose
object cache). But not all calls do this! In some cases we expect that
the object might not exist, and pass OBJECT_INFO_QUICK to tell the
low-level routines not to bother re-scanning. And the tag auto-following
code is one such caller, since we are asking about oids that the other
side has (but we might not have locally).
To deal with this, we explicitly call reprepare_packed_git() ourselves
after fetching a pack; this goes all the way back to 48ec3e5c07
(Incorporate fetched packs in future object traversal, 2008-06-15). But
that only helps if we call fetch_pack() in the main fetch process. When
we're using a transport helper, it happens in a separate sub-process,
and the parent process is left with old values. So this is only a
problem with protocols which require a separate helper process (like
http).
This patch fixes it by teaching the parent process in the transport
helper relationship to make that same reprepare call after the helper
finishes fetching.
You might be left with some lingering questions, like:
1. Why only the v0 protocol, and not v2? It's because in v2 the child
helper doesn't actually run fetch_pack(); it merely establishes a
tunnel over which the main process can talk to the remote side (so
the fetch_pack() and reprepare happen in the main process).
2. Wouldn't we have the same bug even before the 61c7711cfe added
the loose object cache? For example, when we store the fetch as a
pack locally, wouldn't our packed_git list still be out of date?
If we store a pack, everything works because other parts of the
fetch process happen to trigger a call to reprepare_packed_git().
In particular, before storing whatever ref was originally
requested, we'll make sure we have the pointed-to object, and that
call happens without the QUICK flag. So in that case we'll see that
we don't know about it, reprepare, and then repeat our lookup. And
now we _do_ know about the pack, and further calls with QUICK will
find its contents.
Whereas when we unpack the result into loose objects, we never get
that same invalidation trigger. We didn't have packs before, and we
don't after. But when we do the loose object lookup, we find the
object. There's no way to realize that we didn't have the object
before the pack, and that having it now means things have changed
(in theory we could do a superfluous cache lookup to see that it
was missing from the old cache; but depending on the tags the other
side showed us, we might not even have filled in that part of the
cache earlier).
3. Why does the included test use "--depth 1"? This is important
because without it, we happen to invalidate the cache as a side
effect of other parts of the fetch process. What happens in a
non-shallow fetch is something like this:
1. we call find_non_local_tags() once before actually getting the
pack, to see if there are any tags we can fill in from what we
already have. This fills in the cache (which is obviously
missing objects we're about to fetch).
2. before fetching the actual pack, fetch_and_consume_refs()
calls check_exist_and_connected(), to see if we even need to
fetch a pack at all. This doesn't use QUICK (though arguably
it could, as it's purely an optimization). And since it sees
there are objects we are indeed missing, that triggers a
reprepare_packed_git() call, which throws out the loose object
cache.
3. after fetching, now we call find_non_local_tags() again. And
since step (2) invalidated our loose object cache, we find
the new objects and create the tags.
So everything works, but mostly due to luck. Whereas in a fetch
with --depth, we skip step 2 entirely, and thus the out-of-date
cache is still in place for step 3, giving us the wrong answer.
So the test works with a small "--depth 1" fetch, which makes sure that
we don't store the pack from the other side, and that we don't trigger
the accidental cache invalidation. And of course it forces the use of
v0 along with using the http protocol.
Signed-off-by: Jeff King <peff@peff.net>
---
t/t5551-http-fetch-smart.sh | 18 ++++++++++++++++++
transport-helper.c | 3 +++
2 files changed, 21 insertions(+)
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index e069737b80..a623a1058c 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -733,4 +733,22 @@ test_expect_success 'no empty path components' '
! grep "//" log
'
+test_expect_success 'tag following always works over v0 http' '
+ upstream=$HTTPD_DOCUMENT_ROOT_PATH/tags &&
+ git init "$upstream" &&
+ (
+ cd "$upstream" &&
+ git commit --allow-empty -m base &&
+ git tag not-annotated &&
+ git tag -m foo annotated
+ ) &&
+ git init tags &&
+ git -C tags -c protocol.version=0 \
+ fetch --depth 1 $HTTPD_URL/smart/tags \
+ refs/tags/annotated:refs/tags/annotated &&
+ git -C "$upstream" for-each-ref refs/tags >expect &&
+ git -C tags for-each-ref >actual &&
+ test_cmp expect actual
+'
+
test_done
diff --git a/transport-helper.c b/transport-helper.c
index e34a8f47cf..07e42e239a 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -17,6 +17,7 @@
#include "refspec.h"
#include "transport-internal.h"
#include "protocol.h"
+#include "packfile.h"
static int debug;
@@ -432,6 +433,8 @@ static int fetch_with_fetch(struct transport *transport,
warning(_("%s unexpectedly said: '%s'"), data->name, buf.buf);
}
strbuf_release(&buf);
+
+ reprepare_packed_git(the_repository);
return 0;
}
--
2.43.0.721.gf5abbd674f
^ permalink raw reply related
* What's cooking in git.git (Jan 2024, #07; Tue, 23)
From: Junio C Hamano @ 2024-01-24 0:25 UTC (permalink / raw)
To: git
Here are the topics that have been cooking in my tree. Commits
prefixed with '+' are in 'next' (being in 'next' is a sign that a
topic is stable enough to be used and are candidate to be in a
future release). Commits prefixed with '-' are only in 'seen', and
aren't considered "accepted" at all and may be annotated with an URL
to a message that raises issues but they are no means exhaustive. A
topic without enough support may be discarded after a long period of
no activity (of course they can be resubmit when new interests
arise).
Copies of the source code to Git live in many repositories, and the
following is a list of the ones I push into or their mirrors. Some
repositories have only a subset of branches.
With maint, master, next, seen, todo:
git://git.kernel.org/pub/scm/git/git.git/
git://repo.or.cz/alt-git.git/
https://kernel.googlesource.com/pub/scm/git/git/
https://github.com/git/git/
https://gitlab.com/git-vcs/git/
With all the integration branches and topics broken out:
https://github.com/gitster/git/
Even though the preformatted documentation in HTML and man format
are not sources, they are published in these repositories for
convenience (replace "htmldocs" with "manpages" for the manual
pages):
git://git.kernel.org/pub/scm/git/git-htmldocs.git/
https://github.com/gitster/git-htmldocs.git/
Release tarballs are available at:
https://www.kernel.org/pub/software/scm/git/
--------------------------------------------------
[New Topics]
* al/t2400-depipe (2024-01-20) 1 commit
(merged to 'next' on 2024-01-22 at a20d4a9a7f)
+ t2400: avoid losing exit status to pipes
Coding style fix.
Will merge to 'master'.
source: <20240120021547.199-1-ach.lumap@gmail.com>
* kl/allow-working-in-dot-git-in-non-bare-repository (2024-01-20) 1 commit
- setup: allow cwd=.git w/ bareRepository=explicit
Loosen "disable repository discovery of a bare repository" check,
triggered by setting safe.bareRepository configuration variable to
'explicit', to exclude the ".git/" directory inside a non-bare
repository from the check.
Will merge to 'next'.
source: <pull.1645.git.1705709303098.gitgitgadget@gmail.com>
* rs/parse-options-with-keep-unknown-abbrev-fix (2024-01-22) 2 commits
(merged to 'next' on 2024-01-23 at a216b482cd)
+ parse-options: simplify positivation handling
+ parse-options: fully disable option abbreviation with PARSE_OPT_KEEP_UNKNOWN
"git diff --no-rename A B" did not disable rename detection but did
not trigger an error from the command line parser.
Will merge to 'master'.
source: <579fd5bc-3bfd-427f-b22d-dab5e7e56eb2@web.de>
source: <fb3c679a-5f00-4934-b028-6b2d081cd5b2@web.de>
* pb/ci-github-skip-logs-for-broken-tests (2024-01-22) 1 commit
(merged to 'next' on 2024-01-23 at f5e3ab2092)
+ ci(github): also skip logs of broken test cases
GitHub CI update.
Will merge to 'master'.
cf. <dbe25fff-e1d4-41f2-8f8f-c538e8c2a77e@github.com>
source: <pull.1649.git.git.1705808313306.gitgitgadget@gmail.com>
* pb/complete-log-more (2024-01-22) 4 commits
- completion: complete missing 'git log' options
- completion: complete --encoding
- completion: complete --patch-with-raw
- completion: complete missing rev-list options
The completion script (in contrib/) learned more options that can
be used with "git log".
Will merge to 'next'.
source: <pull.1650.git.git.1705810071.gitgitgadget@gmail.com>
* jc/reftable-core-fsync (2024-01-23) 1 commit
- reftable: honor core.fsync
The write codepath for the reftable data learned to honor
core.fsync configuration.
Will merge to 'next'.
source: <pull.1654.git.git.1706035870956.gitgitgadget@gmail.com>
--------------------------------------------------
[Cooking]
* kh/maintenance-use-xdg-when-it-should (2024-01-18) 4 commits
(merged to 'next' on 2024-01-19 at 9c8c7b2e9d)
+ maintenance: use XDG config if it exists
+ config: factor out global config file retrieval
+ config: rename global config function
+ config: format newlines
When $HOME/.gitignore is missing but XDG config file available, we
should write into the latter, not former. "git gc" and "git
maintenance" wrote into a wrong "global config" file, which have
been corrected.
Will merge to 'master'.
source: <cover.1705593810.git.code@khaugsbakk.name>
* ps/gitlab-ci-macos (2024-01-18) 6 commits
(merged to 'next' on 2024-01-19 at a239dc8140)
+ ci: add macOS jobs to GitLab CI
+ ci: make p4 setup on macOS more robust
+ ci: handle TEST_OUTPUT_DIRECTORY when printing test failures
+ Makefile: detect new Homebrew location for ARM-based Macs
+ t7527: decrease likelihood of racing with fsmonitor daemon
+ Merge branch 'ps/gitlab-ci-static-analysis' into ps/gitlab-ci-macos
CI for GitLab learned to drive macOS jobs.
Will merge to 'master'.
source: <cover.1705573336.git.ps@pks.im>
* ad/custom-merge-placeholder-for-symbolic-pathnames (2024-01-18) 1 commit
- merge-ll: expose revision names to custom drivers
The labels on conflict markers for the common ancestor, our version,
and the other version are available to custom 3-way merge driver
via %S, %X, and %Y placeholders.
Expecting a reroll.
cf. <xmqqcytvei3c.fsf@gitster.g>
source: <pull.1648.v3.git.git.1705615794307.gitgitgadget@gmail.com>
* cp/unit-test-prio-queue (2024-01-22) 1 commit
- tests: move t0009-prio-queue.sh to the new unit testing framework
Migrate priority queue test to unit testing framework.
source: <pull.1642.v4.git.1705865326185.gitgitgadget@gmail.com>
* jc/reffiles-tests (2024-01-22) 12 commits
- t5312: move reffiles specific tests to t0601
- t4202: move reffiles specific tests to t0600
- t3903: make drop stash test ref backend agnostic
- t1503: move reffiles specific tests to t0600
- t1415: move reffiles specific tests to t0601
- t1410: move reffiles specific tests to t0600
- t1406: move reffiles specific tests to t0600
- t1405: move reffiles specific tests to t0601
- t1404: move reffiles specific tests to t0600
- t1414: convert test to use Git commands instead of writing refs manually
- remove REFFILES prerequisite for some tests in t1405 and t2017
- t3210: move to t0601
Tests on ref API are moved around to prepare for reftable.
Will merge to 'next'.
cf. <Za5TW-q4cKS8pNNc@tanuki>
source: <pull.1647.v2.git.git.1705695540.gitgitgadget@gmail.com>
* ml/log-merge-with-cherry-pick-and-other-pseudo-heads (2024-01-17) 2 commits
- revision: implement `git log --merge` also for rebase/cherry_pick/revert
- revision: ensure MERGE_HEAD is a ref in prepare_show_merge
"git log --merge" learned to pay attention to CHERRY_PICK_HEAD and
other kinds of *_HEAD pseudorefs.
Comments?
source: <xmqqzfxa9usx.fsf@gitster.g>
* nb/rebase-x-shell-docfix (2024-01-17) 1 commit
(merged to 'next' on 2024-01-22 at db49e10354)
+ rebase: fix documentation about used shell in -x
Doc update.
Will merge to 'master'.
source: <20240117085347.948960-1-nik.borisov@suse.com>
* tc/show-ref-exists-fix (2024-01-18) 1 commit
(merged to 'next' on 2024-01-22 at 831452f2dd)
+ builtin/show-ref: treat directory as non-existing in --exists
Update to a new feature recently added, "git show-ref --exists".
Will merge to 'master'.
source: <20240110141559.387815-2-toon@iotcl.com>
* gt/t0024-style-fixes (2024-01-20) 2 commits
(merged to 'next' on 2024-01-22 at 36b46efbd0)
+ t0024: style fix
+ t0024: avoid losing exit status to pipes
Coding style fix.
Will merge to 'master'.
source: <20240118215407.8609-1-shyamthakkar001@gmail.com>
* jc/majordomo-to-subspace (2024-01-20) 1 commit
(merged to 'next' on 2024-01-22 at 6a95f43de4)
+ Docs: majordomo@vger.kernel.org has been decomissioned
Doc update.
Will merge to 'master'.
source: <xmqqmst1hsd6.fsf@gitster.g>
* js/oss-fuzz-build-in-ci (2024-01-19) 2 commits
(merged to 'next' on 2024-01-22 at 2954da5a39)
+ ci: build and run minimal fuzzers in GitHub CI
+ fuzz: fix fuzz test build rules
oss-fuzz tests are built and run in CI.
Will merge to 'master'.
source: <cover.1705700054.git.steadmon@google.com>
* kn/for-all-refs (2024-01-19) 5 commits
- for-each-ref: avoid filtering on empty pattern
- refs: introduce `refs_for_each_all_refs()`
- refs: extract out `loose_fill_ref_dir_regular_file()`
- refs: make `is_pseudoref_syntax()` stricter
- refs: expose `is_pseudoref_syntax()`
"git for-each-ref" filters its output with prefixes given from the
command line, but it did not honor an empty string to mean "pass
everything", which has been corrected.
Expecting a reroll.
cf. <CAOLa=ZQOcqGBJOSehok4BYGUE8RKtnE9eiJYogeT8E6NWZ25xw@mail.gmail.com>
cf. <Za-gF_Hp_lXViGWw@tanuki>
source: <20240119142705.139374-1-karthik.188@gmail.com>
* ps/not-so-many-refs-are-special (2024-01-19) 7 commits
(merged to 'next' on 2024-01-22 at f70f463847)
+ Documentation: add "special refs" to the glossary
+ refs: redefine special refs
+ refs: convert MERGE_AUTOSTASH to become a normal pseudo-ref
+ sequencer: introduce functions to handle autostashes via refs
+ refs: convert AUTO_MERGE to become a normal pseudo-ref
+ sequencer: delete REBASE_HEAD in correct repo when picking commits
+ sequencer: clean up pseudo refs with REF_NO_DEREF
Define "special ref" as a very narrow set that consists of
FETCH_HEAD and MERGE_HEAD, and clarify everything else that used to
be classified as such are actually just pseudorefs.
Will merge to 'master'.
source: <cover.1705659748.git.ps@pks.im>
* es/some-up-to-date-messages-must-stay (2024-01-12) 1 commit
(merged to 'next' on 2024-01-16 at 2b598f7de2)
+ messages: mark some strings with "up-to-date" not to touch
Comment updates to help developers not to attempt to modify
messages from plumbing commands that must stay constant.
It might make sense to reassess the plumbing needs every few years,
but that should be done as a separate effort.
Will merge to 'master'.
source: <20240112171910.11131-1-ericsunshine@charter.net>
* bk/complete-bisect (2024-01-18) 5 commits
- completion: git-bisect view recognized but not completed
- completion: custom git-bisect terms
- completion: move to maintain define-before-use
- completion: git-log opts to bisect visualize
- completion: complete new old actions, start opts
Command line completion support (in contrib/) has been
updated for "git bisect".
Expecting a reroll.
cf. <ZaofJhHsFjRxx7a3@tanuki>
source: <20240118204323.1113859-1-britton.kerin@gmail.com>
* bk/complete-dirname-for-am-and-format-patch (2024-01-12) 1 commit
- completion: dir-type optargs for am, format-patch
Command line completion support (in contrib/) has been
updated for a few commands to complete directory names where a
directory name is expected.
Needs review.
source: <d37781c3-6af2-409b-95a8-660a9b92d20b@smtp-relay.sendinblue.com>
* bk/complete-send-email (2024-01-12) 1 commit
- completion: don't complete revs when --no-format-patch
Command line completion support (in contrib/) has been taught to
avoid offering revision names as candidates to "git send-email" when
the command is used to send pre-generated files.
Needs review.
source: <a718b5ee-afb0-44bd-a299-3208fac43506@smtp-relay.sendinblue.com>
* gt/test-commit-o-i-options (2024-01-17) 2 commits
(merged to 'next' on 2024-01-19 at 0377e2c148)
+ t7501: add tests for --amend --signoff
+ t7501: add tests for --include and --only
A few tests to "git commit -o <pathspec>" and "git commit -i
<pathspec>" has been added.
Will merge to 'master'.
source: <20240117161421.17333-1-shyamthakkar001@gmail.com>
* jt/tests-with-reftable (2024-01-12) 2 commits
(merged to 'next' on 2024-01-19 at 498d203a57)
+ t5541: remove lockfile creation
+ t1401: remove lockfile creation
Tweak a few tests not to manually modify the reference database
(hence easier to work with other backends like reftable).
Will merge to 'master'.
source: <pull.1634.v2.git.1705004670.gitgitgadget@gmail.com>
* la/strvec-comment-fix (2024-01-12) 1 commit
(merged to 'next' on 2024-01-19 at 120ef16493)
+ strvec: use correct member name in comments
Comment fix.
Will merge to 'master'.
source: <pull.1640.git.1705043195997.gitgitgadget@gmail.com>
* la/trailer-api (2024-01-12) 10 commits
- trailer: delete obsolete argument handling code from API
- trailer: move arg handling to interpret-trailers.c
- trailer: prepare to move parse_trailers_from_command_line_args() to builtin
- trailer: spread usage of "trailer_block" language
- trailer: make trailer_info struct private
- sequencer: use the trailer iterator
- trailer: delete obsolete formatting functions
- trailer: unify trailer formatting machinery
- trailer: include "trailer" term in API functions
- trailer: move process_trailers() to interpret-trailers.c
Code clean-up.
Needs review.
source: <pull.1632.git.1704869487.gitgitgadget@gmail.com>
* ps/tests-with-ref-files-backend (2024-01-12) 6 commits
- t: mark tests regarding git-pack-refs(1) to be backend specific
- t5526: break test submodule differently
- t1419: mark test suite as files-backend specific
- t1302: make tests more robust with new extensions
- t1301: mark test for `core.sharedRepository` as reffiles specific
- t1300: make tests more robust with non-default ref backends
Prepare existing tests on refs to work better with non-default
backends.
Needs review.
source: <cover.1704877233.git.ps@pks.im>
* ne/doc-filter-blob-limit-fix (2024-01-16) 1 commit
(merged to 'next' on 2024-01-19 at 4f78975728)
+ rev-list-options: fix off-by-one in '--filter=blob:limit=<n>' explainer
Docfix.
Will merge to 'master'.
source: <pull.1645.git.git.1705261850650.gitgitgadget@gmail.com>
* ps/commit-graph-write-leakfix (2024-01-15) 1 commit
(merged to 'next' on 2024-01-19 at df537fac39)
+ commit-graph: fix memory leak when not writing graph
Leakfix.
Will merge to 'master'.
source: <0feab5e7d5bc6275e2c7671cd8f6786ea86fd610.1702891190.git.ps@pks.im>
* rj/advice-delete-branch-not-fully-merged (2024-01-11) 3 commits
(merged to 'next' on 2024-01-19 at 7102eb6b79)
+ branch: make the advice to force-deleting a conditional one
+ advice: fix an unexpected leading space
+ advice: sort the advice related lists
(this branch is used by rj/advice-disable-how-to-disable.)
The error message given when "git branch -d branch" fails due to
commits unique to the branch has been split into an error and a new
conditional advice message.
Will merge to 'master'.
source: <4aedc15c-4b3f-4f5e-abea-581b501600f8@gmail.com>
* en/diffcore-delta-final-line-fix (2024-01-18) 1 commit
(merged to 'next' on 2024-01-22 at 7141d202cb)
+ diffcore-delta: avoid ignoring final 'line' of file
Rename detection logic ignored the final line of a file if it is an
incomplete line.
Will merge to 'master'.
source: <pull.1637.v2.git.1705119973690.gitgitgadget@gmail.com>
* mj/gitweb-unreadable-config-error (2024-01-10) 1 commit
(merged to 'next' on 2024-01-19 at 790b7a7855)
+ gitweb: die when a configuration file cannot be read
When given an existing but unreadable file as a configuration file,
gitweb behaved as if the file did not exist at all, but now it
errors out. This is a change that may break backward compatibility.
Will merge to 'master'.
source: <20240110225709.30168-1-marcelo.jimenez@gmail.com>
* ps/completion-with-reftable-fix (2024-01-16) 5 commits
(merged to 'next' on 2024-01-19 at 8854a7d903)
+ completion: treat dangling symrefs as existing pseudorefs
+ completion: silence pseudoref existence check
+ completion: improve existence check for pseudo-refs
+ t9902: verify that completion does not print anything
+ completion: discover repo path in `__git_pseudoref_exists ()`
Completion update to prepare for reftable
Will merge to 'master'.
source: <cover.1705314554.git.ps@pks.im>
* ps/reftable-optimize-io (2024-01-18) 7 commits
(merged to 'next' on 2024-01-22 at b867e8b9a8)
+ reftable/stack: fix race in up-to-date check
+ reftable/stack: unconditionally reload stack after commit
(merged to 'next' on 2024-01-12 at 4096e880e0)
+ reftable/blocksource: use mmap to read tables
+ reftable/blocksource: refactor code to match our coding style
+ reftable/stack: use stat info to avoid re-reading stack list
+ reftable/stack: refactor reloading to use file descriptor
+ reftable/stack: refactor stack reloading to have common exit path
Low-level I/O optimization for reftable.
Will merge to 'master'.
source: <cover.1704966670.git.ps@pks.im>
source: <cover.1705585037.git.ps@pks.im>
* rj/advice-disable-how-to-disable (2024-01-16) 2 commits
(merged to 'next' on 2024-01-23 at f456f4937d)
+ advice: allow disabling the automatic hint in advise_if_enabled()
+ Merge branch 'rj/advice-delete-branch-not-fully-merged' into rj/advice-disable-how-to-disable
(this branch uses rj/advice-delete-branch-not-fully-merged.)
All conditional "advice" messages show how to turn them off, which
becomes repetitive. Add a configuration variable to omit the
instruction.
Will merge to 'master'.
source: <6a842ef8-b390-4739-9eef-e867d55ed5ea@gmail.com>
* vd/fsck-submodule-url-test (2024-01-19) 4 commits
(merged to 'next' on 2024-01-19 at dad35ae82c)
+ submodule-config.c: strengthen URL fsck check
+ t7450: test submodule urls
+ test-submodule: remove command line handling for check-name
+ submodule-config.h: move check_submodule_url
Tighten URL checks fsck makes in a URL recorded for submodules.
Will merge to 'master'.
source: <pull.1635.v2.git.1705542918.gitgitgadget@gmail.com>
* sd/negotiate-trace-fix (2024-01-03) 1 commit
- push: region_leave trace for negotiate_using_fetch
Tracing fix.
Will merge to 'next'?
source: <20240103224054.1940209-1-delmerico@google.com>
* ps/worktree-refdb-initialization (2024-01-08) 7 commits
(merged to 'next' on 2024-01-19 at e8c649a3ac)
+ builtin/worktree: create refdb via ref backend
+ worktree: expose interface to look up worktree by name
+ builtin/worktree: move setup of commondir file earlier
+ refs/files: skip creation of "refs/{heads,tags}" for worktrees
+ setup: move creation of "refs/" into the files backend
+ refs: prepare `refs_init_db()` for initializing worktree refs
+ Merge branch 'ps/refstorage-extension' into ps/worktree-refdb-initialization
Instead of manually creating refs/ hierarchy on disk upon a
creation of a secondary worktree, which is only usable via the
files backend, use the refs API to populate it.
Will merge to 'master'.
source: <cover.1704705733.git.ps@pks.im>
* cp/apply-core-filemode (2023-12-26) 3 commits
- apply: code simplification
- apply: correctly reverse patch's pre- and post-image mode bits
- apply: ignore working tree filemode when !core.filemode
"git apply" on a filesystem without filemode support have learned
to take a hint from what is in the index for the path, even when
not working with the "--index" or "--cached" option, when checking
the executable bit match what is required by the preimage in the
patch.
Needs review.
source: <20231226233218.472054-1-gitster@pobox.com>
* al/unit-test-ctype (2024-01-16) 1 commit
(merged to 'next' on 2024-01-19 at fcdad0d06c)
+ unit-tests: rewrite t/helper/test-ctype.c as a unit test
Move test-ctype helper to the unit-test framework.
Will merge to 'master'.
source: <20240112102743.1440-1-ach.lumap@gmail.com>
* ja/doc-placeholders-fix (2023-12-26) 2 commits
- doc: enforce placeholders in documentation
- doc: enforce dashes in placeholders
Docfix.
Needs review.
source: <pull.1626.git.1703539287.gitgitgadget@gmail.com>
* jc/bisect-doc (2023-12-09) 1 commit
- bisect: document "terms" subcommand more fully
Doc update.
Needs review.
source: <xmqqzfyjmk02.fsf@gitster.g>
* tb/pair-chunk-expect (2023-11-10) 8 commits
- midx: read `OOFF` chunk with `pair_chunk_expect()`
- midx: read `OIDL` chunk with `pair_chunk_expect()`
- commit-graph: read `BIDX` chunk with `pair_chunk_expect()`
- commit-graph: read `GDAT` chunk with `pair_chunk_expect()`
- commit-graph: read `CDAT` chunk with `pair_chunk_expect()`
- commit-graph: read `OIDL` chunk with `pair_chunk_expect()`
- chunk-format: introduce `pair_chunk_expect()` helper
- Merge branch 'jk/chunk-bounds-more' into HEAD
Further code clean-up.
Needs review.
source: <cover.1699569246.git.me@ttaylorr.com>
* tb/path-filter-fix (2024-01-16) 17 commits
- bloom: introduce `deinit_bloom_filters()`
- commit-graph: reuse existing Bloom filters where possible
- object.h: fix mis-aligned flag bits table
- commit-graph: drop unnecessary `graph_read_bloom_data_context`
- commit-graph.c: unconditionally load Bloom filters
- bloom: prepare to discard incompatible Bloom filters
- bloom: annotate filters with hash version
- commit-graph: new Bloom filter version that fixes murmur3
- repo-settings: introduce commitgraph.changedPathsVersion
- t4216: test changed path filters with high bit paths
- t/helper/test-read-graph: implement `bloom-filters` mode
- bloom.h: make `load_bloom_filter_from_graph()` public
- t/helper/test-read-graph.c: extract `dump_graph_info()`
- gitformat-commit-graph: describe version 2 of BDAT
- commit-graph: ensure Bloom filters are read with consistent settings
- revision.c: consult Bloom filters for root commits
- t/t4216-log-bloom.sh: harden `test_bloom_filters_not_used()`
The Bloom filter used for path limited history traversal was broken
on systems whose "char" is unsigned; update the implementation and
bump the format version to 2.
Will merge to 'next'?
source: <cover.1705442923.git.me@ttaylorr.com>
* ak/color-decorate-symbols (2023-10-23) 7 commits
- log: add color.decorate.pseudoref config variable
- refs: exempt pseudorefs from pattern prefixing
- refs: add pseudorefs array and iteration functions
- log: add color.decorate.ref config variable
- log: add color.decorate.symbol config variable
- log: use designated inits for decoration_colors
- config: restructure color.decorate documentation
A new config for coloring.
Needs review.
source: <20231023221143.72489-1-andy.koppe@gmail.com>
* eb/hash-transition (2023-10-02) 30 commits
- t1016-compatObjectFormat: add tests to verify the conversion between objects
- t1006: test oid compatibility with cat-file
- t1006: rename sha1 to oid
- test-lib: compute the compatibility hash so tests may use it
- builtin/ls-tree: let the oid determine the output algorithm
- object-file: handle compat objects in check_object_signature
- tree-walk: init_tree_desc take an oid to get the hash algorithm
- builtin/cat-file: let the oid determine the output algorithm
- rev-parse: add an --output-object-format parameter
- repository: implement extensions.compatObjectFormat
- object-file: update object_info_extended to reencode objects
- object-file-convert: convert commits that embed signed tags
- object-file-convert: convert commit objects when writing
- object-file-convert: don't leak when converting tag objects
- object-file-convert: convert tag objects when writing
- object-file-convert: add a function to convert trees between algorithms
- object: factor out parse_mode out of fast-import and tree-walk into in object.h
- cache: add a function to read an OID of a specific algorithm
- tag: sign both hashes
- commit: export add_header_signature to support handling signatures on tags
- commit: convert mergetag before computing the signature of a commit
- commit: write commits for both hashes
- object-file: add a compat_oid_in parameter to write_object_file_flags
- object-file: update the loose object map when writing loose objects
- loose: compatibilty short name support
- loose: add a mapping between SHA-1 and SHA-256 for loose objects
- repository: add a compatibility hash algorithm
- object-names: support input of oids in any supported hash
- oid-array: teach oid-array to handle multiple kinds of oids
- object-file-convert: stubs for converting from one object format to another
Teach a repository to work with both SHA-1 and SHA-256 hash algorithms.
Needs review.
source: <878r8l929e.fsf@gmail.froward.int.ebiederm.org>
* jx/remote-archive-over-smart-http (2024-01-22) 6 commits
(merged to 'next' on 2024-01-23 at 5fa4633015)
+ transport-helper: call do_take_over() in process_connect
+ transport-helper: call do_take_over() in connect_helper
+ http-backend: new rpc-service for git-upload-archive
+ transport-helper: protocol v2 supports upload-archive
+ remote-curl: supports git-upload-archive service
+ transport-helper: no connection restriction in connect_helper
"git archive --remote=<remote>" learned to talk over the smart
http (aka stateless) transport.
Will merge to 'master'.
source: <cover.1705841443.git.zhiyou.jx@alibaba-inc.com>
* jc/rerere-cleanup (2023-08-25) 4 commits
- rerere: modernize use of empty strbuf
- rerere: try_merge() should use LL_MERGE_ERROR when it means an error
- rerere: fix comment on handle_file() helper
- rerere: simplify check_one_conflict() helper function
Code clean-up.
Not ready to be reviewed yet.
source: <20230824205456.1231371-1-gitster@pobox.com>
^ permalink raw reply
* Re: [PATCH] reftable: honor core.fsync
From: Junio C Hamano @ 2024-01-23 21:50 UTC (permalink / raw)
To: John Cai via GitGitGadget; +Cc: git, John Cai
In-Reply-To: <xmqq34unn8x4.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> A comment and a half.
>
> * Can't the new "how to flush" go to the write-option structure?
> If you represent "no flush" as a NULL pointer in the flush member,
> most of the changes to the _test files can go, no?
Nah, that was a stupid comment. These are used to populate the
members of the reftable_writer instance being created, and it does
make sense to have flush_func immediately next to writer_func.
The part about using NULL as the value to say "do not use any flusher"
still stands, though. You do not have to expose noop_flush into the
global namespace that way.
Thanks.
^ permalink raw reply
* Re: [PATCH] reftable: honor core.fsync
From: John Cai @ 2024-01-23 21:42 UTC (permalink / raw)
To: Junio C Hamano; +Cc: John Cai via GitGitGadget, git
In-Reply-To: <xmqq34unn8x4.fsf@gitster.g>
Hi Junio,
On 23 Jan 2024, at 14:31, Junio C Hamano wrote:
> "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> This commits adds a flush function pointer as a new member of
>> reftable_writer because we are not sure that the first argument to the
>> *write function pointer always contains a file descriptor. In the case of
>> strbuf_add_void, the first argument is a buffer. This way, we can pass
>> in a corresponding flush function that knows how to flush depending on
>> which writer is being used.
>
> A comment and a half.
>
> * Can't the new "how to flush" go to the write-option structure?
> If you represent "no flush" as a NULL pointer in the flush member,
> most of the changes to the _test files can go, no?
That's a good option and cuts down on code changes. Thanks for the suggestion.
>
> * For a function
>
> int func(int ac, char **av);
>
> a literal pointer to it can legally be written as either
>
> int (*funcp)(int, char **) = &func;
> int (*funcp)(int, char **) = func;
>
> but it is my understanding that this codebase prefers the latter,
> a tradition which goes back to 2005 when Linus was still writing
> a lot of code, i.e. the identifier that is the name of the
> function, without & in front.
good to know, thanks
John
^ permalink raw reply
* Re: [PATCH] reftable: honor core.fsync
From: John Cai @ 2024-01-23 21:38 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: git
In-Reply-To: <200cff64-cf53-4f91-bdf4-5afae2d2a127@app.fastmail.com>
Hi Kristoffer,
On 23 Jan 2024, at 16:06, Kristoffer Haugsbakk wrote:
> On Tue, Jan 23, 2024, at 19:51, John Cai via GitGitGadget wrote:
>> This commits adds a flush function pointer as a new member of
>
> I guess you meant singular “This commit”?
>
>> This commits adds a flush function pointer as a new member of
>> […]
>> This patch does not contain tests as they will need to wait for another
>
> Out of these two “This commit” is more true for the future `git log`.
yes this was a typo, thanks for catching that!
>
> --
> Kristoffer Haugsbakk
thanks
John
^ permalink raw reply
* Re: sd/negotiate-trace-fix
From: Junio C Hamano @ 2024-01-23 21:34 UTC (permalink / raw)
To: Josh Steadmon; +Cc: git, delmerico
In-Reply-To: <ZbAu49xkxEzJ3ZkX@google.com>
Josh Steadmon <steadmon@google.com> writes:
> On 2024.01.19 17:56, Junio C Hamano wrote:
> [snip]
>> * sd/negotiate-trace-fix (2024-01-03) 1 commit
>> - push: region_leave trace for negotiate_using_fetch
>>
>> Tracing fix.
>>
>> Waiting for a review response.
>> cf. <xmqqbka27zu9.fsf@gitster.g>
>> source: <20240103224054.1940209-1-delmerico@google.com>
>
> Hi Junio,
>
> Were there other open questions you had on this series? It looks like
> Sam answered all the questions in your xmqqbka27zu9.fsf@gitster.g reply.
Thanks for a reminder.
> In your other followup you mentioned running CI with tracing enabled for
> all tests. This is something that we suggested before [1] but it was
> pretty thoroughly rejected for runtime reasons. Perhaps a more efficient
> check for region_enter/leave pairs could be an option, though.
Yeah, perhaps. It does not sound like an issue that must block this
patch in either case.
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