On Tue, Mar 01 2022, Taylor Blau wrote:

I realize that you're just copying an existing pattern within
builtin/reflog.c here, but we should really call this "--no-progress",
not "-v" or "--verbose".

I did that in the last 3 patches here, which I was waiting on
re-submitting:
https://lore.kernel.org/git/cover-00.12-00000000000-20211130T213319Z-avarab@gmail.com/ (local)

I.e. the whole reason we use --verbose here is entirely historical, it
used to (well, still is without those patches) very verbose line-by-line
output.

Whereas for other things we turn progress on by default, and --verbose
is something very different.

So please have this by "int progress = 1", and have a "PARSE_OPT_NONEG"
"no-progress" option instead, there's no reason we need to propagate the
existing UX mistake in "reflog expire".

[I reversed the order you wrote the following, due to the obvious
digression...] :)

When renaming a remote, Git needs to rename all remote tracking
references to the remote's new name (e.g., renaming
"refs/remotes/old/foo" to "refs/remotes/new/foo" when renaming a remote
from "old" to "new").

This can be somewhat slow when there are many references to rename,
since each rename is done in a separate call to rename_ref() as opposed
to grouping all renames together into the same transaction. It would be
nice to execute all renames as a single transaction, but there is a
snag: the reference transaction backend doesn't support renames during a
transaction (only individually, via rename_ref()).

The reasons there are described in more detail in [1], but the main
problem is that in order to preserve the existing reflog, it must be
moved while holding both locks (i.e., on "oldname" and "newname"), and
the ref transaction code doesn't support inserting arbitrary actions
into the middle of a transaction like that.

As an aside, adding support for this to the ref transaction code is
less straightforward than inserting both a ref_update() and ref_delete()
call into the same transaction. rename_ref()'s special handling to
detect D/F conflicts would need to be rewritten for the transaction code
if we wanted to proactively catch D/F conflicts when renaming a
reference during a transaction. The reftable backend could support this
much more readily because of its lack of D/F conflicts.

As an aside I think the reftable code "emulates" the D/F conflicts of
the files backend, but I'm not sure (this is from vague memory).

And I've run into the same limitations you're describing here with the
ref transactions code. I tried to transaction-ize the "branch rename"
the other day, there we copy an existing reflog in its entirety.

It would be really nice if you could plug things into the
transaction. It would have to be things that could support the same
do-stuff/abort-stuff semantics, e.g. being able to create a file with
arbitrary contents, then unlink() it if we're rolling back.

But in any case, all of that is a much larger change than is really
required here, so I (also) digress :)

As an even further aside: If we had a slight "twist" on the ref backend
where we were able to store the reflogs next to the actual ref files,
e.g.:

    .git/refs/heads/master
    .git/refs/heads/.master.reflog

Which would work since that '.master.reflog' name isn't a valid refname
(it contains "."), couldn't we safely do this whole operation on a POSIX
FS with a simple:

    mv .git/refs/remotes/origin .git/refs/remotes/newname

I.e. even if you had concurrent *.lock files in play from other
transactions having those would mean that we had a file descriptor open
to file in question, and a file being renamed underneath you doesn't
change your ability to write to that open file.

Although I guess we rename() them into place, and if that target
moves..., so we'd need to pre-open them.

Hrm.

Anyway, that's an even larger digression & can of worms, but something I
wondered about when I saw this take a LOOOONG time in the past, "why
doesn't it just rename the folders?".