Junio C Hamano <gitster@xxxxxxxxx> writes： > > Lidong Yan <yldhome2d2@xxxxxxxxx> 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". Understood. I initially thought that NEEDSWORK indicated work that was guaranteed to be implemented later. > >> 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). I’d like to revert the comment to original shape and split it into separate commit. The reason I changed this comment was simply because I found it a bit confusing when reading through the code. > >> 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. I would just discard the commit about NEEDSWORK. Thanks for your review, Lidong
