* Re: Breaking change with "git log -n" since 2.43
From: Maarten Ackermans @ 2024-02-21 15:07 UTC (permalink / raw)
To: Sean Allred; +Cc: Kristoffer Haugsbakk, git
In-Reply-To: <m04je1dhdx.fsf@epic96565.epic.com>
I would suggest displaying a warning in case of invalid input (such as
this out of range error), and to fall back to output all as if the
"-n" flag was unspecified. If more strict handling is still desired,
it could instead be a deprecation warning with a grace period, giving
applications some time to update their git usage.
On Wed, Feb 21, 2024 at 9:25 PM Sean Allred <allred.sean@gmail.com> wrote:
>
>
> Maarten Ackermans <maarten.ackermans@gmail.com> writes:
> > Applications that have been relying on undocumented features and
> > limits since they were introduced, now face a hard crash: "fatal:
> > '9007199254740991': not an integer". Regardless of whether this is an
> > improvement for future implementations, a crash in existing ones is a
> > suboptimal experience at the least.
>
> What behavior would you propose instead?
>
> --
> Sean Allred
^ permalink raw reply
* Re: Breaking change with "git log -n" since 2.43
From: Sean Allred @ 2024-02-21 15:17 UTC (permalink / raw)
To: Maarten Ackermans; +Cc: Kristoffer Haugsbakk, git
In-Reply-To: <CAB=tB2uZb+8QLmrk_tK5PKJtDE=RmBr=eBBb7U7ygSmkFoXvWg@mail.gmail.com>
Maarten Ackermans <maarten.ackermans@gmail.com> writes:
> I would suggest displaying a warning in case of invalid input (such as
> this out of range error), and to fall back to output all as if the
> "-n" flag was unspecified.
Was this the prior behavior? It sounds like from the commit you
referenced, atoi() simply would've stopped parsing after a point and
you'd end up with a (large, but finite) value for `-n`. I'm definitely
reading between the lines here, though, and I must admit I've never
provided such bogus input to git-log myself.
--
Sean Allred
^ permalink raw reply
* Re: Breaking change with "git log -n" since 2.43
From: Kristoffer Haugsbakk @ 2024-02-21 15:32 UTC (permalink / raw)
To: Maarten Ackermans; +Cc: git, Sean Allred
In-Reply-To: <CAB=tB2uZb+8QLmrk_tK5PKJtDE=RmBr=eBBb7U7ygSmkFoXvWg@mail.gmail.com>
On Wed, Feb 21, 2024, at 16:07, Maarten Ackermans wrote:
> I would suggest displaying a warning in case of invalid input (such as
> this out of range error), and to fall back to output all as if the
> "-n" flag was unspecified. If more strict handling is still desired,
> it could instead be a deprecation warning with a grace period, giving
> applications some time to update their git usage.
From 71a1e9482:
“ As a natural consequence, an argument that does not begin with a
digit (e.g., "q") silently becomes zero, too.
It sounds like the non-breaking behavior for non-number input like `q`
is to silently become `0`.[1] But then that too-large number input would
also become `0`, which doesn’t help for that JavaScript
application/library. Unless `strtol_i` is able to differentiate between
different errors by returning different negative numbers?
† 1: Or else you risk breaking usages where they rely on bad input
becoming `0`
--
Kristoffer Haugsbakk
^ permalink raw reply
* Re: Breaking change with "git log -n" since 2.43
From: Kristoffer Haugsbakk @ 2024-02-21 15:38 UTC (permalink / raw)
To: Maarten Ackermans; +Cc: git, Sean Allred
In-Reply-To: <CAB=tB2tgbLjBPvgBQDoNJi7e8+LMzxHSbg6D2jKUSJXPmQFrxA@mail.gmail.com>
On Wed, Feb 21, 2024, at 15:21, Maarten Ackermans wrote:
> Whether or not that is Git's definition of a breaking change, the
> message of the commit in question acknowledges that the commands in
> the "log" family are the oldest in the system:
I was assuming Semantic Versioning (SV). Not because that’s the policy
in Git (?) but because this problem happened in a JS app./library. And
SV as far as I understand it define _breaking changes_ relative to the
“public API”. And for git-log(1) the public API is the documentation,
right?
(But maybe I’ve just never managed to understand SV.)
I also see now that this is some SV app./library.
--
Kristoffer Haugsbakk
^ permalink raw reply
* Re: Breaking change with "git log -n" since 2.43
From: Sean Allred @ 2024-02-21 15:34 UTC (permalink / raw)
To: Maarten Ackermans; +Cc: Kristoffer Haugsbakk, git
In-Reply-To: <CAB=tB2vKj45yr3amMbhv_dYBdqYOtoiMS7Ecx4WO1TE2STHEsA@mail.gmail.com>
Maarten Ackermans <maarten.ackermans@gmail.com> writes:
> I think you’re right, so reinstating the original behavior with a
> deprecation warning would be more prudent.
>
> After the grace period, if invalid input is given, fall back to output
> all with a warning. Or you can go the strict route again and crash
> with a fatal error (hard to imagine an actual use case for such a
> large, specific number, anyway).
To be clear, I'm not advocating we go back to the prior behavior. I'm
just clarifying what your proposal would mean.
I don't pretend to have any nuanced understanding of how the Git project
handles situations like this, but IMHO, I would have to fall back to the
principle of least surprise. Rejecting invalid input is not a crash --
it is a useful guardrail to ensure the user and the computer agree on
what's been requested / what's going to happen.
Again, I don't have any experience with how similar situations are
handled here, but I would contend that the behavior you describe as the
breaking behavior is the behavior that should exist long-term.
--
Sean Allred
^ permalink raw reply
* Re: What's cooking in git.git (Feb 2024, #07; Tue, 20)
From: Junio C Hamano @ 2024-02-21 16:44 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git
In-Reply-To: <CAOLa=ZQOtDgyWxmV+C9r_wfqo-KdJ5uqW4KwJKD8WdT9qM0t2Q@mail.gmail.com>
Karthik Nayak <karthik.188@gmail.com> writes:
>> * kn/for-all-refs (2024-02-12) 6 commits
>> - for-each-ref: add new option to include root refs
>> - ref-filter: rename 'FILTER_REFS_ALL' to 'FILTER_REFS_REGULAR'
>> - refs: introduce `refs_for_each_include_root_refs()`
>> - refs: extract out `loose_fill_ref_dir_regular_file()`
>> - refs: introduce `is_pseudoref()` and `is_headref()`
>> - Merge branch 'ps/reftable-backend' into kn/for-all-refs
>> (this branch uses ps/reftable-backend.)
>>
>> "git for-each-ref" filters its output with prefixes given from the
>> command line, but it did not honor an empty string to mean "pass
>> everything", which has been corrected.
>>
>> Will merge to 'next'?
>> source: <20240211183923.131278-1-karthik.188@gmail.com>
>
> Let me know if there's something more I could do here.
Hmph, I do not recall what the plan for this thing was. Wasn't it
ejected from 'next' so that its UI can be rethought? Or did that
happen already and what is in 'seen' already with a new UI?
Thanks.
^ permalink raw reply
* Re: [PATCH 1/2] object-name: detect and report empty reflogs
From: Eric Sunshine @ 2024-02-21 16:48 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Yasushi SHOJI
In-Reply-To: <0fac6ebb098c7e8cdc87cb75f2dcffdc4b1ccfaa.1708509190.git.ps@pks.im>
On Wed, Feb 21, 2024 at 5:03 AM Patrick Steinhardt <ps@pks.im> wrote:
> [...]
> Start to detect and report empty or missing reflogs in `read_ref_at()`
> and report them to the caller. This results in a change in behaviour
> when asking for `ref@{0}` with an empty or missing reflog because we now
> die instead of returning the object ID of the ref itself. This adapted
> behaviour should lead to less surprises as we now really only report
> object IDs to the caller that actually come from the reflog, thus making
> the user experience a whole lot more consistent.
> [...]
> Reported-by: Yasushi SHOJI <yasushi.shoji@gmail.com>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> diff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh
> @@ -110,10 +110,31 @@ test_expect_success '@{1} works with only one reflog entry' '
> +test_expect_success '@{0} fails with empty reflog' '
> + git checkout -B empty-reflog main &&
> + git reflog expire --expire=now refs/heads/empty-reflog &&
> + cat >expect <<-EOF &&
> + fatal: Needed a single revision
> + EOF
Typically, we would use <<-\EOF rather than <<-EOF to signal to the
reader that no variable interpolation is happening in the here-doc
body.
A simpler alternative in this case would, of course, be to use `echo`:
echo "fatal: Needed a single revision" >expect &&
> + test_must_fail git rev-parse --verify missing-reflog@{0} 2>err &&
> + test_cmp expect err
> +'
> +
> +test_expect_success '@{0} fails with missing reflog' '
> + git -c core.logAllRefUpdates=false checkout -B missing-reflog main &&
> + cat >expect <<-EOF &&
> + fatal: Needed a single revision
> + EOF
Ditto.
> + test_must_fail git rev-parse --verify missing-reflog@{0} 2>err &&
> + test_cmp expect err
> +'
Probably neither comment is worth a reroll, but if you reroll for some
other reason, perhaps consider them.
^ permalink raw reply
* Re: Bug: impure renames may be reported as pure renames
From: Junio C Hamano @ 2024-02-21 17:11 UTC (permalink / raw)
To: Elijah Newren; +Cc: Mário Guimarães, git
In-Reply-To: <CABPp-BG2m3_fyxFL6Hw4o5HhoKVd7_tYchDxCdTaXxdxZke-9g@mail.gmail.com>
Elijah Newren <newren@gmail.com> writes:
> Heh, good point. And more generally, due to how the similarity checks
> work (split the file into lines/chunks, hash each to an integer, then
> sort the list of integers and compare the list of integers between two
> files), whenever you keep all the original lines of a file but permute
> their order, you will see a 100% match.
Stepping back a bit, there internally is a distinction between
"identical files" and "similar but not identical files". The
file with permuted lines are not identical but are very similar.
The decision not to surface these two to end-users was made long
ago, and after finding identical renames, we skip similarity
computation when minimum_score is set to -M100 from the command
line, which is a documented UI that cannot change.
So you are right to say that similarity index of an inexact rename
should be capped at 99 and never reach 100. I do not know offhand
if your "how about" computation ...
> static int similarity_index(struct diff_filepair *p)
> {
> - return p->score * 100 / MAX_SCORE;
> + return p->score * 100 / (MAX_SCORE+1);
> }
... is the way to go, though. We have filepair, so shouldn't we be
doing more like
if (identical_rename(p))
return 100;
else
return 99 * p->score / MAX_SCORE;
instead?
It will also keep working when a minor pet peeve of mine is fixed.
If you have a file with 1000 lines, all hashing to distinct values,
and if you change only one line, even though the similarity is
computed as 99% by the current logic, it actually is 99.9% that is
much closer to 100%. But we always round down, which feels wrong.
Alternatively, we could cap p->score to (MAX_SCORE-1) when
similarity is _computed_ for filepair between non-identical blobs.
That should happen in diffcore-rename.c:estimate_similarity().
It would allow us to still _show_ "1 line changed among 1000 lines"
case as R100 and still reject inexact renames via "-M100" from the
command line, I think, as the "exact renames only" short-cut works
with the finer-grained "score" values and not the coarser "percent"
values, which might give us a better approach.
^ permalink raw reply
* Re: [PATCH 1/2] object-name: detect and report empty reflogs
From: Jeff King @ 2024-02-21 17:31 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Yasushi SHOJI
In-Reply-To: <0fac6ebb098c7e8cdc87cb75f2dcffdc4b1ccfaa.1708509190.git.ps@pks.im>
On Wed, Feb 21, 2024 at 10:56:36AM +0100, Patrick Steinhardt wrote:
> This behaviour goes back to 6436a20284 (refs: allow @{n} to work with
> n-sized reflog, 2021-01-07), which fixed a bug that wouldn't allow a
> user to return the n'th reflog entry with an n-sized reflog. With this
> commit, `read_ref_at()` started to special case reading the first entry
> of the reflog via a separate `read_ref_at_ent_newest()` function. The
> problem here is that we forgot to check whether the callback was invoked
> at all, and thus we don't notice empty reflogs.
I'm on the fence no whether the current @{0} behavior is sensible and
should be preserved. I agree it mostly worked by luck, but the presence
of the test makes me think at least the intention was there.
But assuming that is a good direction, there's one thing that puzzles me
about your patch:
> @@ -1084,6 +1084,7 @@ static int read_ref_at_ent_newest(struct object_id *ooid UNUSED,
> struct read_ref_at_cb *cb = cb_data;
>
> set_read_ref_cutoffs(cb, timestamp, tz, message);
> + cb->found_it = 1;
> oidcpy(cb->oid, noid);
> /* We just want the first entry */
> return 1;
OK, so we note whether the callback was invoked, which is good...
> @@ -1123,7 +1124,7 @@ int read_ref_at(struct ref_store *refs, const char *refname,
>
> if (cb.cnt == 0) {
> refs_for_each_reflog_ent_reverse(refs, refname, read_ref_at_ent_newest, &cb);
> - return 0;
> + return !cb.found_it;
> }
...but here we just return without an error message. Whereas later in
the function, we have logic to produce the "log for %s is empty"
message. So now we will produce a message if you ask for branch@{1} in
an empty reflog, but not for branch@{0}, and the caller is responsible
for printing an error in the latter case.
If we instead set reccnt for branch@{0} as we would for branch@{1}, then
we can fall through and share the error handling (like it was before
6436a20284, when they used the same callback):
diff --git a/refs.c b/refs.c
index c633abf284..7d5e7a9ba6 100644
--- a/refs.c
+++ b/refs.c
@@ -1084,6 +1084,8 @@ static int read_ref_at_ent_newest(struct object_id *ooid UNUSED,
struct read_ref_at_cb *cb = cb_data;
set_read_ref_cutoffs(cb, timestamp, tz, message);
+ cb->reccnt++;
+ cb->found_it = 1;
oidcpy(cb->oid, noid);
/* We just want the first entry */
return 1;
@@ -1121,12 +1123,10 @@ int read_ref_at(struct ref_store *refs, const char *refname,
cb.cutoff_cnt = cutoff_cnt;
cb.oid = oid;
- if (cb.cnt == 0) {
+ if (cb.cnt == 0)
refs_for_each_reflog_ent_reverse(refs, refname, read_ref_at_ent_newest, &cb);
- return 0;
- }
-
- refs_for_each_reflog_ent_reverse(refs, refname, read_ref_at_ent, &cb);
+ else
+ refs_for_each_reflog_ent_reverse(refs, refname, read_ref_at_ent, &cb);
if (!cb.reccnt) {
if (flags & GET_OID_QUIETLY)
And it all just works without having to touch get_oid_basic() or
cmd_show_branch() at all. Do note that one of the tests needs to be
updated to account for the slightly different format of the error
message; but again, I think that is showing off the inconsistency in
having the error message in two places.
-Peff
^ permalink raw reply related
* Re: [PATCH 1/3] doc: git-rev-parse: enforce command-line description syntax
From: Junio C Hamano @ 2024-02-21 17:31 UTC (permalink / raw)
To: Jean-Noël Avila; +Cc: Jean-Noël Avila via GitGitGadget, git
In-Reply-To: <67dca173-3048-430b-88a1-d3b5d853f84b@free.fr>
Jean-Noël Avila <jn.avila@free.fr> writes:
>>> ---short[=length]::
>>> +--short[=<length>]::
>>> Same as `--verify` but shortens the object name to a unique
>>> prefix with at least `length` characters. The minimum length
>> This same comment applies throughout this patch, but in other places
>> when we use <placeholder> in the option argument description, don't
>> we use the same <placeholder> in text as well? I am wondering if
>> the `length` (typeset in fixed-width) should become <length>. What
>> do other recent[*] documentation pages commonly do?
>
> This is another part of the inconsistences in documentation that I'd
> like to tackle (hopefully, not in another life).
>
> Using angle brackets for placeholders everywhere they appear is a
> visual link to the preceding syntax description, but may feel a bit
> heavy on some cases. Anyway, I'm all for applying the rule everywhere,
> for the sake of consistency.
I agree that if <placeholder> is not an appropriate way to spell
them in the explanation text, we would want to change them
consistently everywhere, and until then, using the angle-bracketted
<placeholder> that is common would be better. The text will be
modified again when we decide to switch from <placeholder> to
something else, so updating them now may be a wasted effort, but (1)
we may decide that <placeholder> is good enough after all, or (2) it
may make it easier to mechanically identify words whose mark-up
should be converted if we consistently use <placeholder> now, even
if we know it won't be the final mark-up.
So I am inclined to say that we should first do `length` -> <length>
in the body text in the short term. But I also think we should
*not* do so as part of this patch, whose focus is how the option
enumeration header should mark up the option arguments.
> Backticks and single quotes are used indistinctively (by the way,
> asciidoctor does not process single quotes as markup) and are not used
> everywhere they should. Using backticks is also a good hint for
> translators to mean "verbatim, do not translate". Obviously, the
> placeholders ask for translation, so the backtick rule should not
> apply to them, even if another formating would be welcome :
> _<placeholder>_ for instance?
Yes. The way AsciiDoc renders (at least HTML) an unadorned <placeholder>
is not so great.
In "git-add.html" manual page, we see these examples. The first one
(unadorned) does not make the placeholder word stand out enough; the
second one that does `<file>` makes it stand out better, but as you
said, the `verbatim` mark-up is semantically wrong.
https://git.github.io/htmldocs/git-add.html#:~:text=For%20more%20details%20about%20the%20%3Cpathspec%3E%20syntax
https://git.github.io/htmldocs/git-add.html#:~:text=Pathspec%20is%20passed%20in%20%3Cfile%3E%20instead%20of%20commandline%20args.%20If%20%3Cfile%3E%20is%20exactly%20%2D%20then%20standard%20input%20is%20used.%20Pathspec
The last part of the Documentation/CodingGuidelines document talks
about how to mark up placeholders but it does not go beyond saying
that they are written as <hyphen-in-between-words-in-angle-braket>.
Whatever mark-up rule we decide to use, we should document it there.
Thanks.
^ permalink raw reply
* Re: [PATCH] rebase: make warning less passive aggressive
From: Junio C Hamano @ 2024-02-21 17:31 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Harmen Stoppels via GitGitGadget, git, Harmen Stoppels
In-Reply-To: <ZdWiyYhKAB3_P6_k@tanuki>
Patrick Steinhardt <ps@pks.im> writes:
>> if (options.action != ACTION_NONE && !in_progress)
>> - die(_("No rebase in progress?"));
>> + die(_("No rebase in progress"));
>
> While we're at it changing this message, do we also want to convert it
> to start with a lower-case letter so that it aligns with our error
> message style?
Good eyes. Thanks for a good suggestion.
^ permalink raw reply
* Re: [PATCH 2/2] builtin/show-branch: detect empty reflogs
From: Jeff King @ 2024-02-21 17:35 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Yasushi SHOJI
In-Reply-To: <6107efeffa3a9e87ed95f4fe592d9b480887c510.1708509190.git.ps@pks.im>
On Wed, Feb 21, 2024 at 10:56:40AM +0100, Patrick Steinhardt wrote:
> diff --git a/builtin/show-branch.c b/builtin/show-branch.c
> index b01ec761d2..8837415031 100644
> --- a/builtin/show-branch.c
> +++ b/builtin/show-branch.c
> @@ -785,6 +785,8 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
> if (read_ref_at(get_main_ref_store(the_repository),
> ref, flags, 0, base + i, &oid, &logmsg,
> ×tamp, &tz, NULL)) {
> + if (!i)
> + die(_("log for %s is empty"), ref);
> reflog = i;
> break;
> }
There's another call to read_ref_at() earlier in the function that does
not check the return code. I think it is OK, though, because it always
passes a timestamp rather than a count, so it never hits the
special-case added by 6436a20284.
But I think this highlights my confusion with the first patch; most of
the time read_ref_at() will print the appropriate message for us and
die, but for this one weird case we have to do it ourselves. It makes
more sense to me for it to behave consistently for an empty reflog,
whether we were looking for branch@{0} or branch@{1}.
-Peff
^ permalink raw reply
* [PATCH v2] rebase: make warning less passive aggressive
From: Harmen Stoppels via GitGitGadget @ 2024-02-21 17:38 UTC (permalink / raw)
To: git; +Cc: Harmen Stoppels, Harmen Stoppels
In-Reply-To: <pull.1669.git.1708442603395.gitgitgadget@gmail.com>
From: Harmen Stoppels <me@harmenstoppels.nl>
When you run `git rebase --continue` when no rebase is in progress, git
outputs `fatal: No rebase in progress?` which is not a question but a
statement. Make it appear as a statement, and use lowercase to align
with error message style.
Signed-off-by: Harmen Stoppels <me@harmenstoppels.nl>
---
rebase: make warning less passive aggressive
cc: Patrick Steinhardt ps@pks.im
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1669%2Fhaampie%2Ffix%2Fpassive-agressive-message-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1669/haampie/fix/passive-agressive-message-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1669
Range-diff vs v1:
1: 4dfc578da84 < -: ----------- rebase: make warning less passive aggressive
-: ----------- > 1: a6f7a3a2477 rebase: make warning less passive aggressive
builtin/rebase.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 5b086f651a6..6ead9465a42 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1254,7 +1254,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
die(_("options '%s' and '%s' cannot be used together"), "--root", "--fork-point");
if (options.action != ACTION_NONE && !in_progress)
- die(_("No rebase in progress?"));
+ die(_("no rebase in progress"));
if (options.action == ACTION_EDIT_TODO && !is_merge(&options))
die(_("The --edit-todo action can only be used during "
base-commit: 96c8a0712e569dd2812bf4fb5e72113caf326500
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH] rebase: make warning less passive aggressive
From: Michal Suchánek @ 2024-02-21 17:38 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Harmen Stoppels via GitGitGadget, git, Harmen Stoppels
In-Reply-To: <xmqqv86jqc2e.fsf@gitster.g>
On Tue, Feb 20, 2024 at 09:29:13AM -0800, Junio C Hamano wrote:
> "Harmen Stoppels via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Harmen Stoppels <me@harmenstoppels.nl>
> >
> > When you run `git rebase --continue` when no rebase is in progress, git
> > outputs `fatal: no rebase in progress?` which is not a question but a
> > statement. This commit makes it appear as a statement.
>
> "This commit makes it appear" -> "Make it appear" (see
> SubmittingPatches).
>
> > builtin/rebase.c | 2 +-
>
> This change is very good, but a commit that touches code should not
> touch po/ localizations in this project. They are updated to match
> the code change by respective language teams.
Or if it does touch the po files it could as well update the
translations.
There are changes that can be trivially translated without any knowledge
of the target language.
Thanks
Michal
^ permalink raw reply
* Re: Segfault: git show-branch --reflog refs/pullreqs/1
From: Jeff King @ 2024-02-21 17:38 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: Yasushi SHOJI, Denton Liu, Junio C Hamano, Git Mailing List
In-Reply-To: <ZdXLBIv1vLjhQUgx@tanuki>
On Wed, Feb 21, 2024 at 11:05:56AM +0100, Patrick Steinhardt wrote:
> > I am still trying to wrap my head around how it can get such wrong
> > results for show-branch when asking for "git rev-parse branch@{0}", etc,
> > are correct. I think it is that "rev-parse branch@{0}" is only looking
> > at the output oid and does not consider the reflog message at all. So I
> > think it is subtly broken, but in a way that happens to work for that
> > caller. But I'm not sure of the correct fix. At least not at this time
> > of night.
> >
> > Cc-ing folks involved in 6436a20284.
>
> Ah, our mails crossed, but we came to the same conclusion. Things indeed
> are subtly broken here and work just by chance because all callers pre
> initialize the object ID. So in the case where the reflog is missing or
> empty we'd use that pre-initialized object ID because `read_ref_at()`
> does not indicate the failure to the callers.
>
> I think that this behaviour is not sensible in the first place. When
> asking for the reflog, we should only ever return object IDs parsed from
> the reflog. Falling back to parsing the ref itself does not make much
> sense. I've thus sent a patch series that changes the behaviour here.
I left some comments on your patches. But assuming we are OK to change
the branch@{0} behavior for the empty log, the approach is sound.
That still leaves us with the bug in showing the message (which is
easily fixed), and the weird off-by-one output caused by 6436a20284.
Obviously the segfault fix can be taken independently of the rest, but I
wonder if some deeper refactoring of what 6436a20284 did will be
necessary.
-Peff
^ permalink raw reply
* Re: Segfault: git show-branch --reflog refs/pullreqs/1
From: Junio C Hamano @ 2024-02-21 17:44 UTC (permalink / raw)
To: Jeff King; +Cc: Yasushi SHOJI, Denton Liu, Git Mailing List
In-Reply-To: <20240221084250.GA25385@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> with an empty reflog file (added by that same commit). The code in
> get_oid_basic() to parse reflogs doesn't suffer from the same bugs: it
> checks up front that the reflog file exists, it preloads the output oid
> with the current ref value, and it doesn't look at other fields (like
> the reflog message).
It is a usability hack to allow foo@{0} to resolve successfully,
instead of causing a failure, when there is no reflog entries for
foo, I would think. As to the "show-branch -g", the intent is to
see the recent evolution of the branch, so I am fine if we do not
give any output when no reflog entries exist (i.e. "no evolution
behind the current state---it just is"), or just one entry output
for "foo@{0}" to say "there is only one recent state".
Even though it may feel wrong to successfully resolve foo@{0} when
reflog for foo does not exist at the mechanical level (read: the
implementors of reflog mechanism may find the usability hack a bad
idea), I suspect at the end-user level it may be closer to what
people expect out of foo@{0} (i.e. "give me the latest").
Thanks.
^ permalink raw reply
* Re: Breaking change with "git log -n" since 2.43
From: Jeff King @ 2024-02-21 17:51 UTC (permalink / raw)
To: Maarten Ackermans; +Cc: git
In-Reply-To: <CAB=tB2vB0LbP=DznSqTFYHCRxDxd6U=Q+P33yeBzGssq2eK1vA@mail.gmail.com>
On Wed, Feb 21, 2024 at 08:32:46PM +0700, Maarten Ackermans wrote:
> To reproduce, the command `git log -n 9007199254740991` fails on
> 2.43.2, whereas it didn't on 2.42.0. This specific number corresponds
> to the Number.MAX_SAFE_INTEGER (2^53 - 1) in JavaScript (docs:
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/MAX_SAFE_INTEGER).
> The max value that is supported now is a signed 32-bit integer (2^31 -
> 1).
>
> I suppose git simply ignored the extra digits of the number, as the
> commit message describes.
The max value was always a signed 32-bit integer. The extra digits
weren't ignored, but rather there was integer truncation at the C level.
I believe that is technically implementation defined by the compiler,
though in practice your value would generally become -1.
But passing, say, 9007199254740993 would give quite unexpected results
(the truncated value is "1" and we'd show only a single commit).
So I'm sympathetic that your specific number used to work and now
doesn't, but it feels like going back to the truncating behavior is a
step in the wrong direction.
If the goal is to have no limit at all, then passing an explicit "-1"
works, though I don't think that's a documented outcome. I do suspect
that we _would_ try to keep that historical behavior, as there is no
other way to cancel a previous "-n" or otherwise say "no limit". It
might be worth formalizing that with documentation and a test.
-Peff
^ permalink raw reply
* Re: [PATCH] rebase: make warning less passive aggressive
From: Junio C Hamano @ 2024-02-21 17:56 UTC (permalink / raw)
To: Michal Suchánek
Cc: Harmen Stoppels via GitGitGadget, git, Harmen Stoppels
In-Reply-To: <20240221173822.GA9696@kitsune.suse.cz>
Michal Suchánek <msuchanek@suse.de> writes:
> Or if it does touch the po files it could as well update the
> translations.
>
> There are changes that can be trivially translated without any knowledge
> of the target language.
You need to know the target language before deciding it is OK and
sufficient to just remove ̉¿? from around a sentence to make it from
a question to a statement of a fact. You may know enough, but
reviewers may not be. In addition, it would allow bypassing the
language teams, which is the most serious problem I see in such an
attitude.
^ permalink raw reply
* Re: [PATCH] rebase: make warning less passive aggressive
From: Junio C Hamano @ 2024-02-21 18:02 UTC (permalink / raw)
To: Michal Suchánek
Cc: Harmen Stoppels via GitGitGadget, git, Harmen Stoppels
In-Reply-To: <xmqqr0h5og57.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Michal Suchánek <msuchanek@suse.de> writes:
>
>> Or if it does touch the po files it could as well update the
>> translations.
>>
>> There are changes that can be trivially translated without any knowledge
>> of the target language.
>
> You need to know the target language before deciding it is OK and
> sufficient to just remove ̉¿? from around a sentence to make it from
> a question to a statement of a fact. You may know enough, but
> reviewers may not be. In addition, it would allow bypassing the
> language teams, which is the most serious problem I see in such an
> attitude.
Oops, "attitude" -> "approach".
^ permalink raw reply
* Re: [PATCH 0/5] promise: introduce promises to track success or error
From: Jeff King @ 2024-02-21 18:03 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: phillip.wood, Philip Peterson via GitGitGadget, git,
Johannes Schindelin, Emily Shaffer, Philip Peterson
In-Reply-To: <ZdSYxF3Hd6Zqt3Wd@tanuki>
On Tue, Feb 20, 2024 at 01:19:16PM +0100, Patrick Steinhardt wrote:
> While we're already at it throwing ideas around, I also have to wonder
> whether this would be a long-term solution towards computer-friendly
> errors. One of the problems we quite frequently hit in Gitaly is that we
> are forced to parse error messages in order to figure out why exactly
> something has failed. Needless to say, this is quite fragile and also
> feels very wrong.
>
> Now if we had a more structured way to pass errors around this might
> also enable us to convey more meaning to the caller of Git commands. In
> a hypothetical world where all errors were using an explicit error type,
> then this error type could eventually become richer and contain more
> information that is relevant to the calling script. And if such rich
> error information was available, then it would not be that far fetched
> to ask Git to emit errors in a computer-parsable format like for example
> JSON.
I think what I'm proposing (and if I understand correctly what Phillip
was thinking) is somewhat orthogonal. I agree that structured errors are
nice for computers to read. But it does open up a pretty big can of
worms regarding classifying each error, especially as root causes are
often multi-level.
For example, imagine that the caller asks to resolve a ref. We might get
a syscall error opening the loose ref. Or we might get one opening the
packed-refs file (in a reftable world, you might imagine errors opening
various other files). What is the structured error? Obviously it is "we
can't resolve that ref" at some level. But the caller might want to know
about the failed open (whether it is just ENOENT, or if we ran into
EPERM or even EIO).
Or looking at higher levels; if I ask for the merge base between A and
B, but one of those can't be resolved, how do we communicate that error?
It is some sort of "cannot resolve" error, but it needs to be
parameterized to know which is which.
All of those questions can be answered, of course, but now we are
developing a micro-format that lets us describe all errors in a
standardized way. And I think that is going to put a burden on the code
which is generating the errors (and potentially on the consumers, too,
if they have to decipher the structure to figure out what they want).
Whereas what I was really advocating for is punting on the structured
thing entirely. We keep our unstructured string errors for the most
part, but we simply let the caller pass in a context that tells us what
to do with them. That lets us keep providing specific error messages
from low-level functions without printing to stderr or exiting, which
higher-level code (especially lib-ified code) would not want.
I think it could also be the first building block for making more
structured errors (since those low-level callers are free to provide
lots of details), but it doesn't have to be.
-Peff
^ permalink raw reply
* Re: [PATCH 3/4] completion: reflog with implicit "show"
From: Rubén Justo @ 2024-02-21 18:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List
In-Reply-To: <xmqqwmqyr3mb.fsf@gitster.g>
On Tue, Feb 20, 2024 at 17:46:20 -0800, Junio C Hamano wrote:
> $ git reflog sho<TAB>
> shot show
>
> That is what I would expect.
Thank you for responding.
Of course that's the logical expectation.
However I'm not sure if it is sensible to offer completion for
sub-commands mixed with branch names.
Furthermore, I am also worried that such an approach implies making the
user pay, probably unnecessarily many times, for __git_complete_refs in
cases such as:
$ git reflog <TAB><TAB> ;# the user may be just exploring the sub-commands
$ git reflog s<TAB> ;# the user may be lazy and just want "show "
$ git reflog show<TAB> ;# likewise, to complete the SP
$ git reflog expir<TAB> ;# how often a expir... branch is expected here?
The experienced user, if not most users, should be intuitive enough to
circumvent the corner cases:
$ git reflog <TAB><TAB>
delete expire show
...
$ git reflog s<TAB>
...
$ git reflog show s<TAB>
...
$ git reflog show shot
This is why I choose to fallback to __git_complete_ref only when no
other option is available.
If you think, or anyone else, that these concerns don't make sense, I'm
open to make it work as you described.
^ permalink raw reply
* Re: [PATCH] git-remote.txt: fix typo
From: Junio C Hamano @ 2024-02-21 18:18 UTC (permalink / raw)
To: Jakub Wilk; +Cc: git, Abhradeep Chakraborty
In-Reply-To: <20240221083554.5255-1-jwilk@jwilk.net>
Jakub Wilk <jwilk@jwilk.net> writes:
> Signed-off-by: Jakub Wilk <jwilk@jwilk.net>
> ---
> Documentation/git-remote.txt | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
> index 1dec314834..932a5c3ea4 100644
> --- a/Documentation/git-remote.txt
> +++ b/Documentation/git-remote.txt
> @@ -35,7 +35,7 @@ OPTIONS
> -v::
> --verbose::
> Be a little more verbose and show remote url after name.
> - For promisor remotes, also show which filter (`blob:none` etc.)
> + For promisor remotes, also show which filters (`blob:none` etc.)
> are configured.
> NOTE: This must be placed between `remote` and subcommand.
When you give more than one filters to "git clone", they are used to
create a single "combined" filter, so strictly speaking, "also show
which filter is configured" is a grammatically valid that might be
more technically correct. E.g. the user may see
$ git clone --filter=blob:none --filter=tree:2 $there sample-repo
$ git -C sample-repo remote -v show
origin ... (fetch) [combine:blob:none+tree:2]
origin ... (push)
in the output. It may not be a bad idea to update the code to show
the filter information in a way that is more clear that we are
seeing a list of filters (perhaps showing the above as "[blob:none
tree:2]" might be a good starting point, as a totally separate topic
(#leftoverbits).
After taking your patch as-is, that is. In user's mind, they
consider they gave two filters, and making a single combined filter
is something Git did without being asked for its own convenience,
so "which filters are configured" is, while it may be technically
less correct, much closer to what the end-user perceives.
Thanks.
^ permalink raw reply
* Re: [PATCH] rebase: make warning less passive aggressive
From: Michal Suchánek @ 2024-02-21 18:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Harmen Stoppels via GitGitGadget, git, Harmen Stoppels
In-Reply-To: <xmqqjzmxofvn.fsf@gitster.g>
On Wed, Feb 21, 2024 at 10:02:04AM -0800, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Michal Suchánek <msuchanek@suse.de> writes:
> >
> >> Or if it does touch the po files it could as well update the
> >> translations.
> >>
> >> There are changes that can be trivially translated without any knowledge
> >> of the target language.
> >
> > You need to know the target language before deciding it is OK and
> > sufficient to just remove ̉¿? from around a sentence to make it from
> > a question to a statement of a fact. You may know enough, but
> > reviewers may not be. In addition, it would allow bypassing the
> > language teams, which is the most serious problem I see in such an
> > attitude.
>
> Oops, "attitude" -> "approach".
That's certainly true that even if you know how to remove a question
mark you may not find a reviewer who knows how to remove one.
Nonetheless, if you do find both a patch author and a reviewer that know
how to remove a question mark what is the problem with not involving
several language teams to remove a question mark?
Thanks
Michal
^ permalink raw reply
* Re: [PATCH] builtin/stash: configs keepIndex, includeUntracked
From: Ricardo C @ 2024-02-21 19:13 UTC (permalink / raw)
To: Junio C Hamano, Phillip Wood; +Cc: phillip.wood, git
In-Reply-To: <xmqqjzmzqb85.fsf@gitster.g>
On 2/20/24 12:47, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
>
>> That's a good idea but I think it would be better to test that "git
>> stash create" is not affected by the config as we don't want to change
>> the behavior of its behavior.
>
> Yup. Making sure "stash create" stays the same is a very good
> thing. Thanks for suggesting it.
I agree. I'll make sure to add testing for it if/when we decide to go through
with this change.
> Regardling the need to have an escape hatch that is well publicized
> long before a potentially harmful switch can safely happen, one way
> out might be to
>
> - Declare "git stash create" is a plumbing, but the rest is
> considered Porcelain. Publicize it well and wait for the word to
> spread out. Folks who are in favor of adding these configuration
> variables to the system may need to do some legwork, spreading
> the word around at stackoverflow and various other venues,
> scouring public repositories at GitHub and other hosting sites
> and studying third-party uses of "git stash" that should be
> replaced with "git stash create" followed by "git update-ref",
> and raising issues to notify the owners of such projects, etc.
>
> - Add breaking configuration variables after a few year
This feels like quite a lot of work. I guess that the main question now is
whether this change is worth the effort. What would publicizing this look
like? Updating documentation? Sending out an announcement?
I can also imagine some tools would dislike having to switch to the more
manual stash+update-ref, so maybe easier plumbing should be added?
> FWIW, I can see how permanently enabling "include untracked" may be
> OK in a workflow, but I cannot see the utility of permanently
> enabling "keepindex" at all. Sometimes I'd want to clear the slate
> so that I can try building and testing what I added to the index and
> that is why I want to use the option. But a lot of other times, I
> want to clear the slate to go back to the very pristine state that
> is equal to the HEAD. As the need to switch between the two modes
> happens quite often, the way to defeat configured stash.keepindex
> takes quite many keystrokes, and in general I think the regular
> stashing happens far more often than keepindex stashing, I'd find
> that using shorthand "-k" on the command line occasionally without
> having this configuration variable would be what people would end up
> using anyway.
Permanently enabling keepIndex is mainly intended for people that like to
stash their unstaged changes before committing (e.g., for testing them
independently of other changes). The main issue with what you recommend is
that, if they forget to use `-k`, then the entire state of the index is lost,
which is especially problematic if the changes were interactively staged. A
report of someone having issues with this workflow is here[1].
Perhaps a better solution would be to provide some mechanism by which to also
save the state of the index when stashing changes, and also restore it when
applying the stash. I figure this change in behavior would be much less
problematic.
Another option would be to go closer to the route of `git switch` and `git
restore`, where a separate command with a more user-friendly interface is
added. Then all of `git stash` could be kept as plumbing, and this new command
would be entirely porcelain. I don't think that this change would be worth
introducing a new command for, but if there are other changes that could also
be included, then it might make sense.
> And there is always "[alias] stk = stash -k" available ;-)
Yeah, currently I use a shell alias for `git stash -ku`, but that doesn't help
when I'm using other git frontends.
[1] https://bsd.network/@ed1conf/111783574839749798
^ permalink raw reply
* Re: [PATCH] builtin/stash: configs keepIndex, includeUntracked
From: Junio C Hamano @ 2024-02-21 20:09 UTC (permalink / raw)
To: Ricardo C; +Cc: Phillip Wood, phillip.wood, git
In-Reply-To: <b84e77a1-93e4-4de0-a0cd-4d76a5691a9b@gmail.com>
Ricardo C <rpc01234@gmail.com> writes:
> Permanently enabling keepIndex is mainly intended for people that like
> to stash their unstaged changes before committing (e.g., for testing
> them independently of other changes). The main issue with what you
> recommend is that, if they forget to use `-k`, then the entire state
> of the index is lost, which is especially problematic if the changes
> were interactively staged.
Doesn't "git stash pop --index" meant to recover from such a
mistake, though? If you stash, because your "git commit" notices
there is no change after you did "git stash" without "-k", your
recovery to "pop --index" would apply the changes to the index and
to the working tree on top of exactly the same commit, so there is
no risk of losing any changes by doing so, right? IOW, such a pop
will always round-trip.
^ 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