Re: [RFC PATCH v3 0/2] small fixes for git.c and setup.c

[Junio C Hamano <gitster@pobox.com>]

Lidong Yan <yldhome2d2@gmail.com> writes:

> I've been reading through the git code from the beginning. This
> patch series fixes some NEEDSWORKs and cleans up some unnecessary
> uses of the_repository that I came across.

FYI, when we have "NEEDSWORK: do X", the intention is often "we
haven't spent enough brain cycles when we wrote this comment, so the
first step is to evaluate if doing X is a sensible thing in the
first place, and only if that is the case, do X".

> The first commit replace the use of the_repository to run_builtin()'s
> argument repo. Since each caller pass the_repository to run_builtin(),
> this replacement is safe.

That change is safe.  I'd rather see the comment left intact or
reverted to the original shape to clarify what code the comment
applies to (see the other message).

> The second commit takes care of a NEEDSWORK in setup_git_directory_gently()
> we now properly error out if we hit a .git that is not a file or directory
> when looking for the .git.

We used to just ignore and keep going to check the parent directory,
right?  Now we would error out when .git is a FIFO or device or any
other random things.  Is a bit of behaviour change, but I am not
sure if it is worth doing.  As finding these weird non-file things
in your working tree and naming them ".git" is extremely rare and
useless (from Git's point of view), I suspect that the user is
deliberately doing so for whatever reason they have, so it smells
like this change has very little chance to detect a real problem
with a larger chance to break a set-up that was deliberately done by
the end-user.  I dunno.

Thanks.