* Re: [PATCH 11/12] builtin/show-ref: add new mode to check for reference existence
From: Eric Sunshine @ 2023-10-24 21:01 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Junio C Hamano, Han-Wen Nienhuys
In-Reply-To: <2f876e61dd36a8887a1286bb8db9fb6577c55c9b.1698152926.git.ps@pks.im>
On Tue, Oct 24, 2023 at 9:11 AM Patrick Steinhardt <ps@pks.im> wrote:
> While we have multiple ways to show the value of a given reference, we
> do not have any way to check whether a reference exists at all. While
> commands like git-rev-parse(1) or git-show-ref(1) can be used to check
> for reference existence in case the reference resolves to something
> sane, neither of them can be used to check for existence in some other
> scenarios where the reference does not resolve cleanly:
>
> - References which have an invalid name cannot be resolved.
>
> - References to nonexistent objects cannot be resolved.
>
> - Dangling symrefs can be resolved via git-symbolic-ref(1), but this
> requires the caller to special case existence checks depending on
> whteher or not a reference is symbolic or direct.
s/whteher/whether/
> Furthermore, git-rev-list(1) and other commands do not let the caller
> distinguish easily between an actually missing reference and a generic
> error.
>
> Taken together, this gseems like sufficient motivation to introduce a
s/gseems/seems/
> separate plumbing command to explicitly check for the existence of a
> reference without trying to resolve its contents.
>
> This new command comes in the form of `git show-ref --exists`. This
> new mode will exit successfully when the reference exists, with a
> specific error code of 2 when it does not exist, or with 1 when there
> has been a generic error.
>
> Note that the only way to properly implement this command is by using
> the internal `refs_read_raw_ref()` function. While the public function
> `refs_resolve_ref_unsafe()` can be made to behave in the same way by
> passing various flags, it does not provide any way to obtain the errno
> with which the reference backend failed when reading the reference. As
> such, it becomes impossible for us to distinguish generic errors from
> the explicit case where the reference wasn't found.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> diff --git a/Documentation/git-show-ref.txt b/Documentation/git-show-ref.txt
> @@ -65,6 +70,12 @@ OPTIONS
> +--exists::
> +
> + Check whether the given reference exists. Returns an error code of 0 if
We probably want to call this "exit code" rather than "error code"
since the latter is unnecessarily scary sounding for the success case
(when the ref does exit).
> + it does, 2 if it is missing, and 128 in case looking up the reference
> + failed with an error other than the reference being missing.
The commit message says it returns 1 for a generic error, but this
inconsistently says it returns 128 for that case. The actual
implementation returns 1.
> diff --git a/builtin/show-ref.c b/builtin/show-ref.c
> @@ -214,6 +215,41 @@ static int cmd_show_ref__patterns(const struct patterns_options *opts,
> +static int cmd_show_ref__exists(const char **refs)
> +{
> + struct strbuf unused_referent = STRBUF_INIT;
> + struct object_id unused_oid;
> + unsigned int unused_type;
> + int failure_errno = 0;
> + const char *ref;
> + int ret = 1;
> +
> + if (!refs || !*refs)
> + die("--exists requires a reference");
> + ref = *refs++;
> + if (*refs)
> + die("--exists requires exactly one reference");
> +
> + if (refs_read_raw_ref(get_main_ref_store(the_repository), ref,
> + &unused_oid, &unused_referent, &unused_type,
> + &failure_errno)) {
> + if (failure_errno == ENOENT) {
> + error(_("reference does not exist"));
The documentation doesn't mention this printing any output, and indeed
one would intuitively expect a boolean-like operation to not produce
any printed output since its exit code indicates the result (except,
of course, in the case of a real error).
> + ret = 2;
> + } else {
> + error(_("failed to look up reference: %s"), strerror(failure_errno));
Or use error_errno():
errno = failure_errno;
error_errno(_("failed to look up reference: %s"));
> + }
> +
> + goto out;
> + }
> +
> + ret = 0;
> +
> +out:
> + strbuf_release(&unused_referent);
> + return ret;
> +}
It's a bit odd having `ret` be 1 at the outset rather than 0, thus
making the logic a bit more difficult to reason about. I would have
expected it to be organized like this:
int ret = 0;
if (refs_read_raw_ref(...)) {
if (failure_errno == ENOENT) {
ret = 2;
} else {
ret = 1;
errno = failure_errno;
error_errno(_("failed to look up reference: %s"));
}
}
strbuf_release(...);
return ret;
> @@ -272,13 +309,15 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix)
> + if ((!!exclude_existing_opts.enabled + !!verify + !!exists) > 1)
> + die(_("only one of --exclude-existing, --exists or --verify can be given"));
When reviewing an earlier patch in this series, I forgot to mention
that we can simplify the life of translators by using placeholders:
die(_("options '%s', '%s' or '%s' cannot be used together"),
"--exclude-existing", "--exists", "--verify");
which ensures that they don't translate the literal option names, and
makes it possible to reuse the translated message in multiple
locations (since it doesn't mention hard-coded option names).
^ permalink raw reply
* Re: [RESEND v2] git-rebase.txt: rewrite docu for fixup/squash (again)
From: Oswald Buddenhagen @ 2023-10-24 21:19 UTC (permalink / raw)
To: Marc Branchaud
Cc: git, Junio C Hamano, Phillip Wood, Christian Couder,
Charvi Mendiratta
In-Reply-To: <e33f919d-1b6a-4944-ab5d-93ad0d323b68@xiplink.com>
On Tue, Oct 24, 2023 at 10:01:07AM -0400, Marc Branchaud wrote:
>I will only say that, I personally don't read man pages from
>start-to-end like a novel. I jump to the part that explains the thing
>I need to learn about. So I think your assumptions about what context
>a reader might have in mind when they see this text are invalid.
>
we are speaking about the context of a single paragraph, so that doesn't
seem like a relevant objection.
>> +The commit message for the folded commit is the concatenation of the
>> +message of the first commit with those of commits identified by "squash"
>
>s/message of the first commit/picked commit's message/
>
that does indeed sound better, but i think it's more confusing (and
potentially even more so when translated directly). i guess one could
use "pick'd commit's", but that's kind of ugly again.
>> +commands, omitting those of commits identified by "fixup" commands,
>> +unless "fixup -c" is used. In the latter case, the message is obtained
>> +only from the "fixup -c" commit (having more than one of these is
>> +incorrect).
>
>As Phillip said, this is wrong. I agree with Phillip that the
>documentation should reflect the actual implementation, not what we hope
>the implementation might be some day.
>
there is also the middle ground of making it intentionally vague in
anticipation of a possible change. my current draft says "if multiple
are present, the last one takes precedence, but this should not be
relied upon".
>> +The first commit which contributes to the suggested commit message
>> also
>
>s/suggested/folded/ -- with "fixup -C" there is no "suggested" message.
>
that's a good point, but i want to emphasize the fact that it's the
pre-edit message, i.e., that trimming down the squashed message doesn't
change anything.
anyway, this part will be postponed to another contribution anyway (see
parallel thread).
thanks
^ permalink raw reply
* Re: using oldest date when squashing commits
From: Johannes Sixt @ 2023-10-24 21:19 UTC (permalink / raw)
To: Junio C Hamano, Phillip Wood; +Cc: Oswald Buddenhagen, phillip.wood, git
In-Reply-To: <xmqqpm143p46.fsf@gitster.g>
Am 24.10.23 um 19:30 schrieb Junio C Hamano:
> Phillip Wood <phillip.wood123@gmail.com> writes:
>> "fixup -c/-C" were conceived as a way to reword a commit message at
>> the same time as optionally fixing up the commit's content.
>
> Yup, it still is a "fix", meaning the identity and the spirit of the
> commit being fixed are unchanged.
That's a pitty, because that is not at all what *I* use "fixup -C" for.
To update the commit message, I use "squash" (or occasionally "reword").
I use "fixup -C" after the following events:
1. Commit unfinished changes for whatever reason. Usually the commit
message just says "WIP <topic>" because that's what it is.
2. Make a fixup commit for an earlier commit because doing the fixup now
gets it out of the way, and often delaying it until after the completed
change would cause merge conflicts.
3. Complete the WIP including the commit message.
I would now use "fixup -C" on commit 3, because its metadata reflects
reality more accurately than that of 1. Commit 3 often comes days after 1.
-- Hannes
^ permalink raw reply
* Re: [PATCH 1/1] merge-file: add an option to process object IDs
From: brian m. carlson @ 2023-10-24 21:23 UTC (permalink / raw)
To: Eric Sunshine; +Cc: git, Junio C Hamano, Elijah Newren, Phillip Wood
In-Reply-To: <CAPig+cT_yq-ke4RTpTdTTLsnJFxCtyGAP2K0mQ_S23jJYtUp=w@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1793 bytes --]
On 2023-10-24 at 20:12:52, Eric Sunshine wrote:
> On Tue, Oct 24, 2023 at 3:58 PM brian m. carlson
> <sandals@crustytoothpaste.net> wrote:
> > git merge-file knows how to merge files on the file system already. It
> > would be helpful, however, to allow it to also merge single blobs.
> > Teach it an `--object-id` option which means that its arguments are
> > object IDs and not files to allow it to do so.
> >
> > Since we obviously won't be writing the data to the first argument,
> > either write to the object store and print the object ID, or honor the
> > -p argument and print it to standard out.
> >
> > We handle the empty blob specially since read_mmblob doesn't read it
> > directly, instead throwing an error, and otherwise users cannot specify
> > an empty ancestor.
> >
> > Signed-off-by: brian m. carlson <bk2204@github.com>
> > ---
> > diff --git a/builtin/merge-file.c b/builtin/merge-file.c
> > @@ -99,20 +116,29 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
> > if (ret >= 0) {
> > - const char *filename = argv[0];
> > - char *fpath = prefix_filename(prefix, argv[0]);
> > - FILE *f = to_stdout ? stdout : fopen(fpath, "wb");
> > + if (object_id && !to_stdout) {
> > + struct object_id oid;
> > + if (result.size)
> > + write_object_file(result.ptr, result.size, OBJ_BLOB, &oid);
>
> Should this be caring about errors by checking the return value of
> write_object_file()?
Probably so. I think I saw write_object_file_prepare returned void and
misinterpreted that as write_object_file returning void.
--
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]
^ permalink raw reply
* Re: [RESEND v2] git-rebase.txt: rewrite docu for fixup/squash (again)
From: Oswald Buddenhagen @ 2023-10-24 21:31 UTC (permalink / raw)
To: Taylor Blau
Cc: git, Junio C Hamano, Phillip Wood, Christian Couder,
Charvi Mendiratta, Marc Branchaud
In-Reply-To: <ZTamhY1sTpp1N6n+@nand.local>
On Mon, Oct 23, 2023 at 12:59:49PM -0400, Taylor Blau wrote:
>The line wrapping is a little odd: it looks like each sentence begins
>on a its own line.
>
this is indeed the case.
>Did you mean for there to be a visual separation between those
>sentences in the rendered doc?
>
no.
the idea is to keep the churn down in later edits, by making reflowing
the entire paragraph visibly unnecesary. i can change it if it's deemed
too weird.
regards
^ permalink raw reply
* Re: [PATCH] send-email: move validation code below process_address_list
From: Junio C Hamano @ 2023-10-24 21:55 UTC (permalink / raw)
To: Michael Strawbridge; +Cc: Jeff King, Bagas Sanjaya, Git Mailing List
In-Reply-To: <ee56c4df-e030-45f9-86a9-94fb3540db60@amd.com>
Michael Strawbridge <michael.strawbridge@amd.com> writes:
> Subject: [PATCH] send-email: move validation code below process_address_list
>
> Move validation logic below processing of email address lists so that
> email validation gets the proper email addresses.
Hmph, without this patch, the tip of 'seen' passes t9001 on my box,
but with it, it claims that it failed 58, 87, and 89.
Here is how #58 fails (the last part of "cd t && sh t9001-*.sh -i -v").
expecting success of 9001.58 'In-Reply-To without --chain-reply-to':
clean_fake_sendmail &&
echo "<unique-message-id@example.com>" >expect &&
git send-email \
--from="Example <nobody@example.com>" \
--to=nobody@example.com \
--no-chain-reply-to \
--in-reply-to="$(cat expect)" \
--smtp-server="$(pwd)/fake.sendmail" \
$patches $patches $patches \
2>errors &&
# The first message is a reply to --in-reply-to
sed -n -e "s/^In-Reply-To: *\(.*\)/\1/p" msgtxt1 >actual &&
test_cmp expect actual &&
# Second and subsequent messages are replies to the first one
sed -n -e "s/^Message-ID: *\(.*\)/\1/p" msgtxt1 >expect &&
sed -n -e "s/^In-Reply-To: *\(.*\)/\1/p" msgtxt2 >actual &&
test_cmp expect actual &&
sed -n -e "s/^In-Reply-To: *\(.*\)/\1/p" msgtxt3 >actual &&
test_cmp expect actual
0001-Second.patch
0001-Second.patch
0001-Second.patch
(mbox) Adding cc: A <author@example.com> from line 'From: A <author@example.com>'
(mbox) Adding cc: One <one@example.com> from line 'Cc: One <one@example.com>, two@example.com'
(mbox) Adding cc: two@example.com from line 'Cc: One <one@example.com>, two@example.com'
(body) Adding cc: C O Mitter <committer@example.com> from line 'Signed-off-by: C O Mitter <committer@example.com>'
OK. Log says:
Sendmail: /usr/local/google/home/jch/w/git.git/t/trash directory.t9001-send-email/fake.sendmail -i nobody@example.com author@example.com one@example.com two@example.com committer@example.com
From: Example <nobody@example.com>
To: nobody@example.com
Cc: A <author@example.com>,
One <one@example.com>,
two@example.com,
C O Mitter <committer@example.com>
Subject: [PATCH 1/1] Second.
Date: Tue, 24 Oct 2023 21:52:27 +0000
Message-ID: <20231024215229.1787922-1-nobody@example.com>
X-Mailer: git-send-email 2.42.0-705-g1a1f985ecc
In-Reply-To: <unique-message-id@example.com>
References: <unique-message-id@example.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Result: OK
(mbox) Adding cc: A <author@example.com> from line 'From: A <author@example.com>'
(mbox) Adding cc: One <one@example.com> from line 'Cc: One <one@example.com>, two@example.com'
(mbox) Adding cc: two@example.com from line 'Cc: One <one@example.com>, two@example.com'
(body) Adding cc: C O Mitter <committer@example.com> from line 'Signed-off-by: C O Mitter <committer@example.com>'
OK. Log says:
Sendmail: /usr/local/google/home/jch/w/git.git/t/trash directory.t9001-send-email/fake.sendmail -i nobody@example.com author@example.com one@example.com two@example.com committer@example.com
From: Example <nobody@example.com>
To: nobody@example.com
Cc: A <author@example.com>,
One <one@example.com>,
two@example.com,
C O Mitter <committer@example.com>
Subject: [PATCH 1/1] Second.
Date: Tue, 24 Oct 2023 21:52:28 +0000
Message-ID: <20231024215229.1787922-2-nobody@example.com>
X-Mailer: git-send-email 2.42.0-705-g1a1f985ecc
In-Reply-To: <unique-message-id@example.com>
References: <unique-message-id@example.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Result: OK
(mbox) Adding cc: A <author@example.com> from line 'From: A <author@example.com>'
(mbox) Adding cc: One <one@example.com> from line 'Cc: One <one@example.com>, two@example.com'
(mbox) Adding cc: two@example.com from line 'Cc: One <one@example.com>, two@example.com'
(body) Adding cc: C O Mitter <committer@example.com> from line 'Signed-off-by: C O Mitter <committer@example.com>'
OK. Log says:
Sendmail: /usr/local/google/home/jch/w/git.git/t/trash directory.t9001-send-email/fake.sendmail -i nobody@example.com author@example.com one@example.com two@example.com committer@example.com
From: Example <nobody@example.com>
To: nobody@example.com
Cc: A <author@example.com>,
One <one@example.com>,
two@example.com,
C O Mitter <committer@example.com>
Subject: [PATCH 1/1] Second.
Date: Tue, 24 Oct 2023 21:52:29 +0000
Message-ID: <20231024215229.1787922-3-nobody@example.com>
X-Mailer: git-send-email 2.42.0-705-g1a1f985ecc
In-Reply-To: <unique-message-id@example.com>
References: <unique-message-id@example.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Result: OK
--- expect 2023-10-24 21:52:29.115044899 +0000
+++ actual 2023-10-24 21:52:29.119045306 +0000
@@ -1 +1 @@
-<20231024215229.1787922-1-nobody@example.com>
+<unique-message-id@example.com>
not ok 58 - In-Reply-To without --chain-reply-to
#
# clean_fake_sendmail &&
# echo "<unique-message-id@example.com>" >expect &&
# git send-email \
# --from="Example <nobody@example.com>" \
# --to=nobody@example.com \
# --no-chain-reply-to \
# --in-reply-to="$(cat expect)" \
# --smtp-server="$(pwd)/fake.sendmail" \
# $patches $patches $patches \
# 2>errors &&
# # The first message is a reply to --in-reply-to
# sed -n -e "s/^In-Reply-To: *\(.*\)/\1/p" msgtxt1 >actual &&
# test_cmp expect actual &&
# # Second and subsequent messages are replies to the first one
# sed -n -e "s/^Message-ID: *\(.*\)/\1/p" msgtxt1 >expect &&
# sed -n -e "s/^In-Reply-To: *\(.*\)/\1/p" msgtxt2 >actual &&
# test_cmp expect actual &&
# sed -n -e "s/^In-Reply-To: *\(.*\)/\1/p" msgtxt3 >actual &&
# test_cmp expect actual
#
1..58
^ permalink raw reply
* Re: [PATCH] send-email: move validation code below process_address_list
From: Junio C Hamano @ 2023-10-24 22:03 UTC (permalink / raw)
To: Michael Strawbridge; +Cc: Jeff King, Bagas Sanjaya, Git Mailing List
In-Reply-To: <xmqqmsw73cua.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Michael Strawbridge <michael.strawbridge@amd.com> writes:
>
>> Subject: [PATCH] send-email: move validation code below process_address_list
>>
>> Move validation logic below processing of email address lists so that
>> email validation gets the proper email addresses.
>
> Hmph, without this patch, the tip of 'seen' passes t9001 on my box,
> but with it, it claims that it failed 58, 87, and 89.
FWIW, when this patch is used with 'master' (not 'seen'), t9001
claims the same three tests failed. The way #58 fails seems to be
identical to the way 'seen' with this patch failed, shown in the
message I am responding to.
^ permalink raw reply
* Re: [PATCH] SubmittingPatches: call gitk's command "Copy commit reference"
From: Junio C Hamano @ 2023-10-24 22:26 UTC (permalink / raw)
To: Andrei Rybak; +Cc: git
In-Reply-To: <20231024195123.911431-1-rybak.a.v@gmail.com>
Andrei Rybak <rybak.a.v@gmail.com> writes:
> Documentation/SubmittingPatches informs the contributor that gitk's
> context menu command "Copy commit summary" can be used to obtain the
> conventional format of referencing existing commits. This command in
> gitk was renamed to "Copy commit reference" in commit [1], following
> implementation of Git's "reference" pretty format in [2].
>
> Update mention of this gitk command in Documentation/SubmittingPatches
> to its new name.
>
> [1] b8b60957ce (gitk: rename "commit summary" to "commit reference",
> 2019-12-12)
> [2] commit 1f0fc1d (pretty: implement 'reference' format, 2019-11-20)
>
> Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
> ---
> Documentation/SubmittingPatches | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Thanks for a very well researched log message. Will queue.
>
> diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
> index 0e2d3fbb9c..653bb2ad44 100644
> --- a/Documentation/SubmittingPatches
> +++ b/Documentation/SubmittingPatches
> @@ -266,7 +266,7 @@ date)", like this:
> noticed that ...
> ....
>
> -The "Copy commit summary" command of gitk can be used to obtain this
> +The "Copy commit reference" command of gitk can be used to obtain this
> format (with the subject enclosed in a pair of double-quotes), or this
> invocation of `git show`:
^ permalink raw reply
* Re: [PATCH v3 0/5] config-parse: create config parsing library
From: Jonathan Tan @ 2023-10-24 22:50 UTC (permalink / raw)
To: Taylor Blau
Cc: Jonathan Tan, Junio C Hamano, Josh Steadmon, git, calvinwan,
glencbz
In-Reply-To: <ZTbK3QTJYXxYj/M6@nand.local>
Taylor Blau <me@ttaylorr.com> writes:
> But I am not sure that I agree that this series is moving us in the
> right direction necessarily. Or at least I am not convinced that
> shipping the intermediate state is worth doing before we have callers
> that could drop '#include "config.h"' for just the parser.
>
> This feels like churn that does not yield a tangible pay-off, at least
> in the sense that the refactoring and code movement delivers us
> something that we can substantively use today.
>
> I dunno.
>
> Thanks,
> Taylor
Thanks for calling this out. We do want our changes to be good for both
the libification and the non-libification cases as much as possible. As
it is, I do agree that since we won't have callers that can use the new
parser header (I think the likeliest cause of having such a caller is
if we have a "interpret-config" command, like "interpret-trailers"), we
probably shouldn't merge this (at least, the last 2 patches).
I think patches 1-3 are still usable (they make some internals of config
parsing less confusing) but I'm also OK if we hold off on them until
we find a compelling use case that motivates refactoring on the config
parser.
^ permalink raw reply
* Re: [PATCH v4 3/3] rev-list: add commit object support in `--missing` option
From: Junio C Hamano @ 2023-10-25 0:35 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, ps
In-Reply-To: <xmqqbkcn52z4.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> I noticed that nobody releases the resource held by this new oidset.
> Shouldn't we do so in revision.c:release_revisions()?
It seems that linux-leaks CI job noticed the same.
https://github.com/git/git/actions/runs/6633599458/job/18021612518#step:5:2949
I wonder if the following is sufficient?
revision.c | 2 ++
1 file changed, 2 insertions(+)
diff --git c/revision.c w/revision.c
index 724a116401..7a67ff74dc 100644
--- c/revision.c
+++ w/revision.c
@@ -3136,6 +3136,8 @@ void release_revisions(struct rev_info *revs)
clear_decoration(&revs->merge_simplification, free);
clear_decoration(&revs->treesame, free);
line_log_free(revs);
+ if (revs->do_not_die_on_missing_objects)
+ oidset_clear(&revs->missing_objects);
}
static void add_child(struct rev_info *revs, struct commit *parent, struct commit *child)
^ permalink raw reply related
* Re: [PATCH v1 3/4] config: factor out global config file retrievalync-mailbox>
From: Patrick Steinhardt @ 2023-10-25 5:38 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: Taylor Blau, git, stolee
In-Reply-To: <87badbe0-de18-4f8a-9589-314cea46065e@app.fastmail.com>
[-- Attachment #1: Type: text/plain, Size: 2034 bytes --]
On Tue, Oct 24, 2023 at 03:23:11PM +0200, Kristoffer Haugsbakk wrote:
> Hi Taylor and Patrick
>
> On Mon, Oct 23, 2023, at 19:40, Taylor Blau wrote:
> >> Nit: we don't know about the intent of the caller, so they may not want
> >> to write to the file but only read it.
> >
> > I was going to suggest that we allow the caller to pass in the flags
> > that they wish for git_global_config() to pass down to access(2), but
> > was surprised to see that we always use R_OK.
> >
> > But thinking on it for a moment longer, I realized that we don't care
> > about write-level permissions for the config, since we want to instead
> > open $GIT_DIR/config.lock for writing, and then rename() it into place,
> > meaning we only care about whether or not we have write permissions on
> > $GIT_DIR itself.
> >
> > I think in the existing location of this code, the "if we should write"
> > portion of the comment is premature, since we don't know for sure
> > whether or not we are writing. So I'd be fine with leaving it as-is, but
> > changing the comment seems easy enough to do...
> >
> >> > + * location; error out even if XDG_CONFIG_HOME
> >> > + * is set and points at a sane location.
> >> > + */
> >> > + die(_("$HOME not set"));
> >>
> >> Is it sensible to `die()` here in this new function that behaves more
> >> like a library function? I imagine it would be more sensible to indicate
> >> the error to the user and let them handle it accordingly.
> >
> > Agreed.
> >
> > Thanks,
> > Taylor
>
> What do you guys think the signature of `git_global_config` should be?
Either of the following:
- `int git_global_config(char **out_pat)`
- `char **git_global_config(void)`
In the first case you'd signal error via a non-zero return value,
whereas in the second case you would signal it via a `NULL` return
value.
To decide which one to go with I'd recommend to check whether there is
any similar precedent in "config.h" and what style that precedent uses.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 2/3] Revert "send-email: extract email-parsing code into a subroutine"
From: Jeff King @ 2023-10-25 6:11 UTC (permalink / raw)
To: Oswald Buddenhagen
Cc: Michael Strawbridge, Junio C Hamano, Bagas Sanjaya,
Git Mailing List
In-Reply-To: <ZTbOnsxBFERPLN3F@ugly>
On Mon, Oct 23, 2023 at 09:50:54PM +0200, Oswald Buddenhagen wrote:
> > The "//" one would
> > work, but we support perl versions old enough that they don't have it.
> >
> according to my grepping, that ship has sailed.
> also, why _would_ you support such ancient perl versions? that makes even
> less sense to me than supporting ancient c compilers.
It may be reasonable to bump the default perl version for the script.
But that would require somebody digging into what tends to ship these
days (which can be sometimes be surprising; witness macos using old
versions of bash due to license issues), and then updating the "use
5.008" in the script.
The "//" operator was added in perl 5.10. I'm not sure what you found
that makes you think the ship has sailed. The only hits for "//" I see
look like the end of substitution regexes ("s/foo//" and similar). But
if we are not consistent with the "use" claim, that is worth fixing.
-Peff
^ permalink raw reply
* Re: [PATCH v4 3/3] rev-list: add commit object support in `--missing` option
From: Patrick Steinhardt @ 2023-10-25 6:40 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, gitster
In-Reply-To: <20231024122631.158415-4-karthik.188@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 8550 bytes --]
On Tue, Oct 24, 2023 at 02:26:31PM +0200, Karthik Nayak wrote:
> The `--missing` object option in rev-list currently works only with
> missing blobs/trees. For missing commits the revision walker fails with
> a fatal error.
>
> Let's extend the functionality of `--missing` option to also support
> commit objects. This is done by adding a `missing_objects` field to
> `rev_info`. This field is an `oidset` to which we'll add the missing
> commits as we encounter them. The revision walker will now continue the
> traversal and call `show_commit()` even for missing commits. In rev-list
> we can then check if the commit is a missing commit and call the
> existing code for parsing `--missing` objects.
>
> A scenario where this option would be used is to find the boundary
> objects between different object directories. Consider a repository with
> a main object directory (GIT_OBJECT_DIRECTORY) and one or more alternate
> object directories (GIT_ALTERNATE_OBJECT_DIRECTORIES). In such a
> repository, using the `--missing=print` option while disabling the
> alternate object directory allows us to find the boundary objects
> between the main and alternate object directory.
>
> Helped-by: Patrick Steinhardt <ps@pks.im>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
> builtin/rev-list.c | 6 +++
> list-objects.c | 3 ++
> revision.c | 16 +++++++-
> revision.h | 4 ++
> t/t6022-rev-list-missing.sh | 74 +++++++++++++++++++++++++++++++++++++
> 5 files changed, 101 insertions(+), 2 deletions(-)
> create mode 100755 t/t6022-rev-list-missing.sh
>
> diff --git a/builtin/rev-list.c b/builtin/rev-list.c
> index 98542e8b3c..37b52520b5 100644
> --- a/builtin/rev-list.c
> +++ b/builtin/rev-list.c
> @@ -149,6 +149,12 @@ static void show_commit(struct commit *commit, void *data)
>
> display_progress(progress, ++progress_counter);
>
> + if (revs->do_not_die_on_missing_objects &&
> + oidset_contains(&revs->missing_objects, &commit->object.oid)) {
> + finish_object__ma(&commit->object);
> + return;
> + }
> +
> if (show_disk_usage)
> total_disk_usage += get_object_disk_usage(&commit->object);
>
> diff --git a/list-objects.c b/list-objects.c
> index 47296dff2f..260089388c 100644
> --- a/list-objects.c
> +++ b/list-objects.c
> @@ -389,6 +389,9 @@ static void do_traverse(struct traversal_context *ctx)
> */
> if (!ctx->revs->tree_objects)
> ; /* do not bother loading tree */
> + else if (ctx->revs->do_not_die_on_missing_objects &&
> + oidset_contains(&ctx->revs->missing_objects, &commit->object.oid))
> + ;
> else if (repo_get_commit_tree(the_repository, commit)) {
> struct tree *tree = repo_get_commit_tree(the_repository,
> commit);
> diff --git a/revision.c b/revision.c
> index 219dc76716..e60646c1a7 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -6,6 +6,7 @@
> #include "object-name.h"
> #include "object-file.h"
> #include "object-store-ll.h"
> +#include "oidset.h"
> #include "tag.h"
> #include "blob.h"
> #include "tree.h"
> @@ -1112,6 +1113,9 @@ static int process_parents(struct rev_info *revs, struct commit *commit,
>
> if (commit->object.flags & ADDED)
> return 0;
> + if (revs->do_not_die_on_missing_objects &&
> + oidset_contains(&revs->missing_objects, &commit->object.oid))
Nit: indentation is off here.
> + return 0;
> commit->object.flags |= ADDED;
>
> if (revs->include_check &&
> @@ -1168,7 +1172,8 @@ static int process_parents(struct rev_info *revs, struct commit *commit,
> for (parent = commit->parents; parent; parent = parent->next) {
> struct commit *p = parent->item;
> int gently = revs->ignore_missing_links ||
> - revs->exclude_promisor_objects;
> + revs->exclude_promisor_objects ||
> + revs->do_not_die_on_missing_objects;
> if (repo_parse_commit_gently(revs->repo, p, gently) < 0) {
> if (revs->exclude_promisor_objects &&
> is_promisor_object(&p->object.oid)) {
> @@ -1176,7 +1181,11 @@ static int process_parents(struct rev_info *revs, struct commit *commit,
> break;
> continue;
> }
> - return -1;
> +
> + if (!revs->do_not_die_on_missing_objects)
> + return -1;
> + else
> + oidset_insert(&revs->missing_objects, &p->object.oid);
> }
> if (revs->sources) {
> char **slot = revision_sources_at(revs->sources, p);
> @@ -3800,6 +3809,9 @@ int prepare_revision_walk(struct rev_info *revs)
> FOR_EACH_OBJECT_PROMISOR_ONLY);
> }
>
> + if (revs->do_not_die_on_missing_objects)
> + oidset_init(&revs->missing_objects, 0);
> +
While we're initializing the new oidset, we never clear it. We should
probably call `oidset_clear()` in `release_revisions()`. And if we
unconditionally initialized the oidset here then we can also call
`oiadset_clear()` unconditionally there, which should be preferable
given that `oidset_init()` does not allocate memory when no initial size
was given.
> if (!revs->reflog_info)
> prepare_to_use_bloom_filter(revs);
> if (!revs->unsorted_input)
> diff --git a/revision.h b/revision.h
> index c73c92ef40..f6bf422f0e 100644
> --- a/revision.h
> +++ b/revision.h
> @@ -4,6 +4,7 @@
> #include "commit.h"
> #include "grep.h"
> #include "notes.h"
> +#include "oidset.h"
> #include "pretty.h"
> #include "diff.h"
> #include "commit-slab-decl.h"
> @@ -373,6 +374,9 @@ struct rev_info {
>
> /* Location where temporary objects for remerge-diff are written. */
> struct tmp_objdir *remerge_objdir;
> +
> + /* Missing objects to be tracked without failing traversal. */
> + struct oidset missing_objects;
As far as I can see we only use this set to track missing commits, but
none of the other objects. The name thus feels a bit misleading to me,
as a reader might rightfully assume that it contains _all_ missing
objects after the revwalk. Should we rename it to `missing_commits` to
clarify?
Patrick
> };
>
> /**
> diff --git a/t/t6022-rev-list-missing.sh b/t/t6022-rev-list-missing.sh
> new file mode 100755
> index 0000000000..40265a4f66
> --- /dev/null
> +++ b/t/t6022-rev-list-missing.sh
> @@ -0,0 +1,74 @@
> +#!/bin/sh
> +
> +test_description='handling of missing objects in rev-list'
> +
> +TEST_PASSES_SANITIZE_LEAK=true
> +. ./test-lib.sh
> +
> +# We setup the repository with two commits, this way HEAD is always
> +# available and we can hide commit 1.
> +test_expect_success 'create repository and alternate directory' '
> + test_commit 1 &&
> + test_commit 2 &&
> + test_commit 3
> +'
> +
> +for obj in "HEAD~1" "HEAD~1^{tree}" "HEAD:1.t"
> +do
> + test_expect_success "rev-list --missing=error fails with missing object $obj" '
> + oid="$(git rev-parse $obj)" &&
> + path=".git/objects/$(test_oid_to_path $oid)" &&
> +
> + mv "$path" "$path.hidden" &&
> + test_when_finished "mv $path.hidden $path" &&
> +
> + test_must_fail git rev-list --missing=error --objects \
> + --no-object-names HEAD
> + '
> +done
> +
> +for obj in "HEAD~1" "HEAD~1^{tree}" "HEAD:1.t"
> +do
> + for action in "allow-any" "print"
> + do
> + test_expect_success "rev-list --missing=$action with missing $obj" '
> + oid="$(git rev-parse $obj)" &&
> + path=".git/objects/$(test_oid_to_path $oid)" &&
> +
> + # Before the object is made missing, we use rev-list to
> + # get the expected oids.
> + git rev-list --objects --no-object-names \
> + HEAD ^$obj >expect.raw &&
> +
> + # Blobs are shared by all commits, so evethough a commit/tree
> + # might be skipped, its blob must be accounted for.
> + if [ $obj != "HEAD:1.t" ]; then
> + echo $(git rev-parse HEAD:1.t) >>expect.raw &&
> + echo $(git rev-parse HEAD:2.t) >>expect.raw
> + fi &&
> +
> + mv "$path" "$path.hidden" &&
> + test_when_finished "mv $path.hidden $path" &&
> +
> + git rev-list --missing=$action --objects --no-object-names \
> + HEAD >actual.raw &&
> +
> + # When the action is to print, we should also add the missing
> + # oid to the expect list.
> + case $action in
> + allow-any)
> + ;;
> + print)
> + grep ?$oid actual.raw &&
> + echo ?$oid >>expect.raw
> + ;;
> + esac &&
> +
> + sort actual.raw >actual &&
> + sort expect.raw >expect &&
> + test_cmp expect actual
> + '
> + done
> +done
> +
> +test_done
> --
> 2.42.0
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH] send-email: move validation code below process_address_list
From: Jeff King @ 2023-10-25 6:50 UTC (permalink / raw)
To: Michael Strawbridge; +Cc: Junio C Hamano, Bagas Sanjaya, Git Mailing List
In-Reply-To: <ee56c4df-e030-45f9-86a9-94fb3540db60@amd.com>
On Tue, Oct 24, 2023 at 04:19:43PM -0400, Michael Strawbridge wrote:
> Move validation logic below processing of email address lists so that
> email validation gets the proper email addresses.
>
> This fixes email address validation errors when the optional
> perl module Email::Valid is installed and multiple addresses are passed
> in on a single to/cc argument like --to=foo@example.com,bar@example.com.
Is there a test we can include here?
> @@ -2023,6 +1999,30 @@ sub process_file {
> return 1;
> }
>
> +if ($validate) {
So the new spot is right before we call process_file() on each of the
input files. It is a little hard to follow because of the number of
functions defined inline in the middle of the script, but I think that
is a reasonable spot. It is after we have called process_address_list()
on to/cc/bcc, which I think fixes the regression. But it is also after
we sanitize $reply_to, etc, which seems like a good idea.
But I think putting it down that far is the source of the test failures.
The culprit seems not to be the call to validate_patch() in the loop you
moved, but rather pre_process_file(), which was added in your earlier
a8022c5f7b (send-email: expose header information to git-send-email's
sendemail-validate hook, 2023-04-19).
It looks like the issue is the global $message_num variable which is
incremented by pre_process_file(). On line 1755 (on the current tip of
master), we set it to 0. And your patch moves the validation across
there (from line ~799 to ~2023).
And that's why the extra increments didn't matter when you added the
calls to pre_process_file() in your earlier patch; they all happened
before we reset $message_num to 0. But now they happen after.
To be fair, this is absolutely horrific perl code. There's over a
thousand lines of function definitions, and then hidden in the middle
are some global variable assignments!
So we have a few options, I think:
1. Reset $message_num to 0 after validating (maybe we also need
to reset $in_reply_to, etc, set at the same time? I didn't check).
This feels like a hack.
2. Move the validation down, but not so far down. Like maybe right
after we set up the @files list with the $compose.final name. This
is the smallest diff, but feels like we haven't really made the
world a better place.
3. Move the $message_num, etc, initialization to right before we call
the process_file() loop, which is what expects to use them. Like
this (squashed into your patch):
diff --git a/git-send-email.perl b/git-send-email.perl
index a898dbc76e..d44db14223 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1730,10 +1730,6 @@ sub send_message {
return 1;
}
-$in_reply_to = $initial_in_reply_to;
-$references = $initial_in_reply_to || '';
-$message_num = 0;
-
sub pre_process_file {
my ($t, $quiet) = @_;
@@ -2023,6 +2019,10 @@ sub process_file {
delete $ENV{GIT_SENDEMAIL_FILE_TOTAL};
}
+$in_reply_to = $initial_in_reply_to;
+$references = $initial_in_reply_to || '';
+$message_num = 0;
+
foreach my $t (@files) {
while (!process_file($t)) {
# user edited the file
That seems to make the test failures go away. It is still weird that the
validation code is calling pre_process_file(), which increments
$message_num, without us having set it up in any meaningful way. I'm not
sure if there are bugs lurking there or not. I'm not impressed by the
general quality of this code, and I'm kind of afraid to keep looking
deeper.
-Peff
^ permalink raw reply related
* Re: Regression: git send-email fails with "Use of uninitialized value $address" + "unable to extract a valid address"
From: Jeff King @ 2023-10-25 7:21 UTC (permalink / raw)
To: Uwe Kleine-König; +Cc: Michael Strawbridge, Luben Tuikov, git, entwicklung
In-Reply-To: <20231024204318.gi6b4ygqbilm2yke@pengutronix.de>
On Tue, Oct 24, 2023 at 10:43:18PM +0200, Uwe Kleine-König wrote:
> I debugged a bit and if I do
>
> mv .git/hooks/sendemail-validate .git/hooks/sendemail-validate.bak
>
> git send-email --to 'Uwe Kleine-König <u.kleine-koenig@pengutronix.de>'
> starts to work for me, too.
>
> I'd guess the content of my sendemail-validate script doesn't matter
> much, but for the record, it's:
>
> #!/bin/sh
> # installed by patatt install-hook
> patatt sign --hook "${1}"
>
> Does the problem reproduce on your end with a sendemail-validate script?
I can reproduce with:
git init
echo foo >file && git add file && git commit -m foo
echo 'exit 0' >.git/hooks/sendemail-validate
chmod +x .git/hooks/sendemail-validate
git send-email --dry-run --to='pëff <peff@peff.net>' -1
Note that the bug will only trigger if Email::Valid is installed. I
think this is the same issue being discussed elsewhere. The call to
process_address_list() sanitizes it to use rfc2047 encoding, which is
necessary for it to be syntactically valid.
So the patch to move the validation later in the process here:
https://lore.kernel.org/git/ee56c4df-e030-45f9-86a9-94fb3540db60@amd.com/
fixes it.
-Peff
^ permalink raw reply
* Re: [PATCH v1 3/4] config: factor out global config file retrievalync-mailbox>
From: Kristoffer Haugsbakk @ 2023-10-25 7:33 UTC (permalink / raw)
To: Patrick Steinhardt, Taylor Blau; +Cc: git, stolee
In-Reply-To: <ZTip7JWm-WRWTImU@tanuki>
On Wed, Oct 25, 2023, at 07:38, Patrick Steinhardt wrote:
>> What do you guys think the signature of `git_global_config` should be?
>
> Either of the following:
>
> - `int git_global_config(char **out_pat)`
> - `char **git_global_config(void)`
>
> In the first case you'd signal error via a non-zero return value,
> whereas in the second case you would signal it via a `NULL` return
> value.
>
> To decide which one to go with I'd recommend to check whether there is
> any similar precedent in "config.h" and what style that precedent uses.
Okay thanks. So no parameter for determining whether one is writing or
just reading the file.
Cheers
--
Kristoffer Haugsbakk
^ permalink raw reply
* Re: Regression: git send-email fails with "Use of uninitialized value $address" + "unable to extract a valid address"
From: Uwe Kleine-König @ 2023-10-25 7:40 UTC (permalink / raw)
To: Jeff King; +Cc: Luben Tuikov, Michael Strawbridge, git, entwicklung
In-Reply-To: <20231025072104.GA2145145@coredump.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 1850 bytes --]
Hello,
On Wed, Oct 25, 2023 at 03:21:04AM -0400, Jeff King wrote:
> On Tue, Oct 24, 2023 at 10:43:18PM +0200, Uwe Kleine-König wrote:
>
> > I debugged a bit and if I do
> >
> > mv .git/hooks/sendemail-validate .git/hooks/sendemail-validate.bak
> >
> > git send-email --to 'Uwe Kleine-König <u.kleine-koenig@pengutronix.de>'
> > starts to work for me, too.
> >
> > I'd guess the content of my sendemail-validate script doesn't matter
> > much, but for the record, it's:
> >
> > #!/bin/sh
> > # installed by patatt install-hook
> > patatt sign --hook "${1}"
> >
> > Does the problem reproduce on your end with a sendemail-validate script?
>
> I can reproduce with:
>
> git init
> echo foo >file && git add file && git commit -m foo
> echo 'exit 0' >.git/hooks/sendemail-validate
> chmod +x .git/hooks/sendemail-validate
> git send-email --dry-run --to='pëff <peff@peff.net>' -1
>
> Note that the bug will only trigger if Email::Valid is installed.
I can confirm I have this package installed (via Debian's
libemail-valid-perl).
> I think this is the same issue being discussed elsewhere. The call to
> process_address_list() sanitizes it to use rfc2047 encoding, which is
> necessary for it to be syntactically valid.
>
> So the patch to move the validation later in the process here:
>
> https://lore.kernel.org/git/ee56c4df-e030-45f9-86a9-94fb3540db60@amd.com/
>
> fixes it.
Tested and indeed that fixes my usecase. With that patch also the
original regression is fixed and I can do
git send-email -1 --to 'Uwe Kleine-König <u.kleine-koenig@pengutronix.de>, pëff <peff@peff.net>'
again. \o/
Thanks
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH v5 1/5] bulk-checkin: extract abstract `bulk_checkin_source`
From: Jeff King @ 2023-10-25 7:37 UTC (permalink / raw)
To: Taylor Blau
Cc: git, Elijah Newren, Eric W. Biederman, Junio C Hamano,
Patrick Steinhardt
In-Reply-To: <696aa027e46ddec310812fad2d4b12082447d925.1698101088.git.me@ttaylorr.com>
On Mon, Oct 23, 2023 at 06:44:56PM -0400, Taylor Blau wrote:
> +struct bulk_checkin_source {
> + off_t (*read)(struct bulk_checkin_source *, void *, size_t);
> + off_t (*seek)(struct bulk_checkin_source *, off_t);
> +
> + union {
> + struct {
> + int fd;
> + } from_fd;
> + } data;
> +
> + size_t size;
> + const char *path;
> +};
The virtual functions combined with the union are a funny mix of
object-oriented and procedural code. The bulk_checkin_source has
totally virtualized functions, but knows about all of the ancillary data
each set of virtualized functions might want. ;)
I think the more pure OO version would embed the parent, and have each
concrete type define its own struct type, like:
struct bulk_checkin_source_fd {
struct bulk_checkin_source src;
int fd;
};
That works great if the code which constructs it knows which concrete
type it wants, and can just do:
struct bulk_checkin_source_fd src;
init_bulk_checkin_source_from_fd(&src, ...);
If even the construction is somewhat virtualized, then you are stuck
with heap constructors like:
struct bulk_checkin_source *bulk_checkin_source_from_fd(...);
Not too bad, but you have to remember to free now.
Alternatively, I think some of our other OO code just leaves room for
a type-specific void pointer, like:
struct bulk_checkin_source {
...the usual stuff...
void *magic_type_data;
};
and then the init_bulk_checkin_source_from_fd() function allocates its
own heap struct for the magic_type_data field and sticks the int in
there.
That said, both of those are a lot more annoying to use in C (more
boilerplate, more casting, and more opportunities to get something
wrong, including leaks). So I don't mind this in-between state. It is a
funny layering violating from an OO standpoint, but it's not like we
expect an unbounded set of concrete types to "inherit" from the source
struct.
-Peff
^ permalink raw reply
* Re: [PATCH] send-email: move validation code below process_address_list
From: Uwe Kleine-König @ 2023-10-25 7:43 UTC (permalink / raw)
To: Michael Strawbridge
Cc: Jeff King, Junio C Hamano, Bagas Sanjaya, Git Mailing List
In-Reply-To: <ee56c4df-e030-45f9-86a9-94fb3540db60@amd.com>
[-- Attachment #1: Type: text/plain, Size: 1386 bytes --]
Hello,
On Tue, Oct 24, 2023 at 04:19:43PM -0400, Michael Strawbridge wrote:
> >From 09ea51d63cebdf9ff0c073ef86e21b4b09c268e5 Mon Sep 17 00:00:00 2001
> From: Michael Strawbridge <michael.strawbridge@amd.com>
> Date: Wed, 11 Oct 2023 16:13:13 -0400
> Subject: [PATCH] send-email: move validation code below process_address_list
>
> Move validation logic below processing of email address lists so that
> email validation gets the proper email addresses.
>
> This fixes email address validation errors when the optional
> perl module Email::Valid is installed and multiple addresses are passed
> in on a single to/cc argument like --to=foo@example.com,bar@example.com.
>
> Reported-by: Bagas Sanjaya <bagasdotme@gmail.com>
> Signed-off-by: Michael Strawbridge <michael.strawbridge@amd.com>
If you do Fixes: trailers as the kernel does, this could get:
Fixes: a8022c5f7b67 ("send-email: expose header information to git-send-email's sendemail-validate hook")
I tested this patch on top of main (2e8e77cbac8a) and it fixes the
regression I reported in a separate thread (where Jeff pointed out this
patch as fixing it).
Tested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Thanks
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH v5 3/5] bulk-checkin: introduce `index_blob_bulk_checkin_incore()`
From: Patrick Steinhardt @ 2023-10-25 7:58 UTC (permalink / raw)
To: Taylor Blau
Cc: git, Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano
In-Reply-To: <d8cf8e4395375f88fe4e1ade2b79a3be6ce5fb12.1698101088.git.me@ttaylorr.com>
[-- Attachment #1: Type: text/plain, Size: 6245 bytes --]
On Mon, Oct 23, 2023 at 06:45:01PM -0400, Taylor Blau wrote:
> Introduce `index_blob_bulk_checkin_incore()` which allows streaming
> arbitrary blob contents from memory into the bulk-checkin pack.
>
> In order to support streaming from a location in memory, we must
> implement a new kind of bulk_checkin_source that does just that. These
> implementation in spread out across:
Nit: the commit message is a bit off here. Probably not worth a reroll
though.
> - init_bulk_checkin_source_incore()
> - bulk_checkin_source_read_incore()
> - bulk_checkin_source_seek_incore()
>
> Note that, unlike file descriptors, which manage their own offset
> internally, we have to keep track of how many bytes we've read out of
> the buffer, and make sure we don't read past the end of the buffer.
>
> This will be useful in a couple of more commits in order to provide the
> `merge-tree` builtin with a mechanism to create a new pack containing
> any objects it created during the merge, instead of storing those
> objects individually as loose.
>
> Similar to the existing `index_blob_bulk_checkin()` function, the
> entrypoint delegates to `deflate_obj_to_pack_incore()`. That function in
> turn delegates to deflate_obj_to_pack(), which is responsible for
> formatting the pack header and then deflating the contents into the
> pack.
>
> Consistent with the rest of the bulk-checkin mechanism, there are no
> direct tests here. In future commits when we expose this new
> functionality via the `merge-tree` builtin, we will test it indirectly
> there.
>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
> bulk-checkin.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++++
> bulk-checkin.h | 4 +++
> 2 files changed, 79 insertions(+)
>
> diff --git a/bulk-checkin.c b/bulk-checkin.c
> index 79776e679e..b728210bc7 100644
> --- a/bulk-checkin.c
> +++ b/bulk-checkin.c
> @@ -148,6 +148,10 @@ struct bulk_checkin_source {
> struct {
> int fd;
> } from_fd;
> + struct {
> + const void *buf;
> + size_t nr_read;
> + } incore;
> } data;
>
> size_t size;
> @@ -166,6 +170,36 @@ static off_t bulk_checkin_source_seek_from_fd(struct bulk_checkin_source *source
> return lseek(source->data.from_fd.fd, offset, SEEK_SET);
> }
>
> +static off_t bulk_checkin_source_read_incore(struct bulk_checkin_source *source,
> + void *buf, size_t nr)
> +{
> + const unsigned char *src = source->data.incore.buf;
> +
> + if (source->data.incore.nr_read > source->size)
> + BUG("read beyond bulk-checkin source buffer end "
> + "(%"PRIuMAX" > %"PRIuMAX")",
> + (uintmax_t)source->data.incore.nr_read,
> + (uintmax_t)source->size);
> +
> + if (nr > source->size - source->data.incore.nr_read)
> + nr = source->size - source->data.incore.nr_read;
> +
> + src += source->data.incore.nr_read;
> +
> + memcpy(buf, src, nr);
> + source->data.incore.nr_read += nr;
> + return nr;
> +}
> +
> +static off_t bulk_checkin_source_seek_incore(struct bulk_checkin_source *source,
> + off_t offset)
> +{
> + if (!(0 <= offset && offset < source->size))
> + return (off_t)-1;
At the risk of showing my own ignorance, but why is the cast here
necessary?
Patrick
> + source->data.incore.nr_read = offset;
> + return source->data.incore.nr_read;
> +}
> +
> static void init_bulk_checkin_source_from_fd(struct bulk_checkin_source *source,
> int fd, size_t size,
> const char *path)
> @@ -181,6 +215,22 @@ static void init_bulk_checkin_source_from_fd(struct bulk_checkin_source *source,
> source->path = path;
> }
>
> +static void init_bulk_checkin_source_incore(struct bulk_checkin_source *source,
> + const void *buf, size_t size,
> + const char *path)
> +{
> + memset(source, 0, sizeof(struct bulk_checkin_source));
> +
> + source->read = bulk_checkin_source_read_incore;
> + source->seek = bulk_checkin_source_seek_incore;
> +
> + source->data.incore.buf = buf;
> + source->data.incore.nr_read = 0;
> +
> + source->size = size;
> + source->path = path;
> +}
> +
> /*
> * Read the contents from 'source' for 'size' bytes, streaming it to the
> * packfile in state while updating the hash in ctx. Signal a failure
> @@ -359,6 +409,19 @@ static int deflate_obj_to_pack(struct bulk_checkin_packfile *state,
> return 0;
> }
>
> +static int deflate_obj_to_pack_incore(struct bulk_checkin_packfile *state,
> + struct object_id *result_oid,
> + const void *buf, size_t size,
> + const char *path, enum object_type type,
> + unsigned flags)
> +{
> + struct bulk_checkin_source source;
> +
> + init_bulk_checkin_source_incore(&source, buf, size, path);
> +
> + return deflate_obj_to_pack(state, result_oid, &source, type, 0, flags);
> +}
> +
> static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
> struct object_id *result_oid,
> int fd, size_t size,
> @@ -421,6 +484,18 @@ int index_blob_bulk_checkin(struct object_id *oid,
> return status;
> }
>
> +int index_blob_bulk_checkin_incore(struct object_id *oid,
> + const void *buf, size_t size,
> + const char *path, unsigned flags)
> +{
> + int status = deflate_obj_to_pack_incore(&bulk_checkin_packfile, oid,
> + buf, size, path, OBJ_BLOB,
> + flags);
> + if (!odb_transaction_nesting)
> + flush_bulk_checkin_packfile(&bulk_checkin_packfile);
> + return status;
> +}
> +
> void begin_odb_transaction(void)
> {
> odb_transaction_nesting += 1;
> diff --git a/bulk-checkin.h b/bulk-checkin.h
> index aa7286a7b3..1b91daeaee 100644
> --- a/bulk-checkin.h
> +++ b/bulk-checkin.h
> @@ -13,6 +13,10 @@ int index_blob_bulk_checkin(struct object_id *oid,
> int fd, size_t size,
> const char *path, unsigned flags);
>
> +int index_blob_bulk_checkin_incore(struct object_id *oid,
> + const void *buf, size_t size,
> + const char *path, unsigned flags);
> +
> /*
> * Tell the object database to optimize for adding
> * multiple objects. end_odb_transaction must be called
> --
> 2.42.0.425.g963d08ddb3.dirty
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v5 5/5] builtin/merge-tree.c: implement support for `--write-pack`
From: Patrick Steinhardt @ 2023-10-25 7:58 UTC (permalink / raw)
To: Taylor Blau
Cc: git, Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano
In-Reply-To: <3595db76a525fcebc3c896e231246704b044310c.1698101088.git.me@ttaylorr.com>
[-- Attachment #1: Type: text/plain, Size: 11501 bytes --]
On Mon, Oct 23, 2023 at 06:45:06PM -0400, Taylor Blau wrote:
> When using merge-tree often within a repository[^1], it is possible to
> generate a relatively large number of loose objects, which can result in
> degraded performance, and inode exhaustion in extreme cases.
>
> Building on the functionality introduced in previous commits, the
> bulk-checkin machinery now has support to write arbitrary blob and tree
> objects which are small enough to be held in-core. We can use this to
> write any blob/tree objects generated by ORT into a separate pack
> instead of writing them out individually as loose.
>
> This functionality is gated behind a new `--write-pack` option to
> `merge-tree` that works with the (non-deprecated) `--write-tree` mode.
>
> The implementation is relatively straightforward. There are two spots
> within the ORT mechanism where we call `write_object_file()`, one for
> content differences within blobs, and another to assemble any new trees
> necessary to construct the merge. In each of those locations,
> conditionally replace calls to `write_object_file()` with
> `index_blob_bulk_checkin_incore()` or `index_tree_bulk_checkin_incore()`
> depending on which kind of object we are writing.
>
> The only remaining task is to begin and end the transaction necessary to
> initialize the bulk-checkin machinery, and move any new pack(s) it
> created into the main object store.
>
> [^1]: Such is the case at GitHub, where we run presumptive "test merges"
> on open pull requests to see whether or not we can light up the merge
> button green depending on whether or not the presumptive merge was
> conflicted.
>
> This is done in response to a number of user-initiated events,
> including viewing an open pull request whose last test merge is stale
> with respect to the current base and tip of the pull request. As a
> result, merge-tree can be run very frequently on large, active
> repositories.
>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
> Documentation/git-merge-tree.txt | 4 ++
> builtin/merge-tree.c | 5 ++
> merge-ort.c | 42 +++++++++++----
> merge-recursive.h | 1 +
> t/t4301-merge-tree-write-tree.sh | 93 ++++++++++++++++++++++++++++++++
> 5 files changed, 136 insertions(+), 9 deletions(-)
>
> diff --git a/Documentation/git-merge-tree.txt b/Documentation/git-merge-tree.txt
> index ffc4fbf7e8..9d37609ef1 100644
> --- a/Documentation/git-merge-tree.txt
> +++ b/Documentation/git-merge-tree.txt
> @@ -69,6 +69,10 @@ OPTIONS
> specify a merge-base for the merge, and specifying multiple bases is
> currently not supported. This option is incompatible with `--stdin`.
>
> +--write-pack::
> + Write any new objects into a separate packfile instead of as
> + individual loose objects.
> +
> [[OUTPUT]]
> OUTPUT
> ------
> diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
> index a35e0452d6..218442ac9b 100644
> --- a/builtin/merge-tree.c
> +++ b/builtin/merge-tree.c
> @@ -19,6 +19,7 @@
> #include "tree.h"
> #include "config.h"
> #include "strvec.h"
> +#include "bulk-checkin.h"
>
> static int line_termination = '\n';
>
> @@ -416,6 +417,7 @@ struct merge_tree_options {
> int name_only;
> int use_stdin;
> struct merge_options merge_options;
> + int write_pack;
> };
>
> static int real_merge(struct merge_tree_options *o,
> @@ -441,6 +443,7 @@ static int real_merge(struct merge_tree_options *o,
> _("not something we can merge"));
>
> opt.show_rename_progress = 0;
> + opt.write_pack = o->write_pack;
>
> opt.branch1 = branch1;
> opt.branch2 = branch2;
> @@ -553,6 +556,8 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
> N_("specify a merge-base for the merge")),
> OPT_STRVEC('X', "strategy-option", &xopts, N_("option=value"),
> N_("option for selected merge strategy")),
> + OPT_BOOL(0, "write-pack", &o.write_pack,
> + N_("write new objects to a pack instead of as loose")),
> OPT_END()
> };
>
> diff --git a/merge-ort.c b/merge-ort.c
> index 3653725661..523577d71e 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -48,6 +48,7 @@
> #include "tree.h"
> #include "unpack-trees.h"
> #include "xdiff-interface.h"
> +#include "bulk-checkin.h"
>
> /*
> * We have many arrays of size 3. Whenever we have such an array, the
> @@ -2108,10 +2109,19 @@ static int handle_content_merge(struct merge_options *opt,
> if ((merge_status < 0) || !result_buf.ptr)
> ret = error(_("failed to execute internal merge"));
>
> - if (!ret &&
> - write_object_file(result_buf.ptr, result_buf.size,
> - OBJ_BLOB, &result->oid))
> - ret = error(_("unable to add %s to database"), path);
> + if (!ret) {
> + ret = opt->write_pack
> + ? index_blob_bulk_checkin_incore(&result->oid,
> + result_buf.ptr,
> + result_buf.size,
> + path, 1)
> + : write_object_file(result_buf.ptr,
> + result_buf.size,
> + OBJ_BLOB, &result->oid);
> + if (ret)
> + ret = error(_("unable to add %s to database"),
> + path);
> + }
>
> free(result_buf.ptr);
> if (ret)
> @@ -3597,7 +3607,8 @@ static int tree_entry_order(const void *a_, const void *b_)
> b->string, strlen(b->string), bmi->result.mode);
> }
>
> -static int write_tree(struct object_id *result_oid,
> +static int write_tree(struct merge_options *opt,
> + struct object_id *result_oid,
> struct string_list *versions,
> unsigned int offset,
> size_t hash_size)
> @@ -3631,8 +3642,14 @@ static int write_tree(struct object_id *result_oid,
> }
>
> /* Write this object file out, and record in result_oid */
> - if (write_object_file(buf.buf, buf.len, OBJ_TREE, result_oid))
> + ret = opt->write_pack
> + ? index_tree_bulk_checkin_incore(result_oid,
> + buf.buf, buf.len, "", 1)
> + : write_object_file(buf.buf, buf.len, OBJ_TREE, result_oid);
> +
> + if (ret)
> ret = -1;
> +
> strbuf_release(&buf);
> return ret;
> }
> @@ -3797,8 +3814,8 @@ static int write_completed_directory(struct merge_options *opt,
> */
> dir_info->is_null = 0;
> dir_info->result.mode = S_IFDIR;
> - if (write_tree(&dir_info->result.oid, &info->versions, offset,
> - opt->repo->hash_algo->rawsz) < 0)
> + if (write_tree(opt, &dir_info->result.oid, &info->versions,
> + offset, opt->repo->hash_algo->rawsz) < 0)
> ret = -1;
> }
>
> @@ -4332,9 +4349,13 @@ static int process_entries(struct merge_options *opt,
> fflush(stdout);
> BUG("dir_metadata accounting completely off; shouldn't happen");
> }
> - if (write_tree(result_oid, &dir_metadata.versions, 0,
> + if (write_tree(opt, result_oid, &dir_metadata.versions, 0,
> opt->repo->hash_algo->rawsz) < 0)
> ret = -1;
> +
> + if (opt->write_pack)
> + end_odb_transaction();
> +
> cleanup:
> string_list_clear(&plist, 0);
> string_list_clear(&dir_metadata.versions, 0);
> @@ -4878,6 +4899,9 @@ static void merge_start(struct merge_options *opt, struct merge_result *result)
> */
> strmap_init(&opt->priv->conflicts);
>
> + if (opt->write_pack)
> + begin_odb_transaction();
> +
> trace2_region_leave("merge", "allocate/init", opt->repo);
> }
>
> diff --git a/merge-recursive.h b/merge-recursive.h
> index 3d3b3e3c29..5c5ff380a8 100644
> --- a/merge-recursive.h
> +++ b/merge-recursive.h
> @@ -48,6 +48,7 @@ struct merge_options {
> unsigned renormalize : 1;
> unsigned record_conflict_msgs_as_headers : 1;
> const char *msg_header_prefix;
> + unsigned write_pack : 1;
>
> /* internal fields used by the implementation */
> struct merge_options_internal *priv;
> diff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh
> index b2c8a43fce..d2a8634523 100755
> --- a/t/t4301-merge-tree-write-tree.sh
> +++ b/t/t4301-merge-tree-write-tree.sh
> @@ -945,4 +945,97 @@ test_expect_success 'check the input format when --stdin is passed' '
> test_cmp expect actual
> '
>
> +packdir=".git/objects/pack"
> +
> +test_expect_success 'merge-tree can pack its result with --write-pack' '
> + test_when_finished "rm -rf repo" &&
> + git init repo &&
> +
> + # base has lines [3, 4, 5]
> + # - side adds to the beginning, resulting in [1, 2, 3, 4, 5]
> + # - other adds to the end, resulting in [3, 4, 5, 6, 7]
> + #
> + # merging the two should result in a new blob object containing
> + # [1, 2, 3, 4, 5, 6, 7], along with a new tree.
> + test_commit -C repo base file "$(test_seq 3 5)" &&
> + git -C repo branch -M main &&
> + git -C repo checkout -b side main &&
> + test_commit -C repo side file "$(test_seq 1 5)" &&
> + git -C repo checkout -b other main &&
> + test_commit -C repo other file "$(test_seq 3 7)" &&
> +
> + find repo/$packdir -type f -name "pack-*.idx" >packs.before &&
> + tree="$(git -C repo merge-tree --write-pack \
> + refs/tags/side refs/tags/other)" &&
> + blob="$(git -C repo rev-parse $tree:file)" &&
> + find repo/$packdir -type f -name "pack-*.idx" >packs.after &&
While we do assert that we write a new packfile, we don't assert whether
parts of the written object may have been written as loose objects. Do
we want to tighten the checks to verify that?
Patrick
> + test_must_be_empty packs.before &&
> + test_line_count = 1 packs.after &&
> +
> + git show-index <$(cat packs.after) >objects &&
> + test_line_count = 2 objects &&
> + grep "^[1-9][0-9]* $tree" objects &&
> + grep "^[1-9][0-9]* $blob" objects
> +'
> +
> +test_expect_success 'merge-tree can write multiple packs with --write-pack' '
> + test_when_finished "rm -rf repo" &&
> + git init repo &&
> + (
> + cd repo &&
> +
> + git config pack.packSizeLimit 512 &&
> +
> + test_seq 512 >f &&
> +
> + # "f" contains roughly ~2,000 bytes.
> + #
> + # Each side ("foo" and "bar") adds a small amount of data at the
> + # beginning and end of "base", respectively.
> + git add f &&
> + test_tick &&
> + git commit -m base &&
> + git branch -M main &&
> +
> + git checkout -b foo main &&
> + {
> + echo foo && cat f
> + } >f.tmp &&
> + mv f.tmp f &&
> + git add f &&
> + test_tick &&
> + git commit -m foo &&
> +
> + git checkout -b bar main &&
> + echo bar >>f &&
> + git add f &&
> + test_tick &&
> + git commit -m bar &&
> +
> + find $packdir -type f -name "pack-*.idx" >packs.before &&
> + # Merging either side should result in a new object which is
> + # larger than 1M, thus the result should be split into two
> + # separate packs.
> + tree="$(git merge-tree --write-pack \
> + refs/heads/foo refs/heads/bar)" &&
> + blob="$(git rev-parse $tree:f)" &&
> + find $packdir -type f -name "pack-*.idx" >packs.after &&
> +
> + test_must_be_empty packs.before &&
> + test_line_count = 2 packs.after &&
> + for idx in $(cat packs.after)
> + do
> + git show-index <$idx || return 1
> + done >objects &&
> +
> + # The resulting set of packs should contain one copy of both
> + # objects, each in a separate pack.
> + test_line_count = 2 objects &&
> + grep "^[1-9][0-9]* $tree" objects &&
> + grep "^[1-9][0-9]* $blob" objects
> +
> + )
> +'
> +
> test_done
> --
> 2.42.0.425.g963d08ddb3.dirty
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v5 0/5] merge-ort: implement support for packing objects together
From: Patrick Steinhardt @ 2023-10-25 7:58 UTC (permalink / raw)
To: Taylor Blau
Cc: git, Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano
In-Reply-To: <cover.1698101088.git.me@ttaylorr.com>
[-- Attachment #1: Type: text/plain, Size: 18764 bytes --]
On Mon, Oct 23, 2023 at 06:44:49PM -0400, Taylor Blau wrote:
> (Rebased onto the current tip of 'master', which is ceadf0f3cf (The
> twentieth batch, 2023-10-20)).
>
> This series implements support for a new merge-tree option,
> `--write-pack`, which causes any newly-written objects to be packed
> together instead of being stored individually as loose.
>
> This is a minor follow-up that could be taken instead of v4 (though the
> changes between these two most recent rounds are stylistic and a matter
> of subjective opinion).
>
> This moves us the bulk_checkin_source structure introduced in response
> to Junio's suggestion during the last round further in the OOP
> direction. Instead of switching on the enum type of the source, have
> function pointers for read() and seek() respectively.
>
> The functionality at the end is the same, but this avoids some of the
> namespacing issues that Peff pointed out while looking at v4. But I
> think that this approach ended up being less heavy-weight than I had
> originally imagined, so I think that this version is a worthwhile
> improvement over v4.
>
> Beyond that, the changes since last time can be viewed in the range-diff
> below. Thanks in advance for any review!
Overall this version looks good to me. I've only got two smallish nits
and one question regarding the tests.
Thanks!
Patrick
> [1]: https://lore.kernel.org/git/xmqq34y7plj4.fsf@gitster.g/
>
> Taylor Blau (5):
> bulk-checkin: extract abstract `bulk_checkin_source`
> bulk-checkin: generify `stream_blob_to_pack()` for arbitrary types
> bulk-checkin: introduce `index_blob_bulk_checkin_incore()`
> bulk-checkin: introduce `index_tree_bulk_checkin_incore()`
> builtin/merge-tree.c: implement support for `--write-pack`
>
> Documentation/git-merge-tree.txt | 4 +
> builtin/merge-tree.c | 5 +
> bulk-checkin.c | 197 +++++++++++++++++++++++++++----
> bulk-checkin.h | 8 ++
> merge-ort.c | 42 +++++--
> merge-recursive.h | 1 +
> t/t4301-merge-tree-write-tree.sh | 93 +++++++++++++++
> 7 files changed, 316 insertions(+), 34 deletions(-)
>
> Range-diff against v4:
> 1: 97bb6e9f59 ! 1: 696aa027e4 bulk-checkin: extract abstract `bulk_checkin_source`
> @@ bulk-checkin.c: static int already_written(struct bulk_checkin_packfile *state,
> }
>
> +struct bulk_checkin_source {
> -+ enum { SOURCE_FILE } type;
> ++ off_t (*read)(struct bulk_checkin_source *, void *, size_t);
> ++ off_t (*seek)(struct bulk_checkin_source *, off_t);
> +
> -+ /* SOURCE_FILE fields */
> -+ int fd;
> ++ union {
> ++ struct {
> ++ int fd;
> ++ } from_fd;
> ++ } data;
> +
> -+ /* common fields */
> + size_t size;
> + const char *path;
> +};
> +
> -+static off_t bulk_checkin_source_seek_to(struct bulk_checkin_source *source,
> -+ off_t offset)
> ++static off_t bulk_checkin_source_read_from_fd(struct bulk_checkin_source *source,
> ++ void *buf, size_t nr)
> +{
> -+ switch (source->type) {
> -+ case SOURCE_FILE:
> -+ return lseek(source->fd, offset, SEEK_SET);
> -+ default:
> -+ BUG("unknown bulk-checkin source: %d", source->type);
> -+ }
> ++ return read_in_full(source->data.from_fd.fd, buf, nr);
> +}
> +
> -+static ssize_t bulk_checkin_source_read(struct bulk_checkin_source *source,
> -+ void *buf, size_t nr)
> ++static off_t bulk_checkin_source_seek_from_fd(struct bulk_checkin_source *source,
> ++ off_t offset)
> +{
> -+ switch (source->type) {
> -+ case SOURCE_FILE:
> -+ return read_in_full(source->fd, buf, nr);
> -+ default:
> -+ BUG("unknown bulk-checkin source: %d", source->type);
> -+ }
> ++ return lseek(source->data.from_fd.fd, offset, SEEK_SET);
> ++}
> ++
> ++static void init_bulk_checkin_source_from_fd(struct bulk_checkin_source *source,
> ++ int fd, size_t size,
> ++ const char *path)
> ++{
> ++ memset(source, 0, sizeof(struct bulk_checkin_source));
> ++
> ++ source->read = bulk_checkin_source_read_from_fd;
> ++ source->seek = bulk_checkin_source_seek_from_fd;
> ++
> ++ source->data.from_fd.fd = fd;
> ++
> ++ source->size = size;
> ++ source->path = path;
> +}
> +
> /*
> @@ bulk-checkin.c: static int stream_blob_to_pack(struct bulk_checkin_packfile *sta
> - ssize_t read_result = read_in_full(fd, ibuf, rsize);
> + ssize_t read_result;
> +
> -+ read_result = bulk_checkin_source_read(source, ibuf,
> -+ rsize);
> ++ read_result = source->read(source, ibuf, rsize);
> if (read_result < 0)
> - die_errno("failed to read from '%s'", path);
> + die_errno("failed to read from '%s'",
> @@ bulk-checkin.c: static int deflate_blob_to_pack(struct bulk_checkin_packfile *st
> unsigned header_len;
> struct hashfile_checkpoint checkpoint = {0};
> struct pack_idx_entry *idx = NULL;
> -+ struct bulk_checkin_source source = {
> -+ .type = SOURCE_FILE,
> -+ .fd = fd,
> -+ .size = size,
> -+ .path = path,
> -+ };
> ++ struct bulk_checkin_source source;
> ++
> ++ init_bulk_checkin_source_from_fd(&source, fd, size, path);
>
> seekback = lseek(fd, 0, SEEK_CUR);
> if (seekback == (off_t) -1)
> @@ bulk-checkin.c: static int deflate_blob_to_pack(struct bulk_checkin_packfile *st
> state->offset = checkpoint.offset;
> flush_bulk_checkin_packfile(state);
> - if (lseek(fd, seekback, SEEK_SET) == (off_t) -1)
> -+ if (bulk_checkin_source_seek_to(&source, seekback) == (off_t)-1)
> ++ if (source.seek(&source, seekback) == (off_t)-1)
> return error("cannot seek back");
> }
> the_hash_algo->final_oid_fn(result_oid, &ctx);
> 2: 9d633df339 < -: ---------- bulk-checkin: generify `stream_blob_to_pack()` for arbitrary types
> 3: d5bbd7810e ! 2: 596bd028a7 bulk-checkin: refactor deflate routine to accept a `bulk_checkin_source`
> @@ Metadata
> Author: Taylor Blau <me@ttaylorr.com>
>
> ## Commit message ##
> - bulk-checkin: refactor deflate routine to accept a `bulk_checkin_source`
> + bulk-checkin: generify `stream_blob_to_pack()` for arbitrary types
>
> - Prepare for a future change where we will want to use a routine very
> - similar to the existing `deflate_blob_to_pack()` but over arbitrary
> - sources (i.e. either open file-descriptors, or a location in memory).
> + The existing `stream_blob_to_pack()` function is named based on the fact
> + that it knows only how to stream blobs into a bulk-checkin pack.
>
> - Extract out a common "deflate_obj_to_pack()" routine that acts on a
> - bulk_checkin_source, instead of a (int, size_t) pair. Then rewrite
> - `deflate_blob_to_pack()` in terms of it.
> + But there is no longer anything in this function which prevents us from
> + writing objects of arbitrary types to the bulk-checkin pack. Prepare to
> + write OBJ_TREEs by removing this assumption, adding an `enum
> + object_type` parameter to this function's argument list, and renaming it
> + to `stream_obj_to_pack()` accordingly.
>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
>
> ## bulk-checkin.c ##
> +@@ bulk-checkin.c: static void init_bulk_checkin_source_from_fd(struct bulk_checkin_source *source,
> + * status before calling us just in case we ask it to call us again
> + * with a new pack.
> + */
> +-static int stream_blob_to_pack(struct bulk_checkin_packfile *state,
> +- git_hash_ctx *ctx, off_t *already_hashed_to,
> +- struct bulk_checkin_source *source,
> +- unsigned flags)
> ++static int stream_obj_to_pack(struct bulk_checkin_packfile *state,
> ++ git_hash_ctx *ctx, off_t *already_hashed_to,
> ++ struct bulk_checkin_source *source,
> ++ enum object_type type, unsigned flags)
> + {
> + git_zstream s;
> + unsigned char ibuf[16384];
> +@@ bulk-checkin.c: static int stream_blob_to_pack(struct bulk_checkin_packfile *state,
> +
> + git_deflate_init(&s, pack_compression_level);
> +
> +- hdrlen = encode_in_pack_object_header(obuf, sizeof(obuf), OBJ_BLOB,
> +- size);
> ++ hdrlen = encode_in_pack_object_header(obuf, sizeof(obuf), type, size);
> + s.next_out = obuf + hdrlen;
> + s.avail_out = sizeof(obuf) - hdrlen;
> +
> @@ bulk-checkin.c: static void prepare_to_stream(struct bulk_checkin_packfile *state,
> die_errno("unable to write pack header");
> }
> @@ bulk-checkin.c: static void prepare_to_stream(struct bulk_checkin_packfile *stat
> unsigned header_len;
> struct hashfile_checkpoint checkpoint = {0};
> struct pack_idx_entry *idx = NULL;
> -- struct bulk_checkin_source source = {
> -- .type = SOURCE_FILE,
> -- .fd = fd,
> -- .size = size,
> -- .path = path,
> -- };
> +- struct bulk_checkin_source source;
>
> +- init_bulk_checkin_source_from_fd(&source, fd, size, path);
> +-
> - seekback = lseek(fd, 0, SEEK_CUR);
> - if (seekback == (off_t) -1)
> - return error("cannot find the current offset");
> @@ bulk-checkin.c: static int deflate_blob_to_pack(struct bulk_checkin_packfile *st
> prepare_to_stream(state, flags);
> if (idx) {
> @@ bulk-checkin.c: static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
> + idx->offset = state->offset;
> crc32_begin(state->f);
> }
> - if (!stream_obj_to_pack(state, &ctx, &already_hashed_to,
> -- &source, OBJ_BLOB, flags))
> +- if (!stream_blob_to_pack(state, &ctx, &already_hashed_to,
> +- &source, flags))
> ++ if (!stream_obj_to_pack(state, &ctx, &already_hashed_to,
> + source, type, flags))
> break;
> /*
> @@ bulk-checkin.c: static int deflate_blob_to_pack(struct bulk_checkin_packfile *st
> hashfile_truncate(state->f, &checkpoint);
> state->offset = checkpoint.offset;
> flush_bulk_checkin_packfile(state);
> -- if (bulk_checkin_source_seek_to(&source, seekback) == (off_t)-1)
> -+ if (bulk_checkin_source_seek_to(source, seekback) == (off_t)-1)
> +- if (source.seek(&source, seekback) == (off_t)-1)
> ++ if (source->seek(source, seekback) == (off_t)-1)
> return error("cannot seek back");
> }
> the_hash_algo->final_oid_fn(result_oid, &ctx);
> @@ bulk-checkin.c: static int deflate_blob_to_pack(struct bulk_checkin_packfile *st
> + int fd, size_t size,
> + const char *path, unsigned flags)
> +{
> -+ struct bulk_checkin_source source = {
> -+ .type = SOURCE_FILE,
> -+ .fd = fd,
> -+ .size = size,
> -+ .path = path,
> -+ };
> -+ off_t seekback = lseek(fd, 0, SEEK_CUR);
> ++ struct bulk_checkin_source source;
> ++ off_t seekback;
> ++
> ++ init_bulk_checkin_source_from_fd(&source, fd, size, path);
> ++
> ++ seekback = lseek(fd, 0, SEEK_CUR);
> + if (seekback == (off_t) -1)
> + return error("cannot find the current offset");
> +
> 4: e427fe6ad3 < -: ---------- bulk-checkin: implement `SOURCE_INCORE` mode for `bulk_checkin_source`
> 5: 48095afe80 ! 3: d8cf8e4395 bulk-checkin: introduce `index_blob_bulk_checkin_incore()`
> @@ Metadata
> ## Commit message ##
> bulk-checkin: introduce `index_blob_bulk_checkin_incore()`
>
> - Now that we have factored out many of the common routines necessary to
> - index a new object into a pack created by the bulk-checkin machinery, we
> - can introduce a variant of `index_blob_bulk_checkin()` that acts on
> - blobs whose contents we can fit in memory.
> + Introduce `index_blob_bulk_checkin_incore()` which allows streaming
> + arbitrary blob contents from memory into the bulk-checkin pack.
> +
> + In order to support streaming from a location in memory, we must
> + implement a new kind of bulk_checkin_source that does just that. These
> + implementation in spread out across:
> +
> + - init_bulk_checkin_source_incore()
> + - bulk_checkin_source_read_incore()
> + - bulk_checkin_source_seek_incore()
> +
> + Note that, unlike file descriptors, which manage their own offset
> + internally, we have to keep track of how many bytes we've read out of
> + the buffer, and make sure we don't read past the end of the buffer.
>
> This will be useful in a couple of more commits in order to provide the
> `merge-tree` builtin with a mechanism to create a new pack containing
> @@ Commit message
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
>
> ## bulk-checkin.c ##
> +@@ bulk-checkin.c: struct bulk_checkin_source {
> + struct {
> + int fd;
> + } from_fd;
> ++ struct {
> ++ const void *buf;
> ++ size_t nr_read;
> ++ } incore;
> + } data;
> +
> + size_t size;
> +@@ bulk-checkin.c: static off_t bulk_checkin_source_seek_from_fd(struct bulk_checkin_source *source
> + return lseek(source->data.from_fd.fd, offset, SEEK_SET);
> + }
> +
> ++static off_t bulk_checkin_source_read_incore(struct bulk_checkin_source *source,
> ++ void *buf, size_t nr)
> ++{
> ++ const unsigned char *src = source->data.incore.buf;
> ++
> ++ if (source->data.incore.nr_read > source->size)
> ++ BUG("read beyond bulk-checkin source buffer end "
> ++ "(%"PRIuMAX" > %"PRIuMAX")",
> ++ (uintmax_t)source->data.incore.nr_read,
> ++ (uintmax_t)source->size);
> ++
> ++ if (nr > source->size - source->data.incore.nr_read)
> ++ nr = source->size - source->data.incore.nr_read;
> ++
> ++ src += source->data.incore.nr_read;
> ++
> ++ memcpy(buf, src, nr);
> ++ source->data.incore.nr_read += nr;
> ++ return nr;
> ++}
> ++
> ++static off_t bulk_checkin_source_seek_incore(struct bulk_checkin_source *source,
> ++ off_t offset)
> ++{
> ++ if (!(0 <= offset && offset < source->size))
> ++ return (off_t)-1;
> ++ source->data.incore.nr_read = offset;
> ++ return source->data.incore.nr_read;
> ++}
> ++
> + static void init_bulk_checkin_source_from_fd(struct bulk_checkin_source *source,
> + int fd, size_t size,
> + const char *path)
> +@@ bulk-checkin.c: static void init_bulk_checkin_source_from_fd(struct bulk_checkin_source *source,
> + source->path = path;
> + }
> +
> ++static void init_bulk_checkin_source_incore(struct bulk_checkin_source *source,
> ++ const void *buf, size_t size,
> ++ const char *path)
> ++{
> ++ memset(source, 0, sizeof(struct bulk_checkin_source));
> ++
> ++ source->read = bulk_checkin_source_read_incore;
> ++ source->seek = bulk_checkin_source_seek_incore;
> ++
> ++ source->data.incore.buf = buf;
> ++ source->data.incore.nr_read = 0;
> ++
> ++ source->size = size;
> ++ source->path = path;
> ++}
> ++
> + /*
> + * Read the contents from 'source' for 'size' bytes, streaming it to the
> + * packfile in state while updating the hash in ctx. Signal a failure
> @@ bulk-checkin.c: static int deflate_obj_to_pack(struct bulk_checkin_packfile *state,
> return 0;
> }
> @@ bulk-checkin.c: static int deflate_obj_to_pack(struct bulk_checkin_packfile *sta
> + const char *path, enum object_type type,
> + unsigned flags)
> +{
> -+ struct bulk_checkin_source source = {
> -+ .type = SOURCE_INCORE,
> -+ .buf = buf,
> -+ .size = size,
> -+ .read = 0,
> -+ .path = path,
> -+ };
> ++ struct bulk_checkin_source source;
> ++
> ++ init_bulk_checkin_source_incore(&source, buf, size, path);
> +
> + return deflate_obj_to_pack(state, result_oid, &source, type, 0, flags);
> +}
> 6: 60568f9281 = 4: 2670192802 bulk-checkin: introduce `index_tree_bulk_checkin_incore()`
> 7: b9be9df122 ! 5: 3595db76a5 builtin/merge-tree.c: implement support for `--write-pack`
> @@ Documentation/git-merge-tree.txt: OPTIONS
>
> ## builtin/merge-tree.c ##
> @@
> - #include "quote.h"
> #include "tree.h"
> #include "config.h"
> + #include "strvec.h"
> +#include "bulk-checkin.h"
>
> static int line_termination = '\n';
>
> @@ builtin/merge-tree.c: struct merge_tree_options {
> - int show_messages;
> int name_only;
> int use_stdin;
> + struct merge_options merge_options;
> + int write_pack;
> };
>
> static int real_merge(struct merge_tree_options *o,
> @@ builtin/merge-tree.c: static int real_merge(struct merge_tree_options *o,
> - init_merge_options(&opt, the_repository);
> + _("not something we can merge"));
>
> opt.show_rename_progress = 0;
> + opt.write_pack = o->write_pack;
> @@ builtin/merge-tree.c: static int real_merge(struct merge_tree_options *o,
> opt.branch1 = branch1;
> opt.branch2 = branch2;
> @@ builtin/merge-tree.c: int cmd_merge_tree(int argc, const char **argv, const char *prefix)
> - &merge_base,
> - N_("commit"),
> N_("specify a merge-base for the merge")),
> + OPT_STRVEC('X', "strategy-option", &xopts, N_("option=value"),
> + N_("option for selected merge strategy")),
> + OPT_BOOL(0, "write-pack", &o.write_pack,
> + N_("write new objects to a pack instead of as loose")),
> OPT_END()
>
> base-commit: ceadf0f3cf51550166a387ec8508bb55e7883057
> --
> 2.42.0.425.g963d08ddb3.dirty
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [OUTREACHY] Final Application For Git Internshhip
From: Christian Couder @ 2023-10-25 8:03 UTC (permalink / raw)
To: Naomi Ibe; +Cc: git
In-Reply-To: <CACS=G2yJa3xvCPN6bqsa4+vSkwsdUouhqNvuH6y_CC2cJ0YSmQ@mail.gmail.com>
On Tue, Oct 24, 2023 at 9:48 AM Naomi Ibe <naomi.ibeh69@gmail.com> wrote:
>
> I'm Ibe Naomi Amarachi,I'm a Nigerian and I currently live in
> Lagos,Nigeria. I'm a graduate of the African Leadership Xcelerator
> Software Engineering program (ALX SE), with specialization in backend
> web development and I'm applying for the "Moving Existing Tests to a
> Unit Testing Framework" project
>
> Some of my projects that involve working with Shell,C and Git can be found here:
>
> https://github.com/Amarajah/alx-system_engineering-devops
>
> https://github.com/Amarajah/alx-low_level_programming
>
> Git has been a part of my software engineering journey from day one
> and it's allowed me to collaborate with peers and also keep track of
> my personal projects. Currently Git uses end-to-end tests for error
> conditions that could easily be captured by unit tests, the project is
> aimed at turning end-to-end tests to unit tests, and I'd love to be a
> part of it
Maybe a period is missing at the end of the above sentence. Otherwise Ok.
> My microproject contribution is here:
>
> https://public-inbox.org/git/20231009011652.1791-1-naomi.ibeh69@gmail.com/T/#u
>
> And here is my updated contribution link after review by the Git community:
>
> https://public-inbox.org/git/xmqqttqox5cp.fsf@gitster.g/T/#u
It could help to say if your contribution has been merged to 'master',
'next', 'seen' or not at all.
> Below is my project timeline:
> (Of course I'd be very much willing to work with the community and
> mentors to edit it so it perfectly meets up to the community's
> expectations)
>
> October 2, 2023 - October 30, 2023
>
> Familiarizing myself with the community , mailing list and
> contributing my microproject
>
> November 20, 2023 - December 4, 2023
>
> Familiarizing myself with the already existing tests and also the
> chosen unit test framework
>
> Do more research on the internship projects and find better ways to
> get it done in harmony with coding guidelines and community
> requirements
>
> December 4, 2023 - December 31, 2023
>
> First document the initial state of the test files, then make sure all
> test files conform to coding guidelines down to the tiniest details
> (e.g git/t/helper/test-write-cache.c and git/t/helper/test-advise.c
> have die() messages which do not conform to coding guidelines)
We don't advise spending a lot of time during your internship with
small things that could be part of someone else's microproject later.
You should be focused on the internship goal first.
Of course if you are migrating some code to the new unit test
framework, it's Ok to improve that code before migrating it. But no
need to improve everything under t/helper before starting to migrate
parts of the code there.
I think that one of the important tasks to be done early is to
identify what code in t/helper is unit testing C code and what code is
really about helping other tests in the t/t*.sh scripts. It would be
nice if you could give an example of each kind of code.
> January 2, 2024 - January 31, 2024
>
> Run the tests and verify they still work as originally intended
>
> Begin migrations of test files
>
> Test migrated files and make necessary changes based on feedback
> received from teammates and mentors
An example of how you would migrate parts of a test, or how a migrated
test would look like, would be nice.
> February 1, 2024 - March 1, 2024
>
> Continue testing migrated files and making necessary changes based on
> feedback received from teammates and mentors
>
> Document each step and request for reviews from teammates and mentors
>
> Tidy up the project,make sure all necessary files are migrated, they
> all work as intended, they are well documented and that there are no
> conflicts
Thanks,
Christian.
^ permalink raw reply
* Re: [PATCH v1 3/4] config: factor out global config file retrievalync-mailbox>
From: Patrick Steinhardt @ 2023-10-25 8:05 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: Taylor Blau, git, stolee
In-Reply-To: <2b764f52-d3ae-467f-a915-fb73beb247bb@app.fastmail.com>
[-- Attachment #1: Type: text/plain, Size: 1096 bytes --]
On Wed, Oct 25, 2023 at 09:33:23AM +0200, Kristoffer Haugsbakk wrote:
> On Wed, Oct 25, 2023, at 07:38, Patrick Steinhardt wrote:
> >> What do you guys think the signature of `git_global_config` should be?
> >
> > Either of the following:
> >
> > - `int git_global_config(char **out_pat)`
> > - `char **git_global_config(void)`
> >
> > In the first case you'd signal error via a non-zero return value,
> > whereas in the second case you would signal it via a `NULL` return
> > value.
> >
> > To decide which one to go with I'd recommend to check whether there is
> > any similar precedent in "config.h" and what style that precedent uses.
>
> Okay thanks. So no parameter for determining whether one is writing or
> just reading the file.
This parameter would only exist for the purpose of the error message,
right? If so, I think that'd be overkill. If we want to have differing
errors depending on how the function is called the best way to handle
that would likely be to generate the error message at the callsite
instead of in the library itself.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [Outreachy] Move existing tests to a unit testing framework
From: Christian Couder @ 2023-10-25 8:18 UTC (permalink / raw)
To: Achu Luma; +Cc: git, Junio C Hamano
In-Reply-To: <CAFR+8DzdFbwaiHtZSdLMqWYWh=fK0WA4c48+eBug-ZeAgddhcQ@mail.gmail.com>
On Tue, Oct 24, 2023 at 4:25 PM Achu Luma <ach.lumap@gmail.com> wrote:
>
> On Mon, Oct 23, 2023 at 2:41 PM Christian Couder
> <christian.couder@gmail.com> wrote:
> > Maybe if you have time you could add some descriptions or comments
> > related to the above emails and documents. For example you could tell
> > what the new unit test framework will be like, how the unit tests will
> > look like, etc. Maybe a short overview would be nice.
> >
> sure,
> 1- https://lore.kernel.org/git/0169ce6fb9ccafc089b74ae406db0d1a8ff8ac65.1688165272.git.steadmon@google.com/
> :
> The emails highlight the significant milestones achieved in
> defining and testing the custom TAP
> framework for writing git unit tests. It also contains some
> examples of implementation such as
> that of STRBUF_INIT with output:
> ok 1 - static initialization works
> 1..1
>
> 2- https://github.com/steadmon/git/blob/unit-tests-asciidoc/Documentation/technical/unit-tests.adoc:
> From this technical doc, the new unit test framework in the Git
> project represents a significant
> enhancement, introducing a systematic and efficient approach to
> unit testing. The custom git
> TAP implementation was selected from several alternatives based
> on strict criteria as the most
> suitable test framework for porting the unit tests.
> The unit tests are written in pure C, eliminating the need for
> the previous shell/test-tool helper
> setup, simplifying test configuration, data handling, and
> reducing testing runtime.
> Each unit test is encapsulated as a function and employs a range
> of predefined check functions
> for validation. These checks can evaluate conditions, compare
> integers or characters, validate
> strings, and more, providing comprehensive coverage for test scenarios.
>
> When a test is run using the TEST() macro, it undergoes a series
> of checks, and if any check fails,
> a diagnostic message is printed to aid in debugging. This
> diagnostic output includes information
> about the specific check that failed, the file and line number
> where it occurred, and a clear comparison
> of the expected and actual values. Such detailed reporting
> simplifies the identification and resolution
> of issues, contributing to codebase stability.
>
> Additionally, the framework supports features like skipping tests
> with explanations, sending custom
> diagnostic messages using test_msg(), and marking known-to-fail
> checks using TEST_TODO().
> This flexibility allows developers to tailor their tests to
> specific scenarios while ensuring a
> comprehensive testing suite.
Ok, please add all these explanations above as well as those below to
your application document. We prefer that you consider your
application document like a patch. So you would send to the mailing
list several versions of it for review before submitting officially.
> > You could also try to apply the patches in the series that adds the
> > test framework, or alternatively use the 'seen' branch where the
> > series has been merged, and start playing with it by writing, or
> > porting, a small example test.
> >
> ok, I think I can push a patch for one.
I don't think it's necessary to send it to the mailing list for now,
but it should definitely be part of your application document.
> > > -- Create a New C Test File: For each unit test I plan to migrate, create a new C source file (.c) in the Git project's test suite directory(t/unit-tests). Name it appropriately to reflect the purpose of the test.
> >
> > Could you provide an example of what the new name would be for an
> > existing test that is worth porting?
> >
> Sure... let's consider an existing unit test in t/helper directory
> such as t/helper/test-date.c or
> its shell named t0006-date.sh, which is part of the current
> shell-based test suite. In the context
> of the new unit testing framework, this test could be reimagined and renamed as
> "t-date.c". The "t-" prefix is typically used for test program files
> in Git, and "date" is retained to
> reflect the nature of the tests within this suite.
Ok.
> > > -- Include Necessary Headers:In the new C test file, include the necessary Git unit test framework headers. Typically, this includes headers like "test-lib.h" and others relevant to the specific test.
> > > #include "test-lib.h"
> >
> > Maybe you could continue the above example and tell which headers
> > would be needed for it?
> >
> > > -- Convert Test Logic: Refactor the test logic from the original Shell script into the new C-based test format. Use the testing macros provided by the Git unit test framework, such as test_expect_success, test_expect_failure, etc., to define the tests.
> > > test_expect_success("simple progress display", "{
> > > // Test logic here...
> > > }");
> >
> > Ok, a simple example would be nice too.
> >
> we can continue with the example used for naming: test-date.c. a
> typical t-date.c unit test would look
> like the following:
> --
> #include "test-lib.h"
> #include "date.h"
> --
> date.h here is a necessary header file. Now refactoring the test logic
> from the original shell script:
> --
> #include "test-lib.h"
> #include "date.h"
>
> static void test_parse_dates(void)
> {
> const char *dates[] = { "invalid_date", "2023-10-17 10:00:00 +0200", NULL };
>
> for (const char **argv = dates; *argv; argv++) {
> check_int(parse_dates((const char *[]){ *argv, NULL }), 0);
> }
> }
> --
Nice!
> > > -- Add Test Descriptions: Provide clear and informative descriptions for each test using the testing macros. These descriptions will help in identifying the purpose of each test when the test suite is run.
> >
> > This would seem to be part of the previous step, as you would have to
> > provide a description when using the testing macro. But Ok.
> >
> > > -- Define a Test Entry Point: Create a cmd_main function as the entry point for the C-based tests. Inside this function, include the test functions using the testing macros.
> > > int cmd_main(int argc, const char **argv) {
> > > // Test functions...
> > > return test_done();
> > > }
> >
> > Yeah, continuing an example would be nice.
> >
> Continuing, we can add a test entrance as follows:
> --
> #include "test-lib.h"
> #include "date.h"
>
> static void test_parse_dates(void)
> {
> const char *dates[] = { "invalid_date", "2023-10-17 10:00:00 +0200", NULL };
>
> for (const char **argv = dates; *argv; argv++) {
> check_int(parse_dates((const char *[]){ *argv, NULL }), 0);
> }
> }
>
> int main(int argc UNUSED, const char **argv UNUSED)
> {
> TEST(test_parse_dates, "Test date parsing");
>
> return test_done();
> }
> --
>
> A typical unit tests with the custom TAP framework would look
> something like above. This might run in theory
> but I have not yet run it as I used it here just for demonstration.
> The unit tests can be built using
> "make unit-tests." Additionally, Makefile can be modified to add the
> file to the build:
> --
> UNIT_TEST_PROGRAMS += t-date
> --
Great!
Thanks,
Christian.
^ 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