Re: [PATCH] fetch: allow running as different users in shared repositories
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Mar 25, 2020, 19:04 UTC
- Message-ID
- <nycvar.QRO.7.76.6.2003252001560.46@tvgsbejvaqbjf.bet>
- In-Reply-To
- <20200319010321.18614-1-vz-git@zeitlins.org>
Hi Vadim,
On Thu, 19 Mar 2020, Vadim Zeitlin wrote:
Show 7 quoted lines
> The function fopen_for_writing(), which was added in 79d7582e32 (commit: > allow editing the commit message even in shared repos, 2016-01-06) and > used for overwriting FETCH_HEAD since ea56518dfe (Handle more file > writes correctly in shared repos, 2016-01-11), didn't do it correctly in > shared repositories under Linux. > > This happened because in this situation the file FETCH_HEAD has mode 644
I wonder why that is. In a shared repository, it should have mode 664, I thought.
Show 20 quoted lines
> and attempting to overwrite it when running git-fetch under an account > different from the one that was had originally created it, failed with > EACCES, and not EPERM. However fopen_for_writing() only checked for the > latter, and not the former, so it didn't even try removing the existing > file and recreating it, as it was supposed to do. > > Fix this by checking for either EACCES or EPERM. The latter doesn't seem > to be ever returned in a typical situation by open(2) under Linux, but > keep checking for it as it is presumably returned under some other > platform, although it's not really clear where does this happen. > > Signed-off-by: Vadim Zeitlin <vz-git@zeitlins.org> > --- > I couldn't find any system that would return EPERM for a "normal" > permissions denied error, so maybe it's not worth checking for it, but I > wanted to minimize the number of changes to the existing behaviour. At the > very least, testing for EACCES is definitely necessary under Linux, where > openat(2) returns it, and not EPERM, in the situation described above, i.e. > non-writable file (even if it's in a writable directory, allowing to unlink > it without problems).
That rationale makes sense to me, as does the patch.
Thanks, Johannes
Show 26 quoted lines
> ---
> wrapper.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/wrapper.c b/wrapper.c
> index e1eaef2e16..f5607241da 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -373,11 +373,12 @@ FILE *fopen_for_writing(const char *path)
> {
> FILE *ret = fopen(path, "w");
>
> - if (!ret && errno == EPERM) {
> + if (!ret && (errno == EACCES || errno == EPERM)) {
> + int open_error = errno;
> if (!unlink(path))
> ret = fopen(path, "w");
> else
> - errno = EPERM;
> + errno = open_error;
> }
> return ret;
> }
> --
> 2.26.0.rc2
>