On 13/05/2020 11:06, Phillip Wood wrote:
> On 12/05/2020 17:48, Han-Wen Nienhuys wrote:
[...]
>>>> -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.
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
>
>>>> + 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
>
