On 12/05/2020 17:48, Han-Wen Nienhuys wrote:
> On Tue, May 12, 2020 at 12:22 PM Phillip Wood <phillip.wood123@xxxxxxxxx> wrote:
>>> if (ref_type(refname) == REF_TYPE_PSEUDOREF) {
>>> - assert(refs == get_main_ref_store(the_repository));
>>
>> I don't think you meant to delete this line, the equivalent line is left
>> untouched in the next hunk and there are comments in refs.h saying this
>> should only me called on the main repository
>
> yep. - Whoops. Fixed.
>
>>> -struct ref_storage_be refs_be_files = {
>>> - NULL,
>>> - "files",
>>> +struct ref_storage_be refs_be_files = { NULL,
>>> + "files",
>>> + files_ref_store_create,
>
>> 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.
>>> + 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.
>
>> 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
