* Re: [PATCH v5 00/14] Introduce new `git replay` command
From: Elijah Newren @ 2023-10-30 17:18 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Christian Couder, git, Junio C Hamano, Patrick Steinhardt,
John Cai, Derrick Stolee, Phillip Wood, Calvin Wan, Toon Claes,
Dragan Simic, Linus Arver
In-Reply-To: <e04cfbdc-fd28-c645-8f5d-132f7ceec6be@gmx.de>
Hi Johannes,
On Sun, Oct 29, 2023 at 7:14 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> Hi Elijah,
>
> On Sat, 28 Oct 2023, Elijah Newren wrote:
>
> > On Tue, Oct 10, 2023 at 5:39 AM Christian Couder
> > <christian.couder@gmail.com> wrote:
> > > * Patch 12/15 (replay: disallow revision specific options and
> > > pathspecs) in version 4 has been removed, so there are now only 14
> > > patches instead of 15 in the series. This follows a suggestion by
> > > Dscho, and goes in the direction Elijah initially wanted before
> > > Derrick Stolee argued for disallowing revision specific options and
> > > pathspecs.
> >
> > [... snipping many parts that I agree with...]
> >
> > > Also instead of forcing reverse order we use the reverse order by
> > > default but allow it to be changed using `--reverse`. Thanks to
> > > Dscho.
> >
> > I can see why this might sometimes be useful for exclusively linear
> > history, but it seems to open a can of worms and possibly unfixable
> > corner cases for non-linear history. I'd rather not do this, or at
> > least pull it out of this series and let us discuss it in some follow
> > up series. There are some other alternatives that might handle such
> > usecases better.
>
> I find myself wishing for an easy way to reverse commits, if only to
> switch around the latest two commits while stopped during a rebase.
>
> So it would have been nice for me if there had been an easy, worktree-less
> way to make that happen.
Seems reasonable; we'll definitely want to keep this in mind.
> I guess this would be going in the direction of reordering commits,
> though, something we deliberately left for later?
Yes, I think that's a good framing for it.
Thanks,
Elijah
^ permalink raw reply
* Re: [PATCH] reflog: fix expire --single-worktree
From: Taylor Blau @ 2023-10-30 17:21 UTC (permalink / raw)
To: Junio C Hamano; +Cc: René Scharfe, Git List, John Cai
In-Reply-To: <xmqqa5s1hxhh.fsf@gitster.g>
On Mon, Oct 30, 2023 at 07:31:22AM +0900, Junio C Hamano wrote:
> René Scharfe <l.s.r@web.de> writes:
>
> > ... and added a non-printable short flag for it, presumably by
> > accident.
>
> Very well spotted.
>
> FWIW, with the following patch on top of this patch, all tests pass
> (and without your fix, of course this notices the "\001" and breaks
> numerous tests that use "git reflog"). So you seem to have found
> the only one broken instance (among those that are tested, anyway).
This makes sense to me, but obviously won't catch non-tested cases. I
thought that a new Cocinelle rule might be appropriate here, but it is
frustratingly difficult to specify a constraint like:
OPT_BOOL(e1, e2, e3, ...)
with
!(e1 == 0 || (33 <= e1 && e1 <= 127))
I'll think on it a little bit, but this seems low priority enough that I
don't feel compelled to urgently deal with adding a new Coccinelle rule.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH] sequencer: remove use of comment character
From: Elijah Newren @ 2023-10-30 17:26 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Tony Tung via GitGitGadget, git, Tony Tung
In-Reply-To: <xmqq7cn4g3nx.fsf@gitster.g>
On Sun, Oct 29, 2023 at 9:01 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Tony Tung via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Tony Tung <tonytung@merly.org>
> >
> > Instead of using the hardcoded `# `, use the
> > user-defined comment_line_char. Adds a test
> > to prevent regressions.
>
> Good spotting.
>
> Two observations.
>
> (1) There are a few more places that need similar treatment in the
> same file; you may want to fix them all while at it.
>
> (2) The second argument to strbuf_commented_addf() is always the
> comment_line_char global variable, not just inside this file
> but all callers across the codebase. We probably should drop
> it and have the strbuf_commented_addf() helper itself refer to
> the global. That way, if we ever want to change the global
> variable reference to something else (e.g. function call), we
> only have to touch a single place.
>
> The latter is meant as #leftoverbits and will be a lot wider
> clean-up that we may want to do long after this patch hits out
> codebase. The "other places" I spotted for the former are the
> following, but needs to be taken with a huge grain of salt, as it
> has not even been compile tested.
>
> Thanks.
>
> sequencer.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git c/sequencer.c w/sequencer.c
> index d584cac8ed..33208b1660 100644
> --- c/sequencer.c
> +++ w/sequencer.c
> @@ -1893,8 +1893,8 @@ static void update_squash_message_for_fixup(struct strbuf *msg)
> size_t orig_msg_len;
> int i = 1;
>
> - strbuf_addf(&buf1, "# %s\n", _(first_commit_msg_str));
> - strbuf_addf(&buf2, "# %s\n", _(skip_first_commit_msg_str));
> + strbuf_addf(&buf1, comment_line_char, "%s\n", _(first_commit_msg_str));
> + strbuf_addf(&buf2, comment_line_char, "%s\n", _(skip_first_commit_msg_str));
> s = start = orig_msg = strbuf_detach(msg, &orig_msg_len);
> while (s) {
> const char *next;
> @@ -2269,8 +2269,8 @@ static int do_pick_commit(struct repository *r,
> next = parent;
> next_label = msg.parent_label;
> if (opts->commit_use_reference) {
> - strbuf_addstr(&msgbuf,
> - "# *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***");
> + strbuf_commented_addf(&msgbuf, comment_line_char, "%s",
> + "*** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***");
> } else if (skip_prefix(msg.subject, "Revert \"", &orig_subject) &&
> /*
> * We don't touch pre-existing repeated reverts, because
>
I thought the point of the comment_line_char was so that commit
messages could have lines starting with '#'. That rationale doesn't
apply to the TODO list generation or parsing, and I'm not sure if we
want to add the same complexity there. If we do want to add the same
complexity there, I'm worried that making these changes are
insufficent; there are some other hardcoded '#' references in the code
(as a quick greps for '".*#' and "'#'" will turn up). Since those
other references include parsing as well as generation, I think we
might actually be introducing bugs in the TODO list handling if we
only partially convert it, but someone would need to double check.
^ permalink raw reply
* Re: Repository cloned using SSH does not respect bare repository initial branch
From: Jeff King @ 2023-10-30 17:43 UTC (permalink / raw)
To: Sheik; +Cc: git
In-Reply-To: <b310e254-f6d3-4715-b042-341bf5a98bbc@gmail.com>
On Tue, Oct 31, 2023 at 02:24:46AM +1100, Sheik wrote:
> Server version is same as client (v2.42.0) as I ran these commands all on
> the same machine.
OK. The next thing I'd check is running both commands with:
GIT_TRACE_PACKET=1 git clone ...
to see the protocol trace, and how it differs between the two. What I
suspect you may see is that the local clone is using the "v2" protocol
(a capabilities report, followed by "ls-refs", which mentions the symref
value of HEAD), and the ssh one uses the older "v0" (it goes straight to
the ref advertisement).
Quoting from 59e1205d16 (ls-refs: report unborn targets of symrefs,
2021-02-05), the commit I mentioned before:
This change is only for protocol v2. A similar change for protocol
v0 would require independent protocol design (there being no
analogous position to signal support for "unborn") and client-side
plumbing of the data required, so the scope of this patch set is
limited to protocol v2.
So in v0 the server doesn't pass back sufficient information for the
client to know about the name of the unborn HEAD branch.
If that's the culprit, the next question of course is why we'd do v2
locally versus v0 overssh. And that probably has to do with how we
trigger the protocol upgrade. To see if the server supports v2, the
client passes extra information "out of band". For git-over-http, this
happens in an extra HTTP header. For local repositories, it happens in
an environment variable ($GIT_PROTOCOL). For git-over-ssh it happens in
that sameenvironment variable, which we instruct the ssh client to pass
using "-o SendEnv". But:
1. If your ssh client doesn't look like openssh, we don't know if it
supports "-o" and may skip it. See the discussion in ssh.variant in
"git help config".
2. Some servers need to be configured to allow the client to set
environment variables. In the case of openssh, you'd want a line
like this in your sshd_config file:
AcceptEnv GIT_PROTOCOL
Of the two, I'd guess that the second one is more likely to be your
problem (since you're running Linux, where openssh is the norm).
-Peff
^ permalink raw reply
* Re: [PATCH] tests: handle "funny" exit code 127 produced by MSVC-compiled exes
From: Jeff King @ 2023-10-30 17:56 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget; +Cc: git, Johannes Schindelin
In-Reply-To: <pull.1604.git.1698680732691.gitgitgadget@gmail.com>
On Mon, Oct 30, 2023 at 03:45:32PM +0000, Johannes Schindelin via GitGitGadget wrote:
> Now, `jk/tree-name-and-depth-limit` introduces a pair of test cases that
> expect a command that produces a stack overflow to fail, which it
> typically does with exit code 139 (which means SIGSEGV).
I think you're misinterpreting the purpose of the tests from that
series; they're not intended to segfault. Quoting from t6700:
# We'll test against two depths here: a small one that will let us check the
# behavior of the config setting easily, and a large one that should be
# forbidden by default. Testing the default depth will let us know whether our
# default is enough to prevent segfaults on systems that run the tests.
So for the "big tree" tests in that file, we are looking for a
controlled failure rather than a segfault. And indeed, the end of that
series already lowered the default to accommodate the msys windows
build; see the discussion in 4d5693ba05 (lower core.maxTreeDepth default
to 2048, 2023-08-31).
So I think the test is working as designed here: it is showing us that
the default value is not sufficient to protect MSVC builds from running
out of stack space. There are a few options there:
1. We can lower the default everywhere.
2. We can lower it just for MSVC builds.
3. We can accept the situation and skip the tests for that build.
There's a bit more discussion in the commit I referenced above.
> Let's work around this by:
>
> 1) recording which C compiler was used, and
>
> 2) adding an MSVC-only exception to `test_must_fail` to treat 127 as a
> regular failure.
>
> There is a slight downside of this approach in that a real missing
> command could be mistaken for a failure. However, this would be caught
> on other platforms, and besides, we use `test_must_fail` only for `git`
> and `scalar` anymore, and we can be pretty certain that both are there.
I think there is another much worse downside to your patch: we will stop
noticing when MSVC builds segfault in the tests. The purpose of
test_must_fail is to allow controlled and expected failure returns from
the command, but still report on unexpected situations (signal death,
command not found, and so on).
-Peff
^ permalink raw reply
* Re: [PATCH v2 03/12] builtin/show-ref: fix leaking string buffer
From: Taylor Blau @ 2023-10-30 18:10 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Junio C Hamano, Eric Sunshine, Han-Wen Nienhuys
In-Reply-To: <bb0d656a0b40b79c90dc2505239976a93f18f432.1698314128.git.ps@pks.im>
On Thu, Oct 26, 2023 at 11:56:29AM +0200, Patrick Steinhardt wrote:
> Fix a leaking string buffer in `git show-ref --exclude-existing`. While
> the buffer is technically not leaking because its variable is declared
> as static, there is no inherent reason why it should be.
Well spotted and fixed. I ran the test suite in GIT_TEST_PASSING_SANITIZE_LEAK's
"check" mode and didn't find anything that was made leak-free by this
patch not already marked as such. So this (and the rest of the series up
to this point) LGTM.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH v3 5/8] ci: unify setup of some environment variables
From: Dragan Simic @ 2023-10-30 18:22 UTC (permalink / raw)
To: phillip.wood; +Cc: Patrick Steinhardt, git, Junio C Hamano, Oswald Buddenhagen
In-Reply-To: <87430c6c-91c0-4be1-b89d-bf442b3f018b@gmail.com>
On 2023-10-30 16:09, Phillip Wood wrote:
> On 30/10/2023 12:15, Patrick Steinhardt wrote:
>> Both GitHub Actions and Azue Pipelines set up the environment
>> variables
>> GIT_TEST_OPTS, GIT_PROVE_OPTS and MAKEFLAGS. And while most values are
>> actually the same, the setup is completely duplicate. With the
>> upcoming
>> support for GitLab CI this duplication would only extend even further.
>>
>> Unify the setup of those environment variables so that only the
>> uncommon
>> parts are separated. While at it, we also perform some additional
>> small
>> improvements:
>>
>> - We use nproc instead of a hardcoded count of jobs for make and
>> prove. This ensures that the number of concurrent processes
>> adapts
>> to the host automatically.
>
> Sadly this makes the Windows and MacOS jobs fail on GitHub Actions as
> nproc is not installed[1]. Perhaps we could do
>
> --jobs="$(nproc || echo 2)"
It would be better to use the following, to also suppress any error
messages:
--jobs=$(nproc 2> /dev/null || echo 2)
Having the quotation marks is also pretty much redundant.
> instead. (Maybe 2 is a bit low but the current value of 10 seems
> pretty high for the number of cores on the runners that we use)
^ permalink raw reply
* Re: [PATCH v2 04/12] builtin/show-ref: fix dead code when passing patterns
From: Taylor Blau @ 2023-10-30 18:24 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Junio C Hamano, Eric Sunshine, Han-Wen Nienhuys
In-Reply-To: <87afcee830caab2782ce693c1f961df6fea6e7b5.1698314128.git.ps@pks.im>
On Thu, Oct 26, 2023 at 11:56:33AM +0200, Patrick Steinhardt wrote:
> When passing patterns to `git show-ref` we have some code that will
> cause us to die if `verify && !quiet` is true. But because `verify`
> indicates a different subcommand of git-show-ref(1) that causes us to
> execute `cmd_show_ref__verify()` and not `cmd_show_ref__patterns()`, the
> condition cannot ever be true.
>
> Let's remove this dead code.
Makes sense. Let's read on...
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH v2 05/12] builtin/show-ref: refactor `--exclude-existing` options
From: Taylor Blau @ 2023-10-30 18:37 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Junio C Hamano, Eric Sunshine, Han-Wen Nienhuys
In-Reply-To: <bed2a8a07696371e07c0b2d1282ed51c0b1b9fee.1698314128.git.ps@pks.im>
On Thu, Oct 26, 2023 at 11:56:37AM +0200, Patrick Steinhardt wrote:
> It's not immediately obvious options which options are applicable to
> what subcommand in git-show-ref(1) because all options exist as global
> state. This can easily cause confusion for the reader.
>
> Refactor options for the `--exclude-existing` subcommand to be contained
> in a separate structure. This structure is stored on the stack and
> passed down as required. Consequently, it clearly delimits the scope of
> those options and requires the reader to worry less about global state.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
All makes sense, but...
> @@ -19,8 +19,7 @@ static const char * const show_ref_usage[] = {
> };
>
> static int deref_tags, show_head, tags_only, heads_only, found_match, verify,
> - quiet, hash_only, abbrev, exclude_arg;
> -static const char *exclude_existing_arg;
> + quiet, hash_only, abbrev;
>
> static void show_one(const char *refname, const struct object_id *oid)
> {
> @@ -95,6 +94,11 @@ static int add_existing(const char *refname,
> return 0;
> }
>
> +struct exclude_existing_options {
> + int enabled;
...do we need an .enabled here? I think checking whether or not .pattern
is NULL is sufficient, but perhaps there is another use of .enabled
later on in the series...
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH v2 05/12] builtin/show-ref: refactor `--exclude-existing` options
From: Taylor Blau @ 2023-10-30 18:55 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Junio C Hamano, Eric Sunshine, Han-Wen Nienhuys
In-Reply-To: <bed2a8a07696371e07c0b2d1282ed51c0b1b9fee.1698314128.git.ps@pks.im>
On Thu, Oct 26, 2023 at 11:56:37AM +0200, Patrick Steinhardt wrote:
> @@ -95,6 +94,11 @@ static int add_existing(const char *refname,
> return 0;
> }
>
> +struct exclude_existing_options {
> + int enabled;
> + const char *pattern;
> +};
> +
Thinking on my earlier suggestion more, I wondered if using the
OPT_SUBCOMMAND() function might make things easier to organize and
eliminate the need for things like .enabled or having to define structs
for each of the sub-commands.
But I don't think that this is (easily) possible to do, since
`--exclude-existing` is behind a command-line option, not a separate
mode (e.g. "commit-graph verify", not "commit-graph --verify"). So I
think you *could* make it work with some combination of OPT_SUBCOMMAND
and callbacks to set the function pointer yourself when given the
`--exclude-existing` option. But I think that's sufficiently gross as to
not be worth it.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH v2 06/12] builtin/show-ref: stop using global variable to count matches
From: Taylor Blau @ 2023-10-30 19:14 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Junio C Hamano, Eric Sunshine, Han-Wen Nienhuys
In-Reply-To: <d52a5e8ced2adc5c9315edea9fc497d1ffa30125.1698314128.git.ps@pks.im>
On Thu, Oct 26, 2023 at 11:56:42AM +0200, Patrick Steinhardt wrote:
> When passing patterns to git-show-ref(1) we're checking whether any
> reference matches -- if none does, we indicate this condition via an
> unsuccessful exit code.
s/does/do, but not a big enough deal to reroll IMHO.
> We're using a global variable to count these matches, which is required
> because the counter is getting incremented in a callback function. But
> now that we have the `struct show_ref_data` in place, we can get rid of
> the global variable and put the counter in there instead.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> builtin/show-ref.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
Looks all good to me!
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH v2 09/12] builtin/show-ref: ensure mutual exclusiveness of subcommands
From: Taylor Blau @ 2023-10-30 19:31 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Junio C Hamano, Eric Sunshine, Han-Wen Nienhuys
In-Reply-To: <5ba566723e8742e6df150b12f1d044089ff62b59.1698314128.git.ps@pks.im>
On Thu, Oct 26, 2023 at 11:56:57AM +0200, Patrick Steinhardt wrote:
> The git-show-ref(1) command has three different modes, of which one is
> implicit and the other two can be chosen explicitly by passing a flag.
> But while these modes are standalone and cause us to execute completely
> separate code paths, we gladly accept the case where a user asks for
> both `--exclude-existing` and `--verify` at the same time even though it
> is not obvious what will happen. Spoiler: we ignore `--verify` and
> execute the `--exclude-existing` mode.
>
> Let's explicitly detect this invalid usage and die in case both modes
> were requested.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> builtin/show-ref.c | 4 ++++
> t/t1403-show-ref.sh | 5 +++++
> 2 files changed, 9 insertions(+)
>
> diff --git a/builtin/show-ref.c b/builtin/show-ref.c
> index 87bc45d2d13..1768aef77b3 100644
> --- a/builtin/show-ref.c
> +++ b/builtin/show-ref.c
> @@ -271,6 +271,10 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix)
> argc = parse_options(argc, argv, prefix, show_ref_options,
> show_ref_usage, 0);
>
> + if ((!!exclude_existing_opts.enabled + !!verify) > 1)
> + die(_("only one of '%s' or '%s' can be given"),
> + "--exclude-existing", "--verify");
> +
This is technically correct, but I was surprised to see it written this
way instead of
if (exclude_existing_opts.enabled && verify)
die(...);
I don't think it's a big deal either way, I was just curious why you
chose one over the other.
> +test_expect_success 'show-ref sub-modes are mutually exclusive' '
> + test_must_fail git show-ref --verify --exclude-existing 2>err &&
> + grep "only one of ${SQ}--exclude-existing${SQ} or ${SQ}--verify${SQ} can be given" err
> +'
grepping is fine here, but since you have the exact error message, it
may be worth switching to test_cmp.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH v2 00/12] show-ref: introduce mode to check for ref existence
From: Taylor Blau @ 2023-10-30 19:32 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Junio C Hamano, Eric Sunshine, Han-Wen Nienhuys
In-Reply-To: <cover.1698314128.git.ps@pks.im>
On Thu, Oct 26, 2023 at 11:56:16AM +0200, Patrick Steinhardt wrote:
> Hi,
>
> this is the second version of my patch series that introduces a new `git
> show-ref --exists` mode to check for reference existence.
All looks quite reasonable to me. I'm happy with this series as-is,
though I left a few minor comments and suggestions throughout.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH] merge: --ff-one-only to apply FF if commit is one
From: Taylor Blau @ 2023-10-30 20:01 UTC (permalink / raw)
To: Ruslan Yakauleu via GitGitGadget; +Cc: git, Ruslan Yakauleu
In-Reply-To: <pull.1599.git.git.1698224280816.gitgitgadget@gmail.com>
Hi Ruslan,
On Wed, Oct 25, 2023 at 08:58:00AM +0000, Ruslan Yakauleu via GitGitGadget wrote:
> From: Ruslan Yakauleu <ruslan.yakauleu@gmail.com>
>
> A new option --ff-one-only to control the merging strategy.
> For one commit option works like -ff to avoid extra merge commit.
> In other cases the option works like --no-ff to create merge commit for
> complex features.
This seems like a pretty niche feature to want to introduce a new option
for. I would imagine the alternative is something like:
ff="--no-ff"
if test 1 -eq $(git rev-list @{u}..)
then
ff="--ff"
fi
[on upstream @{u}]
git merge "$ff" "$branch"
I don't have a great sense of how many users might want or benefit from
something like this. My sense is that there aren't many, but I could
very easily be wrong here.
In any case, my sense is that this is probably too niche to introduce a
new command-line option just to implement this behavior when the above
implementation is pretty straightforward. Regardless, here's my review
of the patch...
> @@ -631,6 +633,8 @@ static int git_merge_config(const char *k, const char *v,
> fast_forward = boolval ? FF_ALLOW : FF_NO;
> } else if (v && !strcmp(v, "only")) {
> fast_forward = FF_ONLY;
> + } else if (v && !strcmp(v, "one-only")) {
> + fast_forward = FF_ONE_ONLY;
The configuration handling and documentation all look good.
> } /* do not barf on values from future versions of git */
> return 0;
> } else if (!strcmp(k, "merge.defaulttoupstream")) {
> @@ -1527,6 +1531,18 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
> free(list);
> }
>
> + if (fast_forward == FF_ONE_ONLY) {
> + fast_forward = FF_NO;
> +
> + /* check that we have one and only one commit to merge */
> + if (squash || ((!remoteheads->next &&
> + !common->next &&
> + oideq(&common->item->object.oid, &head_commit->object.oid)) &&
> + oideq(&remoteheads->item->parents->item->object.oid, &head_commit->object.oid))) {
> + fast_forward = FF_ALLOW;
> + }
And this rather long conditional looks right, too. This patch could
definitely benefit from some tests, though...
Thanks,
Taylor
^ permalink raw reply
* [RFC PATCH 0/3] Avoid passing global comment_line_char repeatedly
From: Jonathan Tan @ 2023-10-30 20:22 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, Junio C Hamano, Phillip Wood, Dragan Simic
In-Reply-To: <db6702ba-11a7-44c1-af2a-95b080aaeb77@gmail.com>
> While I agree with your reasoning here, I think that parameter was
> recently added as part of the libification effort - I can't remember
> exactly why and am too lazy to look it up so I've cc'd Calvin and
> Johathan instead.
Thanks Phillip for noticing this. Putting on my libification hat,
this was probably because we wanted to remove strbuf's dependency on
environment, so that we wouldn't need to include it in git-std-lib. If
we were to merge these patches, libification would probably still be
doable if we stubbed the global comment_line_char.
Removing my libification hat, I think it's better to solve this issue
by moving the functions into environment.{c,h} instead, following
the example of functions like strbuf_worktree_ref() in worktree.h
and strbuf_utf8_align() in utf8.h that, when operating on both strbuf
and a specific domain, are placed in the domain's header file, not in
strbuf.h. This avoids a situation in which strbuf.h contains everything
string-related.
The main issue with this is that by not centralizing all strbuf-related
functionality, some strbuf-related helper functions that could have been
private now need to be made public, but I think that a similar issue
would be faced if we don't centralize, say, all environment-related
functionality (some environment-related helper functions would have to
be made public, although I didn't encounter this problem with this patch
set).
I've attached some patches to illustrate what I've described above.
Jonathan Tan (1):
strbuf: make add_lines() public
Junio C Hamano (2):
strbuf_commented_addf(): drop the comment_line_char parameter
strbuf_add_commented_lines(): drop the comment_line_char parameter
add-patch.c | 8 ++---
branch.c | 3 +-
builtin/branch.c | 2 +-
builtin/merge.c | 8 ++---
builtin/notes.c | 9 +++---
builtin/stripspace.c | 2 +-
builtin/tag.c | 4 +--
commit.c | 2 +-
environment.c | 31 ++++++++++++++++++++
environment.h | 14 +++++++++
fmt-merge-msg.c | 9 ++----
rebase-interactive.c | 8 ++---
sequencer.c | 14 ++++-----
strbuf.c | 69 ++++++++++----------------------------------
strbuf.h | 19 ++----------
wt-status.c | 6 ++--
16 files changed, 98 insertions(+), 110 deletions(-)
--
2.42.0.820.g83a721a137-goog
^ permalink raw reply
* [RFC PATCH 1/3] strbuf: make add_lines() public
From: Jonathan Tan @ 2023-10-30 20:22 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, Junio C Hamano, Phillip Wood, Dragan Simic
In-Reply-To: <cover.1698696798.git.jonathantanmy@google.com>
Subsequent patches will require the ability to add different prefixes
to different lines (depending on their contents), so make this
functionality available from outside strbuf.c.
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
branch.c | 3 ++-
commit.c | 2 +-
strbuf.c | 39 ++++++++++++++++-----------------------
strbuf.h | 3 ++-
4 files changed, 21 insertions(+), 26 deletions(-)
diff --git a/branch.c b/branch.c
index 06f7af9dd4..04a8b90b6a 100644
--- a/branch.c
+++ b/branch.c
@@ -721,7 +721,8 @@ static int submodule_create_branch(struct repository *r,
return ret;
ret = finish_command(&child);
strbuf_read(&child_err, child.err, 0);
- strbuf_add_lines(&out_buf, out_prefix, child_err.buf, child_err.len);
+ strbuf_add_lines(&out_buf, out_prefix, out_prefix,
+ child_err.buf, child_err.len);
if (ret)
fprintf(stderr, "%s", out_buf.buf);
diff --git a/commit.c b/commit.c
index b3223478bc..7caafcde01 100644
--- a/commit.c
+++ b/commit.c
@@ -1361,7 +1361,7 @@ static void add_extra_header(struct strbuf *buffer,
{
strbuf_addstr(buffer, extra->key);
if (extra->len)
- strbuf_add_lines(buffer, " ", extra->value, extra->len);
+ strbuf_add_lines(buffer, " ", " ", extra->value, extra->len);
else
strbuf_addch(buffer, '\n');
}
diff --git a/strbuf.c b/strbuf.c
index 7827178d8e..9ee639519a 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -339,26 +339,6 @@ void strbuf_addf(struct strbuf *sb, const char *fmt, ...)
va_end(ap);
}
-static void add_lines(struct strbuf *out,
- const char *prefix1,
- const char *prefix2,
- const char *buf, size_t size)
-{
- while (size) {
- const char *prefix;
- const char *next = memchr(buf, '\n', size);
- next = next ? (next + 1) : (buf + size);
-
- prefix = ((prefix2 && (buf[0] == '\n' || buf[0] == '\t'))
- ? prefix2 : prefix1);
- strbuf_addstr(out, prefix);
- strbuf_add(out, buf, next - buf);
- size -= next - buf;
- buf = next;
- }
- strbuf_complete_line(out);
-}
-
void strbuf_add_commented_lines(struct strbuf *out, const char *buf,
size_t size, char comment_line_char)
{
@@ -369,7 +349,7 @@ void strbuf_add_commented_lines(struct strbuf *out, const char *buf,
xsnprintf(prefix1, sizeof(prefix1), "%c ", comment_line_char);
xsnprintf(prefix2, sizeof(prefix2), "%c", comment_line_char);
}
- add_lines(out, prefix1, prefix2, buf, size);
+ strbuf_add_lines(out, prefix1, prefix2, buf, size);
}
void strbuf_commented_addf(struct strbuf *sb, char comment_line_char,
@@ -747,10 +727,23 @@ ssize_t strbuf_read_file(struct strbuf *sb, const char *path, size_t hint)
return len;
}
-void strbuf_add_lines(struct strbuf *out, const char *prefix,
+void strbuf_add_lines(struct strbuf *out, const char *default_prefix,
+ const char *tab_or_nl_prefix,
const char *buf, size_t size)
{
- add_lines(out, prefix, NULL, buf, size);
+ while (size) {
+ const char *prefix;
+ const char *next = memchr(buf, '\n', size);
+ next = next ? (next + 1) : (buf + size);
+
+ prefix = (buf[0] == '\n' || buf[0] == '\t')
+ ? tab_or_nl_prefix : default_prefix;
+ strbuf_addstr(out, prefix);
+ strbuf_add(out, buf, next - buf);
+ size -= next - buf;
+ buf = next;
+ }
+ strbuf_complete_line(out);
}
void strbuf_addstr_xml_quoted(struct strbuf *buf, const char *s)
diff --git a/strbuf.h b/strbuf.h
index e959caca87..3559e73dd8 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -599,7 +599,8 @@ void strbuf_list_free(struct strbuf **list);
void strbuf_strip_file_from_path(struct strbuf *sb);
void strbuf_add_lines(struct strbuf *sb,
- const char *prefix,
+ const char *default_prefix,
+ const char *tab_or_nl_prefix,
const char *buf,
size_t size);
--
2.42.0.820.g83a721a137-goog
^ permalink raw reply related
* [RFC PATCH 2/3] strbuf_commented_addf(): drop the comment_line_char parameter
From: Jonathan Tan @ 2023-10-30 20:22 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Phillip Wood, Dragan Simic, Jonathan Tan
In-Reply-To: <cover.1698696798.git.jonathantanmy@google.com>
From: Junio C Hamano <gitster@pobox.com>
All the callers of this function supply the global variable
comment_line_char as an argument to its second parameter. Remove
the parameter to allow us in the future to change the reference to
the global variable with something else, like a function call.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
add-patch.c | 8 ++++----
builtin/branch.c | 2 +-
builtin/merge.c | 8 ++++----
builtin/tag.c | 4 ++--
environment.c | 18 ++++++++++++++++++
environment.h | 7 +++++++
rebase-interactive.c | 2 +-
sequencer.c | 4 ++--
strbuf.c | 19 +------------------
strbuf.h | 7 -------
wt-status.c | 2 +-
11 files changed, 41 insertions(+), 40 deletions(-)
diff --git a/add-patch.c b/add-patch.c
index bfe19876cd..471a0037be 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1106,11 +1106,11 @@ static int edit_hunk_manually(struct add_p_state *s, struct hunk *hunk)
size_t i;
strbuf_reset(&s->buf);
- strbuf_commented_addf(&s->buf, comment_line_char,
+ strbuf_commented_addf(&s->buf,
_("Manual hunk edit mode -- see bottom for "
"a quick guide.\n"));
render_hunk(s, hunk, 0, 0, &s->buf);
- strbuf_commented_addf(&s->buf, comment_line_char,
+ strbuf_commented_addf(&s->buf,
_("---\n"
"To remove '%c' lines, make them ' ' lines "
"(context).\n"
@@ -1119,13 +1119,13 @@ static int edit_hunk_manually(struct add_p_state *s, struct hunk *hunk)
s->mode->is_reverse ? '+' : '-',
s->mode->is_reverse ? '-' : '+',
comment_line_char);
- strbuf_commented_addf(&s->buf, comment_line_char, "%s",
+ strbuf_commented_addf(&s->buf, "%s",
_(s->mode->edit_hunk_hint));
/*
* TRANSLATORS: 'it' refers to the patch mentioned in the previous
* messages.
*/
- strbuf_commented_addf(&s->buf, comment_line_char,
+ strbuf_commented_addf(&s->buf,
_("If it does not apply cleanly, you will be "
"given an opportunity to\n"
"edit again. If all lines of the hunk are "
diff --git a/builtin/branch.c b/builtin/branch.c
index 2ec190b14a..b2f171e10b 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -668,7 +668,7 @@ static int edit_branch_description(const char *branch_name)
exists = !read_branch_desc(&buf, branch_name);
if (!buf.len || buf.buf[buf.len-1] != '\n')
strbuf_addch(&buf, '\n');
- strbuf_commented_addf(&buf, comment_line_char,
+ strbuf_commented_addf(&buf,
_("Please edit the description for the branch\n"
" %s\n"
"Lines starting with '%c' will be stripped.\n"),
diff --git a/builtin/merge.c b/builtin/merge.c
index d748d46e13..8f0e8be7c3 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -857,15 +857,15 @@ static void prepare_to_commit(struct commit_list *remoteheads)
strbuf_addch(&msg, '\n');
if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS) {
wt_status_append_cut_line(&msg);
- strbuf_commented_addf(&msg, comment_line_char, "\n");
+ strbuf_commented_addf(&msg, "\n");
}
- strbuf_commented_addf(&msg, comment_line_char,
+ strbuf_commented_addf(&msg,
_(merge_editor_comment));
if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS)
- strbuf_commented_addf(&msg, comment_line_char,
+ strbuf_commented_addf(&msg,
_(scissors_editor_comment));
else
- strbuf_commented_addf(&msg, comment_line_char,
+ strbuf_commented_addf(&msg,
_(no_scissors_editor_comment), comment_line_char);
}
if (signoff)
diff --git a/builtin/tag.c b/builtin/tag.c
index 3918eacbb5..a85a0d8def 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -314,10 +314,10 @@ static void create_tag(const struct object_id *object, const char *object_ref,
struct strbuf buf = STRBUF_INIT;
strbuf_addch(&buf, '\n');
if (opt->cleanup_mode == CLEANUP_ALL)
- strbuf_commented_addf(&buf, comment_line_char,
+ strbuf_commented_addf(&buf,
_(tag_template), tag, comment_line_char);
else
- strbuf_commented_addf(&buf, comment_line_char,
+ strbuf_commented_addf(&buf,
_(tag_template_nocleanup), tag, comment_line_char);
write_or_die(fd, buf.buf, buf.len);
strbuf_release(&buf);
diff --git a/environment.c b/environment.c
index bb3c2a96a3..d9f64cffa0 100644
--- a/environment.c
+++ b/environment.c
@@ -416,3 +416,21 @@ int print_sha1_ellipsis(void)
}
return cached_result;
}
+
+void strbuf_commented_addf(struct strbuf *sb,
+ const char *fmt, ...)
+{
+ va_list params;
+ struct strbuf buf = STRBUF_INIT;
+ int incomplete_line = sb->len && sb->buf[sb->len - 1] != '\n';
+
+ va_start(params, fmt);
+ strbuf_vaddf(&buf, fmt, params);
+ va_end(params);
+
+ strbuf_add_commented_lines(sb, buf.buf, buf.len, comment_line_char);
+ if (incomplete_line)
+ sb->buf[--sb->len] = '\0';
+
+ strbuf_release(&buf);
+}
diff --git a/environment.h b/environment.h
index e5351c9dd9..5778f5a8e4 100644
--- a/environment.h
+++ b/environment.h
@@ -229,4 +229,11 @@ extern const char *excludes_file;
*/
int print_sha1_ellipsis(void);
+/**
+ * Add a formatted string prepended by a comment character and a
+ * blank to the buffer.
+ */
+__attribute__((format (printf, 2, 3)))
+void strbuf_commented_addf(struct strbuf *sb, const char *fmt, ...);
+
#endif
diff --git a/rebase-interactive.c b/rebase-interactive.c
index d9718409b3..3f33da7f03 100644
--- a/rebase-interactive.c
+++ b/rebase-interactive.c
@@ -71,7 +71,7 @@ void append_todo_help(int command_count,
if (!edit_todo) {
strbuf_addch(buf, '\n');
- strbuf_commented_addf(buf, comment_line_char,
+ strbuf_commented_addf(buf,
Q_("Rebase %s onto %s (%d command)",
"Rebase %s onto %s (%d commands)",
command_count),
diff --git a/sequencer.c b/sequencer.c
index d584cac8ed..5d348a3f12 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -675,11 +675,11 @@ void append_conflicts_hint(struct index_state *istate,
}
strbuf_addch(msgbuf, '\n');
- strbuf_commented_addf(msgbuf, comment_line_char, "Conflicts:\n");
+ strbuf_commented_addf(msgbuf, "Conflicts:\n");
for (i = 0; i < istate->cache_nr;) {
const struct cache_entry *ce = istate->cache[i++];
if (ce_stage(ce)) {
- strbuf_commented_addf(msgbuf, comment_line_char,
+ strbuf_commented_addf(msgbuf,
"\t%s\n", ce->name);
while (i < istate->cache_nr &&
!strcmp(ce->name, istate->cache[i]->name))
diff --git a/strbuf.c b/strbuf.c
index 9ee639519a..b1717270a2 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -1,4 +1,5 @@
#include "git-compat-util.h"
+#include "environment.h"
#include "gettext.h"
#include "hex-ll.h"
#include "strbuf.h"
@@ -352,24 +353,6 @@ void strbuf_add_commented_lines(struct strbuf *out, const char *buf,
strbuf_add_lines(out, prefix1, prefix2, buf, size);
}
-void strbuf_commented_addf(struct strbuf *sb, char comment_line_char,
- const char *fmt, ...)
-{
- va_list params;
- struct strbuf buf = STRBUF_INIT;
- int incomplete_line = sb->len && sb->buf[sb->len - 1] != '\n';
-
- va_start(params, fmt);
- strbuf_vaddf(&buf, fmt, params);
- va_end(params);
-
- strbuf_add_commented_lines(sb, buf.buf, buf.len, comment_line_char);
- if (incomplete_line)
- sb->buf[--sb->len] = '\0';
-
- strbuf_release(&buf);
-}
-
void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap)
{
int len;
diff --git a/strbuf.h b/strbuf.h
index 3559e73dd8..4c58dc25e9 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -374,13 +374,6 @@ void strbuf_humanise_rate(struct strbuf *buf, off_t bytes);
__attribute__((format (printf,2,3)))
void strbuf_addf(struct strbuf *sb, const char *fmt, ...);
-/**
- * Add a formatted string prepended by a comment character and a
- * blank to the buffer.
- */
-__attribute__((format (printf, 3, 4)))
-void strbuf_commented_addf(struct strbuf *sb, char comment_line_char, const char *fmt, ...);
-
__attribute__((format (printf,2,0)))
void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap);
diff --git a/wt-status.c b/wt-status.c
index 9f45bf6949..54b2775730 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1102,7 +1102,7 @@ void wt_status_append_cut_line(struct strbuf *buf)
{
const char *explanation = _("Do not modify or remove the line above.\nEverything below it will be ignored.");
- strbuf_commented_addf(buf, comment_line_char, "%s", cut_line);
+ strbuf_commented_addf(buf, "%s", cut_line);
strbuf_add_commented_lines(buf, explanation, strlen(explanation), comment_line_char);
}
--
2.42.0.820.g83a721a137-goog
^ permalink raw reply related
* [RFC PATCH 3/3] strbuf_add_commented_lines(): drop the comment_line_char parameter
From: Jonathan Tan @ 2023-10-30 20:22 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Phillip Wood, Dragan Simic, Jonathan Tan
In-Reply-To: <cover.1698696798.git.jonathantanmy@google.com>
From: Junio C Hamano <gitster@pobox.com>
All the callers of this function supply the global variable
comment_line_char as an argument to its last parameter. Remove the
parameter to allow us in the future to change the reference to the
global variable with something else, like a function call.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
builtin/notes.c | 9 ++++-----
builtin/stripspace.c | 2 +-
environment.c | 15 ++++++++++++++-
environment.h | 7 +++++++
fmt-merge-msg.c | 9 +++------
rebase-interactive.c | 6 +++---
sequencer.c | 10 ++++------
strbuf.c | 13 -------------
strbuf.h | 9 ---------
wt-status.c | 4 ++--
10 files changed, 38 insertions(+), 46 deletions(-)
diff --git a/builtin/notes.c b/builtin/notes.c
index 9f38863dd5..355ecce07a 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -181,7 +181,7 @@ static void write_commented_object(int fd, const struct object_id *object)
if (strbuf_read(&buf, show.out, 0) < 0)
die_errno(_("could not read 'show' output"));
- strbuf_add_commented_lines(&cbuf, buf.buf, buf.len, comment_line_char);
+ strbuf_add_commented_lines(&cbuf, buf.buf, buf.len);
write_or_die(fd, cbuf.buf, cbuf.len);
strbuf_release(&cbuf);
@@ -209,10 +209,9 @@ static void prepare_note_data(const struct object_id *object, struct note_data *
copy_obj_to_fd(fd, old_note);
strbuf_addch(&buf, '\n');
- strbuf_add_commented_lines(&buf, "\n", strlen("\n"), comment_line_char);
- strbuf_add_commented_lines(&buf, _(note_template), strlen(_(note_template)),
- comment_line_char);
- strbuf_add_commented_lines(&buf, "\n", strlen("\n"), comment_line_char);
+ strbuf_add_commented_lines(&buf, "\n", strlen("\n"));
+ strbuf_add_commented_lines(&buf, _(note_template), strlen(_(note_template)));
+ strbuf_add_commented_lines(&buf, "\n", strlen("\n"));
write_or_die(fd, buf.buf, buf.len);
write_commented_object(fd, object);
diff --git a/builtin/stripspace.c b/builtin/stripspace.c
index 7b700a9fb1..11e475760c 100644
--- a/builtin/stripspace.c
+++ b/builtin/stripspace.c
@@ -13,7 +13,7 @@ static void comment_lines(struct strbuf *buf)
size_t len;
msg = strbuf_detach(buf, &len);
- strbuf_add_commented_lines(buf, msg, len, comment_line_char);
+ strbuf_add_commented_lines(buf, msg, len);
free(msg);
}
diff --git a/environment.c b/environment.c
index d9f64cffa0..cc1b85afb6 100644
--- a/environment.c
+++ b/environment.c
@@ -428,9 +428,22 @@ void strbuf_commented_addf(struct strbuf *sb,
strbuf_vaddf(&buf, fmt, params);
va_end(params);
- strbuf_add_commented_lines(sb, buf.buf, buf.len, comment_line_char);
+ strbuf_add_commented_lines(sb, buf.buf, buf.len);
if (incomplete_line)
sb->buf[--sb->len] = '\0';
strbuf_release(&buf);
}
+
+void strbuf_add_commented_lines(struct strbuf *out,
+ const char *buf, size_t size)
+{
+ static char prefix1[3];
+ static char prefix2[2];
+
+ if (prefix1[0] != comment_line_char) {
+ xsnprintf(prefix1, sizeof(prefix1), "%c ", comment_line_char);
+ xsnprintf(prefix2, sizeof(prefix2), "%c", comment_line_char);
+ }
+ strbuf_add_lines(out, prefix1, prefix2, buf, size);
+}
diff --git a/environment.h b/environment.h
index 5778f5a8e4..f801dbe36e 100644
--- a/environment.h
+++ b/environment.h
@@ -236,4 +236,11 @@ int print_sha1_ellipsis(void);
__attribute__((format (printf, 2, 3)))
void strbuf_commented_addf(struct strbuf *sb, const char *fmt, ...);
+/**
+ * Add a NUL-terminated string to the buffer. Each line will be prepended
+ * by a comment character and a blank.
+ */
+void strbuf_add_commented_lines(struct strbuf *out,
+ const char *buf, size_t size);
+
#endif
diff --git a/fmt-merge-msg.c b/fmt-merge-msg.c
index 66e47449a0..adc85d2a72 100644
--- a/fmt-merge-msg.c
+++ b/fmt-merge-msg.c
@@ -509,8 +509,7 @@ static void fmt_tag_signature(struct strbuf *tagbuf,
strbuf_complete_line(tagbuf);
if (sig->len) {
strbuf_addch(tagbuf, '\n');
- strbuf_add_commented_lines(tagbuf, sig->buf, sig->len,
- comment_line_char);
+ strbuf_add_commented_lines(tagbuf, sig->buf, sig->len);
}
}
@@ -556,8 +555,7 @@ static void fmt_merge_msg_sigs(struct strbuf *out)
strbuf_addch(&tagline, '\n');
strbuf_add_commented_lines(&tagline,
origins.items[first_tag].string,
- strlen(origins.items[first_tag].string),
- comment_line_char);
+ strlen(origins.items[first_tag].string));
strbuf_insert(&tagbuf, 0, tagline.buf,
tagline.len);
strbuf_release(&tagline);
@@ -565,8 +563,7 @@ static void fmt_merge_msg_sigs(struct strbuf *out)
strbuf_addch(&tagbuf, '\n');
strbuf_add_commented_lines(&tagbuf,
origins.items[i].string,
- strlen(origins.items[i].string),
- comment_line_char);
+ strlen(origins.items[i].string));
fmt_tag_signature(&tagbuf, &sig, buf, len);
}
strbuf_release(&payload);
diff --git a/rebase-interactive.c b/rebase-interactive.c
index 3f33da7f03..1138bd37ba 100644
--- a/rebase-interactive.c
+++ b/rebase-interactive.c
@@ -78,7 +78,7 @@ void append_todo_help(int command_count,
shortrevisions, shortonto, command_count);
}
- strbuf_add_commented_lines(buf, msg, strlen(msg), comment_line_char);
+ strbuf_add_commented_lines(buf, msg, strlen(msg));
if (get_missing_commit_check_level() == MISSING_COMMIT_CHECK_ERROR)
msg = _("\nDo not remove any line. Use 'drop' "
@@ -87,7 +87,7 @@ void append_todo_help(int command_count,
msg = _("\nIf you remove a line here "
"THAT COMMIT WILL BE LOST.\n");
- strbuf_add_commented_lines(buf, msg, strlen(msg), comment_line_char);
+ strbuf_add_commented_lines(buf, msg, strlen(msg));
if (edit_todo)
msg = _("\nYou are editing the todo file "
@@ -98,7 +98,7 @@ void append_todo_help(int command_count,
msg = _("\nHowever, if you remove everything, "
"the rebase will be aborted.\n\n");
- strbuf_add_commented_lines(buf, msg, strlen(msg), comment_line_char);
+ strbuf_add_commented_lines(buf, msg, strlen(msg));
}
int edit_todo_list(struct repository *r, struct todo_list *todo_list,
diff --git a/sequencer.c b/sequencer.c
index 5d348a3f12..29c8b5e32b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1859,7 +1859,7 @@ static void add_commented_lines(struct strbuf *buf, const void *str, size_t len)
s += count;
len -= count;
}
- strbuf_add_commented_lines(buf, s, len, comment_line_char);
+ strbuf_add_commented_lines(buf, s, len);
}
/* Does the current fixup chain contain a squash command? */
@@ -1958,7 +1958,7 @@ static int append_squash_message(struct strbuf *buf, const char *body,
strbuf_addf(buf, _(nth_commit_msg_fmt),
++opts->current_fixup_count + 1);
strbuf_addstr(buf, "\n\n");
- strbuf_add_commented_lines(buf, body, commented_len, comment_line_char);
+ strbuf_add_commented_lines(buf, body, commented_len);
/* buf->buf may be reallocated so store an offset into the buffer */
fixup_off = buf->len;
strbuf_addstr(buf, body + commented_len);
@@ -2048,8 +2048,7 @@ static int update_squash_messages(struct repository *r,
_(first_commit_msg_str));
strbuf_addstr(&buf, "\n\n");
if (is_fixup_flag(command, flag))
- strbuf_add_commented_lines(&buf, body, strlen(body),
- comment_line_char);
+ strbuf_add_commented_lines(&buf, body, strlen(body));
else
strbuf_addstr(&buf, body);
@@ -2068,8 +2067,7 @@ static int update_squash_messages(struct repository *r,
strbuf_addf(&buf, _(skip_nth_commit_msg_fmt),
++opts->current_fixup_count + 1);
strbuf_addstr(&buf, "\n\n");
- strbuf_add_commented_lines(&buf, body, strlen(body),
- comment_line_char);
+ strbuf_add_commented_lines(&buf, body, strlen(body));
} else
return error(_("unknown command: %d"), command);
repo_unuse_commit_buffer(r, commit, message);
diff --git a/strbuf.c b/strbuf.c
index b1717270a2..43027eab76 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -340,19 +340,6 @@ void strbuf_addf(struct strbuf *sb, const char *fmt, ...)
va_end(ap);
}
-void strbuf_add_commented_lines(struct strbuf *out, const char *buf,
- size_t size, char comment_line_char)
-{
- static char prefix1[3];
- static char prefix2[2];
-
- if (prefix1[0] != comment_line_char) {
- xsnprintf(prefix1, sizeof(prefix1), "%c ", comment_line_char);
- xsnprintf(prefix2, sizeof(prefix2), "%c", comment_line_char);
- }
- strbuf_add_lines(out, prefix1, prefix2, buf, size);
-}
-
void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap)
{
int len;
diff --git a/strbuf.h b/strbuf.h
index 4c58dc25e9..ffa2fe3055 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -282,15 +282,6 @@ void strbuf_remove(struct strbuf *sb, size_t pos, size_t len);
void strbuf_splice(struct strbuf *sb, size_t pos, size_t len,
const void *data, size_t data_len);
-/**
- * Add a NUL-terminated string to the buffer. Each line will be prepended
- * by a comment character and a blank.
- */
-void strbuf_add_commented_lines(struct strbuf *out,
- const char *buf, size_t size,
- char comment_line_char);
-
-
/**
* Add data of given length to the buffer.
*/
diff --git a/wt-status.c b/wt-status.c
index 54b2775730..b390c77334 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1027,7 +1027,7 @@ static void wt_longstatus_print_submodule_summary(struct wt_status *s, int uncom
if (s->display_comment_prefix) {
size_t len;
summary_content = strbuf_detach(&summary, &len);
- strbuf_add_commented_lines(&summary, summary_content, len, comment_line_char);
+ strbuf_add_commented_lines(&summary, summary_content, len);
free(summary_content);
}
@@ -1103,7 +1103,7 @@ void wt_status_append_cut_line(struct strbuf *buf)
const char *explanation = _("Do not modify or remove the line above.\nEverything below it will be ignored.");
strbuf_commented_addf(buf, "%s", cut_line);
- strbuf_add_commented_lines(buf, explanation, strlen(explanation), comment_line_char);
+ strbuf_add_commented_lines(buf, explanation, strlen(explanation));
}
void wt_status_add_cut_line(FILE *fp)
--
2.42.0.820.g83a721a137-goog
^ permalink raw reply related
* Re: [PATCH v4 00/17] bloom: changed-path Bloom filters v2 (& sundries)
From: Taylor Blau @ 2023-10-30 20:24 UTC (permalink / raw)
To: SZEDER Gábor
Cc: Junio C Hamano, git, Elijah Newren, Eric W. Biederman, Jeff King,
Patrick Steinhardt
In-Reply-To: <20231023202212.GA5470@szeder.dev>
On Mon, Oct 23, 2023 at 10:22:12PM +0200, SZEDER Gábor wrote:
> On Fri, Oct 20, 2023 at 01:27:00PM -0400, Taylor Blau wrote:
> > I'm optimistic that with the amount of careful review that this topic
> > has already received, that this round should do the trick.
>
> Unfortunately, I can't share this optimism. This series still lacks
> tests exercising the interaction of different versions of Bloom
> filters and split commit graphs, and the one such test that I sent a
> while ago demonstrates that it's still broken. And it's getting
> worse: back then I didn't send the related test that merged
> commit-graph layers containing different Bloom filter versions,
> because happened to succeed even back then; but, alas, with this
> series even that test fails.
I am very confused here, the tests that you're referring to have been
added to (and pass in) this series. What am I missing here?
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH v2] upload-pack: add tracing for fetches
From: Taylor Blau @ 2023-10-30 20:25 UTC (permalink / raw)
To: Jeff King; +Cc: Robert Coup via GitGitGadget, git, Robert Coup
In-Reply-To: <20231023185555.GD1537181@coredump.intra.peff.net>
On Mon, Oct 23, 2023 at 02:55:55PM -0400, Jeff King wrote:
> On Tue, Oct 17, 2023 at 09:12:47PM +0000, Robert Coup via GitGitGadget wrote:
>
> > Information on how users are accessing hosted repositories can be
> > helpful to server operators. For example, being able to broadly
> > differentiate between fetches and initial clones; the use of shallow
> > repository features; or partial clone filters.
> >
> > a29263c (fetch-pack: add tracing for negotiation rounds, 2022-08-02)
> > added some information on have counts to fetch-pack itself to help
> > diagnose negotiation; but from a git-upload-pack (server) perspective,
> > there's no means of accessing such information without using
> > GIT_TRACE_PACKET to examine the protocol packets.
> >
> > Improve this by emitting a Trace2 JSON event from upload-pack with
> > summary information on the contents of a fetch request.
> >
> > * haves, wants, and want-ref counts can help determine (broadly) between
> > fetches and clones, and the use of single-branch, etc.
> > * shallow clone depth, tip counts, and deepening options.
> > * any partial clone filter type.
> >
> > Signed-off-by: Robert Coup <robert@coup.net.nz>
> > ---
> > upload-pack: add tracing for fetches
> >
> >
> > Changes since V1
> > ================
> >
> > * Don't generate the JSON event unless Trace2 is active.
> > * Code style fix.
>
> Thanks, the first bullet point there addressed my only concern.
Agreed, this looks great.
Thanks,
Taylor
^ permalink raw reply
* Re: What's cooking in git.git (Oct 2023, #09; Mon, 30)
From: Junio C Hamano @ 2023-10-30 20:59 UTC (permalink / raw)
To: Robert Coup; +Cc: git
In-Reply-To: <CAFLLRpJCPxRMJEs5GxNuqmANn+DpuSAfGb9C94qvPuPNDdaWsA@mail.gmail.com>
Robert Coup <robert.coup@koordinates.com> writes:
> Any thoughts on picking up "upload-pack: add tracing for fetches" [1]?
> It received positive feedback from Taylor and Peff, and I submitted a
> v2 [2] addressing the comments that Peff has re-reviewed.
Thanks for pinging. It slipped through the cracks.
Queued.
^ permalink raw reply
* Re: [PATCH v2 1/2] commit-graph: introduce envvar to disable commit existence checks
From: Taylor Blau @ 2023-10-30 21:29 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Karthik Nayak, Junio C Hamano, Jeff King
In-Reply-To: <a89c4355285bc0bb0ec339818e6fe907f9ffd30e.1698060036.git.ps@pks.im>
On Mon, Oct 23, 2023 at 01:27:16PM +0200, Patrick Steinhardt wrote:
> Our `lookup_commit_in_graph()` helper tries to look up commits from the
> commit graph and, if it doesn't exist there, falls back to parsing it
> from the object database instead. This is intended to speed up the
> lookup of any such commit that exists in the database. There is an edge
> case though where the commit exists in the graph, but not in the object
> database. To avoid returning such stale commits the helper function thus
> double checks that any such commit parsed from the graph also exists in
> the object database. This makes the function safe to use even when
> commit graphs aren't updated regularly.
>
> We're about to introduce the same pattern into other parts of our code
> base though, namely `repo_parse_commit_internal()`. Here the extra
> sanity check is a bit of a tougher sell: `lookup_commit_in_graph()` was
> a newly introduced helper, and as such there was no performance hit by
> adding this sanity check. If we added `repo_parse_commit_internal()`
> with that sanity check right from the beginning as well, this would
> probably never have been an issue to begin with. But by retrofitting it
> with this sanity check now we do add a performance regression to
> preexisting code, and thus there is a desire to avoid this or at least
> give an escape hatch.
>
> In practice, there is no inherent reason why either of those functions
> should have the sanity check whereas the other one does not: either both
> of them are able to detect this issue or none of them should be. This
> also means that the default of whether we do the check should likely be
> the same for both. To err on the side of caution, we thus rather want to
> make `repo_parse_commit_internal()` stricter than to loosen the checks
> that we already have in `lookup_commit_in_graph()`.
All well reasoned. I think the most compelling reason is that we're
already doing this extra check in lookup_commit_in_graph(), and having
that be somewhat inconsistent with repo_parse_commit_internal() feels
error-prone to me.
> The escape hatch is added in the form of a new GIT_COMMIT_GRAPH_PARANOIA
> environment variable that mirrors GIT_REF_PARANOIA. If enabled, which is
> the default, we will double check that commits looked up in the commit
> graph via `lookup_commit_in_graph()` also exist in the object database.
> This same check will also be added in `repo_parse_commit_internal()`.
Sounds good.
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> Documentation/git.txt | 9 +++++++++
> commit-graph.c | 6 +++++-
> commit-graph.h | 6 ++++++
> t/t5318-commit-graph.sh | 21 +++++++++++++++++++++
> 4 files changed, 41 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git.txt b/Documentation/git.txt
> index 11228956cd..22c2b537aa 100644
> --- a/Documentation/git.txt
> +++ b/Documentation/git.txt
> @@ -911,6 +911,15 @@ for full details.
> should not normally need to set this to `0`, but it may be
> useful when trying to salvage data from a corrupted repository.
>
> +`GIT_COMMIT_GRAPH_PARANOIA`::
> + If this Boolean environment variable is set to false, ignore the
> + case where commits exist in the commit graph but not in the
> + object database. Normally, Git will check whether commits loaded
> + from the commit graph exist in the object database to avoid
> + issues with stale commit graphs, but this check comes with a
> + performance penalty. The default is `1` (i.e., be paranoid about
> + stale commits in the commit graph).
> +
The first two sentences seem to be flipped. Perhaps:
When loading a commit object from the commit-graph, Git will perform
an existence check on the object in the ODB before parsing it out of
the commit-graph. The default is "true", which enables the
aforementioned behavior. Setting this to "false" disables the
existential check when parsing commits from a commit-graph.
> `GIT_ALLOW_PROTOCOL`::
> If set to a colon-separated list of protocols, behave as if
> `protocol.allow` is set to `never`, and each of the listed
> diff --git a/commit-graph.c b/commit-graph.c
> index fd2f700b2e..12ec31902e 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -939,14 +939,18 @@ int repo_find_commit_pos_in_graph(struct repository *r, struct commit *c,
>
> struct commit *lookup_commit_in_graph(struct repository *repo, const struct object_id *id)
> {
> + static int object_paranoia = -1;
> struct commit *commit;
> uint32_t pos;
>
> + if (object_paranoia == -1)
> + object_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 1);
> +
I don't think that this is a reroll-able issue, but calling this
variable object_paranoia to store a setting for *graph* paranoia feels
like a good itch to scratch. But obviously not a big deal ;-).
> @@ -821,4 +821,25 @@ test_expect_success 'overflow during generation version upgrade' '
> )
> '
>
> +test_expect_success 'stale commit cannot be parsed when given directly' '
> + test_when_finished "rm -rf repo" &&
> + git init repo &&
> + (
> + cd repo &&
> + test_commit A &&
> + test_commit B &&
> + git commit-graph write --reachable &&
> +
> + oid=$(git rev-parse B) &&
> + rm .git/objects/"$(test_oid_to_path "$oid")" &&
> +
> + # Verify that it is possible to read the commit from the
> + # commit graph when not being paranoid, ...
> + GIT_COMMIT_GRAPH_PARANOIA=false git rev-list B &&
> + # ... but parsing the commit when double checking that
> + # it actually exists in the object database should fail.
> + test_must_fail git rev-list -1 B
Would "cat-file -p" be more direct here than "rev-list -1"?
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH v2 2/2] commit: detect commits that exist in commit-graph but not in the ODB
From: Taylor Blau @ 2023-10-30 21:31 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Karthik Nayak, Junio C Hamano, Jeff King
In-Reply-To: <0476d4855562b677ced106a4cc7788b46434cf21.1698060036.git.ps@pks.im>
On Mon, Oct 23, 2023 at 01:27:20PM +0200, Patrick Steinhardt wrote:
> @@ -572,8 +573,21 @@ int repo_parse_commit_internal(struct repository *r,
> return -1;
> if (item->object.parsed)
> return 0;
> - if (use_commit_graph && parse_commit_in_graph(r, item))
> + if (use_commit_graph && parse_commit_in_graph(r, item)) {
> + static int object_paranoia = -1;
> +
> + if (object_paranoia == -1)
> + object_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 1);
The same note here about object_paranoia versus graph_paranoia, but
otherwise this patch looks good to me, modulo one typo below.
> @@ -842,4 +842,31 @@ test_expect_success 'stale commit cannot be parsed when given directly' '
> )
> '
>
> +test_expect_success 'stale commit cannot be parsed when traversing graph' '
> + test_when_finished "rm -rf repo" &&
> + git init repo &&
> + (
> + cd repo &&
> +
> + test_commit A &&
> + test_commit B &&
> + test_commit C &&
> + git commit-graph write --reachable &&
> +
> + # Corrupt the repository by deleting the intermittent commit
s/intermittent/intermediate
> + # object. Commands should notice that this object is absent and
> + # thus that the repository is corrupt even if the commit graph
> + # exists.
> + oid=$(git rev-parse B) &&
> + rm .git/objects/"$(test_oid_to_path "$oid")" &&
> +
> + # Again, we should be able to parse the commit when not
> + # being paranoid about commit graph staleness...
> + GIT_COMMIT_GRAPH_PARANOIA=false git rev-parse HEAD~2 &&
> + # ... but fail when we are paranoid.
> + test_must_fail git rev-parse HEAD~2 2>error &&
> + grep "error: commit $oid exists in commit-graph but not in the object database" error
> + )
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH v2 2/2] commit: detect commits that exist in commit-graph but not in the ODB
From: Taylor Blau @ 2023-10-30 21:32 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Patrick Steinhardt, git, Karthik Nayak, Jeff King
In-Reply-To: <xmqqy1fr3kh6.fsf@gitster.g>
On Tue, Oct 24, 2023 at 12:10:13PM -0700, Junio C Hamano wrote:
> > We look at a ~30% regression in general, but in general we're still a
> > whole lot faster than without the commit graph. To counteract this, the
> > new check can be turned off with the `GIT_COMMIT_GRAPH_PARANOIA` envvar.
>
> Very nicely described. Will queue. I'll go offline for the rest of
> the week but if there are no significant issues discovered by the
> time I come back, let's declare a victory and merge these two
> patches down to 'next'.
I think we're close here. There are a couple of small comments that I
made throughout these two patches, but nothing major.
Thanks,
Taylor
^ permalink raw reply
* Re: Method for Calculating Statistics of Developer Contribution to a Specified Branch.
From: Taylor Blau @ 2023-10-30 21:49 UTC (permalink / raw)
To: Hongyi Zhao; +Cc: brian m. carlson, Git List
In-Reply-To: <CAGP6PO+SeZPzD21nErX=Vq=+d6oy-kg+diu=irot3enOhpQNMg@mail.gmail.com>
On Tue, Oct 17, 2023 at 07:37:46PM +0800, Hongyi Zhao wrote:
> I want to calculate a certain developer's contribution based on
> different standards of code line count and the importance of the code.
I agree with brian that "number of lines added/removed" is not a perfect
measure of productivity ;-).
But I think that there is a slightly cleaner way to compute the result
you're after, like so:
git rev-list --author="$who" origin/main |
git diff-tree --stdin -r --numstat --no-commit-id |
awk '{ s += $1 + $2 } END { print s }'
Thanks,
Taylor
^ 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