All of lore.kernel.org
 help / color / mirror / Atom feed
From: Ferry Huberts <mailings@hupie.com>
To: Lars Hjemli <hjemli@gmail.com>
Cc: git@vger.kernel.org
Subject: Re: [CGit] [PATCH 0/6] Communicate the repo name to the filter scripts
Date: Sat, 19 Feb 2011 14:54:51 +0100	[thread overview]
Message-ID: <4D5FCBAB.6060009@hupie.com> (raw)
In-Reply-To: <AANLkTimASPjeHor3R6c=i1xpjftxrg4NnFaLJfReuZ-X@mail.gmail.com>

On 02/19/2011 09:46 AM, Lars Hjemli wrote:
> On Thu, Feb 17, 2011 at 22:11, Ferry Huberts <mailings@hupie.com> wrote:
>> This patch series fixes two bugs and communicates the repo name
>> to the filter scripts.
> 
> Thanks, but I think the current filter invocations with unnamed,
> positional command arguments was a mistake. We should probably fix it
> instead of extending it, taking care not to break backwards
> compatibility.
> 
> The easiest fix would be to add some environment variables:
> * GIT_DIR
> * CGIT_REPO_NAME
> * CGIT_REPO_REL_URL
> * CGIT_REPO_ABS_URL
> * CGIT_BRANCH_NAME
> * CGIT_COMMIT_ID
> * CGIT_PATH_FILTER
> 
> What do you think?
> 
> --
> larsh

I've been thinking about this too and did think about this solution but
also about a solution in which these settings are written to a temporary
file. I have no preference as long as the repo settings are communicated
to the filter script. Writing to a file is probably bad for performance
though.

The patches I sent just plainly solve my own problem and I was hoping a
bit for this discussion :-)

Your proposal sounds reasonable and a good approach.

I'm a bit worried about the GIT_DIR env var, about it conflicting with
the real git env var. It's probably best to rename that one to CGIT_GIT_DIR.

Also, we'd have to be sure that when we set these env vars that they're
only propagated to the filter script we're going to run and not to other
envs: running filter scripts in parallel must not be prevented.


Have you looked at the first 2 patches?
These solve bugs and can be applied regardless of this discussion.


please let me know what you think.


grtz
-- 
Ferry Huberts

  reply	other threads:[~2011-02-19 13:55 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-02-17 21:11 [CGit] [PATCH 0/6] Communicate the repo name to the filter scripts Ferry Huberts
2011-02-17 21:11 ` [CGit] [PATCH 1/6] source_filter: fix a memory leak Ferry Huberts
2011-02-17 21:11 ` [CGit] [PATCH 2/6] new_filter: correctly initialise all arguments for a new filter Ferry Huberts
2011-02-17 21:11 ` [CGit] [PATCH 3/6] new_filter: determine extra_args from filter type Ferry Huberts
2011-02-17 21:11 ` [CGit] [PATCH 4/6] source_filter: also communicate the repo name to the filter Ferry Huberts
2011-02-17 21:11 ` [CGit] [PATCH 5/6] commit_filter: " Ferry Huberts
2011-02-17 21:11 ` [CGit] [PATCH 6/6] about_filter: " Ferry Huberts
2011-02-17 21:23 ` [CGit] [PATCH 0/6] Communicate the repo name to the filter scripts Ferry Huberts
2011-02-19  8:46 ` Lars Hjemli
2011-02-19 13:54   ` Ferry Huberts [this message]
2011-02-24 13:32   ` Ferry Huberts

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4D5FCBAB.6060009@hupie.com \
    --to=mailings@hupie.com \
    --cc=git@vger.kernel.org \
    --cc=hjemli@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.