From: Maciej Pasternacki <maciej@pasternacki.net>
Date: Sun, 19 Oct 2008 13:47:04 GMT
Subject: Re: [PATCH] -C/--chdir command line option
Message-ID: <86685067-021C-4DC5-A462-AA6834208BDE@pasternacki.net>
In-Reply-To: <20081019131745.GA8643@coredump.intra.peff.net>


On 2008-10-19, at 15:17, Jeff King wrote:

> On Sat, Oct 18, 2008 at 05:02:27PM -0700, Maciej Pasternacki wrote:
>
>> In my project cl-librarian, which is a layer built upon different
>> version control systems, I need to run git pull, but start it from
>> outside of work tree; however, pull needs to be in work tree (as in
>> getcwd()) in order to update said work tree.  --git-dir and
>> --work-tree options don't work; I discussed it on #git yesterday,
>> and it turned out that this issue is nontrivial:
>> [...]
>> the best workaround that occured to me was adding -C/--chdir option  
>> to
>> main git binary, like one that Make accepts.  This fixed my  
>> problem, I
>> paste the resulting patch below:
>
> I'm not completely opposed to -C, as it does match some other tools,
> but it does seem superfluous with --git-dir and --work-tree. Which  
> makes
> me feel like we are just papering over a bug instead of actually  
> fixing
> it (and to be honest, I always wondered what the point of "make -C"  
> was
> in the face of "cd dir && make").

The point of make -C is exactly my usage pattern: invoking make from  
external program.  I have seen, at least in Lisp (external-program)  
and Python (os.spawn, subprocess), libraries to safely and portably  
execute external program by giving literal list of arguments (an easy  
to use wrapper to fork()+exec(); I don't know if such feature exists  
for C).  This allows me not to worry about quoting strange characters  
in pathnames (think injections, if just making it possible to use "&"  
character in directory name is not good enough a reason).  The  
equivalent of "cd dir && make" would be to use system() or invoke sh - 
c; both would require shell-quoting the directory pathname, and would  
carry portability issues -- how do I know that shell on target system  
is actually called "sh" and it supports "&&"?  At least Windows  
systems would be problematic.

The -C option allows me to just specify the directory as it is, and  
child process, after the fork, will take care of the chdir().  No  
quoting, no portability problems, injection-safety included.

As for -C being superfluous: --git-dir and --work-tree seem to support  
weird usage patterns (like work tree separate from git-dir), but it  
seems to me that --work-tree could be just made a synonym of -C, and  
it would behave as expected without creating chicken-and-egg problem  
that doener on #git found (which I don't really understand, but I also  
can't see what --work-tree allows that -C would not -- except creating  
more fragility).  I won't push on --work-tree and -C being  
functionally synonymous, since I don't know git well enough, it's just  
my impression at the moment.

> Please follow the usual list conventions; this stuff should be the
> actual headers of your mail, not in the body of the message. And  
> some of
> the justification you gave above should be part of the commit message.

ACK.  That was how I understood instructions linked from git.or.cz,  
I'll paste message correctly for next version of the patch.

>> +		} else if (!strcmp(cmd, "-C") || !strcmp(cmd, "--chdir")) {
>> +                        chdir((*argv)[1]);
>
> No error detection and reporting? I think I would be quite happy for
> "git -C /some/path clean -dx" to bail out when it saw that it couldn't
> change directory instead of just deleting the contents of wherever I
> happened to be.

Oops.  Looks like I'm spoiled by high-level languages that would  
automatically catch and display an error.  I'll prepare patch v2 today.

> Also, the envchanged flag should probably be set, as for the git-dir  
> and
> work-tree options.

OK.  I thought it means literally environment change, as in setenv().

Thank you for your comments, off to make v2 patch,
Maciej.

-- 
-><- Maciej Pasternacki -><- http://www.pasternacki.net/ -><-