* Re: [BUG] git commit --trailer --verbose puts trailer below scissors line
From: Jeff King @ 2024-02-17 6:04 UTC (permalink / raw)
To: Philippe Blain; +Cc: Linus Arver, Git mailing list
In-Reply-To: <8b4738ad-62cd-789e-712e-bd45a151b4ac@gmail.com>
On Fri, Feb 16, 2024 at 04:04:18PM -0500, Philippe Blain wrote:
> Hello,
>
> I've just found a bug in the current master: running
>
> git commit --trailer="key: value" --verbose
>
> puts the trailer below the diff, and thus below the scissors
> line (and so it is discarded).
>
> I checked that it works OK in 2.42.1 but not in 2.43.2 so it is not
> a new regression in the 2.44 cycle, but I thought I'd write now in case
> someone spots the culprit commit faster than me.
>
> I'll start a bisection now.
Looks like it bisects to 97e9d0b78a (trailer: find the end of the log
message, 2023-10-20). Here's a test that demonstrates it (signed-off-by:
me in case anyone wants to incorporate it into a fix):
diff --git a/t/t7502-commit-porcelain.sh b/t/t7502-commit-porcelain.sh
index b5bf7de7cd..d8e216613f 100755
--- a/t/t7502-commit-porcelain.sh
+++ b/t/t7502-commit-porcelain.sh
@@ -485,6 +485,24 @@ test_expect_success 'commit --trailer not confused by --- separator' '
test_cmp expected actual
'
+test_expect_success 'commit --trailer with --verbose' '
+ cat >msg <<-\EOF &&
+ subject
+
+ body
+ EOF
+ GIT_EDITOR=: git commit --edit -F msg --allow-empty \
+ --trailer="my-trailer: value" --verbose &&
+ {
+ cat msg &&
+ echo &&
+ echo "my-trailer: value"
+ } >expected &&
+ git cat-file commit HEAD >commit.msg &&
+ sed -e "1,/^\$/d" commit.msg >actual &&
+ test_cmp expected actual
+'
+
test_expect_success 'multiple -m' '
>negative &&
-Peff
^ permalink raw reply related
* Re: [PATCH 1/4] notes: print note blob to stdout directly
From: Junio C Hamano @ 2024-02-17 5:56 UTC (permalink / raw)
To: Jeff King; +Cc: Maarten Bosmans, git, Teng Long, Maarten Bosmans
In-Reply-To: <20240217051650.GB539459@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Digging in the history, it looks like we use "git show" at all because
> this was adapted from a shell script (though that shell script probably
> ought to have been using cat-file in the first place; maybe back then we
> thought we'd support non-blobs ;) ).
Yup, that is why I suspected that we planned to store non-blobs.
> Hmm, good question. I can't think offhand of a way that the user could
> convince "git show <oid>" to do anything different than just dumping the
> literal contents. It is not even handed a path that could trigger
> .gitattributes or similar.
show_blob_object() directly calls stream_blob_to_fd() without any
filter, as the hardcoded invocation of "git show <oid>" in the note
codepath does not allow --textconv at all, so we probably are safe
to assume that the contents will appear as-is, not even going
through EOL conversion (which is a bit puzzling, to be honest,
though). Lack of path I am not worried about, as you can easily
declare with '*' wildcard that everything in the notes tree is of
the same type. But if the stream_blob_to_fd() interface does not
have anywhere smudge filters or textconv filters can hook into
without some command line option, we do not have to worry about it.
> Sometimes, of course, we have to support weird stuff anyway because it
> has existed for a long time and we don't want to break users. But this
> is really pushing my gut-feeling limit of what is reasonable / plausible
> for somebody to be doing.
>
> Of course I may be missing some other case where "show" behaves in a
> useful way that is different than a straight dump of the blob. But if
> not, I'd almost say that getting rid of the extra "show" call now is a
> good thing, because it locks in the simple behavior. ;)
Yes, of course. It would be great if we can just stream the blob
contents out in-process.
Thanks.
^ permalink raw reply
* Re: [PATCH] git: extend --no-lazy-fetch to work across subprocesses
From: Jeff King @ 2024-02-17 5:40 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Christian Couder
In-Reply-To: <xmqqr0hcglpk.fsf_-_@gitster.g>
On Fri, Feb 16, 2024 at 01:09:43PM -0800, Junio C Hamano wrote:
> Modeling after how the `--no-replace-objects` option is made usable
> across subprocess spawning (e.g., cURL based remote helpers are
> spawned as a separate process while running "git fetch"), allow the
> `--no-lazy-fetch` option to be passed across process boundary.
>
> Do not model how the value of GIT_NO_REPLACE_OBJECTS environment
> variable is ignored, though. Just use the usual git_env_bool() to
> allow "export GIT_NO_LAZY_FETCH=0" and "unset GIT_NO_LAZY_FETCH"
> to be equivalents.
Nice. I wondered if we would need to hide fetch_if_missing behind a
function that lazy-evaluates the environment variable, but it looks like
setup_git_env() is central enough to cover us here.
> diff --git a/Documentation/git.txt b/Documentation/git.txt
> index 2f1cb3ef4e..be2829003d 100644
> --- a/Documentation/git.txt
> +++ b/Documentation/git.txt
> @@ -183,6 +183,8 @@ If you just want to run git as if it was started in `<path>` then use
> Do not fetch missing objects from the promisor remote on
> demand. Useful together with `git cat-file -e <object>` to
> see if the object is locally available.
> + This is equivalent to setting the `GIT_NO_LAZY_FETCH`
> + environment variable to `1`.
As with the other patch, I'd suggest adding an entry to the list of
environment variables later in the manpage.
> --- a/environment.h
> +++ b/environment.h
> @@ -35,6 +35,7 @@ const char *getenv_safe(struct strvec *argv, const char *name);
> #define EXEC_PATH_ENVIRONMENT "GIT_EXEC_PATH"
> #define CEILING_DIRECTORIES_ENVIRONMENT "GIT_CEILING_DIRECTORIES"
> #define NO_REPLACE_OBJECTS_ENVIRONMENT "GIT_NO_REPLACE_OBJECTS"
> +#define NO_LAZY_FETCH_ENVIRONMENT "GIT_NO_LAZY_FETCH"
> #define GIT_REPLACE_REF_BASE_ENVIRONMENT "GIT_REPLACE_REF_BASE"
> #define GITATTRIBUTES_FILE ".gitattributes"
> #define INFOATTRIBUTES_FILE "info/attributes"
A small nit, but maybe worth keeping the two replace-related variables
next to each other, rather than sticking the new one in the middle?
> diff --git a/git.c b/git.c
> index 28e8bf7497..d11d4dc77b 100644
> --- a/git.c
> +++ b/git.c
> @@ -189,6 +189,9 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
> *envchanged = 1;
> } else if (!strcmp(cmd, "--no-lazy-fetch")) {
> fetch_if_missing = 0;
> + setenv(NO_LAZY_FETCH_ENVIRONMENT, "1", 1);
> + if (envchanged)
> + *envchanged = 1;
> } else if (!strcmp(cmd, "--no-replace-objects")) {
> disable_replace_refs();
> setenv(NO_REPLACE_OBJECTS_ENVIRONMENT, "1", 1);
I _suspect_ this makes the fetch_if_missing call redundant, since we
should be parsing these before doing any repo setup that would trigger
the code that reads the environment variable.
This should probably also be xsetenv(), though as you can see in the
context we are not very consistent about using it. :) But certainly if
we failed to set it I would prefer to see an error rather than
accidentally lazy-fetching.
> diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
> index 5b7bee888d..59629cea1f 100755
> --- a/t/t0410-partial-clone.sh
> +++ b/t/t0410-partial-clone.sh
> @@ -665,6 +665,15 @@ test_expect_success 'lazy-fetch when accessing object not in the_repository' '
> git -C partial.git rev-list --objects --missing=print HEAD >out &&
> grep "[?]$FILE_HASH" out &&
>
> + # The no-lazy-fetch mechanism prevents Git from fetching
> + test_must_fail env GIT_NO_LAZY_FETCH=1 \
> + git -C partial.git cat-file -e "$FILE_HASH" &&
> + test_must_fail git --no-lazy-fetch -C partial.git cat-file -e "$FILE_HASH" &&
> +
> + # Sanity check that the file is still missing
> + git -C partial.git rev-list --objects --missing=print HEAD >out &&
> + grep "[?]$FILE_HASH" out &&
OK, we exercise it by setting the variable directly. A more interesting
one might be:
git -c alias.foo='!git cat-file' --no-lazy-fetch ...
which should fail without the patch.
I also wondered why we were testing the direct-builtin invocation here,
since we are building on top of the original --no-lazy-fetch patch, but
it looks like it is because that patch didn't add any tests at all). I
think they would make more sense as a single patch, but I guess the
original is already in 'next' (though I suppose it is about to be
rewound). I am OK with it either way.
-Peff
^ permalink raw reply
* Re: [PATCH] git: --no-lazy-fetch option
From: Jeff King @ 2024-02-17 5:29 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Christian Couder
In-Reply-To: <xmqq1q9cl3xv.fsf@gitster.g>
On Fri, Feb 16, 2024 at 09:22:20AM -0800, Junio C Hamano wrote:
> Here is a preliminary clean-up only for Documentation. Will not be
> queuing before the final, but just so that I won't forget.
I think this is an improvement, but two small comments...
> ------- >8 ------------- >8 ------------- >8 -------
> Subject: [PATCH] git: document GIT_NO_REPLACE_OBJECTS environment variable
>
> This variable is used as the primary way to disable the object
> replacement mechanism, with the "--no-replace-objects" command line
> option as an end-user visible way to set it, but has not been
> documented.
>
> The original reason why it was left undocumented might be because it
> was meant as an internal implementation detail, but the thing is,
> that our tests use the environment variable directly without the
> command line option, and there certainly are folks who learned its
> use from there, making it impossible to deprecate or change its
> behaviour by now.
I agree that we should document these sorts of variables; they really
are part of the public interface since it's so easy for users to see
them in their own scripts, aliases, etc, when we set them.
But in fact do document this environment variable in git-replace(1), and
have for a long time. So I don't think we need to even make claims about
its undocumented state. :)
> --no-replace-objects::
> - Do not use replacement refs to replace Git objects. See
> - linkgit:git-replace[1] for more information.
> + Do not use replacement refs to replace Git objects.
> + This is equivalent to exporting the `GIT_NO_REPLACE_OBJECTS`
> + environment variable with any value.
> + See linkgit:git-replace[1] for more information.
The text both before and after your patch links to git-replace, which
covers the variable. So I think the "before" state is actually not that
bad. But I still think mentioning it explicitly is better still.
However, should it get an entry in the "ENVIRONMENT VARIABLES" section?
We cover stuff there like GIT_LITERAL_PATHSPECS, which is triggered in
the same way.
> Add documentation and note that for this variable, unlike many
> boolean-looking environment variables, only the presence matters,
> not what value it is set to.
Yuck. IMHO depending on GIT_NO_REPLACE_OBJECTS=0 is somewhat crazy, and
we could consider the current behavior a bug. It's probably not _that_
big a deal either way (because I would not expect anybody to set it that
way in the first place). But I wonder if being consistent across
variables trumps retaining weird historical compatibility for such a
far-fetched case. I dunno. I guess this is https://xkcd.com/1172/. :)
-Peff
^ permalink raw reply
* Re: [PATCH 1/4] notes: print note blob to stdout directly
From: Jeff King @ 2024-02-17 5:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Maarten Bosmans, git, Teng Long, Maarten Bosmans
In-Reply-To: <xmqqcysxkjrq.fsf@gitster.g>
On Thu, Feb 15, 2024 at 10:25:45PM -0800, Junio C Hamano wrote:
> > Of course it's reasonable to also store notes that aren't meant to be
> > displayed via git-log, etc, at all. The textconv-caching machinery
> > stores its results in a separate notes ref. Those should generally be
> > text (since the point is to come up with something diff-able). But I
> > think it would be perfectly reasonable for another mechanism to make use
> > of notes in the same way and store binary goo.
>
> Yup.
>
> The question is if our current use of "git show" allows creative use
> of such binary data attached as notes. The patch in question will
> break such users, as it would become impossible once we start
> bypassing the "git show" and emitting the binary data directly to
> the standard output stream.
Hmm, good question. I can't think offhand of a way that the user could
convince "git show <oid>" to do anything different than just dumping the
literal contents. It is not even handed a path that could trigger
.gitattributes or similar.
The only difference I can think of is that "git show" triggers a pager
by default. Probably "git notes show" should be doing the same (and
honestly, that would be less confusing to me; the fact that configuring
"pager.show" would affect "git notes show" is surprising to me).
Digging in the history, it looks like we use "git show" at all because
this was adapted from a shell script (though that shell script probably
ought to have been using cat-file in the first place; maybe back then we
thought we'd support non-blobs ;) ).
> Because the pathname a note blob is stored at is unpredictable and
> not under end-user control, it would not be practical to define
> different smudge filters per note object using the attribute
> mechanism, but if you limit the types of binary data you store in
> your notes (e.g., refs/notes/thumbnail may be a note ref whose
> contents are all jpeg thumbnail images, where your main history is
> your photo archive), you might be able to convince the "git show"
> invocation we use to show the note object to show that thumbnail
> image by using something involving "display" (from imagemagick---of
> course you could use other programs that allows you to view the
> image on different platforms) in your smudge filter. Bypassing "git
> show" and sending the blob contents directly to the standard output
> would be a grave regression for such a user.
Like I said, I do not think there is a way to convince "git show" in
that way. Unless perhaps something like:
git -c pager.show='f() { cat >tmp.jpg; display tmp; }; f' \
notes show --ref thumbnail
But that's pretty awful. Why would you do that instead of just "git
notes show >tmp; display tmp" yourself? And again, even if we wanted to
support such a monstrosity, I feel like setting pager.notes.show would
be much more intuitive than piggy-backing on "git show".
Sometimes, of course, we have to support weird stuff anyway because it
has existed for a long time and we don't want to break users. But this
is really pushing my gut-feeling limit of what is reasonable / plausible
for somebody to be doing.
Of course I may be missing some other case where "show" behaves in a
useful way that is different than a straight dump of the blob. But if
not, I'd almost say that getting rid of the extra "show" call now is a
good thing, because it locks in the simple behavior. ;)
-Peff
^ permalink raw reply
* [PATCH] t0303: check that helper_test_clean removes all credentials
From: Jeff King @ 2024-02-17 4:58 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Bo Anderson via GitGitGadget, git, Bo Anderson
In-Reply-To: <xmqqle7lskvg.fsf@gitster.g>
On Thu, Feb 15, 2024 at 09:22:27AM -0800, Junio C Hamano wrote:
> > I wonder if we might want something like this, as well, which can catch
> > leftovers:
>
> Sounds like a good hygiene ;-).
Unfortunately, it is not quite so simple. There are a bunch of other
tests run by t0303 that are not in t0302, because credential-store does
not support caching, expiration, etc.
Putting it in t0301 would be better, but we don't have a way to inspect
the internal state of the cache daemon. We could perhaps add one, but
here's an even hackier solution. ;)
-- >8 --
Subject: [PATCH] t0303: check that helper_test_clean removes all credentials
Our lib-credential.sh library comes with a "clean" function that removes
all of the credentials used in its tests (to avoid leaving cruft in
system credential storage). But it's easy to add a test that uses a new
credential but forget to add it to the clean function. E.g., the case
fixed by 83e6eb7d7a (t/lib-credential: clean additional credential,
2024-02-15).
We should be able to catch this automatically, but it's a little tricky.
We can't just compare the contents of the helper's storage before and
after the test run, because there isn't a way to ask a helper to dump
all of its storage. And in most cases we don't have direct access to the
underlying storage (since the whole point of the helper is to abstract
that away). We can work around that by using our own "store" helper,
since we can directly inspect its state by looking at its on-disk file.
But there's a catch: the "store" helper doesn't support features like
caching or expiration, so using it naively fails tests (and skipping
those tests would give us incomplete coverage). Implementing all of
those features would be non-trivial. But we can hack around that by
overriding the "check" function used by the tests to turn most requests
into noop success (except for "approve" requests, which actually store
things).
And then at the end we can check that running the "clean" function takes
us back to an empty state.
Note that because we've skipped any tests that erase credentials
(because of our noop check function), the state we see at cleanup time
may be larger than it would be normally. That's OK. The point of the
clean function is to clean up any cruft we _might_ have left in place,
so we're just being doubly thorough.
The way this is bolted onto t0303 feels a little messy. But it's really
the best place to do it, because then we know that it is running the
exact sequence of tests that we'd use for testing a real external
helper. In a normal run of "make test" it currently does nothing (the
idea is that you run it manually after pointing it at some helper
program). But now with this patch, "make test" will sanity-check the
script itself.
Signed-off-by: Jeff King <peff@peff.net>
---
This should be applied on top of ba/credential-test-clean-fix,
naturally, or it will fail. :)
t/t0303-credential-external.sh | 26 ++++++++++++++++++++++++--
1 file changed, 24 insertions(+), 2 deletions(-)
diff --git a/t/t0303-credential-external.sh b/t/t0303-credential-external.sh
index 095574bfc6..72ae405c3e 100755
--- a/t/t0303-credential-external.sh
+++ b/t/t0303-credential-external.sh
@@ -32,9 +32,24 @@ commands.
. ./test-lib.sh
. "$TEST_DIRECTORY"/lib-credential.sh
+# If we're not given a specific external helper to run against,
+# there isn't much to test. But we can still run through our
+# battery of tests with a fake helper and check that the
+# test themselves are self-consistent and clean up after
+# themselves.
+#
+# We'll use the "store" helper, since we can easily inspect
+# its state by looking at the on-disk file. But since it doesn't
+# implement any caching or expiry logic, we'll cheat and override
+# the "check" function to just report all results as OK.
if test -z "$GIT_TEST_CREDENTIAL_HELPER"; then
- skip_all="used to test external credential helpers"
- test_done
+ GIT_TEST_CREDENTIAL_HELPER=store
+ GIT_TEST_CREDENTIAL_HELPER_TIMEOUT=store
+ check () {
+ test "$1" = "approve" || return 0
+ git -c credential.helper=store credential approve
+ }
+ check_cleanup=t
fi
test -z "$GIT_TEST_CREDENTIAL_HELPER_SETUP" ||
@@ -59,4 +74,11 @@ fi
# might be long-term system storage
helper_test_clean "$GIT_TEST_CREDENTIAL_HELPER"
+if test "$check_cleanup" = "t"
+then
+ test_expect_success 'test cleanup removes everything' '
+ test_must_be_empty "$HOME/.git-credentials"
+ '
+fi
+
test_done
--
2.44.0.rc1.410.g8325d5f159
^ permalink raw reply related
* Re: [PATCH] RelNotes: minor typo fixes in 2.44.0 draft
From: Kousik Sanagavarapu @ 2024-02-17 2:20 UTC (permalink / raw)
To: Todd Zullinger; +Cc: git, Junio C Hamano
In-Reply-To: <20240217002806.231340-1-tmz@pobox.com>
Todd Zullinger <tmz@pobox.com> wrote:
> - * When $HOME/.gitignore is missing but XDG config file available, we
> + * When $HOME/.gitconfig is missing but XDG config file available, we
s/file available/file is available
^ permalink raw reply
* Re: [PATCH v2 02/30] oid-array: teach oid-array to handle multiple kinds of oids
From: Linus Arver @ 2024-02-17 1:59 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Junio C Hamano, git, brian m. carlson, Eric Sunshine,
Eric W. Biederman
In-Reply-To: <87le7lkoai.fsf@gmail.froward.int.ebiederm.org>
"Eric W. Biederman" <ebiederm@gmail.com> writes:
> Linus Arver <linusa@google.com> writes:
>
>>
>> I would still prefer the "early return for errors" style even in this
>> case. This is because I much prefer to have the question "how can things
>> go wrong?" answered first, and dealt with, such that as I read
>> top-to-bottom I am left with less things I have to consider to
>> understand the "happy path". WRT emphasizing the "algos equal each
>> other" concern, a simple comment like
>
> void_hashcmp is a function that reports how two elements are ordered.
> There is no error handling.
>
> There are in fact two cases that need to be handled with oid_array being
> able to contain more then one kind of hash at a time.
>
> The two entries are ordered by oidcmp.
> The two entries are ordered by hash algorithm.
>
> The order that is maintained is first everything is ordered by hash
> algorithm, then for entries of identical hash algorithm they are ordered
> by oidcmp.
>
> So I don't think the concept of early return for errors can ever apply
> to any version of void_hashcmp.
Ah, yes. It appears that I simply read the code wrong. Thank you for the
clarification, and I agree with your analysis and preferred style.
Sorry for the noise.
^ permalink raw reply
* Re: Question about migrating a repository
From: Chris Torek @ 2024-02-17 1:22 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: Gabor Urban, git
In-Reply-To: <6965c59e-8edd-4d91-81da-3600a61569a3@app.fastmail.com>
On Fri, Feb 16, 2024 at 11:47 AM Kristoffer Haugsbakk
<code@khaugsbakk.name> wrote:
> Your repository is fine. There’s nothing that you need to do.
This is correct, but, if you'd like to make your local Git stop
mentioning `origin`
entirely, you can simply run:
git remote remove origin
which will cause your (new) local Git repository to forget that it got
copied from
somewhere else.
You can shorten the word `remove` to just `rm` (as in the Linux/Unix
"remove file" command).
Chris
^ permalink raw reply
* Re: [PATCH v2] diff: implement config.diff.renames=copies-harder
From: Sam James @ 2024-02-17 0:54 UTC (permalink / raw)
To: Sam James; +Cc: Elijah Newren, git, Junio C Hamano
In-Reply-To: <87wmr4aw9q.fsf@gentoo.org>
[-- Attachment #1: Type: text/plain, Size: 11374 bytes --]
Sam James <sam@gentoo.org> writes:
> [[PGP Signed Part:Undecided]]
>
> Elijah Newren <newren@gmail.com> writes:
>
>> Hi,
>>
>> It helps when providing a new iteration of a patch to do two things:
>> (1) provide a range-diff against the previous version to make it
>> easier for reviewers to see what has changed
>> (2) either reply to the previous submission or link to it in your
>> header so reviewers can find previous comments
>
> Thanks, I'll do that for upcoming v3.
>
>>
>> In this case, v1 was over at
>> https://lore.kernel.org/git/pull.1606.git.1699010701704.gitgitgadget@gmail.com/.
>>
>> On Tue, Dec 26, 2023 at 12:21 PM Sam James <sam@gentoo.org> wrote:
>>>
>>> This patch adds a config value for 'diff.renames' called 'copies-harder'
>>> which make it so '-C -C' is in effect always passed for 'git log -p',
>>> 'git diff', etc.
>>>
>>> This allows specifying that 'git log -p', 'git diff', etc should always act
>>> as if '-C --find-copies-harder' was passed.
>>>
>>> It has proven this especially useful for certain types of repository (like
>>> Gentoo's ebuild repositories) because files are often copies of a previous
>>> version:
>>>
>>> Suppose a directory 'sys-devel/gcc' contains recipes for building
>>> GCC, with one file for each supported upstream branch:
>>> gcc-13.x.build.recipe
>>> gcc-12.x.build.recipe
>>> gcc-11.x.build.recipe
>>> gcc-10.x.build.recipe
>>>
>>> gcc-13.x.build.recipe was started as a copy of gcc-12.x.build.recipe
>>> (which was started as a copy of gcc-11.x.build.recipe, etc.). Previous versions
>>> are kept around to support parallel installation of multiple versions.
>>>
>>> Being able to easily observe the diff relative to other recipes within the
>>> directory has been a quality of life improvement for such repo layouts.
>>>
>>> Cc: Elijah Newren <newren@gmail.com>
>>> Cc: Junio C Hamano <gitster@pobox.com>
>>> Signed-off-by: Sam James <sam@gentoo.org>
>>
>> While the "Cc:" lines don't hurt, note that everything above this
>> point is included in the commit message, so we'd typically rather
>> those two lines be below the triple-dashed line.
>
> ACK.
>
>>
>>> ---
>>> v2: Improve the commit message using Elijah Newren's example (it is indeed
>>> precisely what I was thinking of, but phrased better), and improve the documentation
>>> to explain better what the new config option actually does & refer to git-diff's
>>> --find-copies-harder.
>>>
>>> Documentation/config/diff.txt | 8 +++++---
>>> Documentation/config/status.txt | 3 ++-
>>> diff.c | 12 +++++++++---
>>> diff.h | 1 +
>>> diffcore-rename.c | 4 ++--
>>> merge-ort.c | 2 +-
>>> merge-recursive.c | 2 +-
>>> 7 files changed, 21 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/Documentation/config/diff.txt b/Documentation/config/diff.txt
>>> index bd5ae0c337..cdd8a74ec0 100644
>>> --- a/Documentation/config/diff.txt
>>> +++ b/Documentation/config/diff.txt
>>> @@ -131,9 +131,11 @@ diff.renames::
>>> Whether and how Git detects renames. If set to "false",
>>> rename detection is disabled. If set to "true", basic rename
>>> detection is enabled. If set to "copies" or "copy", Git will
>>> - detect copies, as well. Defaults to true. Note that this
>>> - affects only 'git diff' Porcelain like linkgit:git-diff[1] and
>>> - linkgit:git-log[1], and not lower level commands such as
>>> + detect copies, as well. If set to "copies-harder", Git will spend extra
>>> + cycles to find more copies even in unmodified paths, see
>>> + '--find-copies-harder' in linkgit:git-diff[1]. Defaults to true.
>>> + Note that this affects only 'git diff' Porcelain like linkgit:git-diff[1]
>>> + and linkgit:git-log[1], and not lower level commands such as
>>> linkgit:git-diff-files[1].
>>>
>>> diff.suppressBlankEmpty::
>>> diff --git a/Documentation/config/status.txt b/Documentation/config/status.txt
>>> index 2ff8237f8f..7ca7a4becd 100644
>>> --- a/Documentation/config/status.txt
>>> +++ b/Documentation/config/status.txt
>>> @@ -33,7 +33,8 @@ status.renames::
>>> Whether and how Git detects renames in linkgit:git-status[1] and
>>> linkgit:git-commit[1] . If set to "false", rename detection is
>>> disabled. If set to "true", basic rename detection is enabled.
>>> - If set to "copies" or "copy", Git will detect copies, as well.
>>> + If set to "copies" or "copy", Git will detect copies, as well. If
>>> + set to "copies-harder", Git will try harder to detect copies.
>>
>> It'd be nice to have the improved text from diff.renames here ("If set
>> to "copies-harder", Git will spend extra cycles to find more copies
>> even in unmodified paths.").
>>
>>> Defaults to the value of diff.renames.
>>>
>>> status.showStash::
>>> diff --git a/diff.c b/diff.c
>>> index a2def45644..585acf9a99 100644
>>> --- a/diff.c
>>> +++ b/diff.c
>>> @@ -206,8 +206,11 @@ int git_config_rename(const char *var, const char *value)
>>> {
>>> if (!value)
>>> return DIFF_DETECT_RENAME;
>>> + if (!strcasecmp(value, "copies-harder"))
>>> + return DIFF_DETECT_COPY_HARDER;
>>> if (!strcasecmp(value, "copies") || !strcasecmp(value, "copy"))
>>> - return DIFF_DETECT_COPY;
>>> + return DIFF_DETECT_COPY;
>>> +
>>
>> I pointed out in response to v1 that these last couple lines, while a
>> nice cleanup, should be in a separate patch.
>>
>>> return git_config_bool(var,value) ? DIFF_DETECT_RENAME : 0;
>>> }
>>>
>>> @@ -4849,8 +4852,11 @@ void diff_setup_done(struct diff_options *options)
>>> else
>>> options->flags.diff_from_contents = 0;
>>>
>>> - if (options->flags.find_copies_harder)
>>> + /* Just fold this in as it makes the patch-to-git smaller */
>>> + if (options->flags.find_copies_harder || options->detect_rename == DIFF_DETECT_COPY_HARDER) {
>>
>> Again, I already responded to v1 that four of your lines were too long
>> and needed to be split. All four remain unsplit in your resubmission.
>> For future reference, when you resubmit a patch, please either (a)
>> first respond to the review explaining why you don't agree with the
>> suggested changes, or (b) include the fixes reviewers point out, or
>> (c) state in your cover letter or explanation after the '---' why you
>> chose to not include the suggested changes.
>
> I apologise for the rookie errors, I don't really have an excuse either.
>
>>
>>> + options->flags.find_copies_harder = 1;
>>> options->detect_rename = DIFF_DETECT_COPY;
>>> + }
>>>
>>> if (!options->flags.relative_name)
>>> options->prefix = NULL;
>>> @@ -5281,7 +5287,7 @@ static int diff_opt_find_copies(const struct option *opt,
>>> if (*arg != 0)
>>> return error(_("invalid argument to %s"), opt->long_name);
>>>
>>> - if (options->detect_rename == DIFF_DETECT_COPY)
>>> + if (options->detect_rename == DIFF_DETECT_COPY || options->detect_rename == DIFF_DETECT_COPY_HARDER)
>>> options->flags.find_copies_harder = 1;
>>> else
>>> options->detect_rename = DIFF_DETECT_COPY;
>>> diff --git a/diff.h b/diff.h
>>> index 66bd8aeb29..b29e5b777f 100644
>>> --- a/diff.h
>>> +++ b/diff.h
>>> @@ -555,6 +555,7 @@ int git_config_rename(const char *var, const char *value);
>>>
>>> #define DIFF_DETECT_RENAME 1
>>> #define DIFF_DETECT_COPY 2
>>> +#define DIFF_DETECT_COPY_HARDER 3
>>>
>>> #define DIFF_PICKAXE_ALL 1
>>> #define DIFF_PICKAXE_REGEX 2
>>> diff --git a/diffcore-rename.c b/diffcore-rename.c
>>> index 5a6e2bcac7..856291d66f 100644
>>> --- a/diffcore-rename.c
>>> +++ b/diffcore-rename.c
>>> @@ -299,7 +299,7 @@ static int find_identical_files(struct hashmap *srcs,
>>> }
>>> /* Give higher scores to sources that haven't been used already */
>>> score = !source->rename_used;
>>> - if (source->rename_used && options->detect_rename != DIFF_DETECT_COPY)
>>> + if (source->rename_used && options->detect_rename != DIFF_DETECT_COPY && options->detect_rename != DIFF_DETECT_COPY_HARDER)
>>> continue;
>>> score += basename_same(source, target);
>>> if (score > best_score) {
>>> @@ -1405,7 +1405,7 @@ void diffcore_rename_extended(struct diff_options *options,
>>> trace2_region_enter("diff", "setup", options->repo);
>>> info.setup = 0;
>>> assert(!dir_rename_count || strmap_empty(dir_rename_count));
>>> - want_copies = (detect_rename == DIFF_DETECT_COPY);
>>> + want_copies = (detect_rename == DIFF_DETECT_COPY || detect_rename == DIFF_DETECT_COPY_HARDER);
>>> if (dirs_removed && (break_idx || want_copies))
>>> BUG("dirs_removed incompatible with break/copy detection");
>>> if (break_idx && relevant_sources)
>>> diff --git a/merge-ort.c b/merge-ort.c
>>> index 6491070d96..7749835465 100644
>>> --- a/merge-ort.c
>>> +++ b/merge-ort.c
>>> @@ -4782,7 +4782,7 @@ static void merge_start(struct merge_options *opt, struct merge_result *result)
>>> * sanity check them anyway.
>>> */
>>> assert(opt->detect_renames >= -1 &&
>>> - opt->detect_renames <= DIFF_DETECT_COPY);
>>> + opt->detect_renames <= DIFF_DETECT_COPY_HARDER);
>>> assert(opt->verbosity >= 0 && opt->verbosity <= 5);
>>> assert(opt->buffer_output <= 2);
>>> assert(opt->obuf.len == 0);
>>> diff --git a/merge-recursive.c b/merge-recursive.c
>>> index e3beb0801b..d52dd53660 100644
>>> --- a/merge-recursive.c
>>> +++ b/merge-recursive.c
>>> @@ -3708,7 +3708,7 @@ static int merge_start(struct merge_options *opt, struct tree *head)
>>> assert(opt->branch1 && opt->branch2);
>>>
>>> assert(opt->detect_renames >= -1 &&
>>> - opt->detect_renames <= DIFF_DETECT_COPY);
>>> + opt->detect_renames <= DIFF_DETECT_COPY_HARDER);
>>> assert(opt->detect_directory_renames >= MERGE_DIRECTORY_RENAMES_NONE &&
>>> opt->detect_directory_renames <= MERGE_DIRECTORY_RENAMES_TRUE);
>>> assert(opt->rename_limit >= -1);
>>> --
>>> 2.43.0
>>
>> The patch looks close, but it does continue to violate style
>> guidelines and make unrelated changes, like v1. And the wording in
>> one of the documentation blurbs could be improved a little. Looking
>> forward to a v3 with those fixed.
>
> I've not finished checking yet, but I think
> 5825268db1058516d05be03d6a8d8d55eea5a943 fixed a bug which I'd been
> relying on for something, where I may need to add another option for git
> log to suppress copies (for scripts).
>
> Should I send that in a series when doing v3 if it ends up being
> required?
Ah, nevermind on that, I figured it out & no need.
>
> thanks,
> sam
>
> [[End of PGP Signed Part]]
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 377 bytes --]
^ permalink raw reply
* [PATCH] RelNotes: minor typo fixes in 2.44.0 draft
From: Todd Zullinger @ 2024-02-17 0:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
Signed-off-by: Todd Zullinger <tmz@pobox.com>
---
Documentation/RelNotes/2.44.0.txt | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/Documentation/RelNotes/2.44.0.txt b/Documentation/RelNotes/2.44.0.txt
index 7dd8d75844..731c3f7592 100644
--- a/Documentation/RelNotes/2.44.0.txt
+++ b/Documentation/RelNotes/2.44.0.txt
@@ -3,7 +3,7 @@ Git v2.44 Release Notes
Backward Compatibility Notes
- * "git chekcout -B <branch>" used to allow switching to a branch that
+ * "git checkout -B <branch>" used to allow switching to a branch that
is in use on another worktree, but this was by mistake. The users
need to use "--ignore-other-worktrees" option.
@@ -54,7 +54,7 @@ UI, Workflows & Features
gitweb behaved as if the file did not exist at all, but now it
errors out. This is a change that may break backward compatibility.
- * When $HOME/.gitignore is missing but XDG config file available, we
+ * When $HOME/.gitconfig is missing but XDG config file available, we
should write into the latter, not former. "git gc" and "git
maintenance" wrote into a wrong "global config" file, which have
been corrected.
--
2.44.0.rc1
^ permalink raw reply related
* Re: [PATCH] git: extend --no-lazy-fetch to work across subprocesses
From: Linus Arver @ 2024-02-16 23:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King, Christian Couder
In-Reply-To: <xmqq7cj4ggir.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
>>> Do not model how the value of GIT_NO_REPLACE_OBJECTS environment
>>> variable is ignored, though. Just use the usual git_env_bool() to
>>> allow "export GIT_NO_LAZY_FETCH=0" and "unset GIT_NO_LAZY_FETCH"
>>> to be equivalents.
>>
>> s/equivalents/equivalent
>
> I meant to say that these two are "equivalents" (noun, plural).
Ah, I see.
> I can rephrase to
>
> Just use git_env_bool() to make "export GIT_NO_LAZY_FETCH=0"
> an equivalent to "unset GIT_NO_LAZY_FETCH".
>
> though, of course.
SGTM either way. Thanks.
^ permalink raw reply
* [PATCH v5 9/9] format_trailers_from_commit(): indirectly call trailer_info_get()
From: Linus Arver via GitGitGadget @ 2024-02-16 23:09 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.v5.git.1708124950.gitgitgadget@gmail.com>
From: Linus Arver <linusa@google.com>
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.
In a future patch, we'll change format_trailer_info() to use the parsed
trailer_item objects instead of the string 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..e6665c99cc3 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(trailers);
struct trailer_info info;
- trailer_info_get(opts, msg, &info);
+ parse_trailers(opts, &info, msg, &trailers);
+
/* 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(&trailers);
trailer_info_release(&info);
}
--
gitgitgadget
^ permalink raw reply related
* [PATCH v5 8/9] format_trailer_info(): move "fast path" to caller
From: Linus Arver via GitGitGadget @ 2024-02-16 23:09 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.v5.git.1708124950.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 v5 7/9] format_trailers(): use strbuf instead of FILE
From: Linus Arver via GitGitGadget @ 2024-02-16 23:09 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.v5.git.1708124950.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 v5 6/9] trailer_info_get(): reorder parameters
From: Linus Arver via GitGitGadget @ 2024-02-16 23:09 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.v5.git.1708124950.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 v5 5/9] trailer: start preparing for formatting unification
From: Linus Arver via GitGitGadget @ 2024-02-16 23:09 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.v5.git.1708124950.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).
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 v5 4/9] trailer: move interpret_trailers() to interpret-trailers.c
From: Linus Arver via GitGitGadget @ 2024-02-16 23:09 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.v5.git.1708124950.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 v5 3/9] trailer: prepare to expose functions as part of API
From: Linus Arver via GitGitGadget @ 2024-02-16 23:09 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.v5.git.1708124950.gitgitgadget@gmail.com>
From: Linus Arver <linusa@google.com>
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
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.
Take the opportunity to start putting trailer processing options (opts)
as the first parameter. This will be the pattern going forward in this
series.
Helped-by: Junio C Hamano <gitster@pobox.com>
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 v5 2/9] shortlog: add test for de-duplicating folded trailers
From: Linus Arver via GitGitGadget @ 2024-02-16 23:09 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.v5.git.1708124950.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 v5 1/9] trailer: free trailer_info _after_ all related usage
From: Linus Arver via GitGitGadget @ 2024-02-16 23:09 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.v5.git.1708124950.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 v5 0/9] Enrich Trailer API
From: Linus Arver via GitGitGadget @ 2024-02-16 23:09 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.v4.git.1707196348.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 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: prepare to expose functions as part of API
trailer: move interpret_trailers() to interpret-trailers.c
trailer: start preparing for formatting unification
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-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1632/listx/trailer-api-refactor-part-1-v5
Pull-Request: https://github.com/gitgitgadget/git/pull/1632
Range-diff vs v4:
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: 4372af244f0 trailer: prepare to expose functions as part of API
4: 4073b8eb510 = 4: 4073b8eb510 trailer: move interpret_trailers() to interpret-trailers.c
5: b2a0f7829a1 = 5: b2a0f7829a1 trailer: start preparing for formatting unification
6: c1760f80356 = 6: c1760f80356 trailer_info_get(): reorder parameters
7: 9dc912b5bc5 = 7: 9dc912b5bc5 format_trailers(): use strbuf instead of FILE
8: b97c06d8bc3 = 8: b97c06d8bc3 format_trailer_info(): move "fast path" to caller
9: 6906910417a ! 9: 7c656b3f775 format_trailers_from_commit(): indirectly call trailer_info_get()
@@ Commit message
format_trailer_info() only looks at the "trailers" string array, not the
trailer_item objects which parse_trailers() populates.
- In the next patch, we'll change format_trailer_info() to use the parsed
+ In a future patch, we'll change format_trailer_info() to use the parsed
trailer_item objects instead of the string array.
Signed-off-by: Linus Arver <linusa@google.com>
10: f5b7ba08aa7 < -: ----------- format_trailer_info(): use trailer_item objects
11: 457f2a839d5 < -: ----------- format_trailer_info(): drop redundant unfold_value()
12: a72eca301f7 < -: ----------- format_trailer_info(): append newline for non-trailer lines
13: ad77c33e457 < -: ----------- trailer: begin formatting unification
14: 11f854399db < -: ----------- format_trailer_info(): teach it about opts->trim_empty
15: ba1f387747b < -: ----------- format_trailer_info(): avoid double-printing the separator
16: 31725832224 < -: ----------- trailer: finish formatting unification
17: 6f17c022b15 < -: ----------- trailer: teach iterator about non-trailer lines
18: cc92dfb0bda < -: ----------- sequencer: use the trailer iterator
19: f5f0d06613f < -: ----------- trailer: make trailer_info struct private
20: 607ae7a90cd < -: ----------- trailer: retire trailer_info_get() from API
21: 38f4b4c4135 < -: ----------- trailer: spread usage of "trailer_block" language
22: 94bf182e3ff < -: ----------- trailer: prepare to delete "parse_trailers_from_command_line_args()"
23: 3bfe4809ecb < -: ----------- trailer: add new helper functions to API
24: 80e1958bb8d < -: ----------- trailer_add_arg_item(): drop new_trailer_item usage
25: a9080597a28 < -: ----------- trailer: deprecate "new_trailer_item" struct from API
26: 9720526dd8a < -: ----------- trailer: unify "--trailer ..." arg handling
27: 26df2514acb < -: ----------- trailer_set_*(): put out parameter at the end
28: 14927038d85 < -: ----------- trailer: introduce "template" term for readability
--
gitgitgadget
^ permalink raw reply
* Re: [PATCH] git: extend --no-lazy-fetch to work across subprocesses
From: Junio C Hamano @ 2024-02-16 23:01 UTC (permalink / raw)
To: Linus Arver; +Cc: git, Jeff King, Christian Couder
In-Reply-To: <owlyh6i882k4.fsf@fine.c.googlers.com>
Linus Arver <linusa@google.com> writes:
> FWIW, I see some typos. Otherwise this patch along with your "git:
> document GIT_NO_REPLACE_OBJECTS environment variable" one both LGTM for
> wording/readability. I must defer to Peff and others for "is this patch
> actually doing the right thing?" ;).
>
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Modeling after how the `--no-replace-objects` option is made usable
>> across subprocess spawning (e.g., cURL based remote helpers are
>> spawned as a separate process while running "git fetch"), allow the
>> `--no-lazy-fetch` option to be passed across process boundary.
>
> s/boundary/boundaries
Right; thanks.
>
>> Do not model how the value of GIT_NO_REPLACE_OBJECTS environment
>> variable is ignored, though. Just use the usual git_env_bool() to
>> allow "export GIT_NO_LAZY_FETCH=0" and "unset GIT_NO_LAZY_FETCH"
>> to be equivalents.
>
> s/equivalents/equivalent
I meant to say that these two are "equivalents" (noun, plural).
I can rephrase to
Just use git_env_bool() to make "export GIT_NO_LAZY_FETCH=0"
an equivalent to "unset GIT_NO_LAZY_FETCH".
though, of course.
^ permalink raw reply
* Re: [BUG] git commit --trailer --verbose puts trailer below scissors line
From: Linus Arver @ 2024-02-16 22:34 UTC (permalink / raw)
To: Philippe Blain, Git mailing list
In-Reply-To: <8b4738ad-62cd-789e-712e-bd45a151b4ac@gmail.com>
Philippe Blain <levraiphilippeblain@gmail.com> writes:
> Hello,
>
> I've just found a bug in the current master: running
>
> git commit --trailer="key: value" --verbose
>
> puts the trailer below the diff, and thus below the scissors
> line (and so it is discarded).
>
> I checked that it works OK in 2.42.1 but not in 2.43.2 so it is not
> a new regression in the 2.44 cycle, but I thought I'd write now in case
> someone spots the culprit commit faster than me.
>
> I'll start a bisection now.
>
> Cheers,
>
> Philippe.
Thank you for the bug report. Looking forward to reviewing it (I am also
interested because I am in the middle of a large cleanup of
trailer.{c,h} [1]).
[1] https://lore.kernel.org/git/owlyplwx87s9.fsf@fine.c.googlers.com/
^ permalink raw reply
* Re: [PATCH] git: extend --no-lazy-fetch to work across subprocesses
From: Linus Arver @ 2024-02-16 22:30 UTC (permalink / raw)
To: Junio C Hamano, git; +Cc: Jeff King, Christian Couder
In-Reply-To: <xmqqr0hcglpk.fsf_-_@gitster.g>
FWIW, I see some typos. Otherwise this patch along with your "git:
document GIT_NO_REPLACE_OBJECTS environment variable" one both LGTM for
wording/readability. I must defer to Peff and others for "is this patch
actually doing the right thing?" ;).
Junio C Hamano <gitster@pobox.com> writes:
> Modeling after how the `--no-replace-objects` option is made usable
> across subprocess spawning (e.g., cURL based remote helpers are
> spawned as a separate process while running "git fetch"), allow the
> `--no-lazy-fetch` option to be passed across process boundary.
s/boundary/boundaries
> Do not model how the value of GIT_NO_REPLACE_OBJECTS environment
> variable is ignored, though. Just use the usual git_env_bool() to
> allow "export GIT_NO_LAZY_FETCH=0" and "unset GIT_NO_LAZY_FETCH"
> to be equivalents.
s/equivalents/equivalent
^ 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