* Re: Why does the includeif woks how it does?
From: Dominik von Haller @ 2024-02-20 6:54 UTC (permalink / raw)
To: Junio C Hamano
Cc: brian m. carlson, git@vger.kernel.org, rsbecker@nexbridge.com
Thanks, you all for your insights. <3
So to summarize, it was implemented through a gitdir because of possible confusions with the working directory. And there is no other implementation because the use case is not there.
For the new implementation I would always use the working directory. If set with git -C than use that. Should also not be a Problem because the working directory it is set internally anyway.
As far as the possible implementation of this goes, I would prefer an includeif with an extra option dir instead of gitdir. This would prevent confusion between includewhere and includeif.
The mentioned Problems seem to be easily figured out.
Every includeif == true will get included. Every more specific path will overwrite properties if set before. Gitdir is more specific than just dir.
Somewhat like that I would say is very intuitive.
Another possible approach which I would also say is logical is a top-down approach. Every Includeif would be executed from top to bottom. Every true includeif will be included and as before would overwrite properties if necessary.
Best regards
Dominik von Haller
^ permalink raw reply
* [PATCH v2] documentation: send-email: use camel case consistently
From: Dragan Simic @ 2024-02-20 7:04 UTC (permalink / raw)
To: git; +Cc: gitster
Correct a few random "sendemail.*" configuration parameter names in the
documentation that, for some reason, didn't use camel case format.
Signed-off-by: Dragan Simic <dsimic@manjaro.org>
---
Notes:
The version 2 includes feedback from Junio, by changing "Ssl" to "SSL"
in a couple of places where "sendemail.smtpSSLCertPath" is mentioned.
There are already instances of "SSL" being used, for example in various
"http.proxySSL*" configuration parameter names.
Documentation/config/sendemail.txt | 12 ++++++------
Documentation/git-send-email.txt | 18 +++++++++---------
2 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/Documentation/config/sendemail.txt b/Documentation/config/sendemail.txt
index 7fc770ee9e69..6a869d67eb90 100644
--- a/Documentation/config/sendemail.txt
+++ b/Documentation/config/sendemail.txt
@@ -8,7 +8,7 @@ sendemail.smtpEncryption::
See linkgit:git-send-email[1] for description. Note that this
setting is not subject to the 'identity' mechanism.
-sendemail.smtpsslcertpath::
+sendemail.smtpSSLCertPath::
Path to ca-certificates (either a directory or a single file).
Set it to an empty string to disable certificate verification.
@@ -62,27 +62,27 @@ sendemail.chainReplyTo::
sendemail.envelopeSender::
sendemail.from::
sendemail.headerCmd::
-sendemail.signedoffbycc::
+sendemail.signedOffByCc::
sendemail.smtpPass::
-sendemail.suppresscc::
+sendemail.suppressCc::
sendemail.suppressFrom::
sendemail.to::
-sendemail.tocmd::
+sendemail.toCmd::
sendemail.smtpDomain::
sendemail.smtpServer::
sendemail.smtpServerPort::
sendemail.smtpServerOption::
sendemail.smtpUser::
sendemail.thread::
sendemail.transferEncoding::
sendemail.validate::
sendemail.xmailer::
These configuration variables all provide a default for
linkgit:git-send-email[1] command-line options. See its
documentation for details.
-sendemail.signedoffcc (deprecated)::
- Deprecated alias for `sendemail.signedoffbycc`.
+sendemail.signedOffCc (deprecated)::
+ Deprecated alias for `sendemail.signedOffByCc`.
sendemail.smtpBatchSize::
Number of messages to be sent per connection, after that a relogin
diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index d1ef6a204e68..8264f8738093 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -138,7 +138,7 @@ Note that no attempts whatsoever are made to validate the encoding.
--compose-encoding=<encoding>::
Specify encoding of compose message. Default is the value of the
- 'sendemail.composeencoding'; if that is unspecified, UTF-8 is assumed.
+ 'sendemail.composeEncoding'; if that is unspecified, UTF-8 is assumed.
--transfer-encoding=(7bit|8bit|quoted-printable|base64|auto)::
Specify the transfer encoding to be used to send the message over SMTP.
@@ -174,7 +174,7 @@ Sending
Specify a command to run to send the email. The command should
be sendmail-like; specifically, it must support the `-i` option.
The command will be executed in the shell if necessary. Default
- is the value of `sendemail.sendmailcmd`. If unspecified, and if
+ is the value of `sendemail.sendmailCmd`. If unspecified, and if
--smtp-server is also unspecified, git-send-email will search
for `sendmail` in `/usr/sbin`, `/usr/lib` and $PATH.
@@ -269,7 +269,7 @@ must be used for each option.
certificates concatenated together: see verify(1) -CAfile and
-CApath for more information on these). Set it to an empty string
to disable certificate verification. Defaults to the value of the
- `sendemail.smtpsslcertpath` configuration variable, if set, or the
+ `sendemail.smtpSSLCertPath` configuration variable, if set, or the
backing SSL library's compiled-in default otherwise (which should
be the best choice on most platforms).
@@ -313,7 +313,7 @@ Automating
Specify a command to execute once per patch file which
should generate patch file specific "To:" entries.
Output of this command must be single email address per line.
- Default is the value of 'sendemail.tocmd' configuration value.
+ Default is the value of 'sendemail.toCmd' configuration value.
--cc-cmd=<command>::
Specify a command to execute once per patch file which
@@ -348,19 +348,19 @@ Automating
--[no-]signed-off-by-cc::
If this is set, add emails found in the `Signed-off-by` trailer or Cc: lines to the
- cc list. Default is the value of `sendemail.signedoffbycc` configuration
+ cc list. Default is the value of `sendemail.signedOffByCc` configuration
value; if that is unspecified, default to --signed-off-by-cc.
--[no-]cc-cover::
If this is set, emails found in Cc: headers in the first patch of
the series (typically the cover letter) are added to the cc list
- for each email set. Default is the value of 'sendemail.cccover'
+ for each email set. Default is the value of 'sendemail.ccCover'
configuration value; if that is unspecified, default to --no-cc-cover.
--[no-]to-cover::
If this is set, emails found in To: headers in the first patch of
the series (typically the cover letter) are added to the to list
- for each email set. Default is the value of 'sendemail.tocover'
+ for each email set. Default is the value of 'sendemail.toCover'
configuration value; if that is unspecified, default to --no-to-cover.
--suppress-cc=<category>::
@@ -384,7 +384,7 @@ Automating
- 'all' will suppress all auto cc values.
--
+
-Default is the value of `sendemail.suppresscc` configuration value; if
+Default is the value of `sendemail.suppressCc` configuration value; if
that is unspecified, default to 'self' if --suppress-from is
specified, as well as 'body' if --no-signed-off-cc is specified.
@@ -471,7 +471,7 @@ Information
Instead of the normal operation, dump the shorthand alias names from
the configured alias file(s), one per line in alphabetical order. Note
that this only includes the alias name and not its expanded email addresses.
- See 'sendemail.aliasesfile' for more information about aliases.
+ See 'sendemail.aliasesFile' for more information about aliases.
CONFIGURATION
^ permalink raw reply related
* Re: [PATCH v2 5/5] notes: use strbuf_attach to take ownership of the object contents
From: Maarten Bosmans @ 2024-02-20 7:42 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20240220021220.GC2713741@coredump.intra.peff.net>
Op di 20 feb 2024 om 03:12 schreef Jeff King <peff@peff.net>:
>
> On Sun, Feb 18, 2024 at 08:59:38PM +0100, Maarten Bosmans wrote:
>
> > @@ -705,12 +703,11 @@ static int append_edit(int argc, const char **argv, const char *prefix)
> > if (!prev_buf)
> > die(_("unable to read %s"), oid_to_hex(note));
> > if (size)
> > - strbuf_add(&buf, prev_buf, size);
> > + strbuf_attach(&buf, prev_buf, size, size + 1);
> > if (d.buf.len && size)
> > append_separator(&buf);
> > strbuf_insert(&d.buf, 0, buf.buf, buf.len);
> >
> > - free(prev_buf);
> > strbuf_release(&buf);
> > }
>
> Is it possible for "size" to be 0, but prev_buf to be non-NULL? I assume
> it is so if the previous note is the empty object (and anyway, we'd have
> died earlier if prev_buf was NULL). In that case your patch introduces a
> leak (we do not attach prev_buf to buf, but we no longer free prev_buf).
You are right. I think the `if (size)` is not needed and removing it
would remove the potential for a leak.
> I'm a little skeptical that this is actually increasing the speed of the
> command in a measurable way, though. It's one allocation/copy, right
> next to a big old strbuf_insert() that is going to splice into an
> existing array.
Yeah, I was doubting this patch a bit too. The simple idiom of
starting with an empty strbuf and appending strings to it seems pretty
nice and clear, so may be there's value in leaving it at that.
The speed increase is not measurable of course. I was simply operating
in full on lets-eliminate-all-sources-of-overhead mode while profiling
the notes show code.
I'll drop the patch, in order to keep focus in this series.
Maarten
^ permalink raw reply
* Re: [PATCH 2/6] dir-iterator: support iteration in sorted order
From: Patrick Steinhardt @ 2024-02-20 8:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq8r3g10tf.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 9982 bytes --]
On Mon, Feb 19, 2024 at 03:39:08PM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > The `struct dir_iterator` is a helper that allows us to iterate through
> > directory entries. This iterator returns entries in the exact same order
> > as readdir(3P) does -- or in other words, it guarantees no specific
> > order at all.
> >
> > This is about to become problematic as we are introducing a new reflog
> > subcommand to list reflogs. As the "files" backend uses the directory
> > iterator to enumerate reflogs, returning reflog names and exposing them
> > to the user would inherit the indeterministic ordering. Naturally, it
> > would make for a terrible user interface to show a list with no
> > discernible order. While this could be handled at a higher level by the
> > new subcommand itself by collecting and ordering the reflogs, this would
> > be inefficient and introduce latency when there are many reflogs.
>
> I do not quite understand this argument. Why is sorting at higher
> level less (or more, for that matter) efficient than doing so at
> lower level? We'd need to sort somewhere no matter what, and I of
> course have no problem in listing in a deterministic order.
By sorting at a lower level we only need to sort the respective
directory entries and can then return them without having to recurse
into all subdirectories yet. Sorting at a higher level would require us
to first collect _all_ reflogs and then sort them.
Will rephrase a bit.
> > Instead, introduce a new option into the directory iterator that asks
> > for its entries to be yielded in lexicographical order. If set, the
> > iterator will read all directory entries greedily end sort them before
> > we start to iterate over them.
>
> "end" -> "and". And of course without such sorting option, this
> codepath is allowed to yield entries in any order that is the
> easiest to produce? That makes sense.
>
> > While this will of course also incur overhead as we cannot yield the
> > directory entries immediately, it should at least be more efficient than
> > having to sort the complete list of reflogs as we only need to sort one
> > directory at a time.
>
> True. The initial latency before we see the first byte of the
> output often matters more in perceived performance the throughput.
> As we need to sort to give a reasonable output, that cannot be
> avoided.
>
> > This functionality will be used in a follow-up commit.
> >
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> > dir-iterator.c | 87 ++++++++++++++++++++++++++++++++++++++++----------
> > dir-iterator.h | 3 ++
> > 2 files changed, 73 insertions(+), 17 deletions(-)
> >
> > diff --git a/dir-iterator.c b/dir-iterator.c
> > index f58a97e089..396c28178f 100644
> > --- a/dir-iterator.c
> > +++ b/dir-iterator.c
> > @@ -2,9 +2,12 @@
> > #include "dir.h"
> > #include "iterator.h"
> > #include "dir-iterator.h"
> > +#include "string-list.h"
> >
> > struct dir_iterator_level {
> > DIR *dir;
> > + struct string_list entries;
> > + size_t entries_idx;
>
> Does it deserve a comment that "dir == NULL" is used as a signal
> that we have read the level and sorted its contents into the
> "entries" list (and also we have already called closedir(), of
> course)?
Yeah, probably.
> > @@ -72,6 +75,40 @@ static int push_level(struct dir_iterator_int *iter)
> > return -1;
> > }
> >
> > + string_list_init_dup(&level->entries);
> > + level->entries_idx = 0;
> > +
> > + /*
> > + * When the iterator is sorted we read and sort all directory entries
> > + * directly.
> > + */
> > + if (iter->flags & DIR_ITERATOR_SORTED) {
> > + while (1) {
> > + struct dirent *de;
> > +
> > + errno = 0;
> > + de = readdir(level->dir);
> > + if (!de) {
> > + if (errno && errno != ENOENT) {
> > + warning_errno("error reading directory '%s'",
> > + iter->base.path.buf);
> > + return -1;
> > + }
> > +
> > + break;
> > + }
> > +
> > + if (is_dot_or_dotdot(de->d_name))
> > + continue;
>
> The condition to skip an entry currently is simple enough that "."
> and ".." are the only ones that are skipped, but it must be kept in
> sync with the condition in dir_iterator_advance().
>
> If it becomes more complex than it is now (e.g., we may start to
> skip any name that begins with a dot, like ".git" or ".dummy"), it
> probably is a good idea *not* to add the same filtering logic here
> and in dir_iterator_advance(). Instead, keep the filtering here to
> an absolute minumum, and filter the name, whether it came from
> readdir() or from the .entries string list, in a single copy of
> filtering logic in dir_iterator_advance() function.
>
> We could drop the dot-or-dotdot filter here, too, if we want to
> ensure that unified filtering will be correctly done over there.
Fair point. As you mention further down below, there are two ways to
approach it:
- Just filter at the later stage and accept that we'll allocate memory
for entries that are about to be discarded.
- Create a function `should_include_entry()` that gets called at both
code sites.
I don't think the allocation overhead should matter much, but neither
does it hurt to create a common `should_include_entry()` function. And
as both are trivial to implement I rather lean towards the more
efficient variant, even though the efficiency gain should be negligible.
> > + string_list_append(&level->entries, de->d_name);
> > + }
> > + string_list_sort(&level->entries);
> > +
> > + closedir(level->dir);
> > + level->dir = NULL;
> > + }
> > +
> > return 0;
> > }
> >
> > @@ -88,6 +125,7 @@ static int pop_level(struct dir_iterator_int *iter)
> > warning_errno("error closing directory '%s'",
> > iter->base.path.buf);
> > level->dir = NULL;
> > + string_list_clear(&level->entries, 0);
> >
> > return --iter->levels_nr;
> > }
>
> It is somewhat interesting that the original code already has
> conditional call to closedir() and prepares .dir to be NULL,
> so that we do not have to make it conditional here.
>
> > @@ -136,30 +174,43 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)
> >
> > /* Loop until we find an entry that we can give back to the caller. */
> > while (1) {
> > - struct dirent *de;
> > struct dir_iterator_level *level =
> > &iter->levels[iter->levels_nr - 1];
> > + struct dirent *de;
> > + const char *name;
>
> Not a huge deal but this is an unnecessary reordering, right?
Right.
> > strbuf_setlen(&iter->base.path, level->prefix_len);
> > +
> > + if (level->dir) {
> > + errno = 0;
> > + de = readdir(level->dir);
> > + if (!de) {
> > + if (errno) {
> > + warning_errno("error reading directory '%s'",
> > + iter->base.path.buf);
> > + if (iter->flags & DIR_ITERATOR_PEDANTIC)
> > + goto error_out;
> > + } else if (pop_level(iter) == 0) {
> > + return dir_iterator_abort(dir_iterator);
> > + }
> > + continue;
> > }
> >
> > + if (is_dot_or_dotdot(de->d_name))
> > + continue;
>
> This is the target of the "if we will end up filtering even more in
> the future, it would probably be a good idea not to duplicate the
> logic to decide what gets filtered in this function and in
> push_level()" comment. If we wanted to go that route, we can get
> rid of the filtering from push_level(), and move this filter code
> outside this if/else before calling prepare_next_entry_data().
>
> The fact that .entries.nr represents the number of entries that are
> shown is unusable (because there is an unsorted codepath that does
> not even populate .entries), so I am not worried about correctness
> gotchas caused by including names in .entries to be filtered out.
> But an obvious downside is that the size of the list to be sorted
> will become larger.
>
> Or we could introduce a shared helper function that takes a name and
> decides if it is to be included, and replace the is_dot_or_dotdot()
> call here and in the push_level() with calls to that helper.
>
> In any case, that is primarily a maintainability issue. The code
> posted as-is is correct.
Yeah, let's use the proposed helper function. In fact, I think we can
share even more code than merely the filtering part: the errno handling
is a bit special, and the warning is the same across both code sites,
too.
Patrick
> > + name = de->d_name;
> > + } else {
> > + if (level->entries_idx >= level->entries.nr) {
> > + if (pop_level(iter) == 0)
> > + return dir_iterator_abort(dir_iterator);
> > + continue;
> > + }
> > +
> > + name = level->entries.items[level->entries_idx++].string;
> > + }
> > +
> > + if (prepare_next_entry_data(iter, name)) {
> > if (errno != ENOENT && iter->flags & DIR_ITERATOR_PEDANTIC)
> > goto error_out;
> > continue;
> > @@ -188,6 +239,8 @@ int dir_iterator_abort(struct dir_iterator *dir_iterator)
> > warning_errno("error closing directory '%s'",
> > iter->base.path.buf);
> > }
> > +
> > + string_list_clear(&level->entries, 0);
> > }
> >
> > free(iter->levels);
> > diff --git a/dir-iterator.h b/dir-iterator.h
> > index 479e1ec784..6d438809b6 100644
> > --- a/dir-iterator.h
> > +++ b/dir-iterator.h
> > @@ -54,8 +54,11 @@
> > * and ITER_ERROR is returned immediately. In both cases, a meaningful
> > * warning is emitted. Note: ENOENT errors are always ignored so that
> > * the API users may remove files during iteration.
> > + *
> > + * - DIR_ITERATOR_SORTED: sort directory entries alphabetically.
> > */
> > #define DIR_ITERATOR_PEDANTIC (1 << 0)
> > +#define DIR_ITERATOR_SORTED (1 << 1)
> >
> > struct dir_iterator {
> > /* The current path: */
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 4/6] refs: drop unused params from the reflog iterator callback
From: Patrick Steinhardt @ 2024-02-20 8:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqplwsyotj.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 2472 bytes --]
On Mon, Feb 19, 2024 at 04:14:16PM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > The ref and reflog iterators share much of the same underlying code to
> > iterate over the corresponding entries. This results in some weird code
> > because the reflog iterator also exposes an object ID as well as a flag
> > to the callback function. Neither of these fields do refer to the reflog
> > though -- they refer to the corresponding ref with the same name. This
> > is quite misleading. In practice at least the object ID cannot really be
> > implemented in any other way as a reflog does not have a specific object
> > ID in the first place. This is further stressed by the fact that none of
> > the callbacks except for our test helper make use of these fields.
>
> Interesting observation. Of course this will make the callstack
> longer by another level of indirection ...
It actually doesn't -- the old code had a `do_for_each_ref_helper()`
that did the same wrapping, so the level of indirection is exactly the
same. Over there we have it to drop the unused `struct repository`
parameter.
Patrick
> > +struct do_for_each_reflog_help {
> > + each_reflog_fn *fn;
> > + void *cb_data;
> > +};
> > +
> > +static int do_for_each_reflog_helper(struct repository *r UNUSED,
> > + const char *refname,
> > + const struct object_id *oid UNUSED,
> > + int flags,
> > + void *cb_data)
> > +{
> > + struct do_for_each_reflog_help *hp = cb_data;
> > + return hp->fn(refname, hp->cb_data);
> > +}
>
> ... but I think it would be worth it.
>
> > +/*
> > + * The signature for the callback function for the {refs_,}for_each_reflog()
> > + * functions below. The memory pointed to by the refname argument is only
> > + * guaranteed to be valid for the duration of a single callback invocation.
> > + */
> > +typedef int each_reflog_fn(const char *refname, void *cb_data);
> > +
> > /*
> > * Calls the specified function for each reflog file until it returns nonzero,
> > * and returns the value. Reflog file order is unspecified.
> > */
> > -int refs_for_each_reflog(struct ref_store *refs, each_ref_fn fn, void *cb_data);
> > -int for_each_reflog(each_ref_fn fn, void *cb_data);
> > +int refs_for_each_reflog(struct ref_store *refs, each_reflog_fn fn, void *cb_data);
> > +int for_each_reflog(each_reflog_fn fn, void *cb_data);
>
> Nice simplification.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 5/6] refs: stop resolving ref corresponding to reflogs
From: Patrick Steinhardt @ 2024-02-20 8:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqjzn0yote.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 6112 bytes --]
On Mon, Feb 19, 2024 at 04:14:21PM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > Refactor the code to call `check_refname_format()` directly instead of
> > trying to resolve the ref. This is significantly more efficient given
> > that we don't have to hit the object database anymore to list reflogs.
> > And second, it ensures that we end up showing reflogs of broken refs,
> > which will help to make the reflog more useful.
>
> And the user would notice corrupt ones among those reflogs listed
> when using "rev-list -g" on the reflog anyway? Which sounds like a
> sensible thing to do.
Yeah. Overall the user experience is still quite lacking when you have
such "funny" reflogs. Corrupted ones would result in errors as you
mentioned, and that's to be expected in my opinion.
The more dubious behaviour is that `git reflog show $REFLOG` refuses to
show the reflog when the corresponding ref is missing. This is something
I plan to address in a follow-up patch series.
> > Note that this really only impacts the case where the corresponding ref
> > is corrupt. Reflogs for nonexistent refs would have been returned to the
> > caller beforehand already as we did not pass `RESOLVE_REF_READING` to
> > the function, and thus `refs_resolve_ref_unsafe()` would have returned
> > successfully in that case.
>
> What do "Reflogs for nonexistent refs" really mean? With the files
> backend, if "git branch -d main" that removed the "main" branch
> somehow forgot to remove the ".git/logs/refs/heads/main" file, the
> reflog entries in such a file is for nonexistent ref. Is that what
> you meant?
Yes. Would "Reflogs which do not have a corresponding ref with the same
name" be clearer?
> As a tool to help diagnosing and correcting minor repo
> breakages, finding such a leftover file that should not exist is a
> good idea, I would think.
>
> Would we see missing reflog for a ref that exists in the iteration?
> I guess we shouldn't, as the reflog iterator that recursively
> enumerates files under "$GIT_DIR/logs/" would not see such a missing
> reflog by definition.
No, and I'd claim we shouldn't. The reflog mechanism gives the user
control over which reflogs should and which shouldn't exist. For one,
`core.logAllRefUpdates` allows the user to either enable or disable the
reflog mechanism. If set to "false" then no reflogs are created, with
"true" some are created, and with "always" we always end up creating
reflogs. So depending on this setting it's expected that a subset of
reflogs do not exist.
But that'also not the whole story yet. Theoretically speaking, reflogs
have a subtle opt-in mechanism: once a reflog is created, we will
continue writing to it no matter what `core.logAllRefUpdates` says. So
it's feasible to have `core.logAllRefUpdates=false`, but then explicitly
create a specific reflog so that you log changes to a specific ref.
With this behaviour in mind I'd say that we shouldn't log missing
reflogs.
> > diff --git a/refs/files-backend.c b/refs/files-backend.c
> > index 2b3c99b00d..741148087d 100644
> > --- a/refs/files-backend.c
> > +++ b/refs/files-backend.c
> > @@ -2130,17 +2130,9 @@ static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator)
> > while ((ok = dir_iterator_advance(diter)) == ITER_OK) {
> > if (!S_ISREG(diter->st.st_mode))
> > continue;
> > - if (diter->basename[0] == '.')
> > + if (check_refname_format(diter->basename,
> > + REFNAME_ALLOW_ONELEVEL))
> > continue;
>
> A tangent.
>
> I've never liked the code arrangement in the check_refname_format()
> that assumes that each level can be separately checked with exactly
> the same logic, and the only thing ALLOW_ONELEVEL does is to include
> pseudorefs and HEAD; this makes such assumption even more ingrained.
> I am not sure what to think about it, but let's keep reading.
Yeah. This code here is basically just copied over from
`refs_resolve_ref_unsafe()` to ensure that it remains compatible. In a
future patch series we might include a new option `--include-broken`
that would also surface broken-but-safe reflog names.
But going down the tangent even more: one think I've noticed is that the
way `check_refname_format()` is structured is also wildly inefficient.
It's quite astonishing that when iterating over refs, we spend _more_
time in `check_refname_format()` than reading the refs from disk,
parsing them and massaging them into their final representation.
Overall, the whole infra to check refnames could use some improvement.
But this has already been discussed in other threads recently.
Patrick
> > - if (ends_with(diter->basename, ".lock"))
> > - continue;
>
> This can safely go, as it is rejected by check_refname_format().
>
> > - if (!refs_resolve_ref_unsafe(iter->ref_store,
> > - diter->relative_path, 0,
> > - NULL, NULL)) {
> > - error("bad ref for %s", diter->path.buf);
> > - continue;
> > - }
>
> This is the focus of this step in the series. We did not abort the
> iteration before, but now we no longer issue any error message.
>
> > iter->base.refname = diter->relative_path;
> > return ITER_OK;
> > diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
> > index 889bb1f1ba..efbbf23c72 100644
> > --- a/refs/reftable-backend.c
> > +++ b/refs/reftable-backend.c
> > @@ -1659,11 +1659,9 @@ static int reftable_reflog_iterator_advance(struct ref_iterator *ref_iterator)
> > if (iter->last_name && !strcmp(iter->log.refname, iter->last_name))
> > continue;
> >
> > - if (!refs_resolve_ref_unsafe(&iter->refs->base, iter->log.refname,
> > - 0, NULL, NULL)) {
> > - error(_("bad ref for %s"), iter->log.refname);
> > + if (check_refname_format(iter->log.refname,
> > + REFNAME_ALLOW_ONELEVEL))
> > continue;
> > - }
>
> This side is much more straight-forward. Looking good.
>
> >
> > free(iter->last_name);
> > iter->last_name = xstrdup(iter->log.refname);
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 6/6] builtin/reflog: introduce subcommand to list reflogs
From: Patrick Steinhardt @ 2024-02-20 8:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq7cj0ynys.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 3649 bytes --]
On Mon, Feb 19, 2024 at 04:32:43PM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh
> > index d2f5f42e67..6d8d5a253d 100755
> > --- a/t/t1410-reflog.sh
> > +++ b/t/t1410-reflog.sh
> > @@ -436,4 +436,73 @@ test_expect_success 'empty reflog' '
> > test_must_be_empty err
> > '
> >
> > +test_expect_success 'list reflogs' '
> > + test_when_finished "rm -rf repo" &&
> > + git init repo &&
> > + (
> > + cd repo &&
> > + git reflog list >actual &&
> > + test_must_be_empty actual &&
> > +
> > + test_commit A &&
> > + cat >expect <<-EOF &&
> > + HEAD
> > + refs/heads/main
> > + EOF
> > + git reflog list >actual &&
> > + test_cmp expect actual &&
> > +
> > + git branch b &&
> > + cat >expect <<-EOF &&
> > + HEAD
> > + refs/heads/b
> > + refs/heads/main
> > + EOF
> > + git reflog list >actual &&
> > + test_cmp expect actual
> > + )
> > +'
>
> OK. This is a quite boring baseline.
>
> > +test_expect_success 'reflog list returns error with additional args' '
> > + cat >expect <<-EOF &&
> > + error: list does not accept arguments: ${SQ}bogus${SQ}
> > + EOF
> > + test_must_fail git reflog list bogus 2>err &&
> > + test_cmp expect err
> > +'
>
> Makes sense.
>
> > +test_expect_success 'reflog for symref with unborn target can be listed' '
> > + test_when_finished "rm -rf repo" &&
> > + git init repo &&
> > + (
> > + cd repo &&
> > + test_commit A &&
> > + git symbolic-ref HEAD refs/heads/unborn &&
> > + cat >expect <<-EOF &&
> > + HEAD
> > + refs/heads/main
> > + EOF
> > + git reflog list >actual &&
> > + test_cmp expect actual
> > + )
> > +'
>
> Should this be under REFFILES? Ah, no, "git symbolic-ref" is valid
> under reftable as well, so there is no need to.
>
> Without [5/6], would it have failed to show the reflog for HEAD?
I initially thought so, but no. `refs_resolve_ref_unsafe()` is weird as
it returns successfully even if a symref cannot be resolved unless you
pass `RESOLVE_REF_READING`, which we didn't.
The case where it does make a difference is if we had a corrupt ref. So
if you "echo garbage >.git/refs/heads/branch", then the corresponding
reflog would not have been listed. Even worse, even after this patch
series it's still impossible to `git reflog show` the reflog because we
fail to resolve the ref itself, which basically breaks the whole point
of the reflog.
This is something that I plan to address in a follow-up patch series.
> > +test_expect_success 'reflog with invalid object ID can be listed' '
> > + test_when_finished "rm -rf repo" &&
> > + git init repo &&
> > + (
> > + cd repo &&
> > + test_commit A &&
> > + test-tool ref-store main update-ref msg refs/heads/missing \
> > + $(test_oid deadbeef) "$ZERO_OID" REF_SKIP_OID_VERIFICATION &&
> > + cat >expect <<-EOF &&
> > + HEAD
> > + refs/heads/main
> > + refs/heads/missing
> > + EOF
> > + git reflog list >actual &&
> > + test_cmp expect actual
> > + )
> > +'
>
> OK.
>
> > test_done
>
> It would have been "interesting" to see an example of "there is a
> reflog but the underlying ref for it is missing" case, but I think
> that falls into a minor repository corruption category, so lack of
> such a test is also fine.
The reason why I didn't include such a test is that it's by necessity
specific to the backend: we don't have any way to delete a ref without
also deleting the corresponding reflog. So we'd have to manually delete
it, which only works with the REFFILES backend.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 3/6] refs/files: sort reflogs returned by the reflog iterator
From: Patrick Steinhardt @ 2024-02-20 8:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq34to0znj.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 2447 bytes --]
On Mon, Feb 19, 2024 at 04:04:16PM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > We use a directory iterator to return reflogs via the reflog iterator.
> > This iterator returns entries in the same order as readdir(3P) would and
> > will thus yield reflogs with no discernible order.
> >
> > Set the new `DIR_ITERATOR_SORTED` flag that was introduced in the
> > preceding commit so that the order is deterministic. While the effect of
> > this can only been observed in a test tool, a subsequent commit will
> > start to expose this functionality to users via a new `git reflog list`
> > subcommand.
> >
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> > refs/files-backend.c | 4 ++--
> > t/t0600-reffiles-backend.sh | 4 ++--
> > t/t1405-main-ref-store.sh | 2 +-
> > t/t1406-submodule-ref-store.sh | 2 +-
> > 4 files changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/refs/files-backend.c b/refs/files-backend.c
> > index 75dcc21ecb..2ffc63185f 100644
> > --- a/refs/files-backend.c
> > +++ b/refs/files-backend.c
> > @@ -2193,7 +2193,7 @@ static struct ref_iterator *reflog_iterator_begin(struct ref_store *ref_store,
> >
> > strbuf_addf(&sb, "%s/logs", gitdir);
> >
> > - diter = dir_iterator_begin(sb.buf, 0);
> > + diter = dir_iterator_begin(sb.buf, DIR_ITERATOR_SORTED);
> > if (!diter) {
> > strbuf_release(&sb);
> > return empty_ref_iterator_begin();
> > @@ -2202,7 +2202,7 @@ static struct ref_iterator *reflog_iterator_begin(struct ref_store *ref_store,
> > CALLOC_ARRAY(iter, 1);
> > ref_iterator = &iter->base;
> >
> > - base_ref_iterator_init(ref_iterator, &files_reflog_iterator_vtable, 0);
> > + base_ref_iterator_init(ref_iterator, &files_reflog_iterator_vtable, 1);
>
> This caught my attention. Once we apply this patch, the only way
> base_ref_iterator_init() can receive 0 for its last parameter
> (i.e. 'ordered') is via the merge_ref_iterator_begin() call in
> files_reflog_iterator_begin() that passes 0 as 'ordered'. If we
> force files_reflog_iterator_begin() to ask for an ordered
> merge_ref_iterator, then we will have no unordered ref iterators.
> Am I reading the code right?
Ah, true indeed. The "files" reflog iterator was the only remaining
iterator that wasn't ordered. I'll include an additional patch on top
that drops the `ordered` bit altogether.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH v2 0/7] reflog: introduce subcommand to list reflogs
From: Patrick Steinhardt @ 2024-02-20 9:06 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <cover.1708353264.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 10804 bytes --]
Hi,
this is the second version of my patch series that introduces a new `git
reflog list` subcommand to list available reflogs in a repository.
Changes compared to v1:
- Patch 2: Clarified the commit message to hopefully explain better
why a higher level implementation of reflog sorting would have
increased latency.
- Patch 2: Introduced a helper function that unifies the logic to
yield the next directory entry.
- Patch 3: Mark the merged reflog iterator as sorted, which I missed
in my previous round.
- Patch 4: This patch is new and simplifies the code to require all
ref iterators to be sorted.
Junio, I noticed that you already merged v1 of this patch series to
`next`. I was a bit surprised to see it merged down this fast, so I
assume that this is only done due to the pending Git v2.44 release and
that you plan to reroll `next` anyway. I thus didn't send follow-up
patches but resent the whole patch series as v2. If I misinterpreted
your intent I'm happy to send the changes as follow-up patches instead.
The patch series continues to depend on ps/reftable-backend at
8a0bebdeae (refs/reftable: fix leak when copying reflog fails,
2024-02-08).
Thanks!
Patrick
Patrick Steinhardt (7):
dir-iterator: pass name to `prepare_next_entry_data()` directly
dir-iterator: support iteration in sorted order
refs/files: sort reflogs returned by the reflog iterator
refs: always treat iterators as ordered
refs: drop unused params from the reflog iterator callback
refs: stop resolving ref corresponding to reflogs
builtin/reflog: introduce subcommand to list reflogs
Documentation/git-reflog.txt | 3 +
builtin/fsck.c | 4 +-
builtin/reflog.c | 37 +++++++++++-
dir-iterator.c | 105 ++++++++++++++++++++++++++++-----
dir-iterator.h | 3 +
refs.c | 27 ++++++---
refs.h | 11 +++-
refs/debug.c | 3 +-
refs/files-backend.c | 27 ++-------
refs/iterator.c | 26 +++-----
refs/packed-backend.c | 2 +-
refs/ref-cache.c | 2 +-
refs/refs-internal.h | 18 +-----
refs/reftable-backend.c | 20 ++-----
revision.c | 4 +-
t/helper/test-ref-store.c | 18 ++++--
t/t0600-reffiles-backend.sh | 24 ++++----
t/t1405-main-ref-store.sh | 8 +--
t/t1406-submodule-ref-store.sh | 8 +--
t/t1410-reflog.sh | 69 ++++++++++++++++++++++
20 files changed, 286 insertions(+), 133 deletions(-)
Range-diff against v1:
1: 12de25dfe2 = 1: 12de25dfe2 dir-iterator: pass name to `prepare_next_entry_data()` directly
2: 8a588175db ! 2: 788afce189 dir-iterator: support iteration in sorted order
@@ Commit message
iterator to enumerate reflogs, returning reflog names and exposing them
to the user would inherit the indeterministic ordering. Naturally, it
would make for a terrible user interface to show a list with no
- discernible order. While this could be handled at a higher level by the
- new subcommand itself by collecting and ordering the reflogs, this would
- be inefficient and introduce latency when there are many reflogs.
+ discernible order.
+
+ While this could be handled at a higher level by the new subcommand
+ itself by collecting and ordering the reflogs, this would be inefficient
+ because we would first have to collect all reflogs before we can sort
+ them, which would introduce additional latency when there are many
+ reflogs.
Instead, introduce a new option into the directory iterator that asks
for its entries to be yielded in lexicographical order. If set, the
- iterator will read all directory entries greedily end sort them before
+ iterator will read all directory entries greedily and sort them before
we start to iterate over them.
While this will of course also incur overhead as we cannot yield the
@@ dir-iterator.c
struct dir_iterator_level {
DIR *dir;
+
++ /*
++ * The directory entries of the current level. This list will only be
++ * populated when the iterator is ordered. In that case, `dir` will be
++ * set to `NULL`.
++ */
+ struct string_list entries;
+ size_t entries_idx;
-
++
/*
* The length of the directory part of path at this level
+ * (including a trailing '/'):
+@@ dir-iterator.c: struct dir_iterator_int {
+ unsigned int flags;
+ };
+
++static int next_directory_entry(DIR *dir, const char *path,
++ struct dirent **out)
++{
++ struct dirent *de;
++
++repeat:
++ errno = 0;
++ de = readdir(dir);
++ if (!de) {
++ if (errno) {
++ warning_errno("error reading directory '%s'",
++ path);
++ return -1;
++ }
++
++ return 1;
++ }
++
++ if (is_dot_or_dotdot(de->d_name))
++ goto repeat;
++
++ *out = de;
++ return 0;
++}
++
+ /*
+ * Push a level in the iter stack and initialize it with information from
+ * the directory pointed by iter->base->path. It is assumed that this
@@ dir-iterator.c: static int push_level(struct dir_iterator_int *iter)
return -1;
}
@@ dir-iterator.c: static int push_level(struct dir_iterator_int *iter)
+ * directly.
+ */
+ if (iter->flags & DIR_ITERATOR_SORTED) {
-+ while (1) {
-+ struct dirent *de;
++ struct dirent *de;
+
-+ errno = 0;
-+ de = readdir(level->dir);
-+ if (!de) {
-+ if (errno && errno != ENOENT) {
-+ warning_errno("error reading directory '%s'",
-+ iter->base.path.buf);
++ while (1) {
++ int ret = next_directory_entry(level->dir, iter->base.path.buf, &de);
++ if (ret < 0) {
++ if (errno != ENOENT &&
++ iter->flags & DIR_ITERATOR_PEDANTIC)
+ return -1;
-+ }
-+
++ continue;
++ } else if (ret > 0) {
+ break;
+ }
+
-+ if (is_dot_or_dotdot(de->d_name))
-+ continue;
-+
+ string_list_append(&level->entries, de->d_name);
+ }
+ string_list_sort(&level->entries);
@@ dir-iterator.c: static int pop_level(struct dir_iterator_int *iter)
return --iter->levels_nr;
}
@@ dir-iterator.c: int dir_iterator_advance(struct dir_iterator *dir_iterator)
-
- /* Loop until we find an entry that we can give back to the caller. */
- while (1) {
-- struct dirent *de;
+ struct dirent *de;
struct dir_iterator_level *level =
&iter->levels[iter->levels_nr - 1];
-+ struct dirent *de;
+ const char *name;
strbuf_setlen(&iter->base.path, level->prefix_len);
- errno = 0;
- de = readdir(level->dir);
--
+
- if (!de) {
- if (errno) {
- warning_errno("error reading directory '%s'",
- iter->base.path.buf);
-- if (iter->flags & DIR_ITERATOR_PEDANTIC)
-- goto error_out;
++ if (level->dir) {
++ int ret = next_directory_entry(level->dir, iter->base.path.buf, &de);
++ if (ret < 0) {
+ if (iter->flags & DIR_ITERATOR_PEDANTIC)
+ goto error_out;
- } else if (pop_level(iter) == 0) {
- return dir_iterator_abort(dir_iterator);
-+
-+ if (level->dir) {
-+ errno = 0;
-+ de = readdir(level->dir);
-+ if (!de) {
-+ if (errno) {
-+ warning_errno("error reading directory '%s'",
-+ iter->base.path.buf);
-+ if (iter->flags & DIR_ITERATOR_PEDANTIC)
-+ goto error_out;
-+ } else if (pop_level(iter) == 0) {
++ continue;
++ } else if (ret > 0) {
++ if (pop_level(iter) == 0)
+ return dir_iterator_abort(dir_iterator);
-+ }
+ continue;
}
- continue;
@@ dir-iterator.c: int dir_iterator_advance(struct dir_iterator *dir_iterator)
- if (is_dot_or_dotdot(de->d_name))
- continue;
-+ if (is_dot_or_dotdot(de->d_name))
-+ continue;
-
-- if (prepare_next_entry_data(iter, de->d_name)) {
+ name = de->d_name;
+ } else {
+ if (level->entries_idx >= level->entries.nr) {
@@ dir-iterator.c: int dir_iterator_advance(struct dir_iterator *dir_iterator)
+ return dir_iterator_abort(dir_iterator);
+ continue;
+ }
-+
+
+- if (prepare_next_entry_data(iter, de->d_name)) {
+ name = level->entries.items[level->entries_idx++].string;
+ }
+
3: e4e4fac05c ! 3: 32b24a3d4b refs/files: sort reflogs returned by the reflog iterator
@@ refs/files-backend.c: static struct ref_iterator *reflog_iterator_begin(struct r
iter->dir_iterator = diter;
iter->ref_store = ref_store;
strbuf_release(&sb);
+@@ refs/files-backend.c: static struct ref_iterator *files_reflog_iterator_begin(struct ref_store *ref_st
+ return reflog_iterator_begin(ref_store, refs->gitcommondir);
+ } else {
+ return merge_ref_iterator_begin(
+- 0, reflog_iterator_begin(ref_store, refs->base.gitdir),
++ 1, reflog_iterator_begin(ref_store, refs->base.gitdir),
+ reflog_iterator_begin(ref_store, refs->gitcommondir),
+ reflog_iterator_select, refs);
+ }
## t/t0600-reffiles-backend.sh ##
@@ t/t0600-reffiles-backend.sh: test_expect_success 'for_each_reflog()' '
-: ---------- > 4: 4254f23fd4 refs: always treat iterators as ordered
4: be512ef268 ! 5: 240334df6c refs: drop unused params from the reflog iterator callback
@@ refs/reftable-backend.c: static int reftable_reflog_iterator_advance(struct ref_
}
@@ refs/reftable-backend.c: static struct reftable_reflog_iterator *reflog_iterator_for_stack(struct reftabl
iter = xcalloc(1, sizeof(*iter));
- base_ref_iterator_init(&iter->base, &reftable_reflog_iterator_vtable, 1);
+ base_ref_iterator_init(&iter->base, &reftable_reflog_iterator_vtable);
iter->refs = refs;
- iter->base.oid = &iter->oid;
5: a7459b9483 = 6: 7928661318 refs: stop resolving ref corresponding to reflogs
6: cddb2de939 = 7: d7b9cff4c3 builtin/reflog: introduce subcommand to list reflogs
--
2.44.0-rc1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH v2 1/7] dir-iterator: pass name to `prepare_next_entry_data()` directly
From: Patrick Steinhardt @ 2024-02-20 9:06 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <cover.1708418805.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1694 bytes --]
When adding the next directory entry for `struct dir_iterator` we pass
the complete `struct dirent *` to `prepare_next_entry_data()` even
though we only need the entry's name.
Refactor the code to pass in the name, only. This prepares for a
subsequent commit where we introduce the ability to iterate through
dir entries in an ordered manner.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
dir-iterator.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/dir-iterator.c b/dir-iterator.c
index 278b04243a..f58a97e089 100644
--- a/dir-iterator.c
+++ b/dir-iterator.c
@@ -94,15 +94,15 @@ static int pop_level(struct dir_iterator_int *iter)
/*
* Populate iter->base with the necessary information on the next iteration
- * entry, represented by the given dirent de. Return 0 on success and -1
+ * entry, represented by the given name. Return 0 on success and -1
* otherwise, setting errno accordingly.
*/
static int prepare_next_entry_data(struct dir_iterator_int *iter,
- struct dirent *de)
+ const char *name)
{
int err, saved_errno;
- strbuf_addstr(&iter->base.path, de->d_name);
+ strbuf_addstr(&iter->base.path, name);
/*
* We have to reset these because the path strbuf might have
* been realloc()ed at the previous strbuf_addstr().
@@ -159,7 +159,7 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)
if (is_dot_or_dotdot(de->d_name))
continue;
- if (prepare_next_entry_data(iter, de)) {
+ if (prepare_next_entry_data(iter, de->d_name)) {
if (errno != ENOENT && iter->flags & DIR_ITERATOR_PEDANTIC)
goto error_out;
continue;
--
2.44.0-rc1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v2 2/7] dir-iterator: support iteration in sorted order
From: Patrick Steinhardt @ 2024-02-20 9:06 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <cover.1708418805.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 6195 bytes --]
The `struct dir_iterator` is a helper that allows us to iterate through
directory entries. This iterator returns entries in the exact same order
as readdir(3P) does -- or in other words, it guarantees no specific
order at all.
This is about to become problematic as we are introducing a new reflog
subcommand to list reflogs. As the "files" backend uses the directory
iterator to enumerate reflogs, returning reflog names and exposing them
to the user would inherit the indeterministic ordering. Naturally, it
would make for a terrible user interface to show a list with no
discernible order.
While this could be handled at a higher level by the new subcommand
itself by collecting and ordering the reflogs, this would be inefficient
because we would first have to collect all reflogs before we can sort
them, which would introduce additional latency when there are many
reflogs.
Instead, introduce a new option into the directory iterator that asks
for its entries to be yielded in lexicographical order. If set, the
iterator will read all directory entries greedily and sort them before
we start to iterate over them.
While this will of course also incur overhead as we cannot yield the
directory entries immediately, it should at least be more efficient than
having to sort the complete list of reflogs as we only need to sort one
directory at a time.
This functionality will be used in a follow-up commit.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
dir-iterator.c | 99 +++++++++++++++++++++++++++++++++++++++++++-------
dir-iterator.h | 3 ++
2 files changed, 89 insertions(+), 13 deletions(-)
diff --git a/dir-iterator.c b/dir-iterator.c
index f58a97e089..de619846f2 100644
--- a/dir-iterator.c
+++ b/dir-iterator.c
@@ -2,10 +2,19 @@
#include "dir.h"
#include "iterator.h"
#include "dir-iterator.h"
+#include "string-list.h"
struct dir_iterator_level {
DIR *dir;
+ /*
+ * The directory entries of the current level. This list will only be
+ * populated when the iterator is ordered. In that case, `dir` will be
+ * set to `NULL`.
+ */
+ struct string_list entries;
+ size_t entries_idx;
+
/*
* The length of the directory part of path at this level
* (including a trailing '/'):
@@ -43,6 +52,31 @@ struct dir_iterator_int {
unsigned int flags;
};
+static int next_directory_entry(DIR *dir, const char *path,
+ struct dirent **out)
+{
+ struct dirent *de;
+
+repeat:
+ errno = 0;
+ de = readdir(dir);
+ if (!de) {
+ if (errno) {
+ warning_errno("error reading directory '%s'",
+ path);
+ return -1;
+ }
+
+ return 1;
+ }
+
+ if (is_dot_or_dotdot(de->d_name))
+ goto repeat;
+
+ *out = de;
+ return 0;
+}
+
/*
* Push a level in the iter stack and initialize it with information from
* the directory pointed by iter->base->path. It is assumed that this
@@ -72,6 +106,35 @@ static int push_level(struct dir_iterator_int *iter)
return -1;
}
+ string_list_init_dup(&level->entries);
+ level->entries_idx = 0;
+
+ /*
+ * When the iterator is sorted we read and sort all directory entries
+ * directly.
+ */
+ if (iter->flags & DIR_ITERATOR_SORTED) {
+ struct dirent *de;
+
+ while (1) {
+ int ret = next_directory_entry(level->dir, iter->base.path.buf, &de);
+ if (ret < 0) {
+ if (errno != ENOENT &&
+ iter->flags & DIR_ITERATOR_PEDANTIC)
+ return -1;
+ continue;
+ } else if (ret > 0) {
+ break;
+ }
+
+ string_list_append(&level->entries, de->d_name);
+ }
+ string_list_sort(&level->entries);
+
+ closedir(level->dir);
+ level->dir = NULL;
+ }
+
return 0;
}
@@ -88,6 +151,7 @@ static int pop_level(struct dir_iterator_int *iter)
warning_errno("error closing directory '%s'",
iter->base.path.buf);
level->dir = NULL;
+ string_list_clear(&level->entries, 0);
return --iter->levels_nr;
}
@@ -139,27 +203,34 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)
struct dirent *de;
struct dir_iterator_level *level =
&iter->levels[iter->levels_nr - 1];
+ const char *name;
strbuf_setlen(&iter->base.path, level->prefix_len);
- errno = 0;
- de = readdir(level->dir);
- if (!de) {
- if (errno) {
- warning_errno("error reading directory '%s'",
- iter->base.path.buf);
+ if (level->dir) {
+ int ret = next_directory_entry(level->dir, iter->base.path.buf, &de);
+ if (ret < 0) {
if (iter->flags & DIR_ITERATOR_PEDANTIC)
goto error_out;
- } else if (pop_level(iter) == 0) {
- return dir_iterator_abort(dir_iterator);
+ continue;
+ } else if (ret > 0) {
+ if (pop_level(iter) == 0)
+ return dir_iterator_abort(dir_iterator);
+ continue;
}
- continue;
- }
- if (is_dot_or_dotdot(de->d_name))
- continue;
+ name = de->d_name;
+ } else {
+ if (level->entries_idx >= level->entries.nr) {
+ if (pop_level(iter) == 0)
+ return dir_iterator_abort(dir_iterator);
+ continue;
+ }
- if (prepare_next_entry_data(iter, de->d_name)) {
+ name = level->entries.items[level->entries_idx++].string;
+ }
+
+ if (prepare_next_entry_data(iter, name)) {
if (errno != ENOENT && iter->flags & DIR_ITERATOR_PEDANTIC)
goto error_out;
continue;
@@ -188,6 +259,8 @@ int dir_iterator_abort(struct dir_iterator *dir_iterator)
warning_errno("error closing directory '%s'",
iter->base.path.buf);
}
+
+ string_list_clear(&level->entries, 0);
}
free(iter->levels);
diff --git a/dir-iterator.h b/dir-iterator.h
index 479e1ec784..6d438809b6 100644
--- a/dir-iterator.h
+++ b/dir-iterator.h
@@ -54,8 +54,11 @@
* and ITER_ERROR is returned immediately. In both cases, a meaningful
* warning is emitted. Note: ENOENT errors are always ignored so that
* the API users may remove files during iteration.
+ *
+ * - DIR_ITERATOR_SORTED: sort directory entries alphabetically.
*/
#define DIR_ITERATOR_PEDANTIC (1 << 0)
+#define DIR_ITERATOR_SORTED (1 << 1)
struct dir_iterator {
/* The current path: */
--
2.44.0-rc1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v2 3/7] refs/files: sort reflogs returned by the reflog iterator
From: Patrick Steinhardt @ 2024-02-20 9:06 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <cover.1708418805.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 3875 bytes --]
We use a directory iterator to return reflogs via the reflog iterator.
This iterator returns entries in the same order as readdir(3P) would and
will thus yield reflogs with no discernible order.
Set the new `DIR_ITERATOR_SORTED` flag that was introduced in the
preceding commit so that the order is deterministic. While the effect of
this can only been observed in a test tool, a subsequent commit will
start to expose this functionality to users via a new `git reflog list`
subcommand.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
refs/files-backend.c | 6 +++---
t/t0600-reffiles-backend.sh | 4 ++--
t/t1405-main-ref-store.sh | 2 +-
t/t1406-submodule-ref-store.sh | 2 +-
4 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 75dcc21ecb..a7b7cdef36 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2193,7 +2193,7 @@ static struct ref_iterator *reflog_iterator_begin(struct ref_store *ref_store,
strbuf_addf(&sb, "%s/logs", gitdir);
- diter = dir_iterator_begin(sb.buf, 0);
+ diter = dir_iterator_begin(sb.buf, DIR_ITERATOR_SORTED);
if (!diter) {
strbuf_release(&sb);
return empty_ref_iterator_begin();
@@ -2202,7 +2202,7 @@ static struct ref_iterator *reflog_iterator_begin(struct ref_store *ref_store,
CALLOC_ARRAY(iter, 1);
ref_iterator = &iter->base;
- base_ref_iterator_init(ref_iterator, &files_reflog_iterator_vtable, 0);
+ base_ref_iterator_init(ref_iterator, &files_reflog_iterator_vtable, 1);
iter->dir_iterator = diter;
iter->ref_store = ref_store;
strbuf_release(&sb);
@@ -2246,7 +2246,7 @@ static struct ref_iterator *files_reflog_iterator_begin(struct ref_store *ref_st
return reflog_iterator_begin(ref_store, refs->gitcommondir);
} else {
return merge_ref_iterator_begin(
- 0, reflog_iterator_begin(ref_store, refs->base.gitdir),
+ 1, reflog_iterator_begin(ref_store, refs->base.gitdir),
reflog_iterator_begin(ref_store, refs->gitcommondir),
reflog_iterator_select, refs);
}
diff --git a/t/t0600-reffiles-backend.sh b/t/t0600-reffiles-backend.sh
index e6a5f1868f..4f860285cc 100755
--- a/t/t0600-reffiles-backend.sh
+++ b/t/t0600-reffiles-backend.sh
@@ -287,7 +287,7 @@ test_expect_success 'for_each_reflog()' '
mkdir -p .git/worktrees/wt/logs/refs/bisect &&
echo $ZERO_OID > .git/worktrees/wt/logs/refs/bisect/wt-random &&
- $RWT for-each-reflog | cut -d" " -f 2- | sort >actual &&
+ $RWT for-each-reflog | cut -d" " -f 2- >actual &&
cat >expected <<-\EOF &&
HEAD 0x1
PSEUDO-WT 0x0
@@ -297,7 +297,7 @@ test_expect_success 'for_each_reflog()' '
EOF
test_cmp expected actual &&
- $RMAIN for-each-reflog | cut -d" " -f 2- | sort >actual &&
+ $RMAIN for-each-reflog | cut -d" " -f 2- >actual &&
cat >expected <<-\EOF &&
HEAD 0x1
PSEUDO-MAIN 0x0
diff --git a/t/t1405-main-ref-store.sh b/t/t1405-main-ref-store.sh
index 976bd71efb..cfb583f544 100755
--- a/t/t1405-main-ref-store.sh
+++ b/t/t1405-main-ref-store.sh
@@ -74,7 +74,7 @@ test_expect_success 'verify_ref(new-main)' '
'
test_expect_success 'for_each_reflog()' '
- $RUN for-each-reflog | sort -k2 | cut -d" " -f 2- >actual &&
+ $RUN for-each-reflog | cut -d" " -f 2- >actual &&
cat >expected <<-\EOF &&
HEAD 0x1
refs/heads/main 0x0
diff --git a/t/t1406-submodule-ref-store.sh b/t/t1406-submodule-ref-store.sh
index e6a7f7334b..40332e23cc 100755
--- a/t/t1406-submodule-ref-store.sh
+++ b/t/t1406-submodule-ref-store.sh
@@ -63,7 +63,7 @@ test_expect_success 'verify_ref(new-main)' '
'
test_expect_success 'for_each_reflog()' '
- $RUN for-each-reflog | sort | cut -d" " -f 2- >actual &&
+ $RUN for-each-reflog | cut -d" " -f 2- >actual &&
cat >expected <<-\EOF &&
HEAD 0x1
refs/heads/main 0x0
--
2.44.0-rc1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v2 4/7] refs: always treat iterators as ordered
From: Patrick Steinhardt @ 2024-02-20 9:06 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <cover.1708418805.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 11337 bytes --]
In the preceding commit we have converted the reflog iterator of the
"files" backend to be ordered, which was the only remaining ref iterator
that wasn't ordered. Refactor the ref iterator infrastructure so that we
always assume iterators to be ordered, thus simplifying the code.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
refs.c | 4 ----
refs/debug.c | 3 +--
refs/files-backend.c | 7 +++----
refs/iterator.c | 26 ++++++++------------------
refs/packed-backend.c | 2 +-
refs/ref-cache.c | 2 +-
refs/refs-internal.h | 18 ++----------------
refs/reftable-backend.c | 8 ++++----
8 files changed, 20 insertions(+), 50 deletions(-)
diff --git a/refs.c b/refs.c
index fff343c256..dc25606a82 100644
--- a/refs.c
+++ b/refs.c
@@ -1594,10 +1594,6 @@ struct ref_iterator *refs_ref_iterator_begin(
if (trim)
iter = prefix_ref_iterator_begin(iter, "", trim);
- /* Sanity check for subclasses: */
- if (!iter->ordered)
- BUG("reference iterator is not ordered");
-
return iter;
}
diff --git a/refs/debug.c b/refs/debug.c
index 634681ca44..c7531b17f0 100644
--- a/refs/debug.c
+++ b/refs/debug.c
@@ -181,7 +181,6 @@ static int debug_ref_iterator_advance(struct ref_iterator *ref_iterator)
trace_printf_key(&trace_refs, "iterator_advance: %s (0)\n",
diter->iter->refname);
- diter->base.ordered = diter->iter->ordered;
diter->base.refname = diter->iter->refname;
diter->base.oid = diter->iter->oid;
diter->base.flags = diter->iter->flags;
@@ -222,7 +221,7 @@ debug_ref_iterator_begin(struct ref_store *ref_store, const char *prefix,
drefs->refs->be->iterator_begin(drefs->refs, prefix,
exclude_patterns, flags);
struct debug_ref_iterator *diter = xcalloc(1, sizeof(*diter));
- base_ref_iterator_init(&diter->base, &debug_ref_iterator_vtable, 1);
+ base_ref_iterator_init(&diter->base, &debug_ref_iterator_vtable);
diter->iter = res;
trace_printf_key(&trace_refs, "ref_iterator_begin: \"%s\" (0x%x)\n",
prefix, flags);
diff --git a/refs/files-backend.c b/refs/files-backend.c
index a7b7cdef36..51d57d98d2 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -879,8 +879,7 @@ static struct ref_iterator *files_ref_iterator_begin(
CALLOC_ARRAY(iter, 1);
ref_iterator = &iter->base;
- base_ref_iterator_init(ref_iterator, &files_ref_iterator_vtable,
- overlay_iter->ordered);
+ base_ref_iterator_init(ref_iterator, &files_ref_iterator_vtable);
iter->iter0 = overlay_iter;
iter->repo = ref_store->repo;
iter->flags = flags;
@@ -2202,7 +2201,7 @@ static struct ref_iterator *reflog_iterator_begin(struct ref_store *ref_store,
CALLOC_ARRAY(iter, 1);
ref_iterator = &iter->base;
- base_ref_iterator_init(ref_iterator, &files_reflog_iterator_vtable, 1);
+ base_ref_iterator_init(ref_iterator, &files_reflog_iterator_vtable);
iter->dir_iterator = diter;
iter->ref_store = ref_store;
strbuf_release(&sb);
@@ -2246,7 +2245,7 @@ static struct ref_iterator *files_reflog_iterator_begin(struct ref_store *ref_st
return reflog_iterator_begin(ref_store, refs->gitcommondir);
} else {
return merge_ref_iterator_begin(
- 1, reflog_iterator_begin(ref_store, refs->base.gitdir),
+ reflog_iterator_begin(ref_store, refs->base.gitdir),
reflog_iterator_begin(ref_store, refs->gitcommondir),
reflog_iterator_select, refs);
}
diff --git a/refs/iterator.c b/refs/iterator.c
index 6b680f610e..f9a9a808e0 100644
--- a/refs/iterator.c
+++ b/refs/iterator.c
@@ -25,11 +25,9 @@ int ref_iterator_abort(struct ref_iterator *ref_iterator)
}
void base_ref_iterator_init(struct ref_iterator *iter,
- struct ref_iterator_vtable *vtable,
- int ordered)
+ struct ref_iterator_vtable *vtable)
{
iter->vtable = vtable;
- iter->ordered = !!ordered;
iter->refname = NULL;
iter->oid = NULL;
iter->flags = 0;
@@ -74,7 +72,7 @@ struct ref_iterator *empty_ref_iterator_begin(void)
struct empty_ref_iterator *iter = xcalloc(1, sizeof(*iter));
struct ref_iterator *ref_iterator = &iter->base;
- base_ref_iterator_init(ref_iterator, &empty_ref_iterator_vtable, 1);
+ base_ref_iterator_init(ref_iterator, &empty_ref_iterator_vtable);
return ref_iterator;
}
@@ -207,7 +205,6 @@ static struct ref_iterator_vtable merge_ref_iterator_vtable = {
};
struct ref_iterator *merge_ref_iterator_begin(
- int ordered,
struct ref_iterator *iter0, struct ref_iterator *iter1,
ref_iterator_select_fn *select, void *cb_data)
{
@@ -222,7 +219,7 @@ struct ref_iterator *merge_ref_iterator_begin(
* references through only if they exist in both iterators.
*/
- base_ref_iterator_init(ref_iterator, &merge_ref_iterator_vtable, ordered);
+ base_ref_iterator_init(ref_iterator, &merge_ref_iterator_vtable);
iter->iter0 = iter0;
iter->iter1 = iter1;
iter->select = select;
@@ -271,12 +268,9 @@ struct ref_iterator *overlay_ref_iterator_begin(
} else if (is_empty_ref_iterator(back)) {
ref_iterator_abort(back);
return front;
- } else if (!front->ordered || !back->ordered) {
- BUG("overlay_ref_iterator requires ordered inputs");
}
- return merge_ref_iterator_begin(1, front, back,
- overlay_iterator_select, NULL);
+ return merge_ref_iterator_begin(front, back, overlay_iterator_select, NULL);
}
struct prefix_ref_iterator {
@@ -315,16 +309,12 @@ static int prefix_ref_iterator_advance(struct ref_iterator *ref_iterator)
if (cmp > 0) {
/*
- * If the source iterator is ordered, then we
+ * As the source iterator is ordered, we
* can stop the iteration as soon as we see a
* refname that comes after the prefix:
*/
- if (iter->iter0->ordered) {
- ok = ref_iterator_abort(iter->iter0);
- break;
- } else {
- continue;
- }
+ ok = ref_iterator_abort(iter->iter0);
+ break;
}
if (iter->trim) {
@@ -396,7 +386,7 @@ struct ref_iterator *prefix_ref_iterator_begin(struct ref_iterator *iter0,
CALLOC_ARRAY(iter, 1);
ref_iterator = &iter->base;
- base_ref_iterator_init(ref_iterator, &prefix_ref_iterator_vtable, iter0->ordered);
+ base_ref_iterator_init(ref_iterator, &prefix_ref_iterator_vtable);
iter->iter0 = iter0;
iter->prefix = xstrdup(prefix);
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index a499a91c7e..4e826c05ff 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -1111,7 +1111,7 @@ static struct ref_iterator *packed_ref_iterator_begin(
CALLOC_ARRAY(iter, 1);
ref_iterator = &iter->base;
- base_ref_iterator_init(ref_iterator, &packed_ref_iterator_vtable, 1);
+ base_ref_iterator_init(ref_iterator, &packed_ref_iterator_vtable);
if (exclude_patterns)
populate_excluded_jump_list(iter, snapshot, exclude_patterns);
diff --git a/refs/ref-cache.c b/refs/ref-cache.c
index a372a00941..9f9797209a 100644
--- a/refs/ref-cache.c
+++ b/refs/ref-cache.c
@@ -486,7 +486,7 @@ struct ref_iterator *cache_ref_iterator_begin(struct ref_cache *cache,
CALLOC_ARRAY(iter, 1);
ref_iterator = &iter->base;
- base_ref_iterator_init(ref_iterator, &cache_ref_iterator_vtable, 1);
+ base_ref_iterator_init(ref_iterator, &cache_ref_iterator_vtable);
ALLOC_GROW(iter->levels, 10, iter->levels_alloc);
iter->levels_nr = 1;
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 83e0f0bba3..1e8a9f9f13 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -312,13 +312,6 @@ enum do_for_each_ref_flags {
*/
struct ref_iterator {
struct ref_iterator_vtable *vtable;
-
- /*
- * Does this `ref_iterator` iterate over references in order
- * by refname?
- */
- unsigned int ordered : 1;
-
const char *refname;
const struct object_id *oid;
unsigned int flags;
@@ -390,11 +383,9 @@ typedef enum iterator_selection ref_iterator_select_fn(
* Iterate over the entries from iter0 and iter1, with the values
* interleaved as directed by the select function. The iterator takes
* ownership of iter0 and iter1 and frees them when the iteration is
- * over. A derived class should set `ordered` to 1 or 0 based on
- * whether it generates its output in order by reference name.
+ * over.
*/
struct ref_iterator *merge_ref_iterator_begin(
- int ordered,
struct ref_iterator *iter0, struct ref_iterator *iter1,
ref_iterator_select_fn *select, void *cb_data);
@@ -423,8 +414,6 @@ struct ref_iterator *overlay_ref_iterator_begin(
* As an convenience to callers, if prefix is the empty string and
* trim is zero, this function returns iter0 directly, without
* wrapping it.
- *
- * The resulting ref_iterator is ordered if iter0 is.
*/
struct ref_iterator *prefix_ref_iterator_begin(struct ref_iterator *iter0,
const char *prefix,
@@ -435,14 +424,11 @@ struct ref_iterator *prefix_ref_iterator_begin(struct ref_iterator *iter0,
/*
* Base class constructor for ref_iterators. Initialize the
* ref_iterator part of iter, setting its vtable pointer as specified.
- * `ordered` should be set to 1 if the iterator will iterate over
- * references in order by refname; otherwise it should be set to 0.
* This is meant to be called only by the initializers of derived
* classes.
*/
void base_ref_iterator_init(struct ref_iterator *iter,
- struct ref_iterator_vtable *vtable,
- int ordered);
+ struct ref_iterator_vtable *vtable);
/*
* Base class destructor for ref_iterators. Destroy the ref_iterator
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index a14f2ad7f4..70a16dfb9e 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -479,7 +479,7 @@ static struct reftable_ref_iterator *ref_iterator_for_stack(struct reftable_ref_
int ret;
iter = xcalloc(1, sizeof(*iter));
- base_ref_iterator_init(&iter->base, &reftable_ref_iterator_vtable, 1);
+ base_ref_iterator_init(&iter->base, &reftable_ref_iterator_vtable);
iter->prefix = prefix;
iter->base.oid = &iter->oid;
iter->flags = flags;
@@ -575,7 +575,7 @@ static struct ref_iterator *reftable_be_iterator_begin(struct ref_store *ref_sto
* single iterator.
*/
worktree_iter = ref_iterator_for_stack(refs, refs->worktree_stack, prefix, flags);
- return merge_ref_iterator_begin(1, &worktree_iter->base, &main_iter->base,
+ return merge_ref_iterator_begin(&worktree_iter->base, &main_iter->base,
iterator_select, NULL);
}
@@ -1723,7 +1723,7 @@ static struct reftable_reflog_iterator *reflog_iterator_for_stack(struct reftabl
int ret;
iter = xcalloc(1, sizeof(*iter));
- base_ref_iterator_init(&iter->base, &reftable_reflog_iterator_vtable, 1);
+ base_ref_iterator_init(&iter->base, &reftable_reflog_iterator_vtable);
iter->refs = refs;
iter->base.oid = &iter->oid;
@@ -1758,7 +1758,7 @@ static struct ref_iterator *reftable_be_reflog_iterator_begin(struct ref_store *
worktree_iter = reflog_iterator_for_stack(refs, refs->worktree_stack);
- return merge_ref_iterator_begin(1, &worktree_iter->base, &main_iter->base,
+ return merge_ref_iterator_begin(&worktree_iter->base, &main_iter->base,
iterator_select, NULL);
}
--
2.44.0-rc1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v2 5/7] refs: drop unused params from the reflog iterator callback
From: Patrick Steinhardt @ 2024-02-20 9:06 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <cover.1708418805.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 12615 bytes --]
The ref and reflog iterators share much of the same underlying code to
iterate over the corresponding entries. This results in some weird code
because the reflog iterator also exposes an object ID as well as a flag
to the callback function. Neither of these fields do refer to the reflog
though -- they refer to the corresponding ref with the same name. This
is quite misleading. In practice at least the object ID cannot really be
implemented in any other way as a reflog does not have a specific object
ID in the first place. This is further stressed by the fact that none of
the callbacks except for our test helper make use of these fields.
Split up the infrastucture so that ref and reflog iterators use separate
callback signatures. This allows us to drop the nonsensical fields from
the reflog iterator.
Note that internally, the backends still use the same shared infra to
iterate over both types. As the backends should never end up being
called directly anyway, this is not much of a problem and thus kept
as-is for simplicity's sake.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/fsck.c | 4 +---
builtin/reflog.c | 3 +--
refs.c | 23 +++++++++++++++++++----
refs.h | 11 +++++++++--
refs/files-backend.c | 8 +-------
refs/reftable-backend.c | 8 +-------
revision.c | 4 +---
t/helper/test-ref-store.c | 18 ++++++++++++------
t/t0600-reffiles-backend.sh | 24 ++++++++++++------------
t/t1405-main-ref-store.sh | 8 ++++----
t/t1406-submodule-ref-store.sh | 8 ++++----
11 files changed, 65 insertions(+), 54 deletions(-)
diff --git a/builtin/fsck.c b/builtin/fsck.c
index a7cf94f67e..f892487c9b 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -509,9 +509,7 @@ static int fsck_handle_reflog_ent(struct object_id *ooid, struct object_id *noid
return 0;
}
-static int fsck_handle_reflog(const char *logname,
- const struct object_id *oid UNUSED,
- int flag UNUSED, void *cb_data)
+static int fsck_handle_reflog(const char *logname, void *cb_data)
{
struct strbuf refname = STRBUF_INIT;
diff --git a/builtin/reflog.c b/builtin/reflog.c
index a5a4099f61..3a0c4d4322 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -60,8 +60,7 @@ struct worktree_reflogs {
struct string_list reflogs;
};
-static int collect_reflog(const char *ref, const struct object_id *oid UNUSED,
- int flags UNUSED, void *cb_data)
+static int collect_reflog(const char *ref, void *cb_data)
{
struct worktree_reflogs *cb = cb_data;
struct worktree *worktree = cb->worktree;
diff --git a/refs.c b/refs.c
index dc25606a82..f9261267f0 100644
--- a/refs.c
+++ b/refs.c
@@ -2512,18 +2512,33 @@ int refs_verify_refname_available(struct ref_store *refs,
return ret;
}
-int refs_for_each_reflog(struct ref_store *refs, each_ref_fn fn, void *cb_data)
+struct do_for_each_reflog_help {
+ each_reflog_fn *fn;
+ void *cb_data;
+};
+
+static int do_for_each_reflog_helper(struct repository *r UNUSED,
+ const char *refname,
+ const struct object_id *oid UNUSED,
+ int flags,
+ void *cb_data)
+{
+ struct do_for_each_reflog_help *hp = cb_data;
+ return hp->fn(refname, hp->cb_data);
+}
+
+int refs_for_each_reflog(struct ref_store *refs, each_reflog_fn fn, void *cb_data)
{
struct ref_iterator *iter;
- struct do_for_each_ref_help hp = { fn, cb_data };
+ struct do_for_each_reflog_help hp = { fn, cb_data };
iter = refs->be->reflog_iterator_begin(refs);
return do_for_each_repo_ref_iterator(the_repository, iter,
- do_for_each_ref_helper, &hp);
+ do_for_each_reflog_helper, &hp);
}
-int for_each_reflog(each_ref_fn fn, void *cb_data)
+int for_each_reflog(each_reflog_fn fn, void *cb_data)
{
return refs_for_each_reflog(get_main_ref_store(the_repository), fn, cb_data);
}
diff --git a/refs.h b/refs.h
index 303c5fac4d..895579aeb7 100644
--- a/refs.h
+++ b/refs.h
@@ -534,12 +534,19 @@ int for_each_reflog_ent(const char *refname, each_reflog_ent_fn fn, void *cb_dat
/* youngest entry first */
int for_each_reflog_ent_reverse(const char *refname, each_reflog_ent_fn fn, void *cb_data);
+/*
+ * The signature for the callback function for the {refs_,}for_each_reflog()
+ * functions below. The memory pointed to by the refname argument is only
+ * guaranteed to be valid for the duration of a single callback invocation.
+ */
+typedef int each_reflog_fn(const char *refname, void *cb_data);
+
/*
* Calls the specified function for each reflog file until it returns nonzero,
* and returns the value. Reflog file order is unspecified.
*/
-int refs_for_each_reflog(struct ref_store *refs, each_ref_fn fn, void *cb_data);
-int for_each_reflog(each_ref_fn fn, void *cb_data);
+int refs_for_each_reflog(struct ref_store *refs, each_reflog_fn fn, void *cb_data);
+int for_each_reflog(each_reflog_fn fn, void *cb_data);
#define REFNAME_ALLOW_ONELEVEL 1
#define REFNAME_REFSPEC_PATTERN 2
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 51d57d98d2..48cc60d71b 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2115,10 +2115,8 @@ static int files_for_each_reflog_ent(struct ref_store *ref_store,
struct files_reflog_iterator {
struct ref_iterator base;
-
struct ref_store *ref_store;
struct dir_iterator *dir_iterator;
- struct object_id oid;
};
static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator)
@@ -2129,8 +2127,6 @@ static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator)
int ok;
while ((ok = dir_iterator_advance(diter)) == ITER_OK) {
- int flags;
-
if (!S_ISREG(diter->st.st_mode))
continue;
if (diter->basename[0] == '.')
@@ -2140,14 +2136,12 @@ static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator)
if (!refs_resolve_ref_unsafe(iter->ref_store,
diter->relative_path, 0,
- &iter->oid, &flags)) {
+ NULL, NULL)) {
error("bad ref for %s", diter->path.buf);
continue;
}
iter->base.refname = diter->relative_path;
- iter->base.oid = &iter->oid;
- iter->base.flags = flags;
return ITER_OK;
}
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 70a16dfb9e..5247e09d58 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -1637,7 +1637,6 @@ struct reftable_reflog_iterator {
struct reftable_ref_store *refs;
struct reftable_iterator iter;
struct reftable_log_record log;
- struct object_id oid;
char *last_name;
int err;
};
@@ -1648,8 +1647,6 @@ static int reftable_reflog_iterator_advance(struct ref_iterator *ref_iterator)
(struct reftable_reflog_iterator *)ref_iterator;
while (!iter->err) {
- int flags;
-
iter->err = reftable_iterator_next_log(&iter->iter, &iter->log);
if (iter->err)
break;
@@ -1663,7 +1660,7 @@ static int reftable_reflog_iterator_advance(struct ref_iterator *ref_iterator)
continue;
if (!refs_resolve_ref_unsafe(&iter->refs->base, iter->log.refname,
- 0, &iter->oid, &flags)) {
+ 0, NULL, NULL)) {
error(_("bad ref for %s"), iter->log.refname);
continue;
}
@@ -1671,8 +1668,6 @@ static int reftable_reflog_iterator_advance(struct ref_iterator *ref_iterator)
free(iter->last_name);
iter->last_name = xstrdup(iter->log.refname);
iter->base.refname = iter->log.refname;
- iter->base.oid = &iter->oid;
- iter->base.flags = flags;
break;
}
@@ -1725,7 +1720,6 @@ static struct reftable_reflog_iterator *reflog_iterator_for_stack(struct reftabl
iter = xcalloc(1, sizeof(*iter));
base_ref_iterator_init(&iter->base, &reftable_reflog_iterator_vtable);
iter->refs = refs;
- iter->base.oid = &iter->oid;
ret = refs->err;
if (ret)
diff --git a/revision.c b/revision.c
index 2424c9bd67..ac45c6d8f2 100644
--- a/revision.c
+++ b/revision.c
@@ -1686,9 +1686,7 @@ static int handle_one_reflog_ent(struct object_id *ooid, struct object_id *noid,
return 0;
}
-static int handle_one_reflog(const char *refname_in_wt,
- const struct object_id *oid UNUSED,
- int flag UNUSED, void *cb_data)
+static int handle_one_reflog(const char *refname_in_wt, void *cb_data)
{
struct all_refs_cb *cb = cb_data;
struct strbuf refname = STRBUF_INIT;
diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c
index 702ec1f128..7a0f6cac53 100644
--- a/t/helper/test-ref-store.c
+++ b/t/helper/test-ref-store.c
@@ -221,15 +221,21 @@ static int cmd_verify_ref(struct ref_store *refs, const char **argv)
return ret;
}
+static int each_reflog(const char *refname, void *cb_data UNUSED)
+{
+ printf("%s\n", refname);
+ return 0;
+}
+
static int cmd_for_each_reflog(struct ref_store *refs,
const char **argv UNUSED)
{
- return refs_for_each_reflog(refs, each_ref, NULL);
+ return refs_for_each_reflog(refs, each_reflog, NULL);
}
-static int each_reflog(struct object_id *old_oid, struct object_id *new_oid,
- const char *committer, timestamp_t timestamp,
- int tz, const char *msg, void *cb_data UNUSED)
+static int each_reflog_ent(struct object_id *old_oid, struct object_id *new_oid,
+ const char *committer, timestamp_t timestamp,
+ int tz, const char *msg, void *cb_data UNUSED)
{
printf("%s %s %s %" PRItime " %+05d%s%s", oid_to_hex(old_oid),
oid_to_hex(new_oid), committer, timestamp, tz,
@@ -241,14 +247,14 @@ static int cmd_for_each_reflog_ent(struct ref_store *refs, const char **argv)
{
const char *refname = notnull(*argv++, "refname");
- return refs_for_each_reflog_ent(refs, refname, each_reflog, refs);
+ return refs_for_each_reflog_ent(refs, refname, each_reflog_ent, refs);
}
static int cmd_for_each_reflog_ent_reverse(struct ref_store *refs, const char **argv)
{
const char *refname = notnull(*argv++, "refname");
- return refs_for_each_reflog_ent_reverse(refs, refname, each_reflog, refs);
+ return refs_for_each_reflog_ent_reverse(refs, refname, each_reflog_ent, refs);
}
static int cmd_reflog_exists(struct ref_store *refs, const char **argv)
diff --git a/t/t0600-reffiles-backend.sh b/t/t0600-reffiles-backend.sh
index 4f860285cc..56a3196b83 100755
--- a/t/t0600-reffiles-backend.sh
+++ b/t/t0600-reffiles-backend.sh
@@ -287,23 +287,23 @@ test_expect_success 'for_each_reflog()' '
mkdir -p .git/worktrees/wt/logs/refs/bisect &&
echo $ZERO_OID > .git/worktrees/wt/logs/refs/bisect/wt-random &&
- $RWT for-each-reflog | cut -d" " -f 2- >actual &&
+ $RWT for-each-reflog >actual &&
cat >expected <<-\EOF &&
- HEAD 0x1
- PSEUDO-WT 0x0
- refs/bisect/wt-random 0x0
- refs/heads/main 0x0
- refs/heads/wt-main 0x0
+ HEAD
+ PSEUDO-WT
+ refs/bisect/wt-random
+ refs/heads/main
+ refs/heads/wt-main
EOF
test_cmp expected actual &&
- $RMAIN for-each-reflog | cut -d" " -f 2- >actual &&
+ $RMAIN for-each-reflog >actual &&
cat >expected <<-\EOF &&
- HEAD 0x1
- PSEUDO-MAIN 0x0
- refs/bisect/random 0x0
- refs/heads/main 0x0
- refs/heads/wt-main 0x0
+ HEAD
+ PSEUDO-MAIN
+ refs/bisect/random
+ refs/heads/main
+ refs/heads/wt-main
EOF
test_cmp expected actual
'
diff --git a/t/t1405-main-ref-store.sh b/t/t1405-main-ref-store.sh
index cfb583f544..3eee758bce 100755
--- a/t/t1405-main-ref-store.sh
+++ b/t/t1405-main-ref-store.sh
@@ -74,11 +74,11 @@ test_expect_success 'verify_ref(new-main)' '
'
test_expect_success 'for_each_reflog()' '
- $RUN for-each-reflog | cut -d" " -f 2- >actual &&
+ $RUN for-each-reflog >actual &&
cat >expected <<-\EOF &&
- HEAD 0x1
- refs/heads/main 0x0
- refs/heads/new-main 0x0
+ HEAD
+ refs/heads/main
+ refs/heads/new-main
EOF
test_cmp expected actual
'
diff --git a/t/t1406-submodule-ref-store.sh b/t/t1406-submodule-ref-store.sh
index 40332e23cc..c01f0f14a1 100755
--- a/t/t1406-submodule-ref-store.sh
+++ b/t/t1406-submodule-ref-store.sh
@@ -63,11 +63,11 @@ test_expect_success 'verify_ref(new-main)' '
'
test_expect_success 'for_each_reflog()' '
- $RUN for-each-reflog | cut -d" " -f 2- >actual &&
+ $RUN for-each-reflog >actual &&
cat >expected <<-\EOF &&
- HEAD 0x1
- refs/heads/main 0x0
- refs/heads/new-main 0x0
+ HEAD
+ refs/heads/main
+ refs/heads/new-main
EOF
test_cmp expected actual
'
--
2.44.0-rc1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v2 6/7] refs: stop resolving ref corresponding to reflogs
From: Patrick Steinhardt @ 2024-02-20 9:06 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <cover.1708418805.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 3407 bytes --]
The reflog iterator tries to resolve the corresponding ref for every
reflog that it is about to yield. Historically, this was done due to
multiple reasons:
- It ensures that the refname is safe because we end up calling
`check_refname_format()`. Also, non-conformant refnames are skipped
altogether.
- The iterator used to yield the resolved object ID as well as its
flags to the callback. This info was never used though, and the
corresponding parameters were dropped in the preceding commit.
- When a ref is corrupt then the reflog is not emitted at all.
We're about to introduce a new `git reflog list` subcommand that will
print all reflogs that the refdb knows about. Skipping over reflogs
whose refs are corrupted would be quite counterproductive in this case
as the user would have no way to learn about reflogs which may still
exist in their repository to help and rescue such a corrupted ref. Thus,
the only remaining reason for why we'd want to resolve the ref is to
verify its refname.
Refactor the code to call `check_refname_format()` directly instead of
trying to resolve the ref. This is significantly more efficient given
that we don't have to hit the object database anymore to list reflogs.
And second, it ensures that we end up showing reflogs of broken refs,
which will help to make the reflog more useful.
Note that this really only impacts the case where the corresponding ref
is corrupt. Reflogs for nonexistent refs would have been returned to the
caller beforehand already as we did not pass `RESOLVE_REF_READING` to
the function, and thus `refs_resolve_ref_unsafe()` would have returned
successfully in that case.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
refs/files-backend.c | 12 ++----------
refs/reftable-backend.c | 6 ++----
2 files changed, 4 insertions(+), 14 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 48cc60d71b..4726b04baa 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2129,17 +2129,9 @@ static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator)
while ((ok = dir_iterator_advance(diter)) == ITER_OK) {
if (!S_ISREG(diter->st.st_mode))
continue;
- if (diter->basename[0] == '.')
+ if (check_refname_format(diter->basename,
+ REFNAME_ALLOW_ONELEVEL))
continue;
- if (ends_with(diter->basename, ".lock"))
- continue;
-
- if (!refs_resolve_ref_unsafe(iter->ref_store,
- diter->relative_path, 0,
- NULL, NULL)) {
- error("bad ref for %s", diter->path.buf);
- continue;
- }
iter->base.refname = diter->relative_path;
return ITER_OK;
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 5247e09d58..f3200a1886 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -1659,11 +1659,9 @@ static int reftable_reflog_iterator_advance(struct ref_iterator *ref_iterator)
if (iter->last_name && !strcmp(iter->log.refname, iter->last_name))
continue;
- if (!refs_resolve_ref_unsafe(&iter->refs->base, iter->log.refname,
- 0, NULL, NULL)) {
- error(_("bad ref for %s"), iter->log.refname);
+ if (check_refname_format(iter->log.refname,
+ REFNAME_ALLOW_ONELEVEL))
continue;
- }
free(iter->last_name);
iter->last_name = xstrdup(iter->log.refname);
--
2.44.0-rc1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v2 7/7] builtin/reflog: introduce subcommand to list reflogs
From: Patrick Steinhardt @ 2024-02-20 9:06 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <cover.1708418805.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 6086 bytes --]
While the git-reflog(1) command has subcommands to show reflog entries
or check for reflog existence, it does not have any subcommands that
would allow the user to enumerate all existing reflogs. This makes it
quite hard to discover which reflogs a repository has. While this can
be worked around with the "files" backend by enumerating files in the
".git/logs" directory, users of the "reftable" backend don't enjoy such
a luxury.
Introduce a new subcommand `git reflog list` that lists all reflogs the
repository knows of to fill this gap.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
Documentation/git-reflog.txt | 3 ++
builtin/reflog.c | 34 ++++++++++++++++++
t/t1410-reflog.sh | 69 ++++++++++++++++++++++++++++++++++++
3 files changed, 106 insertions(+)
diff --git a/Documentation/git-reflog.txt b/Documentation/git-reflog.txt
index ec64cbff4c..a929c52982 100644
--- a/Documentation/git-reflog.txt
+++ b/Documentation/git-reflog.txt
@@ -10,6 +10,7 @@ SYNOPSIS
--------
[verse]
'git reflog' [show] [<log-options>] [<ref>]
+'git reflog list'
'git reflog expire' [--expire=<time>] [--expire-unreachable=<time>]
[--rewrite] [--updateref] [--stale-fix]
[--dry-run | -n] [--verbose] [--all [--single-worktree] | <refs>...]
@@ -39,6 +40,8 @@ actions, and in addition the `HEAD` reflog records branch switching.
`git reflog show` is an alias for `git log -g --abbrev-commit
--pretty=oneline`; see linkgit:git-log[1] for more information.
+The "list" subcommand lists all refs which have a corresponding reflog.
+
The "expire" subcommand prunes older reflog entries. Entries older
than `expire` time, or entries older than `expire-unreachable` time
and not reachable from the current tip, are removed from the reflog.
diff --git a/builtin/reflog.c b/builtin/reflog.c
index 3a0c4d4322..63cd4d8b29 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -7,11 +7,15 @@
#include "wildmatch.h"
#include "worktree.h"
#include "reflog.h"
+#include "refs.h"
#include "parse-options.h"
#define BUILTIN_REFLOG_SHOW_USAGE \
N_("git reflog [show] [<log-options>] [<ref>]")
+#define BUILTIN_REFLOG_LIST_USAGE \
+ N_("git reflog list")
+
#define BUILTIN_REFLOG_EXPIRE_USAGE \
N_("git reflog expire [--expire=<time>] [--expire-unreachable=<time>]\n" \
" [--rewrite] [--updateref] [--stale-fix]\n" \
@@ -29,6 +33,11 @@ static const char *const reflog_show_usage[] = {
NULL,
};
+static const char *const reflog_list_usage[] = {
+ BUILTIN_REFLOG_LIST_USAGE,
+ NULL,
+};
+
static const char *const reflog_expire_usage[] = {
BUILTIN_REFLOG_EXPIRE_USAGE,
NULL
@@ -46,6 +55,7 @@ static const char *const reflog_exists_usage[] = {
static const char *const reflog_usage[] = {
BUILTIN_REFLOG_SHOW_USAGE,
+ BUILTIN_REFLOG_LIST_USAGE,
BUILTIN_REFLOG_EXPIRE_USAGE,
BUILTIN_REFLOG_DELETE_USAGE,
BUILTIN_REFLOG_EXISTS_USAGE,
@@ -238,6 +248,29 @@ static int cmd_reflog_show(int argc, const char **argv, const char *prefix)
return cmd_log_reflog(argc, argv, prefix);
}
+static int show_reflog(const char *refname, void *cb_data UNUSED)
+{
+ printf("%s\n", refname);
+ return 0;
+}
+
+static int cmd_reflog_list(int argc, const char **argv, const char *prefix)
+{
+ struct option options[] = {
+ OPT_END()
+ };
+ struct ref_store *ref_store;
+
+ argc = parse_options(argc, argv, prefix, options, reflog_list_usage, 0);
+ if (argc)
+ return error(_("%s does not accept arguments: '%s'"),
+ "list", argv[0]);
+
+ ref_store = get_main_ref_store(the_repository);
+
+ return refs_for_each_reflog(ref_store, show_reflog, NULL);
+}
+
static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
{
struct cmd_reflog_expire_cb cmd = { 0 };
@@ -417,6 +450,7 @@ int cmd_reflog(int argc, const char **argv, const char *prefix)
parse_opt_subcommand_fn *fn = NULL;
struct option options[] = {
OPT_SUBCOMMAND("show", &fn, cmd_reflog_show),
+ OPT_SUBCOMMAND("list", &fn, cmd_reflog_list),
OPT_SUBCOMMAND("expire", &fn, cmd_reflog_expire),
OPT_SUBCOMMAND("delete", &fn, cmd_reflog_delete),
OPT_SUBCOMMAND("exists", &fn, cmd_reflog_exists),
diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh
index d2f5f42e67..6d8d5a253d 100755
--- a/t/t1410-reflog.sh
+++ b/t/t1410-reflog.sh
@@ -436,4 +436,73 @@ test_expect_success 'empty reflog' '
test_must_be_empty err
'
+test_expect_success 'list reflogs' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ git reflog list >actual &&
+ test_must_be_empty actual &&
+
+ test_commit A &&
+ cat >expect <<-EOF &&
+ HEAD
+ refs/heads/main
+ EOF
+ git reflog list >actual &&
+ test_cmp expect actual &&
+
+ git branch b &&
+ cat >expect <<-EOF &&
+ HEAD
+ refs/heads/b
+ refs/heads/main
+ EOF
+ git reflog list >actual &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success 'reflog list returns error with additional args' '
+ cat >expect <<-EOF &&
+ error: list does not accept arguments: ${SQ}bogus${SQ}
+ EOF
+ test_must_fail git reflog list bogus 2>err &&
+ test_cmp expect err
+'
+
+test_expect_success 'reflog for symref with unborn target can be listed' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit A &&
+ git symbolic-ref HEAD refs/heads/unborn &&
+ cat >expect <<-EOF &&
+ HEAD
+ refs/heads/main
+ EOF
+ git reflog list >actual &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success 'reflog with invalid object ID can be listed' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit A &&
+ test-tool ref-store main update-ref msg refs/heads/missing \
+ $(test_oid deadbeef) "$ZERO_OID" REF_SKIP_OID_VERIFICATION &&
+ cat >expect <<-EOF &&
+ HEAD
+ refs/heads/main
+ refs/heads/missing
+ EOF
+ git reflog list >actual &&
+ test_cmp expect actual
+ )
+'
+
test_done
--
2.44.0-rc1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH] Revert "Declare both git-switch and git-restore experimental"
From: Matthieu Baerts (NGI0) @ 2024-02-20 9:29 UTC (permalink / raw)
To: git
Cc: Nguyễn Thái Ngọc Duy, Junio C Hamano,
Matthieu Baerts (NGI0)
This reverts commit 4e43b7ff1ea4b6f16b93a432b6718e9ab38749bd.
Recently, I wanted to recommend the use of git-switch and git-restore
instead of git-checkout in new documentation pages. But then, I found
out that these two commands were still marked as experimental in the
documentation. git-switch and git-restore have been marked as such since
their introduction, in version 2.23.
That was for good reasons, according to the reverted commit:
> These two commands are basically redesigned git-checkout. We will not
> have that many opportunities to redo (because we run out of verbs, and
> that would also increase maintenance cost).
The reverted commit also mentions this:
> To play it safe, let's declare the two commands experimental in one or
> two releases. If there is a serious flaw in the UI, we could still fix
> it. If everything goes well and nobody complains loudly, we can remove
> the experimental status by reverting this patch.
Version 2.44 is approaching, almost 5 years after the introduction of
these two commands, it then looks safe to remove this experimental
status.
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Notes:
Here is a simple 'git revert', as suggested in 4e43b7ff1e ("Declare both
git-switch and git-restore experimental"), without any conflicts to
resolve.
BTW, thank you very much for maintaining and still improving this great
tool!
Documentation/git-restore.txt | 2 --
Documentation/git-switch.txt | 2 --
2 files changed, 4 deletions(-)
diff --git a/Documentation/git-restore.txt b/Documentation/git-restore.txt
index 975825b44a..4f5531c440 100644
--- a/Documentation/git-restore.txt
+++ b/Documentation/git-restore.txt
@@ -28,8 +28,6 @@ otherwise from the index. Use `--source` to restore from a different commit.
See "Reset, restore and revert" in linkgit:git[1] for the differences
between the three commands.
-THIS COMMAND IS EXPERIMENTAL. THE BEHAVIOR MAY CHANGE.
-
OPTIONS
-------
-s <tree>::
diff --git a/Documentation/git-switch.txt b/Documentation/git-switch.txt
index f38e4c8afa..96cfd9ba52 100644
--- a/Documentation/git-switch.txt
+++ b/Documentation/git-switch.txt
@@ -29,8 +29,6 @@ Switching branches does not require a clean index and working tree
however if the operation leads to loss of local changes, unless told
otherwise with `--discard-changes` or `--merge`.
-THIS COMMAND IS EXPERIMENTAL. THE BEHAVIOR MAY CHANGE.
-
OPTIONS
-------
<branch>::
--
2.43.0
^ permalink raw reply related
* Re: [PATCH] Revert "Declare both git-switch and git-restore experimental"
From: Kristoffer Haugsbakk @ 2024-02-20 9:36 UTC (permalink / raw)
To: Matthieu Baerts (NGI0)
Cc: Nguyễn Thái Ngọc Duy, Junio C Hamano, git
In-Reply-To: <20240220092957.1296283-2-matttbe@kernel.org>
On Tue, Feb 20, 2024, at 10:29, Matthieu Baerts (NGI0) wrote:
> This reverts commit 4e43b7ff1ea4b6f16b93a432b6718e9ab38749bd.
> Version 2.44 is approaching, almost 5 years after the introduction of
> these two commands, it then looks safe to remove this experimental
> status.
Is this only based on the amount of time passed? Has there been any
relevant discussions on the mailing list that discuss how mature these
commands are and if they should be changed (with presumably a “no” to
the question about being changed)?
--
Kristoffer Haugsbakk
^ permalink raw reply
* Re: [PATCH] git-difftool--helper: honor `--trust-exit-code` with `--dir-diff`
From: Patrick Steinhardt @ 2024-02-20 9:56 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jean-Rémy Falleri, David Aguilar
In-Reply-To: <xmqqttm8i8hb.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 1754 bytes --]
On Fri, Feb 16, 2024 at 10:12:32AM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
[snip]
> > diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh
> > index e4e820e680..09d8542917 100755
> > --- a/git-difftool--helper.sh
> > +++ b/git-difftool--helper.sh
> > @@ -91,6 +91,20 @@ then
> > # ignore the error from the above --- run_merge_tool
> > # will diagnose unusable tool by itself
> > run_merge_tool "$merge_tool" false
> > +
> > + status=$?
> > + if test $status -ge 126
> > + then
> > + # Command not found (127), not executable (126) or
> > + # exited via a signal (>= 128).
> > + exit $status
> > + fi
>
> So these errors spawning the tool backend are always reported,
> regardless of the trust-exit-code settings. OK.
>
> > + if test "$status" != 0 &&
> > + test "$GIT_DIFFTOOL_TRUST_EXIT_CODE" = true
> > + then
> > + exit $status
> > + fi
>
> I found this somehow harder to reason about than necessary. Just
>
> if test "$GIT_DIFFTOOL_TRUST_EXIT_CODE" = true
> then
> exit $status
> fi
>
> would have been a more straight-forward expression of what we want
> to happen here, i.e. "if we are told to report the tool's exit
> status, we do so, regardless of what the exit status is".
>
> Not that the construct in your patch is wrong---we will exit with 0
> at the end even when "trust-exit-code" thing is true and the tool
> returned success.
Fair point indeed. Looks like I was a bit too lazy here by simply
copying over the construct from the non-dir-diff case. Over there we
need to special case the 0 exit code because we don't want to exit the
loop in that case. But here it's completely unnecessary.
Will adapt, thanks!
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH] Revert "Declare both git-switch and git-restore experimental"
From: Matthieu Baerts @ 2024-02-20 9:58 UTC (permalink / raw)
To: Kristoffer Haugsbakk
Cc: Nguyễn Thái Ngọc Duy, Junio C Hamano, git
In-Reply-To: <3523e325-98bf-4d2d-847b-28e5c4a85ec5@app.fastmail.com>
Hi Kristoffer,
Thank you for your comment.
On 20/02/2024 10:36, Kristoffer Haugsbakk wrote:
> On Tue, Feb 20, 2024, at 10:29, Matthieu Baerts (NGI0) wrote:
>> This reverts commit 4e43b7ff1ea4b6f16b93a432b6718e9ab38749bd.
>> Version 2.44 is approaching, almost 5 years after the introduction of
>> these two commands, it then looks safe to remove this experimental
>> status.
>
> Is this only based on the amount of time passed? Has there been any
> relevant discussions on the mailing list that discuss how mature these
> commands are and if they should be changed (with presumably a “no” to
> the question about being changed)?
It is only based on the amount of time passed, indeed.
I initially wanted to start a discussion on the mailing list: "is it
normal these commands are still marked as experimental?". Then I saw the
patch introducing this status, which was suggesting doing a revert in
version 2.24 or 2.25. That's why I sent this, to start the discussions
with a patch that is ready to apply. Is it not OK to do that here?
Also, when I quickly looked at the history, I didn't see any behaviour
changes since their introduction. Maybe there was a minor change with
commit 088018e34d ("restore: default to HEAD when combining --staged and
--worktree"), but it looks more like a fix than a behaviour change.
Cheers,
Matt
--
Sponsored by the NGI0 Core fund.
^ permalink raw reply
* [PATCH v2] git-difftool--helper: honor `--trust-exit-code` with `--dir-diff`
From: Patrick Steinhardt @ 2024-02-20 10:08 UTC (permalink / raw)
To: git; +Cc: Jean-Rémy Falleri, Junio C Hamano
In-Reply-To: <976C9BF2-CB82-429A-B9FA-6A14BCFFCA3D@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 7857 bytes --]
The `--trust-exit-code` option for git-diff-tool(1) was introduced via
2b52123fcf (difftool: add support for --trust-exit-code, 2014-10-26).
When set, it makes us return the exit code of the invoked diff tool when
diffing multiple files. This patch didn't change the code path where
`--dir-diff` was passed because we already returned the exit code of the
diff tool unconditionally in that case.
This was changed a month later via c41d3fedd8 (difftool--helper: add
explicit exit statement, 2014-11-20), where an explicit `exit 0` was
added to the end of git-difftool--helper.sh. While the stated intent of
that commit was merely a cleanup, it had the consequence that we now
to ignore the exit code of the diff tool when `--dir-diff` was set. This
change in behaviour is thus very likely an unintended side effect of
this patch.
Now there are two ways to fix this:
- We can either restore the original behaviour, which unconditionally
returned the exit code of the diffing tool when `--dir-diff` is
passed.
- Or we can make the `--dir-diff` case respect the `--trust-exit-code`
flag.
The fact that we have been ignoring exit codes for 7 years by now makes
me rather lean towards the latter option. Furthermore, respecting the
flag in one case but not the other would needlessly make the user
interface more complex.
Fix the bug so that we also honor `--trust-exit-code` for dir diffs and
adjust the documentation accordingly.
Reported-by: Jean-Rémy Falleri <jr.falleri@gmail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
Range-diff against v1:
1: fd6cf7a85a ! 1: 0fac668f8f git-difftool--helper: honor `--trust-exit-code` with `--dir-diff`
@@ git-difftool--helper.sh: then
+ exit $status
+ fi
+
-+ if test "$status" != 0 &&
-+ test "$GIT_DIFFTOOL_TRUST_EXIT_CODE" = true
++ if test "$GIT_DIFFTOOL_TRUST_EXIT_CODE" = true
+ then
+ exit $status
+ fi
Documentation/git-difftool.txt | 1 -
git-difftool--helper.sh | 13 +++++
t/t7800-difftool.sh | 99 ++++++++++++++++++----------------
3 files changed, 67 insertions(+), 46 deletions(-)
diff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt
index c05f97aca9..a616f8b2e6 100644
--- a/Documentation/git-difftool.txt
+++ b/Documentation/git-difftool.txt
@@ -105,7 +105,6 @@ instead. `--no-symlinks` is the default on Windows.
`merge.tool` until a tool is found.
--[no-]trust-exit-code::
- 'git-difftool' invokes a diff tool individually on each file.
Errors reported by the diff tool are ignored by default.
Use `--trust-exit-code` to make 'git-difftool' exit when an
invoked diff tool returns a non-zero exit code.
diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh
index e4e820e680..dd0c9a5b7f 100755
--- a/git-difftool--helper.sh
+++ b/git-difftool--helper.sh
@@ -91,6 +91,19 @@ then
# ignore the error from the above --- run_merge_tool
# will diagnose unusable tool by itself
run_merge_tool "$merge_tool" false
+
+ status=$?
+ if test $status -ge 126
+ then
+ # Command not found (127), not executable (126) or
+ # exited via a signal (>= 128).
+ exit $status
+ fi
+
+ if test "$GIT_DIFFTOOL_TRUST_EXIT_CODE" = true
+ then
+ exit $status
+ fi
else
# Launch the merge tool on each path provided by 'git diff'
while test $# -gt 6
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 6a36be1e63..96ae5d5880 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -91,58 +91,67 @@ test_expect_success 'difftool forwards arguments to diff' '
rm for-diff
'
-test_expect_success 'difftool ignores exit code' '
- test_config difftool.error.cmd false &&
- git difftool -y -t error branch
-'
+for opt in '' '--dir-diff'
+do
+ test_expect_success "difftool ${opt} ignores exit code" "
+ test_config difftool.error.cmd false &&
+ git difftool ${opt} -y -t error branch
+ "
-test_expect_success 'difftool forwards exit code with --trust-exit-code' '
- test_config difftool.error.cmd false &&
- test_must_fail git difftool -y --trust-exit-code -t error branch
-'
+ test_expect_success "difftool ${opt} forwards exit code with --trust-exit-code" "
+ test_config difftool.error.cmd false &&
+ test_must_fail git difftool ${opt} -y --trust-exit-code -t error branch
+ "
-test_expect_success 'difftool forwards exit code with --trust-exit-code for built-ins' '
- test_config difftool.vimdiff.path false &&
- test_must_fail git difftool -y --trust-exit-code -t vimdiff branch
-'
+ test_expect_success "difftool ${opt} forwards exit code with --trust-exit-code for built-ins" "
+ test_config difftool.vimdiff.path false &&
+ test_must_fail git difftool ${opt} -y --trust-exit-code -t vimdiff branch
+ "
-test_expect_success 'difftool honors difftool.trustExitCode = true' '
- test_config difftool.error.cmd false &&
- test_config difftool.trustExitCode true &&
- test_must_fail git difftool -y -t error branch
-'
+ test_expect_success "difftool ${opt} honors difftool.trustExitCode = true" "
+ test_config difftool.error.cmd false &&
+ test_config difftool.trustExitCode true &&
+ test_must_fail git difftool ${opt} -y -t error branch
+ "
-test_expect_success 'difftool honors difftool.trustExitCode = false' '
- test_config difftool.error.cmd false &&
- test_config difftool.trustExitCode false &&
- git difftool -y -t error branch
-'
+ test_expect_success "difftool ${opt} honors difftool.trustExitCode = false" "
+ test_config difftool.error.cmd false &&
+ test_config difftool.trustExitCode false &&
+ git difftool ${opt} -y -t error branch
+ "
-test_expect_success 'difftool ignores exit code with --no-trust-exit-code' '
- test_config difftool.error.cmd false &&
- test_config difftool.trustExitCode true &&
- git difftool -y --no-trust-exit-code -t error branch
-'
+ test_expect_success "difftool ${opt} ignores exit code with --no-trust-exit-code" "
+ test_config difftool.error.cmd false &&
+ test_config difftool.trustExitCode true &&
+ git difftool ${opt} -y --no-trust-exit-code -t error branch
+ "
-test_expect_success 'difftool stops on error with --trust-exit-code' '
- test_when_finished "rm -f for-diff .git/fail-right-file" &&
- test_when_finished "git reset -- for-diff" &&
- write_script .git/fail-right-file <<-\EOF &&
- echo failed
- exit 1
- EOF
- >for-diff &&
- git add for-diff &&
- test_must_fail git difftool -y --trust-exit-code \
- --extcmd .git/fail-right-file branch >actual &&
- test_line_count = 1 actual
-'
+ test_expect_success "difftool ${opt} stops on error with --trust-exit-code" "
+ test_when_finished 'rm -f for-diff .git/fail-right-file' &&
+ test_when_finished 'git reset -- for-diff' &&
+ write_script .git/fail-right-file <<-\EOF &&
+ echo failed
+ exit 1
+ EOF
+ >for-diff &&
+ git add for-diff &&
+ test_must_fail git difftool ${opt} -y --trust-exit-code \
+ --extcmd .git/fail-right-file branch >actual &&
+ test_line_count = 1 actual
+ "
-test_expect_success 'difftool honors exit status if command not found' '
- test_config difftool.nonexistent.cmd i-dont-exist &&
- test_config difftool.trustExitCode false &&
- test_must_fail git difftool -y -t nonexistent branch
-'
+ test_expect_success "difftool ${opt} honors exit status if command not found" "
+ test_config difftool.nonexistent.cmd i-dont-exist &&
+ test_config difftool.trustExitCode false &&
+ if test "${opt}" = '--dir-diff'
+ then
+ expected_code=127
+ else
+ expected_code=128
+ fi &&
+ test_expect_code \${expected_code} git difftool ${opt} -y -t nonexistent branch
+ "
+done
test_expect_success 'difftool honors --gui' '
difftool_test_setup &&
--
2.44.0-rc1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* Re: [PATCH 0/5] promise: introduce promises to track success or error
From: Phillip Wood @ 2024-02-20 10:53 UTC (permalink / raw)
To: Jeff King, phillip.wood
Cc: Philip Peterson via GitGitGadget, git, Johannes Schindelin,
Emily Shaffer, Philip Peterson
In-Reply-To: <20240220025722.GE2713741@coredump.intra.peff.net>
Hi Peff
On 20/02/2024 02:57, Jeff King wrote:
> On Mon, Feb 19, 2024 at 02:25:29PM +0000, Phillip Wood wrote:
>
>> I'm sure this has been discussed in the past but I didn't manage to turn
>> anything up with a quick search of the archive on lore.kernel.org.
>
> There's some discussion in this sub-thread:
>
> https://lore.kernel.org/git/20171103191309.sth4zjokgcupvk2e@sigill.intra.peff.net/
>
> that also references this earlier thread:
>
> https://lore.kernel.org/git/20160927191955.mympqgylrxhkp24n@sigill.intra.peff.net/
Thanks for digging up those links
> I still think this is a reasonable way to go. At one point I had a
> proof-of-concept conversion of some of the ref code, but I don't think I
> have it any more.
Ah, that's interesting - the ref transaction functions already take a
struct strbuf to populate with an error message so maybe that would be a
simple place to start with a conversion to an error struct.
Best Wishes
Phillip
^ permalink raw reply
* Re: [PATCH] builtin/stash: configs keepIndex, includeUntracked
From: Phillip Wood @ 2024-02-20 11:01 UTC (permalink / raw)
To: Ricardo C, phillip.wood, git
In-Reply-To: <df020dcc-afe8-4dd8-b476-4ca032206214@gmail.com>
Hi Ricardo
On 18/02/2024 17:54, Ricardo C wrote:
> Hi Phillip,
>
> On 2/18/24 05:32, Phillip Wood wrote:
>> How does "stash.keepIndex" interact with "git rebase --autostash" and
>> "git merge --autostash"? I think both those commands expect a clean
>> index after running "git stash". They could just override the config
>> setting but it might get a bit confusing if some commands respect the
>> config and others don't.
>
> Both `git rebase --autostash` and `git merge --autostash` seem to be
> hardcoded to clean the index, regardless of the configuration or CLI
> flags. They do not use regular `git stash` to do so, but rather `git
> stash create`. This is unaffected by my changes, since it follows a
> different code path and does not accept `--keep-index` nor
> `--include-untracked`.
Ah, I'd forgotten that we hand "git stash create" as well as "git stash
push". It's good that these changes do not affect "git stash create" but
I think Junio is right to be concerned about the effect on tools that
are using "git stash push" where they arguably ought to be using "git
stash create".
> I'll add some tests for `git rebase --autostash`
> and `git merge --autostash`, just in case this behavior is changed in
> the future and causes breakage.
That's a good idea but I think it would be better to test that "git
stash create" is not affected by the config as we don't want to change
the behavior of its behavior.
>> I've only given the patch a very quick scan, but it looked sensible.
>> The only thing that jumped out at me was that quite a few tests seem
>> to do
>>
>> git init repo &&
>> (
>> cd repo &&
>> # test things
>> ) &&
>>
>> Our normal practice is to run all the tests in the same file in the
>> same repository rather than setting up a new one each time.
>
> I was doing this because it makes comparing different commands easier,
> but looking through other tests again, it seems like I should be
> comparing the outputs to hardcoded files anyway.
Yes, that is our usual practice.
Best Wishes
Phillip
^ permalink raw reply
* Re: [PATCH] Revert "Declare both git-switch and git-restore experimental"
From: Kristoffer Haugsbakk @ 2024-02-20 11:36 UTC (permalink / raw)
To: Matthieu Baerts (NGI0)
Cc: Nguyễn Thái Ngọc Duy, Junio C Hamano, git
In-Reply-To: <95eb92cb-7954-41c0-b542-5169ed5f9892@kernel.org>
On Tue, Feb 20, 2024, at 10:58, Matthieu Baerts wrote:
> Hi Kristoffer,
>
> Thank you for your comment.
>
> On 20/02/2024 10:36, Kristoffer Haugsbakk wrote:
>> On Tue, Feb 20, 2024, at 10:29, Matthieu Baerts (NGI0) wrote:
>>> This reverts commit 4e43b7ff1ea4b6f16b93a432b6718e9ab38749bd.
>>> Version 2.44 is approaching, almost 5 years after the introduction of
>>> these two commands, it then looks safe to remove this experimental
>>> status.
>>
>> Is this only based on the amount of time passed? Has there been any
>> relevant discussions on the mailing list that discuss how mature these
>> commands are and if they should be changed (with presumably a “no” to
>> the question about being changed)?
>
> It is only based on the amount of time passed, indeed.
>
> I initially wanted to start a discussion on the mailing list: "is it
> normal these commands are still marked as experimental?". Then I saw the
> patch introducing this status, which was suggesting doing a revert in
> version 2.24 or 2.25. That's why I sent this, to start the discussions
> with a patch that is ready to apply. Is it not OK to do that here?
>
> Also, when I quickly looked at the history, I didn't see any behaviour
> changes since their introduction. Maybe there was a minor change with
> commit 088018e34d ("restore: default to HEAD when combining --staged and
> --worktree"), but it looks more like a fix than a behaviour change.
All good reasons.
The only reason why I ask is because I was vaguely aware of some
discussions (don’t know how long ago) where someone was skeptical about
changing one of the two experimental commands, and then someone else in
turn expressed some frustration about this concern since they are after
all marked experimental. And the context was some UI/UX problems with
the command.
But we’ll see.
--
Kristoffer Haugsbakk
^ permalink raw reply
* Re: [PATCH v2 1/5] log: Move show_blob_object() to log.c
From: Maarten Bosmans @ 2024-02-20 11:40 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git
In-Reply-To: <20240220015928.GB2713741@coredump.intra.peff.net>
Op di 20 feb 2024 om 02:59 schreef Jeff King <peff@peff.net>:
> All that said, I'm not sure based on our previous discussion why we
> can't just call stream_blob_to_fd(). Looking at show_blob_object(), most
> of the logic is about recording the tree-context of the given name and
> using it for textconv. But since we know we are feeding a bare oid,
> that would never kick in. So I don't know if there's any value in
> sharing this function more widely in the first place.
Indeed. My original analysis of what `git show` does when invoked by
`git notes show` led to the conclusion that the only thing worthwhile
to keep is the `setup_pager()` call. Thanks for confirming this.
I'll reroll in a couple of days with the V1 approach back.
Maarten
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox