* [PATCH 2/2] sequencer: unset GIT_CHERRY_PICK_HELP for 'exec' commands
From: Vegard Nossum @ 2024-02-05 14:13 UTC (permalink / raw)
To: gitster; +Cc: git, Jonathan Nieder, Vegard Nossum, Phillip Wood
In-Reply-To: <0adb1068-ef10-44ed-ad1d-e0927a09245d@gmail.com>
Running "git cherry-pick" as an x-command in the rebase plan loses the
original authorship information.
To fix this, unset GIT_CHERRY_PICK_HELP for 'exec' commands.
Link: https://lore.kernel.org/git/0adb1068-ef10-44ed-ad1d-e0927a09245d@gmail.com/
Suggested-by: Phillip Wood <phillip.wood123@gmail.com>
Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
---
sequencer.c | 1 +
t/t3515-cherry-pick-rebase.sh | 2 +-
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/sequencer.c b/sequencer.c
index 91de546b32..f49a871ac0 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3641,6 +3641,7 @@ static int do_exec(struct repository *r, const char *command_line)
fprintf(stderr, _("Executing: %s\n"), command_line);
cmd.use_shell = 1;
strvec_push(&cmd.args, command_line);
+ strvec_push(&cmd.env, "GIT_CHERRY_PICK_HELP");
status = run_command(&cmd);
/* force re-reading of the cache */
diff --git a/t/t3515-cherry-pick-rebase.sh b/t/t3515-cherry-pick-rebase.sh
index ffe6f5fe2a..5cb2b96f66 100755
--- a/t/t3515-cherry-pick-rebase.sh
+++ b/t/t3515-cherry-pick-rebase.sh
@@ -23,7 +23,7 @@ test_expect_success 'cherry-pick preserves authorship information' '
test_cmp expected actual
'
-test_expect_failure 'cherry-pick inside rebase preserves authorship information' '
+test_expect_success 'cherry-pick inside rebase preserves authorship information' '
git checkout -B tmp feature &&
echo "x git cherry-pick -x foo" >rebase-plan &&
test_must_fail env GIT_SEQUENCE_EDITOR="cp rebase-plan" git rebase -i feature &&
--
2.34.1
^ permalink raw reply related
* [PATCH] Always check the return value of `repo_read_object_file()`
From: Johannes Schindelin via GitGitGadget @ 2024-02-05 14:35 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
From: Johannes Schindelin <johannes.schindelin@gmx.de>
There are a couple of places in Git's source code where the return value
is not checked. As a consequence, they are susceptible to segmentation
faults.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Always check the return value of repo_read_object_file()
I ran into this today, when I had tried git am -3 to import changes from
a repository into a different repository that has the first repository's
code vendored in. To make this work, I set
GIT_ALTERNATE_OBJECT_DIRECTORIES accordingly for the git am -3 call, but
forgot to set it for a subsequent git diff call, which then segfaulted.
There are still a couple of places left where there are checks but they
look dubious to me, as they simply continue as if an empty blob had been
read, for example in builtin/tag.c. However, there are checks that avoid
segfaults, so I left them alone.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1650%2Fdscho%2Fsafer-object-reads-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1650/dscho/safer-object-reads-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1650
bisect.c | 3 +++
builtin/cat-file.c | 10 ++++++++--
builtin/grep.c | 2 ++
builtin/notes.c | 6 ++++--
combine-diff.c | 2 ++
rerere.c | 3 +++
6 files changed, 22 insertions(+), 4 deletions(-)
diff --git a/bisect.c b/bisect.c
index f1273c787d9..f75e50c3397 100644
--- a/bisect.c
+++ b/bisect.c
@@ -158,6 +158,9 @@ static void show_list(const char *debug, int counted, int nr,
const char *subject_start;
int subject_len;
+ if (!buf)
+ die(_("unable to read %s"), oid_to_hex(&commit->object.oid));
+
fprintf(stderr, "%c%c%c ",
(commit_flags & TREESAME) ? ' ' : 'T',
(commit_flags & UNINTERESTING) ? 'U' : ' ',
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 7d4899348a3..bbf851138ec 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -221,6 +221,10 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
&type,
&size);
const char *target;
+
+ if (!buffer)
+ die(_("unable to read %s"), oid_to_hex(&oid));
+
if (!skip_prefix(buffer, "object ", &target) ||
get_oid_hex(target, &blob_oid))
die("%s not a valid tag", oid_to_hex(&oid));
@@ -416,6 +420,8 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d
contents = repo_read_object_file(the_repository, oid, &type,
&size);
+ if (!contents)
+ die("object %s disappeared", oid_to_hex(oid));
if (use_mailmap) {
size_t s = size;
@@ -423,8 +429,6 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d
size = cast_size_t_to_ulong(s);
}
- if (!contents)
- die("object %s disappeared", oid_to_hex(oid));
if (type != data->type)
die("object %s changed type!?", oid_to_hex(oid));
if (data->info.sizep && size != data->size && !use_mailmap)
@@ -481,6 +485,8 @@ static void batch_object_write(const char *obj_name,
buf = repo_read_object_file(the_repository, &data->oid, &data->type,
&data->size);
+ if (!buf)
+ die(_("unable to read %s"), oid_to_hex(&data->oid));
buf = replace_idents_using_mailmap(buf, &s);
data->size = cast_size_t_to_ulong(s);
diff --git a/builtin/grep.c b/builtin/grep.c
index c8e33f97755..982bcfc4b1d 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -571,6 +571,8 @@ static int grep_cache(struct grep_opt *opt,
data = repo_read_object_file(the_repository, &ce->oid,
&type, &size);
+ if (!data)
+ die(_("unable to read tree %s"), oid_to_hex(&ce->oid));
init_tree_desc(&tree, data, size);
hit |= grep_tree(opt, pathspec, &tree, &name, 0, 0);
diff --git a/builtin/notes.c b/builtin/notes.c
index e65cae0bcf7..caf20fd5bdd 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -716,9 +716,11 @@ static int append_edit(int argc, const char **argv, const char *prefix)
struct strbuf buf = STRBUF_INIT;
char *prev_buf = repo_read_object_file(the_repository, note, &type, &size);
- if (prev_buf && size)
+ if (!prev_buf)
+ die(_("unable to read %s"), oid_to_hex(note));
+ if (size)
strbuf_add(&buf, prev_buf, size);
- if (d.buf.len && prev_buf && size)
+ if (d.buf.len && size)
append_separator(&buf);
strbuf_insert(&d.buf, 0, buf.buf, buf.len);
diff --git a/combine-diff.c b/combine-diff.c
index db94581f724..d6d6fa16894 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -337,6 +337,8 @@ static char *grab_blob(struct repository *r,
free_filespec(df);
} else {
blob = repo_read_object_file(r, oid, &type, size);
+ if (!blob)
+ die(_("unable to read %s"), oid_to_hex(oid));
if (type != OBJ_BLOB)
die("object '%s' is not a blob!", oid_to_hex(oid));
}
diff --git a/rerere.c b/rerere.c
index ca7e77ba68c..13c94ded037 100644
--- a/rerere.c
+++ b/rerere.c
@@ -973,6 +973,9 @@ static int handle_cache(struct index_state *istate,
mmfile[i].ptr = repo_read_object_file(the_repository,
&ce->oid, &type,
&size);
+ if (!mmfile[i].ptr)
+ die(_("unable to read %s"),
+ oid_to_hex(&ce->oid));
mmfile[i].size = size;
}
}
base-commit: 2a540e432fe5dff3cfa9d3bf7ca56db2ad12ebb9
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH 2/2] sequencer: unset GIT_CHERRY_PICK_HELP for 'exec' commands
From: Kristoffer Haugsbakk @ 2024-02-05 14:38 UTC (permalink / raw)
To: Vegard Nossum; +Cc: git, Jonathan Nieder, Phillip Wood, Junio C Hamano
In-Reply-To: <20240205141335.762947-1-vegard.nossum@oracle.com>
On Mon, Feb 5, 2024, at 15:13, Vegard Nossum wrote:
> Link: https://lore.kernel.org/git/0adb1068-ef10-44ed-ad1d-e0927a09245d@gmail.com/
> Suggested-by: Phillip Wood <phillip.wood123@gmail.com>
> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
`Link` is not really used a lot. Junio’s `refs/notes/amlog` will point
back to the patch (which is often close to the “suggested by” and so
on).
--
Kristoffer Haugsbakk
^ permalink raw reply
* Re: [PATCH v3 2/2] add-patch: classify '@' as a synonym for 'HEAD'
From: Ghanshyam Thakkar @ 2024-02-05 15:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, ps
In-Reply-To: <xmqq34u9qiu5.fsf@gitster.g>
On Sun Feb 4, 2024 at 4:03 AM IST, Junio C Hamano wrote:
> Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:
>
> > Therefore, make a new function user_meant_head() which takes the
> > revision string and compares it to 'HEAD' as well as '@'. However, in
> > builtin/checkout.c, there is a logic to convert all command line input
> > rev to the raw object name for underlying machinery (e.g., diff-index)
> > that does not recognize the <a>...<b> notation, but we'd need to leave
> > 'HEAD' intact. Now we need to teach that '@' is a synonym to 'HEAD'
> > to that code and leave '@' intact, too.
>
> I am not sure what that "However" wants to say.
>
> - Now we have a helper function to see what the end-user said, and
> tell if the end-user meant the state that is currently checked
> out (aka "HEAD" but some folks like a synonym "@"[*]), or if the
> end-user meant some other "concrete" branch.
>
> - In builtin/checkout.c, there is a logic to convert unless what
> the end-user meant is the state that is currently checked out.
>
> Isn't the natural conclusion that follows these two stentences
> "therefore, the latter is a very good place to use that helper
> function, too"?
Yeah, I did not use the helper function in builtin/checkout.c. Hence the
"However". But I agree on the point of exporting the function.
Therefore I have attached the patch with the updated message below.
> Side note: the "@" is already problematic not just because
> "git branch @" would not refuse to create "refs/heads/@",
> but there is no ref "@" (like $GIT_DIR/refs/@ or $GIT_DIR/@)
> when it is used as a synonym for "HEAD". There is a check
> in builtin/checkout.c:update_refs_for_switch() that runs
> strcmp() on a token given by the end-user from the command
> line with "HEAD" to notice the no-op case "git checkout
> HEAD" but the code does not trigger when "@" is given, and
> it happens to work by accident. I really wish we didn't add
> that oddball synonym, but that is water under the bridge by
> now.
well, I suppose it is maybe annoying from the development perspective,
but users seem to like the concept of it[1].
> In any case, I think we'd find more places that currently treats the
> token "HEAD" given directly by the end-user specially and may want
> to teach at least some of them to also accept "@" the same way, and
> the helper function you are introducing may become useful in the
> future, at which time we may move it to a more public header. If it
> needs to be shared already between add-patch.c and builtin/checkout.c
> (I am guessing what you meant with "However" as an excuse for open
> coding it instead of sharing the code), perhaps we should do so without
> waiting for that future, though. I dunno.
Yeah, that "However" was for not using the helper function.
> If we choose to do so, for now, a squashable patch may look like the
> attached, but we'd need to update the log message while squashing it
> in.
Thanks for the patch. Updated message is below.
[Footnote]
[1]: https://www.reddit.com/r/git/comments/k15cqm/do_you_know_is_a_shortcut_for_head/
-- >8 --
Subject: [PATCH] add-patch: classify '@' as a synonym for 'HEAD'
Currently, (checkout, reset, restore) commands correctly take '@' as a
synonym for 'HEAD'. However, in patch mode (-p/--patch), for both '@' and
'HEAD', different prompts/messages are given by the commands mentioned
above (because of applying reverse mode(-R) in case of '@'). This is due to
the literal and only string comparison with the word 'HEAD' in run_add_p().
Synonymity between '@' and 'HEAD' is obviously desired, especially since
'@' already resolves to 'HEAD'.
Therefore, make a new function the_user_meant_head() which takes the
revision string and compares it to 'HEAD' as well as '@'. Also in
builtin/checkout.c, there is a logic to convert all command line input
rev to the raw object name for underlying machinery (e.g., diff-index)
that does not recognize the <a>...<b> notation, but we'd need to leave
'HEAD' intact. Now we need to teach that '@' is a synonym to 'HEAD'
to that code and leave '@' intact, too. Therefore, this function is
already useful in add-patch.c and builtin/checkout.c and may also
become helpful in other places in future. Therefore, it makes sense to
export it.
There is one unintended side-effect/behavior change of this, even if
there exists a branch named '@', when providing '@' as a rev-source to
(checkout, reset, restore) commands in patch mode, it will consider it
as HEAD. This is due to the behavior of diff-index. However, naming a
branch '@' is an obvious foot-gun and there are many existing commands
which already take '@' for 'HEAD' regardless of whether 'refs/heads/@'
exists or not (e.g., 'git log @', 'git push origin @' etc.). Therefore,
this should be fine.
Also, add tests for checking the above mentioned synonymity.
Co-authored-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
---
add-interactive.h | 14 ++++++++++++
add-patch.c | 6 ++---
builtin/checkout.c | 10 ++++-----
t/t2016-checkout-patch.sh | 46 ++++++++++++++++++++++-----------------
t/t2071-restore-patch.sh | 18 +++++++++------
t/t7105-reset-patch.sh | 10 +++++++++
6 files changed, 69 insertions(+), 35 deletions(-)
diff --git a/add-interactive.h b/add-interactive.h
index 693f125e8e..ca7326336d 100644
--- a/add-interactive.h
+++ b/add-interactive.h
@@ -38,4 +38,18 @@ enum add_p_mode {
int run_add_p(struct repository *r, enum add_p_mode mode,
const char *revision, const struct pathspec *ps);
+/*
+ * When the user gives these tokens from the command line, they mean
+ * the state that the currently checked out state came from. This
+ * single bit of information affects the direction in which the patch
+ * is presented to the end-user: are we showing a patch to go back to
+ * the currently committed state, or are we showing a patch to move
+ * forward to the given commit that may be different from the
+ * committed state we started with?
+ */
+static inline int the_user_meant_head(const char *rev)
+{
+ return !strcmp(rev, "HEAD") || !strcmp(rev, "@");
+}
+
#endif
diff --git a/add-patch.c b/add-patch.c
index 68f525b35c..5502acebb8 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1729,21 +1729,21 @@ int run_add_p(struct repository *r, enum add_p_mode mode,
if (mode == ADD_P_STASH)
s.mode = &patch_mode_stash;
else if (mode == ADD_P_RESET) {
- if (!revision || !strcmp(revision, "HEAD"))
+ if (!revision || the_user_meant_head(revision))
s.mode = &patch_mode_reset_head;
else
s.mode = &patch_mode_reset_nothead;
} else if (mode == ADD_P_CHECKOUT) {
if (!revision)
s.mode = &patch_mode_checkout_index;
- else if (!strcmp(revision, "HEAD"))
+ else if (the_user_meant_head(revision))
s.mode = &patch_mode_checkout_head;
else
s.mode = &patch_mode_checkout_nothead;
} else if (mode == ADD_P_WORKTREE) {
if (!revision)
s.mode = &patch_mode_checkout_index;
- else if (!strcmp(revision, "HEAD"))
+ else if (the_user_meant_head(revision))
s.mode = &patch_mode_worktree_head;
else
s.mode = &patch_mode_worktree_nothead;
diff --git a/builtin/checkout.c b/builtin/checkout.c
index a6e30931b5..63c669b157 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -539,12 +539,12 @@ static int checkout_paths(const struct checkout_opts *opts,
* recognized by diff-index), we will always replace the name
* with the hex of the commit (whether it's in `...` form or
* not) for the run_add_interactive() machinery to work
- * properly. However, there is special logic for the HEAD case
- * so we mustn't replace that. Also, when we were given a
- * tree-object, new_branch_info->commit would be NULL, but we
- * do not have to do any replacement, either.
+ * properly. However, there is special logic for the 'HEAD' and
+ * '@' case so we mustn't replace that. Also, when we were
+ * given a tree-object, new_branch_info->commit would be NULL,
+ * but we do not have to do any replacement, either.
*/
- if (rev && new_branch_info->commit && strcmp(rev, "HEAD"))
+ if (rev && new_branch_info->commit && !the_user_meant_head(rev))
rev = oid_to_hex_r(rev_oid, &new_branch_info->commit->object.oid);
if (opts->checkout_index && opts->checkout_worktree)
diff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh
index 747eb5563e..c4f9bf09aa 100755
--- a/t/t2016-checkout-patch.sh
+++ b/t/t2016-checkout-patch.sh
@@ -38,26 +38,32 @@ test_expect_success 'git checkout -p with staged changes' '
verify_state dir/foo index index
'
-test_expect_success 'git checkout -p HEAD with NO staged changes: abort' '
- set_and_save_state dir/foo work head &&
- test_write_lines n y n | git checkout -p HEAD &&
- verify_saved_state bar &&
- verify_saved_state dir/foo
-'
-
-test_expect_success 'git checkout -p HEAD with NO staged changes: apply' '
- test_write_lines n y y | git checkout -p HEAD &&
- verify_saved_state bar &&
- verify_state dir/foo head head
-'
-
-test_expect_success 'git checkout -p HEAD with change already staged' '
- set_state dir/foo index index &&
- # the third n is to get out in case it mistakenly does not apply
- test_write_lines n y n | git checkout -p HEAD &&
- verify_saved_state bar &&
- verify_state dir/foo head head
-'
+for opt in "HEAD" "@"
+do
+ test_expect_success "git checkout -p $opt with NO staged changes: abort" '
+ set_and_save_state dir/foo work head &&
+ test_write_lines n y n | git checkout -p $opt >output &&
+ verify_saved_state bar &&
+ verify_saved_state dir/foo &&
+ test_grep "Discard" output
+ '
+
+ test_expect_success "git checkout -p $opt with NO staged changes: apply" '
+ test_write_lines n y y | git checkout -p $opt >output &&
+ verify_saved_state bar &&
+ verify_state dir/foo head head &&
+ test_grep "Discard" output
+ '
+
+ test_expect_success "git checkout -p $opt with change already staged" '
+ set_state dir/foo index index &&
+ # the third n is to get out in case it mistakenly does not apply
+ test_write_lines n y n | git checkout -p $opt >output &&
+ verify_saved_state bar &&
+ verify_state dir/foo head head &&
+ test_grep "Discard" output
+ '
+done
test_expect_success 'git checkout -p HEAD^...' '
# the third n is to get out in case it mistakenly does not apply
diff --git a/t/t2071-restore-patch.sh b/t/t2071-restore-patch.sh
index b5c5c0ff7e..3dc9184b4a 100755
--- a/t/t2071-restore-patch.sh
+++ b/t/t2071-restore-patch.sh
@@ -44,13 +44,17 @@ test_expect_success PERL 'git restore -p with staged changes' '
verify_state dir/foo index index
'
-test_expect_success PERL 'git restore -p --source=HEAD' '
- set_state dir/foo work index &&
- # the third n is to get out in case it mistakenly does not apply
- test_write_lines n y n | git restore -p --source=HEAD &&
- verify_saved_state bar &&
- verify_state dir/foo head index
-'
+for opt in "HEAD" "@"
+do
+ test_expect_success PERL "git restore -p --source=$opt" '
+ set_state dir/foo work index &&
+ # the third n is to get out in case it mistakenly does not apply
+ test_write_lines n y n | git restore -p --source=$opt >output &&
+ verify_saved_state bar &&
+ verify_state dir/foo head index &&
+ test_grep "Discard" output
+ '
+done
test_expect_success PERL 'git restore -p --source=HEAD^' '
set_state dir/foo work index &&
diff --git a/t/t7105-reset-patch.sh b/t/t7105-reset-patch.sh
index 05079c7246..ec7f16dfb6 100755
--- a/t/t7105-reset-patch.sh
+++ b/t/t7105-reset-patch.sh
@@ -33,6 +33,16 @@ test_expect_success PERL 'git reset -p' '
test_grep "Unstage" output
'
+for opt in "HEAD" "@"
+do
+ test_expect_success PERL "git reset -p $opt" '
+ test_write_lines n y | git reset -p $opt >output &&
+ verify_state dir/foo work head &&
+ verify_saved_state bar &&
+ test_grep "Unstage" output
+ '
+done
+
test_expect_success PERL 'git reset -p HEAD^' '
test_write_lines n y | git reset -p HEAD^ >output &&
verify_state dir/foo work parent &&
--
2.43.0
^ permalink raw reply related
* Re: [PATCH v2 2/9] reftable: introduce macros to allocate arrays
From: Karthik Nayak @ 2024-02-05 15:48 UTC (permalink / raw)
To: Patrick Steinhardt, git; +Cc: Eric Sunshine, Junio C Hamano
In-Reply-To: <2dde581a0256e4634ca4a64f313a98204763906a.1706772591.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 808 bytes --]
Patrick Steinhardt <ps@pks.im> writes:
> Similar to the preceding commit, let's carry over macros to allocate
> arrays with `REFTABLE_ALLOC_ARRAY()` and `REFTABLE_CALLOC_ARRAY()`. This
> requires us to change the signature of `reftable_calloc()`, which only
> takes a single argument right now and thus puts the burden on the caller
> to calculate the final array's size. This is a net improvement though as
> it means that we can now provide proper overflow checks when multiplying
> the array size with the member size.
>
> Convert callsites of `reftable_calloc()` to the new signature, using the
> new macros where possible.
What about converting users of `reftable_malloc()` to use
`REFTABLE_ALLOC_ARRAY()`. This means currently `REFTABLE_ALLOC_ARRAY()`
is defined and never used in this patch series.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply
* Re: [PATCH] Always check the return value of `repo_read_object_file()`
From: Karthik Nayak @ 2024-02-05 16:10 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget, git; +Cc: Johannes Schindelin
In-Reply-To: <pull.1650.git.1707143753726.gitgitgadget@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 740 bytes --]
Hello,
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:
> diff --git a/bisect.c b/bisect.c
> index f1273c787d9..f75e50c3397 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -158,6 +158,9 @@ static void show_list(const char *debug, int counted, int nr,
> const char *subject_start;
> int subject_len;
>
> + if (!buf)
> + die(_("unable to read %s"), oid_to_hex(&commit->object.oid));
> +
Nit: We know that `repo_read_object_file()` fails on corrupt objects, so
this means that this is only happening when the object doesn't exist. I
wonder if it makes more sense to replace "unable to read %s" which is a
little ambiguous with something like "object %q doesn't exist".
Otherwise, the patch looks good, thanks!
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply
* Re: [RFC PATCH v2 4/6] test-tool run-command testsuite: support unit tests
From: phillip.wood123 @ 2024-02-05 16:16 UTC (permalink / raw)
To: Josh Steadmon, git; +Cc: johannes.schindelin, peff, phillip.wood, gitster
In-Reply-To: <b5665386b56df91fa5d95ee5b11288b5853546f0.1706921262.git.steadmon@google.com>
Hi Josh
On 03/02/2024 00:50, Josh Steadmon wrote:
> Teach the testsuite runner in `test-tool run-command testsuite` how to
> run unit tests: if TEST_SHELL_PATH is not set, assume that we're running
> the programs directly from CWD, rather than defaulting to "sh" as an
> interpreter.
Judging from the last patch in this series it seems likely that we'll
want to run unit tests and integration tests parallel. In which case it
might be better to look at the filename extension to decide whether to
sh as an interpreter so that we can avoid having to use a wrapper
script. Then
cd t
helper/test-tool run-command testsuite 't[0-9]*.sh' 'unit-tests/bin/*'
would run the integration tests via "sh" and the unit-tests directly.
We'd need to figure out how to look for tests in both directories as
well though...
Best Wishes
Phillip
> With this change, you can now use test-tool to run the unit tests:
> $ make
> $ cd t/unit-tests/bin
> $ ../../helper/test-tool run-command testsuite
>
> This should be helpful on Windows to allow running tests without
> requiring Perl (for `prove`), as discussed in [1] and [2].
>
> This again breaks backwards compatibility, as it is now required to set
> TEST_SHELL_PATH properly for executing shell scripts, but again, as
> noted in [2], there are no longer any such invocations in our codebase.
>
> [1] https://lore.kernel.org/git/nycvar.QRO.7.76.6.2109091323150.59@tvgsbejvaqbjf.bet/
> [2] https://lore.kernel.org/git/850ea42c-f103-68d5-896b-9120e2628686@gmx.de/
>
> Signed-off-by: Josh Steadmon <steadmon@google.com>
> ---
> t/helper/test-run-command.c | 18 +++++++++++++++---
> 1 file changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c
> index e6bd792274..a0b8dc6fd7 100644
> --- a/t/helper/test-run-command.c
> +++ b/t/helper/test-run-command.c
> @@ -158,6 +158,8 @@ static int testsuite(int argc, const char **argv)
> .task_finished = test_finished,
> .data = &suite,
> };
> + struct strbuf progpath = STRBUF_INIT;
> + size_t path_prefix_len;
>
> argc = parse_options(argc, argv, NULL, options,
> testsuite_usage, PARSE_OPT_STOP_AT_NON_OPTION);
> @@ -165,9 +167,14 @@ static int testsuite(int argc, const char **argv)
> if (max_jobs <= 0)
> max_jobs = online_cpus();
>
> + /*
> + * If we run without a shell, we have to provide the relative path to
> + * the executables.
> + */
> suite.shell_path = getenv("TEST_SHELL_PATH");
> if (!suite.shell_path)
> - suite.shell_path = "sh";
> + strbuf_addstr(&progpath, "./");
> + path_prefix_len = progpath.len;
>
> dir = opendir(".");
> if (!dir)
> @@ -180,13 +187,17 @@ static int testsuite(int argc, const char **argv)
>
> /* No pattern: match all */
> if (!argc) {
> - string_list_append(&suite.tests, p);
> + strbuf_setlen(&progpath, path_prefix_len);
> + strbuf_addstr(&progpath, p);
> + string_list_append(&suite.tests, progpath.buf);
> continue;
> }
>
> for (i = 0; i < argc; i++)
> if (!wildmatch(argv[i], p, 0)) {
> - string_list_append(&suite.tests, p);
> + strbuf_setlen(&progpath, path_prefix_len);
> + strbuf_addstr(&progpath, p);
> + string_list_append(&suite.tests, progpath.buf);
> break;
> }
> }
> @@ -213,6 +224,7 @@ static int testsuite(int argc, const char **argv)
>
> string_list_clear(&suite.tests, 0);
> string_list_clear(&suite.failed, 0);
> + strbuf_release(&progpath);
>
> return ret;
> }
^ permalink raw reply
* [Outreachy][PATCH 1/2] date: refactor 64 bit prereq code into reusable functions
From: Achu Luma @ 2024-02-05 16:25 UTC (permalink / raw)
To: git; +Cc: christian.couder, Achu Luma, Christian Couder
In a following commit we are going to port code from
"t/helper/test-date.c" and "t/t0006-date.sh" to a new
"t/unit-tests/t-date.c" file using the recently added unit test
framework.
We cannot fully port all the code from "t/helper/test-date.c" though, as
the test-tool date helper is still used by a number of "t/*.sh" tests.
The TIME_IS_64BIT and TIME_T_IS_64BIT prereqs are especially used by
"t5000-tar-tree.sh", "t5318-commit-graph.sh" and
"t5328-commit-graph-64bit-time.sh" while checking those prereqs will be
required in the new "t/unit-tests/t-date.c" file too.
To avoid duplicating in both "t/helper/test-date.c" and
"t/unit-tests/t-date.c" the small amount of code checking these prereqs,
let's move it into inline functions in "date.h".
The names of these new inline functions contain "TIME_IS_64BIT" or
"TIME_T_IS_64BIT" as it will simplify the macros we will use when
we will port code to "t/unit-tests/t-date.c" in a following commit.
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Achu Luma <ach.lumap@gmail.com>
---
date.h | 6 ++++++
t/helper/test-date.c | 4 ++--
2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/date.h b/date.h
index 6136212a19..fb70490a51 100644
--- a/date.h
+++ b/date.h
@@ -70,4 +70,10 @@ void datestamp(struct strbuf *out);
timestamp_t approxidate_careful(const char *, int *);
int date_overflows(timestamp_t date);
time_t tm_to_time_t(const struct tm *tm);
+static inline int check_prereq_TIME_IS_64BIT(void) {
+ return sizeof(timestamp_t) == 8;
+}
+static inline int check_prereq_TIME_T_IS_64BIT(void) {
+ return sizeof(time_t) == 8;
+}
#endif
diff --git a/t/helper/test-date.c b/t/helper/test-date.c
index 0683d46574..be0b8679c3 100644
--- a/t/helper/test-date.c
+++ b/t/helper/test-date.c
@@ -126,9 +126,9 @@ int cmd__date(int argc UNUSED, const char **argv)
else if (!strcmp(*argv, "getnanos"))
getnanos(argv+1);
else if (!strcmp(*argv, "is64bit"))
- return sizeof(timestamp_t) == 8 ? 0 : 1;
+ return !check_prereq_TIME_IS_64BIT();
else if (!strcmp(*argv, "time_t-is64bit"))
- return sizeof(time_t) == 8 ? 0 : 1;
+ return !check_prereq_TIME_T_IS_64BIT();
else
usage(usage_msg);
return 0;
--
2.43.0.windows.1
^ permalink raw reply related
* [Outreachy][PATCH 2/2] Port helper/test-date.c to unit-tests/t-date.c
From: Achu Luma @ 2024-02-05 16:25 UTC (permalink / raw)
To: git; +Cc: christian.couder, Achu Luma, Christian Couder
In-Reply-To: <20240205162506.1835-1-ach.lumap@gmail.com>
In the recent codebase update (8bf6fbd (Merge branch
'js/doc-unit-tests', 2023-12-09)), a new unit testing framework was
merged, providing a standardized approach for testing C code. Prior to
this update, some unit tests relied on the test helper mechanism,
lacking a dedicated unit testing framework. It's more natural to perform
these unit tests using the new unit test framework.
This commit migrates the unit tests for C date functions
(show_date_relative(), show_date(), etc) from the legacy approach
using the test-tool command `test-tool date` in t/helper/test-date.c
to the new unit testing framework (t/unit-tests/test-lib.h).
The migration involves refactoring the tests to utilize the testing
macros provided by the framework (TEST() and check_*()).
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Achu Luma <ach.lumap@gmail.com>
---
I am currently facing challenges in handling the time zone (TZ)
variable, in t-date.c. The tests, which were initially passing
in the t0006-date.sh on Windows, fail after porting to the new
framework. I have tried to set the timezone using the setenv,
but unfortunately, the issue persists. I suspect that the problem
might be related to how Windows C compilers process the setenv
function compared to POSIX environments. Please If anyone has
insights into managing environment variables in a cross-platform
manner or has encountered similar issues, your help would be
highly appreciated.
t/helper/test-date.c | 91 +---------------
t/t0006-date.sh | 169 ------------------------------
t/unit-tests/t-date.c | 237 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 238 insertions(+), 259 deletions(-)
delete mode 100755 t/t0006-date.sh
create mode 100644 t/unit-tests/t-date.c
diff --git a/t/helper/test-date.c b/t/helper/test-date.c
index be0b8679c3..b9cb2c5455 100644
--- a/t/helper/test-date.c
+++ b/t/helper/test-date.c
@@ -3,89 +3,11 @@
#include "trace.h"
static const char *usage_msg = "\n"
-" test-tool date relative [time_t]...\n"
-" test-tool date human [time_t]...\n"
-" test-tool date show:<format> [time_t]...\n"
-" test-tool date parse [date]...\n"
-" test-tool date approxidate [date]...\n"
" test-tool date timestamp [date]...\n"
" test-tool date getnanos [start-nanos]\n"
" test-tool date is64bit\n"
" test-tool date time_t-is64bit\n";
-static void show_relative_dates(const char **argv)
-{
- struct strbuf buf = STRBUF_INIT;
-
- for (; *argv; argv++) {
- time_t t = atoi(*argv);
- show_date_relative(t, &buf);
- printf("%s -> %s\n", *argv, buf.buf);
- }
- strbuf_release(&buf);
-}
-
-static void show_human_dates(const char **argv)
-{
- for (; *argv; argv++) {
- time_t t = atoi(*argv);
- printf("%s -> %s\n", *argv, show_date(t, 0, DATE_MODE(HUMAN)));
- }
-}
-
-static void show_dates(const char **argv, const char *format)
-{
- struct date_mode mode = DATE_MODE_INIT;
-
- parse_date_format(format, &mode);
- for (; *argv; argv++) {
- char *arg;
- timestamp_t t;
- int tz;
-
- /*
- * Do not use our normal timestamp parsing here, as the point
- * is to test the formatting code in isolation.
- */
- t = parse_timestamp(*argv, &arg, 10);
- while (*arg == ' ')
- arg++;
- tz = atoi(arg);
-
- printf("%s -> %s\n", *argv, show_date(t, tz, &mode));
- }
-
- date_mode_release(&mode);
-}
-
-static void parse_dates(const char **argv)
-{
- struct strbuf result = STRBUF_INIT;
-
- for (; *argv; argv++) {
- timestamp_t t;
- int tz;
-
- strbuf_reset(&result);
- parse_date(*argv, &result);
- if (sscanf(result.buf, "%"PRItime" %d", &t, &tz) == 2)
- printf("%s -> %s\n",
- *argv, show_date(t, tz, DATE_MODE(ISO8601)));
- else
- printf("%s -> bad\n", *argv);
- }
- strbuf_release(&result);
-}
-
-static void parse_approxidate(const char **argv)
-{
- for (; *argv; argv++) {
- timestamp_t t;
- t = approxidate(*argv);
- printf("%s -> %s\n", *argv, show_date(t, 0, DATE_MODE(ISO8601)));
- }
-}
-
static void parse_approx_timestamp(const char **argv)
{
for (; *argv; argv++) {
@@ -106,22 +28,11 @@ static void getnanos(const char **argv)
int cmd__date(int argc UNUSED, const char **argv)
{
- const char *x;
argv++;
if (!*argv)
usage(usage_msg);
- if (!strcmp(*argv, "relative"))
- show_relative_dates(argv+1);
- else if (!strcmp(*argv, "human"))
- show_human_dates(argv+1);
- else if (skip_prefix(*argv, "show:", &x))
- show_dates(argv+1, x);
- else if (!strcmp(*argv, "parse"))
- parse_dates(argv+1);
- else if (!strcmp(*argv, "approxidate"))
- parse_approxidate(argv+1);
- else if (!strcmp(*argv, "timestamp"))
+ if (!strcmp(*argv, "timestamp"))
parse_approx_timestamp(argv+1);
else if (!strcmp(*argv, "getnanos"))
getnanos(argv+1);
diff --git a/t/t0006-date.sh b/t/t0006-date.sh
deleted file mode 100755
index e18b160286..0000000000
--- a/t/t0006-date.sh
+++ /dev/null
@@ -1,169 +0,0 @@
-#!/bin/sh
-
-test_description='test date parsing and printing'
-
-TEST_PASSES_SANITIZE_LEAK=true
-. ./test-lib.sh
-
-# arbitrary reference time: 2009-08-30 19:20:00
-GIT_TEST_DATE_NOW=1251660000; export GIT_TEST_DATE_NOW
-
-check_relative() {
- t=$(($GIT_TEST_DATE_NOW - $1))
- echo "$t -> $2" >expect
- test_expect_${3:-success} "relative date ($2)" "
- test-tool date relative $t >actual &&
- test_cmp expect actual
- "
-}
-
-check_relative 5 '5 seconds ago'
-check_relative 300 '5 minutes ago'
-check_relative 18000 '5 hours ago'
-check_relative 432000 '5 days ago'
-check_relative 1728000 '3 weeks ago'
-check_relative 13000000 '5 months ago'
-check_relative 37500000 '1 year, 2 months ago'
-check_relative 55188000 '1 year, 9 months ago'
-check_relative 630000000 '20 years ago'
-check_relative 31449600 '12 months ago'
-check_relative 62985600 '2 years ago'
-
-check_show () {
- format=$1
- time=$2
- expect=$3
- prereqs=$4
- zone=$5
- test_expect_success $prereqs "show date ($format:$time)" '
- echo "$time -> $expect" >expect &&
- TZ=${zone:-$TZ} test-tool date show:"$format" "$time" >actual &&
- test_cmp expect actual
- '
-}
-
-# arbitrary but sensible time for examples
-TIME='1466000000 +0200'
-check_show iso8601 "$TIME" '2016-06-15 16:13:20 +0200'
-check_show iso8601-strict "$TIME" '2016-06-15T16:13:20+02:00'
-check_show rfc2822 "$TIME" 'Wed, 15 Jun 2016 16:13:20 +0200'
-check_show short "$TIME" '2016-06-15'
-check_show default "$TIME" 'Wed Jun 15 16:13:20 2016 +0200'
-check_show raw "$TIME" '1466000000 +0200'
-check_show unix "$TIME" '1466000000'
-check_show iso-local "$TIME" '2016-06-15 14:13:20 +0000'
-check_show raw-local "$TIME" '1466000000 +0000'
-check_show unix-local "$TIME" '1466000000'
-
-check_show 'format:%z' "$TIME" '+0200'
-check_show 'format-local:%z' "$TIME" '+0000'
-check_show 'format:%Z' "$TIME" ''
-check_show 'format-local:%Z' "$TIME" 'UTC'
-check_show 'format:%%z' "$TIME" '%z'
-check_show 'format-local:%%z' "$TIME" '%z'
-
-check_show 'format:%Y-%m-%d %H:%M:%S' "$TIME" '2016-06-15 16:13:20'
-check_show 'format-local:%Y-%m-%d %H:%M:%S' "$TIME" '2016-06-15 09:13:20' '' EST5
-
-check_show 'format:%s' '123456789 +1234' 123456789
-check_show 'format:%s' '123456789 -1234' 123456789
-check_show 'format-local:%s' '123456789 -1234' 123456789
-
-# arbitrary time absurdly far in the future
-FUTURE="5758122296 -0400"
-check_show iso "$FUTURE" "2152-06-19 18:24:56 -0400" TIME_IS_64BIT,TIME_T_IS_64BIT
-check_show iso-local "$FUTURE" "2152-06-19 22:24:56 +0000" TIME_IS_64BIT,TIME_T_IS_64BIT
-
-check_parse() {
- echo "$1 -> $2" >expect
- test_expect_${4:-success} "parse date ($1${3:+ TZ=$3})" "
- TZ=${3:-$TZ} test-tool date parse '$1' >actual &&
- test_cmp expect actual
- "
-}
-
-check_parse 2008 bad
-check_parse 2008-02 bad
-check_parse 2008-02-14 bad
-check_parse '2008-02-14 20:30:45' '2008-02-14 20:30:45 +0000'
-check_parse '2008-02-14 20:30:45 -0500' '2008-02-14 20:30:45 -0500'
-check_parse '2008.02.14 20:30:45 -0500' '2008-02-14 20:30:45 -0500'
-check_parse '20080214T20:30:45' '2008-02-14 20:30:45 +0000'
-check_parse '20080214T20:30' '2008-02-14 20:30:00 +0000'
-check_parse '20080214T20' '2008-02-14 20:00:00 +0000'
-check_parse '20080214T203045' '2008-02-14 20:30:45 +0000'
-check_parse '20080214T2030' '2008-02-14 20:30:00 +0000'
-check_parse '20080214T000000.20' '2008-02-14 00:00:00 +0000'
-check_parse '20080214T00:00:00.20' '2008-02-14 00:00:00 +0000'
-check_parse '20080214T203045-04:00' '2008-02-14 20:30:45 -0400'
-check_parse '20080214T203045 -04:00' '2008-02-14 20:30:45 -0400'
-check_parse '20080214T203045.019-04:00' '2008-02-14 20:30:45 -0400'
-check_parse '2008-02-14 20:30:45.019-04:00' '2008-02-14 20:30:45 -0400'
-check_parse '2008-02-14 20:30:45 -0015' '2008-02-14 20:30:45 -0015'
-check_parse '2008-02-14 20:30:45 -5' '2008-02-14 20:30:45 +0000'
-check_parse '2008-02-14 20:30:45 -5:' '2008-02-14 20:30:45 +0000'
-check_parse '2008-02-14 20:30:45 -05' '2008-02-14 20:30:45 -0500'
-check_parse '2008-02-14 20:30:45 -:30' '2008-02-14 20:30:45 +0000'
-check_parse '2008-02-14 20:30:45 -05:00' '2008-02-14 20:30:45 -0500'
-check_parse '2008-02-14 20:30:45' '2008-02-14 20:30:45 -0500' EST5
-check_parse 'Thu, 7 Apr 2005 15:14:13 -0700' '2005-04-07 15:14:13 -0700'
-
-check_approxidate() {
- echo "$1 -> $2 +0000" >expect
- test_expect_${3:-success} "parse approxidate ($1)" "
- test-tool date approxidate '$1' >actual &&
- test_cmp expect actual
- "
-}
-
-check_approxidate now '2009-08-30 19:20:00'
-check_approxidate '5 seconds ago' '2009-08-30 19:19:55'
-check_approxidate 5.seconds.ago '2009-08-30 19:19:55'
-check_approxidate 10.minutes.ago '2009-08-30 19:10:00'
-check_approxidate yesterday '2009-08-29 19:20:00'
-check_approxidate 3.days.ago '2009-08-27 19:20:00'
-check_approxidate '12:34:56.3.days.ago' '2009-08-27 12:34:56'
-check_approxidate 3.weeks.ago '2009-08-09 19:20:00'
-check_approxidate 3.months.ago '2009-05-30 19:20:00'
-check_approxidate 2.years.3.months.ago '2007-05-30 19:20:00'
-
-check_approxidate '6am yesterday' '2009-08-29 06:00:00'
-check_approxidate '6pm yesterday' '2009-08-29 18:00:00'
-check_approxidate '3:00' '2009-08-30 03:00:00'
-check_approxidate '15:00' '2009-08-30 15:00:00'
-check_approxidate 'noon today' '2009-08-30 12:00:00'
-check_approxidate 'noon yesterday' '2009-08-29 12:00:00'
-check_approxidate 'January 5th noon pm' '2009-01-05 12:00:00'
-check_approxidate '10am noon' '2009-08-29 12:00:00'
-
-check_approxidate 'last tuesday' '2009-08-25 19:20:00'
-check_approxidate 'July 5th' '2009-07-05 19:20:00'
-check_approxidate '06/05/2009' '2009-06-05 19:20:00'
-check_approxidate '06.05.2009' '2009-05-06 19:20:00'
-
-check_approxidate 'Jun 6, 5AM' '2009-06-06 05:00:00'
-check_approxidate '5AM Jun 6' '2009-06-06 05:00:00'
-check_approxidate '6AM, June 7, 2009' '2009-06-07 06:00:00'
-
-check_approxidate '2008-12-01' '2008-12-01 19:20:00'
-check_approxidate '2009-12-01' '2009-12-01 19:20:00'
-
-check_date_format_human() {
- t=$(($GIT_TEST_DATE_NOW - $1))
- echo "$t -> $2" >expect
- test_expect_success "human date $t" '
- test-tool date human $t >actual &&
- test_cmp expect actual
-'
-}
-
-check_date_format_human 18000 "5 hours ago" # 5 hours ago
-check_date_format_human 432000 "Tue Aug 25 19:20" # 5 days ago
-check_date_format_human 1728000 "Mon Aug 10 19:20" # 3 weeks ago
-check_date_format_human 13000000 "Thu Apr 2 08:13" # 5 months ago
-check_date_format_human 31449600 "Aug 31 2008" # 12 months ago
-check_date_format_human 37500000 "Jun 22 2008" # 1 year, 2 months ago
-check_date_format_human 55188000 "Dec 1 2007" # 1 year, 9 months ago
-check_date_format_human 630000000 "Sep 13 1989" # 20 years ago
-
-test_done
diff --git a/t/unit-tests/t-date.c b/t/unit-tests/t-date.c
new file mode 100644
index 0000000000..dd5dbbb2e0
--- /dev/null
+++ b/t/unit-tests/t-date.c
@@ -0,0 +1,237 @@
+#include "test-lib.h"
+#include "date.h"
+#include "strbuf.h"
+
+/* Reference time: 2009-08-30 19:20:00. */
+#define GIT_TEST_DATE_NOW 1251660000
+
+/* The time corresponds to Wed, 15 Jun 2016 16:13:20 +0200. */
+static const char test_time[] = "1466000000 +0200";
+
+enum prerequisites {
+ TIME_IS_64BIT = 1 << 0,
+ TIME_T_IS_64BIT = 1 << 1,
+};
+
+/* Macro to check prerequisites */
+#define CHECK_PREREQ(var, prereq) \
+ do { \
+ if ((var) & prereq && !check_prereq_##prereq()) { \
+ test_skip("missing prerequisite " #prereq); \
+ return 0; \
+ } \
+ } while (0)
+
+/* Return 1 if all prereqs are satisfied, 0 otherwise */
+static int check_prereqs(unsigned int prereqs) {
+ CHECK_PREREQ(prereqs, TIME_IS_64BIT);
+ CHECK_PREREQ(prereqs, TIME_T_IS_64BIT);
+
+ return 1;
+}
+
+static void set_TZ_env(const char *zone) {
+ setenv("TZ", zone, 1);
+ tzset();
+}
+
+static void check_relative_dates(int time_val, const char *expected_date) {
+ struct strbuf buf = STRBUF_INIT;
+ timestamp_t diff = GIT_TEST_DATE_NOW - time_val;
+
+ show_date_relative(diff, &buf);
+ check_str(buf.buf, expected_date);
+ strbuf_release(&buf);
+}
+
+#define TEST_RELATIVE_DATE(value, expected_output) \
+ TEST(check_relative_dates(value, expected_output), \
+ "relative date (%s) works", #expected_output )
+
+static void check_show_date(const char *format, const char *TIME, const char *expected, unsigned int prereqs, const char *zone) {
+ struct date_mode mode = DATE_MODE_INIT;
+ char *arg;
+ timestamp_t t;
+ int tz;
+
+ if (!check_prereqs(prereqs))
+ return;
+ if (strcmp(zone, ""))
+ set_TZ_env(zone);
+
+ parse_date_format(format, &mode);
+ t = parse_timestamp(TIME, &arg, 10);
+ tz = atoi(arg);
+
+ check_str(show_date(t, tz, &mode), expected);
+
+ if (strcmp(zone, ""))
+ set_TZ_env("UTC");
+ date_mode_release(&mode);
+}
+
+#define TEST_SHOW_DATE(format, time, expected, prereqs, zone) \
+ TEST(check_show_date(format, time, expected, prereqs, zone), \
+ "show date (%s) works", #format)
+
+static void check_parse_date(const char *given, const char *expected, const char *zone) {
+ struct strbuf result = STRBUF_INIT;
+ timestamp_t t;
+ int tz;
+
+ if (strcmp(zone, ""))
+ set_TZ_env(zone);
+
+ parse_date(given, &result);
+ if (sscanf(result.buf, "%"PRItime" %d", &t, &tz) == 2)
+ check_str(show_date(t, tz, DATE_MODE(ISO8601)), expected);
+ else
+ check_str("bad", expected);
+
+ if (strcmp(zone, ""))
+ set_TZ_env("UTC");
+ strbuf_release(&result);
+}
+
+#define TEST_PARSE_DATE(given, expected_output, zone) \
+ TEST(check_parse_date(given, expected_output, zone), \
+ "parse date (%s) works", #expected_output)
+
+static void check_approxidate(const char *given, const char *expected) {
+ timestamp_t t = approxidate(given);
+ char *expected_with_offset = xstrfmt("%s +0000", expected);
+
+ check_str(show_date(t, 0, DATE_MODE(ISO8601)), expected_with_offset);
+ free(expected_with_offset);
+}
+
+#define TEST_APPROXIDATE(given, expected_output) \
+ TEST(check_approxidate(given, expected_output), \
+ "parse approxidate (%s) works", #given)
+
+static void check_date_format_human(int given, const char *expected) {
+ timestamp_t diff = GIT_TEST_DATE_NOW - given;
+ check_str(show_date(diff, 0, DATE_MODE(HUMAN)), expected);
+}
+
+#define TEST_DATE_FORMAT_HUMAN(given, expected_output) \
+ TEST(check_date_format_human(given, expected_output), \
+ "human date (%s) works", #given)
+
+int cmd_main(int argc, const char **argv) {
+ set_TZ_env("UTC");
+ setenv("GIT_TEST_DATE_NOW", "1251660000", 1);
+ setenv("LANG", "C", 1);
+
+ TEST_RELATIVE_DATE(5, "5 seconds ago");
+ TEST_RELATIVE_DATE(300, "5 minutes ago");
+ TEST_RELATIVE_DATE(18000, "5 hours ago");
+ TEST_RELATIVE_DATE(432000, "5 days ago");
+ TEST_RELATIVE_DATE(1728000, "3 weeks ago");
+ TEST_RELATIVE_DATE(13000000, "5 months ago");
+ TEST_RELATIVE_DATE(37500000, "1 year, 2 months ago");
+ TEST_RELATIVE_DATE(55188000, "1 year, 9 months ago");
+ TEST_RELATIVE_DATE(630000000, "20 years ago");
+ TEST_RELATIVE_DATE(31449600, "12 months ago");
+ TEST_RELATIVE_DATE(62985600, "2 years ago");
+
+ TEST_SHOW_DATE("iso8601", test_time, "2016-06-15 16:13:20 +0200", 0, "");
+ TEST_SHOW_DATE("iso8601-strict", test_time, "2016-06-15T16:13:20+02:00", 0, "");
+ TEST_SHOW_DATE("rfc2822", test_time, "Wed, 15 Jun 2016 16:13:20 +0200", 0, "");
+ TEST_SHOW_DATE("short", test_time, "2016-06-15", 0, "");
+ TEST_SHOW_DATE("default", test_time, "Wed Jun 15 16:13:20 2016 +0200", 0, "");
+ TEST_SHOW_DATE("raw", test_time, test_time, 0, "");
+ TEST_SHOW_DATE("unix", test_time, "1466000000", 0, "");
+ TEST_SHOW_DATE("iso-local", test_time, "2016-06-15 14:13:20 +0000", 0, "");
+ TEST_SHOW_DATE("raw-local", test_time, "1466000000 +0000", 0, "");
+ TEST_SHOW_DATE("unix-local", test_time, "1466000000", 0, "");
+
+ TEST_SHOW_DATE("format:%z", test_time, "+0200", 0, "");
+ TEST_SHOW_DATE("format-local:%z", test_time, "+0000", 0, "");
+ TEST_SHOW_DATE("format:%Z", test_time, "", 0, "");
+ TEST_SHOW_DATE("format-local:%Z", test_time, "UTC", 0, "");
+ TEST_SHOW_DATE("format:%%z", test_time, "%z", 0, "");
+ TEST_SHOW_DATE("format-local:%%z", test_time, "%z", 0, "");
+
+ TEST_SHOW_DATE("format:%Y-%m-%d %H:%M:%S", test_time, "2016-06-15 16:13:20", 0, "");
+
+ TEST_SHOW_DATE("format-local:%Y-%m-%d %H:%M:%S", test_time, "2016-06-15 09:13:20", 0, "EST5");
+
+ TEST_SHOW_DATE("format:%s", "123456789 +1234", "123456789", 0, "");
+ TEST_SHOW_DATE("format:%s", "123456789 -1234", "123456789", 0, "");
+ TEST_SHOW_DATE("format-local:%s", "123456789 -1234", "123456789", 0, "");
+
+ /* Arbitrary time absurdly far in the future */
+ TEST_SHOW_DATE("iso", "5758122296 -0400", "2152-06-19 18:24:56 -0400", TIME_IS_64BIT | TIME_T_IS_64BIT, "");
+ TEST_SHOW_DATE("iso-local", "5758122296 -0400", "2152-06-19 22:24:56 +0000", TIME_IS_64BIT | TIME_T_IS_64BIT, "");
+
+ TEST_PARSE_DATE("2000", "bad", "");
+ TEST_PARSE_DATE("2008-02", "bad", "");
+ TEST_PARSE_DATE("2008-02-14", "bad", "");
+ TEST_PARSE_DATE("2008-02-14 20:30:45", "2008-02-14 20:30:45 +0000", "");
+ TEST_PARSE_DATE("2008-02-14 20:30:45 -0500", "2008-02-14 20:30:45 -0500", "");
+ TEST_PARSE_DATE("2008.02.14 20:30:45 -0500", "2008-02-14 20:30:45 -0500", "");
+ TEST_PARSE_DATE("20080214T20:30:45", "2008-02-14 20:30:45 +0000", "");
+ TEST_PARSE_DATE("20080214T20:30", "2008-02-14 20:30:00 +0000", "");
+ TEST_PARSE_DATE("20080214T20", "2008-02-14 20:00:00 +0000", "");
+ TEST_PARSE_DATE("20080214T203045", "2008-02-14 20:30:45 +0000", "");
+ TEST_PARSE_DATE("20080214T2030", "2008-02-14 20:30:00 +0000", "");
+ TEST_PARSE_DATE("20080214T000000.20", "2008-02-14 00:00:00 +0000", "");
+ TEST_PARSE_DATE("20080214T00:00:00.20", "2008-02-14 00:00:00 +0000", "");
+ TEST_PARSE_DATE("20080214T203045-04:00", "2008-02-14 20:30:45 -0400", "");
+
+ TEST_PARSE_DATE("20080214T203045 -04:00", "2008-02-14 20:30:45 -0400", "");
+ TEST_PARSE_DATE("20080214T203045.019-04:00", "2008-02-14 20:30:45 -0400", "");
+ TEST_PARSE_DATE("2008-02-14 20:30:45.019-04:00", "2008-02-14 20:30:45 -0400", "");
+ TEST_PARSE_DATE("2008-02-14 20:30:45 -0015", "2008-02-14 20:30:45 -0015", "");
+ TEST_PARSE_DATE("2008-02-14 20:30:45 -5", "2008-02-14 20:30:45 +0000", "");
+ TEST_PARSE_DATE("2008-02-14 20:30:45 -5:", "2008-02-14 20:30:45 +0000", "");
+ TEST_PARSE_DATE("2008-02-14 20:30:45 -05", "2008-02-14 20:30:45 -0500", "");
+ TEST_PARSE_DATE("2008-02-14 20:30:45 -:30", "2008-02-14 20:30:45 +0000", "");
+ TEST_PARSE_DATE("2008-02-14 20:30:45 -05:00", "2008-02-14 20:30:45 -0500", "");
+ TEST_PARSE_DATE("2008-02-14 20:30:45", "2008-02-14 20:30:45 -0500", "EST5");
+
+ TEST_PARSE_DATE("Thu, 7 Apr 2005 15:14:13 -0700", "2005-04-07 15:14:13 -0700", "");
+
+ TEST_APPROXIDATE("now", "2009-08-30 19:20:00");
+ TEST_APPROXIDATE("5 seconds ago", "2009-08-30 19:19:55");
+ TEST_APPROXIDATE("10 minutes ago", "2009-08-30 19:10:00");
+ TEST_APPROXIDATE("yesterday", "2009-08-29 19:20:00");
+ TEST_APPROXIDATE("3 days ago", "2009-08-27 19:20:00");
+ TEST_APPROXIDATE("12:34:56.3 days ago", "2009-08-27 12:34:56");
+ TEST_APPROXIDATE("3 weeks ago", "2009-08-09 19:20:00");
+ TEST_APPROXIDATE("3 months ago", "2009-05-30 19:20:00");
+ TEST_APPROXIDATE("2 years 3 months ago", "2007-05-30 19:20:00");
+
+ TEST_APPROXIDATE("6am yesterday", "2009-08-29 06:00:00");
+ TEST_APPROXIDATE("6pm yesterday", "2009-08-29 18:00:00");
+ TEST_APPROXIDATE("3:00", "2009-08-30 03:00:00");
+ TEST_APPROXIDATE("15:00", "2009-08-30 15:00:00");
+ TEST_APPROXIDATE("noon today", "2009-08-30 12:00:00");
+ TEST_APPROXIDATE("noon yesterday", "2009-08-29 12:00:00");
+ TEST_APPROXIDATE("January 5th noon pm", "2009-01-05 12:00:00");
+ TEST_APPROXIDATE("10am noon", "2009-08-29 12:00:00");
+
+ TEST_APPROXIDATE("last tuesday", "2009-08-25 19:20:00");
+ TEST_APPROXIDATE("July 5th", "2009-07-05 19:20:00");
+ TEST_APPROXIDATE("06/05/2009", "2009-06-05 19:20:00");
+ TEST_APPROXIDATE("06.05.2009", "2009-05-06 19:20:00");
+
+ TEST_APPROXIDATE("Jun 6, 5AM", "2009-06-06 05:00:00");
+ TEST_APPROXIDATE("5AM Jun 6", "2009-06-06 05:00:00");
+ TEST_APPROXIDATE("6AM, June 7, 2009", "2009-06-07 06:00:00");
+
+ TEST_APPROXIDATE("2008-12-01", "2008-12-01 19:20:00");
+ TEST_APPROXIDATE("2009-12-01", "2009-12-01 19:20:00");
+
+ TEST_DATE_FORMAT_HUMAN(18000, "5 hours ago");
+ TEST_DATE_FORMAT_HUMAN(432000, "Tue Aug 25 19:20");
+ TEST_DATE_FORMAT_HUMAN(1728000, "Mon Aug 10 19:20");
+ TEST_DATE_FORMAT_HUMAN(13000000, "Thu Apr 2 08:13");
+ TEST_DATE_FORMAT_HUMAN(31449600, "Aug 31 2008");
+ TEST_DATE_FORMAT_HUMAN(37500000, "Jun 22 2008");
+ TEST_DATE_FORMAT_HUMAN(55188000, "Dec 1 2007");
+ TEST_DATE_FORMAT_HUMAN(630000000, "Sep 13 1989");
+
+ return test_done();
+}
--
2.43.0.windows.1
^ permalink raw reply related
* Re: [PATCH v3 2/2] add-patch: classify '@' as a synonym for 'HEAD'
From: Phillip Wood @ 2024-02-05 16:37 UTC (permalink / raw)
To: Ghanshyam Thakkar, git; +Cc: gitster, ps
In-Reply-To: <20240203112619.979239-6-shyamthakkar001@gmail.com>
Hi Ghanshyam
I think this is a useful addition, I've left a couple of comments below
On 03/02/2024 11:25, Ghanshyam Thakkar wrote:
> diff --git a/add-patch.c b/add-patch.c
> index 68f525b35c..7d565dcb33 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -378,6 +378,11 @@ static int parse_hunk_header(struct add_p_state *s, struct hunk *hunk)
> return 0;
> }
>
> +static inline int user_meant_head(const char *rev)
> +{
> + return !strcmp(rev, "HEAD") || !strcmp(rev, "@");
> +}
> +
As well as the places you have converted we also have an explicit test
for "HEAD" in parse_diff() which looks like
if (s->revision) {
struct object_id oid;
strvec_push(&args,
/* could be on an unborn branch */
!strcmp("HEAD", s->revision) &&
repo_get_oid(the_repository, "HEAD", &oid) ?
empty_tree_oid_hex() : s->revision);
}
I suspect we need to update that code as well to accept "@" as a synonym
for "HEAD" on an unborn branch.
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index a6e30931b5..79e208ee6d 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -539,12 +539,13 @@ static int checkout_paths(const struct checkout_opts *opts,
> * recognized by diff-index), we will always replace the name
> * with the hex of the commit (whether it's in `...` form or
> * not) for the run_add_interactive() machinery to work
> - * properly. However, there is special logic for the HEAD case
> - * so we mustn't replace that. Also, when we were given a
> - * tree-object, new_branch_info->commit would be NULL, but we
> - * do not have to do any replacement, either.
> + * properly. However, there is special logic for the 'HEAD' and
> + * '@' case so we mustn't replace that. Also, when we were
> + * given a tree-object, new_branch_info->commit would be NULL,
> + * but we do not have to do any replacement, either.
> */
> - if (rev && new_branch_info->commit && strcmp(rev, "HEAD"))
> + if (rev && new_branch_info->commit && strcmp(rev, "HEAD") &&
> + strcmp(rev, "@"))
> rev = oid_to_hex_r(rev_oid, &new_branch_info->commit->object.oid);
I agree with Junio's suggestion to use the user_meant_head() here.
Looking at this made me wonder why builtin/reset.c does not need
updating. The answer seems to be that reset passes in the revision as
given on the commandline after checking it refers to a valid tree
whereas for checkout the revision for on the commandline might contain
"..." which run_add_p() cannot handle.
> diff --git a/t/t2071-restore-patch.sh b/t/t2071-restore-patch.sh
> index b5c5c0ff7e..3dc9184b4a 100755
> --- a/t/t2071-restore-patch.sh
> +++ b/t/t2071-restore-patch.sh
> @@ -44,13 +44,17 @@ test_expect_success PERL 'git restore -p with staged changes' '
It is a pre-existing problem but all these "PERL" prerequisites are
no-longer required as we've removed the perl implementation of "add -p"
> verify_state dir/foo index index
> '
>
> -test_expect_success PERL 'git restore -p --source=HEAD' '
> - set_state dir/foo work index &&
> - # the third n is to get out in case it mistakenly does not apply
> - test_write_lines n y n | git restore -p --source=HEAD &&
> - verify_saved_state bar &&
> - verify_state dir/foo head index
> -'
> +for opt in "HEAD" "@"
> +do
> + test_expect_success PERL "git restore -p --source=$opt" '
> + set_state dir/foo work index &&
> + # the third n is to get out in case it mistakenly does not apply
> + test_write_lines n y n | git restore -p --source=$opt >output &&
> + verify_saved_state bar &&
> + verify_state dir/foo head index &&
> + test_grep "Discard" output
It is good to see that we're now testing for a reversed patch here.
Best Wishes
Phillip
^ permalink raw reply
* Re: [PATCH] Add ideas for GSoC 2024
From: Christian Couder @ 2024-02-05 16:43 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: git, Taylor Blau, Junio C Hamano, Victoria Dye, Karthik Nayak
In-Reply-To: <106b8e7be9ddc2d24670b01d54347dfcf9aef196.1707122040.git.ps@pks.im>
On Mon, Feb 5, 2024 at 9:39 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> Add project ideas for the GSoC 2024.
> ---
>
> I came up with four different topics:
>
> - The reftable unit test refactorings. This topic can also be squashed
> into the preexisting unit test topics, I wouldn't mind. In that case
> I'd be happy to be a possible mentor, too.
>
> - Ref consistency checks for git-fsck(1). This should be rather
> straight forward and make for an interesting topic.
>
> - Making git-bisect(1)'s state more self-contained as recently
> discussed. This topic is easy to implement, but the backwards
> compatibility issues might require a lot of attention.
>
> - Implementing support for reftables in the "dumb" HTTP protocol. It's
> quite niche given that the dumb protocol isn't really used much
> nowadays anymore. But it could make for an interesting project
> regardless.
>
> It's hard to estimate for me whether their scope is either too small or
> too big. So please feel free to chime in and share your concerns if you
> think that any of those proposals don't make much sense in your opinion.
Thanks a lot for these ideas! I have applied your patch and pushed it.
I have a few concerns though, see below.
> SoC-2024-Ideas.md | 129 ++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 129 insertions(+)
>
> diff --git a/SoC-2024-Ideas.md b/SoC-2024-Ideas.md
> index 3efbcaf..286aea0 100644
> --- a/SoC-2024-Ideas.md
> +++ b/SoC-2024-Ideas.md
> @@ -39,3 +39,132 @@ Languages: C, shell(bash)
> Possible mentors:
> * Christian Couder < <christian.couder@gmail.com> >
>
> +### Convert reftable unit tests to use the unit testing framework
> +
> +The "reftable" unit tests in "t0032-reftable-unittest.sh"
> +predate the unit testing framework that was recently
> +introduced into Git. These tests should be converted to use
> +the new framework.
> +
> +See:
> +
> + - this discussion <https://lore.kernel.org/git/cover.1692297001.git.steadmon@google.com/>
> +
> +Expected Project Size: 175 hours or 350 hours
> +
> +Difficulty: Low
"Difficulty: Low" might not be very accurate from the point of view of
contributors. I think it's always quite difficult to contribute
something significant to Git, and sometimes more than we expected.
> +Languages: C, shell(bash)
> +
> +Possible mentors:
> +* Patrick Steinhardt < <ps@pks.im> >
> +* Karthik Nayak < <karthik.188@gmail.com> >
> +
> +### Implement consistency checks for refs
> +
> +The git-fsck(1) command is used to check various data
> +structures for consistency. Notably missing though are
> +consistency checks for the refdb. While git-fsck(1)
> +implicitly checks some of the properties of the refdb
> +because it uses its refs for a connectivity check, these
> +checks aren't sufficient to properly ensure that all refs
> +are properly consistent.
> +
> +The goal of this project would be to introduce consistency
> +checks that can be implemented by the ref backend. Initially
> +these checks may only apply to the "files" backend. With the
> +ongoing efforts to upstream a new "reftable" backend the
> +effort may be extended.
> +
> +See:
> +
> + - https://lore.kernel.org/git/6cfee0e4-3285-4f18-91ff-d097da9de737@rd10.de/
> + - https://lore.kernel.org/git/cover.1706601199.git.ps@pks.im/
> +
> +Expected Project Size: 175 hours or 350 hours
> +
> +Difficulty: Medium
> +
> +Languages: C, shell(bash)
> +
> +Possible mentors:
> +* Patrick Steinhardt < <ps@pks.im> >
> +* Karthik Nayak < <karthik.188@gmail.com> >
> +
> +### Refactor git-bisect(1) to make its state self-contained
> +
> +The git-bisect(1) command is used to find a commit in a
> +range of commits that introduced a specific bug. Starting a
> +bisection run creates a set of state files into the Git
> +repository which record various different parameters like
> +".git/BISECT_START". These files look almost like refs
> +due to their names being all-uppercase. This has led to
> +confusion with the new "reftable" backend because it wasn't
> +quite clear whether those files are in fact refs or not.
> +
> +As it turns out they are not refs and should never be
> +treated like one. Overall, it has been concluded that the
> +way those files are currently stored is not ideal. Instead
> +of having a proliferation of files in the Git directory, it
> +was discussed whether the bisect state should be moved into
> +its own "bisect-state" subdirectory. This would make it more
> +self-contained and thereby avoid future confusion. It is
> +also aligned with the sequencer state used by rebases, which
> +is neatly contained in the "rebase-apply" and "rebase-merge"
> +directories.
> +
> +The goal of this project would be to realize this change.
> +While rearchitecting the layout should be comparatively easy
> +to do, the harder part will be to hash out how to handle
> +backwards compatibility.
> +
> +See:
> +
> + - https://lore.kernel.org/git/Za-gF_Hp_lXViGWw@tanuki/
From reading the discussion it looks like everyone is Ok with doing
this. I really hope that we are not missing something that might make
us decide early not to do this though.
> +Expected Project Size: 175 hours or 350 hours
> +
> +Difficulty: Medium
> +
> +Languages: C, shell(bash)
> +
> +Possible mentors:
> +* Patrick Steinhardt < <ps@pks.im> >
> +* Karthik Nayak < <karthik.188@gmail.com> >
^ permalink raw reply
* Re: Git in GSoC 2024
From: Christian Couder @ 2024-02-05 17:07 UTC (permalink / raw)
To: Kaartic Sivaraam
Cc: Patrick Steinhardt, git, Taylor Blau, Junio C Hamano,
Victoria Dye, Karthik Nayak
In-Reply-To: <d4797f27-825b-4e2b-85a6-cc30f33934e3@gmail.com>
Hi Kaartic, Patrick, Karthik and all,
On Sat, Feb 3, 2024 at 12:41 PM Kaartic Sivaraam
<kaartic.sivaraam@gmail.com> wrote:
> On 01/02/24 15:08, Patrick Steinhardt wrote:
> > On Wed, Jan 31, 2024 at 11:27:13PM +0530, Kaartic Sivaraam wrote:
> >>
> >> I created a fairly rough SoC ideas page for now including a barebones
> >> information about the unit test migration idea:
> >>
> >> https://git.github.io/SoC-2024-Ideas/
Thanks for creating the page!
I have just applied the patch Patrick sent to the mailing list with
the ideas related to reftable.
> >> Note well that the existing idea's description is far from complete and I
> >> mostly just cooked it up to serve as a template for how the idea entry could
> >> look like. Kindly contribute towards elaborating the same :-)
> >>
> >> Also, feel free to suggest ideas you have around refs and reftable backed,
> >> Patrick. Those would be helpful.
> >
> > I'll have a the beginning of next week and will think about topics
> > meanwhile.
> >
>
> Thanks, Patrick! It would be great if you could share the same as soon
> as possible. The deadline for applying to GSoC is Feb 6 (18:00 UTC) and
> we need the ideas page to be decent enough before we go ahead with
> applying for this year.
>
> If the elaborate project description could take time, feel free to share
> a paragraph or two that are supplemented with a few references. That
> should be sufficient for applying to GSoC.
Yeah, we need a decent idea page, but it doesn't need to be finalized.
I think we can still make changes after the deadline (which is for the
Open Source Orgs to apply).
> Christian,
>
> It would be great if you could look into and improve the detail for the
> unit test migration idea. I just added a very terse description based on
> what I could get my hands on. If you think the description we used for
> the Outreachy round would do, kindly update the page with the same or
> kindly share it here so that I could update the same in the ideas page :-)
The project description for Outreachy was not very elaborate and is
now quite outdated. I have instead improved the project description in
the Ideas page.
> > Yeah, as long as there is a co-mentor that can take over during my
> > absence I'm happy to do it. Karthik said that he'd be willing to cover
> > me, which I think would be a good fit given that he's already got quite
> > a bit of exposure to the reftable backend internally at GitLab. Thanks!
>
> Sounds good. Thank you for volunteering to co-mentor, Karthik!
Yeah, thank you Patrick and Karthik!
^ permalink raw reply
* [PATCH] commit.c: ensure strchrnul() doesn't scan beyond range
From: Chandra Pratap via GitGitGadget @ 2024-02-05 17:21 UTC (permalink / raw)
To: git; +Cc: Chandra Pratap, Chandra Pratap
From: Chandra Pratap <chandrapratap3519@gmail.com>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
commit.c: ensure strchrnul() doesn't scan beyond range
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1652%2FChand-ra%2Fstrchrnul-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1652/Chand-ra/strchrnul-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1652
commit.c | 8 +-------
1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/commit.c b/commit.c
index ef679a0b939..a65b8e92e94 100644
--- a/commit.c
+++ b/commit.c
@@ -1743,15 +1743,9 @@ const char *find_header_mem(const char *msg, size_t len,
int key_len = strlen(key);
const char *line = msg;
- /*
- * NEEDSWORK: It's possible for strchrnul() to scan beyond the range
- * given by len. However, current callers are safe because they compute
- * len by scanning a NUL-terminated block of memory starting at msg.
- * Nonetheless, it would be better to ensure the function does not look
- * at msg beyond the len provided by the caller.
- */
while (line && line < msg + len) {
const char *eol = strchrnul(line, '\n');
+ assert(eol - line <= len);
if (line == eol)
return NULL;
base-commit: a54a84b333adbecf7bc4483c0e36ed5878cac17b
--
gitgitgadget
^ permalink raw reply related
* RE: [Outreachy][PATCH 1/2] date: refactor 64 bit prereq code into reusable functions
From: rsbecker @ 2024-02-05 17:34 UTC (permalink / raw)
To: 'Achu Luma', git; +Cc: christian.couder, 'Christian Couder'
In-Reply-To: <20240205162506.1835-1-ach.lumap@gmail.com>
On Monday, February 5, 2024 11:25 AM, Achu Luma wrote:
>In a following commit we are going to port code from "t/helper/test-date.c"
and
>"t/t0006-date.sh" to a new "t/unit-tests/t-date.c" file using the recently
added unit
>test framework.
>
>We cannot fully port all the code from "t/helper/test-date.c" though, as
the test-
>tool date helper is still used by a number of "t/*.sh" tests.
>The TIME_IS_64BIT and TIME_T_IS_64BIT prereqs are especially used by
"t5000-
>tar-tree.sh", "t5318-commit-graph.sh" and
"t5328-commit-graph-64bit-time.sh"
>while checking those prereqs will be required in the new
"t/unit-tests/t-date.c" file
>too.
>
>To avoid duplicating in both "t/helper/test-date.c" and
"t/unit-tests/t-date.c" the
>small amount of code checking these prereqs, let's move it into inline
functions in
>"date.h".
>
>The names of these new inline functions contain "TIME_IS_64BIT" or
>"TIME_T_IS_64BIT" as it will simplify the macros we will use when we will
port code
>to "t/unit-tests/t-date.c" in a following commit.
>
>Mentored-by: Christian Couder <chriscool@tuxfamily.org>
>Signed-off-by: Achu Luma <ach.lumap@gmail.com>
>---
> date.h | 6 ++++++
> t/helper/test-date.c | 4 ++--
> 2 files changed, 8 insertions(+), 2 deletions(-)
>
>diff --git a/date.h b/date.h
>index 6136212a19..fb70490a51 100644
>--- a/date.h
>+++ b/date.h
>@@ -70,4 +70,10 @@ void datestamp(struct strbuf *out); timestamp_t
>approxidate_careful(const char *, int *); int date_overflows(timestamp_t
date);
>time_t tm_to_time_t(const struct tm *tm);
>+static inline int check_prereq_TIME_IS_64BIT(void) {
>+ return sizeof(timestamp_t) == 8;
>+}
>+static inline int check_prereq_TIME_T_IS_64BIT(void) {
>+ return sizeof(time_t) == 8;
>+}
> #endif
>diff --git a/t/helper/test-date.c b/t/helper/test-date.c index
>0683d46574..be0b8679c3 100644
>--- a/t/helper/test-date.c
>+++ b/t/helper/test-date.c
>@@ -126,9 +126,9 @@ int cmd__date(int argc UNUSED, const char **argv)
> else if (!strcmp(*argv, "getnanos"))
> getnanos(argv+1);
> else if (!strcmp(*argv, "is64bit"))
>- return sizeof(timestamp_t) == 8 ? 0 : 1;
>+ return !check_prereq_TIME_IS_64BIT();
> else if (!strcmp(*argv, "time_t-is64bit"))
>- return sizeof(time_t) == 8 ? 0 : 1;
>+ return !check_prereq_TIME_T_IS_64BIT();
> else
> usage(usage_msg);
> return 0;
>--
>2.43.0.windows.1
I would suggest that you also take into account whether time_t is signed or
not (more difficult perhaps). Some platforms use signed time_t to allow
representation of dates prior to 1970-01-01, while others make this signed.
Some other platforms (S/390 for example) have retained time_t as 32-bits but
have a time64_t for 64 bits. It might be useful to account for this.
--Randall
^ permalink raw reply
* Re: Git in GSoC 2024
From: Kaartic Sivaraam @ 2024-02-05 18:39 UTC (permalink / raw)
To: Christian Couder
Cc: Patrick Steinhardt, git, Taylor Blau, Junio C Hamano,
Victoria Dye, Karthik Nayak
In-Reply-To: <CAP8UFD3GBT7s1jGOc=fe6XdYGF1c--tMBDiy_sDg1Afsa=drDw@mail.gmail.com>
Hi Christian,
On 05/02/24 22:37, Christian Couder wrote:
> Hi Kaartic, Patrick, Karthik and all,
>
> Thanks for creating the page!
>
> I have just applied the patch Patrick sent to the mailing list with
> the ideas related to reftable.
>
Thank you! I've now successfully submitted the application for Git using
the Ideas page we have :-)
Let's hope that we get selected this year. We should know about that by
February 21 - 18:00 UTC.
> Yeah, we need a decent idea page, but it doesn't need to be finalized.
> I think we can still make changes after the deadline (which is for the
> Open Source Orgs to apply).
>
Indeed. Let's see if we could get any new ideas / potential mentors for
this GSoC :-]
>> Christian,
>>
>> It would be great if you could look into and improve the detail for the
>> unit test migration idea. I just added a very terse description based on
>> what I could get my hands on. If you think the description we used for
>> the Outreachy round would do, kindly update the page with the same or
>> kindly share it here so that I could update the same in the ideas page :-)
>
> The project description for Outreachy was not very elaborate and is
> now quite outdated. I have instead improved the project description in
> the Ideas page.
>
That's great. Thanks!
--
Sivaraam
^ permalink raw reply
* Re: [PATCH v3 4/4] for-each-ref: avoid filtering on empty pattern
From: Phillip Wood @ 2024-02-05 18:48 UTC (permalink / raw)
To: Karthik Nayak, git; +Cc: gitster, ps
In-Reply-To: <20240129113527.607022-5-karthik.188@gmail.com>
Hi Karthik
On 29/01/2024 11:35, Karthik Nayak wrote:
> When the user uses an empty string pattern (""), we don't match any refs
> in git-for-each-ref(1). This is because in git-for-each-ref(1), we use
> path based matching and an empty string doesn't match any path.
>
> In this commit we change this behavior by making empty string pattern
> match all references. This is done by introducing a new flag
> `FILTER_REFS_NO_FILTER` in `ref-filter.c`, which uses the newly
> introduced `refs_for_each_all_refs()` function to iterate over all the
> refs in the repository.
It actually iterates over all the refs in the current worktree, not all
the refs in the repository. I have to say that I find it extremely
unintuitive that "" behaves differently to not giving a pattern. I
wonder if we can find a better UI here - perhaps a command line option
to include pseudorefs?
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
> Documentation/git-for-each-ref.txt | 3 ++-
> builtin/for-each-ref.c | 21 +++++++++++++++++-
> ref-filter.c | 13 ++++++++++--
> ref-filter.h | 4 +++-
> t/t6302-for-each-ref-filter.sh | 34 ++++++++++++++++++++++++++++++
> 5 files changed, 70 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> index be9543f684..b1cb482bf5 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -32,7 +32,8 @@ OPTIONS
> If one or more patterns are given, only refs are shown that
> match against at least one pattern, either using fnmatch(3) or
> literally, in the latter case matching completely or from the
> - beginning up to a slash.
> + beginning up to a slash. If an empty string is provided all refs
> + are printed, including HEAD and pseudorefs.
I think it would be helpful to clarify that it is all the refs for the
current worktree that are printed, not all the refs in the repository -
we still don't have a way to iterate over the per-worktree refs from
other worktrees
Best Wishes
Phillip
^ permalink raw reply
* Re: [PATCH] Add ideas for GSoC 2024
From: Kaartic Sivaraam @ 2024-02-05 18:55 UTC (permalink / raw)
To: Christian Couder, Patrick Steinhardt
Cc: git, Taylor Blau, Junio C Hamano, Victoria Dye, Karthik Nayak
In-Reply-To: <CAP8UFD3F95TzytdpKO=LLf6Y_ejxwh9QtgAxRNKgMXW-72hjgQ@mail.gmail.com>
Hi Patrick, Christian and all,
On 05/02/24 22:13, Christian Couder wrote:
>
> Thanks a lot for these ideas! I have applied your patch and pushed it.
>
Yeah. Thanks for sharing these great ideas! I've submitted the
application using the new ideas page now as mentioned in the parent thread.
>> +### Convert reftable unit tests to use the unit testing framework
>> +
>> +The "reftable" unit tests in "t0032-reftable-unittest.sh"
>> +predate the unit testing framework that was recently
>> +introduced into Git. These tests should be converted to use
>> +the new framework.
>> +
>> +See:
>> +
>> + - this discussion <https://lore.kernel.org/git/cover.1692297001.git.steadmon@google.com/>
>> +
>> +Expected Project Size: 175 hours or 350 hours
>> +
>> +Difficulty: Low
>
> "Difficulty: Low" might not be very accurate from the point of view of
> contributors. I think it's always quite difficult to contribute
> something significant to Git, and sometimes more than we expected.
>
Makes sense. Also, I'm kind of cat-one-the-wall about whether it makes
sense to have two projects about the unit test migration effort itself.
If we're clear that both of them would not overlap, it should be fine.
Otherwise, it would be better to merge them as Patrick suggests.
That said, how helpful would it be to link the following doc in the unit
testing related ideas?
https://github.com/git/git/blob/master/Documentation/technical/unit-tests.txt
>> +### Implement consistency checks for refs
>> +
>>
>> [ ... snip ... ]
>>
>> +
>> + - https://lore.kernel.org/git/6cfee0e4-3285-4f18-91ff-d097da9de737@rd10.de/
>> + - https://lore.kernel.org/git/cover.1706601199.git.ps@pks.im/
>> +
>> [ .... snip ... ]
>> +
>> +### Implement support for reftables in "dumb" HTTP transport
Would it worth linking the reftable technical doc for the above ideas?
https://git-scm.com/docs/reftable
I could see it goes into a lot of detail. I'm just wondering if link to
it would help someone who's looking to learn about reftable.
--
Sivaraam
^ permalink raw reply
* Re: [PATCH v3 5/7] refs: add pseudorefs array and iteration functions
From: Kousik Sanagavarapu @ 2024-02-05 18:55 UTC (permalink / raw)
To: Andy Koppe; +Cc: git, Junio C Hamano
In-Reply-To: <20231023221143.72489-6-andy.koppe@gmail.com>
Andy Koppe <andy.koppe@gmail.com> wrote:
> Define const array 'pseudorefs' with the names of the pseudorefs that
> are documented in gitrevisions.1, and add functions for_each_pseudoref()
> and refs_for_each_pseudoref() for iterating over them.
>
> The functions process the pseudorefs in the same way as head_ref() and
> refs_head_ref() process HEAD, invoking an each_ref_fn callback on each
> pseudoref that exists.
>
> This is in preparation for adding pseudorefs to log decorations.
>
> Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
> ---
[...]
> +/*
> + * List of documented pseudorefs. This needs to be kept in sync with the list
> + * in Documentation/revisions.txt.
> + */
> +static const char *const pseudorefs[] = {
> + "FETCH_HEAD",
> + "ORIG_HEAD",
> + "MERGE_HEAD",
> + "REBASE_HEAD",
> + "CHERRY_PICK_HEAD",
> + "REVERT_HEAD",
> + "BISECT_HEAD",
> + "AUTO_MERGE",
> +};
> +
> struct ref_namespace_info ref_namespace[] = {
> [NAMESPACE_HEAD] = {
> .ref = "HEAD",
> @@ -1549,6 +1564,33 @@ int head_ref(each_ref_fn fn, void *cb_data)
> return refs_head_ref(get_main_ref_store(the_repository), fn, cb_data);
> }
The first thing that popped up in my head was "Should we somehow use
is_pseudoref_syntax() instead of manually listing these?" (although I
read in this thread later that Junio was okay with the listing) but then ...
I thought I saw something similar in some other thread (which entered
the mailing list much after this patch series was submitted) ...
https://lore.kernel.org/git/20231221170715.110565-2-karthik.188@gmail.com/T/
The whole thread is really interesting but some points that are worth to
be mentioned in this context are
" ... Patrick's reftable work based on Han-Wen's work revealed
the need to treat FETCH_HEAD and MERGE_HEAD as "even more
pecurilar than pseudorefs" that need different term (tentatively
called "special refs") ... "
So since we are introducing this array in refs.c, which acts as a "refs
API" currently
"A lot more reasonable thing to do may be to scan the
$GIT_DIR for files whose name satisfy refs.c:is_pseudoref_syntax()
and list them, instead of having a hardcoded list of these special
refs. In addition, when reftable and other backends that can
natively store things outside refs/ hierarchy is in use, they ought
to know what they have so enumerating these would not be an issue
for them without having such a hardcoded table of names."
All that said, the above mentioned thread led to a series of patches for
a different purpose than this [1] (which are currently on their way to
"master" according to the latest "What's Cooking" email on Feb 2). The
ones that have significance w.r.t. to THIS patch series though, are
https://lore.kernel.org/git/20240129113527.607022-2-karthik.188@gmail.com/
https://lore.kernel.org/git/20240129113527.607022-4-karthik.188@gmail.com/
(ignoring the reftable part).
I find these to make sense HERE because using the functions introduced
THERE are much more robust when dealing with pseudorefs and can be used
HERE.
I haven't given it much thought but I think we would still end up
writing "for_each_pseudoref()", although much differently from below
(and can't use "refs_for_each_all_refs()" directly) because of how we
call this function in PATCH 7/7 when actually doing the decoration - that
is the decoration for pseudorefs is different (?)
Another approach would be I think to refactor the whole of how
decorations with refs work and somehow use "refs_for_each_all_refs()"
with its callback handling how we decorate the various refs - I need to
dig deeper :) - since the end goal is to support showing all kinds of
refs when showing the log
$ git log -1 --clear-decorations --oneline master
2a540e432f (ORIG_HEAD, FETCH_HEAD, upstream/master, upstream/HEAD, master) The thirteenth batch
(with color enabled)
> +int refs_for_each_pseudoref(struct ref_store *refs,
> + each_ref_fn fn, void *cb_data)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(pseudorefs); i++) {
> + struct object_id oid;
> + int flag;
> +
> + if (refs_resolve_ref_unsafe(refs, pseudorefs[i],
> + RESOLVE_REF_READING, &oid, &flag)) {
> + int ret = fn(pseudorefs[i], &oid, flag, cb_data);
> +
> + if (ret)
> + return ret;
> + }
> + }
> +
> + return 0;
> +}
> +
> +int for_each_pseudoref(each_ref_fn fn, void *cb_data)
> +{
> + return refs_for_each_pseudoref(get_main_ref_store(the_repository),
> + fn, cb_data);
> +}
> +
> struct ref_iterator *refs_ref_iterator_begin(
> struct ref_store *refs,
> const char *prefix,
> diff --git a/refs.h b/refs.h
> index 23211a5ea1..7b55cced31 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -320,6 +320,8 @@ typedef int each_repo_ref_fn(struct repository *r,
> */
> int refs_head_ref(struct ref_store *refs,
> each_ref_fn fn, void *cb_data);
> +int refs_for_each_pseudoref(struct ref_store *refs,
> + each_ref_fn fn, void *cb_data);
> int refs_for_each_ref(struct ref_store *refs,
> each_ref_fn fn, void *cb_data);
> int refs_for_each_ref_in(struct ref_store *refs, const char *prefix,
> @@ -334,6 +336,9 @@ int refs_for_each_remote_ref(struct ref_store *refs,
> /* just iterates the head ref. */
> int head_ref(each_ref_fn fn, void *cb_data);
>
> +/* iterates pseudorefs. */
> +int for_each_pseudoref(each_ref_fn fn, void *cb_data);
> +
> /* iterates all refs. */
> int for_each_ref(each_ref_fn fn, void *cb_data);
>
> --
> 2.42.GIT
So yeah, I just wanted to point out the above things as we would need to
refactor this commit and the commits following this - patches 6/7 and 7/7.
Thanks
[1]: https://lore.kernel.org/git/20240129113527.607022-1-karthik.188@gmail.com/
^ permalink raw reply
* Re: What's cooking in git.git (Feb 2024, #02; Fri, 2)
From: Phillip Wood @ 2024-02-05 18:57 UTC (permalink / raw)
To: Junio C Hamano, git
In-Reply-To: <xmqqmssirm6t.fsf@gitster.g>
Hi Junio
On 03/02/2024 08:23, Junio C Hamano wrote:
> * kn/for-all-refs (2024-01-29) 4 commits
> (merged to 'next' on 2024-01-30 at e7a9234a8b)
> + for-each-ref: avoid filtering on empty pattern
> + refs: introduce `refs_for_each_all_refs()`
> + refs: extract out `loose_fill_ref_dir_regular_file()`
> + refs: introduce `is_pseudoref()` and `is_headref()`
>
> "git for-each-ref" filters its output with prefixes given from the
> command line, but it did not honor an empty string to mean "pass
> everything", which has been corrected.
I've been a bit slow to look at the latest re-roll but having done so
I'm concerned that the UI could use some improvement. If I understand
correctly the proposal is to make
git for-each-ref
and
git for-each-ref ""
behave differently so that the latter prints the pseudorefs from the
current worktree and the former does not. I can't help feeling that's
the sort of thing that makes people complain that git is hard to
understand. I wonder if we'd be better off adding an option to include
pseudorefs and have an empty pattern behave the same as no pattern.
Best Wishes
Phillip
> Will merge to 'master'.
> source: <20240129113527.607022-1-karthik.188@gmail.com>
^ permalink raw reply
* Re: [PATCH] commit.c: ensure strchrnul() doesn't scan beyond range
From: René Scharfe @ 2024-02-05 19:57 UTC (permalink / raw)
To: Chandra Pratap via GitGitGadget, git; +Cc: Chandra Pratap, Chandra Pratap
In-Reply-To: <pull.1652.git.1707153705840.gitgitgadget@gmail.com>
Am 05.02.24 um 18:21 schrieb Chandra Pratap via GitGitGadget:
> From: Chandra Pratap <chandrapratap3519@gmail.com>
>
> Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
> ---
> commit.c: ensure strchrnul() doesn't scan beyond range
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1652%2FChand-ra%2Fstrchrnul-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1652/Chand-ra/strchrnul-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1652
>
> commit.c | 8 +-------
> 1 file changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/commit.c b/commit.c
> index ef679a0b939..a65b8e92e94 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -1743,15 +1743,9 @@ const char *find_header_mem(const char *msg, size_t len,
> int key_len = strlen(key);
> const char *line = msg;
>
> - /*
> - * NEEDSWORK: It's possible for strchrnul() to scan beyond the range
> - * given by len. However, current callers are safe because they compute
> - * len by scanning a NUL-terminated block of memory starting at msg.
> - * Nonetheless, it would be better to ensure the function does not look
> - * at msg beyond the len provided by the caller.
> - */
> while (line && line < msg + len) {
> const char *eol = strchrnul(line, '\n');
> + assert(eol - line <= len);
Something like this might work in Verse, but C is more simple-minded.
You can't undo an out-of-bounds access after the fact, and assert()
would be compiled out if the code is built with NDEBUG anyway.
If you want to make the code work with buffers that lack a terminating
NUL then you need to replace the strchrnul() call with something that
respects buffer lengths. You could e.g. call memchr(). Don't forget
to check for NUL to preserve the original behavior. Or you could roll
your own custom replacement, perhaps like this:
char *strnchrnul(const char *s, int c, size_t len)
{
while (len-- && *s && *s != c)
s++;
return (char *)s;
}
A test with the new unit-test framework would be nice. It should be
possible to show that the current code runs over the passed len,
without causing undefined behavior. E.g. find_header_mem("foo bar",
2, "foo", &len) is safe, but returns "bar" instead of NULL.
>
> if (line == eol)
> return NULL;
>
> base-commit: a54a84b333adbecf7bc4483c0e36ed5878cac17b
^ permalink raw reply
* git-gui desktop launcher
From: Tobias Boesch @ 2024-02-05 20:12 UTC (permalink / raw)
To: git
Hello everyone,
quoting from downstream issue:
https://gitlab.archlinux.org/archlinux/packaging/packages/git/-/issues/5
-------------------------
"As far as I can see git gui cannot easily be used by me on arch.
A .desktop entry is missing for me.
I created one that opens git gui.
It also adds an entry in the "Open With..." menu of file managers (I
tested only with Nautilus). Opeing git gui with this entry git gui is
opened in the folder where the menu was opened.
If it is a git repository git gui open it. If it is no git repository
git gui opens just as if it was called from the desktop launcher.
Since it took a while to create it and adds value for me I would like
to share it to be added to the git package by default.
It is far from being perfect. It's a first working version. For me
personally it is enough.
Before tweaking it further to fit the packaging standards I would like
to ask if is desired to be added.
.desktop file proposal
[Desktop Entry]
Name=git gui
Comment=A portable graphical interface to Git
Exec=/bin/bash -c 'if [[ "$0" = "/bin/bash" ]]; then git gui; else cd
"$0" && git gui; fi' %F
Icon=/usr/share/git-gui/lib/git-gui.ico
Type=Application
Terminal=false
Categories=Development;
I think upstream has any interest to add this. Therefore I ask here."
-------------------------
The arch package maintainer proposed to try to to add this to upstream
before just putting it into the arch package.
Here I am asking if it could be added to git.
If it's worth to add it, I would take the time to improve it if there
are suggestions or comments on the current version.
Best wishes and thanks for developing git.
Tobias
^ permalink raw reply
* Re: [PATCH v3 2/2] add-patch: classify '@' as a synonym for 'HEAD'
From: Ghanshyam Thakkar @ 2024-02-05 20:38 UTC (permalink / raw)
To: phillip.wood, git; +Cc: gitster, ps
In-Reply-To: <9f9f26f1-5460-468e-a893-5caf7fbea981@gmail.com>
On Mon Feb 5, 2024 at 10:07 PM IST, Phillip Wood wrote:
> On 03/02/2024 11:25, Ghanshyam Thakkar wrote:
> > diff --git a/add-patch.c b/add-patch.c
> > index 68f525b35c..7d565dcb33 100644
> > --- a/add-patch.c
> > +++ b/add-patch.c
> > @@ -378,6 +378,11 @@ static int parse_hunk_header(struct add_p_state *s, struct hunk *hunk)
> > return 0;
> > }
> >
> > +static inline int user_meant_head(const char *rev)
> > +{
> > + return !strcmp(rev, "HEAD") || !strcmp(rev, "@");
> > +}
> > +
>
> As well as the places you have converted we also have an explicit test
> for "HEAD" in parse_diff() which looks like
>
> if (s->revision) {
> struct object_id oid;
> strvec_push(&args,
> /* could be on an unborn branch */
> !strcmp("HEAD", s->revision) &&
> repo_get_oid(the_repository, "HEAD", &oid) ?
> empty_tree_oid_hex() : s->revision);
> }
>
> I suspect we need to update that code as well to accept "@" as a synonym
> for "HEAD" on an unborn branch.
I had already considered that. Updating here will not have any effect,
because on unborn branch, we do not allow naming HEAD or @. This case is
for when we run without naming any revision (i.e. git reset -p) on
unborn branch. In such cases, we pass 'HEAD' as a default value.
>
> > diff --git a/builtin/checkout.c b/builtin/checkout.c
> > index a6e30931b5..79e208ee6d 100644
> > --- a/builtin/checkout.c
> > +++ b/builtin/checkout.c
> > @@ -539,12 +539,13 @@ static int checkout_paths(const struct checkout_opts *opts,
> > * recognized by diff-index), we will always replace the name
> > * with the hex of the commit (whether it's in `...` form or
> > * not) for the run_add_interactive() machinery to work
> > - * properly. However, there is special logic for the HEAD case
> > - * so we mustn't replace that. Also, when we were given a
> > - * tree-object, new_branch_info->commit would be NULL, but we
> > - * do not have to do any replacement, either.
> > + * properly. However, there is special logic for the 'HEAD' and
> > + * '@' case so we mustn't replace that. Also, when we were
> > + * given a tree-object, new_branch_info->commit would be NULL,
> > + * but we do not have to do any replacement, either.
> > */
> > - if (rev && new_branch_info->commit && strcmp(rev, "HEAD"))
> > + if (rev && new_branch_info->commit && strcmp(rev, "HEAD") &&
> > + strcmp(rev, "@"))
> > rev = oid_to_hex_r(rev_oid, &new_branch_info->commit->object.oid);
>
> I agree with Junio's suggestion to use the user_meant_head() here.
> Looking at this made me wonder why builtin/reset.c does not need
> updating. The answer seems to be that reset passes in the revision as
> given on the commandline after checking it refers to a valid tree
> whereas for checkout the revision for on the commandline might contain
> "..." which run_add_p() cannot handle.
I was not able to run reset with '...'. I ran,
'git reset main...$ANOTHERBRANCH'
but it gave me "fatal: ambiguous argument 'main...$ANOTHERBRANCH'"
error, with or without -p. While,
'git restore --source=main...$ANOTHERBRANCH .' and
'git checkout main...$ANOTHERBRANCH' works fine, with or without -p.
> > diff --git a/t/t2071-restore-patch.sh b/t/t2071-restore-patch.sh
> > index b5c5c0ff7e..3dc9184b4a 100755
> > --- a/t/t2071-restore-patch.sh
> > +++ b/t/t2071-restore-patch.sh
> > @@ -44,13 +44,17 @@ test_expect_success PERL 'git restore -p with staged changes' '
>
> It is a pre-existing problem but all these "PERL" prerequisites are
> no-longer required as we've removed the perl implementation of "add -p"
I can send a separate patch to clean up this script, removing PERL
pre-req from all tests.
> > verify_state dir/foo index index
> > '
> >
> > -test_expect_success PERL 'git restore -p --source=HEAD' '
> > - set_state dir/foo work index &&
> > - # the third n is to get out in case it mistakenly does not apply
> > - test_write_lines n y n | git restore -p --source=HEAD &&
> > - verify_saved_state bar &&
> > - verify_state dir/foo head index
> > -'
> > +for opt in "HEAD" "@"
> > +do
> > + test_expect_success PERL "git restore -p --source=$opt" '
> > + set_state dir/foo work index &&
> > + # the third n is to get out in case it mistakenly does not apply
> > + test_write_lines n y n | git restore -p --source=$opt >output &&
> > + verify_saved_state bar &&
> > + verify_state dir/foo head index &&
> > + test_grep "Discard" output
>
> It is good to see that we're now testing for a reversed patch here.
>
> Best Wishes
>
> Phillip
Thanks for the review.
^ permalink raw reply
* [PATCH 0/4] Speed up git-notes show
From: Maarten Bosmans @ 2024-02-05 20:49 UTC (permalink / raw)
To: git; +Cc: Teng Long
First time contributor here, trying my first git.git patch series.
BACKGROUND
We have a script that runs a range of build tests for all new commits in the
repository and adds a line to the commit note with the result from the test.
Something along the lines of:
occa-build-jit-gnu-cuda-develop: PASSED (<hostname>, 2024-01-01 00:00:00+01:00)
Pretty useful to quickly check that all commits at least build, not only for
master, but also in progress feature branches. (a passing test suite is
generally only required at the merge point)
PROBLEM
The bash script loops over all remote refs and lists the commits newer than
<N> days ago. For each commit its note is read and grep'ed for an existing
test name to see whether the build test needs to run again. The `git note show`
command that is in this loop nest only takes 14ms to execute, but as it is in
a loop, those times add up.
ANALYSIS
When asked to show a note for a specific commit, git looks up the blob hash
for the note and executes `git show` with that hash. That of course adds
the child process overhead, but also causes the initialization of a lot of
log related configuration, such as for decorations or the mailmap. Simply
outputting the blob directly in the main process reduces the run time by
almost halve.
When looking through the git show implementation for useful stuff that command
does that should also be done when showing a note, I could only find the
`setup_pager()` call. All other git show functionality was related to showing
commits or other non-blob objects.
The only thing I was not 100% sure of was the textconv_object stuff. From what
I could deduce was that this is only ever used on blobs that represent files in
a tree, not on blobs that represent note objects. So I did not include any
textconv calls in the git notes code.
The first commit is the main one fixing performance. The other three are just
eliminating some overhead I noticed when going through the git notes code.
Maarten Bosmans (4):
notes: print note blob to stdout directly
notes: use exisisting function stream_blob_to_fd
notes: do not clean up right before calling die()
notes: use strbuf_attach to take ownership of the object contents
builtin/notes.c | 37 ++++++++++---------------------------
1 file changed, 10 insertions(+), 27 deletions(-)
--
2.35.3
^ permalink raw reply
* [PATCH 2/4] notes: use exisisting function stream_blob_to_fd
From: Maarten Bosmans @ 2024-02-05 20:49 UTC (permalink / raw)
To: git; +Cc: Teng Long, Maarten Bosmans
In-Reply-To: <20240205204932.16653-1-maarten.bosmans@vortech.nl>
From: Maarten Bosmans <mkbosmans@gmail.com>
From: Maarten Bosmans <maarten.bosmans@vortech.nl>
Use functionality from streaming.c and remove the copy_obj_to_fd()
function local to notes.c.
Streaming the blob to stdout instead of copying through an
intermediate buffer might also be more efficient, but at the
size a typical note is, this is unlikely to matter a lot.
Signed-off-by: Maarten Bosmans <maarten.bosmans@vortech.nl>
---
builtin/notes.c | 14 +-------------
1 file changed, 1 insertion(+), 13 deletions(-)
diff --git a/builtin/notes.c b/builtin/notes.c
index 8efe9809b2..048b8c559c 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -149,18 +149,6 @@ static int list_each_note(const struct object_id *object_oid,
return 0;
}
-static void copy_obj_to_fd(int fd, const struct object_id *oid)
-{
- unsigned long size;
- enum object_type type;
- char *buf = repo_read_object_file(the_repository, oid, &type, &size);
- if (buf) {
- if (size)
- write_or_die(fd, buf, size);
- free(buf);
- }
-}
-
static void write_commented_object(int fd, const struct object_id *object)
{
struct child_process show = CHILD_PROCESS_INIT;
@@ -205,7 +193,7 @@ static void prepare_note_data(const struct object_id *object, struct note_data *
if (d->given)
write_or_die(fd, d->buf.buf, d->buf.len);
else if (old_note)
- copy_obj_to_fd(fd, old_note);
+ stream_blob_to_fd(fd, old_note, NULL, 0);
strbuf_addch(&buf, '\n');
strbuf_add_commented_lines(&buf, "\n", strlen("\n"), comment_line_char);
--
2.35.3
^ permalink raw reply related
* [PATCH 1/4] notes: print note blob to stdout directly
From: Maarten Bosmans @ 2024-02-05 20:49 UTC (permalink / raw)
To: git; +Cc: Teng Long, Maarten Bosmans
In-Reply-To: <20240205204932.16653-1-maarten.bosmans@vortech.nl>
From: Maarten Bosmans <mkbosmans@gmail.com>
From: Maarten Bosmans <maarten.bosmans@vortech.nl>
Avoid the need to launch a subprocess by calling stream_blob_to_fd
directly. This does not only get rid of the overhead of a separate
child process, but also avoids the initalization of the whole log
machinery that `git show` does. That is needed for example to show
decorated commits and applying the mailmap. For simply displaying
a blob however, the only useful thing show does is enabling the pager.
Signed-off-by: Maarten Bosmans <maarten.bosmans@vortech.nl>
---
builtin/notes.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/builtin/notes.c b/builtin/notes.c
index e65cae0bcf..8efe9809b2 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -20,7 +20,8 @@
#include "repository.h"
#include "pretty.h"
#include "refs.h"
-#include "exec-cmd.h"
+#include "pager.h"
+#include "streaming.h"
#include "run-command.h"
#include "parse-options.h"
#include "string-list.h"
@@ -751,7 +752,7 @@ static int show(int argc, const char **argv, const char *prefix)
struct notes_tree *t;
struct object_id object;
const struct object_id *note;
- int retval;
+ int retval = 0;
struct option options[] = {
OPT_END()
};
@@ -776,8 +777,9 @@ static int show(int argc, const char **argv, const char *prefix)
retval = error(_("no note found for object %s."),
oid_to_hex(&object));
else {
- const char *show_args[3] = {"show", oid_to_hex(note), NULL};
- retval = execv_git_cmd(show_args);
+ setup_pager();
+ if (stream_blob_to_fd(1, note, NULL, 0))
+ die(_("object %s is not a blob"), oid_to_hex(note));
}
free_notes(t);
return retval;
--
2.35.3
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox