Re: [PATCH] check-ref-format: require a repository for --branch

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 17, 2017, 02:40 UTC
Message-ID: <xmqq376iqyy6.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20171017012718.GC19335@alpha.vpn.ikke.info>

Kevin Daudt <me@ikke.info> writes:

Show 11 quoted lines

>> +	setup_git_directory_gently(&nongit);
>> +
>> +	if (!nongit)
>> +		malformed = (strbuf_check_branch_ref(&sb, arg) ||
>> +			     !skip_prefix(sb.buf, "refs/heads/", &name));
>> +	else
>> +		malformed = check_branch_ref_format(arg);
>> +
>
> Would it make sense to swap the logic and get rid of the double
> negative (!nongit)?

I am trying to follow the pattern "handle the normal case that have been supported forever first, and then handle new exception next", so that it is easier to see that there is no behaviour change in the normal case, so I do not think it makes it easier to see to swap the if/else cases.

Show 9 quoted lines

>
>> +	if (malformed)
>>  		die("'%s' is not a valid branch name", arg);
>> -	printf("%s\n", sb.buf + 11);
>> +	printf("%s\n", name);
>> +	strbuf_release(&sb);
>>  	return 0;
>>  }
>>