On 13/05/2020 11:06, Phillip Wood wrote:

On 12/05/2020 17:48, Han-Wen Nienhuys wrote:

[...]

quoted

quoted

quoted

-struct ref_storage_be refs_be_files = {
-     NULL,
-     "files",
+struct ref_storage_be refs_be_files = { NULL,
+                                     "files",
+                                     files_ref_store_create,

quoted

The formatting has gone haywire

This was clang-format's automatic reformatting. I've looked a bit
closer, and it was reformatting because the initializer list was
missing the ',' on the final entry. I've added that now.

It sounds like we need to look at the clang format rules we use, I think
the original code is correctly formatted in this case (we've only
allowed trailing commas relatively recently), it shouldn't need changing
(which clutters up the patch with unrelated changes) just to satisfy
clang-format.

I spent some time trying to fix the clang-format rules this afternoon
and got nowhere. Searching the web revealed plenty of questions but no
useful answers beyond "add a trailing comma" and it's not even clear to
me if that works all time. It is a shame clang-format does not seem to
support a fairly common way of formatting structure initializers. That
the formatting changes so much based on the presence or absence of the
trailing comma is really confusing and unhelpful.

Best Wishes

Phillip

quoted

quoted

quoted

+     NULL, NULL,

Should the wrappers above that invoke these virtual functions check they
are non-null before dereferencing them? It would be better to die with
BUG() than segfault.

Done.

Great, I did wonder after I had sent the email if it would be better to
implement the BUG() in the virtual functions in the packed-refs backend
to avoid the check overhead in the other backends but it's probably not
worth worrying about.

quoted

quoted

I think this patch basically works but I'm concerned by the potential
NULL pointer dereference. While it's unfair to judge a patch by it's
formatting the changes to the formatting of existing code and the
dropped assertion rightly or wrongly gave me the impression lack of
attention which does make me concerned that there are other more serious
unintentional changes in the rest of the series.

I prefer leaving formatting up to automated tooling. They're better at
following mechanical rules precisely.

Right but in this case the changes completely obscured the important
changes. What really raised a flag for we was that two definitions of
the same structure where reformatted in completely different ways - I do
use clang format but I also sanity check the results before submitting a
patch.

I was looking at our use of git_path_cherry_pick_head() in sequencer.c
(I mostly work on rebase) the other day, there are quite a few places we
rely on CHERRY_PICK_HEAD/REVERT being a file, I suspect we probably do
the same in wt-status.c and builtin/commit.c. The uses are generally
    file_exists(git_path_cherry_pick_head())
which I think we could just change to use get_oid()
We also unlink() it in several places which we could replace with
delete_ref() but we blindly call unlink() in some places so the ref
might not exist.

In the sequencer we always use the refs api when handling REBASE_HEAD, I
haven't got round to checking builtin/rebase.c and builtin/am.c for that
yet.

Is there a plan to handle FETCH_HEAD with reftable? git rev-parse will
parse the first oid in the file which is handy if you've only fetched a
single ref, but the file itself contains several lines like

2407200be2a37bfca57ea1a9474318822fec49b0		branch 'git-alias' of
ssh://pi/home/pi/git

It might be special cased in the refs code already I haven't checked

MERGE_HEAD also contains multiple lines for octopus merges but I'm not
sure if people really need to run rev-parse on that.

Apologies if the FETCH_HEAD and MERGE_HEAD points have already been
covered elsewhere, this thread has got quite long.

Best Wishes

Phillip