Git development
 help / color / mirror / Atom feed
* Re: [PATCH] -C/--chdir command line option
From: Maciej Pasternacki @ 2008-10-20  7:28 UTC (permalink / raw)
  To: git
In-Reply-To: <DDFCD680-C477-4BE5-AB71-3F26048E26D1@pasternacki.net>

On 2008-10-20, at 06:55, Junio C Hamano wrote:

>>> 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
>>
>> Hmm. Yeah, thinking about it more, -C is not really superfluous with
>> respect to those options. You don't want to say "here is the work- 
>> tree,
>> and here is the git-dir". You want to say "find the work-tree and
>> git-dir for me using the usual rules, as if I were in this  
>> directory."
>
> I think that interpretation of -C, if the option existed, makes  
> sense, but
> I do not understand why the tool that drives git refuses to chdir to  
> the
> repository for itself in the first place.

The tool (a) manages many repositories, and I don't want to chdir()  
back and forth, (b) should be able to manage those repositories not  
disturbing anything other.  It comes as a Common Lisp library, which  
will usually be called by end user, but can also be called  
programmatically, and I wouldn't want any library to change my cwd; it  
could chdir() back, of course, but this would still be a race  
condition if other threads were running at the same time.

And there is the Lisp-specific thing, that the language comes from  
before Unix domination, and things as simple as changing the process'  
cwd are actually nontrivial to do portably (there is equivalent of cwd  
inside Lisp world, which is used by Lisp-level file access routines,  
but chdir() is not in ANSI standard, is done differently by every  
implementation, and would require some kind of compatibility layer).

For me, -C would make life much easier, and I gave other arguments for  
it before.  It would suffice if --work-tree worked with "git pull", if  
-C doesn't pass, but it seems to me that -C has some merits.

Regards,
Maciej.

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

^ permalink raw reply

* Re: Archiving tags/branches?
From: Pete Harlan @ 2008-10-20  6:36 UTC (permalink / raw)
  To: Johan Herland; +Cc: git, SZEDER Gábor
In-Reply-To: <200810181532.59883.johan@herland.net>

Johan Herland wrote:
> BTW, the best way IMHO to archive old refs is to clone your repo (with all 
> tags/branches) to a backup disk, and then regularly push (git push --all && 
> git push --tags) your new tags/branches to this backup. You are now free to 
> delete these tags/branches from your work repo (they will not be deleted 
> from the backup unless you use "git push --mirror"). And if you ever need 
> to retrieve an old tag/branch, it's just a matter of pulling it from the 
> backup repo. Nice, clean, flexible, and requires no changes to git.
> 
> 
> Have fun! :)
> 
> ...Johan

Hi,

Thank you; that indeed seems to work and solves the problem of managing
refs/archived-tags manually.

Using a secondary repo solely to overcome a flat tag/branch namespace
feels hackish.  Perhaps git will benefit someday from work in this area,
but until I come up with a patch your suggestion should work fine.  Just
knowing I didn't overlook an existing feature helps a lot.

--Pete

^ permalink raw reply

* Re: [PATCH] -C/--chdir command line option
From: Alex Riesen @ 2008-10-20  6:26 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jeff King, Maciej Pasternacki, git
In-Reply-To: <7vhc774ydr.fsf@gitster.siamese.dyndns.org>

2008/10/20 Junio C Hamano <gitster@pobox.com>:
> Junio C Hamano <gitster@pobox.com> writes:
> I wouldn't however suggest allowing the syntax to set environment
> variables, like:
>
>        $ git GIT_AUTHOR_NAME="A U Thor" commit
>
> as this is something your shell lets you do easily, i.e:
>
>        $ GIT_AUTHOR_NAME="A U Thor" git commit
>

No, someplace else it doesn't (yes, windows again).
It is mostly hard to do there, because the system shell is so
primitive, and installation of something sane (sh or env) is
an additional (and hard to explain to windows zealots) hassle.

And even in something like Perl it is hard: you cannot
change the environment just for the child process, you
have to change your own, run the process and change
it back.

So, yes, I like your suggestion, but I'd like to _include_
setting all the Git's environment.

^ permalink raw reply

* Re: Archiving tags/branches?
From: Pete Harlan @ 2008-10-20  6:14 UTC (permalink / raw)
  To: David Symonds; +Cc: git
In-Reply-To: <ee77f5c20810171950j9ab85bfi6eddca167f86fda2@mail.gmail.com>

David Symonds wrote:
> On Sat, Oct 18, 2008 at 12:43 PM, Pete Harlan <pgit@pcharlan.com>
> wrote:
>> Hi,
>> 
>> I'm looking for a way to manage an ever-growing list of tags.  I've
>> read some git docs, but am new to git and wonder if the below
>> method doesn't work or if there's a standard practice I haven't run
>> into.
>> 
>> Most of the tags in my repo are uninteresting to look at, but can't
>> be deleted.  (Code releases for the most part, or stalled topic
>> branches.) If I wanted to archive those, it looks like this would
>> work:
> 
> Is it really true that they can't be deleted? The only reason to
> avoid it might be for preventing Git's GC from cleaning them up, but
> if all your branches/tags are reachable via "interesting"
> branches/tags then you could just slap the tag name and SHA1 in a
> text file somewhere.
> 
> 
> Dave.

Thank you for your response.  The tags aren't reachable; they're
dead-end branches.

Our development history looks like this:

o---o---o---o---o---o---o---o---o---o---o---o---o---o master
 \                   \                   \
  o---o---o r1.0      o---o---o r1.1      o---o---o r1.2

with releases branched off the development line and stabilized during
QA.  Fixes into the release branches are cherry-picked out of master,
with no merges.

With a new release every few weeks, the tags pile up.

(There are workflows, such as git.git's, where the release tags form one
long line of development, and when we start using git we may use a
different workflow, but the above was our svn workflow, for the
obvious reason that svn doesn't understand merges.  We're going to
import hundreds of such branches in the conversion to git; most such
names are noise, but we don't want to lose the history.)

I would think a built-in feature for archiving refs would be useful to
other projects, even when the tags/branches are reachable and therefore
one could manually stash them in a file.  Getting the design right is
tricky because there are a lot of different ways to approach it, but the
idea seems generally useful to me.

One direction would be to support directory commands for tags, using
refs/tags and refs/branches as the root directories of trees.  (This was
the solution in svn, which naturally supports a hierarchy of branches.)
 Another would be to have a regexp for hiding tags/branches with a
certain pattern (e.g., leading '.').  What I'll probably do in the short
term is write an alias that lists the most recent 10 tags and use that
most of the time.

--Pete

^ permalink raw reply

* Re: [PATCH v2] Fix testcase failure when extended attributes are in use
From: Junio C Hamano @ 2008-10-20  5:52 UTC (permalink / raw)
  To: Deskin Miller; +Cc: git, heikki.orsila
In-Reply-To: <20081019234723.GB2015@riemann.deskinm.fdns.net>

Deskin Miller <deskinm@umich.edu> writes:

> I hope I've made the case for the necessity of a patch (if nothing else, t1301
> fails 10/16 tests on my system without some sort of patch), and I'll happily
> agree that stripping any printable is a better choice.  Thanks for the
> feedback; FWIW, your suggested patch here is
>
> Tested-by: Deskin Miller <deskinm@umich.edu>

Thanks.  Applied to 'maint' and will appear in the next pushout (but not
tonight).

^ permalink raw reply

* Re: [PATCH] -C/--chdir command line option
From: Junio C Hamano @ 2008-10-20  5:41 UTC (permalink / raw)
  To: Jeff King; +Cc: Maciej Pasternacki, git
In-Reply-To: <7vr66b50gy.fsf@gitster.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> writes:

> The only excuse I remember seeing in the thread was that "make has '-C'
> option, so let's have it, because it is similar", which does not justfiy
> addition of that option to git at all to me.

Just after having said that "make has it, so let's have it ourselves, too"
is a non-excuse to any feature bloat, one thing I could accept a patch to
imitate what "make" does, especially if we are going to actually clean up
the startup sequence like we have discussed sometime ago to fix breakage
around --work-tree, is to have VAR=VAL (e.g. "make CFLAGS=-O2") on the
command line.  We could use that syntax to allow configuration variables
to be overridden, like so:

	$ git core.whitespace=cr-at-eol log -p master..next

I wouldn't however suggest allowing the syntax to set environment
variables, like:

	$ git GIT_AUTHOR_NAME="A U Thor" commit

as this is something your shell lets you do easily, i.e:

	$ GIT_AUTHOR_NAME="A U Thor" git commit

But overriding some configuration variables for a single command
invocation is not something you can do without actually editing the
configuration file for some variables, so the former would be justified.

^ permalink raw reply

* Re: Usability of git stash
From: Junio C Hamano @ 2008-10-20  5:29 UTC (permalink / raw)
  To: Jeff King
  Cc: Shawn O. Pearce, Anders Melchiorsen, Brandon Casey, David Kastrup,
	git
In-Reply-To: <20081020003644.GA10412@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

>   git checkout next ;# which is where I usually am anyway
>   hack hack hack
>   # oops, I have been building this directly on top of next and it
>   # really needs to be a feature-branch on maint
>   git stash
>   git checkout -b jk/maint-fix-whatever origin/maint
>   git stash apply
>
> The equivalent non-stash commands would be "commit -m wip" and
> "cherry-pick". But the stash saves me the trouble later of having to
> delete the wip cruft on top of next.

The equivalent would be:

	git checkout -m -b jk/maint-fix-whatever origin/maint

no need for stash, wip-commit, nor cherry-pick.

The advantage of using "stash then stash apply" (not "stash pop") or
"wip-commit with cherry-pick" is that you can reset, take a deep breath,
and redo it, when the resulting merge conflict gets too hairy.

> I disagree. I think the strength of stash is that it is divorced from
> the history. So it is more like a cherry-pick (or diff | apply, which is
> what it was intended to replace).

I agree with you; it really is the "diff saved somewhere, later applied".

^ permalink raw reply

* Re: Usability of git stash
From: Miles Bader @ 2008-10-20  5:23 UTC (permalink / raw)
  To: Anders Melchiorsen; +Cc: Brandon Casey, David Kastrup, git
In-Reply-To: <87prly5k5r.fsf@cup.kalibalik.dk>

Anders Melchiorsen <mail@cup.kalibalik.dk> writes:
> I wonder whether experienced users even use stash a lot. Personally,
> after getting my head around the DAG, and thus getting more
> comfortable with git reset, I tend to make "WIP" commits instead.

I certainly do, and my impression is that some of the biggest boosters
of stash have always been the git heavy hitters (it's certainly not one
of these "implemented due to nagging by newbies" features)...

-Miles

-- 
Liberty, n. One of imagination's most precious possessions.

^ permalink raw reply

* Re: [PATCH] -C/--chdir command line option
From: Junio C Hamano @ 2008-10-20  4:55 UTC (permalink / raw)
  To: Jeff King; +Cc: Maciej Pasternacki, git
In-Reply-To: <20081019141634.GA8997@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

> On Sun, Oct 19, 2008 at 03:47:04PM +0200, Maciej Pasternacki wrote:
>
>> 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 
>
> Hmm. Yeah, thinking about it more, -C is not really superfluous with
> respect to those options. You don't want to say "here is the work-tree,
> and here is the git-dir". You want to say "find the work-tree and
> git-dir for me using the usual rules, as if I were in this directory."

I think that interpretation of -C, if the option existed, makes sense, but
I do not understand why the tool that drives git refuses to chdir to the
repository for itself in the first place.

The only excuse I remember seeing in the thread was that "make has '-C'
option, so let's have it, because it is similar", which does not justfiy
addition of that option to git at all to me.

With "make", the -C option can be justified as a necessary tool to write a
recursive Makefile that can be sanely and easily processed without
actually executing any commands (iow, imaging implementing "make" that
allows you to write "cd there && $(MAKE)" in the toplevel Makefile and
tells the users what would happen in "there" when run as "make -n").

And even in "make" context, not all implementations have it.  I think it
is only GNU and fairly recent BSDs.

^ permalink raw reply

* Re: Is XDL_MERGE_ZEALOUS too zealous (or maybe not zealous enough)?
From: Matt McCutchen @ 2008-10-20  3:42 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Johannes Schindelin, git
In-Reply-To: <7v3ais5hb3.fsf_-_@gitster.siamese.dyndns.org>

On Sun, 2008-10-19 at 15:52 -0700, Junio C Hamano wrote:
> However, the usual simplified merge shows this (run "git checkout --merge
> builtin-checkout.c" if you have done the above):
> 
> <<<<<<< ours
> 	/* --track without -b should DWIM */
> 	if (0 < opts.track && !opts.new_branch) {
> 		const char *argv0 = argv[0];
> 	...
> 		opts.new_branch = argv0 + 1;
> 	}
> 
> 	if (opts.track == BRANCH_TRACK_UNSPECIFIED)
> 		opts.track = git_branch_track;
> =======
> 	if (conflict_style) {
> 		opts.merge = 1; /* implied */
> 		git_xmerge_config("merge.conflictstyle", conflict_style, NULL);
> 	}
> 
> 	if (!opts.new_branch && (opts.track != git_branch_track))
> 		die("git checkout: --track and --no-track require -b");
> >>>>>>> theirs
> 
> Removing the two lines from the simplified "theirs" is not what I would
> suggest (it would be actively wrong), but I wonder if we can do something
> clever to help users with a merge like this.

IMHO, the solution is just to use diff3 style.  I never understood how I
was supposed to intuit the correct result of a merge from the two sides
without seeing the common ancestor, so I am glad to have the diff3 style
working now.

Matt

^ permalink raw reply

* Re: Usability of git stash
From: Jeff King @ 2008-10-20  0:36 UTC (permalink / raw)
  To: Shawn O. Pearce; +Cc: Anders Melchiorsen, Brandon Casey, David Kastrup, git
In-Reply-To: <20081019184029.GF14786@spearce.org>

On Sun, Oct 19, 2008 at 11:40:30AM -0700, Shawn O. Pearce wrote:

> Ditto.  I never use "git stash".  Its command line usage is too
> unfriendly for me, so I tend to prefer making WIP commits.  If I
> need to stash something I'll do:
> 
>   git commit -a -m wip
>   ... some time later ..
>   git checkout branch
>   git reset --soft HEAD^

That's what I do, too, except when I want to move changes from one
branch to another, or split some changes from their history. So
something like:

  git checkout next ;# which is where I usually am anyway
  hack hack hack
  # oops, I have been building this directly on top of next and it
  # really needs to be a feature-branch on maint
  git stash
  git checkout -b jk/maint-fix-whatever origin/maint
  git stash apply

The equivalent non-stash commands would be "commit -m wip" and
"cherry-pick". But the stash saves me the trouble later of having to
delete the wip cruft on top of next.

Side note: obviously this uses the stash only as a push/pop stack. I
have never personally had a situation where I wanted a named stash or
multiple stashes over a wip commit.

> Personally I wish git-stash wasn't invented the way it is.  I would
> have rather seen it as macros to do a quick:
> 
> 	git commit -m wip-index-state
> 	git commit -A -m wip-worktree-state
> 
> and unwind it with essentially:
> 
> 	git reset --mixed HEAD^
> 	git reset --soft HEAD^

I disagree. I think the strength of stash is that it is divorced from
the history. So it is more like a cherry-pick (or diff | apply, which is
what it was intended to replace).

> time they ran the stash.  I think its rare you'd stash something
> then switch to another branch to apply it.  But that could easily
> be done with cherry-pick.

I guess we are viewing the tool oppositely. :)

-Peff

^ permalink raw reply

* Re: [PATCH v2] Fix testcase failure when extended attributes are in use
From: Deskin Miller @ 2008-10-19 23:47 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, heikki.orsila
In-Reply-To: <7viqro73w5.fsf@gitster.siamese.dyndns.org>

On Sun, Oct 19, 2008 at 12:59:06PM -0700, Junio C Hamano wrote:
> Deskin Miller <deskinm@umich.edu> writes:
> 
> > My patch, on the other hand, is to deal with 'ls' output in case a file has
> > certain filesystem extended attributes.  These could be e.g. POSIX ACLs, or a
> > SELinux security context, or perhaps others.  If such an extended attribute is
> > present, 'ls -l' will print permissions with a '+' appended, e.g.
> > -rw-r--r--+
> > Instead of
> > -rw-r--r--
> > ...
> > For what it's worth, I've experienced this failure on my Ubuntu 8.04 laptop
> > with SELinux permissive mode, so it's possible ls behaves slightly differently
> > on other systems; I've not been able to determine this one way or another.
> 
> Is there way to explicitly tell "ls -l" not to do this, I have to wonder?

I looked through all the options to ls several times, and there wasn't anything
which stood out to me.  The output format of ls is rather rigid.
 
> POSIX.1 says that the file mode written under the -l option is
> "%c%s%s%s%c" (where the first %c is for type, three %s are for owner,
> group and other perm, and the last %c is "optional alternate access method
> flag").  If there is no alternate or additional access control method
> associated with the file, the "optional alternate access method flag"
> would be a single SP, otherwise it would be a printable character.

My fault for not reading up more carefully.  In that case, yes, I agree that we
should be able to deal with any such flag character, which my patch does not
do.
 
> If we drop the default ACL from the trash directory like Matt's patch
> does, does a file created in there (i.e. the ones we check with /bin/ls)
> still have "alternate or additional access control method associated with"
> it?

Yes.  SELinux, in particular, associates a 'security context' with *every*
file, which is used in SELinux access control checks.  This is stored as a
filesystem extended attribute in the 'security.selinux' namespace, and one will
exist from the moment the file is created.

One could dream up other access control methods, such as a kernel module which
would deny access to any files with the string 'frob' in the filename, and
suppose that ls could be extended to recognise the presence of this module and
alter its display accordingly.  There is clearly no way to ensure that no
'alternate or additional access control method' is associated with a file,
since the POSIX.1 language is sufficiently general to allow for almost
anything.

> Somehow it feels wrong that you need your patch, but if you do, stripping
> only the trailing '+' as your patch does not look sufficient, either.
> Shouldn't we be stripping the last letter if the length of actual is
> longer than strlen("-rwxrwxrwx"), as any printable can come there?

I hope I've made the case for the necessity of a patch (if nothing else, t1301
fails 10/16 tests on my system without some sort of patch), and I'll happily
agree that stripping any printable is a better choice.  Thanks for the
feedback; FWIW, your suggested patch here is

Tested-by: Deskin Miller <deskinm@umich.edu>

>  t/t1301-shared-repo.sh |   10 ++++++----
>  1 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git c/t/t1301-shared-repo.sh i/t/t1301-shared-repo.sh
> index 2275caa..653362b 100755
> --- c/t/t1301-shared-repo.sh
> +++ i/t/t1301-shared-repo.sh
> @@ -20,6 +20,10 @@ test_expect_success 'shared = 0400 (faulty permission u-w)' '
>  	test $ret != "0"
>  '
>  
> +modebits () {
> +	ls -l "$1" | sed -e 's|^\(..........\).*|\1|'
> +}
> +
>  for u in 002 022
>  do
>  	test_expect_success "shared=1 does not clear bits preset by umask $u" '
> @@ -85,8 +89,7 @@ do
>  
>  		rm -f .git/info/refs &&
>  		git update-server-info &&
> -		actual="$(ls -l .git/info/refs)" &&
> -		actual=${actual%% *} &&
> +		actual="$(modebits .git/info/refs)" &&
>  		test "x$actual" = "x-$y" || {
>  			ls -lt .git/info
>  			false
> @@ -98,8 +101,7 @@ do
>  
>  		rm -f .git/info/refs &&
>  		git update-server-info &&
> -		actual="$(ls -l .git/info/refs)" &&
> -		actual=${actual%% *} &&
> +		actual="$(modebits .git/info/refs)" &&
>  		test "x$actual" = "x-$x" || {
>  			ls -lt .git/info
>  			false

^ permalink raw reply

* Re: [PATCH v2] Add command line option --chdir/-C to allow setting git process work directory.
From: Maciej Pasternacki @ 2008-10-19 23:13 UTC (permalink / raw)
  To: git
In-Reply-To: <0B4A1BD5-5FC3-4B29-B0BB-77A85AEDF5B2@pasternacki.net>

On 2008-10-19, at 23:02, Jeff King wrote:

> I took a closer look at this. I'm not sure that there is actually a
> problem with moving the cd_to_toplevel before require_work_tree.
> cd_to_toplevel makes sure there is actually something to cdup to. In  
> my
> test without a work-tree, "git rev-parse --show-cdup" simply printed
> nothing, causing no "cd" to occur.

I won't speculate on whether moving cd_to_toplevel up will introduce  
problem or not.  I am not familiar with git internals, and I just  
repeat an opinion gathered on #git.  However, -C seem to be better  
suited for simple cases, conceptually cleaner, and may make the  
learning curve a bit less steep.  There is no need for consdering all  
possible --work-tree/--git-dir combinations and their implications, it  
is possible just to say "cd there, and act as usual".

> So:
>
> 1. I think we can fix this breakage by swapping the two.

I can prepare a patch if that's expected, but probably I won't  
understand what it exactly does and what are implications of this  
change.

> 2. There is still breakage in other scripts, some of which may need
>    quite in-depth fixes (e.g., git-bisect requires a work tree but
>    does not chdir at all. For the require_work_tree test to work, it
>    needs to be inside the work tree, and the script will need a
>    careful looking over to see what ramifications that has).

I really don't think I'm competent enough to fix more complex cases  
without breaking twice as many things; I don't feel confident even  
with a simple swap in git-pull at the moment

> 3. I think your -C option has some merit independent of this, since
>    it allows you to chdir and still use the usual lookup rules (e.g.,
>    see if it is bare vs a regular repository). But I don't feel
>    strongly about it one way or the other.

In my use case, it definitely has merits as it makes git behave  
exactly as if user himself wrote "git pull" inside the work tree, and  
I am still not sure wheter expected --work-tree/--git-dir combinations  
would behave the same.  Probably I'll need to add more commands and  
more complex use cases as I develop cl-librarian, and -C would make  
this job way easier.

Regards,
Maciej.

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

^ permalink raw reply

* Re: Usability of git stash
From: Stephan Beyer @ 2008-10-19 23:12 UTC (permalink / raw)
  To: Shawn O. Pearce; +Cc: Anders Melchiorsen, Brandon Casey, David Kastrup, git
In-Reply-To: <20081019184029.GF14786@spearce.org>

Hi,

Shawn O. Pearce wrote:
> Personally I wish git-stash wasn't invented the way it is.  I would
> have rather seen it as macros to do a quick:
> 
> 	git commit -m wip-index-state
> 	git commit -A -m wip-worktree-state
> 
> and unwind it with essentially:
> 
> 	git reset --mixed HEAD^
> 	git reset --soft HEAD^
> 
> then its a lot less black magic to users, as they can see it in the
> DAG, and its more explicitly tied to the branch they were on at the
> time they ran the stash.  I think its rare you'd stash something
> then switch to another branch to apply it.  But that could easily
> be done with cherry-pick.

Just a side note:
I, for example, have a stash in which some "valgrind ..." string is
prepended to some lines in some test scripts.  I apply the stash on
different branches and after testing I reset the file (or checkout -f
another branch).  I never really want to commit these changes.

So, the alternative for this use case (if there was no git-stash) is
to have a "valgrind" branch somewhere and do some "cherry-pick -n" of
those commits, right?

Hmm, it seems that this is the better way in the long run. But
"git-stash" seemed to be the natural way at first.

Regards,
  Stephan

-- 
Stephan Beyer <s-beyer@gmx.net>, PGP 0x6EDDD207FCC5040F

^ permalink raw reply

* Is XDL_MERGE_ZEALOUS too zealous (or maybe not zealous enough)?
From: Junio C Hamano @ 2008-10-19 22:52 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: git, Matt McCutchen
In-Reply-To: <7vhc785izq.fsf@gitster.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> writes:

> Again, good eyes.  I think the two lines should go; my fault at cdb22c4
> (Merge branch 'jc/better-conflict-resolution' into next, 2008-09-02).

Hmm, I am somewhat unhappy.  If you run:

	$ git checkout cdb22c4^
        $ git merge cdb22c4^2
	$ git checkout --conflict=diff3 builtin-checkout.c

and look at builtin-checkout.c.  You will find this:

<<<<<<< ours
	/* --track without -b should DWIM */
	if (0 < opts.track && !opts.new_branch) {
		const char *argv0 = argv[0];
	...
		opts.new_branch = argv0 + 1;
	}

	if (opts.track == BRANCH_TRACK_UNSPECIFIED)
		opts.track = git_branch_track;
|||||||
	if (!opts.new_branch && (opts.track != git_branch_track))
		die("git checkout: --track and --no-track require -b");
=======
	if (conflict_style) {
		opts.merge = 1; /* implied */
		git_xmerge_config("merge.conflictstyle", conflict_style, NULL);
	}

	if (!opts.new_branch && (opts.track != git_branch_track))
		die("git checkout: --track and --no-track require -b");
>>>>>>> theirs

The two lines in the middle was from the common ancestor, which was not
touched by the merged branch (that added the "if (conflict_style) {}"
block) and was lost by a rewrite in "ours".

However, the usual simplified merge shows this (run "git checkout --merge
builtin-checkout.c" if you have done the above):

<<<<<<< ours
	/* --track without -b should DWIM */
	if (0 < opts.track && !opts.new_branch) {
		const char *argv0 = argv[0];
	...
		opts.new_branch = argv0 + 1;
	}

	if (opts.track == BRANCH_TRACK_UNSPECIFIED)
		opts.track = git_branch_track;
=======
	if (conflict_style) {
		opts.merge = 1; /* implied */
		git_xmerge_config("merge.conflictstyle", conflict_style, NULL);
	}

	if (!opts.new_branch && (opts.track != git_branch_track))
		die("git checkout: --track and --no-track require -b");
>>>>>>> theirs

Removing the two lines from the simplified "theirs" is not what I would
suggest (it would be actively wrong), but I wonder if we can do something
clever to help users with a merge like this.

^ permalink raw reply

* Re: "git checkout: --track and --no-track require -b" check accidentally resurrected?
From: Junio C Hamano @ 2008-10-19 22:15 UTC (permalink / raw)
  To: Matt McCutchen; +Cc: git
In-Reply-To: <1224377652.19061.12.camel@mattlaptop2.local>

Matt McCutchen <matt@mattmccutchen.net> writes:

> Merge commit 9ba929ed resurrected the following two-line check, which
> was removed in the first parent and unchanged in the second:
>
> 	if (!opts.new_branch && (opts.track != git_branch_track))
> 		die("git checkout: --track and --no-track require -b");
>
> Is this intentional?  Does it make a difference?
>
> (I noticed this while carefully examining 9ba929ed to find out why "git
> merge" stopped honoring merge.conflictstyle.  Ironically, I hit this bug
> again during the examination.)

Again, good eyes.  I think the two lines should go; my fault at cdb22c4
(Merge branch 'jc/better-conflict-resolution' into next, 2008-09-02).

Thanks.

^ permalink raw reply

* [GITK PATCH v2] Add menu accelerators
From: Robin Rosenberg @ 2008-10-19 22:00 UTC (permalink / raw)
  To: Paul Mackerras; +Cc: git
In-Reply-To: <18681.53866.855255.688290@cargo.ozlabs.ibm.com>

Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
---
 gitk |   80 +++++++++++++++++++++++++++++++++---------------------------------
 1 files changed, 40 insertions(+), 40 deletions(-)

This one is slightly better than the first one.  Works with gitk id...id too.

-- robin

diff --git a/gitk b/gitk
index 3678de1..df039b9 100755
--- a/gitk
+++ b/gitk
@@ -1817,26 +1817,26 @@ proc makewindow {} {
     # The "mc" arguments here are purely so that xgettext
     # sees the following string as needing to be translated
     makemenu .bar {
-	{mc "File" cascade {
-	    {mc "Update" command updatecommits -accelerator F5}
-	    {mc "Reload" command reloadcommits}
-	    {mc "Reread references" command rereadrefs}
-	    {mc "List references" command showrefs}
-	    {mc "Quit" command doquit}
+	{mc "&File" cascade {
+	    {mc "&Update" command updatecommits -accelerator F5}
+	    {mc "&Reload" command reloadcommits}
+	    {mc "R&eread references" command rereadrefs}
+	    {mc "&List references" command showrefs}
+	    {mc "&Quit" command doquit}
 	}}
-	{mc "Edit" cascade {
-	    {mc "Preferences" command doprefs}
+	{mc "&Edit" cascade {
+	    {mc "&Preferences" command doprefs}
 	}}
-	{mc "View" cascade {
-	    {mc "New view..." command {newview 0}}
-	    {mc "Edit view..." command editview -state disabled}
-	    {mc "Delete view" command delview -state disabled}
+	{mc "&View" cascade {
+	    {mc "&New view..." command {newview 0}}
+	    {mc "&Edit view..." command editview -state disabled}
+	    {mc "&Delete view" command delview -state disabled}
 	    {xx "" separator}
-	    {mc "All files" radiobutton {selectedview 0} -command {showview 0}}
+	    {mc "&All files" radiobutton {selectedview 0} -command {showview 0}}
 	}}
-	{mc "Help" cascade {
-	    {mc "About gitk" command about}
-	    {mc "Key bindings" command keys}
+	{mc "&Help" cascade {
+	    {mc "&About gitk" command about}
+	    {mc "&Key bindings" command keys}
 	}}
     }
     . configure -menu .bar
@@ -2220,39 +2220,39 @@ proc makewindow {} {
 
     set rowctxmenu .rowctxmenu
     makemenu $rowctxmenu {
-	{mc "Diff this -> selected" command {diffvssel 0}}
-	{mc "Diff selected -> this" command {diffvssel 1}}
-	{mc "Make patch" command mkpatch}
-	{mc "Create tag" command mktag}
-	{mc "Write commit to file" command writecommit}
-	{mc "Create new branch" command mkbranch}
-	{mc "Cherry-pick this commit" command cherrypick}
-	{mc "Reset HEAD branch to here" command resethead}
+	{mc "Diff &this -> selected" command {diffvssel 0}}
+	{mc "Diff &selected -> this" command {diffvssel 1}}
+	{mc "Make &patch" command mkpatch}
+	{mc "Create ta&g" command mktag}
+	{mc "&Write commit to file" command writecommit}
+	{mc "Create new &branch" command mkbranch}
+	{mc "&Cherry-pick this commit" command cherrypick}
+	{mc "&Reset HEAD branch to here" command resethead}
     }
     $rowctxmenu configure -tearoff 0
 
     set fakerowmenu .fakerowmenu
     makemenu $fakerowmenu {
-	{mc "Diff this -> selected" command {diffvssel 0}}
-	{mc "Diff selected -> this" command {diffvssel 1}}
-	{mc "Make patch" command mkpatch}
+	{mc "Diff &this -> selected" command {diffvssel 0}}
+	{mc "Diff &selected -> this" command {diffvssel 1}}
+	{mc "Make &patch" command mkpatch}
     }
     $fakerowmenu configure -tearoff 0
 
     set headctxmenu .headctxmenu
     makemenu $headctxmenu {
-	{mc "Check out this branch" command cobranch}
-	{mc "Remove this branch" command rmbranch}
+	{mc "&Check out this branch" command cobranch}
+	{mc "&Remove this branch" command rmbranch}
     }
     $headctxmenu configure -tearoff 0
 
     global flist_menu
     set flist_menu .flistctxmenu
     makemenu $flist_menu {
-	{mc "Highlight this too" command {flist_hl 0}}
-	{mc "Highlight this only" command {flist_hl 1}}
-	{mc "External diff" command {external_diff}}
-	{mc "Blame parent commit" command {external_blame 1}}
+	{mc "Highlight this &too" command {flist_hl 0}}
+	{mc "Highlight this &only" command {flist_hl 1}}
+	{mc "E&xternal diff" command {external_diff}}
+	{mc "&Blame parent commit" command {external_blame 1}}
     }
     $flist_menu configure -tearoff 0
 }
@@ -3414,8 +3414,8 @@ proc showview {n} {
 
     set curview $n
     set selectedview $n
-    .bar.view entryconf [mca "Edit view..."] -state [expr {$n == 0? "disabled": "normal"}]
-    .bar.view entryconf [mca "Delete view"] -state [expr {$n == 0? "disabled": "normal"}]
+    .bar.view entryconf [mca "&Edit view..."] -state [expr {$n == 0? "disabled": "normal"}]
+    .bar.view entryconf [mca "&Delete view"] -state [expr {$n == 0? "disabled": "normal"}]
 
     run refill_reflist
     if {![info exists viewcomplete($n)]} {
@@ -7361,9 +7361,9 @@ proc rowmenu {x y id} {
     } else {
 	set menu $fakerowmenu
     }
-    $menu entryconfigure [mca "Diff this -> selected"] -state $state
-    $menu entryconfigure [mca "Diff selected -> this"] -state $state
-    $menu entryconfigure [mca "Make patch"] -state $state
+    $menu entryconfigure [mca "Diff &this -> selected"] -state $state
+    $menu entryconfigure [mca "Diff &selected -> this"] -state $state
+    $menu entryconfigure [mca "Make &patch"] -state $state
     tk_popup $menu $x $y
 }
 
@@ -10184,8 +10184,8 @@ if {$cmdline_files ne {} || $revtreeargs ne {} || $revtreeargscmd ne {}} {
     set viewperm(1) 0
     set vdatemode(1) 0
     addviewmenu 1
-    .bar.view entryconf [mca "Edit view..."] -state normal
-    .bar.view entryconf [mca "Delete view"] -state normal
+    .bar.view entryconf [mca "&Edit view..."] -state normal
+    .bar.view entryconf [mca "&Delete view"] -state normal
 }
 
 if {[info exists permviews]} {
-- 
1.6.0.2.308.gef4a

^ permalink raw reply related

* Re: [PATCH] git-merge-recursive: honor merge.conflictstyle once again
From: Junio C Hamano @ 2008-10-19 21:50 UTC (permalink / raw)
  To: Matt McCutchen; +Cc: git
In-Reply-To: <1224376850.19061.1.camel@mattlaptop2.local>

Matt McCutchen <matt@mattmccutchen.net> writes:

> This was originally implemented in c236bcd06138bcbc929b86ad1a513635bf4847b2
> but was lost to a mismerge in 9ba929ed652f5ed7707f1c684999af4ad02c4925.

Good eyes; thanks.

^ permalink raw reply

* Re: Usability of git stash
From: Shawn O. Pearce @ 2008-10-19 21:49 UTC (permalink / raw)
  To: Leo Razoumov; +Cc: git, Anders Melchiorsen, Brandon Casey, David Kastrup
In-Reply-To: <ee2a733e0810191408m2cbe1ba5n540ad0fb99b49708@mail.gmail.com>

Leo Razoumov <slonik.az@gmail.com> wrote:
> On 10/19/08, Shawn O. Pearce <spearce@spearce.org> wrote:
> > [..snip..]
> > Indeed, that's an advantage of the wip commit approach, you can shove
> >  the untracked files quickly into the wip commit, especially with 1.6:
> >
> >         git commit -A -m wip
> >
> > [..snip..]
> 
> What version of git are you using?
> I am using git-1.6.0.2 on Linux and "-A" is illegal option for "git commit"

Oh.  Sorry.  I thought I saw a patch for that go by.  I guess I mean:

	git add -A && git commit

-- 
Shawn.

^ permalink raw reply

* Re: [PATCH] parse-opt: migrate builtin-checkout-index.
From: Junio C Hamano @ 2008-10-19 21:45 UTC (permalink / raw)
  To: Miklos Vajna; +Cc: Pierre Habouzit, git
In-Reply-To: <1224292643-28704-1-git-send-email-vmiklos@frugalware.org>

Miklos Vajna <vmiklos@frugalware.org> writes:

>> > +   if (argc && read_from_stdin)
>> > +           die("--stdin must be at the end");
>>
>> Is this comment still correct?  Do the original and your version act
>> the
>> same way when the user says "checkout --stdin -f", for example?  I
>> suspect
>> the original refused it and yours take it (and do much more sensible
>> thing), which would be an improvement, but then the error message
>> should
>> be reworded perhaps?
>
> Unless I missed something, that was a limitation of the option parser.
> checkout-index --stdin -f works fine for me after removing those two
> lines, so I left them out from the updated patch.

Thanks.  I think you got what I meant and dropping the part is right.

"--stdin -f" was rejected by the original code, and you improved to take
it with the new parser.  In fact, the above quoted if() statement should
not trigger when "--stdin -f" is given, due to the way the new option
parser is structured.  The original had an explicit "break" in the loop
when it saw "--stdin".  The above would still trigger if "--stdin foo" is
given, but there is a code to catch that already, so it is not necessary.

^ permalink raw reply

* Re: [PATCH] Teach/Fix pull/fetch -q/-v options
From: Junio C Hamano @ 2008-10-19 21:26 UTC (permalink / raw)
  To: Tuncer Ayaz; +Cc: git
In-Reply-To: <1224445691-1366-1-git-send-email-tuncer.ayaz@gmail.com>

Tuncer Ayaz <tuncer.ayaz@gmail.com> writes:

> 2) my adaption of the following two lines from
> builtin-fetch.c to the new verbosity option:
>     if (verbosity == VERBOSE)
>         transport->verbose = 1;
>     if (verbosity == QUIET)
>         transport->verbose = -1;

Hmm, what's wrong with it?  Looks Ok to me...

>  static struct option builtin_fetch_options[] = {
> -	OPT__QUIET(&quiet),
> -	OPT__VERBOSE(&verbose),
> +	{ OPTION_CALLBACK, 'q', "quiet", NULL, NULL,
> +		"operate quietly",
> +		PARSE_OPT_NOARG, option_parse_quiet },
> +	{ OPTION_CALLBACK, 'v', "verbose", NULL, NULL,
> +		"be verbose",
> +		PARSE_OPT_NOARG, option_parse_verbose },

Isn't there a OPTION_FOO that assigns a constant to the given variable?

> @@ -192,7 +211,6 @@ static int s_update_ref(const char *action,
>  
>  static int update_local_ref(struct ref *ref,
>  			    const char *remote,
> -			    int verbose,
>  			    char *display)
>  {
>  	struct commit *current = NULL, *updated;
> ...
> @@ -366,18 +384,19 @@ static int store_updated_refs(const char *url, const char *remote_name,
>  			note);
>  
>  		if (ref)
> -			rc |= update_local_ref(ref, what, verbose, note);
> +			rc |= update_local_ref(ref, what, note);

Hmph, in the existing code, do_fetch()->fetch_refs()->store_updated_refs()
callchain relies on the "verbose" to be global anyway, so losing the
ability to call update_local_ref() with verbosity as parameter is not a
huge deal.

I however think it would be more beneficial in the longer term to keep
"verbosity" as parameter so the caller can tweak what the callee does, and
making large part of what cmd_fetch() does callable from outside.  That
would involve making the builtin_fetch_options[] on-stack, and passing
verbosity (and possibly other variables currently used as file-scope
global) as parameters, which is outside of the scope of your patch, but it
is something to keep in mind.

> diff --git a/git-pull.sh b/git-pull.sh
> index 75c3610..dc613db 100755
> --- a/git-pull.sh
> +++ b/git-pull.sh
> @@ -16,6 +16,7 @@ cd_to_toplevel
>  test -z "$(git ls-files -u)" ||
>  	die "You are in the middle of a conflicted merge."
>  
> +verbosity=
>  strategy_args= no_stat= no_commit= squash= no_ff= log_arg=
>  curr_branch=$(git symbolic-ref -q HEAD)
>  curr_branch_short=$(echo "$curr_branch" | sed "s|refs/heads/||")

It would fit at the end of the next line just fine, wouldn't it?

> @@ -23,6 +24,10 @@ rebase=$(git config --bool branch.$curr_branch_short.rebase)
>  while :
>  do
>  	case "$1" in
> +	-q|--quiet)
> +		verbosity="$verbosity -q" ;;
> +	-v|--verbose)
> +		verbosity="$verbosity -v" ;;

You know verbosity flags (-q and -v) are "the last one wins", so I do not
see much point in this concatenation.

^ permalink raw reply

* [GITK PATCH] Add menu accelerators
From: Robin Rosenberg @ 2008-10-19 21:19 UTC (permalink / raw)
  To: Paul Mackerras; +Cc: git
In-Reply-To: <18681.53866.855255.688290@cargo.ozlabs.ibm.com>

Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
---

lördagen den 18 oktober 2008 14.11.22 skrev Paul Mackerras:
> Have a look at what I just pushed out.  It adds infrastructure to let
> us use "&" in menu items to specify an alt+letter accelerator, but in
> a different way to your patches.  If you'd like to redo your patch to
> add "&" to the menu items, that would be good.

Looks fine. Seems to work too. Then we want to do the same thing
with buttons. Btw, is it only for me that the "popup menu key" does
not work for in gitk?

This patch does not include re-generated po's, nor the updated swedish translation.

-- robin

 gitk |   72 +++++++++++++++++++++++++++++++++---------------------------------
 1 files changed, 36 insertions(+), 36 deletions(-)

diff --git a/gitk b/gitk
index 3678de1..e65c0ce 100755
--- a/gitk
+++ b/gitk
@@ -1817,26 +1817,26 @@ proc makewindow {} {
     # The "mc" arguments here are purely so that xgettext
     # sees the following string as needing to be translated
     makemenu .bar {
-	{mc "File" cascade {
-	    {mc "Update" command updatecommits -accelerator F5}
-	    {mc "Reload" command reloadcommits}
-	    {mc "Reread references" command rereadrefs}
-	    {mc "List references" command showrefs}
-	    {mc "Quit" command doquit}
+	{mc "&File" cascade {
+	    {mc "&Update" command updatecommits -accelerator F5}
+	    {mc "&Reload" command reloadcommits}
+	    {mc "R&eread references" command rereadrefs}
+	    {mc "&List references" command showrefs}
+	    {mc "&Quit" command doquit}
 	}}
-	{mc "Edit" cascade {
-	    {mc "Preferences" command doprefs}
+	{mc "&Edit" cascade {
+	    {mc "&Preferences" command doprefs}
 	}}
-	{mc "View" cascade {
-	    {mc "New view..." command {newview 0}}
-	    {mc "Edit view..." command editview -state disabled}
-	    {mc "Delete view" command delview -state disabled}
+	{mc "&View" cascade {
+	    {mc "&New view..." command {newview 0}}
+	    {mc "&Edit view..." command editview -state disabled}
+	    {mc "&Delete view" command delview -state disabled}
 	    {xx "" separator}
-	    {mc "All files" radiobutton {selectedview 0} -command {showview 0}}
+	    {mc "&All files" radiobutton {selectedview 0} -command {showview 0}}
 	}}
-	{mc "Help" cascade {
-	    {mc "About gitk" command about}
-	    {mc "Key bindings" command keys}
+	{mc "&Help" cascade {
+	    {mc "&About gitk" command about}
+	    {mc "&Key bindings" command keys}
 	}}
     }
     . configure -menu .bar
@@ -2220,39 +2220,39 @@ proc makewindow {} {
 
     set rowctxmenu .rowctxmenu
     makemenu $rowctxmenu {
-	{mc "Diff this -> selected" command {diffvssel 0}}
-	{mc "Diff selected -> this" command {diffvssel 1}}
-	{mc "Make patch" command mkpatch}
-	{mc "Create tag" command mktag}
-	{mc "Write commit to file" command writecommit}
-	{mc "Create new branch" command mkbranch}
-	{mc "Cherry-pick this commit" command cherrypick}
-	{mc "Reset HEAD branch to here" command resethead}
+	{mc "Diff &this -> selected" command {diffvssel 0}}
+	{mc "Diff &selected -> this" command {diffvssel 1}}
+	{mc "Make &patch" command mkpatch}
+	{mc "Create ta&g" command mktag}
+	{mc "&Write commit to file" command writecommit}
+	{mc "Create new &branch" command mkbranch}
+	{mc "&Cherry-pick this commit" command cherrypick}
+	{mc "&Reset HEAD branch to here" command resethead}
     }
     $rowctxmenu configure -tearoff 0
 
     set fakerowmenu .fakerowmenu
     makemenu $fakerowmenu {
-	{mc "Diff this -> selected" command {diffvssel 0}}
-	{mc "Diff selected -> this" command {diffvssel 1}}
-	{mc "Make patch" command mkpatch}
+	{mc "Diff &this -> selected" command {diffvssel 0}}
+	{mc "Diff &selected -> this" command {diffvssel 1}}
+	{mc "Make &patch" command mkpatch}
     }
     $fakerowmenu configure -tearoff 0
 
     set headctxmenu .headctxmenu
     makemenu $headctxmenu {
-	{mc "Check out this branch" command cobranch}
-	{mc "Remove this branch" command rmbranch}
+	{mc "&Check out this branch" command cobranch}
+	{mc "&Remove this branch" command rmbranch}
     }
     $headctxmenu configure -tearoff 0
 
     global flist_menu
     set flist_menu .flistctxmenu
     makemenu $flist_menu {
-	{mc "Highlight this too" command {flist_hl 0}}
-	{mc "Highlight this only" command {flist_hl 1}}
-	{mc "External diff" command {external_diff}}
-	{mc "Blame parent commit" command {external_blame 1}}
+	{mc "Highlight this &too" command {flist_hl 0}}
+	{mc "Highlight this &only" command {flist_hl 1}}
+	{mc "E&xternal diff" command {external_diff}}
+	{mc "&Blame parent commit" command {external_blame 1}}
     }
     $flist_menu configure -tearoff 0
 }
@@ -7361,9 +7361,9 @@ proc rowmenu {x y id} {
     } else {
 	set menu $fakerowmenu
     }
-    $menu entryconfigure [mca "Diff this -> selected"] -state $state
-    $menu entryconfigure [mca "Diff selected -> this"] -state $state
-    $menu entryconfigure [mca "Make patch"] -state $state
+    $menu entryconfigure [mca "Diff &this -> selected"] -state $state
+    $menu entryconfigure [mca "Diff &selected -> this"] -state $state
+    $menu entryconfigure [mca "Make &patch"] -state $state
     tk_popup $menu $x $y
 }
 
-- 
1.6.0.2.308.gef4a

^ permalink raw reply related

* Re: Usability of git stash
From: Leo Razoumov @ 2008-10-19 21:08 UTC (permalink / raw)
  To: Shawn O. Pearce, git; +Cc: Anders Melchiorsen, Brandon Casey, David Kastrup
In-Reply-To: <20081019184029.GF14786@spearce.org>

On 10/19/08, Shawn O. Pearce <spearce@spearce.org> wrote:
> [..snip..]
> Indeed, that's an advantage of the wip commit approach, you can shove
>  the untracked files quickly into the wip commit, especially with 1.6:
>
>         git commit -A -m wip
>
> [..snip..]

What version of git are you using?
I am using git-1.6.0.2 on Linux and "-A" is illegal option for "git commit"

--Leo--

^ permalink raw reply

* Re: [PATCH v2] Add command line option --chdir/-C to allow setting git process work directory.
From: Jeff King @ 2008-10-19 21:02 UTC (permalink / raw)
  To: Maciej Pasternacki; +Cc: git
In-Reply-To: <20081019161819.GA12495@charybdis.dreamhost.com>

On Sun, Oct 19, 2008 at 09:18:19AM -0700, Maciej Pasternacki wrote:

> When "git pull" is ran outside of work tree, it is unable to update work tree
> with pulled changes; specifying --git-dir and --work-tree does not help here
> because "cd_to_toplevel" call in git-pull occurs after "require_work_tree";
> changing order may break the commend if there is no working tree.  Some more
> commands behave in a similar way.

I took a closer look at this. I'm not sure that there is actually a
problem with moving the cd_to_toplevel before require_work_tree.
cd_to_toplevel makes sure there is actually something to cdup to. In my
test without a work-tree, "git rev-parse --show-cdup" simply printed
nothing, causing no "cd" to occur.

Moreover, git-pull is not the only script with this problem. There are
quite a few with

  require_work_tree
  cd_to_toplevel

which will break in the same way with --work-tree. And there are even
more with just require_work_tree that don't cd_to_toplevel at all. So I
suspect that --work-tree is largely broken in our scripts and nobody has
actually tried it.

So:

  1. I think we can fix this breakage by swapping the two.

  2. There is still breakage in other scripts, some of which may need
     quite in-depth fixes (e.g., git-bisect requires a work tree but
     does not chdir at all. For the require_work_tree test to work, it
     needs to be inside the work tree, and the script will need a
     careful looking over to see what ramifications that has).

  3. I think your -C option has some merit independent of this, since
     it allows you to chdir and still use the usual lookup rules (e.g.,
     see if it is bare vs a regular repository). But I don't feel
     strongly about it one way or the other.

-Peff

^ permalink raw reply

* Re: [PATCH] parse-opt: migrate builtin-checkout-index.
From: Junio C Hamano @ 2008-10-19 20:59 UTC (permalink / raw)
  To: Pierre Habouzit; +Cc: Miklos Vajna, git
In-Reply-To: <20081019201934.GO16610@artemis.corp>

Pierre Habouzit <madcoder@debian.org> writes:

> On Sun, Oct 19, 2008 at 08:11:31PM +0000, Junio C Hamano wrote:
>> Miklos Vajna <vmiklos@frugalware.org> writes:
>
>> >> Right, I fixed this in option_parse_z(). --no-z should set
>> >> line_termination to \n instead of 1.
>> >
>> > Originally in option_parse_z() I had
>> >
>> >         line_termination = unset;
>> >
>> > which is in fact right, because (as Pierre pointed out) unset for short
>> > options are always false, but I changed it to
>> >
>> >         line_termination = 0;
>> >
>> > to make it more readable.
>> 
>> I think Pierre's comment is short-sighted.  Think of what would happen
>> when somebody adds "--nul" as a longer equivalent to "-z", since it is
>> extremely easy to do things like that with the use of parse-opt API?
>
> Err I was only pointing out that --no-z would no nothing, I actually
> didn't really read the argument :)  I didn't say having --null was a bad
> idea,...

It is a good practice to anticipate potential future breakages, assess the
cost to avoid them, avoid the ones that can be avoided with minimum cost
(and document the others you know will be broken).  That's the key to
produce maintainable piece of code.  The change necessary to Miklos's
original code to do this is quite simple (i.e. flip between '\0' and
'\n'), and I didn't see any room for argument like "short option won't let
you say --no-z".  That's where my comment about "short-sighted"-ness comes
from.

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox