From: Andrzej Hunt <andrzej@ahunt.org>
Date: Sun, 14 Mar 2021 17:56:51 GMT
Subject: Re: [PATCH 4/7] worktree: fix leak in dwim_branch()
Message-ID: <b328381d-2e38-af02-e84d-cfb8174103ee@ahunt.org>
In-Reply-To: <YEZ4BQTeT/vdX+tK@coredump.intra.peff.net>

On 08/03/2021 20:16, Jeff King wrote:
> On Mon, Mar 08, 2021 at 06:36:17PM +0000, Andrzej Hunt via GitGitGadget wrote:
> 
>> Make sure that we release the temporary strbuf during dwim_branch() for
>> all codepaths (and not just for the early return).
> 
> Makes sense. Two style nits:

Thank, I'll fix both.
> 
>> -	if (!strbuf_check_branch_ref(&ref, branchname) &&
>> -	    ref_exists(ref.buf)) {
>> -		strbuf_release(&ref);
>> +
>> +	branch_exists = (!strbuf_check_branch_ref(&ref, branchname) &&
>> +			 ref_exists(ref.buf));
> 
> We'd usually omit the extra parentheses here. I.e.,:
> 
>    branch_exists = !strbuf_check_branch_ref(&ref, branchname) &&
>                    ref_exists(ref.buf);

I've made this change - but I have a question about the formatting:
- In the few examples that I could find, the second line is aligned 
using spaces (i.e. same number of tabs as the previous line, followed by 
spaces to align correctly). However that appears to violate the 
indent-with-non-tab style check - should I switch to 100% tabs instead?

That check has been around since:

e2f6331a14 (.gitattributes: CR at the end of the line is an error, 
2009-06-19)

So maybe it's being intentionally ignored in the cases that I've seen (I 
only noticed it for my series because of the automated checks on Github) 
- but I thought I should ask to sure.