Re: [PATCH] daemon, path.c: fix a bug with ~ in repo paths

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 21, 2016, 15:58 UTC
Message-ID: <xmqqlgxhd77u.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CACsJy8AzWqJ_9JQ_BGnAB3E3GrGSTS40z8umbtO6mQJukWOooQ@mail.gmail.com>

Duy Nguyen <pclouds@gmail.com> writes:

Show 5 quoted lines

> The amount of changes is unbelievable for fixing such a rare case
> though. I wonder if we can just detect this in daemon.c and pass
> "./~foo/bar" instead of "~foo/bar" to enter_repo() in non-strict mode
> to "disable" expand_user_path(). If it works, it's much simpler
> changes (even though a bit hacky)

Conceptually, it ought to be updating the code that says "Does it begin with a tilde? Then try to do user-path expansion and fail if that is not enabled and if it succeeds use the resulting directory" to "Is user-path enabled and does it begin with a tilde? Then try to do user-path expansion and if it succeeds use the resulting directory". Compared to that mental model we have with this codepath, I agree that the change does look quite invasive and large.

It is OK for a change to be large, as long as the end result is easier to read and understand than the original. I am undecided if that is the case with this patch, though.

Also I am a bit worried about the change in the semantics. While it is not the best way to achieve this, the server operators could use it as a way to add directories whose contents need to be hidden to give them names that begin with a tilde without enabling user-path expansion. This change may be a new and useful feature for Luke, but to these server operators this change can be a new bug---it is probably a minor new bug as they can work it around by using other means to hide these directories, though.