* Re: [PATCH v2 5/5] ci: add support for GitLab CI
From: Patrick Steinhardt @ 2023-10-30 10:45 UTC (permalink / raw)
To: phillip.wood; +Cc: git, Oswald Buddenhagen
In-Reply-To: <a165fd76-a44b-45c7-b02c-e9b90fda3db3@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3972 bytes --]
On Sun, Oct 29, 2023 at 04:27:44PM +0000, Phillip Wood wrote:
> Hi Patrick
>
> On 27/10/2023 12:19, Patrick Steinhardt wrote:
> > On Fri, Oct 27, 2023 at 11:19:04AM +0100, Phillip Wood wrote:
> > > On 27/10/2023 10:25, Patrick Steinhardt wrote:
> > > > + parallel:
> > > > + matrix:
> > > > + - jobname: linux-sha256
> > > > + image: ubuntu:latest
> > > > + CC: clang
> > > > + - jobname: linux-gcc
> > > > + image: ubuntu:20.04
> > > > + CC: gcc
> > > > + CC_PACKAGE: gcc-8
> > > > + - jobname: linux-TEST-vars
> > > > + image: ubuntu:20.04
> > > > + CC: gcc
> > > > + CC_PACKAGE: gcc-8
> > > > + - jobname: linux-gcc-default
> > > > + image: ubuntu:latest
> > > > + CC: gcc
> > > > + - jobname: linux-leaks
> > > > + image: ubuntu:latest
> > > > + CC: gcc
> > > > + - jobname: linux-asan-ubsan
> > > > + image: ubuntu:latest
> > > > + CC: clang
> > > > + - jobname: linux-musl
> > > > + image: alpine:latest
>
> I assume you've chosen the configurations from the existing GitHub Actions
> that give the best coverage of the various options. One thing I noticed is
> that the is no equivalent of the "pedantic" job that builds git with
> "DEVELOPER=1 DEVOPTS=pedantic"
Yeah, indeed. The list for sure isn't complete, and I also want to
iterate in the future to add support for macOS and Windows. But adding
the pedantic job is easy enough, so I'll include it.
> > > > diff --git a/ci/install-docker-dependencies.sh b/ci/install-docker-dependencies.sh
> > > > index d0bc19d3bb3..1cd92db1876 100755
> > > > --- a/ci/install-docker-dependencies.sh
> > > > +++ b/ci/install-docker-dependencies.sh
> > > > @@ -16,9 +19,13 @@ linux32)
> > > > '
> > > > ;;
> > > > linux-musl)
> > > > - apk add --update build-base curl-dev openssl-dev expat-dev gettext \
> > > > + apk add --update git shadow sudo build-base curl-dev openssl-dev expat-dev gettext \
> > > > pcre2-dev python3 musl-libintl perl-utils ncurses >/dev/null
> > >
> > > It would be helpful to explain the new dependencies in the commit message. I
> > > can see why you're adding sudo, but how were we getting away without
> > > installing the other packages for GitHub Actions?
I've dropped the "git" dependency for now. It doesn't do anything useful
anyway for our Docker-based jobs. Instead, I've added a preparatory
commit that makes the relevant CI library functions more robust by
detecting the case where we either don't have Git available or don't use
a repository.
> > True, that part is missing.
> >
> > - Both sudo and shadow are now required because of `useradd` that we use
> > to set up the unprivileged build.
> >
> > - Git has been required all along, I think. `save_good_tree ()` is used
> > in our CI scripts, and Toon (fellow GitLabber from my team) has
> > noticed that the CI job warned about missing Git. The warning was
> > mostly benign as it seems, but still, doesn't hurt to fix it while at
> > it.
>
> Oh I had a look at this and the docker based jobs on GitHub do not have a
> git repository so installing git means we now get a repository not found
> error from save_good_tree() instead. We should probably make
> save_good_tree() and check_unignored_build_artifacts() return early if there
> isn't a repository but that's orthogonal to this series.
Yup, done now.
> Looking at the test output from the link in your cover letter we should we
> should also install apache2[1] and gnupg[2]
True. Also missing are Bash, CVS, the Perl CGI module, Subversion and
Perforce. I'll add them as available in the respective distro's
repositories.
Patrick
> [1] https://gitlab.com/gitlab-org/git/-/jobs/5349205374#L1444
> [2] https://gitlab.com/gitlab-org/git/-/jobs/5349205374#L1167
>
> Best Wishes
>
> Phillip
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: please add link / url to remote - when - git push
From: Michal Suchánek @ 2023-10-30 10:42 UTC (permalink / raw)
To: Jeff King; +Cc: Torsten Bögershausen, Alexander Mills, git
In-Reply-To: <20231030095401.GA848451@coredump.intra.peff.net>
On Mon, Oct 30, 2023 at 05:54:01AM -0400, Jeff King wrote:
> On Mon, Oct 30, 2023 at 10:45:56AM +0100, Michal Suchánek wrote:
>
> > > Yes, this is a problem. Those messages are streamed out as we receive
> > > them from the server, and before we get any status report back. I don't
> > > think we'd want to buffer them, as they can be arbitrarily large (and
> > > may even be progress reports that are meant to be shown in real-time).
> >
> > Not when the user asked to not show progress.
>
> Sure, if you make the buffering behavior dependent on the presence of
> "-q". I guess that is not that hard to do, but it does increase the
> complexity of the code (you have both a streaming and a buffering code
> path).
>
> Anyway, I am not all that convinced this is a fruitful path versus just
> fixing the server side. But if you want to look into writing a patch, go
> for it.
I will try to bug the forge upstream first.
Thanks
Michal
^ permalink raw reply
* Re: [PATCH] git-push: more visibility for -q option
From: Michal Suchánek @ 2023-10-30 10:40 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqwmvhoy57.fsf@gitster.g>
Hello,
On Fri, Oct 20, 2023 at 01:08:04PM -0700, Junio C Hamano wrote:
> Michal Suchanek <msuchanek@suse.de> writes:
>
> > The -v option listed at the top as option al parameter while -q is not.
>
> "as option al parameter" - ECANNOTPARSE. Probably
>
> The `-v` option is shown in the SYNOPSIS section near the top,
> but `-q` is not shown anywhere there.
>
> or something, I think. I agree showing it next to "-v" would make
> the most sense.
when rebasing to current master I see this is already applied with the
fixed commit message, great!
Thanks
Michal
>
> >
> > List -q alongside -v.
> >
> > Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> > ---
> > Documentation/git-push.txt | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
> > index 5b4edaf4a8..003bc7d9ce 100644
> > --- a/Documentation/git-push.txt
> > +++ b/Documentation/git-push.txt
> > @@ -10,7 +10,7 @@ SYNOPSIS
> > --------
> > [verse]
> > 'git push' [--all | --branches | --mirror | --tags] [--follow-tags] [--atomic] [-n | --dry-run] [--receive-pack=<git-receive-pack>]
> > - [--repo=<repository>] [-f | --force] [-d | --delete] [--prune] [-v | --verbose]
> > + [--repo=<repository>] [-f | --force] [-d | --delete] [--prune] [-q | --quiet] [-v | --verbose]
>
> Yup, the change makes sense. We may want to wrap the first line to
> a more reasonable length in a separate commit, and when that
> happens, we probably would want to start [-v] [-q] on a separate
> line as well, but for now this would do.
>
^ permalink raw reply
* Re: [PATCH 0/2] Avoid passing global comment_line_char repeatedly
From: Phillip Wood @ 2023-10-30 9:59 UTC (permalink / raw)
To: Junio C Hamano, git; +Cc: Calvin Wan, Jonathan Tan
In-Reply-To: <20231030051034.2295242-1-gitster@pobox.com>
Hi Junio
On 30/10/2023 05:10, Junio C Hamano wrote:
> Two strbuf functions used to produce commented lines take the
> comment_line_char as their parameter, but in practice, all callers
> feed the global variable comment_line_char from environment.[ch].
>
> Dropping the parameter from the callchain will make the interface
> less flexible, and less error prone. If we choose to change the
> implementation of the customizable comment line character (e.g., we
> may want to stop referencing the global variable and instead use a
> getter function), we will have fewer places we need to modify.
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.
Best Wishes
Phillip
> 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 ++++----
> builtin/branch.c | 2 +-
> builtin/merge.c | 8 ++++----
> builtin/notes.c | 9 ++++-----
> builtin/stripspace.c | 2 +-
> builtin/tag.c | 4 ++--
> fmt-merge-msg.c | 9 +++------
> rebase-interactive.c | 8 ++++----
> sequencer.c | 14 ++++++--------
> strbuf.c | 9 +++++----
> strbuf.h | 7 +++----
> wt-status.c | 6 +++---
> 12 files changed, 40 insertions(+), 46 deletions(-)
>
^ permalink raw reply
* Re: [PATCH v3] git-rebase.txt: rewrite docu for fixup/squash (again)
From: Phillip Wood @ 2023-10-30 9:55 UTC (permalink / raw)
To: Marc Branchaud, Oswald Buddenhagen, git
Cc: Junio C Hamano, Taylor Blau, Christian Couder, Charvi Mendiratta
In-Reply-To: <56e3e974-a027-439f-871d-c7fbae65a04e@xiplink.com>
On 27/10/2023 14:14, Marc Branchaud wrote:
>> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
>> index e7b39ad244..578d1d34a6 100644
>> --- a/Documentation/git-rebase.txt
>> +++ b/Documentation/git-rebase.txt
>> @@ -890,20 +890,22 @@ command "pick" with the command "reword".
>> To drop a commit, replace the command "pick" with "drop", or just
>> delete the matching line.
>> -If you want to fold two or more commits into one, replace the command
>> -"pick" for the second and subsequent commits with "squash" or "fixup".
>> -If the commits had different authors, the folded commit will be
>> -attributed to the author of the first commit. The suggested commit
>> -message for the folded commit is the concatenation of the first
>> -commit's message with those identified by "squash" commands, omitting
>> the
>> -messages of commits identified by "fixup" commands, unless "fixup -c"
>> -is used. In that case the suggested commit message is only the message
>> -of the "fixup -c" commit, and an editor is opened allowing you to edit
>> -the message. The contents (patch) of the "fixup -c" commit are still
>> -incorporated into the folded commit. If there is more than one "fixup
>> -c"
>> -commit, the message from the final one is used. You can also use
>> -"fixup -C" to get the same behavior as "fixup -c" except without opening
>> -an editor.
>> +If you want to fold two or more commits into one (that is, to combine
>> +their contents/patches), replace the command "pick" for the second and
>> +subsequent commits with "squash" or "fixup".
>> +The commit message for the folded commit is the concatenation of the
>> +message of the first commit with those of commits identified by "squash"
>> +commands, omitting those of commits identified by "fixup" commands,
>> +unless "fixup -c" is used. In the latter case, the message is obtained
>> +only from the "fixup -c" commit (if multiple are present, the last one
>> +takes precedence, but this should not be relied upon).
>
> I like the overall phrasing here.
>
> But I think you should remove the "but this should not be relied upon"
> phrase. This reads as if Git's current behaviour is undefined, which
> most definitely is not true.
I agree it would be better to remove that phrase, as you say it makes it
sounds like the behaviour cannot be relied on.
> Even changing this to something like "but this might change in the
> future" is unhelpful. Everything in Git is subject to change over a
> long-enough time span, so the same could be said about every aspect of Git.
>
> Until the behaviour actually changes, it's perfectly fine for people to
> use multiple "fixup -c" commands. There's no reason to scare them off
> of it.
Indeed
Best Wishes
Phillip
^ permalink raw reply
* Re: please add link / url to remote - when - git push
From: Jeff King @ 2023-10-30 9:54 UTC (permalink / raw)
To: Michal Suchánek; +Cc: Torsten Bögershausen, Alexander Mills, git
In-Reply-To: <20231030094556.GI6241@kitsune.suse.cz>
On Mon, Oct 30, 2023 at 10:45:56AM +0100, Michal Suchánek wrote:
> > Yes, this is a problem. Those messages are streamed out as we receive
> > them from the server, and before we get any status report back. I don't
> > think we'd want to buffer them, as they can be arbitrarily large (and
> > may even be progress reports that are meant to be shown in real-time).
>
> Not when the user asked to not show progress.
Sure, if you make the buffering behavior dependent on the presence of
"-q". I guess that is not that hard to do, but it does increase the
complexity of the code (you have both a streaming and a buffering code
path).
Anyway, I am not all that convinced this is a fruitful path versus just
fixing the server side. But if you want to look into writing a patch, go
for it.
-Peff
^ permalink raw reply
* Re: [PATCH 5/5] ci: add support for GitLab CI
From: Phillip Wood @ 2023-10-30 9:49 UTC (permalink / raw)
To: Oswald Buddenhagen; +Cc: Patrick Steinhardt, git
In-Reply-To: <ZTv3to98QT8Ma+ag@ugly>
On 27/10/2023 18:47, Oswald Buddenhagen wrote:
> On Fri, Oct 27, 2023 at 03:32:48PM +0100, Phillip Wood wrote:
>> On 27/10/2023 11:43, Oswald Buddenhagen wrote:
>>> On Fri, Oct 27, 2023 at 11:22:35AM +0100, Phillip Wood wrote:
>>>>>>> + CI_BRANCH="$CI_COMMIT_REF_NAME"
>>>>>>> + CI_COMMIT="$CI_COMMIT_SHA"
>>>>>>>
>>>>>> assignments need no quoting to prevent word splitting.
>>>>>> repeats below.
>>>>>>
>>>> I think it is quite common for us to quote variables when it isn't
>>>> strictly necessary as it makes it clear to anyone reading the script
>>>> that there is no word splitting going on
>>>
>>>> and ensures that we don't start splitting the variable if the
>>>> contents changes in the future.
>>>>
>>> the point was that it *isn't* content-dependent; it's simply the
>>> shell rules.
>>
>> Oh, I'd misunderstood what you were saying which was that assignment
>> and case statements are not subject to field splitting.
>>
>>> of course, many people (apparently you included) don't know these
>>> subtleties
>>
>> I find this comment to be condescending, needlessly antagonistic and
>> completely at odds with the norms of constructive discussion on this
>> list.
>>
> the observation was necessary for the point i subsequently made (which
> was basically agreeing with the first part of your response).
It was not necessary to phrase it as you did though. Before replying on
Friday I showed your comment to someone else and their reaction was
"That's rude". You could have made your point by saying something like
It is hard to remember all the shell's word splitting rules so
quoting everywhere is not a bad idea.
This is not the first time I've found your comments unnecessarily
adversarial and at odds with the norms of constructive discussion and
respectful disagreement on this list. I don't think I'm the only one
either - in [1] Junio points out an ad-hominem remark and in [2] Marc
comments on the unreceptive tone of you review responses.
I would urge you to try and strike a more conciliatory tone in your
messages - it is perfectly possible to correct or disagree with someone
without alienating them in the process.
Best Wishes
Phillip
[1] https://lore.kernel.org/git/xmqqleeihok5.fsf@gitster.g/
[2]
https://lore.kernel.org/git/e33f919d-1b6a-4944-ab5d-93ad0d323b68@xiplink.com/
^ permalink raw reply
* Re: please add link / url to remote - when - git push
From: Michal Suchánek @ 2023-10-30 9:45 UTC (permalink / raw)
To: Jeff King; +Cc: Torsten Bögershausen, Alexander Mills, git
In-Reply-To: <20231030093919.GF84866@coredump.intra.peff.net>
On Mon, Oct 30, 2023 at 05:39:19AM -0400, Jeff King wrote:
> On Mon, Oct 30, 2023 at 10:30:23AM +0100, Michal Suchánek wrote:
>
> > > > On the other hand, option to NOT display those remote messages is also
> > > > missing. At least with git 2.35 they are displayed even when -q argument
> > > > is given.
> > >
> > > That is also up to GitHub to fix on the server side:
> > >
> > > https://lore.kernel.org/git/20230519090559.GA3515410@coredump.intra.peff.net/
> >
> > If the server did not reject the push the messages it returns could be
> > considered informational.
>
> Hmm, yeah, I could perhaps buy that line of argument. Though it's
> possible they are "warnings" of the sort that should still be emitted
> with "--quiet"; really only the server knows. And note that even "reject
> the push" is not necessarily all-or-nothing. The server may accept some
> subset of the refs.
>
> But...
>
> > Not sure if the final status of the push is available or of the
> > rejection is only noted in these messages, though.
>
> Yes, this is a problem. Those messages are streamed out as we receive
> them from the server, and before we get any status report back. I don't
> think we'd want to buffer them, as they can be arbitrarily large (and
> may even be progress reports that are meant to be shown in real-time).
Not when the user asked to not show progress.
Thanks
Michal
^ permalink raw reply
* Re: please add link / url to remote - when - git push
From: Jeff King @ 2023-10-30 9:39 UTC (permalink / raw)
To: Michal Suchánek; +Cc: Torsten Bögershausen, Alexander Mills, git
In-Reply-To: <20231030093023.GG6241@kitsune.suse.cz>
On Mon, Oct 30, 2023 at 10:30:23AM +0100, Michal Suchánek wrote:
> > > On the other hand, option to NOT display those remote messages is also
> > > missing. At least with git 2.35 they are displayed even when -q argument
> > > is given.
> >
> > That is also up to GitHub to fix on the server side:
> >
> > https://lore.kernel.org/git/20230519090559.GA3515410@coredump.intra.peff.net/
>
> If the server did not reject the push the messages it returns could be
> considered informational.
Hmm, yeah, I could perhaps buy that line of argument. Though it's
possible they are "warnings" of the sort that should still be emitted
with "--quiet"; really only the server knows. And note that even "reject
the push" is not necessarily all-or-nothing. The server may accept some
subset of the refs.
But...
> Not sure if the final status of the push is available or of the
> rejection is only noted in these messages, though.
Yes, this is a problem. Those messages are streamed out as we receive
them from the server, and before we get any status report back. I don't
think we'd want to buffer them, as they can be arbitrarily large (and
may even be progress reports that are meant to be shown in real-time).
-Peff
^ permalink raw reply
* Re: Repository cloned using SSH does not respect bare repository initial branch
From: Jeff King @ 2023-10-30 9:36 UTC (permalink / raw)
To: Sheik; +Cc: git
In-Reply-To: <63eb269e-72b9-4830-98fc-aeef8b8180d7@gmail.com>
On Thu, Oct 26, 2023 at 07:36:36AM +1100, Sheik wrote:
> Repository cloned using SSH does not use the branch configured in the bare
> repository however repository cloned using filesystem does as expected.
> Shouldn't they both behave the same?
What version of Git is running on the ssh server?
Your example seems to show that the parent repository has an unborn
branch (i.e., HEAD points to "refs/heads/test", but there are no commits
yet). I think the server-side bits you need for that to work showed up
in 59e1205d16 (ls-refs: report unborn targets of symrefs, 2021-02-05),
which is in v2.31.
So even though your client seems to be v2.42 (from the output you gave),
if the server is older it may not be sending sufficient information.
There were also some other fixes on top of that, but I _think_ they were
all client-side (so your v2.42 clone command should be doing the right
thing).
-Peff
^ permalink raw reply
* Re: What's cooking in git.git (Oct 2023, #09; Mon, 30)
From: Robert Coup @ 2023-10-30 9:31 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq1qddhtpi.fsf@gitster.g>
Hi Junio,
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. Any further
comments are most welcome.
Thanks,
Rob :)
[1] https://lore.kernel.org/git/pull.1598.git.1697040242703.gitgitgadget@gmail.com/
[2] https://lore.kernel.org/git/pull.1598.v2.git.1697577168128.gitgitgadget@gmail.com/
^ permalink raw reply
* Re: please add link / url to remote - when - git push
From: Michal Suchánek @ 2023-10-30 9:30 UTC (permalink / raw)
To: Jeff King; +Cc: Torsten Bögershausen, Alexander Mills, git
In-Reply-To: <20231030090626.GA84866@coredump.intra.peff.net>
On Mon, Oct 30, 2023 at 05:06:26AM -0400, Jeff King wrote:
> On Mon, Oct 30, 2023 at 09:52:05AM +0100, Michal Suchánek wrote:
>
> > > If we look very carfully at the log, we see that all the messages prefixed
> > > with "remote:" come from the remote (git server).
> > > In your case github. Other "Git repo servers" like gitlab or bitbucket have
> > > the same feature.
> > >
> > > Git itself, running on your local computer, does not produce this links.
> > > If you really want to push directly to the main branch and want to see the
> > > a link, then you better talk to the gihub folks.
> >
> > On the other hand, option to NOT display those remote messages is also
> > missing. At least with git 2.35 they are displayed even when -q argument
> > is given.
>
> That is also up to GitHub to fix on the server side:
>
> https://lore.kernel.org/git/20230519090559.GA3515410@coredump.intra.peff.net/
If the server did not reject the push the messages it returns could be
considered informational.
Not sure if the final status of the push is available or of the
rejection is only noted in these messages, though.
Thanks
Michal
^ permalink raw reply
* Re: Regression: git send-email fails with "Use of uninitialized value $address" + "unable to extract a valid address"
From: Jeff King @ 2023-10-30 9:29 UTC (permalink / raw)
To: Todd Zullinger
Cc: Michael Strawbridge, Junio C Hamano, Uwe Kleine-König,
Luben Tuikov, git, entwicklung
In-Reply-To: <ZTp7xvXDw1GF-NUB@pobox.com>
On Thu, Oct 26, 2023 at 10:46:30AM -0400, Todd Zullinger wrote:
> I'd lean toward dropping the dependency entirely and leave
> the more basic validation of git-send-email in place. That
> may not catch every type of address error, but I would argue
> that what we do without Email::Valid is perfectly reasonable
> for checking basic email address syntax sanity.
I am somewhat tempted to say the same, but in this case didn't it help
us find a real bug? True, the bug was that we were feeding garbage to
Email::Valid because we were calling it to early, and I _think_ the
ultimate emails we sent would have been OK. But I think we were possibly
feeding that garbage to the user-visible validation hook.
That might just mean we need to beef up our homegrown validation a bit
(though as you indicate here, I am generally of the opinion that the
best validation is trying to actually send things).
-Peff
^ permalink raw reply
* Re: [PATCH 2/3] Revert "send-email: extract email-parsing code into a subroutine"
From: Jeff King @ 2023-10-30 9:13 UTC (permalink / raw)
To: Oswald Buddenhagen
Cc: Michael Strawbridge, Junio C Hamano, Bagas Sanjaya,
Git Mailing List
In-Reply-To: <ZTjedSluwyrVY+L9@ugly>
On Wed, Oct 25, 2023 at 11:23:01AM +0200, Oswald Buddenhagen wrote:
> On Wed, Oct 25, 2023 at 02:11:20AM -0400, Jeff King wrote:
> > The "//" operator was added in perl 5.10. I'm not sure what you found
> > that makes you think the ship has sailed. The only hits for "//" I see
> > look like the end of substitution regexes ("s/foo//" and similar).
> >
> grep with spaces around the operator, then you can see the instance in
> git-credential-netrc.perl easily.
Ah, yeah, there is one instance there. That script does not have a "use"
marker, though, and we do not necessarily need or want to be as strict
with contrib/ scripts, which are quite optional compared to core
functionality like send-email.
That said, I do suspect that requiring 5.10 or later would not be too
burdensome these days. If we want to do so, then the first step would be
updating the text in INSTALL, along with the "use" directives in most
files. Probably d48b284183 (perl: bump the required Perl version to 5.8
from 5.6.[21], 2010-09-24) could serve as a template.
-Peff
^ permalink raw reply
* Re: [PATCH 2/2] pretty: add '%aA' to show domain-part of email addresses
From: Jeff King @ 2023-10-30 9:10 UTC (permalink / raw)
To: Andy Koppe; +Cc: Liam Beguin, Junio C Hamano, Kousik Sanagavarapu, git
In-Reply-To: <9a1e3e90-3e94-41fa-897d-5c64c4a42871@gmail.com>
On Sat, Oct 28, 2023 at 07:58:31AM +0100, Andy Koppe wrote:
> > I chose the "a" for "address", but I'm not sold on %aa either.
> > I just couldn't find anything better that wasn't already taken.
> >
> > What about "a@"?
>
> Makes sense, and I suppose there's "%G?" as precedent for using a symbol
> rather than letter in these.
This is pretty subjective, but I somehow find "%a@" hard to parse
visually (despite the fact that yes, "%G?" already crossed that bridge).
But I think the real nail in the coffin is your later comment that we
cannot use capitalization to make the raw/mailmap distinction.
> If that's not suitable though, how about "m" for "mail domain"? It also
> immediately follows "l" for "local-part" in the alphabet.
FWIW, that makes sense to me over "a" (though admittedly it is not
really any less vague than "a", so it really might vary from person to
person).
-Peff
^ permalink raw reply
* Re: please add link / url to remote - when - git push
From: Jeff King @ 2023-10-30 9:06 UTC (permalink / raw)
To: Michal Suchánek; +Cc: Torsten Bögershausen, Alexander Mills, git
In-Reply-To: <20231030085205.GF6241@kitsune.suse.cz>
On Mon, Oct 30, 2023 at 09:52:05AM +0100, Michal Suchánek wrote:
> > If we look very carfully at the log, we see that all the messages prefixed
> > with "remote:" come from the remote (git server).
> > In your case github. Other "Git repo servers" like gitlab or bitbucket have
> > the same feature.
> >
> > Git itself, running on your local computer, does not produce this links.
> > If you really want to push directly to the main branch and want to see the
> > a link, then you better talk to the gihub folks.
>
> On the other hand, option to NOT display those remote messages is also
> missing. At least with git 2.35 they are displayed even when -q argument
> is given.
That is also up to GitHub to fix on the server side:
https://lore.kernel.org/git/20230519090559.GA3515410@coredump.intra.peff.net/
-Peff
^ permalink raw reply
* Re: please add link / url to remote - when - git push
From: Michal Suchánek @ 2023-10-30 8:52 UTC (permalink / raw)
To: Torsten Bögershausen; +Cc: Alexander Mills, git
In-Reply-To: <20231030063633.GA7451@tb-raspi4>
On Mon, Oct 30, 2023 at 07:36:33AM +0100, Torsten Bögershausen wrote:
> On Sun, Oct 29, 2023 at 06:15:35PM -0500, Alexander Mills wrote:
> > When a feature branch is pushed, we get a link in the console to go to
> > remote, however when I push directly to main/master, no such link, eg:
> >
> > ```
> > alex.mills@alex node-be % git push
> > Enumerating objects: 20, done.
> > Counting objects: 100% (20/20), done.
> > Delta compression using up to 12 threads
> > Compressing objects: 100% (10/10), done.
> > Writing objects: 100% (11/11), 1.56 KiB | 799.00 KiB/s, done.
> > Total 11 (delta 7), reused 0 (delta 0), pack-reused 0
> > remote: Resolving deltas: 100% (7/7), completed with 7 local objects.
> > remote: Bypassed rule violations for refs/heads/main:
> > remote:
> > remote: - Changes must be made through a pull request.
> > remote:
> > To github.com:elx-onlinx/beautychat-chatcards.git
> > ffe1e05..bb7b0ef main -> main
> > ```
> >
> > Having the link in the console saves me tremendous time and is
> > extremely effective/efficient. Can we get links in the console plz?
> >
>
> If we look very carfully at the log, we see that all the messages prefixed
> with "remote:" come from the remote (git server).
> In your case github. Other "Git repo servers" like gitlab or bitbucket have
> the same feature.
>
> Git itself, running on your local computer, does not produce this links.
> If you really want to push directly to the main branch and want to see the
> a link, then you better talk to the gihub folks.
On the other hand, option to NOT display those remote messages is also
missing. At least with git 2.35 they are displayed even when -q argument
is given.
Thanks
Michal
^ permalink raw reply
* RE: [EXT] Re: ls-remote bug
From: Lior Zeltzer @ 2023-10-30 8:17 UTC (permalink / raw)
To: René Scharfe, Bagas Sanjaya, Git Mailing List
Cc: Andrzej Hunt, Ævar Arnfjörð Bjarmason,
Junio C Hamano
In-Reply-To: <cc829a27-2580-4e07-a6ff-2c4992131420@web.de>
But why when cancelling stderr redirect to stdout (2>&1) all works well ?
-----Original Message-----
From: René Scharfe <l.s.r@web.de>
Sent: Sunday, October 29, 2023 8:57 PM
To: Lior Zeltzer <liorz@marvell.com>; Bagas Sanjaya <bagasdotme@gmail.com>; Git Mailing List <git@vger.kernel.org>
Cc: Andrzej Hunt <ajrhunt@google.com>; Ævar Arnfjörð Bjarmason <avarab@gmail.com>; Junio C Hamano <gitster@pobox.com>
Subject: Re: [EXT] Re: ls-remote bug
Am 27.10.23 um 13:16 schrieb Lior Zeltzer:
> The reproduction ,as I wrote in the code, should be done with few
> threads in parallel Each working on a list of ~10 repos with each repo
> containing a lot of refs/tags (~1000) All this should be against
> gerrit (my gerrit is 3.8.0)
>
> Also read the notes below regarding the code that was moved between
> 2.31.8 and 2.32.0 in ls-remote.c file I can elaborate more, in a zoom meeting.
>
> 10x
> Lior.
>
>
> -----Original Message-----
> From: Bagas Sanjaya <bagasdotme@gmail.com>
> Sent: Friday, October 27, 2023 4:08 AM
> To: Lior Zeltzer <liorz@marvell.com>; Git Mailing List
> <git@vger.kernel.org>
> Cc: Andrzej Hunt <ajrhunt@google.com>; Ævar Arnfjörð Bjarmason
> <avarab@gmail.com>; Junio C Hamano <gitster@pobox.com>
> Subject: [EXT] Re: ls-remote bug
>
> External Email
>
> ----------------------------------------------------------------------
> On Tue, Oct 24, 2023 at 10:55:24AM +0000, Lior Zeltzer wrote:
>>
>>> uname -a
>> Linux dc3lp-veld0045 3.10.0-1160.21.1.el7.x86_64 #1 SMP Tue Mar 16
>> 18:28:22 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux
>>
>> Gerrit version :
>> 3.8.0
>>
>> Bug description :
>> When running ls-remote : sometime data gets cut in the middle
>>
>> Reproducing :
>> You need a few files with a few repo names (I used 4 files with 10
>> repos each) Call then l1..l4 And the code below just cd into each of
>> them does ls-remote twice and compares the data Doing it in parallel
>> on all lists.
>> Data received in both ls-remotes should be the same , if not, it
>> prints *** Repos should contain a lot of tags and refs
>
> What repo did you find this regression? Did you mean linux.git (Linux kernel)?
>
>>
>> Note :
>> 1. without stderr redirection (2>&1) all works well 2. On local
>> repos (not through gerrit) all works well
>>
>> I compared various git vers and found the bug to be between 2.31.8
>> and
>> 2.32.0 Comparing ls-remote.c file between those vers gave me :
>>
>> Lines :
>> if (transport_disconnect(transport))
>> return 1;
>>
>> moved to end of sub
>>
>> copying ls-remote.c from 2.31.8 to 2.32.0 - fixed the bug
This partly undoes 68ffe095a2 (transport: also free remote_refs in transport_disconnect(), 2021-03-21). With that patch connections are kept open during ref sorting and printing.
Perhaps the other side gets tired of waiting and aborts? Maybe splitting transport_disconnect() into two functions -- one for disconnecting and and for cleaning up -- would make sense? And disconnecting as soon as possible? Just guessing -- didn't actually reproduce the bug. Still, demo patch below.
>>
>>
>>
>> Code reproducing bug :
>>
>> #!/proj/mislcad/areas/DAtools/tools/perl/5.10.1/bin/perl -w use
>> strict; use Cwd qw(cwd);
>>
>> my $count = 4;
>> for my $f (1..$count) {
>> my $child = fork();
>> if (!$child) {
>> my $curr = cwd();
>>
>> my @repos = `cat l$f`;
>> foreach my $repo (@repos) {
>> chomp $repo;
>> print "$repo\n";
>> chdir($repo);
>> my $remote_tags_str = `git ls-remote 2>&1`;
>> my $remote_tags_str2 = `git ls-remote 2>&1 `;
>> chdir($curr);
>> if ( $remote_tags_str ne $remote_tags_str2) {
>> print "***\n";
>> }
>> }
>>
>> exit(0);
>> }
>> }
>> while (wait != -1) {}
>> 1;
>>
>
> I tried reproducing this regression by:
>
> ```
> $ cd /path/to/git.git
> $ git ls-remote 2>&1 > /tmp/root.list
> $ cd builtin/
> $ git ls-remote 2>&1 > /tmp/builtin.list $ cd ../ $ git diff
> --no-index /tmp/root.list /tmp/builtin.list ```
>
> And indeed, the diff was empty (which meant that both listings are same).
>
> Confused...
>
> --
> An old man doll... just what I always wanted! - Clara
---
builtin/ls-remote.c | 8 +++++---
builtin/remote.c | 3 ++-
transport.c | 15 +++++++++++++--
transport.h | 2 ++
4 files changed, 22 insertions(+), 6 deletions(-)
diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c index fc76575430..4c1daa0f92 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -128,6 +128,8 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
int hash_algo = hash_algo_by_ptr(transport_get_hash_algo(transport));
repo_set_hash_algo(the_repository, hash_algo);
}
+ if (transport_disconnect_raw(transport))
+ status = 1;
if (!dest && !quiet)
fprintf(stderr, "From %s\n", *remote->url); @@ -154,12 +156,12 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
if (show_symref_target && ref->symref)
printf("ref: %s\t%s\n", ref->symref, ref->refname);
printf("%s\t%s\n", oid_to_hex(&ref->objectname), ref->refname);
- status = 0; /* we found something */
+ if (status != 1)
+ status = 0; /* we found something */
}
ref_array_clear(&ref_array);
- if (transport_disconnect(transport))
- status = 1;
+ transport_clear(transport);
transport_ls_refs_options_release(&transport_options);
return status;
}
diff --git a/builtin/remote.c b/builtin/remote.c index d91bbe728d..055a221942 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -1000,6 +1000,7 @@ static int get_remote_ref_states(const char *name,
transport = transport_get(states->remote, states->remote->url_nr > 0 ?
states->remote->url[0] : NULL);
remote_refs = transport_get_remote_refs(transport, NULL);
+ transport_disconnect_raw(transport);
states->queried = 1;
if (query & GET_REF_STATES)
@@ -1008,7 +1009,7 @@ static int get_remote_ref_states(const char *name,
get_head_names(remote_refs, states);
if (query & GET_PUSH_REF_STATES)
get_push_ref_states(remote_refs, states);
- transport_disconnect(transport);
+ transport_clear(transport);
} else {
for_each_ref(append_ref_to_tracked_list, states);
string_list_sort(&states->tracked);
diff --git a/transport.c b/transport.c
index 219af8fd50..c71dab75e9 100644
--- a/transport.c
+++ b/transport.c
@@ -1584,17 +1584,28 @@ int transport_connect(struct transport *transport, const char *name,
die(_("operation not supported by protocol")); }
-int transport_disconnect(struct transport *transport)
+int transport_disconnect_raw(struct transport *transport)
{
int ret = 0;
if (transport->vtable->disconnect)
ret = transport->vtable->disconnect(transport);
+ return ret;
+}
+
+void transport_clear(struct transport *transport) {
if (transport->got_remote_refs)
free_refs((void *)transport->remote_refs);
clear_bundle_list(transport->bundles);
free(transport->bundles);
free(transport);
- return ret;
+}
+
+int transport_disconnect(struct transport *transport) {
+ int ret = transport_disconnect_raw(transport);
+ transport_clear(transport);
+ return 0;
}
/*
diff --git a/transport.h b/transport.h
index 6393cd9823..fd75905568 100644
--- a/transport.h
+++ b/transport.h
@@ -320,6 +320,8 @@ int transport_fetch_refs(struct transport *transport, struct ref *refs);
void transport_unlock_pack(struct transport *transport, unsigned int flags);
int transport_disconnect(struct transport *transport);
+void transport_clear(struct transport *transport);
+int transport_disconnect_raw(struct transport *transport);
char *transport_anonymize_url(const char *url);
void transport_take_over(struct transport *transport,
struct child_process *child);
--
2.42.0
^ permalink raw reply related
* Re: [PATCH] credential/wincred: store oauth_refresh_token
From: M Hickford @ 2023-10-30 8:00 UTC (permalink / raw)
To: M Hickford
Cc: git-for-windows, Taylor Blau, Git Mailing List,
Matthew John Cheetham, Jakub Bereżański, patthoyts
In-Reply-To: <CAGJzqskekExeug45Zh-uQ1dpHpKP8HS3obWQyz8B_DzSZiuAwA@mail.gmail.com>
Hi. Is anyone with Windows development experience able to take a look
at this patch to credential-wincred?
https://lore.kernel.org/git/pull.1534.git.1684567247339.gitgitgadget@gmail.com/
https://github.com/gitgitgadget/git/pull/1534
On Thu, 10 Aug 2023 at 20:58, M Hickford <mirth.hickford@gmail.com> wrote:
>
> Hi. A small patch to git-credential-wincred needs review on the Git
> mailing list https://lore.kernel.org/git/pull.1534.git.1684567247339.gitgitgadget@gmail.com/
> . Does anyone here have the expertise to review?
>
> On Sat, 20 May 2023 at 08:20, M Hickford via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> >
> > From: M Hickford <mirth.hickford@gmail.com>
> >
> > a5c7656 (credential: new attribute oauth_refresh_token) introduced
> > a new confidential credential attribute for helpers to store.
> >
> > We encode the new attribute in the CredentialBlob, separated by
> > newline:
> >
> > hunter2
> > oauth_refresh_token=xyzzy
> >
> > This is extensible and backwards compatible. The credential protocol
> > already assumes that attribute values do not contain newlines.
> >
> > Alternatives considered: store oauth_refresh_token in a wincred
> > attribute. This would be insecure because wincred assumes attribute
> > values to be non-confidential.
> >
> > Signed-off-by: M Hickford <mirth.hickford@gmail.com>
> > ---
> > credential/wincred: store oauth_refresh_token
> >
> > I'm not confident in C and found string manipulation difficult, so
> > please review carefully.
> >
> > I tested on Linux, cross compiling with Zig make CC="zig cc -target
> > x86_64-windows-gnu" and tested make GIT_TEST_CREDENTIAL_HELPER=wincred
> > t0303-credential-external.sh (with a shell script git-credential-wincred
> > that wraps wine64 git-credential-wincred.exe "$@")
> >
> > See also similar patch for credential-libsecret "[PATCH v4]
> > credential/libsecret: store new attributes"
> > https://lore.kernel.org/git/pull.1469.v4.git.git.1684306540947.gitgitgadget@gmail.com/T/#u
> >
> > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1534%2Fhickford%2Fwincred-refresh-v1
> > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1534/hickford/wincred-refresh-v1
> > Pull-Request: https://github.com/gitgitgadget/git/pull/1534
> >
> > .../wincred/git-credential-wincred.c | 40 ++++++++++++++++---
> > t/t0303-credential-external.sh | 1 +
> > 2 files changed, 35 insertions(+), 6 deletions(-)
> >
> > diff --git a/contrib/credential/wincred/git-credential-wincred.c b/contrib/credential/wincred/git-credential-wincred.c
> > index 96f10613aee..dc34283b85e 100644
> > --- a/contrib/credential/wincred/git-credential-wincred.c
> > +++ b/contrib/credential/wincred/git-credential-wincred.c
> > @@ -35,7 +35,7 @@ static void *xmalloc(size_t size)
> > }
> >
> > static WCHAR *wusername, *password, *protocol, *host, *path, target[1024],
> > - *password_expiry_utc;
> > + *password_expiry_utc, *oauth_refresh_token;
> >
> > static void write_item(const char *what, LPCWSTR wbuf, int wlen)
> > {
> > @@ -128,6 +128,11 @@ static void get_credential(void)
> > DWORD num_creds;
> > int i;
> > CREDENTIAL_ATTRIBUTEW *attr;
> > + WCHAR *secret;
> > + WCHAR *line;
> > + WCHAR *remaining_lines;
> > + WCHAR *part;
> > + WCHAR *remaining_parts;
> >
> > if (!CredEnumerateW(L"git:*", 0, &num_creds, &creds))
> > return;
> > @@ -137,9 +142,17 @@ static void get_credential(void)
> > if (match_cred(creds[i])) {
> > write_item("username", creds[i]->UserName,
> > creds[i]->UserName ? wcslen(creds[i]->UserName) : 0);
> > - write_item("password",
> > - (LPCWSTR)creds[i]->CredentialBlob,
> > - creds[i]->CredentialBlobSize / sizeof(WCHAR));
> > + secret = xmalloc(creds[i]->CredentialBlobSize);
> > + wcsncpy_s(secret, creds[i]->CredentialBlobSize, (LPCWSTR)creds[i]->CredentialBlob, creds[i]->CredentialBlobSize / sizeof(WCHAR));
> > + line = wcstok_s(secret, L"\r\n", &remaining_lines);
> > + write_item("password", line, wcslen(line));
> > + while(line != NULL) {
> > + part = wcstok_s(line, L"=", &remaining_parts);
> > + if (!wcscmp(part, L"oauth_refresh_token")) {
> > + write_item("oauth_refresh_token", remaining_parts, wcslen(remaining_parts));
> > + }
> > + line = wcstok_s(NULL, L"\r\n", &remaining_lines);
> > + }
> > for (int j = 0; j < creds[i]->AttributeCount; j++) {
> > attr = creds[i]->Attributes + j;
> > if (!wcscmp(attr->Keyword, L"git_password_expiry_utc")) {
> > @@ -148,6 +161,7 @@ static void get_credential(void)
> > break;
> > }
> > }
> > + free(secret);
> > break;
> > }
> >
> > @@ -158,16 +172,26 @@ static void store_credential(void)
> > {
> > CREDENTIALW cred;
> > CREDENTIAL_ATTRIBUTEW expiry_attr;
> > + WCHAR *secret;
> > + int wlen;
> >
> > if (!wusername || !password)
> > return;
> >
> > + if (oauth_refresh_token) {
> > + wlen = _scwprintf(L"%s\r\noauth_refresh_token=%s", password, oauth_refresh_token);
> > + secret = xmalloc(sizeof(WCHAR) * wlen);
> > + _snwprintf_s(secret, sizeof(WCHAR) * wlen, wlen, L"%s\r\noauth_refresh_token=%s", password, oauth_refresh_token);
> > + } else {
> > + secret = _wcsdup(password);
> > + }
> > +
> > cred.Flags = 0;
> > cred.Type = CRED_TYPE_GENERIC;
> > cred.TargetName = target;
> > cred.Comment = L"saved by git-credential-wincred";
> > - cred.CredentialBlobSize = (wcslen(password)) * sizeof(WCHAR);
> > - cred.CredentialBlob = (LPVOID)password;
> > + cred.CredentialBlobSize = wcslen(secret) * sizeof(WCHAR);
> > + cred.CredentialBlob = (LPVOID)_wcsdup(secret);
> > cred.Persist = CRED_PERSIST_LOCAL_MACHINE;
> > cred.AttributeCount = 0;
> > cred.Attributes = NULL;
> > @@ -182,6 +206,8 @@ static void store_credential(void)
> > cred.TargetAlias = NULL;
> > cred.UserName = wusername;
> >
> > + free(secret);
> > +
> > if (!CredWriteW(&cred, 0))
> > die("CredWrite failed");
> > }
> > @@ -253,6 +279,8 @@ static void read_credential(void)
> > password = utf8_to_utf16_dup(v);
> > else if (!strcmp(buf, "password_expiry_utc"))
> > password_expiry_utc = utf8_to_utf16_dup(v);
> > + else if (!strcmp(buf, "oauth_refresh_token"))
> > + oauth_refresh_token = utf8_to_utf16_dup(v);
> > /*
> > * Ignore other lines; we don't know what they mean, but
> > * this future-proofs us when later versions of git do
> > diff --git a/t/t0303-credential-external.sh b/t/t0303-credential-external.sh
> > index f028fd14182..51886b8e259 100755
> > --- a/t/t0303-credential-external.sh
> > +++ b/t/t0303-credential-external.sh
> > @@ -45,6 +45,7 @@ test -z "$GIT_TEST_CREDENTIAL_HELPER_SETUP" ||
> > helper_test_clean "$GIT_TEST_CREDENTIAL_HELPER"
> >
> > helper_test "$GIT_TEST_CREDENTIAL_HELPER"
> > +helper_test_oauth_refresh_token "$GIT_TEST_CREDENTIAL_HELPER"
> >
> > if test -z "$GIT_TEST_CREDENTIAL_HELPER_TIMEOUT"; then
> > say "# skipping timeout tests (GIT_TEST_CREDENTIAL_HELPER_TIMEOUT not set)"
> >
> > base-commit: 9e49351c3060e1fa6e0d2de64505b7becf157f28
> > --
> > gitgitgadget
^ permalink raw reply
* Re: please add link / url to remote - when - git push
From: Junio C Hamano @ 2023-10-30 6:55 UTC (permalink / raw)
To: Torsten Bögershausen; +Cc: Alexander Mills, git
In-Reply-To: <20231030063633.GA7451@tb-raspi4>
Torsten Bögershausen <tboegi@web.de> writes:
> On Sun, Oct 29, 2023 at 06:15:35PM -0500, Alexander Mills wrote:
>> When a feature branch is pushed, we get a link in the console to go to
>> remote, however when I push directly to main/master, no such link, eg:
>>
>> ```
>> alex.mills@alex node-be % git push
>> Enumerating objects: 20, done.
>> Counting objects: 100% (20/20), done.
>> Delta compression using up to 12 threads
>> Compressing objects: 100% (10/10), done.
>> Writing objects: 100% (11/11), 1.56 KiB | 799.00 KiB/s, done.
>> Total 11 (delta 7), reused 0 (delta 0), pack-reused 0
>> remote: Resolving deltas: 100% (7/7), completed with 7 local objects.
>> remote: Bypassed rule violations for refs/heads/main:
>> remote:
>> remote: - Changes must be made through a pull request.
>> remote:
>> To github.com:elx-onlinx/beautychat-chatcards.git
>> ffe1e05..bb7b0ef main -> main
>> ```
>>
>> Having the link in the console saves me tremendous time and is
>> extremely effective/efficient. Can we get links in the console plz?
>>
>
> If we look very carfully at the log, we see that all the messages prefixed
> with "remote:" come from the remote (git server).
> In your case github. Other "Git repo servers" like gitlab or bitbucket have
> the same feature.
>
> Git itself, running on your local computer, does not produce this links.
> If you really want to push directly to the main branch and want to see the
> a link, then you better talk to the gihub folks.
Thanks for a good suggestion.
Another piece of advice we may want to give is that a good bugreport
should illustrate both good and bad case. With only the above one,
I couldn't guess that the complaint was about lack of information
that comes over the wire from remote, *exactly* because the report
failed to show the good case that shows the information and made the
readers guess what thing that does not exist in the output is being
complained about. A good bugreport should not force readers guess.
If the report had both good and bad cases, it would have helped
readers help the reporter much easier.
Thanks.
^ permalink raw reply
* Re: please add link / url to remote - when - git push
From: Torsten Bögershausen @ 2023-10-30 6:36 UTC (permalink / raw)
To: Alexander Mills; +Cc: git
In-Reply-To: <CA+KyZp5mwGJ6YOvjKtfnDMDb9ci3vSq5KNUep6-8EfkHNaxREg@mail.gmail.com>
On Sun, Oct 29, 2023 at 06:15:35PM -0500, Alexander Mills wrote:
> When a feature branch is pushed, we get a link in the console to go to
> remote, however when I push directly to main/master, no such link, eg:
>
> ```
> alex.mills@alex node-be % git push
> Enumerating objects: 20, done.
> Counting objects: 100% (20/20), done.
> Delta compression using up to 12 threads
> Compressing objects: 100% (10/10), done.
> Writing objects: 100% (11/11), 1.56 KiB | 799.00 KiB/s, done.
> Total 11 (delta 7), reused 0 (delta 0), pack-reused 0
> remote: Resolving deltas: 100% (7/7), completed with 7 local objects.
> remote: Bypassed rule violations for refs/heads/main:
> remote:
> remote: - Changes must be made through a pull request.
> remote:
> To github.com:elx-onlinx/beautychat-chatcards.git
> ffe1e05..bb7b0ef main -> main
> ```
>
> Having the link in the console saves me tremendous time and is
> extremely effective/efficient. Can we get links in the console plz?
>
If we look very carfully at the log, we see that all the messages prefixed
with "remote:" come from the remote (git server).
In your case github. Other "Git repo servers" like gitlab or bitbucket have
the same feature.
Git itself, running on your local computer, does not produce this links.
If you really want to push directly to the main branch and want to see the
a link, then you better talk to the gihub folks.
^ permalink raw reply
* Re: [RFC PATCH v2 1/6] status: add noob format from status.noob config
From: Jacob Stopak @ 2023-10-30 6:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqjzr4gaie.fsf@gitster.g>
On Mon, Oct 30, 2023 at 10:32:57AM +0900, Junio C Hamano wrote:
> Jacob Stopak <jacob@initialcommit.io> writes:
>
> > diff --git a/table.c b/table.c
> > new file mode 100644
> > index 0000000000..15600e117f
> > --- /dev/null
> > +++ b/table.c
>
> Yuck, do we need an entirely new file? What trait are the things
> that are thrown into this file together supposed to share [*]? It
> is not very clear to me what the focus of this file is.
>
> Side note: for example, stuff in wt-status.c are to compute
> per-path status of the working tree and in-index files.
I created a new file because:
* It will be used by more than just the status command (as patches 3/6
and 5/6 show it applied to the "add" and "restore" commands
respectively). And it will be applied to more commands as well.
* For these RFC versions I mainly wanted to get some kind of minimal
POC working, so I could get feedback and build on
* It was easier to work on it in it's own file.
That being said, I was probably careless with what I tossed in there.
I intended it to specifically be things related to populating, coloring,
and printing the new table format.
But after reading all of your comments here, I see that I most likely
made the error (which I've made before) of being too specific. If I
generalize this file to be a generic table format that can be used for
anything, then the command-specific details can likely be extracted into
their respective existing files.
Another thing I totally didn't consider until I read your mail is the
possibility of refactoring the existing "git column" functionality to
provide a configurable table output format. At this point I'm not sure
if that's feasible or makes sense, but it's at least worth considering.
> > @@ -0,0 +1,117 @@
> > +#define USE_THE_INDEX_VARIABLE
>
> I personally do not mind, but I suspect many people hate to see this
> compatibility set of macros used in a newly written source file.
Noted, I'll try to update this.
> > +static const char *color(int slot, struct wt_status *s)
> > +{
> > + const char *c = "";
> > + if (want_color(s->use_color))
> > + c = s->color_palette[slot];
> > + if (slot == WT_STATUS_ONBRANCH && color_is_nil(c))
> > + c = s->color_palette[WT_STATUS_HEADER];
> > + return c;
> > +}
>
> Do we need to duplicate this from other files? If this is about
> "git status", perhaps some parts of this patch, the truly new things
> (rather than what was copied, like this one) can be added to
> wt-status.c instead of adding a new file with unclear focus?
I just re-used the status coloring out of laziness, since I was thinking
about this in a narrow way that it's basically replicating the output
of Git status, so I figured I'd use the same colors.
Now that I'm thinking more generally I see why this needs to change,
to make table column output colors flexible.
> > +static void build_table_border(struct strbuf *buf, int cols)
> > +{
> > + strbuf_reset(buf);
> > + strbuf_addchars(buf, '-', cols);
> > +}
>
> This seems to be horizontal border; do we need a separate vertical
> border?
Yes it is a horizontal border, the vertical border pipe characters are
included as a part of each table line being drawn, so I didn't need a
standalone vertical border in order to "draw" the tables. This one could
be renamed to reflect it's horizontal-ness, for clarity.
> > +static void build_table_entry(struct strbuf *buf, char *entry, int cols)
> > +{
> > + strbuf_reset(buf);
> > + strbuf_addchars(buf, ' ', (cols / 3 - 1 - strlen(entry)) / 2);
> > + strbuf_addstr(buf, entry);
> > +
> > + /* Bump right padding if entry length is odd */
> > + if (!(strlen(entry) % 2))
> > + strbuf_addchars(buf, ' ', (cols / 3 - 1 - strlen(entry)) / 2 + 1);
> > + else
> > + strbuf_addchars(buf, ' ', (cols / 3 - 1 - strlen(entry)) / 2);
> > +}
>
> The code assumes that one byte in string "entry" occupies one and
> only one display columns, which is so 20th centry assumption that
> does not care about i18n. Often what takes 3 bytes in a UTF-8
> string occupies 2 display columns, for example. In addition, if you
> plan to color entries in the table, some substring would end up to
> be 0-width.
Thanks for this info. I'll look into these.
> Your pathname may be so long that 1/3 of a window
> width may not be sufficient to show it in its entirety, you might
> need to show it truncated in the middle.
This is exactly what patch 2/6 does (more details on that below). I
should have squashed that into 1/6. I'll do that in the next series.
> utf8.c has support for measuring the display width of UTF-8 string,
> which is used elsewhere in our code. You may want to study it if
> you want to do a "tabular" output. The code in diff.c that shows
> diffstat has many gems to help what this code wants to do, including
> measuring display columns of a string, chomping a long string to fit
> in a desired display columns, etc., by using helpers defined in
> utf8.c
Well that sounds like exactly what I need. I'll take a look, thanks!
> A potential excuse I can think of to have these outside wt-status.c
> and in a separate new file is to have a generic "table" layout
> machinery that is independent from "git status" or what each column
> of the table is showing (in other words, they may not be pathnames),
> and reusable by other subcommands that want to show things in the
> "table" layout.
Yes that is my excuse (altho it may not be a valid one) - this patch
series adds the table output format for the "git add" (see patch 3/6) and
"git restore" (see patch 5/6) commands. Dragan will be working on
implementing verbose config options for several commands, that I will then
use as stepping stones to try and add this table format as the "extended"
(not "noob") verbosity option.
> But even as a candidate for such a generic table
> mechanism, the above falls far short by hardcoding that it can only
> show 3-col table whose columns are evenly distributed and nothing
> else.
Yeah - I was planning to update the table format so that the number of
columns is flexible. For example, I can think of commands like "git
stash" that would likely require 4 columns to display the required info,
so parameterizing the number of columns is on my agenda.
But you're right maybe there are many other useful things that a generic
table format would include. Things like alignment, spacing, etc. I didn't
think about how this might be used outside of my specific use case - I
tend to get tunnel vision on what I'm working on. I'll try to think more
broadly about how a format like this might be applicable, and at least
try to implement it in a way that it can be re-used for other things if
and when the need arises.
> > +static void print_table_body_line(struct strbuf *buf1, struct strbuf *buf2, struct strbuf *buf3, struct wt_status *s)
> > +{
> > + printf(_("|"));
> > + color_fprintf(s->fp, color(WT_STATUS_UNTRACKED, s), "%s", buf1->buf);
> > + printf(_("|"));
> > + color_fprintf(s->fp, color(WT_STATUS_CHANGED, s), "%s", buf2->buf);
> > + printf(_("|"));
> > + color_fprintf(s->fp, color(WT_STATUS_UPDATED, s), "%s", buf3->buf);
> > + printf(_("|\n"));
> > +}
>
> How does the code deal with unknown display width of translated
> version of "|" emitted here? Are you assuming that no matter how
> these are translated, they will always occupy one display column
> each?
I'm not familiar with how the translation process works yet, so _if_ the
version of the "|" that I'm emitting here is a canditate for translation,
it was not even my intention to do so, as I'm just using the vertical
pipe as a column separator and table border. I think what I need is to
make sure these are _not_ translatable, so that we can guarantee the
single character display width?
> > +void print_noob_status(struct wt_status *s)
> > +{
> > + struct winsize w;
> > + int cols;
> > + struct strbuf table_border = STRBUF_INIT;
> > + struct strbuf table_col_entry_1 = STRBUF_INIT;
> > + struct strbuf table_col_entry_2 = STRBUF_INIT;
> > + struct strbuf table_col_entry_3 = STRBUF_INIT;
> > + struct string_list_item *item;
> > +
> > + /* Get terminal width */
> > + ioctl(STDOUT_FILENO, TIOCGWINSZ, &w);
> > + cols = w.ws_col;
>
> Let's not reinvent an incomplete solution before studying and
> finding out what we already have in our codebase. Immediately after
> you got tempted to type TIOCGWINSZ, you can "git grep" the codebase
> for that particular constant to see if we already use it, as that is
> one very reasonable way to achieve what this piece of code wants to
> do (i.e. find out what the display width would be). You'd find
> pager.c:term_columns() and also learned that we want to prepare for
> the case where the ioctl() is not available.
Ah, ok let me look into that. I have utilized git grepping to understand
how various things are done in the codebase, but for some (no good) reason
I didn't think to try that here.
> > + build_table_entry(&table_col_entry_1, "Untracked files", cols);
> > + build_table_entry(&table_col_entry_2, "Unstaged changes", cols);
> > + build_table_entry(&table_col_entry_3, "Staging area", cols);
>
> Shouldn't these three strings be translatable?
Yes. I'll look into how translatable strings are done in the codebase.
> What should happen when these labels are wider than cols/3?
That's what patch 2/6 does. It splices out the center of strings that
are too long to fit in a column, keeping an equal number of characters
at the start and end of the string, and inserts a "..." in the middle.
Altho this makes sense for paths, this could be a bit confusing for
table headers, since it may be hard (or impossible) to read what the
column's purpose is, so it might be wiser to come up with even shorter
versions of the table headers to use when the console width is below a
certain size that makes the original strings unreadable.
> > diff --git a/table.h b/table.h
> > new file mode 100644
> > index 0000000000..c9e8c386de
> > --- /dev/null
> > +++ b/table.h
> > @@ -0,0 +1,6 @@
> > +#ifndef TABLE_H
> > +#define TABLE_H
> > +
> > +void print_noob_status(struct wt_status *s);
> > +
> > +#endif /* TABLE_H */
>
> I am guessing that your plan is to add other "distim_noob_add()" and
> other "noob" variant of operations for various Git subcommands here,
> but I really do not think you want to add table.[ch] that has logic
> for such random set of Git subcommands copied and tweaked from all
> over the place, as the only trait being shared among them will
> become "they are written by Jacob Stopak", that is not a very useful
> grouping of the functions. It is not even "this file collects all
> the code that produce tabular output from Git"---"git status -s"
> already gives tabular output, for example, without using any of the
> "I only want to draw a table with three columns of equal width"
> logic. Adding code that are necessary to add yet another output
> mode for "git status" directly to where various output modes of "git
> status" are implemented, i.e. wt-status.c, and do similar changes for
> each command would make more sense, I would think.
If you look at patches 3/6 and 5/6 I think you'll better understand my
"plan", not that I thought this would by any kind of final version, but
more of a starting point to get feedback and build on (hence the RFC).
I think the reason I clung so hard to "status" is that it contains a
lot of the info that I planned to display across various commands, so
much so that I baked it into the new table file itself. I see now that
needs to be separated out, as that data to be displayed can be calculated
in each command itself, and passed into the table functions to build and
print the thing.
But based on your input in this mail I'm going to:
* Look into the existing "column" formatter to see if it can be
repurposed for this table stuff.
* If not, generalize the table stuff so that it can be used for anything
and compartmentalize the command-specific stuff where it belongs.
> Thanks.
Thank you!
^ permalink raw reply
* Re: [PATCH 0/2] Avoid passing global comment_line_char repeatedly
From: Dragan Simic @ 2023-10-30 5:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <20231030051034.2295242-1-gitster@pobox.com>
On 2023-10-30 06:10, Junio C Hamano wrote:
> Two strbuf functions used to produce commented lines take the
> comment_line_char as their parameter, but in practice, all callers
> feed the global variable comment_line_char from environment.[ch].
>
> Dropping the parameter from the callchain will make the interface
> less flexible, and less error prone. If we choose to change the
> implementation of the customizable comment line character (e.g., we
> may want to stop referencing the global variable and instead use a
> getter function), we will have fewer places we need to modify.
>
> Junio C Hamano (2):
> strbuf_commented_addf(): drop the comment_line_char parameter
> strbuf_add_commented_lines(): drop the comment_line_char parameter
This series looks good to me. It removes unnecessary complexity that
provided pretty much no value.
> add-patch.c | 8 ++++----
> builtin/branch.c | 2 +-
> builtin/merge.c | 8 ++++----
> builtin/notes.c | 9 ++++-----
> builtin/stripspace.c | 2 +-
> builtin/tag.c | 4 ++--
> fmt-merge-msg.c | 9 +++------
> rebase-interactive.c | 8 ++++----
> sequencer.c | 14 ++++++--------
> strbuf.c | 9 +++++----
> strbuf.h | 7 +++----
> wt-status.c | 6 +++---
> 12 files changed, 40 insertions(+), 46 deletions(-)
^ permalink raw reply
* [PATCH 2/2] strbuf_add_commented_lines(): drop the comment_line_char parameter
From: Junio C Hamano @ 2023-10-30 5:10 UTC (permalink / raw)
To: git
In-Reply-To: <20231030051034.2295242-1-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>
---
builtin/notes.c | 9 ++++-----
builtin/stripspace.c | 2 +-
fmt-merge-msg.c | 9 +++------
rebase-interactive.c | 6 +++---
sequencer.c | 10 ++++------
strbuf.c | 6 +++---
strbuf.h | 3 +--
wt-status.c | 4 ++--
8 files changed, 21 insertions(+), 28 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/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 15550b2619..2088f7800a 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -360,8 +360,8 @@ static void add_lines(struct strbuf *out,
strbuf_complete_line(out);
}
-void strbuf_add_commented_lines(struct strbuf *out, const char *buf,
- size_t size, char comment_line_char)
+void strbuf_add_commented_lines(struct strbuf *out,
+ const char *buf, size_t size)
{
static char prefix1[3];
static char prefix2[2];
@@ -384,7 +384,7 @@ 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';
diff --git a/strbuf.h b/strbuf.h
index 981617dc77..4547efa62e 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -287,8 +287,7 @@ void strbuf_splice(struct strbuf *sb, size_t pos, size_t len,
* 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);
+ const char *buf, size_t size);
/**
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-526-g3130c155df
^ permalink raw reply related
* [PATCH 1/2] strbuf_commented_addf(): drop the comment_line_char parameter
From: Junio C Hamano @ 2023-10-30 5:10 UTC (permalink / raw)
To: git
In-Reply-To: <20231030051034.2295242-1-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>
---
add-patch.c | 8 ++++----
builtin/branch.c | 2 +-
builtin/merge.c | 8 ++++----
builtin/tag.c | 4 ++--
rebase-interactive.c | 2 +-
sequencer.c | 4 ++--
strbuf.c | 3 ++-
strbuf.h | 4 ++--
wt-status.c | 2 +-
9 files changed, 19 insertions(+), 18 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/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 7827178d8e..15550b2619 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"
@@ -372,7 +373,7 @@ void strbuf_add_commented_lines(struct strbuf *out, const char *buf,
add_lines(out, prefix1, prefix2, buf, size);
}
-void strbuf_commented_addf(struct strbuf *sb, char comment_line_char,
+void strbuf_commented_addf(struct strbuf *sb,
const char *fmt, ...)
{
va_list params;
diff --git a/strbuf.h b/strbuf.h
index e959caca87..981617dc77 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -378,8 +378,8 @@ 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, 3)))
+void strbuf_commented_addf(struct strbuf *sb, 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-526-g3130c155df
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox