Subject: Re: [PATCH v2 0/4] cat-file: optionally convert to worktree version

From: Junio C Hamano <[email-masked]>
Date: Wed, 24 Aug 2016 10:02:39 -0700

Jeff King <[email-masked]> writes:

> On Wed, Aug 24, 2016 at 09:09:06AM -0700, Junio C Hamano wrote:
>
>> >  +	if (!path)
>> >  +		path = obj_context.path;
>> >  +	else if (obj_context.mode == S_IFINVALID)
>> >  +		obj_context.mode = 0100644;
>> >  +
>> >   	buf = NULL;
>> >   	switch (opt) {
>> >   	case 't':
>> 
>> The above two hunks make all the difference in the ease of reading
>> the remainder of the function.  Very good.
>
> Yeah, I agree. Though it took me a moment to figure out why we were
> setting obj_context.mode but not obj_context.path; the reason is that
> "mode" is convenient to use as local storage, but "path" is not, because
> it is not a pointer but an array.

Wait a minute.  Why is it a cascaded if/elseif, not two independent
if statements that gives a default value?  In other words, wouldn't
these two independent and orthogonal decisions?

 * When forced to use some path, we ignore obj_context.path

 * Whether we are forced to use a path or not, if we do not know the
   mode from the lookup context, we want to use the regular blob
   mode.

So that part of the patch is wrong after all, I would have to say.

	if (!path)
        	path = obj_context.path;
	if (obj_context.mode == S_IFINVALID)
        	obj_context.mode = 0100644;

or something like that, perhaps.

> So it would have been a little clearer to me as:
>
>   const char *path;
>   unsigned mode;
>   ...
>   if (!force_path) {
> 	/* use file info from sha1 lookup */
> 	path = obj_context.path;
> 	mode = obj_context.mode;
>   } else {
> 	/* use path requested by user, and assume it is a regular file */
> 	path = force_path;
> 	mode = 0100644;
>   }

Hmph, if you read it that way, then if/elseif makes some sense, but
we need to assume that the obj_context.mode can be garbage and have
a fallback for it.

Just like

	git cat-file --filters --path=git.c HEAD:t

would error out because HEAD:t is not even a blob, I would expect

	git cat-file --filters --path=git.c :RelNotes

to error out, because the object itself _is_ known to be a
blob that is not a regular file.

And that kind of type checking will not be possible with "if the
user gave us a path, assume it is a regular file".