* Re: [PATCH v4 03/11] commit-reach(repo_in_merge_bases_many): report missing commits
From: Jeff King @ 2024-03-01 6:58 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Patrick Steinhardt, Dirk Gouders, Johannes Schindelin
In-Reply-To: <1938b317a49f4d688bfefd7e5a74ad750a55a91a.1709113458.git.gitgitgadget@gmail.com>
On Wed, Feb 28, 2024 at 09:44:09AM +0000, Johannes Schindelin via GitGitGadget wrote:
> @@ -1402,6 +1436,8 @@ static int merge_mode_and_contents(struct merge_options *opt,
> &o->oid,
> &a->oid,
> &b->oid);
> + if (result->clean < 0)
> + return -1;
Sorry, I accidentally commented on v2 of your series a moment ago,
rather than the most recent version. But this hunk was untouched between
the two, so the comment still applies:
https://lore.kernel.org/git/20240301065647.GA2680308@coredump.intra.peff.net/
-Peff
^ permalink raw reply
* Re: [PATCH v2 03/11] Start reporting missing commits in `repo_in_merge_bases_many()`
From: Jeff King @ 2024-03-01 6:56 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Patrick Steinhardt, Johannes Schindelin
In-Reply-To: <4dd214f91d4783f29b03908cc0034156253889a7.1708608110.git.gitgitgadget@gmail.com>
On Thu, Feb 22, 2024 at 01:21:42PM +0000, Johannes Schindelin via GitGitGadget wrote:
> @@ -1402,6 +1436,8 @@ static int merge_mode_and_contents(struct merge_options *opt,
> &o->oid,
> &a->oid,
> &b->oid);
> + if (result->clean < 0)
> + return -1;
Coverity flagged this code as NO_EFFECT. The issue is that result->clean
is an unsigned single-bit boolean, so it can never be negative. If we
expand the context a bit, it's
result->clean = merge_submodule(opt, &result->blob.oid,
o->path,
&o->oid,
&a->oid,
&b->oid);
if (result->clean < 0)
return -1;
So it's possible there's also a problem in the existing assignment, as a
negative return from merge_submodule() would be truncated. But from my
read of it, it will always return either 0 or 1 (if it sees errors from
repo_in_merge_bases(), for example, it will output an error message and
return 0).
So I think we'd want either:
1. Drop this hunk, and let result->clean stay as a pure boolean.
2. Propagate errors from merge_submodule() using -1, in which case the
code needs to be more like:
int ret = merge_submodule(...);
if (ret < 0)
return -1;
result->clean = !!ret;
I didn't follow the series closely enough to know which would be better.
I guess alternatively it could be a tri-state that carries the error
around (it looks like it is in ort's "struct merge_result"), but that
probably means auditing all of the spots that look at "clean" to see
what they would do with a negative value.
-Peff
^ permalink raw reply
* Re: [PATCH 1/1] t9117: prefer test_path_* helper functions
From: Junio C Hamano @ 2024-03-01 5:09 UTC (permalink / raw)
To: shejialuo; +Cc: git, Eric Sunshine
In-Reply-To: <20240301034606.69673-2-shejialuo@gmail.com>
shejialuo <shejialuo@gmail.com> writes:
> test_expect_success 'basic clone' '
> - test ! -d trunk &&
> + ! test_path_is_dir trunk &&
This is not quite right. Step back and think why we are trying to
use the test_path_* helpers instead of "test [!] -d". What are the
differences between them?
The answer is that, unlike "test [!] -d dir" that is silent whether
"dir" exists or missing, "test_path_is_dir dir" is *not* always
silent. It gives useful messages as necessary. When does it do so?
Here is the definition, from t/test-lib-functions.sh around line
930:
test_path_is_dir () {
test "$#" -ne 1 && BUG "1 param"
if ! test -d "$1"
then
echo "Directory $1 doesn't exist"
false
fi
}
It succeeds silently when "test -d dir" is true, but it complains
loudly when "test -d dir" does not hold. You will be told that the
test is unhappy because "dir" does not exist. That would be easier
to debug than one step among many in &&-chain silently fails.
Now, let's look at the original you rewrote again:
> - test ! -d trunk &&
It says "it is a failure if 'trunk' exists as a directory". If
'trunk' does not exist, it is a very happy state for us. So instead
of silently failing when 'trunk' exists as a directory, you would
want to improve it so that you will get a complaint in such a case,
saying "trunk should *not* exist but it does".
Did you succeed to do so with this rewrite?
> + ! test_path_is_dir trunk &&
The helper "test_path_is_dir" is called with "trunk". As we saw, we
will see complaint when "trunk" does *NOT* exist. When "trunk" does
exist, it will be silent and "test_path_is_dir" will return a success,
which will be inverted with "!" to make it a failure, causing &&-chain
to fail.
So the exit status is not wrong, but it issues a complaint under the
wrong condition. That is not an improvement.
Let's step back one more time. Is the original test happy when
"trunk" existed as a regular file? "test ! -d trunk" says so, but
should it really be? Think.
I suspect that the test is not happy as long as 'trunk' exists,
whether it is a directory or a regular file or a symbolic link.
IOW, it says "I am unhappy if 'trunk' is a directory", but what it
really meant to say was "I am unhappy if there is anything at the
path 'trunk'". IOW, "test ! -e trunk" would be what it really
meant, no?
So the correct rewrite for it would rather be something like
test_path_is_missing trunk &&
instead. This will fail if anything is at path 'trunk', with an
error message saying there shouldn't be anything but there is.
In a peculiar case, which I do not think this one is, a test may
legitimately accept "path" to either (1) exist as long as it is not
a directory, or (2) be missing, as success. In such a case, the
original construct '! test -d path" (or "test ! -d path") would be
appropriate.
But I do not think we have a suitable wrapper to express such a
case, i.e. we do not have a helper like this.
test_path_is_not_dir () {
if test -d "$1"
then
echo "$1 is a directory but it should not be"
false
fi
}
If such a use case were common, we might even do this:
# "test_path_is_dir <dir>" expects <dir> to be a directory.
# "test_path_is_dir ! <dir>" expects <dir> not to be a
# directory.
# In either case, complain only when the expectation is not met.
test_path_is_dir () {
if test "$1" = "!"
then
shift
if test -d "$1"
then
echo "$1 is a directory but it should not be"
return 1
fi
else
if test ! -d "$1"
then
echo "$1 is not a directory"
return 1
fi
fi
true
}
but "we are happy even if path exists as long as it is not a
directory" is a very uncommon thing we want to say in our tests, so
that is why we do not have such a helper function.
HTH.
^ permalink raw reply
* Re: [PATCH 1/1] t9117: prefer test_path_* helper functions
From: Eric Sunshine @ 2024-03-01 4:44 UTC (permalink / raw)
To: shejialuo; +Cc: git, Junio C Hamano
In-Reply-To: <20240301034606.69673-2-shejialuo@gmail.com>
On Thu, Feb 29, 2024 at 10:46 PM shejialuo <shejialuo@gmail.com> wrote:
> test -(e|f|d) does not provide a nice error message when we hit test
> failures, so use test_path_exists, test_path_is_dir and
> test_path_is_file instead.
Thanks for rerolling. t9117 is indeed a better choice[1] than t3070
for the exercise of replacing `test -blah` with `test_path_foo`.
[1]: https://lore.kernel.org/git/CAPig+cR2-6qONkosu7=qEQSJa_fvYuVQ0to47D5qx904zW08Eg@mail.gmail.com/
> Signed-off-by: shejialuo <shejialuo@gmail.com>
> ---
> diff --git a/t/t9117-git-svn-init-clone.sh b/t/t9117-git-svn-init-clone.sh
> @@ -15,39 +15,39 @@ test_expect_success 'setup svnrepo' '
> test_expect_success 'basic clone' '
> - test ! -d trunk &&
> + ! test_path_is_dir trunk &&
Generally speaking, you don't want to use `!` to negate the result of
a `path_is_foo` assertion function. To understand why, take a look at
the definition of `test_path_is_dir`:
test_path_is_dir () {
if ! test -d "$1"
then
echo "Directory $1 doesn't exist"
false
fi
}
The test in question (t9117: "basic clone") is using `test ! -d` to
assert that the directory `trunk` does not yet exist when the test
begins; indeed, under normal circumstances, this directory should not
yet be present. However, the call to test_path_is_dir() asserts that
the directory _does_ exist, which is the opposite of `test ! -d`, and
complains ("Directory trunk doesn't exist") when it doesn't exist. So,
in the normal and typical case for all the tests in this script,
`test_path_is_dir` is going to be complaining even though the
non-existence of that directory is an expected condition.
Although you make the test pass by using `!` to invert the result of
`test_path_is_dir`, the complaint will nevertheless get lodged, and
may very well be confusing for anyone scrutinizing the output of the
tests when running the script with `-v` or `-x`.
So, `test_path_is_dir` is not a good fit for this case which wants to
assert that the path `trunk` does not yet exist. A better choice for
this particular case would be `test_path_is_missing`.
> git svn clone "$svnrepo"/project/trunk &&
> - test -d trunk/.git/svn &&
> - test -e trunk/foo &&
> + test_path_is_dir trunk/.git/svn &&
> + test_path_exists trunk/foo &&
These two changes make sense and the intent directly corresponds to
the original code.
> test_expect_success 'clone to target directory' '
> - test ! -d target &&
> + ! test_path_is_dir target &&
> git svn clone "$svnrepo"/project/trunk target &&
> - test -d target/.git/svn &&
> - test -e target/foo &&
> + test_path_is_dir target/.git/svn &&
> + test_path_exists target/foo &&
> rm -rf target
> '
What follows is probably beyond the scope of your GSoC microproject,
but there is a bit more of interest to note about these tests.
Rather than asserting some initial condition at the start of the test,
it is more common and more robust simply to _ensure_ that the desired
initial condition holds. So, for instance, instead of asserting `test
! -d target`, modern practice is to ensure that `target` doesn't
exist. Thus:
test_expect_success 'clone to target directory' '
rm -rf target &&
git svn clone "$svnrepo"/project/trunk target &&
...
is a more robust implementation. This also addresses the problem that
the `rm -rf target` at the very end of each test won't be executed if
any command earlier in the test fails (due to the short-circuiting
behavior of the &&-operator).
As noted, this type of cleanup is probably overkill for your GSoC
microproject so you need not tackle it. I mention it only for
completeness. Also, if someone does tackle such a cleanup, it should
be done as multiple patches, each making one distinct change (i.e. one
patch dropping `test !-d` and moving `rm -rf` to the start of the
test, and one which employs `test_path_foo` for the remaining `test
-blah` invocations).
^ permalink raw reply
* [PATCH 1/1] t9117: prefer test_path_* helper functions
From: shejialuo @ 2024-03-01 3:46 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Junio C Hamano, shejialuo
In-Reply-To: <20240301034606.69673-1-shejialuo@gmail.com>
test -(e|f|d) does not provide a nice error message when we hit test
failures, so use test_path_exists, test_path_is_dir and
test_path_is_file instead.
Signed-off-by: shejialuo <shejialuo@gmail.com>
---
t/t9117-git-svn-init-clone.sh | 40 +++++++++++++++++------------------
1 file changed, 20 insertions(+), 20 deletions(-)
diff --git a/t/t9117-git-svn-init-clone.sh b/t/t9117-git-svn-init-clone.sh
index 62de819a44..2f964f66aa 100755
--- a/t/t9117-git-svn-init-clone.sh
+++ b/t/t9117-git-svn-init-clone.sh
@@ -15,39 +15,39 @@ test_expect_success 'setup svnrepo' '
'
test_expect_success 'basic clone' '
- test ! -d trunk &&
+ ! test_path_is_dir trunk &&
git svn clone "$svnrepo"/project/trunk &&
- test -d trunk/.git/svn &&
- test -e trunk/foo &&
+ test_path_is_dir trunk/.git/svn &&
+ test_path_exists trunk/foo &&
rm -rf trunk
'
test_expect_success 'clone to target directory' '
- test ! -d target &&
+ ! test_path_is_dir target &&
git svn clone "$svnrepo"/project/trunk target &&
- test -d target/.git/svn &&
- test -e target/foo &&
+ test_path_is_dir target/.git/svn &&
+ test_path_exists target/foo &&
rm -rf target
'
test_expect_success 'clone with --stdlayout' '
- test ! -d project &&
+ ! test_path_is_dir project &&
git svn clone -s "$svnrepo"/project &&
- test -d project/.git/svn &&
- test -e project/foo &&
+ test_path_is_dir project/.git/svn &&
+ test_path_exists project/foo &&
rm -rf project
'
test_expect_success 'clone to target directory with --stdlayout' '
- test ! -d target &&
+ ! test_path_is_dir target &&
git svn clone -s "$svnrepo"/project target &&
- test -d target/.git/svn &&
- test -e target/foo &&
+ test_path_is_dir target/.git/svn &&
+ test_path_exists target/foo &&
rm -rf target
'
test_expect_success 'init without -s/-T/-b/-t does not warn' '
- test ! -d trunk &&
+ ! test_path_is_dir trunk &&
git svn init "$svnrepo"/project/trunk trunk 2>warning &&
! grep -q prefix warning &&
rm -rf trunk &&
@@ -55,7 +55,7 @@ test_expect_success 'init without -s/-T/-b/-t does not warn' '
'
test_expect_success 'clone without -s/-T/-b/-t does not warn' '
- test ! -d trunk &&
+ ! test_path_is_dir trunk &&
git svn clone "$svnrepo"/project/trunk 2>warning &&
! grep -q prefix warning &&
rm -rf trunk &&
@@ -69,7 +69,7 @@ project/trunk:refs/remotes/${prefix}trunk
project/branches/*:refs/remotes/${prefix}*
project/tags/*:refs/remotes/${prefix}tags/*
EOF
- test ! -f actual &&
+ ! test_path_is_file actual &&
git --git-dir=project/.git config svn-remote.svn.fetch >>actual &&
git --git-dir=project/.git config svn-remote.svn.branches >>actual &&
git --git-dir=project/.git config svn-remote.svn.tags >>actual &&
@@ -78,7 +78,7 @@ EOF
}
test_expect_success 'init with -s/-T/-b/-t assumes --prefix=origin/' '
- test ! -d project &&
+ ! test_path_is_dir project &&
git svn init -s "$svnrepo"/project project 2>warning &&
! grep -q prefix warning &&
test_svn_configured_prefix "origin/" &&
@@ -87,7 +87,7 @@ test_expect_success 'init with -s/-T/-b/-t assumes --prefix=origin/' '
'
test_expect_success 'clone with -s/-T/-b/-t assumes --prefix=origin/' '
- test ! -d project &&
+ ! test_path_is_dir project &&
git svn clone -s "$svnrepo"/project 2>warning &&
! grep -q prefix warning &&
test_svn_configured_prefix "origin/" &&
@@ -96,7 +96,7 @@ test_expect_success 'clone with -s/-T/-b/-t assumes --prefix=origin/' '
'
test_expect_success 'init with -s/-T/-b/-t and --prefix "" still works' '
- test ! -d project &&
+ ! test_path_is_dir project &&
git svn init -s "$svnrepo"/project project --prefix "" 2>warning &&
! grep -q prefix warning &&
test_svn_configured_prefix "" &&
@@ -105,7 +105,7 @@ test_expect_success 'init with -s/-T/-b/-t and --prefix "" still works' '
'
test_expect_success 'clone with -s/-T/-b/-t and --prefix "" still works' '
- test ! -d project &&
+ ! test_path_is_dir project &&
git svn clone -s "$svnrepo"/project --prefix "" 2>warning &&
! grep -q prefix warning &&
test_svn_configured_prefix "" &&
@@ -114,7 +114,7 @@ test_expect_success 'clone with -s/-T/-b/-t and --prefix "" still works' '
'
test_expect_success 'init with -T as a full url works' '
- test ! -d project &&
+ ! test_path_is_dir project &&
git svn init -T "$svnrepo"/project/trunk project &&
rm -rf project
'
--
2.44.0
^ permalink raw reply related
* [PATCH V2 0/1] [GSoC][PATCH] t9117: prefer test_path_* helper functions
From: shejialuo @ 2024-03-01 3:46 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Junio C Hamano, shejialuo
In-Reply-To: <20240229150442.490649-1-shejialuo@gmail.com>
As discussed, the original patch is unsutiable, t9117 is a good
candidate script.
shejialuo (1):
t9117: prefer test_path_* helper functions
t/t9117-git-svn-init-clone.sh | 40 +++++++++++++++++------------------
1 file changed, 20 insertions(+), 20 deletions(-)
base-commit: 0f9d4d28b7e6021b7e6db192b7bf47bd3a0d0d1d
--
2.44.0
^ permalink raw reply
* Re: [PATCH 1/1] [GSoC][PATCH] t3070: refactor test -e command
From: shejialuo @ 2024-03-01 2:50 UTC (permalink / raw)
To: sunshine; +Cc: git
In-Reply-To: <CAPig+cR2-6qONkosu7=qEQSJa_fvYuVQ0to47D5qx904zW08Eg@mail.gmail.com>
Thanks for your comment, I will find a candidate script later and submit
a new version patch.
^ permalink raw reply
* Re: [PATCH v2] tests: modernize the test script t0010-racy-git.sh
From: Junio C Hamano @ 2024-03-01 2:03 UTC (permalink / raw)
To: Eric Sunshine
Cc: Aryan Gupta via GitGitGadget, git, Patrick Steinhardt,
Michal Suchánek, Jean-Noël AVILA, Kristoffer Haugsbakk,
Aryan Gupta
In-Reply-To: <CAPig+cQ+JNBwydUq0CsTZGs8mHs3L3fJDuSosd+-WdKwWWw=gg@mail.gmail.com>
Eric Sunshine <sunshine@sunshineco.com> writes:
> The double-negative confused me even when suggesting a replacement.
> What I meant was that a better phrasing would perhaps have been:
>
> 'foo' is empty but should not be
+1. Thanks for caring.
^ permalink raw reply
* Re: What's cooking in git.git (Feb 2024, #09; Tue, 27)
From: Junio C Hamano @ 2024-03-01 2:02 UTC (permalink / raw)
To: Linus Arver; +Cc: git
In-Reply-To: <owlyil264yew.fsf@fine.c.googlers.com>
Linus Arver <linusa@google.com> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> [...]
>>
>> * la/trailer-api (2024-02-16) 9 commits
>> (merged to 'next' on 2024-02-21 at 631e28bbbc)
>> + format_trailers_from_commit(): indirectly call trailer_info_get()
>> ...
>> Will merge to 'master'.
>> source: <pull.1632.v5.git.1708124950.gitgitgadget@gmail.com>
>
> Doh, please wait for my v6 reroll (will send to the list in the next
> half hour) to clean up the commit messages. Thanks.
If we had communication gap and the topic was prematurely merged and
was caught within a day or two, I would be a bit more sympathetic,
but given that this was merged more than a week ago, that's totally
unacceptable.
If you have improvements, please do so as incremental patches on
top. I'll hold the topic in 'next' until we are ready.
Thanks.
^ permalink raw reply
* Re: [PATCH 1/2] refs/reftable: don't fail empty transactions in repo without HEAD
From: Justin Tobler @ 2024-03-01 1:20 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Junio C Hamano, Mike Hommey
In-Reply-To: <95be968e10bd02c64448786e690bbefe5c082577.1709041721.git.ps@pks.im>
On 24/02/27 03:27PM, Patrick Steinhardt wrote:
> Under normal circumstances, it shouldn't ever happen that a repository
> has no HEAD reference. In fact, git-update-ref(1) would fail any request
> to delete the HEAD reference, and a newly initialized repository always
> pre-creates it, too.
>
> But in the next commit, we are going to change git-clone(1) to partially
> initialize the refdb just up to the point where remote helpers can find
> the repository. With that change, we are going to run into a situation
> where repositories have no refs at all.
>
> Now there is a very particular edge case in this situation: when
> preparing an empty ref transacton, we end up returning whatever value
> `read_ref_without_reload()` returned to the caller. Under normal
> conditions this would be fine: "HEAD" should usually exist, and thus the
> function would return `0`. But if "HEAD" doesn't exist, the function
> returns a positive value which we end up returning to the caller.
In what context are reference transactions being created before the
refdb is fully initialized? Is this in regards to repositories
initialized with the reftables backend and used during execution of a
remote-helper? I was originally under the impression that a partially
initalized refdb would appear as the reffile backend before being fully
initialized.
>
> Fix this bug by resetting the return code to `0` and add a test.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
-Justin
^ permalink raw reply
* [PATCH v6 9/9] format_trailers_from_commit(): indirectly call trailer_info_get()
From: Linus Arver via GitGitGadget @ 2024-03-01 0:14 UTC (permalink / raw)
To: git
Cc: Christian Couder, Junio C Hamano, Emily Shaffer, Josh Steadmon,
Randall S. Becker, Christian Couder, Kristoffer Haugsbakk,
Linus Arver, Linus Arver
In-Reply-To: <pull.1632.v6.git.1709252086.gitgitgadget@gmail.com>
From: Linus Arver <linusa@google.com>
This is another preparatory refactor to unify the trailer formatters.
For background, note that the "trailers" string array is the
`char **trailers` member in `struct trailer_info` and that the
trailer_item objects are the elements of the `struct list_head *head`
linked list.
Currently trailer_info_get() only populates `char **trailers`. And
parse_trailers() first calls trailer_info_get() so that it can use the
`char **trailers` to populate a list of `struct trailer_item` objects
Instead of calling trailer_info_get() directly from
format_trailers_from_commit(), make it call parse_trailers() instead
because parse_trailers() already calls trailer_info_get().
This change is a NOP because format_trailer_info() (which
format_trailers_from_commit() wraps around) only looks at the "trailers"
string array, not the trailer_item objects which parse_trailers()
populates. For now we do need to create a dummy
LIST_HEAD(trailer_objects);
because parse_trailers() expects it in its signature.
In a future patch, we'll change format_trailer_info() to use the parsed
trailer_item objects (trailer_objects) instead of the `char **trailers`
array.
Signed-off-by: Linus Arver <linusa@google.com>
---
trailer.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/trailer.c b/trailer.c
index e92d0154d90..798388cbf29 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1140,9 +1140,11 @@ void format_trailers_from_commit(const struct process_trailer_options *opts,
const char *msg,
struct strbuf *out)
{
+ LIST_HEAD(trailer_objects);
struct trailer_info info;
- trailer_info_get(opts, msg, &info);
+ parse_trailers(opts, &info, msg, &trailer_objects);
+
/* If we want the whole block untouched, we can take the fast path. */
if (!opts->only_trailers && !opts->unfold && !opts->filter &&
!opts->separator && !opts->key_only && !opts->value_only &&
@@ -1152,6 +1154,7 @@ void format_trailers_from_commit(const struct process_trailer_options *opts,
} else
format_trailer_info(opts, &info, out);
+ free_trailers(&trailer_objects);
trailer_info_release(&info);
}
--
gitgitgadget
^ permalink raw reply related
* [PATCH v6 8/9] format_trailer_info(): move "fast path" to caller
From: Linus Arver via GitGitGadget @ 2024-03-01 0:14 UTC (permalink / raw)
To: git
Cc: Christian Couder, Junio C Hamano, Emily Shaffer, Josh Steadmon,
Randall S. Becker, Christian Couder, Kristoffer Haugsbakk,
Linus Arver, Linus Arver
In-Reply-To: <pull.1632.v6.git.1709252086.gitgitgadget@gmail.com>
From: Linus Arver <linusa@google.com>
This is another preparatory refactor to unify the trailer formatters.
This allows us to drop the "msg" parameter from format_trailer_info(),
so that it take 3 parameters, similar to format_trailers() which also
takes 3 parameters:
void format_trailers(const struct process_trailer_options *opts,
struct list_head *trailers,
struct strbuf *out)
The short-term goal is to make format_trailer_info() be smart enough to
deprecate format_trailers(). And then ultimately we will rename
format_trailer_info() to format_trailers().
Signed-off-by: Linus Arver <linusa@google.com>
---
trailer.c | 20 +++++++++-----------
1 file changed, 9 insertions(+), 11 deletions(-)
diff --git a/trailer.c b/trailer.c
index cbd643cd1fe..e92d0154d90 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1087,21 +1087,11 @@ void trailer_info_release(struct trailer_info *info)
static void format_trailer_info(const struct process_trailer_options *opts,
const struct trailer_info *info,
- const char *msg,
struct strbuf *out)
{
size_t origlen = out->len;
size_t i;
- /* If we want the whole block untouched, we can take the fast path. */
- if (!opts->only_trailers && !opts->unfold && !opts->filter &&
- !opts->separator && !opts->key_only && !opts->value_only &&
- !opts->key_value_separator) {
- strbuf_add(out, msg + info->trailer_block_start,
- info->trailer_block_end - info->trailer_block_start);
- return;
- }
-
for (i = 0; i < info->trailer_nr; i++) {
char *trailer = info->trailers[i];
ssize_t separator_pos = find_separator(trailer, separators);
@@ -1153,7 +1143,15 @@ void format_trailers_from_commit(const struct process_trailer_options *opts,
struct trailer_info info;
trailer_info_get(opts, msg, &info);
- format_trailer_info(opts, &info, msg, out);
+ /* If we want the whole block untouched, we can take the fast path. */
+ if (!opts->only_trailers && !opts->unfold && !opts->filter &&
+ !opts->separator && !opts->key_only && !opts->value_only &&
+ !opts->key_value_separator) {
+ strbuf_add(out, msg + info.trailer_block_start,
+ info.trailer_block_end - info.trailer_block_start);
+ } else
+ format_trailer_info(opts, &info, out);
+
trailer_info_release(&info);
}
--
gitgitgadget
^ permalink raw reply related
* [PATCH v6 7/9] format_trailers(): use strbuf instead of FILE
From: Linus Arver via GitGitGadget @ 2024-03-01 0:14 UTC (permalink / raw)
To: git
Cc: Christian Couder, Junio C Hamano, Emily Shaffer, Josh Steadmon,
Randall S. Becker, Christian Couder, Kristoffer Haugsbakk,
Linus Arver, Linus Arver
In-Reply-To: <pull.1632.v6.git.1709252086.gitgitgadget@gmail.com>
From: Linus Arver <linusa@google.com>
This is another preparatory refactor to unify the trailer formatters.
Make format_trailers() also write to a strbuf, to align with
format_trailers_from_commit() which also does the same. Doing this makes
format_trailers() behave similar to format_trailer_info() (which will
soon help us replace one with the other).
Signed-off-by: Linus Arver <linusa@google.com>
---
builtin/interpret-trailers.c | 6 +++++-
trailer.c | 13 +++++++------
trailer.h | 3 ++-
3 files changed, 14 insertions(+), 8 deletions(-)
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index d1cf0aa33a2..11f4ce9e4a2 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -140,6 +140,7 @@ static void interpret_trailers(const struct process_trailer_options *opts,
{
LIST_HEAD(head);
struct strbuf sb = STRBUF_INIT;
+ struct strbuf trailer_block = STRBUF_INIT;
struct trailer_info info;
FILE *outfile = stdout;
@@ -169,8 +170,11 @@ static void interpret_trailers(const struct process_trailer_options *opts,
process_trailers_lists(&head, &arg_head);
}
- format_trailers(opts, &head, outfile);
+ /* Print trailer block. */
+ format_trailers(opts, &head, &trailer_block);
free_trailers(&head);
+ fwrite(trailer_block.buf, 1, trailer_block.len, outfile);
+ strbuf_release(&trailer_block);
/* Print the lines after the trailers as is */
if (!opts->only_trailers)
diff --git a/trailer.c b/trailer.c
index f92d844361a..cbd643cd1fe 100644
--- a/trailer.c
+++ b/trailer.c
@@ -144,12 +144,12 @@ static char last_non_space_char(const char *s)
return '\0';
}
-static void print_tok_val(FILE *outfile, const char *tok, const char *val)
+static void print_tok_val(struct strbuf *out, const char *tok, const char *val)
{
char c;
if (!tok) {
- fprintf(outfile, "%s\n", val);
+ strbuf_addf(out, "%s\n", val);
return;
}
@@ -157,13 +157,14 @@ static void print_tok_val(FILE *outfile, const char *tok, const char *val)
if (!c)
return;
if (strchr(separators, c))
- fprintf(outfile, "%s%s\n", tok, val);
+ strbuf_addf(out, "%s%s\n", tok, val);
else
- fprintf(outfile, "%s%c %s\n", tok, separators[0], val);
+ strbuf_addf(out, "%s%c %s\n", tok, separators[0], val);
}
void format_trailers(const struct process_trailer_options *opts,
- struct list_head *trailers, FILE *outfile)
+ struct list_head *trailers,
+ struct strbuf *out)
{
struct list_head *pos;
struct trailer_item *item;
@@ -171,7 +172,7 @@ void format_trailers(const struct process_trailer_options *opts,
item = list_entry(pos, struct trailer_item, list);
if ((!opts->trim_empty || strlen(item->value) > 0) &&
(!opts->only_trailers || item->token))
- print_tok_val(outfile, item->token, item->value);
+ print_tok_val(out, item->token, item->value);
}
}
diff --git a/trailer.h b/trailer.h
index 410c61b62be..1d106b6dd40 100644
--- a/trailer.h
+++ b/trailer.h
@@ -102,7 +102,8 @@ void trailer_info_release(struct trailer_info *info);
void trailer_config_init(void);
void format_trailers(const struct process_trailer_options *,
- struct list_head *trailers, FILE *outfile);
+ struct list_head *trailers,
+ struct strbuf *out);
void free_trailers(struct list_head *);
/*
--
gitgitgadget
^ permalink raw reply related
* [PATCH v6 6/9] trailer_info_get(): reorder parameters
From: Linus Arver via GitGitGadget @ 2024-03-01 0:14 UTC (permalink / raw)
To: git
Cc: Christian Couder, Junio C Hamano, Emily Shaffer, Josh Steadmon,
Randall S. Becker, Christian Couder, Kristoffer Haugsbakk,
Linus Arver, Linus Arver
In-Reply-To: <pull.1632.v6.git.1709252086.gitgitgadget@gmail.com>
From: Linus Arver <linusa@google.com>
This is another preparatory refactor to unify the trailer formatters.
Take
const struct process_trailer_options *opts
as the first parameter, because these options are required for
parsing trailers (e.g., whether to treat "---" as the end of the log
message). And take
struct trailer_info *info
last, because it's an "out parameter" (something that the caller wants
to use as the output of this function).
Signed-off-by: Linus Arver <linusa@google.com>
---
sequencer.c | 2 +-
trailer.c | 11 ++++++-----
trailer.h | 5 +++--
3 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 3cc88d8a800..8e199fc8a47 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -332,7 +332,7 @@ static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,
sb->buf[sb->len - ignore_footer] = '\0';
}
- trailer_info_get(&info, sb->buf, &opts);
+ trailer_info_get(&opts, sb->buf, &info);
if (ignore_footer)
sb->buf[sb->len - ignore_footer] = saved_char;
diff --git a/trailer.c b/trailer.c
index 5025be97899..f92d844361a 100644
--- a/trailer.c
+++ b/trailer.c
@@ -997,7 +997,7 @@ void parse_trailers(const struct process_trailer_options *opts,
struct strbuf val = STRBUF_INIT;
size_t i;
- trailer_info_get(info, str, opts);
+ trailer_info_get(opts, str, info);
for (i = 0; i < info->trailer_nr; i++) {
int separator_pos;
@@ -1032,8 +1032,9 @@ void free_trailers(struct list_head *trailers)
}
}
-void trailer_info_get(struct trailer_info *info, const char *str,
- const struct process_trailer_options *opts)
+void trailer_info_get(const struct process_trailer_options *opts,
+ const char *str,
+ struct trailer_info *info)
{
size_t end_of_log_message = 0, trailer_block_start = 0;
struct strbuf **trailer_lines, **ptr;
@@ -1150,7 +1151,7 @@ void format_trailers_from_commit(const struct process_trailer_options *opts,
{
struct trailer_info info;
- trailer_info_get(&info, msg, opts);
+ trailer_info_get(opts, msg, &info);
format_trailer_info(opts, &info, msg, out);
trailer_info_release(&info);
}
@@ -1161,7 +1162,7 @@ void trailer_iterator_init(struct trailer_iterator *iter, const char *msg)
strbuf_init(&iter->key, 0);
strbuf_init(&iter->val, 0);
opts.no_divider = 1;
- trailer_info_get(&iter->internal.info, msg, &opts);
+ trailer_info_get(&opts, msg, &iter->internal.info);
iter->internal.cur = 0;
}
diff --git a/trailer.h b/trailer.h
index c6d3ee49bbf..410c61b62be 100644
--- a/trailer.h
+++ b/trailer.h
@@ -94,8 +94,9 @@ void parse_trailers(const struct process_trailer_options *,
const char *str,
struct list_head *head);
-void trailer_info_get(struct trailer_info *info, const char *str,
- const struct process_trailer_options *opts);
+void trailer_info_get(const struct process_trailer_options *,
+ const char *str,
+ struct trailer_info *);
void trailer_info_release(struct trailer_info *info);
--
gitgitgadget
^ permalink raw reply related
* [PATCH v6 5/9] trailer: reorder format_trailers_from_commit() parameters
From: Linus Arver via GitGitGadget @ 2024-03-01 0:14 UTC (permalink / raw)
To: git
Cc: Christian Couder, Junio C Hamano, Emily Shaffer, Josh Steadmon,
Randall S. Becker, Christian Couder, Kristoffer Haugsbakk,
Linus Arver, Linus Arver
In-Reply-To: <pull.1632.v6.git.1709252086.gitgitgadget@gmail.com>
From: Linus Arver <linusa@google.com>
Currently there are two functions for formatting trailers in
<trailer.h>:
void format_trailers(const struct process_trailer_options *,
struct list_head *trailers, FILE *outfile);
void format_trailers_from_commit(struct strbuf *out, const char *msg,
const struct process_trailer_options *opts);
and although they are similar enough (even taking the same
process_trailer_options struct pointer) they are used quite differently.
One might intuitively think that format_trailers_from_commit() builds on
top of format_trailers(), but this is not the case. Instead
format_trailers_from_commit() calls format_trailer_info() and
format_trailers() is never called in that codepath.
This is a preparatory refactor to help us deprecate format_trailers() in
favor of format_trailer_info() (at which point we can rename the latter
to the former). When the deprecation is complete, both
format_trailers_from_commit(), and the interpret-trailers builtin will
be able to call into the same helper function (instead of
format_trailers() and format_trailer_info(), respectively). Unifying the
formatters is desirable because it simplifies the API.
Reorder parameters for format_trailers_from_commit() to prefer
const struct process_trailer_options *opts
as the first parameter, because these options are intimately tied to
formatting trailers. And take
struct strbuf *out
last, because it's an "out parameter" (something that the caller wants
to use as the output of this function).
Similarly, reorder parameters for format_trailer_info(), because later
on we will unify the two together.
Signed-off-by: Linus Arver <linusa@google.com>
---
pretty.c | 2 +-
ref-filter.c | 2 +-
trailer.c | 11 ++++++-----
trailer.h | 5 +++--
4 files changed, 11 insertions(+), 9 deletions(-)
diff --git a/pretty.c b/pretty.c
index cf964b060cd..bdbed4295aa 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1759,7 +1759,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
goto trailer_out;
}
if (*arg == ')') {
- format_trailers_from_commit(sb, msg + c->subject_off, &opts);
+ format_trailers_from_commit(&opts, msg + c->subject_off, sb);
ret = arg - placeholder + 1;
}
trailer_out:
diff --git a/ref-filter.c b/ref-filter.c
index 35b989e1dfe..d358953b0ce 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1985,7 +1985,7 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct exp
struct strbuf s = STRBUF_INIT;
/* Format the trailer info according to the trailer_opts given */
- format_trailers_from_commit(&s, subpos, &atom->u.contents.trailer_opts);
+ format_trailers_from_commit(&atom->u.contents.trailer_opts, subpos, &s);
v->s = strbuf_detach(&s, NULL);
} else if (atom->u.contents.option == C_BARE)
diff --git a/trailer.c b/trailer.c
index d23afa0a65c..5025be97899 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1083,10 +1083,10 @@ void trailer_info_release(struct trailer_info *info)
free(info->trailers);
}
-static void format_trailer_info(struct strbuf *out,
+static void format_trailer_info(const struct process_trailer_options *opts,
const struct trailer_info *info,
const char *msg,
- const struct process_trailer_options *opts)
+ struct strbuf *out)
{
size_t origlen = out->len;
size_t i;
@@ -1144,13 +1144,14 @@ static void format_trailer_info(struct strbuf *out,
}
-void format_trailers_from_commit(struct strbuf *out, const char *msg,
- const struct process_trailer_options *opts)
+void format_trailers_from_commit(const struct process_trailer_options *opts,
+ const char *msg,
+ struct strbuf *out)
{
struct trailer_info info;
trailer_info_get(&info, msg, opts);
- format_trailer_info(out, &info, msg, opts);
+ format_trailer_info(opts, &info, msg, out);
trailer_info_release(&info);
}
diff --git a/trailer.h b/trailer.h
index c292d44b62f..c6d3ee49bbf 100644
--- a/trailer.h
+++ b/trailer.h
@@ -115,8 +115,9 @@ void free_trailers(struct list_head *);
* only the trailer block itself, even if the "only_trailers" option is not
* set.
*/
-void format_trailers_from_commit(struct strbuf *out, const char *msg,
- const struct process_trailer_options *opts);
+void format_trailers_from_commit(const struct process_trailer_options *opts,
+ const char *msg,
+ struct strbuf *out);
/*
* An interface for iterating over the trailers found in a particular commit
--
gitgitgadget
^ permalink raw reply related
* [PATCH v6 4/9] trailer: move interpret_trailers() to interpret-trailers.c
From: Linus Arver via GitGitGadget @ 2024-03-01 0:14 UTC (permalink / raw)
To: git
Cc: Christian Couder, Junio C Hamano, Emily Shaffer, Josh Steadmon,
Randall S. Becker, Christian Couder, Kristoffer Haugsbakk,
Linus Arver, Linus Arver
In-Reply-To: <pull.1632.v6.git.1709252086.gitgitgadget@gmail.com>
From: Linus Arver <linusa@google.com>
The interpret-trailers.c builtin is the only place we need to call
interpret_trailers(), so move its definition there (together with a few
helper functions called only by it) and remove its external declaration
from <trailer.h>.
Several helper functions that are called by interpret_trailers() remain
in trailer.c because other callers in the same file still call them.
Declare them in <trailer.h> so that interpret_trailers() (now in
builtin/interpret-trailers.c) can continue calling them as a trailer API
user.
This enriches <trailer.h> with a more granular API, which can then be
unit-tested in the future (because interpret_trailers() by itself does
too many things to be able to be easily unit-tested).
Take this opportunity to demote some file-handling functions out of the
trailer API implementation, as these have nothing to do with trailers.
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Linus Arver <linusa@google.com>
---
builtin/interpret-trailers.c | 93 +++++++++++++++++++++++++++
trailer.c | 119 ++++-------------------------------
trailer.h | 20 +++++-
3 files changed, 123 insertions(+), 109 deletions(-)
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 85a3413baf5..d1cf0aa33a2 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -9,6 +9,7 @@
#include "gettext.h"
#include "parse-options.h"
#include "string-list.h"
+#include "tempfile.h"
#include "trailer.h"
#include "config.h"
@@ -91,6 +92,98 @@ static int parse_opt_parse(const struct option *opt, const char *arg,
return 0;
}
+static struct tempfile *trailers_tempfile;
+
+static FILE *create_in_place_tempfile(const char *file)
+{
+ struct stat st;
+ struct strbuf filename_template = STRBUF_INIT;
+ const char *tail;
+ FILE *outfile;
+
+ if (stat(file, &st))
+ die_errno(_("could not stat %s"), file);
+ if (!S_ISREG(st.st_mode))
+ die(_("file %s is not a regular file"), file);
+ if (!(st.st_mode & S_IWUSR))
+ die(_("file %s is not writable by user"), file);
+
+ /* Create temporary file in the same directory as the original */
+ tail = strrchr(file, '/');
+ if (tail)
+ strbuf_add(&filename_template, file, tail - file + 1);
+ strbuf_addstr(&filename_template, "git-interpret-trailers-XXXXXX");
+
+ trailers_tempfile = xmks_tempfile_m(filename_template.buf, st.st_mode);
+ strbuf_release(&filename_template);
+ outfile = fdopen_tempfile(trailers_tempfile, "w");
+ if (!outfile)
+ die_errno(_("could not open temporary file"));
+
+ return outfile;
+}
+
+static void read_input_file(struct strbuf *sb, const char *file)
+{
+ if (file) {
+ if (strbuf_read_file(sb, file, 0) < 0)
+ die_errno(_("could not read input file '%s'"), file);
+ } else {
+ if (strbuf_read(sb, fileno(stdin), 0) < 0)
+ die_errno(_("could not read from stdin"));
+ }
+}
+
+static void interpret_trailers(const struct process_trailer_options *opts,
+ struct list_head *new_trailer_head,
+ const char *file)
+{
+ LIST_HEAD(head);
+ struct strbuf sb = STRBUF_INIT;
+ struct trailer_info info;
+ FILE *outfile = stdout;
+
+ trailer_config_init();
+
+ read_input_file(&sb, file);
+
+ if (opts->in_place)
+ outfile = create_in_place_tempfile(file);
+
+ parse_trailers(opts, &info, sb.buf, &head);
+
+ /* Print the lines before the trailers */
+ if (!opts->only_trailers)
+ fwrite(sb.buf, 1, info.trailer_block_start, outfile);
+
+ if (!opts->only_trailers && !info.blank_line_before_trailer)
+ fprintf(outfile, "\n");
+
+
+ if (!opts->only_input) {
+ LIST_HEAD(config_head);
+ LIST_HEAD(arg_head);
+ parse_trailers_from_config(&config_head);
+ parse_trailers_from_command_line_args(&arg_head, new_trailer_head);
+ list_splice(&config_head, &arg_head);
+ process_trailers_lists(&head, &arg_head);
+ }
+
+ format_trailers(opts, &head, outfile);
+ free_trailers(&head);
+
+ /* Print the lines after the trailers as is */
+ if (!opts->only_trailers)
+ fwrite(sb.buf + info.trailer_block_end, 1, sb.len - info.trailer_block_end, outfile);
+ trailer_info_release(&info);
+
+ if (opts->in_place)
+ if (rename_tempfile(&trailers_tempfile, file))
+ die_errno(_("could not rename temporary file to %s"), file);
+
+ strbuf_release(&sb);
+}
+
int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
{
struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
diff --git a/trailer.c b/trailer.c
index 916175707d8..d23afa0a65c 100644
--- a/trailer.c
+++ b/trailer.c
@@ -5,7 +5,6 @@
#include "string-list.h"
#include "run-command.h"
#include "commit.h"
-#include "tempfile.h"
#include "trailer.h"
#include "list.h"
/*
@@ -163,8 +162,8 @@ static void print_tok_val(FILE *outfile, const char *tok, const char *val)
fprintf(outfile, "%s%c %s\n", tok, separators[0], val);
}
-static void format_trailers(const struct process_trailer_options *opts,
- struct list_head *trailers, FILE *outfile)
+void format_trailers(const struct process_trailer_options *opts,
+ struct list_head *trailers, FILE *outfile)
{
struct list_head *pos;
struct trailer_item *item;
@@ -366,8 +365,8 @@ static int find_same_and_apply_arg(struct list_head *head,
return 0;
}
-static void process_trailers_lists(struct list_head *head,
- struct list_head *arg_head)
+void process_trailers_lists(struct list_head *head,
+ struct list_head *arg_head)
{
struct list_head *pos, *p;
struct arg_item *arg_tok;
@@ -589,7 +588,7 @@ static int git_trailer_config(const char *conf_key, const char *value,
return 0;
}
-static void trailer_config_init(void)
+void trailer_config_init(void)
{
if (configured)
return;
@@ -719,7 +718,7 @@ static void add_arg_item(struct list_head *arg_head, char *tok, char *val,
list_add_tail(&new_item->list, arg_head);
}
-static void parse_trailers_from_config(struct list_head *config_head)
+void parse_trailers_from_config(struct list_head *config_head)
{
struct arg_item *item;
struct list_head *pos;
@@ -735,8 +734,8 @@ static void parse_trailers_from_config(struct list_head *config_head)
}
}
-static void parse_trailers_from_command_line_args(struct list_head *arg_head,
- struct list_head *new_trailer_head)
+void parse_trailers_from_command_line_args(struct list_head *arg_head,
+ struct list_head *new_trailer_head)
{
struct strbuf tok = STRBUF_INIT;
struct strbuf val = STRBUF_INIT;
@@ -775,17 +774,6 @@ static void parse_trailers_from_command_line_args(struct list_head *arg_head,
free(cl_separators);
}
-static void read_input_file(struct strbuf *sb, const char *file)
-{
- if (file) {
- if (strbuf_read_file(sb, file, 0) < 0)
- die_errno(_("could not read input file '%s'"), file);
- } else {
- if (strbuf_read(sb, fileno(stdin), 0) < 0)
- die_errno(_("could not read from stdin"));
- }
-}
-
static const char *next_line(const char *str)
{
const char *nl = strchrnul(str, '\n');
@@ -1000,10 +988,10 @@ static void unfold_value(struct strbuf *val)
* Parse trailers in "str", populating the trailer info and "head"
* linked list structure.
*/
-static void parse_trailers(struct trailer_info *info,
- const char *str,
- struct list_head *head,
- const struct process_trailer_options *opts)
+void parse_trailers(const struct process_trailer_options *opts,
+ struct trailer_info *info,
+ const char *str,
+ struct list_head *head)
{
struct strbuf tok = STRBUF_INIT;
struct strbuf val = STRBUF_INIT;
@@ -1035,7 +1023,7 @@ static void parse_trailers(struct trailer_info *info,
}
}
-static void free_trailers(struct list_head *trailers)
+void free_trailers(struct list_head *trailers)
{
struct list_head *pos, *p;
list_for_each_safe(pos, p, trailers) {
@@ -1044,87 +1032,6 @@ static void free_trailers(struct list_head *trailers)
}
}
-static struct tempfile *trailers_tempfile;
-
-static FILE *create_in_place_tempfile(const char *file)
-{
- struct stat st;
- struct strbuf filename_template = STRBUF_INIT;
- const char *tail;
- FILE *outfile;
-
- if (stat(file, &st))
- die_errno(_("could not stat %s"), file);
- if (!S_ISREG(st.st_mode))
- die(_("file %s is not a regular file"), file);
- if (!(st.st_mode & S_IWUSR))
- die(_("file %s is not writable by user"), file);
-
- /* Create temporary file in the same directory as the original */
- tail = strrchr(file, '/');
- if (tail)
- strbuf_add(&filename_template, file, tail - file + 1);
- strbuf_addstr(&filename_template, "git-interpret-trailers-XXXXXX");
-
- trailers_tempfile = xmks_tempfile_m(filename_template.buf, st.st_mode);
- strbuf_release(&filename_template);
- outfile = fdopen_tempfile(trailers_tempfile, "w");
- if (!outfile)
- die_errno(_("could not open temporary file"));
-
- return outfile;
-}
-
-void interpret_trailers(const struct process_trailer_options *opts,
- struct list_head *new_trailer_head,
- const char *file)
-{
- LIST_HEAD(head);
- struct strbuf sb = STRBUF_INIT;
- struct trailer_info info;
- FILE *outfile = stdout;
-
- trailer_config_init();
-
- read_input_file(&sb, file);
-
- if (opts->in_place)
- outfile = create_in_place_tempfile(file);
-
- parse_trailers(&info, sb.buf, &head, opts);
-
- /* Print the lines before the trailers */
- if (!opts->only_trailers)
- fwrite(sb.buf, 1, info.trailer_block_start, outfile);
-
- if (!opts->only_trailers && !info.blank_line_before_trailer)
- fprintf(outfile, "\n");
-
-
- if (!opts->only_input) {
- LIST_HEAD(config_head);
- LIST_HEAD(arg_head);
- parse_trailers_from_config(&config_head);
- parse_trailers_from_command_line_args(&arg_head, new_trailer_head);
- list_splice(&config_head, &arg_head);
- process_trailers_lists(&head, &arg_head);
- }
-
- format_trailers(opts, &head, outfile);
- free_trailers(&head);
-
- /* Print the lines after the trailers as is */
- if (!opts->only_trailers)
- fwrite(sb.buf + info.trailer_block_end, 1, sb.len - info.trailer_block_end, outfile);
- trailer_info_release(&info);
-
- if (opts->in_place)
- if (rename_tempfile(&trailers_tempfile, file))
- die_errno(_("could not rename temporary file to %s"), file);
-
- strbuf_release(&sb);
-}
-
void trailer_info_get(struct trailer_info *info, const char *str,
const struct process_trailer_options *opts)
{
diff --git a/trailer.h b/trailer.h
index 37033e631a1..c292d44b62f 100644
--- a/trailer.h
+++ b/trailer.h
@@ -81,15 +81,29 @@ struct process_trailer_options {
#define PROCESS_TRAILER_OPTIONS_INIT {0}
-void interpret_trailers(const struct process_trailer_options *opts,
- struct list_head *new_trailer_head,
- const char *file);
+void parse_trailers_from_config(struct list_head *config_head);
+
+void parse_trailers_from_command_line_args(struct list_head *arg_head,
+ struct list_head *new_trailer_head);
+
+void process_trailers_lists(struct list_head *head,
+ struct list_head *arg_head);
+
+void parse_trailers(const struct process_trailer_options *,
+ struct trailer_info *,
+ const char *str,
+ struct list_head *head);
void trailer_info_get(struct trailer_info *info, const char *str,
const struct process_trailer_options *opts);
void trailer_info_release(struct trailer_info *info);
+void trailer_config_init(void);
+void format_trailers(const struct process_trailer_options *,
+ struct list_head *trailers, FILE *outfile);
+void free_trailers(struct list_head *);
+
/*
* Format the trailers from the commit msg "msg" into the strbuf "out".
* Note two caveats about "opts":
--
gitgitgadget
^ permalink raw reply related
* [PATCH v6 3/9] trailer: rename functions to use 'trailer'
From: Linus Arver via GitGitGadget @ 2024-03-01 0:14 UTC (permalink / raw)
To: git
Cc: Christian Couder, Junio C Hamano, Emily Shaffer, Josh Steadmon,
Randall S. Becker, Christian Couder, Kristoffer Haugsbakk,
Linus Arver, Linus Arver
In-Reply-To: <pull.1632.v6.git.1709252086.gitgitgadget@gmail.com>
From: Linus Arver <linusa@google.com>
Rename process_trailers() to interpret_trailers(), because it matches
the name for the builtin command of the same name
(git-interpret-trailers), which is the sole user of process_trailers().
In a following commit, we will move "interpret_trailers" from trailer.c
to builtin/interpret-trailers.c. That move will necessitate the growth
of the trailer.h API, forcing us to expose some additional functions in
trailer.h.
Rename relevant functions so that they include the term "trailer" in
their name, so that clients of the API will be able to easily identify
them by their "trailer" moniker, just like all the other functions
already exposed by trailer.h.
Rename `struct list_head *head` to `struct list_head *trailers` because
"head" conveys no additional information beyond the "list_head" type.
Reorder parameters for format_trailers_from_commit() to prefer
const struct process_trailer_options *opts
as the first parameter, because these options are intimately tied to
formatting trailers. Parameters like `FILE *outfile` should be last
because they are a kind of 'out' parameter, so put such parameters at
the end. This will be the pattern going forward in this series.
Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Linus Arver <linusa@google.com>
---
builtin/interpret-trailers.c | 4 ++--
trailer.c | 26 +++++++++++++-------------
trailer.h | 6 +++---
3 files changed, 18 insertions(+), 18 deletions(-)
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 033bd1556cf..85a3413baf5 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -132,11 +132,11 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
if (argc) {
int i;
for (i = 0; i < argc; i++)
- process_trailers(argv[i], &opts, &trailers);
+ interpret_trailers(&opts, &trailers, argv[i]);
} else {
if (opts.in_place)
die(_("no input file given for in-place editing"));
- process_trailers(NULL, &opts, &trailers);
+ interpret_trailers(&opts, &trailers, NULL);
}
new_trailers_clear(&trailers);
diff --git a/trailer.c b/trailer.c
index f74915bd8cd..916175707d8 100644
--- a/trailer.c
+++ b/trailer.c
@@ -163,12 +163,12 @@ static void print_tok_val(FILE *outfile, const char *tok, const char *val)
fprintf(outfile, "%s%c %s\n", tok, separators[0], val);
}
-static void print_all(FILE *outfile, struct list_head *head,
- const struct process_trailer_options *opts)
+static void format_trailers(const struct process_trailer_options *opts,
+ struct list_head *trailers, FILE *outfile)
{
struct list_head *pos;
struct trailer_item *item;
- list_for_each(pos, head) {
+ list_for_each(pos, trailers) {
item = list_entry(pos, struct trailer_item, list);
if ((!opts->trim_empty || strlen(item->value) > 0) &&
(!opts->only_trailers || item->token))
@@ -589,7 +589,7 @@ static int git_trailer_config(const char *conf_key, const char *value,
return 0;
}
-static void ensure_configured(void)
+static void trailer_config_init(void)
{
if (configured)
return;
@@ -1035,10 +1035,10 @@ static void parse_trailers(struct trailer_info *info,
}
}
-static void free_all(struct list_head *head)
+static void free_trailers(struct list_head *trailers)
{
struct list_head *pos, *p;
- list_for_each_safe(pos, p, head) {
+ list_for_each_safe(pos, p, trailers) {
list_del(pos);
free_trailer_item(list_entry(pos, struct trailer_item, list));
}
@@ -1075,16 +1075,16 @@ static FILE *create_in_place_tempfile(const char *file)
return outfile;
}
-void process_trailers(const char *file,
- const struct process_trailer_options *opts,
- struct list_head *new_trailer_head)
+void interpret_trailers(const struct process_trailer_options *opts,
+ struct list_head *new_trailer_head,
+ const char *file)
{
LIST_HEAD(head);
struct strbuf sb = STRBUF_INIT;
struct trailer_info info;
FILE *outfile = stdout;
- ensure_configured();
+ trailer_config_init();
read_input_file(&sb, file);
@@ -1110,8 +1110,8 @@ void process_trailers(const char *file,
process_trailers_lists(&head, &arg_head);
}
- print_all(outfile, &head, opts);
- free_all(&head);
+ format_trailers(opts, &head, outfile);
+ free_trailers(&head);
/* Print the lines after the trailers as is */
if (!opts->only_trailers)
@@ -1134,7 +1134,7 @@ void trailer_info_get(struct trailer_info *info, const char *str,
size_t nr = 0, alloc = 0;
char **last = NULL;
- ensure_configured();
+ trailer_config_init();
end_of_log_message = find_end_of_log_message(str, opts->no_divider);
trailer_block_start = find_trailer_block_start(str, end_of_log_message);
diff --git a/trailer.h b/trailer.h
index 1644cd05f60..37033e631a1 100644
--- a/trailer.h
+++ b/trailer.h
@@ -81,9 +81,9 @@ struct process_trailer_options {
#define PROCESS_TRAILER_OPTIONS_INIT {0}
-void process_trailers(const char *file,
- const struct process_trailer_options *opts,
- struct list_head *new_trailer_head);
+void interpret_trailers(const struct process_trailer_options *opts,
+ struct list_head *new_trailer_head,
+ const char *file);
void trailer_info_get(struct trailer_info *info, const char *str,
const struct process_trailer_options *opts);
--
gitgitgadget
^ permalink raw reply related
* [PATCH v6 2/9] shortlog: add test for de-duplicating folded trailers
From: Linus Arver via GitGitGadget @ 2024-03-01 0:14 UTC (permalink / raw)
To: git
Cc: Christian Couder, Junio C Hamano, Emily Shaffer, Josh Steadmon,
Randall S. Becker, Christian Couder, Kristoffer Haugsbakk,
Linus Arver, Linus Arver
In-Reply-To: <pull.1632.v6.git.1709252086.gitgitgadget@gmail.com>
From: Linus Arver <linusa@google.com>
The shortlog builtin was taught to use the trailer iterator interface in
47beb37bc6 (shortlog: match commit trailers with --group, 2020-09-27).
The iterator always unfolds values and this has always been the case
since the time the iterator was first introduced in f0939a0eb1 (trailer:
add interface for iterating over commit trailers, 2020-09-27). Add a
comment line to remind readers of this behavior.
The fact that the iterator always unfolds values is important
(at least for shortlog) because unfolding allows it to recognize both
folded and unfolded versions of the same trailer for de-duplication.
Capture the existing behavior in a new test case to guard against
regressions in this area. This test case is based off of the existing
"shortlog de-duplicates trailers in a single commit" just above it. Now
if we were to remove the call to
unfold_value(&iter->val);
inside the iterator, this new test case will break.
Signed-off-by: Linus Arver <linusa@google.com>
---
t/t4201-shortlog.sh | 32 ++++++++++++++++++++++++++++++++
trailer.c | 1 +
2 files changed, 33 insertions(+)
diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh
index d7382709fc1..f698d0c9ad2 100755
--- a/t/t4201-shortlog.sh
+++ b/t/t4201-shortlog.sh
@@ -312,6 +312,38 @@ test_expect_success 'shortlog de-duplicates trailers in a single commit' '
test_cmp expect actual
'
+# Trailers that have unfolded (single line) and folded (multiline) values which
+# are otherwise identical are treated as the same trailer for de-duplication.
+test_expect_success 'shortlog de-duplicates trailers in a single commit (folded/unfolded values)' '
+ git commit --allow-empty -F - <<-\EOF &&
+ subject one
+
+ this message has two distinct values, plus a repeat (folded)
+
+ Repeated-trailer: Foo foo foo
+ Repeated-trailer: Bar
+ Repeated-trailer: Foo
+ foo foo
+ EOF
+
+ git commit --allow-empty -F - <<-\EOF &&
+ subject two
+
+ similar to the previous, but without the second distinct value
+
+ Repeated-trailer: Foo foo foo
+ Repeated-trailer: Foo
+ foo foo
+ EOF
+
+ cat >expect <<-\EOF &&
+ 2 Foo foo foo
+ 1 Bar
+ EOF
+ git shortlog -ns --group=trailer:repeated-trailer -2 HEAD >actual &&
+ test_cmp expect actual
+'
+
test_expect_success 'shortlog can match multiple groups' '
git commit --allow-empty -F - <<-\EOF &&
subject one
diff --git a/trailer.c b/trailer.c
index e1d83390b66..f74915bd8cd 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1270,6 +1270,7 @@ int trailer_iterator_advance(struct trailer_iterator *iter)
strbuf_reset(&iter->val);
parse_trailer(&iter->key, &iter->val, NULL,
trailer, separator_pos);
+ /* Always unfold values during iteration. */
unfold_value(&iter->val);
return 1;
}
--
gitgitgadget
^ permalink raw reply related
* [PATCH v6 1/9] trailer: free trailer_info _after_ all related usage
From: Linus Arver via GitGitGadget @ 2024-03-01 0:14 UTC (permalink / raw)
To: git
Cc: Christian Couder, Junio C Hamano, Emily Shaffer, Josh Steadmon,
Randall S. Becker, Christian Couder, Kristoffer Haugsbakk,
Linus Arver, Linus Arver
In-Reply-To: <pull.1632.v6.git.1709252086.gitgitgadget@gmail.com>
From: Linus Arver <linusa@google.com>
In de7c27a186 (trailer: use offsets for trailer_start/trailer_end,
2023-10-20), we started using trailer block offsets in trailer_info. In
particular, we dropped the use of a separate stack variable "size_t
trailer_end", in favor of accessing the new "trailer_block_end" member
of trailer_info (as "info.trailer_block_end").
At that time, we forgot to also move the
trailer_info_release(&info);
line to be _after_ this new use of the trailer_info struct. Move it now.
Note that even without this patch, we didn't have leaks or any other
problems because trailer_info_release() only frees memory allocated on
the heap. The "trailer_block_end" member was allocated on the stack back
then (as it is now) so it was still safe to use for all this time.
Reported-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Linus Arver <linusa@google.com>
---
trailer.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/trailer.c b/trailer.c
index 3a0710a4583..e1d83390b66 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1111,13 +1111,12 @@ void process_trailers(const char *file,
}
print_all(outfile, &head, opts);
-
free_all(&head);
- trailer_info_release(&info);
/* Print the lines after the trailers as is */
if (!opts->only_trailers)
fwrite(sb.buf + info.trailer_block_end, 1, sb.len - info.trailer_block_end, outfile);
+ trailer_info_release(&info);
if (opts->in_place)
if (rename_tempfile(&trailers_tempfile, file))
--
gitgitgadget
^ permalink raw reply related
* [PATCH v6 0/9] Enrich Trailer API
From: Linus Arver via GitGitGadget @ 2024-03-01 0:14 UTC (permalink / raw)
To: git
Cc: Christian Couder, Junio C Hamano, Emily Shaffer, Josh Steadmon,
Randall S. Becker, Christian Couder, Kristoffer Haugsbakk,
Linus Arver
In-Reply-To: <pull.1632.v5.git.1708124950.gitgitgadget@gmail.com>
This patch series is the first 9 patches of a larger cleanup/bugfix series
(henceforth "larger series") I've been working on. The main goal of this
series is to begin the process of "libifying" the trailer API. By "API" I
mean the interface exposed in trailer.h. The larger series brings a number
of additional cleanups (exposing and fixing some bugs along the way), and
builds on top of this series.
When the larger series is merged, we will be in a good state to additionally
pursue the following goals:
1. "API reuse inside Git": make the API expressive enough to eliminate any
need by other parts of Git to use the interpret-trailers builtin as a
subprocess (instead they could just use the API directly);
2. "API stability": add unit tests to codify the expected behavior of API
functions; and
3. "API documentation": create developer-focused documentation to explain
how to use the API effectively, noting any API limitations or
anti-patterns.
In the future after libification is "complete", users external to Git will
be able to use the same trailer processing API used by the
interpret-trailers builtin. For example, a web server may want to parse
trailers the same way that Git would parse them, without having to call
interpret-trailers as a subprocess. This use case was the original
motivation behind my work in this area.
With the libification-focused goals out of the way, let's turn to this patch
series in more detail.
In summary this series breaks up "process_trailers()" into smaller pieces,
exposing many of the parts relevant to trailer-related processing in
trailer.h. This will force us to eventually introduce unit tests for these
API functions, but that is a good thing for API stability. We also perform
some preparatory refactors in order to help us unify the trailer formatting
machinery toward the end of this series.
Notable changes in v6
=====================
* Mainly wording changes to commit messages. Thanks to Christian for the
suggestions.
Notable changes in v5
=====================
* Removed patches 10+ from this series. Thanks to Christian for the
suggestion.
* Reworded the log message of patch 09 to reflect the above arrangement, as
suggested by Christian.
Notable changes in v4
=====================
* Patches 3, 4, 5, and 8 have been broken up into smaller steps. There are
28 instead of 10 patches now, but these 28 should be much easier to
review than the (previously condensed) 10.
* NEW Patch 1: "trailer: free trailer_info after all related usage" fixes
awkward use-after-free coding style
* NEW Patch 2: "shortlog: add test for de-duplicating folded trailers"
increases test coverage related to trailer iterators and "unfold_value()"
* NEW Patch 27: "trailer_set_*(): put out parameter at the end" is a small
refactor to reorder parameters.
* Patches 5-16: These smaller patches make up Patch 3 from v3.
* Patches 17-18: These smaller patches make up Patch 4 from v3.
* Patches 19-20: These smaller patches make up Patch 5 from v3.
* Patches 23-26: These smaller patches make up Patch 8 from v3.
* Anonymize unambiguous parameters in <trailer.h>.
Notable changes in v3
=====================
* Squashed Patch 4 into Patch 3 ("trailer: unify trailer formatting
machinery"), to avoid breaking the build ("-Werror=unused-function"
violations)
* NEW (Patch 10): Introduce "trailer template" terminology for readability
(no behavioral change)
* (API function) Rename default_separators() to
trailer_default_separators()
* (API function) Rename new_trailers_clear() to free_trailer_templates()
* trailer.h: for single-parameter functions, anonymize the parameter name
to reduce verbosity
Notable changes in v2
=====================
* (cover letter) Discuss goals of the larger series in more detail,
especially the pimpl idiom
* (cover letter) List bug fixes pending in the larger series that depend on
this series
* Reorder function parameters to have trailer options at the beginning (and
out parameters toward the end)
* "sequencer: use the trailer iterator": prefer C string instead of strbuf
for new "raw" field
* Patch 1 (was Patch 2) also renames ensure_configured() to
trailer_config_init() (forgot to rename this one previously)
Linus Arver (9):
trailer: free trailer_info _after_ all related usage
shortlog: add test for de-duplicating folded trailers
trailer: rename functions to use 'trailer'
trailer: move interpret_trailers() to interpret-trailers.c
trailer: reorder format_trailers_from_commit() parameters
trailer_info_get(): reorder parameters
format_trailers(): use strbuf instead of FILE
format_trailer_info(): move "fast path" to caller
format_trailers_from_commit(): indirectly call trailer_info_get()
builtin/interpret-trailers.c | 101 ++++++++++++++++++-
pretty.c | 2 +-
ref-filter.c | 2 +-
sequencer.c | 2 +-
t/t4201-shortlog.sh | 32 +++++++
trailer.c | 181 +++++++++--------------------------
trailer.h | 31 ++++--
7 files changed, 204 insertions(+), 147 deletions(-)
base-commit: a54a84b333adbecf7bc4483c0e36ed5878cac17b
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1632%2Flistx%2Ftrailer-api-refactor-part-1-v6
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1632/listx/trailer-api-refactor-part-1-v6
Pull-Request: https://github.com/gitgitgadget/git/pull/1632
Range-diff vs v5:
1: 652df25f30e = 1: 652df25f30e trailer: free trailer_info _after_ all related usage
2: fdccaca2ba0 = 2: fdccaca2ba0 shortlog: add test for de-duplicating folded trailers
3: 4372af244f0 ! 3: 7b1d739cddb trailer: prepare to expose functions as part of API
@@ Metadata
Author: Linus Arver <linusa@google.com>
## Commit message ##
- trailer: prepare to expose functions as part of API
+ trailer: rename functions to use 'trailer'
- In the next patch, we will move "process_trailers" from trailer.c to
- builtin/interpret-trailers.c. That move will necessitate the growth of
- the trailer.h API, forcing us to expose some additional functions in
+ Rename process_trailers() to interpret_trailers(), because it matches
+ the name for the builtin command of the same name
+ (git-interpret-trailers), which is the sole user of process_trailers().
+
+ In a following commit, we will move "interpret_trailers" from trailer.c
+ to builtin/interpret-trailers.c. That move will necessitate the growth
+ of the trailer.h API, forcing us to expose some additional functions in
trailer.h.
Rename relevant functions so that they include the term "trailer" in
@@ Commit message
them by their "trailer" moniker, just like all the other functions
already exposed by trailer.h.
- Take the opportunity to start putting trailer processing options (opts)
- as the first parameter. This will be the pattern going forward in this
- series.
+ Rename `struct list_head *head` to `struct list_head *trailers` because
+ "head" conveys no additional information beyond the "list_head" type.
+
+ Reorder parameters for format_trailers_from_commit() to prefer
+
+ const struct process_trailer_options *opts
+
+ as the first parameter, because these options are intimately tied to
+ formatting trailers. Parameters like `FILE *outfile` should be last
+ because they are a kind of 'out' parameter, so put such parameters at
+ the end. This will be the pattern going forward in this series.
Helped-by: Junio C Hamano <gitster@pobox.com>
+ Helped-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Linus Arver <linusa@google.com>
## builtin/interpret-trailers.c ##
4: 4073b8eb510 = 4: 7ac4da3019a trailer: move interpret_trailers() to interpret-trailers.c
5: b2a0f7829a1 ! 5: 47c994ce025 trailer: start preparing for formatting unification
@@ Metadata
Author: Linus Arver <linusa@google.com>
## Commit message ##
- trailer: start preparing for formatting unification
+ trailer: reorder format_trailers_from_commit() parameters
Currently there are two functions for formatting trailers in
<trailer.h>:
@@ Commit message
last, because it's an "out parameter" (something that the caller wants
to use as the output of this function).
+ Similarly, reorder parameters for format_trailer_info(), because later
+ on we will unify the two together.
+
Signed-off-by: Linus Arver <linusa@google.com>
## pretty.c ##
6: c1760f80356 = 6: 7a565580167 trailer_info_get(): reorder parameters
7: 9dc912b5bc5 = 7: 46c7f4c0e81 format_trailers(): use strbuf instead of FILE
8: b97c06d8bc3 = 8: 26b1f19d0e1 format_trailer_info(): move "fast path" to caller
9: 7c656b3f775 ! 9: 0e884d870c8 format_trailers_from_commit(): indirectly call trailer_info_get()
@@ Commit message
This is another preparatory refactor to unify the trailer formatters.
- Instead of calling trailer_info_get() directly, call parse_trailers()
- which already calls trailer_info_get(). This change is a NOP because
- format_trailer_info() only looks at the "trailers" string array, not the
- trailer_item objects which parse_trailers() populates.
+ For background, note that the "trailers" string array is the
+ `char **trailers` member in `struct trailer_info` and that the
+ trailer_item objects are the elements of the `struct list_head *head`
+ linked list.
+
+ Currently trailer_info_get() only populates `char **trailers`. And
+ parse_trailers() first calls trailer_info_get() so that it can use the
+ `char **trailers` to populate a list of `struct trailer_item` objects
+
+ Instead of calling trailer_info_get() directly from
+ format_trailers_from_commit(), make it call parse_trailers() instead
+ because parse_trailers() already calls trailer_info_get().
+
+ This change is a NOP because format_trailer_info() (which
+ format_trailers_from_commit() wraps around) only looks at the "trailers"
+ string array, not the trailer_item objects which parse_trailers()
+ populates. For now we do need to create a dummy
+
+ LIST_HEAD(trailer_objects);
+
+ because parse_trailers() expects it in its signature.
In a future patch, we'll change format_trailer_info() to use the parsed
- trailer_item objects instead of the string array.
+ trailer_item objects (trailer_objects) instead of the `char **trailers`
+ array.
Signed-off-by: Linus Arver <linusa@google.com>
@@ trailer.c: void format_trailers_from_commit(const struct process_trailer_options
const char *msg,
struct strbuf *out)
{
-+ LIST_HEAD(trailers);
++ LIST_HEAD(trailer_objects);
struct trailer_info info;
- trailer_info_get(opts, msg, &info);
-+ parse_trailers(opts, &info, msg, &trailers);
++ parse_trailers(opts, &info, msg, &trailer_objects);
+
/* If we want the whole block untouched, we can take the fast path. */
if (!opts->only_trailers && !opts->unfold && !opts->filter &&
@@ trailer.c: void format_trailers_from_commit(const struct process_trailer_options
} else
format_trailer_info(opts, &info, out);
-+ free_trailers(&trailers);
++ free_trailers(&trailer_objects);
trailer_info_release(&info);
}
--
gitgitgadget
^ permalink raw reply
* Re: [PATCH v2] tests: modernize the test script t0010-racy-git.sh
From: Eric Sunshine @ 2024-03-01 0:06 UTC (permalink / raw)
To: Junio C Hamano
Cc: Aryan Gupta via GitGitGadget, git, Patrick Steinhardt,
Michal Suchánek, Jean-Noël AVILA, Kristoffer Haugsbakk,
Aryan Gupta
In-Reply-To: <CAPig+cSGtcA15aOmvj07Uv-pFZTE58+9gGsQh=8K4BL4KRieQA@mail.gmail.com>
On Thu, Feb 29, 2024 at 6:52 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> [*]: Admittedly, the double-negative in "'foo' is not a non-empty
> file." is more than a little confusing. It probably would have been
> better phrased as "'foo' should be empty but is not".
The double-negative confused me even when suggesting a replacement.
What I meant was that a better phrasing would perhaps have been:
'foo' is empty but should not be
^ permalink raw reply
* Re: What's cooking in git.git (Feb 2024, #09; Tue, 27)
From: Linus Arver @ 2024-02-29 23:56 UTC (permalink / raw)
To: Junio C Hamano, git
In-Reply-To: <xmqqjzmpm9b8.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> [...]
>
> * la/trailer-api (2024-02-16) 9 commits
> (merged to 'next' on 2024-02-21 at 631e28bbbc)
> + format_trailers_from_commit(): indirectly call trailer_info_get()
> + format_trailer_info(): move "fast path" to caller
> + format_trailers(): use strbuf instead of FILE
> + trailer_info_get(): reorder parameters
> + trailer: start preparing for formatting unification
> + trailer: move interpret_trailers() to interpret-trailers.c
> + trailer: prepare to expose functions as part of API
> + shortlog: add test for de-duplicating folded trailers
> + trailer: free trailer_info _after_ all related usage
>
> Code clean-up.
>
> Will merge to 'master'.
> source: <pull.1632.v5.git.1708124950.gitgitgadget@gmail.com>
Doh, please wait for my v6 reroll (will send to the list in the next
half hour) to clean up the commit messages. Thanks.
^ permalink raw reply
* Re: [PATCH v5 3/9] trailer: prepare to expose functions as part of API
From: Linus Arver @ 2024-02-29 23:53 UTC (permalink / raw)
To: Junio C Hamano
Cc: Christian Couder, Linus Arver via GitGitGadget, git,
Christian Couder, Emily Shaffer, Josh Steadmon, Randall S. Becker,
Kristoffer Haugsbakk
In-Reply-To: <xmqqedcuc0w3.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Linus Arver <linusa@google.com> writes:
>
>>> Nit: this patch and the next one will become commits, so perhaps:
>>>
>>> s/In the next patch/In a following commit/
>>
>> TBH I've always wondered whether "patch" or "commit" matters --- I've
>> seen examples of patch series that referred to "commits" instead of
>> "patches", and vice versa. I was hoping to hear an opinion on this, so
>> I'm happy to see (and apply) your suggestion. Thanks.
>
> I think it is just fine to use either; sticking to one you pick
> consistently in the same series would have value. If you prefer
> commit, then fine. If you like patch, that's fine too.
Makes sense, thanks.
^ permalink raw reply
* Re: [PATCH v2] tests: modernize the test script t0010-racy-git.sh
From: Eric Sunshine @ 2024-02-29 23:52 UTC (permalink / raw)
To: Junio C Hamano
Cc: Aryan Gupta via GitGitGadget, git, Patrick Steinhardt,
Michal Suchánek, Jean-Noël AVILA, Kristoffer Haugsbakk,
Aryan Gupta
In-Reply-To: <xmqqa5nic06t.fsf@gitster.g>
On Thu, Feb 29, 2024 at 6:36 PM Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> > If taking it to this extent, then the modernized version of the last
> > couple lines would be:
> >
> > git diff-files -p >out &&
> > test_file_not_empty out
>
> Yes. The modern style seems to prefer temporary files over
> variables; the reason probably is because it tends to be easier to
> remotely post-mortem?
Yes, that seems likely. Functions such as test_file_not_empty() also
provide an immediate indication of what went wrong without having to
spelunk the test detritus:
'foo' is not a non-empty file. [*]
whereas `test "" != "$foo"` provides no output at all, so it's not
immediately clear which part of the test failed. Peff's `verbose`
function was intended to mitigate that problem by making the
expression verbose upon failure:
verbose test "" != "$foo" &&
but never really caught on and was eventually retired by 8ddfce7144
(t: drop "verbose" helper function, 2023-05-08).
[*]: Admittedly, the double-negative in "'foo' is not a non-empty
file." is more than a little confusing. It probably would have been
better phrased as "'foo' should be empty but is not".
^ permalink raw reply
* Re: [PATCH v2] tests: modernize the test script t0010-racy-git.sh
From: Junio C Hamano @ 2024-02-29 23:36 UTC (permalink / raw)
To: Eric Sunshine
Cc: Aryan Gupta via GitGitGadget, git, Patrick Steinhardt,
Michal Suchánek, Jean-Noël AVILA, Kristoffer Haugsbakk,
Aryan Gupta
In-Reply-To: <CAPig+cQ5m86=pLTpFrik0xS6XPyK4tZQx_wkc1xh2r9WDFkhuQ@mail.gmail.com>
Eric Sunshine <sunshine@sunshineco.com> writes:
> On Thu, Feb 29, 2024 at 6:14 PM Junio C Hamano <gitster@pobox.com> wrote:
>> So, we may want to do it more like this, perhaps?
>>
>> test_expect_success "Racy GIT trial #$trial part A" '
>> rm -f .git/index &&
>> echo frotz >infocom &&
>> git update-index --add infocom &&
>> echo xyzzy >infocom &&
>>
>> files=$(git diff-files -p) &&
>> test "" != "$files"
>> '
>
> If taking it to this extent, then the modernized version of the last
> couple lines would be:
>
> git diff-files -p >out &&
> test_file_not_empty out
Yes. The modern style seems to prefer temporary files over
variables; the reason probably is because it tends to be easier to
remotely post-mortem?
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox