* Re: [RESEND v2] git-rebase.txt: rewrite docu for fixup/squash (again)
From: Phillip Wood @ 2023-10-24 9:22 UTC (permalink / raw)
To: Oswald Buddenhagen, phillip.wood
Cc: git, Junio C Hamano, Christian Couder, Charvi Mendiratta,
Marc Branchaud, Johannes Sixt
In-Reply-To: <ZTayxB0Nm7AEyafp@ugly>
Hi Oswald
On 23/10/2023 18:52, Oswald Buddenhagen wrote:
> On Mon, Oct 23, 2023 at 05:01:02PM +0100, Phillip Wood wrote:
>> On 23/10/2023 14:00, Oswald Buddenhagen wrote:
>>> +unless "fixup -c" is used. In the latter case, the message is obtained
>>> +only from the "fixup -c" commit (having more than one of these is
>>> +incorrect).
>>
>> This change is incorrect - it is perfectly fine to have more than one
>> "fixup -c" command. In that case we use the message of the commit of
>> the final "fixup -c" command.
>>
> i know that this is the case, see the previous thread (which i failed to
> link by header, cf.
> https://lore.kernel.org/all/20231020092707.917514-1-oswald.buddenhagen@gmx.de/T/#u ).
Ah, I see Marc has already raised this point.
>> One case where there can be multiple "fixup -c" commands is when a
>> commit has been reworded several times via "git commit
>> --fixup=reword:<commit>" and the user runs "git rebase --autosquash"
>>
> a cleaner solution would be recognizing the situation and not generating
> these contradicting commands in the first place. of course that would be
> more complexity, but it would also allow catching accidental use.
>
> of course i can go back to documenting the status quo, but it seems kind
> of wrong.
I agree there is an argument for improving the implementation of
--autosquash but until we do I think it is counterproductive to change
the documentation like this as it will cause users to wonder why "rebase
--autosquash" generates a todo list that is incorrect according to the
documentation.
>> In the case of
>>
>> pick A
>> fixup -C B
>>
>> don't we keep the authorship from A and just use the commit message
>> from B?
>>
> uhm. we clearly do. that means i was given incorrect advice in
> https://lore.kernel.org/all/YjXRM5HiRizZ035p@ugly/T/#u (and so the
> thread is still looking for a resolution) ...
I'll take a look at that thread and comment there.
I do think it is a good idea to document where the authorship of a
rebased commit comes from.
Best Wishes
Phillip
^ permalink raw reply
* [OUTREACHY] Final Application For Git Internshhip
From: Naomi Ibe @ 2023-10-24 7:47 UTC (permalink / raw)
To: git
I'm Ibe Naomi Amarachi,I'm a Nigerian and I currently live in
Lagos,Nigeria. I'm a graduate of the African Leadership Xcelerator
Software Engineering program (ALX SE), with specialization in backend
web development and I'm applying for the "Moving Existing Tests to a
Unit Testing Framework" project
Some of my projects that involve working with Shell,C and Git can be found here:
https://github.com/Amarajah/alx-system_engineering-devops
https://github.com/Amarajah/alx-low_level_programming
Git has been a part of my software engineering journey from day one
and it's allowed me to collaborate with peers and also keep track of
my personal projects. Currently Git uses end-to-end tests for error
conditions that could easily be captured by unit tests, the project is
aimed at turning end-to-end tests to unit tests, and I'd love to be a
part of it
My microproject contribution is here:
https://public-inbox.org/git/20231009011652.1791-1-naomi.ibeh69@gmail.com/T/#u
And here is my updated contribution link after review by the Git community:
https://public-inbox.org/git/xmqqttqox5cp.fsf@gitster.g/T/#u
Below is my project timeline:
(Of course I'd be very much willing to work with the community and
mentors to edit it so it perfectly meets up to the community's
expectations)
October 2, 2023 - October 30, 2023
Familiarizing myself with the community , mailing list and
contributing my microproject
November 20, 2023 - December 4, 2023
Familiarizing myself with the already existing tests and also the
chosen unit test framework
Do more research on the internship projects and find better ways to
get it done in harmony with coding guidelines and community
requirements
December 4, 2023 - December 31, 2023
First document the initial state of the test files, then make sure all
test files conform to coding guidelines down to the tiniest details
(e.g git/t/helper/test-write-cache.c and git/t/helper/test-advise.c
have die() messages which do not conform to coding guidelines)
January 2, 2024 - January 31, 2024
Run the tests and verify they still work as originally intended
Begin migrations of test files
Test migrated files and make necessary changes based on feedback
received from teammates and mentors
February 1, 2024 - March 1, 2024
Continue testing migrated files and making necessary changes based on
feedback received from teammates and mentors
Document each step and request for reviews from teammates and mentors
Tidy up the project,make sure all necessary files are migrated, they
all work as intended, they are well documented and that there are no
conflicts
^ permalink raw reply
* Re: [PATCH 08/11] t4207: delete replace references via git-update-ref(1)
From: Patrick Steinhardt @ 2023-10-24 6:42 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Han-Wen Nienhuys
In-Reply-To: <ZTaiYEyhKT/yZwHZ@nand.local>
[-- Attachment #1: Type: text/plain, Size: 3339 bytes --]
On Mon, Oct 23, 2023 at 12:42:08PM -0400, Taylor Blau wrote:
> On Wed, Oct 18, 2023 at 07:35:37AM +0200, Patrick Steinhardt wrote:
> > In t4207 we set up a set of replace objects via git-replace(1). Because
> > these references should not be impacting subsequent tests we also set up
> > some cleanup logic that deletes the replacement references via a call to
> > `rm -rf`. This reaches into the internal implementation details of the
> > reference backend and will thus break when we grow an alternative refdb
> > implementation.
> >
> > Refactor the tests to delete the replacement refs via Git commands so
> > that we become independent of the actual refdb that's in use. As we
> > don't have a nice way to delete all replacements or all references in a
> > certain namespace, we opt for a combination of git-for-each-ref(1) and
> > git-update-ref(1)'s `--stdin` mode.
> >
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> > t/t4207-log-decoration-colors.sh | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/t/t4207-log-decoration-colors.sh b/t/t4207-log-decoration-colors.sh
> > index 21986a866df..d138e513a04 100755
> > --- a/t/t4207-log-decoration-colors.sh
> > +++ b/t/t4207-log-decoration-colors.sh
> > @@ -71,7 +71,7 @@ ${c_tag}tag: ${c_reset}${c_tag}A${c_reset}${c_commit})${c_reset} A
> > '
> >
> > test_expect_success 'test coloring with replace-objects' '
> > - test_when_finished rm -rf .git/refs/replace* &&
> > + test_when_finished "git for-each-ref refs/replace*/** --format=${SQ}delete %(refname)${SQ} | git update-ref --stdin" &&
>
> Here and below, should we avoid the for-each-ref showing up on the
> left-hand side of the pipe? I'd think we want something closer to:
>
> test_when_finished "git for-each-ref refs/replace*/** --format=${SQ}delete %(refname)${SQ} >in && git update-ref --stdin <in" &&
>
> But having to quote the --format argument with "${SQ}"s makes the whole
> thing a little awkward to read and parse.
>
> Do you think that something like the below would be a readability
> improvement?
Yes, this certainly looks like a good improvement to me, thanks!
Patrick
> diff --git a/t/t4207-log-decoration-colors.sh b/t/t4207-log-decoration-colors.sh
> index d138e513a0..de8f6638cb 100755
> --- a/t/t4207-log-decoration-colors.sh
> +++ b/t/t4207-log-decoration-colors.sh
> @@ -70,8 +70,13 @@ ${c_tag}tag: ${c_reset}${c_tag}A${c_reset}${c_commit})${c_reset} A
> cmp_filtered_decorations
> '
>
> --- >8 ---
> +remove_replace_refs () {
> + git for-each-ref 'refs/replace*/**' --format='delete %(refname)' >in &&
> + git update-ref --stdin <in
> +}
> +
> test_expect_success 'test coloring with replace-objects' '
> - test_when_finished "git for-each-ref refs/replace*/** --format=${SQ}delete %(refname)${SQ} | git update-ref --stdin" &&
> + test_when_finished remove_replace_refs &&
> test_commit C &&
> test_commit D &&
>
> @@ -99,7 +104,7 @@ EOF
> '
>
> test_expect_success 'test coloring with grafted commit' '
> - test_when_finished "git for-each-ref refs/replace*/** --format=${SQ}delete %(refname)${SQ} | git update-ref --stdin" &&
> + test_when_finished remove_replace_refs &&
>
> git replace --graft HEAD HEAD~2 &&
> --- 8< ---
>
> Thanks,
> Taylor
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v4 4/7] bulk-checkin: implement `SOURCE_INCORE` mode for `bulk_checkin_source`
From: Patrick Steinhardt @ 2023-10-24 6:34 UTC (permalink / raw)
To: Jeff King
Cc: Taylor Blau, git, Elijah Newren, Eric W. Biederman,
Junio C Hamano
In-Reply-To: <20231023185842.GE1537181@coredump.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 1213 bytes --]
On Mon, Oct 23, 2023 at 02:58:42PM -0400, Jeff King wrote:
> On Mon, Oct 23, 2023 at 11:19:13AM +0200, Patrick Steinhardt wrote:
>
> > > + case SOURCE_INCORE:
> > > + assert(source->read <= source->size);
> >
> > Is there any guideline around when to use `assert()` vs `BUG()`? I think
> > that this assertion here is quite critical, because when it does not
> > hold we can end up performing out-of-bounds reads and writes. But as
> > asserts are typically missing in non-debug builds, this safeguard would
> > not do anything for our end users, right?
>
> I don't think we have a written guideline. My philosophy is: always use
> BUG(), because you will never be surprised that the assertion was not
> compiled in (and I think compiling without assertions is almost
> certainly premature optimization, especially given the way we tend to
> use them).
>
> -Peff
I'm inclined to agree with your philosophy. Makes me wonder whether we
should write a Coccinelle rule to catch this. But a quick-and-dirty grep
in our codebase shows that such a rule would cause quite a lot of churn:
$ git grep BUG\( | wc -l
677
$ git grep assert\( | wc -l
549
Probably not worth it.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [RFC PATCH 0/5] Introduce -t, --table for status/add commands
From: Dragan Simic @ 2023-10-24 2:21 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jacob Stopak, Oswald Buddenhagen, git
In-Reply-To: <xmqqil6w6al3.fsf@gitster.g>
On 2023-10-24 04:03, Junio C Hamano wrote:
> I think the "use more verbose report format to help relatively
> inexperienced folks, in exchange for spending more screen real
> estate" is a good direction to think about this thing.
>
> I am not personally interested in adding such support all that much
> myself, but one piece of advice I can offer those who are interested
> is not to be too deeply attached to the word "table".
>
> ... snip ...
>
> So be very careful when choosing what to call this new thing, and
> avoid naming it after the implementation details (e.g., in what
> particular shape the data gets presented) that may turn out not to
> be the most important part of the concept.
Totally agreed, "table" simply sneaked in and remained here as the term.
Perhaps "<command>.verbose = extra" or something similar would be a
good choice.
> [Footnote]
>
> * FWIW, "git status -s" is a tabular presentation. Maybe we can
> add a more verbose form of "-s" and be done with it for the
> command?
That's also an option.
^ permalink raw reply
* Re: [RFC PATCH 0/5] Introduce -t, --table for status/add commands
From: Junio C Hamano @ 2023-10-24 2:03 UTC (permalink / raw)
To: Dragan Simic; +Cc: Jacob Stopak, Oswald Buddenhagen, git
In-Reply-To: <62164acf4a787042dbb6e5abe212559b@manjaro.org>
Dragan Simic <dsimic@manjaro.org> writes:
> IMHO, it would be the best to simply implement support for
> "<command>.verbose = table" in the git configuration, similarly to how
> we already have "commit.verbose = true". That way tables could be
> enabled per command, according to the user's preferences, regardless
> of performing dry runs or not.
I think the "use more verbose report format to help relatively
inexperienced folks, in exchange for spending more screen real
estate" is a good direction to think about this thing.
I am not personally interested in adding such support all that much
myself, but one piece of advice I can offer those who are interested
is not to be too deeply attached to the word "table".
It may be that "git status" (not "status -s" [*] but the current
version for human consumption) shows "paths with changes to be
committed (i.e. changes added to the index exist)" and "paths with
changes you could add to the index (i.e. changes yet to be added to
the index exist)" in a separate list, and Jacob may have found that
it gives a more understandable view into the states of each path if
the output is turned 90-degrees and the changes are shown in a
tabular form. But "table" in this example is merely an
implementation detail of one presentation that is easier to
understand for "git status", and calling it "table" and making it a
word in the vocabulary of <command>.verbose is like a tail wagging
the dog. We want to convey to the users that the option is about
"with extra verbosity, the user is buying a bit more clarity", not
necessarily "use tabular form no matter what".
For some of the commands, tabular presentation might not even be the
presentation form that is the easiest to understand to novices. For
example, I just pushed out today's integration result to some of my
repositories, and "git push" output looks like so:
To github.com:gitster/git.git
+ 5cb4030332...7dc6f5ada8 ak/color-decorate-symbols -> ak/colo...
+ a71066b71b...c80a646458 jch -> jch (forced update)
+ 89a1ffc6a4...416cdf7260 seen -> seen (forced update)
+ 7ff160463b...2c610ca9ff tb/merge-tree-write-pack -> tb/merge...
2e3b7b2460..57243409ad refs/notes/amlog -> refs/notes/amlog
This is already "tabular" and gives enough information to tell me
which ones did not get updated (e.g., I do not see 'next' there) and
which ones are forced ('jch' and 'seen' are usually forced and I'll
notice that I may have made mistakes if they are not). But a
hypothetical presentation that is easier for novices to read may
show "git log --oneline --graph old...new" (or some abbreviated form
of it) between the old and new tips of the affected branches. At
that point, calling the improved output as "table" would make little
sense.
For commands that Jacob found it easier to explain in tabular form,
like "git add", it is very possible that two years down the road,
another Jacob comes around and proposes a different presentation
that is even easier for novices to understand, and it may not be a
tabular form.
So be very careful when choosing what to call this new thing, and
avoid naming it after the implementation details (e.g., in what
particular shape the data gets presented) that may turn out not to
be the most important part of the concept.
[Footnote]
* FWIW, "git status -s" is a tabular presentation. Maybe we can
add a more verbose form of "-s" and be done with it for the
command?
^ permalink raw reply
* Re: [RFC PATCH 0/5] Introduce -t, --table for status/add commands
From: Dragan Simic @ 2023-10-24 1:10 UTC (permalink / raw)
To: Jacob Stopak; +Cc: Oswald Buddenhagen, git
In-Reply-To: <ZTb/HeILRHnZjaz6.jacob@initialcommit.io>
On 2023-10-24 01:17, Jacob Stopak wrote:
> That's true, it would be just as easy to report the results of a
> command
> (and even easier in some cases) than forecasting the result in a
> dry-run.
> The question is how to decide which one? Do you report the results of
> certain commands by default while hints are enabled? And only as a dry
> run
> when the --dry-run / -n flag is added? That actually would make sense
> as
> it wouldn't add "special" behavior to the dry run. The dry run would
> just
> report the exact same default output as the normal command, but omit
> the action.
IMHO, it would be the best to simply implement support for
"<command>.verbose = table" in the git configuration, similarly to how
we already have "commit.verbose = true". That way tables could be
enabled per command, according to the user's preferences, regardless of
performing dry runs or not.
The new hint would be placed somewhere, which should be decided
separately, but having the hint enabled or disabled wouldn't affect
anything else. Just like with the other hints.
^ permalink raw reply
* Re: [PATCH v2] doc/git-bisect: clarify `git bisect run` syntax
From: Junio C Hamano @ 2023-10-24 0:12 UTC (permalink / raw)
To: Javier Mora
Cc: Eric Sunshine, cousteau via GitGitGadget, git, Patrick Steinhardt
In-Reply-To: <CAH1-q0hNSKgr1-dtZac=z7Bx15gON0Y-1pyBM57zuXaFPaJJKQ@mail.gmail.com>
Javier Mora <cousteaulecommandant@gmail.com> writes:
>> the patch subject becomes a bit outdated with this addition.
>
> Right; I wanted to change it to something like "clarify `git bisect
> run` syntax and other minor changes" but wanted to keep the title
> concise.
> I guess I could change it to just "clarify `git bisect` syntax" though
> remove the "run").
Quite honestly, I think at this point we are entering into the
"diminishing returns" territory. The title is still clear enough,
and the patch is good.
Thanks. The patch has been merged to 'next'.
^ permalink raw reply
* Re: [PATCH v3 5/7] refs: add pseudorefs array and iteration functions
From: Junio C Hamano @ 2023-10-24 0:08 UTC (permalink / raw)
To: Andy Koppe; +Cc: git, stolee
In-Reply-To: <20231023221143.72489-6-andy.koppe@gmail.com>
Andy Koppe <andy.koppe@gmail.com> writes:
> Define const array 'pseudorefs' with the names of the pseudorefs that
> are documented in gitrevisions.1, and add functions for_each_pseudoref()
> and refs_for_each_pseudoref() for iterating over them.
Makes sense, and we can later (ab|re)use the same mechanism to
extend "git for-each-ref" that currently only knows how to show
things under "refs/" hierarchy.
> The functions process the pseudorefs in the same way as head_ref() and
> refs_head_ref() process HEAD, invoking an each_ref_fn callback on each
> pseudoref that exists.
Good.
^ permalink raw reply
* Re: [PATCH v5 0/5] merge-ort: implement support for packing objects together
From: Junio C Hamano @ 2023-10-23 23:31 UTC (permalink / raw)
To: Taylor Blau
Cc: git, Elijah Newren, Eric W. Biederman, Jeff King,
Patrick Steinhardt
In-Reply-To: <cover.1698101088.git.me@ttaylorr.com>
Taylor Blau <me@ttaylorr.com> writes:
> But I
> think that this approach ended up being less heavy-weight than I had
> originally imagined, so I think that this version is a worthwhile
> improvement over v4.
;-).
This version is a good place to stop, a bit short of going full OO.
Nicely done.
^ permalink raw reply
* Re: [PATCH 2/2] fetch: no redundant error message for atomic fetch
From: Jiang Xin @ 2023-10-23 23:20 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Git List, Junio C Hamano, Jiang Xin
In-Reply-To: <ZTZF3AbNNuGpy38l@tanuki>
On Mon, Oct 23, 2023 at 6:07 PM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Mon, Oct 23, 2023 at 05:16:20PM +0800, Jiang Xin wrote:
> > On Mon, Oct 23, 2023 at 4:27 PM Patrick Steinhardt <ps@pks.im> wrote:
> > >
> > > On Thu, Oct 19, 2023 at 10:34:33PM +0800, Jiang Xin wrote:
> > > > @@ -1775,10 +1775,8 @@ static int do_fetch(struct transport *transport,
> > > > }
> > > >
> > > > cleanup:
> > > > - if (retcode && transaction) {
> > > > - ref_transaction_abort(transaction, &err);
> > > > + if (retcode && transaction && ref_transaction_abort(transaction, &err))
> > > > error("%s", err.buf);
> > > > - }
> > >
> > > Right. We already call `error()` in all cases where `err` was populated
> > > before we `goto cleanup;`, so calling it unconditionally a second time
> > > here is wrong.
> > >
> > > That being said, `ref_transaction_abort()` will end up calling the
> > > respective backend's implementation of `transaction_abort`, and for the
> > > files backend it actually ignores `err` completely. So if the abort
> > > fails, we would still end up calling `error()` with an empty string.
> >
> > The transaction_abort implementations of the two builtin refs backends
> > will not use "err“ because they never fail (always return 0). Some one
> > may want to implement their own refs backend which may use the "err"
> > variable in their "transaction_abort". So follow the pattern as
> > update-ref.c and files-backend.c to call ref_transaction_abort() is
> > safe.
> >
> > > Furthermore, it can happen that `transaction_commit` fails, writes to
> > > the buffer and then prints the error. If the abort now fails as well, we
> > > would end up printing the error message twice.
> >
> > The abort never fails so error message from transaction_commit() will
> > not reach the code.
>
> With that reasoning we could get rid of the error handling of abort
> completely as it's known not to fail. But only because it does not fail
> right now doesn't mean that it won't in the future, as the infra for it
> to fail is all in place. And in case it ever does the current code will
> run into the bug I described.
If in the future ref_transaction_abort() fails for some reason, the
err variable will be filled with the error message and the previous
error message will be discarded, no duplication will occur. So I think
use this fix is OK.
> So in my opinion, we should either refactor the code to clarify that
> this cannot fail indeed. Or do the right thing and handle the error case
> correctly, which right now we don't.
>
> Patrick
^ permalink raw reply
* Re: [PATCH v2] doc/git-bisect: clarify `git bisect run` syntax
From: Eric Sunshine @ 2023-10-23 23:18 UTC (permalink / raw)
To: Javier Mora; +Cc: cousteau via GitGitGadget, git, Patrick Steinhardt
In-Reply-To: <CAH1-q0hNSKgr1-dtZac=z7Bx15gON0Y-1pyBM57zuXaFPaJJKQ@mail.gmail.com>
On Mon, Oct 23, 2023 at 6:53 PM Javier Mora
<cousteaulecommandant@gmail.com> wrote:
> > the patch subject becomes a bit outdated with this addition.
>
> Right; I wanted to change it to something like "clarify `git bisect
> run` syntax and other minor changes" but wanted to keep the title
> concise.
> I guess I could change it to just "clarify `git bisect` syntax" though
> remove the "run").
Yup.
> > the following two lines are already referencing placeholders
> > <term-new> and <term-old>
>
> That's why I added it; that `(bad|new|<term-new>)` felt a bit awkward
> with no previous explanation of what <term-new> was.
>
> > ...now we have an inconsistency again since this text just uses the
> > generic <term>. However, I haven't convinced myself that we need to
> > care about this inconsistency.
>
> I thought about that, but in THAT case it wasn't necessary because
> <term-new> and <term-old> are never used there (and I wanted to avoid
> making -h too long). But it's true that it feels inconsistent; I may
> add it just for the sake of consistency.
I don't feel strongly about the inconsistency at this point.
> Overall, maybe I should leave that change to a separate patch, even if
> it's a minor correction. (This made more sense when I had in mind the
> plan to move everything from description to synopsis so I would need
> to touch all those lines anyway.) The changes will be compatible
> anyway (they're far away enough to not cause merge conflicts). What
> do you think?
I can certainly see the "{new,bad}" to "(new|bad") and <term> to
<new-term>/<old-term> changes being separated out, making this a two-
or three-patch series.
^ permalink raw reply
* Re: [RFC PATCH 0/5] Introduce -t, --table for status/add commands
From: Jacob Stopak @ 2023-10-23 23:17 UTC (permalink / raw)
To: Oswald Buddenhagen; +Cc: Dragan Simic, git
In-Reply-To: <ZTbVY7Nf+DTYqHky@ugly>
On Mon, Oct 23, 2023 at 10:19:47PM +0200, Oswald Buddenhagen wrote:
> On Mon, Oct 23, 2023 at 12:29:12PM -0700, Jacob Stopak wrote:
> > Those arrows showing how things move only really apply to "simulating"
> > (dry runs) for specific commands like add, restore, rm, commit, stash,
> > etc, so making the --table proposal a default status output would still
> > miss those scenarios.
> >
> you're too focused on the status quo of your own tool. :-)
Ha it's possible. I created git-sim with a very specific use case in mind
so you're right it's probably worth rethinking that while taking into account
Git's other functionality that wasn't in the picture with an external tool.
> there is really nothing that would speak against the real commands reporting
> what they just *actually did*. this would seem rather helpful for noobs and
> other insecure users.
That's true, it would be just as easy to report the results of a command
(and even easier in some cases) than forecasting the result in a dry-run.
The question is how to decide which one? Do you report the results of
certain commands by default while hints are enabled? And only as a dry run
when the --dry-run / -n flag is added? That actually would make sense as
it wouldn't add "special" behavior to the dry run. The dry run would just
report the exact same default output as the normal command, but omit the action.
> if one really wanted, "you can also use this with --dry-run" could be part
> of the hint that would say how to turn off the extra verbosity (or just the
> hint itself, if one likes the verbosity).
Interesting. So many ways to think about how to optimize the user
interaction...
> one could even go one step further and put at least the destructive commands
> into interactive/confirmation mode by default. but that's probably a bridge
> too far, as it would be potentially habit-forming in a bad way.
That would be an interesting add on to our discussion above. So as a
thought experiment, let's pretend there are no restrictions from
traditional users, we could:
1) Enable verbose results output by default to certain commands, which
could include a visual table-based output where applicable, and a
hint to disable it. (Are hints currently command-specific? Or even
scenario-specific within a command? Or is it all or nothing?)
2) Include verbose/table results on dry runs, and add dry run flags onto
commands that should seem to have it for consistency. For example
"git add" has a dry run flag but "git restore" (the hinted inverse
operation) does not. I assume on dry runs hints wouldn't be used to
communicate anything.
3) Well, I'll admit about once every 3 months I run "git stash --all"
when I really meant "git stash --include-untracked" and proceed to
lose a small part of my soul. This would be saved by a simple
confirmation. I find that the stuff like "git reset --hard" doesn't
bother me anymore since I know when to be careful with it and what
things I can get back and how to do it. But I find the nasty ones
are the things that sound like what you want but end up doing
something bad. Not sure there is a way to isolate those though...
I'm rambling now. But maybe for interactivity, at the very least it
could be added to dry runs, like a "Here's what would happen, want
to run it now?" I got this feedback a few times for git-sim as well.
^ permalink raw reply
* Re: [PATCH v2] doc/git-bisect: clarify `git bisect run` syntax
From: Javier Mora @ 2023-10-23 22:53 UTC (permalink / raw)
To: Eric Sunshine; +Cc: cousteau via GitGitGadget, git, Patrick Steinhardt
In-Reply-To: <CAPig+cQuBwzaG7ZssGUY6k8wf8pcGZHAGLnbRy579uTPMKqwKQ@mail.gmail.com>
> the patch subject becomes a bit outdated with this addition.
Right; I wanted to change it to something like "clarify `git bisect
run` syntax and other minor changes" but wanted to keep the title
concise.
I guess I could change it to just "clarify `git bisect` syntax" though
remove the "run").
> the following two lines are already referencing placeholders
> <term-new> and <term-old>
That's why I added it; that `(bad|new|<term-new>)` felt a bit awkward
with no previous explanation of what <term-new> was.
> ...now we have an inconsistency again since this text just uses the
> generic <term>. However, I haven't convinced myself that we need to
> care about this inconsistency.
I thought about that, but in THAT case it wasn't necessary because
<term-new> and <term-old> are never used there (and I wanted to avoid
making -h too long). But it's true that it feels inconsistent; I may
add it just for the sake of consistency.
Overall, maybe I should leave that change to a separate patch, even if
it's a minor correction. (This made more sense when I had in mind the
plan to move everything from description to synopsis so I would need
to touch all those lines anyway.) The changes will be compatible
anyway (they're far away enough to not cause merge conflicts). What
do you think?
^ permalink raw reply
* [PATCH v5 5/5] builtin/merge-tree.c: implement support for `--write-pack`
From: Taylor Blau @ 2023-10-23 22:45 UTC (permalink / raw)
To: git
Cc: Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano,
Patrick Steinhardt
In-Reply-To: <cover.1698101088.git.me@ttaylorr.com>
When using merge-tree often within a repository[^1], it is possible to
generate a relatively large number of loose objects, which can result in
degraded performance, and inode exhaustion in extreme cases.
Building on the functionality introduced in previous commits, the
bulk-checkin machinery now has support to write arbitrary blob and tree
objects which are small enough to be held in-core. We can use this to
write any blob/tree objects generated by ORT into a separate pack
instead of writing them out individually as loose.
This functionality is gated behind a new `--write-pack` option to
`merge-tree` that works with the (non-deprecated) `--write-tree` mode.
The implementation is relatively straightforward. There are two spots
within the ORT mechanism where we call `write_object_file()`, one for
content differences within blobs, and another to assemble any new trees
necessary to construct the merge. In each of those locations,
conditionally replace calls to `write_object_file()` with
`index_blob_bulk_checkin_incore()` or `index_tree_bulk_checkin_incore()`
depending on which kind of object we are writing.
The only remaining task is to begin and end the transaction necessary to
initialize the bulk-checkin machinery, and move any new pack(s) it
created into the main object store.
[^1]: Such is the case at GitHub, where we run presumptive "test merges"
on open pull requests to see whether or not we can light up the merge
button green depending on whether or not the presumptive merge was
conflicted.
This is done in response to a number of user-initiated events,
including viewing an open pull request whose last test merge is stale
with respect to the current base and tip of the pull request. As a
result, merge-tree can be run very frequently on large, active
repositories.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
Documentation/git-merge-tree.txt | 4 ++
builtin/merge-tree.c | 5 ++
merge-ort.c | 42 +++++++++++----
merge-recursive.h | 1 +
t/t4301-merge-tree-write-tree.sh | 93 ++++++++++++++++++++++++++++++++
5 files changed, 136 insertions(+), 9 deletions(-)
diff --git a/Documentation/git-merge-tree.txt b/Documentation/git-merge-tree.txt
index ffc4fbf7e8..9d37609ef1 100644
--- a/Documentation/git-merge-tree.txt
+++ b/Documentation/git-merge-tree.txt
@@ -69,6 +69,10 @@ OPTIONS
specify a merge-base for the merge, and specifying multiple bases is
currently not supported. This option is incompatible with `--stdin`.
+--write-pack::
+ Write any new objects into a separate packfile instead of as
+ individual loose objects.
+
[[OUTPUT]]
OUTPUT
------
diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
index a35e0452d6..218442ac9b 100644
--- a/builtin/merge-tree.c
+++ b/builtin/merge-tree.c
@@ -19,6 +19,7 @@
#include "tree.h"
#include "config.h"
#include "strvec.h"
+#include "bulk-checkin.h"
static int line_termination = '\n';
@@ -416,6 +417,7 @@ struct merge_tree_options {
int name_only;
int use_stdin;
struct merge_options merge_options;
+ int write_pack;
};
static int real_merge(struct merge_tree_options *o,
@@ -441,6 +443,7 @@ static int real_merge(struct merge_tree_options *o,
_("not something we can merge"));
opt.show_rename_progress = 0;
+ opt.write_pack = o->write_pack;
opt.branch1 = branch1;
opt.branch2 = branch2;
@@ -553,6 +556,8 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
N_("specify a merge-base for the merge")),
OPT_STRVEC('X', "strategy-option", &xopts, N_("option=value"),
N_("option for selected merge strategy")),
+ OPT_BOOL(0, "write-pack", &o.write_pack,
+ N_("write new objects to a pack instead of as loose")),
OPT_END()
};
diff --git a/merge-ort.c b/merge-ort.c
index 3653725661..523577d71e 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -48,6 +48,7 @@
#include "tree.h"
#include "unpack-trees.h"
#include "xdiff-interface.h"
+#include "bulk-checkin.h"
/*
* We have many arrays of size 3. Whenever we have such an array, the
@@ -2108,10 +2109,19 @@ static int handle_content_merge(struct merge_options *opt,
if ((merge_status < 0) || !result_buf.ptr)
ret = error(_("failed to execute internal merge"));
- if (!ret &&
- write_object_file(result_buf.ptr, result_buf.size,
- OBJ_BLOB, &result->oid))
- ret = error(_("unable to add %s to database"), path);
+ if (!ret) {
+ ret = opt->write_pack
+ ? index_blob_bulk_checkin_incore(&result->oid,
+ result_buf.ptr,
+ result_buf.size,
+ path, 1)
+ : write_object_file(result_buf.ptr,
+ result_buf.size,
+ OBJ_BLOB, &result->oid);
+ if (ret)
+ ret = error(_("unable to add %s to database"),
+ path);
+ }
free(result_buf.ptr);
if (ret)
@@ -3597,7 +3607,8 @@ static int tree_entry_order(const void *a_, const void *b_)
b->string, strlen(b->string), bmi->result.mode);
}
-static int write_tree(struct object_id *result_oid,
+static int write_tree(struct merge_options *opt,
+ struct object_id *result_oid,
struct string_list *versions,
unsigned int offset,
size_t hash_size)
@@ -3631,8 +3642,14 @@ static int write_tree(struct object_id *result_oid,
}
/* Write this object file out, and record in result_oid */
- if (write_object_file(buf.buf, buf.len, OBJ_TREE, result_oid))
+ ret = opt->write_pack
+ ? index_tree_bulk_checkin_incore(result_oid,
+ buf.buf, buf.len, "", 1)
+ : write_object_file(buf.buf, buf.len, OBJ_TREE, result_oid);
+
+ if (ret)
ret = -1;
+
strbuf_release(&buf);
return ret;
}
@@ -3797,8 +3814,8 @@ static int write_completed_directory(struct merge_options *opt,
*/
dir_info->is_null = 0;
dir_info->result.mode = S_IFDIR;
- if (write_tree(&dir_info->result.oid, &info->versions, offset,
- opt->repo->hash_algo->rawsz) < 0)
+ if (write_tree(opt, &dir_info->result.oid, &info->versions,
+ offset, opt->repo->hash_algo->rawsz) < 0)
ret = -1;
}
@@ -4332,9 +4349,13 @@ static int process_entries(struct merge_options *opt,
fflush(stdout);
BUG("dir_metadata accounting completely off; shouldn't happen");
}
- if (write_tree(result_oid, &dir_metadata.versions, 0,
+ if (write_tree(opt, result_oid, &dir_metadata.versions, 0,
opt->repo->hash_algo->rawsz) < 0)
ret = -1;
+
+ if (opt->write_pack)
+ end_odb_transaction();
+
cleanup:
string_list_clear(&plist, 0);
string_list_clear(&dir_metadata.versions, 0);
@@ -4878,6 +4899,9 @@ static void merge_start(struct merge_options *opt, struct merge_result *result)
*/
strmap_init(&opt->priv->conflicts);
+ if (opt->write_pack)
+ begin_odb_transaction();
+
trace2_region_leave("merge", "allocate/init", opt->repo);
}
diff --git a/merge-recursive.h b/merge-recursive.h
index 3d3b3e3c29..5c5ff380a8 100644
--- a/merge-recursive.h
+++ b/merge-recursive.h
@@ -48,6 +48,7 @@ struct merge_options {
unsigned renormalize : 1;
unsigned record_conflict_msgs_as_headers : 1;
const char *msg_header_prefix;
+ unsigned write_pack : 1;
/* internal fields used by the implementation */
struct merge_options_internal *priv;
diff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh
index b2c8a43fce..d2a8634523 100755
--- a/t/t4301-merge-tree-write-tree.sh
+++ b/t/t4301-merge-tree-write-tree.sh
@@ -945,4 +945,97 @@ test_expect_success 'check the input format when --stdin is passed' '
test_cmp expect actual
'
+packdir=".git/objects/pack"
+
+test_expect_success 'merge-tree can pack its result with --write-pack' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+
+ # base has lines [3, 4, 5]
+ # - side adds to the beginning, resulting in [1, 2, 3, 4, 5]
+ # - other adds to the end, resulting in [3, 4, 5, 6, 7]
+ #
+ # merging the two should result in a new blob object containing
+ # [1, 2, 3, 4, 5, 6, 7], along with a new tree.
+ test_commit -C repo base file "$(test_seq 3 5)" &&
+ git -C repo branch -M main &&
+ git -C repo checkout -b side main &&
+ test_commit -C repo side file "$(test_seq 1 5)" &&
+ git -C repo checkout -b other main &&
+ test_commit -C repo other file "$(test_seq 3 7)" &&
+
+ find repo/$packdir -type f -name "pack-*.idx" >packs.before &&
+ tree="$(git -C repo merge-tree --write-pack \
+ refs/tags/side refs/tags/other)" &&
+ blob="$(git -C repo rev-parse $tree:file)" &&
+ find repo/$packdir -type f -name "pack-*.idx" >packs.after &&
+
+ test_must_be_empty packs.before &&
+ test_line_count = 1 packs.after &&
+
+ git show-index <$(cat packs.after) >objects &&
+ test_line_count = 2 objects &&
+ grep "^[1-9][0-9]* $tree" objects &&
+ grep "^[1-9][0-9]* $blob" objects
+'
+
+test_expect_success 'merge-tree can write multiple packs with --write-pack' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+
+ git config pack.packSizeLimit 512 &&
+
+ test_seq 512 >f &&
+
+ # "f" contains roughly ~2,000 bytes.
+ #
+ # Each side ("foo" and "bar") adds a small amount of data at the
+ # beginning and end of "base", respectively.
+ git add f &&
+ test_tick &&
+ git commit -m base &&
+ git branch -M main &&
+
+ git checkout -b foo main &&
+ {
+ echo foo && cat f
+ } >f.tmp &&
+ mv f.tmp f &&
+ git add f &&
+ test_tick &&
+ git commit -m foo &&
+
+ git checkout -b bar main &&
+ echo bar >>f &&
+ git add f &&
+ test_tick &&
+ git commit -m bar &&
+
+ find $packdir -type f -name "pack-*.idx" >packs.before &&
+ # Merging either side should result in a new object which is
+ # larger than 1M, thus the result should be split into two
+ # separate packs.
+ tree="$(git merge-tree --write-pack \
+ refs/heads/foo refs/heads/bar)" &&
+ blob="$(git rev-parse $tree:f)" &&
+ find $packdir -type f -name "pack-*.idx" >packs.after &&
+
+ test_must_be_empty packs.before &&
+ test_line_count = 2 packs.after &&
+ for idx in $(cat packs.after)
+ do
+ git show-index <$idx || return 1
+ done >objects &&
+
+ # The resulting set of packs should contain one copy of both
+ # objects, each in a separate pack.
+ test_line_count = 2 objects &&
+ grep "^[1-9][0-9]* $tree" objects &&
+ grep "^[1-9][0-9]* $blob" objects
+
+ )
+'
+
test_done
--
2.42.0.425.g963d08ddb3.dirty
^ permalink raw reply related
* [PATCH v5 4/5] bulk-checkin: introduce `index_tree_bulk_checkin_incore()`
From: Taylor Blau @ 2023-10-23 22:45 UTC (permalink / raw)
To: git
Cc: Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano,
Patrick Steinhardt
In-Reply-To: <cover.1698101088.git.me@ttaylorr.com>
The remaining missing piece in order to teach the `merge-tree` builtin
how to write the contents of a merge into a pack is a function to index
tree objects into a bulk-checkin pack.
This patch implements that missing piece, which is a thin wrapper around
all of the functionality introduced in previous commits.
If and when Git gains support for a "compatibility" hash algorithm, the
changes to support that here will be minimal. The bulk-checkin machinery
will need to convert the incoming tree to compute its length under the
compatibility hash, necessary to reconstruct its header. With that
information (and the converted contents of the tree), the bulk-checkin
machinery will have enough to keep track of the converted object's hash
in order to update the compatibility mapping.
Within some thin wrapper around `deflate_obj_to_pack_incore()` (perhaps
`deflate_tree_to_pack_incore()`), the changes should be limited to
something like:
struct strbuf converted = STRBUF_INIT;
if (the_repository->compat_hash_algo) {
if (convert_object_file(&compat_obj,
the_repository->hash_algo,
the_repository->compat_hash_algo, ...) < 0)
die(...);
format_object_header_hash(the_repository->compat_hash_algo,
OBJ_TREE, size);
}
/* compute the converted tree's hash using the compat algorithm */
strbuf_release(&converted);
, assuming related changes throughout the rest of the bulk-checkin
machinery necessary to update the hash of the converted object, which
are likewise minimal in size.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
bulk-checkin.c | 12 ++++++++++++
bulk-checkin.h | 4 ++++
2 files changed, 16 insertions(+)
diff --git a/bulk-checkin.c b/bulk-checkin.c
index b728210bc7..bd6151ba3c 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -496,6 +496,18 @@ int index_blob_bulk_checkin_incore(struct object_id *oid,
return status;
}
+int index_tree_bulk_checkin_incore(struct object_id *oid,
+ const void *buf, size_t size,
+ const char *path, unsigned flags)
+{
+ int status = deflate_obj_to_pack_incore(&bulk_checkin_packfile, oid,
+ buf, size, path, OBJ_TREE,
+ flags);
+ if (!odb_transaction_nesting)
+ flush_bulk_checkin_packfile(&bulk_checkin_packfile);
+ return status;
+}
+
void begin_odb_transaction(void)
{
odb_transaction_nesting += 1;
diff --git a/bulk-checkin.h b/bulk-checkin.h
index 1b91daeaee..89786b3954 100644
--- a/bulk-checkin.h
+++ b/bulk-checkin.h
@@ -17,6 +17,10 @@ int index_blob_bulk_checkin_incore(struct object_id *oid,
const void *buf, size_t size,
const char *path, unsigned flags);
+int index_tree_bulk_checkin_incore(struct object_id *oid,
+ const void *buf, size_t size,
+ const char *path, unsigned flags);
+
/*
* Tell the object database to optimize for adding
* multiple objects. end_odb_transaction must be called
--
2.42.0.425.g963d08ddb3.dirty
^ permalink raw reply related
* [PATCH v5 3/5] bulk-checkin: introduce `index_blob_bulk_checkin_incore()`
From: Taylor Blau @ 2023-10-23 22:45 UTC (permalink / raw)
To: git
Cc: Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano,
Patrick Steinhardt
In-Reply-To: <cover.1698101088.git.me@ttaylorr.com>
Introduce `index_blob_bulk_checkin_incore()` which allows streaming
arbitrary blob contents from memory into the bulk-checkin pack.
In order to support streaming from a location in memory, we must
implement a new kind of bulk_checkin_source that does just that. These
implementation in spread out across:
- init_bulk_checkin_source_incore()
- bulk_checkin_source_read_incore()
- bulk_checkin_source_seek_incore()
Note that, unlike file descriptors, which manage their own offset
internally, we have to keep track of how many bytes we've read out of
the buffer, and make sure we don't read past the end of the buffer.
This will be useful in a couple of more commits in order to provide the
`merge-tree` builtin with a mechanism to create a new pack containing
any objects it created during the merge, instead of storing those
objects individually as loose.
Similar to the existing `index_blob_bulk_checkin()` function, the
entrypoint delegates to `deflate_obj_to_pack_incore()`. That function in
turn delegates to deflate_obj_to_pack(), which is responsible for
formatting the pack header and then deflating the contents into the
pack.
Consistent with the rest of the bulk-checkin mechanism, there are no
direct tests here. In future commits when we expose this new
functionality via the `merge-tree` builtin, we will test it indirectly
there.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
bulk-checkin.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++++
bulk-checkin.h | 4 +++
2 files changed, 79 insertions(+)
diff --git a/bulk-checkin.c b/bulk-checkin.c
index 79776e679e..b728210bc7 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -148,6 +148,10 @@ struct bulk_checkin_source {
struct {
int fd;
} from_fd;
+ struct {
+ const void *buf;
+ size_t nr_read;
+ } incore;
} data;
size_t size;
@@ -166,6 +170,36 @@ static off_t bulk_checkin_source_seek_from_fd(struct bulk_checkin_source *source
return lseek(source->data.from_fd.fd, offset, SEEK_SET);
}
+static off_t bulk_checkin_source_read_incore(struct bulk_checkin_source *source,
+ void *buf, size_t nr)
+{
+ const unsigned char *src = source->data.incore.buf;
+
+ if (source->data.incore.nr_read > source->size)
+ BUG("read beyond bulk-checkin source buffer end "
+ "(%"PRIuMAX" > %"PRIuMAX")",
+ (uintmax_t)source->data.incore.nr_read,
+ (uintmax_t)source->size);
+
+ if (nr > source->size - source->data.incore.nr_read)
+ nr = source->size - source->data.incore.nr_read;
+
+ src += source->data.incore.nr_read;
+
+ memcpy(buf, src, nr);
+ source->data.incore.nr_read += nr;
+ return nr;
+}
+
+static off_t bulk_checkin_source_seek_incore(struct bulk_checkin_source *source,
+ off_t offset)
+{
+ if (!(0 <= offset && offset < source->size))
+ return (off_t)-1;
+ source->data.incore.nr_read = offset;
+ return source->data.incore.nr_read;
+}
+
static void init_bulk_checkin_source_from_fd(struct bulk_checkin_source *source,
int fd, size_t size,
const char *path)
@@ -181,6 +215,22 @@ static void init_bulk_checkin_source_from_fd(struct bulk_checkin_source *source,
source->path = path;
}
+static void init_bulk_checkin_source_incore(struct bulk_checkin_source *source,
+ const void *buf, size_t size,
+ const char *path)
+{
+ memset(source, 0, sizeof(struct bulk_checkin_source));
+
+ source->read = bulk_checkin_source_read_incore;
+ source->seek = bulk_checkin_source_seek_incore;
+
+ source->data.incore.buf = buf;
+ source->data.incore.nr_read = 0;
+
+ source->size = size;
+ source->path = path;
+}
+
/*
* Read the contents from 'source' for 'size' bytes, streaming it to the
* packfile in state while updating the hash in ctx. Signal a failure
@@ -359,6 +409,19 @@ static int deflate_obj_to_pack(struct bulk_checkin_packfile *state,
return 0;
}
+static int deflate_obj_to_pack_incore(struct bulk_checkin_packfile *state,
+ struct object_id *result_oid,
+ const void *buf, size_t size,
+ const char *path, enum object_type type,
+ unsigned flags)
+{
+ struct bulk_checkin_source source;
+
+ init_bulk_checkin_source_incore(&source, buf, size, path);
+
+ return deflate_obj_to_pack(state, result_oid, &source, type, 0, flags);
+}
+
static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
struct object_id *result_oid,
int fd, size_t size,
@@ -421,6 +484,18 @@ int index_blob_bulk_checkin(struct object_id *oid,
return status;
}
+int index_blob_bulk_checkin_incore(struct object_id *oid,
+ const void *buf, size_t size,
+ const char *path, unsigned flags)
+{
+ int status = deflate_obj_to_pack_incore(&bulk_checkin_packfile, oid,
+ buf, size, path, OBJ_BLOB,
+ flags);
+ if (!odb_transaction_nesting)
+ flush_bulk_checkin_packfile(&bulk_checkin_packfile);
+ return status;
+}
+
void begin_odb_transaction(void)
{
odb_transaction_nesting += 1;
diff --git a/bulk-checkin.h b/bulk-checkin.h
index aa7286a7b3..1b91daeaee 100644
--- a/bulk-checkin.h
+++ b/bulk-checkin.h
@@ -13,6 +13,10 @@ int index_blob_bulk_checkin(struct object_id *oid,
int fd, size_t size,
const char *path, unsigned flags);
+int index_blob_bulk_checkin_incore(struct object_id *oid,
+ const void *buf, size_t size,
+ const char *path, unsigned flags);
+
/*
* Tell the object database to optimize for adding
* multiple objects. end_odb_transaction must be called
--
2.42.0.425.g963d08ddb3.dirty
^ permalink raw reply related
* [PATCH v5 2/5] bulk-checkin: generify `stream_blob_to_pack()` for arbitrary types
From: Taylor Blau @ 2023-10-23 22:44 UTC (permalink / raw)
To: git
Cc: Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano,
Patrick Steinhardt
In-Reply-To: <cover.1698101088.git.me@ttaylorr.com>
The existing `stream_blob_to_pack()` function is named based on the fact
that it knows only how to stream blobs into a bulk-checkin pack.
But there is no longer anything in this function which prevents us from
writing objects of arbitrary types to the bulk-checkin pack. Prepare to
write OBJ_TREEs by removing this assumption, adding an `enum
object_type` parameter to this function's argument list, and renaming it
to `stream_obj_to_pack()` accordingly.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
bulk-checkin.c | 61 +++++++++++++++++++++++++++++---------------------
1 file changed, 36 insertions(+), 25 deletions(-)
diff --git a/bulk-checkin.c b/bulk-checkin.c
index 174a6c24e4..79776e679e 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -196,10 +196,10 @@ static void init_bulk_checkin_source_from_fd(struct bulk_checkin_source *source,
* status before calling us just in case we ask it to call us again
* with a new pack.
*/
-static int stream_blob_to_pack(struct bulk_checkin_packfile *state,
- git_hash_ctx *ctx, off_t *already_hashed_to,
- struct bulk_checkin_source *source,
- unsigned flags)
+static int stream_obj_to_pack(struct bulk_checkin_packfile *state,
+ git_hash_ctx *ctx, off_t *already_hashed_to,
+ struct bulk_checkin_source *source,
+ enum object_type type, unsigned flags)
{
git_zstream s;
unsigned char ibuf[16384];
@@ -212,8 +212,7 @@ static int stream_blob_to_pack(struct bulk_checkin_packfile *state,
git_deflate_init(&s, pack_compression_level);
- hdrlen = encode_in_pack_object_header(obuf, sizeof(obuf), OBJ_BLOB,
- size);
+ hdrlen = encode_in_pack_object_header(obuf, sizeof(obuf), type, size);
s.next_out = obuf + hdrlen;
s.avail_out = sizeof(obuf) - hdrlen;
@@ -293,27 +292,23 @@ static void prepare_to_stream(struct bulk_checkin_packfile *state,
die_errno("unable to write pack header");
}
-static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
- struct object_id *result_oid,
- int fd, size_t size,
- const char *path, unsigned flags)
+
+static int deflate_obj_to_pack(struct bulk_checkin_packfile *state,
+ struct object_id *result_oid,
+ struct bulk_checkin_source *source,
+ enum object_type type,
+ off_t seekback,
+ unsigned flags)
{
- off_t seekback, already_hashed_to;
+ off_t already_hashed_to = 0;
git_hash_ctx ctx;
unsigned char obuf[16384];
unsigned header_len;
struct hashfile_checkpoint checkpoint = {0};
struct pack_idx_entry *idx = NULL;
- struct bulk_checkin_source source;
- init_bulk_checkin_source_from_fd(&source, fd, size, path);
-
- seekback = lseek(fd, 0, SEEK_CUR);
- if (seekback == (off_t) -1)
- return error("cannot find the current offset");
-
- header_len = format_object_header((char *)obuf, sizeof(obuf),
- OBJ_BLOB, size);
+ header_len = format_object_header((char *)obuf, sizeof(obuf), type,
+ source->size);
the_hash_algo->init_fn(&ctx);
the_hash_algo->update_fn(&ctx, obuf, header_len);
the_hash_algo->init_fn(&checkpoint.ctx);
@@ -322,8 +317,6 @@ static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
if ((flags & HASH_WRITE_OBJECT) != 0)
CALLOC_ARRAY(idx, 1);
- already_hashed_to = 0;
-
while (1) {
prepare_to_stream(state, flags);
if (idx) {
@@ -331,8 +324,8 @@ static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
idx->offset = state->offset;
crc32_begin(state->f);
}
- if (!stream_blob_to_pack(state, &ctx, &already_hashed_to,
- &source, flags))
+ if (!stream_obj_to_pack(state, &ctx, &already_hashed_to,
+ source, type, flags))
break;
/*
* Writing this object to the current pack will make
@@ -344,7 +337,7 @@ static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
hashfile_truncate(state->f, &checkpoint);
state->offset = checkpoint.offset;
flush_bulk_checkin_packfile(state);
- if (source.seek(&source, seekback) == (off_t)-1)
+ if (source->seek(source, seekback) == (off_t)-1)
return error("cannot seek back");
}
the_hash_algo->final_oid_fn(result_oid, &ctx);
@@ -366,6 +359,24 @@ static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
return 0;
}
+static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
+ struct object_id *result_oid,
+ int fd, size_t size,
+ const char *path, unsigned flags)
+{
+ struct bulk_checkin_source source;
+ off_t seekback;
+
+ init_bulk_checkin_source_from_fd(&source, fd, size, path);
+
+ seekback = lseek(fd, 0, SEEK_CUR);
+ if (seekback == (off_t) -1)
+ return error("cannot find the current offset");
+
+ return deflate_obj_to_pack(state, result_oid, &source, OBJ_BLOB,
+ seekback, flags);
+}
+
void prepare_loose_object_bulk_checkin(void)
{
/*
--
2.42.0.425.g963d08ddb3.dirty
^ permalink raw reply related
* [PATCH v5 1/5] bulk-checkin: extract abstract `bulk_checkin_source`
From: Taylor Blau @ 2023-10-23 22:44 UTC (permalink / raw)
To: git
Cc: Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano,
Patrick Steinhardt
In-Reply-To: <cover.1698101088.git.me@ttaylorr.com>
A future commit will want to implement a very similar routine as in
`stream_blob_to_pack()` with two notable changes:
- Instead of streaming just OBJ_BLOBs, this new function may want to
stream objects of arbitrary type.
- Instead of streaming the object's contents from an open
file-descriptor, this new function may want to "stream" its contents
from memory.
To avoid duplicating a significant chunk of code between the existing
`stream_blob_to_pack()`, extract an abstract `bulk_checkin_source`. This
concept currently is a thin layer of `lseek()` and `read_in_full()`, but
will grow to understand how to perform analogous operations when writing
out an object's contents from memory.
Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
bulk-checkin.c | 65 +++++++++++++++++++++++++++++++++++++++++++-------
1 file changed, 57 insertions(+), 8 deletions(-)
diff --git a/bulk-checkin.c b/bulk-checkin.c
index 6ce62999e5..174a6c24e4 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -140,8 +140,49 @@ static int already_written(struct bulk_checkin_packfile *state, struct object_id
return 0;
}
+struct bulk_checkin_source {
+ off_t (*read)(struct bulk_checkin_source *, void *, size_t);
+ off_t (*seek)(struct bulk_checkin_source *, off_t);
+
+ union {
+ struct {
+ int fd;
+ } from_fd;
+ } data;
+
+ size_t size;
+ const char *path;
+};
+
+static off_t bulk_checkin_source_read_from_fd(struct bulk_checkin_source *source,
+ void *buf, size_t nr)
+{
+ return read_in_full(source->data.from_fd.fd, buf, nr);
+}
+
+static off_t bulk_checkin_source_seek_from_fd(struct bulk_checkin_source *source,
+ off_t offset)
+{
+ return lseek(source->data.from_fd.fd, offset, SEEK_SET);
+}
+
+static void init_bulk_checkin_source_from_fd(struct bulk_checkin_source *source,
+ int fd, size_t size,
+ const char *path)
+{
+ memset(source, 0, sizeof(struct bulk_checkin_source));
+
+ source->read = bulk_checkin_source_read_from_fd;
+ source->seek = bulk_checkin_source_seek_from_fd;
+
+ source->data.from_fd.fd = fd;
+
+ source->size = size;
+ source->path = path;
+}
+
/*
- * Read the contents from fd for size bytes, streaming it to the
+ * Read the contents from 'source' for 'size' bytes, streaming it to the
* packfile in state while updating the hash in ctx. Signal a failure
* by returning a negative value when the resulting pack would exceed
* the pack size limit and this is not the first object in the pack,
@@ -157,7 +198,7 @@ static int already_written(struct bulk_checkin_packfile *state, struct object_id
*/
static int stream_blob_to_pack(struct bulk_checkin_packfile *state,
git_hash_ctx *ctx, off_t *already_hashed_to,
- int fd, size_t size, const char *path,
+ struct bulk_checkin_source *source,
unsigned flags)
{
git_zstream s;
@@ -167,22 +208,27 @@ static int stream_blob_to_pack(struct bulk_checkin_packfile *state,
int status = Z_OK;
int write_object = (flags & HASH_WRITE_OBJECT);
off_t offset = 0;
+ size_t size = source->size;
git_deflate_init(&s, pack_compression_level);
- hdrlen = encode_in_pack_object_header(obuf, sizeof(obuf), OBJ_BLOB, size);
+ hdrlen = encode_in_pack_object_header(obuf, sizeof(obuf), OBJ_BLOB,
+ size);
s.next_out = obuf + hdrlen;
s.avail_out = sizeof(obuf) - hdrlen;
while (status != Z_STREAM_END) {
if (size && !s.avail_in) {
ssize_t rsize = size < sizeof(ibuf) ? size : sizeof(ibuf);
- ssize_t read_result = read_in_full(fd, ibuf, rsize);
+ ssize_t read_result;
+
+ read_result = source->read(source, ibuf, rsize);
if (read_result < 0)
- die_errno("failed to read from '%s'", path);
+ die_errno("failed to read from '%s'",
+ source->path);
if (read_result != rsize)
die("failed to read %d bytes from '%s'",
- (int)rsize, path);
+ (int)rsize, source->path);
offset += rsize;
if (*already_hashed_to < offset) {
size_t hsize = offset - *already_hashed_to;
@@ -258,6 +304,9 @@ static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
unsigned header_len;
struct hashfile_checkpoint checkpoint = {0};
struct pack_idx_entry *idx = NULL;
+ struct bulk_checkin_source source;
+
+ init_bulk_checkin_source_from_fd(&source, fd, size, path);
seekback = lseek(fd, 0, SEEK_CUR);
if (seekback == (off_t) -1)
@@ -283,7 +332,7 @@ static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
crc32_begin(state->f);
}
if (!stream_blob_to_pack(state, &ctx, &already_hashed_to,
- fd, size, path, flags))
+ &source, flags))
break;
/*
* Writing this object to the current pack will make
@@ -295,7 +344,7 @@ static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
hashfile_truncate(state->f, &checkpoint);
state->offset = checkpoint.offset;
flush_bulk_checkin_packfile(state);
- if (lseek(fd, seekback, SEEK_SET) == (off_t) -1)
+ if (source.seek(&source, seekback) == (off_t)-1)
return error("cannot seek back");
}
the_hash_algo->final_oid_fn(result_oid, &ctx);
--
2.42.0.425.g963d08ddb3.dirty
^ permalink raw reply related
* [PATCH v5 0/5] merge-ort: implement support for packing objects together
From: Taylor Blau @ 2023-10-23 22:44 UTC (permalink / raw)
To: git
Cc: Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano,
Patrick Steinhardt
In-Reply-To: <cover.1697736516.git.me@ttaylorr.com>
(Rebased onto the current tip of 'master', which is ceadf0f3cf (The
twentieth batch, 2023-10-20)).
This series implements support for a new merge-tree option,
`--write-pack`, which causes any newly-written objects to be packed
together instead of being stored individually as loose.
This is a minor follow-up that could be taken instead of v4 (though the
changes between these two most recent rounds are stylistic and a matter
of subjective opinion).
This moves us the bulk_checkin_source structure introduced in response
to Junio's suggestion during the last round further in the OOP
direction. Instead of switching on the enum type of the source, have
function pointers for read() and seek() respectively.
The functionality at the end is the same, but this avoids some of the
namespacing issues that Peff pointed out while looking at v4. But I
think that this approach ended up being less heavy-weight than I had
originally imagined, so I think that this version is a worthwhile
improvement over v4.
Beyond that, the changes since last time can be viewed in the range-diff
below. Thanks in advance for any review!
[1]: https://lore.kernel.org/git/xmqq34y7plj4.fsf@gitster.g/
Taylor Blau (5):
bulk-checkin: extract abstract `bulk_checkin_source`
bulk-checkin: generify `stream_blob_to_pack()` for arbitrary types
bulk-checkin: introduce `index_blob_bulk_checkin_incore()`
bulk-checkin: introduce `index_tree_bulk_checkin_incore()`
builtin/merge-tree.c: implement support for `--write-pack`
Documentation/git-merge-tree.txt | 4 +
builtin/merge-tree.c | 5 +
bulk-checkin.c | 197 +++++++++++++++++++++++++++----
bulk-checkin.h | 8 ++
merge-ort.c | 42 +++++--
merge-recursive.h | 1 +
t/t4301-merge-tree-write-tree.sh | 93 +++++++++++++++
7 files changed, 316 insertions(+), 34 deletions(-)
Range-diff against v4:
1: 97bb6e9f59 ! 1: 696aa027e4 bulk-checkin: extract abstract `bulk_checkin_source`
@@ bulk-checkin.c: static int already_written(struct bulk_checkin_packfile *state,
}
+struct bulk_checkin_source {
-+ enum { SOURCE_FILE } type;
++ off_t (*read)(struct bulk_checkin_source *, void *, size_t);
++ off_t (*seek)(struct bulk_checkin_source *, off_t);
+
-+ /* SOURCE_FILE fields */
-+ int fd;
++ union {
++ struct {
++ int fd;
++ } from_fd;
++ } data;
+
-+ /* common fields */
+ size_t size;
+ const char *path;
+};
+
-+static off_t bulk_checkin_source_seek_to(struct bulk_checkin_source *source,
-+ off_t offset)
++static off_t bulk_checkin_source_read_from_fd(struct bulk_checkin_source *source,
++ void *buf, size_t nr)
+{
-+ switch (source->type) {
-+ case SOURCE_FILE:
-+ return lseek(source->fd, offset, SEEK_SET);
-+ default:
-+ BUG("unknown bulk-checkin source: %d", source->type);
-+ }
++ return read_in_full(source->data.from_fd.fd, buf, nr);
+}
+
-+static ssize_t bulk_checkin_source_read(struct bulk_checkin_source *source,
-+ void *buf, size_t nr)
++static off_t bulk_checkin_source_seek_from_fd(struct bulk_checkin_source *source,
++ off_t offset)
+{
-+ switch (source->type) {
-+ case SOURCE_FILE:
-+ return read_in_full(source->fd, buf, nr);
-+ default:
-+ BUG("unknown bulk-checkin source: %d", source->type);
-+ }
++ return lseek(source->data.from_fd.fd, offset, SEEK_SET);
++}
++
++static void init_bulk_checkin_source_from_fd(struct bulk_checkin_source *source,
++ int fd, size_t size,
++ const char *path)
++{
++ memset(source, 0, sizeof(struct bulk_checkin_source));
++
++ source->read = bulk_checkin_source_read_from_fd;
++ source->seek = bulk_checkin_source_seek_from_fd;
++
++ source->data.from_fd.fd = fd;
++
++ source->size = size;
++ source->path = path;
+}
+
/*
@@ bulk-checkin.c: static int stream_blob_to_pack(struct bulk_checkin_packfile *sta
- ssize_t read_result = read_in_full(fd, ibuf, rsize);
+ ssize_t read_result;
+
-+ read_result = bulk_checkin_source_read(source, ibuf,
-+ rsize);
++ read_result = source->read(source, ibuf, rsize);
if (read_result < 0)
- die_errno("failed to read from '%s'", path);
+ die_errno("failed to read from '%s'",
@@ bulk-checkin.c: static int deflate_blob_to_pack(struct bulk_checkin_packfile *st
unsigned header_len;
struct hashfile_checkpoint checkpoint = {0};
struct pack_idx_entry *idx = NULL;
-+ struct bulk_checkin_source source = {
-+ .type = SOURCE_FILE,
-+ .fd = fd,
-+ .size = size,
-+ .path = path,
-+ };
++ struct bulk_checkin_source source;
++
++ init_bulk_checkin_source_from_fd(&source, fd, size, path);
seekback = lseek(fd, 0, SEEK_CUR);
if (seekback == (off_t) -1)
@@ bulk-checkin.c: static int deflate_blob_to_pack(struct bulk_checkin_packfile *st
state->offset = checkpoint.offset;
flush_bulk_checkin_packfile(state);
- if (lseek(fd, seekback, SEEK_SET) == (off_t) -1)
-+ if (bulk_checkin_source_seek_to(&source, seekback) == (off_t)-1)
++ if (source.seek(&source, seekback) == (off_t)-1)
return error("cannot seek back");
}
the_hash_algo->final_oid_fn(result_oid, &ctx);
2: 9d633df339 < -: ---------- bulk-checkin: generify `stream_blob_to_pack()` for arbitrary types
3: d5bbd7810e ! 2: 596bd028a7 bulk-checkin: refactor deflate routine to accept a `bulk_checkin_source`
@@ Metadata
Author: Taylor Blau <me@ttaylorr.com>
## Commit message ##
- bulk-checkin: refactor deflate routine to accept a `bulk_checkin_source`
+ bulk-checkin: generify `stream_blob_to_pack()` for arbitrary types
- Prepare for a future change where we will want to use a routine very
- similar to the existing `deflate_blob_to_pack()` but over arbitrary
- sources (i.e. either open file-descriptors, or a location in memory).
+ The existing `stream_blob_to_pack()` function is named based on the fact
+ that it knows only how to stream blobs into a bulk-checkin pack.
- Extract out a common "deflate_obj_to_pack()" routine that acts on a
- bulk_checkin_source, instead of a (int, size_t) pair. Then rewrite
- `deflate_blob_to_pack()` in terms of it.
+ But there is no longer anything in this function which prevents us from
+ writing objects of arbitrary types to the bulk-checkin pack. Prepare to
+ write OBJ_TREEs by removing this assumption, adding an `enum
+ object_type` parameter to this function's argument list, and renaming it
+ to `stream_obj_to_pack()` accordingly.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
## bulk-checkin.c ##
+@@ bulk-checkin.c: static void init_bulk_checkin_source_from_fd(struct bulk_checkin_source *source,
+ * status before calling us just in case we ask it to call us again
+ * with a new pack.
+ */
+-static int stream_blob_to_pack(struct bulk_checkin_packfile *state,
+- git_hash_ctx *ctx, off_t *already_hashed_to,
+- struct bulk_checkin_source *source,
+- unsigned flags)
++static int stream_obj_to_pack(struct bulk_checkin_packfile *state,
++ git_hash_ctx *ctx, off_t *already_hashed_to,
++ struct bulk_checkin_source *source,
++ enum object_type type, unsigned flags)
+ {
+ git_zstream s;
+ unsigned char ibuf[16384];
+@@ bulk-checkin.c: static int stream_blob_to_pack(struct bulk_checkin_packfile *state,
+
+ git_deflate_init(&s, pack_compression_level);
+
+- hdrlen = encode_in_pack_object_header(obuf, sizeof(obuf), OBJ_BLOB,
+- size);
++ hdrlen = encode_in_pack_object_header(obuf, sizeof(obuf), type, size);
+ s.next_out = obuf + hdrlen;
+ s.avail_out = sizeof(obuf) - hdrlen;
+
@@ bulk-checkin.c: static void prepare_to_stream(struct bulk_checkin_packfile *state,
die_errno("unable to write pack header");
}
@@ bulk-checkin.c: static void prepare_to_stream(struct bulk_checkin_packfile *stat
unsigned header_len;
struct hashfile_checkpoint checkpoint = {0};
struct pack_idx_entry *idx = NULL;
-- struct bulk_checkin_source source = {
-- .type = SOURCE_FILE,
-- .fd = fd,
-- .size = size,
-- .path = path,
-- };
+- struct bulk_checkin_source source;
+- init_bulk_checkin_source_from_fd(&source, fd, size, path);
+-
- seekback = lseek(fd, 0, SEEK_CUR);
- if (seekback == (off_t) -1)
- return error("cannot find the current offset");
@@ bulk-checkin.c: static int deflate_blob_to_pack(struct bulk_checkin_packfile *st
prepare_to_stream(state, flags);
if (idx) {
@@ bulk-checkin.c: static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
+ idx->offset = state->offset;
crc32_begin(state->f);
}
- if (!stream_obj_to_pack(state, &ctx, &already_hashed_to,
-- &source, OBJ_BLOB, flags))
+- if (!stream_blob_to_pack(state, &ctx, &already_hashed_to,
+- &source, flags))
++ if (!stream_obj_to_pack(state, &ctx, &already_hashed_to,
+ source, type, flags))
break;
/*
@@ bulk-checkin.c: static int deflate_blob_to_pack(struct bulk_checkin_packfile *st
hashfile_truncate(state->f, &checkpoint);
state->offset = checkpoint.offset;
flush_bulk_checkin_packfile(state);
-- if (bulk_checkin_source_seek_to(&source, seekback) == (off_t)-1)
-+ if (bulk_checkin_source_seek_to(source, seekback) == (off_t)-1)
+- if (source.seek(&source, seekback) == (off_t)-1)
++ if (source->seek(source, seekback) == (off_t)-1)
return error("cannot seek back");
}
the_hash_algo->final_oid_fn(result_oid, &ctx);
@@ bulk-checkin.c: static int deflate_blob_to_pack(struct bulk_checkin_packfile *st
+ int fd, size_t size,
+ const char *path, unsigned flags)
+{
-+ struct bulk_checkin_source source = {
-+ .type = SOURCE_FILE,
-+ .fd = fd,
-+ .size = size,
-+ .path = path,
-+ };
-+ off_t seekback = lseek(fd, 0, SEEK_CUR);
++ struct bulk_checkin_source source;
++ off_t seekback;
++
++ init_bulk_checkin_source_from_fd(&source, fd, size, path);
++
++ seekback = lseek(fd, 0, SEEK_CUR);
+ if (seekback == (off_t) -1)
+ return error("cannot find the current offset");
+
4: e427fe6ad3 < -: ---------- bulk-checkin: implement `SOURCE_INCORE` mode for `bulk_checkin_source`
5: 48095afe80 ! 3: d8cf8e4395 bulk-checkin: introduce `index_blob_bulk_checkin_incore()`
@@ Metadata
## Commit message ##
bulk-checkin: introduce `index_blob_bulk_checkin_incore()`
- Now that we have factored out many of the common routines necessary to
- index a new object into a pack created by the bulk-checkin machinery, we
- can introduce a variant of `index_blob_bulk_checkin()` that acts on
- blobs whose contents we can fit in memory.
+ Introduce `index_blob_bulk_checkin_incore()` which allows streaming
+ arbitrary blob contents from memory into the bulk-checkin pack.
+
+ In order to support streaming from a location in memory, we must
+ implement a new kind of bulk_checkin_source that does just that. These
+ implementation in spread out across:
+
+ - init_bulk_checkin_source_incore()
+ - bulk_checkin_source_read_incore()
+ - bulk_checkin_source_seek_incore()
+
+ Note that, unlike file descriptors, which manage their own offset
+ internally, we have to keep track of how many bytes we've read out of
+ the buffer, and make sure we don't read past the end of the buffer.
This will be useful in a couple of more commits in order to provide the
`merge-tree` builtin with a mechanism to create a new pack containing
@@ Commit message
Signed-off-by: Taylor Blau <me@ttaylorr.com>
## bulk-checkin.c ##
+@@ bulk-checkin.c: struct bulk_checkin_source {
+ struct {
+ int fd;
+ } from_fd;
++ struct {
++ const void *buf;
++ size_t nr_read;
++ } incore;
+ } data;
+
+ size_t size;
+@@ bulk-checkin.c: static off_t bulk_checkin_source_seek_from_fd(struct bulk_checkin_source *source
+ return lseek(source->data.from_fd.fd, offset, SEEK_SET);
+ }
+
++static off_t bulk_checkin_source_read_incore(struct bulk_checkin_source *source,
++ void *buf, size_t nr)
++{
++ const unsigned char *src = source->data.incore.buf;
++
++ if (source->data.incore.nr_read > source->size)
++ BUG("read beyond bulk-checkin source buffer end "
++ "(%"PRIuMAX" > %"PRIuMAX")",
++ (uintmax_t)source->data.incore.nr_read,
++ (uintmax_t)source->size);
++
++ if (nr > source->size - source->data.incore.nr_read)
++ nr = source->size - source->data.incore.nr_read;
++
++ src += source->data.incore.nr_read;
++
++ memcpy(buf, src, nr);
++ source->data.incore.nr_read += nr;
++ return nr;
++}
++
++static off_t bulk_checkin_source_seek_incore(struct bulk_checkin_source *source,
++ off_t offset)
++{
++ if (!(0 <= offset && offset < source->size))
++ return (off_t)-1;
++ source->data.incore.nr_read = offset;
++ return source->data.incore.nr_read;
++}
++
+ static void init_bulk_checkin_source_from_fd(struct bulk_checkin_source *source,
+ int fd, size_t size,
+ const char *path)
+@@ bulk-checkin.c: static void init_bulk_checkin_source_from_fd(struct bulk_checkin_source *source,
+ source->path = path;
+ }
+
++static void init_bulk_checkin_source_incore(struct bulk_checkin_source *source,
++ const void *buf, size_t size,
++ const char *path)
++{
++ memset(source, 0, sizeof(struct bulk_checkin_source));
++
++ source->read = bulk_checkin_source_read_incore;
++ source->seek = bulk_checkin_source_seek_incore;
++
++ source->data.incore.buf = buf;
++ source->data.incore.nr_read = 0;
++
++ source->size = size;
++ source->path = path;
++}
++
+ /*
+ * Read the contents from 'source' for 'size' bytes, streaming it to the
+ * packfile in state while updating the hash in ctx. Signal a failure
@@ bulk-checkin.c: static int deflate_obj_to_pack(struct bulk_checkin_packfile *state,
return 0;
}
@@ bulk-checkin.c: static int deflate_obj_to_pack(struct bulk_checkin_packfile *sta
+ const char *path, enum object_type type,
+ unsigned flags)
+{
-+ struct bulk_checkin_source source = {
-+ .type = SOURCE_INCORE,
-+ .buf = buf,
-+ .size = size,
-+ .read = 0,
-+ .path = path,
-+ };
++ struct bulk_checkin_source source;
++
++ init_bulk_checkin_source_incore(&source, buf, size, path);
+
+ return deflate_obj_to_pack(state, result_oid, &source, type, 0, flags);
+}
6: 60568f9281 = 4: 2670192802 bulk-checkin: introduce `index_tree_bulk_checkin_incore()`
7: b9be9df122 ! 5: 3595db76a5 builtin/merge-tree.c: implement support for `--write-pack`
@@ Documentation/git-merge-tree.txt: OPTIONS
## builtin/merge-tree.c ##
@@
- #include "quote.h"
#include "tree.h"
#include "config.h"
+ #include "strvec.h"
+#include "bulk-checkin.h"
static int line_termination = '\n';
@@ builtin/merge-tree.c: struct merge_tree_options {
- int show_messages;
int name_only;
int use_stdin;
+ struct merge_options merge_options;
+ int write_pack;
};
static int real_merge(struct merge_tree_options *o,
@@ builtin/merge-tree.c: static int real_merge(struct merge_tree_options *o,
- init_merge_options(&opt, the_repository);
+ _("not something we can merge"));
opt.show_rename_progress = 0;
+ opt.write_pack = o->write_pack;
@@ builtin/merge-tree.c: static int real_merge(struct merge_tree_options *o,
opt.branch1 = branch1;
opt.branch2 = branch2;
@@ builtin/merge-tree.c: int cmd_merge_tree(int argc, const char **argv, const char *prefix)
- &merge_base,
- N_("commit"),
N_("specify a merge-base for the merge")),
+ OPT_STRVEC('X', "strategy-option", &xopts, N_("option=value"),
+ N_("option for selected merge strategy")),
+ OPT_BOOL(0, "write-pack", &o.write_pack,
+ N_("write new objects to a pack instead of as loose")),
OPT_END()
base-commit: ceadf0f3cf51550166a387ec8508bb55e7883057
--
2.42.0.425.g963d08ddb3.dirty
^ permalink raw reply
* Re: [PATCH 0/7] log: decorate pseudorefs and other refs
From: Andy Koppe @ 2023-10-23 22:15 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, stolee
In-Reply-To: <xmqqpm16p4t3.fsf@gitster.g>
On 23/10/2023 01:20, Junio C Hamano wrote:
> Andy Koppe <andy.koppe@gmail.com> writes:
>
>>> [2/7] is a trivial readability improvement. It obviously should be
>>> left outside the scope of this series, but we should notice
>>> the same pattern in similar color tables (e.g., wt-status.c
>>> has one, diff.c has another) and perform the same clean-up as
>>> a #leftoverbits item.
>>
>> Okay, I've removed that commit in v2. (I should have mentioned in the
>> commit message that it was triggered by the inconsistency with the
>> immediately following color_decorate_slots array, which uses
>> designated initializers.)
>
> Sorry, that is not what I meant. [2/7] as a preliminary clean-up to
> work in the same area does make very much sense. What I meant to be
> "outside the scope" was to make similar fixes to other color tables
> that this series does not care about.
Ah, sorry for misreading. Commit reinstated in v3.
>> Fair enough, although the array already contains HEAD and refs/stash
>> as singletons.
>
> But these deserve to be singletons, don't they? There is no other
> thing that behaves like HEAD; there is no other thing that behaves
> like stash; and they do not behave like each other.
They do indeed, but arguably the pseudorefs are singletons rather than a
namespace like refs/heads as well, as there is a defined and documented
set of them.
>> I've rewritten things to not touch the ref_namespace array.
>
> Well, the namespace_info mechanism still may be a good place to have
> the necessary information; it may be that the current implementation
> detail of how a given ref is classified to one of the namespaces is
> too limiting---it essentially allows the string match with the .ref
> member. But we can imagine that it could be extended a bit, e.g.
>
> struct ref_namespace_info {
> char *ref;
> int (*membership)(const char *, const struct ref_namespace_info *);
> ... other members ...;
> };
>
> where the .membership member is used in add_ref_decoration() to
> determine the membership of a given "refname" to the namespace "i"
> perhaps like so:
>
> struct ref_namespace_info *info = &ref_namespace[i];
>
> if (!info->decoration)
> continue;
> + if (info->membership) {
> + if (info->membership(refname, info)) {
> + deco_type = info->decoration;
> + break;
> + }
> + } else if (info->exact) {
> - if (info->exact) {
> if (!strcmp(refname, info->ref)) {
> deco_type = info_decoration;
> break;
> }
>
> Then you can arrange the pseudoref class to use .membership function
> perhaps like this:
>
> static int pseudoref_namespace_membership(
> const char *refname, const struct ref_namespace_info *info UNUSED
> )
> {
> return is_pseudoref(refname);
> }
>
> and make them all into a single class.
That's an interesting idea, but I'm not convinced it would buy us much,
while also potentially complicating things for any other uses of the
ref_namespace array.
My premise here is that we do need a list of the documented pseudorefs,
so that we can iterate through them and add the ones that do exist to
the decorations, whereby I admit that shoe-horning that list into the
ref_namespace array wasn't a good idea. If that premise is wrong, and
there's a better way to discover the pseudorefs, the following might be
moot.
Sending each found pseudoref through add_ref_decoration() and its lookup
of ref_namespace would just confirm what we already know: it's a
pseudoref. Which is why both my initial attempt and the current one
don't actually invoke add_ref_decoration() for them.
Could you have a closer look at the current design? It handles the
pseudorefs separately from proper refs, with their own iteration and
callback functions, which I think makes for simpler more self-contained
changes than v1 or the approach suggested above.
> Having said that, I do not think it makes much sense to decorate a
> commit off of refs/stash, as the true richeness of the stash is not
> in its history but in its reflog, which the decoration code does not
> dig into. But obviously it is not a part of the topic we are
> discussing (unless, of course, we are not "adding" new decoration
> sources and colors, but we are improving the decoration sources and
> colors by adding new useful ones while retiring existing useless
> ones).
I agree refs/stash is a weird one, and that it could be subsumed into
the color.decoration.ref setting for 'refs/*' that I'm adding here,
which is also why I chose the same default color for it. I'd be happy to
drop color.decoration.stash if the minor break in compatibility for
anyone who has customized it is acceptable. The setting would be quietly
ignored.
Another related thought: the '--clear-decorations' option of git-log
seems unfortunately named as it suggests the opposite of what it
actually does, which is to enable all decorations (unless subsequently
constrained with '--decorate-refs{,--exclude}=...').
Regards,
Andy
^ permalink raw reply
* [PATCH v3 7/7] log: add color.decorate.pseudoref config variable
From: Andy Koppe @ 2023-10-23 22:11 UTC (permalink / raw)
To: git; +Cc: gitster, stolee, Andy Koppe
In-Reply-To: <20231023221143.72489-1-andy.koppe@gmail.com>
Add the ability to show pseudorefs such as ORIG_HEAD and MERGE_HEAD in
log decorations. Add config variable color.decorate.pseudoref to
determine their color, defaulting to bold cyan, which is the same as
HEAD.
They will not be shown unless the default decoration filtering is
overridden with relevant log options such as --clear-decorations or
log.initialDecorationSet.
Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
---
Documentation/config/color.txt | 4 +++-
commit.h | 1 +
log-tree.c | 24 +++++++++++++++++++
..._--decorate=full_--clear-decorations_--all | 4 ++--
...f.log_--decorate_--clear-decorations_--all | 4 ++--
t/t4202-log.sh | 21 +++++++++-------
t/t4207-log-decoration-colors.sh | 13 +++++++---
7 files changed, 54 insertions(+), 17 deletions(-)
diff --git a/Documentation/config/color.txt b/Documentation/config/color.txt
index 005a2bdb03..7af7d65f76 100644
--- a/Documentation/config/color.txt
+++ b/Documentation/config/color.txt
@@ -92,6 +92,8 @@ color.decorate.<slot>::
the stash ref
`ref`;;
any other refs (not shown by default)
+`pseudoref`;;
+ pseudorefs such as ORIG_HEAD or MERGE_HEAD (not shown by default)
`grafted`;;
grafted and replaced commits
`symbol`;;
@@ -99,7 +101,7 @@ color.decorate.<slot>::
--
+
(Variable `log.initialDecorationSet` or linkgit:git-log[1] option
-`--clear-decorations` can be used to show all refs.)
+`--clear-decorations` can be used to show all refs and pseudorefs.)
color.grep::
When set to `always`, always highlight matches. When `false` (or
diff --git a/commit.h b/commit.h
index f6b2125fc4..44dd3ce19b 100644
--- a/commit.h
+++ b/commit.h
@@ -56,6 +56,7 @@ enum decoration_type {
DECORATION_REF_STASH,
DECORATION_REF,
DECORATION_REF_HEAD,
+ DECORATION_REF_PSEUDO,
DECORATION_GRAFTED,
DECORATION_SYMBOL,
};
diff --git a/log-tree.c b/log-tree.c
index fb3d87b83d..65ebb74d40 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -41,6 +41,7 @@ static char decoration_colors[][COLOR_MAXLEN] = {
[DECORATION_REF_STASH] = GIT_COLOR_BOLD_MAGENTA,
[DECORATION_REF] = GIT_COLOR_BOLD_MAGENTA,
[DECORATION_REF_HEAD] = GIT_COLOR_BOLD_CYAN,
+ [DECORATION_REF_PSEUDO] = GIT_COLOR_BOLD_CYAN,
[DECORATION_GRAFTED] = GIT_COLOR_BOLD_BLUE,
[DECORATION_SYMBOL] = GIT_COLOR_NIL,
};
@@ -52,6 +53,7 @@ static const char *color_decorate_slots[] = {
[DECORATION_REF_STASH] = "stash",
[DECORATION_REF] = "ref",
[DECORATION_REF_HEAD] = "HEAD",
+ [DECORATION_REF_PSEUDO] = "pseudoref",
[DECORATION_GRAFTED] = "grafted",
[DECORATION_SYMBOL] = "symbol",
};
@@ -208,6 +210,27 @@ static int add_ref_decoration(const char *refname, const struct object_id *oid,
return 0;
}
+static int add_pseudoref_decoration(const char *refname,
+ const struct object_id *oid,
+ int flags UNUSED,
+ void *cb_data)
+{
+ struct object *obj;
+ enum object_type objtype;
+ struct decoration_filter *filter = (struct decoration_filter *)cb_data;
+
+ if (filter && !ref_filter_match(refname, filter))
+ return 0;
+
+ objtype = oid_object_info(the_repository, oid, NULL);
+ if (objtype < 0)
+ return 0;
+
+ obj = lookup_object_by_type(the_repository, oid, objtype);
+ add_name_decoration(DECORATION_REF_PSEUDO, refname, obj);
+ return 0;
+}
+
static int add_graft_decoration(const struct commit_graft *graft,
void *cb_data UNUSED)
{
@@ -236,6 +259,7 @@ void load_ref_decorations(struct decoration_filter *filter, int flags)
decoration_loaded = 1;
decoration_flags = flags;
for_each_ref(add_ref_decoration, filter);
+ for_each_pseudoref(add_pseudoref_decoration, filter);
head_ref(add_ref_decoration, filter);
for_each_commit_graft(add_graft_decoration, filter);
}
diff --git a/t/t4013/diff.log_--decorate=full_--clear-decorations_--all b/t/t4013/diff.log_--decorate=full_--clear-decorations_--all
index 1c030a6554..7d16978e7f 100644
--- a/t/t4013/diff.log_--decorate=full_--clear-decorations_--all
+++ b/t/t4013/diff.log_--decorate=full_--clear-decorations_--all
@@ -33,13 +33,13 @@ Date: Mon Jun 26 00:04:00 2006 +0000
Merge branch 'side'
-commit c7a2ab9e8eac7b117442a607d5a9b3950ae34d5a (refs/heads/side)
+commit c7a2ab9e8eac7b117442a607d5a9b3950ae34d5a (FETCH_HEAD, refs/heads/side)
Author: A U Thor <author@example.com>
Date: Mon Jun 26 00:03:00 2006 +0000
Side
-commit 9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0
+commit 9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0 (ORIG_HEAD)
Author: A U Thor <author@example.com>
Date: Mon Jun 26 00:02:00 2006 +0000
diff --git a/t/t4013/diff.log_--decorate_--clear-decorations_--all b/t/t4013/diff.log_--decorate_--clear-decorations_--all
index 88be82cce3..4f9be50ce0 100644
--- a/t/t4013/diff.log_--decorate_--clear-decorations_--all
+++ b/t/t4013/diff.log_--decorate_--clear-decorations_--all
@@ -33,13 +33,13 @@ Date: Mon Jun 26 00:04:00 2006 +0000
Merge branch 'side'
-commit c7a2ab9e8eac7b117442a607d5a9b3950ae34d5a (side)
+commit c7a2ab9e8eac7b117442a607d5a9b3950ae34d5a (FETCH_HEAD, side)
Author: A U Thor <author@example.com>
Date: Mon Jun 26 00:03:00 2006 +0000
Side
-commit 9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0
+commit 9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0 (ORIG_HEAD)
Author: A U Thor <author@example.com>
Date: Mon Jun 26 00:02:00 2006 +0000
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index af4a123cd2..b14da62e3e 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -927,7 +927,7 @@ test_expect_success 'multiple decorate-refs' '
test_expect_success 'decorate-refs-exclude with glob' '
cat >expect.decorate <<-\EOF &&
Merge-tag-reach (HEAD -> main)
- Merge-tags-octopus-a-and-octopus-b
+ Merge-tags-octopus-a-and-octopus-b (ORIG_HEAD)
seventh (tag: seventh)
octopus-b (tag: octopus-b)
octopus-a (tag: octopus-a)
@@ -944,7 +944,7 @@ test_expect_success 'decorate-refs-exclude with glob' '
test_expect_success 'decorate-refs-exclude without globs' '
cat >expect.decorate <<-\EOF &&
Merge-tag-reach (HEAD -> main)
- Merge-tags-octopus-a-and-octopus-b
+ Merge-tags-octopus-a-and-octopus-b (ORIG_HEAD)
seventh (tag: seventh)
octopus-b (tag: octopus-b, octopus-b)
octopus-a (tag: octopus-a, octopus-a)
@@ -961,7 +961,7 @@ test_expect_success 'decorate-refs-exclude without globs' '
test_expect_success 'multiple decorate-refs-exclude' '
cat >expect.decorate <<-\EOF &&
Merge-tag-reach (HEAD -> main)
- Merge-tags-octopus-a-and-octopus-b
+ Merge-tags-octopus-a-and-octopus-b (ORIG_HEAD)
seventh (tag: seventh)
octopus-b (tag: octopus-b)
octopus-a (tag: octopus-a)
@@ -1022,10 +1022,12 @@ test_expect_success 'decorate-refs-exclude and simplify-by-decoration' '
EOF
git log -n6 --decorate=short --pretty="tformat:%f%d" \
--decorate-refs-exclude="*octopus*" \
+ --decorate-refs-exclude="ORIG_HEAD" \
--simplify-by-decoration >actual &&
test_cmp expect.decorate actual &&
- git -c log.excludeDecoration="*octopus*" log \
- -n6 --decorate=short --pretty="tformat:%f%d" \
+ git -c log.excludeDecoration="*octopus*" \
+ -c log.excludeDecoration="ORIG_HEAD" \
+ log -n6 --decorate=short --pretty="tformat:%f%d" \
--simplify-by-decoration >actual &&
test_cmp expect.decorate actual
'
@@ -1067,9 +1069,10 @@ test_expect_success 'decorate-refs and simplify-by-decoration without output' '
test_cmp expect actual
'
-test_expect_success 'decorate-refs-exclude HEAD' '
+test_expect_success 'decorate-refs-exclude HEAD ORIG_HEAD' '
git log --decorate=full --oneline \
- --decorate-refs-exclude="HEAD" >actual &&
+ --decorate-refs-exclude="HEAD" \
+ --decorate-refs-exclude="ORIG_HEAD" >actual &&
! grep HEAD actual
'
@@ -1107,7 +1110,7 @@ test_expect_success '--clear-decorations overrides defaults' '
cat >expect.all <<-\EOF &&
Merge-tag-reach (HEAD -> refs/heads/main)
- Merge-tags-octopus-a-and-octopus-b
+ Merge-tags-octopus-a-and-octopus-b (ORIG_HEAD)
seventh (tag: refs/tags/seventh)
octopus-b (tag: refs/tags/octopus-b, refs/heads/octopus-b)
octopus-a (tag: refs/tags/octopus-a, refs/heads/octopus-a)
@@ -1139,7 +1142,7 @@ test_expect_success '--clear-decorations clears previous exclusions' '
cat >expect.all <<-\EOF &&
Merge-tag-reach (HEAD -> refs/heads/main)
reach (tag: refs/tags/reach, refs/heads/reach)
- Merge-tags-octopus-a-and-octopus-b
+ Merge-tags-octopus-a-and-octopus-b (ORIG_HEAD)
octopus-b (tag: refs/tags/octopus-b, refs/heads/octopus-b)
octopus-a (tag: refs/tags/octopus-a, refs/heads/octopus-a)
seventh (tag: refs/tags/seventh)
diff --git a/t/t4207-log-decoration-colors.sh b/t/t4207-log-decoration-colors.sh
index 4b51e34f8b..0b32e0bb8e 100755
--- a/t/t4207-log-decoration-colors.sh
+++ b/t/t4207-log-decoration-colors.sh
@@ -18,6 +18,7 @@ test_expect_success setup '
git config color.decorate.tag "reverse bold yellow" &&
git config color.decorate.stash magenta &&
git config color.decorate.ref blue &&
+ git config color.decorate.pseudoref "bold cyan" &&
git config color.decorate.grafted black &&
git config color.decorate.symbol white &&
git config color.decorate.HEAD cyan &&
@@ -30,6 +31,7 @@ test_expect_success setup '
c_tag="<BOLD;REVERSE;YELLOW>" &&
c_stash="<MAGENTA>" &&
c_ref="<BLUE>" &&
+ c_pseudoref="<BOLD;CYAN>" &&
c_HEAD="<CYAN>" &&
c_grafted="<BLACK>" &&
c_symbol="<WHITE>" &&
@@ -46,7 +48,10 @@ test_expect_success setup '
test_commit B &&
git tag v1.0 &&
echo >>A.t &&
- git stash save Changes to A.t
+ git stash save Changes to A.t &&
+ git reset other/main &&
+ git reset ORIG_HEAD &&
+ git revert --no-commit @~
'
cmp_filtered_decorations () {
@@ -63,17 +68,19 @@ ${c_symbol} -> ${c_reset}${c_branch}main${c_reset}${c_symbol}, ${c_reset}\
${c_tag}tag: ${c_reset}${c_tag}v1.0${c_reset}${c_symbol}, ${c_reset}\
${c_tag}tag: ${c_reset}${c_tag}B${c_reset}${c_symbol})${c_reset} B
${c_commit}COMMIT_ID${c_reset}${c_symbol} (${c_reset}\
+${c_pseudoref}ORIG_HEAD${c_reset}${c_symbol}, ${c_reset}\
${c_tag}tag: ${c_reset}${c_tag}A1${c_reset}${c_symbol}, ${c_reset}\
${c_remoteBranch}other/main${c_reset}${c_symbol})${c_reset} A1
${c_commit}COMMIT_ID${c_reset}${c_symbol} (${c_reset}\
${c_stash}refs/stash${c_reset}${c_symbol})${c_reset} On main: Changes to A.t
${c_commit}COMMIT_ID${c_reset}${c_symbol} (${c_reset}\
+${c_pseudoref}REVERT_HEAD${c_reset}${c_symbol}, ${c_reset}\
${c_tag}tag: ${c_reset}${c_tag}A${c_reset}${c_symbol}, ${c_reset}\
${c_ref}refs/foo${c_reset}${c_symbol})${c_reset} A
EOF
- git log --first-parent --no-abbrev --decorate --clear-decorations \
- --oneline --color=always --all >actual &&
+ git log --first-parent --no-abbrev --decorate --color=always \
+ --decorate-refs-exclude=FETCH_HEAD --oneline --all >actual &&
cmp_filtered_decorations
'
--
2.42.GIT
^ permalink raw reply related
* [PATCH v3 6/7] refs: exempt pseudorefs from pattern prefixing
From: Andy Koppe @ 2023-10-23 22:11 UTC (permalink / raw)
To: git; +Cc: gitster, stolee, Andy Koppe
In-Reply-To: <20231023221143.72489-1-andy.koppe@gmail.com>
In normalize_glob_ref(), don't prefix pseudorefs with "refs/", thereby
implementing a NEEDSWORK from b877e617e6e5.
This is in preparation for showing pseudorefs in log decorations, as
they are not matched as intended in decoration filters otherwise. The
function is only used in load_ref_decorations().
Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
---
refs.c | 17 ++++++++++-------
1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/refs.c b/refs.c
index aa7e4c02c5..fbd15a8cff 100644
--- a/refs.c
+++ b/refs.c
@@ -565,13 +565,16 @@ void normalize_glob_ref(struct string_list_item *item, const char *prefix,
if (prefix)
strbuf_addstr(&normalized_pattern, prefix);
- else if (!starts_with(pattern, "refs/") &&
- strcmp(pattern, "HEAD"))
- strbuf_addstr(&normalized_pattern, "refs/");
- /*
- * NEEDSWORK: Special case other symrefs such as REBASE_HEAD,
- * MERGE_HEAD, etc.
- */
+ else if (!starts_with(pattern, "refs/") && strcmp(pattern, "HEAD")) {
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(pseudorefs); i++)
+ if (!strcmp(pattern, pseudorefs[i]))
+ break;
+
+ if (i == ARRAY_SIZE(pseudorefs))
+ strbuf_addstr(&normalized_pattern, "refs/");
+ }
strbuf_addstr(&normalized_pattern, pattern);
strbuf_strip_suffix(&normalized_pattern, "/");
--
2.42.GIT
^ permalink raw reply related
* [PATCH v3 3/7] log: add color.decorate.symbol config variable
From: Andy Koppe @ 2023-10-23 22:11 UTC (permalink / raw)
To: git; +Cc: gitster, stolee, Andy Koppe
In-Reply-To: <20231023221143.72489-1-andy.koppe@gmail.com>
Add new color.decorate.symbol config variable for determining the
color of the prefix, suffix, separator and pointer symbols used in
log --decorate output and related log format placeholders, to allow
them to be colored differently from commit hashes.
For backward compatibility, fall back to the commit hash color that can
be specified with the color.diff.commit variable if the new variable is
not provided.
Add the variable to the color.decorate.<slot> documentation.
Amend t4207-log-decoration-colors.sh to test it. Put ${c_reset} elements
in the expected output at the end of lines for consistency.
Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
---
Documentation/config/color.txt | 2 ++
commit.h | 1 +
log-tree.c | 15 ++++++---
t/t4207-log-decoration-colors.sh | 58 +++++++++++++++++---------------
4 files changed, 43 insertions(+), 33 deletions(-)
diff --git a/Documentation/config/color.txt b/Documentation/config/color.txt
index 3453703f9b..cc0a881125 100644
--- a/Documentation/config/color.txt
+++ b/Documentation/config/color.txt
@@ -92,6 +92,8 @@ color.decorate.<slot>::
the stash ref
`grafted`;;
grafted and replaced commits
+`symbol`;;
+ punctuation symbols surrounding the other elements
--
color.grep::
diff --git a/commit.h b/commit.h
index 28928833c5..cb13e4d5ba 100644
--- a/commit.h
+++ b/commit.h
@@ -56,6 +56,7 @@ enum decoration_type {
DECORATION_REF_STASH,
DECORATION_REF_HEAD,
DECORATION_GRAFTED,
+ DECORATION_SYMBOL,
};
void add_name_decoration(enum decoration_type type, const char *name, struct object *obj);
diff --git a/log-tree.c b/log-tree.c
index 8bdf889f02..890024f205 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -41,6 +41,7 @@ static char decoration_colors[][COLOR_MAXLEN] = {
[DECORATION_REF_STASH] = GIT_COLOR_BOLD_MAGENTA,
[DECORATION_REF_HEAD] = GIT_COLOR_BOLD_CYAN,
[DECORATION_GRAFTED] = GIT_COLOR_BOLD_BLUE,
+ [DECORATION_SYMBOL] = GIT_COLOR_NIL,
};
static const char *color_decorate_slots[] = {
@@ -50,6 +51,7 @@ static const char *color_decorate_slots[] = {
[DECORATION_REF_STASH] = "stash",
[DECORATION_REF_HEAD] = "HEAD",
[DECORATION_GRAFTED] = "grafted",
+ [DECORATION_SYMBOL] = "symbol",
};
static const char *decorate_get_color(int decorate_use_color, enum decoration_type ix)
@@ -312,7 +314,7 @@ void format_decorations(struct strbuf *sb,
{
const struct name_decoration *decoration;
const struct name_decoration *current_and_HEAD;
- const char *color_commit, *color_reset;
+ const char *color_symbol, *color_reset;
const char *prefix = " (";
const char *suffix = ")";
@@ -337,7 +339,10 @@ void format_decorations(struct strbuf *sb,
tag = opts->tag;
}
- color_commit = diff_get_color(use_color, DIFF_COMMIT);
+ color_symbol = decorate_get_color(use_color, DECORATION_SYMBOL);
+ if (color_is_nil(color_symbol))
+ color_symbol = diff_get_color(use_color, DIFF_COMMIT);
+
color_reset = decorate_get_color(use_color, DECORATION_NONE);
current_and_HEAD = current_pointed_by_HEAD(decoration);
@@ -352,7 +357,7 @@ void format_decorations(struct strbuf *sb,
decorate_get_color(use_color, decoration->type);
if (*prefix) {
- strbuf_addstr(sb, color_commit);
+ strbuf_addstr(sb, color_symbol);
strbuf_addstr(sb, prefix);
strbuf_addstr(sb, color_reset);
}
@@ -369,7 +374,7 @@ void format_decorations(struct strbuf *sb,
if (current_and_HEAD &&
decoration->type == DECORATION_REF_HEAD) {
- strbuf_addstr(sb, color_commit);
+ strbuf_addstr(sb, color_symbol);
strbuf_addstr(sb, pointer);
strbuf_addstr(sb, color_reset);
strbuf_addstr(sb, decorate_get_color(use_color, current_and_HEAD->type));
@@ -382,7 +387,7 @@ void format_decorations(struct strbuf *sb,
decoration = decoration->next;
}
if (*suffix) {
- strbuf_addstr(sb, color_commit);
+ strbuf_addstr(sb, color_symbol);
strbuf_addstr(sb, suffix);
strbuf_addstr(sb, color_reset);
}
diff --git a/t/t4207-log-decoration-colors.sh b/t/t4207-log-decoration-colors.sh
index 21986a866d..f4173b6114 100755
--- a/t/t4207-log-decoration-colors.sh
+++ b/t/t4207-log-decoration-colors.sh
@@ -18,6 +18,7 @@ test_expect_success setup '
git config color.decorate.tag "reverse bold yellow" &&
git config color.decorate.stash magenta &&
git config color.decorate.grafted black &&
+ git config color.decorate.symbol white &&
git config color.decorate.HEAD cyan &&
c_reset="<RESET>" &&
@@ -29,6 +30,7 @@ test_expect_success setup '
c_stash="<MAGENTA>" &&
c_HEAD="<CYAN>" &&
c_grafted="<BLACK>" &&
+ c_symbol="<WHITE>" &&
test_commit A &&
git clone . other &&
@@ -53,17 +55,17 @@ cmp_filtered_decorations () {
# to this test since it does not contain any decoration, hence --first-parent
test_expect_success 'commit decorations colored correctly' '
cat >expect <<-EOF &&
- ${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_HEAD}HEAD${c_reset}\
-${c_commit} -> ${c_reset}${c_branch}main${c_reset}${c_commit}, \
-${c_reset}${c_tag}tag: ${c_reset}${c_tag}v1.0${c_reset}${c_commit}, \
-${c_reset}${c_tag}tag: ${c_reset}${c_tag}B${c_reset}${c_commit})${c_reset} B
-${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}\
-${c_tag}tag: ${c_reset}${c_tag}A1${c_reset}${c_commit}, \
-${c_reset}${c_remoteBranch}other/main${c_reset}${c_commit})${c_reset} A1
- ${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}\
-${c_stash}refs/stash${c_reset}${c_commit})${c_reset} On main: Changes to A.t
- ${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}\
-${c_tag}tag: ${c_reset}${c_tag}A${c_reset}${c_commit})${c_reset} A
+ ${c_commit}COMMIT_ID${c_reset}${c_symbol} (${c_reset}${c_HEAD}HEAD${c_reset}\
+${c_symbol} -> ${c_reset}${c_branch}main${c_reset}${c_symbol}, ${c_reset}\
+${c_tag}tag: ${c_reset}${c_tag}v1.0${c_reset}${c_symbol}, ${c_reset}\
+${c_tag}tag: ${c_reset}${c_tag}B${c_reset}${c_symbol})${c_reset} B
+${c_commit}COMMIT_ID${c_reset}${c_symbol} (${c_reset}\
+${c_tag}tag: ${c_reset}${c_tag}A1${c_reset}${c_symbol}, ${c_reset}\
+${c_remoteBranch}other/main${c_reset}${c_symbol})${c_reset} A1
+ ${c_commit}COMMIT_ID${c_reset}${c_symbol} (${c_reset}\
+${c_stash}refs/stash${c_reset}${c_symbol})${c_reset} On main: Changes to A.t
+ ${c_commit}COMMIT_ID${c_reset}${c_symbol} (${c_reset}\
+${c_tag}tag: ${c_reset}${c_tag}A${c_reset}${c_symbol})${c_reset} A
EOF
git log --first-parent --no-abbrev --decorate --oneline --color=always --all >actual &&
@@ -78,14 +80,14 @@ test_expect_success 'test coloring with replace-objects' '
git replace HEAD~1 HEAD~2 &&
cat >expect <<-EOF &&
- ${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_HEAD}HEAD${c_reset}\
-${c_commit} -> ${c_reset}${c_branch}main${c_reset}${c_commit}, \
-${c_reset}${c_tag}tag: ${c_reset}${c_tag}D${c_reset}${c_commit})${c_reset} D
- ${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}\
-${c_tag}tag: ${c_reset}${c_tag}C${c_reset}${c_commit}, \
-${c_reset}${c_grafted}replaced${c_reset}${c_commit})${c_reset} B
- ${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}\
-${c_tag}tag: ${c_reset}${c_tag}A${c_reset}${c_commit})${c_reset} A
+ ${c_commit}COMMIT_ID${c_reset}${c_symbol} (${c_reset}${c_HEAD}HEAD${c_reset}\
+${c_symbol} -> ${c_reset}${c_branch}main${c_reset}${c_symbol}, ${c_reset}\
+${c_tag}tag: ${c_reset}${c_tag}D${c_reset}${c_symbol})${c_reset} D
+ ${c_commit}COMMIT_ID${c_reset}${c_symbol} (${c_reset}\
+${c_tag}tag: ${c_reset}${c_tag}C${c_reset}${c_symbol}, ${c_reset}\
+${c_grafted}replaced${c_reset}${c_symbol})${c_reset} B
+ ${c_commit}COMMIT_ID${c_reset}${c_symbol} (${c_reset}\
+${c_tag}tag: ${c_reset}${c_tag}A${c_reset}${c_symbol})${c_reset} A
EOF
git log --first-parent --no-abbrev --decorate --oneline --color=always HEAD >actual &&
@@ -104,15 +106,15 @@ test_expect_success 'test coloring with grafted commit' '
git replace --graft HEAD HEAD~2 &&
cat >expect <<-EOF &&
- ${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_HEAD}HEAD${c_reset}\
-${c_commit} -> ${c_reset}${c_branch}main${c_reset}${c_commit}, \
-${c_reset}${c_tag}tag: ${c_reset}${c_tag}D${c_reset}${c_commit}, \
-${c_reset}${c_grafted}replaced${c_reset}${c_commit})${c_reset} D
- ${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}\
-${c_tag}tag: ${c_reset}${c_tag}v1.0${c_reset}${c_commit}, \
-${c_reset}${c_tag}tag: ${c_reset}${c_tag}B${c_reset}${c_commit})${c_reset} B
- ${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}\
-${c_tag}tag: ${c_reset}${c_tag}A${c_reset}${c_commit})${c_reset} A
+ ${c_commit}COMMIT_ID${c_reset}${c_symbol} (${c_reset}${c_HEAD}HEAD${c_reset}\
+${c_symbol} -> ${c_reset}${c_branch}main${c_reset}${c_symbol}, ${c_reset}\
+${c_tag}tag: ${c_reset}${c_tag}D${c_reset}${c_symbol}, ${c_reset}\
+${c_grafted}replaced${c_reset}${c_symbol})${c_reset} D
+ ${c_commit}COMMIT_ID${c_reset}${c_symbol} (${c_reset}\
+${c_tag}tag: ${c_reset}${c_tag}v1.0${c_reset}${c_symbol}, ${c_reset}\
+${c_tag}tag: ${c_reset}${c_tag}B${c_reset}${c_symbol})${c_reset} B
+ ${c_commit}COMMIT_ID${c_reset}${c_symbol} (${c_reset}\
+${c_tag}tag: ${c_reset}${c_tag}A${c_reset}${c_symbol})${c_reset} A
EOF
git log --first-parent --no-abbrev --decorate --oneline --color=always HEAD >actual &&
--
2.42.GIT
^ permalink raw reply related
* [PATCH v3 5/7] refs: add pseudorefs array and iteration functions
From: Andy Koppe @ 2023-10-23 22:11 UTC (permalink / raw)
To: git; +Cc: gitster, stolee, Andy Koppe
In-Reply-To: <20231023221143.72489-1-andy.koppe@gmail.com>
Define const array 'pseudorefs' with the names of the pseudorefs that
are documented in gitrevisions.1, and add functions for_each_pseudoref()
and refs_for_each_pseudoref() for iterating over them.
The functions process the pseudorefs in the same way as head_ref() and
refs_head_ref() process HEAD, invoking an each_ref_fn callback on each
pseudoref that exists.
This is in preparation for adding pseudorefs to log decorations.
Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
---
refs.c | 42 ++++++++++++++++++++++++++++++++++++++++++
refs.h | 5 +++++
2 files changed, 47 insertions(+)
diff --git a/refs.c b/refs.c
index fcae5dddc6..aa7e4c02c5 100644
--- a/refs.c
+++ b/refs.c
@@ -65,6 +65,21 @@ static unsigned char refname_disposition[256] = {
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 3, 0, 0, 4, 4
};
+/*
+ * List of documented pseudorefs. This needs to be kept in sync with the list
+ * in Documentation/revisions.txt.
+ */
+static const char *const pseudorefs[] = {
+ "FETCH_HEAD",
+ "ORIG_HEAD",
+ "MERGE_HEAD",
+ "REBASE_HEAD",
+ "CHERRY_PICK_HEAD",
+ "REVERT_HEAD",
+ "BISECT_HEAD",
+ "AUTO_MERGE",
+};
+
struct ref_namespace_info ref_namespace[] = {
[NAMESPACE_HEAD] = {
.ref = "HEAD",
@@ -1549,6 +1564,33 @@ int head_ref(each_ref_fn fn, void *cb_data)
return refs_head_ref(get_main_ref_store(the_repository), fn, cb_data);
}
+int refs_for_each_pseudoref(struct ref_store *refs,
+ each_ref_fn fn, void *cb_data)
+{
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(pseudorefs); i++) {
+ struct object_id oid;
+ int flag;
+
+ if (refs_resolve_ref_unsafe(refs, pseudorefs[i],
+ RESOLVE_REF_READING, &oid, &flag)) {
+ int ret = fn(pseudorefs[i], &oid, flag, cb_data);
+
+ if (ret)
+ return ret;
+ }
+ }
+
+ return 0;
+}
+
+int for_each_pseudoref(each_ref_fn fn, void *cb_data)
+{
+ return refs_for_each_pseudoref(get_main_ref_store(the_repository),
+ fn, cb_data);
+}
+
struct ref_iterator *refs_ref_iterator_begin(
struct ref_store *refs,
const char *prefix,
diff --git a/refs.h b/refs.h
index 23211a5ea1..7b55cced31 100644
--- a/refs.h
+++ b/refs.h
@@ -320,6 +320,8 @@ typedef int each_repo_ref_fn(struct repository *r,
*/
int refs_head_ref(struct ref_store *refs,
each_ref_fn fn, void *cb_data);
+int refs_for_each_pseudoref(struct ref_store *refs,
+ each_ref_fn fn, void *cb_data);
int refs_for_each_ref(struct ref_store *refs,
each_ref_fn fn, void *cb_data);
int refs_for_each_ref_in(struct ref_store *refs, const char *prefix,
@@ -334,6 +336,9 @@ int refs_for_each_remote_ref(struct ref_store *refs,
/* just iterates the head ref. */
int head_ref(each_ref_fn fn, void *cb_data);
+/* iterates pseudorefs. */
+int for_each_pseudoref(each_ref_fn fn, void *cb_data);
+
/* iterates all refs. */
int for_each_ref(each_ref_fn fn, void *cb_data);
--
2.42.GIT
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox