* Re: [RFC] Define "precious" attribute and support it in `git clean`
From: Josh Triplett @ 2023-10-12 8:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Kristoffer Haugsbakk, Sebastian Thiel, git
In-Reply-To: <xmqqo7h6tnib.fsf@gitster.g>
On Tue, Oct 10, 2023 at 10:07:08AM -0700, Junio C Hamano wrote:
> Josh Triplett <josh@joshtriplett.org> writes:
>
> > While I'd love for it to default to that and require an extra option to
> > clean away precious files, I'd expect that that would break people's
> > workflows and finger memory. If someone expects `git clean -x -d -f` to
> > clean away everything, including `.config`, and then it leaves some
> > files in place, that seems likely to cause problems. (Leaving aside that
> > it might break scripted workflows.)
>
> I thought the point of introducing the new "precious" class of
> paths, in addition to the current "tracked", "ignored, untracked,
> and expendable", "not ignored and untracked", is so that people can
> do "git clean -x -d -f" and expect the ".config" that is marked as
> "precious" to stay. Before their Git learned the precious class, if
> they marked ".config" as "ignored, untracked, and expendable", then
> such an invocation of "clean" would have removed it, but if they add
> it to the new "precious" class, their expectation ought to be that
> precious ones are not removed, no? Otherwise I am not quite sure
> what the point of adding such a new protection is.
I'd expect a lot of projects to move things *from* the current "ignored"
state to "precious", once "precious" exists. Linux `.config`, for
instance.
That said, I do agree that the ideal behavior is for clean to preserve
precious files by default, and require an extra option to remove
precious files. If you think that doesn't have backwards-compatibility
considerations, then it certainly seems much easier to jump directly to
that behavior.
- Josh Triplett
^ permalink raw reply
* Re: [Outreachy]Introduction and Problem while installing Git
From: Dorcas Litunya @ 2023-10-12 7:52 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: benson_muite, git
In-Reply-To: <724ff75e-1557-4d87-85a6-a3a58d41af1f@app.fastmail.com>
On Thu, Oct 12, 2023 at 09:43:55AM +0200, Kristoffer Haugsbakk wrote:
> On Thu, Oct 12, 2023, at 09:28, Benson Muite wrote:
> > You maybe able to get these from a package manager, for example on Ubuntu
> > sudo apt-get install curl-dev
>
> This didn't work for me on Ubuntu 22.04. I tried the OpenSSL variant which
> worked.
>
Me too
> sudo apt-get install libcurl4-openssl-dev
>
This package has worked for me on Ubuntu 22.04
Thanks for the help!
> Now I can drop `NO_CURL=true`.
>
> Thanks :)
>
> --
> Kristoffer Haugsbakk
^ permalink raw reply
* Re: [Outreachy]Introduction and Problem while installing Git
From: Kristoffer Haugsbakk @ 2023-10-12 7:43 UTC (permalink / raw)
To: Benson Muite; +Cc: Dorcas Litunya, git
In-Reply-To: <4c5fef38-a671-dd6b-4b10-a531e1ae254a@emailplus.org>
On Thu, Oct 12, 2023, at 09:28, Benson Muite wrote:
> You maybe able to get these from a package manager, for example on Ubuntu
> sudo apt-get install curl-dev
This didn't work for me on Ubuntu 22.04. I tried the OpenSSL variant which
worked.
sudo apt-get install libcurl4-openssl-dev
Now I can drop `NO_CURL=true`.
Thanks :)
--
Kristoffer Haugsbakk
^ permalink raw reply
* Re: [Outreachy]Introduction and Problem while installing Git
From: Benson Muite @ 2023-10-12 7:28 UTC (permalink / raw)
To: Dorcas Litunya, git
In-Reply-To: <ZSeYzdx07Cj67lR4@dorcaslitunya-virtual-machine>
On 10/12/23 09:57, Dorcas Litunya wrote:
> Hello everyone,
> My name is Dorcas Litunya. I am excited to contribute to the git
> community, I am a first time contributor through the Outreachy program.
> I am excited to learn and grow through this project. I am currently
> installing Git and I have been faced with this error once I run the make
> command:
> In file included from http.c:2:
> git-curl-compat.h:3:10: fatal error: curl/curl.h: No such file or directory
> 3 | #include <curl/curl.h>
> | ^~~~~~~~~~~~~
You will need to have curl libraries and development headers.
https://curl.se/libcurl/
You maybe able to get these from a package manager, for example on Ubuntu
sudo apt-get install curl-dev
Fedora
sudo dnf install libcurl-devel
^ permalink raw reply
* [Outreachy]Introduction and Problem while installing Git
From: Dorcas Litunya @ 2023-10-12 6:57 UTC (permalink / raw)
To: git
Hello everyone,
My name is Dorcas Litunya. I am excited to contribute to the git
community, I am a first time contributor through the Outreachy program.
I am excited to learn and grow through this project. I am currently
installing Git and I have been faced with this error once I run the make
command:
In file included from http.c:2:
git-curl-compat.h:3:10: fatal error: curl/curl.h: No such file or directory
3 | #include <curl/curl.h>
| ^~~~~~~~~~~~~
compilation terminated.
make: *** [Makefile:2718: http.o] Error 1
Any assistance on what the possible problem could be will be highly
appreciated.
Kind regards,
Dorcas
^ permalink raw reply
* Wow
From: Edward Zmewski jr @ 2023-10-12 6:27 UTC (permalink / raw)
To: git
Not exactly sure who giving me these directions, but I’m very good at what I do to, all do respect, I tried just don’t register, I can use computers, but you guys are brilliant and I respect it, but I’ll try , but we wouldn’t be having a conversation if I was that good at programming, seriously, my apologies for not thinking to look out for you guys, that’s my mistake, listen I will help you guys if you can help me, thank you
,
^ permalink raw reply
* Re: why does git set X in LESS env var?
From: Dragan Simic @ 2023-10-12 5:57 UTC (permalink / raw)
To: Christoph Anton Mitterer; +Cc: Jeff King, Junio C Hamano, git
In-Reply-To: <07bf5744c7d123635740c62a940999e089339fa4.camel@scientia.org>
On 2023-10-12 05:54, Christoph Anton Mitterer wrote:
> Hey.
>
> Just noted that the popular bat utility apparently also uses -X to make
> -F work (but also mention that this break scrolling).
>
> But it seem they have a check, an if less is version 530 or newer they
> don't set -X.
>
> https://github.com/sharkdp/bat#using-a-different-pager
>
> Could be a way to go for git.
Let me repeat that the described scrolling-related issues are
misleading. I use this, configured through $BAT_PAGER, and have never
experienced such issues in a few years of using bat(1):
├─bash
│ └─bat -n --tabs 8 --wrap never message.txt
│ └─less -R -F -X
^ permalink raw reply
* Re: why does git set X in LESS env var?
From: Dragan Simic @ 2023-10-12 5:46 UTC (permalink / raw)
To: Christoph Anton Mitterer; +Cc: Junio C Hamano, git
In-Reply-To: <2f3ef5568ed19ac5bdcd23f84ddfb13dc6901043.camel@scientia.org>
On 2023-10-12 03:39, Christoph Anton Mitterer wrote:
> On Thu, 2023-10-12 at 02:31 +0200, Dragan Simic wrote:
>>
>> Makes sense, but please see also my other reply on the list. To sum
>> it
>> up, we can have either the current behavior, the inconsistent
>> behavior,
>> or an even more annoying behavior. I believe that the current
>> behavior
>> is the best choice among these three options.
>
> Well as I've said... I don't demand that it's changed, but I simply
> think it's a wrong assumption that it's in any way better or worse.
>
> Leaving back partial output, or when scrolling up&down completely
> messed up output, is surely not per se more annoying or a bigger
> problem than leaving back no output at all in one case (when it doesn't
> fit on one screen) or leaving back output (when it fits).
Let me repeat that the messed up output you're experiencing isn't normal
and has nothing to do with the arguments passed to less(1). That's a
separate issue of the terminal emulator(s) you're using, or in issue of
your specific environment, and should be debugged and addressed as a
separate issue.
To me, having inconsistent displaying of the short and long outputs is
simply not acceptable.
>> Yes, I'm sure, because I'd be fixing that already if that were the
>> case
>> in my environment. :) I use Xfce and its default terminal emulator,
>> though, and I don't know what it's like in other desktop environments
>> and their terminal emulators.
>
> I just tried it with xfce4-terminal 1.1.0 (which AFAICS is the most
> recent version) in Debian, and unless they break anything with custom
> patches, or you distro fixes anything with custom patches... I'd say
> you must suffer from the same issue and probably just try something
> different.
Let me repeat that I don't see those issues, and actually, IIRC, have
never seen them in my 25+ years of using various Linux distributions.
If I've ever seen that, it would've already motivated me to have it
debugged and fixed.
Perhaps something is wrong with your specific environment, because I see
no other reason for this issue.
> Since Debian's less is pretty outdated, I've even compiled a quite
> recent less 643 (there's not even a tarball yet for 644, only a git
> tag).
>
> A made a screen recording... it's not 8K ;-) but I guess you can see
> what I do:
> https://youtu.be/KMs3sLk9nXY
For some reason, I can see it with 360p as the highest available
resolution, so I really can't read what's displayed on the recorded
screen. Strange. Could you, please, upload the video in higher
resolution, perhaps to a file sharing service such as
https://easyupload.io/ ?
^ permalink raw reply
* Re: [PATCH 1/1] branch.c: ammend error messages for die()
From: Isoken Ibizugbe @ 2023-10-12 5:31 UTC (permalink / raw)
To: Rubén Justo; +Cc: git, Junio C Hamano
In-Reply-To: <06bc7b39-ed70-460f-b6f1-47a0c94c0538@gmail.com>
On Wed, Oct 11, 2023 at 7:05 PM Rubén Justo <rjusto@gmail.com> wrote:
>
> On 11-oct-2023 16:24:20, Isoken June Ibizugbe wrote:
>
> Hi Isoken,
>
> > Signed-off-by: Isoken June Ibizugbe <isokenjune@gmail.com>
> > ---
> > builtin/branch.c | 38 +++++++++++++++++++-------------------
> > 1 file changed, 19 insertions(+), 19 deletions(-)
> >
> > diff --git a/builtin/branch.c b/builtin/branch.c
> > index 2ec190b14a..a756543d64 100644
> > --- a/builtin/branch.c
> > +++ b/builtin/branch.c
> > @@ -518,11 +518,11 @@ static void reject_rebase_or_bisect_branch(struct worktree **worktrees,
> > continue;
> >
> > if (is_worktree_being_rebased(wt, target))
> > - die(_("Branch %s is being rebased at %s"),
> > + die(_("branch %s is being rebased at %s"),
>
> OK.
>
> > target, wt->path);
> >
> > if (is_worktree_being_bisected(wt, target))
> > - die(_("Branch %s is being bisected at %s"),
> > + die(_("branch %s is being bisected at %s"),
>
> OK.
>
> > target, wt->path);
> > }
> > }
> > @@ -578,7 +578,7 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
> > if (ref_exists(oldref.buf))
> > recovery = 1;
> > else
> > - die(_("Invalid branch name: '%s'"), oldname);
> > + die(_("invalid branch name: '%s'"), oldname);
>
> OK.
>
> > }
> >
> > for (int i = 0; worktrees[i]; i++) {
> > @@ -594,9 +594,9 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
> >
> > if ((copy || !(oldref_usage & IS_HEAD)) && !ref_exists(oldref.buf)) {
> > if (oldref_usage & IS_HEAD)
> > - die(_("No commit on branch '%s' yet."), oldname);
> > + die(_("no commit on branch '%s' yet"), oldname);
> > else
> > - die(_("No branch named '%s'."), oldname);
> > + die(_("no branch named '%s'"), oldname);
>
> OK. Both.
>
> > }
> >
> > /*
> > @@ -624,9 +624,9 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
> >
> > if (!copy && !(oldref_usage & IS_ORPHAN) &&
> > rename_ref(oldref.buf, newref.buf, logmsg.buf))
> > - die(_("Branch rename failed"));
> > + die(_("branch rename failed"));
> > if (copy && copy_existing_ref(oldref.buf, newref.buf, logmsg.buf))
> > - die(_("Branch copy failed"));
> > + die(_("branch copy failed"));
>
> Ditto
>
> >
> > if (recovery) {
> > if (copy)
> > @@ -640,16 +640,16 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
> > if (!copy && (oldref_usage & IS_HEAD) &&
> > replace_each_worktree_head_symref(worktrees, oldref.buf, newref.buf,
> > logmsg.buf))
> > - die(_("Branch renamed to %s, but HEAD is not updated!"), newname);
> > + die(_("branch renamed to %s, but HEAD is not updated!"), newname);
>
> IMO we can go further here and also remove that final "!". But it's OK
> the way you have done it.
>
> >
> > strbuf_release(&logmsg);
> >
> > strbuf_addf(&oldsection, "branch.%s", interpreted_oldname);
> > strbuf_addf(&newsection, "branch.%s", interpreted_newname);
> > if (!copy && git_config_rename_section(oldsection.buf, newsection.buf) < 0)
> > - die(_("Branch is renamed, but update of config-file failed"));
> > + die(_("branch is renamed, but update of config-file failed"));
> > if (copy && strcmp(interpreted_oldname, interpreted_newname) && git_config_copy_section(oldsection.buf, newsection.buf) < 0)
> > - die(_("Branch is copied, but update of config-file failed"));
> > + die(_("branch is copied, but update of config-file failed"));
>
> OK, both.
>
> > strbuf_release(&oldref);
> > strbuf_release(&newref);
> > strbuf_release(&oldsection);
> > @@ -773,7 +773,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
> >
> > head = resolve_refdup("HEAD", 0, &head_oid, NULL);
> > if (!head)
> > - die(_("Failed to resolve HEAD as a valid ref."));
> > + die(_("failed to resolve HEAD as a valid ref"));
>
> OK.
>
> > if (!strcmp(head, "HEAD"))
> > filter.detached = 1;
> > else if (!skip_prefix(head, "refs/heads/", &head))
> > @@ -866,7 +866,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
> >
> > if (!argc) {
> > if (filter.detached)
> > - die(_("Cannot give description to detached HEAD"));
> > + die(_("cannot give description to detached HEAD"));
>
> OK.
>
> > branch_name = head;
> > } else if (argc == 1) {
> > strbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);
> > @@ -892,7 +892,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
> > if (!argc)
> > die(_("branch name required"));
> > else if ((argc == 1) && filter.detached)
> > - die(copy? _("cannot copy the current branch while not on any.")
> > + die(copy? _("cannot copy the current branch while not on any")
> > : _("cannot rename the current branch while not on any."));
>
> OK. But I think you want to modify also the second message, to remove
> its full stop as well.
>
> > else if (argc == 1)
> > copy_or_rename_branch(head, argv[0], copy, copy + rename > 1);
> > @@ -916,14 +916,14 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
> > if (!branch) {
> > if (!argc || !strcmp(argv[0], "HEAD"))
> > die(_("could not set upstream of HEAD to %s when "
> > - "it does not point to any branch."),
> > + "it does not point to any branch"),
>
> OK.
>
> > new_upstream);
> > die(_("no such branch '%s'"), argv[0]);
> > }
> >
> > if (!ref_exists(branch->refname)) {
> > if (!argc || branch_checked_out(branch->refname))
> > - die(_("No commit on branch '%s' yet."), branch->name);
> > + die(_("no commit on branch '%s' yet"), branch->name);
>
> OK.
>
> > die(_("branch '%s' does not exist"), branch->name);
> > }
> >
> > @@ -946,12 +946,12 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
> > if (!branch) {
> > if (!argc || !strcmp(argv[0], "HEAD"))
> > die(_("could not unset upstream of HEAD when "
> > - "it does not point to any branch."));
> > + "it does not point to any branch"));
> > die(_("no such branch '%s'"), argv[0]);
> > }
> >
> > if (!branch_has_merge_config(branch))
> > - die(_("Branch '%s' has no upstream information"), branch->name);
> > + die(_("branch '%s' has no upstream information"), branch->name);
>
> OK, both.
>
> >
> > strbuf_reset(&buf);
> > strbuf_addf(&buf, "branch.%s.remote", branch->name);
> > @@ -965,11 +965,11 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
> > const char *start_name = argc == 2 ? argv[1] : head;
> >
> > if (filter.kind != FILTER_REFS_BRANCHES)
> > - die(_("The -a, and -r, options to 'git branch' do not take a branch name.\n"
> > + die(_("the -a, and -r, options to 'git branch' do not take a branch name\n"
> > "Did you mean to use: -a|-r --list <pattern>?"));
>
> Good; the full stop removed and here that question mark is valuable. And ...
>
> >
> > if (track == BRANCH_TRACK_OVERRIDE)
> > - die(_("the '--set-upstream' option is no longer supported. Please use '--track' or '--set-upstream-to' instead."));
> > + die(_("the '--set-upstream' option is no longer supported. Please use '--track' or '--set-upstream-to' instead"));
>
> also good. But since we're here, maybe we can break this long message, remove
> the first full stop and leave the second part of the message as is, as a
> suggestion. Like we do in the previous message, above.
>
> For your consideration, I mean something like:
>
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -969,7 +969,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
> "Did you mean to use: -a|-r --list <pattern>?"));
>
> if (track == BRANCH_TRACK_OVERRIDE)
> - die(_("the '--set-upstream' option is no longer supported. Please use '--track' or '--set-upstream-to' instead."));
> + die(_("the '--set-upstream' option is no longer supported\n"
> + "Please use '--track' or '--set-upstream-to' instead."));
>
>
> >
> > if (recurse_submodules) {
> > create_branches_recursively(the_repository, branch_name,
> > --
> > 2.42.0.325.g3a06386e31
> >
>
> One final comment, leaving aside my suggestions; this changes break
> some tests that you need to adjust. Try:
>
> $ make && (cd t; ./t3200-branch.sh -vi)
>
> Or, as Junio has already suggested in another message:
>
> $ make test
>
> I have some unfinished work that you might find useful:
>
> https://lore.kernel.org/git/eb3c689e-efeb-4468-a10f-dd32bc0ee37b@gmail.com/
Thanks for the review. I'll make the requested changes and provide an
update. I'll also make sure to run the tests you suggested and make
the necessary changes.
>
> Thank you for working on this.
^ permalink raw reply
* Re: why does git set X in LESS env var?
From: Dragan Simic @ 2023-10-12 5:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Christoph Anton Mitterer, git
In-Reply-To: <xmqqh6mwipqi.fsf@gitster.g>
On 2023-10-12 03:39, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
>> ... Digging into the history and
>> the changelog, this note is in "changes between less versions 487 and
>> 530":
>>
>> Don't output terminal init sequence if using -F and file fits on one
>> screen.
>
> ;-)
>
> That is really a good one to dig out. So in short, X was needed
> because we wanted to use F, and we could drop it if everybody is
> using recent versions of less, but the default to use FX at the same
> time gives us the same behaviour between both newer and older
> versions of less.
Please note that dropping "-X" and leaving "-F" would actually introduce
the inconsistency that I already mentioned. To reiterate, short outputs
would then remain displayed on screen, while long outputs would
disappear after exiting less(1). I don't think that's the desired
behavior.
>> ... And as you say, if there are people who really
>> care about their LESS options, it is easy for them to override it.
>
> Yup. I really like the discovery of that changelog entry.
>
> Thanks.
^ permalink raw reply
* Re: why does git set X in LESS env var?
From: Christoph Anton Mitterer @ 2023-10-12 3:54 UTC (permalink / raw)
To: Jeff King, Junio C Hamano; +Cc: git
In-Reply-To: <20231012000416.GA520855@coredump.intra.peff.net>
Hey.
Just noted that the popular bat utility apparently also uses -X to make
-F work (but also mention that this break scrolling).
But it seem they have a check, an if less is version 530 or newer they
don't set -X.
https://github.com/sharkdp/bat#using-a-different-pager
Could be a way to go for git.
Cheers,
Chris.
^ permalink raw reply
* Re: why does git set X in LESS env var?
From: Christoph Anton Mitterer @ 2023-10-12 1:39 UTC (permalink / raw)
To: Dragan Simic; +Cc: Junio C Hamano, git
In-Reply-To: <23cc509bfb433e19c7683c97314e4ac8@manjaro.org>
On Thu, 2023-10-12 at 02:31 +0200, Dragan Simic wrote:
>
> Makes sense, but please see also my other reply on the list. To sum
> it
> up, we can have either the current behavior, the inconsistent
> behavior,
> or an even more annoying behavior. I believe that the current
> behavior
> is the best choice among these three options.
Well as I've said... I don't demand that it's changed, but I simply
think it's a wrong assumption that it's in any way better or worse.
Leaving back partial output, or when scrolling up&down completely
messed up output, is surely not per se more annoying or a bigger
problem than leaving back no output at all in one case (when it doesn't
fit on one screen) or leaving back output (when it fits).
> Yes, I'm sure, because I'd be fixing that already if that were the
> case
> in my environment. :) I use Xfce and its default terminal emulator,
> though, and I don't know what it's like in other desktop environments
> and their terminal emulators.
I just tried it with xfce4-terminal 1.1.0 (which AFAICS is the most
recent version) in Debian, and unless they break anything with custom
patches, or you distro fixes anything with custom patches... I'd say
you must suffer from the same issue and probably just try something
different.
Since Debian's less is pretty outdated, I've even compiled a quite
recent less 643 (there's not even a tarball yet for 644, only a git
tag).
A made a screen recording... it's not 8K ;-) but I guess you can see
what I do:
https://youtu.be/KMs3sLk9nXY
Cheers,
Chris.
^ permalink raw reply
* Re: why does git set X in LESS env var?
From: Junio C Hamano @ 2023-10-12 1:39 UTC (permalink / raw)
To: Jeff King; +Cc: Christoph Anton Mitterer, git
In-Reply-To: <20231012000416.GA520855@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> ... Digging into the history and
> the changelog, this note is in "changes between less versions 487 and
> 530":
>
> Don't output terminal init sequence if using -F and file fits on one
> screen.
;-)
That is really a good one to dig out. So in short, X was needed
because we wanted to use F, and we could drop it if everybody is
using recent versions of less, but the default to use FX at the same
time gives us the same behaviour between both newer and older
versions of less.
> ... And as you say, if there are people who really
> care about their LESS options, it is easy for them to override it.
Yup. I really like the discovery of that changelog entry.
Thanks.
^ permalink raw reply
* Re: why does git set X in LESS env var?
From: Dragan Simic @ 2023-10-12 0:31 UTC (permalink / raw)
To: Christoph Anton Mitterer; +Cc: Junio C Hamano, git
In-Reply-To: <8f3bec2752f4c2d3ebdd29d20910a4a94f75f608.camel@scientia.org>
On 2023-10-12 02:22, Christoph Anton Mitterer wrote:
> On Thu, 2023-10-12 at 02:06 +0200, Dragan Simic wrote:
>> There's also scrollback in the terminal, which can be used to show
>> more
>> of the contents that was displayed before exiting the pager.
>
> Sure.
>
>> > Everything that would have come after that is of course not
>> > visible.
>> > The place where I exited may be some "well defined" border, like
>> > the
>> > end of a commit... or anywhere it the middle of a patch (making the
>> > left over remains on the terminal perhaps even ambiguous).
>>
>> If you didn't select some line or page to be displayed, by scrolling
>> within the pager, it obviously isn't going to be displayed, which is
>> the
>> whole point of using a pager instead of "spitting" the whole contents
>> out at once.
>
> It's also clear that it's one point of a pager :-)
>
> But that doesn't change that it's rather a user decision, whether or
> not it makes sense to leave that, what's already been shown by the
> pager, on the terminal after exiting the pager or not.
>
> I don't think people always select the lines in the pager to some
> reasonable border (e.g. end of a commit, end of a hunk, whatever).
> So it's likely that after leaving the pager, the terminal's scrollback
> buffer will contain something that is not complete and may thus be
> ambiguous.
Makes sense, but please see also my other reply on the list. To sum it
up, we can have either the current behavior, the inconsistent behavior,
or an even more annoying behavior. I believe that the current behavior
is the best choice among these three options.
>> That sounds like some issue with your terminal or terminal emulator,
>> which should be debugged and fixed separately. Such misbehavior
>> isn't
>> supposed to happen at all.
>
> Are you sure about that?
>
> Well it happens at least in gnome-terminal, xterm and (KDE) konsole.
Yes, I'm sure, because I'd be fixing that already if that were the case
in my environment. :) I use Xfce and its default terminal emulator,
though, and I don't know what it's like in other desktop environments
and their terminal emulators.
>> I see. Actually, removing "-S" was a good decision, IMHO, because
>> chopping long lines isn't something that a sane set of defaults
>> should
>> do. Many users would probably be confused with the need to use the
>> right arrow to see long lines in their entirety.
>
> Sure.
>
> And having -F is IMO a good default (that virtually everyone would
> want), too.
>
> With respect to -X, I'm less sure whether it's that clear.
Please see my other response, which explains why having "-FX" is
actually a good thing.
^ permalink raw reply
* Re: why does git set X in LESS env var?
From: Christoph Anton Mitterer @ 2023-10-12 0:22 UTC (permalink / raw)
To: Dragan Simic; +Cc: Junio C Hamano, git
In-Reply-To: <ace230a469fabbbbceb38cc884a40b4c@manjaro.org>
On Thu, 2023-10-12 at 02:06 +0200, Dragan Simic wrote:
> There's also scrollback in the terminal, which can be used to show
> more
> of the contents that was displayed before exiting the pager.
Sure.
> > Everything that would have come after that is of course not
> > visible.
> > The place where I exited may be some "well defined" border, like
> > the
> > end of a commit... or anywhere it the middle of a patch (making the
> > left over remains on the terminal perhaps even ambiguous).
>
> If you didn't select some line or page to be displayed, by scrolling
> within the pager, it obviously isn't going to be displayed, which is
> the
> whole point of using a pager instead of "spitting" the whole contents
> out at once.
It's also clear that it's one point of a pager :-)
But that doesn't change that it's rather a user decision, whether or
not it makes sense to leave that, what's already been shown by the
pager, on the terminal after exiting the pager or not.
I don't think people always select the lines in the pager to some
reasonable border (e.g. end of a commit, end of a hunk, whatever).
So it's likely that after leaving the pager, the terminal's scrollback
buffer will contain something that is not complete and may thus be
ambiguous.
>
> That sounds like some issue with your terminal or terminal emulator,
> which should be debugged and fixed separately. Such misbehavior
> isn't
> supposed to happen at all.
Are you sure about that?
Well it happens at least in gnome-terminal, xterm and (KDE) konsole.
> I see. Actually, removing "-S" was a good decision, IMHO, because
> chopping long lines isn't something that a sane set of defaults
> should
> do. Many users would probably be confused with the need to use the
> right arrow to see long lines in their entirety.
Sure.
And having -F is IMO a good default (that virtually everyone would
want), too.
With respect to -X, I'm less sure whether it's that clear.
Cheers,
Chris.
^ permalink raw reply
* Re: why does git set X in LESS env var?
From: Dragan Simic @ 2023-10-12 0:16 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Christoph Anton Mitterer, git
In-Reply-To: <20231012000416.GA520855@coredump.intra.peff.net>
On 2023-10-12 02:04, Jeff King wrote:
> On Wed, Oct 11, 2023 at 03:23:20PM -0700, Junio C Hamano wrote:
>
>> Christoph Anton Mitterer <calestyo@scientia.org> writes:
>>
>> > But I still don't get from that why X would be needed?
>> >
>> > My less manpage documents it as:
>> >> -X or --no‐init
>> >> Disables sending the termcap initialization and deinitialization
>> >> strings to the terminal. This is sometimes desirable if the
>> >> deinitialization string does something unnecessary, like clearing
>> >> the screen.
>> >
>> > Is it to avoid clearing the screen?
>>
>> I think that was the reason we added it back in 2005. In any case,
>> asking "why" is not a useful use of anybody's time, because it is
>> very unlikely to change in the official version we ship, and because
>> it is so easy for any individual who does not like it to drop by
>> exporting the $LESS environment variable.
>
> I agree it is probably not worth changing now, but I think the history
> here is a little interesting.
>
> Yes, I think "X" was added because less would clear the screen after
> exiting, and with "F" this meant you'd see nothing. Here's a thread
> from
> the same time period discussing it:
>
>
> https://lore.kernel.org/git/cc723f590610210623sbee2075i5f2fd441cceb84ae@mail.gmail.com/
>
> But I also think this was a pretty well-known annoyance with "less"
> back
> then.
>
> However, I can't seem to reproduce it now! Digging into the history and
> the changelog, this note is in "changes between less versions 487 and
> 530":
>
> Don't output terminal init sequence if using -F and file fits on one
> screen.
>
> So it seems like the problem has been fixed inside less for recent
> versions. And in theory we _could_ drop "-X" if it is causing problems.
> That version of less is ~5 years old. It does seem a little premature
> to
> assume everybody has it. And as you say, if there are people who really
> care about their LESS options, it is easy for them to override it.
Thanks for this detailed analysis!
It's important to keep in mind that removing "-X" and leaving "-F" would
introduce an inconsistency in the displaying of outputs, by having the
long outputs disappear after exiting the pager manually, and by having
the short outputs remain displayed after the pager exits on its own.
On the other hand, removing both "-X" and "-F" would make people even
more annoyed by requiring them to exit the pager manually even for short
outputs.
Quite frankly, "-FRX" is a just fine set of defaults.
^ permalink raw reply
* Re: why does git set X in LESS env var?
From: Dragan Simic @ 2023-10-12 0:06 UTC (permalink / raw)
To: Christoph Anton Mitterer; +Cc: Junio C Hamano, git
In-Reply-To: <fbb3c2bf1c832f0f16cb913da6b862dd313359ef.camel@scientia.org>
On 2023-10-12 01:43, Christoph Anton Mitterer wrote:
> On Thu, 2023-10-12 at 01:29 +0200, Dragan Simic wrote:
>> I'm not sure what do you mean by the non-cleared remains being
>> chopped
>> off... Could you clarify that a bit, please?
>
> Well if I do say:
> $ reset
> $ git diff HEAD~10
>
> and from there scroll down a bit and then q to exit less (and the
> screen is not cleared), I see the output only so far as I've had
> previously scrolled down in less.
There's also scrollback in the terminal, which can be used to show more
of the contents that was displayed before exiting the pager.
> Everything that would have come after that is of course not visible.
> The place where I exited may be some "well defined" border, like the
> end of a commit... or anywhere it the middle of a patch (making the
> left over remains on the terminal perhaps even ambiguous).
If you didn't select some line or page to be displayed, by scrolling
within the pager, it obviously isn't going to be displayed, which is the
whole point of using a pager instead of "spitting" the whole contents
out at once.
Where and when you exit the pager is up to you only, and you can decide
what will be left on the screen at that point.
> What's worse, when (in less) I scroll down and up again, perhaps
> repeating several times, and then quit... I see (at least in my
> less/terminal combination) things twice and mangled up (i.e. when I
> scroll up the terminal (outside of less)).
That sounds like some issue with your terminal or terminal emulator,
which should be debugged and fixed separately. Such misbehavior isn't
supposed to happen at all.
> So AFAICS, not clearing the screen only works properly when never
> scrolling up again (in less).
It works just fine for me, for example. You're obviously having some
unrelated issues with your terminal emulator.
>> As I already mentioned above, everyone is free to configure the pager
>> behavior in any way they like.
>
> Sure :-)
>
>>
>> > > Exiting if less contents than one full screen was displayed (i.e.
>> > > "-
>> > > F")
>> > > is there to save people from the frustration of quitting a pager
>> > > that
>> > > actually wasn't needed to be executed.
>> >
>> > Same actually here, at least strictly speaking, ... though I (and
>> > probably everybody else?) would really hate it, if that was
>> > removed. ^^
>>
>> I'm afraid that I don't understand very well are you complaining
>> about
>> the presence of "-F" or not?
>
> No :-) As I've said, I like it that way and I and probably everyone
> else would be annoyed, if -F was not present.
>
> I just meant that strictly speaking the same reason why "S" was
> removed, could be applied to "F" as well.
I see. Actually, removing "-S" was a good decision, IMHO, because
chopping long lines isn't something that a sane set of defaults should
do. Many users would probably be confused with the need to use the
right arrow to see long lines in their entirety.
> It is - like -R - not necessary for less to work with git.
>
> But it is, of course, what virtually everyone will want in practise.
Well, "-R" is pretty much mandatory, because coloring the outputs has
become some kind of defacto standard, and it's very useful when viewing
diffs, for example.
>> Quite frankly, I can't stand scrolling in less(1) using the mouse
>> wheel,
>> but I do understand why some people like it.
>
> The main reason I want it is, that things don't get messy, when I
> forget being in less and mouse scroll. ;-)
>
>
> Thanks,
> Chris.
^ permalink raw reply
* Re: why does git set X in LESS env var?
From: Jeff King @ 2023-10-12 0:04 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Christoph Anton Mitterer, git
In-Reply-To: <xmqqa5sokdd3.fsf@gitster.g>
On Wed, Oct 11, 2023 at 03:23:20PM -0700, Junio C Hamano wrote:
> Christoph Anton Mitterer <calestyo@scientia.org> writes:
>
> > But I still don't get from that why X would be needed?
> >
> > My less manpage documents it as:
> >> -X or --no‐init
> >> Disables sending the termcap initialization and deinitialization
> >> strings to the terminal. This is sometimes desirable if the
> >> deinitialization string does something unnecessary, like clearing
> >> the screen.
> >
> > Is it to avoid clearing the screen?
>
> I think that was the reason we added it back in 2005. In any case,
> asking "why" is not a useful use of anybody's time, because it is
> very unlikely to change in the official version we ship, and because
> it is so easy for any individual who does not like it to drop by
> exporting the $LESS environment variable.
I agree it is probably not worth changing now, but I think the history
here is a little interesting.
Yes, I think "X" was added because less would clear the screen after
exiting, and with "F" this meant you'd see nothing. Here's a thread from
the same time period discussing it:
https://lore.kernel.org/git/cc723f590610210623sbee2075i5f2fd441cceb84ae@mail.gmail.com/
But I also think this was a pretty well-known annoyance with "less" back
then.
However, I can't seem to reproduce it now! Digging into the history and
the changelog, this note is in "changes between less versions 487 and
530":
Don't output terminal init sequence if using -F and file fits on one
screen.
So it seems like the problem has been fixed inside less for recent
versions. And in theory we _could_ drop "-X" if it is causing problems.
That version of less is ~5 years old. It does seem a little premature to
assume everybody has it. And as you say, if there are people who really
care about their LESS options, it is easy for them to override it.
-Peff
^ permalink raw reply
* Re: why does git set X in LESS env var?
From: Christoph Anton Mitterer @ 2023-10-11 23:43 UTC (permalink / raw)
To: Dragan Simic; +Cc: Junio C Hamano, git
In-Reply-To: <6457310b8ca0e7d3b288a3bbbe264012@manjaro.org>
On Thu, 2023-10-12 at 01:29 +0200, Dragan Simic wrote:
> I'm not sure what do you mean by the non-cleared remains being
> chopped
> off... Could you clarify that a bit, please?
Well if I do say:
$ reset
$ git diff HEAD~10
and from there scroll down a bit and then q to exit less (and the
screen is not cleared), I see the output only so far as I've had
previously scrolled down in less.
Everything that would have come after that is of course not visible.
The place where I exited may be some "well defined" border, like the
end of a commit... or anywhere it the middle of a patch (making the
left over remains on the terminal perhaps even ambiguous).
What's worse, when (in less) I scroll down and up again, perhaps
repeating several times, and then quit... I see (at least in my
less/terminal combination) things twice and mangled up (i.e. when I
scroll up the terminal (outside of less)).
So AFAICS, not clearing the screen only works properly when never
scrolling up again (in less).
> As I already mentioned above, everyone is free to configure the pager
> behavior in any way they like.
Sure :-)
>
> > > Exiting if less contents than one full screen was displayed (i.e.
> > > "-
> > > F")
> > > is there to save people from the frustration of quitting a pager
> > > that
> > > actually wasn't needed to be executed.
> >
> > Same actually here, at least strictly speaking, ... though I (and
> > probably everybody else?) would really hate it, if that was
> > removed. ^^
>
> I'm afraid that I don't understand very well are you complaining
> about
> the presence of "-F" or not?
No :-) As I've said, I like it that way and I and probably everyone
else would be annoyed, if -F was not present.
I just meant that strictly speaking the same reason why "S" was
removed, could be applied to "F" as well.
It is - like -R - not necessary for less to work with git.
But it is, of course, what virtually everyone will want in practise.
> Quite frankly, I can't stand scrolling in less(1) using the mouse
> wheel,
> but I do understand why some people like it.
The main reason I want it is, that things don't get messy, when I
forget being in less and mouse scroll. ;-)
Thanks,
Chris.
^ permalink raw reply
* Re: gpg.ssh.defaultKeyCommand docs bug?
From: Jeff King @ 2023-10-11 23:41 UTC (permalink / raw)
To: matthew sporleder; +Cc: Fabian Stelzer, Git Mailing List
In-Reply-To: <CAHKF-AsjY_P6mbAs7KWcgL39KbLbu9OE9XiLabghhTn-f0ybzQ@mail.gmail.com>
On Wed, Oct 11, 2023 at 02:16:27PM -0400, matthew sporleder wrote:
> It gave very confusing errors!
>
> key::ssh-rsa ABC123 me@localhost (no new line)
> error: Load key "....: invalid format?
It's hard to say without seeing the whole output, but I suspect this is
actually coming from ssh, not Git. We just dump the output into a
tempfile and feed it to "ssh-keygen -f".
Though I'd think we would see the same issue with user.signingKey in
that case.
So I'm not sure what's going on here (I haven't set up ssh signing to
play with yet).
> key::ABC123 (yes new line)
> error: Couldn't load public key ...: No such file or directory?
That one makes sense to me. The "ssh-rsa" part is important, because
without it, ssh-keygen has no idea what format it is in.
> key::ssh-rsa ABC123 me@localhost (yes new line)
> works, I think
This is the recommended format.
> ssh-rsa ABC123 me@localhost (yes new line)
> works (the script I provided)
And this is the historical one.
So I don't think the documentation is _wrong_ here, but I agree that it
is a bit on the confusing side (especially understanding that "key::"
came later, and that raw "ssh-rsa" is deprecated, which is only
mentioned in user.signingKey, not gpg.ssh.defaultKeyCommand.
And I'm still not sure what's going on with your no-new-line example,
which I'd have expected to work.
-Peff
^ permalink raw reply
* Re: [RFC] Define "precious" attribute and support it in `git clean`
From: Junio C Hamano @ 2023-10-11 23:35 UTC (permalink / raw)
To: Richard Kerry; +Cc: git@vger.kernel.org
In-Reply-To: <AS8PR02MB73027943EE0A30DD8DAAD4639CCCA@AS8PR02MB7302.eurprd02.prod.outlook.com>
Richard Kerry <richard.kerry@eviden.com> writes:
> An option might be to state, in config, whether a project, or
> everything, should be managed on the basis of "all untracked files
> are precious" or "files may be explicitly marked precious", or, as
> now, "nothing is precious".
I do not think there is any need to have a separate "all or none"
option. We do not have to make things more complicated than
necessary.
If all untracked files are precious, a user should be able to say so
with an entry that matches all paths "*" to mark them precious, and
nothing more needs to be done. By default nothing is ignored and
nothing is precious, until you start marking paths with .gitignore
entries.
^ permalink raw reply
* Re: [PATCH 0/20] bounds-checks for chunk-based files
From: Jeff King @ 2023-10-11 23:31 UTC (permalink / raw)
To: Taylor Blau; +Cc: git
In-Reply-To: <ZSb1QAaLX+xcZK4a@nand.local>
On Wed, Oct 11, 2023 at 03:19:28PM -0400, Taylor Blau wrote:
> I reviewed this carefully (well, except for the new Perl script, for
> obvious[^1] reasons ;-)).
>
> Everything mostly looks good to me, though I
> had a handful of review comments throughout. Many of them are trivial
> (e.g. a number of warning() and error() strings should be marked for
> translation, etc.), but a couple of them I think are worth looking at.
Thanks for taking a look. I think it may make sense to come back on top
and adjust a few of the commit messages, along with adding a few
st_mult() overflow checks that you suggest.
> Most notably, I think that by the end of the series, I was convinced
> that having some kind of 'pair_chunk_expectsz()' or similar would be
> useful and eliminate a good chunk of the boilerplate you have to write
> to check the chunk size against an expected value when using
> read_chunk().
This I'm less convinced by. In fact, I _almost_ just dropped
pair_chunk() entirely. Adding an out-parameter for the size at least
forces the caller to consider what to do with the size. But really, I
think the right mindset is "we should be sanity-checking this chunk as
we load it". And having a callback, even if it is a little bit of
boilerplate, helps set that frame of mind.
I dunno. Maybe that is all just programmer pseudo-psychology. But I also
don't like that about half the calls to pair_chunk() can't do a size
check (so we need two functions, or to make the "expect" parameter
optional).
-Peff
^ permalink raw reply
* Re: why does git set X in LESS env var?
From: Dragan Simic @ 2023-10-11 23:29 UTC (permalink / raw)
To: Christoph Anton Mitterer; +Cc: Junio C Hamano, git
In-Reply-To: <ec43820562198de078db7df54d0338edf1f333ea.camel@scientia.org>
On 2023-10-12 01:16, Christoph Anton Mitterer wrote:
> On Thu, 2023-10-12 at 00:51 +0200, Dragan Simic wrote:
>> In general, not clearing the screen (i.e. "-X") is there so the
>> displayed contents is still visible in the terminal after exiting the
>> pager. That wouldn't be the case if the screen was cleared, making
>> it
>> less usable for most users.
>
> Well, I personally, also prefer it that way... but I'd also say that
> just like in the case of `S`, this is not really needed from the git
> side, but rather simply a user choice.
It's about providing a set of sane defaults for less(1), which other
utilities also do, including dmesg, for example. Of course, everyone
can set $PAGER or $GIT_PAGER to fit their own prereferences.
> And since, if the output did not fit one one screen, the non-cleared
> remains may likely be chopped off,... one could argue that some users
> would actually prefer to have it cleared.
I'm not sure what do you mean by the non-cleared remains being chopped
off... Could you clarify that a bit, please?
As I already mentioned above, everyone is free to configure the pager
behavior in any way they like.
>> Exiting if less contents than one full screen was displayed (i.e. "-
>> F")
>> is there to save people from the frustration of quitting a pager that
>> actually wasn't needed to be executed.
>
> Same actually here, at least strictly speaking, ... though I (and
> probably everybody else?) would really hate it, if that was removed. ^^
I'm afraid that I don't understand very well are you complaining about
the presence of "-F" or not?
> Anyway... that's no request from my side to change the default. I just
> wanted to know whether that don't-clear-the-screen part was the
> motivation for the `X`.
AFAIK, there should be no motivation other than not clearing the screen.
Other utilities that invoke the pager internally configure the default
pager options in a very similar way.
> In case someone cares, I've asked less upstream whether there's a way
> to have VTE scrolling work with -X:
> https://github.com/gwsw/less/issues/445
Quite frankly, I can't stand scrolling in less(1) using the mouse wheel,
but I do understand why some people like it.
^ permalink raw reply
* Re: [PATCH 17/20] commit-graph: check bounds when accessing BDAT chunk
From: Jeff King @ 2023-10-11 23:27 UTC (permalink / raw)
To: Taylor Blau; +Cc: git
In-Reply-To: <ZSbzf0o7r5GWAZFF@nand.local>
On Wed, Oct 11, 2023 at 03:11:59PM -0400, Taylor Blau wrote:
> > + if (check_bloom_offset(g, lex_pos, end_index) < 0 ||
> > + check_bloom_offset(g, lex_pos - 1, start_index) < 0)
>
> Can lex_pos ever be zero? I can't think of any reason that it couldn't,
> and indeed the (elided) diff context shows that immediately preceding
> this if-statement is another that checks "if (lex_pos > 0)".
>
> So I think we'd want to avoid checking lex_pos - 1 if we know that
> lex_pos is zero. Not that any of this really matters, since the only
> thing we use the computed value for is showing the graph position in the
> warning() message. So seeing a bogus value there isn't the end of the
> world.
If lex_pos is 0, then we set start_index to 0 manually, which we know to
be valid. So we can't trigger a bogus warning(). My thinking was that it
was OK to just let this fall out naturally as it does here, since it
makes the code shorter. But if we want to cover that case separately,
I think you'd want to split the checks like:
diff --git a/bloom.c b/bloom.c
index 1474aa19fa..d9ad69ad82 100644
--- a/bloom.c
+++ b/bloom.c
@@ -65,16 +65,16 @@ static int load_bloom_filter_from_graph(struct commit_graph *g,
lex_pos = graph_pos - g->num_commits_in_base;
end_index = get_be32(g->chunk_bloom_indexes + 4 * lex_pos);
+ if (check_bloom_offset(g, lex_pos, end_index) < 0)
+ return 0;
- if (lex_pos > 0)
+ if (lex_pos > 0) {
start_index = get_be32(g->chunk_bloom_indexes + 4 * (lex_pos - 1));
- else
+ if (check_bloom_offset(g, lex_pos - 1, start_index) < 0)
+ return 0;
+ } else
start_index = 0;
- if (check_bloom_offset(g, lex_pos, end_index) < 0 ||
- check_bloom_offset(g, lex_pos - 1, start_index) < 0)
- return 0;
-
if (end_index < start_index) {
warning("ignoring decreasing changed-path index offsets"
" (%"PRIuMAX" > %"PRIuMAX") for positions"
-Peff
^ permalink raw reply related
* Re: why does git set X in LESS env var?
From: Christoph Anton Mitterer @ 2023-10-11 23:16 UTC (permalink / raw)
To: Dragan Simic; +Cc: Junio C Hamano, git
In-Reply-To: <eadc03fc56d530ea31790f8a4b47a16e@manjaro.org>
On Thu, 2023-10-12 at 00:51 +0200, Dragan Simic wrote:
> In general, not clearing the screen (i.e. "-X") is there so the
> displayed contents is still visible in the terminal after exiting the
> pager. That wouldn't be the case if the screen was cleared, making
> it
> less usable for most users.
Well, I personally, also prefer it that way... but I'd also say that
just like in the case of `S`, this is not really needed from the git
side, but rather simply a user choice.
And since, if the output did not fit one one screen, the non-cleared
remains may likely be chopped off,... one could argue that some users
would actually prefer to have it cleared.
> Exiting if less contents than one full screen was displayed (i.e. "-
> F")
> is there to save people from the frustration of quitting a pager that
> actually wasn't needed to be executed.
Same actually here, at least strictly speaking, ... though I (and
probably everybody else?) would really hate it, if that was removed. ^^
Anyway... that's no request from my side to change the default. I just
wanted to know whether that don't-clear-the-screen part was the
motivation for the `X`.
In case someone cares, I've asked less upstream whether there's a way
to have VTE scrolling work with -X:
https://github.com/gwsw/less/issues/445
Thanks,
Chris.
^ 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