Re: [PATCH v4 1/3] branch: introduce dont_fail parameter for branchname validation

[Kaartic Sivaraam <kaartic.sivaraam@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2815 bytes --]

On Friday 16 March 2018 01:57 AM, Junio C Hamano wrote:
> Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes:
> 
> So... I am not finding dont_fail that was mentioned on the title
> anywhere else in the patch.  Such lack of attention to detail is
> a bit off-putting.
> 

I'm absolutely sorry x-<

I should have forgotten to update the commit subject during one of the
old rebases in which I renamed the parameter from 'dont_fail' to 'gently'.

I shouldn't have assumed that the commit messages of the old series held
in the reboot too. I should have re-read them "completely" and double
checked it. :-(


>> +enum branch_validation_result {
>> +	/* Flags that convey there are fatal errors */
>> +	VALIDATION_FATAL_BRANCH_EXISTS_NO_FORCE = -3,
>> +	VALIDATION_FATAL_CANNOT_FORCE_UPDATE_CURRENT_BRANCH,
>> +	VALIDATION_FATAL_INVALID_BRANCH_NAME,
>> +	/* Flags that convey there are no fatal errors */
>> +	VALIDATION_PASS_BRANCH_DOESNT_EXIST = 0,
>> +	VALIDATION_PASS_BRANCH_EXISTS = 1,
>> +	VALIDATION_WARN_BRANCH_EXISTS = 2
>> +};
> 
> where adding new error types will force us to touch _two_ lines
> (i.e. either you add a new error before NO_FORCE with value -4 and
> then remove the "= -3" from NO_FORCE, or you add a new error after
> INVALID, and update NO_FORCE to -4), which can easily be screwed up
> by a careless developer.  The current code is not wrong per-se, but
> I wonder if it can be made less error prone.
> 

At the top of my head I could think of 2 ways to get around this,

	- Assigning the actual value to every single entry in the enum.

	  This should solve the issue as a any new entry that would be
	  added is expected to go "with a value". The compiler would
	  warn you in the case of duplicate values. The drawback is: it
	  might be a little restrictive and a little ugly. It would also
	  likely cause maintenance issues if the number of values in the
	  enum get bigger.

	  (Of course this doesn't hold if, the careless programmer
	   shatters "consistency" and adds an entry without a value to
	   the enum and that change gets merged into the codebase ;-) )

	- Using non-negative values for both errors and non-errors.

	  This might make it hard to distinguish errors from non-errors
	  but this would avoid errors completely without much issues,
	  otherwise.

I might prefer the former as I find the possibility of the requirement
to distinguish the errors from non-errors to be high when compared with
the possibility of the requirement to add more new entries to the enum.

Any other ideas/suggestions ?

-- 

Kaartic

QUOTE:

“The most valuable person on any team is the person who makes everyone
else on the team more valuable, not the person who knows the most.”

      - Joel Spolsky


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 870 bytes --]