* Re: [PATCH] use C99 declaration of variable in for() loop
From: Junio C Hamano @ 2024-02-15 23:43 UTC (permalink / raw)
To: brian m. carlson; +Cc: Elia Pinto, git
In-Reply-To: <Zc6abO6RV9RwpABR@tapette.crustytoothpaste.net>
"brian m. carlson" <sandals@crustytoothpaste.net> writes:
> I will also say that sending one giant patch for this may be a bit hard
> to review. While I will defer to Junio's opinion as the maintainer, I
> would be more inclined to review this kind of series if it came in in
> smaller patches, a few at a time, in which case I would find it a
> welcome improvement.
True. As to the specific topic of using "for (int i = 0; ...)",
it is tedious to review for mistakes and 17000+ lines of patch is
not a way to do so. I do not think I would be able to spot a change
in behaviour caused by a hunk like this
int i = 3;
... after some operations ...
- for (i = 0; i < 5; i++)
+ for (int i = 0; i < 5; i++)
if (condition_on_i(i))
break;
... after some operations ...
return i;
after scanning similar changes for 1000+ times in a single huge
patch.
^ permalink raw reply
* Re: [PATCH] cmake: let `test-tool` run the unit tests, too
From: Junio C Hamano @ 2024-02-15 23:45 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Josh Steadmon, Johannes Schindelin
In-Reply-To: <pull.1666.git.1708038924522.gitgitgadget@gmail.com>
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> The `test-tool` recently learned to run the unit tests. To this end, it
> needs to link with `test-lib.c`, which was done in the `Makefile`, and
> this patch does it in the CMake definition, too.
Nice. Will queue. Thanks.
> +add_library(test-lib OBJECT ${CMAKE_SOURCE_DIR}/t/unit-tests/test-lib.c)
>
> list(TRANSFORM test-tool_SOURCES PREPEND "${CMAKE_SOURCE_DIR}/t/helper/")
> add_executable(test-tool ${CMAKE_SOURCE_DIR}/t/helper/test-tool.c ${test-tool_SOURCES} ${test-reftable_SOURCES})
> -target_link_libraries(test-tool common-main)
> +target_link_libraries(test-tool test-lib common-main)
^ permalink raw reply
* Re: [PATCH v2 02/30] oid-array: teach oid-array to handle multiple kinds of oids
From: Linus Arver @ 2024-02-16 0:16 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Junio C Hamano, git, brian m. carlson, Eric Sunshine,
Eric W. Biederman
In-Reply-To: <8734tumekr.fsf@gmail.froward.int.ebiederm.org>
"Eric W. Biederman" <ebiederm@gmail.com> writes:
> Linus Arver <linusa@google.com> writes:
>
>> "Eric W. Biederman" <ebiederm@gmail.com> writes:
>>
>>> From: "Eric W. Biederman" <ebiederm@xmission.com>
>>>
>>> While looking at how to handle input of both SHA-1 and SHA-256 oids in
>>> get_oid_with_context, I realized that the oid_array in
>>> repo_for_each_abbrev might have more than one kind of oid stored in it
>>> simultaneously.
>>>
>>> Update to oid_array_append to ensure that oids added to an oid array
>>
>> s/Update to/Update
>>
>>> always have an algorithm set.
>>>
>>> Update void_hashcmp to first verify two oids use the same hash algorithm
>>> before comparing them to each other.
>>>
>>> With that oid-array should be safe to use with different kinds of
>>
>> s/oid-array/oid_array
>>
>>> oids simultaneously.
>>>
>>> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
>>> ---
>>> oid-array.c | 12 ++++++++++--
>>> 1 file changed, 10 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/oid-array.c b/oid-array.c
>>> index 8e4717746c31..1f36651754ed 100644
>>> --- a/oid-array.c
>>> +++ b/oid-array.c
>>> @@ -6,12 +6,20 @@ void oid_array_append(struct oid_array *array, const struct object_id *oid)
>>> {
>>> ALLOC_GROW(array->oid, array->nr + 1, array->alloc);
>>> oidcpy(&array->oid[array->nr++], oid);
>>> + if (!oid->algo)
>>> + oid_set_algo(&array->oid[array->nr - 1], the_hash_algo);
>>
>> How come we can't set oid->algo _before_ we call oidcpy()? It seems odd
>> that we do the copy first and then modify what we just copied after the
>> fact, instead of making sure that the thing we want to copy is correct
>> before doing the copy.
>>
>> But also, if we are going to make the oid object "correct" before
>> invoking oidcpy(), we might as well do it when the oid is first
>> created/used (in the caller(s) of this function). I don't demand that
>> you find/demonstrate where all these places are in this series (maybe
>> that's a hairy problem to tackle?), but it seems cleaner in principle to
>> fix the creation of oid objects instead of having to make oid users
>> clean up their act like this after using them.
>
> There is a hairy problem here.
>
> I believe for reasons of simplicity when the algo field was added to
> struct object_id it was allowed to be zero for users that don't
> particularly care about the hash algorithm, and are happy to use the git
> default hash algorithm.
>
> Me experience working on this set of change set showed that there
> are oids without their algo set in all kinds of places in the tree.
Ah, I see. Thanks for the clarification.
> I could not think of any sure way to go through the entire tree
> and find those users, so I just made certain that oid array handled
> that case.
>
> I need algo to be set properly in the oids in the oid array so I
> could extend oid_array to hold multiple kinds of oids at the same
> time. To allow multiple kinds of oids at the same time void_hashcmp
> needs a simple and reliable way to tell what the algorithm is of
> any given oid.
Makes sense.
>>
>>> array->sorted = 0;
>>> }
>>>
>>> -static int void_hashcmp(const void *a, const void *b)
>>> +static int void_hashcmp(const void *va, const void *vb)
>>> {
>>> - return oidcmp(a, b);
>>> + const struct object_id *a = va, *b = vb;
>>> + int ret;
>>> + if (a->algo == b->algo)
>>> + ret = oidcmp(a, b);
>>
>> This makes sense (per the commit message description) ...
>>
>>> + else
>>> + ret = a->algo > b->algo ? 1 : -1;
>>
>> ... but this seems to go against it? I thought you wanted to only ever
>> compare hashes if they were of the same algo? It would be good to add a
>> comment explaining why this is OK (we are no longer doing a byte-by-byte
>> comparison of these oids any more here like we do for oidcmp() above
>> which boils down to calling memcmp()).
>
> So the goal of this change is for oid_array to be able to hold hashes
> from multiple algorithms at the same time.
>
> A key part of oid_array is oid_array_sort that allows functions such
> as oid_array_lookup and oid_array_for_each_unique.
>
> To that end there needs to be a total ordering of oids.
>
> The function oidcmp is only defined when two oids are of the same
> algorithm, it does not even test to detect the case of comparing
> mismatched algorithms.
>
> Therefore to get a total ordering of oids. I must use oidcmp
> when the algorithm is the same (the common case) or simply order
> the oids by algorithm when the algorithms are different.
>
>
>
> All of this is relevant to get_oid_with_context as get_oid_with_context
> and it's helper functions contain the logic that determines what
> we do when a hex string that is ambiguous is specified.
>
> In the ambiguous case all of the possible candidates are placed in
> an oid_array, sorted and then displayed.
>
>
> With a repository that can knows both the sha1 and the sha256 oid
> of it's objects it is possible for a short oid to match both
> some sha1 oids and some sha256 oids.
Thanks for the additional clarification. I think a lot of this could
have been added as comments or perhaps in the commit message. The "short
id can match both sha1 or sha256" is a very real scenario we need to
consider in the sha1+sha256 world, indeed.
>>> + return ret;
>>
>> Also, in terms of style I think the "early return for errors" style
>> would be simpler to read. I.e.
>>
>> if (a->algo > b->algo)
>> return 1;
>>
>> if (a->algo < b->algo)
>> return -1;
>>
>> return oidcmd(a, b);
>>
>
> I can see doing:
> if (a->algo == b->algo)
> return oidcmp(a,b);
>
> if (a->algo > b->algo)
> return 1;
> else
> return -1;
>
> Or even:
> if (a->algo == b->algo)
> return oidcmp(a,b);
>
> return a->algo - b->algo;
>
> Although I suspect using subtraction is a bit too clever.
Agreed.
> Comparing for less than, and greater than, and then assuming
> the values are equal hides what is important before calling
> oidcmp which is that the algo values are equal.
I would still prefer the "early return for errors" style even in this
case. This is because I much prefer to have the question "how can things
go wrong?" answered first, and dealt with, such that as I read
top-to-bottom I am left with less things I have to consider to
understand the "happy path". WRT emphasizing the "algos equal each
other" concern, a simple comment like
/* Only compare equal algorithms. */
return oidcmp(a, b);
seems sufficient.
But, of course it is possible (perhaps even likely) that my preferred
style is in the minority. Up to you. Thanks.
^ permalink raw reply
* Re: [PATCH v2 2/4] oidset: refactor oidset_insert_from_set()
From: Linus Arver @ 2024-02-16 1:10 UTC (permalink / raw)
To: Christian Couder
Cc: git, Junio C Hamano, Patrick Steinhardt, John Cai,
Christian Couder
In-Reply-To: <CAP8UFD1aJD5i68ekHuq0UG14X19y=Eo6qKPianF8MKNf6iZ_WQ@mail.gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
> On Tue, Feb 13, 2024 at 10:02 PM Linus Arver <linusa@google.com> wrote:
>>
>> Christian Couder <christian.couder@gmail.com> writes:
>>
>> > In a following commit, we will need to add all the oids from a set into
>> > another set. In "list-objects-filter.c", there is already a static
>> > function called add_all() to do that.
>>
>> Nice find.
>>
>> > Let's rename this function oidset_insert_from_set() and move it into
>> > oidset.{c,h} to make it generally available.
>>
>> At some point (I don't ask for it in this series) we should add unit
>> tests for this newly-exposed function. Presumably the stuff around
>> object/oid handling is stable enough to receive unit tests.
>
> Yeah, ideally there should be unit tests for oidset and all its
> features, but it seems to me that there aren't any. Also oidset is
> based on khash.h which was originally imported from
> https://github.com/attractivechaos/klib/ without tests. So I think
> it's a different topic to add tests from scratch to oidset, khash.h or
> both.
>
> Actually after taking another look, it looks like khash.h or some of
> its features are tested through "helper/test-oidmap.c" and
> "t0016-oidmap.sh". I still think it's another topic to test oidset.
Agreed.
>> > +void oidset_insert_from_set(struct oidset *dest, struct oidset *src)
>> > +{
>> > + struct oidset_iter iter;
>> > + struct object_id *src_oid;
>> > +
>> > + oidset_iter_init(src, &iter);
>> > + while ((src_oid = oidset_iter_next(&iter)))
>>
>> Are the extra parentheses necessary?
>
> Yes. Without them gcc errors out with:
>
> oidset.c: In function ‘oidset_insert_from_set’:
> oidset.c:32:16: error: suggest parentheses around assignment used as
> truth value [-Werror=parentheses]
> 32 | while (src_oid = oidset_iter_next(&iter))
> | ^~~~~~~
>
> Having extra parentheses is a way to tell the compiler that we do want
> to use '=' and not '=='. This helps avoid the very common mistake of
> using '=' where '==' was intended.
Ah, so it is a "please trust me gcc, I know what I am doing" thing and
not a "this is required in C" thing. Makes sense, thanks for clarifying.
Sorry for the noise.
>> > +/**
>> > + * Insert all the oids that are in set 'src' into set 'dest'; a copy
>> > + * is made of each oid inserted into set 'dest'.
>> > + */
>>
>> Just above in oid_insert() there is already a comment about needing to
>> copy each oid.
>
> (It's "oidset_insert()" not "oid_insert()".)
Oops, yes, sorry for the typo.
>> /**
>> * Insert the oid into the set; a copy is made, so "oid" does not need
>> * to persist after this function is called.
>> *
>> * Returns 1 if the oid was already in the set, 0 otherwise. This can be used
>> * to perform an efficient check-and-add.
>> */
>>
>> so perhaps the following wording is simpler?
>>
>> Like oid_insert(), but insert all oids found in 'src'. Calls
>> oid_insert() internally.
>
> (What you suggest would need s/oid_insert/oidset_insert/)
>
> Yeah, it's a bit simpler and shorter, but on the other hand a reader
> might have to read both this and the oidset_insert() doc, so in the
> end I am not sure it's a big win for readability. And if they don't
> read the oidset_insert() doc, they might miss the fact that we are
> copying the oids we insert, which might result in a bug.
When functions are built on top of other functions, I think it is good
practice to point readers to those underlying functions. In this case
the new function is a wrapper around oidset_insert() which does all the
real work. Plus the helper function already has some documentation about
copying behavior that we already thought was important enough to call
out explicitly.
So, tying this definition to that (foundational) helper function sounds
like a good idea to me in terms of readability. IOW we can inform
readers "hey, we're just a wrapper around this other important function
--- go there if you're curious about internals" and emphasizing that
sort of relationship which may not be immediately obvious to those not
familiar with this area would be nice.
Alternatively, we could repeat the same comment WRT copying here but
that seems redundant and prone to maintenance burdens down the road (if
we ever change this behavior we have to change the comment in multiple
functions, possibly).
> Also your wording ties the implementation with oidset_insert(), which
> we might not want if we could find something more performant. See
> Junio's comment on this patch saying his initial reaction was that
> copying underlying bits may even be more efficient.
>
> So I prefer not to change this.
OK.
>> > +void oidset_insert_from_set(struct oidset *dest, struct oidset *src);
>>
>> Perhaps "oidset_insert_all" would be a simpler name? I generally prefer
>> to reuse any descriptors in comments to guide the names. Plus this
>> function used to be called "add_all()" so keeping the "all" naming style
>> feels right.
>
> We already have other related types like 'struct oid-array' and
> 'struct oidmap' to store oids, as well as code that inserts many oids
> into an oidset from a 'struct ref *' linked list or array in a tight
> loop.
Thank you for the additional context I was not aware of.
> So if we want to add functions inserting all the oids from
> instances of such types, how should we call them?
>
> I would say we should use suffixes like: "_from_set", "_from_map",
> "from_array", "_from_ref_list", "_from_ref_array", etc.
I agree.
However, I would like to point out that the function being added in this
patch is a bit special: it is inserting from one "oidset" into another
"oidset". IOW the both the dest and src types are the same.
For the cases where the types are different, I totally agree that using
the suffixes (to encode the type information of the src into the
function name itself) is a good idea.
So I think it's still fine to use "oidset_insert_all" because the only
type in the parameter list is an oidset.
BUT, maybe in our codebase we already use suffixes like this even for
cases where the types are the same? I don't know the answer to this
question. However if we really wanted to be consistent then maybe we
should be using the name oidset_insert_from_oidset() and not
oidset_insert_from_set().
> If we start using just "_all" for oidset, then what should we use for
> the other types? I don't see a good answer to that, so I prefer to
> stick with "_from_set" for oidset.
>
> Thanks.
^ permalink raw reply
* Re: [PATCH v2 4/4] rev-list: allow missing tips with --missing=[print|allow*]
From: Linus Arver @ 2024-02-16 1:24 UTC (permalink / raw)
To: Christian Couder
Cc: git, Junio C Hamano, Patrick Steinhardt, John Cai,
Christian Couder
In-Reply-To: <CAP8UFD3N8h4FnfvFYYWrV54a7WcOwHY862DjxxPKSKr4jEwU7Q@mail.gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
> On Tue, Feb 13, 2024 at 11:33 PM Linus Arver <linusa@google.com> wrote:
>>
>> Christian Couder <christian.couder@gmail.com> writes:
>> > While at it let's add a NEEDSWORK comment to say that we should get
>> > rid of the existing ugly dumb loops that parse the
>> > `--exclude-promisor-objects` and `--missing=...` options early.
>
>> > @@ -1019,6 +1019,10 @@ Unexpected missing objects will raise an error.
>> > +
>> > The form '--missing=print' is like 'allow-any', but will also print a
>> > list of the missing objects. Object IDs are prefixed with a ``?'' character.
>> > ++
>> > +If some tips passed to the traversal are missing, they will be
>> > +considered as missing too, and the traversal will ignore them. In case
>> > +we cannot get their Object ID though, an error will be raised.
>>
>> The only other mention of the term "tips" is for the "--alternate-refs"
>> flag on line 215 which uses "ref tips". Perhaps this is obvious to
>> veteran Git developers but I do wonder if we need to somehow define it
>> (however briefly) the first time we mention it (either in the document
>> as a whole, or just within this newly added paragraph).
>
> I did a quick grep in Documentation/git*.txt and found more than 130
> instances of the 'tip' word. So I think it is quite common in the
> whole Git documentation and our glossary would likely be the right
> place to document it if we decide to do so.
SGTM.
> Anyway I think that topic
> is independent from this small series.
Agreed.
>> Here's an alternate wording
>>
>> Ref tips given on the command line are a special case.
>
> `git rev-list` has a `--stdin` mode which makes it accept tips from
> stdin
Ah, thanks for the context.
> , so talking about the command line is not enough. Also maybe one
> day some config option could be added that makes the command include
> additional tips.
>> They are
>> first dereferenced to Object IDs (if this is not possible, an error
>> will be raised). Then these IDs are checked to see if the objects
>> they refer to exist. If so, the traversal happens starting with
>> these tips. Otherwise, then such tips will not be used for
>> traversal.
>>
>> Even though such missing tips won't be included for traversal, for
>> purposes of the `--missing` flag they will be treated the same way
>> as those objects that did get traversed (and were determined to be
>> missing). For example, if `--missing=print` is given then the Object
>> IDs of these tips will be printed just like all other missing
>> objects encountered during traversal.
>
> Your wording describes what happens correctly, but I don't see much
> added value for this specific patch compared to my wording which is
> shorter.
>
> Here I think, we only need to describe the result of the command in
> the special case that the patch is fixing. We don't need to go into
> details of how the command or even --missing works. It could be
> interesting to go into details of how things work, but I think it's a
> separate topic. Or perhaps it's even actually counter productive to go
> into too much detail as it could prevent us from finding other ways to
> make it work better. Anyway it seems to me to be a separate topic to
> discuss.
Fair enough.
>> But also, I realize that these documentation touch-ups might be better
>> served by an overall pass over the whole document, so it's fine if we
>> decide not to take this suggestion at this time.
>
> Right, I agree. Thanks for telling this.
>
>> Aside: unfortunately we don't seem to define the relationship between
>> ref tips (e.g., "HEAD"), object IDs (40-char hex string), and the actual
>> objects (the real data that we traverse over). It's probably another
>> thing that could be fixed up in the docs in the future.
>
> Yeah, and for rev-list a tip could also be a tree or a blob. It
> doesn't need to be a "ref tip". (Even though a ref can point to a tree
> or a blog, it's very rare in practice.)
Interesting, thanks for the info.
BTW I appreciate you (and others on the list too) taking the time to
explain such subtleties. Although I've been using Git since 2008 a lot
of the terms used around in the codebase can feel quite foreign to me.
So, thanks again.
>> > + * to manage the `--exclude-promisor-objects` and `--missing=...`
>> > + * options below.
>> > + */
>> > for (i = 1; i < argc; i++) {
>> > const char *arg = argv[i];
>> > if (!strcmp(arg, "--exclude-promisor-objects")) {
>> >
>> > [...]
>> >
>> > @@ -2178,13 +2183,18 @@ static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int fl
>> > if (revarg_opt & REVARG_COMMITTISH)
>> > get_sha1_flags |= GET_OID_COMMITTISH;
>> >
>> > + /*
>> > + * Even if revs->do_not_die_on_missing_objects is set, we
>> > + * should error out if we can't even get an oid, ...
>> > + */
>>
>> Perhaps this wording is more precise?
>>
>> If we can't even get an oid, we are forced to error out (regardless
>> of revs->do_not_die_on_missing_objects) because a valid traversal
>> must start from *some* valid oid. OTOH we ignore the ref tip
>> altogether with revs->ignore_missing.
>
> This uses "valid oid" and "valid traversal", but I am not sure it's
> easy to understand what "valid" means in both of these expressions.
>
> Also if all the tips passed to the command are missing, the traversal
> doesn't need to actually start. The command, assuming
> `--missing=print` is passed, just needs to output the oids of the tips
> as missing oids.
>
> I also think that "ref tip" might be misleading as trees and blos can
> be passed as tips.
>
> So I prefer to keep the wording I used.
Makes sense, SGTM.
>> > + * ... as
>> > + * `--missing=print` should be able to report missing oids.
>>
>> I think this comment would be better placed ...
>>
>> > if (get_oid_with_context(revs->repo, arg, get_sha1_flags, &oid, &oc))
>> > return revs->ignore_missing ? 0 : -1;
>> > if (!cant_be_filename)
>> > verify_non_filename(revs->prefix, arg);
>> > object = get_reference(revs, arg, &oid, flags ^ local_flags);
>>
>> ... around here.
>
> In a previous round, I was asked to put such a comment before `if
> (get_oid_with_context(...))`.
Sorry, I missed that.
> So I prefer to avoid some back and forth
> here.
SGTM.
>> > +++ b/t/t6022-rev-list-missing.sh
>> > @@ -78,4 +78,60 @@ do
>> > done
>> > done
>> >
>> > +for missing_tip in "HEAD~1" "HEAD~1^{tree}" "HEAD:1.t"
>> > +do
>> > + # We want to check that things work when both
>> > + # - all the tips passed are missing (case existing_tip = ""), and
>> > + # - there is one missing tip and one existing tip (case existing_tip = "HEAD")
>> > + for existing_tip in "" "HEAD"
>> > + do
>>
>> Though I am biased, these new variable names do make this test that much
>> easier to read. Thanks.
>
> Thanks for suggesting them and for your reviews.
You're welcome!
^ permalink raw reply
* What's cooking in git.git (Feb 2024, #06; Thu, 15)
From: Junio C Hamano @ 2024-02-16 1: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).
Git 2.44-rc1 has been tagged. This round, let's try having just one
release candidate before the final, which is expected to be tagged
on or around 20th.
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]
* jh/fsmonitor-icase-corner-case-fix (2024-02-14) 11 commits
- t7527: update case-insenstive fsmonitor test
- fsmonitor: refactor bit invalidation in refresh callback
- fsmonitor: support case-insensitive non-directory events
- fsmonitor: refactor non-directory callback
- fsmonitor: support case-insensitive directory events
- fsmonitor: refactor untracked-cache invalidation
- fsmonitor: clarify handling of directory events in callback
- fsmonitor: refactor refresh callback for non-directory events
- fsmonitor: refactor refresh callback on directory events
- t7527: add case-insensitve test for FSMonitor
- name-hash: add index_dir_exists2()
FSMonitor client code was confused when FSEvents were given in a
different case on a case-insensitive filesystem, which has been
corrected.
Needs review.
source: <pull.1662.git.1707857541.gitgitgadget@gmail.com>
* mh/credential-oauth-refresh-token-with-osxkeychain (2024-02-14) 1 commit
- credential/osxkeychain: store new attributes
OAuth refresh tokens and password expiry timestamps are now stored
in the osxkeychain backend , just the way libsecret and wincred
backends of the credential subsystem do.
Needs testing.
cf. <CAGJzqsmSzMqEG1OU9dH6CORV6=L7qUAFNJSmi41Lqrajf9mSew@mail.gmail.com>
source: <pull.1663.git.1707860618119.gitgitgadget@gmail.com>
* ps/reftable-iteration-perf-part2 (2024-02-14) 13 commits
- reftable: allow inlining of a few functions
- reftable/record: decode keys in place
- reftable/record: reuse refname when copying
- reftable/record: reuse refname when decoding
- reftable/merged: avoid duplicate pqueue emptiness check
- reftable/merged: circumvent pqueue with single subiter
- reftable/merged: handle subiter cleanup on close only
- reftable/merged: remove unnecessary null check for subiters
- reftable/merged: make subiters own their records
- reftable/merged: advance subiter on subsequent iteration
- reftable/merged: make `merged_iter` structure private
- reftable/pq: use `size_t` to track iterator index
- Merge branch 'ps/reftable-iteration-perf' into ps/reftable-iteration-perf-part2
(this branch uses ps/reftable-iteration-perf.)
The code to iterate over refs with the reftable backend has seen
some optimization.
Needs review.
source: <cover.1707895758.git.ps@pks.im>
* cp/t9146-use-test-path-helpers (2024-02-14) 1 commit
- t9146: replace test -d/-e/-f with appropriate test_path_is_* function
Test script clean-up.
source: <pull.1661.v3.git.1707933048210.gitgitgadget@gmail.com>
* rj/tag-column-fix (2024-02-14) 1 commit
- tag: error when git-column fails
"git tag --column" failed to check the exit status of its "git
column" invocation, which has been corrected.
Will merge to 'next'?
source: <59df085d-0de8-45b1-9b8b-c69e91e56a1f@gmail.com>
* jc/am-whitespace-doc (2024-02-14) 1 commit
- doc: add shortcut to "am --whitespace=<action>"
"git am --help" now tells readers what actions are available in
"git am --whitespace=<action>", in addition to saying that the
option is passed through to the underlying "git apply".
Will merge to 'next'?
source: <xmqqplwyvqby.fsf@gitster.g>
* ba/credential-test-clean-fix (2024-02-15) 1 commit
- t/lib-credential: clean additional credential
Test clean-up.
Will merge to 'next'?
source: <pull.1664.git.1707959036807.gitgitgadget@gmail.com>
* js/cmake-with-test-tool (2024-02-15) 1 commit
- cmake: let `test-tool` run the unit tests, too
(this branch uses js/unit-test-suite-runner.)
"test-tool" is now built in CMake build to also run the unit tests.
May want to roll it into the base topic.
source: <cover.1706921262.git.steadmon@google.com>
--------------------------------------------------
[Graduated to 'master']
* cp/git-flush-is-an-env-bool (2024-02-13) 1 commit
(merged to 'next' on 2024-02-13 at c0850f5675)
+ write-or-die: fix the polarity of GIT_FLUSH environment variable
Recent conversion to allow more than 0/1 in GIT_FLUSH broke the
mechanism by flipping what yes/no means by mistake, which has been
corrected.
Will merge to 'master' and then to 'maint'
source: <xmqqbk8k5eo0.fsf@gitster.g>
* jc/github-actions-update (2024-02-02) 3 commits
(merged to 'next' on 2024-02-07 at 2cd6caaf70)
+ Merge branch 'jc/maint-github-actions-update' into jc/github-actions-update
+ GitHub Actions: update to github-script@v7
+ GitHub Actions: update to checkout@v4
(this branch is used by js/github-actions-update.)
Squelch node.js 16 deprecation warnings from GitHub Actions CI
by updating actions/github-script and actions/checkout that use
node.js 20.
source: <20240202203935.1240458-1-gitster@pobox.com>
* jc/unit-tests-make-relative-fix (2024-02-12) 1 commit
(merged to 'next' on 2024-02-12 at 554eddef80)
+ unit-tests: do show relative file paths on non-Windows, too
The mechanism to report the filename in the source code, used by
the unit-test machinery, assumed that the compiler expanded __FILE__
to the path to the source given to the $(CC), but some compilers
give full path, breaking the output. This has been corrected.
source: <xmqqle7r9enn.fsf_-_@gitster.g>
* js/check-null-from-read-object-file (2024-02-06) 1 commit
(merged to 'next' on 2024-02-12 at 3a18369516)
+ Always check the return value of `repo_read_object_file()`
The code paths that call repo_read_object_file() have been
tightened to react to errors.
source: <pull.1650.git.1707143753726.gitgitgadget@gmail.com>
* js/github-actions-update (2024-02-12) 2 commits
(merged to 'next' on 2024-02-12 at f52de8b126)
+ ci(linux32): add a note about Actions that must not be updated
+ ci: bump remaining outdated Actions versions
(this branch uses jc/github-actions-update.)
Update remaining GitHub Actions jobs to avoid warnings against
using deprecated version of Node.js.
source: <pull.1660.v2.git.1707653489.gitgitgadget@gmail.com>
* jx/dirstat-parseopt-help (2024-02-14) 1 commit
(merged to 'next' on 2024-02-14 at 901101e5f5)
+ diff: mark param1 and param2 as placeholders
The mark-up of diff options has been updated to help translators.
source: <3a82f72f33663f162aa41cb20c0fb3b6786971c9.1707900029.git.worldhello.net@gmail.com>
* pb/complete-config (2024-02-12) 4 commits
(merged to 'next' on 2024-02-13 at d09f5e469a)
+ completion: add and use __git_compute_second_level_config_vars_for_section
+ completion: add and use __git_compute_first_level_config_vars_for_section
+ completion: complete 'submodule.*' config variables
+ completion: add space after config variable names also in Bash 3
The command line completion script (in contrib/) learned to
complete configuration variable names better.
cf. <Zcs34kGTqTbIana6@tanuki>
source: <pull.1660.v3.git.git.1707589943.gitgitgadget@gmail.com>
* pw/gc-during-rebase (2024-02-09) 1 commit
(merged to 'next' on 2024-02-12 at d54c5ce325)
+ prune: mark rebase autostash and orig-head as reachable
The sequencer machinery does not use the ref API and instead
records names of certain objects it needs for its correct operation
in temporary files, which makes these objects susceptible to loss
by garbage collection. These temporary files have been added as
starting points for reachability analysis to fix this.
source: <pull.1656.v2.git.1707495579886.gitgitgadget@gmail.com>
* rs/receive-pack-remove-find-header (2024-02-12) 2 commits
(merged to 'next' on 2024-02-12 at f1bf281e10)
+ receive-pack: use find_commit_header() in check_nonce()
+ receive-pack: use find_commit_header() in check_cert_push_options()
Code simplification.
source: <8b350cae-2180-4ac7-a911-d40043576445@web.de>
* vn/rebase-with-cherry-pick-authorship (2024-02-08) 1 commit
(merged to 'next' on 2024-02-09 at ed35d33595)
+ sequencer: unset GIT_CHERRY_PICK_HELP for 'exec' commands
"git cherry-pick" invoked during "git rebase -i" session lost
the authorship information, which has been corrected.
source: <0adb1068-ef10-44ed-ad1d-e0927a09245d@gmail.com>
--------------------------------------------------
[Cooking]
* ps/ref-tests-update-even-more (2024-02-15) 7 commits
(merged to 'next' on 2024-02-15 at 064b2b4089)
+ t7003: ensure filter-branch prunes reflogs with the reftable backend
+ t2011: exercise D/F conflicts with HEAD with the reftable backend
+ t1405: remove unneeded cleanup step
+ t1404: make D/F conflict tests compatible with reftable backend
+ t1400: exercise reflog with gaps with reftable backend
+ t0410: convert tests to use DEFAULT_REPO_FORMAT prereq
+ t: move tests exercising the "files" backend
More tests that are marked as "ref-files only" have been updated to
improve test coverage of reftable backend.
Will cook in 'next'.
source: <cover.1707985173.git.ps@pks.im>
* rs/use-xstrncmpz (2024-02-12) 1 commit
(merged to 'next' on 2024-02-12 at 37e5f0fc14)
+ use xstrncmpz()
Code clean-up.
Will cook in 'next'.
source: <954b75d0-1504-4f57-b34e-e770a4b7b3ea@web.de>
* kn/for-all-refs (2024-02-12) 6 commits
- for-each-ref: add new option to include root refs
- ref-filter: rename 'FILTER_REFS_ALL' to 'FILTER_REFS_REGULAR'
- refs: introduce `refs_for_each_include_root_refs()`
- refs: extract out `loose_fill_ref_dir_regular_file()`
- refs: introduce `is_pseudoref()` and `is_headref()`
- Merge branch 'ps/reftable-backend' into kn/for-all-refs
(this branch uses ps/reftable-backend.)
"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.
Will merge to 'next'?
source: <20240211183923.131278-1-karthik.188@gmail.com>
* kh/column-reject-negative-padding (2024-02-13) 2 commits
(merged to 'next' on 2024-02-14 at c30c08e495)
+ column: guard against negative padding
+ column: disallow negative padding
"git column" has been taught to reject negative padding value, as
it would lead to nonsense behaviour including division by zero.
Will cook in 'next'.
source: <cover.1707839454.git.code@khaugsbakk.name>
* jc/no-lazy-fetch (2024-02-13) 1 commit
(merged to 'next' on 2024-02-13 at 7c7136e547)
+ git: --no-lazy-fetch option
"git --no-lazy-fetch cmd" allows to run "cmd" while disabling lazy
fetching of objects from the promisor remote, which may be handy
for debugging.
May want to do the environment variable thing as well.
cf. <20240215053056.GD2821179@coredump.intra.peff.net>
source: <xmqq1q9mmtpw.fsf@gitster.g>
* jc/t9210-lazy-fix (2024-02-08) 1 commit
(merged to 'next' on 2024-02-13 at fb61ca2fba)
+ t9210: do not rely on lazy fetching to fail
(this branch is used by cc/rev-list-allow-missing-tips.)
Adjust use of "rev-list --missing" in an existing tests so that it
does not depend on a buggy failure mode.
Will cook in 'next'.
source: <xmqq7cjemttr.fsf@gitster.g>
* gt/at-is-synonym-for-head-in-add-patch (2024-02-13) 2 commits
(merged to 'next' on 2024-02-14 at cd901555d6)
+ add -p tests: remove PERL prerequisites
+ add-patch: classify '@' as a synonym for 'HEAD'
Teach "git checkout -p" and friends that "@" is a synonym for
"HEAD".
Will cook in 'next'.
source: <20240211202035.7196-2-shyamthakkar001@gmail.com>
* js/unit-test-suite-runner (2024-02-03) 7 commits
- t/Makefile: run unit tests alongside shell tests
- unit tests: add rule for running with test-tool
- test-tool run-command testsuite: support unit tests
- test-tool run-command testsuite: remove hardcoded filter
- test-tool run-command testsuite: get shell from env
- t0080: turn t-basic unit test into a helper
- Merge branch 'jk/unit-tests-buildfix' into js/unit-test-suite-runner
(this branch is used by js/cmake-with-test-tool.)
The "test-tool" has been taught to run testsuite tests in parallel,
bypassing the need to use the "prove" tool.
Expecting a reroll.
cf. <20240207225802.GA538110@coredump.intra.peff.net>
source: <cover.1706921262.git.steadmon@google.com>
* ps/reftable-backend (2024-02-07) 3 commits
(merged to 'next' on 2024-02-08 at ba1c4c52bb)
+ refs/reftable: fix leak when copying reflog fails
(merged to 'next' on 2024-02-07 at 1115200acb)
+ ci: add jobs to test with the reftable backend
+ refs: introduce reftable backend
(this branch is used by kn/for-all-refs.)
Integrate the reftable code into the refs framework as a backend.
Will cook in 'next'.
source: <cover.1707288261.git.ps@pks.im>
* cc/rev-list-allow-missing-tips (2024-02-14) 4 commits
- rev-list: allow missing tips with --missing=[print|allow*]
- t6022: fix 'test' style and 'even though' typo
- oidset: refactor oidset_insert_from_set()
- revision: clarify a 'return NULL' in get_reference()
(this branch uses jc/t9210-lazy-fix.)
"git rev-list --missing=print" have learned to optionally take
"--allow-missing-tips", which allows the objects at the starting
points to be missing.
Will merge to 'next'?
source: <20240214142513.4002639-1-christian.couder@gmail.com>
* ps/reftable-iteration-perf (2024-02-12) 7 commits
(merged to 'next' on 2024-02-12 at 6abaf58383)
+ reftable/reader: add comments to `table_iter_next()`
+ reftable/record: don't try to reallocate ref record name
+ reftable/block: swap buffers instead of copying
+ reftable/pq: allocation-less comparison of entry keys
+ reftable/merged: skip comparison for records of the same subiter
+ reftable/merged: allocation-less dropping of shadowed records
+ reftable/record: introduce function to compare records by key
(this branch is used by ps/reftable-iteration-perf-part2.)
The code to iterate over refs with the reftable backend has seen
some optimization.
Will cook in 'next'.
source: <cover.1707726654.git.ps@pks.im>
* js/merge-tree-3-trees (2024-02-07) 6 commits
- cache-tree: avoid an unnecessary check
- Always check `parse_tree*()`'s return value
- t4301: verify that merge-tree fails on missing blob objects
- merge-ort: do check `parse_tree()`'s return value
- merge-tree: fail with a non-zero exit code on missing tree objects
(merged to 'next' on 2024-01-30 at 0c77b04e59)
+ merge-tree: accept 3 trees as arguments
"git merge-tree" has learned that the three trees involved in the
3-way merge only need to be trees, not necessarily commits.
Expecting a reroll.
cf. <CAPig+cSs8MFkLasTULh7tybrFm7SwaT+JeR7HnXjh+-agCHYMw@mail.gmail.com>
cf. <CAPig+cSJz3U+vT==NhX5hcrTjsCggnAzhzQOvZcSXbcEGuYaKQ@mail.gmail.com>
source: <pull.1647.v2.git.1706474063109.gitgitgadget@gmail.com>
source: <pull.1651.v2.git.1707324461.gitgitgadget@gmail.com>
* rj/complete-reflog (2024-01-26) 4 commits
- completion: reflog show <log-options>
- completion: reflog with implicit "show"
- completion: introduce __git_find_subcommand
- completion: introduce __gitcomp_subcommand
The command line completion script (in contrib/) learned to
complete "git reflog" better.
Needs review.
source: <98daf977-dbad-4d3b-a293-6a769895088f@gmail.com>
* ml/log-merge-with-cherry-pick-and-other-pseudo-heads (2024-02-12) 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.
Expecting a reroll.
cf. <790a3f11-5a8c-42f2-7a35-f2900c0299b4@gmail.com>
cf. <8384d1dc-b6c4-b853-9bf6-3d7ccee86d12@gmail.com>
source: <20240210-ml-log-merge-with-cherry-pick-and-other-pseudo-heads-v4-0-3bc9e62808f4@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.
Expecting a reroll.
cf. <40c3a824-a961-490b-94d4-4eb23c8f713d@gmail.com>
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.
cf. <xmqq4jej6i1b.fsf@gitster.g>
source: <a718b5ee-afb0-44bd-a299-3208fac43506@smtp-relay.sendinblue.com>
* la/trailer-api (2024-02-06) 28 commits
- trailer: introduce "template" term for readability
- trailer_set_*(): put out parameter at the end
- trailer: unify "--trailer ..." arg handling
- trailer: deprecate "new_trailer_item" struct from API
- trailer_add_arg_item(): drop new_trailer_item usage
- trailer: add new helper functions to API
- trailer: prepare to delete "parse_trailers_from_command_line_args()"
- trailer: spread usage of "trailer_block" language
- trailer: retire trailer_info_get() from API
- trailer: make trailer_info struct private
- sequencer: use the trailer iterator
- trailer: teach iterator about non-trailer lines
- trailer: finish formatting unification
- format_trailer_info(): avoid double-printing the separator
- format_trailer_info(): teach it about opts->trim_empty
- trailer: begin formatting unification
- format_trailer_info(): append newline for non-trailer lines
- format_trailer_info(): drop redundant unfold_value()
- format_trailer_info(): use trailer_item objects
- format_trailers_from_commit(): indirectly call trailer_info_get()
- format_trailer_info(): move "fast path" to caller
- format_trailers(): use strbuf instead of FILE
- trailer_info_get(): reorder parameters
- trailer: start preparing for formatting unification
- trailer: move interpret_trailers() to interpret-trailers.c
- trailer: prepare to expose functions as part of API
- shortlog: add test for de-duplicating folded trailers
- trailer: free trailer_info _after_ all related usage
Code clean-up.
source: <pull.1632.v4.git.1707196348.gitgitgadget@gmail.com>
* cp/apply-core-filemode (2023-12-26) 3 commits
(merged to 'next' on 2024-02-07 at 089a3fbb86)
+ 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.
Will cook in 'next'.
cf. <xmqqzfwb53a9.fsf@gitster.g>
source: <20231226233218.472054-1-gitster@pobox.com>
* tb/path-filter-fix (2024-01-31) 16 commits
- bloom: introduce `deinit_bloom_filters()`
- commit-graph: reuse existing Bloom filters where possible
- object.h: fix mis-aligned flag bits table
- commit-graph: new Bloom filter version that fixes murmur3
- commit-graph: unconditionally load Bloom filters
- bloom: prepare to discard incompatible Bloom filters
- bloom: annotate filters with hash version
- 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.
Waiting for a final ack?
cf. <ZcFjkfbsBfk7JQIH@nand.local>
source: <cover.1706741516.git.me@ttaylorr.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.
Will merge to and cook in 'next'?
cf. <xmqqv86z5359.fsf@gitster.g>
source: <878r8l929e.fsf@gmail.froward.int.ebiederm.org>
* 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 v4 00/28] Enrich Trailer API
From: Linus Arver @ 2024-02-16 2:17 UTC (permalink / raw)
To: Christian Couder
Cc: Linus Arver via GitGitGadget, git, Christian Couder,
Junio C Hamano, Emily Shaffer, Josh Steadmon, Randall S. Becker
In-Reply-To: <CAP8UFD1vue6Pdh5rccibyPs179FYxRDS2-pDzOmTzF1vhTE-sg@mail.gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
> On Tue, Feb 13, 2024 at 8:39 PM Linus Arver <linusa@google.com> wrote:
>>
>> Christian Couder <christian.couder@gmail.com> writes:
>
> [...]
>
>> >> Thanks to the aggressive refactoring in this series, I've been able to
>> >> identify and fix several bugs in our existing implementation. Those fixes
>> >> build on top of this series but were not included here, in order to keep
>> >> this series small. Below is a "shortlog" of those fixes I have locally:
>> >>
>> >> * "trailer: trailer replacement should not change its position" (If we
>> >> found a trailer we'd like to replace, preserve its position relative to
>> >> the other trailers found in the trailer block, instead of always moving
>> >> it to the beginning or end of the entire trailer block.)
>> >
>> > I believe there was a reason why it was done this way. I don't
>> > remember what it was though.
>>
>> Noted. I'll see what I can find in our commit history.
>>
>> >> * "interpret-trailers: preserve trailers coming from the input" (Sometimes,
>> >> the parsed trailers from the input will be formatted differently
>> >> depending on whether we provide --only-trailers or not. Make the trailers
>> >> that were not modified and which are coming directly from the input get
>> >> formatted the same way, regardless of this flag.)
>> >
>> > It could be a feature to be able to normalize trailers in a certain way.
>>
>> True. But doing such normalization silently is undocumented behavior,
>> and we should provide explicit flags for this sort of thing. Adding such
>> flags might be the right thing to do (let's discuss more when this patch
>> gets proposed). FWIW the behavior I observed is that this normalization
>> only happens for *some* trailers that have configuration options, not
>> all trailers in the input. So it's a special kind of (limited)
>> normalization.
>
> Perhaps because we consider that having some configuration means that
> the user consistently expects things in a certain way.
Yes, this was one possibility I considered after sending my reply. If a
user has gone out of their way to configure something, maybe they do
want things (for those bits) to be normalized.
And adding a flag to disable normalization seems like a good feature to
have also (while keeping the behavior of the interpret-trailers that has
been relatively untouched since its introduction). But anyway I'm
getting a little bit ahead of myself.
>> >> * "interpret-trailers: fail if given unrecognized arguments" (E.g., for
>> >> "--where", only accept recognized WHERE_* enum values. If we get
>> >> something unrecognized, fail with an error instead of silently doing
>> >> nothing. Ditto for "--if-exists" and "--if-missing".)
>> >
>> > It's possible that there was a reason why it was done this way.
>> >
>> > I think you might want to take a look at the discussions on the
>> > mailing list when "interpret-trailers" was developed. There were a lot
>> > of discussions over a long time, and there were a lot of requests and
>> > suggestions about what it should do.
>>
>> I confess I haven't looked too deeply into the original threads
>> surrounding the introduction of "interpret-trailers". But all of the
>> changes which I categorize as bugfixes above have a theme of
>> undocumented modifications.
>>
>> While working on this (and the larger) series around trailers, I only
>> looked into some (not all) of the discussions on the mailing list in
>> this area. Instead, I deferred to
>> Documentation/git-interpret-trailers.txt as the official (authoritative)
>> source of truth. This is partly why I first started out on this project
>> last year by making improvements to that doc. And, this is why seeing so
>> many edge cases where we perform such undocumented modifications smelled
>> more like bugs to me than features.
>>
>> That being said, I cannot disagree with your position that the so-called
>> bugfixes I've reported above could be misguided (and should rather be
>> just updates to missing documentation). Let's not try to decide that
>> here, but instead later when I get to introduce those changes on the
>> list, one at a time. Thanks.
>
> Yeah, it might seem like undocumented features are bad, and I agree
> that reading original discussions can be tiring. But if the latter
> makes it possible to fix undocumented features by just properly
> documenting them, then I think it might just be the best thing to do.
> Ok not to decide about it now though.
Thanks!
^ permalink raw reply
* Re: [PATCH v4 00/28] Enrich Trailer API
From: Linus Arver @ 2024-02-16 2:25 UTC (permalink / raw)
To: Christian Couder, Junio C Hamano
Cc: Linus Arver via GitGitGadget, git, Christian Couder,
Emily Shaffer, Josh Steadmon, Randall S. Becker
In-Reply-To: <CAP8UFD08F0V13X0+CJ1uhMPzPWVMs2okGVMJch0DkQg5M3BWLA@mail.gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
> (Sorry for sending this previously to Junio only)
>
> On Tue, Feb 13, 2024 at 6:30 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Christian Couder <christian.couder@gmail.com> writes:
>
>> >> I presume that [01-09/28] would be the first part, nothing
>> >> controversial and consisting of obvious clean-ups? I do not mind
>> >> merging that part down to remove the future review load if everybody
>> >> agrees.
>> >
>> > Yeah, patches [01-09/28] look good to me.
>>
>> I was hoping that you'll give us more details of what the other 3 or
>> more you would envision the series to be, actually.
>
> I think the next one could be [10-16/28], so until "trailer: finish
> formatting unification".
>
> Then I am not sure about the next one, perhaps [17-20/28] or [17-21/28].
>
> The rest would depend on the splitting of the big patches towards the
> end of the series.
Ack, I'll try to group them like this. Thanks.
^ permalink raw reply
* Re: [PATCH] branch: rework the descriptions of rename and copy operations
From: Dragan Simic @ 2024-02-16 3:32 UTC (permalink / raw)
To: Rubén Justo; +Cc: Junio C Hamano, git
In-Reply-To: <2a4de8c4-4955-4891-859c-58730a41e5af@gmail.com>
On 2024-02-16 00:34, Rubén Justo wrote:
> On 15-feb-2024 14:13:31, Junio C Hamano wrote:
>> Rubén Justo <rjusto@gmail.com> writes:
>>> On 15-feb-2024 19:42:32, Dragan Simic wrote:
>>> Let me chime in just to say that maybe another terms could be
>>> considered
>>> here; like: "<branchname>" and "<newbranchname>" (maybe too long...)
>>> or
>>> so.
>>>
>>> I have no problem with the current terms, but "<branchname>" can be a
>>> sensible choice here as it is already being used for other commands
>>> where, and this may help overall, the consideration: "if ommited, the
>>> current branch is considered" also applies.
>>
>> Actually, we should go in the opposite direction. When the use of
>> names are localized in a narrower context, they can be shortened
>> without losing clarity.
>
> I did not mean to have longer terms, sorry for that.
>
> I was thinking more in the synopsis:
>
> 'git branch' (--set-upstream-to=<upstream> | -u <upstream>)
> [<branchname>]
> 'git branch' --unset-upstream [<branchname>]
> 'git branch' (-m | -M) [<branchname>] <new>
> 'git branch' (-c | -C) [<branchname>] <new>
> 'git branch' (-d | -D) [-r] <branchname>...
> 'git branch' --edit-description [<branchname>]
>
> To have more uniformity in the terms, which can be beneficial to the
> user.
Here's what I think the example from above should eventually look like:
'git branch' (--set-upstream-to=<upstream> | -u <upstream>)
[<name>]
'git branch' --unset-upstream [<name>]
'git branch' (-m | -M) [<old>] <new>
'git branch' (-c | -C) [<old>] <new>
'git branch' (-d | -D) [-r] <name>...
'git branch' --edit-description [<name>]
Though, it's something to be left for future patches, which will move
more argument descriptions to the respective command descriptions.
> We don't say that "--edit-description" defaults to the current branch;
> It is assumed. Perhaps we can take advantage of that assumption in
> -m|-c too.
We don't say that yet, :) because the description of the command for
editing branch descriptions is detached from the description of its
arguments. The plan is to move all of them together.
> Of course, there is no need (perhaps counterproductive) to say "branch"
> if the context makes it clear it is referring to a branch.
^ permalink raw reply
* Not aloud to
From: ross nicholas Oneil thomas @ 2024-02-16 4:14 UTC (permalink / raw)
To: GitHub, Github email, Legal Notice
This is unacceptable for license
https://github.com/chromaui/chromatic-cli/actions/workflows/check-labels.yml
Ross Nicholas Oneil Thomas
ownership of:
www.github.com
www.coinbase.com
www.jsnull.com
(559-816-2950)
^ permalink raw reply
* Re: [PATCH v2 02/30] oid-array: teach oid-array to handle multiple kinds of oids
From: Eric W. Biederman @ 2024-02-16 4:48 UTC (permalink / raw)
To: Linus Arver
Cc: Junio C Hamano, git, brian m. carlson, Eric Sunshine,
Eric W. Biederman
In-Reply-To: <owly1q9d9sau.fsf@fine.c.googlers.com>
Linus Arver <linusa@google.com> writes:
>
> I would still prefer the "early return for errors" style even in this
> case. This is because I much prefer to have the question "how can things
> go wrong?" answered first, and dealt with, such that as I read
> top-to-bottom I am left with less things I have to consider to
> understand the "happy path". WRT emphasizing the "algos equal each
> other" concern, a simple comment like
void_hashcmp is a function that reports how two elements are ordered.
There is no error handling.
There are in fact two cases that need to be handled with oid_array being
able to contain more then one kind of hash at a time.
The two entries are ordered by oidcmp.
The two entries are ordered by hash algorithm.
The order that is maintained is first everything is ordered by hash
algorithm, then for entries of identical hash algorithm they are ordered
by oidcmp.
So I don't think the concept of early return for errors can ever apply
to any version of void_hashcmp.
Eric
^ permalink raw reply
* [PATCH] documentation: send-email: use camel case consistently
From: Dragan Simic @ 2024-02-16 5:19 UTC (permalink / raw)
To: git
Correct a few random "sendemail.*" configuration parameter names in the
documentation that, for some reason, didn't use camel case format.
There's only one "Fixes" tag, while there should actually be a whole bunch
of them to cover all the patches that introduced the configuration parameter
names fixed by this patch. I think we're fine with just one.
Fixes: 0ee42c86cf71 ("config.txt: move sendemail-config.txt to config/")
Signed-off-by: Dragan Simic <dsimic@manjaro.org>
---
Documentation/config/sendemail.txt | 12 ++++++------
Documentation/git-send-email.txt | 18 +++++++++---------
2 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/Documentation/config/sendemail.txt b/Documentation/config/sendemail.txt
index 7fc770ee9e69..bfdf1a633cae 100644
--- a/Documentation/config/sendemail.txt
+++ b/Documentation/config/sendemail.txt
@@ -8,7 +8,7 @@ sendemail.smtpEncryption::
See linkgit:git-send-email[1] for description. Note that this
setting is not subject to the 'identity' mechanism.
-sendemail.smtpsslcertpath::
+sendemail.smtpSslCertPath::
Path to ca-certificates (either a directory or a single file).
Set it to an empty string to disable certificate verification.
@@ -62,27 +62,27 @@ sendemail.chainReplyTo::
sendemail.envelopeSender::
sendemail.from::
sendemail.headerCmd::
-sendemail.signedoffbycc::
+sendemail.signedOffByCc::
sendemail.smtpPass::
-sendemail.suppresscc::
+sendemail.suppressCc::
sendemail.suppressFrom::
sendemail.to::
-sendemail.tocmd::
+sendemail.toCmd::
sendemail.smtpDomain::
sendemail.smtpServer::
sendemail.smtpServerPort::
sendemail.smtpServerOption::
sendemail.smtpUser::
sendemail.thread::
sendemail.transferEncoding::
sendemail.validate::
sendemail.xmailer::
These configuration variables all provide a default for
linkgit:git-send-email[1] command-line options. See its
documentation for details.
-sendemail.signedoffcc (deprecated)::
- Deprecated alias for `sendemail.signedoffbycc`.
+sendemail.signedOffCc (deprecated)::
+ Deprecated alias for `sendemail.signedOffByCc`.
sendemail.smtpBatchSize::
Number of messages to be sent per connection, after that a relogin
diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index d1ef6a204e68..31acfa186457 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -138,7 +138,7 @@ Note that no attempts whatsoever are made to validate the encoding.
--compose-encoding=<encoding>::
Specify encoding of compose message. Default is the value of the
- 'sendemail.composeencoding'; if that is unspecified, UTF-8 is assumed.
+ 'sendemail.composeEncoding'; if that is unspecified, UTF-8 is assumed.
--transfer-encoding=(7bit|8bit|quoted-printable|base64|auto)::
Specify the transfer encoding to be used to send the message over SMTP.
@@ -174,7 +174,7 @@ Sending
Specify a command to run to send the email. The command should
be sendmail-like; specifically, it must support the `-i` option.
The command will be executed in the shell if necessary. Default
- is the value of `sendemail.sendmailcmd`. If unspecified, and if
+ is the value of `sendemail.sendmailCmd`. If unspecified, and if
--smtp-server is also unspecified, git-send-email will search
for `sendmail` in `/usr/sbin`, `/usr/lib` and $PATH.
@@ -269,7 +269,7 @@ must be used for each option.
certificates concatenated together: see verify(1) -CAfile and
-CApath for more information on these). Set it to an empty string
to disable certificate verification. Defaults to the value of the
- `sendemail.smtpsslcertpath` configuration variable, if set, or the
+ `sendemail.smtpSslCertPath` configuration variable, if set, or the
backing SSL library's compiled-in default otherwise (which should
be the best choice on most platforms).
@@ -313,7 +313,7 @@ Automating
Specify a command to execute once per patch file which
should generate patch file specific "To:" entries.
Output of this command must be single email address per line.
- Default is the value of 'sendemail.tocmd' configuration value.
+ Default is the value of 'sendemail.toCmd' configuration value.
--cc-cmd=<command>::
Specify a command to execute once per patch file which
@@ -348,19 +348,19 @@ Automating
--[no-]signed-off-by-cc::
If this is set, add emails found in the `Signed-off-by` trailer or Cc: lines to the
- cc list. Default is the value of `sendemail.signedoffbycc` configuration
+ cc list. Default is the value of `sendemail.signedOffByCc` configuration
value; if that is unspecified, default to --signed-off-by-cc.
--[no-]cc-cover::
If this is set, emails found in Cc: headers in the first patch of
the series (typically the cover letter) are added to the cc list
- for each email set. Default is the value of 'sendemail.cccover'
+ for each email set. Default is the value of 'sendemail.ccCover'
configuration value; if that is unspecified, default to --no-cc-cover.
--[no-]to-cover::
If this is set, emails found in To: headers in the first patch of
the series (typically the cover letter) are added to the to list
- for each email set. Default is the value of 'sendemail.tocover'
+ for each email set. Default is the value of 'sendemail.toCover'
configuration value; if that is unspecified, default to --no-to-cover.
--suppress-cc=<category>::
@@ -384,7 +384,7 @@ Automating
- 'all' will suppress all auto cc values.
--
+
-Default is the value of `sendemail.suppresscc` configuration value; if
+Default is the value of `sendemail.suppressCc` configuration value; if
that is unspecified, default to 'self' if --suppress-from is
specified, as well as 'body' if --no-signed-off-cc is specified.
@@ -471,7 +471,7 @@ Information
Instead of the normal operation, dump the shorthand alias names from
the configured alias file(s), one per line in alphabetical order. Note
that this only includes the alias name and not its expanded email addresses.
- See 'sendemail.aliasesfile' for more information about aliases.
+ See 'sendemail.aliasesFile' for more information about aliases.
CONFIGURATION
^ permalink raw reply related
* Re: [PATCH 1/4] notes: print note blob to stdout directly
From: Junio C Hamano @ 2024-02-16 6:25 UTC (permalink / raw)
To: Jeff King; +Cc: Maarten Bosmans, git, Teng Long, Maarten Bosmans
In-Reply-To: <20240215052640.GC2821179@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Tue, Feb 13, 2024 at 09:35:28AM -0800, Junio C Hamano wrote:
>
>> Remind me if you can if we (1) had plans to allow non-blob objects
>> as notes, or (2) actively support to have non-text blob objects as
>> notes. I _think_ we do assume people may try to add non-text blob
>> as notes (while they accept that they cannot merge two such notes on
>> the same object), but I do not recall if we planned to allow them to
>> store trees and commits.
>
> Short answer: no for (1) and yes for (2).
>
> In my head non-blob notes were always something we'd eventually allow.
> But I don't think the "git notes" tool really helps you at all (it
> insists on being fed message content and makes the blob itself, rather
> than letting you specify an oid).
OK, that makes my worry about dropping "git show" from the codepath
quite a lot. At least we only have to worry about blobs.
> Non-text blob objects, on the other hand, should be easy to do with "git
> notes add -F". Interestingly, the display code (in format_note() again)
> converts embedded NULs into newlines. I think this is an accidental
> behavior due to the use of strchrnul().
>
> Of course it's reasonable to also store notes that aren't meant to be
> displayed via git-log, etc, at all. The textconv-caching machinery
> stores its results in a separate notes ref. Those should generally be
> text (since the point is to come up with something diff-able). But I
> think it would be perfectly reasonable for another mechanism to make use
> of notes in the same way and store binary goo.
Yup.
The question is if our current use of "git show" allows creative use
of such binary data attached as notes. The patch in question will
break such users, as it would become impossible once we start
bypassing the "git show" and emitting the binary data directly to
the standard output stream.
Because the pathname a note blob is stored at is unpredictable and
not under end-user control, it would not be practical to define
different smudge filters per note object using the attribute
mechanism, but if you limit the types of binary data you store in
your notes (e.g., refs/notes/thumbnail may be a note ref whose
contents are all jpeg thumbnail images, where your main history is
your photo archive), you might be able to convince the "git show"
invocation we use to show the note object to show that thumbnail
image by using something involving "display" (from imagemagick---of
course you could use other programs that allows you to view the
image on different platforms) in your smudge filter. Bypassing "git
show" and sending the blob contents directly to the standard output
would be a grave regression for such a user.
^ permalink raw reply
* [PATCH] Always check the return value of `repo_read_object_file()`
From: Teng Long @ 2024-02-16 6:43 UTC (permalink / raw)
To: gitgitgadget; +Cc: git, johannes.schindelin
In-Reply-To: <pull.1650.git.1707143753726.gitgitgadget@gmail.com>
Johannes Schindelin <johannes.schindelin@gmx.de> wrote on Mon, 05 Feb 2024:
Hi, when I do zh_CN l10n work for 2.44, I found some check changes like:
die(_("unable to read tree %s")
in patchset, some old code for this like work is similar but with parentheses
surrounded with the OID parameter:
die(_("unable to read tree (%s)")
I think it's really a small nit, I don't think it's a requirement to immediately
optimize, they're just some small printing consistency formatting issues, so make
some small tips here.
Thanks.
^ permalink raw reply
* Re: git-retry tool or git.retry config (built-in implementation)?
From: Meiswinkel, Jan SF/HZA-ZC3S @ 2024-02-16 8:36 UTC (permalink / raw)
To: bagasdotme@gmail.com
Cc: git@vger.kernel.org, gitster@pobox.com, isaac.chun.to@gmail.com,
yoh@onerussian.com
(let's try this again - had html enabled)
Hi,
I think this thread might have died - a retry-logic for git commands would be great. IMO it is not even needed to continue the transfer from the faulty point, it would be a great benefit already if it dumps the failed request and simply retries the request X times with a backoff factor (just as the user would do manually if his request failed).
Any hopes?
Best
Jan
^ permalink raw reply
* [PATCH] git-difftool--helper: honor `--trust-exit-code` with `--dir-diff`
From: Patrick Steinhardt @ 2024-02-16 8:39 UTC (permalink / raw)
To: git; +Cc: Jean-Rémy Falleri
In-Reply-To: <976C9BF2-CB82-429A-B9FA-6A14BCFFCA3D@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 7492 bytes --]
The `--trust-exit-code` option for git-diff-tool(1) was introduced via
2b52123fcf (difftool: add support for --trust-exit-code, 2014-10-26).
When set, it makes us return the exit code of the invoked diff tool when
diffing multiple files. This patch didn't change the code path where
`--dir-diff` was passed because we already returned the exit code of the
diff tool unconditionally in that case.
This was changed a month later via c41d3fedd8 (difftool--helper: add
explicit exit statement, 2014-11-20), where an explicit `exit 0` was
added to the end of git-difftool--helper.sh. While the stated intent of
that commit was merely a cleanup, it had the consequence that we now
to ignore the exit code of the diff tool when `--dir-diff` was set. This
change in behaviour is thus very likely an unintended side effect of
this patch.
Now there are two ways to fix this:
- We can either restore the original behaviour, which unconditionally
returned the exit code of the diffing tool when `--dir-diff` is
passed.
- Or we can make the `--dir-diff` case respect the `--trust-exit-code`
flag.
The fact that we have been ignoring exit codes for 7 years by now makes
me rather lean towards the latter option. Furthermore, respecting the
flag in one case but not the other would needlessly make the user
interface more complex.
Fix the bug so that we also honor `--trust-exit-code` for dir diffs and
adjust the documentation accordingly.
Reported-by: Jean-Rémy Falleri <jr.falleri@gmail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
Documentation/git-difftool.txt | 1 -
git-difftool--helper.sh | 14 +++++
t/t7800-difftool.sh | 99 ++++++++++++++++++----------------
3 files changed, 68 insertions(+), 46 deletions(-)
diff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt
index c05f97aca9..a616f8b2e6 100644
--- a/Documentation/git-difftool.txt
+++ b/Documentation/git-difftool.txt
@@ -105,7 +105,6 @@ instead. `--no-symlinks` is the default on Windows.
`merge.tool` until a tool is found.
--[no-]trust-exit-code::
- 'git-difftool' invokes a diff tool individually on each file.
Errors reported by the diff tool are ignored by default.
Use `--trust-exit-code` to make 'git-difftool' exit when an
invoked diff tool returns a non-zero exit code.
diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh
index e4e820e680..09d8542917 100755
--- a/git-difftool--helper.sh
+++ b/git-difftool--helper.sh
@@ -91,6 +91,20 @@ then
# ignore the error from the above --- run_merge_tool
# will diagnose unusable tool by itself
run_merge_tool "$merge_tool" false
+
+ status=$?
+ if test $status -ge 126
+ then
+ # Command not found (127), not executable (126) or
+ # exited via a signal (>= 128).
+ exit $status
+ fi
+
+ if test "$status" != 0 &&
+ test "$GIT_DIFFTOOL_TRUST_EXIT_CODE" = true
+ then
+ exit $status
+ fi
else
# Launch the merge tool on each path provided by 'git diff'
while test $# -gt 6
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 6a36be1e63..96ae5d5880 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -91,58 +91,67 @@ test_expect_success 'difftool forwards arguments to diff' '
rm for-diff
'
-test_expect_success 'difftool ignores exit code' '
- test_config difftool.error.cmd false &&
- git difftool -y -t error branch
-'
+for opt in '' '--dir-diff'
+do
+ test_expect_success "difftool ${opt} ignores exit code" "
+ test_config difftool.error.cmd false &&
+ git difftool ${opt} -y -t error branch
+ "
-test_expect_success 'difftool forwards exit code with --trust-exit-code' '
- test_config difftool.error.cmd false &&
- test_must_fail git difftool -y --trust-exit-code -t error branch
-'
+ test_expect_success "difftool ${opt} forwards exit code with --trust-exit-code" "
+ test_config difftool.error.cmd false &&
+ test_must_fail git difftool ${opt} -y --trust-exit-code -t error branch
+ "
-test_expect_success 'difftool forwards exit code with --trust-exit-code for built-ins' '
- test_config difftool.vimdiff.path false &&
- test_must_fail git difftool -y --trust-exit-code -t vimdiff branch
-'
+ test_expect_success "difftool ${opt} forwards exit code with --trust-exit-code for built-ins" "
+ test_config difftool.vimdiff.path false &&
+ test_must_fail git difftool ${opt} -y --trust-exit-code -t vimdiff branch
+ "
-test_expect_success 'difftool honors difftool.trustExitCode = true' '
- test_config difftool.error.cmd false &&
- test_config difftool.trustExitCode true &&
- test_must_fail git difftool -y -t error branch
-'
+ test_expect_success "difftool ${opt} honors difftool.trustExitCode = true" "
+ test_config difftool.error.cmd false &&
+ test_config difftool.trustExitCode true &&
+ test_must_fail git difftool ${opt} -y -t error branch
+ "
-test_expect_success 'difftool honors difftool.trustExitCode = false' '
- test_config difftool.error.cmd false &&
- test_config difftool.trustExitCode false &&
- git difftool -y -t error branch
-'
+ test_expect_success "difftool ${opt} honors difftool.trustExitCode = false" "
+ test_config difftool.error.cmd false &&
+ test_config difftool.trustExitCode false &&
+ git difftool ${opt} -y -t error branch
+ "
-test_expect_success 'difftool ignores exit code with --no-trust-exit-code' '
- test_config difftool.error.cmd false &&
- test_config difftool.trustExitCode true &&
- git difftool -y --no-trust-exit-code -t error branch
-'
+ test_expect_success "difftool ${opt} ignores exit code with --no-trust-exit-code" "
+ test_config difftool.error.cmd false &&
+ test_config difftool.trustExitCode true &&
+ git difftool ${opt} -y --no-trust-exit-code -t error branch
+ "
-test_expect_success 'difftool stops on error with --trust-exit-code' '
- test_when_finished "rm -f for-diff .git/fail-right-file" &&
- test_when_finished "git reset -- for-diff" &&
- write_script .git/fail-right-file <<-\EOF &&
- echo failed
- exit 1
- EOF
- >for-diff &&
- git add for-diff &&
- test_must_fail git difftool -y --trust-exit-code \
- --extcmd .git/fail-right-file branch >actual &&
- test_line_count = 1 actual
-'
+ test_expect_success "difftool ${opt} stops on error with --trust-exit-code" "
+ test_when_finished 'rm -f for-diff .git/fail-right-file' &&
+ test_when_finished 'git reset -- for-diff' &&
+ write_script .git/fail-right-file <<-\EOF &&
+ echo failed
+ exit 1
+ EOF
+ >for-diff &&
+ git add for-diff &&
+ test_must_fail git difftool ${opt} -y --trust-exit-code \
+ --extcmd .git/fail-right-file branch >actual &&
+ test_line_count = 1 actual
+ "
-test_expect_success 'difftool honors exit status if command not found' '
- test_config difftool.nonexistent.cmd i-dont-exist &&
- test_config difftool.trustExitCode false &&
- test_must_fail git difftool -y -t nonexistent branch
-'
+ test_expect_success "difftool ${opt} honors exit status if command not found" "
+ test_config difftool.nonexistent.cmd i-dont-exist &&
+ test_config difftool.trustExitCode false &&
+ if test "${opt}" = '--dir-diff'
+ then
+ expected_code=127
+ else
+ expected_code=128
+ fi &&
+ test_expect_code \${expected_code} git difftool ${opt} -y -t nonexistent branch
+ "
+done
test_expect_success 'difftool honors --gui' '
difftool_test_setup &&
--
2.44.0-rc1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* Enhancement: `git pull` fails with merge conflict even if the local copy is modified in the same manner
From: Xuancong Wang @ 2024-02-16 8:43 UTC (permalink / raw)
To: git
Dear Git developers,
I would like to report a bug or defect in git.
I am in charge of running a real-life study. Very often, I copy the
source files to the working folder and test running the pipeline to
make sure that it does not crash. At the same time, I push my code to
my branch and create a merge request. After my branch gets merged to
the master branch (which takes some time), I run `git pull -f` in the
working folder (which always contains the latest version of my code
manually copied in). This pull operation will fail with conflict
because the source file gets modified both locally and in the updated
repository, even though the final file content is the same.
So my suggestion is that would you make Git a bit smarter so that if
the same file gets modified in the same way both locally and remotely,
would you allow the pull operation to pass? Thanks!
Best regards,
Xuancong
^ permalink raw reply
* [Improvements on messages 0/5] Disambuiguate between options and commands
From: Alexander Shopov @ 2024-02-16 10:15 UTC (permalink / raw)
To: git, gitster, worldhello.net; +Cc: Alexander Shopov
These are trivial fixes to messages.
They make sure commands and options are markes as such.
This will help translators and end users.
This will also reduce the special cases Jiang Xin keeps
for git-po-helper which will ease maintenance.
I am basing these on maint but I have also checked that
they ar still relevant by cherry picking on top of latest and next.
Each patch contains a single message fix. Each patch is sent to
- This list
- Jiang Xin
- Junio C Hamano
- The developer who last edited the line I am changing
These are nice to be merged but this is not urgent.
Alexander Shopov (5):
rebase: trivial fix of error message
transport-helper.c: trivial fix of error message
builtin/remote.c: trivial fix of error message
builtin/clone.c: trivial fix of message
revision.c: trivial fix to message
builtin/clone.c | 2 +-
builtin/rebase.c | 2 +-
builtin/remote.c | 2 +-
revision.c | 2 +-
transport-helper.c | 2 +-
5 files changed, 5 insertions(+), 5 deletions(-)
--
2.43.2
^ permalink raw reply
* [Improvements on messages 1/5] rebase: trivial fix of error message
From: Alexander Shopov @ 2024-02-16 10:15 UTC (permalink / raw)
To: git, gitster, worldhello.net, alexhenrie24; +Cc: Alexander Shopov
In-Reply-To: <20240216101647.28837-1-ash@kambanaria.org>
Mark --rebase-merges as option rather than variable name
Signed-off-by: Alexander Shopov <ash@kambanaria.org>
---
builtin/rebase.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 4084a6abb8..9c6d971515 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -746,7 +746,7 @@ static void parse_rebase_merges_value(struct rebase_options *options, const char
else if (!strcmp("rebase-cousins", value))
options->rebase_cousins = 1;
else
- die(_("Unknown rebase-merges mode: %s"), value);
+ die(_("Unknown --rebase-merges mode: %s"), value);
}
static int rebase_config(const char *var, const char *value,
--
2.43.2
^ permalink raw reply related
* [Improvements on messages 2/5] transport-helper.c: trivial fix of error message
From: Alexander Shopov @ 2024-02-16 10:15 UTC (permalink / raw)
To: git, gitster, worldhello.net, felipe.contreras; +Cc: Alexander Shopov
In-Reply-To: <20240216101647.28837-1-ash@kambanaria.org>
Mark --force as option rather than variable names
Signed-off-by: Alexander Shopov <ash@kambanaria.org>
---
transport-helper.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/transport-helper.c b/transport-helper.c
index e34a8f47cf..7014b9ad70 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -1072,7 +1072,7 @@ static int push_refs_with_export(struct transport *transport,
set_common_push_options(transport, data->name, flags);
if (flags & TRANSPORT_PUSH_FORCE) {
if (set_helper_option(transport, "force", "true") != 0)
- warning(_("helper %s does not support 'force'"), data->name);
+ warning(_("helper %s does not support '--force'"), data->name);
}
helper = get_helper(transport);
--
2.43.2
^ permalink raw reply related
* [Improvements on messages 3/5] builtin/remote.c: trivial fix of error message
From: Alexander Shopov @ 2024-02-16 10:15 UTC (permalink / raw)
To: git, gitster, worldhello.net, peff; +Cc: Alexander Shopov
In-Reply-To: <20240216101647.28837-1-ash@kambanaria.org>
Mark --mirror as option rather than command
Signed-off-by: Alexander Shopov <ash@kambanaria.org>
---
builtin/remote.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/remote.c b/builtin/remote.c
index d91bbe728d..8412d12fa5 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -150,7 +150,7 @@ static int parse_mirror_opt(const struct option *opt, const char *arg, int not)
else if (!strcmp(arg, "push"))
*mirror = MIRROR_PUSH;
else
- return error(_("unknown mirror argument: %s"), arg);
+ return error(_("unknown --mirror argument: %s"), arg);
return 0;
}
--
2.43.2
^ permalink raw reply related
* [Improvements on messages 4/5] builtin/clone.c: trivial fix of message
From: Alexander Shopov @ 2024-02-16 10:15 UTC (permalink / raw)
To: git, gitster, worldhello.net, stefanbeller; +Cc: Alexander Shopov
In-Reply-To: <20240216101647.28837-1-ash@kambanaria.org>
bare in that context is an option, not purely an adjective
Mark it properly
Signed-off-by: Alexander Shopov <ash@kambanaria.org>
---
builtin/clone.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/clone.c b/builtin/clone.c
index 315befa133..f0261bbb01 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -115,7 +115,7 @@ static struct option builtin_clone_options[] = {
OPT_HIDDEN_BOOL(0, "naked", &option_bare,
N_("create a bare repository")),
OPT_BOOL(0, "mirror", &option_mirror,
- N_("create a mirror repository (implies bare)")),
+ N_("create a mirror repository (implies --bare)")),
OPT_BOOL('l', "local", &option_local,
N_("to clone from a local repository")),
OPT_BOOL(0, "no-hardlinks", &option_no_hardlinks,
--
2.43.2
^ permalink raw reply related
* [Improvements on messages 5/5] revision.c: trivial fix to message
From: Alexander Shopov @ 2024-02-16 10:15 UTC (permalink / raw)
To: git, gitster, worldhello.net, newren; +Cc: Alexander Shopov
In-Reply-To: <20240216101647.28837-1-ash@kambanaria.org>
ancestry-path is an option, not a command - mark it as such.
This brings it in sync with the rest of usages in the file
Signed-off-by: Alexander Shopov <ash@kambanaria.org>
---
revision.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/revision.c b/revision.c
index 2424c9bd67..e29aa10781 100644
--- a/revision.c
+++ b/revision.c
@@ -2320,7 +2320,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
} else if (skip_prefix(arg, "--ancestry-path=", &optarg)) {
struct commit *c;
struct object_id oid;
- const char *msg = _("could not get commit for ancestry-path argument %s");
+ const char *msg = _("could not get commit for --ancestry-path argument %s");
revs->ancestry_path = 1;
revs->simplify_history = 0;
--
2.43.2
^ permalink raw reply related
* Re: [PATCH v2 2/4] oidset: refactor oidset_insert_from_set()
From: Christian Couder @ 2024-02-16 10:38 UTC (permalink / raw)
To: Linus Arver
Cc: git, Junio C Hamano, Patrick Steinhardt, John Cai,
Christian Couder
In-Reply-To: <owlyy1bl8b8u.fsf@fine.c.googlers.com>
On Fri, Feb 16, 2024 at 2:10 AM Linus Arver <linusa@google.com> wrote:
>
> Christian Couder <christian.couder@gmail.com> writes:
>
> > On Tue, Feb 13, 2024 at 10:02 PM Linus Arver <linusa@google.com> wrote:
> >> Are the extra parentheses necessary?
> >
> > Yes. Without them gcc errors out with:
> >
> > oidset.c: In function ‘oidset_insert_from_set’:
> > oidset.c:32:16: error: suggest parentheses around assignment used as
> > truth value [-Werror=parentheses]
> > 32 | while (src_oid = oidset_iter_next(&iter))
> > | ^~~~~~~
> >
> > Having extra parentheses is a way to tell the compiler that we do want
> > to use '=' and not '=='. This helps avoid the very common mistake of
> > using '=' where '==' was intended.
>
> Ah, so it is a "please trust me gcc, I know what I am doing" thing and
> not a "this is required in C" thing. Makes sense, thanks for clarifying.
>
> Sorry for the noise.
No worries. It's better to ask in case of doubt.
> >> so perhaps the following wording is simpler?
> >>
> >> Like oid_insert(), but insert all oids found in 'src'. Calls
> >> oid_insert() internally.
> >
> > (What you suggest would need s/oid_insert/oidset_insert/)
> >
> > Yeah, it's a bit simpler and shorter, but on the other hand a reader
> > might have to read both this and the oidset_insert() doc, so in the
> > end I am not sure it's a big win for readability. And if they don't
> > read the oidset_insert() doc, they might miss the fact that we are
> > copying the oids we insert, which might result in a bug.
>
> When functions are built on top of other functions, I think it is good
> practice to point readers to those underlying functions. In this case
> the new function is a wrapper around oidset_insert() which does all the
> real work. Plus the helper function already has some documentation about
> copying behavior that we already thought was important enough to call
> out explicitly.
>
> So, tying this definition to that (foundational) helper function sounds
> like a good idea to me in terms of readability. IOW we can inform
> readers "hey, we're just a wrapper around this other important function
> --- go there if you're curious about internals" and emphasizing that
> sort of relationship which may not be immediately obvious to those not
> familiar with this area would be nice.
>
> Alternatively, we could repeat the same comment WRT copying here but
> that seems redundant and prone to maintenance burdens down the road (if
> we ever change this behavior we have to change the comment in multiple
> functions, possibly).
>
> > Also your wording ties the implementation with oidset_insert(), which
> > we might not want if we could find something more performant. See
> > Junio's comment on this patch saying his initial reaction was that
> > copying underlying bits may even be more efficient.
> >
> > So I prefer not to change this.
>
> OK.
I must say that in cases like this, it's difficult to be right for
sure because it's mostly with enough hindsight that we can tell what
turned out to be a good decision. Here for example, it might be that
someone will find something more performant soon or it might turn out
that the function will never change. We just can't know.
So as long as the wording is clear and good enough, I think there is
no point in trying to improve it as much as possible. Here both your
wording and my wording seem clear and good enough to me. Junio might
change his mind but so far it seems that he found my wording good
enough too. So in cases like this, it's just simpler to keep current
wording. This way I think there is a higher chance that things can be
merged sooner and that we can all be more efficient.
> >> > +void oidset_insert_from_set(struct oidset *dest, struct oidset *src);
> >>
> >> Perhaps "oidset_insert_all" would be a simpler name? I generally prefer
> >> to reuse any descriptors in comments to guide the names. Plus this
> >> function used to be called "add_all()" so keeping the "all" naming style
> >> feels right.
> >
> > We already have other related types like 'struct oid-array' and
> > 'struct oidmap' to store oids, as well as code that inserts many oids
> > into an oidset from a 'struct ref *' linked list or array in a tight
> > loop.
>
> Thank you for the additional context I was not aware of.
>
> > So if we want to add functions inserting all the oids from
> > instances of such types, how should we call them?
> >
> > I would say we should use suffixes like: "_from_set", "_from_map",
> > "from_array", "_from_ref_list", "_from_ref_array", etc.
>
> I agree.
>
> However, I would like to point out that the function being added in this
> patch is a bit special: it is inserting from one "oidset" into another
> "oidset". IOW the both the dest and src types are the same.
>
> For the cases where the types are different, I totally agree that using
> the suffixes (to encode the type information of the src into the
> function name itself) is a good idea.
>
> So I think it's still fine to use "oidset_insert_all" because the only
> type in the parameter list is an oidset.
Yeah, here also I think both "oidset_insert_from_set" and
"oidset_insert_all" are clear and good enough.
> BUT, maybe in our codebase we already use suffixes like this even for
> cases where the types are the same? I don't know the answer to this
> question.
I agree that it could be a good thing to be consistent with similar
structs, but so far for oidmap there is only oidmap_put(), and, for
oid-array, only oid_array_append(). (And no, I didn't look further
than this.)
> However if we really wanted to be consistent then maybe we
> should be using the name oidset_insert_from_oidset() and not
> oidset_insert_from_set().
Yeah, "oidset_insert_from_oidset" and perhaps
"oidset_insert_all_from_oidset" would probably be fine too. Junio
found my wording good enough though, so I think it's just simpler not
to change it.
Also it's not like it can't be improved later if there is a good
reason like consistency with other oid related structs that might get
oidmap_put_all() or oid_array_append_all(). But again we can't predict
what will happen, so...
^ permalink raw reply
* Re: Hello question on Git for Windows 2.43.0 - GUID and/or SWID tag for this title
From: Konstantin Khomoutov @ 2024-02-16 11:00 UTC (permalink / raw)
To: git@vger.kernel.org
In-Reply-To: <LV8PR13MB65609D60ACB8A6EADFBFE3459C4D2@LV8PR13MB6560.namprd13.prod.outlook.com>
On Thu, Feb 15, 2024 at 09:40:47PM +0000, Christian Castro wrote:
>>> I have a question on the GUID and/or SWID tag for Git for Windows 2.43.0.
> Question: Are you a Git for Windows developer, open-source contributor or?
> I ask because I will contact the manufacturer of our inventory product and
> provide them your feedback. But I'd like to know what your role is with Git
> for Windows for as of now I just have a reply from someone named Matthias
> from a live.de email domain. I hope you understand. Truly no offense meant
> on my part.
>
> Therefore, please let me know what your role is with Git for Windows so I
> can send this feedback accordingly and continue working on with our software
> inventory vendor on the issue.
I would say the chief Git-for-Windows maintainer is Johannes Schindelin [1].
But prodding him directly would be impolite: after all, GfW is a F/OSS
project, so none of the folks participating in its development is oblidged to
answer any questions as them are not bound by terms of any contract with the
enterprise you're acting on behalf of - as no such contract exists (please
take no offence - I'm just clearing things up).
So, I would recommend to ask on GfW's Github Discussions [2] or on the
GfW's dedicated mailing list [3].
1. https://github.com/dscho
2. https://github.com/git-for-windows/git/discussions
3. https://groups.google.com/g/git-for-windows/
^ 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