Re: [PATCH v2/GSoC 2/4] path.c: implement xdg_runtime_dir()

From: Jeff King <peff@peff.net>
Date: Mar 17, 2016, 18:13 UTC
Message-ID: <20160317181311.GB23669@sigill.intra.peff.net>
In-Reply-To: <1458233326-7735-2-git-send-email-huiyiqun@gmail.com>

On Fri, Mar 18, 2016 at 12:48:44AM +0800, Hui Yiqun wrote:

Show 17 quoted lines

> this function does the following:
> 
> 1. if $XDG_RUNTIME_DIR is non-empty, `$XDG_RUNTIME_DIR/git` is used in next
> step, otherwise `/tmp/git-$uid` is taken.
> 2. ensure that above directory does exist. what's more, it must has correct
> permission and ownership.
> 3. a newly allocated string consisting of the path of above directory and
> $filename is returned.
> 
> Under following situation, NULL will be returned:
> + the directory mentioned in step 1 exists but have wrong permission or
> ownership.
> + the directory or its parent cannot be created.
> 
> Notice:
> 
> + the caller is responsible for deallocating the returned string.

I see a lot of "what" in your commit message (and in the other ones in this series), but not a lot of "why".

We can see the "what" from the diff already (though it is certainly OK to point out tricky parts). But you probably want to explain the motivation for things, alternatives considered, etc, like:

  - why is using $XDG_RUNTIME_DIR a good thing?

  - why did you choose to fall back to /tmp (as opposed to, say,
    returning NULL and letting the caller handle it)

  - what is the purpose of the ownership/permission rules? You link to
    the XDG spec in an in-code comment, but IMHO that kind of motivation
    probably makes more sense in the commit message.

-Peff