Re: [GSoC][PATCH v3 2/3] cherry-pick/revert: add --skip option

[Rohit Ashiwal <rohit.ashiwal265@gmail.com>]

Hi Junio

On 2019-06-13 17:56 UTC Junio C Hamano <gitster@pobox.com> wrote:
> 
> > +'git cherry-pick' --continue | --skip | --abort | --quit
> 
> Is this correct, or do we need to enclose these choices inside (),
> i.e.
> 
> 	'git cherry-pick' ( --continue | --skip | --abort | --quit )
> 
> ?

Documentation of `git rebase` also lists these options without the
'('s so, I thought to make it similar to that.
 
> It is unclear *why* the function (and more importantly, its callers)
> would want to omit two sanity checks when is_skip is in effect.
> 
> Before this patch introduced such conditional behaviour, the name
> was descriptive enough for this single-purpose function that is a
> file-local helper, but it is no longer a case.  The function needs a
> bit of commentary before it.
> 
> When &&-chaining error checks that are optional, check the condition
> that makes the error checks optional first, i.e.
> 
> 	if (!is_skip &&
> 		!file_exists(...) && !file_exists(...))
> 		return error(...);
> 
> [...]
> 
> no, do not merely give answer to me in your response---write the
> answer as in-code comment to help future readers of the code).
> 
> "Because when we come here, sometimes the XXX_HEAD must exist but
> some other times XXX_HEAD may not exist, so insisting that either
> exists would make the function fail" is *NOT* a good answer, on the
> other hand.  Somebody must still check that the necessary file
> exists when it must exist.

Yes, I should have added some comments. I'll add them in next revision.

Thanks
Rohit