* Re: [PATCH 1/3] Move bundle specific stuff into bundle.[ch]
From: Junio C Hamano @ 2007-07-18 7:26 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: Daniel Barkalow, Johannes Schindelin, git
In-Reply-To: <20070718061931.GZ32566@spearce.org>
"Shawn O. Pearce" <spearce@spearce.org> writes:
> Yea, this I like even better than what I posted. Now we just need
> a suck^H^H^H^Hprogrammer to implement a working prototype and see
> how folks like more realistic diffs generated with it. ;-)
I have to disgree.
We on the git list *live* in pretty much git-only world (ok, I
am ignoring git-svn people for now ;-)), so it might feel it
beneficial to add a new output format to git-diff.
But many users of git generated patch needs to interact with
patches that are not generated by git. I think an additional
postprocessor in patchutils/interdiff toolchest would probably
make much more sense. Then a person who reviews a patch that is
supposed to be line movement can use the filter to verify the
parts that should be only line movements are indeed are
movements and nothing else.
Speaking of "diff" output, what I would like to see is a support
of 'copied context' (i.e. traditional 'context diff' format you
would get from "diff -c"), in addition to the 'unified context'
we support. Generating copied context may help people who need
to give patches to others that accept only copied context format
patches (are there important projects that take only cpied
context format patches?). Being able to accept copied context
format patches in 'git-apply' would also be handy.
Of course, we could use interdiff to mangle copied context
format into unified context format, so 'git-apply' is not a big
deal, though.
^ permalink raw reply
* Re: [PATCH] unpack-trees.c: assume submodules are clean during check-out
From: Junio C Hamano @ 2007-07-18 7:29 UTC (permalink / raw)
To: Sven Verdoolaege; +Cc: git
In-Reply-To: <20070717182828.GA4583MdfPADPa@greensroom.kotnet.org>
Passing of ce instead of path in the unpack-trees callchain
looks like the right thing to do. Good job.
Thanks.
^ permalink raw reply
* Re: git svn dcommit seg fault
From: Eric Wong @ 2007-07-18 7:34 UTC (permalink / raw)
To: Perrin Meyer; +Cc: git
In-Reply-To: <951126.88373.qm@web52807.mail.re2.yahoo.com>
Perrin Meyer <perrinmeyer@yahoo.com> wrote:
>
> I'm able to clone svn repo's fine with
>
> $ git svn clone https://svn.eng.msli.com/perrin/trunk/TESTGIT/ .
>
> and I'm then able to use git commit to commit local changes, but
> when I try
>
> $ git svn dcommit
>
> I get
>
> [perrin@whisper TESTGIT]$ git svn dcommit
> M test.c
> Committed r717
> Segmentation fault
> As far as I can tell, the commit worked fine (verified by trying 'svn
> update' on another box).
>
> I've tried git version 1.5.2.3, 1.5.3-rc2, and the latest build, and
> all give the seg fault.
>
> I'm guessing it has something to do with using the https connection to
> svn?
I primarily work with https repositories using git-svn and I haven't
seen any segfaults in a while. Which version of the SVN libraries do
you have? (git-svn --version will tell you).
--
Eric Wong
^ permalink raw reply
* Re: [PATCH] git-svn: Minimalistic patch which allows svn usernames with space(s).
From: Eric Wong @ 2007-07-18 7:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Richard MUSIL, git
In-Reply-To: <20070717195559.GA20103@muzzle>
Eric Wong <eric@petta-tech.com> wrote:
> Richard MUSIL <richard.musil@st.com> wrote:
> > Changed filter for username in svn-authors file, so even 'user name' is accepted.
> > ---
> > git-svn.perl | 2 +-
> > 1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/git-svn.perl b/git-svn.perl
> > index 01c3904..975075e 100755
> > --- a/git-svn.perl
> > +++ b/git-svn.perl
> > @@ -740,7 +740,7 @@ sub load_authors {
> > my $log = $cmd eq 'log';
> > while (<$authors>) {
> > chomp;
> > - next unless /^(\S+?|\(no author\))\s*=\s*(.+?)\s*<(.+)>\s*$/;
> > + next unless /^(\.+?|\(no author\))\s*=\s*(.+?)\s*<(.+)>\s*$/;
>
> Surely you mean the following:
>
> + next unless /^(.+?|\(no author\))\s*=\s*(.+?)\s*<(.+)>\s*$/;
>
> (No "\" before the ".") "\." matches a dot/period (.), while "."
> matches anything.
Fwiw, with the regexp corrected:
Acked-by: Eric Wong <normalperson@yhbt.net>
--
Eric Wong
^ permalink raw reply
* Re: [PATCH] Rename read_pipe() with read_fd() and make its buffer nul-terminated.
From: Junio C Hamano @ 2007-07-18 7:49 UTC (permalink / raw)
To: Carlos Rica; +Cc: git, Johannes Schindelin, Kristian Høgsberg
In-Reply-To: <469DBC8A.6090704@gmail.com>
Carlos Rica <jasampler@gmail.com> writes:
> The new name is closer to the purpose of the function.
>
> The other change just makes things easier for callers needing a
> NUL-terminated buffer.
>
> Since the function returns only the memory written with data,
> almost always allocating more space than needed because final
> size is unknown, an extra NUL terminating the buffer is harmless.
> It is not included in the returned size, so the function
> remains working as before.
>
> Also, now the function allows the buffer passed to be NULL at first,
> and alloc_nr is now used for growing the buffer, instead size=*2.
Some people may say function name change should be a separate
patch by itself, but I'd let it pass for now...
> diff --git a/sha1_file.c b/sha1_file.c
> index 1efd9ae..563ec07 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -2304,27 +2304,38 @@ int has_sha1_file(const unsigned char *sha1)
> *
> * returns 0 if anything went fine and -1 otherwise
> *
> + * The buffer is always NUL-terminated, not including it in returned size.
> + *
> * NOTE: both buf and size may change, but even when -1 is returned
> * you still have to free() it yourself.
> */
> -int read_pipe(int fd, char** return_buf, unsigned long* return_size)
> +int read_fd(int fd, char** return_buf, unsigned long* return_size)
> {
> char* buf = *return_buf;
Our code seems to prefer pointer declaration to be spelled as
"type **var", not "type** var".
> unsigned long size = *return_size;
> ssize_t iret;
> unsigned long off = 0;
>
> + if (!buf || size <= 1) {
> + size = alloc_nr(size);
> + buf = xrealloc(buf, size);
> + }
> +
Hmph. The reason this is not "!size" is because you would want
more than one. As your plan is to use this mostly for the -F
option of "tag/commit", I suspect using a bit larger default,
such as 80 (just a line), or probably 1k (most log messages
would fit in such a buffer), would be more practical.
> do {
> iret = xread(fd, buf + off, size - off);
> if (iret > 0) {
> off += iret;
> if (off == size) {
> - size *= 2;
> + size = alloc_nr(size);
> buf = xrealloc(buf, size);
> }
> }
> } while (iret > 0);
>
> + if (off == size)
> + buf = xrealloc(buf, size + 1);
> + buf[off] = '\0';
> +
I wonder if doing xread(... (size-1) - off) in the loop would
ensure (off <= size-1) here. You also would need to update the
realloc condition inside loop if you do so.
^ permalink raw reply
* Re: [PATCH] git-svn: Minimalistic patch which allows svn usernames with space(s).
From: Richard MUSIL @ 2007-07-18 8:47 UTC (permalink / raw)
To: git
In-Reply-To: <20070717195559.GA20103@muzzle>
Eric Wong wrote:
> Richard MUSIL <richard.musil@st.com> wrote:
>> Changed filter for username in svn-authors file, so even 'user name' is accepted.
>> ---
>> git-svn.perl | 2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/git-svn.perl b/git-svn.perl
>> index 01c3904..975075e 100755
>> --- a/git-svn.perl
>> +++ b/git-svn.perl
>> @@ -740,7 +740,7 @@ sub load_authors {
>> my $log = $cmd eq 'log';
>> while (<$authors>) {
>> chomp;
>> - next unless /^(\S+?|\(no author\))\s*=\s*(.+?)\s*<(.+)>\s*$/;
>> + next unless /^(\.+?|\(no author\))\s*=\s*(.+?)\s*<(.+)>\s*$/;
>
> Surely you mean the following:
>
> + next unless /^(.+?|\(no author\))\s*=\s*(.+?)\s*<(.+)>\s*$/;
>
> (No "\" before the ".") "\." matches a dot/period (.), while "."
> matches anything.
>
Yes! I am sorry for a confusion, I have tested it on '.' version, but committed the wrong one :(.
Richard
^ permalink raw reply
* Re: [PATCH] Do _not_ call unlink on a directory
From: Thomas Glanzmann @ 2007-07-18 8:50 UTC (permalink / raw)
To: GIT
In-Reply-To: <469DC037.A7D69826@eudaptics.com>
Hello,
> > alias elinks='echo DO *NOT* RUN ELINKS AS ROOT'
> And if you have a file NOTES in $pwd, it will tell you:
> DO NOTES RUN GIT AS ROOT
fixed.
Thomas
^ permalink raw reply
* Re: "git clone" executed as root on solaris 10 shreds UFS (it is possible to create hardlinks for directories as root under solaris)
From: Thomas Glanzmann @ 2007-07-18 8:50 UTC (permalink / raw)
To: Linus Torvalds; +Cc: GIT
In-Reply-To: <20070716180910.GB16878@cip.informatik.uni-erlangen.de>
Hello,
> > I asked the UFS maintainer to reconsider to fix this.
> the bug is filed.
http://bugs.opensolaris.org/view_bug.do?bug_id=6581318
Thomas
^ permalink raw reply
* Re: [PATCH 1/3] Move bundle specific stuff into bundle.[ch]
From: Johannes Schindelin @ 2007-07-18 9:56 UTC (permalink / raw)
To: Daniel Barkalow; +Cc: git
In-Reply-To: <Pine.LNX.4.64.0707172216420.14596@iabervon.org>
Hi,
On Tue, 17 Jul 2007, Daniel Barkalow wrote:
> On Tue, 17 Jul 2007, Johannes Schindelin wrote:
>
> > The transport specific stuff was moved into libgit.a, and the bundle
> > specific stuff will not be left behind.
> >
> > This is a big code move, with one exception: the function unbundle()
> > no longer outputs the list of refs. You have to call
> > list_bundle_refs() yourself for that.
>
> You should use -C on this sort of thing, so that the interesting aspects
> of the patch are easier to see. (It actually comes out longer in this
> case, but it's far easier to tell that the code in the new file is the
> same as the old code.)
Okay, I wanted it to be kept short, since I really get lost easily in
hundreds of "-" lines, with possibly one in the midst being a "+".
> Can you tell I've been rearranging a lot of code lately and trying to
> make the patches not look really scary?
Sorry, I did not pay that close attention.
> Aside from presentation, it looks good to me. Shall I stick the bundle
> changes into my series? I'd like to have them come before the patch to
> switch to builtin-fetch, so that there aren't any revisions where "git
> fetch" doesn't have bundle support.
Looks fine to me. Seems like you should add a SOB line, too.
> And I think it would be best to take part 3 as a review fix to my final
> patch.
Yes, definitely. This shows again (to me, at least), that just looking at
the code is not enough, you have to run it, too, to review patches.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH 1/3] Move bundle specific stuff into bundle.[ch]
From: Johannes Schindelin @ 2007-07-18 10:09 UTC (permalink / raw)
To: Daniel Barkalow; +Cc: git
In-Reply-To: <Pine.LNX.4.64.0707172302560.14596@iabervon.org>
Hi,
On Tue, 17 Jul 2007, Daniel Barkalow wrote:
> On Tue, 17 Jul 2007, Daniel Barkalow wrote:
>
> > On Tue, 17 Jul 2007, Johannes Schindelin wrote:
> >
> > > The transport specific stuff was moved into libgit.a, and the bundle
> > > specific stuff will not be left behind.
> > >
> > > This is a big code move, with one exception: the function unbundle()
> > > no longer outputs the list of refs. You have to call
> > > list_bundle_refs() yourself for that.
> >
> > You should use -C on this sort of thing, so that the interesting
> > aspects of the patch are easier to see. (It actually comes out longer
> > in this case, but it's far easier to tell that the code in the new
> > file is the same as the old code.) Can you tell I've been rearranging
> > a lot of code lately and trying to make the patches not look really
> > scary?
>
> Actually, I ended up touching this up a tiny bit, too: I ordered the
> functions in bundle.c the way they were in builtin-bundle.c (so that the
> patch is more trivial) and removed the blank lines at the end of the file.
> This makes the "git diff -C" output really obvious.
Makes sense. Thanks.
> (Someday, I'd like to have a diff that can show that a substantial block
> of '+' lines matches a block of lines from somewhere in the "before"
> content, so reviewers can verify that the patch reorders code but
> doesn't change it, or changes it in certain ways. But, of course, that's
> both hard to generate and hard to display usefully.)
AFAIR from the thread after
http://thread.gmane.org/gmane.linux.kernel/484924/focus=37498
the problem was not so much the displaying as the applying. You have a
diff for builtin-bundle.c. You want to move code to bundle.c. git-diff
has to rearrange the diff so that bundle.c can copy the code from
builtin-bundle.c, and then it is deleted from builtin-bundle.c. If you
move code criss-cross, it is not possible.
However, if you do something like
diff --git a/builtin-bundle.c:10--20 b/bundle.c:0--30
code move
--- a/builtin-bundle.c:10--20
+++ b/bundle.c:1--11
diff --git ...
it should be possible. We could allow generation of such a diff with
--code-moves, which would detect if there is substantial evidence for a
code move, and produce such a diff.
Note: this kind of code movement diff has to come before _both_ the diffs
for builtin-bundle.c and bundle.c, since chances are that the code had to
be touched a little here and there. And probably you would want a little
context, just to make sure it is the correct code when applying the patch.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH] git-gui: Handle git versions of the form n.n.n.GIT
From: Julian Phillips @ 2007-07-18 10:10 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: Martin Langhoff, git
In-Reply-To: <20070718025442.GX32566@spearce.org>
On Tue, 17 Jul 2007, Shawn O. Pearce wrote:
> The first fixes the -dirty build problem. The second drops off
> the extra information that git-describe throws into the mix when
> it generates output for a non-tagged commit. The third kills the
> rc* component if this is a release candidate. Note that the rc*
> killer must come after the git-describe killer, as the rc* part is
> actually in the real tag. The last one fixes the weird case where
> the user has somehow bungled his git software distribution so it
> cannot generate a git version via git-describe *and* they have no
> `version` file in the source code directory. Such people really
> should fix their git. But anyway we do support it now.
Well, I would say that my git is not broken, but simply temporary. I
have a machine that is not connected to the internet where I want to run
git. Normally I use release tarballs, but at the moment I need the recent
changes to fast-import, so I am running whatever was master at the time I
made the tarball.
As soon as 1.5.3 comes out I will be back to using the official releases.
I just wanted to run git-gui blame (a rather nice tool) to look at what
the result of my latest test import looked like. Since I wasn't using git
for anything other than playing with fast-import, creating a properly
versioned git seemed like more effort than it was worth.
--
Julian
---
The major difference between bonds and bond traders is that the bonds will
eventually mature.
^ permalink raw reply
* Re: Empty directories...
From: Johannes Schindelin @ 2007-07-18 10:26 UTC (permalink / raw)
To: David Kastrup; +Cc: git
In-Reply-To: <858x9ez1li.fsf@lola.goethe.zz>
Hi,
On Wed, 18 Jul 2007, David Kastrup wrote:
> The FAQ answer is weazeling on several accounts:
>
> a) No, git only cares about files, or rather git tracks content and
> empty directories have no content.
>
> In the same manner as empty regular files have no contents, and git
> tracks those. Existence and permissions are important.
We do not track permissions of directories at all. This is because Git is
primarily meant to track source code, and most "permissions" (i.e.
restrictions) do not make any sense there.
> b) The problem is not just that empty directories don't get added into
> the repository. They also don't get removed again when switching to a
> different checkout. When git-diff returns zero, I expect a subsequent
> checkout to not leave complete empty hierarchies around because git
> can't delete any empty leaves which it chose not to track.
I _like_ the behaviour that Git does not remove a directory it added, when
I put some untracked file into it. And switching back to that branch, Git
has no problems, because it sees that the directory is already there. In
case of a file, it would complain, and rightfully so.
See the fundamental difference between a file and a directory now? I
think it boils down to "an empty directory has _no_ contents, but an empty
file has an _empty_ content".
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH 6/6] Add git-rewrite-commits
From: Johannes Schindelin @ 2007-07-18 11:02 UTC (permalink / raw)
To: skimo; +Cc: git, Junio C Hamano
In-Reply-To: <20070716102407.GL999MdfPADPa@greensroom.kotnet.org>
Hi,
On Mon, 16 Jul 2007, Sven Verdoolaege wrote:
> On Mon, Jul 16, 2007 at 01:38:11AM +0100, Johannes Schindelin wrote:
> > On Sun, 15 Jul 2007, Sven Verdoolaege wrote:
> > > > > + if (path_pruning &&
> > > > > + !(commit->object.flags & (TREECHANGE | UNINTERESTING)))
> > > > > + return 1;
> > > >
> > > > Why only with "path_pruning"? Ah yes. Because otherwise, you would
> > > > assume "A" in "A..B" to be pruned.
> > >
> > > TREECHANGE is only set when path pruning is in effect.
> > > If I didn't check for path_pruning, then all commits would be
> > > considered to have been pruned. (Or am I missing something?
> > > Honestly, I found all that TREECHANGE stuff difficult to follow.)
> >
> > AFAICT TREECHANGE means that parents were rewritten.
>
> I think you'll find that if all commits touch a path in the
> path specifiers then all commits will have TREECHANGE set and
> so no parents will be rewritten.
The code suggests otherwise.
But I really have to wonder: why do you play games with TREECHANGE? I had
the impression that commit->parents is set appropriately by the revision
walker, and that you do not have to do _anything_ for that to work.
Maybe the "--grep" thing does not yet. But then you should fix it in
revision.c. Not in builtin-rewrite-commits.c
> > > revision.c itself is also riddled with "prune_fn && ".
> > > Wouldn't it make sense to invert the meaning of this bit and call
> > > it, say, PRUNED, so that the default is off and you would only
> > > have to check if the bit was set ?
> >
> > You meant the TREECHANGE bit? No.
>
> Yes. Why?
Why invert the meaning of a perfectly fine bit? Because you can? It is
working right now, and it is not even a buglet, so what is there to fix?
> > BTW what do you plan to do about my objection to UNINTERESTING, given
> > the example "git rewrite-commits A..B x/y"?
>
> That was based on an apparent misunderstanding of my code
> that I tried to address above. I did not intend to do what
> you claim I do and a quick test confirms that my code does
> indeed not to what you claim it does.
>
> More specifically, the history will not be cut off at A
> because A is marked UNINTERESTING and is therefore not considered
> to have been pruned.
Why do you test for TREECHANGE | UNINTERESTING then?
> A commit is considered pruned if it was either explicitly marked
> as such or if TREECHANGE is not set, but the later check (in is_pruned)
> is only done on commits that were checked for tree changes.
I don't understand. What do you mean by "a commit is pruned"? Does it
mean that this commit was left out from the revision walk? What does that
have to do with TREECHANGE, which means that the parents set was modified?
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH 1/3] Move bundle specific stuff into bundle.[ch]
From: Shawn O. Pearce @ 2007-07-18 6:19 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Daniel Barkalow, Johannes Schindelin, git
In-Reply-To: <7vy7he8cjl.fsf@assigned-by-dhcp.cox.net>
Junio C Hamano <gitster@pobox.com> wrote:
> It might not be actually so bad. But I wonder if it would be
> more obvious if you do not show the whole "a-" lines but leave
> just a marker there. That is (ugliness of "a@@" and "a-" that
> made me wash my eyes needs to be fixed, though -- but that is
> only the syntax):
>
> a@@ -1598,43 +1635,6 a@@ apply_config
>
> ######################################################################
> ##
> a-<<< Block a was originally here >>>
> ## ui construction
>
> set ui_comm {}
>
> You are coming up with a new output format that is only used
> when it is a straight move and nothing else, so by definition
> there is really no need to show both removal and addition.
Yea, this I like even better than what I posted. Now we just need
a suck^H^H^H^Hprogrammer to implement a working prototype and see
how folks like more realistic diffs generated with it. ;-)
--
Shawn.
^ permalink raw reply
* Re: [PATCH 6/6] Add git-rewrite-commits
From: Johannes Schindelin @ 2007-07-18 11:05 UTC (permalink / raw)
To: skimo; +Cc: git, Junio C Hamano
In-Reply-To: <20070716214756.GA15007MdfPADPa@greensroom.kotnet.org>
Hi,
On Mon, 16 Jul 2007, Sven Verdoolaege wrote:
> On Mon, Jul 16, 2007 at 10:04:04PM +0200, Sven Verdoolaege wrote:
> > - a SHA1 of a commit that appears in a commit message is replaced
> > by the rewritten commit iff it was rewritten to a single commit.
> > That is, if the commit was pruned or rewritten (through a commit
> > filter) to more than one commit, then the SHA1 is left alone.
>
> Sorry. I misremembered. I considered doing it this way, but then
> thought it was better to replace the SHA1 with a(n abbreviated) null
> SHA1 to signify that the commit had gone.
Yes. This shows that you did not get my objection. The commit is not
replaced with _no_ commits, but with _multiple_ commits.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH 6/6] Add git-rewrite-commits
From: Johannes Schindelin @ 2007-07-18 11:17 UTC (permalink / raw)
To: skimo; +Cc: git, Junio C Hamano
In-Reply-To: <20070716200404.GT999MdfPADPa@greensroom.kotnet.org>
Hi,
On Mon, 16 Jul 2007, Sven Verdoolaege wrote:
> On Mon, Jul 16, 2007 at 01:38:11AM +0100, Johannes Schindelin wrote:
> > Didn't I mention that it was a severe limitation to think of the sha1
> > mapping of a 1-to-1 mapping? Think of it more as a relation.
>
> The mapping is used in several operations.
> First, there are several things that can happen to a commit
>
> - it's pruned. This includes, for me, path pruning, matching
> and a commit filter returning no SHA1s.
Okay.
> - it's rewritten to another commit that can be considered the
> "moral equivalent" of that commit. This occurs when a commit
> is not pruned, but something else happened to the commit itself or
> one of its ancestors. This excludes, for me, the case
> where a commit filter returns more than one SHA1.
Okay. For me it does not at all exclude that. If I want to replace a
commit by no commit, I write a commit-filter which does not return
anything. If I return more than one SHA1s, I damned well want all of
those be the replacement "commit".
To say "the" replacement "commit", means to mistake the mapping as a
function, a non-relation.
> - it's replaced by more than one SHA1. This can only happen
> in a commit filter.
For the moment, yes.
> There are at least four operations in which this mapping is used:
>
> - if the parents of a commit have been rewritten to one or more
> commits, then they are replaced by the new commits.
Yes, that is the primary use for the mapping.
> If any parent has been pruned, then it is replaced by
> the result of applying this operation on _its_ parents.
Why? This is overy complicated. If a commit has been pruned, why does
the mapping not point to the _non-pruned_ parent? IOW if you have
something like this:
A - B - C - D - E - F
and all commits except A and F are pruned, the mapping for A, B, C, D and
E should _all_ point to the (possibly rewritten) A.
> - any reference (in refs/) that points to a rewritten or pruned
> commit is removed and
> * if the commit was rewritten to a single commit, then it is
> replaced by this commit
> * otherwise, there is no moral equivalent single commit, but
> we want to ensure we can still access the new commits, so
> I create several references, either to each of the many
> commits the old commit was rewritten to, or to each of
> its nearest unpruned ancestors (i.e., the same set as
> described in the previous operation).
I'd argue that it should be an error if a to-be-rewritten ref (and I still
strongly disagree with you that all refs should be rewritten) would point
to multiple commits. Possibly overridable with "--allow-octopus-refs".
But the default should be to error out.
> - a SHA1 of a commit that appears in a commit message is replaced
> by the rewritten commit iff it was rewritten to a single commit.
> That is, if the commit was pruned or rewritten (through a commit
> filter to more than one commit), then the SHA1 is left alone.
Both this behaviour and the one you described in your reply are wrong.
> - the mapping available to filters
> * if the commit was pruned, an empty file is created
> * otherwise a file is created containing all rewritten SHA1s
As I stated above: it is utterly wrong to create an empty mapping for a
commit that was pruned. It does not take long to think of an example:
A - B - C - D
Now, A and D get pruned. Do you want the whole branch to vanish? _Hell,
no_.
> I understand you want the second operation to only apply to refs
> explicitly mentioned on the command line.
You have to at least give the users a chance to grasp what they are doing.
And if that means to change the semantics to something saner, then so be
it.
Ciao,
Dscho
^ permalink raw reply
* Re: Empty directories...
From: Johannes Schindelin @ 2007-07-18 11:24 UTC (permalink / raw)
To: David Kastrup; +Cc: git
In-Reply-To: <86tzs2m1h7.fsf@lola.quinscape.zz>
Hi,
On Wed, 18 Jul 2007, David Kastrup wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > On Wed, 18 Jul 2007, David Kastrup wrote:
> >
> >> The FAQ answer is weazeling on several accounts:
> >>
> >> a) No, git only cares about files, or rather git tracks content and
> >> empty directories have no content.
> >>
> >> In the same manner as empty regular files have no contents, and git
> >> tracks those. Existence and permissions are important.
> >
> > We do not track permissions of directories at all.
>
> Ok, this seems like something that should be done as well, even if we
> can stipulate at first that a directory should have rwx for the user
> in question if you hope to track it.
No, no, no. It should not be tracked. It is the responsibility of the
_user_ to set it to something sane, be that by a umask or by sticky
groups, or by setting the permissions of the parent directory.
It is _nothing_ we want to put into the repository. That is the _wrong_
place to put it.
> > This is because Git is primarily meant to track source code,
>
> Tell that to the man page. It declares git to be "a content tracker"
> right at the front.
Why don't you? I have no problems with the title.
> > and most "permissions" (i.e. restrictions) do not make any sense
> > there.
>
> So why are permissions for files being tracked, then?
This question is invalid. Git only tracks the _executable_ bit. And
again, it is the users' responsibility, by setting the umask, to have the
appropriate bits set for group and others.
> >> b) The problem is not just that empty directories don't get added
> >> into the repository. They also don't get removed again when
> >> switching to a different checkout. When git-diff returns zero, I
> >> expect a subsequent checkout to not leave complete empty hierarchies
> >> around because git can't delete any empty leaves which it chose not
> >> to track.
> >
> > I _like_ the behaviour that Git does not remove a directory it
> > added, when I put some untracked file into it.
>
> But it does not remove a directory it _refused_ to add when there were
> no files at all in it ever. You probably have not read the problem
> description carefully.
I have. But that does not apply here, because I used the term "to add a
directory" in the sense of "mkdir".
> > And switching back to that branch, Git has no problems, because it
> > sees that the directory is already there. In case of a file, it would
> > complain, and rightfully so.
>
> And if you switch to a branch where the directory it did not remove now
> is a file?
Git already throws an error, and rightfully so. I am pleased by the
current behaviour.
> > See the fundamental difference between a file and a directory now?
>
> Condescension is not really solving a problem.
Hey, I only tried to help clarify things.
But since I seem to be unable to, I'll end my efforts with this
suggestion:
If you want to track empty directories, the best thing would be to
- teach git-add to automatically create an empty .gitignore (and error out
if that already exists), and
- teach git-archive to not put .gitignore files into the output by default
(but the directories). This might be a sensible change regardless if
you want to add empty directories to the repository or not.
Ciao,
Dscho
^ permalink raw reply
* Re: Empty directories...
From: Matthieu Moy @ 2007-07-18 11:40 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: David Kastrup, git
In-Reply-To: <Pine.LNX.4.64.0707181218090.14781@racer.site>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> > We do not track permissions of directories at all.
>>
>> Ok, this seems like something that should be done as well, even if we
>> can stipulate at first that a directory should have rwx for the user
>> in question if you hope to track it.
>
> No, no, no. It should not be tracked. It is the responsibility of the
> _user_ to set it to something sane, be that by a umask or by sticky
> groups, or by setting the permissions of the parent directory.
>
> It is _nothing_ we want to put into the repository. That is the _wrong_
> place to put it.
I'm not sure it's wrong to be able to track permissions, but it's
definitely wrong to track them by default.
GNU Arch had some permission tracking, and I got hit by it several
times. You have several things you might have wanted to track:
* read/write for the user. But I can't imagine a case where you
wouldn't want to be able to read and write your own files.
* permissions for group. But that doesn't make any sense when several
persons work on the same project, and don't share the same
/etc/group.
* permissions for others. But that, again, doesn't make sense when
several persons work on the same project with different setups. I
sometimes work at home, where I'm basically the only user, I don't
care at all about permissions for others. At work, it's totally
different, since it's a big NFS shared by all the lab. And I might
very well disclose my work to the rest of the lab, and work with
someone who do not want to do so.
* Execute bit. This one is relevant. Indeed, it's more a kind of
metadata than really a permission (you can still execute the file
with /lib/ld-linux.so.2 /path/to/file or such kind of things).
Using GNU Arch, I got the cases in real life of a project in which
some files had group read permission, some other not, because they
were created by developers having different umask. Worse than this, I
got some group-writable files in my $HOME without noticing it, which
is basically a security hole.
--
Matthieu
^ permalink raw reply
* Re: [PATCH 6/6] Add git-rewrite-commits
From: Sven Verdoolaege @ 2007-07-18 12:05 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano
In-Reply-To: <Pine.LNX.4.64.0707181153200.14781@racer.site>
On Wed, Jul 18, 2007 at 12:02:50PM +0100, Johannes Schindelin wrote:
> Hi,
>
> On Mon, 16 Jul 2007, Sven Verdoolaege wrote:
>
> > On Mon, Jul 16, 2007 at 01:38:11AM +0100, Johannes Schindelin wrote:
> > > On Sun, 15 Jul 2007, Sven Verdoolaege wrote:
> > > > TREECHANGE is only set when path pruning is in effect.
> > > > If I didn't check for path_pruning, then all commits would be
> > > > considered to have been pruned. (Or am I missing something?
> > > > Honestly, I found all that TREECHANGE stuff difficult to follow.)
> > >
> > > AFAICT TREECHANGE means that parents were rewritten.
> >
> > I think you'll find that if all commits touch a path in the
> > path specifiers then all commits will have TREECHANGE set and
> > so no parents will be rewritten.
>
> The code suggests otherwise.
Check again.
> But I really have to wonder: why do you play games with TREECHANGE? I had
> the impression that commit->parents is set appropriately by the revision
> walker,
Only for unpruned commits and the references (explicitly specified
on the command line if you wish) may have been pruned.
> > > > revision.c itself is also riddled with "prune_fn && ".
> > > > Wouldn't it make sense to invert the meaning of this bit and call
> > > > it, say, PRUNED, so that the default is off and you would only
> > > > have to check if the bit was set ?
> > >
> > > You meant the TREECHANGE bit? No.
> >
> > Yes. Why?
>
> Why invert the meaning of a perfectly fine bit? Because you can? It is
> working right now, and it is not even a buglet, so what is there to fix?
Because it is confusing. As explained above, the bit doesn't have a
meaning of its own. You can only interpret the bit if some other
conditions are met.
It would be even more confusing if it meant what you claim it means.
>
> > > BTW what do you plan to do about my objection to UNINTERESTING, given
> > > the example "git rewrite-commits A..B x/y"?
> >
> > That was based on an apparent misunderstanding of my code
> > that I tried to address above. I did not intend to do what
> > you claim I do and a quick test confirms that my code does
> > indeed not to what you claim it does.
> >
> > More specifically, the history will not be cut off at A
> > because A is marked UNINTERESTING and is therefore not considered
> > to have been pruned.
>
> Why do you test for TREECHANGE | UNINTERESTING then?
Exactly for the reason mentioned above.
If the commit is marked UNINTERESTING then it has not been pruned,
because it hasn't even been checked for TREECHANGE.
> > A commit is considered pruned if it was either explicitly marked
> > as such or if TREECHANGE is not set, but the later check (in is_pruned)
> > is only done on commits that were checked for tree changes.
>
> I don't understand. What do you mean by "a commit is pruned"? Does it
> mean that this commit was left out from the revision walk? What does that
> have to do with TREECHANGE, which means that the parents set was modified?
You just claim that that is what it means. The code (see try_to_simplify_commit
where the bit is set) and a simple experiment (explained above) show otherwise.
skimo
^ permalink raw reply
* Re: Empty directories...
From: David Kastrup @ 2007-07-18 12:12 UTC (permalink / raw)
To: git
In-Reply-To: <vpqejj6c52u.fsf@bauges.imag.fr>
Matthieu Moy <Matthieu.Moy@imag.fr> writes:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>>> > We do not track permissions of directories at all.
>>>
>>> Ok, this seems like something that should be done as well, even if we
>>> can stipulate at first that a directory should have rwx for the user
>>> in question if you hope to track it.
>>
>> No, no, no. It should not be tracked. It is the responsibility of the
>> _user_ to set it to something sane, be that by a umask or by sticky
>> groups, or by setting the permissions of the parent directory.
>>
>> It is _nothing_ we want to put into the repository. That is the _wrong_
>> place to put it.
>
> I'm not sure it's wrong to be able to track permissions, but it's
> definitely wrong to track them by default.
I am not sure about "definitely", but there certainly are applications
where it is appropriate.
> * Execute bit. This one is relevant. Indeed, it's more a kind of
> metadata than really a permission (you can still execute the file
> with /lib/ld-linux.so.2 /path/to/file or such kind of things).
Please spare us the sophistry. Probably the most flexible approach
would be to be able to specify a checkout umask, defaulting to 700
(the other bits are then filled in from the normal user umask). For
archival purposes, one would then set it to 777 instead.
There is the question how to deal with checkins. While there is no
harm in checking in the full permissions in case one would need them,
it would likely be a nuisance to track the individual contributor's
settings.
--
David Kastrup
^ permalink raw reply
* [PATCH] Makefile: create an install-symlinks target
From: David Kastrup @ 2007-07-18 10:41 UTC (permalink / raw)
To: git
Use this, for example, to do
rm -rf /opt/git
make prefix=/opt/git install
make symlinkprefix=/usr/local prefix=/opt/git install-symlinks
---
Makefile | 9 ++++++++-
1 files changed, 8 insertions(+), 1 deletions(-)
diff --git a/Makefile b/Makefile
index 73b487f..df2fe8d 100644
--- a/Makefile
+++ b/Makefile
@@ -142,6 +142,7 @@ ALL_LDFLAGS = $(LDFLAGS)
STRIP ?= strip
prefix = $(HOME)
+symlinkprefix = /usr/local
bindir = $(prefix)/bin
gitexecdir = $(bindir)
sharedir = $(prefix)/share
@@ -996,7 +997,13 @@ install-doc:
quick-install-doc:
$(MAKE) -C Documentation quick-install
-
+# The somewhat strange looking lines start with an ignored $(MAKE) in
+# order to be executed also in make -n calls.
+install-symlinks:
+ @: $(MAKE) && cd '$(prefix_SQ)' && find . -mindepth 1 -type d ! \( -iname 'git*' -prune -exec echo rm -rf '$(symlinkprefix)/{}' \; \) -exec echo $(INSTALL) -m 755 -d '$(symlinkprefix)/{}' \;
+ @cd '$(prefix_SQ)' && find . -mindepth 1 -type d ! \( -iname 'git*' -prune -exec rm -rf '$(symlinkprefix)/{}' \; \) -exec $(INSTALL) -m 755 -d '$(symlinkprefix)/{}' \;
+ @: $(MAKE) && cd '$(prefix_SQ)' && find . -mindepth 1 \( -type d -iname 'git*' -prune -o ! -type d \) -exec echo ln -snf '$(prefix_SQ)/{}' '$(symlinkprefix)/{}' \;
+ @cd '$(prefix_SQ)' && find . -mindepth 1 \( -type d -iname 'git*' -prune -o ! -type d \) -exec ln -snf '$(prefix_SQ)/{}' '$(symlinkprefix)/{}' \;
### Maintainer's dist rules
--
1.5.3.rc2.41.gb47b1
^ permalink raw reply related
* Re: [PATCH] Makefile: create an install-symlinks target
From: Johannes Schindelin @ 2007-07-18 12:48 UTC (permalink / raw)
To: David Kastrup; +Cc: git
In-Reply-To: <86bqe9lwy8.fsf@lola.quinscape.zz>
Hi,
On Wed, 18 Jul 2007, David Kastrup wrote:
> Use this, for example, to do
> rm -rf /opt/git
> make prefix=/opt/git install
> make symlinkprefix=/usr/local prefix=/opt/git install-symlinks
You mean
This target allows you to have git installed in one location,
and have symbolic links to all of the programs installed in
another location. For example, if git was installed to /opt/git
with
make prefix=/opt/git install
you can install symbolic links in /usr/local/bin with
make symlinkprefix=/usr/local prefix=/opt/git \
install-symlinks
Hmm. Why not install it with a proper package manager in the correct
place to begin with? Somehow I find so many symbolic links ugly.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH] Makefile: create an install-symlinks target
From: David Kastrup @ 2007-07-18 12:41 UTC (permalink / raw)
To: git
In-Reply-To: <86bqe9lwy8.fsf@lola.quinscape.zz>
David Kastrup <dak@gnu.org> writes:
> Use this, for example, to do
> rm -rf /opt/git
> make prefix=/opt/git install
> make symlinkprefix=/usr/local prefix=/opt/git install-symlinks
> ---
> Makefile | 9 ++++++++-
> 1 files changed, 8 insertions(+), 1 deletions(-)
Oh, by the way: I got _crazy_ when I first committed this, then found
a mistake and corrected it, and then repeatedly did
git add config/emacs/Makefile
git-commit --amend
and got the
> Makefile | 9 ++++++++-
> 1 files changed, 8 insertions(+), 1 deletions(-)
message again and again, while nothing at all changed. Of course,
this was because I added the wrong Makefile.
I am not sure how git could be more helpful here. Just wanted to
report it. Maybe it could have noticed that my "amendment" was empty?
On the other hand, I might have wanted to amend the commit message.
Sigh.
--
David Kastrup
^ permalink raw reply
* Re: [PATCH] Makefile: create an install-symlinks target
From: Alex Riesen @ 2007-07-18 13:08 UTC (permalink / raw)
To: David Kastrup; +Cc: git
In-Reply-To: <86bqe9lwy8.fsf@lola.quinscape.zz>
On 7/18/07, David Kastrup <dak@gnu.org> wrote:
> + @: $(MAKE) && cd '$(prefix_SQ)' && find . -mindepth 1 ...
Sometime about now you'll need to define $(FIND) or even $(GNUFIND)
for find. There are proprietary systems where find is not available or
does not do what you want it to. There is often a gfind installed somewhere
on these systems.
^ permalink raw reply
* [PATCH] filter-branch: get rid of "set -e"
From: Johannes Schindelin @ 2007-07-18 13:17 UTC (permalink / raw)
To: Alex Riesen; +Cc: git, gitster
In-Reply-To: <20070710205202.GA3212@steel.home>
It was reported by Alex Riesen that "set -e" can break something as
trivial as "unset CDPATH" in bash.
So get rid of "set -e".
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
On Tue, 10 Jul 2007, Alex Riesen wrote:
> I have a Debian system where git-filter-branch exits immediately
> after "unset CDPATH" in git-sh-setup (the command exits with 1,
> as CDPATH is not defined). The system still has bash-2.05a.
>
> git-filter-branch has "set -e", which is why the script finishes
> prematurely. If this is not really needed, maybe it can be
> removed?
I hope I got all of the interesting places equipped by the
appropriate conditional die() calls.
git-filter-branch.sh | 31 +++++++++++++++++--------------
1 files changed, 17 insertions(+), 14 deletions(-)
diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 5401970..0d000ed 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -8,8 +8,6 @@
# a new branch. You can specify a number of filters to modify the commits,
# files and trees.
-set -e
-
USAGE="git-filter-branch [-d TEMPDIR] [FILTERS] DESTBRANCH [REV-RANGE]"
. git-sh-setup
@@ -141,9 +139,10 @@ git show-ref "refs/heads/$dstbranch" 2> /dev/null &&
die "branch $dstbranch already exists"
test ! -e "$tempdir" || die "$tempdir already exists, please remove it"
-mkdir -p "$tempdir/t"
-cd "$tempdir/t"
-workdir="$(pwd)"
+mkdir -p "$tempdir/t" &&
+cd "$tempdir/t" &&
+workdir="$(pwd)" ||
+die ""
case "$GIT_DIR" in
/*)
@@ -155,12 +154,12 @@ esac
export GIT_DIR GIT_WORK_TREE=.
export GIT_INDEX_FILE="$(pwd)/../index"
-git read-tree # seed the index file
+git read-tree || die "Could not seed the index"
ret=0
-
-mkdir ../map # map old->new commit ids for rewriting parents
+# map old->new commit ids for rewriting parents
+mkdir ../map || die "Could not create map/ directory"
case "$filter_subdir" in
"")
@@ -170,7 +169,7 @@ case "$filter_subdir" in
*)
git rev-list --reverse --topo-order --default HEAD \
--parents --full-history "$@" -- "$filter_subdir"
-esac > ../revs
+esac > ../revs || die "Could not get the commits"
commits=$(wc -l <../revs | tr -d " ")
test $commits -eq 0 && die "Found nothing to rewrite"
@@ -186,10 +185,11 @@ while read commit parents; do
;;
*)
git read-tree -i -m $commit:"$filter_subdir"
- esac
+ esac || die "Could not initialize the index"
export GIT_COMMIT=$commit
- git cat-file commit "$commit" >../commit
+ git cat-file commit "$commit" >../commit ||
+ die "Cannot read commit $commit"
eval "$(set_ident AUTHOR <../commit)" ||
die "setting author failed for commit $commit"
@@ -199,7 +199,8 @@ while read commit parents; do
die "env filter failed: $filter_env"
if [ "$filter_tree" ]; then
- git checkout-index -f -u -a
+ git checkout-index -f -u -a ||
+ die "Could not checkout the index"
# files that $commit removed are now still in the working tree;
# remove them, else they would be added again
git ls-files -z --others | xargs -0 rm -f
@@ -240,7 +241,8 @@ case "$target_head" in
echo Nothing rewritten
;;
*)
- git update-ref refs/heads/"$dstbranch" $target_head
+ git update-ref refs/heads/"$dstbranch" $target_head ||
+ die "Could not update $dstbranch with $target_head"
if [ $(wc -l <../map/$src_head) -gt 1 ]; then
echo "WARNING: Your commit filter caused the head commit to expand to several rewritten commits. Only the first such commit was recorded as the current $dstbranch head but you will need to resolve the situation now (probably by manually merging the other commits). These are all the commits:" >&2
sed 's/^/ /' ../map/$src_head >&2
@@ -277,7 +279,8 @@ if [ "$filter_tag_name" ]; then
warn "unreferencing tag object $sha1t"
fi
- git update-ref "refs/tags/$new_ref" "$new_sha1"
+ git update-ref "refs/tags/$new_ref" "$new_sha1" ||
+ die "Could not write tag $new_ref"
done
fi
--
1.5.3.rc1.16.g9d6f-dirty
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox