* jk/loosen-urlmatch, was What's cooking in git.git (Nov 2021, #07; Mon, 29)
2021-11-30 2:16 What's cooking in git.git (Nov 2021, #07; Mon, 29) Junio C Hamano
@ 2021-11-30 7:33 ` Jeff King
2021-11-30 13:17 ` brian m. carlson
2021-11-30 20:54 ` ab/run-command + em/missing-pager (was: What's cooking in git.git (Nov 2021, #07; Mon, 29)) Ævar Arnfjörð Bjarmason
` (11 subsequent siblings)
12 siblings, 1 reply; 42+ messages in thread
From: Jeff King @ 2021-11-30 7:33 UTC (permalink / raw)
To: Junio C Hamano; +Cc: brian m. carlson, git
On Mon, Nov 29, 2021 at 06:16:54PM -0800, Junio C Hamano wrote:
> --------------------------------------------------
> [Graduated to 'master']
>
> [...]
> * jk/loosen-urlmatch (2021-10-12) 1 commit
> (merged to 'next' on 2021-10-25 at f66ca39ebe)
> + urlmatch: add underscore to URL_HOST_CHARS
>
> Treat "_" as any other URL-valid characters in an URL when matching
> the per-URL configuration variable names.
Sorry, I hadn't noticed that this one had even made it to 'next', and
was surprised to see it graduate.
I think brian corrected my assertion in the commit message that RFC 1738
says that underscores are OK. They are for URIs in general, but not the
specific case of hostnames in HTTP schemes.
Now that isn't strictly a reason to drop the patch. Even though
underscores aren't allowed, they do work in some limited circumstances,
and curl is happy to take them. So in some sense this is harmonizing our
urlmatch behavior with curl for an iffy-but-workable practice, and there
may be value in that. But it does take us further away from the
standards, which could possibly have surprising consequences down the
road.
I don't have a strong feeling either way on reverting it at this point.
But I wanted to make sure that if we kept it in, we were doing so
consciously, and not just because folks involved in the discussion
didn't realize it was still working its way through the process.
I.e., if brian is likewise surprised and wants to object, this is now
the time. :)
-Peff
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: jk/loosen-urlmatch, was What's cooking in git.git (Nov 2021, #07; Mon, 29)
2021-11-30 7:33 ` jk/loosen-urlmatch, was " Jeff King
@ 2021-11-30 13:17 ` brian m. carlson
0 siblings, 0 replies; 42+ messages in thread
From: brian m. carlson @ 2021-11-30 13:17 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git
[-- Attachment #1: Type: text/plain, Size: 2030 bytes --]
On 2021-11-30 at 07:33:25, Jeff King wrote:
> On Mon, Nov 29, 2021 at 06:16:54PM -0800, Junio C Hamano wrote:
>
> > --------------------------------------------------
> > [Graduated to 'master']
> >
> > [...]
> > * jk/loosen-urlmatch (2021-10-12) 1 commit
> > (merged to 'next' on 2021-10-25 at f66ca39ebe)
> > + urlmatch: add underscore to URL_HOST_CHARS
> >
> > Treat "_" as any other URL-valid characters in an URL when matching
> > the per-URL configuration variable names.
>
> Sorry, I hadn't noticed that this one had even made it to 'next', and
> was surprised to see it graduate.
>
> I think brian corrected my assertion in the commit message that RFC 1738
> says that underscores are OK. They are for URIs in general, but not the
> specific case of hostnames in HTTP schemes.
From my memory of the research I did, they're okay for URIs according to
the grammar, but they're acceptable only if you're using something that
isn't DNS, which as a practical matter is never the case.
> Now that isn't strictly a reason to drop the patch. Even though
> underscores aren't allowed, they do work in some limited circumstances,
> and curl is happy to take them. So in some sense this is harmonizing our
> urlmatch behavior with curl for an iffy-but-workable practice, and there
> may be value in that. But it does take us further away from the
> standards, which could possibly have surprising consequences down the
> road.
>
> I don't have a strong feeling either way on reverting it at this point.
> But I wanted to make sure that if we kept it in, we were doing so
> consciously, and not just because folks involved in the discussion
> didn't realize it was still working its way through the process.
I don't think it's necessarily worth reverting, but we should avoid
continuing to make further improvements in this area. In other words,
we shouldn't take this as an opportunity to support more of this.
--
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]
^ permalink raw reply [flat|nested] 42+ messages in thread
* ab/run-command + em/missing-pager (was: What's cooking in git.git (Nov 2021, #07; Mon, 29))
2021-11-30 2:16 What's cooking in git.git (Nov 2021, #07; Mon, 29) Junio C Hamano
2021-11-30 7:33 ` jk/loosen-urlmatch, was " Jeff King
@ 2021-11-30 20:54 ` Ævar Arnfjörð Bjarmason
2021-11-30 21:17 ` Jeff King
2021-11-30 22:09 ` Eric Sunshine
2021-11-30 21:08 ` ab/ci-updates " Ævar Arnfjörð Bjarmason
` (10 subsequent siblings)
12 siblings, 2 replies; 42+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-30 20:54 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Enzo Matsumiya, Eric Sunshine, Emily Shaffer
On Mon, Nov 29 2021, Junio C Hamano wrote:
> * ab/run-command (2021-11-25) 9 commits
> - run-command API: remove "env" member, always use "env_array"
> - difftool: use "env_array" to simplify memory management
> - run-command API: remove "argv" member, always use "args"
> - run-command API users: use strvec_push(), not argv construction
> - run-command API users: use strvec_pushl(), not argv construction
> - run-command tests: use strvec_pushv(), not argv assignment
> - run-command API users: use strvec_pushv(), not argv assignment
> - upload-archive: use regular "struct child_process" pattern
> - worktree: stop being overly intimate with run_command() internals
I think the only outstanding thing for this topic is Eric's [1] comment
(on his own code). I think that variable shadowing is OK.
1/2 of that patch will also be rewritten in the in-flight hook topic
(but the "reset --hard" shadowing is left in place).
> * em/missing-pager (2021-11-24) 1 commit
> - pager: fix crash when pager program doesn't exist
As noted in [2] I'm happy to get this more isolated fix first. I'd
missed that there was a re-submission[3] until now (since In-Reply-To
wasn't maintained).
The code change in [3] isn't needed anymore when combined with my
ab/run-command.
Depending on how you're planning to advance these perhaps you'd like to
revert that as it's merged with ab/run-command, or I can re-roll
ab/run-command on top of it if you'd like.
We could also just leave that now-redundant child_process_init() in
pager.c, but having something that'll amount to cargo-culting to get
around a bug in dead code doesn't seem ideal. It would be nice to have
the API use reflect "argv" and "env" being gone.
1. https://lore.kernel.org/git/CAPig+cRi6SeuV7k_+9JCcnf79daLZp5B=EyHK-KxC1VGN0B4ig@mail.gmail.com/
2. https://lore.kernel.org/git/211124.865ysie2br.gmgdl@evledraar.gmail.com/
3. https://lore.kernel.org/git/20211125000239.2336-1-ematsumiya@suse.de
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: ab/run-command + em/missing-pager (was: What's cooking in git.git (Nov 2021, #07; Mon, 29))
2021-11-30 20:54 ` ab/run-command + em/missing-pager (was: What's cooking in git.git (Nov 2021, #07; Mon, 29)) Ævar Arnfjörð Bjarmason
@ 2021-11-30 21:17 ` Jeff King
2021-11-30 22:09 ` Eric Sunshine
1 sibling, 0 replies; 42+ messages in thread
From: Jeff King @ 2021-11-30 21:17 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Junio C Hamano, git, Enzo Matsumiya, Eric Sunshine, Emily Shaffer
On Tue, Nov 30, 2021 at 09:54:33PM +0100, Ævar Arnfjörð Bjarmason wrote:
> > * em/missing-pager (2021-11-24) 1 commit
> > - pager: fix crash when pager program doesn't exist
>
> As noted in [2] I'm happy to get this more isolated fix first. I'd
> missed that there was a re-submission[3] until now (since In-Reply-To
> wasn't maintained).
>
> The code change in [3] isn't needed anymore when combined with my
> ab/run-command.
>
> Depending on how you're planning to advance these perhaps you'd like to
> revert that as it's merged with ab/run-command, or I can re-roll
> ab/run-command on top of it if you'd like.
>
> We could also just leave that now-redundant child_process_init() in
> pager.c, but having something that'll amount to cargo-culting to get
> around a bug in dead code doesn't seem ideal. It would be nice to have
> the API use reflect "argv" and "env" being gone.
IMHO that extra init is still doing the right thing, because it means
the struct is in the same known state in every run of setup_pager().
Fixing the argv/env thing stops the immediate bug, but there are other
potential ones. E.g., starting the command will overwrite the in/out/err
parameters; we're OK in this instance because we unconditionally
overwrite them.
So I think reusing a child_process struct without reinitializing it is
always a bit iffy, though it _usually_ works in practice. We should
model good use of the API by initializing on each use.
-Peff
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: ab/run-command + em/missing-pager (was: What's cooking in git.git (Nov 2021, #07; Mon, 29))
2021-11-30 20:54 ` ab/run-command + em/missing-pager (was: What's cooking in git.git (Nov 2021, #07; Mon, 29)) Ævar Arnfjörð Bjarmason
2021-11-30 21:17 ` Jeff King
@ 2021-11-30 22:09 ` Eric Sunshine
1 sibling, 0 replies; 42+ messages in thread
From: Eric Sunshine @ 2021-11-30 22:09 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Junio C Hamano, Git List, Enzo Matsumiya, Emily Shaffer
On Tue, Nov 30, 2021 at 4:07 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> On Mon, Nov 29 2021, Junio C Hamano wrote:
> > * ab/run-command (2021-11-25) 9 commits
> > - worktree: stop being overly intimate with run_command() internals
>
> I think the only outstanding thing for this topic is Eric's [1] comment
> (on his own code). I think that variable shadowing is OK.
>
> 1. https://lore.kernel.org/git/CAPig+cRi6SeuV7k_+9JCcnf79daLZp5B=EyHK-KxC1VGN0B4ig@mail.gmail.com/
I'm also fine with the variable shadowing and don't consider it an
outstanding issue with the topic (it was just a minor observation in
case anyone complained). I didn't see anything else worth commenting
on in the remainder of the series.
^ permalink raw reply [flat|nested] 42+ messages in thread
* ab/ci-updates (was: What's cooking in git.git (Nov 2021, #07; Mon, 29))
2021-11-30 2:16 What's cooking in git.git (Nov 2021, #07; Mon, 29) Junio C Hamano
2021-11-30 7:33 ` jk/loosen-urlmatch, was " Jeff King
2021-11-30 20:54 ` ab/run-command + em/missing-pager (was: What's cooking in git.git (Nov 2021, #07; Mon, 29)) Ævar Arnfjörð Bjarmason
@ 2021-11-30 21:08 ` Ævar Arnfjörð Bjarmason
2021-11-30 21:12 ` ab/config-based-hooks-2 " Ævar Arnfjörð Bjarmason
` (9 subsequent siblings)
12 siblings, 0 replies; 42+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-30 21:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Johannes Schindelin, Victoria Dye
On Mon, Nov 29 2021, Junio C Hamano wrote:
> * ab/ci-updates (2021-11-23) 5 commits
> - CI: don't run "make test" twice in one job
> - CI: use "$runs_on_pool", not "$jobname" to select packages & config
> - CI: rename the "Linux32" job to lower-case "linux32"
> - CI: use shorter names that fit in UX tooltips
> - CI: remove Travis CI support
It looked like there was general happiness with the approach of having
the dead code gone and the GitHub tooltips being more useful.
Given that this is relatively trivial, and if it was causing trouble
we'd likely know by now it would be nice to get this merged down sooner
than later.
As noted in the latest thread on js/scalar there's outstanding
interactions with that larger topic (including some semantic merge
"conflicts"). So having it in "master" to base on would also remove its
interaction with that topic & any other CI modification as a variable.
1. https://lore.kernel.org/git/211130.86mtlleqhm.gmgdl@evledraar.gmail.com/
^ permalink raw reply [flat|nested] 42+ messages in thread* ab/config-based-hooks-2 (was: What's cooking in git.git (Nov 2021, #07; Mon, 29))
2021-11-30 2:16 What's cooking in git.git (Nov 2021, #07; Mon, 29) Junio C Hamano
` (2 preceding siblings ...)
2021-11-30 21:08 ` ab/ci-updates " Ævar Arnfjörð Bjarmason
@ 2021-11-30 21:12 ` Ævar Arnfjörð Bjarmason
2021-11-30 21:17 ` fs/test-prereq " Ævar Arnfjörð Bjarmason
` (8 subsequent siblings)
12 siblings, 0 replies; 42+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-30 21:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Emily Shaffer
On Mon, Nov 29 2021, Junio C Hamano wrote:
> * ab/config-based-hooks-2 (2021-11-24) 18 commits
> - run-command: remove old run_hook_{le,ve}() hook API
> - receive-pack: convert push-to-checkout hook to hook.h
> - read-cache: convert post-index-change to use hook.h
> - commit: convert {pre-commit,prepare-commit-msg} hook to hook.h
> - git-p4: use 'git hook' to run hooks
> - send-email: use 'git hook run' for 'sendemail-validate'
> - git hook run: add an --ignore-missing flag
> - hooks: convert worktree 'post-checkout' hook to hook library
> - hooks: convert non-worktree 'post-checkout' hook to hook library
> - merge: convert post-merge to use hook.h
> - am: convert applypatch-msg to use hook.h
> - rebase: convert pre-rebase to use hook.h
> - hook API: add a run_hooks_l() wrapper
> - am: convert {pre,post}-applypatch to use hook.h
> - gc: use hook library for pre-auto-gc hook
> - hook API: add a run_hooks() wrapper
> - hook: add 'run' subcommand
> - Merge branch 'ab/config-based-hooks-1' into ab/config-based-hooks-2
>
> More "config-based hooks".
I know Emily's keen to get to the conclusion of the "config-based-hooks"
story. I think it's gotten a lot of careful review already, and it would
be nice to have it land soon in this cycle so the follow-up topics can
be queued after it.
I re-rolled it a ~week ago[1], but that was a trivial change to save you
from dealing with a compilation error as it got merged with my own
"ab/run-command" (it was using "env", not "env_array")[1]. Other than
that there's been no change to it for the last month since the v4[2].
1. https://lore.kernel.org/git/cover-v5-00.17-00000000000-20211123T114206Z-avarab@gmail.com/
2. https://lore.kernel.org/git/cover-v4-00.17-00000000000-20211101T184938Z-avarab@gmail.com/
^ permalink raw reply [flat|nested] 42+ messages in thread* fs/test-prereq (was: What's cooking in git.git (Nov 2021, #07; Mon, 29))
2021-11-30 2:16 What's cooking in git.git (Nov 2021, #07; Mon, 29) Junio C Hamano
` (3 preceding siblings ...)
2021-11-30 21:12 ` ab/config-based-hooks-2 " Ævar Arnfjörð Bjarmason
@ 2021-11-30 21:17 ` Ævar Arnfjörð Bjarmason
2021-12-01 8:53 ` Fabian Stelzer
2021-11-30 21:18 ` jc/c99-var-decl-in-for-loop " Ævar Arnfjörð Bjarmason
` (7 subsequent siblings)
12 siblings, 1 reply; 42+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-30 21:17 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Fabian Stelzer
On Mon, Nov 29 2021, Junio C Hamano wrote:
> * fs/test-prereq (2021-11-20) 3 commits
> - test-lib: make BAIL_OUT() work in tests and prereq
> - test-lib: introduce required prereq for test runs
> - test-lib: show missing prereq summary
>
> The test framework learns to list unsatisfied test prerequisites,
> and optionally error out when prerequisites that are expected to be
> satisfied are not.
>
> Will merge to 'next'?
My reading of
https://lore.kernel.org/git/20211130143821.7dz5jj2z2x2q2ytn@fs/ is that
Fabian's planning to re-roll it.
I replied today to that to clarify some edge cases around TAP.
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: fs/test-prereq (was: What's cooking in git.git (Nov 2021, #07; Mon, 29))
2021-11-30 21:17 ` fs/test-prereq " Ævar Arnfjörð Bjarmason
@ 2021-12-01 8:53 ` Fabian Stelzer
0 siblings, 0 replies; 42+ messages in thread
From: Fabian Stelzer @ 2021-12-01 8:53 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: Junio C Hamano, git
On 30.11.2021 22:17, Ævar Arnfjörð Bjarmason wrote:
>
>On Mon, Nov 29 2021, Junio C Hamano wrote:
>
>> * fs/test-prereq (2021-11-20) 3 commits
>> - test-lib: make BAIL_OUT() work in tests and prereq
>> - test-lib: introduce required prereq for test runs
>> - test-lib: show missing prereq summary
>>
>> The test framework learns to list unsatisfied test prerequisites,
>> and optionally error out when prerequisites that are expected to be
>> satisfied are not.
>>
>> Will merge to 'next'?
>
>My reading of
>https://lore.kernel.org/git/20211130143821.7dz5jj2z2x2q2ytn@fs/ is that
>Fabian's planning to re-roll it.
>
>I replied today to that to clarify some edge cases around TAP.
Yes, thanks for that. I resubmitted a new version with the better comment.
Thanks
^ permalink raw reply [flat|nested] 42+ messages in thread
* jc/c99-var-decl-in-for-loop (was: What's cooking in git.git (Nov 2021, #07; Mon, 29))
2021-11-30 2:16 What's cooking in git.git (Nov 2021, #07; Mon, 29) Junio C Hamano
` (4 preceding siblings ...)
2021-11-30 21:17 ` fs/test-prereq " Ævar Arnfjörð Bjarmason
@ 2021-11-30 21:18 ` Ævar Arnfjörð Bjarmason
2021-11-30 23:07 ` ns/tmp-objdir and ns/remerge-diff Elijah Newren
` (6 subsequent siblings)
12 siblings, 0 replies; 42+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-30 21:18 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, brian m. carlson, SZEDER Gábor
On Mon, Nov 29 2021, Junio C Hamano wrote:
> * jc/c99-var-decl-in-for-loop (2021-11-22) 1 commit
> - revision: use C99 declaration of variable in for() loop
>
> Weather balloon to break compilers that do not grok variable
> declaration in the for() loop.
>
> Will merge to 'next'?
I think that sounds good!
The change is trivial, and due to the nature of it having it sooner than
later this cycle can only be better, in case there's some obscure system
we didn't consider, catching that before the next release would be
preferable.
^ permalink raw reply [flat|nested] 42+ messages in thread* ns/tmp-objdir and ns/remerge-diff
2021-11-30 2:16 What's cooking in git.git (Nov 2021, #07; Mon, 29) Junio C Hamano
` (5 preceding siblings ...)
2021-11-30 21:18 ` jc/c99-var-decl-in-for-loop " Ævar Arnfjörð Bjarmason
@ 2021-11-30 23:07 ` Elijah Newren
2021-12-03 19:21 ` Junio C Hamano
2021-11-30 23:35 ` What's cooking in git.git (Nov 2021, #07; Mon, 29) Elijah Newren
` (5 subsequent siblings)
12 siblings, 1 reply; 42+ messages in thread
From: Elijah Newren @ 2021-11-30 23:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List, Neeraj K. Singh
On Tue, Nov 30, 2021 at 6:49 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> The exception is 'ns/tmp-objdir' and a few topics that depend on it,
> which are kicked out of 'next' to give the base topic a chance to
> lose the two "fixup!" band-aid. Careful re-reading of the topic by
> somebody with a fresh set of eyes (meaning, everybody, as this has
> been dormant for almost a month) is very much appreciated to move
> things forward.
ns/tmp-objdir had a re-roll that has not been picked up, at [1] --
perhaps because it's an combination of ns/tmp-objdir and
ns/batched-fsync (it'd be nicer to have those two split). I gave the
ns/tmp-objdir part another read over and was only able to spot two
small things. I think you should mark it as expecting a reroll based
on [2] ("Good catch. I'll fix this.") and [3] ("I'll take this
suggestion."), but I think it could be merged to next quickly after
that.
[1] https://lore.kernel.org/git/pull.1076.v9.git.git.1637020263.gitgitgadget@gmail.com/
[2] https://lore.kernel.org/git/CANQDOddCC7+gGUy1VBxxwvN7ieP+N8mQhbxK2xx6ySqZc6U7-g@mail.gmail.com/
[3] https://lore.kernel.org/git/CANQDOdd7EHUqD_JBdO9ArpvOQYUnU9GSL6EVR7W7XXgNASZyhQ@mail.gmail.com/
> Also ns/remerge-diff that is Neeraj's rebase of the
> remerge-diff topic needs Elijah's Ack at least.
Mark it as expecting a re-roll; I've been waiting for ns/tmp-objdir to
settle so I can rebase on it.
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: ns/tmp-objdir and ns/remerge-diff
2021-11-30 23:07 ` ns/tmp-objdir and ns/remerge-diff Elijah Newren
@ 2021-12-03 19:21 ` Junio C Hamano
2021-12-04 2:58 ` Neeraj Singh
0 siblings, 1 reply; 42+ messages in thread
From: Junio C Hamano @ 2021-12-03 19:21 UTC (permalink / raw)
To: Elijah Newren, Neeraj K. Singh; +Cc: Git Mailing List
Elijah Newren <newren@gmail.com> writes:
> ns/tmp-objdir had a re-roll that has not been picked up, at [1] --
> perhaps because it's an combination of ns/tmp-objdir and
> ns/batched-fsync (it'd be nicer to have those two split). I gave the
> ns/tmp-objdir part another read over and was only able to spot two
> small things. I think you should mark it as expecting a reroll based
> on [2] ("Good catch. I'll fix this.") and [3] ("I'll take this
> suggestion."), but I think it could be merged to next quickly after
> that.
>
> [1] https://lore.kernel.org/git/pull.1076.v9.git.git.1637020263.gitgitgadget@gmail.com/
> [2] https://lore.kernel.org/git/CANQDOddCC7+gGUy1VBxxwvN7ieP+N8mQhbxK2xx6ySqZc6U7-g@mail.gmail.com/
> [3] https://lore.kernel.org/git/CANQDOdd7EHUqD_JBdO9ArpvOQYUnU9GSL6EVR7W7XXgNASZyhQ@mail.gmail.com/
>
>> Also ns/remerge-diff that is Neeraj's rebase of the
>> remerge-diff topic needs Elijah's Ack at least.
>
> Mark it as expecting a re-roll; I've been waiting for ns/tmp-objdir to
> settle so I can rebase on it.
I took a quick look at the rerolled one on list, and I agree that
keeping tmp-objdir and batched-fsync as two separate topics makes
sense, since the former can graduate much more smoothly and quickly,
and it can have other dependant topics.
So I'll mark all three (ns/tmp-objdir, ns/batched-fsync and
remerge-diff) as "Expecting a reroll".
As I announced, I won't be taking any new topics or new rerolls
today (or possibly tomorrow) until I can sift the topics I've
already seen to come up with a tested set of candidate topics to
merge to 'next', so there is no need to rush.
Thanks.
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: ns/tmp-objdir and ns/remerge-diff
2021-12-03 19:21 ` Junio C Hamano
@ 2021-12-04 2:58 ` Neeraj Singh
2021-12-04 5:51 ` Elijah Newren
0 siblings, 1 reply; 42+ messages in thread
From: Neeraj Singh @ 2021-12-04 2:58 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Elijah Newren, Neeraj K. Singh, Git Mailing List
On Fri, Dec 03, 2021 at 11:21:13AM -0800, Junio C Hamano wrote:
> Elijah Newren <newren@gmail.com> writes:
>
> > ns/tmp-objdir had a re-roll that has not been picked up, at [1] --
> > perhaps because it's an combination of ns/tmp-objdir and
> > ns/batched-fsync (it'd be nicer to have those two split). I gave the
> > ns/tmp-objdir part another read over and was only able to spot two
> > small things. I think you should mark it as expecting a reroll based
> > on [2] ("Good catch. I'll fix this.") and [3] ("I'll take this
> > suggestion."), but I think it could be merged to next quickly after
> > that.
> >
> > [1] https://lore.kernel.org/git/pull.1076.v9.git.git.1637020263.gitgitgadget@gmail.com/
> > [2] https://lore.kernel.org/git/CANQDOddCC7+gGUy1VBxxwvN7ieP+N8mQhbxK2xx6ySqZc6U7-g@mail.gmail.com/
> > [3] https://lore.kernel.org/git/CANQDOdd7EHUqD_JBdO9ArpvOQYUnU9GSL6EVR7W7XXgNASZyhQ@mail.gmail.com/
> >
> >> Also ns/remerge-diff that is Neeraj's rebase of the
> >> remerge-diff topic needs Elijah's Ack at least.
> >
> > Mark it as expecting a re-roll; I've been waiting for ns/tmp-objdir to
> > settle so I can rebase on it.
>
> I took a quick look at the rerolled one on list, and I agree that
> keeping tmp-objdir and batched-fsync as two separate topics makes
> sense, since the former can graduate much more smoothly and quickly,
> and it can have other dependant topics.
>
> So I'll mark all three (ns/tmp-objdir, ns/batched-fsync and
> remerge-diff) as "Expecting a reroll".
>
> As I announced, I won't be taking any new topics or new rerolls
> today (or possibly tomorrow) until I can sift the topics I've
> already seen to come up with a tested set of candidate topics to
> merge to 'next', so there is no need to rush.
>
> Thanks.
I submitted a new PR (with a new mail thread) for ns/tmp-objdir. Hopefully
that one can sail in smoothly now.
ns/batched-fsync will take a bit more time to settle. I'm going to post a
new series called ns/core-fsync, which is focused on the extensible interface
for syncing parts of the tree.
Thanks,
Neeraj
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: ns/tmp-objdir and ns/remerge-diff
2021-12-04 2:58 ` Neeraj Singh
@ 2021-12-04 5:51 ` Elijah Newren
0 siblings, 0 replies; 42+ messages in thread
From: Elijah Newren @ 2021-12-04 5:51 UTC (permalink / raw)
To: Neeraj Singh; +Cc: Junio C Hamano, Neeraj K. Singh, Git Mailing List
On Fri, Dec 3, 2021 at 6:58 PM Neeraj Singh <nksingh85@gmail.com> wrote:
>
> On Fri, Dec 03, 2021 at 11:21:13AM -0800, Junio C Hamano wrote:
> > Elijah Newren <newren@gmail.com> writes:
> >
> > > ns/tmp-objdir had a re-roll that has not been picked up, at [1] --
> > > perhaps because it's an combination of ns/tmp-objdir and
> > > ns/batched-fsync (it'd be nicer to have those two split). I gave the
> > > ns/tmp-objdir part another read over and was only able to spot two
> > > small things. I think you should mark it as expecting a reroll based
> > > on [2] ("Good catch. I'll fix this.") and [3] ("I'll take this
> > > suggestion."), but I think it could be merged to next quickly after
> > > that.
> > >
> > > [1] https://lore.kernel.org/git/pull.1076.v9.git.git.1637020263.gitgitgadget@gmail.com/
> > > [2] https://lore.kernel.org/git/CANQDOddCC7+gGUy1VBxxwvN7ieP+N8mQhbxK2xx6ySqZc6U7-g@mail.gmail.com/
> > > [3] https://lore.kernel.org/git/CANQDOdd7EHUqD_JBdO9ArpvOQYUnU9GSL6EVR7W7XXgNASZyhQ@mail.gmail.com/
> > >
> > >> Also ns/remerge-diff that is Neeraj's rebase of the
> > >> remerge-diff topic needs Elijah's Ack at least.
> > >
> > > Mark it as expecting a re-roll; I've been waiting for ns/tmp-objdir to
> > > settle so I can rebase on it.
> >
> > I took a quick look at the rerolled one on list, and I agree that
> > keeping tmp-objdir and batched-fsync as two separate topics makes
> > sense, since the former can graduate much more smoothly and quickly,
> > and it can have other dependant topics.
> >
> > So I'll mark all three (ns/tmp-objdir, ns/batched-fsync and
> > remerge-diff) as "Expecting a reroll".
> >
> > As I announced, I won't be taking any new topics or new rerolls
> > today (or possibly tomorrow) until I can sift the topics I've
> > already seen to come up with a tested set of candidate topics to
> > merge to 'next', so there is no need to rush.
> >
> > Thanks.
>
> I submitted a new PR (with a new mail thread) for ns/tmp-objdir. Hopefully
> that one can sail in smoothly now.
Thanks for sending that out. I wasn't cc'ed on them, and I'm dealing
with multi-day delay of git emails that I'm not cc'ed on, but I read
them over on lore.kernel.org/git and those two patches look good and
appear to me to be ready for next. :-)
> ns/batched-fsync will take a bit more time to settle. I'm going to post a
> new series called ns/core-fsync, which is focused on the extensible interface
> for syncing parts of the tree.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: What's cooking in git.git (Nov 2021, #07; Mon, 29)
2021-11-30 2:16 What's cooking in git.git (Nov 2021, #07; Mon, 29) Junio C Hamano
` (6 preceding siblings ...)
2021-11-30 23:07 ` ns/tmp-objdir and ns/remerge-diff Elijah Newren
@ 2021-11-30 23:35 ` Elijah Newren
2021-12-01 19:29 ` Victoria Dye
2021-11-30 23:45 ` Elijah Newren
` (4 subsequent siblings)
12 siblings, 1 reply; 42+ messages in thread
From: Elijah Newren @ 2021-11-30 23:35 UTC (permalink / raw)
To: Junio C Hamano
Cc: Git Mailing List, Ævar Arnfjörð Bjarmason,
Phillip Wood, Victoria Dye
Not sure if it help/hurts/matters, but thought I'd send a thumbs up on
several of the topics. For topics I have comments other than a thumbs
up for, I'll post separate messages...
On Tue, Nov 30, 2021 at 6:49 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> * pw/xdiff-classify-record-in-histogram (2021-11-18) 3 commits
> ...
> Will merge to 'next'.
>
>
> * ab/mark-leak-free-tests-even-more (2021-11-01) 15 commits
> ...
> Will merge to 'next'.
>
>
> * pw/diff-color-moved-fix (2021-10-27) 15 commits
> ...
> Will merge to 'next'.
>
>
> * vd/sparse-sparsity-fix-on-read (2021-11-24) 4 commits
> ...
> Will merge to 'next'.
Thumb up on all these from me.
> * vd/sparse-reset (2021-11-29) 8 commits
> ...
> Will merge to 'next'.
There were two tiny issues pointed out at
https://lore.kernel.org/git/9c5b7067-b0e3-0153-5cd3-042bae92f91e@github.com/,
but it'd be fine to merge the series as-is and consider those as later
potential improvements.
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: What's cooking in git.git (Nov 2021, #07; Mon, 29)
2021-11-30 23:35 ` What's cooking in git.git (Nov 2021, #07; Mon, 29) Elijah Newren
@ 2021-12-01 19:29 ` Victoria Dye
0 siblings, 0 replies; 42+ messages in thread
From: Victoria Dye @ 2021-12-01 19:29 UTC (permalink / raw)
To: Elijah Newren, Junio C Hamano
Cc: Git Mailing List, Ævar Arnfjörð Bjarmason,
Phillip Wood
Elijah Newren wrote:
>> * vd/sparse-reset (2021-11-29) 8 commits
>> ...
>> Will merge to 'next'.
>
> There were two tiny issues pointed out at
> https://lore.kernel.org/git/9c5b7067-b0e3-0153-5cd3-042bae92f91e@github.com/,
> but it'd be fine to merge the series as-is and consider those as later
> potential improvements.
>
In the interest of avoiding further delays to (and maintenance burden for)
the other in-flight series dependent on this one, I'd prefer including those
fixups in my next set of sparse index integrations.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: What's cooking in git.git (Nov 2021, #07; Mon, 29)
2021-11-30 2:16 What's cooking in git.git (Nov 2021, #07; Mon, 29) Junio C Hamano
` (7 preceding siblings ...)
2021-11-30 23:35 ` What's cooking in git.git (Nov 2021, #07; Mon, 29) Elijah Newren
@ 2021-11-30 23:45 ` Elijah Newren
2021-12-01 1:42 ` Aleen 徐沛文
2021-11-30 23:52 ` en/zdiff3 (was: Re: What's cooking in git.git (Nov 2021, #07; Mon, 29)) Elijah Newren
` (3 subsequent siblings)
12 siblings, 1 reply; 42+ messages in thread
From: Elijah Newren @ 2021-11-30 23:45 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List, 徐沛文 (Aleen)
On Tue, Nov 30, 2021 at 6:49 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> * xw/am-empty (2021-11-22) 2 commits
> - am: support --empty=<option> to handle empty patches
> - doc: git-format-patch: describe the option --always
>
> "git am" learns "--empty=(die|drop|keep)" option to tweak what is
> done to a piece of e-mail without a patch in it.
>
> Will merge to 'next'.
Please don't, at least not this version. There have been newer
submissions with three commits.
I also still find the word 'die' confusing, since to me it suggests
aborting the whole am operation, and the documentation does not dispel
that concern. Even if you don't like 'ask' (for consistency with
git-rebase), I think 'stop' or 'interrupt' would be much better
options than 'die'. If you really want it to be 'die', I think the
behavior needs to be explained in the documentation, rather than just
assumed that users will understand it (because I didn't understand it
until I read the code).
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: What's cooking in git.git (Nov 2021, #07; Mon, 29)
2021-11-30 23:45 ` Elijah Newren
@ 2021-12-01 1:42 ` Aleen 徐沛文
2021-12-01 20:56 ` Elijah Newren
0 siblings, 1 reply; 42+ messages in thread
From: Aleen 徐沛文 @ 2021-12-01 1:42 UTC (permalink / raw)
To: Elijah Newren
Cc: Junio C Hamano, Git Mailing List,
徐沛文 (Aleen), Aleen via GitGitGadget
> Please don't, at least not this version. There have been newer
> submissions with three commits.
>
> I also still find the word 'die' confusing, since to me it suggests
> aborting the whole am operation, and the documentation does not dispel
> that concern. Even if you don't like 'ask' (for consistency with
> git-rebase), I think 'stop' or 'interrupt' would be much better
> options than 'die'. If you really want it to be 'die', I think the
> behavior needs to be explained in the documentation, rather than just
> assumed that users will understand it (because I didn't understand it
> until I read the code).
Dears Newren,
I don't think 'stop' and 'interrupt' words are better to explain more than 'die'
because they still indicate that it will stop or interrupt the whole am session,
rather than stop in the middle of it. It may be a good choice to just add an
additional description about the behaviour when not passing the '--empty' option
or passing '--empty=die'.
Aleen
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: What's cooking in git.git (Nov 2021, #07; Mon, 29)
2021-12-01 1:42 ` Aleen 徐沛文
@ 2021-12-01 20:56 ` Elijah Newren
2021-12-03 18:21 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 42+ messages in thread
From: Elijah Newren @ 2021-12-01 20:56 UTC (permalink / raw)
To: Aleen 徐沛文
Cc: Junio C Hamano, Git Mailing List,
徐沛文 (Aleen), Aleen via GitGitGadget
On Tue, Nov 30, 2021 at 5:42 PM Aleen 徐沛文 <pwxu@coremail.cn> wrote:
>
> > Please don't, at least not this version. There have been newer
> > submissions with three commits.
> >
> > I also still find the word 'die' confusing, since to me it suggests
> > aborting the whole am operation, and the documentation does not dispel
> > that concern. Even if you don't like 'ask' (for consistency with
> > git-rebase), I think 'stop' or 'interrupt' would be much better
> > options than 'die'. If you really want it to be 'die', I think the
> > behavior needs to be explained in the documentation, rather than just
> > assumed that users will understand it (because I didn't understand it
> > until I read the code).
>
> Dears Newren,
>
> I don't think 'stop' and 'interrupt' words are better to explain more than 'die'
> because they still indicate that it will stop or interrupt the whole am session,
> rather than stop in the middle of it.
Since you've been through several rounds of revisions already, if this
is the only remaining issue with your series, I wouldn't try to hold
it up for this issue; I just thought it could be fixed while you were
working on the --allow-empty stuff.
However, while I don't think it's worth holding up your series for
just this issue, I would definitely submit a follow-up RFC patch to
fix the wording, because I do disagree with your assertion here pretty
strongly. Let's look at the meanings of the terms:
die: connotes something pretty final and irreversible -- people tend
not to revive after death as far as recorded history goes; most such
recorded instances tend to be causes for people to debate the
definition of 'dead'.
stop: could be final, but is often used in a transitory setting: "stop
and go traffic", "stopped to catch my breath", "the train will stop
at this station", "stop! I want to get out", etc.
interrupt: seems to nearly always be used as a transitory thing
Now, in the computer science context, all three terms come up relative
to processes. You can interrupt a process (the kernel does all the
time), but it'll usually continue afterwards. Or you can give it a
SIGINT (interrupt from keyboard signal), which the process can catch
and ignore. You can stop a process (and SIGSTOP cannot be caught),
but you can also continue it later. die essentially means the process
is going to be gone for good (at least short of some kind of black
magic like a reversible debugger such as rr).
So, I think it's much more likely that 'die' will be misunderstood to
mean abortion of the entire am-process, than that 'stop' or
'interrupt' would.
> It may be a good choice to just add an
> additional description about the behaviour when not passing the '--empty' option
> or passing '--empty=die'.
That would be good.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: What's cooking in git.git (Nov 2021, #07; Mon, 29)
2021-12-01 20:56 ` Elijah Newren
@ 2021-12-03 18:21 ` Ævar Arnfjörð Bjarmason
2021-12-03 19:28 ` Elijah Newren
0 siblings, 1 reply; 42+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-03 18:21 UTC (permalink / raw)
To: Elijah Newren
Cc: Aleen 徐沛文, Junio C Hamano, Git Mailing List,
徐沛文 (Aleen), Aleen via GitGitGadget
On Wed, Dec 01 2021, Elijah Newren wrote:
> On Tue, Nov 30, 2021 at 5:42 PM Aleen 徐沛文 <pwxu@coremail.cn> wrote:
>>
>> > Please don't, at least not this version. There have been newer
>> > submissions with three commits.
>> >
>> > I also still find the word 'die' confusing, since to me it suggests
>> > aborting the whole am operation, and the documentation does not dispel
>> > that concern. Even if you don't like 'ask' (for consistency with
>> > git-rebase), I think 'stop' or 'interrupt' would be much better
>> > options than 'die'. If you really want it to be 'die', I think the
>> > behavior needs to be explained in the documentation, rather than just
>> > assumed that users will understand it (because I didn't understand it
>> > until I read the code).
>>
>> Dears Newren,
>>
>> I don't think 'stop' and 'interrupt' words are better to explain more than 'die'
>> because they still indicate that it will stop or interrupt the whole am session,
>> rather than stop in the middle of it.
>
> Since you've been through several rounds of revisions already, if this
> is the only remaining issue with your series, I wouldn't try to hold
> it up for this issue; I just thought it could be fixed while you were
> working on the --allow-empty stuff.
FWIW I think it's worth getting the UX issue right, tweaking it is
relatively easy, and if we can decide on what the thing is called
then...
> However, while I don't think it's worth holding up your series for
> just this issue, I would definitely submit a follow-up RFC patch to
> fix the wording, because I do disagree with your assertion here pretty
> strongly. Let's look at the meanings of the terms:
>
> die: connotes something pretty final and irreversible -- people tend
> not to revive after death as far as recorded history goes; most such
> recorded instances tend to be causes for people to debate the
> definition of 'dead'.
>
> stop: could be final, but is often used in a transitory setting: "stop
> and go traffic", "stopped to catch my breath", "the train will stop
> at this station", "stop! I want to get out", etc.
>
> interrupt: seems to nearly always be used as a transitory thing
>
> Now, in the computer science context, all three terms come up relative
> to processes. You can interrupt a process (the kernel does all the
> time), but it'll usually continue afterwards. Or you can give it a
> SIGINT (interrupt from keyboard signal), which the process can catch
> and ignore. You can stop a process (and SIGSTOP cannot be caught),
> but you can also continue it later. die essentially means the process
> is going to be gone for good (at least short of some kind of black
> magic like a reversible debugger such as rr).
>
> So, I think it's much more likely that 'die' will be misunderstood to
> mean abortion of the entire am-process, than that 'stop' or
> 'interrupt' would.
Why are we exposing an --empty=die at all? It's what we do by default,
so why have it? The user can just not provide the "--empty" option, then
they'll get the current behavior of die_user_resolve(), which seems to
have inpired the name for this "die" (it exits with code 128, just like
die()).
Once we get rid of "die" the rest of the UI can follow the example of
the existing "git rebase" options:
--empty={drop,keep,ask}
I.e. the "drop" and "keep" will be the same, no "ask" currently, but it
can be implemented in the future.
Maybe I'm missing something, I haven't used "am" in this way (or rebase
with --empty=*), but this just seems to me to be needlessly exposing a
"die" (or "stop" or whatever) because it's how we implement this.
Whereas for the UX we don't need to call it anything except the absence
of an --empty option, or perhaps --no-empty.
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: What's cooking in git.git (Nov 2021, #07; Mon, 29)
2021-12-03 18:21 ` Ævar Arnfjörð Bjarmason
@ 2021-12-03 19:28 ` Elijah Newren
2021-12-03 19:56 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 42+ messages in thread
From: Elijah Newren @ 2021-12-03 19:28 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Aleen 徐沛文, Junio C Hamano, Git Mailing List,
徐沛文 (Aleen), Aleen via GitGitGadget
On Fri, Dec 3, 2021 at 10:37 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
> On Wed, Dec 01 2021, Elijah Newren wrote:
>
> > On Tue, Nov 30, 2021 at 5:42 PM Aleen 徐沛文 <pwxu@coremail.cn> wrote:
> >>
> >> > Please don't, at least not this version. There have been newer
> >> > submissions with three commits.
> >> >
> >> > I also still find the word 'die' confusing, since to me it suggests
> >> > aborting the whole am operation, and the documentation does not dispel
> >> > that concern. Even if you don't like 'ask' (for consistency with
> >> > git-rebase), I think 'stop' or 'interrupt' would be much better
> >> > options than 'die'. If you really want it to be 'die', I think the
> >> > behavior needs to be explained in the documentation, rather than just
> >> > assumed that users will understand it (because I didn't understand it
> >> > until I read the code).
> >>
> >> Dears Newren,
> >>
> >> I don't think 'stop' and 'interrupt' words are better to explain more than 'die'
> >> because they still indicate that it will stop or interrupt the whole am session,
> >> rather than stop in the middle of it.
> >
> > Since you've been through several rounds of revisions already, if this
> > is the only remaining issue with your series, I wouldn't try to hold
> > it up for this issue; I just thought it could be fixed while you were
> > working on the --allow-empty stuff.
>
> FWIW I think it's worth getting the UX issue right, tweaking it is
> relatively easy, and if we can decide on what the thing is called
> then...
:-)
> > However, while I don't think it's worth holding up your series for
> > just this issue, I would definitely submit a follow-up RFC patch to
> > fix the wording, because I do disagree with your assertion here pretty
> > strongly. Let's look at the meanings of the terms:
> >
> > die: connotes something pretty final and irreversible -- people tend
> > not to revive after death as far as recorded history goes; most such
> > recorded instances tend to be causes for people to debate the
> > definition of 'dead'.
> >
> > stop: could be final, but is often used in a transitory setting: "stop
> > and go traffic", "stopped to catch my breath", "the train will stop
> > at this station", "stop! I want to get out", etc.
> >
> > interrupt: seems to nearly always be used as a transitory thing
> >
> > Now, in the computer science context, all three terms come up relative
> > to processes. You can interrupt a process (the kernel does all the
> > time), but it'll usually continue afterwards. Or you can give it a
> > SIGINT (interrupt from keyboard signal), which the process can catch
> > and ignore. You can stop a process (and SIGSTOP cannot be caught),
> > but you can also continue it later. die essentially means the process
> > is going to be gone for good (at least short of some kind of black
> > magic like a reversible debugger such as rr).
> >
> > So, I think it's much more likely that 'die' will be misunderstood to
> > mean abortion of the entire am-process, than that 'stop' or
> > 'interrupt' would.
>
> Why are we exposing an --empty=die at all? It's what we do by default,
> so why have it? The user can just not provide the "--empty" option, then
> they'll get the current behavior of die_user_resolve(), which seems to
> have inpired the name for this "die" (it exits with code 128, just like
> die()).
That's an interesting angle to take; I hadn't thought of that. It's
worth considering.
We do often introduce options equivalent to the default as a way to
either countermand an earlier option (e.g. --do-walk overrides
--no-walk in git log), or because we want to allow new config options
that change the default while allowing the user to explicitly request
something different (e.g. --no-renames was introduced at the same time
as diff.renames), or because we may want to change the default
behavior and want users to be able to explicitly request a certain
type of behavior (e.g. rename detection is the default and
--no-renames overrides).
It's not clear to me whether that type of flexibility is needed or
whether we can just leave it unnamed. Three points that may affect
that decision: (a) the default (and actually, hardcoded) behavior
before this series for git-am was 'drop', (b) the default behavior
for git-rebase is 'drop' (though it only affects commits which become
empty, something we can't determine in the context of am) and (c)
there was one point during the series that the author asked about just
removing the 'die' implementation and picking a different default.
The above three points suggest to me that there might reasonably be
config added to control this or that the default could change, and
thus that it might be useful to name the interrupt-the-operation
behavior so that users can explicitly request it. But that's
somewhere around three levels of chained "might" conditions, so...
:shrug:
> Once we get rid of "die" the rest of the UI can follow the example of
> the existing "git rebase" options:
>
> --empty={drop,keep,ask}
>
> I.e. the "drop" and "keep" will be the same, no "ask" currently, but it
> can be implemented in the future.
Um, there are minor contextual differences, but what rebase calls
"ask" (interrupt the operation and tell the users what commands they
can use to keep or drop the commit and then resume the operation) _is_
implemented by this series -- it's just being called "die" here.
That's the kind of maddening inconsistency in Git that users complain
about that I really think we should avoid adding to. If for some
reason 'ask' from rebase seems like a bad choice, then I think we
should pick a new name for this interrupt-the-operation behavior that
makes sense (unlike 'die') for git-am and add it to git-rebase as a
preferred synonym to 'ask'.
> Maybe I'm missing something, I haven't used "am" in this way (or rebase
> with --empty=*), but this just seems to me to be needlessly exposing a
> "die" (or "stop" or whatever) because it's how we implement this.
>
> Whereas for the UX we don't need to call it anything except the absence
> of an --empty option, or perhaps --no-empty.
`--no-empty` would semantically be read by users to mean get rid of
empty commits, which would be a synonym of 'drop'. I think it'd be as
confusing as 'die' (and maybe even more so) for naming the
interrupt-the-operation behavior.
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: What's cooking in git.git (Nov 2021, #07; Mon, 29)
2021-12-03 19:28 ` Elijah Newren
@ 2021-12-03 19:56 ` Ævar Arnfjörð Bjarmason
2021-12-06 1:25 ` Aleen 徐沛文
0 siblings, 1 reply; 42+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-03 19:56 UTC (permalink / raw)
To: Elijah Newren
Cc: Aleen 徐沛文, Junio C Hamano, Git Mailing List,
徐沛文 (Aleen), Aleen via GitGitGadget
On Fri, Dec 03 2021, Elijah Newren wrote:
> On Fri, Dec 3, 2021 at 10:37 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>>
>> On Wed, Dec 01 2021, Elijah Newren wrote:
>>
>> > On Tue, Nov 30, 2021 at 5:42 PM Aleen 徐沛文 <pwxu@coremail.cn> wrote:
>> >>
>> >> > Please don't, at least not this version. There have been newer
>> >> > submissions with three commits.
>> >> >
>> >> > I also still find the word 'die' confusing, since to me it suggests
>> >> > aborting the whole am operation, and the documentation does not dispel
>> >> > that concern. Even if you don't like 'ask' (for consistency with
>> >> > git-rebase), I think 'stop' or 'interrupt' would be much better
>> >> > options than 'die'. If you really want it to be 'die', I think the
>> >> > behavior needs to be explained in the documentation, rather than just
>> >> > assumed that users will understand it (because I didn't understand it
>> >> > until I read the code).
>> >>
>> >> Dears Newren,
>> >>
>> >> I don't think 'stop' and 'interrupt' words are better to explain more than 'die'
>> >> because they still indicate that it will stop or interrupt the whole am session,
>> >> rather than stop in the middle of it.
>> >
>> > Since you've been through several rounds of revisions already, if this
>> > is the only remaining issue with your series, I wouldn't try to hold
>> > it up for this issue; I just thought it could be fixed while you were
>> > working on the --allow-empty stuff.
>>
>> FWIW I think it's worth getting the UX issue right, tweaking it is
>> relatively easy, and if we can decide on what the thing is called
>> then...
>
> :-)
>
>> > However, while I don't think it's worth holding up your series for
>> > just this issue, I would definitely submit a follow-up RFC patch to
>> > fix the wording, because I do disagree with your assertion here pretty
>> > strongly. Let's look at the meanings of the terms:
>> >
>> > die: connotes something pretty final and irreversible -- people tend
>> > not to revive after death as far as recorded history goes; most such
>> > recorded instances tend to be causes for people to debate the
>> > definition of 'dead'.
>> >
>> > stop: could be final, but is often used in a transitory setting: "stop
>> > and go traffic", "stopped to catch my breath", "the train will stop
>> > at this station", "stop! I want to get out", etc.
>> >
>> > interrupt: seems to nearly always be used as a transitory thing
>> >
>> > Now, in the computer science context, all three terms come up relative
>> > to processes. You can interrupt a process (the kernel does all the
>> > time), but it'll usually continue afterwards. Or you can give it a
>> > SIGINT (interrupt from keyboard signal), which the process can catch
>> > and ignore. You can stop a process (and SIGSTOP cannot be caught),
>> > but you can also continue it later. die essentially means the process
>> > is going to be gone for good (at least short of some kind of black
>> > magic like a reversible debugger such as rr).
>> >
>> > So, I think it's much more likely that 'die' will be misunderstood to
>> > mean abortion of the entire am-process, than that 'stop' or
>> > 'interrupt' would.
>>
>> Why are we exposing an --empty=die at all? It's what we do by default,
>> so why have it? The user can just not provide the "--empty" option, then
>> they'll get the current behavior of die_user_resolve(), which seems to
>> have inpired the name for this "die" (it exits with code 128, just like
>> die()).
>
> That's an interesting angle to take; I hadn't thought of that. It's
> worth considering.
>
> We do often introduce options equivalent to the default as a way to
> either countermand an earlier option (e.g. --do-walk overrides
> --no-walk in git log), or because we want to allow new config options
> that change the default while allowing the user to explicitly request
> something different (e.g. --no-renames was introduced at the same time
> as diff.renames), or because we may want to change the default
> behavior and want users to be able to explicitly request a certain
> type of behavior (e.g. rename detection is the default and
> --no-renames overrides).
>
> It's not clear to me whether that type of flexibility is needed or
> whether we can just leave it unnamed. Three points that may affect
> that decision: (a) the default (and actually, hardcoded) behavior
> before this series for git-am was 'drop', (b) the default behavior
> for git-rebase is 'drop' (though it only affects commits which become
> empty, something we can't determine in the context of am) and (c)
> there was one point during the series that the author asked about just
> removing the 'die' implementation and picking a different default.
>
> The above three points suggest to me that there might reasonably be
> config added to control this or that the default could change, and
> thus that it might be useful to name the interrupt-the-operation
> behavior so that users can explicitly request it. But that's
> somewhere around three levels of chained "might" conditions, so...
> :shrug:
>
>> Once we get rid of "die" the rest of the UI can follow the example of
>> the existing "git rebase" options:
>>
>> --empty={drop,keep,ask}
>>
>> I.e. the "drop" and "keep" will be the same, no "ask" currently, but it
>> can be implemented in the future.
>
> Um, there are minor contextual differences, but what rebase calls
> "ask" (interrupt the operation and tell the users what commands they
> can use to keep or drop the commit and then resume the operation) _is_
> implemented by this series -- it's just being called "die" here.
>
> That's the kind of maddening inconsistency in Git that users complain
> about that I really think we should avoid adding to. If for some
> reason 'ask' from rebase seems like a bad choice, then I think we
> should pick a new name for this interrupt-the-operation behavior that
> makes sense (unlike 'die') for git-am and add it to git-rebase as a
> preferred synonym to 'ask'.
>
>> Maybe I'm missing something, I haven't used "am" in this way (or rebase
>> with --empty=*), but this just seems to me to be needlessly exposing a
>> "die" (or "stop" or whatever) because it's how we implement this.
>>
>> Whereas for the UX we don't need to call it anything except the absence
>> of an --empty option, or perhaps --no-empty.
>
> `--no-empty` would semantically be read by users to mean get rid of
> empty commits, which would be a synonym of 'drop'. I think it'd be as
> confusing as 'die' (and maybe even more so) for naming the
> interrupt-the-operation behavior.
Ah, I didn't look into the finer details. Yes if it does maps to "ask"
in rebase we could just use that, so would this be consistent?:
--empty=die -> --empty=ask
--empty=drop -> (ditto, no change)
--empty=keep -> (ditto, no change)
I think "ask" is a bit of a weird term for this, but I think consistency
trumps a while-we're-at-it fix here.
Whatever new synonym we'd come up with (if that would be justified, that
itself would add to confusion) could be done as a follow-up and
implemented for both "rebase" and "am".
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: What's cooking in git.git (Nov 2021, #07; Mon, 29)
2021-12-03 19:56 ` Ævar Arnfjörð Bjarmason
@ 2021-12-06 1:25 ` Aleen 徐沛文
2021-12-06 6:28 ` Junio C Hamano
0 siblings, 1 reply; 42+ messages in thread
From: Aleen 徐沛文 @ 2021-12-06 1:25 UTC (permalink / raw)
To: Junio C Hamano
Cc: Elijah Newren, Junio C Hamano, Git Mailing List,
徐沛文 (Aleen), Aleen via GitGitGadget,
Johannes Schindelin, Ævar Arnfjörð Bjarmason
Dears Hamano,
In my opinion, the '--empty' should act as a strategy option like the
'X' option to 'git-rebase'. Since that the default behaviour of not passing
the option is stopped in the middle of an am session, should the 'die' value
dies the whole process but not the middle state? It may also make it not
confusing.
Aleen
> On Fri, Dec 03 2021, Elijah Newren wrote:
>
> > On Fri, Dec 3, 2021 at 10:37 AM Ævar Arnfjörð Bjarmason
> > <avarab@gmail.com> wrote:
> >>
> >> On Wed, Dec 01 2021, Elijah Newren wrote:
> >>
> >> > On Tue, Nov 30, 2021 at 5:42 PM Aleen 徐沛文 <pwxu@coremail.cn> wrote:
> >> >>
> >> >> > Please don't, at least not this version. There have been newer
> >> >> > submissions with three commits.
> >> >> >
> >> >> > I also still find the word 'die' confusing, since to me it suggests
> >> >> > aborting the whole am operation, and the documentation does not dispel
> >> >> > that concern. Even if you don't like 'ask' (for consistency with
> >> >> > git-rebase), I think 'stop' or 'interrupt' would be much better
> >> >> > options than 'die'. If you really want it to be 'die', I think the
> >> >> > behavior needs to be explained in the documentation, rather than just
> >> >> > assumed that users will understand it (because I didn't understand it
> >> >> > until I read the code).
> >> >>
> >> >> Dears Newren,
> >> >>
> >> >> I don't think 'stop' and 'interrupt' words are better to explain more than 'die'
> >> >> because they still indicate that it will stop or interrupt the whole am session,
> >> >> rather than stop in the middle of it.
> >> >
> >> > Since you've been through several rounds of revisions already, if this
> >> > is the only remaining issue with your series, I wouldn't try to hold
> >> > it up for this issue; I just thought it could be fixed while you were
> >> > working on the --allow-empty stuff.
> >>
> >> FWIW I think it's worth getting the UX issue right, tweaking it is
> >> relatively easy, and if we can decide on what the thing is called
> >> then...
> >
> > :-)
> >
> >> > However, while I don't think it's worth holding up your series for
> >> > just this issue, I would definitely submit a follow-up RFC patch to
> >> > fix the wording, because I do disagree with your assertion here pretty
> >> > strongly. Let's look at the meanings of the terms:
> >> >
> >> > die: connotes something pretty final and irreversible -- people tend
> >> > not to revive after death as far as recorded history goes; most such
> >> > recorded instances tend to be causes for people to debate the
> >> > definition of 'dead'.
> >> >
> >> > stop: could be final, but is often used in a transitory setting: "stop
> >> > and go traffic", "stopped to catch my breath", "the train will stop
> >> > at this station", "stop! I want to get out", etc.
> >> >
> >> > interrupt: seems to nearly always be used as a transitory thing
> >> >
> >> > Now, in the computer science context, all three terms come up relative
> >> > to processes. You can interrupt a process (the kernel does all the
> >> > time), but it'll usually continue afterwards. Or you can give it a
> >> > SIGINT (interrupt from keyboard signal), which the process can catch
> >> > and ignore. You can stop a process (and SIGSTOP cannot be caught),
> >> > but you can also continue it later. die essentially means the process
> >> > is going to be gone for good (at least short of some kind of black
> >> > magic like a reversible debugger such as rr).
> >> >
> >> > So, I think it's much more likely that 'die' will be misunderstood to
> >> > mean abortion of the entire am-process, than that 'stop' or
> >> > 'interrupt' would.
> >>
> >> Why are we exposing an --empty=die at all? It's what we do by default,
> >> so why have it? The user can just not provide the "--empty" option, then
> >> they'll get the current behavior of die_user_resolve(), which seems to
> >> have inpired the name for this "die" (it exits with code 128, just like
> >> die()).
> >
> > That's an interesting angle to take; I hadn't thought of that. It's
> > worth considering.
> >
> > We do often introduce options equivalent to the default as a way to
> > either countermand an earlier option (e.g. --do-walk overrides
> > --no-walk in git log), or because we want to allow new config options
> > that change the default while allowing the user to explicitly request
> > something different (e.g. --no-renames was introduced at the same time
> > as diff.renames), or because we may want to change the default
> > behavior and want users to be able to explicitly request a certain
> > type of behavior (e.g. rename detection is the default and
> > --no-renames overrides).
> >
> > It's not clear to me whether that type of flexibility is needed or
> > whether we can just leave it unnamed. Three points that may affect
> > that decision: (a) the default (and actually, hardcoded) behavior
> > before this series for git-am was 'drop', (b) the default behavior
> > for git-rebase is 'drop' (though it only affects commits which become
> > empty, something we can't determine in the context of am) and (c)
> > there was one point during the series that the author asked about just
> > removing the 'die' implementation and picking a different default.
> >
> > The above three points suggest to me that there might reasonably be
> > config added to control this or that the default could change, and
> > thus that it might be useful to name the interrupt-the-operation
> > behavior so that users can explicitly request it. But that's
> > somewhere around three levels of chained "might" conditions, so...
> > :shrug:
> >
> >> Once we get rid of "die" the rest of the UI can follow the example of
> >> the existing "git rebase" options:
> >>
> >> --empty={drop,keep,ask}
> >>
> >> I.e. the "drop" and "keep" will be the same, no "ask" currently, but it
> >> can be implemented in the future.
> >
> > Um, there are minor contextual differences, but what rebase calls
> > "ask" (interrupt the operation and tell the users what commands they
> > can use to keep or drop the commit and then resume the operation) _is_
> > implemented by this series -- it's just being called "die" here.
> >
> > That's the kind of maddening inconsistency in Git that users complain
> > about that I really think we should avoid adding to. If for some
> > reason 'ask' from rebase seems like a bad choice, then I think we
> > should pick a new name for this interrupt-the-operation behavior that
> > makes sense (unlike 'die') for git-am and add it to git-rebase as a
> > preferred synonym to 'ask'.
> >
> >> Maybe I'm missing something, I haven't used "am" in this way (or rebase
> >> with --empty=*), but this just seems to me to be needlessly exposing a
> >> "die" (or "stop" or whatever) because it's how we implement this.
> >>
> >> Whereas for the UX we don't need to call it anything except the absence
> >> of an --empty option, or perhaps --no-empty.
> >
> > `--no-empty` would semantically be read by users to mean get rid of
> > empty commits, which would be a synonym of 'drop'. I think it'd be as
> > confusing as 'die' (and maybe even more so) for naming the
> > interrupt-the-operation behavior.
>
> Ah, I didn't look into the finer details. Yes if it does maps to "ask"
> in rebase we could just use that, so would this be consistent?:
>
> --empty=die -> --empty=ask
> --empty=drop -> (ditto, no change)
> --empty=keep -> (ditto, no change)
>
> I think "ask" is a bit of a weird term for this, but I think consistency
> trumps a while-we're-at-it fix here.
>
> Whatever new synonym we'd come up with (if that would be justified, that
> itself would add to confusion) could be done as a follow-up and
> implemented for both "rebase" and "am".
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: What's cooking in git.git (Nov 2021, #07; Mon, 29)
2021-12-06 1:25 ` Aleen 徐沛文
@ 2021-12-06 6:28 ` Junio C Hamano
2021-12-06 6:44 ` Aleen 徐沛文
2021-12-06 17:37 ` Elijah Newren
0 siblings, 2 replies; 42+ messages in thread
From: Junio C Hamano @ 2021-12-06 6:28 UTC (permalink / raw)
To: Aleen 徐沛文
Cc: Elijah Newren, Git Mailing List, 徐沛文 (Aleen),
Aleen via GitGitGadget, Johannes Schindelin,
Ævar Arnfjörð Bjarmason
Aleen 徐沛文 <pwxu@coremail.cn> writes:
> Dears Hamano,
>
> In my opinion, the '--empty' should act as a strategy option like the
> 'X' option to 'git-rebase'. Since that the default behaviour of not passing
> the option is stopped in the middle of an am session, should the 'die' value
> dies the whole process but not the middle state? It may also make it not
> confusing.
Hmph, unlike "git rebase" or "git merge", "git am" does not employ
different strategy backends, so "-X<option>" would be out of place,
I would think.
Among what we already have, what kills the entire session is called
"git am --abort". Since I do not find it unnatural to say "'git am'
dies" when it stops and gives control back to the user, so that the
user can decide what to do next, I am not sure where the aversion to
the word comes from (on the other hand, I find 'ask' highly
unnatural since we do not ask anything---we just throw the control
back the user).
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: What's cooking in git.git (Nov 2021, #07; Mon, 29)
2021-12-06 6:28 ` Junio C Hamano
@ 2021-12-06 6:44 ` Aleen 徐沛文
2021-12-06 6:46 ` Aleen 徐沛文
2021-12-06 17:23 ` Junio C Hamano
2021-12-06 17:37 ` Elijah Newren
1 sibling, 2 replies; 42+ messages in thread
From: Aleen 徐沛文 @ 2021-12-06 6:44 UTC (permalink / raw)
To: Junio C Hamano
Cc: Elijah Newren, Git Mailing List, 徐沛文 (Aleen),
Aleen via GitGitGadget, Johannes Schindelin,
Ævar Arnfjörð Bjarmason
The confusing point seems why "git am" stops into the middle and gives
control back to the user when passing "die". In common cases, "die"
should stop the whole process, and is it better to distinguish the case
when passing "die" from the default behaviour? Like what the following
snippet is implemented:
case ERR_EMPTY_COMMIT:
printf_ln(_("Patch is empty.\n"
"If you want to record it as an empty commit, run \"git am --allow-empty\"."));
die_user_resolve(state);
case DIE_EMPTY_COMMIT:
am_destroy(state);
die(_("Patch is empty."));
break;
> > Dears Hamano,
> >
> > In my opinion, the '--empty' should act as a strategy option like the
> > 'X' option to 'git-rebase'. Since that the default behaviour of not passing
> > the option is stopped in the middle of an am session, should the 'die' value
> > dies the whole process but not the middle state? It may also make it not
> > confusing.
>
> Hmph, unlike "git rebase" or "git merge", "git am" does not employ
> different strategy backends, so "-X<option>" would be out of place,
> I would think.
>
> Among what we already have, what kills the entire session is called
> "git am --abort". Since I do not find it unnatural to say "'git am'
> dies" when it stops and gives control back to the user, so that the
> user can decide what to do next, I am not sure where the aversion to
> the word comes from (on the other hand, I find 'ask' highly
> unnatural since we do not ask anything---we just throw the control
> back the user).
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: What's cooking in git.git (Nov 2021, #07; Mon, 29)
2021-12-06 6:44 ` Aleen 徐沛文
@ 2021-12-06 6:46 ` Aleen 徐沛文
2021-12-06 17:23 ` Junio C Hamano
1 sibling, 0 replies; 42+ messages in thread
From: Aleen 徐沛文 @ 2021-12-06 6:46 UTC (permalink / raw)
To: Junio C Hamano
Cc: Elijah Newren, Git Mailing List, 徐沛文 (Aleen),
Aleen via GitGitGadget, Johannes Schindelin,
Ævar Arnfjörð Bjarmason
> case ERR_EMPTY_COMMIT:
> printf_ln(_("Patch is empty.\n"
> "If you want to record it as an empty commit, run \"git am --allow-empty\"."));
> die_user_resolve(state);
Sorry for the missing "break" here.
> case DIE_EMPTY_COMMIT:
> am_destroy(state);
> die(_("Patch is empty."));
> break;
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: What's cooking in git.git (Nov 2021, #07; Mon, 29)
2021-12-06 6:44 ` Aleen 徐沛文
2021-12-06 6:46 ` Aleen 徐沛文
@ 2021-12-06 17:23 ` Junio C Hamano
2021-12-07 1:06 ` Aleen 徐沛文
1 sibling, 1 reply; 42+ messages in thread
From: Junio C Hamano @ 2021-12-06 17:23 UTC (permalink / raw)
To: Aleen 徐沛文
Cc: Elijah Newren, Git Mailing List, 徐沛文 (Aleen),
Aleen via GitGitGadget, Johannes Schindelin,
Ævar Arnfjörð Bjarmason
Aleen 徐沛文 <pwxu@coremail.cn> writes:
> The confusing point seems why "git am" stops into the middle and gives
> control back to the user when passing "die". In common cases, "die"
> should stop the whole process, and is it better to distinguish the case
> when passing "die" from the default behaviour? Like what the following
> snippet is implemented:
>
> case ERR_EMPTY_COMMIT:
> printf_ln(_("Patch is empty.\n"
> "If you want to record it as an empty commit, run \"git am --allow-empty\"."));
> die_user_resolve(state);
> case DIE_EMPTY_COMMIT:
> am_destroy(state);
> die(_("Patch is empty."));
> break;
Sorry, but I am afraid that I still don't get it.
As we can see, the ERR_EMPTY_COMMIT case already exists and that is
the behaviour we want when we say "create commits from the messages
with patches, stop and give me control back when you see an empty
commit, so that I can decide what to do". And one of the things you
can do at that point is "am --abort" that causes the am_destroy() to
be called.
That is very much in line with the behaviour the users are used to
see from "git am" when it detects conditions other than "there is no
patch" that needs to stop and give control back to the user,
e.g. when the patch does not apply cleanly or the log message did
not pass msg hook. am_destroy() is destructive, and limiting such a
destructive operation to "am --abort" would avoid mistakes and
surprises. I do not know where you got "In common cases, die should
stop the whole" from, but it is not a friendly thing to do to our
users.
Why should the "in addition, stop when there is no patch", which is
what we already handle just fine, needs to become different? More
importantly, is it worth forcing the users to be aware of the
difference and be extra careful to avoid it?
It is my understanding that the ONLY reason the patch proposes to
add an option other than "skip the step" and "create an empty
commit" is to allow an earlier "--empty=skip" on the command line to
be overridden by a later "--empty=die". If that option does not
make the command behave identically to "--empty=<anything>" option
on the command line, i.e. ERR_EMPTY_COMMIT case, it does not serve
the intended purpose of overriding the earlier option to revert the
behaviour back to the default.
By the way, I agree with an earlier comment (I think it was from Dscho)
that these names should say "${DO_THIS}_ON_EMPTY_COMMIT"; the above
without "ON" feels somewhat strange.
Thanks.
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: What's cooking in git.git (Nov 2021, #07; Mon, 29)
2021-12-06 17:23 ` Junio C Hamano
@ 2021-12-07 1:06 ` Aleen 徐沛文
2021-12-07 1:29 ` Junio C Hamano
0 siblings, 1 reply; 42+ messages in thread
From: Aleen 徐沛文 @ 2021-12-07 1:06 UTC (permalink / raw)
To: Junio C Hamano
Cc: Elijah Newren, Git Mailing List, 徐沛文 (Aleen),
Aleen via GitGitGadget, Johannes Schindelin,
Ævar Arnfjörð Bjarmason
Dears Hamano,
I have doubted that since that the default behaviour is that stopping
when meeting commit messages lacking a patch and giving control back
to the user, is that necessary to provide duplicated '--empty=die'?
Should we just provide '--empty=(drop|keep)'?
Aleen
> Sorry, but I am afraid that I still don't get it.
>
> As we can see, the ERR_EMPTY_COMMIT case already exists and that is
> the behaviour we want when we say "create commits from the messages
> with patches, stop and give me control back when you see an empty
> commit, so that I can decide what to do". And one of the things you
> can do at that point is "am --abort" that causes the am_destroy() to
> be called.
>
> That is very much in line with the behaviour the users are used to
> see from "git am" when it detects conditions other than "there is no
> patch" that needs to stop and give control back to the user,
> e.g. when the patch does not apply cleanly or the log message did
> not pass msg hook. am_destroy() is destructive, and limiting such a
> destructive operation to "am --abort" would avoid mistakes and
> surprises. I do not know where you got "In common cases, die should
> stop the whole" from, but it is not a friendly thing to do to our
> users.
>
> Why should the "in addition, stop when there is no patch", which is
> what we already handle just fine, needs to become different? More
> importantly, is it worth forcing the users to be aware of the
> difference and be extra careful to avoid it?
>
> It is my understanding that the ONLY reason the patch proposes to
> add an option other than "skip the step" and "create an empty
> commit" is to allow an earlier "--empty=skip" on the command line to
> be overridden by a later "--empty=die". If that option does not
> make the command behave identically to "--empty=<anything>" option
> on the command line, i.e. ERR_EMPTY_COMMIT case, it does not serve
> the intended purpose of overriding the earlier option to revert the
> behaviour back to the default.
>
> By the way, I agree with an earlier comment (I think it was from Dscho)
> that these names should say "${DO_THIS}_ON_EMPTY_COMMIT"; the above
> without "ON" feels somewhat strange.
>
> Thanks.
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: What's cooking in git.git (Nov 2021, #07; Mon, 29)
2021-12-07 1:06 ` Aleen 徐沛文
@ 2021-12-07 1:29 ` Junio C Hamano
2021-12-07 1:58 ` Aleen 徐沛文
0 siblings, 1 reply; 42+ messages in thread
From: Junio C Hamano @ 2021-12-07 1:29 UTC (permalink / raw)
To: Aleen 徐沛文
Cc: Elijah Newren, Git Mailing List, 徐沛文 (Aleen),
Aleen via GitGitGadget, Johannes Schindelin,
Ævar Arnfjörð Bjarmason
Aleen 徐沛文 <pwxu@coremail.cn> writes:
>> It is my understanding that the ONLY reason the patch proposes to
>> add an option other than "skip the step" and "create an empty
>> commit" is to allow an earlier "--empty=skip" on the command line to
>> be overridden by a later "--empty=die". If that option does not
>> make the command behave identically to "--empty=<anything>" option
>> on the command line, i.e. ERR_EMPTY_COMMIT case, it does not serve
>> the intended purpose of overriding the earlier option to revert the
>> behaviour back to the default.
[jc: do not top-post, as people read from top to bottom]
> I have doubted that since that the default behaviour is that stopping
> when meeting commit messages lacking a patch and giving control back
> to the user, is that necessary to provide duplicated '--empty=die'?
> Should we just provide '--empty=(drop|keep)'?
Adding an option that aborts and trashes the state directory is much
worse than not having a choice other than drop and keep, which is in
turn a bit worse than having a way to countermand an option that was
given earlier on the command line.
If we were to have an option other than drop/keep, as Elijah
suggested, it may make sense to call it "stop" rather than "die" (I
think "ask" is a mistake, as we do not ask anything to the user).
>> By the way, I agree with an earlier comment (I think it was from Dscho)
>> that these names should say "${DO_THIS}_ON_EMPTY_COMMIT"; the above
>> without "ON" feels somewhat strange.
This still stands, too.
Thanks.
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: What's cooking in git.git (Nov 2021, #07; Mon, 29)
2021-12-07 1:29 ` Junio C Hamano
@ 2021-12-07 1:58 ` Aleen 徐沛文
0 siblings, 0 replies; 42+ messages in thread
From: Aleen 徐沛文 @ 2021-12-07 1:58 UTC (permalink / raw)
To: Junio C Hamano
Cc: Elijah Newren, Git Mailing List, 徐沛文 (Aleen),
Aleen via GitGitGadget, Johannes Schindelin,
Ævar Arnfjörð Bjarmason
> Adding an option that aborts and trashes the state directory is much
> worse than not having a choice other than drop and keep, which is in
> turn a bit worse than having a way to countermand an option that was
> given earlier on the command line.
Another option to countermand an option is only meaningful when it can be
applied as a resume value but not in such a case, I think. "--empty=<option>"
is only kept in each am session, and it seems we can't countermand the option
after the session.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: What's cooking in git.git (Nov 2021, #07; Mon, 29)
2021-12-06 6:28 ` Junio C Hamano
2021-12-06 6:44 ` Aleen 徐沛文
@ 2021-12-06 17:37 ` Elijah Newren
2021-12-06 17:50 ` Junio C Hamano
1 sibling, 1 reply; 42+ messages in thread
From: Elijah Newren @ 2021-12-06 17:37 UTC (permalink / raw)
To: Junio C Hamano
Cc: Aleen 徐沛文, Git Mailing List,
徐沛文 (Aleen), Aleen via GitGitGadget,
Johannes Schindelin, Ævar Arnfjörð Bjarmason
On Sun, Dec 5, 2021 at 10:28 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Aleen 徐沛文 <pwxu@coremail.cn> writes:
>
> > Dears Hamano,
> >
> > In my opinion, the '--empty' should act as a strategy option like the
> > 'X' option to 'git-rebase'. Since that the default behaviour of not passing
> > the option is stopped in the middle of an am session, should the 'die' value
> > dies the whole process but not the middle state? It may also make it not
> > confusing.
>
> Hmph, unlike "git rebase" or "git merge", "git am" does not employ
> different strategy backends, so "-X<option>" would be out of place,
> I would think.
>
> Among what we already have, what kills the entire session is called
> "git am --abort". Since I do not find it unnatural to say "'git am'
> dies" when it stops and gives control back to the user, so that the
> user can decide what to do next, I am not sure where the aversion to
> the word comes from
When I first read the documentation, it sounded to me like it was
implying an abort. I find 'die' very unnatural as a way to explain
this behavior; it's too strong of a word.
> (on the other hand, I find 'ask' highly
> unnatural since we do not ask anything---we just throw the control
> back the user).
Okay, but what about my previous suggestions of 'stop' or 'interrupt'?
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: What's cooking in git.git (Nov 2021, #07; Mon, 29)
2021-12-06 17:37 ` Elijah Newren
@ 2021-12-06 17:50 ` Junio C Hamano
0 siblings, 0 replies; 42+ messages in thread
From: Junio C Hamano @ 2021-12-06 17:50 UTC (permalink / raw)
To: Elijah Newren
Cc: Aleen 徐沛文, Git Mailing List,
徐沛文 (Aleen), Aleen via GitGitGadget,
Johannes Schindelin, Ævar Arnfjörð Bjarmason
Elijah Newren <newren@gmail.com> writes:
> When I first read the documentation, it sounded to me like it was
> implying an abort. I find 'die' very unnatural as a way to explain
> this behavior; it's too strong of a word.
The way we currently document "git am", we have no word "die" used
anywhere in the page. The existing mention of the behaviour are
found in these places:
--continue::
-r::
--resolved::
After a patch failure (e.g. attempting to apply
conflicting patch), the user has applied it by hand and
the index file stores the result of the application.
Make a commit using the authorship and commit log
extracted from the e-mail message and the current index
file, and continue.
This uses "failure", and "the user has applied" implies that the
user somehow got control back. We give "error:" messages to state
that the patch does not apply from apply.c::apply_all_patches() and
the caller silently exits, without calling die.
--show-current-patch[=(diff|raw)]::
Show the message at which `git am` has stopped due to
conflicts. If `raw` is specified, show the raw contents of
the e-mail message; if `diff`, show the diff portion only.
Defaults to `raw`.
This uses "has stopped" to describe the same situation.
>> (on the other hand, I find 'ask' highly
>> unnatural since we do not ask anything---we just throw the control
>> back the user).
>
> Okay, but what about my previous suggestions of 'stop' or 'interrupt'?
I agree that "stop" would be a good word that is already used to
describe "the command cannot make further progress without
assistance by the user, so it stops and gives control back".
After that, the user can say "am --skip", "am --abort", or edit plus
"am --continue".
Thanks.
^ permalink raw reply [flat|nested] 42+ messages in thread
* en/zdiff3 (was: Re: What's cooking in git.git (Nov 2021, #07; Mon, 29))
2021-11-30 2:16 What's cooking in git.git (Nov 2021, #07; Mon, 29) Junio C Hamano
` (8 preceding siblings ...)
2021-11-30 23:45 ` Elijah Newren
@ 2021-11-30 23:52 ` Elijah Newren
2021-12-01 22:15 ` en/zdiff3 Junio C Hamano
2021-12-01 8:59 ` What's cooking in git.git (Nov 2021, #07; Mon, 29) Fabian Stelzer
` (2 subsequent siblings)
12 siblings, 1 reply; 42+ messages in thread
From: Elijah Newren @ 2021-11-30 23:52 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List
On Tue, Nov 30, 2021 at 6:49 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> * en/zdiff3 (2021-09-20) 2 commits
> - update documentation for new zdiff3 conflictStyle
> - xdiff: implement a zealous diff3, or "zdiff3"
>
> "Zealous diff3" style of merge conflict presentation has been added.
>
> What's the status of this thing?
I resubmitted it before you added this question, and have pointed out
the resubmission in response to the past "What's cooking" emails, but
perhaps it'll be easier for me to just resubmit with no changes. I'll
do that.
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: en/zdiff3
2021-11-30 23:52 ` en/zdiff3 (was: Re: What's cooking in git.git (Nov 2021, #07; Mon, 29)) Elijah Newren
@ 2021-12-01 22:15 ` Junio C Hamano
0 siblings, 0 replies; 42+ messages in thread
From: Junio C Hamano @ 2021-12-01 22:15 UTC (permalink / raw)
To: Elijah Newren; +Cc: Git Mailing List
Elijah Newren <newren@gmail.com> writes:
> On Tue, Nov 30, 2021 at 6:49 AM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> * en/zdiff3 (2021-09-20) 2 commits
>> - update documentation for new zdiff3 conflictStyle
>> - xdiff: implement a zealous diff3, or "zdiff3"
>>
>> "Zealous diff3" style of merge conflict presentation has been added.
>>
>> What's the status of this thing?
>
> I resubmitted it before you added this question, and have pointed out
> the resubmission in response to the past "What's cooking" emails, but
> perhaps it'll be easier for me to just resubmit with no changes. I'll
> do that.
Thanks. Yes, for anything that is older than a month, it always is
easier that way.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: What's cooking in git.git (Nov 2021, #07; Mon, 29)
2021-11-30 2:16 What's cooking in git.git (Nov 2021, #07; Mon, 29) Junio C Hamano
` (9 preceding siblings ...)
2021-11-30 23:52 ` en/zdiff3 (was: Re: What's cooking in git.git (Nov 2021, #07; Mon, 29)) Elijah Newren
@ 2021-12-01 8:59 ` Fabian Stelzer
2021-12-03 1:12 ` Junio C Hamano
2021-12-03 5:10 ` [PATCH 0/3] unused-parameter cleanups on top of pw/xdiff-classify-record-in-histogram Jeff King
12 siblings, 0 replies; 42+ messages in thread
From: Fabian Stelzer @ 2021-12-01 8:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
On 29.11.2021 18:16, Junio C Hamano wrote:
>
> * fs/ssh-signing-other-keytypes (2021-11-19) 2 commits
> - ssh signing: make sign/amend test more resilient
> - ssh signing: support non ssh-* keytypes
>
I will send a rebased version of this one on top of
fs/ssh-signing-key-lifetime. Otherwise those two will result in a
(simple) merge conflict in the test prereqs.
>
> * fs/ssh-signing-key-lifetime (2021-11-18) 10 commits
> - ssh signing: verify ssh-keygen in test prereq
> - ssh signing: make fmt-merge-msg consider key lifetime
> - ssh signing: make verify-tag consider key lifetime
> - ssh signing: make git log verify key lifetime
> - ssh signing: make verify-commit consider key lifetime
> - ssh signing: add key lifetime test prereqs
> - ssh signing: use sigc struct to pass payload
> - Merge branch 'ad/ssh-signing-testfix' into fs/ssh-signing-key-lifetime
> - Merge branch 'fs/ssh-signing-fix' into fs/ssh-signing-key-lifetime
> - Merge branch 'fs/ssh-signing' into fs/ssh-signing-key-lifetime
>
> Extend the signing of objects with SSH keys and learn to pay
> attention to the key validity time range when verifying.
>
> Will merge to 'next'?
>
I just resubmitted a new version which cleans up the prereq setup.
Thanks
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: What's cooking in git.git (Nov 2021, #07; Mon, 29)
2021-11-30 2:16 What's cooking in git.git (Nov 2021, #07; Mon, 29) Junio C Hamano
` (10 preceding siblings ...)
2021-12-01 8:59 ` What's cooking in git.git (Nov 2021, #07; Mon, 29) Fabian Stelzer
@ 2021-12-03 1:12 ` Junio C Hamano
2021-12-03 5:10 ` [PATCH 0/3] unused-parameter cleanups on top of pw/xdiff-classify-record-in-histogram Jeff King
12 siblings, 0 replies; 42+ messages in thread
From: Junio C Hamano @ 2021-12-03 1:12 UTC (permalink / raw)
To: git
As of this evening, my tree is not yet in a good enough shape to
issue the next iteration of "What's cooking" report, but I've
managed to merge some topics to 'next'. No, nothing exciting is in
there. This first batch of topics consists of stuff that would have
been in 'next' or even in the release if the previous cycle were
longer by a week or two.
I plan to reserve time for picking and trial-merging other topics
that we want to have in 'next' tomorrow, so that I can finish the
"What's cooking" report with more topics marked for 'next'.
To make sure I can make progress, I'll promise that I will not look
at or comment on patches that are sent to the list tomorrow (or in
the coming few days), except that a reroll of an existing topic will
be taken as a sign that the stale version I have in my tree needs to
be replaced before it can be merged to 'next' (but I won't pick the
new iteration up---I know from experience that it would stall and
delay me).
^ permalink raw reply [flat|nested] 42+ messages in thread* [PATCH 0/3] unused-parameter cleanups on top of pw/xdiff-classify-record-in-histogram
2021-11-30 2:16 What's cooking in git.git (Nov 2021, #07; Mon, 29) Junio C Hamano
` (11 preceding siblings ...)
2021-12-03 1:12 ` Junio C Hamano
@ 2021-12-03 5:10 ` Jeff King
2021-12-03 5:11 ` [PATCH 1/3] xdiff: drop CMP_ENV macro from xhistogram Jeff King
` (3 more replies)
12 siblings, 4 replies; 42+ messages in thread
From: Jeff King @ 2021-12-03 5:10 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Phillip Wood, git
On Mon, Nov 29, 2021 at 06:16:54PM -0800, Junio C Hamano wrote:
> * pw/xdiff-classify-record-in-histogram (2021-11-18) 3 commits
> - xdiff: simplify comparison
> - xdiff: avoid unnecessary memory allocations
> - diff histogram: intern strings
>
> "diff --histogram" optimization.
>
> Will merge to 'next'.
This being merged to 'next' tickled my -Wunused-parameter topic, so
there were a few more opportunities for cleanup.
Definitely not urgent, but I think worth doing. Prepared on top of the
topic branch.
[1/3]: xdiff: drop CMP_ENV macro from xhistogram
[2/3]: xdiff: drop xpparam_t parameter from histogram cmp_recs()
[3/3]: xdiff: drop unused flags parameter from recs_match
xdiff/xdiffi.c | 18 +++++++++---------
xdiff/xhistogram.c | 8 ++------
2 files changed, 11 insertions(+), 15 deletions(-)
-Peff
^ permalink raw reply [flat|nested] 42+ messages in thread* [PATCH 1/3] xdiff: drop CMP_ENV macro from xhistogram
2021-12-03 5:10 ` [PATCH 0/3] unused-parameter cleanups on top of pw/xdiff-classify-record-in-histogram Jeff King
@ 2021-12-03 5:11 ` Jeff King
2021-12-03 5:11 ` [PATCH 2/3] xdiff: drop xpparam_t parameter from histogram cmp_recs() Jeff King
` (2 subsequent siblings)
3 siblings, 0 replies; 42+ messages in thread
From: Jeff King @ 2021-12-03 5:11 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Phillip Wood, git
This macro has been unused since 43ca7530df (xdiff/xhistogram: rely on
xdl_trim_ends(), 2011-08-01). The function that called it went away
there, and its use in the CMP() macro was inlined. It probably should
have been deleted then, but nobody noticed.
Signed-off-by: Jeff King <peff@peff.net>
---
This one isn't strictly enabled by Phillip's new patches, but it cleans
up an unused caller of cmp_recs() so we don't have to worry about it in
the next patch.
xdiff/xhistogram.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/xdiff/xhistogram.c b/xdiff/xhistogram.c
index 6c1c88a69a..399cc236d7 100644
--- a/xdiff/xhistogram.c
+++ b/xdiff/xhistogram.c
@@ -95,9 +95,6 @@ static int cmp_recs(xpparam_t const *xpp,
}
-#define CMP_ENV(xpp, env, s1, l1, s2, l2) \
- (cmp_recs(xpp, REC(env, s1, l1), REC(env, s2, l2)))
-
#define CMP(i, s1, l1, s2, l2) \
(cmp_recs(i->xpp, REC(i->env, s1, l1), REC(i->env, s2, l2)))
--
2.34.1.436.g8c445b282e
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH 2/3] xdiff: drop xpparam_t parameter from histogram cmp_recs()
2021-12-03 5:10 ` [PATCH 0/3] unused-parameter cleanups on top of pw/xdiff-classify-record-in-histogram Jeff King
2021-12-03 5:11 ` [PATCH 1/3] xdiff: drop CMP_ENV macro from xhistogram Jeff King
@ 2021-12-03 5:11 ` Jeff King
2021-12-03 5:12 ` [PATCH 3/3] xdiff: drop unused flags parameter from recs_match Jeff King
2021-12-06 18:59 ` [PATCH 0/3] unused-parameter cleanups on top of pw/xdiff-classify-record-in-histogram Phillip Wood
3 siblings, 0 replies; 42+ messages in thread
From: Jeff King @ 2021-12-03 5:11 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Phillip Wood, git
Since 663c5ad035 (diff histogram: intern strings, 2021-11-17), our
cmp_recs() does not call xdl_recmatch(), and thus no longer needs an
xpparam_t struct from which to get the flags. We can drop the unused
parameter from the function, as well as the macro which wraps it.
There's no functional change here; it's just simplifying things (and
making -Wunused-parameter happier).
Signed-off-by: Jeff King <peff@peff.net>
---
xdiff/xhistogram.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/xdiff/xhistogram.c b/xdiff/xhistogram.c
index 399cc236d7..80794748b0 100644
--- a/xdiff/xhistogram.c
+++ b/xdiff/xhistogram.c
@@ -88,15 +88,14 @@ struct region {
#define REC(env, s, l) \
(env->xdf##s.recs[l - 1])
-static int cmp_recs(xpparam_t const *xpp,
- xrecord_t *r1, xrecord_t *r2)
+static int cmp_recs(xrecord_t *r1, xrecord_t *r2)
{
return r1->ha == r2->ha;
}
#define CMP(i, s1, l1, s2, l2) \
- (cmp_recs(i->xpp, REC(i->env, s1, l1), REC(i->env, s2, l2)))
+ (cmp_recs(REC(i->env, s1, l1), REC(i->env, s2, l2)))
#define TABLE_HASH(index, side, line) \
XDL_HASHLONG((REC(index->env, side, line))->ha, index->table_bits)
--
2.34.1.436.g8c445b282e
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH 3/3] xdiff: drop unused flags parameter from recs_match
2021-12-03 5:10 ` [PATCH 0/3] unused-parameter cleanups on top of pw/xdiff-classify-record-in-histogram Jeff King
2021-12-03 5:11 ` [PATCH 1/3] xdiff: drop CMP_ENV macro from xhistogram Jeff King
2021-12-03 5:11 ` [PATCH 2/3] xdiff: drop xpparam_t parameter from histogram cmp_recs() Jeff King
@ 2021-12-03 5:12 ` Jeff King
2021-12-06 18:59 ` [PATCH 0/3] unused-parameter cleanups on top of pw/xdiff-classify-record-in-histogram Phillip Wood
3 siblings, 0 replies; 42+ messages in thread
From: Jeff King @ 2021-12-03 5:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Phillip Wood, git
Since 6b13bc3232 (xdiff: simplify comparison, 2021-11-17), we do not
call xdl_recmatch() from xdiffi.c's recs_match(), so we no longer need
the "flags" parameter. That in turn lets us drop the flags parameters
from the group-slide functions which call it.
There's no functional change here; it's just making these functions a
little simpler.
Signed-off-by: Jeff King <peff@peff.net>
---
xdiff/xdiffi.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c
index 6a3b9280be..69689fab24 100644
--- a/xdiff/xdiffi.c
+++ b/xdiff/xdiffi.c
@@ -390,7 +390,7 @@ static xdchange_t *xdl_add_change(xdchange_t *xscr, long i1, long i2, long chg1,
}
-static int recs_match(xrecord_t *rec1, xrecord_t *rec2, long flags)
+static int recs_match(xrecord_t *rec1, xrecord_t *rec2)
{
return (rec1->ha == rec2->ha);
}
@@ -756,10 +756,10 @@ static inline int group_previous(xdfile_t *xdf, struct xdlgroup *g)
* following group, expand this group to include it. Return 0 on success or -1
* if g cannot be slid down.
*/
-static int group_slide_down(xdfile_t *xdf, struct xdlgroup *g, long flags)
+static int group_slide_down(xdfile_t *xdf, struct xdlgroup *g)
{
if (g->end < xdf->nrec &&
- recs_match(xdf->recs[g->start], xdf->recs[g->end], flags)) {
+ recs_match(xdf->recs[g->start], xdf->recs[g->end])) {
xdf->rchg[g->start++] = 0;
xdf->rchg[g->end++] = 1;
@@ -777,10 +777,10 @@ static int group_slide_down(xdfile_t *xdf, struct xdlgroup *g, long flags)
* into a previous group, expand this group to include it. Return 0 on success
* or -1 if g cannot be slid up.
*/
-static int group_slide_up(xdfile_t *xdf, struct xdlgroup *g, long flags)
+static int group_slide_up(xdfile_t *xdf, struct xdlgroup *g)
{
if (g->start > 0 &&
- recs_match(xdf->recs[g->start - 1], xdf->recs[g->end - 1], flags)) {
+ recs_match(xdf->recs[g->start - 1], xdf->recs[g->end - 1])) {
xdf->rchg[--g->start] = 1;
xdf->rchg[--g->end] = 0;
@@ -830,7 +830,7 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {
end_matching_other = -1;
/* Shift the group backward as much as possible: */
- while (!group_slide_up(xdf, &g, flags))
+ while (!group_slide_up(xdf, &g))
if (group_previous(xdfo, &go))
BUG("group sync broken sliding up");
@@ -845,7 +845,7 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {
/* Now shift the group forward as far as possible: */
while (1) {
- if (group_slide_down(xdf, &g, flags))
+ if (group_slide_down(xdf, &g))
break;
if (group_next(xdfo, &go))
BUG("group sync broken sliding down");
@@ -872,7 +872,7 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {
* other file that it can align with.
*/
while (go.end == go.start) {
- if (group_slide_up(xdf, &g, flags))
+ if (group_slide_up(xdf, &g))
BUG("match disappeared");
if (group_previous(xdfo, &go))
BUG("group sync broken sliding to match");
@@ -915,7 +915,7 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {
}
while (g.end > best_shift) {
- if (group_slide_up(xdf, &g, flags))
+ if (group_slide_up(xdf, &g))
BUG("best shift unreached");
if (group_previous(xdfo, &go))
BUG("group sync broken sliding to blank line");
--
2.34.1.436.g8c445b282e
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH 0/3] unused-parameter cleanups on top of pw/xdiff-classify-record-in-histogram
2021-12-03 5:10 ` [PATCH 0/3] unused-parameter cleanups on top of pw/xdiff-classify-record-in-histogram Jeff King
` (2 preceding siblings ...)
2021-12-03 5:12 ` [PATCH 3/3] xdiff: drop unused flags parameter from recs_match Jeff King
@ 2021-12-06 18:59 ` Phillip Wood
3 siblings, 0 replies; 42+ messages in thread
From: Phillip Wood @ 2021-12-06 18:59 UTC (permalink / raw)
To: Jeff King, Junio C Hamano; +Cc: Phillip Wood, git
Hi Peff
On 03/12/2021 05:10, Jeff King wrote:
> On Mon, Nov 29, 2021 at 06:16:54PM -0800, Junio C Hamano wrote:
>
>> * pw/xdiff-classify-record-in-histogram (2021-11-18) 3 commits
>> - xdiff: simplify comparison
>> - xdiff: avoid unnecessary memory allocations
>> - diff histogram: intern strings
>>
>> "diff --histogram" optimization.
>>
>> Will merge to 'next'.
>
> This being merged to 'next' tickled my -Wunused-parameter topic, so
> there were a few more opportunities for cleanup.
>
> Definitely not urgent, but I think worth doing. Prepared on top of the
> topic branch.
>
> [1/3]: xdiff: drop CMP_ENV macro from xhistogram
> [2/3]: xdiff: drop xpparam_t parameter from histogram cmp_recs()
> [3/3]: xdiff: drop unused flags parameter from recs_match
Thanks, all three patches look good to me
Best Wishes
Phillip
> xdiff/xdiffi.c | 18 +++++++++---------
> xdiff/xhistogram.c | 8 ++------
> 2 files changed, 11 insertions(+), 15 deletions(-)
>
> -Peff
>
^ permalink raw reply [flat|nested] 42+ messages in thread