Hi Junio
On 2019-06-13 17:56 UTC Junio C Hamano <[email protected]> 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
