Jeff King <peff@xxxxxxxx> 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".
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html